| Message ID | 20240712114704.8708-7-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/iommufd: IOMMUFD Dirty Tracking | expand |
On 7/12/24 13:46, Joao Martins wrote: > In preparation to moving HostIOMMUDevice realize() being able to called > early during attach_device(), remove properties that rely on container > being initialized. > > This means removing caps::aw_bits which requires the > bcontainer::iova_ranges to be inititalized after device is actually > attached. Instead defer that to .get_cap() and call > vfio_device_get_aw_bits() directly. > > 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> Thanks, C. > --- > include/sysemu/host_iommu_device.h | 1 - > backends/iommufd.c | 3 ++- > hw/vfio/container.c | 5 +---- > hw/vfio/iommufd.c | 1 - > 4 files changed, 3 insertions(+), 7 deletions(-) > > diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h > index ee6c813c8b22..20e77cf54568 100644 > --- a/include/sysemu/host_iommu_device.h > +++ b/include/sysemu/host_iommu_device.h > @@ -24,7 +24,6 @@ > */ > 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 5d3dfa917415..41a9dec3b2c5 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 325c7598d5a1..873c919e319c 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -722,7 +722,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; > }
Hi Joao, On 7/12/24 13:46, Joao Martins wrote: > In preparation to moving HostIOMMUDevice realize() being able to called > early during attach_device(), remove properties that rely on container > being initialized. It is difficult to parse the above sentence. Would deserve some rephrasing. Also properties have a different meaning in qemu. > > This means removing caps::aw_bits which requires the > bcontainer::iova_ranges to be inititalized after device is actually initialized > attached. Instead defer that to .get_cap() and call > vfio_device_get_aw_bits() directly. > > Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > include/sysemu/host_iommu_device.h | 1 - > backends/iommufd.c | 3 ++- > hw/vfio/container.c | 5 +---- > hw/vfio/iommufd.c | 1 - > 4 files changed, 3 insertions(+), 7 deletions(-) > > diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h > index ee6c813c8b22..20e77cf54568 100644 > --- a/include/sysemu/host_iommu_device.h > +++ b/include/sysemu/host_iommu_device.h > @@ -24,7 +24,6 @@ > */ > typedef struct HostIOMMUDeviceCaps { > uint32_t type; > - uint8_t aw_bits; the doc comment needs to be updated accordingly. > } HostIOMMUDeviceCaps; > > #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" > diff --git a/backends/iommufd.c b/backends/iommufd.c > index 5d3dfa917415..41a9dec3b2c5 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 325c7598d5a1..873c919e319c 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -722,7 +722,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; > }
On 16/07/2024 18:40, Eric Auger wrote: > Hi Joao, > > On 7/12/24 13:46, Joao Martins wrote: >> In preparation to moving HostIOMMUDevice realize() being able to called >> early during attach_device(), remove properties that rely on container >> being initialized. > It is difficult to parse the above sentence. Would deserve some rephrasing. > > Also properties have a different meaning in qemu. I think I will remove the above paragraph and instead adopt below with some rephrasing: Remove caps::aw_bits which requires the bcontainer::iova_ranges to be inititalized after device is actually initialized 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(). Better? >> >> This means removing caps::aw_bits which requires the >> bcontainer::iova_ranges to be inititalized after device is actually > initialized Yes >> attached. Instead defer that to .get_cap() and call >> vfio_device_get_aw_bits() directly. >> >> Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> include/sysemu/host_iommu_device.h | 1 - >> backends/iommufd.c | 3 ++- >> hw/vfio/container.c | 5 +---- >> hw/vfio/iommufd.c | 1 - >> 4 files changed, 3 insertions(+), 7 deletions(-) >> >> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h >> index ee6c813c8b22..20e77cf54568 100644 >> --- a/include/sysemu/host_iommu_device.h >> +++ b/include/sysemu/host_iommu_device.h >> @@ -24,7 +24,6 @@ >> */ >> typedef struct HostIOMMUDeviceCaps { >> uint32_t type; >> - uint8_t aw_bits; > the doc comment needs to be updated accordingly. >> } HostIOMMUDeviceCaps; >> >> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" >> diff --git a/backends/iommufd.c b/backends/iommufd.c >> index 5d3dfa917415..41a9dec3b2c5 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 325c7598d5a1..873c919e319c 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -722,7 +722,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; >> } >
Hi Joao, On 7/16/24 20:22, Joao Martins wrote: > On 16/07/2024 18:40, Eric Auger wrote: >> Hi Joao, >> >> On 7/12/24 13:46, Joao Martins wrote: >>> In preparation to moving HostIOMMUDevice realize() being able to called >>> early during attach_device(), remove properties that rely on container >>> being initialized. >> It is difficult to parse the above sentence. Would deserve some rephrasing. >> >> Also properties have a different meaning in qemu. > I think I will remove the above paragraph and instead adopt below with some > rephrasing: > > Remove caps::aw_bits which requires the > bcontainer::iova_ranges to be inititalized after device is actually > initialized attached. Instead defer that to .get_cap() and call s/initialized//g > vfio_device_get_aw_bits() directly. > > This is in preparation for HostIOMMUDevice::realize() being called early during > attach_device(). > > Better? Yes sounds better Eric > >>> This means removing caps::aw_bits which requires the >>> bcontainer::iova_ranges to be inititalized after device is actually >> initialized > Yes > >>> attached. Instead defer that to .get_cap() and call >>> vfio_device_get_aw_bits() directly. >>> >>> Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>> --- >>> include/sysemu/host_iommu_device.h | 1 - >>> backends/iommufd.c | 3 ++- >>> hw/vfio/container.c | 5 +---- >>> hw/vfio/iommufd.c | 1 - >>> 4 files changed, 3 insertions(+), 7 deletions(-) >>> >>> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h >>> index ee6c813c8b22..20e77cf54568 100644 >>> --- a/include/sysemu/host_iommu_device.h >>> +++ b/include/sysemu/host_iommu_device.h >>> @@ -24,7 +24,6 @@ >>> */ >>> typedef struct HostIOMMUDeviceCaps { >>> uint32_t type; >>> - uint8_t aw_bits; >> the doc comment needs to be updated accordingly. >>> } HostIOMMUDeviceCaps; >>> >>> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" >>> diff --git a/backends/iommufd.c b/backends/iommufd.c >>> index 5d3dfa917415..41a9dec3b2c5 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 325c7598d5a1..873c919e319c 100644 >>> --- a/hw/vfio/iommufd.c >>> +++ b/hw/vfio/iommufd.c >>> @@ -722,7 +722,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; >>> }
diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h index ee6c813c8b22..20e77cf54568 100644 --- a/include/sysemu/host_iommu_device.h +++ b/include/sysemu/host_iommu_device.h @@ -24,7 +24,6 @@ */ 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 5d3dfa917415..41a9dec3b2c5 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 325c7598d5a1..873c919e319c 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -722,7 +722,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; }
In preparation to moving HostIOMMUDevice realize() being able to called early during attach_device(), remove properties that rely on container being initialized. This means removing caps::aw_bits which requires the bcontainer::iova_ranges to be inititalized after device is actually attached. Instead defer that to .get_cap() and call vfio_device_get_aw_bits() directly. Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- include/sysemu/host_iommu_device.h | 1 - backends/iommufd.c | 3 ++- hw/vfio/container.c | 5 +---- hw/vfio/iommufd.c | 1 - 4 files changed, 3 insertions(+), 7 deletions(-)