Re: [PATCH v10 5/5] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3

[Jing Zhang <jingzhangos@google.com>]

Hi Marc,

On Sun, May 28, 2023 at 4:05 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 22 May 2023 23:18:35 +0100,
> Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Refactor writings for ID_AA64PFR0_EL1.[CSV2|CSV3],
> > ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon based on utilities
> > specific to ID register.
> >
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> >  arch/arm64/include/asm/cpufeature.h |   1 +
> >  arch/arm64/kernel/cpufeature.c      |   2 +-
> >  arch/arm64/kvm/sys_regs.c           | 365 ++++++++++++++++++----------
> >  3 files changed, 243 insertions(+), 125 deletions(-)
>
> Reading the result after applying this series, I feel like a stuck
> record. This final series still contains gems like this:
>
> static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
>                                const struct sys_reg_desc *rd,
>                                u64 val)
> {
>         u8 csv2, csv3;
>
>         /*
>          * 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;
>
>         return set_id_reg(vcpu, rd, val);
> }
>
> Why do we have this? I've asked the question at least 3 times in the
> previous versions, and I still see the same code.
>
> If we have sane limits, the call to arm64_check_features() in
> set_id_reg() will catch the illegal write. So why do we have this at
> all? The whole point of the exercise was to unify the handling. But
> you're actually making it worse.
>
> So what's the catch?
Sorry, I am only aware of one discussion of this code in v8. The
reason I still keep the check here is that the arm64_check_features()
can not catch all illegal writes as this code does.
For example, for CSV2, one concern is:
When arm64_get_spectre_v2_state() != SPECTRE_UNAFFECTED, this code
only allows guest CSV2 to be set to 0, any non-zero value would lead
to -EINVAL. If we remove the check here, the guest CSV2 can be set to
any value lower or equal to host CSV2.
Of course, we can set the sane limit of CSV2 to 0 when
arm64_get_spectre_v2_state() != SPECTRE_UNAFFECTED in
read_sanitised_id_aa64pfr0_el1(). Then we can remove all the checks
here and no specific set_id function for AA64PFR0_EL1 is needed.
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.
Thanks,
Jing