Message-ID: <686df603-d80b-2e0b-3699-02d574b302e3@loongson.cn>
Date: Tue, 9 Jul 2024 09:45:48 +0800
From: maobibo <maobibo@...ngson.cn>
To: Huacai Chen <chenhuacai@...nel.org>
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 2024/7/8 下午5:51, Huacai Chen wrote:
> 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.
LSX/LASX feature probing comes from CPU feature detecting.

> If you insist that all should be merged together, there is probably
> not enough time for the 6.11 merge window.
It is not so hurry for LBT feature, only that PMU function uses the same 
method, that will be delayed also. Almost all user visible features need 
feature detection.

Regards
Bibo Mao

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