| Message ID | 20180306162646.7646-1-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Peter, A couple of comments: On 06/03/18 16:26, Peter Maydell wrote: > We have a KVM_REG_ARM encoding that we use to expose KVM guest registers > to userspace. Define that bit 28 in this encoding indicates secure vs > nonsecure, so we can distinguish the secure and nonsecure banked versions > of a banked AArch32 register. > > For KVM currently, all guest registers are nonsecure, but defining > the bit is useful for userspace. In particular, QEMU uses this > encoding as part of its on-the-wire migration format, and needs to be > able to describe secure-bank registers when it is migrating (fully > emulated) EL3-enabled CPUs. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > arch/arm/include/uapi/asm/kvm.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h > index 6edd177bb1c7..e0d742c36cb1 100644 > --- a/arch/arm/include/uapi/asm/kvm.h > +++ b/arch/arm/include/uapi/asm/kvm.h > @@ -135,6 +135,11 @@ struct kvm_arch_memory_slot { > #define KVM_REG_ARM_CRM_SHIFT 7 > #define KVM_REG_ARM_32_CRN_MASK 0x0000000000007800 > #define KVM_REG_ARM_32_CRN_SHIFT 11 > +/* For KVM currently all guest registers are nonsecure, but we reserve a bit > + * in the encoding to distinguish secure from nonsecure for banked registers. > + */ Nit: /* * This is the canonical comment style. */ It might be worth pointing out in the comment that this only applies to AArch32 system registers that are banked by security. > +#define KVM_REG_ARM_SECURE_MASK 0x0000000010000000 > +#define KVM_REG_ARM_SECURE_SHIFT 28 > > #define ARM_CP15_REG_SHIFT_MASK(x,n) \ > (((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK) > Don't you also need to define it on the arm64 side? Thanks, M.
On 6 March 2018 at 16:43, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 06/03/18 16:26, Peter Maydell wrote: >> +/* For KVM currently all guest registers are nonsecure, but we reserve a bit >> + * in the encoding to distinguish secure from nonsecure for banked registers. >> + */ > > Nit: > > /* > * This is the canonical comment style. > */ > > It might be worth pointing out in the comment that this only applies to > AArch32 system registers that are banked by security. Sure. How about /* * For KVM currently all guest registers are nonsecure, but we reserve a bit * in the encoding to distinguish secure from nonsecure for AArch32 system * registers that are banked by security. This is 1 for the secure banked * register, and 0 for the nonsecure banked register or if the register is * not banked by security. */ ? >> +#define KVM_REG_ARM_SECURE_MASK 0x0000000010000000 >> +#define KVM_REG_ARM_SECURE_SHIFT 28 >> >> #define ARM_CP15_REG_SHIFT_MASK(x,n) \ >> (((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK) >> > > Don't you also need to define it on the arm64 side? I don't think so, because AArch64 registers aren't security banked (except for some GICv3 ones, which aren't set via GET/SET_ONE_REG.) Or is there a case I'm missing? thanks -- PMM
On 06/03/18 16:56, Peter Maydell wrote: > On 6 March 2018 at 16:43, Marc Zyngier <marc.zyngier@arm.com> wrote: >> On 06/03/18 16:26, Peter Maydell wrote: >>> +/* For KVM currently all guest registers are nonsecure, but we reserve a bit >>> + * in the encoding to distinguish secure from nonsecure for banked registers. >>> + */ >> >> Nit: >> >> /* >> * This is the canonical comment style. >> */ >> >> It might be worth pointing out in the comment that this only applies to >> AArch32 system registers that are banked by security. > > Sure. How about > > /* > * For KVM currently all guest registers are nonsecure, but we reserve a bit > * in the encoding to distinguish secure from nonsecure for AArch32 system > * registers that are banked by security. This is 1 for the secure banked > * register, and 0 for the nonsecure banked register or if the register is > * not banked by security. > */ > > ? Looks good to me. > >>> +#define KVM_REG_ARM_SECURE_MASK 0x0000000010000000 >>> +#define KVM_REG_ARM_SECURE_SHIFT 28 >>> >>> #define ARM_CP15_REG_SHIFT_MASK(x,n) \ >>> (((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK) >>> >> >> Don't you also need to define it on the arm64 side? > > I don't think so, because AArch64 registers aren't security banked > (except for some GICv3 ones, which aren't set via GET/SET_ONE_REG.) > Or is there a case I'm missing? There is still the case of AArch32 guests on arm64, for which you'd need to replicate the change to arch/arm64/include/uapi/asm/kvm.h. The two architectures have different userspace APIs. Thanks, M.
On 6 March 2018 at 17:16, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 06/03/18 16:56, Peter Maydell wrote: >> On 6 March 2018 at 16:43, Marc Zyngier <marc.zyngier@arm.com> wrote: >>> On 06/03/18 16:26, Peter Maydell wrote: >>>> +#define KVM_REG_ARM_SECURE_MASK 0x0000000010000000 >>>> +#define KVM_REG_ARM_SECURE_SHIFT 28 >>>> >>>> #define ARM_CP15_REG_SHIFT_MASK(x,n) \ >>>> (((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK) >>>> >>> >>> Don't you also need to define it on the arm64 side? >> >> I don't think so, because AArch64 registers aren't security banked >> (except for some GICv3 ones, which aren't set via GET/SET_ONE_REG.) >> Or is there a case I'm missing? > > There is still the case of AArch32 guests on arm64, for which you'd need > to replicate the change to arch/arm64/include/uapi/asm/kvm.h. The two > architectures have different userspace APIs. I thought that AArch32 guests on arm64 hosts still used the arm64 GET/SET_ONE_REG registers to set the guest's environment? The guest's r0-r7 are in what KVM considers X0..X7, etc, and cp15 registers are accessed by reading/writing their architecturally mapped AArch64 EL1 counterparts. thanks -- PMM
On 06/03/18 18:51, Peter Maydell wrote: > On 6 March 2018 at 17:16, Marc Zyngier <marc.zyngier@arm.com> wrote: >> On 06/03/18 16:56, Peter Maydell wrote: >>> On 6 March 2018 at 16:43, Marc Zyngier <marc.zyngier@arm.com> wrote: >>>> On 06/03/18 16:26, Peter Maydell wrote: >>>>> +#define KVM_REG_ARM_SECURE_MASK 0x0000000010000000 >>>>> +#define KVM_REG_ARM_SECURE_SHIFT 28 >>>>> >>>>> #define ARM_CP15_REG_SHIFT_MASK(x,n) \ >>>>> (((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK) >>>>> >>>> >>>> Don't you also need to define it on the arm64 side? >>> >>> I don't think so, because AArch64 registers aren't security banked >>> (except for some GICv3 ones, which aren't set via GET/SET_ONE_REG.) >>> Or is there a case I'm missing? >> >> There is still the case of AArch32 guests on arm64, for which you'd need >> to replicate the change to arch/arm64/include/uapi/asm/kvm.h. The two >> architectures have different userspace APIs. > > I thought that AArch32 guests on arm64 hosts still used the > arm64 GET/SET_ONE_REG registers to set the guest's environment? > The guest's r0-r7 are in what KVM considers X0..X7, etc, and > cp15 registers are accessed by reading/writing their architecturally > mapped AArch64 EL1 counterparts. You're right, there is no banked state at all on arm64. Ignore me. Can you please respin this patch with the updated comment? I'll queue it for 4.17. Thanks, M.
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h index 6edd177bb1c7..e0d742c36cb1 100644 --- a/arch/arm/include/uapi/asm/kvm.h +++ b/arch/arm/include/uapi/asm/kvm.h @@ -135,6 +135,11 @@ struct kvm_arch_memory_slot { #define KVM_REG_ARM_CRM_SHIFT 7 #define KVM_REG_ARM_32_CRN_MASK 0x0000000000007800 #define KVM_REG_ARM_32_CRN_SHIFT 11 +/* For KVM currently all guest registers are nonsecure, but we reserve a bit + * in the encoding to distinguish secure from nonsecure for banked registers. + */ +#define KVM_REG_ARM_SECURE_MASK 0x0000000010000000 +#define KVM_REG_ARM_SECURE_SHIFT 28 #define ARM_CP15_REG_SHIFT_MASK(x,n) \ (((x) << KVM_REG_ARM_ ## n ## _SHIFT) & KVM_REG_ARM_ ## n ## _MASK)
We have a KVM_REG_ARM encoding that we use to expose KVM guest registers to userspace. Define that bit 28 in this encoding indicates secure vs nonsecure, so we can distinguish the secure and nonsecure banked versions of a banked AArch32 register. For KVM currently, all guest registers are nonsecure, but defining the bit is useful for userspace. In particular, QEMU uses this encoding as part of its on-the-wire migration format, and needs to be able to describe secure-bank registers when it is migrating (fully emulated) EL3-enabled CPUs. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- arch/arm/include/uapi/asm/kvm.h | 5 +++++ 1 file changed, 5 insertions(+)