Re: [PATCH v5 28/32] x86/mm, kexec: Allow kexec to be used with SME

[Dave Young]

On 05/26/17 at 12:17pm, Xunlei Pang wrote:
> On 04/19/2017 at 05:21 AM, Tom Lendacky wrote:
> > Provide support so that kexec can be used to boot a kernel when SME is
> > enabled.
> >
> > Support is needed to allocate pages for kexec without encryption.  This
> > is needed in order to be able to reboot in the kernel in the same manner
> > as originally booted.
> 
> Hi Tom,
> 
> Looks like kdump will break, I didn't see the similar handling for kdump 
> cases, see kernel:
> kimage_alloc_crash_control_pages(), kimage_load_crash_segment(), etc.
> 
> We need to support kdump with SME, kdump 
> kernel/initramfs/purgatory/elfcorehdr/etc
> are all loaded into the reserved memory(see crashkernel=X) by userspace 
> kexec-tools.

For kexec_load, it is loaded by kexec-tools, we have in kernel loader
syscall kexec_file_load, it is handled in kernel.

> I think a straightforward way would be to mark the whole reserved memory 
> range without
> encryption before loading all the kexec segments for kdump, I guess we can 
> handle this
> easily in arch_kexec_unprotect_crashkres().
> 
> Moreover, now that "elfcorehdr=X" is left as decrypted, it needs to be 
> remapped to the
> encrypted data.

Tom, could you have a try on kdump according to suggestion from Xunlei?
It is just based on theoretical patch understanding, there could be
other issues when you work on it. Feel free to ask if we can help on
anything.

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

Re: [PATCH 3/4] iommu: add qcom_iommu

[Rob Clark]

On Fri, May 26, 2017 at 8:56 AM, Robin Murphy  wrote:
> On 25/05/17 18:33, Rob Clark wrote:
>> An iommu driver for Qualcomm "B" family devices which do not completely
>> implement the ARM SMMU spec.  These devices have context-bank register
>> layout that is similar to ARM SMMU, but no global register space (or at
>> least not one that is accessible).
>
> I still object to this description, because the SMMU_SCR1.GASRAE = 1
> usage model is explicitly *specified* by the ARM SMMU spec! It's merely
> that the arm-smmu driver is designed for the case where we do have
> control of the global space and stage 2 contexts.

hmm, ok.. well, I've no idea what secure world is doing, but it sounds
plausible that GASRAE is set to 1.. at least that would match how
things behave.

In that case, I wonder if the driver should have a more generic name
than "qcom_iommu" (and likewise for compat strings, etc)?  I've really
no idea if qcom is the only one doing this.  In either case,
suggestions welcome.  (I had assumed someone would have bikeshedded
the name/compat-strings by now ;-))

>> Signed-off-by: Rob Clark 
>> ---
>> v1: original
>> v2: bindings cleanups and kconfig issues that kbuild robot pointed out
>> v3: fix issues pointed out by Rob H. and actually make device removal
>> work
>> v4: fix WARN_ON() splats reported by Archit
>> v5: some fixes to build as a module.. note that it cannot actually
>> be built as a module yet (at minimum a bunch of other iommu syms
>> that are needed are not exported, but there may be more to it
>> than that), but at least qcom_iommu is ready should it become
>> possible to build iommu drivers as modules.
>
> Note that with the 4.12 probe-deferral changes, modules totally work!
> For any master which probed before the IOMMU driver was loaded, you can
> then hook them up after the fact by just unbinding and rebinding their
> drivers - it's really cool.

hmm, ok, last time I tried this was 4.11 + iommu-next for 4.12 (plus a
couple other -next trees), since 4.12-rc1 wasn't out yet.. but at that
time, we needed at least a few EXPORT_SYMBOL()s, plus probably some
sort of fix for iommu bug I was trying to fix/paper-over in
<20170505180837.11326-1-robdcl...@gmail.com> (at least if you wanted
module unload to work).  For the former issue, I can send patches to
add EXPORT_SYMBOL()s (or is EXPORT_SYMBOL_GPL() preferred?).. for
latter, well I spend 80% or my time working on userspace level part of
gpu driver stack, and 80% of my kernel time working in drm, so I'll
leave this to someone who spends more than 4% of their time working on
the iommu subsystem ;-)

