From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Magnus Damm <magnus.damm@gmail.com>,
iommu@lists.linux-foundation.org,
laurent.pinchart+renesas@ideasonboard.com,
geert+renesas@glider.be, joro@8bytes.org,
linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
horms+renesas@verge.net.au, m.szyprowski@samsung.com
Subject: Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
Date: Sat, 12 Nov 2016 03:57:46 +0200 [thread overview]
Message-ID: <30201191.g28hZNr0Fc@avalon> (raw)
In-Reply-To: <dbb87293-4760-607f-841c-368a5692ecc9@arm.com>
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@opensource.se>
> >>>
> >>> 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@opensource.se>
> >>> ---
> >>>
> >>> 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
next prev parent reply other threads:[~2016-11-12 1:57 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-19 23:35 [PATCH v6 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V6 Magnus Damm
2016-10-19 23:35 ` [PATCH v6 01/07] iommu/ipmmu-vmsa: Remove platform data handling Magnus Damm
2016-10-19 23:35 ` [PATCH v6 02/07] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context Magnus Damm
2016-11-11 0:46 ` Laurent Pinchart
2016-10-19 23:36 ` [PATCH v6 03/07] iommu/ipmmu-vmsa: Break out utlb parsing code Magnus Damm
2016-11-11 1:00 ` Laurent Pinchart
2016-10-19 23:36 ` [PATCH v6 04/07] iommu/ipmmu-vmsa: Break out domain allocation code Magnus Damm
2016-11-11 1:02 ` Laurent Pinchart
2016-10-19 23:36 ` [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops Magnus Damm
2016-10-21 17:52 ` Robin Murphy
2016-11-10 11:42 ` Joerg Roedel
2016-11-11 1:13 ` Laurent Pinchart
2016-11-11 10:37 ` Joerg Roedel
2016-11-11 1:50 ` Laurent Pinchart
2016-11-11 14:44 ` Robin Murphy
2016-11-12 1:57 ` Laurent Pinchart [this message]
2016-11-11 1:24 ` Laurent Pinchart
2016-11-11 2:01 ` [PATCH] iommu/ipmmu-vmsa: Unify domain alloc/free implementations Laurent Pinchart
2016-10-19 23:36 ` [PATCH v6 06/07] iommu/ipmmu-vmsa: ARM and ARM64 archdata access Magnus Damm
2016-10-21 17:32 ` Robin Murphy
2016-11-11 1:27 ` Laurent Pinchart
2016-10-19 23:36 ` [PATCH v6 07/07] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency Magnus Damm
2016-11-10 11:42 ` [PATCH v6 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V6 Joerg Roedel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=30201191.g28hZNr0Fc@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=geert+renesas@glider.be \
--cc=horms+renesas@verge.net.au \
--cc=iommu@lists.linux-foundation.org \
--cc=joro@8bytes.org \
--cc=laurent.pinchart+renesas@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=magnus.damm@gmail.com \
--cc=robin.murphy@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).