On Tue, Dec 6, 2016 at 5:12 PM, Auger Eric <eric.au...@redhat.com> wrote: > Hi, > > On 28/11/2016 14:05, Christoffer Dall wrote: >> On Wed, Nov 23, 2016 at 06:31:48PM +0530, vijay.kil...@gmail.com wrote: >>> From: Vijaya Kumar K <vijaya.ku...@cavium.com> >>> >>> 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 <vijaya.ku...@cavium.com> >>> --- >>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 ---------- >>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 102 >>> ++++++++++++++++++++++++++++++++------- >>> virt/kvm/arm/vgic/vgic-mmio.c | 78 +++++++++++++++++++++++++++--- >>> virt/kvm/arm/vgic/vgic-mmio.h | 19 ++++++++ >>> 4 files changed, 175 insertions(+), 49 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 50f42f0..8e76d04 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c >>> @@ -207,6 +207,66 @@ 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)) { >>> + /* soft_pending is set irrespective of irq type >>> + * (level or edge) to avoid dependency that VM should >>> + * restore irq config before pending info. >>> + */ >> >> nit: kernel commenting style >> >>> + 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; > I am confused by the comment above. Since we test the irq config here > don't we assume the config was restored before the pending state? >>> + spin_unlock(&irq->irq_lock); >>> + } >>> + >>> + vgic_put_irq(vcpu->kvm, irq); >>> + } >>> +} >>> + >>> /* We want to avoid outer shareable. */ >>> u64 vgic_sanitise_shareability(u64 field) >>> { >>> @@ -356,7 +416,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. >>> */ >>> -#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, bpi, acc) >>> \ >>> +#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(off, rd, wr, ur, uw, bpi, >>> acc) \ >>> { \ >>> .reg_offset = off, \ >>> .bits_per_irq = bpi, \ >>> @@ -371,6 +431,8 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu >>> *vcpu, >>> .access_flags = acc, \ >>> .read = rd, \ >>> .write = wr, \ >>> + .uaccess_read = ur, \ >>> + .uaccess_write = uw, \ >>> } >>> >>> static const struct vgic_register_region vgic_v3_dist_registers[] = { >>> @@ -378,40 +440,42 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu >>> *vcpu, >>> vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16, >>> VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR, >>> - vgic_mmio_read_rao, vgic_mmio_write_wi, 1, >>> + vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1, >>> VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER, >>> - vgic_mmio_read_enable, vgic_mmio_write_senable, 1, >>> + vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1, >>> VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICENABLER, >>> - vgic_mmio_read_enable, vgic_mmio_write_cenable, 1, >>> + vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1, >>> VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR, >>> - vgic_mmio_read_pending, vgic_mmio_write_spending, 1, >>> + vgic_mmio_read_pending, vgic_mmio_write_spending, >>> + vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1, >>> VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR, >>> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 1, >>> + vgic_mmio_read_pending, vgic_mmio_write_cpending, >>> + vgic_mmio_read_raz, vgic_mmio_write_wi, 1, >>> VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER, >>> - vgic_mmio_read_active, vgic_mmio_write_sactive, 1, >>> + vgic_mmio_read_active, vgic_mmio_write_sactive, NULL, NULL, 1, >>> VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICACTIVER, >>> - vgic_mmio_read_active, vgic_mmio_write_cactive, 1, >>> + vgic_mmio_read_active, vgic_mmio_write_cactive, NULL, NULL, 1, >>> VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IPRIORITYR, >>> - vgic_mmio_read_priority, vgic_mmio_write_priority, 8, >>> - VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), >>> + vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL, >>> + 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ITARGETSR, >>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 8, >>> + vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8, >>> VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICFGR, >>> - vgic_mmio_read_config, vgic_mmio_write_config, 2, >>> + vgic_mmio_read_config, vgic_mmio_write_config, NULL, NULL, 2, >>> VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGRPMODR, >>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, >>> + vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, >>> VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IROUTER, >>> - vgic_mmio_read_irouter, vgic_mmio_write_irouter, 64, >>> + vgic_mmio_read_irouter, vgic_mmio_write_irouter, NULL, NULL, >>> 64, >>> VGIC_ACCESS_64bit | VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_LENGTH(GICD_IDREGS, >>> vgic_mmio_read_v3_idregs, vgic_mmio_write_wi, 48, >>> @@ -449,11 +513,13 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu >>> *vcpu, >>> REGISTER_DESC_WITH_LENGTH(GICR_ICENABLER0, >>> vgic_mmio_read_enable, vgic_mmio_write_cenable, 4, >>> VGIC_ACCESS_32bit), >>> - REGISTER_DESC_WITH_LENGTH(GICR_ISPENDR0, >>> - vgic_mmio_read_pending, vgic_mmio_write_spending, 4, >>> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ISPENDR0, >>> + vgic_mmio_read_pending, vgic_mmio_write_spending, >>> + vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4, >>> VGIC_ACCESS_32bit), >>> - REGISTER_DESC_WITH_LENGTH(GICR_ICPENDR0, >>> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 4, >>> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ICPENDR0, >>> + vgic_mmio_read_pending, vgic_mmio_write_cpending, >>> + vgic_mmio_read_raz, vgic_mmio_write_wi, 4, >>> VGIC_ACCESS_32bit), >>> REGISTER_DESC_WITH_LENGTH(GICR_ISACTIVER0, >>> vgic_mmio_read_active, vgic_mmio_write_sactive, 4, >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >>> index ebe1b9f..d5f3ee2 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >>> @@ -484,6 +484,74 @@ static bool check_region(const struct kvm *kvm, >>> return false; >>> } >>> >>> +static const struct vgic_register_region * >>> +vgic_get_mmio_region(struct kvm_vcpu *vcpu, struct vgic_io_device *iodev, >>> + gpa_t addr, int len) >>> +{ >>> + const struct vgic_register_region *region; >>> + >>> + region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions, >>> + addr - iodev->base_addr); >>> + if (!region || !check_region(vcpu->kvm, region, addr, len)) >>> + return NULL; >>> + >>> + return region; >>> +} >>> + >>> +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(vcpu, iodev, addr, sizeof(u32)); >>> + if (!region) { >>> + *val = 0; >>> + return 0; > do we really want to return 0 here? -ENXIO? > I see that dispatch_mmio_read/write return 0 in that case but I don't > see any reason either? Other kvm_io_device_ops seem to return > -EOPNOTSUPP in such a case.
Yes, This was discussed and decided to fix it outside of this series. https://www.spinics.net/lists/arm-kernel/msg533695.html _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm