Re: [PATCH v6 1/7] arm/arm64: vgic-new: Implement support for userspace access
On Fri, Sep 23, 2016 at 10:50:38AM +0100, Marc Zyngier wrote: > On 22/09/16 15:01, Vijay Kilari wrote: > > On Thu, Sep 22, 2016 at 5:38 PM, Marc Zyngier wrote: > >> On 20/09/16 07:12, vijay.kil...@gmail.com wrote: > >>> From: Vijaya Kumar K > >>> +static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device > >>> *dev, > >>> + gpa_t addr, u32 *val) > >>> +{ > >>> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev); > >>> + const struct vgic_register_region *region; > >>> + struct kvm_vcpu *r_vcpu; > >>> + > >>> + region = vgic_get_mmio_region(iodev, addr, sizeof(u32)); > >>> + if (!region) { > >>> + *val = 0; > >>> + return 0; > >> > >> This is not the previous semantic of vgic_uaccess, and I cannot see why > >> blindly ignoring an access to an undefined region would be acceptable. > >> What am I missing? > > > > AFAIK, the vgic_uaccess is not making any check on undefined > > region/register. > > However, dispatch_mmio_read/write are returning 0 if check of region is > > failed > > Hmmm. Fair enough. I don't really like it, but that's something for > another day. > Agreed, we should raise an error in that case, but it's independent of this series. -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 1/7] arm/arm64: vgic-new: Implement support for userspace access
On 22/09/16 15:01, Vijay Kilari wrote: > On Thu, Sep 22, 2016 at 5:38 PM, Marc Zyngier wrote: >> On 20/09/16 07:12, vijay.kil...@gmail.com wrote: >>> From: Vijaya Kumar K >>> +static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device >>> *dev, >>> + gpa_t addr, u32 *val) >>> +{ >>> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev); >>> + const struct vgic_register_region *region; >>> + struct kvm_vcpu *r_vcpu; >>> + >>> + region = vgic_get_mmio_region(iodev, addr, sizeof(u32)); >>> + if (!region) { >>> + *val = 0; >>> + return 0; >> >> This is not the previous semantic of vgic_uaccess, and I cannot see why >> blindly ignoring an access to an undefined region would be acceptable. >> What am I missing? > > AFAIK, the vgic_uaccess is not making any check on undefined region/register. > However, dispatch_mmio_read/write are returning 0 if check of region is failed Hmmm. Fair enough. I don't really like it, but that's something for another day. M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 1/7] arm/arm64: vgic-new: Implement support for userspace access
On Thu, Sep 22, 2016 at 5:38 PM, Marc Zyngier wrote: > On 20/09/16 07:12, vijay.kil...@gmail.com wrote: >> From: Vijaya Kumar K >> >> Read and write of some registers like ISPENDR and ICPENDR >> from userspace requires special handling when compared to >> guest access for these registers. >> >> Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt >> for handling of ISPENDR, ICPENDR registers handling. >> >> Add infrastructure to support guest and userspace read >> and write for the required registers >> Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c >> >> Signed-off-by: Vijaya Kumar K >> --- >> virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 -- >> virt/kvm/arm/vgic/vgic-mmio-v3.c | 98 >> >> virt/kvm/arm/vgic/vgic-mmio.c| 78 >> virt/kvm/arm/vgic/vgic-mmio.h| 19 >> 4 files changed, 169 insertions(+), 51 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c >> b/virt/kvm/arm/vgic/vgic-mmio-v2.c >> index b44b359..0b32f40 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c >> @@ -406,31 +406,6 @@ int vgic_v2_has_attr_regs(struct kvm_device *dev, >> struct kvm_device_attr *attr) >> return -ENXIO; >> } >> >> -/* >> - * When userland tries to access the VGIC register handlers, we need to >> - * create a usable struct vgic_io_device to be passed to the handlers and we >> - * have to set up a buffer similar to what would have happened if a guest >> MMIO >> - * access occurred, including doing endian conversions on BE systems. >> - */ >> -static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev, >> - bool is_write, int offset, u32 *val) >> -{ >> - unsigned int len = 4; >> - u8 buf[4]; >> - int ret; >> - >> - if (is_write) { >> - vgic_data_host_to_mmio_bus(buf, len, *val); >> - ret = kvm_io_gic_ops.write(vcpu, &dev->dev, offset, len, buf); >> - } else { >> - ret = kvm_io_gic_ops.read(vcpu, &dev->dev, offset, len, buf); >> - if (!ret) >> - *val = vgic_data_mmio_bus_to_host(buf, len); >> - } >> - >> - return ret; >> -} >> - >> int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write, >> int offset, u32 *val) >> { >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c >> b/virt/kvm/arm/vgic/vgic-mmio-v3.c >> index 0d3c76a..ce2708d 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c >> @@ -209,6 +209,62 @@ static unsigned long vgic_mmio_read_v3_idregs(struct >> kvm_vcpu *vcpu, >> return 0; >> } >> >> +static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len) >> +{ >> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); >> + u32 value = 0; >> + int i; >> + >> + /* >> + * A level triggerred interrupt pending state is latched in both >> + * "soft_pending" and "line_level" variables. Userspace will save >> + * and restore soft_pending and line_level separately. >> + * Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt >> + * handling of ISPENDR and ICPENDR. >> + */ >> + for (i = 0; i < len * 8; i++) { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + >> i); >> + >> + if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending) >> + value |= (1U << i); >> + if (irq->config == VGIC_CONFIG_EDGE && irq->pending) >> + value |= (1U << i); >> + >> + vgic_put_irq(vcpu->kvm, irq); >> + } >> + >> + return value; >> +} >> + >> +static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu, >> + gpa_t addr, unsigned int len, >> + unsigned long val) >> +{ >> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); >> + int i; >> + >> + for (i = 0; i < len * 8; i++) { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + >> i); >> + >> + spin_lock(&irq->irq_lock); >> + if (test_bit(i, &val)) { >> + irq->pending = true; >> + irq->soft_pending = true; >> + vgic_queue_irq_unlock(vcpu->kvm, irq); >> + } else { >> + irq->soft_pending = false; >> + if (irq->config == VGIC_CONFIG_EDGE || >> + (irq->config == VGIC_CONFIG_LEVEL && >> + !irq->line_level)) >> + irq->pending = false; >> + spin_unlock(&irq->irq_lock); >> + } >> + >> + vgic_put_irq(vcpu->kvm, irq); >> + } >> +} >> + >> /* We want to avoid outer shareable. */ >> u64 vgic_sanitise_shareability(u64 field) >>
Re: [PATCH v6 1/7] arm/arm64: vgic-new: Implement support for userspace access
On 20/09/16 07:12, vijay.kil...@gmail.com wrote: > From: Vijaya Kumar K > > Read and write of some registers like ISPENDR and ICPENDR > from userspace requires special handling when compared to > guest access for these registers. > > Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt > for handling of ISPENDR, ICPENDR registers handling. > > Add infrastructure to support guest and userspace read > and write for the required registers > Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c > > Signed-off-by: Vijaya Kumar K > --- > virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 -- > virt/kvm/arm/vgic/vgic-mmio-v3.c | 98 > > virt/kvm/arm/vgic/vgic-mmio.c| 78 > virt/kvm/arm/vgic/vgic-mmio.h| 19 > 4 files changed, 169 insertions(+), 51 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c > b/virt/kvm/arm/vgic/vgic-mmio-v2.c > index b44b359..0b32f40 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c > @@ -406,31 +406,6 @@ int vgic_v2_has_attr_regs(struct kvm_device *dev, struct > kvm_device_attr *attr) > return -ENXIO; > } > > -/* > - * When userland tries to access the VGIC register handlers, we need to > - * create a usable struct vgic_io_device to be passed to the handlers and we > - * have to set up a buffer similar to what would have happened if a guest > MMIO > - * access occurred, including doing endian conversions on BE systems. > - */ > -static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev, > - bool is_write, int offset, u32 *val) > -{ > - unsigned int len = 4; > - u8 buf[4]; > - int ret; > - > - if (is_write) { > - vgic_data_host_to_mmio_bus(buf, len, *val); > - ret = kvm_io_gic_ops.write(vcpu, &dev->dev, offset, len, buf); > - } else { > - ret = kvm_io_gic_ops.read(vcpu, &dev->dev, offset, len, buf); > - if (!ret) > - *val = vgic_data_mmio_bus_to_host(buf, len); > - } > - > - return ret; > -} > - > int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write, > int offset, u32 *val) > { > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c > b/virt/kvm/arm/vgic/vgic-mmio-v3.c > index 0d3c76a..ce2708d 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c > @@ -209,6 +209,62 @@ static unsigned long vgic_mmio_read_v3_idregs(struct > kvm_vcpu *vcpu, > return 0; > } > > +static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len) > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > + u32 value = 0; > + int i; > + > + /* > + * A level triggerred interrupt pending state is latched in both > + * "soft_pending" and "line_level" variables. Userspace will save > + * and restore soft_pending and line_level separately. > + * Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt > + * handling of ISPENDR and ICPENDR. > + */ > + for (i = 0; i < len * 8; i++) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + > + if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending) > + value |= (1U << i); > + if (irq->config == VGIC_CONFIG_EDGE && irq->pending) > + value |= (1U << i); > + > + vgic_put_irq(vcpu->kvm, irq); > + } > + > + return value; > +} > + > +static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu, > + gpa_t addr, unsigned int len, > + unsigned long val) > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > + int i; > + > + for (i = 0; i < len * 8; i++) { > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > + > + spin_lock(&irq->irq_lock); > + if (test_bit(i, &val)) { > + irq->pending = true; > + irq->soft_pending = true; > + vgic_queue_irq_unlock(vcpu->kvm, irq); > + } else { > + irq->soft_pending = false; > + if (irq->config == VGIC_CONFIG_EDGE || > + (irq->config == VGIC_CONFIG_LEVEL && > + !irq->line_level)) > + irq->pending = false; > + spin_unlock(&irq->irq_lock); > + } > + > + vgic_put_irq(vcpu->kvm, irq); > + } > +} > + > /* We want to avoid outer shareable. */ > u64 vgic_sanitise_shareability(u64 field) > { > @@ -358,7 +414,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu > *vcpu, > * We take some special care here to fix the calculation of the register > * offset. > */