Re: [PATCH 1/3] KVM: arm64: correct PSTATE on exception entry

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux