Re: [PATCH 08/19] KVM: arm64: vgic-v3: Push user access into vgic_v3_cpu_sysregs_uaccess()

[Marc Zyngier <maz@kernel.org>]

Hi Reiji,

On Tue, 12 Jul 2022 07:11:39 +0100,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> Hi Marc,
> 
> On Wed, Jul 6, 2022 at 9:43 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > In order to start making the vgic sysreg access from userspace
> > similar to all the other sysregs, push the userspace memory
> > access one level down into vgic_v3_cpu_sysregs_uaccess().
> >
> > The next step will be to rely on the sysreg infrastructure
> > to perform this task.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/vgic-sys-reg-v3.c      | 22 +++++++++++++------
> >  arch/arm64/kvm/vgic/vgic-kvm-device.c | 31 ++++++---------------------
> >  arch/arm64/kvm/vgic/vgic.h            |  4 ++--
> >  3 files changed, 23 insertions(+), 34 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c
> > index 85a5e1d15e9f..8c56e285fde9 100644
> > --- a/arch/arm64/kvm/vgic-sys-reg-v3.c
> > +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
> > @@ -278,15 +278,21 @@ int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *
> >         return -ENXIO;
> >  }
> >
> > -int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> > -                               u64 *reg)
> > +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu,
> > +                               struct kvm_device_attr *attr,
> > +                               bool is_write)
> >  {
> > +       u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> >         struct sys_reg_params params;
> >         const struct sys_reg_desc *r;
> > -       u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64;
> > +       u64 sysreg;
> >
> > -       if (is_write)
> > -               params.regval = *reg;
> > +       sysreg = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64;
> 
> Why don't you use attr_to_id() here ?

This actually happens in the following patch. Happy to move the change
here though.

> 
> 
> > +
> > +       if (is_write) {
> > +               if (get_user(params.regval, uaddr))
> > +                       return -EFAULT;
> > +       }
> >         params.is_write = is_write;
> >
> >         r = find_reg_by_id(sysreg, &params, gic_v3_icc_reg_descs,
> > @@ -297,8 +303,10 @@ int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> >         if (!r->access(vcpu, &params, r))
> >                 return -EINVAL;
> >
> > -       if (!is_write)
> > -               *reg = params.regval;
> > +       if (!is_write) {
> > +               if (put_user(params.regval, uaddr))
> > +                       return -EFAULT;
> > +       }
> >
> >         return 0;
> >  }
> > diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > index c6d52a1fd9c8..d8269300632d 100644
> > --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > @@ -561,14 +561,9 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> >                 if (!is_write)
> >                         *reg = tmp32;
> >                 break;
> > -       case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS: {
> > -               u64 regid;
> > -
> > -               regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
> > -               ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
> > -                                                 regid, reg);
> > +       case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
> > +               ret = vgic_v3_cpu_sysregs_uaccess(vcpu, attr, is_write);
> 
> Nit: Since @reg that is passed to vgic_v3_attr_regs_access() will be NULL
> for KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS, I think it would be more clear
> if you could update the comment for vgic_v3_attr_regs_access accordingly.
> 
> ----
> /*
>  * vgic_v3_attr_regs_access - allows user space to access VGIC v3 state
>  *
>  * @dev:      kvm device handle
>  * @attr:     kvm device attribute
>  * @reg:      address the value is read or written
>    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  * @is_write: true if userspace is writing a register
>  */
> static int vgic_v3_attr_regs_access(struct kvm_device *dev,
>                                     struct kvm_device_attr *attr,
>                                     u64 *reg, bool is_write)

@reg disappears completely in patch #12. Do you see value in rewriting
this comment even if I end-up removing it 4 patches down the line?

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel