| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fb5a5fb6-5c40-7d18-bb8d-2da3747480ff@loongson.cn>
Date: Fri, 7 Jun 2024 14:31:41 +0800
From: maobibo <maobibo@...ngson.cn>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: WANG Xuerui <kernel@...0n.name>, Tianrui Zhao <zhaotianrui@...ngson.cn>,
kvm@...r.kernel.org, loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] LoongArch: KVM: Add feature passing from user space
On 2024/6/7 上午11:58, Huacai Chen wrote:
> Hi, Bibo,
>
>
> On Thu, Jun 6, 2024 at 8:05 PM maobibo <maobibo@...ngson.cn> wrote:
>>
>>
>>
>> On 2024/6/6 下午7:54, maobibo wrote:
>>> Xuerui,
>>>
>>> Thanks for your reviewing.
>>> I reply inline.
>>>
>>> On 2024/6/6 下午7:20, WANG Xuerui wrote:
>>>> Hi,
>>>>
>>>> On 6/6/24 15:48, Bibo Mao wrote:
>>>>> Currently features defined in cpucfg CPUCFG_KVM_FEATURE comes from
>>>>> kvm kernel mode only. Some features are defined in user space VMM,
>>>>
>>>> "come from kernel side only. But currently KVM is not aware of
>>>> user-space VMM features which makes it hard to employ optimizations
>>>> that are both (1) specific to the VM use case and (2) requiring
>>>> cooperation from user space."
>>> Will modify in next version.
>>>>
>>>>> however KVM module does not know. Here interface is added to update
>>>>> register CPUCFG_KVM_FEATURE from user space, only bit 24 - 31 is valid.
>>>>>
>>>>> Feature KVM_LOONGARCH_VCPU_FEAT_VIRT_EXTIOI is added from user mdoe.
>>>>> FEAT_VIRT_EXTIOI is virt EXTIOI extension which can route interrupt
>>>>> to 256 VCPUs rather than 4 CPUs like real hw.
>>>>
>>>> "A new feature bit ... is added which can be set from user space.
>>>> FEAT_... indicates that the VM EXTIOI can route interrupts to 256
>>>> vCPUs, rather than 4 like with real HW."
>>> will modify in next version.
>>>
>>>>
>>>> (Am I right in paraphrasing the "EXTIOI" part?)
>>>>
>>>>>
>>>>> Signed-off-by: Bibo Mao <maobibo@...ngson.cn>
>>>>> ---
>>>>> arch/loongarch/include/asm/kvm_host.h | 4 +++
>>>>> arch/loongarch/include/asm/loongarch.h | 5 ++++
>>>>> arch/loongarch/include/uapi/asm/kvm.h | 2 ++
>>>>> arch/loongarch/kvm/exit.c | 1 +
>>>>> arch/loongarch/kvm/vcpu.c | 36 +++++++++++++++++++++++---
>>>>> 5 files changed, 44 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/loongarch/include/asm/kvm_host.h
>>>>> b/arch/loongarch/include/asm/kvm_host.h
>>>>> index 88023ab59486..8fa50d757247 100644
>>>>> --- a/arch/loongarch/include/asm/kvm_host.h
>>>>> +++ b/arch/loongarch/include/asm/kvm_host.h
>>>>> @@ -135,6 +135,9 @@ enum emulation_result {
>>>>> #define KVM_LARCH_HWCSR_USABLE (0x1 << 4)
>>>>> #define KVM_LARCH_LBT (0x1 << 5)
>>>>> +#define KVM_LOONGARCH_USR_FEAT_MASK \
>>>>> + BIT(KVM_LOONGARCH_VCPU_FEAT_VIRT_EXTIOI)
>>>>> +
>>>>> struct kvm_vcpu_arch {
>>>>> /*
>>>>> * Switch pointer-to-function type to unsigned long
>>>>> @@ -210,6 +213,7 @@ struct kvm_vcpu_arch {
>>>>> u64 last_steal;
>>>>> struct gfn_to_hva_cache cache;
>>>>> } st;
>>>>> + unsigned int usr_features;
>>>>> };
>>>>> static inline unsigned long readl_sw_gcsr(struct loongarch_csrs
>>>>> *csr, int reg)
>>>>> diff --git a/arch/loongarch/include/asm/loongarch.h
>>>>> b/arch/loongarch/include/asm/loongarch.h
>>>>> index 7a4633ef284b..4d9837512c19 100644
>>>>> --- a/arch/loongarch/include/asm/loongarch.h
>>>>> +++ b/arch/loongarch/include/asm/loongarch.h
>>>>> @@ -167,9 +167,14 @@
>>>>> #define CPUCFG_KVM_SIG (CPUCFG_KVM_BASE + 0)
>>>>> #define KVM_SIGNATURE "KVM\0"
>>>>> +/*
>>>>> + * BIT 24 - 31 is features configurable by user space vmm
>>>>> + */
>>>>> #define CPUCFG_KVM_FEATURE (CPUCFG_KVM_BASE + 4)
>>>>> #define KVM_FEATURE_IPI BIT(1)
>>>>> #define KVM_FEATURE_STEAL_TIME BIT(2)
>>>>> +/* With VIRT_EXTIOI feature, interrupt can route to 256 VCPUs */
>>>>> +#define KVM_FEATURE_VIRT_EXTIOI BIT(24)
>>>>> #ifndef __ASSEMBLY__
>>>>
>>>> What about assigning a new CPUCFG leaf ID for separating the two kinds
>>>> of feature flags very cleanly?
>>> For compatible issue like new kernel on old KVM host, to add a new
>>> CPUCFG leaf ID, a new feature need be defined on existing
>>> CPUCFG_KVM_FEATURE register. Such as:
>>> #define KVM_FEATURE_EXTEND_CPUCFG BIT(3)
>>>
>>> VM need check feature KVM_FEATURE_EXTEND_CPUCFG at first, and then read
>>> the new CPUCFG leaf ID if feature EXTEND_CPUCFG is enabled.
>>>
>>> That maybe makes it complicated since feature bit is enough now.
>> The default return value is zero with old kvm host, it is possible to
>> use a new CPUCFG leaf ID. Both methods are ok for me.
>>
>> Huacai,
>> What is your optnion about this?
> I don't think we need a new leaf, but is this feature detection
> duplicated with EXTIOI_VIRT_FEATURES here?
> https://lore.kernel.org/lkml/871q5a2lo8.ffs@tglx/T/#t
It is for compatible consideration. The result is unknown on old
hypervisor if new kernel accesses newly added IOCSR register
EXTIOI_VIRT_FEATURES.
For cpucfg instruction it is returns zero if it is not supported,
however there is no such spec for iocsr read or write instruction.
Regards
Bibo
>
> Huacai
>
>>
>> Regards
>> Bibo Mao
>>>>
>>>>> @@ -896,7 +907,24 @@ static int kvm_loongarch_vcpu_get_attr(struct
>>>>> kvm_vcpu *vcpu,
>>>>> static int kvm_loongarch_cpucfg_set_attr(struct kvm_vcpu *vcpu,
>>>>> struct kvm_device_attr *attr)
>>>>> {
>>>>> - return -ENXIO;
>>>>> + u64 __user *user = (u64 __user *)attr->addr;
>>>>> + u64 val, valid_flags;
>>>>> +
>>>>> + switch (attr->attr) {
>>>>> + case CPUCFG_KVM_FEATURE:
>>>>> + if (get_user(val, user))
>>>>> + return -EFAULT;
>>>>> +
>>>>> + valid_flags = KVM_LOONGARCH_USR_FEAT_MASK;
>>>>> + if (val & ~valid_flags)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + vcpu->arch.usr_features |= val;
>>>>
>>>> Isn't this usage of "|=" instead of "=" implying that the feature bits
>>>> could not get disabled after being enabled earlier, for whatever reason?
>>> yes, "=" is better. Will modify in next version.
>>>
>>> Regards
>>> Bibo Mao
>>>>
>>>>> + return 0;
>>>>> +
>>>>> + default:
>>>>> + return -ENXIO;
>>>>> + }
>>>>> }
>>>>> static int kvm_loongarch_vcpu_set_attr(struct kvm_vcpu *vcpu,
>>>>>
>>>>> base-commit: 2df0193e62cf887f373995fb8a91068562784adc
>>>>
>>>
>>
>>
Powered by blists - more mailing lists