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

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 3 May 2018 11:36:35 +0800
Peter Xu <peterx@redhat.com> wrote:

> On Tue, May 01, 2018 at 10:43:32AM -0600, Alex Williamson wrote:
> 
> [...]
> 
> > @@ -743,6 +843,60 @@ static void vfio_nvidia_quirk_mirror_write(void *opaque, hwaddr addr,
> >                            addr + mirror->offset, data, size);
> >          trace_vfio_quirk_nvidia_bar0_msi_ack(vdev->vbasedev.name);
> >      }
> > +
> > +    /*
> > +     * Automatically add an ioeventfd to handle any repeated write with the
> > +     * same data and size above the standard PCI config space header.  This is
> > +     * primarily expected to accelerate the MSI-ACK behavior, such as noted
> > +     * above.  Current hardware/drivers should trigger an ioeventfd at config
> > +     * offset 0x704 (region offset 0x88704), with data 0x0, size 4.
> > +     *
> > +     * The criteria of 10 successive hits is arbitrary but reliably adds the
> > +     * MSI-ACK region.  Note that as some writes are bypassed via the ioeventfd,
> > +     * the remaining ones have a greater chance of being seen successively.
> > +     * To avoid the pathological case of burning up all of QEMU's open file
> > +     * handles, arbitrarily limit this algorithm from adding no more than 10
> > +     * ioeventfds, print an error if we would have added an 11th, and then
> > +     * stop counting.
> > +     */
> > +    if (!vdev->no_kvm_ioeventfd &&
> > +        addr > PCI_STD_HEADER_SIZEOF && last->added < MAX_DYN_IOEVENTFD + 1) {
> > +        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) {  
> 
> I'm not sure whether the quirks list can be a long one, otherwise not
> sure whether we can just cache the quirk pointer inside the mirror to
> avoid the loop.

The list here only has two entries, but it does seem simple enough to
add a VFIOQuirk* to LastData which we set when it's allocated to avoid
this loop.  I'll test and post and update tomorrow.
 
> > +                        if (quirk->data == mirror) {
> > +                            QLIST_INSERT_HEAD(&quirk->ioeventfds,
> > +                                              ioeventfd, next);
> > +                            break;
> > +                        }
> > +                    }  
> 
> [...]
> 
> > +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;  
> 
> Would it possible in the future that match_data will be false?
> Otherwise I'm not sure whether we can even omit this field.

I don't see how we could, and you're right, it's pretty useless.  I'll
drop it in the next version.  Thanks!

Alex