>> v6: Add additional pm-runtime get/puts around paths that can hit
>> TLB inv, to avoid unclocked register access if device using the
>> iommu is not powered on.  And pre-emptively clear interrupts
>> before registering IRQ handler just in case the bootloader has
>> left us a surpise.
>>
>>  drivers/iommu/Kconfig  |  10 +
>>  drivers/iommu/Makefile |   1 +
>>  drivers/iommu/qcom_iommu.c | 878 
>> +
>>  3 files changed, 889 insertions(+)
>>  create mode 100644 drivers/iommu/qcom_iommu.c
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index 6ee3a25..aa4b628 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -367,4 +367,14 @@ config MTK_IOMMU_V1
>>
>> if unsure, say N here.
>>
>> +config QCOM_IOMMU
>> + # Note: iommu drivers cannot (yet?) be built as modules
>> + bool "Qualcomm IOMMU Support"
>> + depends on ARCH_QCOM || COMPILE_TEST
>> + select IOMMU_API
>> + select IOMMU_IO_PGTABLE_LPAE
>> + select ARM_DMA_USE_IOMMU
>> + help
>> +   Support for IOMMU on certain Qualcomm SoCs.
>> +
>>  endif # IOMMU_SUPPORT
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index 195f7b9..b910aea 100644
>> --- a/drivers/iommu/Makefile
>> +++ b/drivers/iommu/Makefile
>> @@ -27,3 +27,4 @@ obj-$(CONFIG_TEGRA_IOMMU_SMMU) += tegra-smmu.o
>>  obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
>>  obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
>>  obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
>> +obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o
>> diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
>> new file mode 100644
>> index 000..bfaf97c
>> --- /dev/null
>> +++ b/drivers/iommu/qcom_iommu.c
>> @@ -0,0 +1,878 @@
>> +/*
>> + * IOMMU API for QCOM secure IOMMUs.  Somewhat based on arm-smmu.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You

[PATCH] iommu/amd: Propagate IOVA allocation failure

[Robin Murphy]

Unlike the old allocator, alloc_iova_fast() will return 0 if it failed
to allocate a PFN. Since the callers of dma_ops_alloc_iova() would end
up treating that as a valid address, translate it to the DMA_ERROR_CODE
that they would expect.

Fixes: 256e4621c21a ("iommu/amd: Make use of the generic IOVA allocator")
Signed-off-by: Robin Murphy 
---

Just something I spotted whilst comparing dma_map_page() callchains...

 drivers/iommu/amd_iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 63cacf5d6cf2..489dc302899e 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1555,6 +1555,9 @@ static unsigned long dma_ops_alloc_iova(struct device 
*dev,
if (!pfn)
pfn = alloc_iova_fast(&dma_dom->iovad, pages, 
IOVA_PFN(dma_mask));
 
+   if (!pfn)
+   return DMA_ERROR_CODE;
+
return (pfn << PAGE_SHIFT);
 }
 
-- 
2.12.2.dirty

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

Re: [PATCH 1/4] Docs: dt: document qcom iommu bindings

[Rob Clark]

On Fri, May 26, 2017 at 7:33 AM, Robin Murphy  wrote:
> On 25/05/17 18:33, Rob Clark wrote:
>> Cc: devicet...@vger.kernel.org
>> Signed-off-by: Rob Clark 
>> Reviewed-by: Rob Herring 
>> ---
>>  .../devicetree/bindings/iommu/qcom,iommu.txt   | 121 
>> +
>>  1 file changed, 121 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iommu/qcom,iommu.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt 
>> b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
>> new file mode 100644
>> index 000..0d50f84
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
>> @@ -0,0 +1,121 @@
>> +* QCOM IOMMU v1 Implementation
>> +
>> +Qualcomm "B" family devices which are not compatible with arm-smmu have
>> +a similar looking IOMMU but without access to the global register space,
>> +and optionally requiring additional configuration to route context irqs
>> +to non-secure vs secure interrupt line.
>> +
>> +** Required properties:
>> +
>> +- compatible   : Should be one of:
>> +
>> +"qcom,msm8916-iommu"
>> +
>> + Followed by "qcom,msm-iommu-v1".
>> +
>> +- clock-names  : Should be a pair of "iface" (required for IOMMUs
>> + register group access) and "bus" (required for
>> + the IOMMUs underlying bus access).
>> +
>> +- clocks   : Phandles for respective clocks described by
>> + clock-names.
>> +
>> +- #address-cells   : must be 1.
>> +
>> +- #size-cells  : must be 1.
>> +
>> +- #iommu-cells : Must be 1.
>
> You need to document what the value in the cell means for this binding.
> AFAICS it looks to be the hardware context bank index, but I wonder if
> it might be simpler to use the child node index instead.

yeah, it is the ctx bank idx.. I suppose it could work either way.  I
guess the advantage of ctx bank index is that it wouldn't shift if
adding child nodes incrementally (which at least might be more likely
for someone wiring up things incrementally when bringing up a new SoC
without docs.. and we've had a few cases of that).

(Ie. I think there were some cases where there are gaps in the context
banks.. and not really sure if those where just context banks that
downstream kernel didn't bother using or what.)

>> +- ranges   : Base address and size of the iommu context banks.
>> +
>> +- qcom,iommu-secure-id  : secure-id.
>> +
>> +- List of sub-nodes, one per translation context bank.  Each sub-node
>> +  has the following required properties:
>> +
>> +  - compatible : Should be one of:
>> +- "qcom,msm-iommu-v1-ns"  : non-secure context bank
>> +- "qcom,msm-iommu-v1-sec" : secure context bank
>> +  - reg: Base address and size of context bank within the iommu
>> +  - interrupts : The context fault irq.
>> +
>> +** Optional properties:
>> +
>> +- reg  : Base address and size of the SMMU local base, should
>> + be only specified if the iommu requires configuration
>> + for routing of context bank irq's to secure vs non-
>> + secure lines.  (Ie. if the iommu contains secure
>> + context banks)
>> +
>> +
>> +** Examples:
>> +
>> + apps_iommu: iommu@1e2 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + #iommu-cells = <1>;
>> + compatible = "qcom,msm8916-iommu", "qcom,msm-iommu-v1";
>> + ranges = <0 0x1e2 0x4>;
>> + reg = <0x1ef 0x3000>;
>> + clocks = <&gcc GCC_SMMU_CFG_CLK>,
>> +  <&gcc GCC_APSS_TCU_CLK>;
>> + clock-names = "iface", "bus";
>> + qcom,iommu-secure-id = <17>;
>> +
>> + // mdp_0:
>> + iommu-ctx@4000 {
>> + compatible = "qcom,msm-iommu-v1-ns";
>> + reg = <0x4000 0x1000>;
>> + interrupts = ;
>> + };
>> +
>> + // venus_ns:
>> + iommu-ctx@5000 {
>> + compatible = "qcom,msm-iommu-v1-sec";
>> + reg = <0x5000 0x1000>;
>> + interrupts = ;
>> + };
>> + };
>> +
>> + gpu_iommu: iommu@1f08000 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + #iommu-cells = <1>;
>> + compatible = "qcom,msm8916-iommu", "qcom,msm-iommu-v1";
>> + ranges = <0 0x1f08000 0x1>;
>> + clocks = <&gcc GCC_SMMU_CFG_CLK>,
>> +  <&gcc GCC_GFX_TCU_CLK>;
>> + clock-names = "iface", "bus";
>> + qcom,iommu-secure-id = <18>;
>> +
>> + // gfx3d_user:
>> + iommu-ctx@1f09000 {
>
> @1000?
>
>> + compatible = "qcom,msm-iommu-v1-ns";
>> + reg = <0x1000 0x1000>;
>> +  

Re: [PATCH v5 17/32] x86/mm: Add support to access boot related data in the clear

[Borislav Petkov]

On Fri, May 26, 2017 at 11:22:36AM -0500, Tom Lendacky wrote:
> In addition to the same issue as efi.memmap.phys_map, efi_phys has
> the __initdata attribute so it will be released/freed which will cause
> problems in checks performed afterwards.

Sounds to me like we should drop the __initdata attr and prepare them
much earlier for use by the SME code.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 29/32] x86/mm: Add support to encrypt the kernel in-place

[Borislav Petkov]

On Thu, May 25, 2017 at 05:24:27PM -0500, Tom Lendacky wrote:
> I guess I could do that, but this will probably only end up clearing a
> single PGD entry anyway since it's highly doubtful the address range
> would cross a 512GB boundary.

Or you can compute how many 512G-covering, i.e., PGD entries there are
and clear just the right amnount. :^)

> I can change the name. As for the use of ENTRY... without the
> ENTRY/ENDPROC combination I was receiving a warning about a return
> instruction outside of a callable function. It looks like I can just
> define the "sme_enc_routine:" label with the ENDPROC and the warning
> goes away and the global is avoided. It doesn't like the local labels
> (.L...) so I'll use the new name.

Is that warning from objtool or where does it come from?

How do I trigger it locally?

> The hardware will try to optimize rep movsb into large chunks assuming
> things are aligned, sizes are large enough, etc. so we don't have to
> explicitly specify and setup for a rep movsq.

I thought the hw does that for movsq too?

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 17/32] x86/mm: Add support to access boot related data in the clear

[Tom Lendacky]

On 5/18/2017 2:50 PM, Matt Fleming wrote:

On Mon, 15 May, at 08:35:17PM, Borislav Petkov wrote:

On Tue, Apr 18, 2017 at 04:19:21PM -0500, Tom Lendacky wrote:


+   paddr = boot_params.efi_info.efi_memmap_hi;
+   paddr <<= 32;
+   paddr |= boot_params.efi_info.efi_memmap;
+   if (phys_addr == paddr)
+   return true;
+
+   paddr = boot_params.efi_info.efi_systab_hi;
+   paddr <<= 32;
+   paddr |= boot_params.efi_info.efi_systab;


So those two above look like could be two global vars which are
initialized somewhere in the EFI init path:

efi_memmap_phys and efi_systab_phys or so.

Matt ?

And then you won't need to create that paddr each time on the fly. I
mean, it's not a lot of instructions but still...


We should already have the physical memmap address available in
'efi.memmap.phys_map'.


Unfortunately memremap_is_efi_data() is called before the efi structure
gets initialized, so I can't use that value.



And the physical address of the system table should be in
'efi_phys.systab'. See efi_init().


In addition to the same issue as efi.memmap.phys_map, efi_phys has
the __initdata attribute so it will be released/freed which will cause
problems in checks performed afterwards.

Thanks,
Tom




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

Re: [PATCH v5 31/32] x86: Add sysfs support for Secure Memory Encryption

[Tom Lendacky]

On 5/26/2017 12:04 AM, Xunlei Pang wrote:

On 05/26/2017 at 10:49 AM, Dave Young wrote:

Ccing Xunlei he is reading the patches see what need to be done for
kdump. There should still be several places to handle to make kdump work.

On 05/18/17 at 07:01pm, Borislav Petkov wrote:

On Tue, Apr 18, 2017 at 04:22:12PM -0500, Tom Lendacky wrote:

Add sysfs support for SME so that user-space utilities (kdump, etc.) can
determine if SME is active.

But why do user-space tools need to know that?

I mean, when we load the kdump kernel, we do it with the first kernel,
with the kexec_load() syscall, AFAICT. And that code does a lot of
things during that init, like machine_kexec_prepare()->init_pgtable() to
prepare the ident mapping of the second kernel, for example.

What I'm aiming at is that the first kernel knows *exactly* whether SME
is enabled or not and doesn't need to tell the second one through some
sysfs entries - it can do that during loading.

So I don't think we need any userspace things at all...

If kdump kernel can get the SME status from hardware register then this
should be not necessary and this patch can be dropped.


Yes, I also agree with dropping this one.


Consensus is to drop, so it will be.

Thanks,
Tom



Regards,
Xunlei


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

Re: [PATCH v2] arm: dma-mapping: Reset the device's dma_ops

[Sricharan R]

Hi Laurent,

On 5/26/2017 7:44 PM, Laurent Pinchart wrote:
> Hi Sricharan,
> 
> Thank you for the patch.
> 
> On Friday 26 May 2017 16:13:37 Sricharan R wrote:
>> arch_teardown_dma_ops() being the inverse of arch_setup_dma_ops()
>> ,dma_ops should be cleared in the teardown path. Currently, only the
>> device's iommu mapping structures are cleared in arch_teardown_dma_ops,
>> but not the dma_ops. So on the next reprobe, dma_ops left in place is
>> stale from the first IOMMU setup, but iommu mappings has been disposed
>> of. This is a problem when the probe of the device is deferred and
>> recalled with the IOMMU probe deferral.
>>
>> So for fixing this, slightly refactor by moving the code from
>> __arm_iommu_detach_device to arm_iommu_detach_device and cleanup
>> the former. This takes care of resetting the dma_ops in the teardown
>> path.
>>
>> Signed-off-by: Sricharan R 
>> Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time for
>> platform/amba/pci bus devices")
> 
> Reviewed-by: Laurent Pinchart 
> 
> Could you please push this upstream along with "[PATCH] ARM: dma-mapping: 
> Don't tear third-party mappings" ?
> 
> (And feel free to s/tear/tear down/ in the subject of that patch, I've only 
> noticed now that I forgot one word)

ok, will change the above and post it with the other patches.

Regards,
 Sricharan

> 
>> ---
>>  arch/arm/mm/dma-mapping.c | 25 ++---
>>  1 file changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index c742dfd..6e82e87 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -2311,7 +2311,14 @@ int arm_iommu_attach_device(struct device *dev,
>>  }
>>  EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
>>
>> -static void __arm_iommu_detach_device(struct device *dev)
>> +/**
>> + * arm_iommu_detach_device
>> + * @dev: valid struct device pointer
>> + *
>> + * Detaches the provided device from a previously attached map.
>> + * This voids the dma operations (dma_map_ops pointer)
>> + */
>> +void arm_iommu_detach_device(struct device *dev)
>>  {
>>  struct dma_iommu_mapping *mapping;
>>
>> @@ -2324,22 +2331,10 @@ static void __arm_iommu_detach_device(struct device
>> *dev) iommu_detach_device(mapping->domain, dev);
>>  kref_put(&mapping->kref, release_iommu_mapping);
>>  to_dma_iommu_mapping(dev) = NULL;
>> +set_dma_ops(dev, NULL);
>>
>>  pr_debug("Detached IOMMU controller from %s device.\n", 
> dev_name(dev));
>>  }
>> -
>> -/**
>> - * arm_iommu_detach_device
>> - * @dev: valid struct device pointer
>> - *
>> - * Detaches the provided device from a previously attached map.
>> - * This voids the dma operations (dma_map_ops pointer)
>> - */
>> -void arm_iommu_detach_device(struct device *dev)
>> -{
>> -__arm_iommu_detach_device(dev);
>> -set_dma_ops(dev, NULL);
>> -}
>>  EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
>>
>>  static const struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
>> @@ -2379,7 +2374,7 @@ static void arm_teardown_iommu_dma_ops(struct device
>> *dev) if (!mapping)
>>  return;
>>
>> -__arm_iommu_detach_device(dev);
>> +arm_iommu_detach_device(dev);
>>  arm_iommu_release_mapping(mapping);
>>  }
> 

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] arm: dma-mapping: Reset the device's dma_ops

