On Thu, 30 Nov 2017 17:00:35 +1100
Alexey Kardashevskiy <aik@xxxxxxxxx> wrote:
> On 30/11/17 05:27, Alex Williamson wrote:
> > On Thu, 23 Nov 2017 15:56:26 +1100
> > Alexey Kardashevskiy <aik@xxxxxxxxx> wrote:
> >
> >> It is currently possible to have a sparse capability with 1 areas which
> >> starts at 0 and 0 bytes long. One example is:
> >>
> >> Texas Instruments TUSB73x0 SuperSpeed USB 3.0 xHCI Host Controller
> >> [...]
> >> Region 0: Memory at 3fe280000000 (64-bit, non-prefetchable) [size=64K]
> >> Region 2: Memory at 3fe280010000 (64-bit, non-prefetchable) [size=8K]
> >> [...]
> >> Capabilities: [c0] MSI-X: Enable+ Count=8 Masked-
> >> Vector table: BAR=2 offset=00000000
> >> PBA: BAR=2 offset=00001000
> >>
> >> With PAGE_SIZE=64K, MSIX BAR occupies the entire BAR2 and cannot be
> >> mapped.
> >>
> >> This makes it explicit - if sparse->areas is empty, then advertise
> >> nr_areas as 0.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx>
> >> ---
> >>
> >> QEMU gets it right as vfio_setup_region_sparse_mmaps() checks for size
> >> after QEMU's 24acf72b9a291ce "vfio: Handle zero-length sparse mmap ranges"
> >> but why not make it explicit in the first place?
> >>
> >>
> >> ---
> >> drivers/vfio/pci/vfio_pci.c | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> >> index f041b1a..a201c45 100644
> >> --- a/drivers/vfio/pci/vfio_pci.c
> >> +++ b/drivers/vfio/pci/vfio_pci.c
> >> @@ -597,6 +597,10 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev,
> >> i++;
> >> }
> >>
> >> + /* With all alignments, there are no gaps left to mmap */
> >> + if (i == 0)
> >> + sparse->nr_areas = 0;
> >> +
> >
> > Ok, but why does 0 become a special case? Shouldn't we set
> > sparse->nr_areas = i? Thanks,
>
> This is what is returned to QEMU now - 1 sparse region, starts at 0, 0
> bytes long. I am missing the point in having such region...
I'm asking why your patch is necessary vs:
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index f041b1a6cf66..c062437bbf44 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -582,8 +582,6 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev
if (!sparse)
return -ENOMEM;
- sparse->nr_areas = nr_areas;
-
if (vdev->msix_offset & PAGE_MASK) {
sparse->areas[i].offset = 0;
sparse->areas[i].size = vdev->msix_offset & PAGE_MASK;
@@ -597,6 +595,8 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev
i++;
}
+ sparse->nr_areas = i;
+
ret = vfio_info_add_capability(caps, VFIO_REGION_INFO_CAP_SPARSE_MMAP,
sparse);
kfree(sparse);
And if we do that, then perhaps we don't even need to calculate
nr_areas and we can get rid of more code:
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index f041b1a6cf66..ae0d5a31aa6b 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -566,24 +566,16 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vd
struct vfio_info_cap *caps)
{
struct vfio_region_info_cap_sparse_mmap *sparse;
- size_t end, size;
- int nr_areas = 2, i = 0, ret;
+ size_t end;
+ int i = 0, ret;
end = pci_resource_len(vdev->pdev, vdev->msix_bar);
- /* 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;
-
- size = sizeof(*sparse) + (nr_areas * sizeof(*sparse->areas));
-
- sparse = kzalloc(size, GFP_KERNEL);
+ sparse = kzalloc(sizeof(*sparse) + (2 * sizeof(*sparse->areas)),
+ GFP_KERNEL);
if (!sparse)
return -ENOMEM;
- sparse->nr_areas = nr_areas;
-
if (vdev->msix_offset & PAGE_MASK) {
sparse->areas[i].offset = 0;
sparse->areas[i].size = vdev->msix_offset & PAGE_MASK;
@@ -597,6 +589,8 @@ static int msix_sparse_mmap_cap(struct vfio_pci_device *vdev
i++;
}
+ sparse->nr_areas = i;
+
ret = vfio_info_add_capability(caps, VFIO_REGION_INFO_CAP_SPARSE_MMAP,
sparse);
kfree(sparse);
