diff mbox series

[v5,06/13] vfio/{iommufd,container}: Remove caps::aw_bits

Message ID 20240719120501.81279-7-joao.m.martins@oracle.com
State New
Headers show
Series hw/iommufd: IOMMUFD Dirty Tracking | expand

Commit Message

Joao Martins July 19, 2024, 12:04 p.m. UTC 
Remove caps::aw_bits which requires the bcontainer::iova_ranges being
initialized after device is actually attached. Instead defer that to
.get_cap() and call vfio_device_get_aw_bits() directly.

This is in preparation for HostIOMMUDevice::realize() being called early
during attach_device().

Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com
---
 include/sysemu/host_iommu_device.h | 3 ---
 backends/iommufd.c                 | 3 ++-
 hw/vfio/container.c                | 5 +----
 hw/vfio/iommufd.c                  | 1 -
 4 files changed, 3 insertions(+), 9 deletions(-)

Comments

Duan, Zhenzhong July 22, 2024, 5:22 a.m. UTC | #1 
>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Subject: [PATCH v5 06/13] vfio/{iommufd,container}: Remove caps::aw_bits
>
>Remove caps::aw_bits which requires the bcontainer::iova_ranges being
>initialized after device is actually attached. Instead defer that to
>.get_cap() and call vfio_device_get_aw_bits() directly.
>
>This is in preparation for HostIOMMUDevice::realize() being called early
>during attach_device().
>
>Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>Reviewed-by: Cédric Le Goater <clg@redhat.com
>---
> include/sysemu/host_iommu_device.h | 3 ---
> backends/iommufd.c                 | 3 ++-
> hw/vfio/container.c                | 5 +----
> hw/vfio/iommufd.c                  | 1 -
> 4 files changed, 3 insertions(+), 9 deletions(-)
>
>diff --git a/include/sysemu/host_iommu_device.h
>b/include/sysemu/host_iommu_device.h
>index ee6c813c8b22..cdeeccec7671 100644
>--- a/include/sysemu/host_iommu_device.h
>+++ b/include/sysemu/host_iommu_device.h
>@@ -19,12 +19,9 @@
>  * struct HostIOMMUDeviceCaps - Define host IOMMU device capabilities.
>  *
>  * @type: host platform IOMMU type.
>- *
>- * @aw_bits: host IOMMU address width. 0xff if no limitation.
>  */
> typedef struct HostIOMMUDeviceCaps {
>     uint32_t type;
>-    uint8_t aw_bits;
> } HostIOMMUDeviceCaps;
>
> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
>diff --git a/backends/iommufd.c b/backends/iommufd.c
>index a94d3b90c05c..58032e588f49 100644
>--- a/backends/iommufd.c
>+++ b/backends/iommufd.c
>@@ -18,6 +18,7 @@
> #include "qemu/error-report.h"
> #include "monitor/monitor.h"
> #include "trace.h"
>+#include "hw/vfio/vfio-common.h"
> #include <sys/ioctl.h>
> #include <linux/iommufd.h>
>
>@@ -270,7 +271,7 @@ static int
>hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp)
>     case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE:
>         return caps->type;
>     case HOST_IOMMU_DEVICE_CAP_AW_BITS:
>-        return caps->aw_bits;
>+        return vfio_device_get_aw_bits(hiod->agent);

I just realized there is an open here. hiod->agent is not necessarily VFIO device, can be VDPA device.
May need a bit more work on this.

Thanks
Zhenzhong

