Re: [PATCH] LoongArch: KVM: Add feature passing from user space
From: maobibo
Date: Fri Jun 07 2024 - 02:32:06 EST
On 2024/6/7 上午11:58, Huacai Chen wrote:
Hi, Bibo,
On Thu, Jun 6, 2024 at 8:05 PM maobibo <maobibo@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
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