On Sat, 20 May 2023 00:04:53 +0100, "Jitindar Singh, Suraj" <surajjs@xxxxxxxxxx> wrote: > > On Fri, 2023-05-19 at 10:16 +0100, Marc Zyngier wrote: > > CAUTION: This email originated from outside of the organization. Do > > not click links or open attachments unless you can confirm the sender > > and know the content is safe. > > > > > > > > On Thu, 18 May 2023 22:08:15 +0100, > > "Jitindar Singh, Suraj" <surajjs@xxxxxxxxxx> wrote: > > > > > > Reading init_cpu_ftr_reg() is hurting my head :p but don't we have > > > basically 2 cases here? > > > > > > 1. We are unaffected by spectre|meltdown i.e. > > > arm64_get_[spectre|meltdown]_v2_state() will return > > > SPECTRE_UNAFFECTED > > > and we will set the limit to 1 for the csv[1|2] bit fields in > > > read_sanitised_id_aa64pfr0_el1() > > > > > > or > > > > > > 2. We ARE affected by spectre|meltdown in which case > > > arm64_get_[spectre|meltdown]_v2_state() will be > > > SPECTRE_VULNERABLE|SPECTRE_MITIGATED in which case > > > read_sanitised_ftr_reg() will return a value with csv[1|2] set to 0 > > > essentially setting the limit for either of these bit fields to 0 > > > in > > > read_sanitised_id_aa64pfr0_el1(). > > > > What is "WE"? > > The CPU Hilarious. > > > > > > > > > Are we trying to catch the case where csv[1|2] is 0 on the host but > > > we > > > are unaffected as detected by e.g. cpuid and that cpu happens to > > > incorrectly not set csv[1|2] in the ID register but we still want > > > to > > > allow the guest to see those bits as set? > > > > > > This isn't really related to the CR as I know this is existing code > > > which has just been moved around and sorry if I'm missing something > > > obvious. > > > > This code is called from *userspace*, and tries to cope with a VM > > being restored. So we have 3 (not 2 cases): > > > > - both the source and the destination have the same level of CSVx > > support, and all is great in the world > > > > - the source has CSVx==0, while the destination has CSVx=1. All good, > > as we won't be lying to the guest, and the extra mitigation > > executed by the guest isn't a functional problem. The guest will > > still see CSVx=0 after migration. > > > > - the source has CSVx=1, while the destination has CSVx=0. This isn't > > an acceptable situation, and we return an error > > > > Is that clearer? > > Yes thanks, that all makes sense. > > My initial question was why we needed to enforce the limit essentially > twice in set_id_aa64pfr0_el1(). > > Once with: > /* > * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as > * it doesn't promise more than what is actually provided (the > * guest could otherwise be covered in ectoplasmic residue). > */ > csv2 = cpuid_feature_extract_unsigned_field(val, > ID_AA64PFR0_EL1_CSV2_SHIFT); > if (csv2 > 1 || > (csv2 && arm64_get_spectre_v2_state() != > SPECTRE_UNAFFECTED)) > return -EINVAL; > > /* Same thing for CSV3 */ > csv3 = cpuid_feature_extract_unsigned_field(val, > ID_AA64PFR0_EL1_CSV3_SHIFT); > if (csv3 > 1 || > (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED)) > return -EINVAL; > > And then again with the check in arm64_check_features(). Ah, I see what you mean. Yes, this isn't right. I asked for the generic code to be used in the past, but the old stuff was left in. Which is obviously not great. M. -- Without deviation from the norm, progress is not possible.
