Re: [Qemu-devel] [PATCH v2 3/4] vfio/quirks: ioeventfd quirk acceleration

[Alex Williamson]

On Thu, 3 May 2018 16:33:25 +0200
Auger Eric <address@hidden> wrote:
> > +    if (!vdev->no_kvm_ioeventfd &&
> > +        addr > PCI_STD_HEADER_SIZEOF && last->added < MAX_DYN_IOEVENTFD + 
> > 1) {  
> nit: <= MAX_DYN_IOEVENTFD?

Done, also the addr test should be >=, it doesn't make sense to exclude
the first byte after the standard capabilities, though for this quirk
we know we're looking well above this.

> > +        if (addr != last->addr || data != last->data || size != 
> > last->size) {
> > +            last->addr = addr;
> > +            last->data = data;
> > +            last->size = size;
> > +            last->hits = 1;
> > +        } else if (++last->hits >= HITS_FOR_IOEVENTFD) {
> > +            if (last->added < MAX_DYN_IOEVENTFD) {
> > +                VFIOIOEventFD *ioeventfd;
> > +                ioeventfd = vfio_ioeventfd_init(vdev, mirror->mem, addr, 
> > size,
> > +                                        data, 
> > &vdev->bars[mirror->bar].region,
> > +                                        mirror->offset + addr, true);
> > +                if (ioeventfd) {
> > +                    VFIOQuirk *quirk;
> > +
> > +                    QLIST_FOREACH(quirk,
> > +                                  &vdev->bars[mirror->bar].quirks, next) {
> > +                        if (quirk->data == mirror) {
> > +                            QLIST_INSERT_HEAD(&quirk->ioeventfds,
> > +                                              ioeventfd, next);
> > +                            break;
> > +                        }
> > +                    }
> > +
> > +                    assert(quirk != NULL); /* Check not found */
> > +
> > +                    last->added++;
> > +                }
> > +            } else {
> > +                last->added++;
> > +
> > +                error_report("NVIDIA ioeventfd queue full for %s, unable 
> > to "
> > +                             "accelerate 0x%"HWADDR_PRIx", data 
> > 0x%"PRIx64", "
> > +                             "size %u", vdev->vbasedev.name, addr, data, 
> > size);  
> nit: warn_report?

Done

... 
> > +typedef struct VFIOIOEventFD {
> > +    QLIST_ENTRY(VFIOIOEventFD) next;
> > +    MemoryRegion *mr;
> > +    hwaddr addr;
> > +    unsigned size;
> > +    uint64_t data;
> > +    EventNotifier e;
> > +    VFIORegion *region;
> > +    hwaddr region_addr;
> > +    bool match_data;
> > +    bool dynamic;  
> maybe the "dynamic" semantics may be docuemnted in the code and not only
> in the commit message.

Done, added

/* Added runtime, removed on device reset */

Thanks!

Alex