>     default:
>         error_setg(errp, "%s: unsupported capability %x", hiod->name, cap);
>         return -EINVAL;
>diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>index 88ede913d6f7..c27f448ba26e 100644
>--- a/hw/vfio/container.c
>+++ b/hw/vfio/container.c
>@@ -1144,7 +1144,6 @@ static bool
>hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>     VFIODevice *vdev = opaque;
>
>     hiod->name = g_strdup(vdev->name);
>-    hiod->caps.aw_bits = vfio_device_get_aw_bits(vdev);
>     hiod->agent = opaque;
>
>     return true;
>@@ -1153,11 +1152,9 @@ static bool
>hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
> static int hiod_legacy_vfio_get_cap(HostIOMMUDevice *hiod, int cap,
>                                     Error **errp)
> {
>-    HostIOMMUDeviceCaps *caps = &hiod->caps;
>-
>     switch (cap) {
>     case HOST_IOMMU_DEVICE_CAP_AW_BITS:
>-        return caps->aw_bits;
>+        return vfio_device_get_aw_bits(hiod->agent);
>     default:
>         error_setg(errp, "%s: unsupported capability %x", hiod->name, cap);
>         return -EINVAL;
>diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>index 545f4a404125..028533bc39b9 100644
>--- a/hw/vfio/iommufd.c
>+++ b/hw/vfio/iommufd.c
>@@ -724,7 +724,6 @@ static bool
>hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>
>     hiod->name = g_strdup(vdev->name);
>     caps->type = type;
>-    caps->aw_bits = vfio_device_get_aw_bits(vdev);
>
>     return true;
> }
>--
>2.17.2
Joao Martins July 22, 2024, 8:53 a.m. UTC | #2 
On 22/07/2024 06:22, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Subject: [PATCH v5 06/13] vfio/{iommufd,container}: Remove caps::aw_bits
>>
>> Remove caps::aw_bits which requires the bcontainer::iova_ranges being
>> initialized after device is actually attached. Instead defer that to
>> .get_cap() and call vfio_device_get_aw_bits() directly.
>>
>> This is in preparation for HostIOMMUDevice::realize() being called early
>> during attach_device().
>>
>> Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Reviewed-by: Cédric Le Goater <clg@redhat.com
>> ---
>> include/sysemu/host_iommu_device.h | 3 ---
>> backends/iommufd.c                 | 3 ++-
>> hw/vfio/container.c                | 5 +----
>> hw/vfio/iommufd.c                  | 1 -
>> 4 files changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/sysemu/host_iommu_device.h
>> b/include/sysemu/host_iommu_device.h
>> index ee6c813c8b22..cdeeccec7671 100644
>> --- a/include/sysemu/host_iommu_device.h
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -19,12 +19,9 @@
>>  * struct HostIOMMUDeviceCaps - Define host IOMMU device capabilities.
>>  *
>>  * @type: host platform IOMMU type.
>> - *
>> - * @aw_bits: host IOMMU address width. 0xff if no limitation.
>>  */
>> typedef struct HostIOMMUDeviceCaps {
>>     uint32_t type;
>> -    uint8_t aw_bits;
>> } HostIOMMUDeviceCaps;
>>
>> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index a94d3b90c05c..58032e588f49 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -18,6 +18,7 @@
>> #include "qemu/error-report.h"
>> #include "monitor/monitor.h"
>> #include "trace.h"
>> +#include "hw/vfio/vfio-common.h"
>> #include <sys/ioctl.h>
>> #include <linux/iommufd.h>
>>
>> @@ -270,7 +271,7 @@ static int
>> hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp)
>>     case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE:
>>         return caps->type;
>>     case HOST_IOMMU_DEVICE_CAP_AW_BITS:
>> -        return caps->aw_bits;
>> +        return vfio_device_get_aw_bits(hiod->agent);
> 
> I just realized there is an open here. hiod->agent is not necessarily VFIO device, can be VDPA device.
> May need a bit more work on this.
> 

Broadly speaking I agree, that this needs some sort of IOMMUDevice structure
with a agent type that it needs to abstract from instead of an opaque object.

But feels unrelated to this patch exactly, as the existing code was already
making assumptions that ::opaque is a VFIODevice.

