Re: [PATCH v5 3/6] LoongArch: KVM: Add cpucfg area for kvm hypervisor

From: Jiaxun Yang
Date: Tue Feb 27 2024 - 00:24:18 EST

Next message: Andrew Jeffery: "[PATCH v5] dt-bindings: gpio: aspeed,ast2400-gpio: Convert to DT schema"
Previous message: Thorsten Scherer: "Re: [PATCH] siox: constify the struct device_type usage"
In reply to: maobibo: "Re: [PATCH v5 3/6] LoongArch: KVM: Add cpucfg area for kvm hypervisor"
Next in thread: maobibo: "Re: [PATCH v5 3/6] LoongArch: KVM: Add cpucfg area for kvm hypervisor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

在2024年2月27日二月上午3:14，maobibo写道：
> On 2024/2/27 上午4:02, Jiaxun Yang wrote:
>>
>>
>> 在2024年2月26日二月上午8:04，maobibo写道：
>>> On 2024/2/26 下午2:12, Huacai Chen wrote:
>>>> On Mon, Feb 26, 2024 at 10:04 AM maobibo <maobibo@xxxxxxxxxxx> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2024/2/24 下午5:13, Huacai Chen wrote:
>>>>>> Hi, Bibo,
>>>>>>
>>>>>> On Thu, Feb 22, 2024 at 11:28 AM Bibo Mao <maobibo@xxxxxxxxxxx> 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.
>>
>> I do believe that all those stuff should not be exposed to guest user space
>> for security reasons.
> Can you add PLV checking when cpucfg 0x40000000-0x400000FF is emulated?
> if it is user mode return value is zero and it is kernel mode emulated
> value will be returned. It can avoid information leaking.

Please don’t do insane stuff here, applications are not expecting exception from
cpucfg.

>
>>
>> Also for different implementations of hypervisors they may have different
>> PV features behavior, using hypcall to perform feature detection
>> can pass more information to help us cope with hypervisor diversity.
> How do different hypervisors can be detected firstly? On x86 MSR is
> used for all hypervisors detection and on ARM64 hyperv used
> acpi_gbl_FADT and kvm use smc forcely, host mode can execute smc
> instruction without exception on ARM64.

That’s hypcall ABI design choices, those information can come from firmware
or privileged CSRs on LoongArch.

>
> I do not know why hypercall is better than cpucfg on LoongArch, cpucfg
> is basic intruction however hypercall is not, it is part of LVZ feature.

KVM can only work with LVZ right?

>
>>>
>>>> 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.
>>
>> Those are *device* level information, they must be passed in firmware
>> interfaces to keep processor emulation sane.
> File arch/x86/hyperv/hv_apic.c can be referenced, apic features comes
> from ms_hyperv.hints and HYPERV_CPUID_ENLIGHTMENT_INFO cpuid info, not
> must be passed by firmware interface.

That’s not KVM, that’s Hyper V. At Linux Kernel we enjoy the benefits of better
modularity on device abstractions, please don’t break it.

Thanks

>
> Regards
> Bibo Mao
>>
>> Thanks
>>
>>>
>>> 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@xxxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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/exitc
>>>>>>> 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
>>>>>>>
>>>>>
>>>>>
>>

--
- Jiaxun

Next message: Andrew Jeffery: "[PATCH v5] dt-bindings: gpio: aspeed,ast2400-gpio: Convert to DT schema"
Previous message: Thorsten Scherer: "Re: [PATCH] siox: constify the struct device_type usage"
In reply to: maobibo: "Re: [PATCH v5 3/6] LoongArch: KVM: Add cpucfg area for kvm hypervisor"
Next in thread: maobibo: "Re: [PATCH v5 3/6] LoongArch: KVM: Add cpucfg area for kvm hypervisor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]