Received-SPF: pass (google.com: domain of linux-kernel+bounces-204169-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <9bb552c8-fe86-43dc-9c4e-0b95c99fb25c@xen0n.name>
Date: Thu, 6 Jun 2024 19:20:10 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] LoongArch: KVM: Add feature passing from user space
To: Bibo Mao <maobibo@loongson.cn>, 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>
Content-Language: en-US
From: WANG Xuerui <kernel@xen0n.name>
In-Reply-To: <20240606074850.2651896-1-maobibo@loongson.cn>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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

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

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

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

> +		return 0;
> +
> +	default:
> +		return -ENXIO;
> +	}
>   }
>   
>   static int kvm_loongarch_vcpu_set_attr(struct kvm_vcpu *vcpu,
> 
> base-commit: 2df0193e62cf887f373995fb8a91068562784adc

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/