On 09/07/2024 10:04, Joao Martins wrote:
> On 09/07/2024 07:28, Cédric Le Goater wrote:
>> On 7/8/24 4:34 PM, Joao Martins wrote:
>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>> index 2ca9a32cc7b6..1b5b46d28ed6 100644
>>> --- a/hw/vfio/iommufd.c
>>> +++ b/hw/vfio/iommufd.c
>>> @@ -212,6 +212,20 @@ static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice
>>> *vbasedev, Error **errp)
>>> return true;
>>> }
>>> +static bool iommufd_device_dirty_tracking(IOMMUFDBackend *iommufd,
>>> + VFIODevice *vbasedev)
>>> +{
>>> + enum iommu_hw_info_type type;
>>> + uint64_t caps;
>>> +
>>> + if (!iommufd_backend_get_device_info(iommufd, vbasedev->devid, &type,
>>> + NULL, 0, &caps, NULL)) {
>>
>> I think we should report the error and not ignore it.
>>
>> That said, since we are probing the hw features of the host IOMMU device,
>> could we use the data cached in the HostIOMMUDevice struct instead ?
>> This means would need to move the ->realize() call doing the probing
>> before attaching the device in vfio_attach_device(). That way we would
>> catch probing errors in one place. Does this make sense ?
>
> Yeap. It also helps centralizing cap checking in addition.
>
> This stanadlone use of iommufd_backend_get_device_info() was also annoying me
> a
> little, and there doesn't seem to have a reason not to move the initialization
> of caps earlier. I'll do that
Maybe I was a little too early into this. I had the snip below, but I forgot
that vbasedev::devid / vbasedev::iommufd are used by hiod realize() and that is
done in the very beginning of ->attach_device() of iommufd backend. Not enterily
sure how to unravel that hmmm
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 7cdb969fd396..42931891bf8e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1552,17 +1552,17 @@ bool vfio_attach_device(char *name, VFIODevice
*vbasedev,
assert(ops);
- if (!ops->attach_device(name, vbasedev, as, errp)) {
- return false;
- }
-
hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) {
object_unref(hiod);
- ops->detach_device(vbasedev);
return false;
}
+
vbasedev->hiod = hiod;
+ if (!ops->attach_device(name, vbasedev, as, errp)) {
+ object_unref(hiod);
+ return false;
+ }
return true;
}
