RE: [PATCH RFCv2 2/8] vfio/iommufd: Introduce auto domain creation

2024-02-26 Thread Duan, Zhenzhong


>-Original Message-
>From: Joao Martins 
>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 +, 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 
>>> ---
>>> 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, >hwpt_list, next) {
>>> +ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id,
>);
>>> +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


Re: [PATCH RFCv2 2/8] vfio/iommufd: Introduce auto domain creation

2024-02-20 Thread Joao Martins
On 19/02/2024 08:58, Avihai Horon wrote:
> Hi Joao,
> 
> On 12/02/2024 15:56, Joao Martins wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 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 
>> ---
>> 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. So opted for autodomains userspace approach to avoid the
>> duplication of hwpt-alloc flags vs attach-ioas flags. I lack mdev real
>> drivers atm, so testing with those is still TBD.
>>
>> Opinions, comments, welcome!
>> ---
>>   backends/iommufd.c    | 29 +
>>   backends/trace-events |  1 +
>>   hw/vfio/iommufd.c | 78 +++
>>   include/hw/vfio/vfio-common.h |  9 
>>   include/sysemu/iommufd.h  |  4 ++
>>   5 files changed, 121 insertions(+)
>>
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index 8486894f1b3f..2970135af4b9 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -211,6 +211,35 @@ int iommufd_backend_unmap_dma(IOMMUFDBackend *be,
>> uint32_t ioas_id,
>>   return ret;
>>   }
>>
>> +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,
>> +    };
>> +
>> +    ret = ioctl(iommufd, IOMMU_HWPT_ALLOC, _hwpt);
>> +    trace_iommufd_backend_alloc_hwpt(iommufd, dev_id, pt_id, flags, 
>> data_type,
>> + data_len, (uint64_t)data_ptr,
>> + alloc_hwpt.out_hwpt_id, ret);
>> +    if (ret) {
>> +    error_report("IOMMU_HWPT_ALLOC failed: %m");
>> +    } else {
>> +    *out_hwpt = alloc_hwpt.out_hwpt_id;
>> +    }
>> +    return !ret ? 0 : -errno;
>> +}
>> +
>>   static const TypeInfo iommufd_backend_info = {
>>   .name = TYPE_IOMMUFD_BACKEND,
>>   .parent = TYPE_OBJECT,
>> diff --git a/backends/trace-events b/backends/trace-events
>> index d45c6e31a67e..f83a276a4253 100644
>> --- a/backends/trace-events
>> +++ b/backends/trace-events
>> @@ -13,5 +13,6 @@ iommu_backend_set_fd(int fd) "pre-opened /dev/iommu fd=%d"
>>   iommufd_backend_map_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t
>> size, void *vaddr, bool readonly, int ret) " iommufd=%d ioas=%d
>> iova=0x%"PRIx64" size=0x%"PRIx64" addr=%p readonly=%d (%d)"
>>   iommufd_backend_unmap_dma_non_exist(int iommufd, uint32_t ioas, uint64_t
>> iova, uint64_t size, int ret) " Unmap nonexistent mapping: iommufd=%d ioas=%d
>> iova=0x%"PRIx64" size=0x%"PRIx64" (%d)"
>>   iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova,
>> uint64_t size, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" 
>> size=0x%"PRIx64"
>> (%d)"
>> +iommufd_backend_alloc_hwpt(int iommufd, uint32_t dev_id, uint32_t pt_id,
>> uint32_t flags, uint32_t hwpt_type, uint32_t len, uint64_t data_ptr, uint32_t
>> out_hwpt_id, int ret) " iommufd=%d dev_id=%u pt_id=%u flags=0x%x hwpt_type=%u
>> len=%u data_ptr=0x%"PRIx64" out_hwpt=%u (%d)"
>>   iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas, int 

Re: [PATCH RFCv2 2/8] vfio/iommufd: Introduce auto domain creation

2024-02-19 Thread Avihai Horon

Hi Joao,

On 12/02/2024 15:56, Joao Martins wrote:

External email: Use caution opening links or attachments


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 
---
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. So opted for autodomains userspace approach to avoid the
duplication of hwpt-alloc flags vs attach-ioas flags. I lack mdev real
drivers atm, so testing with those is still TBD.

