From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [RFC PATCH v2 1/4] arm/arm64: vgic-new: Introduce 64-bit reg access support Date: Tue, 16 Aug 2016 17:01:14 +0200 Message-ID: <20160816150114.GC14088@cbox> References: <1470740326-27751-1-git-send-email-vijay.kilari@gmail.com> <1470740326-27751-2-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id B635E49B30 for ; Tue, 16 Aug 2016 10:51:51 -0400 (EDT) Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id tJ-yokBAVKOe for ; Tue, 16 Aug 2016 10:51:50 -0400 (EDT) Received: from mail-wm0-f41.google.com (mail-wm0-f41.google.com [74.125.82.41]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 37BD049B2F for ; Tue, 16 Aug 2016 10:51:49 -0400 (EDT) Received: by mail-wm0-f41.google.com with SMTP id i5so173708282wmg.0 for ; Tue, 16 Aug 2016 07:59:16 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1470740326-27751-2-git-send-email-vijay.kilari@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: vijay.kilari@gmail.com Cc: Prasun.Kapoor@cavium.com, marc.zyngier@arm.com, Vijaya Kumar K , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org List-Id: kvmarm@lists.cs.columbia.edu On Tue, Aug 09, 2016 at 04:28:43PM +0530, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > 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 > --- > 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 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: christoffer.dall@linaro.org (Christoffer Dall) Date: Tue, 16 Aug 2016 17:01:14 +0200 Subject: [RFC PATCH v2 1/4] arm/arm64: vgic-new: Introduce 64-bit reg access support In-Reply-To: <1470740326-27751-2-git-send-email-vijay.kilari@gmail.com> References: <1470740326-27751-1-git-send-email-vijay.kilari@gmail.com> <1470740326-27751-2-git-send-email-vijay.kilari@gmail.com> Message-ID: <20160816150114.GC14088@cbox> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Aug 09, 2016 at 04:28:43PM +0530, vijay.kilari at gmail.com wrote: > From: Vijaya Kumar K > > 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 > --- > 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 >