> Thanks
> Zhenzhong
> 
>>     default:
>>         error_setg(errp, "%s: unsupported capability %x", hiod->name, cap);
>>         return -EINVAL;
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index 88ede913d6f7..c27f448ba26e 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -1144,7 +1144,6 @@ static bool
>> hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>     VFIODevice *vdev = opaque;
>>
>>     hiod->name = g_strdup(vdev->name);
>> -    hiod->caps.aw_bits = vfio_device_get_aw_bits(vdev);
>>     hiod->agent = opaque;
>>
>>     return true;
>> @@ -1153,11 +1152,9 @@ static bool
>> hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>> static int hiod_legacy_vfio_get_cap(HostIOMMUDevice *hiod, int cap,
>>                                     Error **errp)
>> {
>> -    HostIOMMUDeviceCaps *caps = &hiod->caps;
>> -
>>     switch (cap) {
>>     case HOST_IOMMU_DEVICE_CAP_AW_BITS:
>> -        return caps->aw_bits;
>> +        return vfio_device_get_aw_bits(hiod->agent);
>>     default:
>>         error_setg(errp, "%s: unsupported capability %x", hiod->name, cap);
>>         return -EINVAL;
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 545f4a404125..028533bc39b9 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -724,7 +724,6 @@ static bool
>> hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>
>>     hiod->name = g_strdup(vdev->name);
>>     caps->type = type;
>> -    caps->aw_bits = vfio_device_get_aw_bits(vdev);
>>
>>     return true;
>> }
>> --
>> 2.17.2
>
Duan, Zhenzhong July 23, 2024, 5:30 a.m. UTC | #3 
>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Subject: Re: [PATCH v5 06/13] vfio/{iommufd,container}: Remove
>caps::aw_bits
>
>On 22/07/2024 06:22, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Joao Martins <joao.m.martins@oracle.com>
>>> Subject: [PATCH v5 06/13] vfio/{iommufd,container}: Remove
>caps::aw_bits
>>>
>>> Remove caps::aw_bits which requires the bcontainer::iova_ranges being
>>> initialized after device is actually attached. Instead defer that to
>>> .get_cap() and call vfio_device_get_aw_bits() directly.
>>>
>>> This is in preparation for HostIOMMUDevice::realize() being called early
>>> during attach_device().
>>>
>>> Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> Reviewed-by: Cédric Le Goater <clg@redhat.com
>>> ---
>>> include/sysemu/host_iommu_device.h | 3 ---
>>> backends/iommufd.c                 | 3 ++-
>>> hw/vfio/container.c                | 5 +----
>>> hw/vfio/iommufd.c                  | 1 -
>>> 4 files changed, 3 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/sysemu/host_iommu_device.h
>>> b/include/sysemu/host_iommu_device.h
>>> index ee6c813c8b22..cdeeccec7671 100644
>>> --- a/include/sysemu/host_iommu_device.h
>>> +++ b/include/sysemu/host_iommu_device.h
>>> @@ -19,12 +19,9 @@
>>>  * struct HostIOMMUDeviceCaps - Define host IOMMU device capabilities.
>>>  *
>>>  * @type: host platform IOMMU type.
>>> - *
>>> - * @aw_bits: host IOMMU address width. 0xff if no limitation.
>>>  */
>>> typedef struct HostIOMMUDeviceCaps {
>>>     uint32_t type;
>>> -    uint8_t aw_bits;
>>> } HostIOMMUDeviceCaps;
>>>
>>> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
>>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>>> index a94d3b90c05c..58032e588f49 100644
>>> --- a/backends/iommufd.c
>>> +++ b/backends/iommufd.c
>>> @@ -18,6 +18,7 @@
>>> #include "qemu/error-report.h"
>>> #include "monitor/monitor.h"
>>> #include "trace.h"
>>> +#include "hw/vfio/vfio-common.h"
>>> #include <sys/ioctl.h>
>>> #include <linux/iommufd.h>
>>>
>>> @@ -270,7 +271,7 @@ static int
>>> hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp)
>>>     case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE:
>>>         return caps->type;
>>>     case HOST_IOMMU_DEVICE_CAP_AW_BITS:
>>> -        return caps->aw_bits;
>>> +        return vfio_device_get_aw_bits(hiod->agent);
>>
>> I just realized there is an open here. hiod->agent is not necessarily VFIO
>device, can be VDPA device.
>> May need a bit more work on this.
>>
>
>Broadly speaking I agree, that this needs some sort of IOMMUDevice
>structure
>with a agent type that it needs to abstract from instead of an opaque object.
>
>But feels unrelated to this patch exactly, as the existing code was already
>making assumptions that ::opaque is a VFIODevice.

Currently only VFIODevice is supported, so hiod->agent can only points to a VFIODevice.
In future, when VDPA is supported, hiod->agent can point to some kind of VDPADevice structure after ::realize() initialize it.

But I'm ok to leave it to VDPA to fix this as for now hiod->agent only points to VFIODevice.

Thanks
Zhenzhong

