Re: [PATCH] arm64: Add KRYO4XX gold CPU core to spectre-v2 safe list

[Marc Zyngier <maz@kernel.org>]

On 2020-08-13 10:40, Will Deacon wrote:
> On Thu, Aug 13, 2020 at 02:49:37PM +0530, Sai Prakash Ranjan wrote:
>> On 2020-08-13 14:33, Will Deacon wrote:
>> > On Thu, Aug 13, 2020 at 01:48:34PM +0530, Sai Prakash Ranjan wrote:
>> > > KRYO4XX gold/big CPU cores are based on Cortex-A76 which has CSV2
>> > > bits set and are spectre-v2 safe. But on big.LITTLE systems where
>> > > they are coupled with other CPU cores such as the KRYO4XX silver
>> > > based on Cortex-A55 which are spectre-v2 safe but do not have CSV2
>> > > bits set, the system wide safe value will be set to the lowest value
>> > > of CSV2 bits as per FTR_LOWER_SAFE defined for CSV2 bits of register
>> > > ID_AA64PFR0_EL1.
>> > >
>> > > This is a problem when booting a guest kernel on gold CPU cores
>> > > where it will incorrectly report ARM_SMCCC_ARCH_WORKAROUND_1 warning
>> > > and consider them as vulnerable for Spectre variant 2 due to system
>> > > wide safe value which is used in kvm emulation code when reading id
>> > > registers. One wrong way of fixing this is to set the FTR_HIGHER_SAFE
>> > > for CSV2 bits, so instead add the KRYO4XX gold CPU core to the safe
>> > > list which will be consulted even when the sanitised read reports
>> > > that CSV2 bits are not set for KRYO4XX gold cores.
>> > >
>> > > Reported-by: Stephen Boyd <swboyd@chromium.org>
>> > > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> > > ---
>> > >  arch/arm64/kernel/cpu_errata.c | 1 +
>> > >  1 file changed, 1 insertion(+)
>> > >
>> > > diff --git a/arch/arm64/kernel/cpu_errata.c
>> > > b/arch/arm64/kernel/cpu_errata.c
>> > > index 6bd1d3ad037a..6cbdd2d98a2a 100644
>> > > --- a/arch/arm64/kernel/cpu_errata.c
>> > > +++ b/arch/arm64/kernel/cpu_errata.c
>> > > @@ -545,6 +545,7 @@ static const struct midr_range
>> > > spectre_v2_safe_list[] = {
>> > >  	MIDR_ALL_VERSIONS(MIDR_HISI_TSV110),
>> > >  	MIDR_ALL_VERSIONS(MIDR_QCOM_KRYO_3XX_SILVER),
>> > >  	MIDR_ALL_VERSIONS(MIDR_QCOM_KRYO_4XX_SILVER),
>> > > +	MIDR_ALL_VERSIONS(MIDR_QCOM_KRYO_4XX_GOLD),
>> >
>> > We shouldn't be putting CPUs in the safe list when they have CSV2
>> > reporting
>> > that they are mitigated in hardware, so I don't think this is the right
>> > approach.
>> >
>> 
>> Ok but the only thing I find wrong in this approach is that it is a
>> redundant
>> information because CSV2 is already advertising the mitigation, but 
>> again
>> CSV2 check is done first so it doesn't really hurt to add it to the 
>> safe
>> list because we already know that it is safe.
> 
> It simply doesn't scale. That's why CSV2 exists in the first place, so 
> we
> don't have to modify the kernel everytime a new CPU is invented.
> 
>> > Sounds more like KVM should advertise CSV2 for the vCPUs if all of the
>> > physical CPUs without CSV2 set are on the safe list. But then again, KVM
>> > has always been slightly in denial about big.LITTLE because you can't
>> > sensibly expose it to a guest if there are detectable differences...
>> >
>> 
>> Sorry but I don't see how the guest kernel will see the CSV2 bits set 
>> for
>> gold CPU cores without actually adding them to the safe list or 
>> reading the
>> not sanitised value of ID_AA64PFR0_EL1 ?
> 
> Well that's for somebody to figure out in the patch. I'm just saying 
> that
> adding cores to the safe list when they already have a CSV2 field 
> conveying
> the same information is the wrong approach. The right appproach is for 
> KVM
> to expose CSV2 as set when the system is not affected by the erratum.

A sensible way to fix this would be with something like that:

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 077293b5115f..2735db21ff0d 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1131,6 +1131,9 @@ static u64 read_id_reg(const struct kvm_vcpu 
*vcpu,
  		if (!vcpu_has_sve(vcpu))
  			val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
  		val &= ~(0xfUL << ID_AA64PFR0_AMU_SHIFT);
+		if (!(val & (0xfUL << ID_AA64PFR0_CSV2_SHIFT)) &&
+		    get_spectre_v2_workaround_state() == 
ARM64_BP_HARDEN_NOT_REQUIRED)
+			val |= (1UL << ID_AA64PFR0_CSV2_SHIFT);
  	} else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
  		val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
  			 (0xfUL << ID_AA64ISAR1_API_SHIFT) |


         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel