diff mbox series

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

Message ID 20220706164304.1582687-9-maz@kernel.org (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: vgic-v3 userspace access consolidation (and other goodies) | expand

Commit Message

Marc Zyngier July 6, 2022, 4:42 p.m. UTC
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(-)

Comments

Reiji Watanabe July 12, 2022, 6:11 a.m. UTC | #1
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 ?


> +
> +       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@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Marc Zyngier July 12, 2022, 6:52 a.m. UTC | #2
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.
Reiji Watanabe July 13, 2022, 3:26 a.m. UTC | #3
Hi Marc,

On Mon, Jul 11, 2022 at 11:53 PM Marc Zyngier <maz@kernel.org> wrote:
>
> 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.

IMHO it makes more sense to have the change in this patch
because vgic_v3_cpu_sysregs_uaccess in this patch already
takes "attr" as the arguments.
Having said that, I'm fine with keeping the current code:)

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

Oh, I haven't seen PATCH#12 yet. Then let me withdraw my comment.

Thank you,
Reiji
diff mbox series

Patch

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