Hi Marc, On 25/01/2022 18:36, Marc Zyngier wrote: > On Tue, 25 Jan 2022 18:19:45 +0000, > James Morse <james.morse@xxxxxxx> wrote: >> On 25/01/2022 16:51, Marc Zyngier wrote: >>> On Tue, 25 Jan 2022 15:38:03 +0000, >>> James Morse <james.morse@xxxxxxx> 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 _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
