>-----Original Message-----
>From: Joao Martins <joao.m.mart...@oracle.com>
>Subject: Re: [PATCH RFCv2 2/8] vfio/iommufd: Introduce auto domain
>creation
>
>On 12/02/2024 16:27, Jason Gunthorpe wrote:
>> On Mon, Feb 12, 2024 at 01:56:37PM +0000, Joao Martins wrote:
>>> There's generally two modes of operation for IOMMUFD:
>>>
>>> * The simple user API which intends to perform relatively simple things
>>> with IOMMUs e.g. DPDK. It generally creates an IOAS and attach to VFIO
>>> and mainly performs IOAS_MAP and UNMAP.
>>>
>>> * The native IOMMUFD API where you have fine grained control of the
>>> IOMMU domain and model it accordingly. This is where most new feature
>>> are being steered to.
>>>
>>> For dirty tracking 2) is required, as it needs to ensure that
>>> the stage-2/parent IOMMU domain will only attach devices
>>> that support dirty tracking (so far it is all homogeneous in x86, likely
>>> not the case for smmuv3). Such invariant on dirty tracking provides a
>>> useful guarantee to VMMs that will refuse incompatible device
>>> attachments for IOMMU domains.
>>>
>>> For dirty tracking such property is enabled/enforced via HWPT_ALLOC,
>>> which is responsible for creating an IOMMU domain. This is contrast to
>>> the 'simple API' where the IOMMU domain is created by IOMMUFD
>>> automatically when it attaches to VFIO (usually referred as autodomains)
>>>
>>> To support dirty tracking with the advanced IOMMUFD API, it needs
>>> similar logic, where IOMMU domains are created and devices attached to
>>> compatible domains. Essentially mimmicing kernel
>>> iommufd_device_auto_get_domain(). If this fails (i.e. mdevs) it falls back
>>> to IOAS attach.
>>>
>>> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
>>> ---
>>> Right now the only alternative to a userspace autodomains
>implementation
>>> is to mimmicing all the flags being added to HWPT_ALLOC but into VFIO
>>> IOAS attach.
>>
>> It was my expectation that VMM userspace would implement direct hwpt
>> support. This is needed in all kinds of cases going forward because
>> hwpt allocation is not uniform across iommu instances and managing
>> this in the kernel is only feasible for simpler cases. Dirty tracking
>> would be one of them.
>>
>
>/me nods
>
>>> +int iommufd_backend_alloc_hwpt(int iommufd, uint32_t dev_id,
>>> + uint32_t pt_id, uint32_t flags,
>>> + uint32_t data_type, uint32_t data_len,
>>> + void *data_ptr, uint32_t *out_hwpt)
>>> +{
>>> + int ret;
>>> + struct iommu_hwpt_alloc alloc_hwpt = {
>>> + .size = sizeof(struct iommu_hwpt_alloc),
>>> + .flags = flags,
>>> + .dev_id = dev_id,
>>> + .pt_id = pt_id,
>>> + .data_type = data_type,
>>> + .data_len = data_len,
>>> + .data_uptr = (uint64_t)data_ptr,
>>> + .__reserved = 0,
>>
>> Unless the coding style requirs this it is not necessary to zero in
>> the __reserved within a bracketed in initializer..
>>
>
>Ah yes; and no other helper is doing this, so definitely doesn't look code
>style. I'll remove it.
>
>>> +static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>>> + VFIOIOMMUFDContainer *container,
>>> + Error **errp)
>>> +{
>>> + int iommufd = vbasedev->iommufd_dev.iommufd->fd;
>>> + VFIOIOASHwpt *hwpt;
>>> + Error *err = NULL;
>>> + int ret = -EINVAL;
>>> + uint32_t hwpt_id;
>>> +
>>> + /* Try to find a domain */
>>> + QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>>> + ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id,
>&err);
>>> + if (ret) {
>>> + /* -EINVAL means the domain is incompatible with the device. */
>>> + if (ret == -EINVAL) {
>>
>> Please send a kernel kdoc patch to document this..
>>
>
>Ack
>
>> The approach looks good to me
>>
>> The nesting patches surely need this too?
>
>From what I understand, yes, but they seem to be able to hid this inside
>intel-iommu for the parent hwpt allocation somehow. I'll let them comment;
Yes, we have similar implementation in nesting series. See
vtd_device_attach_container() in
https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg02750.html
Thanks
Zhenzhong
