mbox series

[RFC,v5,0/4] util/vfio-helpers: Add support for multiple IRQs

Message ID 20200908180342.280749-1-philmd@redhat.com
Headers show
Series util/vfio-helpers: Add support for multiple IRQs | expand

Message

Philippe Mathieu-Daudé Sept. 8, 2020, 6:03 p.m. UTC 
This series intends to setup the VFIO helper to allow
binding notifiers on different IRQs.

For the NVMe use case, we only care about MSIX interrupts.
To not disrupt other users, introduce the qemu_vfio_pci_init_msix_irqs
function to initialize multiple MSIX IRQs and attach eventfd to
them.

Since RFC v4:
- addressed Alex review comment:
  check ioctl(VFIO_DEVICE_SET_IRQS) return value

Since RFC v3:
- addressed Alex and Stefan review comments

Since RFC v2:
- new patch to report vfio-helpers is not supported on AA64/POWER

(NVMe block driver series will follow).

Based-on: <20200908115322.325832-1-kwolf@redhat.com>
(Block layer pending pull request)

Philippe Mathieu-Daudé (4):
  util/vfio-helpers: Improve reporting unsupported IOMMU type
  util/vfio-helpers: Report error when IOMMU page size is not supported
  util/vfio-helpers: Introduce qemu_vfio_pci_init_msix_irqs()
  block/nvme: Use qemu_vfio_pci_init_msix_irqs() to initialize our IRQ

 include/qemu/vfio-helpers.h |  6 ++-
 block/nvme.c                |  9 +++-
 util/vfio-helpers.c         | 87 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 97 insertions(+), 5 deletions(-)

Comments

Fam Zheng Sept. 9, 2020, 8:38 a.m. UTC | #1 
On 2020-09-08 20:03, Philippe Mathieu-Daudé wrote:
> This driver uses the host page size to align its memory regions,
> but this size is not always compatible with the IOMMU. Add a
> check if the size matches, and bails out with listing the sizes
> the IOMMU supports.
> 
> Example on Aarch64:
> 
>  $ qemu-system-aarch64 -M virt -drive if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw
>  qemu-system-aarch64: -drive if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw: Unsupported IOMMU page size: 4 KiB
>  Available page size:
>   64 KiB
>   512 MiB
> 
> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  util/vfio-helpers.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 55b4107ce69..6d9ec7d365c 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include <sys/ioctl.h>
>  #include <linux/vfio.h>
>  #include "qapi/error.h"
> @@ -316,6 +317,25 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
>          ret = -errno;
>          goto fail;
>      }
> +    if (!(iommu_info.flags & VFIO_IOMMU_INFO_PGSIZES)) {
> +        error_setg(errp, "Failed to get IOMMU page size info");
> +        ret = -errno;

We don't have errno here, do we?

> +        goto fail;
> +    }
> +    if (!extract64(iommu_info.iova_pgsizes,
> +                   ctz64(qemu_real_host_page_size), 1)) {
> +        g_autofree char *host_page_size = size_to_str(qemu_real_host_page_size);
> +        error_setg(errp, "Unsupported IOMMU page size: %s", host_page_size);
> +        error_append_hint(errp, "Available page size:\n");
> +        for (int i = 0; i < 64; i++) {
> +            if (extract64(iommu_info.iova_pgsizes, i, 1)) {
> +                g_autofree char *iova_pgsizes = size_to_str(1UL << i);
> +                error_append_hint(errp, " %s\n", iova_pgsizes);

Interesting... Since it's printing page size which is fairly low level,
why not just plain (hex) numbers?

Fam

> +            }
> +        }
> +        ret = -EINVAL;
> +        goto fail;
> +    }
>  
>      s->device = ioctl(s->group, VFIO_GROUP_GET_DEVICE_FD, device);
>  
> -- 
> 2.26.2
> 
>
Philippe Mathieu-Daudé Sept. 9, 2020, 2:10 p.m. UTC | #2 
On 9/9/20 10:38 AM, Fam Zheng wrote:
> On 2020-09-08 20:03, Philippe Mathieu-Daudé wrote:
>> This driver uses the host page size to align its memory regions,
>> but this size is not always compatible with the IOMMU. Add a
>> check if the size matches, and bails out with listing the sizes
>> the IOMMU supports.
>>
>> Example on Aarch64:
>>
>>  $ qemu-system-aarch64 -M virt -drive if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw
>>  qemu-system-aarch64: -drive if=none,id=nvme0,file=nvme://0006:90:00.0/1,format=raw: Unsupported IOMMU page size: 4 KiB
>>  Available page size:
>>   64 KiB
>>   512 MiB
>>
>> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  util/vfio-helpers.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
>> index 55b4107ce69..6d9ec7d365c 100644
>> --- a/util/vfio-helpers.c
>> +++ b/util/vfio-helpers.c
>> @@ -11,6 +11,7 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu/cutils.h"
>>  #include <sys/ioctl.h>
>>  #include <linux/vfio.h>
>>  #include "qapi/error.h"
>> @@ -316,6 +317,25 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
>>          ret = -errno;
>>          goto fail;
>>      }
>> +    if (!(iommu_info.flags & VFIO_IOMMU_INFO_PGSIZES)) {
>> +        error_setg(errp, "Failed to get IOMMU page size info");
>> +        ret = -errno;
> 
> We don't have errno here, do we?

Oops thanks, I'll replace by "ret = -EINVAL;".

> 
>> +        goto fail;
>> +    }
>> +    if (!extract64(iommu_info.iova_pgsizes,
>> +                   ctz64(qemu_real_host_page_size), 1)) {
>> +        g_autofree char *host_page_size = size_to_str(qemu_real_host_page_size);
>> +        error_setg(errp, "Unsupported IOMMU page size: %s", host_page_size);
>> +        error_append_hint(errp, "Available page size:\n");
>> +        for (int i = 0; i < 64; i++) {
>> +            if (extract64(iommu_info.iova_pgsizes, i, 1)) {
>> +                g_autofree char *iova_pgsizes = size_to_str(1UL << i);
>> +                error_append_hint(errp, " %s\n", iova_pgsizes);
> 
> Interesting... Since it's printing page size which is fairly low level,
> why not just plain (hex) numbers?

Because this error is reported to the user (not the developer)
and depends of the host/iommu.

If you don't mind I'll keep it as it (as the code is written).

Thank you for reviewing the series!

Phil.

> 
> Fam
> 
>> +            }
>> +        }
>> +        ret = -EINVAL;
>> +        goto fail;
>> +    }
>>  
>>      s->device = ioctl(s->group, VFIO_GROUP_GET_DEVICE_FD, device);
>>  
>> -- 
>> 2.26.2
>>
>>
>