On Tue, Aug 09, 2016 at 04:28:43PM +0530, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>
> vgic_attr_regs_access() handles only 32-bit register
> value. Pass u64 as parameter and locally handle 32-bit
> reads and writes depending on attribute group.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> ---
> virt/kvm/arm/vgic/vgic-kvm-device.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 0130c4b..06de322 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -236,12 +236,13 @@ void kvm_register_vgic_device(unsigned long type)
> */
> static int vgic_attr_regs_access(struct kvm_device *dev,
> struct kvm_device_attr *attr,
> - u32 *reg, bool is_write)
> + u64 *reg, bool is_write)
> {
> gpa_t addr;
> int cpuid, ret, c;
> struct kvm_vcpu *vcpu, *tmp_vcpu;
> int vcpu_lock_idx = -1;
> + u32 tmp32;
>
> cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
> KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> @@ -272,12 +273,19 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
> vcpu_lock_idx = c;
> }
>
> + if (is_write)
> + tmp32 = *reg;
> +
I'm not a fan of this, from seeing that you do the read conversion
inside the case statements I gather you put this here so you only have
to have it once, even though you throw it away if you're doing 64-bit
accesses?
But a greater concern is the vgic_init() call above, which you don't
handle.
I thought we were supposed to get rid of all this lazy vgic init stuff.
Let me send you a patch series of how to rework this vgic_attr function
so that you can reuse some of the functionality and implement a new
gicv3 function on top of that.
Thanks,
-Christoffer
> 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, &tmp32);
> + if (!is_write)
> + *reg = tmp32;
> 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, &tmp32);
> + if (!is_write)
> + *reg = tmp32;
> break;
> default:
> ret = -EINVAL;
> @@ -309,11 +317,13 @@ static int vgic_v2_set_attr(struct kvm_device *dev,
> 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;
> + u32 tmp32;
> + u64 reg;
>
> - if (get_user(reg, uaddr))
> + if (get_user(tmp32, uaddr))
> return -EFAULT;
>
> + reg = tmp32;
> return vgic_attr_regs_access(dev, attr, ®, true);
> }
> }
> @@ -334,12 +344,14 @@ static int vgic_v2_get_attr(struct kvm_device *dev,
> 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;
> + u32 tmp32;
> + u64 reg;
>
> ret = vgic_attr_regs_access(dev, attr, ®, false);
> if (ret)
> return ret;
> - return put_user(reg, uaddr);
> + tmp32 = reg;
> + return put_user(tmp32, uaddr);
> }
> }
>
> --
> 1.9.1
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
