[PATCH 5/5] KVM: arm64: Handle AArch32 SPSR_{irq,abt,und,fiq} as RAZ/WI

[Miguel Luis]

Hi Marc, Oliver,

> On 24 Oct 2023, at 17:25, Marc Zyngier <maz at kernel.org> wrote:
> 
> On Mon, 23 Oct 2023 19:55:10 +0100,
> Miguel Luis <miguel.luis at oracle.com> wrote:
>> 
>> Hi Marc,
>> 
>>> On 23 Oct 2023, at 09:54, Marc Zyngier <maz at 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 at 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.