Received-SPF: pass (google.com: domain of linux-kernel+bounces-80721-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Subject: Re: [PATCH v5 3/6] LoongArch: KVM: Add cpucfg area for kvm hypervisor
To: Huacai Chen <chenhuacai@kernel.org>, Jiaxun Yang <jiaxun.yang@flygoat.com>
Cc: Tianrui Zhao <zhaotianrui@loongson.cn>, Juergen Gross <jgross@suse.com>,
 Paolo Bonzini <pbonzini@redhat.com>, loongarch@lists.linux.dev,
 linux-kernel@vger.kernel.org, virtualization@lists.linux.dev,
 kvm@vger.kernel.org
References: <20240222032803.2177856-1-maobibo@loongson.cn>
 <20240222032803.2177856-4-maobibo@loongson.cn>
 <CAAhV-H5eqXMqTYVb6cAVqOsDNcEDeP9HzaMKw69KFQeVaAYEdA@mail.gmail.com>
 <d1a6c424-b710-74d6-29f6-e0d8e597e1fb@loongson.cn>
 <CAAhV-H7p114hWUVrYRfKiBX3teG8sG7xmEW-Q-QT3i+xdLqDEA@mail.gmail.com>
From: maobibo <maobibo@loongson.cn>
Message-ID: <06647e4a-0027-9c9f-f3bd-cd525d37b6d8@loongson.cn>
Date: Mon, 26 Feb 2024 16:04:50 +0800
User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <CAAhV-H7p114hWUVrYRfKiBX3teG8sG7xmEW-Q-QT3i+xdLqDEA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit


On 2024/2/26 下午2:12, Huacai Chen wrote:
> On Mon, Feb 26, 2024 at 10:04 AM maobibo <maobibo@loongson.cn> wrote:
>>
>>
>>
>> On 2024/2/24 下午5:13, Huacai Chen wrote:
>>> Hi, Bibo,
>>>
>>> On Thu, Feb 22, 2024 at 11:28 AM Bibo Mao <maobibo@loongson.cn> wrote:
>>>>
>>>> Instruction cpucfg can be used to get processor features. And there
>>>> is trap exception when it is executed in VM mode, and also it is
>>>> to provide cpu features to VM. On real hardware cpucfg area 0 - 20
>>>> is used.  Here one specified area 0x40000000 -- 0x400000ff is used
>>>> for KVM hypervisor to privide PV features, and the area can be extended
>>>> for other hypervisors in future. This area will never be used for
>>>> real HW, it is only used by software.
>>> After reading and thinking, I find that the hypercall method which is
>>> used in our productive kernel is better than this cpucfg method.
>>> Because hypercall is more simple and straightforward, plus we don't
>>> worry about conflicting with the real hardware.
>> No, I do not think so. cpucfg is simper than hypercall, hypercall can
>> be in effect when system runs in guest mode. In some scenario like TCG
>> mode, hypercall is illegal intruction, however cpucfg can work.
> Nearly all architectures use hypercall except x86 for its historical
Only x86 support multiple hypervisors and there is multiple hypervisor 
in x86 only. It is an advantage, not historical reason.

> reasons. If we use CPUCFG, then the hypervisor information is
> unnecessarily leaked to userspace, and this may be a security issue.
> Meanwhile, I don't think TCG mode needs PV features.
Besides PV features, there is other features different with real hw such 
as virtio device, virtual interrupt controller.

Regards
Bibo Mao

> 
> I consulted with Jiaxun before, and maybe he can give some more comments.
> 
>>
>> Extioi virtualization extension will be added later, cpucfg can be used
>> to get extioi features. It is unlikely that extioi driver depends on
>> PARA_VIRT macro if hypercall is used to get features.
> CPUCFG is per-core information, if we really need something about
> extioi, it should be in iocsr (LOONGARCH_IOCSR_FEATURES).
> 
> 
> Huacai
> 
>>
>> Regards
>> Bibo Mao
>>
>>>
>>> Huacai
>>>
>>>>
>>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>>> ---
>>>>    arch/loongarch/include/asm/inst.h      |  1 +
>>>>    arch/loongarch/include/asm/loongarch.h | 10 ++++++
>>>>    arch/loongarch/kvm/exit.c              | 46 +++++++++++++++++---------
>>>>    3 files changed, 41 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/arch/loongarch/include/asm/inst.h b/arch/loongarch/include/asm/inst.h
>>>> index d8f637f9e400..ad120f924905 100644
>>>> --- a/arch/loongarch/include/asm/inst.h
>>>> +++ b/arch/loongarch/include/asm/inst.h
>>>> @@ -67,6 +67,7 @@ enum reg2_op {
>>>>           revhd_op        = 0x11,
>>>>           extwh_op        = 0x16,
>>>>           extwb_op        = 0x17,
>>>> +       cpucfg_op       = 0x1b,
>>>>           iocsrrdb_op     = 0x19200,
>>>>           iocsrrdh_op     = 0x19201,
>>>>           iocsrrdw_op     = 0x19202,
>>>> diff --git a/arch/loongarch/include/asm/loongarch.h b/arch/loongarch/include/asm/loongarch.h
>>>> index 46366e783c84..a1d22e8b6f94 100644
>>>> --- a/arch/loongarch/include/asm/loongarch.h
>>>> +++ b/arch/loongarch/include/asm/loongarch.h
>>>> @@ -158,6 +158,16 @@
>>>>    #define  CPUCFG48_VFPU_CG              BIT(2)
>>>>    #define  CPUCFG48_RAM_CG               BIT(3)
>>>>
>>>> +/*
>>>> + * cpucfg index area: 0x40000000 -- 0x400000ff
>>>> + * SW emulation for KVM hypervirsor
>>>> + */
>>>> +#define CPUCFG_KVM_BASE                        0x40000000UL
>>>> +#define CPUCFG_KVM_SIZE                        0x100
>>>> +#define CPUCFG_KVM_SIG                 CPUCFG_KVM_BASE
>>>> +#define  KVM_SIGNATURE                 "KVM\0"
>>>> +#define CPUCFG_KVM_FEATURE             (CPUCFG_KVM_BASE + 4)
>>>> +
>>>>    #ifndef __ASSEMBLY__
>>>>
>>>>    /* CSR */
>>>> diff --git a/arch/loongarch/kvm/exit.c b/arch/loongarch/kvm/exit.c
>>>> index 923bbca9bd22..6a38fd59d86d 100644
>>>> --- a/arch/loongarch/kvm/exit.c
>>>> +++ b/arch/loongarch/kvm/exit.c
>>>> @@ -206,10 +206,37 @@ int kvm_emu_idle(struct kvm_vcpu *vcpu)
>>>>           return EMULATE_DONE;
>>>>    }
>>>>
>>>> -static int kvm_trap_handle_gspr(struct kvm_vcpu *vcpu)
>>>> +static int kvm_emu_cpucfg(struct kvm_vcpu *vcpu, larch_inst inst)
>>>>    {
>>>>           int rd, rj;
>>>>           unsigned int index;
>>>> +
>>>> +       rd = inst.reg2_format.rd;
>>>> +       rj = inst.reg2_format.rj;
>>>> +       ++vcpu->stat.cpucfg_exits;
>>>> +       index = vcpu->arch.gprs[rj];
>>>> +
>>>> +       /*
>>>> +        * By LoongArch Reference Manual 2.2.10.5
>>>> +        * Return value is 0 for undefined cpucfg index
>>>> +        */
>>>> +       switch (index) {
>>>> +       case 0 ... (KVM_MAX_CPUCFG_REGS - 1):
>>>> +               vcpu->arch.gprs[rd] = vcpu->arch.cpucfg[index];
>>>> +               break;
>>>> +       case CPUCFG_KVM_SIG:
>>>> +               vcpu->arch.gprs[rd] = *(unsigned int *)KVM_SIGNATURE;
>>>> +               break;
>>>> +       default:
>>>> +               vcpu->arch.gprs[rd] = 0;
>>>> +               break;
>>>> +       }
>>>> +
>>>> +       return EMULATE_DONE;
>>>> +}
>>>> +
>>>> +static int kvm_trap_handle_gspr(struct kvm_vcpu *vcpu)
>>>> +{
>>>>           unsigned long curr_pc;
>>>>           larch_inst inst;
>>>>           enum emulation_result er = EMULATE_DONE;
>>>> @@ -224,21 +251,8 @@ static int kvm_trap_handle_gspr(struct kvm_vcpu *vcpu)
>>>>           er = EMULATE_FAIL;
>>>>           switch (((inst.word >> 24) & 0xff)) {
>>>>           case 0x0: /* CPUCFG GSPR */
>>>> -               if (inst.reg2_format.opcode == 0x1B) {
>>>> -                       rd = inst.reg2_format.rd;
>>>> -                       rj = inst.reg2_format.rj;
>>>> -                       ++vcpu->stat.cpucfg_exits;
>>>> -                       index = vcpu->arch.gprs[rj];
>>>> -                       er = EMULATE_DONE;
>>>> -                       /*
>>>> -                        * By LoongArch Reference Manual 2.2.10.5
>>>> -                        * return value is 0 for undefined cpucfg index
>>>> -                        */
>>>> -                       if (index < KVM_MAX_CPUCFG_REGS)
>>>> -                               vcpu->arch.gprs[rd] = vcpu->arch.cpucfg[index];
>>>> -                       else
>>>> -                               vcpu->arch.gprs[rd] = 0;
>>>> -               }
>>>> +               if (inst.reg2_format.opcode == cpucfg_op)
>>>> +                       er = kvm_emu_cpucfg(vcpu, inst);
>>>>                   break;
>>>>           case 0x4: /* CSR{RD,WR,XCHG} GSPR */
>>>>                   er = kvm_handle_csr(vcpu, inst);
>>>> --
>>>> 2.39.3
>>>>
>>
>>