Re: [RFC PATCH v3 2/5] util/vfio-helpers: Report error on unsupported host architectures

[Philippe Mathieu-Daudé]

On 8/18/20 7:12 PM, Alex Williamson wrote:
> On Tue, 18 Aug 2020 18:45:06 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> The vfio-helpers implementation expects a TYPEv1 IOMMU, see
>> qemu_vfio_init_pci:
>>
>>   263     if (!ioctl(s->container, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU)) {
>>   264         error_setg_errno(errp, errno, "VFIO IOMMU check failed");
>>
>> Thus POWER SPAPR IOMMU is obviously not supported.
>>
>> The implementation only cares about host page size alignment
>> (usually 4KB on X86), not the IOMMU one, which is be problematic
>> on Aarch64, when 64MB page size is used. So Aarch64 is not
>> supported neither.
>>
>> Report an error when the host architecture is different than X86:
>>
>>  $ qemu-system-aarch64 \
>>     -drive file=nvme://0001:01:00.0/1,if=none,id=drive0 \
>>     -device virtio-blk-pci,drive=drive0
>>   qemu-system-aarch64: -drive file=nvme://0001:01:00.0/1,if=none,id=drive0: 
>> QEMU VFIO utility is not supported on this architecture
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Cc: Eric Auger <eric.auger@redhat.com>
>> Cc: Drew Jones <drjones@redhat.com>
>> Cc: Laurent Vivier <lvivier@redhat.com>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  util/vfio-helpers.c | 26 +++++++++++++++++++++++++-
>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
>> index e399e330e26..60017936e3e 100644
>> --- a/util/vfio-helpers.c
>> +++ b/util/vfio-helpers.c
>> @@ -420,14 +420,38 @@ static void qemu_vfio_open_common(QEMUVFIOState *s)
>>      qemu_ram_foreach_block(qemu_vfio_init_ramblock, s);
>>  }
>>  
>> +/**
>> + * Return if the host architecture is supported.
>> + *
>> + * aarch64: IOMMU page alignment not respected
>> + * ppc64:   SPAPR IOMMU window not configured
>> + * x86-64:  Only architecture validated
>> + * other:   Untested
>> + */
>> +static bool qemu_vfio_arch_supported(void)
>> +{
>> +    bool supported = false;
>> +
>> +#if defined(HOST_X86_64)
>> +    supported = true;
>> +#endif
>> +
>> +    return supported;
>> +}
> 
> Why does this need to be hard coded to specific architectures rather
> than probing for type1 IOMMU support and looking at the iova_pgsizes
> from VFIO_IOMMU_GET_INFO to see if there's a compatible size?  It
> requires us to get a bit deeper into the device initialization, but we
> should still be able to unwind out of the device realize.  Otherwise
> we're throwing out aarch64 running of 4KB for no reason, right?  Thanks,

Ah yes, much clever! Thanks Alex :)

> 
> Alex
> 
> 
>>  /**
>>   * Open a PCI device, e.g. "0000:00:01.0".
>>   */
>>  QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp)
>>  {
>>      int r;
>> -    QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
>> +    QEMUVFIOState *s;
>>  
>> +    if (!qemu_vfio_arch_supported()) {
>> +        error_setg(errp,
>> +                   "QEMU VFIO utility is not supported on this 
>> architecture");
>> +        return NULL;
>> +    }
>> +    s = g_new0(QEMUVFIOState, 1);
>>      r = qemu_vfio_init_pci(s, device, errp);
>>      if (r) {
>>          g_free(s);
>