[Laurent Pinchart]

Hi Sricharan,

Thank you for the patch.

On Friday 26 May 2017 16:13:37 Sricharan R wrote:
> arch_teardown_dma_ops() being the inverse of arch_setup_dma_ops()
> ,dma_ops should be cleared in the teardown path. Currently, only the
> device's iommu mapping structures are cleared in arch_teardown_dma_ops,
> but not the dma_ops. So on the next reprobe, dma_ops left in place is
> stale from the first IOMMU setup, but iommu mappings has been disposed
> of. This is a problem when the probe of the device is deferred and
> recalled with the IOMMU probe deferral.
> 
> So for fixing this, slightly refactor by moving the code from
> __arm_iommu_detach_device to arm_iommu_detach_device and cleanup
> the former. This takes care of resetting the dma_ops in the teardown
> path.
> 
> Signed-off-by: Sricharan R 
> Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time for
> platform/amba/pci bus devices")

Reviewed-by: Laurent Pinchart 

Could you please push this upstream along with "[PATCH] ARM: dma-mapping: 
Don't tear third-party mappings" ?

(And feel free to s/tear/tear down/ in the subject of that patch, I've only 
noticed now that I forgot one word)

> ---
>  arch/arm/mm/dma-mapping.c | 25 ++---
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index c742dfd..6e82e87 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -2311,7 +2311,14 @@ int arm_iommu_attach_device(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
> 
> -static void __arm_iommu_detach_device(struct device *dev)
> +/**
> + * arm_iommu_detach_device
> + * @dev: valid struct device pointer
> + *
> + * Detaches the provided device from a previously attached map.
> + * This voids the dma operations (dma_map_ops pointer)
> + */
> +void arm_iommu_detach_device(struct device *dev)
>  {
>   struct dma_iommu_mapping *mapping;
> 
> @@ -2324,22 +2331,10 @@ static void __arm_iommu_detach_device(struct device
> *dev) iommu_detach_device(mapping->domain, dev);
>   kref_put(&mapping->kref, release_iommu_mapping);
>   to_dma_iommu_mapping(dev) = NULL;
> + set_dma_ops(dev, NULL);
> 
>   pr_debug("Detached IOMMU controller from %s device.\n", 
dev_name(dev));
>  }
> -
> -/**
> - * arm_iommu_detach_device
> - * @dev: valid struct device pointer
> - *
> - * Detaches the provided device from a previously attached map.
> - * This voids the dma operations (dma_map_ops pointer)
> - */
> -void arm_iommu_detach_device(struct device *dev)
> -{
> - __arm_iommu_detach_device(dev);
> - set_dma_ops(dev, NULL);
> -}
>  EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
> 
>  static const struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
> @@ -2379,7 +2374,7 @@ static void arm_teardown_iommu_dma_ops(struct device
> *dev) if (!mapping)
>   return;
> 
> - __arm_iommu_detach_device(dev);
> + arm_iommu_detach_device(dev);
>   arm_iommu_release_mapping(mapping);
>  }

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH V3 6/8] arm: dma-mapping: Reset the device's dma_ops

