| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 2 Jul 2024 16:42:07 +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/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?
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