Hi Marc,
On Wed, May 31, 2023 at 12:31 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Tue, 30 May 2023 22:18:04 +0100,
> Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
> >
> > Hi Marc,
> >
> > On Sun, May 28, 2023 at 4:05 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > On Mon, 22 May 2023 23:18:35 +0100,
> > > Jing Zhang <jingzhangos@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > > 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.
Got it. Then it would be good to remove the check here. Will do that.
>
> > 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.
Understood.
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Thanks,
Jing
