RE: [RFC PATCH 0/6] Auxiliary IOMMU domains and Arm SMMUv3

["Tian, Kevin" <kevin.tian-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

> From: 'joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org' [mailto:joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org]
> Sent: Friday, December 7, 2018 6:29 PM
> 
> Hi,
> 
> On Mon, Nov 26, 2018 at 07:29:45AM +0000, Tian, Kevin wrote:
> > btw Baolu just reminded me one thing which is worthy of noting here.
> > 'primary' vs. 'aux' concept makes sense only when we look from a device
> > p.o.v. That binding relationship is not (*should not be*) carry-and-
> forwarded
> > cross devices. every domain must be explicitly attached to other devices
> > (instead of implicitly attached being above example), and new
> primary/aux
> > attribute on another device will be decided at attach time.
> 
> Okay, so after all the discussions we had I learned a few more things
> about the scalable mode feature and thought a bit longer about how to
> best support it in the IOMMU-API.

Thanks for thinking through this.

> 
> The concept of sub-domains I initially proposed certainly makes no
> sense, but scalable-mode specific attach/detach functions do. So instead
> of a sub-domain mode, I'd like to propose device-feature sets.

Can I interpret above as that you agree with the aux domain concept (i.e. one
device can be linked to multiple domains) in general, and now we're just trying
to address the remaining open in API level?

> 
> The posted patch-set already includes this as device-attributes, but I
> don't like this naming as we are really talking about additional
> feature sets of a device. So how about we introduce this:
> 
> 	enum iommu_dev_features {
> 		/* ... */
> 		IOMMU_DEV_FEAT_AUX,
> 		IOMMU_DEV_FEAT_SVA,
> 		/* ... */
> 	};
> 

Does above represent whether a device implements aux/sva features,
or whether a device has been enabled by driver to support aux/sva 
features?

> 	/* Check if a device supports a given feature of the IOMMU-API */
> 	bool iommu_dev_has_feature(struct device *dev, enum
> iommu_dev_features *feat);

If the latter we also need iommu_dev_set_feature so driver can poke
it based on its own configuration. 

> 
> 	/*
> 	 * Only works if iommu_dev_has_feature(dev,
> IOMMU_DEV_FEAT_AUX)
> 	 * returns true
> 	 *
> 	 * Also, as long as domains are attached to a device through
> 	 * this interface, any trys to call iommu_attach_device() should
> 	 * fail (iommu_detach_device() can't fail, so we fail on the
> 	 * tryint to re-attach). This should make us safe against a
> 	 * device being attached to a guest as a whole while there are
> 	 * still pasid users on it (aux and sva).

yes, it makes sense.

> 	 */
> 	int iommu_aux_attach_device(struct iommu_domain *domain,
> 				    struct device *dev);
> 
> 	int iommu_aux_detach_device(struct iommu_domain *domain,
> 				    struct device *dev);
> 	/*
> 	 * I know we are targeting a system-wide pasid-space, so that
> 	 * the pasid would be the same for one domain on all devices,
> 	 * let's just keep the option open to have different
> 	 * pasid-spaces in one system. Also this way we can use it to
> 	 * check whether the domain is attached to this device at all.
> 	 *
> 	 * returns pasid or <0 if domain has no pasid on that device.
> 	 */
> 	int iommu_aux_get_pasid(struct iommu_domain *domain,
> 				struct device *dev);
> 
> 	/* So we need a iommu_aux_detach_all()? */

for what scenario?

> 
> This concept can also be easily extended for supporting SVA in parallel
> on the same device, with the same constraints regarding the behavior of
> iommu_attach_device()/iommu_detach_device().
> 
> So what do you think about that approach?
> 
> Regards,
> 
> 	Joerg
> 
>