Re: [PATCH v4 07/12] vfio/{iommufd,container}: Initialize HostIOMMUDeviceCaps during attach_device()

[Joao Martins]

On 17/07/2024 14:41, Eric Auger wrote:
> On 7/17/24 14:33, Joao Martins wrote:
>> On 17/07/2024 13:19, Eric Auger wrote:
>>> Hi Joao,
>>>
>>> On 7/12/24 13:46, Joao Martins wrote:
>>>> Fetch IOMMU hw raw caps behind the device and thus move the
>>> what does mean "Fetch IOMMU hw raw caps behind the device'"
>> Fetching the out_capabilities field from GET_HW_INFO which essentially tell 
>> us
>> if the IOMMU behind the device supports dirty tracking.
> that's much clearer than the 1st sentence
>>
>>>> HostIOMMUDevice::realize() to be done during the attach of the device. It
>>>> allows it to cache the information obtained from IOMMU_GET_HW_INFO from
>>> what do you mean by " It allows it to cache the information obtained
>>> from IOMMU_GET_HW_INFO from iommufd early on"
>> /me nods
> ?

By caching I mean that invoking realize() earlier allow us to store the value of
@out_capabilities in HostIOMMUDevice::caps for later use and avoid having to
call GET_HW_INFO Again. 'Early on' refers to me doing this at the beginning of
attach_device().

>>
>>>> iommufd early on. However, while legacy HostIOMMUDevice caps
>>> what does mean "legacy HostIOMMUDevice caps always return true"?
>> That means that it can't fail, and the data in there is synthetic:
>>
>>     VFIODevice *vdev = opaque;
>>
>>     hiod->name = g_strdup(vdev->name);
>>     hiod->agent = opaque;
>>
>>     return true;
>>
>> The IOMMUFD one might fail if GET_HW_INFO fails.
> so you talk about hiod_legacy_vfio_realize() and not "
> 
> legacy HostIOMMUDevice caps"!
> 
It's both. Legacy doesn't need to initialize @caps. Whereby in IOMMUFD we do and
with actual info (the capabilities) and in order to do that, we need the backend
initialized. And *that* ioctl() may fail.

>>
>>>> always return true and doesn't have dependency on other things, the IOMMUFD
>>>> backend requires the iommufd FD to be connected and having a devid to be
>>>> able to query capabilities. Hence when exactly is HostIOMMUDevice
>>>> initialized inside backend ::attach_device() implementation is backend
>>>> specific.
>>>>
>>>> This is in preparation to fetch parse hw capabilities and understand if
>>> fetch parse?
>>>> dirty tracking is supported by device backing IOMMU without necessarily
>>>> duplicating the amount of calls we do to IOMMU_GET_HW_INFO.
>>> But we move code from generic place to BE specific place?
>>>
>> No because in IOMMUFD needs the backend connected, while the legacy backend
>> doesn't. Otherwise this patch wouldn't be needed to be backend specific.
>>
>>> Sorry I feel really hard to understand the commit msg in general
>>>
>> How about this:
>>
>>     Fetch IOMMU hw raw caps behind the device and thus move the
> You need to tell what the patch does and why.
> 

IMHO, I already do that -- what we are having here is a parsing issue on my
english (likely because it's a bit convoluted).

Me asking you how it sounds is for me to calibrate against how you understand it
or literality of the text (or lack of thereof).

> "Fetch IOMMU hw raw caps behind the device" sentence does not clearly fit in 
> any.
> 
>>     HostIOMMUDevice::realize() to be done during the attach of the device.
>>
>>     This is in preparation to fetch parse hw capabilities and understand if
>>     dirty tracking is supported by device backing IOMMU without necessarily
>>     duplicating the amount of calls we do to IOMMU_GET_HW_INFO.
>>
>>     Note that the HostIOMMUDevice data with legacy backend is synthetic
>>     and doesn't need any information from the (type1-iommu) backend. While 
>> the
>>     IOMMUFD backend requires the iommufd FD to be connected and having a 
>> devid
>>     to be able to query device capabilities seeded in HostIOMMUDevice. This
>>     means that HostIOMMUDevice initialization (i.e. ::realized() is invoked) 
>> is
>>     container backend specific.
>>
>>
>>