On Wed, 16 Apr 2014 15:29:35 +0200
Eric Auger <eric.auger@xxxxxxxxxx> wrote:
> On 04/11/2014 03:34 AM, Kim Phillips wrote:
> > On Wed, 9 Apr 2014 16:33:07 +0100
> > Eric Auger <eric.auger@xxxxxxxxxx> 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
