Hi Marc,
On Fri, Jul 21, 2023 at 2:31 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Fri, 21 Jul 2023 09:38:23 +0100,
> Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> >
> > On Thu, Jul 20 2023, Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
> >
> > > Hi Cornelia,
> > >
> > > On Thu, Jul 20, 2023 at 1:52 AM Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> > >>
> > >> On Tue, Jul 18 2023, Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
> > >>
> > >> > All valid fields in ID_AA64DFR0_EL1 and ID_DFR0_EL1 are writable
> > >> > from usrespace with this change.
> > >>
> > >> Typo: s/usrespace/userspace/
> > > Thanks.
> > >>
> > >> >
> > >> > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> > >> > ---
> > >> > arch/arm64/kvm/sys_regs.c | 4 ++--
> > >> > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >> >
> > >> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > >> > index 053d8057ff1e..f33aec83f1b4 100644
> > >> > --- a/arch/arm64/kvm/sys_regs.c
> > >> > +++ b/arch/arm64/kvm/sys_regs.c
> > >> > @@ -2008,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(63, 0), },
> > >> > ID_HIDDEN(ID_AFR0_EL1),
> > >> > AA32_ID_SANITISED(ID_MMFR0_EL1),
> > >> > AA32_ID_SANITISED(ID_MMFR1_EL1),
> > >> > @@ -2057,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 = GENMASK(63, 0), },
> > >> > ID_SANITISED(ID_AA64DFR1_EL1),
> > >> > ID_UNALLOCATED(5,2),
> > >> > ID_UNALLOCATED(5,3),
> > >>
> > >> How does userspace find out whether a given id reg is actually writable,
> > >> other than trying to write to it?
> > >>
> > > No mechanism was provided to userspace to discover if a given idreg or
> > > any fields of a given idreg is writable. The write to a readonly idreg
> > > can also succeed (write ignored) without any error if what's written
> > > is exactly the same as what the idreg holds or if it is a write to
> > > AArch32 idregs on an AArch64-only system.
> >
> > Hm, I'm not sure that's a good thing for the cases where we want to
> > support mix-and-match userspace and kernels. Userspace may want to know
> > upfront whether it can actually tweak the contents of an idreg or not
> > (for example, in the context of using CPU models for compatibility), so
> > that it can reject or warn about certain configurations that may not
> > turn out as the user expects.
> >
> > > Not sure if it is worth adding an API to return the writable mask for
> > > idregs, since we want to enable the writable for all allocated
> > > unhidden idregs eventually.
> >
> > We'd enable any new idregs for writing from the start in the future, I
> > guess?
> >
> > I see two approaches here:
> > - add an API to get a list of idregs with their writable masks
> > - add a capability "you can write to all idregs whatever you'd expect to
> > be able to write there architecture wise", which would require to add
> > support for all idregs prior to exposing that cap
> >
> > The second option would be the easier one (if we don't manage to break
> > it in the future :)
>
> I'm not sure the last option is even possible. The architecture keeps
> allocating new ID registers in the op0==3, op1=={0, 1, 3}, CRn==0,
> CRm=={0-7}, op2=={0-7} space, so fields that were RES0 until then
> start having a non-0 value.
For now, the per VM ID emulated ID registers support only covers space
for op0==3, op1==0, CRn==0, CRm=={1-7}, op2=={0-8}. For others, mask
value of 0 would be returned in the new ioctl.
>
> This could lead to a situation where you move from a system that
> didn't know about ID_AA64MMFR6_EL1.XYZ to a system that advertises it,
> and for which the XYZ instruction has another behaviour. Bad things
> follow.
>
> My preference would be a single ioctl that returns the full list of
> writeable masks in the ID reg range. It is big, but not crazy big
> (1536 bytes, if I haven't messed up), and includes the non ID_*_EL1
> sysreg such as MPIDR_EL1, CTR_EL1, SMIDR_EL1.
Just want to double confirm that would the ioclt return the list of
only writable masks, not the list of {idreg_name, mask} pair? So, the
VMM will need to index idreg's writable mask by op1, CRm, op2?
>
> It would allow the VMM to actively write zeroes to any writable ID
> register it doesn't know about, or for which it doesn't have anything
> to restore. It is also relatively future proof, as it covers
> *everything* the architecture has provisioned for the future (by the
> time that space is exhausted, I hope none of us will still be involved
> with this crap).
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
>
Thanks,
Jing
