On Fri, Dec 27, 2019 at 03:42:34PM +0000, Alexandru Elisei wrote:
> Hi,
>
> On 12/20/19 3:05 PM, Mark Rutland wrote:
> > When KVM injects an exception into a guest, it generates the CPSR value
> > from scratch, configuring CPSR.{M,A,I,T,E}, and setting all other
> > bits to zero.
> >
> > This isn't correct, as the architecture specifies that some CPSR 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
>
> This patch applies equally well to 32bit KVM, where we have a SPSR register.
Indeed. The above wasn't intended to imply otherwise, but I'll add the
following to make that clear:
| Note that this code is used by both arm and arm64, and is intended to
| fuction with the SPSR_EL2 and SPSR_hyp layouts.
[...]
> > /* arm64 compatibility macros */
> > +#define PSR_AA32_MODE_FIQ FIQ_MODE
> > +#define PSR_AA32_MODE_SVC SVC_MODE
> > #define PSR_AA32_MODE_ABT ABT_MODE
> > #define PSR_AA32_MODE_UND UND_MODE
> > #define PSR_AA32_T_BIT PSR_T_BIT
> > +#define PSR_AA32_F_BIT PSR_F_BIT
> > #define PSR_AA32_I_BIT PSR_I_BIT
> > #define PSR_AA32_A_BIT PSR_A_BIT
> > #define PSR_AA32_E_BIT PSR_E_BIT
> > #define PSR_AA32_IT_MASK PSR_IT_MASK
> > +#define PSR_AA32_GE_MASK 0x000f0000
> > +#define PSR_AA32_PAN_BIT 0x00400000
> > +#define PSR_AA32_SSBS_BIT 0x00800000
> > +#define PSR_AA32_DIT_BIT 0x01000000
>
> This is actually PSR_J_BIT on arm. Maybe it's worth defining it like that to
> make the overlap obvious.
Another bug! These are intended to match the SPSR_hyp view, where DIT is
bit 21, and so this should be:
#define PSR_AA32_DIT_BIT 0x0x00200000
... placed immediately before the PAN bit.
We don't need a macro for the J bit as it was obsoleted and made RES0 by
the ARMv7 virtualization extensions.
[...]
> > + // CPSR.PAN is unchanged unless overridden by SCTLR.SPAN
> > + // See ARM DDI 0487E.a, page G8-6246
> > + new |= (old & PSR_AA32_PAN_BIT);
> > + if (sctlr & BIT(23))
> > + new |= PSR_AA32_PAN_BIT;
>
> PSTATE.PAN is unconditionally set when SCTLR.SPAN is clear.
Indeed, and this time the reference is explicit about that. :/
I've updated this to:
| // CPSR.PAN is unchanged unless SCTLR.SPAN == 0b0
| // SCTLR.SPAN is RES1 when ARMv8.1-PAN is not implemented
| // See ARM DDI 0487E.a, page G8-6246
| new |= (old & PSR_AA32_PAN_BIT);
| if (!(sctlr & BIT(23)))
| new |= PSR_AA32_PAN_BIT;
[...]
> I've also checked that the ARM documentation references match the code.
Thanks, your review is much appreciated!
Mark.
