Re: [PATCH v5 5/9] iommufd: Add iommufd fault object

[Baolu Lu]

On 5/15/24 4:37 PM, Tian, Kevin wrote:
> > @@ -395,6 +396,8 @@ struct iommufd_device {
> >   	/* always the physical device */
> >   	struct device *dev;
> >   	bool enforce_cache_coherency;
> > +	/* outstanding faults awaiting response indexed by fault group id */
> > +	struct xarray faults;
> this...
>
> > +struct iommufd_fault {
> > +	struct iommufd_object obj;
> > +	struct iommufd_ctx *ictx;
> > +	struct file *filep;
> > +
> > +	/* The lists of outstanding faults protected by below mutex. */
> > +	struct mutex mutex;
> > +	struct list_head deliver;
> > +	struct list_head response;
> ...and here worth a discussion.
>
> First the response list is not used. If continuing the choice of queueing
> faults per device it should be removed.

You are right. I have removed the response list in the new version.

> But I wonder whether it makes more sense to keep this response
> queue per fault object. sounds simpler to me.
>
> Also it's unclear why we need the response message to carry the
> same info as the request while only id/code/cookie are used.
>
> +struct iommu_hwpt_page_response {
> +	__u32 size;
> +	__u32 flags;
> +	__u32 dev_id;
> +	__u32 pasid;
> +	__u32 grpid;
> +	__u32 code;
> +	__u32 cookie;
> +	__u32 reserved;
> +};
>
> If we keep the response queue in the fault object, the response message
> only needs to carry size/flags/code/cookie and cookie can identify the
> pending message uniquely in the response queue.

It seems fine from the code's point of view. Let's wait and see whether
there are any concerns from the uAPI's perspective.

Best regards,
baolu