[PATCH 4/4] KVM: arm64: Workaround Cortex-A510's single-step and PAC trap errata

[James Morse]

Hi Marc,

On 25/01/2022 18:36, Marc Zyngier wrote:
> On Tue, 25 Jan 2022 18:19:45 +0000,
> James Morse <james.morse at arm.com> wrote:
>> On 25/01/2022 16:51, Marc Zyngier wrote:
>>> On Tue, 25 Jan 2022 15:38:03 +0000,
>>> James Morse <james.morse at arm.com> wrote:
>>>>
>>>> Cortex-A510's erratum #2077057 causes SPSR_EL2 to be corrupted when
>>>> single-stepping authenticated ERET instructions. A single step is
>>>> expected, but a pointer authentication trap is taken instead. The
>>>> erratum causes SPSR_EL1 to be copied to SPSR_EL2, which could allow
>>>> EL1 to cause a return to EL2 with a guest controlled ELR_EL2.
>>>>
>>>> Because the conditions require an ERET into active-not-pending state,
>>>> this is only a problem for the EL2 when EL2 is stepping EL1. In this case
>>>> the previous SPSR_EL2 value is preserved in struct kvm_vcpu, and can be
>>>> restored.

>>>> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
>>>> index 331dd10821df..93bf140cc972 100644
>>>> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
>>>> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
>>>> @@ -440,6 +442,22 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
>>>>  			write_sysreg_el2(read_sysreg_el2(SYS_ELR) - 4, SYS_ELR);
>>>>  	}
>>>>  
>>>> +	/*
>>>> +	 * Check for the conditions of Cortex-A510's #2077057. When these occur
>>>> +	 * SPSR_EL2 can't be trusted, but isn't needed either as it is
>>>> +	 * unchanged from the value in vcpu_gp_regs(vcpu)->pstate.
>>>> +	 * Did we just take a PAC exception when a step exception was expected?
>>>> +	 */
>>>> +	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_2077057) &&
>>>
>>> nit: we can drop this IS_ENABLED()...
>>
>> Hmmm, I thought dead code elimination was a good thing!
>> Without the cpu_errata.c match, (which is also guarded by #ifdef),
>> the cap will never be true, even if an affected CPU showed up. This
>> way the compiler knows it can remove all this.

> I don't dispute that. However, experience shows that the more of these
> we sprinkle around, the quicker this code bitrots as we don't build
> for all the possible configurations. In general, we hardly have any
> dependency on configuration symbols, and rely on static keys got
> things not be terribly sucky.

I'll remove it - but I'm not convinced:

If this were in an #ifdef block, I agree that would prevent the compiler from even seeing
this code, and it would certainly bitrot if that Kconfig isn't selected, and would not be
build-tested properly in all the bizarre cases that only CI systems try.

But the IS_ENABLED() stuff lets the compiler can see inside that block, and means the
compiler can check the types etc, before eventually removing it as the condition becomes
"if(false)".

For example:
|	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_2077057))
|		synchronize_vcpu_pstate(42);

This fails to compile regardless of the value of the Kconfig symbol:
| In file included from arch/arm64/kvm/hyp/vhe/switch.c:7:
| ./arch/arm64/kvm/hyp/include/hyp/switch.h: In function ‘synchronize_vcpu_pstate’:
| ./arch/arm64/kvm/hyp/include/hyp/switch.h:415:27: warning: passing argument 1 of
| ‘synchronize_vcpu_pstate’ makes pointer from integer without a cast [-Wint-conversion]
|   415 |   synchronize_vcpu_pstate(42);
|
| ./arch/arm64/kvm/hyp/include/hyp/switch.h:405:61: note: expected ‘struct kvm_vcpu *’ but
| argument is of type ‘int’
|  405 | static inline void synchronize_vcpu_pstate(struct kvm_vcpu *vcpu, u64 *exit_code)

etc.

| cat .config | grep 2077057
| # CONFIG_ARM64_ERRATUM_2077057 is not set


I guess you're worried about link errors.

[..]

>>> Other than that, I'm happy to take the series as a whole ASAP, if only
>>> for the two pretty embarrassing bug fixes. If you can respin it
>>> shortly and address the comments above, I'd like it into -rc2.
>>
>> Will do. Shout if you strongly care about the IS_ENABLED().
> 
> I'd really like it gone. Otherwise, I'll never compile that code.


Thanks,

James