[PATCH v9 01/11] arm/arm64: vgic: Implement support for userspace access

Auger Eric eric.auger at redhat.com
Fri Dec 16 04:40:14 PST 2016


Hi Vijaya,

On 15/12/2016 08:36, Vijay Kilari wrote:
> On Tue, Dec 6, 2016 at 5:12 PM, Auger Eric <eric.auger at redhat.com> wrote:
>> Hi,
>>
>> On 28/11/2016 14:05, Christoffer Dall wrote:
>>> On Wed, Nov 23, 2016 at 06:31:48PM +0530, vijay.kilari at gmail.com wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar at 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.Kumar at 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

OK. Sorry I missed that. Other comments I sent on v9 remain applicable
on v10 I think.

Thanks

Eric
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 



More information about the linux-arm-kernel mailing list