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

On Sat, 20 May 2023 00:04:53 +0100,
"Jitindar Singh, Suraj" [off-list ref] wrote:

On Fri, 2023-05-19 at 10:16 +0100, Marc Zyngier wrote:

quoted

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" [off-list ref] wrote:

quoted

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.

quoted

quoted

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.

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