Received-SPF: pass (google.com: domain of linux-kernel+bounces-205276-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240606074850.2651896-1-maobibo@loongson.cn> <9bb552c8-fe86-43dc-9c4e-0b95c99fb25c@xen0n.name>
 <2774b010-8033-2167-474a-cb1b29b27d2b@loongson.cn> <ca286a23-f22b-092c-20d0-6ab20fd0883f@loongson.cn>
In-Reply-To: <ca286a23-f22b-092c-20d0-6ab20fd0883f@loongson.cn>
From: Huacai Chen <chenhuacai@kernel.org>
Date: Fri, 7 Jun 2024 11:58:42 +0800
Message-ID: <CAAhV-H4R7GO33jggAHsq0A6qLejdhnht5eBLs3OSasCkcYwJmQ@mail.gmail.com>
Subject: Re: [PATCH] LoongArch: KVM: Add feature passing from user space
To: maobibo <maobibo@loongson.cn>
Cc: WANG Xuerui <kernel@xen0n.name>, Tianrui Zhao <zhaotianrui@loongson.cn>, kvm@vger.kernel.org, 
	loongarch@lists.linux.dev, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi, Bibo,


On Thu, Jun 6, 2024 at 8:05=E2=80=AFPM maobibo <maobibo@loongson.cn> wrote:
>
>
>
> On 2024/6/6 =E4=B8=8B=E5=8D=887:54, maobibo wrote:
> > Xuerui,
> >
> > Thanks for your reviewing.
> > I reply inline.
> >
> > On 2024/6/6 =E4=B8=8B=E5=8D=887: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 vali=
d.
> >>>
> >>> 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?
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

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 =3D (u64 __user *)attr->addr;
> >>> +    u64 val, valid_flags;
> >>> +
> >>> +    switch (attr->attr) {
> >>> +    case CPUCFG_KVM_FEATURE:
> >>> +        if (get_user(val, user))
> >>> +            return -EFAULT;
> >>> +
> >>> +        valid_flags =3D KVM_LOONGARCH_USR_FEAT_MASK;
> >>> +        if (val & ~valid_flags)
> >>> +            return -EINVAL;
> >>> +
> >>> +        vcpu->arch.usr_features |=3D val;
> >>
> >> Isn't this usage of "|=3D" instead of "=3D" implying that the feature =
bits
> >> could not get disabled after being enabled earlier, for whatever reaso=
n?
> > yes, "=3D" 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
> >>
> >
>
>