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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marc,

On Wed, Jul 6, 2022 at 9:43 AM Marc Zyngier <maz@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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 ?


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

Thank you,
Reiji


>                 break;
> -       }
>         case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
>                 unsigned int info, intid;
>
> @@ -617,15 +612,8 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
>                 reg = tmp32;
>                 return vgic_v3_attr_regs_access(dev, attr, &reg, true);
>         }
> -       case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS: {
> -               u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> -               u64 reg;
> -
> -               if (get_user(reg, uaddr))
> -                       return -EFAULT;
> -
> -               return vgic_v3_attr_regs_access(dev, attr, &reg, true);
> -       }
> +       case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
> +               return vgic_v3_attr_regs_access(dev, attr, NULL, true);
>         case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
>                 u32 __user *uaddr = (u32 __user *)(long)attr->addr;
>                 u64 reg;
> @@ -681,15 +669,8 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
>                 tmp32 = reg;
>                 return put_user(tmp32, uaddr);
>         }
> -       case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS: {
> -               u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> -               u64 reg;
> -
> -               ret = vgic_v3_attr_regs_access(dev, attr, &reg, false);
> -               if (ret)
> -                       return ret;
> -               return put_user(reg, uaddr);
> -       }
> +       case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
> +               return vgic_v3_attr_regs_access(dev, attr, NULL, false);
>         case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
>                 u32 __user *uaddr = (u32 __user *)(long)attr->addr;
>                 u64 reg;
> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
> index ffc2d3c81b28..c23118467a35 100644
> --- a/arch/arm64/kvm/vgic/vgic.h
> +++ b/arch/arm64/kvm/vgic/vgic.h
> @@ -245,8 +245,8 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>                          int offset, u32 *val);
>  int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>                          int offset, u32 *val);
> -int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> -                        u64 id, u64 *val);
> +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu,
> +                               struct kvm_device_attr *attr, bool is_write);
>  int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
>  int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>                                     u32 intid, u64 *val);
> --
> 2.34.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm



[Index of Archives]     [Linux KVM]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux