Re: [PATCH 13/19] KVM: arm64: vgic-v2: Consolidate userspace access for MMIO registers

[Marc Zyngier <maz@xxxxxxxxxx>]

On Thu, 14 Jul 2022 05:43:27 +0100,
Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
> 
> Hi Marc,
> 
> On Wed, Jul 6, 2022 at 10:05 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> >
> > Align the GICv2 MMIO accesses from userspace with the way the GICv3
> > code is now structured.
> >
> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > ---
> >  arch/arm64/kvm/vgic/vgic-kvm-device.c | 40 ++++++++++++---------------
> >  1 file changed, 18 insertions(+), 22 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > index 925875722027..ddead333c232 100644
> > --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > @@ -348,17 +348,18 @@ bool lock_all_vcpus(struct kvm *kvm)
> >   *
> >   * @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_v2_attr_regs_access(struct kvm_device *dev,
> >                                     struct kvm_device_attr *attr,
> > -                                   u32 *reg, bool is_write)
> > +                                   bool is_write)
> >  {
> > +       u32 __user *uaddr = (u32 __user *)(unsigned long)attr->addr;
> >         struct vgic_reg_attr reg_attr;
> >         gpa_t addr;
> >         struct kvm_vcpu *vcpu;
> >         int ret;
> > +       u32 val;
> >
> >         ret = vgic_v2_parse_attr(dev, attr, &reg_attr);
> >         if (ret)
> > @@ -367,6 +368,10 @@ static int vgic_v2_attr_regs_access(struct kvm_device *dev,
> >         vcpu = reg_attr.vcpu;
> >         addr = reg_attr.addr;
> >
> > +       if (is_write)
> > +               if (get_user(val, uaddr))
> > +                       return -EFAULT;
> > +
> >         mutex_lock(&dev->kvm->lock);
> >
> >         ret = vgic_init(dev->kvm);
> > @@ -380,10 +385,10 @@ static int vgic_v2_attr_regs_access(struct kvm_device *dev,
> >
> >         switch (attr->group) {
> >         case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> > -               ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg);
> > +               ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, &val);
> >                 break;
> >         case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> > -               ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg);
> > +               ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, &val);
> >                 break;
> >         default:
> >                 ret = -EINVAL;
> > @@ -393,6 +398,11 @@ static int vgic_v2_attr_regs_access(struct kvm_device *dev,
> >         unlock_all_vcpus(dev->kvm);
> >  out:
> >         mutex_unlock(&dev->kvm->lock);
> > +
> > +       if (!ret && !is_write)
> > +               if (put_user(val, uaddr))
> > +                       ret = -EFAULT;
> > +
> >         return ret;
> >  }
> >
> > @@ -407,15 +417,8 @@ static int vgic_v2_set_attr(struct kvm_device *dev,
> >
> >         switch (attr->group) {
> >         case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> > -       case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
> > -               u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> > -               u32 reg;
> > -
> > -               if (get_user(reg, uaddr))
> > -                       return -EFAULT;
> > -
> > -               return vgic_v2_attr_regs_access(dev, attr, &reg, true);
> > -       }
> > +       case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> > +               return vgic_v2_attr_regs_access(dev, attr, true);
> >         }
> >
> >         return -ENXIO;
> > @@ -432,15 +435,8 @@ static int vgic_v2_get_attr(struct kvm_device *dev,
> >
> >         switch (attr->group) {
> >         case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> > -       case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
> > -               u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> > -               u32 reg = 0;
> > -
> > -               ret = vgic_v2_attr_regs_access(dev, attr, &reg, false);
> > -               if (ret)
> > -                       return ret;
> > -               return put_user(reg, uaddr);
> > -       }
> > +       case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> > +               return vgic_v2_attr_regs_access(dev, attr, false);
> >         }
> >
> >         return -ENXIO;
> 
> For vgic_v2_{set,get}_attr(), perhaps it might be even simpler
> to call vgic_{set,get}_common_attr() from the "default" case
> of "switch (attr->group)".
> This is not directly related to this patch though:)

Indeed. This also applies to v3, and there is a couple more cleanups
that can be added. I'll add that as an extra patch, as the result is
rather nice.

> Reviewed-by: Reiji Watanabe <reijiw@xxxxxxxxxx>

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm