[PATCH] iommufd: fix returns ENOENT in iommufd_viommu_get_vdev_id()

[PATCH] iommufd: fix returns ENOENT in iommufd_viommu_get_vdev_id()

[Richard Gemego]

In iommufd_viommu_get_vdev_id(), if viommu does not have any vdevs, fix
this func to return 0 instead of ENOENT.

Fixes: ea94b211c548 ("iommufd/viommu: Add iommufd_viommu_get_vdev_id helper")
Signed-off-by: Richard Gemego <richardgemego@gmail.com>
Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
---
 drivers/iommu/iommufd/driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
index 922cd1fe7ec2..ad635af8555b 100644
--- a/drivers/iommu/iommufd/driver.c
+++ b/drivers/iommu/iommufd/driver.c
@@ -68,6 +68,7 @@ int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
 			break;
 		}
 	}
+	if (!index) 
+		rc = 0;
 	xa_unlock(&viommu->vdevs);
 	return rc;
 }
-- 
2.43.0

Re: [PATCH] iommufd: fix returns ENOENT in iommufd_viommu_get_vdev_id()

[Pranjal Shrivastava]

On Mon, Aug 04, 2025 at 06:08:59PM +0800, Richard Gemego wrote:

Hi Richard, 
> In iommufd_viommu_get_vdev_id(), if viommu does not have any vdevs, fix
> this func to return 0 instead of ENOENT.
> 
> Fixes: ea94b211c548 ("iommufd/viommu: Add iommufd_viommu_get_vdev_id helper")
> Signed-off-by: Richard Gemego <richardgemego@gmail.com>
> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
> ---
>  drivers/iommu/iommufd/driver.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
> index 922cd1fe7ec2..ad635af8555b 100644
> --- a/drivers/iommu/iommufd/driver.c
> +++ b/drivers/iommu/iommufd/driver.c
> @@ -68,6 +68,7 @@ int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
>  			break;
>  		}
>  	}
> +	if (!index) 
> +		rc = 0;
>  	xa_unlock(&viommu->vdevs);
>  	return rc;
>  }

I'm afraid, I don't understand the motivation behind this? IIUC, if a 
dev is not associated to the vIOMMU -ENOENT is returned. This patch
seems to be returning 0 when the xarray is empty. The issue is that
returning 0, would indicate the caller that `*vdev_id` holds a valid 
value which doesn't seem to be the case here?

It seems the original behavior is correct. Nicolin/Jason am I missing
something here?

Thanks,
Praan.

Re: [PATCH] iommufd: fix returns ENOENT in iommufd_viommu_get_vdev_id()

[Nicolin Chen]

On Mon, Aug 04, 2025 at 11:25:42AM +0000, Pranjal Shrivastava wrote:
> On Mon, Aug 04, 2025 at 06:08:59PM +0800, Richard Gemego wrote:
> > In iommufd_viommu_get_vdev_id(), if viommu does not have any vdevs, fix
> > this func to return 0 instead of ENOENT.

I think this is missing some justification explaining why this is
a fix.

> > Fixes: ea94b211c548 ("iommufd/viommu: Add iommufd_viommu_get_vdev_id helper")
> > Signed-off-by: Richard Gemego <richardgemego@gmail.com>
> > Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
> > ---
> >  drivers/iommu/iommufd/driver.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
> > index 922cd1fe7ec2..ad635af8555b 100644
> > --- a/drivers/iommu/iommufd/driver.c
> > +++ b/drivers/iommu/iommufd/driver.c
> > @@ -68,6 +68,7 @@ int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
> >  			break;
> >  		}
> >  	}
> > +	if (!index) 
> > +		rc = 0;
> >  	xa_unlock(&viommu->vdevs);
> >  	return rc;
> >  }
> 
> I'm afraid, I don't understand the motivation behind this? IIUC, if a 
> dev is not associated to the vIOMMU -ENOENT is returned. This patch
> seems to be returning 0 when the xarray is empty. The issue is that
> returning 0, would indicate the caller that `*vdev_id` holds a valid 
> value which doesn't seem to be the case here?

The function is designed to return -ENOENT for an empty xarray:

/* Return -ENOENT if device is not associated to the vIOMMU */

And 0 would be a valid vdev ID in the xarray.

Nicolin

