Re: [PATCH v2] iommufd/device: Enforce reserved IOVA also when attached to hwpt_nested

[Nicolin Chen <nicolinc@nvidia.com>]

On Tue, Aug 06, 2024 at 02:42:42PM -0300, Jason Gunthorpe wrote:
> > -static int iommufd_hwpt_paging_attach(struct iommufd_hwpt_paging *hwpt_paging,
> > -				      struct iommufd_device *idev)
> > +static int
> > +iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
> > +				    struct iommufd_hw_pagetable *hwpt)
> >  {
> > +	struct iommufd_hwpt_paging *hwpt_paging = to_hwpt_paging(hwpt);
> >  	int rc;
> 
> This seems like the wrong place to put these types, a big point point of the
> struct iommufd_hwpt_paging was to mark functions that should only be
> operating on a paging hwpt.
> 
> The caller is expected to figure out what it is doing.
> 
> >  int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
> >  				struct iommufd_device *idev)
> >  {
> > @@ -363,11 +380,9 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
> >  		goto err_unlock;
> >  	}
> >  
> > -	if (hwpt_is_paging(hwpt)) {
> > -		rc = iommufd_hwpt_paging_attach(to_hwpt_paging(hwpt), idev);
> > -		if (rc)
> > -			goto err_unlock;
> > -	}
> 
> Like we had here, so it would be a bit nicer to write it as more like:
> 
> hwpt_paging = to_hwpt_paging(hwpt);
> if (hwpt_paging) {
> 	rc = iommufd_hwpt_paging_attach(hwpt_paging, idev);
> }
> 
> Then we can keep the clearer labeling of the function signatures.

Okay. Will keep the hwpt_paging type in the function signatures.

> > @@ -321,7 +321,20 @@ static inline bool hwpt_is_paging(struct iommufd_hw_pagetable *hwpt)
> >  static inline struct iommufd_hwpt_paging *
> >  to_hwpt_paging(struct iommufd_hw_pagetable *hwpt)
> >  {
> > -	return container_of(hwpt, struct iommufd_hwpt_paging, common);
> > +	switch (hwpt->obj.type) {
> > +	case IOMMUFD_OBJ_HWPT_PAGING:
> > +		return container_of(hwpt, struct iommufd_hwpt_paging, common);
> > +	case IOMMUFD_OBJ_HWPT_NESTED:
> > +		return container_of(hwpt, struct iommufd_hwpt_nested, common)->parent;
> > +	default:
> > +		return NULL;
> > +	}
> > +}
> 
> There are alot of existing callers of this, I think it should get a
> new function to do this behavior and it would only be used in a few
> places.
> 
> It is not OK for a NESTING to get into most of the places that are
> already calling this.

OK. Let's add a find_hwpt_paging(struct iommufd_hw_pagetable *);

Thanks
Nicolin