Re: [Qemu-devel] [RFC v2 4/6] vfio: Add initial IRQ support in QEMU platform device

[Kim Phillips]

On Wed, 16 Apr 2014 15:29:35 +0200
Eric Auger <address@hidden> wrote:

> On 04/11/2014 03:34 AM, Kim Phillips wrote:
> > On Wed, 9 Apr 2014 16:33:07 +0100
> > Eric Auger <address@hidden> wrote:
> >> @@ -108,12 +108,13 @@ static const MemMapEntry a15memmap[] = {
> >>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that 
> >> size */
> >>      /* 0x10000000 .. 0x40000000 reserved for PCI */
> >>      [VIRT_MEM] = { 0x40000000, 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