* [RFC PATCH kernel] vfio-pci: Fix sparse capability when no parts of MSIX BAR can be mapped
@ 2017-11-23 4:56 Alexey Kardashevskiy
2017-11-29 18:27 ` Alex Williamson
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Kardashevskiy @ 2017-11-23 4:56 UTC (permalink / raw)
To: kvm; +Cc: Alexey Kardashevskiy, Alex Williamson
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH kernel] vfio-pci: Fix sparse capability when no parts of MSIX BAR can be mapped
2017-11-23 4:56 [RFC PATCH kernel] vfio-pci: Fix sparse capability when no parts of MSIX BAR can be mapped Alexey Kardashevskiy
@ 2017-11-29 18:27 ` Alex Williamson
2017-11-30 6:00 ` Alexey Kardashevskiy
0 siblings, 1 reply; 4+ messages in thread
From: Alex Williamson @ 2017-11-29 18:27 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: kvm
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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH kernel] vfio-pci: Fix sparse capability when no parts of MSIX BAR can be mapped
2017-11-29 18:27 ` Alex Williamson
@ 2017-11-30 6:00 ` Alexey Kardashevskiy
2017-11-30 15:47 ` Alex Williamson
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Kardashevskiy @ 2017-11-30 6:00 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH kernel] vfio-pci: Fix sparse capability when no parts of MSIX BAR can be mapped
2017-11-30 6:00 ` Alexey Kardashevskiy
@ 2017-11-30 15:47 ` Alex Williamson
0 siblings, 0 replies; 4+ messages in thread
From: Alex Williamson @ 2017-11-30 15:47 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: kvm
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);
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-11-30 15:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-23 4:56 [RFC PATCH kernel] vfio-pci: Fix sparse capability when no parts of MSIX BAR can be mapped Alexey Kardashevskiy
2017-11-29 18:27 ` Alex Williamson
2017-11-30 6:00 ` Alexey Kardashevskiy
2017-11-30 15:47 ` Alex Williamson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.