Re: [PATCH v8 06/12] ARM: dts: Add description of System MMU of Exynos SoCs

2013-10-07 Thread Rob Herring
On Fri, Jul 26, 2013 at 6:28 AM, Cho KyongHo  wrote:
> Signed-off-by: Cho KyongHo 
> ---
>  .../bindings/iommu/samsung,exynos4210-sysmmu.txt   |  103 +++
>  arch/arm/boot/dts/exynos4.dtsi |  122 
>  arch/arm/boot/dts/exynos4210.dtsi  |   25 ++
>  arch/arm/boot/dts/exynos4x12.dtsi  |   76 +
>  arch/arm/boot/dts/exynos5250.dtsi  |  291 
> 
>  5 files changed, 617 insertions(+), 0 deletions(-)
>  create mode 100644 
> Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
>
> diff --git 
> a/Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
> b/Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
> new file mode 100644
> index 000..92f0a33
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
> @@ -0,0 +1,103 @@
> +Samsung Exynos4210 IOMMU H/W, System MMU (System Memory Management Unit)
> +
> +Samsung's Exynos architecture contains System MMU that enables scattered
> +physical memory chunks visible as a contiguous region to DMA-capable 
> peripheral
> +devices like MFC, FIMC, FIMD, GScaler, FIMC-IS and so forth.
> +
> +System MMU is a sort of IOMMU and support identical translation table format 
> to
> +ARMv7 translation tables with minimum set of page properties including access
> +permissions, shareability and security protection. In addition, System MMU 
> has
> +another capabilities like L2 TLB or block-fetch buffers to minimize 
> translation
> +latency.
> +
> +A System MMU is dedicated to a single master peripheral device.  Thus, it is
> +important to specify the correct System MMU in the device node of its master
> +device. Whereas a System MMU is dedicated to a master device, the master 
> device
> +may have more than one System MMU.
> +
> +Required properties:
> +- compatible: Should be "samsung,exynos4210-sysmmu"
> +- reg: A tuple of base address and size of System MMU registers.
> +- interrupt-parent: The phandle of the interrupt controller of System MMU
> +- interrupts: A tuple of numbers that indicates the interrupt source.
> +- clock-names: Should be "sysmmu" if the System MMU is needed to gate its 
> clock.
> +   Please refer to the following documents:
> +  Documentation/devicetree/bindings/clock/clock-bindings.txt
> +  Documentation/devicetree/bindings/clock/exynos4-clock.txt
> +  Documentation/devicetree/bindings/clock/exynos5250-clock.txt
> +  Optional "master" if the clock to the System MMU is gated by
> +  another gate clock other than "sysmmu". The System MMU driver
> +  sets "master" the parent of "sysmmu".
> +  Exynos4 SoCs, there needs no "master" clocks.
> +  Exynos5 SoCs, some System MMUs must have "master" clocks.
> +- clocks: Required if the System MMU is needed to gate its clock.
> + Please refer to the documents listed above.
> +- samsung,power-domain: Required if the System MMU is needed to gate its 
> power.
> + Please refer to the following document:
> + Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> +
> +Required properties for the master peripheral devices:
> +- iommu: phandles to the System MMUs of the device

You have not addressed my comments from the last version. We do not
need 2 (or more) different ways to describe the connection between
masters and iommu's. Use mmu-masters property here to describe the
connection.

Rob
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH 7/9] iommu/arm-smmu: Add function that conditionally isolates all masters of all SMMUs

