On Mon, 23 Oct 2023 19:55:10 +0100,
Miguel Luis <miguel.luis@xxxxxxxxxx> wrote:
>
> Hi Marc,
>
> > On 23 Oct 2023, at 09:54, Marc Zyngier <maz@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
--
Without deviation from the norm, progress is not possible.
