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

[Reiji Watanabe <reijiw@xxxxxxxxxx>]

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:)

Reviewed-by: Reiji Watanabe <reijiw@xxxxxxxxxx>

Thank you,
Reiji
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm