Hi,
On 1/8/20 11:12 AM, Mark Rutland wrote:
> 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?
It's fine as it is, no need to change it.
>
>>> + */
>>> +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;
Looks good.
>
> [...]
>
>> 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).
Makes sense.
Thanks,
Alex
>
> Thanks,
> Mark
