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

[Jing Zhang]

Hi Marc,

On Sun, Aug 27, 2023 at 12:31 PM Jing Zhang <jingzhangos at google.com> wrote:
>
> Hi Marc,
>
> On Sat, Aug 26, 2023 at 3:51 AM Marc Zyngier <maz at kernel.org> wrote:
> >
> > On Tue, 22 Aug 2023 19:35:12 +0100,
> > Jing Zhang <jingzhangos at google.com> wrote:
> > >
> > > 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.
> >
> > The problem is that you don't even have a statement about how this is
> > supposed to be handled. What are the rules? How can the VMM author
> > *know*?
> >
> > That's my real issue with this series: at no point do we state when an
> > ID register can be written, what are the rules that must be followed,
> > where is the split in responsibility between KVM and the VMM, and what
> > is the expected behaviour when the VMM exposes something that is
> > completely outlandish to the guest (such as the M-profile debug).
> >
> > Do you see the issue? We can always fix the code. But once we've
> > exposed that to userspace, there is no going back. And I really want
> > everybody's buy-in on that front, including the VMM people.
>
> Understood.
> Looks like it would be less complicated to have KVM do all the sanity
> checks to determine if a feature field is configured correctly.
> The determination can be done by following rules:
> 1. Architecture constraints from ARM ARM.
> 2. KVM constraints. (Those features not exposed by KVM should be read-only)
> 3. VCPU features. (The write mask needs to be determined on-the-fly)

Does this sound good to you to have all sanity checks in KVM?

Thanks,
Jing

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