Message-ID: <CAAhV-H7Pqf6_MLwhe8eC0XvspMnUvxB=HdLvPsAT6U=m5SozCg@mail.gmail.com>
Date: Mon, 8 Jul 2024 17:51:29 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: maobibo <maobibo@...ngson.cn>
Cc: Tianrui Zhao <zhaotianrui@...ngson.cn>, WANG Xuerui <kernel@...0n.name>, kvm@...r.kernel.org, 
	loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] LoongArch: KVM: Add LBT feature detection function

On Thu, Jul 4, 2024 at 9:24 AM maobibo <maobibo@...ngson.cn> wrote:
>
>
>
> On 2024/7/3 下午11:35, Huacai Chen wrote:
> > On Wed, Jul 3, 2024 at 11:15 AM maobibo <maobibo@...ngson.cn> wrote:
> >>
> >>
> >>
> >> On 2024/7/2 下午11:43, Huacai Chen wrote:
> >>> On Tue, Jul 2, 2024 at 4:42 PM maobibo <maobibo@...ngson.cn> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2024/7/2 下午3:28, Huacai Chen wrote:
> >>>>> On Tue, Jul 2, 2024 at 12:13 PM maobibo <maobibo@...ngson.cn> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2024/7/2 上午10:34, Huacai Chen wrote:
> >>>>>>> On Tue, Jul 2, 2024 at 10:25 AM maobibo <maobibo@...ngson.cn> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 2024/7/2 上午9:59, Huacai Chen wrote:
> >>>>>>>>> On Tue, Jul 2, 2024 at 9:51 AM maobibo <maobibo@...ngson.cn> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Huacai,
> >>>>>>>>>>
> >>>>>>>>>> On 2024/7/1 下午6:26, Huacai Chen wrote:
> >>>>>>>>>>> On Mon, Jul 1, 2024 at 9:27 AM maobibo <maobibo@...ngson.cn> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Huacai,
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 2024/6/30 上午10:07, Huacai Chen wrote:
> >>>>>>>>>>>>> Hi, Bibo,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Wed, Jun 26, 2024 at 2:32 PM Bibo Mao <maobibo@...ngson.cn> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Two kinds of LBT feature detection are added here, one is VCPU
> >>>>>>>>>>>>>> feature, the other is VM feature. VCPU feature dection can only
> >>>>>>>>>>>>>> work with VCPU thread itself, and requires VCPU thread is created
> >>>>>>>>>>>>>> already. So LBT feature detection for VM is added also, it can
> >>>>>>>>>>>>>> be done even if VM is not created, and also can be done by any
> >>>>>>>>>>>>>> thread besides VCPU threads.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Loongson Binary Translation (LBT) feature is defined in register
> >>>>>>>>>>>>>> cpucfg2. Here LBT capability detection for VCPU is added.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Here ioctl command KVM_HAS_DEVICE_ATTR is added for VM, and macro
> >>>>>>>>>>>>>> KVM_LOONGARCH_VM_FEAT_CTRL is added to check supported feature. And
> >>>>>>>>>>>>>> three sub-features relative with LBT are added as following:
> >>>>>>>>>>>>>>         KVM_LOONGARCH_VM_FEAT_X86BT
> >>>>>>>>>>>>>>         KVM_LOONGARCH_VM_FEAT_ARMBT
> >>>>>>>>>>>>>>         KVM_LOONGARCH_VM_FEAT_MIPSBT
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Signed-off-by: Bibo Mao <maobibo@...ngson.cn>
> >>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>         arch/loongarch/include/uapi/asm/kvm.h |  6 ++++
> >>>>>>>>>>>>>>         arch/loongarch/kvm/vcpu.c             |  6 ++++
> >>>>>>>>>>>>>>         arch/loongarch/kvm/vm.c               | 44 ++++++++++++++++++++++++++-
> >>>>>>>>>>>>>>         3 files changed, 55 insertions(+), 1 deletion(-)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> diff --git a/arch/loongarch/include/uapi/asm/kvm.h b/arch/loongarch/include/uapi/asm/kvm.h
> >>>>>>>>>>>>>> index ddc5cab0ffd0..c40f7d9ffe13 100644
> >>>>>>>>>>>>>> --- a/arch/loongarch/include/uapi/asm/kvm.h
> >>>>>>>>>>>>>> +++ b/arch/loongarch/include/uapi/asm/kvm.h
> >>>>>>>>>>>>>> @@ -82,6 +82,12 @@ struct kvm_fpu {
> >>>>>>>>>>>>>>         #define KVM_IOC_CSRID(REG)             LOONGARCH_REG_64(KVM_REG_LOONGARCH_CSR, REG)
> >>>>>>>>>>>>>>         #define KVM_IOC_CPUCFG(REG)            LOONGARCH_REG_64(KVM_REG_LOONGARCH_CPUCFG, REG)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> +/* Device Control API on vm fd */
> >>>>>>>>>>>>>> +#define KVM_LOONGARCH_VM_FEAT_CTRL     0
> >>>>>>>>>>>>>> +#define  KVM_LOONGARCH_VM_FEAT_X86BT   0
> >>>>>>>>>>>>>> +#define  KVM_LOONGARCH_VM_FEAT_ARMBT   1
> >>>>>>>>>>>>>> +#define  KVM_LOONGARCH_VM_FEAT_MIPSBT  2
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>         /* Device Control API on vcpu fd */
> >>>>>>>>>>>>>>         #define KVM_LOONGARCH_VCPU_CPUCFG      0
> >>>>>>>>>>>>>>         #define KVM_LOONGARCH_VCPU_PVTIME_CTRL 1
> >>>>>>>>>>>>> If you insist that LBT should be a vm feature, then I suggest the
> >>>>>>>>>>>>> above two also be vm features. Though this is an UAPI change, but
> >>>>>>>>>>>>> CPUCFG is upstream in 6.10-rc1 and 6.10-final hasn't been released. We
> >>>>>>>>>>>>> have a chance to change it now.
> >>>>>>>>>>>>
> >>>>>>>>>>>> KVM_LOONGARCH_VCPU_PVTIME_CTRL need be attr percpu since every vcpu
> >>>>>>>>>>>> has is own different gpa address.
> >>>>>>>>>>> Then leave this as a vm feature.
> >>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> For KVM_LOONGARCH_VCPU_CPUCFG attr, it will not changed. We cannot break
> >>>>>>>>>>>> the API even if it is 6.10-rc1, VMM has already used this. Else there is
> >>>>>>>>>>>> uapi breaking now, still will be in future if we cannot control this.
> >>>>>>>>>>> UAPI changing before the first release is allowed, which means, we can
> >>>>>>>>>>> change this before the 6.10-final, but cannot change it after
> >>>>>>>>>>> 6.10-final.
> >>>>>>>>>> Now QEMU has already synced uapi to its own directory, also I never hear
> >>>>>>>>>> about this, with my experience with uapi change, there is only newly
> >>>>>>>>>> added or removed deprecated years ago.
> >>>>>>>>>>
> >>>>>>>>>> Is there any documentation about UAPI change rules?
> >>>>>>>>> No document, but learn from my more than 10 years upstream experience.
> >>>>>>>> Can you show me an example about with your rich upstream experience?
> >>>>>>> A simple example,
> >>>>>>> e877d705704d7c8fe17b6b5ebdfdb14b84c revert
> >>>>>>> 1dccdba084897443d116508a8ed71e0ac8a0 and it changes UAPI.
> >>>>>>> 1dccdba084897443d116508a8ed71e0ac8a0 is upstream in 6.9-rc1, and
> >>>>>>> e877d705704d7c8fe17b6b5ebdfdb14b84c can revert the behavior before
> >>>>>>> 6.9-final, but not after that.
> >>>>>>>
> >>>>>>> Before the first release, the code status is treated as "unstable", so
> >>>>>>> revert, modify is allowed. But after the first release, even if an
> >>>>>>> "error" should also be treated as a "bad feature".
> >>>>>> Huacai,
> >>>>>>
> >>>>>> Thanks for showing the example.
> >>>>>>
> >>>>>> For this issue, Can we adding new uapi and mark the old as deprecated?
> >>>>>> so that it can be removed after years.
> >>>>> Unnecessary, just remove the old one. Deprecation is for the usage
> >>>>> after the first release.
> >>>>>
> >>>>>>
> >>>>>> For me, it is too frequent to revert the old uapi, it is not bug and
> >>>>>> only that we have better method now. Also QEMU has synchronized the uapi
> >>>>>> to its directory already.
> >>>>> QEMU also hasn't been released after synchronizing the uapi, so it is
> >>>>> OK to remove the old api now.
> >>>> No, I will not do such thing. It is just a joke to revert the uapi.
> >>>>
> >>>> So just create new world and old world on Loongarch system again?
> >>> Again, code status before the first release is *unstable*, that status
> >>> is not enough to be a "world".
> >>>
> >>> It's your responsibility to make a good design at the beginning, but
> >>> you fail to do that. Fortunately we are before the first release;
> >>> unfortunately you don't want to do that.
> >> Yes, this is flaw at the beginning, however it can works and new abi can
> >> be added.
> >>
> >> If there is no serious bug and it is synced to QEMU already, I am not
> >> willing to revert uabi. Different projects have its own schedule plan,
> >> that is one reason. The most important reason may be that different
> >> peoples have different ways handling these issues.
> > In another thread I found that Jiaxun said he has a solution to make
> > LBT be a vcpu feature and still works well. However, that may take
> > some time and is too late for 6.11.
>
> It is welcome if Jiaxun provide patch for host machine type, I have no
> time give any feedback with suggestion of Jianxun now.
>
> >
> > But we have another choice now: just remove the UAPI and vm.c parts in
> > this series, let the LBT main parts be upstream in 6.11, and then
> > solve other problems after 6.11. Even if Jiaxun's solution isn't
> > usable, we can still use this old vm feature solution then.
>
> There is not useul if only LBT main part goes upstream. VMM cannot use
> LBT if control part is not merged.
There is no control part UAPI for LSX/LASX, but it works.
If you insist that all should be merged together, there is probably
not enough time for the 6.11 merge window.

Huacai

>
>  From another side, what do you like to do? Reviewing patch of others
> and give comments whatever grammar spelling or useful suggestions, or
> Writing patch which needs much efforts rather than somethings like
> feature configuration, BSP drivers.
>
> Regards
> Bibo Mao
>
> >
> >
> > Huacai
> >>
> >> Regards
> >> Bibo, Mao
> >>>
> >>>
> >>> Huacai
> >>>
> >>>>
> >>>> Regards
> >>>> Bibo, Mao
> >>>>
> >>>>>
> >>>>> Huacai
> >>>>>
> >>>>>>
> >>>>>> Regards
> >>>>>> Bibo, Mao
> >>>>>>>
> >>>>>>> Huacai
> >>>>>>>
> >>>>>>>
> >>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> How about adding new extra features capability for VM such as?
> >>>>>>>>>>>> +#define  KVM_LOONGARCH_VM_FEAT_LSX   3
> >>>>>>>>>>>> +#define  KVM_LOONGARCH_VM_FEAT_LASX  4
> >>>>>>>>>>> They should be similar as LBT, if LBT is vcpu feature, they should
> >>>>>>>>>>> also be vcpu features; if LBT is vm feature, they should also be vm
> >>>>>>>>>>> features.
> >>>>>>>>>> On other architectures, with function kvm_vm_ioctl_check_extension()
> >>>>>>>>>>          KVM_CAP_XSAVE2/KVM_CAP_PMU_CAPABILITY on x86
> >>>>>>>>>>          KVM_CAP_ARM_PMU_V3/KVM_CAP_ARM_SVE on arm64
> >>>>>>>>>> These features are all cpu features, at the same time they are VM features.
> >>>>>>>>>>
> >>>>>>>>>> If they are cpu features, how does VMM detect validity of these features
> >>>>>>>>>> passing from command line? After all VCPUs are created and send bootup
> >>>>>>>>>> command to these VCPUs? That is too late, VMM main thread is easy to
> >>>>>>>>>> detect feature validity if they are VM features also.
> >>>>>>>>>>
> >>>>>>>>>> To be honest, I am not familiar with KVM still, only get further
> >>>>>>>>>> understanding after actual problems solving. Welcome to give comments,
> >>>>>>>>>> however please read more backgroud if you insist on, else there will be
> >>>>>>>>>> endless argument again.
> >>>>>>>>> I just say CPUCFG/LSX/LASX and LBT should be in the same class, I
> >>>>>>>>> haven't insisted on whether they should be vcpu features or vm
> >>>>>>>>> features.
> >>>>>>>> It is reasonable if LSX/LASX/LBT should be in the same class, since
> >>>>>>>> there is feature options such as lsx=on/off,lasx=on/off,lbt=on/off.
> >>>>>>>>
> >>>>>>>> What is the usage about CPUCFG capability used for VM feature? It is not
> >>>>>>>> a detailed feature, it is only feature-set indicator like cpuid.
> >>>>>>>>
> >>>>>>>> Regards
> >>>>>>>> Bibo Mao
> >>>>>>>>>
> >>>>>>>>> Huacai
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Regards
> >>>>>>>>>> Bibo, Mao
> >>>>>>>>>>>
> >>>>>>>>>>> Huacai
> >>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Regards
> >>>>>>>>>>>> Bibo Mao
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Huacai
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> >>>>>>>>>>>>>> index 233d28d0e928..9734b4d8db05 100644
> >>>>>>>>>>>>>> --- a/arch/loongarch/kvm/vcpu.c
> >>>>>>>>>>>>>> +++ b/arch/loongarch/kvm/vcpu.c
> >>>>>>>>>>>>>> @@ -565,6 +565,12 @@ static int _kvm_get_cpucfg_mask(int id, u64 *v)
> >>>>>>>>>>>>>>                                *v |= CPUCFG2_LSX;
> >>>>>>>>>>>>>>                        if (cpu_has_lasx)
> >>>>>>>>>>>>>>                                *v |= CPUCFG2_LASX;
> >>>>>>>>>>>>>> +               if (cpu_has_lbt_x86)
> >>>>>>>>>>>>>> +                       *v |= CPUCFG2_X86BT;
> >>>>>>>>>>>>>> +               if (cpu_has_lbt_arm)
> >>>>>>>>>>>>>> +                       *v |= CPUCFG2_ARMBT;
> >>>>>>>>>>>>>> +               if (cpu_has_lbt_mips)
> >>>>>>>>>>>>>> +                       *v |= CPUCFG2_MIPSBT;
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>                        return 0;
> >>>>>>>>>>>>>>                case LOONGARCH_CPUCFG3:
> >>>>>>>>>>>>>> diff --git a/arch/loongarch/kvm/vm.c b/arch/loongarch/kvm/vm.c
> >>>>>>>>>>>>>> index 6b2e4f66ad26..09e05108c68b 100644
> >>>>>>>>>>>>>> --- a/arch/loongarch/kvm/vm.c
> >>>>>>>>>>>>>> +++ b/arch/loongarch/kvm/vm.c
> >>>>>>>>>>>>>> @@ -99,7 +99,49 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >>>>>>>>>>>>>>                return r;
> >>>>>>>>>>>>>>         }
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> +static int kvm_vm_feature_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
> >>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>> +       switch (attr->attr) {
> >>>>>>>>>>>>>> +       case KVM_LOONGARCH_VM_FEAT_X86BT:
> >>>>>>>>>>>>>> +               if (cpu_has_lbt_x86)
> >>>>>>>>>>>>>> +                       return 0;
> >>>>>>>>>>>>>> +               return -ENXIO;
> >>>>>>>>>>>>>> +       case KVM_LOONGARCH_VM_FEAT_ARMBT:
> >>>>>>>>>>>>>> +               if (cpu_has_lbt_arm)
> >>>>>>>>>>>>>> +                       return 0;
> >>>>>>>>>>>>>> +               return -ENXIO;
> >>>>>>>>>>>>>> +       case KVM_LOONGARCH_VM_FEAT_MIPSBT:
> >>>>>>>>>>>>>> +               if (cpu_has_lbt_mips)
> >>>>>>>>>>>>>> +                       return 0;
> >>>>>>>>>>>>>> +               return -ENXIO;
> >>>>>>>>>>>>>> +       default:
> >>>>>>>>>>>>>> +               return -ENXIO;
> >>>>>>>>>>>>>> +       }
> >>>>>>>>>>>>>> +}
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> +static int kvm_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
> >>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>> +       switch (attr->group) {
> >>>>>>>>>>>>>> +       case KVM_LOONGARCH_VM_FEAT_CTRL:
> >>>>>>>>>>>>>> +               return kvm_vm_feature_has_attr(kvm, attr);
> >>>>>>>>>>>>>> +       default:
> >>>>>>>>>>>>>> +               return -ENXIO;
> >>>>>>>>>>>>>> +       }
> >>>>>>>>>>>>>> +}
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>         int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> >>>>>>>>>>>>>>         {
> >>>>>>>>>>>>>> -       return -ENOIOCTLCMD;
> >>>>>>>>>>>>>> +       struct kvm *kvm = filp->private_data;
> >>>>>>>>>>>>>> +       void __user *argp = (void __user *)arg;
> >>>>>>>>>>>>>> +       struct kvm_device_attr attr;
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> +       switch (ioctl) {
> >>>>>>>>>>>>>> +       case KVM_HAS_DEVICE_ATTR:
> >>>>>>>>>>>>>> +               if (copy_from_user(&attr, argp, sizeof(attr)))
> >>>>>>>>>>>>>> +                       return -EFAULT;
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> +               return kvm_vm_has_attr(kvm, &attr);
> >>>>>>>>>>>>>> +       default:
> >>>>>>>>>>>>>> +               return -EINVAL;
> >>>>>>>>>>>>>> +       }
> >>>>>>>>>>>>>>         }
> >>>>>>>>>>>>>> --
> >>>>>>>>>>>>>> 2.39.3
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>
> >>
> >>
>
>

Powered by blists - more mailing lists