Hi Alex,
On Fri, Dec 27, 2019 at 01:01:57PM +0000, Alexandru Elisei wrote:
> 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.
> > +/*
> > + * 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.
That was intentional; there I was only providing rationale for how to
review the patch...
> > + * Here we manipulate the fields in order of the AArch64 SPSR_ELx layout, from
> > + * MSB to LSB.
... as also commented here.
I can drop the reference from the commit message, if that's confusing?
> > + */
> > +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.
very good spot, and that's a much better reference.
I had mistakenly assumed SPAN took effect when 0b1, since it wasn't
called nSPAN, and page D5-2578 doesn't mention the polarity of the bit:
| When ARMv8.1-PAN is implemented, the SCTLR_EL1.SPAN and SCTLR_EL2.SPAN
| bits are used to control whether the PAN bit is set on an exception to
| EL1 or EL2.
I've updated this to be:
| // PSTATE.PAN is unchanged unless SCTLR_ELx.SPAN == 0b0
| // SCTLR_ELx.SPAN is RES1 when ARMv8.1-PAN is not implemented
| // See ARM DDI 0487E.a, page D13-3264.
| new |= (old & PSR_PAN_BIT);
| if (!(sctlr & SCTLR_EL1_SPAN))
| new |= PSR_PAN_BIT;
[...]
> I've also checked the ARM ARM pages mentioned in the comments, and the
> references are correct. The SPSR_EL2 layouts for exceptions taken from AArch64,
> respectively AArch32, states are compatible with the way we create the SPSR_EL2
> that will be used for eret'ing to the guest, just like the comment says.
Thanks for confirming this!
> I have a suggestion. I think that in ARM ARM, shuffling things between sections
> happens a lot less often than adding/removing things from one particular
> section, so the pages referenced are more likely to change in later versions.
> How about referencing the section instead of the exact page? Something like:
> "This layout can be seen in the diagram on ARM DDI 0487E.a, section C5.2.18,
> when an exception is taken from AArch64 state"?
I did something like that initially, but the comments got very verbose,
and so I moved to doc + page/section numbers alone.
The section numbers and headings also vary between revisions of the ARM
ARM, so I'd prefer to leave this as-is for now. I think it's always
going to be necessary to look at the referenced version of the ARM ARM
(in addition to a subsequent revision when updating things).
Thanks,
Mark
