RE: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Varun Sethi <Varun.Sethi-KZfg59tc24xl57MIdRCFDg@public.gmane.org>]

> -----Original Message-----
> From: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org [mailto:iommu-
> bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org] On Behalf Of Andreas Herrmann
> Sent: Wednesday, January 29, 2014 7:44 PM
> To: Sethi Varun-B16395
> Cc: Andreas Herrmann; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Will Deacon;
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> notifier block
> 
> On Tue, Jan 28, 2014 at 11:00:35AM +0000, Varun Sethi wrote:
> >
> >
> > > -----Original Message-----
> > > From: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org [mailto:iommu-
> > > bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org] On Behalf Of Andreas Herrmann
> > > Sent: Friday, January 24, 2014 1:27 AM
> > > To: Sethi Varun-B16395
> > > Cc: Andreas Herrmann; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Will Deacon;
> > > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> > > notifier block
> > >
> > > On Wed, Jan 22, 2014 at 07:07:40PM +0000, Varun Sethi wrote:
> > > > > -----Original Message-----
> > > > > From: Will Deacon [mailto:will.deacon-5wv7dgnIgG8@public.gmane.org]
> > > > > Sent: Wednesday, January 22, 2014 9:04 PM
> > > > > To: Sethi Varun-B16395
> > > > > Cc: Andreas Herrmann; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org;
> > > > > linux-arm- kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; Andreas Herrmann
> > > > > Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce
> > > > > iommu_group notifier block
> > > > >
> > > > > On Wed, Jan 22, 2014 at 01:54:11PM +0000, Varun Sethi wrote:
> > > > > > > > > Ok, so are you suggesting that we perform the isolation
> > > > > > > > > mapping in arm_smmu_add_device and drop the notifier
> > > altogether?
> > > > > > > > I think that should be fine, until we want to delay
> > > > > > > > mapping creation till driver bind time.
> > > > > > >
> > > > > > > Is there a hard dependency on that?
> > > > > > >
> > > > > > Not sure, may be Andreas can answer that.
> > > > >
> > > > > Ok. Andreas? I would have thought doing this *earlier* shouldn't
> > > > > be a problem (the DMA ops must be swizzled before the driver is
> probed).
> > > > >
> > > > > > > > But the "isolate device" property should dictate iommu
> > > > > > > > group
> > > > > creation.
> > > > > > >
> > > > > > > The reason we added automatic group creation (per-device) is
> > > > > > > for VFIO, which expects all devices to be in a group
> > > > > > > regardless of the device isolation configuration.
> > > > > > >
> > > > > > IIUC, with the "isolate devices" property we ensure that there
> > > > > > would be independent SMR and S2CR per device. Is that correct?
> > > > >
> > > > > Yes, as part of giving them independent sets of page tables.
> > > > > Initially these tables don't have any valid mappings, but the
> > > > > dma-mapping API will populate them in response to
> > > dma_map_*/dma_alloc/etc.
> > > > >
> > > > > Not sure what this has to do with us putting devices into their
> > > > > own groups...
> > >
> > > > [Sethi Varun-B16395] Devices in an iommu group would share the dma
> > > > mapping, so shouldn't they be sharing the SMR and context
> registers?
> > >
> > > I aggree with the context but SMRs won't be shared. I think you just
> > > can say that a certain subset of the available SMRs will be used to
> > > map all devices in an iommu group to the same context. Depending on
> > > what streamIDs each device has you might have to use separate SMRs
> > > for each device to map it to the same context.
> 
> > [Sethi Varun-B16395] IIUC the SMRs would unique per device, but there
> > is a possibility where the number of context registers are restricted?
> > In that case IOMMU groups should correspond to unique stream IDs (and
> > corresponding SMRs).
> 
> > > > In case of the "isolate devices" property, each device would have
> > > > its own SMR and context registers, thus allowing the devices to
> > > > have independent mappings and allowing them to fall in separate
> > > > iommu groups.
> > >
> > > I aggree with Varun's view here. (Now that I take iommu groups into
> > > consideration.)
> > >
> > > But my goal with the "isolate devices" thing was twofold:
> > >
> > > 1. actual make use of SMMU for address translation for all master
> > >   devices (instead of just bypassing the SMMU)
> 
> > [Sethi Varun-B16395] even if masters have to share translation? But
> > from the patch it seemed that we are creating mapping only if we find
> > the isolate devices property.
> 
> Yes, the patch creates mappings only if isolate-devices property is
> given. Currently there is no trigger to create a common mapping for all
> devices attached to the same SMMU.
> 
> > > plus
> > >
> > > 2. put each master into it's own domain to isolate it
> > >
> > > The latter matches usage of separate iommu groups for devices. If we
> > > now use the isolate devices property to just control whether master
> > > devices fall into the same or separate iommu groups it seems to me
> > > we would need to have another mechanism that triggers the creation
> > > of a mapping for a group.
> > >
> > > What do you think?
> > >
> 
> > [Sethi Varun-B16395] As mentioned before, we should be having iommu
> > group per device (having a unique stream id). Isolate devices property
> > just ensures that each device has a unique context. Now, for the
> > devices for which we don't have the isolate device property, can't we
> > have the multiple devices point to a common mapping.
> 
> Hmm, the isolate-devices option is per SMMU. So if this is set for an
> SMMU the driver tries to create a mapping per device. (And this is done
> for all master devices of this SMMU.)
> 
> Are you requesting to change the default behaviour of the driver to
> create a shared mapping in case the isolate-devices property is not
> specified in DT for an SMMU node?
> 
> Or maybe your concern is that you have x master devices for an SMMU which
> support y number of contexts and x > y? Which would imply that not all
> devices can be isolated and some need to share a context?
> 
[Sethi Varun-B16395] I am trying to understand the requirement for the "isolate devices" property. Currently no default in mapping is created in the SMMU driver. The new property allows default mapping to be created for all masters. Why can't we create default mappings for all masters without this property?

-Varun