[Laurent Pinchart]

Hi Russell,

On Thursday 25 May 2017 16:05:41 Russell King - ARM Linux wrote:
> On Wed, May 24, 2017 at 02:26:13PM +0300, Laurent Pinchart wrote:
> > Again, the patch I propose is the simplest v4.12-rc fix I can think of,
> > short of reverting your complete IOMMU probe deferral patch series. Let's
> > focus on the v4.12-rc fix, and then discuss how to move forward in v4.13
> > and beyond.
>
> Except, I don't think it fixes the problem that Sricharan is trying to
> fix, namely that of DMA ops that have been setup, torn down, and are
> trying to be re-setup again.

You're right, it's two separate problems, and we need to fix them both.

> The issue here is that results in a stale setup, because the dma_ops are
> left in-place from the first iommu setup, but the iommu mapping has been
> disposed of.
> 
> Your patch only avoids the problem of tearing down someone else's DMA
> ops.  We need a combination of both patches together.

Yes exactly. I should read e-mails to the end before starting typing the reply 
:-)

> What needs to happen for Sricharan's problem to be resolved is:
> 
> 1. move all of __arm_iommu_detach_device() into arm_iommu_detach_device().
> 2. replace the __arm_iommu_detach_device() call in
> arm_teardown_iommu_dma_ops() with arm_iommu_detach_device().
> 
> as I don't see any need to have a different order in
> arm_teardown_iommu_dma_ops().

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 3/4] iommu: add qcom_iommu

