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, ¶ms, 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, ¶ms, 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, ®, 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, ®, 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, ®, 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
