| Message ID | 1414776414-13426-13-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 31, 2014 at 05:26:47PM +0000, Andre Przywara wrote: > vgic_set_attr() and vgic_get_attr() contain both code specific for > the emulated GIC as well as code for the userland facing, generic > part of the GIC. > Split the guest GIC facing code of from the generic part to allow > easier splitting later. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> It's not really clear to me which data is specific to the emulated gic and which is not or why you have to do this (yet), for example, the _common function is now dealing with the GRP_ADDR case which is very GICv2 specific (so far). But I assume this will make sense as I progress through the series. Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Hi Christoffer, On 04/11/14 19:30, Christoffer Dall wrote: > On Fri, Oct 31, 2014 at 05:26:47PM +0000, Andre Przywara wrote: >> vgic_set_attr() and vgic_get_attr() contain both code specific for >> the emulated GIC as well as code for the userland facing, generic >> part of the GIC. >> Split the guest GIC facing code of from the generic part to allow >> easier splitting later. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > It's not really clear to me which data is specific to the emulated gic > and which is not or why you have to do this (yet), for example, the > _common function is now dealing with the GRP_ADDR case which is very > GICv2 specific (so far). But I assume this will make sense as I > progress through the series. Admittedly this is somewhat of a corner case. Actually I tried to keep as much code common (in vgic.c) as possible, and it was possible without much pain for GRP_ADDR and kvm_vgic_addr. Also I consider this call part of the switching and connecting functionality of the VGIC. Looking at the code again I think I had it in -emul.c before, but decided to move it back for some reason (probably some other code dependency which needed to be exposed). So unless I find some time ;-) and a good reason to move it I tend to keep it here. Cheers, Andre.
On 05/11/14 10:27, Andre Przywara wrote: > Hi Christoffer, > > On 04/11/14 19:30, Christoffer Dall wrote: >> On Fri, Oct 31, 2014 at 05:26:47PM +0000, Andre Przywara wrote: >>> vgic_set_attr() and vgic_get_attr() contain both code specific for >>> the emulated GIC as well as code for the userland facing, generic >>> part of the GIC. >>> Split the guest GIC facing code of from the generic part to allow >>> easier splitting later. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> >> It's not really clear to me which data is specific to the emulated gic >> and which is not or why you have to do this (yet), for example, the >> _common function is now dealing with the GRP_ADDR case which is very >> GICv2 specific (so far). But I assume this will make sense as I >> progress through the series. > > Admittedly this is somewhat of a corner case. Actually I tried to keep > as much code common (in vgic.c) as possible, and it was possible without > much pain for GRP_ADDR and kvm_vgic_addr. > Also I consider this call part of the switching and connecting > functionality of the VGIC. > Looking at the code again I think I had it in -emul.c before, but > decided to move it back for some reason (probably some other code > dependency which needed to be exposed). So unless I find some time ;-) > and a good reason to move it I tend to keep it here. ... just found that kvm_vgic_addr() is not static, but also called from the (now legacy) KVM_ARM_SET_DEVICE_ADDR ioctl. So it was causing more churn to move it than it gave us clean separation. Cheers, Andre. -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
On Wed, Nov 05, 2014 at 10:27:43AM +0000, Andre Przywara wrote: > Hi Christoffer, > > On 04/11/14 19:30, Christoffer Dall wrote: > > On Fri, Oct 31, 2014 at 05:26:47PM +0000, Andre Przywara wrote: > >> vgic_set_attr() and vgic_get_attr() contain both code specific for > >> the emulated GIC as well as code for the userland facing, generic > >> part of the GIC. > >> Split the guest GIC facing code of from the generic part to allow > >> easier splitting later. > >> > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > It's not really clear to me which data is specific to the emulated gic > > and which is not or why you have to do this (yet), for example, the > > _common function is now dealing with the GRP_ADDR case which is very > > GICv2 specific (so far). But I assume this will make sense as I > > progress through the series. > > Admittedly this is somewhat of a corner case. Actually I tried to keep > as much code common (in vgic.c) as possible, and it was possible without > much pain for GRP_ADDR and kvm_vgic_addr. > Also I consider this call part of the switching and connecting > functionality of the VGIC. > Looking at the code again I think I had it in -emul.c before, but > decided to move it back for some reason (probably some other code > dependency which needed to be exposed). So unless I find some time ;-) > and a good reason to move it I tend to keep it here. > That's fine, my comment was more directed at the commit message than the code itself; the patch itself looks ok. -Christoffer
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index da501a2..6ff5acd 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -2383,7 +2383,8 @@ out: return ret; } -static int vgic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) +static int vgic_set_common_attr(struct kvm_device *dev, + struct kvm_device_attr *attr) { int r; @@ -2399,17 +2400,6 @@ static int vgic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) r = kvm_vgic_addr(dev->kvm, type, &addr, true); return (r == -ENODEV) ? -ENXIO : r; } - - 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; - - if (get_user(reg, uaddr)) - return -EFAULT; - - return vgic_attr_regs_access(dev, attr, ®, true); - } case KVM_DEV_ARM_VGIC_GRP_NR_IRQS: { u32 __user *uaddr = (u32 __user *)(long)attr->addr; u32 val; @@ -2446,7 +2436,33 @@ static int vgic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) return -ENXIO; } -static int vgic_get_attr(struct kvm_device *dev, struct kvm_device_attr *attr) +static int vgic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) +{ + int ret; + + ret = vgic_set_common_attr(dev, attr); + if (ret != -ENXIO) + return ret; + + switch (attr->group) { + 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; + + if (get_user(reg, uaddr)) + return -EFAULT; + + return vgic_attr_regs_access(dev, attr, ®, true); + } + + } + + return -ENXIO; +} + +static int vgic_get_common_attr(struct kvm_device *dev, + struct kvm_device_attr *attr) { int r = -ENXIO; @@ -2464,27 +2480,41 @@ static int vgic_get_attr(struct kvm_device *dev, struct kvm_device_attr *attr) return -EFAULT; break; } + case KVM_DEV_ARM_VGIC_GRP_NR_IRQS: { + u32 __user *uaddr = (u32 __user *)(long)attr->addr; + + r = put_user(dev->kvm->arch.vgic.nr_irqs, uaddr); + break; + } + + } + + return r; +} + +static int vgic_get_attr(struct kvm_device *dev, struct kvm_device_attr *attr) +{ + int ret; + + ret = vgic_get_common_attr(dev, attr); + if (ret != -ENXIO) + return ret; + switch (attr->group) { 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; - r = vgic_attr_regs_access(dev, attr, ®, false); - if (r) - return r; - r = put_user(reg, uaddr); - break; - } - case KVM_DEV_ARM_VGIC_GRP_NR_IRQS: { - u32 __user *uaddr = (u32 __user *)(long)attr->addr; - r = put_user(dev->kvm->arch.vgic.nr_irqs, uaddr); - break; + ret = vgic_attr_regs_access(dev, attr, ®, false); + if (ret) + return ret; + return put_user(reg, uaddr); } } - return r; + return -ENXIO; } static int vgic_has_attr_regs(const struct mmio_range *ranges,
vgic_set_attr() and vgic_get_attr() contain both code specific for the emulated GIC as well as code for the userland facing, generic part of the GIC. Split the guest GIC facing code of from the generic part to allow easier splitting later. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- virt/kvm/arm/vgic.c | 78 +++++++++++++++++++++++++++++++++++---------------- 1 file changed, 54 insertions(+), 24 deletions(-)