On Mon, Sep 2, 2024 at 10:45 AM maobibo <maobibo@xxxxxxxxxxx> wrote:
>
>
>
> On 2024/9/2 上午10:20, Huacai Chen wrote:
> > On Mon, Sep 2, 2024 at 9:56 AM maobibo <maobibo@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>> ---
> >>>> 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?
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
> >>>>
> >>
> >>
>
>