2013-10-07 Thread Andreas Herrmann
On Fri, Sep 27, 2013 at 09:00:01AM -0400, Will Deacon wrote:
> Hi Andreas,
> 
> On Thu, Sep 26, 2013 at 11:36:19PM +0100, Andreas Herrmann wrote:
> > (Depending on DT information and module parameters) each device is put
> > into its own protection domain (if possible).  For configuration with
> > one or just a few masters per SMMU that is easy to achieve.
> > 
> > In case of many devices per SMMU (e.g. MMU-500 with it's distributed
> > translation support) isolation of each device might not be possible --
> > depending on number of available SMR groups and/or context banks.
> > 
> > Default is that device isolation is contolled per SMMU with SMMU node
> > property "linux,arm-smmu-isolate-devices" in a DT. If the property is
> > set for an SMMU node device isolation is performed.
> > 
> > Also introduce a module parameter:
> 
> Actually, I think I'd rather do away with the module paramater / command
> line option altogether in favour of DT.
> 
> > +extern struct platform_device *of_find_device_by_node(struct device_node 
> > *np);
> > +
> > +static int arm_smmu_isolate_devices(void)
> > +{
> > +   struct dma_iommu_mapping *mapping;
> > +   struct arm_smmu_device *smmu;
> > +   struct rb_node *rbn;
> > +   struct arm_smmu_master *master;
> > +   struct platform_device *pdev;
> > +   struct device *dev;
> > +   void __iomem *gr0_base;
> > +   u32 cr0;
> > +   int ret = 0;
> > +   size_t size;
> > +
> > +   list_for_each_entry(smmu, &arm_smmu_devices, list) {
> > +   if (arm_smmu_disable_isolation ||
> > +   (!(smmu->features & ARM_SMMU_FEAT_ISOLATE_DEVICES)
> > +   && !arm_smmu_force_isolation))
> > +   continue;
> > +   rbn = rb_first(&smmu->masters);
> > +   while (rbn) {
> > +   master = container_of(rbn, struct arm_smmu_master, 
> > node);
> > +   pdev = of_find_device_by_node(master->of_node);
> > +   if (!pdev)
> > +   break;
> > +   dev = &pdev->dev;
> > +
> > +   size = (size_t) dev->coherent_dma_mask;
> > +   size = size ? : (unsigned long) dev->dma_mask;
> 
> Hmm, this could be *huge* with 64-bit capable DMA controllers (think LPAE).

Yes, agreed.
(And even for 32-bit DMA this requires a large bitmap_size for the
mapping.)

> Russell also has some pending dma mask cleanup, which might break some
> assumptions here:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/199397.html
> 
> (namely that we're offsetting everything from zero).

What do you think is a reasonable general value for the size of a
mapping? (Do we need a DT property to specify this?)

What about a size of 128 MB -- if I'm not mistaken this requires a
bitmap_size of 4K.


Andreas
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device

2013-10-07 Thread Alex Williamson
On Mon, 2013-10-07 at 05:46 +, Bhushan Bharat-R65777 wrote:
> 
> > -Original Message-
> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Friday, October 04, 2013 11:42 PM
> > To: Bhushan Bharat-R65777
> > Cc: j...@8bytes.org; b...@kernel.crashing.org; ga...@kernel.crashing.org; 
> > linux-
> > ker...@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux-
> > p...@vger.kernel.org; ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > foundation.org
> > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a device
> > 
> > On Fri, 2013-10-04 at 17:23 +, Bhushan Bharat-R65777 wrote:
> > >
> > > > -Original Message-
> > > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > > Sent: Friday, October 04, 2013 10:43 PM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: j...@8bytes.org; b...@kernel.crashing.org;
> > > > ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
> > > > linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
> > > > ag...@suse.de; Wood Scott-B07421; iommu@lists.linux- foundation.org
> > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > device
> > > >
> > > > On Fri, 2013-10-04 at 16:47 +, Bhushan Bharat-R65777 wrote:
> > > > >
> > > > > > -Original Message-
> > > > > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > > > > Sent: Friday, October 04, 2013 9:15 PM
> > > > > > To: Bhushan Bharat-R65777
> > > > > > Cc: j...@8bytes.org; b...@kernel.crashing.org;
> > > > > > ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
> > > > > > linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
> > > > > > ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > > foundation.org
> > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain of a
> > > > > > device
> > > > > >
> > > > > > On Fri, 2013-10-04 at 09:54 +, Bhushan Bharat-R65777 wrote:
> > > > > > >
> > > > > > > > -Original Message-
> > > > > > > > From: linux-pci-ow...@vger.kernel.org
> > > > > > > > [mailto:linux-pci-ow...@vger.kernel.org]
> > > > > > > > On Behalf Of Alex Williamson
> > > > > > > > Sent: Wednesday, September 25, 2013 10:16 PM
> > > > > > > > To: Bhushan Bharat-R65777
> > > > > > > > Cc: j...@8bytes.org; b...@kernel.crashing.org;
> > > > > > > > ga...@kernel.crashing.org; linux- ker...@vger.kernel.org;
> > > > > > > > linuxppc-...@lists.ozlabs.org; linux- p...@vger.kernel.org;
> > > > > > > > ag...@suse.de; Wood Scott-B07421; iommu@lists.linux-
> > > > > > > > foundation.org; Bhushan Bharat-R65777
> > > > > > > > Subject: Re: [PATCH 2/7] iommu: add api to get iommu_domain
> > > > > > > > of a device
> > > > > > > >
> > > > > > > > On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > > > > > > > > This api return the iommu domain to which the device is 
> > > > > > > > > attached.
> > > > > > > > > The iommu_domain is required for making API calls related to
> > iommu.
> > > > > > > > > Follow up patches which use this API to know iommu maping.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Bharat Bhushan
> > > > > > > > > 
> > > > > > > > > ---
> > > > > > > > >  drivers/iommu/iommu.c |   10 ++
> > > > > > > > >  include/linux/iommu.h |7 +++
> > > > > > > > >  2 files changed, 17 insertions(+), 0 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > > > > > > index
> > > > > > > > > fbe9ca7..6ac5f50 100644
> > > > > > > > > --- a/drivers/iommu/iommu.c
> > > > > > > > > +++ b/drivers/iommu/iommu.c
> > > > > > > > > @@ -696,6 +696,16 @@ void iommu_detach_device(struct
> > > > > > > > > iommu_domain *domain, struct device *dev)  }
> > > > > > > > > EXPORT_SYMBOL_GPL(iommu_detach_device);
> > > > > > > > >
> > > > > > > > > +struct iommu_domain *iommu_get_dev_domain(struct device 
> > > > > > > > > *dev) {
> > > > > > > > > + struct iommu_ops *ops = dev->bus->iommu_ops;
> > > > > > > > > +
> > > > > > > > > + if (unlikely(ops == NULL || ops->get_dev_iommu_domain ==
> > NULL))
> > > > > > > > > + return NULL;
> > > > > > > > > +
> > > > > > > > > + return ops->get_dev_iommu_domain(dev); }
> > > > > > > > > +EXPORT_SYMBOL_GPL(iommu_get_dev_domain);
> > > > > > > >
> > > > > > > > What prevents this from racing iommu_domain_free()?  There's
> > > > > > > > no references acquired, so there's no reason for the caller
> > > > > > > > to assume the
> > > > > > pointer is valid.
> > > > > > >
> > > > > > > Sorry for late query, somehow this email went into a folder
> > > > > > > and escaped;
> > > > > > >
> > > > > > > Just to be sure, there is not lock at generic "struct
> > > > > > > iommu_domain", but IP
> > > > > > specific structure (link FSL domain) linked in
> > > > > > iommu_domain->priv have a lock, so we need to ensure this race
> > > > > > in FSL iommu code (say drivers/iommu/fsl_pamu_domain.c), right?
> > > > > >
> > > > > > No, it's not suff

RE: [PATCH 2/7] iommu: add api to get iommu_domain of a device

2013-10-07 Thread Bhushan Bharat-R65777
> > > Do you really want module dependencies between vfio and your core
> > > kernel MSI setup?  Look at the vfio external user interface that we've
> already defined.
> > > That allows other components of the kernel to get a proper reference
> > > to a vfio group.  From there you can work out how to get what you
> > > want.  Another alternative is that vfio could register an MSI to
> > > IOVA mapping with architecture code when the mapping is created.
> > > The MSI setup path could then do a lookup in architecture code for
> > > the mapping.  You could even store the MSI to IOVA mapping in VFIO
> > > and create an interface where SET_IRQ passes that mapping into setup code.
> >
> > Ok, What I want is to get IOVA associated with a physical address
> > (physical address of MSI-bank).
> > And currently I do not see a way to know IOVA of a physical address
> > and doing all this domain get and then search through all of
> > iommu-windows of that domain.
> >
> > What if we add an iommu-API which can return the IOVA mapping of a
> > physical address. Current use case is setting up MSI's for aperture
> > type of IOMMU also getting a phys_to_iova() mapping is independent of
> > VFIO, your thought?
> 
> A physical address can be mapped to multiple IOVAs, so the interface seems
> flawed by design.  It also has the same problem as above, it's a backdoor that
> can be called asynchronous to the owner of the domain, so what reason is there
> to believe the result?  It just replaces an iommu_domain pointer with an IOVA.
> VFIO knows this mapping, so why are we trying to go behind its back and ask 
> the
> IOMMU?
IOMMU is the final place where mapping is created, so may be today it is 
calling on behalf of VFIO, tomorrow it can be for normal Linux or some other 
interface. But I am fine to directly talk to vfio and will not try to solve a 
problem which does not exists today.

MSI subsystem knows pdev (pci device) and physical address, then what interface 
it will use to get the IOVA from VFIO?

Thanks
-Bharat

>  Thanks,
> 
> Alex
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v8 06/12] ARM: dts: Add description of System MMU of Exynos SoCs

2013-10-07 Thread Cho KyongHo
On Mon, 07 Oct 2013 08:44:54 -0500, Rob Herring wrote:
> On Fri, Jul 26, 2013 at 6:28 AM, Cho KyongHo  wrote:
> > Signed-off-by: Cho KyongHo 
> > ---
> >  .../bindings/iommu/samsung,exynos4210-sysmmu.txt   |  103 +++
> >  arch/arm/boot/dts/exynos4.dtsi |  122 
> >  arch/arm/boot/dts/exynos4210.dtsi  |   25 ++
> >  arch/arm/boot/dts/exynos4x12.dtsi  |   76 +
> >  arch/arm/boot/dts/exynos5250.dtsi  |  291 
> > 
> >  5 files changed, 617 insertions(+), 0 deletions(-)
> >  create mode 100644 
> > Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
> >
> > diff --git 
> > a/Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
> > b/Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
> > new file mode 100644
> > index 000..92f0a33
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
> > @@ -0,0 +1,103 @@
> > +Samsung Exynos4210 IOMMU H/W, System MMU (System Memory Management Unit)
> > +
> > +Samsung's Exynos architecture contains System MMU that enables scattered
> > +physical memory chunks visible as a contiguous region to DMA-capable 
> > peripheral
> > +devices like MFC, FIMC, FIMD, GScaler, FIMC-IS and so forth.
> > +
> > +System MMU is a sort of IOMMU and support identical translation table 
> > format to
> > +ARMv7 translation tables with minimum set of page properties including 
> > access
> > +permissions, shareability and security protection. In addition, System MMU 
> > has
> > +another capabilities like L2 TLB or block-fetch buffers to minimize 
> > translation
> > +latency.
> > +
> > +A System MMU is dedicated to a single master peripheral device.  Thus, it 
> > is
> > +important to specify the correct System MMU in the device node of its 
> > master
> > +device. Whereas a System MMU is dedicated to a master device, the master 
> > device
> > +may have more than one System MMU.
> > +
> > +Required properties:
> > +- compatible: Should be "samsung,exynos4210-sysmmu"
> > +- reg: A tuple of base address and size of System MMU registers.
> > +- interrupt-parent: The phandle of the interrupt controller of System MMU
> > +- interrupts: A tuple of numbers that indicates the interrupt source.
> > +- clock-names: Should be "sysmmu" if the System MMU is needed to gate its 
> > clock.
> > +   Please refer to the following documents:
> > +  Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +  Documentation/devicetree/bindings/clock/exynos4-clock.txt
> > +  Documentation/devicetree/bindings/clock/exynos5250-clock.txt
> > +  Optional "master" if the clock to the System MMU is gated by
> > +  another gate clock other than "sysmmu". The System MMU driver
> > +  sets "master" the parent of "sysmmu".
> > +  Exynos4 SoCs, there needs no "master" clocks.
> > +  Exynos5 SoCs, some System MMUs must have "master" clocks.
> > +- clocks: Required if the System MMU is needed to gate its clock.
> > + Please refer to the documents listed above.
> > +- samsung,power-domain: Required if the System MMU is needed to gate its 
> > power.
> > + Please refer to the following document:
> > + Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> > +
> > +Required properties for the master peripheral devices:
> > +- iommu: phandles to the System MMUs of the device
> 
> You have not addressed my comments from the last version. We do not
> need 2 (or more) different ways to describe the connection between
> masters and iommu's. Use mmu-masters property here to describe the
> connection.
> 

Sorry, I forgot to reply.
I just thought the meaning of your comment that it should be align with ARM 
System MMU.
I now understand and it should be changed to mmu-masters property
because it is now in the kernel.

Thank you.


> Rob
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu