> From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> Sent: Monday, January 9, 2023 10:55 PM
>
> >> + mutex_lock(&device->dev_set->lock);
> >> + pt_id = attach.pt_id;
> >> + ret = vfio_iommufd_attach(device,
> >> + pt_id != IOMMUFD_INVALID_ID ? &pt_id :
> >> NULL);
> >> + if (ret)
> >> + goto out_unlock;
> >> +
> >> + if (pt_id != IOMMUFD_INVALID_ID) {
> >
> > it's clearer to use an 'attach' local variable
>
> not quit get. We already have 'attach' in above lines.:-)
Probably a different name. I just meant a variable which is
more descriptive than above condition check (and duplicated
in two places)
>
> >> diff --git a/include/uapi/linux/iommufd.h
> b/include/uapi/linux/iommufd.h
> >> index 98ebba80cfa1..87680274c01b 100644
> >> --- a/include/uapi/linux/iommufd.h
> >> +++ b/include/uapi/linux/iommufd.h
> >> @@ -9,6 +9,8 @@
> >>
> >> #define IOMMUFD_TYPE (';')
> >>
> >> +#define IOMMUFD_INVALID_ID 0 /* valid ID starts from 1 */
> >
> > Can you point out where valid IDs are guaranteed to start
> > from 1?
> >
> > According to _iommufd_object_alloc() it uses xa_limit_32b as
> > limit which includes '0' as the lowest ID
>
> xa_init_flags(&ictx->objects, XA_FLAGS_ALLOC1 | XA_FLAGS_ACCOUNT);
>
> yes, but the xarray init uses XA_FLAGS_ALLOC1, and it means to allocate
> ID from 1.
>
> /* ALLOC is for a normal 0-based alloc. ALLOC1 is for an 1-based alloc */
You are right. I overlooked that flag.
