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

[Marc Zyngier]

On Tue, 30 May 2023 22:18:04 +0100,
Jing Zhang <jingzhangos at google.com> wrote:
> 
> Hi Marc,
> 
> On Sun, May 28, 2023 at 4:05 AM Marc Zyngier <maz at kernel.org> wrote:
> >
> > On Mon, 22 May 2023 23:18:35 +0100,
> > Jing Zhang <jingzhangos at 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 at 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.

Sorry, this doesn't make sense. Lower is always fine. If you meant
'higher', then I agree that it would be bad. But that doesn't make
keeping this code the right outcome.

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

This is what I have been asking for all along: the "sanitised" view of
the register *must* return the absolute limit for the fields that are
flagged as writable by "mask".

If we need extra code, then something is really wrong. The core
feature code manages that without any special casing, and we should be
able to reach the same level.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.