Re: [PATCH] iommufd: fix returns ENOENT in iommufd_viommu_get_vdev_id()

[Richard Gemego]

Hello, maintainers!

The motivation of this fix is when iommufd in selftests tested on RISC-V,

I found that `alloc_hwpt_nested` items are failed, then I followed the

function calling chains and found that in iommufd_viommu_get_vdev_id(),

there is always not any vdev in viommu. I wonder if it is because RISC-V

does not support vIOMMU well, so that there isn't any any vdev in

viommu, then iommufd_viommu_get_vdev_id() will always return

-ENOENT. Have you ever tested it on RISC-V? Will viommu always have

at least one vdev on ARM (since you have mentioned ARM in ea94b211c548

patch series), so iommufd_viommu_get_vdev_id() can return 0 in iommufd

selftest?

Thank you!

On 8/5/2025 1:44 AM, Nicolin Chen wrote:
> On Mon, Aug 04, 2025 at 11:25:42AM +0000, Pranjal Shrivastava wrote:
>> On Mon, Aug 04, 2025 at 06:08:59PM +0800, Richard Gemego wrote:
>>> In iommufd_viommu_get_vdev_id(), if viommu does not have any vdevs, fix
>>> this func to return 0 instead of ENOENT.
> I think this is missing some justification explaining why this is
> a fix.
>
>>> Fixes: ea94b211c548 ("iommufd/viommu: Add iommufd_viommu_get_vdev_id helper")
>>> Signed-off-by: Richard Gemego <richardgemego@gmail.com>
>>> Signed-off-by: Quan Zhou <zhouquan@iscas.ac.cn>
>>> ---
>>>   drivers/iommu/iommufd/driver.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
>>> index 922cd1fe7ec2..ad635af8555b 100644
>>> --- a/drivers/iommu/iommufd/driver.c
>>> +++ b/drivers/iommu/iommufd/driver.c
>>> @@ -68,6 +68,7 @@ int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
>>>   			break;
>>>   		}
>>>   	}
>>> +	if (!index)
>>> +		rc = 0;
>>>   	xa_unlock(&viommu->vdevs);
>>>   	return rc;
>>>   }
>> I'm afraid, I don't understand the motivation behind this? IIUC, if a
>> dev is not associated to the vIOMMU -ENOENT is returned. This patch
>> seems to be returning 0 when the xarray is empty. The issue is that
>> returning 0, would indicate the caller that `*vdev_id` holds a valid
>> value which doesn't seem to be the case here?
> The function is designed to return -ENOENT for an empty xarray:
>
> /* Return -ENOENT if device is not associated to the vIOMMU */
>
> And 0 would be a valid vdev ID in the xarray.
>
> Nicolin

Re: [PATCH] iommufd: fix returns ENOENT in iommufd_viommu_get_vdev_id()

[Nicolin Chen]

On Tue, Aug 05, 2025 at 10:46:20AM +0800, Richard Gemego wrote:
> The motivation of this fix is when iommufd in selftests tested on RISC-V,
> 
> I found that `alloc_hwpt_nested` items are failed

That TEST_F is a non-viommu case, using the hwpt model, i.e.
mock_domain_alloc_nested() that does not set mock_viommu pointer,
leaving it a NULL value. Any vIOMMU/vdev specific thing should not
be invoked in this TEST_F.

> then I followed the
> function calling chains and found that in iommufd_viommu_get_vdev_id(),
> there is always not any vdev in viommu.

Then mock_domain_nop_attach() won't call iommufd_viommu_get_vdev_id
at all since the "new_viommu" there should be kept as NULL.

If you see this function is called for alloc_hwpt_nested TEST_F,
there must be something else going wrong. I would start to check
the mock_viommu pointer to see if it is set (and then by what).

> I wonder if it is because RISC-V
> does not support vIOMMU well, so that there isn't any any vdev in
> viommu, then iommufd_viommu_get_vdev_id() will always return
> -ENOENT. Have you ever tested it on RISC-V? Will viommu always have
> at least one vdev on ARM (since you have mentioned ARM in ea94b211c548
> patch series), so iommufd_viommu_get_vdev_id() can return 0 in iommufd
> selftest?

No. The underlying architecture should be independent, although the
kernel config file or compiler can be a factor.

Nicolin