[Robin Murphy]

On 25/05/17 18:33, Rob Clark wrote:
> An iommu driver for Qualcomm "B" family devices which do not completely
> implement the ARM SMMU spec.  These devices have context-bank register
> layout that is similar to ARM SMMU, but no global register space (or at
> least not one that is accessible).

I still object to this description, because the SMMU_SCR1.GASRAE = 1
usage model is explicitly *specified* by the ARM SMMU spec! It's merely
that the arm-smmu driver is designed for the case where we do have
control of the global space and stage 2 contexts.

> Signed-off-by: Rob Clark 
> ---
> v1: original
> v2: bindings cleanups and kconfig issues that kbuild robot pointed out
> v3: fix issues pointed out by Rob H. and actually make device removal
> work
> v4: fix WARN_ON() splats reported by Archit
> v5: some fixes to build as a module.. note that it cannot actually
> be built as a module yet (at minimum a bunch of other iommu syms
> that are needed are not exported, but there may be more to it
> than that), but at least qcom_iommu is ready should it become
> possible to build iommu drivers as modules.

Note that with the 4.12 probe-deferral changes, modules totally work!
For any master which probed before the IOMMU driver was loaded, you can
then hook them up after the fact by just unbinding and rebinding their
drivers - it's really cool.

> v6: Add additional pm-runtime get/puts around paths that can hit
> TLB inv, to avoid unclocked register access if device using the
> iommu is not powered on.  And pre-emptively clear interrupts
> before registering IRQ handler just in case the bootloader has
> left us a surpise.
> 
>  drivers/iommu/Kconfig  |  10 +
>  drivers/iommu/Makefile |   1 +
>  drivers/iommu/qcom_iommu.c | 878 
> +
>  3 files changed, 889 insertions(+)
>  create mode 100644 drivers/iommu/qcom_iommu.c
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 6ee3a25..aa4b628 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -367,4 +367,14 @@ config MTK_IOMMU_V1
>  
> if unsure, say N here.
>  
> +config QCOM_IOMMU
> + # Note: iommu drivers cannot (yet?) be built as modules
> + bool "Qualcomm IOMMU Support"
> + depends on ARCH_QCOM || COMPILE_TEST
> + select IOMMU_API
> + select IOMMU_IO_PGTABLE_LPAE
> + select ARM_DMA_USE_IOMMU
> + help
> +   Support for IOMMU on certain Qualcomm SoCs.
> +
>  endif # IOMMU_SUPPORT
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 195f7b9..b910aea 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -27,3 +27,4 @@ obj-$(CONFIG_TEGRA_IOMMU_SMMU) += tegra-smmu.o
>  obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
>  obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
>  obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
> +obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o
> diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
> new file mode 100644
> index 000..bfaf97c
> --- /dev/null
> +++ b/drivers/iommu/qcom_iommu.c
> @@ -0,0 +1,878 @@
> +/*
> + * IOMMU API for QCOM secure IOMMUs.  Somewhat based on arm-smmu.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + *
> + * Copyright (C) 2013 ARM Limited
> + * Copyright (C) 2017 Red Hat
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "io-pgtable.h"
> +#include "arm-smmu-regs.h"
> +
> +#define SMMU_INTR_SEL_NS 0x2000
> +
> +struct qcom_iommu_dev {
> + /* IOMMU core code handle */
> + struct iommu_device  iommu;
> + struct device   *dev;
> + struct clk  *iface_clk;
> + struct clk  *bus_clk;
> + void __iomem*local_base;
> + u32  sec_id;
> + struct list_head context_list;   /* list of qcom_iommu_context 
> */

