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. >> >> >>
