Re: [Qemu-devel] [RFC v2 4/6] vfio: Add initial IRQ support in QEMU platform device
On Wed, 16 Apr 2014 15:29:35 +0200
Eric Auger wrote:
> On 04/11/2014 03:34 AM, Kim Phillips wrote:
> > On Wed, 9 Apr 2014 16:33:07 +0100
> > Eric Auger wrote:
> >> @@ -108,12 +108,13 @@ static const MemMapEntry a15memmap[] = {
> >> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that
> >> size */
> >> /* 0x1000 .. 0x4000 reserved for PCI */
> >> [VIRT_MEM] = { 0x4000, 1ULL * 1024 * 1024 * 1024 },
> >> -[VIRT_ETHERNET] = { 0xfff51000, 0x1000 },
> >> +[VIRT_ETHERNET] = { 0xfff41000, 0x1000 },
> >
> > this change isn't explained (the device is at physical 0xfff51000,
> > not 0xfff41000), and looks like it belongs in the first patch of the
> > series anyway. Note: see e.g., commit f5fdcd6e5 "hw/arm: Add
> > 'virt' platform" for an example of how to re-compose commit message
> > text for patches that undergo a change of author.
> >
> Hi Kim,
Hi Eric,
> I acknowledge the change is not justified in the context of IRQ support
> introduction. I will remove it.
> I changed this because the address used in the prior patch was
> misleading to me, as I reported in one comment. Indeed 0xFFF51000 is the
> host physical address of the device but here we specify the guest
> physical address which in general does not relate at all with the host
> physical address.
I see there's churn in other threads in this area...good.
> >> +char *name;
> >> +uint32_t mmap_timeout; /* mmap timeout value in ms */
> >> +VFIORegion regions[PLATFORM_NUM_REGIONS];
> >
> > this is a regression over the last version of the third patch I sent
> > you; 'regions' should remain dynamically allocated - here, they're
> > being fixed.
> OK I did that way to reuse
> vfio_irq_eoi(container_of(region, VFIODevice, regions[region->nr]))
> in vfio_region_write/read.
>
> But this definitively can be improved.
it has to, in order to support a variable number of regions. Maybe
use the VFIODevice itself as the opaque pointer instead of 'fd' in
struct VFIORegion?
> >> +/* TODO: fix this number of regions,
> >> + * currently a single region is handled
> >> + */
> >
> > please do :)
> Can't we image to have a separate patch for this story of number of
> regions, overall?
well I'd expect the next revision of this series to blend better
with the the way it was done in "vfio: add vfio-platform
support" (the existing 3rd patch), i.e., add interrupt support
without breaking support for a variable number of regions.
> >> +static void vfio_irq_eoi(VFIODevice *vdev)
> >> +{
> >> +
> >> +VFIOINTp *intp;
> >> +bool eoi_done = false;
> >> +
> >> +QLIST_FOREACH(intp, &vdev->intp_list, next) {
> >> +if (intp->pending) {
> >> +if (eoi_done) {
> >> +DPRINTF("several IRQ pending simultaneously: \
> >> + this is not a supported case yet\n");
> >> +}
> >
> > If the thing to do in this case is not remap MMIO to the 'fast
> > path', then we should do that. Otherwise, I think I'd protect this
> > with DEBUG in the meantime..
> I did not understand that comment
As it stands, that code is only DPRINTF'ing in the irq->pending
case, and it's adding a linked list to the VFIOINT struct which
otherwise is equal to that of the vfio-pci implementation. All we
need to do in the irq pending case is *not* switch back to the 'fast
path' (direct MMIO access, ie., still use vfio_region_{read,write}
()), right? In which case, all we may need is an interrupt pending
counter in the VFIODevice struct, that this code should check.
Looking at the code again, I noticed vfio_disable_irqindex(),
vfio_unmask_int{x,p}() are almost verbatim copies - can they be
merged into common.c? I also noticed the platform code doesn't have
a vfio_mask_intx() equivalent but can't tell if it needs it ATM.
> BR
>
> Eric
Thanks Eric,
Kim
Re: [Qemu-devel] [RFC v2 4/6] vfio: Add initial IRQ support in QEMU platform device
On 04/11/2014 03:34 AM, Kim Phillips wrote:
> On Wed, 9 Apr 2014 16:33:07 +0100
> Eric Auger wrote:
>
>> This work is inspired of PCI INTx. The code was prepared to support
>> multiple IRQs but this was not tested at that stage. Similarly to what
>> is done on PCI, the device register space is RAM unmapped on IRQ hit
>> in order to trap the end of interrupt (EOI). On mmap timer hit, if the
>> EOI was trapped, the mmapping is restored. the physical interrupt is
>> unmasked on the first read/write with the MMIO register space.
>>
>> Tested on Calxeda Midway xgmac which can be directly assigned to one
>> virt VM.
>>
>> Acknowledgements:
>> - vfio device name, guest physical address and IRQ indexes are
>> hardcoded. This will be improved in another patch
>> - currently tested on a single IRQ (xgmac main IRQ)
>> - a KVM patch is required to invalidate stage2 entries on RAM memory
>> region destruction (see [PATCH] ARM: KVM: Handle IPA unmapping on
>> memory region deletion)
>>
>> Signed-off-by: Eric Auger
>> ---
>
> thanks for this, Eric.
>
>> hw/arm/virt.c | 13 ++-
>> hw/vfio/platform.c | 316
>> +
>> 2 files changed, 304 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 5d43cf0..31ae7d2 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -108,12 +108,13 @@ static const MemMapEntry a15memmap[] = {
>> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size
>> */
>> /* 0x1000 .. 0x4000 reserved for PCI */
>> [VIRT_MEM] = { 0x4000, 1ULL * 1024 * 1024 * 1024 },
>> -[VIRT_ETHERNET] = { 0xfff51000, 0x1000 },
>> +[VIRT_ETHERNET] = { 0xfff41000, 0x1000 },
>
> this change isn't explained (the device is at physical 0xfff51000,
> not 0xfff41000), and looks like it belongs in the first patch of the
> series anyway. Note: see e.g., commit f5fdcd6e5 "hw/arm: Add
> 'virt' platform" for an example of how to re-compose commit message
> text for patches that undergo a change of author.
>
Hi Kim,
I acknowledge the change is not justified in the context of IRQ support
introduction. I will remove it.
I changed this because the address used in the prior patch was
misleading to me, as I reported in one comment. Indeed 0xFFF51000 is the
host physical address of the device but here we specify the guest
physical address which in general does not relate at all with the host
physical address.
>> };
>>
>> static const int a15irqmap[] = {
>> [VIRT_UART] = 1,
>> [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>> +[VIRT_ETHERNET] = 77,
>> };
>>
>> static VirtBoardInfo machines[] = {
>> @@ -299,8 +300,12 @@ static void create_ethernet(const VirtBoardInfo *vbi,
>> qemu_irq *pic)
>> hwaddr base = vbi->memmap[VIRT_ETHERNET].base;
>> hwaddr size = vbi->memmap[VIRT_ETHERNET].size;
>> const char compat[] = "calxeda,hb-xgmac";
>> +int irqm = vbi->irqmap[VIRT_ETHERNET];
>> +int irqp = irqm+1;
>> +int irqlp = irqm+2;
>>
>> -sysbus_create_simple("vfio-platform", base, NULL);
>> +sysbus_create_varargs("vfio-platform", base,
>> + pic[irqm], pic[irqp], pic[irqlp], NULL);
>>
>> nodename = g_strdup_printf("/ethernet@%" PRIx64, base);
>> qemu_fdt_add_subnode(vbi->fdt, nodename);
>> @@ -308,6 +313,10 @@ static void create_ethernet(const VirtBoardInfo *vbi,
>> qemu_irq *pic)
>> /* Note that we can't use setprop_string because of the embedded NUL */
>> qemu_fdt_setprop(vbi->fdt, nodename, "compatible", compat,
>> sizeof(compat));
>> qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg", 2, base, 2,
>> size);
>> +qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupts",
>> +0x0, irqm, 0x4,
>> +0x0, irqp, 0x4,
>> +0x0, irqlp, 0x4);
>
> fwiw, it would have been nice to reuse the irq variable names used
> in hw/net/xgmac.c, but whatever...
OK, I used irqM for Main, irqP for Power and irqLP for Low Power. The
driver only uses 2 of them: main and power. I can rename. Anyway this
code is bound to disappear. See next patch.
>
>> g_free(nodename);
>> }
>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>> index 138fb13..f148edd 100644
>> --- a/hw/vfio/platform.c
>> +++ b/hw/vfio/platform.c
>> @@ -24,6 +24,7 @@
>> #include "config.h"
>> #include "exec/address-spaces.h"
>> #include "exec/memory.h"
>> +
>> #include "qemu-common.h"
>> #include "qemu/error-report.h"
>> #include "qemu/event_notifier.h"
>> @@ -31,6 +32,7 @@
>> #include "qemu/range.h"
>> #include "sysemu/kvm.h"
>> #include "sysemu/sysemu.h"
>> +
>> #include "hw/qdev-properties.h"
>> #include "migration/vmstate.h"
>> #include "hw/hw.h"
>> @@ -41,12 +43,15 @@
>> #define DEBUG_VFIO
>> #ifdef DEBUG_VFIO
>> #define DPRINTF(fmt, ...) \
>> -do { fprintf(stderr, "vfio: " fmt, ## __VA_ARGS_
Re: [Qemu-devel] [RFC v2 4/6] vfio: Add initial IRQ support in QEMU platform device
On Wed, 9 Apr 2014 16:33:07 +0100
Eric Auger wrote:
> This work is inspired of PCI INTx. The code was prepared to support
> multiple IRQs but this was not tested at that stage. Similarly to what
> is done on PCI, the device register space is RAM unmapped on IRQ hit
> in order to trap the end of interrupt (EOI). On mmap timer hit, if the
> EOI was trapped, the mmapping is restored. the physical interrupt is
> unmasked on the first read/write with the MMIO register space.
>
> Tested on Calxeda Midway xgmac which can be directly assigned to one
> virt VM.
>
> Acknowledgements:
> - vfio device name, guest physical address and IRQ indexes are
> hardcoded. This will be improved in another patch
> - currently tested on a single IRQ (xgmac main IRQ)
> - a KVM patch is required to invalidate stage2 entries on RAM memory
> region destruction (see [PATCH] ARM: KVM: Handle IPA unmapping on
> memory region deletion)
>
> Signed-off-by: Eric Auger
> ---
thanks for this, Eric.
> hw/arm/virt.c | 13 ++-
> hw/vfio/platform.c | 316
> +
> 2 files changed, 304 insertions(+), 25 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 5d43cf0..31ae7d2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -108,12 +108,13 @@ static const MemMapEntry a15memmap[] = {
> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size
> */
> /* 0x1000 .. 0x4000 reserved for PCI */
> [VIRT_MEM] = { 0x4000, 1ULL * 1024 * 1024 * 1024 },
> -[VIRT_ETHERNET] = { 0xfff51000, 0x1000 },
> +[VIRT_ETHERNET] = { 0xfff41000, 0x1000 },
this change isn't explained (the device is at physical 0xfff51000,
not 0xfff41000), and looks like it belongs in the first patch of the
series anyway. Note: see e.g., commit f5fdcd6e5 "hw/arm: Add
'virt' platform" for an example of how to re-compose commit message
text for patches that undergo a change of author.
> };
>
> static const int a15irqmap[] = {
> [VIRT_UART] = 1,
> [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
> +[VIRT_ETHERNET] = 77,
> };
>
> static VirtBoardInfo machines[] = {
> @@ -299,8 +300,12 @@ static void create_ethernet(const VirtBoardInfo *vbi,
> qemu_irq *pic)
> hwaddr base = vbi->memmap[VIRT_ETHERNET].base;
> hwaddr size = vbi->memmap[VIRT_ETHERNET].size;
> const char compat[] = "calxeda,hb-xgmac";
> +int irqm = vbi->irqmap[VIRT_ETHERNET];
> +int irqp = irqm+1;
> +int irqlp = irqm+2;
>
> -sysbus_create_simple("vfio-platform", base, NULL);
> +sysbus_create_varargs("vfio-platform", base,
> + pic[irqm], pic[irqp], pic[irqlp], NULL);
>
> nodename = g_strdup_printf("/ethernet@%" PRIx64, base);
> qemu_fdt_add_subnode(vbi->fdt, nodename);
> @@ -308,6 +313,10 @@ static void create_ethernet(const VirtBoardInfo *vbi,
> qemu_irq *pic)
> /* Note that we can't use setprop_string because of the embedded NUL */
> qemu_fdt_setprop(vbi->fdt, nodename, "compatible", compat,
> sizeof(compat));
> qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg", 2, base, 2,
> size);
> +qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupts",
> +0x0, irqm, 0x4,
> +0x0, irqp, 0x4,
> +0x0, irqlp, 0x4);
fwiw, it would have been nice to reuse the irq variable names used
in hw/net/xgmac.c, but whatever...
> g_free(nodename);
> }
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index 138fb13..f148edd 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -24,6 +24,7 @@
> #include "config.h"
> #include "exec/address-spaces.h"
> #include "exec/memory.h"
> +
> #include "qemu-common.h"
> #include "qemu/error-report.h"
> #include "qemu/event_notifier.h"
> @@ -31,6 +32,7 @@
> #include "qemu/range.h"
> #include "sysemu/kvm.h"
> #include "sysemu/sysemu.h"
> +
> #include "hw/qdev-properties.h"
> #include "migration/vmstate.h"
> #include "hw/hw.h"
> @@ -41,12 +43,15 @@
> #define DEBUG_VFIO
> #ifdef DEBUG_VFIO
> #define DPRINTF(fmt, ...) \
> -do { fprintf(stderr, "vfio: " fmt, ## __VA_ARGS__); } while (0)
> +do { fprintf(stderr, "vfio: %s: " fmt, __func__, ## __VA_ARGS__); } \
> +while (0)
these changes are unrelated to the subject of this "add IRQ" patch -
either make them their own patch (in between the last one and this
one), or merge them with the prior patch (that introduces the code),
but since their nature starts to drift from the corresponding PCI
code counterpart, I can't help think why they shouldn't be applied
there, too. Probably best dropping them for now - these and
cleaning up more of the superfluous DPRINTFs added in this series.
> #else
> #define DPRINTF(fmt, ...) \
> do { } while (0)
> #endif
>
> +#define PLATFORM_NUM_REGIONS 10
> +
this is a regression from the third patch in this series - see
