[RFC PATCH kernel] vfio-pci: Fix sparse capability when no parts of MSIX BAR can be mapped

[RFC PATCH kernel] vfio-pci: Fix sparse capability when no parts of MSIX BAR can be mapped

[Alexey Kardashevskiy]

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@ozlabs.ru>
---

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;
+
 	ret = vfio_info_add_capability(caps, VFIO_REGION_INFO_CAP_SPARSE_MMAP,
 				       sparse);
 	kfree(sparse);
-- 
2.11.0

Re: [RFC PATCH kernel] vfio-pci: Fix sparse capability when no parts of MSIX BAR can be mapped

[Alex Williamson]

On Thu, 23 Nov 2017 15:56:26 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> 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@ozlabs.ru>
> ---
> 
> 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,

Alex

>  	ret = vfio_info_add_capability(caps, VFIO_REGION_INFO_CAP_SPARSE_MMAP,
>  				       sparse);
>  	kfree(sparse);

Re: [RFC PATCH kernel] vfio-pci: Fix sparse capability when no parts of MSIX BAR can be mapped

[Alexey Kardashevskiy]

On 30/11/17 05:27, Alex Williamson wrote:
> On Thu, 23 Nov 2017 15:56:26 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> 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@ozlabs.ru>
>> ---
>>
>> 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...


> 
> Alex
> 
>>  	ret = vfio_info_add_capability(caps, VFIO_REGION_INFO_CAP_SPARSE_MMAP,
>>  				       sparse);
>>  	kfree(sparse);
> 


-- 
Alexey

Re: [RFC PATCH kernel] vfio-pci: Fix sparse capability when no parts of MSIX BAR can be mapped

[Alex Williamson]

On Thu, 30 Nov 2017 17:00:35 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 30/11/17 05:27, Alex Williamson wrote:
> > On Thu, 23 Nov 2017 15:56:26 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> 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@ozlabs.ru>
> >> ---
> >>
> >> 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);