Re: [PATCH] vfio: Simplify capability helper

[Peter Xu <peterx@redhat.com>]

On Wed, Dec 13, 2017 at 08:35:39AM -0700, Alex Williamson wrote:
> On Wed, 13 Dec 2017 16:04:48 +0100
> Auger Eric <eric.auger@redhat.com> wrote:
> 
> > Hi Peter,
> > 
> > On 13/12/17 07:49, Peter Xu wrote:
> > > On Tue, Dec 12, 2017 at 12:59:39PM -0700, Alex Williamson wrote:  
> > >> The vfio_info_add_capability() helper requires the caller to pass a
> > >> capability ID, which it then uses to fill in header fields, assuming
> > >> hard coded versions.  This makes for an awkward and rigid interface.
> > >> The only thing we want this helper to do is allocate sufficient
> > >> space in the caps buffer and chain this capability into the list.
> > >> Reduce it to that simple task.
> > >>
> > >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>  
> > > 
> > > Reviewed-by: Peter Xu <peterx@redhat.com>
> > > 
> > > Though during review I had a question related to the function
> > > msix_sparse_mmap_cap(): Is it possible that one PCI device BAR is very
> > > small (e.g., 4K) that only contains the MSI-X table (and another small
> > > PBA area)?  If so, should we just mark that region unmappable instead
> > > of setting vfio_region_info_cap_sparse_mmap.nr_areas to 1 in
> > > msix_sparse_mmap_cap()?
> > > 
> > > 	/* If MSI-X table is aligned to the start or end, only one area */
> > > 	if (((vdev->msix_offset & PAGE_MASK) == 0) ||
> > > 	    (PAGE_ALIGN(vdev->msix_offset + vdev->msix_size) >= end))
> > > 		nr_areas = 1;
> > > 
> > > Thanks,
> > >   
> > if I understand the code correctly, if the MSI-X table exactly matches
> > the BAR, a sparse mmap region is reported with offset/size = 0. Is that
> > correct?
> 
> Yes, and that was a compatibility choice when the sparse mmap was
> added, retaining the per region mmap flag, but essentially excluding
> the whole area with the sparse mmap.  It seemed like it'd be easier for
> userspace to understand the distinction.

I see.

> Now we're trying to remove
> the whole mess and allow mmaps covering the MSI-X vector table because
> it's a performance killer for systems where the page size is >4K.

Yeah, I just noticed that.  Thanks for explaining!

-- 
Peter Xu