vfio/iommufd: no hw_info for mediated devices

[PATCH] vfio/iommufd: no hw_info for mediated devices

Posted by Steve Sistare 2 months, 2 weeks ago

IOMMU_GET_HW_INFO fails for a vfio mediated device (aka mdev), because
the devid is associated with kernel type IOMMUFD_OBJ_ACCESS, not
IOMMUFD_OBJ_DEVICE.  Assume IOMMU_HW_INFO_TYPE_NONE and proceed.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/vfio/iommufd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index cb7257b..d8928d4 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -635,6 +635,7 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
 static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
                                       Error **errp)
 {
+    Error *err = NULL;
     VFIODevice *vdev = opaque;
     HostIOMMUDeviceCaps *caps = &hiod->caps;
     enum iommu_hw_info_type type;
@@ -645,8 +646,9 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
     hiod->agent = opaque;
 
     if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
-                                         &type, &data, sizeof(data), errp)) {
-        return false;
+                                         &type, &data, sizeof(data), &err)) {
+        warn_report_err(err);
+        type = IOMMU_HW_INFO_TYPE_NONE;
     }
 
     hiod->name = g_strdup(vdev->name);
-- 
1.8.3.1

Re: [PATCH] vfio/iommufd: no hw_info for mediated devices

Posted by Joao Martins 2 months, 2 weeks ago

On 10/09/2024 18:40, Steve Sistare wrote:
> IOMMU_GET_HW_INFO fails for a vfio mediated device (aka mdev), because
> the devid is associated with kernel type IOMMUFD_OBJ_ACCESS, not
> IOMMUFD_OBJ_DEVICE.  Assume IOMMU_HW_INFO_TYPE_NONE and proceed.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>

> ---
>  hw/vfio/iommufd.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index cb7257b..d8928d4 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -635,6 +635,7 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
>  static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>                                        Error **errp)
>  {
> +    Error *err = NULL;
>      VFIODevice *vdev = opaque;
>      HostIOMMUDeviceCaps *caps = &hiod->caps;
>      enum iommu_hw_info_type type;
> @@ -645,8 +646,9 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>      hiod->agent = opaque;
>  
>      if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
> -                                         &type, &data, sizeof(data), errp)) {
> -        return false;
> +                                         &type, &data, sizeof(data), &err)) {
> +        warn_report_err(err);
> +        type = IOMMU_HW_INFO_TYPE_NONE;
>      }
>  
>      hiod->name = g_strdup(vdev->name);

At least we aren't supposed to call realize() on an mdev.

