Hi Vijaya,
On 15/12/2016 08:36, Vijay Kilari wrote:
> On Tue, Dec 6, 2016 at 5:12 PM, Auger Eric <eric.auger@xxxxxxxxxx> wrote:
>> Hi,
>>
>> On 28/11/2016 14:05, Christoffer Dall wrote:
>>> On Wed, Nov 23, 2016 at 06:31:48PM +0530, vijay.kilari@xxxxxxxxx wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
