Received-SPF: pass (google.com: domain of linux-kernel+bounces-204227-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Subject: Re: [PATCH] LoongArch: KVM: Add feature passing from user space
From: maobibo <maobibo@loongson.cn>
To: WANG Xuerui <kernel@xen0n.name>, Tianrui Zhao <zhaotianrui@loongson.cn>,
 Huacai Chen <chenhuacai@kernel.org>
Cc: kvm@vger.kernel.org, loongarch@lists.linux.dev,
 linux-kernel@vger.kernel.org
References: <20240606074850.2651896-1-maobibo@loongson.cn>
 <9bb552c8-fe86-43dc-9c4e-0b95c99fb25c@xen0n.name>
 <2774b010-8033-2167-474a-cb1b29b27d2b@loongson.cn>
Message-ID: <ca286a23-f22b-092c-20d0-6ab20fd0883f@loongson.cn>
Date: Thu, 6 Jun 2024 20:05:14 +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: <2774b010-8033-2167-474a-cb1b29b27d2b@loongson.cn>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit


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@loongson.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?

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
>>
>