Hi Marc,
On Sat, Aug 26, 2023 at 3:51 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Tue, 22 Aug 2023 19:35:12 +0100,
> Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
> >
> > Hi Marc,
> >
> > On Tue, Aug 22, 2023 at 12:06 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > On Mon, 21 Aug 2023 22:22:37 +0100,
> > > Jing Zhang <jingzhangos@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > > 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)
Thanks,
Jing
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
