Re: [PATCH 7/7] iommu: Add iommu_domain::domain_ops

[Lu Baolu]

On 1/24/22 5:37 PM, Christoph Hellwig wrote:
> On Mon, Jan 24, 2022 at 03:11:02PM +0800, Lu Baolu wrote:
> > Add a domain specific callback set, domain_ops, for vendor iommu driver
> > to provide domain specific operations. Move domain-specific callbacks
> > from iommu_ops to the domain_ops and hook them when a domain is allocated.
>
> Ah, that's what I meant earlier.  Perfect!
>
> Nit:  I don't think vendor is the right term here.
>
> Maybe something like:
>
> iommut: split struct iommu_ops
>
> Move the domain specific operations out of struct iommu_ops into a new
> structure that only has domain specific operations.  This solves
> the problem of needing to know if the method vector for a given
> operation needs to be retreived from the device or th domain.

Sure. Will use above description.

> > +struct domain_ops {
>
> This needs to keep an iommu in the name, i.e. iommu_domain_ops.

Sure.

> > +	phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova);
>
> Overly long line.

./scripts/checkpatch.pl --strict *.patch

didn't give me a WARN or CHECK. I will make it short anyway.

> > +static inline void iommu_domain_set_ops(struct iommu_domain *domain,
> > +					const struct domain_ops *ops)
> > +{
> > +	domain->ops = ops;
> > +}
>
> Do we really need this helper?

Unnecessary. I can set the pointer directly in the drivers.

> > +static const struct domain_ops amd_domain_ops;
>
> Can we avoid these forward declarations or would that cause too much
> churn?

I don't like this either. But it's common to put the ops at the bottom
of the file in almost all iommu drivers.

Best regards,
baolu