Opinions, comments, welcome!
---
  backends/iommufd.c| 29 +
  backends/trace-events |  1 +
  hw/vfio/iommufd.c | 78 +++
  include/hw/vfio/vfio-common.h |  9 
  include/sysemu/iommufd.h  |  4 ++
  5 files changed, 121 insertions(+)

diff --git a/backends/iommufd.c b/backends/iommufd.c
index 8486894f1b3f..2970135af4b9 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -211,6 +211,35 @@ int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t 
ioas_id,
  return ret;
  }

+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,
+};
+
+ret = ioctl(iommufd, IOMMU_HWPT_ALLOC, _hwpt);
+trace_iommufd_backend_alloc_hwpt(iommufd, dev_id, pt_id, flags, data_type,
+ data_len, (uint64_t)data_ptr,
+ alloc_hwpt.out_hwpt_id, ret);
+if (ret) {
+error_report("IOMMU_HWPT_ALLOC failed: %m");
+} else {
+*out_hwpt = alloc_hwpt.out_hwpt_id;
+}
+return !ret ? 0 : -errno;
+}
+
  static const TypeInfo iommufd_backend_info = {
  .name = TYPE_IOMMUFD_BACKEND,
  .parent = TYPE_OBJECT,
diff --git a/backends/trace-events b/backends/trace-events
index d45c6e31a67e..f83a276a4253 100644
--- a/backends/trace-events
+++ b/backends/trace-events
@@ -13,5 +13,6 @@ iommu_backend_set_fd(int fd) "pre-opened /dev/iommu fd=%d"
  iommufd_backend_map_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, void *vaddr, bool 
readonly, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" addr=%p 
readonly=%d (%d)"
  iommufd_backend_unmap_dma_non_exist(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) 
" Unmap nonexistent mapping: iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" 
(%d)"
  iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) " 
iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)"
+iommufd_backend_alloc_hwpt(int iommufd, uint32_t dev_id, uint32_t pt_id, uint32_t flags, uint32_t 
hwpt_type, uint32_t len, uint64_t data_ptr, uint32_t out_hwpt_id, int ret) " iommufd=%d 
dev_id=%u pt_id=%u flags=0x%x hwpt_type=%u len=%u data_ptr=0x%"PRIx64" out_hwpt=%u 
(%d)"
  iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas, int ret) " iommufd=%d 
ioas=%d (%d)"
  iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d 
(%d)"
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 7d39d7a5fa51..ca7ec45e725c 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -219,10 +219,82 @@ static int iommufd_cdev_detach_ioas_hwpt(VFIODevice 
*vbasedev, Error **errp)
  

Re: [PATCH RFCv2 2/8] vfio/iommufd: Introduce auto domain creation

2024-02-13 Thread Joao Martins
On 12/02/2024 13:56, Joao Martins wrote:
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index 8486894f1b3f..2970135af4b9 100644
> --- a/backends/iommufd.c
> +++ b/backends/iommufd.c
> @@ -211,6 +211,35 @@ int iommufd_backend_unmap_dma(IOMMUFDBackend *be, 
> uint32_t ioas_id,
>  return ret;
>  }
>  
> +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)

This function looks to be against the style of the file. Which is to have
IOMMUFDBackend as an argument rather than passing the file descriptor directly.

Likewise for dev_id now that we have IOMMUFDDevice structure.

I've fixed that for the next version (whether or not we require Zhengzhong 
series).

Joao



Re: [PATCH RFCv2 2/8] vfio/iommufd: Introduce auto domain creation

2024-02-12 Thread Joao Martins
On 12/02/2024 16:27, Jason Gunthorpe wrote:
> On Mon, Feb 12, 2024 at 01:56:37PM +, 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 
>> ---
>> 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, >hwpt_list, next) {
>> +ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id, );
>> +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;



Re: [PATCH RFCv2 2/8] vfio/iommufd: Introduce auto domain creation

2024-02-12 Thread Jason Gunthorpe
On Mon, Feb 12, 2024 at 01:56:37PM +, 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 
> ---
> 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.

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

> +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, >hwpt_list, next) {
> +ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id, );
> +if (ret) {
> +/* -EINVAL means the domain is incompatible with the device. */
> +if (ret == -EINVAL) {

Please send a kernel kdoc patch to document this..

The approach looks good to me

The nesting patches surely need this too?

Jason