| Message ID | 20220817214818.3243383-6-oliver.upton@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Treat 32bit ID registers as RAZ/WI on 64bit-only system | expand |
On Wed, 17 Aug 2022 22:48:17 +0100, Oliver Upton <oliver.upton@linux.dev> wrote: > > One of the oddities of the architecture is that the AArch64 views of the > AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any EL. > Nonetheless, KVM exposes these registers to userspace for the sake of > save/restore. It is possible that the UNKNOWN value could differ between > systems, leading to a rejected write from userspace. > > Avoid the issue altogether by handling the AArch32 ID registers as > RAZ/WI when on an AArch64-only system. > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > --- > arch/arm64/kvm/sys_regs.c | 63 ++++++++++++++++++++++++++------------- > 1 file changed, 43 insertions(+), 20 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 9f06c85f26b8..5f6a633182c8 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1145,6 +1145,20 @@ static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > return 0; > } > > +static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *r) > +{ > + /* > + * AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any > + * EL. Promote to RAZ/WI in order to guarantee consistency between > + * systems. > + */ > + if (!kvm_supports_32bit_el0()) > + return REG_RAZ | REG_USER_WI; This is probably only a nit, but why does one visibility has a _USER_ tag while the other doesn't? In other word, what sysregs are WI from userspace that aren't so from the guest? Also, do we have any cases where RAZ and WI would be used independently? My gut feeling is that RAZ implies WI in most (all?) cases. If this assumption holds, shouldn't we simply rename REG_RAZ to REG_RAZ_WI and be done with it? Thanks, M.
Hey Marc, Thanks for the review! On Tue, Aug 23, 2022 at 06:05:28PM +0100, Marc Zyngier wrote: > On Wed, 17 Aug 2022 22:48:17 +0100, > Oliver Upton <oliver.upton@linux.dev> wrote: > > > > One of the oddities of the architecture is that the AArch64 views of the > > AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any EL. > > Nonetheless, KVM exposes these registers to userspace for the sake of > > save/restore. It is possible that the UNKNOWN value could differ between > > systems, leading to a rejected write from userspace. > > > > Avoid the issue altogether by handling the AArch32 ID registers as > > RAZ/WI when on an AArch64-only system. > > > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > > --- > > arch/arm64/kvm/sys_regs.c | 63 ++++++++++++++++++++++++++------------- > > 1 file changed, 43 insertions(+), 20 deletions(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index 9f06c85f26b8..5f6a633182c8 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -1145,6 +1145,20 @@ static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > > return 0; > > } > > > > +static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu, > > + const struct sys_reg_desc *r) > > +{ > > + /* > > + * AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any > > + * EL. Promote to RAZ/WI in order to guarantee consistency between > > + * systems. > > + */ > > + if (!kvm_supports_32bit_el0()) > > + return REG_RAZ | REG_USER_WI; > > This is probably only a nit, but why does one visibility has a _USER_ > tag while the other doesn't? In other word, what sysregs are WI from > userspace that aren't so from the guest? > > Also, do we have any cases where RAZ and WI would be used > independently? My gut feeling is that RAZ implies WI in most (all?) > cases. If this assumption holds, shouldn't we simply rename REG_RAZ to > REG_RAZ_WI and be done with it? Yeah, this reads a bit strange, but there is some reason around it (I think!) As it applies to ID registers, REG_RAZ already implies RAZ w/ immutable writes (-EINVAL if something different is written). As such I didn't want to change the meaning of the other ID registers to WI and only ignore writes for the registers that could have an UNKNOWN value. Furthermore, I added the _USER_ tag to make it clear that we aren't magically allowing writes from the guest to these registers. I think we will need an additional visibility bit (or special accessor, which I tried to avoid) to precisely apply WI to the 32bit registers, but if the _USER_ tag is distracting I can get rid of it. After all, hardware should politely UNDEF the guest when writing to such a register. Thoughts? -- Thanks, Oliver
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 9f06c85f26b8..5f6a633182c8 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1145,6 +1145,20 @@ static unsigned int id_visibility(const struct kvm_vcpu *vcpu, return 0; } +static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu, + const struct sys_reg_desc *r) +{ + /* + * AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any + * EL. Promote to RAZ/WI in order to guarantee consistency between + * systems. + */ + if (!kvm_supports_32bit_el0()) + return REG_RAZ | REG_USER_WI; + + return id_visibility(vcpu, r); +} + static unsigned int raz_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { @@ -1341,6 +1355,15 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu, .visibility = id_visibility, \ } +/* sys_reg_desc initialiser for known cpufeature ID registers */ +#define AA32_ID_SANITISED(name) { \ + SYS_DESC(SYS_##name), \ + .access = access_id_reg, \ + .get_user = get_id_reg, \ + .set_user = set_id_reg, \ + .visibility = aa32_id_visibility, \ +} + /* * sys_reg_desc initialiser for architecturally unallocated cpufeature ID * register with encoding Op0=3, Op1=0, CRn=0, CRm=crm, Op2=op2 @@ -1428,33 +1451,33 @@ static const struct sys_reg_desc sys_reg_descs[] = { /* AArch64 mappings of the AArch32 ID registers */ /* CRm=1 */ - ID_SANITISED(ID_PFR0_EL1), - ID_SANITISED(ID_PFR1_EL1), - ID_SANITISED(ID_DFR0_EL1), + AA32_ID_SANITISED(ID_PFR0_EL1), + AA32_ID_SANITISED(ID_PFR1_EL1), + AA32_ID_SANITISED(ID_DFR0_EL1), ID_HIDDEN(ID_AFR0_EL1), - ID_SANITISED(ID_MMFR0_EL1), - ID_SANITISED(ID_MMFR1_EL1), - ID_SANITISED(ID_MMFR2_EL1), - ID_SANITISED(ID_MMFR3_EL1), + AA32_ID_SANITISED(ID_MMFR0_EL1), + AA32_ID_SANITISED(ID_MMFR1_EL1), + AA32_ID_SANITISED(ID_MMFR2_EL1), + AA32_ID_SANITISED(ID_MMFR3_EL1), /* CRm=2 */ - ID_SANITISED(ID_ISAR0_EL1), - ID_SANITISED(ID_ISAR1_EL1), - ID_SANITISED(ID_ISAR2_EL1), - ID_SANITISED(ID_ISAR3_EL1), - ID_SANITISED(ID_ISAR4_EL1), - ID_SANITISED(ID_ISAR5_EL1), - ID_SANITISED(ID_MMFR4_EL1), - ID_SANITISED(ID_ISAR6_EL1), + AA32_ID_SANITISED(ID_ISAR0_EL1), + AA32_ID_SANITISED(ID_ISAR1_EL1), + AA32_ID_SANITISED(ID_ISAR2_EL1), + AA32_ID_SANITISED(ID_ISAR3_EL1), + AA32_ID_SANITISED(ID_ISAR4_EL1), + AA32_ID_SANITISED(ID_ISAR5_EL1), + AA32_ID_SANITISED(ID_MMFR4_EL1), + AA32_ID_SANITISED(ID_ISAR6_EL1), /* CRm=3 */ - ID_SANITISED(MVFR0_EL1), - ID_SANITISED(MVFR1_EL1), - ID_SANITISED(MVFR2_EL1), + AA32_ID_SANITISED(MVFR0_EL1), + AA32_ID_SANITISED(MVFR1_EL1), + AA32_ID_SANITISED(MVFR2_EL1), ID_UNALLOCATED(3,3), - ID_SANITISED(ID_PFR2_EL1), + AA32_ID_SANITISED(ID_PFR2_EL1), ID_HIDDEN(ID_DFR1_EL1), - ID_SANITISED(ID_MMFR5_EL1), + AA32_ID_SANITISED(ID_MMFR5_EL1), ID_UNALLOCATED(3,7), /* AArch64 ID registers */
One of the oddities of the architecture is that the AArch64 views of the AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any EL. Nonetheless, KVM exposes these registers to userspace for the sake of save/restore. It is possible that the UNKNOWN value could differ between systems, leading to a rejected write from userspace. Avoid the issue altogether by handling the AArch32 ID registers as RAZ/WI when on an AArch64-only system. Signed-off-by: Oliver Upton <oliver.upton@linux.dev> --- arch/arm64/kvm/sys_regs.c | 63 ++++++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 20 deletions(-)