Why not just an array? You can already determine at probe time what the
maximum size needs to be, and it looks like it would make a fair chunk
of the context code less horrible by turning it into simple indexing.

> +};
> +
> +struct qcom_iommu_ctx {
> + struct 

Re: [PATCH 1/4] Docs: dt: document qcom iommu bindings

[Robin Murphy]

On 25/05/17 18:33, Rob Clark wrote:
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Rob Clark 
> Reviewed-by: Rob Herring 
> ---
>  .../devicetree/bindings/iommu/qcom,iommu.txt   | 121 
> +
>  1 file changed, 121 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> 
> diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt 
> b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> new file mode 100644
> index 000..0d50f84
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> @@ -0,0 +1,121 @@
> +* QCOM IOMMU v1 Implementation
> +
> +Qualcomm "B" family devices which are not compatible with arm-smmu have
> +a similar looking IOMMU but without access to the global register space,
> +and optionally requiring additional configuration to route context irqs
> +to non-secure vs secure interrupt line.
> +
> +** Required properties:
> +
> +- compatible   : Should be one of:
> +
> +"qcom,msm8916-iommu"
> +
> + Followed by "qcom,msm-iommu-v1".
> +
> +- clock-names  : Should be a pair of "iface" (required for IOMMUs
> + register group access) and "bus" (required for
> + the IOMMUs underlying bus access).
> +
> +- clocks   : Phandles for respective clocks described by
> + clock-names.
> +
> +- #address-cells   : must be 1.
> +
> +- #size-cells  : must be 1.
> +
> +- #iommu-cells : Must be 1.

You need to document what the value in the cell means for this binding.
AFAICS it looks to be the hardware context bank index, but I wonder if
it might be simpler to use the child node index instead.

> +- ranges   : Base address and size of the iommu context banks.
> +
> +- qcom,iommu-secure-id  : secure-id.
> +
> +- List of sub-nodes, one per translation context bank.  Each sub-node
> +  has the following required properties:
> +
> +  - compatible : Should be one of:
> +- "qcom,msm-iommu-v1-ns"  : non-secure context bank
> +- "qcom,msm-iommu-v1-sec" : secure context bank
> +  - reg: Base address and size of context bank within the iommu
> +  - interrupts : The context fault irq.
> +
> +** Optional properties:
> +
> +- reg  : Base address and size of the SMMU local base, should
> + be only specified if the iommu requires configuration
> + for routing of context bank irq's to secure vs non-
> + secure lines.  (Ie. if the iommu contains secure
> + context banks)
> +
> +
> +** Examples:
> +
> + apps_iommu: iommu@1e2 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + #iommu-cells = <1>;
> + compatible = "qcom,msm8916-iommu", "qcom,msm-iommu-v1";
> + ranges = <0 0x1e2 0x4>;
> + reg = <0x1ef 0x3000>;
> + clocks = <&gcc GCC_SMMU_CFG_CLK>,
> +  <&gcc GCC_APSS_TCU_CLK>;
> + clock-names = "iface", "bus";
> + qcom,iommu-secure-id = <17>;
> +
> + // mdp_0:
> + iommu-ctx@4000 {
> + compatible = "qcom,msm-iommu-v1-ns";
> + reg = <0x4000 0x1000>;
> + interrupts = ;
> + };
> +
> + // venus_ns:
> + iommu-ctx@5000 {
> + compatible = "qcom,msm-iommu-v1-sec";
> + reg = <0x5000 0x1000>;
> + interrupts = ;
> + };
> + };
> +
> + gpu_iommu: iommu@1f08000 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + #iommu-cells = <1>;
> + compatible = "qcom,msm8916-iommu", "qcom,msm-iommu-v1";
> + ranges = <0 0x1f08000 0x1>;
> + clocks = <&gcc GCC_SMMU_CFG_CLK>,
> +  <&gcc GCC_GFX_TCU_CLK>;
> + clock-names = "iface", "bus";
> + qcom,iommu-secure-id = <18>;
> +
> + // gfx3d_user:
> + iommu-ctx@1f09000 {

@1000?

> + compatible = "qcom,msm-iommu-v1-ns";
> + reg = <0x1000 0x1000>;
> + interrupts = ;
> + };
> +
> + // gfx3d_priv:
> + iommu-ctx@1f0a000 {

@2000?

Robin.

> + compatible = "qcom,msm-iommu-v1-ns";
> + reg = <0x2000 0x1000>;
> + interrupts = ;
> + };
> + };
> +
> + ...
> +
> + venus: video-codec@1d0 {
> + ...
> + iommus = <&apps_iommu 5>;
> + };
> +
> + mdp: mdp@1a01000 {
> + ...
> + iommus = <&apps_iommu 4>;
> + };
> +
> + gpu@01c0 {
> + ...
> + iommus = <&gpu_iommu 1>, <&gpu_iommu 2>;
> + };
> 

