| Message ID | 20240719120501.81279-7-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/iommufd: IOMMUFD Dirty Tracking | expand |
>-----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
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 >
>-----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 --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; }