Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

Hi Robin,

On Friday 11 Nov 2016 14:44:29 Robin Murphy wrote:
> On 11/11/16 01:50, Laurent Pinchart wrote:
> > On Friday 21 Oct 2016 18:52:53 Robin Murphy wrote:
> >> On 20/10/16 00:36, Magnus Damm wrote:
> >>> From: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
> >>> 
> >>> Introduce an alternative set of iommu_ops suitable for 64-bit ARM
> >>> as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. Also adjust the
> >>> Kconfig to depend on ARM or IOMMU_DMA.
> >>> 
> >>> Signed-off-by: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
> >>> ---
> >>> 
> >>>  Changes since V5:
> >>>  - Made domain allocation/free code more consistent - thanks Joerg!
> >>>  
> >>>  Changes since V4:
> >>>  - Added Kconfig hunk to depend on ARM or IOMMU_DMA
> >>>  
> >>>  Changes since V3:
> >>>  - Removed group parameter from ipmmu_init_platform_device()
> >>>  
> >>>  Changes since V2:
> >>>  
> >>>  - Included this new patch from the following series:
> >>>    [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
> >>>  
> >>>  - Use only a single iommu_ops structure with #ifdef CONFIG_IOMMU_DMA
> >>>  - Folded in #ifdefs to handle CONFIG_ARM and CONFIG_IOMMU_DMA
> >>>  - of_xlate() is now used without #ifdefs
> >>>  - Made sure code compiles on both 32-bit and 64-bit ARM.
> >>>  
> >>>  drivers/iommu/Kconfig      |    1
> >>>  drivers/iommu/ipmmu-vmsa.c |  122 +++++++++++++++++++++++++++++++++---
> >>>  2 files changed, 115 insertions(+), 8 deletions(-)
> > 
> > [snip]
> > 
> >>> --- 0006/drivers/iommu/ipmmu-vmsa.c
> >>> +++ work/drivers/iommu/ipmmu-vmsa.c
> >>> 2016-10-20 08:16:48.440607110 +0900
> > 
> > [snip]
> > 
> >>> -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> >>> -{
> >>> -	if (type != IOMMU_DOMAIN_UNMANAGED)
> >>> -		return NULL;
> >> 
> >> I *think* that if we did the initial check thus:
> >> 	if (type != IOMMU_DOMAIN_UNMANAGED ||
> >> 	    (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
> >> 		return NULL;
> > 
> > I assume you meant
> > 
> >  	if (type != IOMMU_DOMAIN_UNMANAGED &&
> >  	    (!IS_ENABLED(CONFIG_IOMMU_DMA) || type != IOMMU_DOMAIN_DMA))
> >  		return NULL;
> > 
> > But how about just
> > 
> >  	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA))
> >  		return NULL;
> > 
> > as type will never be set to IOMMU_DOMAIN_DMA on ARM32 ?
> 
> Actually it can be, but *only* at this point, because
> iommu_group_get_for_dev() will always attempt to allocate a default
> domain.

If I'm not mistaken iommu_group_get_for_dev() is the only function that tries 
to create an IOMMU_DOMAIN_DMA domain, and is only called in core code by 
iommu_request_dm_for_dev(). That function isn't called by the IPMMU driver, 
and with Magnus' patches the IPMMU driver calls iommu_group_get_for_dev() on 
ARM64 platforms only (in ipmmu_add_device_dma(), only compiled in when 
CONFIG_IOMMU_DMA is set, which only happens on ARM64).

> Having the additional check up-front just saves going through
> the whole IOVA domain allocation only to tear it all down again when
> get_cookie() returns -ENODEV. You're right that it's not strictly
> necessary (and that I got my DeMorganning wrong), though.
> 
> Robin.
> 
> >> it shouldn't be necessary to split the function at all - we then just
> >> wrap the {get,put}_cookie() bits in "if (type ==  IOMMU_DOMAIN_DMA)" and
> >> in the 32-bit ARM case they just don't run as that can never be true.
> >> 
> >>> -
> >>> -	return __ipmmu_domain_alloc(type);
> >>> -}
> >>> -

-- 
Regards,

Laurent Pinchart