On Tue, 15 Jan 2019 10:10:21 +0800
Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
> Hi,
>
> On 1/14/19 8:26 PM, Jonathan Cameron wrote:
> > On Thu, 10 Jan 2019 11:00:23 +0800
> > Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
> >
> >> When multiple domains per device has been enabled by the
> >> device driver, the device will tag the default PASID for
> >> the domain to all DMA traffics out of the subset of this
> >> device; and the IOMMU should translate the DMA requests
> >> in PASID granularity.
> >>
> >> This adds the intel_iommu_aux_attach/detach_device() ops
> >> to support managing PASID granular translation structures
> >> when the device driver has enabled multiple domains per
> >> device.
> >>
> >> Cc: Ashok Raj <ashok.raj@xxxxxxxxx>
> >> Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> >> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> >> Signed-off-by: Sanjay Kumar <sanjay.k.kumar@xxxxxxxxx>
> >> Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx>
> >> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >
> > The following is probably a rather naive review given I don't know
> > the driver or hardware well at all. Still, it seems like things
> > are a lot less balanced than I'd expect and isn't totally obvious
> > to me why that is.
>
> Thank you!
You are welcome.
...
> >> +/*
> >> + * Check whether a @domain could be attached to the @dev through the
> >> + * aux-domain attach/detach APIs.
> >> + */
> >> +static inline bool
> >> +is_aux_domain(struct device *dev, struct iommu_domain *domain)
> >
> > I'm finding the distinction between an aux domain capability on
> > a given device and whether one is actually in use to be obscured
> > slightly in the function naming.
> >
> > This one for example is actually checking if we have a domain
> > that is capable of being enabled for aux domain use, but not
> > yet actually in that mode?
> >
> > Mind you I'm not sure I have a better answer for the naming.
> > can_aux_domain_be_enabled? is_unattached_aux_domain?
> >
> >
>
> device aux mode vs. normal mode
> ===============================
>
> When we talk about the auxiliary mode (simply aux-mode), it means "the
> device works in aux-mode or normal mode". "normal mode" means that the
> device (and it's corresponding IOMMU) supports only RID (PCI Request ID)
> based DMA translation; while, aux-mode means the the device (and it's
> IOMMU) supports fine-grained DMA translation, like PASID based DMA
> translation with Intel VT-d scalable mode.
>
> We are adding below APIs to switch a device between these two modes:
>
> int iommu_dev_enable/disable_feature(dev, IOMMU_DEV_FEAT_AUX)
>
> And this API (still under discussion) to check which mode the device is
> working in:
>
> bool iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_AUX)
>
> aux-domain
> ==========
>
> If a device is working in aux-mode and we are going to attach a domain
> to this device, we say "this domain will be attached to the device in
> aux mode", and simply "aux domain". So a domain is "normal" when it is
> going to attach to a device in normal mode; and is "aux-domain" when it
> is going to attach to a device in aux mode.
Hmm.. OK I guess. It still feels like there is more need to refer to
the docs than there should be. Still, your code and I may well never
read it again so I don't mind :)
>
> >
> >> +{
