On Fri, 27 Dec 2019 13:01:57 +0000,
Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
>
> Hi,
>
> On 12/20/19 3:05 PM, Mark Rutland wrote:
> > When KVM injects an exception into a guest, it generates the PSTATE
> > value from scratch, configuring PSTATE.{M[4:0],DAIF}, and setting all
> > other bits to zero.
> >
> > This isn't correct, as the architecture specifies that some PSTATE bits
> > are (conditionally) cleared or set upon an exception, and others are
> > unchanged from the original context.
> >
> > This patch adds logic to match the architectural behaviour. To make this
> > simple to follow/audit/extend, documentation references are provided,
> > and bits are configured in order of their layout in SPSR_EL2. This
> > layout can be seen in the diagram on ARM DDI 0487E.a page C5-429.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> > Cc: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> > Cc: Drew Jones <drjones@xxxxxxxxxx>
> > Cc: James Morse <james.morse@xxxxxxx>
> > Cc: Julien Thierry <julien.thierry.kdev@xxxxxxxxx>
> > Cc: Marc Zyngier <maz@xxxxxxxxxx>
> > Cc: Peter Maydell <peter.maydell@xxxxxxxxxx>
> > Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > arch/arm64/include/uapi/asm/ptrace.h | 1 +
> > arch/arm64/kvm/inject_fault.c | 69 +++++++++++++++++++++++++++++++++---
> > 2 files changed, 65 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h
> > index 7ed9294e2004..d1bb5b69f1ce 100644
> > --- a/arch/arm64/include/uapi/asm/ptrace.h
> > +++ b/arch/arm64/include/uapi/asm/ptrace.h
> > @@ -49,6 +49,7 @@
> > #define PSR_SSBS_BIT 0x00001000
> > #define PSR_PAN_BIT 0x00400000
> > #define PSR_UAO_BIT 0x00800000
> > +#define PSR_DIT_BIT 0x01000000
> > #define PSR_V_BIT 0x10000000
> > #define PSR_C_BIT 0x20000000
> > #define PSR_Z_BIT 0x40000000
> > diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
> > index a9d25a305af5..270d91c05246 100644
> > --- a/arch/arm64/kvm/inject_fault.c
> > +++ b/arch/arm64/kvm/inject_fault.c
> > @@ -14,9 +14,6 @@
> > #include <asm/kvm_emulate.h>
> > #include <asm/esr.h>
> >
> > -#define PSTATE_FAULT_BITS_64 (PSR_MODE_EL1h | PSR_A_BIT | PSR_F_BIT | \
> > - PSR_I_BIT | PSR_D_BIT)
> > -
> > #define CURRENT_EL_SP_EL0_VECTOR 0x0
> > #define CURRENT_EL_SP_ELx_VECTOR 0x200
> > #define LOWER_EL_AArch64_VECTOR 0x400
> > @@ -50,6 +47,68 @@ static u64 get_except_vector(struct kvm_vcpu *vcpu, enum exception_type type)
> > return vcpu_read_sys_reg(vcpu, VBAR_EL1) + exc_offset + type;
> > }
> >
> > +/*
> > + * When an exception is taken, most PSTATE fields are left unchanged in the
> > + * handler. However, some are explicitly overridden (e.g. M[4:0]). Luckily all
> > + * of the inherited bits have the same position in the AArch64/AArch32 SPSR_ELx
> > + * layouts, so we don't need to shuffle these for exceptions from AArch32 EL0.
> > + *
> > + * For the SPSR_ELx layout for AArch64, see ARM DDI 0487E.a page C5-429.
> > + * For the SPSR_ELx layout for AArch32, see ARM DDI 0487E.a page C5-426.
>
> The commit message mentions only the SPSR_ELx layout for AArch64.
>
> > + *
> > + * Here we manipulate the fields in order of the AArch64 SPSR_ELx layout, from
> > + * MSB to LSB.
> > + */
> > +static unsigned long get_except64_pstate(struct kvm_vcpu *vcpu)
> > +{
> > + unsigned long sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL1);
> > + unsigned long old, new;
> > +
> > + old = *vcpu_cpsr(vcpu);
> > + new = 0;
> > +
> > + new |= (old & PSR_N_BIT);
> > + new |= (old & PSR_Z_BIT);
> > + new |= (old & PSR_C_BIT);
> > + new |= (old & PSR_V_BIT);
> > +
> > + // TODO: TCO (if/when ARMv8.5-MemTag is exposed to guests)
> > +
> > + new |= (old & PSR_DIT_BIT);
> > +
> > + // PSTATE.UAO is set to zero upon any exception to AArch64
> > + // See ARM DDI 0487E.a, page D5-2579.
> > +
> > + // PSTATE.PAN is unchanged unless overridden by SCTLR_ELx.SPAN
> > + // See ARM DDI 0487E.a, page D5-2578.
> > + new |= (old & PSR_PAN_BIT);
> > + if (sctlr & SCTLR_EL1_SPAN)
> > + new |= PSR_PAN_BIT;
>
> On page D13-3264, it is stated that the PAN bit is set unconditionally if
> SCTLR_EL1.SPAN is clear, not set.
Indeed. Given that when ARMv8.1-PAN is not implemented, SCTLR_EL1[23]
is RES1, it seems surprising to force PAN based on this bit being set.
I've now dropped this series from my tree until Mark has a chance to
clarify this.
Thanks,
M.
--
Jazz is not dead, it just smells funny.
