| Message ID | 20231023095444.1587322-6-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: NV trap forwarding fixes | expand |
Hi Marc, > On 23 Oct 2023, at 09:54, Marc Zyngier <maz@kernel.org> wrote: > > When trapping accesses from a NV guest that tries to access > SPSR_{irq,abt,und,fiq}, make sure we handle them as RAZ/WI, > as if AArch32 wasn't implemented. > > This involves a bit of repainting to make the visibility > handler more generic. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/sysreg.h | 4 ++++ > arch/arm64/kvm/sys_regs.c | 16 +++++++++++++--- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 4a20a7dc5bc4..5e65f51c10d2 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -505,6 +505,10 @@ > #define SYS_SPSR_EL2 sys_reg(3, 4, 4, 0, 0) > #define SYS_ELR_EL2 sys_reg(3, 4, 4, 0, 1) > #define SYS_SP_EL1 sys_reg(3, 4, 4, 1, 0) > +#define SYS_SPSR_irq sys_reg(3, 4, 4, 3, 0) > +#define SYS_SPSR_abt sys_reg(3, 4, 4, 3, 1) > +#define SYS_SPSR_und sys_reg(3, 4, 4, 3, 2) > +#define SYS_SPSR_fiq sys_reg(3, 4, 4, 3, 3) > #define SYS_IFSR32_EL2 sys_reg(3, 4, 5, 0, 1) > #define SYS_AFSR0_EL2 sys_reg(3, 4, 5, 1, 0) > #define SYS_AFSR1_EL2 sys_reg(3, 4, 5, 1, 1) > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 0071ccccaf00..be1ebd2c5ba0 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1791,8 +1791,8 @@ static unsigned int el2_visibility(const struct kvm_vcpu *vcpu, > * HCR_EL2.E2H==1, and only in the sysreg table for convenience of > * handling traps. Given that, they are always hidden from userspace. > */ > -static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu, > - const struct sys_reg_desc *rd) > +static unsigned int hidden_user_visibility(const struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *rd) > { > return REG_HIDDEN_USER; > } > @@ -1803,7 +1803,7 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu, > .reset = rst, \ > .reg = name##_EL1, \ > .val = v, \ > - .visibility = elx2_visibility, \ > + .visibility = hidden_user_visibility, \ > } > > /* > @@ -2387,6 +2387,16 @@ static const struct sys_reg_desc sys_reg_descs[] = { > EL2_REG(ELR_EL2, access_rw, reset_val, 0), > { SYS_DESC(SYS_SP_EL1), access_sp_el1}, > > + /* AArch32 SPSR_* are RES0 if trapped from a NV guest */ > + { SYS_DESC(SYS_SPSR_irq), .access = trap_raz_wi, > + .visibility = hidden_user_visibility }, > + { SYS_DESC(SYS_SPSR_abt), .access = trap_raz_wi, > + .visibility = hidden_user_visibility }, > + { SYS_DESC(SYS_SPSR_und), .access = trap_raz_wi, > + .visibility = hidden_user_visibility }, > + { SYS_DESC(SYS_SPSR_fiq), .access = trap_raz_wi, > + .visibility = hidden_user_visibility }, > + I’m trying to understand this patch and its surroundings. Those SPSR_* registers UNDEF at EL0. I do not understand why use REG_HIDDEN_USER instead of REG_HIDDEN. Also, could you please explain what is happening at PSTATE.EL == EL1 and if EL2Enabled() && HCR_EL2.NV == ‘1’ ? For my education on this subject, I would be very grateful if you could help me understand this. Thank you Miguel > { SYS_DESC(SYS_IFSR32_EL2), trap_undef, reset_unknown, IFSR32_EL2 }, > EL2_REG(AFSR0_EL2, access_rw, reset_val, 0), > EL2_REG(AFSR1_EL2, access_rw, reset_val, 0), > -- > 2.39.2 >
On Mon, 23 Oct 2023 19:55:10 +0100, Miguel Luis <miguel.luis@oracle.com> wrote: > > Hi Marc, > > > On 23 Oct 2023, at 09:54, Marc Zyngier <maz@kernel.org> wrote: > > > > When trapping accesses from a NV guest that tries to access > > SPSR_{irq,abt,und,fiq}, make sure we handle them as RAZ/WI, > > as if AArch32 wasn't implemented. > > > > This involves a bit of repainting to make the visibility > > handler more generic. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/include/asm/sysreg.h | 4 ++++ > > arch/arm64/kvm/sys_regs.c | 16 +++++++++++++--- > > 2 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > > index 4a20a7dc5bc4..5e65f51c10d2 100644 > > --- a/arch/arm64/include/asm/sysreg.h > > +++ b/arch/arm64/include/asm/sysreg.h > > @@ -505,6 +505,10 @@ > > #define SYS_SPSR_EL2 sys_reg(3, 4, 4, 0, 0) > > #define SYS_ELR_EL2 sys_reg(3, 4, 4, 0, 1) > > #define SYS_SP_EL1 sys_reg(3, 4, 4, 1, 0) > > +#define SYS_SPSR_irq sys_reg(3, 4, 4, 3, 0) > > +#define SYS_SPSR_abt sys_reg(3, 4, 4, 3, 1) > > +#define SYS_SPSR_und sys_reg(3, 4, 4, 3, 2) > > +#define SYS_SPSR_fiq sys_reg(3, 4, 4, 3, 3) > > #define SYS_IFSR32_EL2 sys_reg(3, 4, 5, 0, 1) > > #define SYS_AFSR0_EL2 sys_reg(3, 4, 5, 1, 0) > > #define SYS_AFSR1_EL2 sys_reg(3, 4, 5, 1, 1) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index 0071ccccaf00..be1ebd2c5ba0 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -1791,8 +1791,8 @@ static unsigned int el2_visibility(const struct kvm_vcpu *vcpu, > > * HCR_EL2.E2H==1, and only in the sysreg table for convenience of > > * handling traps. Given that, they are always hidden from userspace. > > */ > > -static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu, > > - const struct sys_reg_desc *rd) > > +static unsigned int hidden_user_visibility(const struct kvm_vcpu *vcpu, > > + const struct sys_reg_desc *rd) > > { > > return REG_HIDDEN_USER; > > } > > @@ -1803,7 +1803,7 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu, > > .reset = rst, \ > > .reg = name##_EL1, \ > > .val = v, \ > > - .visibility = elx2_visibility, \ > > + .visibility = hidden_user_visibility, \ > > } > > > > /* > > @@ -2387,6 +2387,16 @@ static const struct sys_reg_desc sys_reg_descs[] = { > > EL2_REG(ELR_EL2, access_rw, reset_val, 0), > > { SYS_DESC(SYS_SP_EL1), access_sp_el1}, > > > > + /* AArch32 SPSR_* are RES0 if trapped from a NV guest */ > > + { SYS_DESC(SYS_SPSR_irq), .access = trap_raz_wi, > > + .visibility = hidden_user_visibility }, > > + { SYS_DESC(SYS_SPSR_abt), .access = trap_raz_wi, > > + .visibility = hidden_user_visibility }, > > + { SYS_DESC(SYS_SPSR_und), .access = trap_raz_wi, > > + .visibility = hidden_user_visibility }, > > + { SYS_DESC(SYS_SPSR_fiq), .access = trap_raz_wi, > > + .visibility = hidden_user_visibility }, > > + > > I’m trying to understand this patch and its surroundings. > > Those SPSR_* registers UNDEF at EL0. I do not understand > why use REG_HIDDEN_USER instead of REG_HIDDEN. USER here means host userspace, not guest EL0. That's because the various SPSR_* registers are already visible from userspace as KVM_REG_ARM_CORE_REG(spsr[KVM_SPSR_*]), and the above entries are solely for the purpose of handling a trap (and thus must not be exposed in the list of available sysregs). This is similar to what we are doing for the ELx2 registers, which are already exposed as EL0/EL1 registers. > Also, could you please explain what is happening at PSTATE.EL == EL1 > and if EL2Enabled() && HCR_EL2.NV == ‘1’ ? We directly take the trap and not forward it. This isn't exactly the letter of the architecture, but at the same time, treating these registers as RAZ/WI is the only valid implementation. I don't immediately see a problem with taking this shortcut. M.
On Tue, Oct 24, 2023 at 06:25:33PM +0100, Marc Zyngier wrote: > On Mon, 23 Oct 2023 19:55:10 +0100, Miguel Luis <miguel.luis@oracle.com> wrote: > > Also, could you please explain what is happening at PSTATE.EL == EL1 > > and if EL2Enabled() && HCR_EL2.NV == ‘1’ ? > > We directly take the trap and not forward it. This isn't exactly the > letter of the architecture, but at the same time, treating these > registers as RAZ/WI is the only valid implementation. I don't > immediately see a problem with taking this shortcut. Ugh, that's annoying. The other EL2 views of AArch32 state UNDEF if EL1 doesn't implement AArch32. It'd be nice to get a relaxation in the architecture to allow an UNDEF here. Broadening the scope to KVM's emulation of AArch64-only behavior, I think we should be a bit more aggressive in sanitising AArch32 support from the ID registers. That way any AA64-only behavior in KVM is architectural from the guest POV. Maybe something like: diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 57c8190d5438..045f41900433 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1447,6 +1447,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, return REG_HIDDEN; } +#define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit) \ +({ \ + u64 __f_val = FIELD_GET(reg##_##field##_MASK, val); \ + (val) &= ~reg##_##field##_MASK; \ + (val) |= FIELD_PREP(reg##_##field##_MASK, \ + min(__f_val, (u64)reg##_##field##_##limit)); \ + (val); \ +}) + + static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) { @@ -1477,19 +1487,38 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, val |= SYS_FIELD_PREP_ENUM(ID_AA64PFR0_EL1, GIC, IMP); } + /* + * Hide AArch32 support if the vCPU wasn't configured for it. The + * architecture requires all higher ELs to be AArch64-only in this + * situation as well. + */ + if (!vcpu_el1_is_32bit(vcpu)) { + val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL1, IMP); + val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL2, IMP); + val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL3, IMP); + } + val &= ~ID_AA64PFR0_EL1_AMU_MASK; return val; } -#define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit) \ -({ \ - u64 __f_val = FIELD_GET(reg##_##field##_MASK, val); \ - (val) &= ~reg##_##field##_MASK; \ - (val) |= FIELD_PREP(reg##_##field##_MASK, \ - min(__f_val, (u64)reg##_##field##_##limit)); \ - (val); \ -}) +static u64 set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, + const struct sys_reg_desc *rd, + u64 val) +{ + /* + * Older versions of KVM freely reported AArch32 support, even if the + * vCPU was configured for AArch64. + */ + if (!vcpu_el1_is_32bit(vcpu)) { + val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL1, IMP); + val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL2, IMP); + val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64PFR0_EL1, EL3, IMP); + } + + return set_id_reg(vcpu, rd, val); +} static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) @@ -2055,7 +2084,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { { SYS_DESC(SYS_ID_AA64PFR0_EL1), .access = access_id_reg, .get_user = get_id_reg, - .set_user = set_id_reg, + .set_user = set_id_aa64pfr0_el1, .reset = read_sanitised_id_aa64pfr0_el1, .val = ~(ID_AA64PFR0_EL1_AMU | ID_AA64PFR0_EL1_MPAM |
On Tue, Oct 24, 2023 at 10:41:57PM +0000, Oliver Upton wrote: > On Tue, Oct 24, 2023 at 06:25:33PM +0100, Marc Zyngier wrote: > > On Mon, 23 Oct 2023 19:55:10 +0100, Miguel Luis <miguel.luis@oracle.com> wrote: > > > Also, could you please explain what is happening at PSTATE.EL == EL1 > > > and if EL2Enabled() && HCR_EL2.NV == ‘1’ ? > > > > We directly take the trap and not forward it. This isn't exactly the > > letter of the architecture, but at the same time, treating these > > registers as RAZ/WI is the only valid implementation. I don't > > immediately see a problem with taking this shortcut. > > Ugh, that's annoying. The other EL2 views of AArch32 state UNDEF if EL1 > doesn't implement AArch32. It'd be nice to get a relaxation in the > architecture to allow an UNDEF here. Correction (I wasn't thinking): RES0 behavior should be invariant, much like the UNDEF behavior of the other AA32-specific registers.
On Wed, 25 Oct 2023 00:04:27 +0100, Oliver Upton <oliver.upton@linux.dev> wrote: > > On Tue, Oct 24, 2023 at 10:41:57PM +0000, Oliver Upton wrote: > > On Tue, Oct 24, 2023 at 06:25:33PM +0100, Marc Zyngier wrote: > > > On Mon, 23 Oct 2023 19:55:10 +0100, Miguel Luis <miguel.luis@oracle.com> wrote: > > > > Also, could you please explain what is happening at PSTATE.EL == EL1 > > > > and if EL2Enabled() && HCR_EL2.NV == ‘1’ ? > > > > > > We directly take the trap and not forward it. This isn't exactly the > > > letter of the architecture, but at the same time, treating these > > > registers as RAZ/WI is the only valid implementation. I don't > > > immediately see a problem with taking this shortcut. > > > > Ugh, that's annoying. The other EL2 views of AArch32 state UNDEF if EL1 > > doesn't implement AArch32. It'd be nice to get a relaxation in the > > architecture to allow an UNDEF here. > > Correction (I wasn't thinking): RES0 behavior should be invariant, much > like the UNDEF behavior of the other AA32-specific registers. I'm not sure what you're asking for exactly here, so let me explain my understanding of the architecture on this point, which is that the *32_EL2 registers are different in nature from the SPSR_* registers. IFAR32_EL2 and co are accessors for the equivalent AArch32 registers. If AArch32 isn't implemented, then these registers should UNDEF, because there is nothing to access at all. The status of SPSR_* is more subtle: the AArch32 exception model is banked (irq, fiq, abt, und), and for each bank we have a triplet (LR_*, SP_*, SPSR_*), plus the extra R[8-12]_fiq. On taking an exception from AArch32 EL1 to AArch64 EL2, all the (LR_*, SP_*, R*_fiq) are stored as part of the AArch64 GPRs (X16-X30, see I_PYKVS). And what about SPSR_*? Well, they live as extra registers that are populated on exception entry. But they are similar in nature to the GPRs that store the rest of the stuff. When AArch32 isn't implemented, the natural choice is to keep them around, only as RES0, because they are just GPRs. Of course, all of this is an architectural choice. If I had to change anything, I'd rather have everything to UNDEF. But there is some logic in the madness. And frankly, we will never see an AArch32-capable, NV-capable HW implementation ever, so this is all fairly academic. Cheers, M.
On Wed, Oct 25, 2023 at 09:28:03AM +0100, Marc Zyngier wrote: > On Wed, 25 Oct 2023 00:04:27 +0100, Oliver Upton <oliver.upton@linux.dev> wrote: > > Correction (I wasn't thinking): RES0 behavior should be invariant, much > > like the UNDEF behavior of the other AA32-specific registers. > > I'm not sure what you're asking for exactly here, so let me explain my > understanding of the architecture on this point, which is that the > *32_EL2 registers are different in nature from the SPSR_* registers. Damn, I still didn't manage to get my point across! > IFAR32_EL2 and co are accessors for the equivalent AArch32 registers. > If AArch32 isn't implemented, then these registers should UNDEF, > because there is nothing to access at all. > > The status of SPSR_* is more subtle: the AArch32 exception model is > banked (irq, fiq, abt, und), and for each bank we have a triplet > (LR_*, SP_*, SPSR_*), plus the extra R[8-12]_fiq. On taking an > exception from AArch32 EL1 to AArch64 EL2, all the (LR_*, SP_*, > R*_fiq) are stored as part of the AArch64 GPRs (X16-X30, see I_PYKVS). Thanks. Yeah, I've got a pretty good handle on what's going on here. What I really was trying to compare is the way these aliases into AA32 state are handled, and the annoying difference between the two sets. IFAR32 and friends UNDEF unconditionally w/o AArch32, which I quite like. To your point, the SPSR_* accessors still trap even if AArch32 is not implemented. I was suggesting in passing that it'd be nice if the architecture alternatively allowed for these to read as RES0 (no trap) if NV==1 and AArch32 is not implemented, which aligns with your change. But after all... > we will never see an AArch32-capable, NV-capable HW implementation > ever, so this is all fairly academic. None of this matters in the first place :)
On Wed, 25 Oct 2023 09:46:13 +0100, Oliver Upton <oliver.upton@linux.dev> wrote: > > To your point, the SPSR_* accessors still trap even if AArch32 is not > implemented. I was suggesting in passing that it'd be nice if the > architecture alternatively allowed for these to read as RES0 (no trap) > if NV==1 and AArch32 is not implemented, which aligns with your change. Ah, I see what you mean now. Yeah, I'll put that on the bucket list of things I need to write up. It has quite a few entries already, and I really should get to it. Thanks, M.
Hi Marc, Oliver, > On 24 Oct 2023, at 17:25, Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 23 Oct 2023 19:55:10 +0100, > Miguel Luis <miguel.luis@oracle.com> wrote: >> >> Hi Marc, >> >>> On 23 Oct 2023, at 09:54, Marc Zyngier <maz@kernel.org> wrote: >>> >>> When trapping accesses from a NV guest that tries to access >>> SPSR_{irq,abt,und,fiq}, make sure we handle them as RAZ/WI, >>> as if AArch32 wasn't implemented. >>> >>> This involves a bit of repainting to make the visibility >>> handler more generic. >>> >>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>> --- >>> arch/arm64/include/asm/sysreg.h | 4 ++++ >>> arch/arm64/kvm/sys_regs.c | 16 +++++++++++++--- >>> 2 files changed, 17 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h >>> index 4a20a7dc5bc4..5e65f51c10d2 100644 >>> --- a/arch/arm64/include/asm/sysreg.h >>> +++ b/arch/arm64/include/asm/sysreg.h >>> @@ -505,6 +505,10 @@ >>> #define SYS_SPSR_EL2 sys_reg(3, 4, 4, 0, 0) >>> #define SYS_ELR_EL2 sys_reg(3, 4, 4, 0, 1) >>> #define SYS_SP_EL1 sys_reg(3, 4, 4, 1, 0) >>> +#define SYS_SPSR_irq sys_reg(3, 4, 4, 3, 0) >>> +#define SYS_SPSR_abt sys_reg(3, 4, 4, 3, 1) >>> +#define SYS_SPSR_und sys_reg(3, 4, 4, 3, 2) >>> +#define SYS_SPSR_fiq sys_reg(3, 4, 4, 3, 3) >>> #define SYS_IFSR32_EL2 sys_reg(3, 4, 5, 0, 1) >>> #define SYS_AFSR0_EL2 sys_reg(3, 4, 5, 1, 0) >>> #define SYS_AFSR1_EL2 sys_reg(3, 4, 5, 1, 1) >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>> index 0071ccccaf00..be1ebd2c5ba0 100644 >>> --- a/arch/arm64/kvm/sys_regs.c >>> +++ b/arch/arm64/kvm/sys_regs.c >>> @@ -1791,8 +1791,8 @@ static unsigned int el2_visibility(const struct kvm_vcpu *vcpu, >>> * HCR_EL2.E2H==1, and only in the sysreg table for convenience of >>> * handling traps. Given that, they are always hidden from userspace. >>> */ >>> -static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu, >>> - const struct sys_reg_desc *rd) >>> +static unsigned int hidden_user_visibility(const struct kvm_vcpu *vcpu, >>> + const struct sys_reg_desc *rd) >>> { >>> return REG_HIDDEN_USER; >>> } >>> @@ -1803,7 +1803,7 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu, >>> .reset = rst, \ >>> .reg = name##_EL1, \ >>> .val = v, \ >>> - .visibility = elx2_visibility, \ >>> + .visibility = hidden_user_visibility, \ >>> } >>> >>> /* >>> @@ -2387,6 +2387,16 @@ static const struct sys_reg_desc sys_reg_descs[] = { >>> EL2_REG(ELR_EL2, access_rw, reset_val, 0), >>> { SYS_DESC(SYS_SP_EL1), access_sp_el1}, >>> >>> + /* AArch32 SPSR_* are RES0 if trapped from a NV guest */ >>> + { SYS_DESC(SYS_SPSR_irq), .access = trap_raz_wi, >>> + .visibility = hidden_user_visibility }, >>> + { SYS_DESC(SYS_SPSR_abt), .access = trap_raz_wi, >>> + .visibility = hidden_user_visibility }, >>> + { SYS_DESC(SYS_SPSR_und), .access = trap_raz_wi, >>> + .visibility = hidden_user_visibility }, >>> + { SYS_DESC(SYS_SPSR_fiq), .access = trap_raz_wi, >>> + .visibility = hidden_user_visibility }, >>> + >> >> I’m trying to understand this patch and its surroundings. >> >> Those SPSR_* registers UNDEF at EL0. I do not understand >> why use REG_HIDDEN_USER instead of REG_HIDDEN. > > USER here means host userspace, not guest EL0. That's because the > various SPSR_* registers are already visible from userspace as > KVM_REG_ARM_CORE_REG(spsr[KVM_SPSR_*]), and the above entries are > solely for the purpose of handling a trap (and thus must not be > exposed in the list of available sysregs). > > This is similar to what we are doing for the ELx2 registers, which are > already exposed as EL0/EL1 registers. > >> Also, could you please explain what is happening at PSTATE.EL == EL1 >> and if EL2Enabled() && HCR_EL2.NV == ‘1’ ? > > We directly take the trap and not forward it. This isn't exactly the > letter of the architecture, but at the same time, treating these > registers as RAZ/WI is the only valid implementation. I don't > immediately see a problem with taking this shortcut. > Thank you for explaining and expanding on this topic. I will take some time to absorb all your comments. Thank you Miguel > M. > > -- > Without deviation from the norm, progress is not possible.
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 4a20a7dc5bc4..5e65f51c10d2 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -505,6 +505,10 @@ #define SYS_SPSR_EL2 sys_reg(3, 4, 4, 0, 0) #define SYS_ELR_EL2 sys_reg(3, 4, 4, 0, 1) #define SYS_SP_EL1 sys_reg(3, 4, 4, 1, 0) +#define SYS_SPSR_irq sys_reg(3, 4, 4, 3, 0) +#define SYS_SPSR_abt sys_reg(3, 4, 4, 3, 1) +#define SYS_SPSR_und sys_reg(3, 4, 4, 3, 2) +#define SYS_SPSR_fiq sys_reg(3, 4, 4, 3, 3) #define SYS_IFSR32_EL2 sys_reg(3, 4, 5, 0, 1) #define SYS_AFSR0_EL2 sys_reg(3, 4, 5, 1, 0) #define SYS_AFSR1_EL2 sys_reg(3, 4, 5, 1, 1) diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 0071ccccaf00..be1ebd2c5ba0 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1791,8 +1791,8 @@ static unsigned int el2_visibility(const struct kvm_vcpu *vcpu, * HCR_EL2.E2H==1, and only in the sysreg table for convenience of * handling traps. Given that, they are always hidden from userspace. */ -static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu, - const struct sys_reg_desc *rd) +static unsigned int hidden_user_visibility(const struct kvm_vcpu *vcpu, + const struct sys_reg_desc *rd) { return REG_HIDDEN_USER; } @@ -1803,7 +1803,7 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu, .reset = rst, \ .reg = name##_EL1, \ .val = v, \ - .visibility = elx2_visibility, \ + .visibility = hidden_user_visibility, \ } /* @@ -2387,6 +2387,16 @@ static const struct sys_reg_desc sys_reg_descs[] = { EL2_REG(ELR_EL2, access_rw, reset_val, 0), { SYS_DESC(SYS_SP_EL1), access_sp_el1}, + /* AArch32 SPSR_* are RES0 if trapped from a NV guest */ + { SYS_DESC(SYS_SPSR_irq), .access = trap_raz_wi, + .visibility = hidden_user_visibility }, + { SYS_DESC(SYS_SPSR_abt), .access = trap_raz_wi, + .visibility = hidden_user_visibility }, + { SYS_DESC(SYS_SPSR_und), .access = trap_raz_wi, + .visibility = hidden_user_visibility }, + { SYS_DESC(SYS_SPSR_fiq), .access = trap_raz_wi, + .visibility = hidden_user_visibility }, + { SYS_DESC(SYS_IFSR32_EL2), trap_undef, reset_unknown, IFSR32_EL2 }, EL2_REG(AFSR0_EL2, access_rw, reset_val, 0), EL2_REG(AFSR1_EL2, access_rw, reset_val, 0),
When trapping accesses from a NV guest that tries to access SPSR_{irq,abt,und,fiq}, make sure we handle them as RAZ/WI, as if AArch32 wasn't implemented. This involves a bit of repainting to make the visibility handler more generic. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/sysreg.h | 4 ++++ arch/arm64/kvm/sys_regs.c | 16 +++++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-)