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
>
> >
> > 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().
Thanks,
Suraj
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
