[PATCH v9 05/11] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Jing Zhang]

Hi Marc,

On Tue, Aug 22, 2023 at 12:06 AM Marc Zyngier <maz at kernel.org> wrote:
>
> On Mon, 21 Aug 2023 22:22:37 +0100,
> Jing Zhang <jingzhangos at google.com> wrote:
> >
> > All valid fields in ID_AA64DFR0_EL1 and ID_DFR0_EL1 are writable
> > from userspace with this change.
> > RES0 fields and those fields hidden by KVM are not writable.
> >
> > Signed-off-by: Jing Zhang <jingzhangos at google.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index afade7186675..20fc38bad4e8 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1931,6 +1931,8 @@ static bool access_spsr(struct kvm_vcpu *vcpu,
> >       return true;
> >  }
> >
> > +#define ID_AA64DFR0_EL1_RES0_MASK (GENMASK(59, 56) | GENMASK(27, 24) | GENMASK(19, 16))
> > +
> >  /*
> >   * Architected system registers.
> >   * Important: Must be sorted ascending by Op0, Op1, CRn, CRm, Op2
> > @@ -2006,7 +2008,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >         .set_user = set_id_dfr0_el1,
> >         .visibility = aa32_id_visibility,
> >         .reset = read_sanitised_id_dfr0_el1,
> > -       .val = ID_DFR0_EL1_PerfMon_MASK, },
> > +       .val = GENMASK(31, 0), },
>
> Can you *please* look at the register and realise that we *don't*
> support writing to the whole of the low 32 bits? What does it mean to
> allow selecting the M-profile debug? Or the memory-mapped trace?
>
> You are advertising a lot of crap to userspace, and that's definitely
> not on.
>
> >       ID_HIDDEN(ID_AFR0_EL1),
> >       AA32_ID_SANITISED(ID_MMFR0_EL1),
> >       AA32_ID_SANITISED(ID_MMFR1_EL1),
> > @@ -2055,7 +2057,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >         .get_user = get_id_reg,
> >         .set_user = set_id_aa64dfr0_el1,
> >         .reset = read_sanitised_id_aa64dfr0_el1,
> > -       .val = ID_AA64DFR0_EL1_PMUVer_MASK, },
> > +       .val = ~(ID_AA64DFR0_EL1_PMSVer_MASK | ID_AA64DFR0_EL1_RES0_MASK), },
>
> And it is the same thing here. Where is the handling code to deal with
> variable breakpoint numbers? Oh wait, there is none. Really, the only
> thing we support writing to are the PMU and Debug versions. And
> nothing else.
>
> What does it mean for userspace? Either the write will be denied
> despite being advertised a writable field (remember the first patch of
> the series???), or we'll blindly accept the write and further ignore
> the requested values. Do you really think any of this is acceptable?
>
> This is the *9th* version of this series, and we're still battling
> over some extremely basic userspace issues... I don't think we can
> merge this series as is stands.

I removed sanity checks across multiple fields after the discussion
here: https://lore.kernel.org/all/ZKRC80hb4hXwW8WK@thinky-boi/
I might have misunderstood the discussion. I thought we'd let VMM do
more complete sanity checks.

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

Jing