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

[Jitindar Singh, Suraj]

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

> 
> > 
> > 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.