Store the agent device (VFIO or VDPA) in the host IOMMU device.
This will allow easy access to some of its resources.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
include/sysemu/host_iommu_device.h | 1 +
hw/vfio/container.c | 1 +
hw/vfio/iommufd.c | 2 ++
3 files changed, 4 insertions(+)
diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
index a57873958b..3e5f058e7b 100644
--- a/include/sysemu/host_iommu_device.h
+++ b/include/sysemu/host_iommu_device.h
@@ -34,6 +34,7 @@ struct HostIOMMUDevice {
Object parent_obj;
char *name;
+ void *agent; /* pointer to agent device, ie. VFIO or VDPA device */
HostIOMMUDeviceCaps caps;
};
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 26e6f7fb4f..b728b978a2 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1145,6 +1145,7 @@ static bool hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
hiod->name = g_strdup(vdev->name);
hiod->caps.aw_bits = vfio_device_get_aw_bits(vdev);
+ hiod->agent = opaque;
return true;
}
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 409ed3dcc9..dbdae1adbb 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -631,6 +631,8 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
struct iommu_hw_info_vtd vtd;
} data;
+ hiod->agent = opaque;
+
if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
&type, &data, sizeof(data), errp)) {
return false;
--
2.41.0On 6/13/24 11:20 AM, Eric Auger wrote:
> Store the agent device (VFIO or VDPA) in the host IOMMU device.
> This will allow easy access to some of its resources.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> include/sysemu/host_iommu_device.h | 1 +
> hw/vfio/container.c | 1 +
> hw/vfio/iommufd.c | 2 ++
> 3 files changed, 4 insertions(+)
>
> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
> index a57873958b..3e5f058e7b 100644
> --- a/include/sysemu/host_iommu_device.h
> +++ b/include/sysemu/host_iommu_device.h
> @@ -34,6 +34,7 @@ struct HostIOMMUDevice {
> Object parent_obj;
>
> char *name;
> + void *agent; /* pointer to agent device, ie. VFIO or VDPA device */
> HostIOMMUDeviceCaps caps;
> };
>
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 26e6f7fb4f..b728b978a2 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -1145,6 +1145,7 @@ static bool hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>
> hiod->name = g_strdup(vdev->name);
> hiod->caps.aw_bits = vfio_device_get_aw_bits(vdev);
> + hiod->agent = opaque;
>
> return true;
> }
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 409ed3dcc9..dbdae1adbb 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -631,6 +631,8 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
> struct iommu_hw_info_vtd vtd;
> } data;
>
> + hiod->agent = opaque;
> +
This opaque pointer could be assigned in vfio_attach_device().
Talking of which, why are we passing a 'VFIODevice *' parameter to
HostIOMMUDeviceClass::realize ? I don't see a good reason
I think a 'VFIOContainerBase *' would be more appropriate since
'HostIOMMUDevice' represents a device on the host which is common
to all VFIO devices.
In that case, HostIOMMUDevice::agent wouldn't need to be opaque
anymore. It could simply be a 'VFIOContainerBase *' and
hiod_legacy_vfio_get_iova_ranges() in patch 3 would grab the
'iova_ranges' from the 'VFIOContainerBase *' directly.
This means some rework :
* vfio_device_get_aw_bits() would use a 'VFIOContainerBase *' instead.
* HostIOMMUDevice::name would be removed. This is just for error messages.
* hiod_iommufd_vfio_realize() would use VFIOIOMMUFDContainer::be.
That said, I think we need the QOMification changes first.
Thanks,
C.
> if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
> &type, &data, sizeof(data), errp)) {
> return false;
Hi Cédric,
On 6/14/24 11:13, Cédric Le Goater wrote:
> On 6/13/24 11:20 AM, Eric Auger wrote:
>> Store the agent device (VFIO or VDPA) in the host IOMMU device.
>> This will allow easy access to some of its resources.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>> include/sysemu/host_iommu_device.h | 1 +
>> hw/vfio/container.c | 1 +
>> hw/vfio/iommufd.c | 2 ++
>> 3 files changed, 4 insertions(+)
>>
>> diff --git a/include/sysemu/host_iommu_device.h
>> b/include/sysemu/host_iommu_device.h
>> index a57873958b..3e5f058e7b 100644
>> --- a/include/sysemu/host_iommu_device.h
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -34,6 +34,7 @@ struct HostIOMMUDevice {
>> Object parent_obj;
>> char *name;
>> + void *agent; /* pointer to agent device, ie. VFIO or VDPA device */
>> HostIOMMUDeviceCaps caps;
>> };
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index 26e6f7fb4f..b728b978a2 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -1145,6 +1145,7 @@ static bool
>> hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>> hiod->name = g_strdup(vdev->name);
>> hiod->caps.aw_bits = vfio_device_get_aw_bits(vdev);
>> + hiod->agent = opaque;
>> return true;
>> }
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 409ed3dcc9..dbdae1adbb 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -631,6 +631,8 @@ static bool
>> hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>> struct iommu_hw_info_vtd vtd;
>> } data;
>> + hiod->agent = opaque;
>> +
>
> This opaque pointer could be assigned in vfio_attach_device().
>
> Talking of which, why are we passing a 'VFIODevice *' parameter to
> HostIOMMUDeviceClass::realize ? I don't see a good reason
>
> I think a 'VFIOContainerBase *' would be more appropriate since
> 'HostIOMMUDevice' represents a device on the host which is common
> to all VFIO devices.
>
> In that case, HostIOMMUDevice::agent wouldn't need to be opaque
> anymore. It could simply be a 'VFIOContainerBase *' and
> hiod_legacy_vfio_get_iova_ranges() in patch 3 would grab the
> 'iova_ranges' from the 'VFIOContainerBase *' directly.
>
> This means some rework :
>
> * vfio_device_get_aw_bits() would use a 'VFIOContainerBase *' instead.
> * HostIOMMUDevice::name would be removed. This is just for error
> messages.
> * hiod_iommufd_vfio_realize() would use VFIOIOMMUFDContainer::be.
>
> That said, I think we need the QOMification changes first.
OK I need to review your series first. At the moment I have just
addressed Zhenzhong's comment in v4, just sent.
Thanks
Eric
>
> Thanks,
>
> C.
>
>
>
>
>> if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
>> &type, &data,
>> sizeof(data), errp)) {
>> return false;
>
>> Talking of which, why are we passing a 'VFIODevice *' parameter to >> HostIOMMUDeviceClass::realize ? I don't see a good reason >> >> I think a 'VFIOContainerBase *' would be more appropriate since >> 'HostIOMMUDevice' represents a device on the host which is common >> to all VFIO devices. >> >> In that case, HostIOMMUDevice::agent wouldn't need to be opaque >> anymore. It could simply be a 'VFIOContainerBase *' and >> hiod_legacy_vfio_get_iova_ranges() in patch 3 would grab the >> 'iova_ranges' from the 'VFIOContainerBase *' directly. >> >> This means some rework : >> >> * vfio_device_get_aw_bits() would use a 'VFIOContainerBase *' instead. >> * HostIOMMUDevice::name would be removed. This is just for error >> messages. >> * hiod_iommufd_vfio_realize() would use VFIOIOMMUFDContainer::be. >> >> That said, I think we need the QOMification changes first. > > OK I need to review your series first. At the moment I have just > addressed Zhenzhong's comment in v4, just sent. Yep. Just take a look at mine. If both of you agree with above proposal, I can care of it and resend all 3. It's a small change. Thanks, C.
Hi Cédric, >-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Sent: Friday, June 14, 2024 6:05 PM >To: eric.auger@redhat.com; eric.auger.pro@gmail.com; qemu- >devel@nongnu.org; qemu-arm@nongnu.org; mst@redhat.com; jean- >philippe@linaro.org; peter.maydell@linaro.org; yanghliu@redhat.com; Duan, >Zhenzhong <zhenzhong.duan@intel.com> >Cc: alex.williamson@redhat.com; jasowang@redhat.com; >pbonzini@redhat.com; berrange@redhat.com >Subject: Re: [PATCH v3 1/7] HostIOMMUDevice: Store the VFIO/VDPA agent > > >>> Talking of which, why are we passing a 'VFIODevice *' parameter to >>> HostIOMMUDeviceClass::realize ? I don't see a good reason >>> >>> I think a 'VFIOContainerBase *' would be more appropriate since >>> 'HostIOMMUDevice' represents a device on the host which is common >>> to all VFIO devices. >>> >>> In that case, HostIOMMUDevice::agent wouldn't need to be opaque >>> anymore. It could simply be a 'VFIOContainerBase *' and >>> hiod_legacy_vfio_get_iova_ranges() in patch 3 would grab the >>> 'iova_ranges' from the 'VFIOContainerBase *' directly. >>> >>> This means some rework : >>> >>> * vfio_device_get_aw_bits() would use a 'VFIOContainerBase *' instead. >>> * HostIOMMUDevice::name would be removed. This is just for error >>> messages. >>> * hiod_iommufd_vfio_realize() would use VFIOIOMMUFDContainer::be. >>> >>> That said, I think we need the QOMification changes first. >> >> OK I need to review your series first. At the moment I have just >> addressed Zhenzhong's comment in v4, just sent. > >Yep. Just take a look at mine. If both of you agree with above >proposal, I can care of it and resend all 3. It's a small change. I would suggest using opaque pointer and VFIODevice for two reasons, 1. in nesting series vIOMMU needs to attach/detaching hwpt which is VFIODevice operations. See https://github.com/yiliu1765/qemu/commit/3ca559d35adc9840555e361a56708af4c6338b3d 2. If we plan to support VDPA Device in future, the opaque pointer can also point to an VDPADevice structure. Thanks Zhenzhong
On 6/17/24 3:25 AM, Duan, Zhenzhong wrote: > Hi Cédric, > >> -----Original Message----- >> From: Cédric Le Goater <clg@redhat.com> >> Sent: Friday, June 14, 2024 6:05 PM >> To: eric.auger@redhat.com; eric.auger.pro@gmail.com; qemu- >> devel@nongnu.org; qemu-arm@nongnu.org; mst@redhat.com; jean- >> philippe@linaro.org; peter.maydell@linaro.org; yanghliu@redhat.com; Duan, >> Zhenzhong <zhenzhong.duan@intel.com> >> Cc: alex.williamson@redhat.com; jasowang@redhat.com; >> pbonzini@redhat.com; berrange@redhat.com >> Subject: Re: [PATCH v3 1/7] HostIOMMUDevice: Store the VFIO/VDPA agent >> >> >>>> Talking of which, why are we passing a 'VFIODevice *' parameter to >>>> HostIOMMUDeviceClass::realize ? I don't see a good reason >>>> >>>> I think a 'VFIOContainerBase *' would be more appropriate since >>>> 'HostIOMMUDevice' represents a device on the host which is common >>>> to all VFIO devices. >>>> >>>> In that case, HostIOMMUDevice::agent wouldn't need to be opaque >>>> anymore. It could simply be a 'VFIOContainerBase *' and >>>> hiod_legacy_vfio_get_iova_ranges() in patch 3 would grab the >>>> 'iova_ranges' from the 'VFIOContainerBase *' directly. >>>> >>>> This means some rework : >>>> >>>> * vfio_device_get_aw_bits() would use a 'VFIOContainerBase *' instead. >>>> * HostIOMMUDevice::name would be removed. This is just for error >>>> messages. >>>> * hiod_iommufd_vfio_realize() would use VFIOIOMMUFDContainer::be. >>>> >>>> That said, I think we need the QOMification changes first. >>> >>> OK I need to review your series first. At the moment I have just >>> addressed Zhenzhong's comment in v4, just sent. >> >> Yep. Just take a look at mine. If both of you agree with above >> proposal, I can care of it and resend all 3. It's a small change. > > I would suggest using opaque pointer and VFIODevice for two reasons, > 1. in nesting series vIOMMU needs to attach/detaching hwpt which is VFIODevice operations. > See https://github.com/yiliu1765/qemu/commit/3ca559d35adc9840555e361a56708af4c6338b3d OK. I realized later on that we needed the device id anyhow. > > 2. If we plan to support VDPA Device in future, the opaque pointer can also point > to an VDPADevice structure. Thanks, C.
© 2016 - 2024 Red Hat, Inc.