[PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3

[Marc Zyngier]

On Sat, 20 May 2023 00:04:53 +0100,
"Jitindar Singh, Suraj" <surajjs at amazon.com> 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 at amazon.com> 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.