_

[PATCH v2] arm: dma-mapping: Reset the device's dma_ops

[Sricharan R]

arch_teardown_dma_ops() being the inverse of arch_setup_dma_ops()
,dma_ops should be cleared in the teardown path. Currently, only the
device's iommu mapping structures are cleared in arch_teardown_dma_ops,
but not the dma_ops. So on the next reprobe, dma_ops left in place is
stale from the first IOMMU setup, but iommu mappings has been disposed
of. This is a problem when the probe of the device is deferred and
recalled with the IOMMU probe deferral.

So for fixing this, slightly refactor by moving the code from
__arm_iommu_detach_device to arm_iommu_detach_device and cleanup
the former. This takes care of resetting the dma_ops in the teardown
path.

Signed-off-by: Sricharan R 
Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time for 
platform/amba/pci bus devices")
---
 arch/arm/mm/dma-mapping.c | 25 ++---
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c742dfd..6e82e87 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2311,7 +2311,14 @@ int arm_iommu_attach_device(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
 
-static void __arm_iommu_detach_device(struct device *dev)
+/**
+ * arm_iommu_detach_device
+ * @dev: valid struct device pointer
+ *
+ * Detaches the provided device from a previously attached map.
+ * This voids the dma operations (dma_map_ops pointer)
+ */
+void arm_iommu_detach_device(struct device *dev)
 {
struct dma_iommu_mapping *mapping;
 
@@ -2324,22 +2331,10 @@ static void __arm_iommu_detach_device(struct device 
*dev)
iommu_detach_device(mapping->domain, dev);
kref_put(&mapping->kref, release_iommu_mapping);
to_dma_iommu_mapping(dev) = NULL;
+   set_dma_ops(dev, NULL);
 
pr_debug("Detached IOMMU controller from %s device.\n", dev_name(dev));
 }
-
-/**
- * arm_iommu_detach_device
- * @dev: valid struct device pointer
- *
- * Detaches the provided device from a previously attached map.
- * This voids the dma operations (dma_map_ops pointer)
- */
-void arm_iommu_detach_device(struct device *dev)
-{
-   __arm_iommu_detach_device(dev);
-   set_dma_ops(dev, NULL);
-}
 EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
 
 static const struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
@@ -2379,7 +2374,7 @@ static void arm_teardown_iommu_dma_ops(struct device *dev)
if (!mapping)
return;
 
-   __arm_iommu_detach_device(dev);
+   arm_iommu_detach_device(dev);
arm_iommu_release_mapping(mapping);
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

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