Date: Wed, 13 Dec 2017 08:35:39 -0700
From: Alex Williamson <alex.williamson@redhat.com>
To: Auger Eric <eric.auger@redhat.com>
Cc: Peter Xu <peterx@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, intel-gvt-dev@lists.freedesktop.org,
        aik@ozlabs.ru, kwankhede@nvidia.com, zhenyuw@linux.intel.com,
        zhi.a.wang@intel.com
Subject: Re: [PATCH] vfio: Simplify capability helper
Message-ID: <20171213083539.768ed23f@t450s.home>
In-Reply-To: <0bb4e51a-6fd2-e946-8927-b8d9fe491b07@redhat.com>
References: <20171212195850.13691.40496.stgit@gimli.home>
        <20171213064936.GC7780@xz-mi>
        <0bb4e51a-6fd2-e946-8927-b8d9fe491b07@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1934
Lines: 45

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.  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.
Thanks,

Alex