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

[Richard Gemego <richardgemego@gmail.com>]

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