| 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
| ||
|
Message-ID: <cf461609-5dfb-431c-4764-065aa884f28f@loongson.cn>
Date: Mon, 2 Sep 2024 12:15:35 +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, Jiaxun Yang <jiaxun.yang@...goat.com>
Subject: Re: [PATCH v6 3/3] LoongArch: KVM: Add vm migration support for LBT
registers
On 2024/9/2 下午12:04, Huacai Chen wrote:
> On Mon, Sep 2, 2024 at 10:45 AM maobibo <maobibo@...ngson.cn> wrote:
>>
>>
>>
>> On 2024/9/2 上午10:20, Huacai Chen wrote:
>>> On Mon, Sep 2, 2024 at 9:56 AM maobibo <maobibo@...ngson.cn> wrote:
>>>>
>>>>
>>>> Hi Huacai,
>>>>
>>>> On 2024/8/31 下午10:49, Huacai Chen wrote:
>>>>> Hi, Bibo,
>>>>>
>>>>> On Tue, Jul 30, 2024 at 3:57 PM Bibo Mao <maobibo@...ngson.cn> wrote:
>>>>>>
>>>>>> Every vcpu has separate LBT registers. And there are four scr registers,
>>>>>> one flags and ftop register for LBT extension. When VM migrates, VMM
>>>>>> needs to get LBT registers for every vcpu.
>>>>>>
>>>>>> Here macro KVM_REG_LOONGARCH_LBT is added for new vcpu lbt register type,
>>>>>> the following macro is added to get/put LBT registers.
>>>>>> KVM_REG_LOONGARCH_LBT_SCR0
>>>>>> KVM_REG_LOONGARCH_LBT_SCR1
>>>>>> KVM_REG_LOONGARCH_LBT_SCR2
>>>>>> KVM_REG_LOONGARCH_LBT_SCR3
>>>>>> KVM_REG_LOONGARCH_LBT_EFLAGS
>>>>>> KVM_REG_LOONGARCH_LBT_FTOP
>>>>>>
>>>>>> Signed-off-by: Bibo Mao <maobibo@...ngson.cn>
>>>>>> ---
>>>>>> arch/loongarch/include/uapi/asm/kvm.h | 9 +++++
>>>>>> arch/loongarch/kvm/vcpu.c | 56 +++++++++++++++++++++++++++
>>>>>> 2 files changed, 65 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/loongarch/include/uapi/asm/kvm.h b/arch/loongarch/include/uapi/asm/kvm.h
>>>>>> index 49bafac8b22d..003fb766c93f 100644
>>>>>> --- a/arch/loongarch/include/uapi/asm/kvm.h
>>>>>> +++ b/arch/loongarch/include/uapi/asm/kvm.h
>>>>>> @@ -64,6 +64,7 @@ struct kvm_fpu {
>>>>>> #define KVM_REG_LOONGARCH_KVM (KVM_REG_LOONGARCH | 0x20000ULL)
>>>>>> #define KVM_REG_LOONGARCH_FPSIMD (KVM_REG_LOONGARCH | 0x30000ULL)
>>>>>> #define KVM_REG_LOONGARCH_CPUCFG (KVM_REG_LOONGARCH | 0x40000ULL)
>>>>>> +#define KVM_REG_LOONGARCH_LBT (KVM_REG_LOONGARCH | 0x50000ULL)
>>>>>> #define KVM_REG_LOONGARCH_MASK (KVM_REG_LOONGARCH | 0x70000ULL)
>>>>> I think KVM_REG_LOONGARCH_MASK should contain all above register
>>>>> classes, so should it be (KVM_REG_LOONGARCH | 0x370000ULL)?
>>>> Sorry, maybe I miss something. What is the meaning of 0x370000ULL? How
>>>> does the value come from?
>>> It seems I misunderstood the mask, please ignore.
>>>
>>>>
>>>>>
>>>>>> #define KVM_CSR_IDX_MASK 0x7fff
>>>>>> #define KVM_CPUCFG_IDX_MASK 0x7fff
>>>>>> @@ -77,6 +78,14 @@ struct kvm_fpu {
>>>>>> /* Debugging: Special instruction for software breakpoint */
>>>>>> #define KVM_REG_LOONGARCH_DEBUG_INST (KVM_REG_LOONGARCH_KVM | KVM_REG_SIZE_U64 | 3)
>>>>>>
>>>>>> +/* LBT registers */
>>>>>> +#define KVM_REG_LOONGARCH_LBT_SCR0 (KVM_REG_LOONGARCH_LBT | KVM_REG_SIZE_U64 | 1)
>>>>>> +#define KVM_REG_LOONGARCH_LBT_SCR1 (KVM_REG_LOONGARCH_LBT | KVM_REG_SIZE_U64 | 2)
>>>>>> +#define KVM_REG_LOONGARCH_LBT_SCR2 (KVM_REG_LOONGARCH_LBT | KVM_REG_SIZE_U64 | 3)
>>>>>> +#define KVM_REG_LOONGARCH_LBT_SCR3 (KVM_REG_LOONGARCH_LBT | KVM_REG_SIZE_U64 | 4)
>>>>>> +#define KVM_REG_LOONGARCH_LBT_EFLAGS (KVM_REG_LOONGARCH_LBT | KVM_REG_SIZE_U64 | 5)
>>>>>> +#define KVM_REG_LOONGARCH_LBT_FTOP (KVM_REG_LOONGARCH_LBT | KVM_REG_SIZE_U64 | 6)
>>>>> FTOP is a 32bit register in other place of the kernel, is it correct
>>>>> to use U64 here?
>>>> It is deliberate and there is no 32bit compat requirement for kvm. ALL
>>>> regiester interfaces are defined as 64-bit.
>>>> On kernel and qemu side, ftop can be defined as 32bit still, however the
>>>> interface is 64-bit. So there is forced type conversion between u32 and
>>>> u64. There is no problem here.
>>> If you are sure, then no problem. But there is indeed KVM_REG_SIZE_U32
>>> in include/uapi/linux/kvm.h, and if we append more fields after ftop,
>>> define it as U64 may break memcpy().
>> yes, there is KVM_REG_SIZE_U32 definition, however LoongArch KVM does
>> not use it, else the safer checking is a little complicated. Now
>> parameter with KVM_REG_SIZE_U32 is simply treated as illegal.
> I think just add a "case KVM_REG_SIZE_U32" in kvm_set_reg/kvm_get_reg
> is OK, kvm_set_one_reg/kvm_get_one_reg don't need any modifications.
> No?
No, it is not so simple. If KVM_REG_SIZE_U32 is supported, if such user
application passes such parameters KVM_REG_LOONGARCH_LBT_FTOP64 or
KVM_REG_LOONGARCH_LBT_EFLAGS32 , should it be treated as illegal or
normal? If it is illegal, where should the safety checking code be put?
#define KVM_REG_LOONGARCH_LBT_FTOP64 (KVM_REG_LOONGARCH_LBT |
KVM_REG_SIZE_U64 | 6)
#define KVM_REG_LOONGARCH_LBT_FTOP32 (KVM_REG_LOONGARCH_LBT |
KVM_REG_SIZE_U32 | 6)
#define KVM_REG_LOONGARCH_LBT_EFLAGS64 (KVM_REG_LOONGARCH_LBT |
KVM_REG_SIZE_U64 | 5)
#define KVM_REG_LOONGARCH_LBT_EFLAGS32 (KVM_REG_LOONGARCH_LBT |
KVM_REG_SIZE_U32 | 5)
Regards
Bibo Mao
>
> Huacai
>>
>> And no memcpy() is used for ftop/cpucfg, there is assignment for every
>> single register like this:
>>
>> For cpucfg read/write:
>> vcpu->arch.cpucfg[id] = (u32)v;
>> *v = vcpu->arch.cpucfg[id];
>> For ftop read/write:
>> vcpu->arch.fpu.ftop = v;
>> *v = vcpu->arch.fpu.ftop;
>>
>> Regards
>> Bibo Mao
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> #define LOONGARCH_REG_SHIFT 3
>>>>>> #define LOONGARCH_REG_64(TYPE, REG) (TYPE | KVM_REG_SIZE_U64 | (REG << LOONGARCH_REG_SHIFT))
>>>>>> #define KVM_IOC_CSRID(REG) LOONGARCH_REG_64(KVM_REG_LOONGARCH_CSR, REG)
>>>>>> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
>>>>>> index b5324885a81a..b2500d4fa729 100644
>>>>>> --- a/arch/loongarch/kvm/vcpu.c
>>>>>> +++ b/arch/loongarch/kvm/vcpu.c
>>>>>> @@ -597,6 +597,34 @@ static int kvm_get_one_reg(struct kvm_vcpu *vcpu,
>>>>>> break;
>>>>>> }
>>>>>> break;
>>>>>> + case KVM_REG_LOONGARCH_LBT:
>>>>> What about adding FPU/LSX/LASX registers (if needed for migration) in
>>>>> kvm_{get, set}_one_reg() here?
>>>> If there is 512bit SIMD or other requirement, it will be added in
>>>> kvm_{get, set}_one_reg(). For FPU/LSX/LASX registers, there is common
>>>> API KVM_GET_FPU/KVM_SET_FPU here. The impmentation of QEMU only gets
>>>> FPU, the upper LSX/LASX is lost, we will submit a patch in qemu side,
>>>> the kvm kernel side is ok.
>>> OK, no problem.
>>>
>>> Huacai
>>>>
>>>> /*
>>>> * for KVM_GET_FPU and KVM_SET_FPU
>>>> */
>>>> struct kvm_fpu {
>>>> __u32 fcsr;
>>>> __u64 fcc; /* 8x8 */
>>>> struct kvm_fpureg {
>>>> __u64 val64[4];
>>>> } fpr[32];
>>>> };
>>>>
>>>> Regards
>>>> Bibo Mao
>>>>>
>>>>> Huacai
>>>>>
>>>>>> + if (!kvm_guest_has_lbt(&vcpu->arch))
>>>>>> + return -ENXIO;
>>>>>> +
>>>>>> + switch (reg->id) {
>>>>>> + case KVM_REG_LOONGARCH_LBT_SCR0:
>>>>>> + *v = vcpu->arch.lbt.scr0;
>>>>>> + break;
>>>>>> + case KVM_REG_LOONGARCH_LBT_SCR1:
>>>>>> + *v = vcpu->arch.lbt.scr1;
>>>>>> + break;
>>>>>> + case KVM_REG_LOONGARCH_LBT_SCR2:
>>>>>> + *v = vcpu->arch.lbt.scr2;
>>>>>> + break;
>>>>>> + case KVM_REG_LOONGARCH_LBT_SCR3:
>>>>>> + *v = vcpu->arch.lbt.scr3;
>>>>>> + break;
>>>>>> + case KVM_REG_LOONGARCH_LBT_EFLAGS:
>>>>>> + *v = vcpu->arch.lbt.eflags;
>>>>>> + break;
>>>>>> + case KVM_REG_LOONGARCH_LBT_FTOP:
>>>>>> + *v = vcpu->arch.fpu.ftop;
>>>>>> + break;
>>>>>> + default:
>>>>>> + ret = -EINVAL;
>>>>>> + break;
>>>>>> + }
>>>>>> + break;
>>>>>> default:
>>>>>> ret = -EINVAL;
>>>>>> break;
>>>>>> @@ -663,6 +691,34 @@ static int kvm_set_one_reg(struct kvm_vcpu *vcpu,
>>>>>> break;
>>>>>> }
>>>>>> break;
>>>>>> + case KVM_REG_LOONGARCH_LBT:
>>>>>> + if (!kvm_guest_has_lbt(&vcpu->arch))
>>>>>> + return -ENXIO;
>>>>>> +
>>>>>> + switch (reg->id) {
>>>>>> + case KVM_REG_LOONGARCH_LBT_SCR0:
>>>>>> + vcpu->arch.lbt.scr0 = v;
>>>>>> + break;
>>>>>> + case KVM_REG_LOONGARCH_LBT_SCR1:
>>>>>> + vcpu->arch.lbt.scr1 = v;
>>>>>> + break;
>>>>>> + case KVM_REG_LOONGARCH_LBT_SCR2:
>>>>>> + vcpu->arch.lbt.scr2 = v;
>>>>>> + break;
>>>>>> + case KVM_REG_LOONGARCH_LBT_SCR3:
>>>>>> + vcpu->arch.lbt.scr3 = v;
>>>>>> + break;
>>>>>> + case KVM_REG_LOONGARCH_LBT_EFLAGS:
>>>>>> + vcpu->arch.lbt.eflags = v;
>>>>>> + break;
>>>>>> + case KVM_REG_LOONGARCH_LBT_FTOP:
>>>>>> + vcpu->arch.fpu.ftop = v;
>>>>>> + break;
>>>>>> + default:
>>>>>> + ret = -EINVAL;
>>>>>> + break;
>>>>>> + }
>>>>>> + break;
>>>>>> default:
>>>>>> ret = -EINVAL;
>>>>>> break;
>>>>>> --
>>>>>> 2.39.3
>>>>>>
>>>>
>>>>
>>
>>
Powered by blists - more mailing lists