[RFC PATCH v3 10/29] KVM: arm64: Make ID_AA64DFR0_EL1 writable

[Reiji Watanabe]

Hi Eric,

On Thu, Nov 25, 2021 at 12:30 PM Eric Auger <eauger at redhat.com> wrote:
>
> Hi Reiji,
>
> On 11/17/21 7:43 AM, Reiji Watanabe wrote:
> > This patch adds id_reg_info for ID_AA64DFR0_EL1 to make it writable
> > by userspace.
> >
> > Return an error if userspace tries to set PMUVER field of the
> > register to a value that conflicts with the PMU configuration.
> >
> > Since number of context-aware breakpoints must be no more than number
> > of supported breakpoints according to Arm ARM, return an error
> > if userspace tries to set CTX_CMPS field to such value.
> >
> > Signed-off-by: Reiji Watanabe <reijiw at google.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 84 ++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 73 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 772e3d3067b2..0faf458b0efb 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -626,6 +626,45 @@ static int validate_id_aa64mmfr0_el1(struct kvm_vcpu *vcpu,
> >       return 0;
> >  }
> >
> > +static bool id_reg_has_pmu(u64 val, u64 shift, unsigned int min)
> I would rename the function as the name currently is misleading. The
> function validate the val filed @shift againt @min

Thank you for the comment.

The @min is the minimum value that indicates PMUv3 support.
So, if the field value is >= @min, it means PMUv3 is supported.
I want the function to check whether or not @val indicates PMUv3 support,
and that's how the function is used.
I can see what you meant focusing on the function though.
But, if we renaming it to xxx_validate, that would be misleading in the
codes that use the function.

> > +{
> > +     unsigned int pmu = cpuid_feature_extract_unsigned_field(val, shift);
> > +
> > +     /*
> > +      * Treat IMPLEMENTATION DEFINED functionality as unimplemented for
> > +      * ID_AA64DFR0_EL1.PMUVer/ID_DFR0_EL1.PerfMon.
> > +      */
> > +     if (pmu == 0xf)
> > +             pmu = 0;
> Shouldn't we simply forbid the userspace to set 0xF?

This function is to check whether or not the field value indicates PMUv3.
Setting the field to 0xf is forbidden by arm64_check_features().
Having said that, since arm64_check_features() will be implemented by
using arm64_ftr_bits, which treats AA64DFR0.PMUVER and DFR0.PERFMON
as signed fields.
So, it will be forbidden in a different way in the next version.

Thanks,
Reiji

> > +
> > +     return (pmu >= min);
> > +}