>
>> Thanks
>> Zhenzhong
>>
>>>     default:
>>>         error_setg(errp, "%s: unsupported capability %x", hiod->name, cap);
>>>         return -EINVAL;
>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>> index 88ede913d6f7..c27f448ba26e 100644
>>> --- a/hw/vfio/container.c
>>> +++ b/hw/vfio/container.c
>>> @@ -1144,7 +1144,6 @@ static bool
>>> hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>>     VFIODevice *vdev = opaque;
>>>
>>>     hiod->name = g_strdup(vdev->name);
>>> -    hiod->caps.aw_bits = vfio_device_get_aw_bits(vdev);
>>>     hiod->agent = opaque;
>>>
>>>     return true;
>>> @@ -1153,11 +1152,9 @@ static bool
>>> hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>> static int hiod_legacy_vfio_get_cap(HostIOMMUDevice *hiod, int cap,
>>>                                     Error **errp)
>>> {
>>> -    HostIOMMUDeviceCaps *caps = &hiod->caps;
>>> -
>>>     switch (cap) {
>>>     case HOST_IOMMU_DEVICE_CAP_AW_BITS:
>>> -        return caps->aw_bits;
>>> +        return vfio_device_get_aw_bits(hiod->agent);
>>>     default:
>>>         error_setg(errp, "%s: unsupported capability %x", hiod->name, cap);
>>>         return -EINVAL;
>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>> index 545f4a404125..028533bc39b9 100644
>>> --- a/hw/vfio/iommufd.c
>>> +++ b/hw/vfio/iommufd.c
>>> @@ -724,7 +724,6 @@ static bool
>>> hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>>
>>>     hiod->name = g_strdup(vdev->name);
>>>     caps->type = type;
>>> -    caps->aw_bits = vfio_device_get_aw_bits(vdev);
>>>
>>>     return true;
>>> }
>>> --
>>> 2.17.2
>>
diff mbox series

Patch

diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
index ee6c813c8b22..cdeeccec7671 100644
--- a/include/sysemu/host_iommu_device.h
+++ b/include/sysemu/host_iommu_device.h
@@ -19,12 +19,9 @@ 
  * struct HostIOMMUDeviceCaps - Define host IOMMU device capabilities.
  *
  * @type: host platform IOMMU type.
- *
- * @aw_bits: host IOMMU address width. 0xff if no limitation.
  */
 typedef struct HostIOMMUDeviceCaps {
     uint32_t type;
-    uint8_t aw_bits;
 } HostIOMMUDeviceCaps;
 
 #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
diff --git a/backends/iommufd.c b/backends/iommufd.c
index a94d3b90c05c..58032e588f49 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -18,6 +18,7 @@ 
 #include "qemu/error-report.h"
 #include "monitor/monitor.h"
 #include "trace.h"
+#include "hw/vfio/vfio-common.h"
 #include <sys/ioctl.h>
 #include <linux/iommufd.h>
 
@@ -270,7 +271,7 @@  static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp)
     case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE:
         return caps->type;
     case HOST_IOMMU_DEVICE_CAP_AW_BITS:
-        return caps->aw_bits;
+        return vfio_device_get_aw_bits(hiod->agent);
     default:
         error_setg(errp, "%s: unsupported capability %x", hiod->name, cap);
         return -EINVAL;
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 88ede913d6f7..c27f448ba26e 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1144,7 +1144,6 @@  static bool hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
     VFIODevice *vdev = opaque;
 
     hiod->name = g_strdup(vdev->name);
-    hiod->caps.aw_bits = vfio_device_get_aw_bits(vdev);
     hiod->agent = opaque;
 
     return true;
@@ -1153,11 +1152,9 @@  static bool hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
 static int hiod_legacy_vfio_get_cap(HostIOMMUDevice *hiod, int cap,
                                     Error **errp)
 {
-    HostIOMMUDeviceCaps *caps = &hiod->caps;
-
     switch (cap) {
     case HOST_IOMMU_DEVICE_CAP_AW_BITS:
-        return caps->aw_bits;
+        return vfio_device_get_aw_bits(hiod->agent);
     default:
         error_setg(errp, "%s: unsupported capability %x", hiod->name, cap);
         return -EINVAL;
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 545f4a404125..028533bc39b9 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -724,7 +724,6 @@  static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
 
     hiod->name = g_strdup(vdev->name);
     caps->type = type;
-    caps->aw_bits = vfio_device_get_aw_bits(vdev);
 
     return true;
 }