See commit 9f17604195c ("vfio/iommufd: Don't initialize nor set a
HOST_IOMMU_DEVICE with mdev"). There's another refactor later on in commit
83a4d596a93 ("vfio/{iommufd, container}: Invoke HostIOMMUDevice::realize()
during attach_device()") where we also don't realize() when vbasedev->hiod is NULL.

Unless the stty mdevs aren't matching examples, then this shouldn't be possible
(I didn't have one with real hw behind to test).

So if you are hitting this codepath then vbasedev::mdev is false, which I don't
understand how it's possible?

Re: [PATCH] vfio/iommufd: no hw_info for mediated devices

Posted by Steven Sistare 2 months, 2 weeks ago

On 9/10/2024 3:29 PM, Joao Martins wrote:
> On 10/09/2024 18:40, Steve Sistare wrote:
>> IOMMU_GET_HW_INFO fails for a vfio mediated device (aka mdev), because
>> the devid is associated with kernel type IOMMUFD_OBJ_ACCESS, not
>> IOMMUFD_OBJ_DEVICE.  Assume IOMMU_HW_INFO_TYPE_NONE and proceed.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> 
>> ---
>>   hw/vfio/iommufd.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index cb7257b..d8928d4 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -635,6 +635,7 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
>>   static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>                                         Error **errp)
>>   {
>> +    Error *err = NULL;
>>       VFIODevice *vdev = opaque;
>>       HostIOMMUDeviceCaps *caps = &hiod->caps;
>>       enum iommu_hw_info_type type;
>> @@ -645,8 +646,9 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>       hiod->agent = opaque;
>>   
>>       if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
>> -                                         &type, &data, sizeof(data), errp)) {
>> -        return false;
>> +                                         &type, &data, sizeof(data), &err)) {
>> +        warn_report_err(err);
>> +        type = IOMMU_HW_INFO_TYPE_NONE;
>>       }
>>   
>>       hiod->name = g_strdup(vdev->name);
> 
> At least we aren't supposed to call realize() on an mdev.
> 
> See commit 9f17604195c ("vfio/iommufd: Don't initialize nor set a
> HOST_IOMMU_DEVICE with mdev"). There's another refactor later on in commit
> 83a4d596a93 ("vfio/{iommufd, container}: Invoke HostIOMMUDevice::realize()
> during attach_device()") where we also don't realize() when vbasedev->hiod is NULL.
> 
> Unless the stty mdevs aren't matching examples, then this shouldn't be possible
> (I didn't have one with real hw behind to test).
> 
> So if you are hitting this codepath then vbasedev::mdev is false, which I don't
> understand how it's possible?

I exercised this with the mtty sample mdev, and mdev is true:

vfio_mdev  (83b8f4f2-509f-382f-3c1e-e6bfe0fa1001) is_mdev 1
iommufd_cdev_getfd  /dev/vfio/devices/vfio1 (fd=26)
iommufd_backend_connect fd=15 owned=1 users=1
iommufd_cdev_connect_and_bind  [iommufd=15] Successfully bound device 83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 (fd=26): output devid=1
iommufd_backend_alloc_ioas  iommufd=15 ioas=2
iommufd_cdev_alloc_ioas  [iommufd=15] new IOMMUFD container with ioasid=2
iommufd_cdev_attach_ioas_hwpt  [iommufd=15] Successfully attached device 83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 (26) to id=2
iommufd_backend_map_dma  iommufd=15 ioas=2 iova=0x0 size=0xc0000 addr=0x7fb80f600000 readonly=0 (0)
iommufd_backend_map_dma  iommufd=15 ioas=2 iova=0xc0000 size=0x20000 addr=0x7fb80d400000 readonly=1 (0)
iommufd_backend_map_dma  iommufd=15 ioas=2 iova=0x100000 size=0xff00000 addr=0x7fb80f700000 readonly=0 (0)
iommufd_cdev_device_info  83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 (26) num_irqs=5 num_regions=9 flags=2
qemu-kvm: -device vfio-pci,iommufd=iommufd1,sysfsdev=/sys/devices/virtual/mtty/mtty/83b8f4f2-509f-382f-3c1e-e6bfe0fa1001:
   warning: Failed to get hardware info: No such file or directory

My qemu tree is a bit old, I will update and look for the issues you described.

- Steve

Re: [PATCH] vfio/iommufd: no hw_info for mediated devices

Posted by Steven Sistare 2 months, 1 week ago

On 9/10/2024 3:51 PM, Steven Sistare wrote:
> On 9/10/2024 3:29 PM, Joao Martins wrote:
>> On 10/09/2024 18:40, Steve Sistare wrote:
>>> IOMMU_GET_HW_INFO fails for a vfio mediated device (aka mdev), because
>>> the devid is associated with kernel type IOMMUFD_OBJ_ACCESS, not
>>> IOMMUFD_OBJ_DEVICE.  Assume IOMMU_HW_INFO_TYPE_NONE and proceed.
>>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>
>>> ---
>>>   hw/vfio/iommufd.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>> index cb7257b..d8928d4 100644
>>> --- a/hw/vfio/iommufd.c
>>> +++ b/hw/vfio/iommufd.c
>>> @@ -635,6 +635,7 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
>>>   static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>>                                         Error **errp)
>>>   {
>>> +    Error *err = NULL;
>>>       VFIODevice *vdev = opaque;
>>>       HostIOMMUDeviceCaps *caps = &hiod->caps;
>>>       enum iommu_hw_info_type type;
>>> @@ -645,8 +646,9 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>>       hiod->agent = opaque;
>>>       if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
>>> -                                         &type, &data, sizeof(data), errp)) {
>>> -        return false;
>>> +                                         &type, &data, sizeof(data), &err)) {
>>> +        warn_report_err(err);
>>> +        type = IOMMU_HW_INFO_TYPE_NONE;
>>>       }
>>>       hiod->name = g_strdup(vdev->name);
>>
>> At least we aren't supposed to call realize() on an mdev.
>>
>> See commit 9f17604195c ("vfio/iommufd: Don't initialize nor set a
>> HOST_IOMMU_DEVICE with mdev"). There's another refactor later on in commit
>> 83a4d596a93 ("vfio/{iommufd, container}: Invoke HostIOMMUDevice::realize()
>> during attach_device()") where we also don't realize() when vbasedev->hiod is NULL.
>>
>> Unless the stty mdevs aren't matching examples, then this shouldn't be possible
>> (I didn't have one with real hw behind to test).
>>
>> So if you are hitting this codepath then vbasedev::mdev is false, which I don't
>> understand how it's possible?
> 
> I exercised this with the mtty sample mdev, and mdev is true:
> 
> [...]
> 
> My qemu tree is a bit old, I will update and look for the issues you described.

After updating all is well without my patch, so I withdraw it, thanks.

- Steve