Re: [PATCH] ACPI/IORT: Make dma masks set-up IORT specific
On Mon, Dec 05, 2016 at 12:26:19PM +, Lorenzo Pieralisi wrote: > The introduction of acpi_dma_configure() allows to configure DMA > and related IOMMU for any device that is DMA capable. To achieve > that goal it ensures DMA masks are set-up to sane default values > before proceeding with IOMMU and DMA ops configuration. > > On x86/ia64 systems, through acpi_bind_one(), acpi_dma_configure() is > called for every device that has an ACPI companion, in that every device > is considered DMA capable on x86/ia64 systems (ie acpi_get_dma_attr() API), > which has the side effect of initializing dma masks also for > pseudo-devices (eg CPUs and memory nodes) and potentially for devices > whose dma masks were not set-up before the acpi_dma_configure() API was > introduced, which may have noxious side effects. > > Therefore, in preparation for IORT firmware specific DMA masks set-up, > wrap the default DMA masks set-up in acpi_dma_configure() inside an IORT > specific wrapper that reverts to a NOP on x86/ia64 systems, restoring the > default expected behaviour on x86/ia64 systems and keeping DMA default > masks set-up on IORT based (ie ARM) arch configurations. > > Signed-off-by: Lorenzo Pieralisi > Cc: Will Deacon > Cc: Hanjun Guo > Cc: Bjorn Helgaas > Cc: Robin Murphy > Cc: Tomasz Nowicki > Cc: Joerg Roedel > Cc: "Rafael J. Wysocki" > Cc: Sricharan R > --- > Joerg, > > pending Rafael's ACK on it, given the 4.10 release timing and that the > series is queued via the IOMMU tree please consider applying this patch to > your arm/smmu branch for 4.10, it is not fixing a bug but it is modifying > the x86/ia64 code path; I prefer preventing any issue related to default > dma masks on x86/ia64 so I hope it can get merged along with the rest of > the ACPI IORT SMMU series. > > Thanks a lot and apologies, > Lorenzo > > drivers/acpi/arm64/iort.c | 22 ++ > drivers/acpi/scan.c | 14 +----- > include/linux/acpi_iort.h | 2 ++ > 3 files changed, 25 insertions(+), 13 deletions(-) Looks straightforward to me: Acked-by: Will Deacon Joerg can probably just pick this on top of his queue. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V6 6/6] iommu/arm-smmu: Set privileged attribute to 'default' instead of 'unprivileged'
On Fri, Dec 02, 2016 at 08:25:09PM +0530, Sricharan R wrote: > Currently the driver sets all the device transactions privileges > to UNPRIVILEGED, but there are cases where the iommu masters wants > to isolate privileged supervisor and unprivileged user. > So don't override the privileged setting to unprivileged, instead > set it to default as incoming and let it be controlled by the pagetable > settings. > > Signed-off-by: Sricharan R Acked-by: Will Deacon I'll let you and Robin sort out what you want to do with this series :) Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/4] iommu/arm-smmu: Disable stalling faults for all endpoints
Hi Rob, On Tue, Dec 06, 2016 at 06:30:21PM -0500, Rob Clark wrote: > On Thu, Aug 18, 2016 at 9:05 AM, Will Deacon wrote: > > Enabling stalling faults can result in hardware deadlock on poorly > > designed systems, particularly those with a PCI root complex upstream of > > the SMMU. > > > > Although it's not really Linux's job to save hardware integrators from > > their own misfortune, it *is* our job to stop userspace (e.g. VFIO > > clients) from hosing the system for everybody else, even if they might > > already be required to have elevated privileges. > > > > Given that the fault handling code currently executes entirely in IRQ > > context, there is nothing that can sensibly be done to recover from > > things like page faults anyway, so let's rip this code out for now and > > avoid the potential for deadlock. > > so, I'd like to re-introduce this feature, I *guess* as some sort of > opt-in quirk (ie. disabled by default unless something in DT tells you > otherwise?? But I'm open to suggestions. I'm not entirely sure what > hw was having problems due to this feature.) > > On newer snapdragon devices we are using arm-smmu for the GPU, and > halting the GPU so the driver's fault handler can dump some GPU state > on faults is enormously helpful for debugging and tracking down where > in the gpu cmdstream the fault was triggered. In addition, we will > eventually want the ability to update pagetables from fault handler > and resuming the faulting transition. I'm not against reintroducing this, but it would certainly need to be opt-in, as you suggest. If we want to try to use stall faults to enable demand paging for DMA, then that means running core mm code to resolve the fault and we can't do that in irq context. Consequently, we have to hand this off to a thread, which means the hardware must allow the SS bit to remain set without immediately reasserting the interrupt line. Furthermore, we can't handle multiple faults on a context-bank, so we'd need to restrict ourselves to one device (i.e. faulting stream) per domain (CB). I think that means we want both specific compatible strings to identify the SS bit behaviour, but also a way to opt-in for the stall model as a separate property to indicate that the SoC integration can support this without e.g. deadlocking. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: prevent corruption of ste stage-1 context ptr
Hi Nate, Thanks for the patch. On Mon, Dec 19, 2016 at 03:38:38PM -0500, Nate Watterson wrote: > To ensure that the stage-1 context ptr for an ste points to the > intended context descriptor, this patch adds code to clear away > the stale context ptr value prior to or'ing in the new one. > > Signed-off-by: Nate Watterson > --- > drivers/iommu/arm-smmu-v3.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index 4d6ec44..093f9f1 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -1080,6 +1080,8 @@ static void arm_smmu_write_strtab_ent(struct > arm_smmu_device *smmu, u32 sid, > if (smmu->features & ARM_SMMU_FEAT_STALLS) > dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD); > > + val &= ~(STRTAB_STE_0_S1CTXPTR_MASK << > + STRTAB_STE_0_S1CTXPTR_SHIFT); > val |= (ste->s1_cfg->cdptr_dma & STRTAB_STE_0_S1CTXPTR_MASK > << STRTAB_STE_0_S1CTXPTR_SHIFT) | > STRTAB_STE_0_CFG_S1_TRANS; Good catch. We only clear the Config field at present, although I think it would be better if we just did val = 0 instead of clearing the Config field, and then just recreate all of the S1-related fields (ctxptr, fmt, cdmax) if we're installing a stage-1 STE. The other STE fields aren't treated as read-modify-write, so it's more consistent not to treat the initial dword specially other than for determining ste_live. What do you think? Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: avoid over allocating for l2 stream tables
Hi Nate, On Mon, Dec 19, 2016 at 03:26:40PM -0500, Nate Watterson wrote: > Currently, all l2 stream tables are being allocated with space for > (1< physically supports. To avoid allocating memory for inaccessible > stes, this patch limits the span of an l2 table to be no larger > than the sid size of the smmu to which it belongs. > > Signed-off-by: Nate Watterson > --- > drivers/iommu/arm-smmu-v3.c | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) I can't help but think you'd be better off using a linear stream table in this scenario. If we hack the feature check for ARM_SMMU_FEAT_2_LVL_STRTAB so that it doesn't report support for 2 level tables if the number of sids is less than that covered by a single l2 entry, would that solve your problem? Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168
On Tue, Dec 20, 2016 at 11:52:58AM +, Marc Zyngier wrote:
> On 20/12/16 11:06, Geetha sowjanya wrote:
> > From: Tirumalesh Chalamarla
> > +#ifdef CONFIG_CAVIUM_ERRATUM_28168
> > +/*
> > + * Cavium ThunderX erratum 28168
> > + *
> > + * Due to erratum #28168 PCI-inbound MSI-X store to the interrupt
> > + * controller are delivered to the interrupt controller before older
> > + * PCI-inbound memory stores are committed. Doing a sync on SMMU
> > + * will make sure all prior data transfers are completed before
> > + * invoking ISR.
> > + *
> > + */
> > +void dev_smmu_tlb_sync(struct device *dev)
> > +{
> > + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > + struct arm_smmu_device *smmu = fwspec_smmu(fwspec);
> > +
> > + if (smmu)
> > + __arm_smmu_tlb_sync(smmu);
> > +}
> > +#endif
>
> I'll let Robin and Will comment on this, but it strikes be as rather odd
> that nothing will happen if the SMMU is in bypass or simply compiled
> out. So this workaround is at best incomplete.
Agreed. Unless the SMMU is the cause of the issue, relying on it to
implement the workaround is fragile and unnecessarily introduces a linkage
between SMMU driver internals and the interrupt handling code.
Not a fan.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/4] iommu/arm-smmu: Disable stalling faults for all endpoints
On Mon, Dec 19, 2016 at 02:33:36PM +0530, Sricharan wrote: > >On Tue, Dec 06, 2016 at 06:30:21PM -0500, Rob Clark wrote: > >> On Thu, Aug 18, 2016 at 9:05 AM, Will Deacon wrote: > >> > Enabling stalling faults can result in hardware deadlock on poorly > >> > designed systems, particularly those with a PCI root complex upstream of > >> > the SMMU. > >> > > >> > Although it's not really Linux's job to save hardware integrators from > >> > their own misfortune, it *is* our job to stop userspace (e.g. VFIO > >> > clients) from hosing the system for everybody else, even if they might > >> > already be required to have elevated privileges. > >> > > >> > Given that the fault handling code currently executes entirely in IRQ > >> > context, there is nothing that can sensibly be done to recover from > >> > things like page faults anyway, so let's rip this code out for now and > >> > avoid the potential for deadlock. > >> > >> so, I'd like to re-introduce this feature, I *guess* as some sort of > >> opt-in quirk (ie. disabled by default unless something in DT tells you > >> otherwise?? But I'm open to suggestions. I'm not entirely sure what > >> hw was having problems due to this feature.) > >> > >> On newer snapdragon devices we are using arm-smmu for the GPU, and > >> halting the GPU so the driver's fault handler can dump some GPU state > >> on faults is enormously helpful for debugging and tracking down where > >> in the gpu cmdstream the fault was triggered. In addition, we will > >> eventually want the ability to update pagetables from fault handler > >> and resuming the faulting transition. > > > >I'm not against reintroducing this, but it would certainly need to be > >opt-in, as you suggest. If we want to try to use stall faults to enable > >demand paging for DMA, then that means running core mm code to resolve > >the fault and we can't do that in irq context. Consequently, we have to > >hand this off to a thread, which means the hardware must allow the SS > >bit to remain set without immediately reasserting the interrupt line. > >Furthermore, we can't handle multiple faults on a context-bank, so we'd > >need to restrict ourselves to one device (i.e. faulting stream) per > >domain (CB). > > > >I think that means we want both specific compatible strings to identify > >the SS bit behaviour, but also a way to opt-in for the stall model as a > >separate property to indicate that the SoC integration can support this > >without e.g. deadlocking. > > > > To understand the reason on the need for the quirk based on SS bit behavior, > if the platform supports stall model and enabled, then SS bit should be > implemented > and remain set until the RESUME register is written back, means same behavior > always ? The behaviour of the SS bit is IMPLEMENTATION DEFINED per the architecture, so we need to know which way the given implementation chose to go. If we want to support paging, then we absolutely need a way to return from the interrupt handler without having handled the stall (i.e. without having written to the RESUME register). That means that we mustn't take the same interrupt immediately, otherwise we'll end up getting stuck in an infinite fault. One hacky option would be to mask the interrupt at the GIC, but that adds an additional requirement of one interrupt per context bank, which isn't typically implemented in my experience. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH] iommu/arm-smmu: Add global SMR masking property
On Tue, Dec 20, 2016 at 09:29:21PM -0600, Rob Herring wrote: > On Fri, Dec 16, 2016 at 01:19:29PM +, Robin Murphy wrote: > > The current SMR masking support using a 2-cell iommu-specifier is > > primarily intended to handle individual masters with large and/or > > complex Stream ID assignments; it quickly gets a bit clunky in other SMR > > use-cases where we just want to consistently mask out the same part of > > every Stream ID (e.g. for MMU-500 configurations where the appended TBU > > number gets in the way unnecessarily). Let's add a new property to allow > > a single global mask value to better fit the latter situation. > > > > CC: Stuart Yoder > > Signed-off-by: Robin Murphy > > --- > > > > Compile-tested only... > > > > Documentation/devicetree/bindings/iommu/arm,smmu.txt | 8 > > drivers/iommu/arm-smmu.c | 4 +++- > > 2 files changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > index e862d1485205..98f5cbe5fdb4 100644 > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > @@ -60,6 +60,14 @@ conditions. > >aliases of secure registers have to be used during > >SMMU configuration. > > > > +- stream-match-mask : Specifies a fixed SMR mask value to combine with > > Needs a vendor prefix. Why does this need a vendor prefix? I'm not fussed either way, but since the stream-match-mask is an optional architectural concept and not specific to an implementation, it seems strange to me that it would need a prefix whereas something like #global-interrupts does not. > > + the Stream ID value from every iommu-specifier. This > > + may be used instead of an "#iommu-cells" value of 2 > > + when there is no need for per-master SMR masks, but > > + it is still desired to mask some portion of every > > + Stream ID (e.g. for certain MMU-500 configurations > > + given globally unique external IDs). Robin -- it might be worth a sentence here saying that the property is ignored if stream matching isn't supported by the hardware. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Drop the of_iommu_{set/get}_ops() interface
On Tue, Jan 03, 2017 at 05:34:56PM +, Lorenzo Pieralisi wrote:
> With the introduction of the new iommu_{register/get}_instance()
> interface in commit e4f10ffe4c9b ("iommu: Make of_iommu_set/get_ops() DT
> agnostic") (based on struct fwnode_handle as look-up token, so firmware
> agnostic) to register IOMMU instances with the core IOMMU layer there is
> no reason to keep the old OF based interface around any longer.
>
> Convert all the IOMMU drivers (and OF IOMMU core code) that rely on the
> of_iommu_{set/get}_ops() to the new kernel interface to register/retrieve
> IOMMU instances and remove the of_iommu_{set/get}_ops() remaining glue
> code in order to complete the interface rework.
>
> Signed-off-by: Lorenzo Pieralisi
> Cc: Matthias Brugger
> Cc: Will Deacon
> Cc: Robin Murphy
> Cc: Joerg Roedel
> Cc: Marek Szyprowski
> ---
> Exynos, msm and mtk code compile tested only owing to lack of
> test platforms, I would appreciate some help in testing this
> patch on those platforms before merging it even if it is just
> a simple interface conversion.
>
> Thanks,
> Lorenzo
>
> drivers/iommu/exynos-iommu.c | 2 +-
> drivers/iommu/msm_iommu.c| 2 +-
> drivers/iommu/mtk_iommu.c| 2 +-
> drivers/iommu/of_iommu.c | 4 ++--
> include/linux/of_iommu.h | 11 ---
> 5 files changed, 5 insertions(+), 16 deletions(-)
Thanks for following up with this cleanup, Lorenzo. I'll queue it locally,
and send it to Joerg for 4.11 if he doesn't apply it manually before then.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V8 6/9] arm/dma-mapping: Implement DMA_ATTR_PRIVILEGED
On Mon, Jan 02, 2017 at 06:42:41PM +0530, Sricharan R wrote:
> The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
> are only accessible to privileged DMA engines. Adding it to the
> arm dma-mapping.c so that the ARM32 DMA IOMMU mapper can make use of it.
>
> Signed-off-by: Sricharan R
> ---
> arch/arm/mm/dma-mapping.c | 60
> +++
> 1 file changed, 30 insertions(+), 30 deletions(-)
Reviewed-by: Will Deacon
Russell: do you mind if I take this via the ARM SMMU tree (which goes
through Joerg's IOMMU tree)?
Will
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index ab77100..82d3e79 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1171,6 +1171,25 @@ static int __init dma_debug_do_init(void)
>
> #ifdef CONFIG_ARM_DMA_USE_IOMMU
>
> +static int __dma_info_to_prot(enum dma_data_direction dir, unsigned long
> attrs)
> +{
> + int prot = 0;
> +
> + if (attrs & DMA_ATTR_PRIVILEGED)
> + prot |= IOMMU_PRIV;
> +
> + switch (dir) {
> + case DMA_BIDIRECTIONAL:
> + return prot | IOMMU_READ | IOMMU_WRITE;
> + case DMA_TO_DEVICE:
> + return prot | IOMMU_READ;
> + case DMA_FROM_DEVICE:
> + return prot | IOMMU_WRITE;
> + default:
> + return prot;
> + }
> +}
> +
> /* IOMMU */
>
> static int extend_iommu_mapping(struct dma_iommu_mapping *mapping);
> @@ -1394,7 +1413,8 @@ static int __iommu_free_buffer(struct device *dev,
> struct page **pages,
> * Create a mapping in device IO address space for specified pages
> */
> static dma_addr_t
> -__iommu_create_mapping(struct device *dev, struct page **pages, size_t size)
> +__iommu_create_mapping(struct device *dev, struct page **pages, size_t size,
> +unsigned long attrs)
> {
> struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> @@ -1419,7 +1439,7 @@ static int __iommu_free_buffer(struct device *dev,
> struct page **pages,
>
> len = (j - i) << PAGE_SHIFT;
> ret = iommu_map(mapping->domain, iova, phys, len,
> - IOMMU_READ|IOMMU_WRITE);
> + __dma_info_to_prot(DMA_BIDIRECTIONAL, attrs));
> if (ret < 0)
> goto fail;
> iova += len;
> @@ -1476,7 +1496,8 @@ static struct page **__iommu_get_pages(void *cpu_addr,
> unsigned long attrs)
> }
>
> static void *__iommu_alloc_simple(struct device *dev, size_t size, gfp_t gfp,
> - dma_addr_t *handle, int coherent_flag)
> + dma_addr_t *handle, int coherent_flag,
> + unsigned long attrs)
> {
> struct page *page;
> void *addr;
> @@ -1488,7 +1509,7 @@ static void *__iommu_alloc_simple(struct device *dev,
> size_t size, gfp_t gfp,
> if (!addr)
> return NULL;
>
> - *handle = __iommu_create_mapping(dev, &page, size);
> + *handle = __iommu_create_mapping(dev, &page, size, attrs);
> if (*handle == DMA_ERROR_CODE)
> goto err_mapping;
>
> @@ -1522,7 +1543,7 @@ static void *__arm_iommu_alloc_attrs(struct device
> *dev, size_t size,
>
> if (coherent_flag == COHERENT || !gfpflags_allow_blocking(gfp))
> return __iommu_alloc_simple(dev, size, gfp, handle,
> - coherent_flag);
> + coherent_flag, attrs);
>
> /*
>* Following is a work-around (a.k.a. hack) to prevent pages
> @@ -1537,7 +1558,7 @@ static void *__arm_iommu_alloc_attrs(struct device
> *dev, size_t size,
> if (!pages)
> return NULL;
>
> - *handle = __iommu_create_mapping(dev, pages, size);
> + *handle = __iommu_create_mapping(dev, pages, size, attrs);
> if (*handle == DMA_ERROR_CODE)
> goto err_buffer;
>
> @@ -1672,27 +1693,6 @@ static int arm_iommu_get_sgtable(struct device *dev,
> struct sg_table *sgt,
>GFP_KERNEL);
> }
>
> -static int __dma_direction_to_prot(enum dma_data_direction dir)
> -{
> - int prot;
> -
> - switch (dir) {
> - case DMA_BIDIRECTIONAL:
> - prot = IOMMU_READ | IOMMU_WRITE;
> - break;
> - case DMA_TO_DEVICE:
> - prot = IOMMU_READ;
> - break;
> - case DMA_FROM_DEVICE:
> -
Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling
On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote:
> TODO maybe we want two options, one to enable stalling, and 2nd to punt
> handling to wq? I haven't needed to use mm APIs from fault handler yet
> (although it is something that I think we'll want some day). Perhaps
> stalling support is limited to just letting driver dump some extra
> debugging information otherwise. Threaded handling probably only useful
> with stalling, but inverse may not always be true.
I'd actually like to see this stuck on a worker thread, because I think
that's more generally useful and I don't want to have a situation where
sometimes the IOMMU fault notifier is run in IRQ context and sometimes it's
not.
>
> Signed-off-by: Rob Clark
> ---
> .../devicetree/bindings/iommu/arm,smmu.txt | 3 ++
> drivers/iommu/arm-smmu.c | 42
> ++
> 2 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index ef465b0..5f405a6 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -68,6 +68,9 @@ conditions.
>aliases of secure registers have to be used during
>SMMU configuration.
>
> +- arm,smmu-enable-stall : Enable stall mode to stall memory transactions
> + and resume after fault is handled
> +
> ** Deprecated properties:
>
> - mmu-masters (deprecated in favour of the generic "iommus" binding) :
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index d505432..a71cb8f 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -350,6 +350,7 @@ struct arm_smmu_device {
> u32 features;
>
> #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
> +#define ARM_SMMU_OPT_ENABLE_STALL (1 << 1)
> u32 options;
> enum arm_smmu_arch_version version;
> enum arm_smmu_implementationmodel;
> @@ -425,6 +426,7 @@ static bool using_legacy_binding, using_generic_binding;
>
> static struct arm_smmu_option_prop arm_smmu_options[] = {
> { ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" },
> + { ARM_SMMU_OPT_ENABLE_STALL, "arm,smmu-enable-stall" },
> { 0, NULL},
> };
>
> @@ -676,7 +678,8 @@ static struct iommu_gather_ops arm_smmu_gather_ops = {
>
> static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> {
> - u32 fsr, fsynr;
> + int flags, ret;
> + u32 fsr, fsynr, resume;
> unsigned long iova;
> struct iommu_domain *domain = dev;
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> @@ -690,15 +693,40 @@ static irqreturn_t arm_smmu_context_fault(int irq, void
> *dev)
> if (!(fsr & FSR_FAULT))
> return IRQ_NONE;
>
> + if (fsr & FSR_IGN)
> + dev_err_ratelimited(smmu->dev,
> + "Unexpected context fault (fsr 0x%x)\n",
> + fsr);
> +
> fsynr = readl_relaxed(cb_base + ARM_SMMU_CB_FSYNR0);
> - iova = readq_relaxed(cb_base + ARM_SMMU_CB_FAR);
> + flags = fsynr & FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ;
>
> - dev_err_ratelimited(smmu->dev,
> - "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cb=%d\n",
> - fsr, iova, fsynr, cfg->cbndx);
> + iova = readq_relaxed(cb_base + ARM_SMMU_CB_FAR);
> + if (!report_iommu_fault(domain, smmu->dev, iova, flags)) {
> + ret = IRQ_HANDLED;
> + resume = RESUME_RETRY;
> + } else {
> + dev_err_ratelimited(smmu->dev,
> + "Unhandled context fault: iova=0x%08lx, fsynr=0x%x,
> cb=%d\n",
> + iova, fsynr, cfg->cbndx);
> + ret = IRQ_NONE;
> + resume = RESUME_TERMINATE;
> + }
>
> + /* Clear the faulting FSR */
> writel(fsr, cb_base + ARM_SMMU_CB_FSR);
> - return IRQ_HANDLED;
> +
> + /* Retry or terminate any stalled transactions */
> + if (fsr & FSR_SS) {
> + /* Should we care about ending up w/ a stalled transaction
> + * when we didn't ask for it? I guess for now best to call
> + * attention to it and resume anyways.
> + */
> + WARN_ON(!(smmu->options & ARM_SMMU_OPT_ENABLE_STALL));
I don't think we need to care about this. If we're getting stall faults
with CFCFG clear, then something has gone drastically wrong in the hardware
and we'll probably see "Unhandled context fault" anyway.
> + writel_relaxed(resume, cb_base + ARM_SMMU_CB_RESUME);
> + }
> +
> + return ret;
> }
>
> static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> @@ -824,6 +852,8 @@ static void arm_smmu_init_context_bank(struct
> arm_smmu_
Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling
On Thu, Jan 05, 2017 at 12:08:57PM +, Mark Rutland wrote: > On Thu, Jan 05, 2017 at 11:55:29AM +0000, Will Deacon wrote: > > On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote: > > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > > b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > > index ef465b0..5f405a6 100644 > > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > > @@ -68,6 +68,9 @@ conditions. > > >aliases of secure registers have to be used during > > >SMMU configuration. > > > > > > +- arm,smmu-enable-stall : Enable stall mode to stall memory transactions > > > + and resume after fault is handled > > The wording here seems to describe a policy rather than a property. > > Can you elaborate on when/why this is required/preferred/valid? It's not a policy, it's a hardware capability. There are some non-probeable reasons why stalling mode is unsafe or unusable: http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/474530.html Some of these are specific to the SMMU implementation (e.g. whether or not the SS bit can remain set without reasserting the IRQ) and some are specific to the integration (e.g. whether or not stalling an endpoint can deadlock the SoC). The key point is that, without support from both the implementation and the integration, stalls are unusable. > > > static irqreturn_t arm_smmu_global_fault(int irq, void *dev) > > > @@ -824,6 +852,8 @@ static void arm_smmu_init_context_bank(struct > > > arm_smmu_domain *smmu_domain, > > > > > > /* SCTLR */ > > > reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M; > > > + if (smmu->options & ARM_SMMU_OPT_ENABLE_STALL) > > > + reg |= SCTLR_CFCFG; > > > > I wonder if this should also be predicated on the compatible string, so > > that the "arm,smmu-enable-stall" property is ignored (with a warning) if > > the compatible string isn't specific enough to identify an implementation > > with the required SS behaviour? On the other hand, it feels pretty > > redundant and a single "stalling works" property is all we need. > > Can you elaborate on what "stalling works" entails? Is that just the SS > bit behaviour? are there integration or endpoint-specific things that we > need to care about? See above. The "stalling works" property (arm,smmu-enable-stall) would indicate that both the implementation *and* the integration are such that stalling is usable for demand paging. I suspect there are endpoints that can't deal with stalls (e.g. they might timeout and signal a RAS event), but in that case their respective device drivers should ensure that any DMA buffers are pinned and/or register a fault handler to request termination of the faulting transaction. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling
On Thu, Jan 05, 2017 at 02:07:31PM +, Mark Rutland wrote: > On Thu, Jan 05, 2017 at 02:00:05PM +0000, Will Deacon wrote: > > On Thu, Jan 05, 2017 at 12:08:57PM +, Mark Rutland wrote: > > > On Thu, Jan 05, 2017 at 11:55:29AM +, Will Deacon wrote: > > > > On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote: > > > > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > > > > b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > > > > index ef465b0..5f405a6 100644 > > > > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > > > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > > > > @@ -68,6 +68,9 @@ conditions. > > > > >aliases of secure registers have to be used during > > > > >SMMU configuration. > > > > > > > > > > +- arm,smmu-enable-stall : Enable stall mode to stall memory > > > > > transactions > > > > > + and resume after fault is handled > > > > > > The wording here seems to describe a policy rather than a property. > > > > > > Can you elaborate on when/why this is required/preferred/valid? > > > > It's not a policy, it's a hardware capability. There are some non-probeable > > reasons why stalling mode is unsafe or unusable: > > > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/474530.html > > Ok. My point was that the wording above is an imperative -- it tells > the kernel to enable stall mode, not if/why it is safe to do so (i.e. it > is a policy, not a property). > > It sounds like that's just a wording issue. Something like > "arm,stalling-is-usable" (along with a descrition of when that > can/should be in the DT) would be vastly better. Why does it need a vendor prefix? I'm not down on the convention there. "stalling-safe" or "stalling-supported" are alternative strings. > > > > > static irqreturn_t arm_smmu_global_fault(int irq, void *dev) > > > > > @@ -824,6 +852,8 @@ static void arm_smmu_init_context_bank(struct > > > > > arm_smmu_domain *smmu_domain, > > > > > > > > > > /* SCTLR */ > > > > > reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M; > > > > > + if (smmu->options & ARM_SMMU_OPT_ENABLE_STALL) > > > > > + reg |= SCTLR_CFCFG; > > > > > > > > I wonder if this should also be predicated on the compatible string, so > > > > that the "arm,smmu-enable-stall" property is ignored (with a warning) if > > > > the compatible string isn't specific enough to identify an > > > > implementation > > > > with the required SS behaviour? On the other hand, it feels pretty > > > > redundant and a single "stalling works" property is all we need. > > > > > > Can you elaborate on what "stalling works" entails? Is that just the SS > > > bit behaviour? are there integration or endpoint-specific things that we > > > need to care about? > > > > See above. The "stalling works" property (arm,smmu-enable-stall) would > > indicate that both the implementation *and* the integration are such > > that stalling is usable for demand paging. I suspect there are endpoints > > that can't deal with stalls (e.g. they might timeout and signal a RAS > > event), but in that case their respective device drivers should ensure > > that any DMA buffers are pinned and/or register a fault handler to > > request termination of the faulting transaction. > > Ok. It would be good to elaborate on what "stalling is useable" means in > the property description. i.e. what specificallty the implementation and > integration need to ensure. We can describe some of those guarantees in the property description, but it's difficult to enumerate them exhaustively. For example, you wouldn't want stalling to lead to data corruption, denial of service, or for the thing to catch fire, but having those as explicit requirements is a bit daft. It's also impossible to check that you thought of everything. Aside from renaming the option, I'm really after an opinion on whether it's better to have one property or combine it with the compatible string, because I can see benefits of both and don't much care either way. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling
On Thu, Jan 05, 2017 at 10:27:27AM -0500, Rob Clark wrote: > On Thu, Jan 5, 2017 at 6:55 AM, Will Deacon wrote: > > On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote: > >> TODO maybe we want two options, one to enable stalling, and 2nd to punt > >> handling to wq? I haven't needed to use mm APIs from fault handler yet > >> (although it is something that I think we'll want some day). Perhaps > >> stalling support is limited to just letting driver dump some extra > >> debugging information otherwise. Threaded handling probably only useful > >> with stalling, but inverse may not always be true. > > > > I'd actually like to see this stuck on a worker thread, because I think > > that's more generally useful and I don't want to have a situation where > > sometimes the IOMMU fault notifier is run in IRQ context and sometimes it's > > not. > > So I was talking a bit w/ Jordan on IRC yesterday.. and we also have > the GPU's hw hang-detect to contend with. So I *suspect* that when we > get to the point of using this to do things like page in things from > swap and resume the faulting transaction, we probably want to get > called immediately from the IRQ handler so we can disable the hw > hang-detect. Well, if you want to use an SMMU for paging, then the GPU driver would need to request that explicitly when allocating its DMA buffers, to that would be the time to either delay or disable the hang detection. > I'm not sure if the better solution then would be to have two fault > callbacks, one immediately from the IRQ and a later one from wq. Or > let the driver handle the wq business and give it a way to tell the > IOMMU when to resume. > > I kinda think we should punt on the worker thread for now until we are > ready to resume faulting transactions, because I guess a strong chance > that whatever way we do it now will be wrong ;-) I guess what I'm after is for you to change the interrupt handlers to be threaded, like they are for SMMUv3. I *think* you can do that with a NULL thread_fn for now, and just call report_iommu_fault from the handler. The return value of that could, in theory, be used to queued the paging request and wake the paging thread in future. > > I wonder if this should also be predicated on the compatible string, so > > that the "arm,smmu-enable-stall" property is ignored (with a warning) if > > the compatible string isn't specific enough to identify an implementation > > with the required SS behaviour? On the other hand, it feels pretty > > redundant and a single "stalling works" property is all we need. > > We could also drop the property and key the behavior on specific > compat strings I guess. Having both seems a bit odd. Anyways, I'll > defer to DT folks about what the cleaner approach is. As Robin pointed out, we do need to be able to distinguish the integration of the device from the device itself. For example, MMU-9000 might be capable of stalling, but if it's bolted to a PCI RC, it's not safe to do so. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling
On Thu, Jan 05, 2017 at 03:32:50PM +, Robin Murphy wrote: > On 05/01/17 14:47, Will Deacon wrote: > > On Thu, Jan 05, 2017 at 02:07:31PM +, Mark Rutland wrote: > >> Ok. It would be good to elaborate on what "stalling is useable" means in > >> the property description. i.e. what specificallty the implementation and > >> integration need to ensure. > > > > We can describe some of those guarantees in the property description, but > > it's difficult to enumerate them exhaustively. For example, you wouldn't > > want stalling to lead to data corruption, denial of service, or for the > > thing to catch fire, but having those as explicit requirements is a bit > > daft. It's also impossible to check that you thought of everything. > > > > Aside from renaming the option, I'm really after an opinion on whether > > it's better to have one property or combine it with the compatible > > string, because I can see benefits of both and don't much care either > > way. > > The SMMU implementation side of the decision (i.e. independence of IRQ > assertion vs. SS) seems like exactly the sort of stuff the compatible > string already has covered. The integration side I'm less confident can > be described this way at all - the "this device definitely won't go > wrong if stalled for an indefinite length of time" is inherently a > per-master thing, so a single property on the SMMU implying that for > every device connected to it seems a bit optimistic, and breaks down as > soon as you have one device in the system for which that isn't true (a > PCI root complex, say), even if that guy's traffic never crosses paths > with whichever few devices you actually care about using stalls with. > > I think this needs to be some kind of "arm,smmu-stall-safe" property > placed on individual master device nodes (mad idea: or even an extra > cell of flags in the IOMMU specifier) to encapsulate both that the given > device itself is OK with being stalled, and that it's integrated in such > a way that its stalled transactions cannot disrupt any *other* device > (e.g. it has a TBU all to itself). Then upon initialising a context bank > on a suitable SMMU implementation, we set CFCFG based on whatever device > is being attached to the corresponding domain, and refuse any subsequent > attempts to attach a non-stallable device to a stalling domain (and > possibly even vice-versa). If we're going to add per-master properties, I'd *really* like them to be independent of the IOMMU in use. That is, we should be able to re-use this property as part of supporting SVM for platform devices in future. But I think we agree that we need: 1. A compatible string for the SMMU that can be used to infer the SS behaviour in the driver 2. A property on the SMMU to say that it's been integrated in such a way that stalling is safe (doesn't deadlock) 3. A generic master property that says that the device can DMA to unpinned memory Anything else? Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling
On Thu, Jan 05, 2017 at 05:03:30PM +, Robin Murphy wrote: > On 05/01/17 16:07, Will Deacon wrote: > > On Thu, Jan 05, 2017 at 03:32:50PM +, Robin Murphy wrote: > >> I think this needs to be some kind of "arm,smmu-stall-safe" property > >> placed on individual master device nodes (mad idea: or even an extra > >> cell of flags in the IOMMU specifier) to encapsulate both that the given > >> device itself is OK with being stalled, and that it's integrated in such > >> a way that its stalled transactions cannot disrupt any *other* device > >> (e.g. it has a TBU all to itself). Then upon initialising a context bank > >> on a suitable SMMU implementation, we set CFCFG based on whatever device > >> is being attached to the corresponding domain, and refuse any subsequent > >> attempts to attach a non-stallable device to a stalling domain (and > >> possibly even vice-versa). > > > > If we're going to add per-master properties, I'd *really* like them to be > > independent of the IOMMU in use. That is, we should be able to re-use this > > property as part of supporting SVM for platform devices in future. > > I'd argue that they are still fairly separate things despite the > overlap: stalling is a specific ARM SMMU architecture thing (in both > architectures) which may be used for purposes unrelated to SVM; > conversely SVM implemented via PRI or similar mechanisms should be > pretty much oblivious to the transaction fault model. But SVM for a platform device is likely to require working stalls, so they're highly related concepts. Having the former be a generic property but the second tied to the SMMU means we have to duplicate information (that is, we end up with a generic "SVM-capable" property that implies that stalling works). I don't think we want that at all. > > But I think we agree that we need: > > > > 1. A compatible string for the SMMU that can be used to infer the SS > > behaviour in the driver > > > > 2. A property on the SMMU to say that it's been integrated in such a > > way that stalling is safe (doesn't deadlock) > > That's still got to be a per-master property, not a SMMU property, I > think. To illustrate: > > [A] [B] [C] >| |_| > __|__|___ > | TBU || TBU | > |_| SMMU |_| > |__|__|__| >| | > > Say A and B are instances of some device happy to be stalled, and C is a > PCIe RC, and each is attached to their own context bank - enabling > stalls for A is definitely fine. However even though B and C are using > different context banks, enabling stalls for B might deadlock C if it > results in more total outstanding transactions than the TBU's slave port > supports. Therefore A can happily claim to be stall-safe, but B cannot > due to its integration with respect to C. So in this case, don't say that B and C can DMA to unpinned memory. You need the third property. This property (property 2) is concerned with the SMMU itself because, e.g. the way the walker has been integrated can cause a deadlock. > And yes, I can point you at some existing hardware which really does > posess a topology like that. > > > 3. A generic master property that says that the device can DMA to > > unpinned memory > > That sounds a bit *too* generic to me, given that there are multiple > incompatible ways that could be implemented. I'm not the biggest fan of > properties with heavily context-specific interpretations, especially > when there's more than a hint of software implementation details in the mix. So what would you propose for SVM? I really want that to be opt-in, so a per-master flag seems like the right way forward to me. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 00/19] KVM PCIe/MSI passthrough on ARM/ARM64 and IOVA reserved regions
On Tue, Jan 10, 2017 at 03:09:24PM +0100, Joerg Roedel wrote: > On Mon, Jan 09, 2017 at 01:45:51PM +, Eric Auger wrote: > > Eric Auger (17): > > iommu: Rename iommu_dm_regions into iommu_resv_regions > > iommu: Add a new type field in iommu_resv_region > > iommu: iommu_alloc_resv_region > > iommu: Only map direct mapped regions > > iommu: iommu_get_group_resv_regions > > iommu: Implement reserved_regions iommu-group sysfs file > > iommu/vt-d: Implement reserved region get/put callbacks > > iommu/amd: Declare MSI and HT regions as reserved IOVA regions > > iommu/arm-smmu: Implement reserved region get/put callbacks > > iommu/arm-smmu-v3: Implement reserved region get/put callbacks > > IOMMU patches look good, what is the plan to merge this? I'd like to > take the IOMMU patches and can provide a branch for someone else to base > the rest on. I'm perfectly happy with this going through you. In fact, I suspect you could take the whole series once Marc is happy with the few remaining irq domain niggles (which are really minor at this point). That just leaves the VFIO bits -- Alex, are you happy with those now? If not, we can hold off on the last three patches for the time being. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 11/19] iommu/arm-smmu: Implement reserved region get/put callbacks
On Mon, Jan 09, 2017 at 01:46:02PM +, Eric Auger wrote: > The get() populates the list with the MSI IOVA reserved window. > > At the moment an arbitray MSI IOVA window is set at 0x800 > of size 1MB. This will allow to report those info in iommu-group > sysfs. > > Signed-off-by: Eric Auger > > --- > > v3 -> v4: > - do not handle PCI host bridge windows anymore > - encode prot > > RFC v2 -> v3: > - use existing get/put_resv_regions > > RFC v1 -> v2: > - use defines for MSI IOVA base and length > --- > drivers/iommu/arm-smmu.c | 28 ++++ > 1 file changed, 28 insertions(+) Acked-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 12/19] iommu/arm-smmu-v3: Implement reserved region get/put callbacks
On Mon, Jan 09, 2017 at 01:46:03PM +, Eric Auger wrote: > iommu/arm-smmu: Implement reserved region get/put callbacks > > The get() populates the list with the MSI IOVA reserved window. > > At the moment an arbitray MSI IOVA window is set at 0x800 > of size 1MB. This will allow to report those info in iommu-group > sysfs. > > Signed-off-by: Eric Auger > > --- > > v4: creation > --- > drivers/iommu/arm-smmu-v3.c | 28 > 1 file changed, 28 insertions(+) Acked-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 19/19] iommu/arm-smmu: Do not advertise IOMMU_CAP_INTR_REMAP anymore
On Mon, Jan 09, 2017 at 01:46:10PM +, Eric Auger wrote: > IOMMU_CAP_INTR_REMAP has been advertised in arm-smmu(-v3) although > on ARM this property is not attached to the IOMMU but rather is > implemented in the MSI controller (GICv3 ITS). > > Now vfio_iommu_type1 checks MSI remapping capability at MSI controller > level, let's correct this. > > Signed-off-by: Eric Auger > --- > drivers/iommu/arm-smmu-v3.c | 2 -- > drivers/iommu/arm-smmu.c| 2 -- > 2 files changed, 4 deletions(-) Dependent on the previous two VFIO patches: Acked-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling
Hi Rob, On Fri, Jan 06, 2017 at 11:26:49AM -0500, Rob Clark wrote: > On Thu, Jan 5, 2017 at 10:49 AM, Will Deacon wrote: > > On Thu, Jan 05, 2017 at 10:27:27AM -0500, Rob Clark wrote: > >> I'm not sure if the better solution then would be to have two fault > >> callbacks, one immediately from the IRQ and a later one from wq. Or > >> let the driver handle the wq business and give it a way to tell the > >> IOMMU when to resume. > >> > >> I kinda think we should punt on the worker thread for now until we are > >> ready to resume faulting transactions, because I guess a strong chance > >> that whatever way we do it now will be wrong ;-) > > > > I guess what I'm after is for you to change the interrupt handlers to be > > threaded, like they are for SMMUv3. I *think* you can do that with a NULL > > thread_fn for now, and just call report_iommu_fault from the handler. > > The return value of that could, in theory, be used to queued the paging > > request and wake the paging thread in future. > > If we only pass in the non-threaded irq fxn, I'm not really sure how > that changes anything.. or maybe I'm not understanding what you mean. > > But yeah, I guess we could use request_threaded_irq() to get both IRQ > context notification and a later thread context notification rather > than doing the wq thing. Either way the iommu API has to change > slightly. > > >> > I wonder if this should also be predicated on the compatible string, so > >> > that the "arm,smmu-enable-stall" property is ignored (with a warning) if > >> > the compatible string isn't specific enough to identify an implementation > >> > with the required SS behaviour? On the other hand, it feels pretty > >> > redundant and a single "stalling works" property is all we need. > >> > >> We could also drop the property and key the behavior on specific > >> compat strings I guess. Having both seems a bit odd. Anyways, I'll > >> defer to DT folks about what the cleaner approach is. > > > > As Robin pointed out, we do need to be able to distinguish the integration > > of the device from the device itself. For example, MMU-9000 might be capable > > of stalling, but if it's bolted to a PCI RC, it's not safe to do so. > > Hmm, well we install the fault handler on the iommu_domain.. perhaps > maybe a combo of dts property (or deciding based on more specific > compat string), plus extra param passed in to > iommu_set_fault_hander(). The dts property or compat string to > indicate whether the iommu (and how it is wired up) can handle stalls, > and enable_stall param when fault handler is registered to indicate > whether the device itself can cope.. if either can't do stalling, then > don't set CFCFG. I thought about this some more, and I think you're right. Having iommu_set_fault_handler take a flags parameter indicating that, for example, the fault handler can deal with paging, is all we need to implement the per-master opt-in functionality for stalling faults. There's no real requirement to standardise a generic firmware property for that (but we still need *something* that says stalling is usable on the SMMU -- perhaps just the compatible string is ok). Taking this further, there's then no need for the threaded IRQ function in the SMMUv2 driver after all. Instead, we pass a continuation function pointer and opaque token from the SMMU driver to the fault handler in IRQ context (this will be in thread context for SMMUv3, but that should be fine). The fault handler can then stash these someplace, and signal a wakeup for its own threaded handler, which ultimately calls the SMMU continuation function with the opaque token as a parameter when it's done with the fault. I think that's enough to get things rolling without adding lots of infrastructure to the SMMU driver initially. If a pattern emerges amongst users of the interface, then we could consolidate some of the work handling back into IOMMU core. What do you think? It should all be pretty straightforward for what you want to do. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling
On Tue, Jan 10, 2017 at 02:20:13PM -0500, Rob Clark wrote: > On Tue, Jan 10, 2017 at 12:52 PM, Will Deacon wrote: > > On Fri, Jan 06, 2017 at 11:26:49AM -0500, Rob Clark wrote: > >> Hmm, well we install the fault handler on the iommu_domain.. perhaps > >> maybe a combo of dts property (or deciding based on more specific > >> compat string), plus extra param passed in to > >> iommu_set_fault_hander(). The dts property or compat string to > >> indicate whether the iommu (and how it is wired up) can handle stalls, > >> and enable_stall param when fault handler is registered to indicate > >> whether the device itself can cope.. if either can't do stalling, then > >> don't set CFCFG. > > > > I thought about this some more, and I think you're right. Having > > iommu_set_fault_handler take a flags parameter indicating that, for example, > > the fault handler can deal with paging, is all we need to implement the > > per-master opt-in functionality for stalling faults. There's no real > > requirement to standardise a generic firmware property for that (but > > we still need *something* that says stalling is usable on the SMMU -- > > perhaps just the compatible string is ok). > > btw, it occurred to me that maybe it should be flags param to > iommu_attach_device() (just in case fault handler not installed?) > otoh stalling without a fault handler is silly, but I guess we need it > to infer whether stalling can be supported by other devices on same > iommu.. tbh I'm on a bit shaky ground when it comes to multiple > devices per iommu since the SoC's I'm familiar with do it the other > way around. But I guess you have thought more about the multi-device > case, so figured I should suggest it.. I don't think it works at attach time, because the stalling property belongs to the domain, rather than the individual devices within it. Similarly, I don't think we should allow this property to be toggled once devices have been attached. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling
On Wed, Jan 11, 2017 at 03:59:30PM -0500, Rob Clark wrote: > On Wed, Jan 11, 2017 at 4:36 AM, Will Deacon wrote: > > On Tue, Jan 10, 2017 at 02:20:13PM -0500, Rob Clark wrote: > >> On Tue, Jan 10, 2017 at 12:52 PM, Will Deacon wrote: > >> > On Fri, Jan 06, 2017 at 11:26:49AM -0500, Rob Clark wrote: > >> >> Hmm, well we install the fault handler on the iommu_domain.. perhaps > >> >> maybe a combo of dts property (or deciding based on more specific > >> >> compat string), plus extra param passed in to > >> >> iommu_set_fault_hander(). The dts property or compat string to > >> >> indicate whether the iommu (and how it is wired up) can handle stalls, > >> >> and enable_stall param when fault handler is registered to indicate > >> >> whether the device itself can cope.. if either can't do stalling, then > >> >> don't set CFCFG. > >> > > >> > I thought about this some more, and I think you're right. Having > >> > iommu_set_fault_handler take a flags parameter indicating that, for > >> > example, > >> > the fault handler can deal with paging, is all we need to implement the > >> > per-master opt-in functionality for stalling faults. There's no real > >> > requirement to standardise a generic firmware property for that (but > >> > we still need *something* that says stalling is usable on the SMMU -- > >> > perhaps just the compatible string is ok). > >> > >> btw, it occurred to me that maybe it should be flags param to > >> iommu_attach_device() (just in case fault handler not installed?) > >> otoh stalling without a fault handler is silly, but I guess we need it > >> to infer whether stalling can be supported by other devices on same > >> iommu.. tbh I'm on a bit shaky ground when it comes to multiple > >> devices per iommu since the SoC's I'm familiar with do it the other > >> way around. But I guess you have thought more about the multi-device > >> case, so figured I should suggest it.. > > > > I don't think it works at attach time, because the stalling property belongs > > to the domain, rather than the individual devices within it. Similarly, I > > don't think we should allow this property to be toggled once devices have > > been attached. > > > > hmm, I was more thinking of cases where drivers for particular devices > need some work (ie. like potentially disabling hw hang detect during > faults).. I guess we could have three levels, that all have to be true > in order to enable stall: smmu, domain (pass flags in to > iommu_domain_alloc()??), and device (iommu_attach_device())? Hooking iommu_set_fault_handler, as you originally suggested, is the best way to set the flag on the domain. I think we just need to enforce that iommu_set_fault_handler is called prior to attaching devices to a domain, so that the IOMMU driver can configure the domain appropriately on the first attach. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: limit use of 2-level stream tables
On Tue, Jan 10, 2017 at 02:47:13PM -0500, Nate Watterson wrote: > In the current arm-smmu-v3 driver, all smmus that support 2-level > stream tables are being forced to use them. This is suboptimal for > smmus that support fewer stream id bits than would fill in a single > second level table. This patch limits the use of 2-level tables to > smmus that both support the feature and whose first level table can > possibly contain more than a single entry. > > Signed-off-by: Nate Watterson > --- > drivers/iommu/arm-smmu-v3.c | 21 ++--- > 1 file changed, 10 insertions(+), 11 deletions(-) Thanks Nate, I'll queue this for 4.11. Sorry for messing you about before. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4] iommu/arm-smmu: Support for Extended Stream ID (16 bit)
On Thu, Jan 19, 2017 at 05:36:36PM +0300, Aleksey Makarov wrote: > It is the time we have the real 16-bit Stream ID user, which is the > ThunderX. Its IO topology uses 1:1 map for Requester ID to Stream ID > translation for each root complex which allows to get full 16-bit > Stream ID. Firmware assigns bus IDs that are greater than 128 (0x80) > to some buses under PEM (external PCIe interface). Eventually SMMU > drops devices on that buses because their Stream ID is out of range: > > pci 0006:90:00.0: stream ID 0x9000 out of range for SMMU (0x7fff) > > To fix above issue enable the Extended Stream ID optional feature > when available. > > Reviewed-by: Tomasz Nowicki > Signed-off-by: Aleksey Makarov > Tested-by: Tomasz Nowicki > --- > v4: > change the commit message: > - explain the reason why the warning happens (Tomasz Nowicki) > - add 'Reviewed-by' and 'Tested-by' from Tomasz Nowicki Thanks, I'll queue this for 4.11. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] iommu/arm-smmu: Fix for ThunderX erratum #27704
On Mon, Jan 16, 2017 at 08:16:07AM +0100, Tomasz Nowicki wrote: > The goal of erratum #27704 workaround was to make sure that ASIDs and VMIDs > are unique across all SMMU instances on affected Cavium systems. > > Currently, the workaround code partitions ASIDs and VMIDs by increasing > global cavium_smmu_context_count which in turn becomes the base ASID and VMID > value for the given SMMU instance upon the context bank initialization. > > For systems with multiple SMMU instances this approach implies the risk > of crossing 8-bit ASID, like for 1-socket CN88xx capable of 4 SMMUv2, > 128 context banks each: > SMMU_0 (0-127 ASID RANGE) > SMMU_1 (127-255 ASID RANGE) > SMMU_2 (256-383 ASID RANGE) <--- crossing 8-bit ASID > SMMU_3 (384-511 ASID RANGE) <--- crossing 8-bit ASID > > Since now we use 8-bit ASID (SMMU_CBn_TCR2.AS = 0) we effectively misconfigure > ASID[15:8] bits of SMMU_CBn_TTBRm register for SMMU_2/3. Moreover, we still > assume non-zero ASID[15:8] bits upon context invalidation. In the end, > except SMMU_0/1 devices all other devices under other SMMUs will fail on guest > power off/on. Since we try to invalidate TLB with 16-bit ASID but we actually > have 8-bit zero padded 16-bit entry. > > This patch adds 16-bit ASID support for stage-1 AArch64 contexts so that > we use ASIDs consistently for all SMMU instances. > > Signed-off-by: Tomasz Nowicki > Reviewed-by: Robin Murphy > Reviewed-by: Tirumalesh Chalamarla > --- > drivers/iommu/arm-smmu.c | 3 +++ > 1 file changed, 3 insertions(+) Thanks, queued for 4.11. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V5 08/12] iommu/arm-smmu: Clean up early-probing workarounds
On Thu, Jan 19, 2017 at 08:35:52PM +0530, Sricharan R wrote: > From: Robin Murphy > > Now that the appropriate ordering is enforced via profe-deferral of > masters in core code, rip it all out and bask in the simplicity. > > Signed-off-by: Robin Murphy > [Sricharan: Rebased on top of ACPI IORT SMMU series] > Signed-off-by: Sricharan R > --- > * No change > > drivers/iommu/arm-smmu-v3.c | 46 ++- > drivers/iommu/arm-smmu.c| 58 > +++-- > 2 files changed, 10 insertions(+), 94 deletions(-) Acked-by: Will Deacon I can't wait to see this series merged. What's the plan for that? Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V5 07/12] arm64: dma-mapping: Remove the notifier trick to handle early setting of dma_ops
On Thu, Jan 19, 2017 at 08:35:51PM +0530, Sricharan R wrote: > With arch_setup_dma_ops now being called late during device's probe after > the device's iommu is probed, the notifier trick required to handle the > early setup of dma_ops before the iommu group gets created is not > required. So removing the notifier's here. > > Signed-off-by: Sricharan R > [rm: clean up even more] > Signed-off-by: Robin Murphy > --- > * No change > > arch/arm64/mm/dma-mapping.c | 132 > > 1 file changed, 12 insertions(+), 120 deletions(-) Acked-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 0/5] Implement SMMU passthrough using the default domain
Hi all, A number of people have expressed interest in having the SMMU come up in a passthrough configuration, and then allow subsequent translation for things such as VFIO. Rather than do this in each SMMU driver, it's much cleaner to allow the default domain to be configured to be something other than DMA. This patch series implements a command-line option to configure the default domain type. Currently, it supports "dma" and "identity" which is sufficient for the passthrough use-case. Tested on an ARM fastmodel. All feedback welcome, Will --->8 Will Deacon (5): iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains arm64: dma-mapping: Only swizzle DMA ops for IOMMU_DOMAIN_DMA iommu: Allow default domain type to be set on the kernel command line arch/arm64/mm/dma-mapping.c | 17 - drivers/iommu/arm-smmu-v3.c | 20 ++-- drivers/iommu/arm-smmu.c| 26 +++--- drivers/iommu/iommu.c | 19 +-- 4 files changed, 70 insertions(+), 12 deletions(-) -- 2.1.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/5] iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains
In preparation for allowing the default domain type to be overridden,
this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the
ARM SMMU driver.
An identity domain is created by placing the corresponding S2CR
registers into "bypass" mode, which allows transactions to flow through
the SMMU without any translation.
Signed-off-by: Will Deacon
---
drivers/iommu/arm-smmu.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index a328ffb75509..0f5e42a719e5 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -404,6 +404,7 @@ enum arm_smmu_domain_stage {
ARM_SMMU_DOMAIN_S1 = 0,
ARM_SMMU_DOMAIN_S2,
ARM_SMMU_DOMAIN_NESTED,
+ ARM_SMMU_DOMAIN_BYPASS,
};
struct arm_smmu_domain {
@@ -824,6 +825,12 @@ static int arm_smmu_init_domain_context(struct
iommu_domain *domain,
if (smmu_domain->smmu)
goto out_unlock;
+ if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+ smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
+ smmu_domain->smmu = smmu;
+ goto out_unlock;
+ }
+
/*
* Mapping the requested stage onto what we support is surprisingly
* complicated, mainly because the spec allows S1+S2 SMMUs without
@@ -984,7 +991,7 @@ static void arm_smmu_destroy_domain_context(struct
iommu_domain *domain)
void __iomem *cb_base;
int irq;
- if (!smmu)
+ if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
return;
/*
@@ -1007,7 +1014,9 @@ static struct iommu_domain
*arm_smmu_domain_alloc(unsigned type)
{
struct arm_smmu_domain *smmu_domain;
- if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+ if (type != IOMMU_DOMAIN_UNMANAGED &&
+ type != IOMMU_DOMAIN_DMA &&
+ type != IOMMU_DOMAIN_IDENTITY)
return NULL;
/*
* Allocate the domain and initialise some of its data structures.
@@ -1205,10 +1214,15 @@ static int arm_smmu_domain_add_master(struct
arm_smmu_domain *smmu_domain,
{
struct arm_smmu_device *smmu = smmu_domain->smmu;
struct arm_smmu_s2cr *s2cr = smmu->s2crs;
- enum arm_smmu_s2cr_type type = S2CR_TYPE_TRANS;
u8 cbndx = smmu_domain->cfg.cbndx;
+ enum arm_smmu_s2cr_type type;
int i, idx;
+ if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS)
+ type = S2CR_TYPE_BYPASS;
+ else
+ type = S2CR_TYPE_TRANS;
+
for_each_cfg_sme(fwspec, i, idx) {
if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
continue;
--
2.1.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4/5] arm64: dma-mapping: Only swizzle DMA ops for IOMMU_DOMAIN_DMA
The arm64 DMA-mapping implementation sets the DMA ops to the IOMMU DMA
ops if we detect that an IOMMU is present for the master and the DMA
ranges are valid.
In the case when the IOMMU domain for the device is not of type
IOMMU_DOMAIN_DMA, then we have no business swizzling the ops, since
we're not in control of the underlying address space. This patch leaves
the DMA ops alone for masters attached to non-DMA IOMMU domains.
Signed-off-by: Will Deacon
---
arch/arm64/mm/dma-mapping.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index e04082700bb1..5d3c6ad621e8 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -831,14 +831,21 @@ static bool do_iommu_attach(struct device *dev, const
struct iommu_ops *ops,
* then the IOMMU core will have already configured a group for this
* device, and allocated the default domain for that group.
*/
- if (!domain || iommu_dma_init_domain(domain, dma_base, size, dev)) {
- pr_warn("Failed to set up IOMMU for device %s; retaining
platform DMA ops\n",
- dev_name(dev));
- return false;
+ if (!domain)
+ goto out_err;
+
+ if (domain->type == IOMMU_DOMAIN_DMA) {
+ if (iommu_dma_init_domain(domain, dma_base, size, dev))
+ goto out_err;
+
+ dev->archdata.dma_ops = &iommu_dma_ops;
}
- dev->archdata.dma_ops = &iommu_dma_ops;
return true;
+out_err:
+ pr_warn("Failed to set up IOMMU for device %s; retaining platform DMA
ops\n",
+dev_name(dev));
+ return false;
}
static void queue_iommu_attach(struct device *dev, const struct iommu_ops *ops,
--
2.1.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 3/5] iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains
In preparation for allowing the default domain type to be overridden,
this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the
ARM SMMUv3 driver.
An identity domain is created by placing the corresponding stream table
entries into "bypass" mode, which allows transactions to flow through
the SMMU without any translation.
Signed-off-by: Will Deacon
---
drivers/iommu/arm-smmu-v3.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index c254325b0c7a..d33291274455 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -629,6 +629,7 @@ enum arm_smmu_domain_stage {
ARM_SMMU_DOMAIN_S1 = 0,
ARM_SMMU_DOMAIN_S2,
ARM_SMMU_DOMAIN_NESTED,
+ ARM_SMMU_DOMAIN_BYPASS,
};
struct arm_smmu_domain {
@@ -1385,7 +1386,9 @@ static struct iommu_domain
*arm_smmu_domain_alloc(unsigned type)
{
struct arm_smmu_domain *smmu_domain;
- if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+ if (type != IOMMU_DOMAIN_UNMANAGED &&
+ type != IOMMU_DOMAIN_DMA &&
+ type != IOMMU_DOMAIN_IDENTITY)
return NULL;
/*
@@ -1516,6 +1519,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain
*domain)
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct arm_smmu_device *smmu = smmu_domain->smmu;
+ if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+ smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
+ return 0;
+ }
+
/* Restrict the stage to what we can actually support */
if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
@@ -1651,7 +1659,9 @@ static int arm_smmu_attach_dev(struct iommu_domain
*domain, struct device *dev)
ste->bypass = false;
ste->valid = true;
- if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
+ if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS) {
+ ste->bypass = true;
+ } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
ste->s1_cfg = &smmu_domain->s1_cfg;
ste->s2_cfg = NULL;
arm_smmu_write_ctx_desc(smmu, ste->s1_cfg);
--
2.1.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/5] iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains
The ARM SMMU drivers provide a DOMAIN_ATTR_NESTING domain attribute,
which allows callers of the IOMMU API to request that the page table
for a domain is installed at stage-2, if supported by the hardware.
Since setting this attribute only makes sense for UNMANAGED domains,
this patch returns -ENODEV if the domain_{get,set}_attr operations are
called on other domain types.
Signed-off-by: Will Deacon
---
drivers/iommu/arm-smmu-v3.c | 6 ++
drivers/iommu/arm-smmu.c| 6 ++
2 files changed, 12 insertions(+)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4d6ec444a9d6..c254325b0c7a 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1839,6 +1839,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain
*domain,
{
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+ return -ENODEV;
+
switch (attr) {
case DOMAIN_ATTR_NESTING:
*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
@@ -1854,6 +1857,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain
*domain,
int ret = 0;
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+ return -ENODEV;
+
mutex_lock(&smmu_domain->init_mutex);
switch (attr) {
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index a60cded8a6ed..a328ffb75509 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1497,6 +1497,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain
*domain,
{
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+ return -ENODEV;
+
switch (attr) {
case DOMAIN_ATTR_NESTING:
*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
@@ -1512,6 +1515,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain
*domain,
int ret = 0;
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+ return -ENODEV;
+
mutex_lock(&smmu_domain->init_mutex);
switch (attr) {
--
2.1.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 5/5] iommu: Allow default domain type to be set on the kernel command line
The IOMMU core currently initialises the default domain for each group
to IOMMU_DOMAIN_DMA, under the assumption that devices will use
IOMMU-backed DMA ops by default. However, in some cases it is desirable
for the DMA ops to bypass the IOMMU for performance reasons, reserving
use of translation for subsystems such as VFIO that require it for
enforcing device isolation.
Rather than modify each IOMMU driver to provide different semantics for
DMA domains, instead we introduce a command line parameter that can be
used to change the type of the default domain. Passthrough can then be
specified using "iommu.default_domain=identity" on the kernel command
line.
Signed-off-by: Will Deacon
---
drivers/iommu/iommu.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index dbe7f653bb7c..69f7f1a75543 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -36,6 +36,7 @@
static struct kset *iommu_group_kset;
static DEFINE_IDA(iommu_group_ida);
+static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
struct iommu_callback_data {
const struct iommu_ops *ops;
@@ -86,6 +87,20 @@ static int __iommu_attach_group(struct iommu_domain *domain,
static void __iommu_detach_group(struct iommu_domain *domain,
struct iommu_group *group);
+static int __init iommu_set_def_domain_type(char *str)
+{
+ if (!str)
+ return -EINVAL;
+
+ if (!strcmp(str, "identity"))
+ iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
+ else if (!strcmp(str, "dma"))
+ iommu_def_domain_type = IOMMU_DOMAIN_DMA;
+
+ return 0;
+}
+early_param("iommu.default_domain", iommu_set_def_domain_type);
+
static ssize_t iommu_group_attr_show(struct kobject *kobj,
struct attribute *__attr, char *buf)
{
@@ -847,8 +862,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device
*dev)
* IOMMU driver.
*/
if (!group->default_domain) {
- group->default_domain = __iommu_domain_alloc(dev->bus,
-IOMMU_DOMAIN_DMA);
+ group->default_domain =
+ __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
if (!group->domain)
group->domain = group->default_domain;
}
--
2.1.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 10/18] iommu/arm-smmu: Implement reserved region get/put callbacks
On Thu, Jan 19, 2017 at 08:57:55PM +, Eric Auger wrote: > The get() populates the list with the MSI IOVA reserved window. > > At the moment an arbitray MSI IOVA window is set at 0x800 > of size 1MB. This will allow to report those info in iommu-group > sysfs. > > Signed-off-by: Eric Auger > Reviewed-by: Tomasz Nowicki > Tested-by: Tomasz Nowicki > Tested-by: Bharat Bhushan > > --- I acked this before: Acked-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 08/18] iommu/vt-d: Implement reserved region get/put callbacks
[adding David Woodhouse, since he maintains this driver]
Will
On Thu, Jan 19, 2017 at 08:57:53PM +, Eric Auger wrote:
> This patch registers the [FEE0_h - FEF0_000h] 1MB MSI
> range as a reserved region and RMRR regions as direct regions.
>
> This will allow to report those reserved regions in the
> iommu-group sysfs.
>
> Signed-off-by: Eric Auger
>
> ---
> v6 -> v7:
> - report RMRR regions as direct regions
> - Due to the usage of rcu_read_lock, the rmrr reserved region
> allocation is done on rmrr allocation.
> - use IOMMU_RESV_RESERVED
>
> RFCv2 -> RFCv3:
> - use get/put_resv_region callbacks.
>
> RFC v1 -> RFC v2:
> - fix intel_iommu_add_reserved_regions name
> - use IOAPIC_RANGE_START and IOAPIC_RANGE_END defines
> - return if the MSI region is already registered;
> ---
> drivers/iommu/intel-iommu.c | 92
> -
> 1 file changed, 74 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 8a18525..bce59a5 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -440,6 +440,7 @@ struct dmar_rmrr_unit {
> u64 end_address;/* reserved end address */
> struct dmar_dev_scope *devices; /* target devices */
> int devices_cnt;/* target device count */
> + struct iommu_resv_region *resv; /* reserved region handle */
> };
>
> struct dmar_atsr_unit {
> @@ -4246,27 +4247,40 @@ static inline void init_iommu_pm_ops(void) {}
> int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
> {
> struct acpi_dmar_reserved_memory *rmrr;
> + int prot = DMA_PTE_READ|DMA_PTE_WRITE;
> struct dmar_rmrr_unit *rmrru;
> + size_t length;
>
> rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
> if (!rmrru)
> - return -ENOMEM;
> + goto out;
>
> rmrru->hdr = header;
> rmrr = (struct acpi_dmar_reserved_memory *)header;
> rmrru->base_address = rmrr->base_address;
> rmrru->end_address = rmrr->end_address;
> +
> + length = rmrr->end_address - rmrr->base_address + 1;
> + rmrru->resv = iommu_alloc_resv_region(rmrr->base_address, length, prot,
> + IOMMU_RESV_DIRECT);
> + if (!rmrru->resv)
> + goto free_rmrru;
> +
> rmrru->devices = dmar_alloc_dev_scope((void *)(rmrr + 1),
> ((void *)rmrr) + rmrr->header.length,
> &rmrru->devices_cnt);
> - if (rmrru->devices_cnt && rmrru->devices == NULL) {
> - kfree(rmrru);
> - return -ENOMEM;
> - }
> + if (rmrru->devices_cnt && rmrru->devices == NULL)
> + goto free_all;
>
> list_add(&rmrru->list, &dmar_rmrr_units);
>
> return 0;
> +free_all:
> + kfree(rmrru->resv);
> +free_rmrru:
> + kfree(rmrru);
> +out:
> + return -ENOMEM;
> }
>
> static struct dmar_atsr_unit *dmar_find_atsr(struct acpi_dmar_atsr *atsr)
> @@ -4480,6 +4494,7 @@ static void intel_iommu_free_dmars(void)
> list_for_each_entry_safe(rmrru, rmrr_n, &dmar_rmrr_units, list) {
> list_del(&rmrru->list);
> dmar_free_dev_scope(&rmrru->devices, &rmrru->devices_cnt);
> + kfree(rmrru->resv);
> kfree(rmrru);
> }
>
> @@ -5203,6 +5218,45 @@ static void intel_iommu_remove_device(struct device
> *dev)
> iommu_device_unlink(iommu->iommu_dev, dev);
> }
>
> +static void intel_iommu_get_resv_regions(struct device *device,
> + struct list_head *head)
> +{
> + struct iommu_resv_region *reg;
> + struct dmar_rmrr_unit *rmrr;
> + struct device *i_dev;
> + int i;
> +
> + rcu_read_lock();
> + for_each_rmrr_units(rmrr) {
> + for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
> + i, i_dev) {
> + if (i_dev != device)
> + continue;
> +
> + list_add_tail(&rmrr->resv->list, head);
> + }
> + }
> + rcu_read_unlock();
> +
> + reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
> + IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
> + 0, IOMMU_RESV_RESERVED);
> + if (!reg)
> + return;
> + list_add_tail(®->list, head);
> +}
> +
> +static void intel_iommu_put_resv_regions(struct device *dev,
> + struct list_head *head)
> +{
> + struct iommu_resv_region *entry, *next;
> +
> + list_for_each_entry_safe(entry, next, head, list) {
> + if (entry->type == IOMMU_RESV_RESERVED)
> + kfree(entry);
> + }
> +}
> +
> #ifdef CONFIG_INTEL_IOMMU_SVM
> #define MAX_NR_PASID_BITS (20)
> static inline unsigned long intel_iommu_get_pts(struct intel_iommu *io
Re: [PATCH v2 2/2] iommu/dma: Implement PCI allocation optimisation
On Mon, Jan 16, 2017 at 01:24:55PM +, Robin Murphy wrote: > Whilst PCI devices may have 64-bit DMA masks, they still benefit from > using 32-bit addresses wherever possible in order to avoid DAC (PCI) or > longer address packets (PCIe), which may incur a performance overhead. > Implement the same optimisation as other allocators by trying to get a > 32-bit address first, only falling back to the full mask if that fails. > > Signed-off-by: Robin Murphy > --- > drivers/iommu/dma-iommu.c | 21 +++-- > 1 file changed, 15 insertions(+), 6 deletions(-) Looks good to me, and has the added benefit of getting PCI ethernet working on Juno when using the SMMU. Unintended side-effect, but: Acked-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/2] iommu/dma: Stop getting dma_32bit_pfn wrong
On Mon, Jan 16, 2017 at 01:24:54PM +, Robin Murphy wrote: > iommu_dma_init_domain() was originally written under the misconception > that dma_32bit_pfn represented some sort of size limit for IOVA domains. > Since the truth is almost the exact opposite of that, rework the logic > and comments to reflect its real purpose of optimising lookups when > allocating from a subset of the available 64-bit space. > > Signed-off-by: Robin Murphy > --- > > Sending this as a v2 since both patches have been seen before, and #1 is > ever so slightly tweaked. #2 applies on top of Eric's MSI series, since > that seems ready to go now - there is a trivial merge conflict otherwise > around the extra argument in the __alloc_iova() call. > > Robin. > > drivers/iommu/dma-iommu.c | 23 ++- > 1 file changed, 18 insertions(+), 5 deletions(-) Tested-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/5] iommu: Allow default domain type to be set on the kernel command line
On Thu, Jan 26, 2017 at 06:15:55PM +0100, Joerg Roedel wrote: > On Thu, Jan 19, 2017 at 06:19:15PM +0000, Will Deacon wrote: > > Rather than modify each IOMMU driver to provide different semantics for > > DMA domains, instead we introduce a command line parameter that can be > > used to change the type of the default domain. Passthrough can then be > > specified using "iommu.default_domain=identity" on the kernel command > > line. > > I like the general idea of this, but the above is a terrible name for a > kernel commandline-parameter. The x86 iommus support iommu=pt which is > pretty much the same as this patch does. Happy to bikeshed the name ;) > How about something like "iommu.passthrough=0/1"? And please add the > parameter to the kernel documentation too. Sure, if you think that the identity domain is the only thing we'll ever want to set (so far, it's the only thing people have asked me for). Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains
On Thu, Jan 26, 2017 at 06:03:30PM +0100, Joerg Roedel wrote:
> On Thu, Jan 19, 2017 at 06:19:11PM +0000, Will Deacon wrote:
> > The ARM SMMU drivers provide a DOMAIN_ATTR_NESTING domain attribute,
> > which allows callers of the IOMMU API to request that the page table
> > for a domain is installed at stage-2, if supported by the hardware.
> >
> > Since setting this attribute only makes sense for UNMANAGED domains,
> > this patch returns -ENODEV if the domain_{get,set}_attr operations are
> > called on other domain types.
>
> Isn't -EINVAL more suitable here? In the end the domain passed in is
> invalid because it does not support attributes, no?
Sure, I can change that.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/5] arm64: dma-mapping: Only swizzle DMA ops for IOMMU_DOMAIN_DMA
On Thu, Jan 19, 2017 at 07:00:25PM +, Robin Murphy wrote: > On 19/01/17 18:19, Will Deacon wrote: > > The arm64 DMA-mapping implementation sets the DMA ops to the IOMMU DMA > > ops if we detect that an IOMMU is present for the master and the DMA > > ranges are valid. > > > > In the case when the IOMMU domain for the device is not of type > > IOMMU_DOMAIN_DMA, then we have no business swizzling the ops, since > > we're not in control of the underlying address space. This patch leaves > > the DMA ops alone for masters attached to non-DMA IOMMU domains. > > In fact, I don't think there would be any harm in taking this one > through arm64 straight away. The DMA ops can't be expected to work > successfully on any old domain, so it's a reasonable sanity check > regardless. > > Reviewed-by: Robin Murphy Good point; I'll queue this one for 4.11 via arm64. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[GIT PULL] iommu/arm-smmu: Updates for 4.11
Hi Joerg,
Please pull these arm-smmu updates for 4.11. Not much this time around:
16-bit SID support on SMMUv2 and a stream table optimisation on SMMUv3.
There's also a trivial cleanup to of_iommu_{set/get}_ops() [they are
removed] which we promised to make after the IORT stuff went in last
time around.
Thanks,
Will
--->8
The following changes since commit a121103c922847ba5010819a3f250f1f7fc84ab8:
Linux 4.10-rc3 (2017-01-08 14:18:17 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git
for-joerg/arm-smmu/updates
for you to fetch changes up to 3677a649a751c8f061ba379a98c369473ccac980:
iommu/arm-smmu: Fix for ThunderX erratum #27704 (2017-01-26 18:16:58 +)
Aleksey Makarov (1):
iommu/arm-smmu: Support for Extended Stream ID (16 bit)
Lorenzo Pieralisi (1):
iommu: Drop the of_iommu_{set/get}_ops() interface
Nate Watterson (2):
iommu/arm-smmu-v3: Clear prior settings when updating STEs
iommu/arm-smmu-v3: limit use of 2-level stream tables
Tomasz Nowicki (1):
iommu/arm-smmu: Fix for ThunderX erratum #27704
drivers/iommu/arm-smmu-v3.c | 31 ---
drivers/iommu/arm-smmu.c | 72 +++-
drivers/iommu/exynos-iommu.c | 2 +-
drivers/iommu/msm_iommu.c| 2 +-
drivers/iommu/mtk_iommu.c| 2 +-
drivers/iommu/of_iommu.c | 4 +--
include/linux/of_iommu.h | 11 ---
7 files changed, 68 insertions(+), 56 deletions(-)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[GIT PULL] iommu: IOMMU_PRIV support for 4.11
Hi Joerg, Please pull the following IOMMU changes for 4.11. These patches from Sricharan add support for "privileged" IOMMU mappings, which are useful with master devices that support transactions at different privilege levels and want to control the permissions independently. Cheers, Will --->8 The following changes since commit a121103c922847ba5010819a3f250f1f7fc84ab8: Linux 4.10-rc3 (2017-01-08 14:18:17 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git iommu/iommu-priv for you to fetch changes up to fa8c57db44068a77903d9688382b461a836eee68: iommu: Better document the IOMMU_PRIV flag (2017-01-27 13:49:35 +) Jeremy Gebben (1): iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag Mitchel Humpherys (4): iommu: add IOMMU_PRIV attribute common: DMA-mapping: add DMA_ATTR_PRIVILEGED attribute arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED dmaengine: pl330: Make sure microcode is privileged Robin Murphy (3): iommu/io-pgtable-arm-v7s: Add support for the IOMMU_PRIV flag Revert "iommu/arm-smmu: Set PRIVCFG in stage 1 STEs" iommu: Better document the IOMMU_PRIV flag Sricharan R (2): arm/dma-mapping: Implement DMA_ATTR_PRIVILEGED iommu/arm-smmu: Set privileged attribute to 'default' instead of 'unprivileged' Documentation/DMA-attributes.txt | 10 +++ arch/arm/mm/dma-mapping.c | 60 +++--- arch/arm64/mm/dma-mapping.c| 6 ++-- drivers/dma/pl330.c| 5 ++-- drivers/iommu/arm-smmu-v3.c| 7 + drivers/iommu/arm-smmu.c | 2 +- drivers/iommu/dma-iommu.c | 12 ++-- drivers/iommu/io-pgtable-arm-v7s.c | 6 +++- drivers/iommu/io-pgtable-arm.c | 5 +++- include/linux/dma-iommu.h | 3 +- include/linux/dma-mapping.h| 7 + include/linux/iommu.h | 10 +++ 12 files changed, 85 insertions(+), 48 deletions(-) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[GIT PULL] iommu: KVM PCIe/MSI passthrough on ARM/ARM64 for 4.11
Hi Joerg, Please pull Eric's guest-MSI series for 4.11. This has been through considerable review and associated rework (including a session at LPC), but it has stabilised at last and we all seem to be happy with it. Eric's done a great job of respinning these and remaining patient while we pulled him in a bunch of different directions. With these patches applied, it's possible for us to pass PCI devices through to KVM guests on arm64 using VFIO and have them signal interrupts using MSIs targetting the ITS via the SMMU. Acronym soup, sure, but it's much better than legacy wired irqs! Cheers, Will --->8 The following changes since commit a121103c922847ba5010819a3f250f1f7fc84ab8: Linux 4.10-rc3 (2017-01-08 14:18:17 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git iommu/guest-msi for you to fetch changes up to 5018c8d5ef0c172592eb98cf10e253d47b544ba8: iommu/arm-smmu: Do not advertise IOMMU_CAP_INTR_REMAP anymore (2017-01-23 15:00:47 +) Eric Auger (17): iommu: Rename iommu_dm_regions into iommu_resv_regions iommu: Add a new type field in iommu_resv_region iommu: iommu_alloc_resv_region iommu: Only map direct mapped regions iommu: iommu_get_group_resv_regions iommu: Implement reserved_regions iommu-group sysfs file iommu/vt-d: Implement reserved region get/put callbacks iommu/amd: Declare MSI and HT regions as reserved IOVA regions iommu/arm-smmu: Implement reserved region get/put callbacks iommu/arm-smmu-v3: Implement reserved region get/put callbacks irqdomain: Add irq domain MSI and MSI_REMAP flags genirq/msi: Set IRQ_DOMAIN_FLAG_MSI on MSI domain creation irqdomain: irq_domain_check_msi_remap irqchip/gicv3-its: Sets IRQ_DOMAIN_FLAG_MSI_REMAP vfio/type1: Allow transparent MSI IOVA allocation vfio/type1: Check MSI remapping at irq domain level iommu/arm-smmu: Do not advertise IOMMU_CAP_INTR_REMAP anymore Robin Murphy (1): iommu/dma: Allow MSI-only cookies .../ABI/testing/sysfs-kernel-iommu_groups | 12 ++ drivers/iommu/amd_iommu.c | 54 --- drivers/iommu/arm-smmu-v3.c| 30 +++- drivers/iommu/arm-smmu.c | 30 +++- drivers/iommu/dma-iommu.c | 119 +++--- drivers/iommu/intel-iommu.c| 92 --- drivers/iommu/iommu.c | 177 +++-- drivers/irqchip/irq-gic-v3-its.c | 1 + drivers/vfio/vfio_iommu_type1.c| 37 - include/linux/dma-iommu.h | 6 + include/linux/iommu.h | 46 -- include/linux/irqdomain.h | 36 + kernel/irq/irqdomain.c | 39 + kernel/irq/msi.c | 4 +- 14 files changed, 590 insertions(+), 93 deletions(-) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V7 08/11] drivers: acpi: Handle IOMMU lookup failure with deferred probing or error
On Mon, Jan 30, 2017 at 09:33:50AM -0500, Sinan Kaya wrote: > On 1/30/2017 9:23 AM, Nate Watterson wrote: > > On 2017-01-30 08:59, Sinan Kaya wrote: > >> On 1/30/2017 7:22 AM, Robin Murphy wrote: > >>> On 29/01/17 17:53, Sinan Kaya wrote: > On 1/24/2017 7:37 AM, Lorenzo Pieralisi wrote: > > [+hanjun, tomasz, sinan] > > > > It is quite a key patchset, I would be glad if they can test on their > > respective platforms with IORT. > > > > Tested on top of 4.10-rc5. > > 1.Platform Hidma device passed dmatest > 2.Seeing some USB stalls on a platform USB device. > 3.PCIe NVME drive probed and worked fine with MSI interrupts after > boot. > 4. NVMe driver didn't probe following a hotplug insertion and > received an > SMMU error event during the insertion. > >>> > >>> What was the SMMU error - a translation/permission fault (implying the > >>> wrong DMA ops) or a bad STE fault (implying we totally failed to tell > >>> the SMMU about the device at all)? > >>> > >> > >> root@ubuntu:/sys/bus/pci/slots/4# echo 0 > power > >> > >> [__204.698522]_iommu:_Removing_device_0003:01:00.0_from_group_0 > >> [ 204.708704] pciehp 0003:00:00.0:pcie004: Slot(4): Link Down > >> [ 204.708723] pciehp 0003:00:00.0:pcie004: Slot(4): Link Down event > >> ignored; already powering off > >> > >> root@ubuntu:/sys/bus/pci/slots/4# > >> > >> [__254.820440]_iommu:_Adding_device_0003:01:00.0_to_group_8 > >> [ 254.820599] nvme nvme0: pci function 0003:01:00.0 > >> [ 254.820621] nvme 0003:01:00.0: enabling device ( -> 0002) > >> [ 261.948558] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x0a received: > >> [ 261.948561] arm-smmu-v3 arm-smmu-v3.0.auto: 0x010a > >> [ 261.948563] arm-smmu-v3 arm-smmu-v3.0.auto: 0x > >> [ 261.948564] arm-smmu-v3 arm-smmu-v3.0.auto: 0x > >> [ 261.948566] arm-smmu-v3 arm-smmu-v3.0.auto: 0x > > > Looks like C_BAD_CD. Can you please try with: > > iommu/arm-smmu-v3: Clear prior settings when updating STEs > > This resolved the issue. Can we pull Nate's patch to 4.10 so that I don't see > this issue again. I already sent the pull request to Joerg for 4.11. Do you see this problem without Sricharan's patches (i.e. vanilla mainline)? If so, we'll need to send the patch to stable after -rc1. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/5] Implement SMMU passthrough using the default domain
On Thu, Feb 02, 2017 at 10:02:50AM -0500, Rob Clark wrote: > On Thu, Jan 26, 2017 at 12:18 PM, Joerg Roedel wrote: > > On Tue, Jan 24, 2017 at 08:42:23PM +0530, Sricharan wrote: > >> Thanks for this series. We had a case with the GPU. > >> The GPU's iommu was setup by kernel and the GPU > >> also does dynamic updates for on-the-fly switching between > >> process pagetables. GPU driver was not using DMA domain and > >> the GPU's firmware was always expecting to run out of contextbank > >> '0' (although not correct) , which was not the case after the DMA domain > >> was made default as '0' was getting allocated for DMA domain and > >> there were concerns about reusing the DMA domain as well. > >> Now with this series, looks there is an way out of that that can be tried. > >> > >> So should the default domain not be per device specific selectable ? > > > > Note that iommu-drivers can request direct-mapping for any given device > > on its initializtion. This is used on x86 for devices that need a 1-1 > > mapping for some reason. > > > > Also device drivers can use the iommu-api and assign their own domain to > > a device, which allows them to manage the dma address space on their > > own. > > Part of the problem is that dev->archdata.dma_ops gets wired up to > iommu_dma_ops. Which isn't so bad on it's own, except that cache ops > are not exposed to drivers, forcing us to use dma-mapping API > (dma_map_sg, etc) for cache operations. > > Possibly we should just expose cache op's to drivers bypass this abuse > of dma-mapping. > > btw, Will, we definitely want this to *not* rely on kcmdline for the > gpu with it's own private iommu case.. I still need to understand the unmanaged domain case, but I don't really see why that's related to this series to be honest. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/5] Implement SMMU passthrough using the default domain
On Thu, Feb 02, 2017 at 09:15:19PM +0530, Sricharan wrote: > Hi Rob, > > >-Original Message- > >From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org] > >On Behalf Of Rob Clark > >Sent: Thursday, February 02, 2017 8:33 PM > >To: Joerg Roedel > >Cc: Will Deacon ; iommu@lists.linux-foundation.org; > >Sricharan ; linux-arm- > >ker...@lists.infradead.org > >Subject: Re: [PATCH 0/5] Implement SMMU passthrough using the default domain > > > >On Thu, Jan 26, 2017 at 12:18 PM, Joerg Roedel wrote: > >> On Tue, Jan 24, 2017 at 08:42:23PM +0530, Sricharan wrote: > >>> Thanks for this series. We had a case with the GPU. > >>> The GPU's iommu was setup by kernel and the GPU > >>> also does dynamic updates for on-the-fly switching between > >>> process pagetables. GPU driver was not using DMA domain and > >>> the GPU's firmware was always expecting to run out of contextbank > >>> '0' (although not correct) , which was not the case after the DMA domain > >>> was made default as '0' was getting allocated for DMA domain and > >>> there were concerns about reusing the DMA domain as well. > >>> Now with this series, looks there is an way out of that that can be tried. > >>> > >>> So should the default domain not be per device specific selectable ? > >> > >> Note that iommu-drivers can request direct-mapping for any given device > >> on its initializtion. This is used on x86 for devices that need a 1-1 > >> mapping for some reason. > >> > >> Also device drivers can use the iommu-api and assign their own domain to > >> a device, which allows them to manage the dma address space on their > >> own. > > > >Part of the problem is that dev->archdata.dma_ops gets wired up to > >iommu_dma_ops. Which isn't so bad on it's own, except that cache ops > >are not exposed to drivers, forcing us to use dma-mapping API > >(dma_map_sg, etc) for cache operations. > > > >Possibly we should just expose cache op's to drivers bypass this abuse > >of dma-mapping. > > > [1], with this, when the default domain in not DOMAIN_DMA, then > dev->archdata.dma_ops is not set to iommu_dma_ops , instead remains > to be swiotlb_ops. Is that not correct for gpu's unmanaged domain case ? > > https://www.spinics.net/lists/arm-kernel/msg556209.html > > >btw, Will, we definitely want this to *not* rely on kcmdline for the > >gpu with it's own private iommu case.. > > Ya, that changes behavior for all devices and some might want > DMA_DOMAIN and some UNMANAGED. My patch changes the *default* domain. When would this ever be UNMANAGED? If you're using an UNMANAGED domain, I think you need to take care of the DMA ops yourself. There are likely missing functions to do that, but they should be added in a separate series. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v1] iommu/io-pgtable-arm: Check for leaf entry right after finding it
On Mon, Feb 13, 2017 at 01:07:02PM +0200, Oleksandr Tyshchenko wrote: > Any comments? Looks fine to me, but I don't think it's urgent and I already sent my SMMU pull for 4.11. I'll send this as a fix after the merge window. I suspect we need something similar for io-pgtable-arm-v7s.c, too. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v1] iommu/io-pgtable-arm: Check for leaf entry right after finding it
On Mon, Feb 13, 2017 at 01:50:29PM +0200, Oleksandr Tyshchenko wrote: > On Mon, Feb 13, 2017 at 1:27 PM, Will Deacon wrote: > > On Mon, Feb 13, 2017 at 01:07:02PM +0200, Oleksandr Tyshchenko wrote: > >> Any comments? > > > > Looks fine to me, but I don't think it's urgent and I already sent my > > SMMU pull for 4.11. I'll send this as a fix after the merge window. > OK. Thank you. > > > > > I suspect we need something similar for io-pgtable-arm-v7s.c, too. > Agree. On the whole I will be able to make similar patch for arm-v7s, > but I won't be 100% sure > since I don't have any boards where arm-v7s compatible IOMMU installed. > > Shall I make patch for arm-v7s too? Yes, please. Robin seems to enjoy using short-descriptor, so he might give it a spin for you if you ask nicely. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: RFC on No ACS Support and SMMUv3 Support
On Mon, Feb 13, 2017 at 08:54:04PM -0500, Sinan Kaya wrote: > On 2/13/2017 8:46 PM, Alex Williamson wrote: > >> My first goal is to support virtual function passthrough for device's that > >> are directly > >> connected. This will be possible with the quirk I proposed and it will be > >> the most > >> secure solution. It can certainly be generalized for other systems. > > Why is this anything more than a quirk for the affected PCIe root port > > vendor:device IDs and use of pci_device_group() to evaluate the rest of > > the topology, as appears is already done? Clearly a blanket exception > > for the platform wouldn't necessarily be correct if a user could plugin > > a device that adds a PCIe switch. > > I was going to go this direction first. I wanted to check with everybody to > see > if there are other/better alternatives possible via either changing > pci_device_group or changing the smmuv3 driver. Just to echo what Alex has been saying, I really don't think we should support this type of system by quirking the topology code in the SMMU driver. The SMMU isn't at fault here; the problems are all upstream of that. Legitimising non-ACS machines in the SMMU driver gives little incentive for people to build systems correctly and undermines the security guarantees that the SMMU (and VFIO) are trying to provide. I appreciate that I/O virtualisation on arm64 has been a learning curve for everybody involved, but that's not an excuse for moving the goalposts when it comes to device isolation. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v1] iommu/io-pgtable-arm-v7s: Check for leaf entry right after finding it
On Tue, Feb 21, 2017 at 03:31:26PM +0200, Oleksandr Tyshchenko wrote:
> On Tue, Feb 21, 2017 at 2:00 PM, Robin Murphy wrote:
> > Would it not be more logical (and simpler) to just check that the thing
> > we dereference is valid to dereference when we dereference it? i.e.:
> >
> > -8<-
> > diff --git a/drivers/iommu/io-pgtable-arm-v7s.c
> > b/drivers/iommu/io-pgtable-arm-v7s.c
> > index 0769276c0537..f3112f9ff494 100644
> > --- a/drivers/iommu/io-pgtable-arm-v7s.c
> > +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> > @@ -418,8 +418,10 @@ static int __arm_v7s_map(struct arm_v7s_io_pgtable
> > *data, unsigned long iova,
> > pte |= ARM_V7S_ATTR_NS_TABLE;
> >
> > __arm_v7s_set_pte(ptep, pte, 1, cfg);
> > - } else {
> > + } else if (ARM_V7S_PTE_IS_TABLE(pte, lvl)) {
> > cptep = iopte_deref(pte, lvl);
> > + } else {
> > + return -EEXIST;
> > }
> >
> > /* Rinse, repeat */
> > ->8-
>
> Agree. Sounds reasonable.
>
> >
> > I think the equivalent could be done in LPAE as well.
>
> OK.
>
> I will resend both modified patches without RFC prefix, right?
Sounds good to me.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 4/5] iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains
In preparation for allowing the default domain type to be overridden,
this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the
ARM SMMUv3 driver.
An identity domain is created by placing the corresponding stream table
entries into "bypass" mode, which allows transactions to flow through
the SMMU without any translation.
Signed-off-by: Will Deacon
---
drivers/iommu/arm-smmu-v3.c | 58 +
1 file changed, 37 insertions(+), 21 deletions(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index e18dbcd26f66..75fa4809f49e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -554,9 +554,14 @@ struct arm_smmu_s2_cfg {
};
struct arm_smmu_strtab_ent {
- boolvalid;
-
- boolbypass; /* Overrides s1/s2 config */
+ /*
+* An STE is "assigned" if the master emitting the corresponding SID
+* is attached to a domain. The behaviour of an unassigned STE is
+* determined by the disable_bypass parameter, whereas an assigned
+* STE behaves according to s1_cfg/s2_cfg, which themselves are
+* configured according to the domain type.
+*/
+ boolassigned;
struct arm_smmu_s1_cfg *s1_cfg;
struct arm_smmu_s2_cfg *s2_cfg;
};
@@ -632,6 +637,7 @@ enum arm_smmu_domain_stage {
ARM_SMMU_DOMAIN_S1 = 0,
ARM_SMMU_DOMAIN_S2,
ARM_SMMU_DOMAIN_NESTED,
+ ARM_SMMU_DOMAIN_BYPASS,
};
struct arm_smmu_domain {
@@ -1005,9 +1011,9 @@ static void arm_smmu_write_strtab_ent(struct
arm_smmu_device *smmu, u32 sid,
* This is hideously complicated, but we only really care about
* three cases at the moment:
*
-* 1. Invalid (all zero) -> bypass (init)
-* 2. Bypass -> translation (attach)
-* 3. Translation -> bypass (detach)
+* 1. Invalid (all zero) -> bypass/fault (init)
+* 2. Bypass/fault -> translation/bypass (attach)
+* 3. Translation/bypass -> bypass/fault (detach)
*
* Given that we can't update the STE atomically and the SMMU
* doesn't read the thing in a defined order, that leaves us
@@ -1046,11 +1052,15 @@ static void arm_smmu_write_strtab_ent(struct
arm_smmu_device *smmu, u32 sid,
}
/* Nuke the existing STE_0 value, as we're going to rewrite it */
- val = ste->valid ? STRTAB_STE_0_V : 0;
+ val = STRTAB_STE_0_V;
+
+ /* Bypass/fault */
+ if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) {
+ if (!ste->assigned && disable_bypass)
+ val |= STRTAB_STE_0_CFG_ABORT;
+ else
+ val |= STRTAB_STE_0_CFG_BYPASS;
- if (ste->bypass) {
- val |= disable_bypass ? STRTAB_STE_0_CFG_ABORT
- : STRTAB_STE_0_CFG_BYPASS;
dst[0] = cpu_to_le64(val);
dst[1] = cpu_to_le64(STRTAB_STE_1_SHCFG_INCOMING
<< STRTAB_STE_1_SHCFG_SHIFT);
@@ -,10 +1121,7 @@ static void arm_smmu_write_strtab_ent(struct
arm_smmu_device *smmu, u32 sid,
static void arm_smmu_init_bypass_stes(u64 *strtab, unsigned int nent)
{
unsigned int i;
- struct arm_smmu_strtab_ent ste = {
- .valid = true,
- .bypass = true,
- };
+ struct arm_smmu_strtab_ent ste = { .assigned = false };
for (i = 0; i < nent; ++i) {
arm_smmu_write_strtab_ent(NULL, -1, strtab, &ste);
@@ -1378,7 +1385,9 @@ static struct iommu_domain
*arm_smmu_domain_alloc(unsigned type)
{
struct arm_smmu_domain *smmu_domain;
- if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+ if (type != IOMMU_DOMAIN_UNMANAGED &&
+ type != IOMMU_DOMAIN_DMA &&
+ type != IOMMU_DOMAIN_IDENTITY)
return NULL;
/*
@@ -1509,6 +1518,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain
*domain)
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct arm_smmu_device *smmu = smmu_domain->smmu;
+ if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+ smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
+ return 0;
+ }
+
/* Restrict the stage to what we can actually support */
if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
@@ -1597,7 +1611,7 @@ static void arm_smmu_detach_dev(struct device *dev)
{
struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv;
- master->ste.bypass = true;
+ master->ste.assigned = false;
arm_smmu_install_ste_for_dev(dev->
[PATCH v2 0/5] Implement SMMU passthrough using the default domain
Hi all, This is version two of the patches previously posted here: http://lists.infradead.org/pipermail/linux-arm-kernel/2017-January/481275.html I've addressed the review feedback I receive there, but the main change is that the SMMUv3 driver now handles passthrough domains independently of the disable_bypass parameter. All feedback welcome, Will --->8 Will Deacon (5): iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains iommu: Allow default domain type to be set on the kernel command line Documentation/admin-guide/kernel-parameters.txt | 6 ++ drivers/iommu/arm-smmu-v3.c | 76 +++-- drivers/iommu/arm-smmu.c| 26 - drivers/iommu/iommu.c | 17 +- 4 files changed, 90 insertions(+), 35 deletions(-) -- 2.1.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 3/5] iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void
arm_smmu_install_ste_for_dev cannot fail and always returns 0, however
the fact that it returns int means that callers end up implementing
redundant error handling code which complicates STE tracking and is
never executed.
This patch changes the return type of arm_smmu_install_ste_for_dev
to avoid, to make it explicit that it cannot fail.
Signed-off-by: Will Deacon
---
drivers/iommu/arm-smmu-v3.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 3d38e682071a..e18dbcd26f66 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1579,7 +1579,7 @@ static __le64 *arm_smmu_get_step_for_sid(struct
arm_smmu_device *smmu, u32 sid)
return step;
}
-static int arm_smmu_install_ste_for_dev(struct iommu_fwspec *fwspec)
+static void arm_smmu_install_ste_for_dev(struct iommu_fwspec *fwspec)
{
int i;
struct arm_smmu_master_data *master = fwspec->iommu_priv;
@@ -1591,8 +1591,6 @@ static int arm_smmu_install_ste_for_dev(struct
iommu_fwspec *fwspec)
arm_smmu_write_strtab_ent(smmu, sid, step, &master->ste);
}
-
- return 0;
}
static void arm_smmu_detach_dev(struct device *dev)
@@ -1600,8 +1598,7 @@ static void arm_smmu_detach_dev(struct device *dev)
struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv;
master->ste.bypass = true;
- if (arm_smmu_install_ste_for_dev(dev->iommu_fwspec) < 0)
- dev_warn(dev, "failed to install bypass STE\n");
+ arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
}
static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
@@ -1653,10 +1650,7 @@ static int arm_smmu_attach_dev(struct iommu_domain
*domain, struct device *dev)
ste->s2_cfg = &smmu_domain->s2_cfg;
}
- ret = arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
- if (ret < 0)
- ste->valid = false;
-
+ arm_smmu_install_ste_for_dev(dev->iommu_fwspec);
out_unlock:
mutex_unlock(&smmu_domain->init_mutex);
return ret;
--
2.1.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 5/5] iommu: Allow default domain type to be set on the kernel command line
The IOMMU core currently initialises the default domain for each group
to IOMMU_DOMAIN_DMA, under the assumption that devices will use
IOMMU-backed DMA ops by default. However, in some cases it is desirable
for the DMA ops to bypass the IOMMU for performance reasons, reserving
use of translation for subsystems such as VFIO that require it for
enforcing device isolation.
Rather than modify each IOMMU driver to provide different semantics for
DMA domains, instead we introduce a command line parameter that can be
used to change the type of the default domain. Passthrough can then be
specified using "iommu.passthrough=1" on the kernel command line.
Signed-off-by: Will Deacon
---
Documentation/admin-guide/kernel-parameters.txt | 6 ++
drivers/iommu/iommu.c | 17 +++--
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 986e44387dad..243895f6872e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1635,6 +1635,12 @@
nobypass[PPC/POWERNV]
Disable IOMMU bypass, using IOMMU for PCI devices.
+ iommu.passthrough=
+ [ARM64] Configure DMA to bypass the IOMMU by default.
+ Format: { "0" | "1" }
+ 0 - Use IOMMU translation for DMA.
+ 1 - Bypass the IOMMU for DMA.
+ unset - Use IOMMU translation for DMA.
io7=[HW] IO7 for Marvel based alpha systems
See comment before marvel_specify_io7 in
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8ea14f41a979..42a842e3f95f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -36,6 +36,7 @@
static struct kset *iommu_group_kset;
static DEFINE_IDA(iommu_group_ida);
+static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
struct iommu_callback_data {
const struct iommu_ops *ops;
@@ -111,6 +112,18 @@ static int __iommu_attach_group(struct iommu_domain
*domain,
static void __iommu_detach_group(struct iommu_domain *domain,
struct iommu_group *group);
+static int __init iommu_set_def_domain_type(char *str)
+{
+ bool pt;
+
+ if (!str || strtobool(str, &pt))
+ return -EINVAL;
+
+ iommu_def_domain_type = pt ? IOMMU_DOMAIN_IDENTITY : IOMMU_DOMAIN_DMA;
+ return 0;
+}
+early_param("iommu.passthrough", iommu_set_def_domain_type);
+
static ssize_t iommu_group_attr_show(struct kobject *kobj,
struct attribute *__attr, char *buf)
{
@@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device
*dev)
* IOMMU driver.
*/
if (!group->default_domain) {
- group->default_domain = __iommu_domain_alloc(dev->bus,
-IOMMU_DOMAIN_DMA);
+ group->default_domain =
+ __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
if (!group->domain)
group->domain = group->default_domain;
}
--
2.1.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 1/5] iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains
The ARM SMMU drivers provide a DOMAIN_ATTR_NESTING domain attribute,
which allows callers of the IOMMU API to request that the page table
for a domain is installed at stage-2, if supported by the hardware.
Since setting this attribute only makes sense for UNMANAGED domains,
this patch returns -ENODEV if the domain_{get,set}_attr operations are
called on other domain types.
Signed-off-by: Will Deacon
---
drivers/iommu/arm-smmu-v3.c | 6 ++
drivers/iommu/arm-smmu.c| 6 ++
2 files changed, 12 insertions(+)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 5806a6acc94e..3d38e682071a 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1837,6 +1837,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain
*domain,
{
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+ return -EINVAL;
+
switch (attr) {
case DOMAIN_ATTR_NESTING:
*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
@@ -1852,6 +1855,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain
*domain,
int ret = 0;
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+ return -EINVAL;
+
mutex_lock(&smmu_domain->init_mutex);
switch (attr) {
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index abf6496843a6..6426819348be 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1549,6 +1549,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain
*domain,
{
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+ return -EINVAL;
+
switch (attr) {
case DOMAIN_ATTR_NESTING:
*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
@@ -1564,6 +1567,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain
*domain,
int ret = 0;
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+ return -EINVAL;
+
mutex_lock(&smmu_domain->init_mutex);
switch (attr) {
--
2.1.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 2/5] iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains
In preparation for allowing the default domain type to be overridden,
this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the
ARM SMMU driver.
An identity domain is created by placing the corresponding S2CR
registers into "bypass" mode, which allows transactions to flow through
the SMMU without any translation.
Reviewed-by: Robin Murphy
Signed-off-by: Will Deacon
---
drivers/iommu/arm-smmu.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 6426819348be..04530e968132 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -416,6 +416,7 @@ enum arm_smmu_domain_stage {
ARM_SMMU_DOMAIN_S1 = 0,
ARM_SMMU_DOMAIN_S2,
ARM_SMMU_DOMAIN_NESTED,
+ ARM_SMMU_DOMAIN_BYPASS,
};
struct arm_smmu_domain {
@@ -838,6 +839,12 @@ static int arm_smmu_init_domain_context(struct
iommu_domain *domain,
if (smmu_domain->smmu)
goto out_unlock;
+ if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+ smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
+ smmu_domain->smmu = smmu;
+ goto out_unlock;
+ }
+
/*
* Mapping the requested stage onto what we support is surprisingly
* complicated, mainly because the spec allows S1+S2 SMMUs without
@@ -998,7 +1005,7 @@ static void arm_smmu_destroy_domain_context(struct
iommu_domain *domain)
void __iomem *cb_base;
int irq;
- if (!smmu)
+ if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
return;
/*
@@ -1021,7 +1028,9 @@ static struct iommu_domain
*arm_smmu_domain_alloc(unsigned type)
{
struct arm_smmu_domain *smmu_domain;
- if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+ if (type != IOMMU_DOMAIN_UNMANAGED &&
+ type != IOMMU_DOMAIN_DMA &&
+ type != IOMMU_DOMAIN_IDENTITY)
return NULL;
/*
* Allocate the domain and initialise some of its data structures.
@@ -1250,10 +1259,15 @@ static int arm_smmu_domain_add_master(struct
arm_smmu_domain *smmu_domain,
{
struct arm_smmu_device *smmu = smmu_domain->smmu;
struct arm_smmu_s2cr *s2cr = smmu->s2crs;
- enum arm_smmu_s2cr_type type = S2CR_TYPE_TRANS;
u8 cbndx = smmu_domain->cfg.cbndx;
+ enum arm_smmu_s2cr_type type;
int i, idx;
+ if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS)
+ type = S2CR_TYPE_BYPASS;
+ else
+ type = S2CR_TYPE_TRANS;
+
for_each_cfg_sme(fwspec, i, idx) {
if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
continue;
--
2.1.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/5] Implement SMMU passthrough using the default domain
Hi Joerg, On Tue, Mar 21, 2017 at 04:46:24PM +0100, Joerg Roedel wrote: > On Fri, Mar 10, 2017 at 08:49:31PM +0000, Will Deacon wrote: > > Will Deacon (5): > > iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains > > iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains > > iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void > > iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY > > domains > > iommu: Allow default domain type to be set on the kernel command line > > > > Documentation/admin-guide/kernel-parameters.txt | 6 ++ > > drivers/iommu/arm-smmu-v3.c | 76 > > +++-- > > drivers/iommu/arm-smmu.c| 26 - > > drivers/iommu/iommu.c | 17 +- > > 4 files changed, 90 insertions(+), 35 deletions(-) > > Besides my one comment on the last patch this series looks good to me. > Do you plan to include it (with the fall-back) into your pull-request? Yes, that would certainly be easiest for me, given the changes to the SMMU drivers. However, if you prefer it separately then I can do that too. Cheers, Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 4/5] iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains
Hi Robin,
On Thu, Mar 16, 2017 at 06:19:48PM +, Robin Murphy wrote:
> On 16/03/17 16:24, Nate Watterson wrote:
> > On 2017-03-10 15:49, Will Deacon wrote:
> >> In preparation for allowing the default domain type to be overridden,
> >> this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the
> >> ARM SMMUv3 driver.
> >>
> >> An identity domain is created by placing the corresponding stream table
> >> entries into "bypass" mode, which allows transactions to flow through
> >> the SMMU without any translation.
> >>
> >
> > What about masters that require SMMU intervention to override their
> > native memory attributes to make them consistent with the CCA (acpi)
> > or dma-coherent (dt) values specified in FW?
>
> Well, we've already broken them ;) My interpretation of "dma-coherent"
> is as the equivalent of DACS=1,CPM=1, i.e. not dependent on SMMU
> override. For the CCA=1,DACS=0 case (I'm going to pretend the DT
> equivalent will never exist...) the first problem to solve is how to
> inherit the appropriate configuration from the firmware, because right
> now we're not even pretending to support that.
Indeed, and that would need to be added as a separate patch series when
the need arises.
> >> /* Nuke the existing STE_0 value, as we're going to rewrite it */
> >> -val = ste->valid ? STRTAB_STE_0_V : 0;
> >> +val = STRTAB_STE_0_V;
> >> +
> >> +/* Bypass/fault */
> >> +if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) {
> >> +if (!ste->assigned && disable_bypass)
>
> ...yuck. After about 5 minutes of staring at that, I've convinced myself
> that it would make much more sense to always clear the strtab_ent
> configs on detach, such that you never need the outer !ste->assigned
> check here...
I was deliberately keeping the strtab_ent intact in case we ever grow
support for nested translation, where we might well want to detach a
stage 1 but keep the stage 2 installed. I don't think the code is that
bad, so I'd like to leave it like it is for now.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/5] iommu: Allow default domain type to be set on the kernel command line
On Tue, Mar 21, 2017 at 04:45:27PM +0100, Joerg Roedel wrote:
> On Fri, Mar 10, 2017 at 08:49:36PM +0000, Will Deacon wrote:
> > @@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct
> > device *dev)
> > * IOMMU driver.
> > */
> > if (!group->default_domain) {
> > - group->default_domain = __iommu_domain_alloc(dev->bus,
> > -IOMMU_DOMAIN_DMA);
> > + group->default_domain =
> > + __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
>
> It would be good to have a fall-back here if we are talking to an IOMMU
> driver that uses default domains, but does not support identity-mapped
> domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this
> category. A dev_warn() also makes sense in case allocating a identity
> domain fails.
Sure, something like the diff below?
Will
--->8
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 42a842e3f95f..f787626a745d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct
device *dev)
* IOMMU driver.
*/
if (!group->default_domain) {
- group->default_domain =
- __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
+ struct iommu_domain *dom;
+
+ dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
+ if (!dom) {
+ dev_warn(dev,
+"failed to allocate default IOMMU domain of
type %u; falling back to IOMMU_DOMAIN_DMA",
+iommu_def_domain_type);
+ dom = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_DMA);
+ }
+
+ group->default_domain = dom;
if (!group->domain)
- group->domain = group->default_domain;
+ group->domain = dom;
}
ret = iommu_group_add_device(group, dev);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/5] iommu: Allow default domain type to be set on the kernel command line
On Tue, Mar 21, 2017 at 05:46:29PM +, Robin Murphy wrote:
> On 21/03/17 17:21, Will Deacon wrote:
> > On Tue, Mar 21, 2017 at 04:45:27PM +0100, Joerg Roedel wrote:
> >> On Fri, Mar 10, 2017 at 08:49:36PM +, Will Deacon wrote:
> >>> @@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct
> >>> device *dev)
> >>>* IOMMU driver.
> >>>*/
> >>> if (!group->default_domain) {
> >>> - group->default_domain = __iommu_domain_alloc(dev->bus,
> >>> - IOMMU_DOMAIN_DMA);
> >>> + group->default_domain =
> >>> + __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
> >>
> >> It would be good to have a fall-back here if we are talking to an IOMMU
> >> driver that uses default domains, but does not support identity-mapped
> >> domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this
> >> category. A dev_warn() also makes sense in case allocating a identity
> >> domain fails.
> >
> > Sure, something like the diff below?
> >
> > Will
> >
> > --->8
> >
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 42a842e3f95f..f787626a745d 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct
> > device *dev)
> > * IOMMU driver.
> > */
> > if (!group->default_domain) {
> > - group->default_domain =
> > - __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
> > + struct iommu_domain *dom;
> > +
> > + dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
> > + if (!dom) {
> > + dev_warn(dev,
> > +"failed to allocate default IOMMU domain of
> > type %u; falling back to IOMMU_DOMAIN_DMA",
> > +iommu_def_domain_type);
>
> Conversely, that's going to be noisy if iommu_def_domain_type was
> IOMMU_DOMAIN_DMA to begin with. I think it makes sense to warn if the
> user asked for a specific default domain type on the command line and
> that didn't work, but maybe not to bother otherwise. Plus, if they asked
> for passthrough, then not allocating a default domain at all is probably
> closer to the desired result than installing a DMA ops domain would be.
You're right -- I'll hack this about to check if the default type isn't
DOMAIN_DMA before warning about the allocation failure.
Cheers,
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[GIT PULL] iommu/arm-smmu: Fixes for 4.11
Hi Joerg,
Please pull these two ARM io-pgtable fixes from Oleksandr for 4.11. They're
not critical, but they mean that we detect misuses in the iommu_{map,unmap}
API instead of deferencing junk pointers in the kernel. I've had them queued
locally for a while, so Robin and I have given them a fair workout.
Thanks,
Will
--->8
The following changes since commit c1ae3cfa0e89fa1a7ecc4c99031f5e9ae99d9201:
Linux 4.11-rc1 (2017-03-05 12:59:56 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git
for-joerg/arm-smmu/fixes
for you to fetch changes up to a03849e7210277fa212779b7cd9c30e1ab6194b2:
iommu/io-pgtable-arm-v7s: Check for leaf entry before dereferencing it
(2017-03-10 18:23:34 +)
Oleksandr Tyshchenko (2):
iommu/io-pgtable-arm: Check for leaf entry before dereferencing it
iommu/io-pgtable-arm-v7s: Check for leaf entry before dereferencing it
drivers/iommu/io-pgtable-arm-v7s.c | 6 +-
drivers/iommu/io-pgtable-arm.c | 6 +-
2 files changed, 10 insertions(+), 2 deletions(-)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/4] iommu/arm-smmu: Handle size mismatches better
Hi Robin,
On Tue, Mar 07, 2017 at 06:09:04PM +, Robin Murphy wrote:
> We currently warn if the firmware-described region size differs from the
> SMMU address space size reported by the hardware, but continue to use
> the former to calculate where our context bank base should be,
> effectively guaranteeing that things will not work correctly.
>
> Since over-mapping is effectively harmless, and under-mapping can be OK
> provided all the usable context banks are still covered, let's let the
> hardware information take precedence in the case of a mismatch, such
> that we get the correct context bank base and in most cases things will
> actually work instead of silently misbehaving. And at worst, if the
> firmware is wrong enough to have not mapped something we actually try to
> use, the resulting out-of-bounds access will hopefully provide a much
> more obvious clue.
>
> Signed-off-by: Robin Murphy
> ---
> drivers/iommu/arm-smmu.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index abf6496843a6..bc7ef6a0c54d 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1864,10 +1864,12 @@ static int arm_smmu_device_cfg_probe(struct
> arm_smmu_device *smmu)
> /* Check for size mismatch of SMMU address space from mapped region */
> size = 1 << (((id >> ID1_NUMPAGENDXB_SHIFT) & ID1_NUMPAGENDXB_MASK) +
> 1);
> size *= 2 << smmu->pgshift;
> - if (smmu->size != size)
> + if (smmu->size != size) {
> dev_warn(smmu->dev,
> "SMMU address space size (0x%lx) differs from mapped
> region size (0x%lx)!\n",
> size, smmu->size);
> + smmu->size = size;
> + }
I'm not really in favour of this, but I admit that this case is a bit weird.
Basically, we always have two ways to determine the size of the SMMU:
1. The device-tree reg property, since we need the base address to be
passed there and this will include a size.
2. The ID register on the hardware itself
Generally, I prefer that properties passed by firmware override those
baked into the hardware, since this allows us to deal with broken ID
registers easily. In this case, we basically have the size override from
the reg property, so it takes precedence, but we warn if it differs from
the hardware value so hopefully broken DTs are easily diagnosed.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] iommu/arm-smmu: Use per-context TLB sync as appropriate
Hi Robin,
This mostly looks great, but I have a couple of minor comments below.
On Tue, Mar 07, 2017 at 06:09:07PM +, Robin Murphy wrote:
> TLB synchronisation typically involves the SMMU blocking all incoming
> transactions until the TLBs report completion of all outstanding
> operations. In the common SMMUv2 configuration of a single distributed
> SMMU serving multiple peripherals, that means that a single unmap
> request has the potential to bring the hammer down on the entire system
> if synchronised globally. Since stage 1 contexts, and stage 2 contexts
> under SMMUv2, offer local sync operations, let's make use of those
> wherever we can in the hope of minimising global disruption.
>
> To that end, rather than add any more branches to the already unwieldy
> monolithic TLB maintenance ops, break them up into smaller, neater,
> functions which we can then mix and match as appropriate.
>
> Signed-off-by: Robin Murphy
> ---
> drivers/iommu/arm-smmu.c | 156
> ++-
> 1 file changed, 100 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index c8aafe304171..f7411109670f 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -237,6 +237,8 @@ enum arm_smmu_s2cr_privcfg {
> #define ARM_SMMU_CB_S1_TLBIVAL 0x620
> #define ARM_SMMU_CB_S2_TLBIIPAS2 0x630
> #define ARM_SMMU_CB_S2_TLBIIPAS2L0x638
> +#define ARM_SMMU_CB_TLBSYNC 0x7f0
> +#define ARM_SMMU_CB_TLBSTATUS0x7f4
> #define ARM_SMMU_CB_ATS1PR 0x800
> #define ARM_SMMU_CB_ATSR 0x8f0
>
> @@ -569,14 +571,13 @@ static void __arm_smmu_free_bitmap(unsigned long *map,
> int idx)
> }
>
> /* Wait for any pending TLB invalidations to complete */
> -static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
> +static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu,
> + void __iomem *sync, void __iomem *status)
Given that you take the arm_smmu_device anyway, I think I'd prefer just
passing the offsets for sync and status and avoiding the additions
in the caller (a bit like your other patch in this series ;).
> static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
> @@ -617,48 +638,66 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long
> iova, size_t size,
> {
> struct arm_smmu_domain *smmu_domain = cookie;
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> - struct arm_smmu_device *smmu = smmu_domain->smmu;
> bool stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
> - void __iomem *reg;
> + void __iomem *reg = ARM_SMMU_CB(smmu_domain->smmu, cfg->cbndx);
> + size_t step;
>
> - if (stage1) {
> - reg = ARM_SMMU_CB(smmu, cfg->cbndx);
> - reg += leaf ? ARM_SMMU_CB_S1_TLBIVAL : ARM_SMMU_CB_S1_TLBIVA;
> -
> - if (cfg->fmt != ARM_SMMU_CTX_FMT_AARCH64) {
> - iova &= ~12UL;
> - iova |= cfg->asid;
> - do {
> - writel_relaxed(iova, reg);
> - iova += granule;
> - } while (size -= granule);
> - } else {
> - iova >>= 12;
> - iova |= (u64)cfg->asid << 48;
> - do {
> - writeq_relaxed(iova, reg);
> - iova += granule >> 12;
> - } while (size -= granule);
> - }
> - } else if (smmu->version == ARM_SMMU_V2) {
> - reg = ARM_SMMU_CB(smmu, cfg->cbndx);
> + if (stage1)
> + reg += leaf ? ARM_SMMU_CB_S1_TLBIVAL :
> + ARM_SMMU_CB_S1_TLBIVA;
> + else
> reg += leaf ? ARM_SMMU_CB_S2_TLBIIPAS2L :
> ARM_SMMU_CB_S2_TLBIIPAS2;
> - iova >>= 12;
> - do {
> - smmu_write_atomic_lq(iova, reg);
> - iova += granule >> 12;
> - } while (size -= granule);
> +
> + if (stage1 && cfg->fmt != ARM_SMMU_CTX_FMT_AARCH64) {
> + iova &= ~12UL;
> + iova |= cfg->asid;
> + step = granule;
> } else {
> - reg = ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_TLBIVMID;
> - writel_relaxed(cfg->vmid, reg);
> + iova >>= 12;
> + step = granule >> 12;
> + if (stage1)
> + iova |= (u64)cfg->asid << 48;
> }
> +
> + do {
> + smmu_write_atomic_lq(iova, reg);
> + iova += step;
> + } while (size -= granule);
There seems to be a lot of refactoring going on here, but I'm not entirely
comfortable with the unconditional move to smmu_write_atomic_lq. Given the
way in which arm_smmu_tlb_inv_range_nosync is now called (i.e. only for
stage-1 or SMMUv2 stage-2), then I think you just need to delete
Re: [PATCH 5/4] iommu/arm-smmu: Poll for TLB sync completion more effectively
On Thu, Mar 23, 2017 at 05:59:40PM +, Robin Murphy wrote:
> On relatively slow development platforms and software models, the
> inefficiency of our TLB sync loop tends not to show up - for instance on
> a Juno r1 board I typically see the TLBI has completed of its own accord
> by the time we get to the sync, such that the latter finishes instantly.
>
> However, on larger systems doing real I/O, it's less realistic for the
> TLBs to go idle immediately, and at that point falling into the 1MHz
> polling loop turns out to throw away performance drastically. Let's
> strike a balance by polling more than once between pauses, such that we
> have much more chance of catching normal operations completing before
> committing to the fixed delay, but also backing off exponentially, since
> if a sync really hasn't completed within one or two "reasonable time"
> periods, it becomes increasingly unlikely that it ever will.
>
> Signed-off-by: Robin Murphy
> ---
> drivers/iommu/arm-smmu.c | 18 +-
> 1 file changed, 9 insertions(+), 9 deletions(-)
Thanks, I like this patch.
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index f7411109670f..aa17f3d937a0 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -162,6 +162,7 @@
> #define ARM_SMMU_GR0_sTLBGSTATUS 0x74
> #define sTLBGSTATUS_GSACTIVE (1 << 0)
> #define TLB_LOOP_TIMEOUT 100 /* 1s! */
> +#define TLB_SPIN_COUNT 10
>
> /* Stream mapping registers */
> #define ARM_SMMU_GR0_SMR(n) (0x800 + ((n) << 2))
> @@ -574,18 +575,17 @@ static void __arm_smmu_free_bitmap(unsigned long *map,
> int idx)
> static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu,
> void __iomem *sync, void __iomem *status)
> {
> - int count = 0;
> + unsigned int spin_count, delay;
>
> writel_relaxed(0, sync);
> - while (readl_relaxed(status) & sTLBGSTATUS_GSACTIVE) {
> - cpu_relax();
> - if (++count == TLB_LOOP_TIMEOUT) {
> - dev_err_ratelimited(smmu->dev,
> - "TLB sync timed out -- SMMU may be deadlocked\n");
> - return;
> - }
> - udelay(1);
> + for (delay = 1; delay < TLB_LOOP_TIMEOUT; delay *= 2) {
> + for (spin_count = TLB_SPIN_COUNT; spin_count > 0; spin_count--)
> + if (!(readl_relaxed(status) & sTLBGSTATUS_GSACTIVE))
> + return;
Can you keep the cpu_relax in the inner loop please?
> + udelay(delay);
> }
> + dev_err_ratelimited(smmu->dev,
> + "TLB sync timed out -- SMMU may be deadlocked\n");
Whilst we can have WFE-based spinning with SMMUv3, I suppose we should
do something similar in queue_poll_cons... Fancy taking a look?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/4] ARM SMMU per-context TLB sync
On Tue, Mar 07, 2017 at 06:09:03PM +, Robin Murphy wrote: > The discussion around context-level access for Qualcomm SMMUs reminded > me to dig up this patch I started ages ago and finish it off. As it's > ended up, it's now a mini-series, with some new preparatory cleanup > manifesting as patches 2 and 3. Patch 1 is broken out of patch 3 for > clarity as somewhat of a fix in its own right, in that it's really an > entirely unrelated thing which came up in parallel, but happens to > be inherent to code I'm touching here anyway. Apart from the first patch, most of this is looking good. Please can you respin the series without the first patch and with my comments addressed? Cheers, Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/4] ARM SMMU TLB sync improvements
On Thu, Mar 30, 2017 at 05:56:28PM +0100, Robin Murphy wrote: > Here's a quick v2 to address your comments and drop the needless meddling > (whaddaya know, it makes the whole lot look simpler!) > > I'll put it on my list to take a look at SMMUv3 queue polling as suggested. Thanks, I've queued this with Jordan's tags. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/3] IOVA allocation improvements for iommu-dma
Hi Robin, Joerg, On Wed, Mar 15, 2017 at 01:33:13PM +, Robin Murphy wrote: > Here's the first bit of lock contention removal to chew on - feedback > welcome! Note that for the current users of the io-pgtable framework, > this is most likely to simply push more contention onto the io-pgtable > lock, so may not show a great improvement alone. Will and I both have > rough proof-of-concept implementations of lock-free io-pgtable code > which we need to sit down and agree on at some point, hopefullt fairly > soon. > > I've taken the opportunity to do a bit of cleanup and refactoring > within the series to make the final state of the code nicer, but the > diffstat still turns out surprisingly reasonable in the end - it would > actually be negative but for the new comments! > > Magnus, Shimoda-san, the first two patches should be of interest as they > constitute the allocation rework I mentioned a while back[1] - if you > still need to implement that scary workaround, this should make it > simple to hook IPMMU-specific calls into the alloc and free paths, and > let the driver take care of the details internally. What's the plan for merging this? Whilst we still have the iopgtable lock contention to resolve, this is a good incremental step towards improving our scalability and sits nicely alongside the SMMUv2 TLB improvements I just queued. Thanks, Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V3 0/5] iommu/arm-smmu: Add runtime pm/sleep support
On Thu, Mar 09, 2017 at 09:05:43PM +0530, Sricharan R wrote: > This series provides the support for turning on the arm-smmu's > clocks/power domains using runtime pm. This is done using the > recently introduced device links patches, which lets the symmu's > runtime to follow the master's runtime pm, so the smmu remains > powered only when the masters use it. Do you have any numbers for the power savings you achieve with this? How often do we actually manage to stop the SMMU clocks on an SoC with a handful of masters? In other words, is this too coarse-grained to be useful, or is it common that all the devices upstream of the SMMU are suspended? Thanks, Will > > Took some reference from the exynos runtime patches [2]. > Tested this with MDP, GPU, VENUS devices on apq8096-db820c board. > > Previous version of the patchset [1]. > > [V3] >* Reworked the patches to keep the clocks init/enabling function > seperately for each compatible. > >* Added clocks bindings for MMU40x/500. > >* Added a new compatible for qcom,smmu-v2 implementation and > the clock bindings for the same. > >* Rebased on top of 4.11-rc1 > > [V2] >* Split the patches little differently. > >* Addressed comments. > >* Removed the patch #4 [3] from previous post > for arm-smmu context save restore. Planning to > post this separately after reworking/addressing Robin's > feedback. > >* Reversed the sequence to disable clocks than enabling. > This was required for those cases where the > clocks are populated in a dependent order from DT. > > [1] https://www.spinics.net/lists/linux-arm-msm/msg23870.html > [2] https://lkml.org/lkml/2016/10/20/70 > [3] https://patchwork.kernel.org/patch/9389717/ > > Sricharan R (5): > iommu/arm-smmu: Add pm_runtime/sleep ops > iommu/arm-smmu: Add support for MMU40x/500 clocks > drivers: arm-smmu: Add clock support for QCOM_SMMUV2 > iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device > iommu/arm-smmu: Add the device_link between masters and smmu > > .../devicetree/bindings/iommu/arm,smmu.txt | 35 +++ > drivers/iommu/arm-smmu.c | 349 > - > 2 files changed, 373 insertions(+), 11 deletions(-) > > -- > 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 V3 0/5] iommu/arm-smmu: Add runtime pm/sleep support
On Fri, Mar 31, 2017 at 10:58:16PM -0400, Rob Clark wrote: > On Fri, Mar 31, 2017 at 1:54 PM, Will Deacon wrote: > > On Thu, Mar 09, 2017 at 09:05:43PM +0530, Sricharan R wrote: > >> This series provides the support for turning on the arm-smmu's > >> clocks/power domains using runtime pm. This is done using the > >> recently introduced device links patches, which lets the symmu's > >> runtime to follow the master's runtime pm, so the smmu remains > >> powered only when the masters use it. > > > > Do you have any numbers for the power savings you achieve with this? > > How often do we actually manage to stop the SMMU clocks on an SoC with > > a handful of masters? > > > > In other words, is this too coarse-grained to be useful, or is it common > > that all the devices upstream of the SMMU are suspended? > > well, if you think about a phone/tablet with a command mode panel, > pretty much all devices will be suspended most of the time ;-) Well, that's really what I was asking about. I assumed that periodic modem/radio transactions would keep the SMMU clocked, so would like to get a rough idea of the power savings achieved with this coarse-grained approach. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu: Fix 16bit ASID configuration
On Mon, Apr 03, 2017 at 11:16:33PM +0530, Sunil Kovvuri wrote:
> On Tue, Mar 28, 2017 at 4:11 PM, wrote:
> > From: Sunil Goutham
> >
> > 16bit ASID should be enabled before initializing TTBR0/1,
> > otherwise only LSB 8bit ASID will be considered. Hence
> > moving configuration of TTBCR register ahead of TTBR0/1
> > while initializing context bank.
> >
> > Signed-off-by: Sunil Goutham
> > ---
> > drivers/iommu/arm-smmu.c | 41 ++---
> > 1 file changed, 22 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 9b33700..2845d73 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -758,6 +758,28 @@ static void arm_smmu_init_context_bank(struct
> > arm_smmu_domain *smmu_domain,
> > }
> > writel_relaxed(reg, gr1_base + ARM_SMMU_GR1_CBAR(cfg->cbndx));
> >
> > + /* TTBCR */
> > + if (stage1) {
> > + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> > + reg = pgtbl_cfg->arm_v7s_cfg.tcr;
> > + reg2 = 0;
> > + } else {
> > + reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> > + reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
> > + reg2 |= TTBCR2_SEP_UPSTREAM;
> > + /* 16bit ASID should be enabled before write to
> > TTBR,
> > +* otherwise only LSB 8bit will be considered.
> > +*/
> > + if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> > + reg2 |= TTBCR2_AS;
> > + }
> > + if (smmu->version > ARM_SMMU_V1)
> > + writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
> > + } else {
> > + reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
> > + }
> > + writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
> > +
> > /* TTBRs */
> > if (stage1) {
> > u16 asid = ARM_SMMU_CB_ASID(smmu, cfg);
> > @@ -781,25 +803,6 @@ static void arm_smmu_init_context_bank(struct
> > arm_smmu_domain *smmu_domain,
> > writeq_relaxed(reg64, cb_base + ARM_SMMU_CB_TTBR0);
> > }
> >
> > - /* TTBCR */
> > - if (stage1) {
> > - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> > - reg = pgtbl_cfg->arm_v7s_cfg.tcr;
> > - reg2 = 0;
> > - } else {
> > - reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
> > - reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
> > - reg2 |= TTBCR2_SEP_UPSTREAM;
> > - if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> > - reg2 |= TTBCR2_AS;
> > - }
> > - if (smmu->version > ARM_SMMU_V1)
> > - writel_relaxed(reg2, cb_base + ARM_SMMU_CB_TTBCR2);
> > - } else {
> > - reg = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
> > - }
> > - writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR);
> > -
> > /* MAIRs (stage-1 only) */
> > if (stage1) {
> > if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH32_S) {
> > --
> > 2.7.4
> >
>
> Any comments or feedback on this patch ?
I've picked it up.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[GIT PULL] iommu/arm-smmu: Updates for 4.12
Hi Joerg, Please pull these arm-smmu updates for 4.12. Highlights include: * TLB sync optimisations for SMMUv2 * Support for using an IDENTITY domain in conjunction with DMA ops * Support for SMR masking * Support for 16-bit ASIDs (was previously broken) Thanks, Will --->8 The following changes since commit c02ed2e75ef4c74e41e421acb4ef1494671585e8: Linux 4.11-rc4 (2017-03-26 14:15:16 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git for-joerg/arm-smmu/updates for you to fetch changes up to 022f4e4f31fea69702f3ec810dc567af6a6d86d8: iommu/io-pgtable-arm: Avoid shift overflow in block size (2017-04-06 16:06:44 +0100) Robert Richter (1): iommu/arm-smmu: Print message when Cavium erratum 27704 was detected Robin Murphy (7): iommu: Better document the IOMMU_PRIV flag iommu/arm-smmu: Simplify ASID/VMID handling iommu/arm-smmu: Tidy up context bank indexing iommu/arm-smmu: Use per-context TLB sync as appropriate iommu/arm-smmu: Poll for TLB sync completion more effectively iommu/arm-smmu: Add global SMR masking property iommu/io-pgtable-arm: Avoid shift overflow in block size Sunil Goutham (1): iommu/arm-smmu: Fix 16-bit ASID configuration Will Deacon (5): iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains iommu: Allow default domain type to be set on the kernel command line Documentation/admin-guide/kernel-parameters.txt| 6 + .../devicetree/bindings/iommu/arm,smmu.txt | 28 +++ drivers/iommu/arm-smmu-v3.c| 76 +++--- drivers/iommu/arm-smmu.c | 259 + drivers/iommu/io-pgtable-arm.c | 2 +- drivers/iommu/iommu.c | 28 ++- include/linux/iommu.h | 11 +- 7 files changed, 281 insertions(+), 129 deletions(-) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 3/7] iommu/arm-smmu-v3: Introduce smmu option USE_SHARED_IRQS for Silicon errata
On Tue, Apr 11, 2017 at 04:54:26PM +0100, Robin Murphy wrote:
> On 11/04/17 15:42, linucher...@gmail.com wrote:
> > From: Geetha
> >
> > Cavium 99xx SMMU implementation doesn't not support unique irq lines for
> > gerror, eventq and cmdq-sync. USE_SHARED_IRQS option enables to use single
> > irq line for all three interrupts.
>
> AFAICS, there's nothing actually wrong with using shared wired IRQs -
> the architecture spec doesn't appear to say anything about it. I think
> it might suffice to simply add IRQF_SHARED if we can see the SMMU
> doesn't support MSIs anyway - it doesn't really seem like something we
> need to treat as a specific quirk.
No, this is not permitted by the spec. See 3.18.2 ("Interrupt sources"),
where it's clear that each source asserts a *unique* wired interrupt.
Geetha: does your implementation support MSIs?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 0/7] Cavium CN99xx SMMUv3 Errata workarounds
On Tue, Apr 11, 2017 at 08:12:38PM +0530, linucher...@gmail.com wrote: > From: Linu Cherian > > Cavium CN99xx SMMUv3 implementation has two Silicon Erratas. > 1. Errata ID #74 >SMMU register alias Page 1 is not implemented > 2. Errata ID #126 >SMMU doesnt support unique IRQ lines for gerror, eventq and cmdq-sync Is this device in production, or just part of a test chip? Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 3/7] iommu/arm-smmu-v3: Introduce smmu option USE_SHARED_IRQS for Silicon errata
On Tue, Apr 11, 2017 at 05:38:21PM +0100, Robin Murphy wrote:
> On 11/04/17 17:21, Will Deacon wrote:
> > On Tue, Apr 11, 2017 at 04:54:26PM +0100, Robin Murphy wrote:
> >> On 11/04/17 15:42, linucher...@gmail.com wrote:
> >>> From: Geetha
> >>>
> >>> Cavium 99xx SMMU implementation doesn't not support unique irq lines for
> >>> gerror, eventq and cmdq-sync. USE_SHARED_IRQS option enables to use single
> >>> irq line for all three interrupts.
> >>
> >> AFAICS, there's nothing actually wrong with using shared wired IRQs -
> >> the architecture spec doesn't appear to say anything about it. I think
> >> it might suffice to simply add IRQF_SHARED if we can see the SMMU
> >> doesn't support MSIs anyway - it doesn't really seem like something we
> >> need to treat as a specific quirk.
> >
> > No, this is not permitted by the spec. See 3.18.2 ("Interrupt sources"),
> > where it's clear that each source asserts a *unique* wired interrupt.
>
> Perhaps I'm reading it too generously; it does indeed specify that the
> *implementation* has to provide a unique output for each source, but
> other than suggesting a particular mode of operation based on that I
> don't see anything actually forbidding the *integration* from then just
> munging those lines together externally, as integrators so often like to
> do. That's the case I had in mind.
Sure, but then there wouldn't be any point in the architecture mandating
a unique source, would there? What next, OR all the address lines together
too?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu: Return IOVA in iova_to_phys when SMMU is bypassed
On Mon, Apr 17, 2017 at 05:27:26PM +0530, sunil.kovv...@gmail.com wrote: > From: Sunil Goutham > > For software initiated address translation, when domain type is > IOMMU_DOMAIN_IDENTITY i.e SMMU is bypassed, mimic HW behavior > i.e return the same IOVA as translated address. > > This patch is an extension to Will Deacon's patchset > "Implement SMMU passthrough using the default domain". Are you actually seeing an issue here? If so, why isn't SMMUv3 affected too? > Signed-off-by: Sunil Goutham > --- > drivers/iommu/arm-smmu.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 41afb07..2f4a130 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -1405,6 +1405,9 @@ static phys_addr_t arm_smmu_iova_to_phys(struct > iommu_domain *domain, > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct io_pgtable_ops *ops= smmu_domain->pgtbl_ops; > > + if (domain->type == IOMMU_DOMAIN_IDENTITY) > + return iova; > + > if (!ops) > return 0; I'd have thought ops would be NULL, since arm_smmu_init_domain_context doesn't allocate them for an identity domain. I don't understand this patch. Please can you explain the problem more clearly? Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: arm-smmu: correct sid to mask
On Fri, Apr 21, 2017 at 05:03:36PM +0800, Peng Fan wrote:
> From code "SMR mask 0x%x out of range for SMMU",
> so, we need to use mask, not sid.
>
> Signed-off-by: Peng Fan
> Cc: Will Deacon
> Cc: Robin Murphy
> ---
> drivers/iommu/arm-smmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index b493c99..5a06de2 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1467,7 +1467,7 @@ static int arm_smmu_add_device(struct device *dev)
> }
> if (mask & ~smmu->smr_mask_mask) {
> dev_err(dev, "SMR mask 0x%x out of range for SMMU
> (0x%x)\n",
> - sid, smmu->smr_mask_mask);
> + mask, smmu->smr_mask_mask);
> goto out_free;
Looks like a copy-paste error to me:
Acked-by: Will Deacon
Joerg: do you mind picking this one up, please?
Thanks,
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu: Return IOVA in iova_to_phys when SMMU is bypassed
On Mon, Apr 24, 2017 at 09:23:16PM +0530, Sunil Kovvuri wrote: > On Mon, Apr 24, 2017 at 8:14 PM, Will Deacon wrote: > > On Mon, Apr 17, 2017 at 05:27:26PM +0530, sunil.kovv...@gmail.com wrote: > >> From: Sunil Goutham > >> > >> For software initiated address translation, when domain type is > >> IOMMU_DOMAIN_IDENTITY i.e SMMU is bypassed, mimic HW behavior > >> i.e return the same IOVA as translated address. > >> > >> This patch is an extension to Will Deacon's patchset > >> "Implement SMMU passthrough using the default domain". > > > > Are you actually seeing an issue here? If so, why isn't SMMUv3 affected too? > Yes and SMMUv3 should also be effected but as of now I don't see any use case. > If needed, i can re-submit the patch with changes in SMMUv3 as well. Yes, please. > >> Signed-off-by: Sunil Goutham > >> --- > >> drivers/iommu/arm-smmu.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > >> index 41afb07..2f4a130 100644 > >> --- a/drivers/iommu/arm-smmu.c > >> +++ b/drivers/iommu/arm-smmu.c > >> @@ -1405,6 +1405,9 @@ static phys_addr_t arm_smmu_iova_to_phys(struct > >> iommu_domain *domain, > >> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > >> struct io_pgtable_ops *ops= smmu_domain->pgtbl_ops; > >> > >> + if (domain->type == IOMMU_DOMAIN_IDENTITY) > >> + return iova; > >> + > >> if (!ops) > >> return 0; > > > > I'd have thought ops would be NULL, since arm_smmu_init_domain_context > > doesn't allocate them for an identity domain. > Yes ops is set to NULL. Argh, sorry, I completely overlooked that we return 0 in that case, rather than the iova. > > I don't understand this patch. Please can you explain the problem more > > clearly? > AFAIK for any driver outside IOMMU there is only one way to identify > if device is attached to > IOMMU or not and that is by checking iommu_domain. And I don't think > it would be appropriate > for the driver to check domain->type before calling 'iommu_iova_to_phys()' > API. > > The difference between IOMMU disabled and IOMMU being in passthrough > mode is that, in the > later case device is still attached to default domain but in former's > case it's NULL. So there is no > way to differentiate for the external driver whether IOMMU is in > passthrough mode or DMA mode. > > And since ops is NULL in passthrough mode, 'iommu_iova_to_phys()' will > return zero. > > Use case for your reference > https://lkml.org/lkml/2017/3/7/299 > This driver is for a NIC interface on platform which supports SMMUv2. Blimey, that driver is horrible, but I take your point on the API. Please repost, fixing SMMUv3 at the same time. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Increase SMMU CMD queue poll timeout
On Mon, Apr 24, 2017 at 05:29:36PM +0530, Geetha sowjanya wrote:
> From: Geetha
>
> When large memory is being unmapped, huge no of tlb invalidation cmds are
> submitted followed by a SYNC command. This sometimes hits CMD queue full and
> poll on queue drain is being timedout throwing error message 'CMD_SYNC
> timeout'.
>
> Although there is no functional issue, error message confuses user. Hence
> increased
> poll timeout to 500us
Hmm, what are you doing to unmap that much? Is this VFIO teardown? Do you
have 7c6d90e2bb1a ("iommu/io-pgtable-arm: Fix iova_to_phys for block
entries") applied?
Will
>
> Signed-off-by: Geetha
> ---
> drivers/iommu/arm-smmu-v3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 591bb96..1dcd154 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -407,7 +407,7 @@
> #define PRIQ_1_ADDR_MASK 0xfUL
>
> /* High-level queue structures */
> -#define ARM_SMMU_POLL_TIMEOUT_US 100
> +#define ARM_SMMU_POLL_TIMEOUT_US 500
>
> #define MSI_IOVA_BASE0x800
> #define MSI_IOVA_LENGTH 0x10
> --
> 1.9.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Increase SMMU CMD queue poll timeout
On Mon, Apr 24, 2017 at 10:26:53PM +0530, Sunil Kovvuri wrote:
> On Mon, Apr 24, 2017 at 9:38 PM, Will Deacon wrote:
> > On Mon, Apr 24, 2017 at 05:29:36PM +0530, Geetha sowjanya wrote:
> >> From: Geetha
> >>
> >> When large memory is being unmapped, huge no of tlb invalidation cmds are
> >> submitted followed by a SYNC command. This sometimes hits CMD queue full
> >> and
> >> poll on queue drain is being timedout throwing error message 'CMD_SYNC
> >> timeout'.
> >>
> >> Although there is no functional issue, error message confuses user. Hence
> >> increased
> >> poll timeout to 500us
> >
> > Hmm, what are you doing to unmap that much? Is this VFIO teardown? Do you
> > have 7c6d90e2bb1a ("iommu/io-pgtable-arm: Fix iova_to_phys for block
> > entries") applied?
>
> Yes it's VFIO teardown and again yes the above fix is applied.
> But i didn't get how above fix is related.
> TLB invalidation commands are submitted at 'arm_smmu_tlb_inv_range_nosync()'
> and it's a loop over granule size.
>
> 1357 do {
> 1358 arm_smmu_cmdq_issue_cmd(smmu, &cmd);
> 1359 cmd.tlbi.addr += granule;
> 1360 } while (size -= granule);
>
> So if invalidation size is big then huge no of invalidation commands
> will be submitted
> irrespective of fix that you pointed above, right ?
VFIO has some logic to batch up invalidations, but this didn't work properly
for us without the fix above. However, I guess you have a huge memory range
that's mapped with 2M sections or something, so there are still loads of
entries to invalidate.
I would much prefer it if VFIO could just teardown the whole address space
so that we could do an invalidate all, but there's a chicken-and-egg problem
with page accounting iirc.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Increase SMMU CMD queue poll timeout
On Wed, Apr 26, 2017 at 02:50:04PM +0530, Sunil Kovvuri wrote:
> On Mon, Apr 24, 2017 at 10:35 PM, Will Deacon wrote:
> > On Mon, Apr 24, 2017 at 10:26:53PM +0530, Sunil Kovvuri wrote:
> >> On Mon, Apr 24, 2017 at 9:38 PM, Will Deacon wrote:
> >> > On Mon, Apr 24, 2017 at 05:29:36PM +0530, Geetha sowjanya wrote:
> >> >> From: Geetha
> >> >>
> >> >> When large memory is being unmapped, huge no of tlb invalidation cmds
> >> >> are
> >> >> submitted followed by a SYNC command. This sometimes hits CMD queue
> >> >> full and
> >> >> poll on queue drain is being timedout throwing error message 'CMD_SYNC
> >> >> timeout'.
> >> >>
> >> >> Although there is no functional issue, error message confuses user.
> >> >> Hence increased
> >> >> poll timeout to 500us
> >> >
> >> > Hmm, what are you doing to unmap that much? Is this VFIO teardown? Do you
> >> > have 7c6d90e2bb1a ("iommu/io-pgtable-arm: Fix iova_to_phys for block
> >> > entries") applied?
> >>
> >> Yes it's VFIO teardown and again yes the above fix is applied.
> >> But i didn't get how above fix is related.
> >> TLB invalidation commands are submitted at
> >> 'arm_smmu_tlb_inv_range_nosync()'
> >> and it's a loop over granule size.
> >>
> >> 1357 do {
> >> 1358 arm_smmu_cmdq_issue_cmd(smmu, &cmd);
> >> 1359 cmd.tlbi.addr += granule;
> >> 1360 } while (size -= granule);
> >>
> >> So if invalidation size is big then huge no of invalidation commands
> >> will be submitted
> >> irrespective of fix that you pointed above, right ?
> >
> > VFIO has some logic to batch up invalidations, but this didn't work properly
> > for us without the fix above. However, I guess you have a huge memory range
> > that's mapped with 2M sections or something, so there are still loads of
> > entries to invalidate.
> >
> > I would much prefer it if VFIO could just teardown the whole address space
> > so that we could do an invalidate all, but there's a chicken-and-egg problem
> > with page accounting iirc.
> >
>
> We can definitely look into this from VFIO perspective but for now I am
> guessing
> this patch is fine, as no functionality is being changed.
> What do you say ?
Thinking about it some more, I'd rather we rework the polling loop so that:
1. It's structured more like the arm-smmu.c TLB loop queued for 4.11
(so we don't udelay(1) if the thing doesn't sync immediately)
2. Have a larger timeout for the drain case, which I think is what you're
running into. This could even be 1s, like arm-smmu.c.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/arm-smmu: Return IOVA in iova_to_phys when SMMU is bypassed
Hi Sunil, On Tue, Apr 25, 2017 at 03:27:52PM +0530, sunil.kovv...@gmail.com wrote: > From: Sunil Goutham > > For software initiated address translation, when domain type is > IOMMU_DOMAIN_IDENTITY i.e SMMU is bypassed, mimic HW behavior > i.e return the same IOVA as translated address. > > This patch is an extension to Will Deacon's patchset > "Implement SMMU passthrough using the default domain". > > Signed-off-by: Sunil Goutham > --- > > V2 > - As per Will's suggestion applied fix to SMMUv3 driver as well. This follows what the AMD driver does, so: Acked-by: Will Deacon but I still think that having drivers/net/ethernet/cavium/thunder/nicvf_queues.c poke around with the physical address to get at the struct pages underlying a DMA buffer is really dodgy. Is there no way this can be avoided, perhaps by tracking the pages some other way (although I don't understand why you're having to mess with the page reference counts to start with)? At least, I think you should be checking the domain type in nicvf_iova_to_phys, which clearly expects a DMA domain if one exists at all. Joerg: sorry, this is another one for you to pick up if possible. Cheers, Will > drivers/iommu/arm-smmu-v3.c | 3 +++ > drivers/iommu/arm-smmu.c| 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index 05b4592..d412bdd 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -1714,6 +1714,9 @@ arm_smmu_iova_to_phys(struct iommu_domain *domain, > dma_addr_t iova) > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops; > > + if (domain->type == IOMMU_DOMAIN_IDENTITY) > + return iova; > + > if (!ops) > return 0; > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index bfab4f7..81088cd 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -1459,6 +1459,9 @@ static phys_addr_t arm_smmu_iova_to_phys(struct > iommu_domain *domain, > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct io_pgtable_ops *ops= smmu_domain->pgtbl_ops; > > + if (domain->type == IOMMU_DOMAIN_IDENTITY) > + return iova; > + > if (!ops) > return 0; > > -- > 2.7.4 > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/arm-smmu: Return IOVA in iova_to_phys when SMMU is bypassed
On Wed, Apr 26, 2017 at 04:13:29PM +0530, Sunil Kovvuri wrote: > On Wed, Apr 26, 2017 at 3:31 PM, Will Deacon wrote: > > Hi Sunil, > > > > On Tue, Apr 25, 2017 at 03:27:52PM +0530, sunil.kovv...@gmail.com wrote: > >> From: Sunil Goutham > >> > >> For software initiated address translation, when domain type is > >> IOMMU_DOMAIN_IDENTITY i.e SMMU is bypassed, mimic HW behavior > >> i.e return the same IOVA as translated address. > >> > >> This patch is an extension to Will Deacon's patchset > >> "Implement SMMU passthrough using the default domain". > >> > >> Signed-off-by: Sunil Goutham > >> --- > >> > >> V2 > >> - As per Will's suggestion applied fix to SMMUv3 driver as well. > > > > This follows what the AMD driver does, so: > > > > Acked-by: Will Deacon > > Thanks, > > > > > but I still think that having > > drivers/net/ethernet/cavium/thunder/nicvf_queues.c > > poke around with the physical address to get at the struct pages underlying > > a DMA buffer is really dodgy. > > Driver is not dealing with page structures to be precise, just like > for any other NIC device, driver needs to know the virtual address > of the packet to where it's DMA'ed, so that SKB if framed and > handed over to network stack. Due to reasons mentioned below, > in this driver it's not possible to maintain a list of DMA addresses to > Virtual address mappings. Hence using IOMMU API, DMA address > is translated to physical address and finally to virtual address. I don't > see anything dodgy here. It's dodgy because you're the only NIC driver using iommu_iova_to_phys directly and, afaict, the driver could just stash either the struct page or the virtual address at the point of allocation. > > Is there no way this can be avoided, perhaps by tracking the pages some > > other way > > I have explained that in the commit message > -- > Also VNIC doesn't have a seperate receive buffer ring per receive > queue, so there is no 1:1 descriptor index matching between CQE_RX > and the index in buffer ring from where a buffer has been used for > DMA'ing. Unlike other NICs, here it's not possible to maintain dma > address to virt address mappings within the driver. This leaves us > no other choice but to use IOMMU's IOVA address conversion API to > get buffer's virtual address which can be given to network stack > for processing. > -- > > >(although I don't understand why you're having to mess with the page > >reference > >counts to start with)? > Not sure why you say it's a mess, adjusting page reference counts is quite > common if you check other NIC drivers. On ARM64 especially when using > 64KB pages, if we have only one packet buffer for each page then we > will have to set aside a whole lot of memory which sometimes is not possible > on embedded platforms. Hence multiple pkt buffers per page, and page reference > is set accordingly. I wasn't saying that was a mess, I was just saying that I didn't understand why you mess (verb) with the page reference counts (my ignorance of the network layer). The code that I think is a mess is: phys_addr = nicvf_iova_to_phys(nic, buf_addr); [...] put_page(virt_to_page(phys_to_virt(phys_addr))); because: (a) You have the information you need at allocation time, but you've failed to record that and are trying to use the IOMMU API to reconstruct the CPU virtual address (b) When there isn't an IOMMU present, you assume that bus addresses == physical addresses (c) You assume that the DMA buffer is mapped in the linear mapping that's probably all true for ThunderX/arm64, but it's generally not portable or reliable code. If you could get a handle to the struct page that you allocated in the first place, then you could use page_address to get its virtual address instead of having to go via the physical address. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74
On Thu, Apr 27, 2017 at 05:42:37PM +0100, Mark Rutland wrote: > On Thu, Apr 27, 2017 at 05:16:23PM +0530, Geetha sowjanya wrote: > > + /* > > +* Override the size, for Cavium CN99xx implementations > > +* which doesn't support the page 1 SMMU register space. > > +*/ > > + cpu_model = read_cpuid_id() & MIDR_CPU_MODEL_MASK; > > + if (cpu_model == MIDR_THUNDERX_99XX || > > + cpu_model == MIDR_BRCM_VULCAN) > > + size = SZ_64K; > > If you're trying to identify an SMMU erratum, identify the SMMU, not the > CPU it happens to be paired with this time. > > There are ID registers in the SMMU you can use to do so. > > NAK to using the CPU ID here. Agreed. I had some off-list discussion with Geetha where we agreed to use the "silicon ID", which I assumed was the SMMU IIDR register. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] drivers/of_iommu: ignore SMMU DT nodes with status 'disabled'
Hi Ard,
[+ devicetree@]
On Fri, Apr 14, 2017 at 01:43:15PM +0100, Ard Biesheuvel wrote:
> DT nodes may have a status property, and if they do, such nodes should
> only be considered present if the status property is set to 'okay'.
>
> Currently, we call the init function of IOMMUs described by the device
> tree without taking this into account, which may result in the output
> below on systems where some SMMUs may be legally disabled.
>
> Failed to initialise IOMMU /smb/smmu@e020
> Failed to initialise IOMMU /smb/smmu@e0c0
> arm-smmu e0a0.smmu: probing hardware configuration...
> arm-smmu e0a0.smmu: SMMUv1 with:
> arm-smmu e0a0.smmu: stage 2 translation
> arm-smmu e0a0.smmu: coherent table walk
> arm-smmu e0a0.smmu: stream matching with 32 register groups, mask 0x7fff
> arm-smmu e0a0.smmu: 8 context banks (8 stage-2 only)
> arm-smmu e0a0.smmu: Supported page sizes: 0x60211000
> arm-smmu e0a0.smmu: Stage-2: 40-bit IPA -> 40-bit PA
> Failed to initialise IOMMU /smb/smmu@e060
> Failed to initialise IOMMU /smb/smmu@e080
>
> Since this is not an error condition, only call the init function if
> the device is enabled, which also inhibits the spurious error messages.
>
> Signed-off-by: Ard Biesheuvel
> ---
> drivers/iommu/of_iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 2683e9fc0dcf..2dd1206e6c0d 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -183,7 +183,7 @@ static int __init of_iommu_init(void)
> for_each_matching_node_and_match(np, matches, &match) {
> const of_iommu_init_fn init_fn = match->data;
>
> - if (init_fn(np))
> + if (of_device_is_available(np) && init_fn(np))
> pr_err("Failed to initialise IOMMU %s\n",
> of_node_full_name(np));
> }
Is there a definition of what status = "disabled" is supposed to mean for an
IOMMU? For example, that could mean that the firmware has pre-programmed the
SMMU with particular translations or memory attributes (a bit like the
CCA=1, CPM=1, DACS=0 case in ACPI IORT), or even disabled DMA traffic
altogether.
So I think we'd need an update to the generic IOMMU binding text to say
exactly what the semantics are supposed to be here.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] drivers/of_iommu: ignore SMMU DT nodes with status 'disabled'
On Fri, Apr 28, 2017 at 02:14:49PM +0100, Ard Biesheuvel wrote:
> On 28 April 2017 at 14:11, Will Deacon wrote:
> > Hi Ard,
> >
> > [+ devicetree@]
> >
> > On Fri, Apr 14, 2017 at 01:43:15PM +0100, Ard Biesheuvel wrote:
> >> DT nodes may have a status property, and if they do, such nodes should
> >> only be considered present if the status property is set to 'okay'.
> >>
> >> Currently, we call the init function of IOMMUs described by the device
> >> tree without taking this into account, which may result in the output
> >> below on systems where some SMMUs may be legally disabled.
> >>
> >> Failed to initialise IOMMU /smb/smmu@e020
> >> Failed to initialise IOMMU /smb/smmu@e0c0
> >> arm-smmu e0a0.smmu: probing hardware configuration...
> >> arm-smmu e0a0.smmu: SMMUv1 with:
> >> arm-smmu e0a0.smmu: stage 2 translation
> >> arm-smmu e0a0.smmu: coherent table walk
> >> arm-smmu e0a0.smmu: stream matching with 32 register groups, mask
> >> 0x7fff
> >> arm-smmu e0a0.smmu: 8 context banks (8 stage-2 only)
> >> arm-smmu e0a0.smmu: Supported page sizes: 0x60211000
> >> arm-smmu e0a0.smmu: Stage-2: 40-bit IPA -> 40-bit PA
> >> Failed to initialise IOMMU /smb/smmu@e060
> >> Failed to initialise IOMMU /smb/smmu@e080
> >>
> >> Since this is not an error condition, only call the init function if
> >> the device is enabled, which also inhibits the spurious error messages.
> >>
> >> Signed-off-by: Ard Biesheuvel
> >> ---
> >> drivers/iommu/of_iommu.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> >> index 2683e9fc0dcf..2dd1206e6c0d 100644
> >> --- a/drivers/iommu/of_iommu.c
> >> +++ b/drivers/iommu/of_iommu.c
> >> @@ -183,7 +183,7 @@ static int __init of_iommu_init(void)
> >> for_each_matching_node_and_match(np, matches, &match) {
> >> const of_iommu_init_fn init_fn = match->data;
> >>
> >> - if (init_fn(np))
> >> + if (of_device_is_available(np) && init_fn(np))
> >> pr_err("Failed to initialise IOMMU %s\n",
> >> of_node_full_name(np));
> >> }
> >
> > Is there a definition of what status = "disabled" is supposed to mean for an
> > IOMMU? For example, that could mean that the firmware has pre-programmed the
> > SMMU with particular translations or memory attributes (a bit like the
> > CCA=1, CPM=1, DACS=0 case in ACPI IORT), or even disabled DMA traffic
> > altogether.
> >
> > So I think we'd need an update to the generic IOMMU binding text to say
> > exactly what the semantics are supposed to be here.
> >
>
> I agree that it might make sense to describe the behavior of the IOMMU
> when it is left in the state we found it in. But that is not the same
> as status=disabled.
>
> The DTS subtree contains loads and loads of boilerplate
> configurations, where only some pieces are enabled in the final image
> by setting status=okay. So a node that has status 'disabled' should be
> treated as 'not present', not as 'present but can be ignored under
> assumptions such and such'
>
> In other words, I think we are talking about two different issues here.
I'm not so sure... if we have a master device that has an iommus= property
pointing to an IOMMU with status="disabled", I really don't know whether we
should:
1. Assume the master can do DMA with a 1:1 mapping of memory and no
changes to memory attributes
2. Assume the master can do DMA with a 1:1 mapping of memory, but
potentially with changes to the attributes
3. Assume the master can do DMA, but with some pre-existing translation
(what?)
4. Assume the master can't do DMA
and I also don't know whether the "dma-coherent" property remains valid.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74
Hi Geetha, On Tue, May 02, 2017 at 12:01:15PM +0530, Geetha Akula wrote: > SMMU_IIDR register is broken on T99, that the reason we are using MIDR. Urgh, that's unfortunate. In what way is it broken? > If using MIDR is not accepted, can we enable errata based on SMMU resource > size? > some thing like below. No, you need to get your model number added to IORT after all if the IIDR can't uniqely identify the part. Sorry, Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
On Wed, May 03, 2017 at 06:49:09PM +0530, Sunil Kovvuri wrote:
> On Thu, Apr 27, 2017 at 4:43 PM, wrote:
> > From: Sunil Goutham
> >
> > Modified polling on CMDQ consumer similar to how polling is done for TLB
> > SYNC
> > completion in SMMUv2 driver. Code changes are done with reference to
> >
> > 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively
> >
> > Poll timeout has been increased which addresses issue of 100us timeout not
> > sufficient, when command queue is full with TLB invalidation commands.
> >
> > Signed-off-by: Sunil Goutham
> > Signed-off-by: Geetha
> > ---
> > drivers/iommu/arm-smmu-v3.c | 15 ---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index d412bdd..34599d4 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -379,6 +379,9 @@
> > #define CMDQ_SYNC_0_CS_NONE(0UL << CMDQ_SYNC_0_CS_SHIFT)
> > #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
> >
> > +#define CMDQ_DRAIN_TIMEOUT_US 1000
> > +#define CMDQ_SPIN_COUNT10
> > +
> > /* Event queue */
> > #define EVTQ_ENT_DWORDS4
> > #define EVTQ_MAX_SZ_SHIFT 7
> > @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
> > */
> > static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> > {
> > - ktime_t timeout = ktime_add_us(ktime_get(),
> > ARM_SMMU_POLL_TIMEOUT_US);
> > + ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US);
> > + unsigned int spin_cnt, delay = 1;
> >
> > while (queue_sync_cons(q), (drain ? !queue_empty(q) :
> > queue_full(q))) {
> > if (ktime_compare(ktime_get(), timeout) > 0)
> > @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q,
> > bool drain, bool wfe)
> > if (wfe) {
> > wfe();
> > } else {
> > - cpu_relax();
> > - udelay(1);
> > + for (spin_cnt = 0;
> > +spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
> > + cpu_relax();
> > + continue;
> > + }
> > + udelay(delay);
> > + delay *= 2;
> > }
> > }
> >
> > --
> > 2.7.4
> >
>
> Sorry for the ignorance.
> Is there a patchwork where I can check current status of ARM IOMMU
> related patches ?
>
> And is this patch accepted, if not any comments / feedback ?
Please be patient: the merge window is open and it's not been long since you
posted the patch, which looks pretty bonkers at first glance.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
On Wed, May 03, 2017 at 04:33:57PM +0100, Robin Murphy wrote:
> On 27/04/17 12:13, sunil.kovv...@gmail.com wrote:
> > From: Sunil Goutham
> >
> > Modified polling on CMDQ consumer similar to how polling is done for TLB
> > SYNC
> > completion in SMMUv2 driver. Code changes are done with reference to
> >
> > 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively
> >
> > Poll timeout has been increased which addresses issue of 100us timeout not
> > sufficient, when command queue is full with TLB invalidation commands.
> >
> > Signed-off-by: Sunil Goutham
> > Signed-off-by: Geetha
> > ---
> > drivers/iommu/arm-smmu-v3.c | 15 ---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index d412bdd..34599d4 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -379,6 +379,9 @@
> > #define CMDQ_SYNC_0_CS_NONE(0UL << CMDQ_SYNC_0_CS_SHIFT)
> > #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
> >
> > +#define CMDQ_DRAIN_TIMEOUT_US 1000
> > +#define CMDQ_SPIN_COUNT10
> > +
> > /* Event queue */
> > #define EVTQ_ENT_DWORDS4
> > #define EVTQ_MAX_SZ_SHIFT 7
> > @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
> > */
> > static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
> > {
> > - ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
> > + ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US);
> > + unsigned int spin_cnt, delay = 1;
> >
> > while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
> > if (ktime_compare(ktime_get(), timeout) > 0)
> > @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q,
> > bool drain, bool wfe)
> > if (wfe) {
> > wfe();
> > } else {
> > - cpu_relax();
> > - udelay(1);
> > + for (spin_cnt = 0;
> > +spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
> > + cpu_relax();
> > + continue;
> > + }
> > + udelay(delay);
> > + delay *= 2;
>
> Sorry, I can't make sense of this. The referenced commit uses the spin
> loop to poll opportunistically a few times before delaying. This loop
> just adds a short open-coded udelay to an exponential udelay, and it's
> not really clear that that's any better than a fixed udelay (especially
> as the two cases in which we poll are somewhat different).
>
> What's wrong with simply increasing the timeout value alone?
I asked that the timeout is only increased for the drain case, and that
we fix the issue here where we udelat if cons didn't move immediately:
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/503389.html
but I don't think the patch above actually achieves any of that.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
On Wed, May 03, 2017 at 09:24:13PM +0530, Sunil Kovvuri wrote:
> On Wed, May 3, 2017 at 9:07 PM, Will Deacon wrote:
> > On Wed, May 03, 2017 at 06:49:09PM +0530, Sunil Kovvuri wrote:
> >> On Thu, Apr 27, 2017 at 4:43 PM, wrote:
> >> > From: Sunil Goutham
> >> >
> >> > Modified polling on CMDQ consumer similar to how polling is done for TLB
> >> > SYNC
> >> > completion in SMMUv2 driver. Code changes are done with reference to
> >> >
> >> > 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more
> >> > effectively
> >> >
> >> > Poll timeout has been increased which addresses issue of 100us timeout
> >> > not
> >> > sufficient, when command queue is full with TLB invalidation commands.
> >> >
> >> > Signed-off-by: Sunil Goutham
> >> > Signed-off-by: Geetha
> >> > ---
> >> > drivers/iommu/arm-smmu-v3.c | 15 ---
> >> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> >> > index d412bdd..34599d4 100644
> >> > --- a/drivers/iommu/arm-smmu-v3.c
> >> > +++ b/drivers/iommu/arm-smmu-v3.c
> >> > @@ -379,6 +379,9 @@
> >> > #define CMDQ_SYNC_0_CS_NONE(0UL << CMDQ_SYNC_0_CS_SHIFT)
> >> > #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
> >> >
> >> > +#define CMDQ_DRAIN_TIMEOUT_US 1000
> >> > +#define CMDQ_SPIN_COUNT10
> >> > +
> >> > /* Event queue */
> >> > #define EVTQ_ENT_DWORDS4
> >> > #define EVTQ_MAX_SZ_SHIFT 7
> >> > @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
> >> > */
> >> > static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool
> >> > wfe)
> >> > {
> >> > - ktime_t timeout = ktime_add_us(ktime_get(),
> >> > ARM_SMMU_POLL_TIMEOUT_US);
> >> > + ktime_t timeout = ktime_add_us(ktime_get(),
> >> > CMDQ_DRAIN_TIMEOUT_US);
> >> > + unsigned int spin_cnt, delay = 1;
> >> >
> >> > while (queue_sync_cons(q), (drain ? !queue_empty(q) :
> >> > queue_full(q))) {
> >> > if (ktime_compare(ktime_get(), timeout) > 0)
> >> > @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue
> >> > *q, bool drain, bool wfe)
> >> > if (wfe) {
> >> > wfe();
> >> > } else {
> >> > - cpu_relax();
> >> > - udelay(1);
> >> > + for (spin_cnt = 0;
> >> > +spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) {
> >> > + cpu_relax();
> >> > + continue;
> >> > + }
> >> > + udelay(delay);
> >> > + delay *= 2;
> >> > }
> >> > }
> >> >
> >> > --
> >> > 2.7.4
> >> >
> >>
> >> Sorry for the ignorance.
> >> Is there a patchwork where I can check current status of ARM IOMMU
> >> related patches ?
> >>
> >> And is this patch accepted, if not any comments / feedback ?
> >
> > Please be patient: the merge window is open and it's not been long since you
> > posted the patch, which looks pretty bonkers at first glance.
> >
> > Will
>
> Look at this
> https://lkml.org/lkml/2017/4/3/605
> The same thing, i pinged after a week and you said you already picked it up.
> All I am asking is how do i know the current status, how many days
> would normally
> be considered being patient ?
At least wait until the merge window is over if it's not a fix, or keep an
eye on the relevant branches (see below).
> Instead of troubling you, is there a patchwork where i can check the status ?
No, but I pick patches up on my iommu/devel branch here:
https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/
and at some point they appear on for-joerg/arm-smmu/updates, which I send
to Joerg (who is the iommu maintainer). He then puts them into linux-next
before they get sent for inclusion in mainline during the next merge window.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/7] ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.
On Fri, May 05, 2017 at 07:56:17AM -0700, David Daney wrote:
> On 05/05/2017 06:53 AM, Hanjun Guo wrote:
> >On 2017/5/5 20:08, Geetha sowjanya wrote:
> >>From: Linu Cherian
> >>
> >>Add SMMUv3 model definition for ThunderX2.
> >>
> >>Signed-off-by: Linu Cherian
> >>Signed-off-by: Geetha Sowjanya
> >>---
> >> include/acpi/actbl2.h | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >>diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> >>index faa9f2c..76a6f5d 100644
> >>--- a/include/acpi/actbl2.h
> >>+++ b/include/acpi/actbl2.h
> >>@@ -779,6 +779,8 @@ struct acpi_iort_smmu {
> >> #define ACPI_IORT_SMMU_CORELINK_MMU400 0x0002/* ARM Corelink
> >>MMU-400 */
> >> #define ACPI_IORT_SMMU_CORELINK_MMU500 0x0003/* ARM Corelink
> >>MMU-500 */
> >>
> >>+#define ACPI_IORT_SMMU_V3_CAVIUM_CN99XX 0x0002 /* Cavium ThunderX2
> >>SMMUv3 */
> >
> >There are some other model numbers in the unreleased spec,
> >I think we need to wait for the updated IORT spec to
> >be released.
> >
>
> ... or if we are fairly confident that the identifier will not need to
> change, we can merge this as is and establish a de facto specification that
> the Real IORT specification will then be forced to follow.
>
> Is there anything other than bureaucratic inertia holding up the real
> specification?
My understanding is that IORT is going to be published imminently (i.e.
before the next kernel release), so it makes sense to wait rather than fork
the spec.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [v5 1/4] ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.
On Thu, May 11, 2017 at 02:26:02AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, May 10, 2017 05:01:55 PM Geetha sowjanya wrote:
> > From: Linu Cherian
> >
> > Add SMMUv3 model definition for ThunderX2.
> >
> > Signed-off-by: Linu Cherian
> > Signed-off-by: Geetha Sowjanya
>
> This is an ACPICA change, but you have not included the ACPICA maintainers
> into your original CC list (added now).
>
> Bob, Lv, how should this be routed?
>
> Do you want to apply this patch upstream first or can we make this change in
> Linux and upstream in parallel? That shouldn't be a big deal, right?
I think we're still waiting for the updated IORT document to be published (I
think this should be in the next week or so), so I don't think we should
commit the new ID before that happens.
Will
> > ---
> > include/acpi/actbl2.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> > index faa9f2c..76a6f5d 100644
> > --- a/include/acpi/actbl2.h
> > +++ b/include/acpi/actbl2.h
> > @@ -779,6 +779,8 @@ struct acpi_iort_smmu {
> > #define ACPI_IORT_SMMU_CORELINK_MMU400 0x0002 /* ARM Corelink MMU-400
> > */
> > #define ACPI_IORT_SMMU_CORELINK_MMU500 0x0003 /* ARM Corelink MMU-500
> > */
> >
> > +#define ACPI_IORT_SMMU_V3_CAVIUM_CN99XX 0x0002 /* Cavium ThunderX2
> > SMMUv3 */
> > +
> > /* Masks for Flags field above */
> >
> > #define ACPI_IORT_SMMU_DVM_SUPPORTED(1)
> >
>
> Thanks,
> Rafael
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [v5 1/4] ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.
On Thu, May 11, 2017 at 04:40:51PM +0200, Rafael J. Wysocki wrote: > On Thursday, May 11, 2017 09:45:25 AM Will Deacon wrote: > > On Thu, May 11, 2017 at 02:26:02AM +0200, Rafael J. Wysocki wrote: > > > On Wednesday, May 10, 2017 05:01:55 PM Geetha sowjanya wrote: > > > > From: Linu Cherian > > > > > > > > Add SMMUv3 model definition for ThunderX2. > > > > > > > > Signed-off-by: Linu Cherian > > > > Signed-off-by: Geetha Sowjanya > > > > > > This is an ACPICA change, but you have not included the ACPICA maintainers > > > into your original CC list (added now). > > > > > > Bob, Lv, how should this be routed? > > > > > > Do you want to apply this patch upstream first or can we make this change > > > in > > > Linux and upstream in parallel? That shouldn't be a big deal, right? > > > > I think we're still waiting for the updated IORT document to be published (I > > think this should be in the next week or so), so I don't think we should > > commit the new ID before that happens. > > OK, thanks for the heads-up. > > I think it's better to submit the actbl2.h update directly to ACPICA > upstream when the doc is published and then work on top of that. The doc is now published: http://infocenter.arm.com/help/topic/com.arm.doc.den0049c/DEN0049C_IO_Remapping_Table.pdf so the new model numbers are confirmed. Cheers, Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Sricharan, On Wed, May 03, 2017 at 03:54:59PM +0530, Sricharan R wrote: > On 5/3/2017 3:24 PM, Robin Murphy wrote: > > On 02/05/17 19:35, Geert Uytterhoeven wrote: > >> On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R > >> wrote: > >>> From: Laurent Pinchart > >>> > >>> Failures to look up an IOMMU when parsing the DT iommus property need to > >>> be handled separately from the .of_xlate() failures to support deferred > >>> probing. > >>> > >>> The lack of a registered IOMMU can be caused by the lack of a driver for > >>> the IOMMU, the IOMMU device probe not having been performed yet, having > >>> been deferred, or having failed. > >>> > >>> The first case occurs when the device tree describes the bus master and > >>> IOMMU topology correctly but no device driver exists for the IOMMU yet > >>> or the device driver has not been compiled in. Return NULL, the caller > >>> will configure the device without an IOMMU. > >>> > >>> The second and third cases are handled by deferring the probe of the bus > >>> master device which will eventually get reprobed after the IOMMU. > >>> > >>> The last case is currently handled by deferring the probe of the bus > >>> master device as well. A mechanism to either configure the bus master > >>> device without an IOMMU or to fail the bus master device probe depending > >>> on whether the IOMMU is optional or mandatory would be a good > >>> enhancement. > >>> > >>> Tested-by: Marek Szyprowski > >>> Signed-off-by: Laurent Pichart > >>> Signed-off-by: Sricharan R > >> > >> This patch broke Renesas R-Car Gen3 platforms in renesas-drivers. > >> As the IOMMU nodes in DT are not yet enabled, all devices having iommus > >> properties in DT now fail to probe. > > > > How exactly do they fail to probe? Per d7b0558230e4, if there are no ops > > registered then they should merely defer until we reach the point of > > giving up and ignoring the IOMMU. Is it just that you have no other > > late-probing drivers or post-init module loads to kick the deferred > > queue after that point? I did try to find a way to explicitly kick it > > from a suitably late initcall, but there didn't seem to be any obvious > > public interface - anyone have any suggestions? > > > > I think that's more of a general problem with the probe deferral > > mechanism itself (I've seen the same thing happen with some of the > > CoreSight stuff on Juno due to the number of inter-component > > dependencies) rather than any specific fault of this series. > > > > I was thinking of an additional check like below to avoid the > situation ? > > From 499b6e662f60f23740b8880882b0a16f16434501 Mon Sep 17 00:00:00 2001 > From: Sricharan R > Date: Wed, 3 May 2017 13:16:59 +0530 > Subject: [PATCH] iommu: of: Fix check for returning EPROBE_DEFER > > While returning EPROBE_DEFER for iommu masters > take in to account of iommu nodes that could be > marked in DT as 'status=disabled', in which case > simply return NULL and let the master's probe > continue rather than deferring. > > Signed-off-by: Sricharan R > --- > drivers/iommu/of_iommu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 9f44ee8..e6e9bec 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -118,6 +118,7 @@ static bool of_iommu_driver_present(struct device_node > *np) > > ops = iommu_ops_from_fwnode(fwnode); > if ((ops && !ops->of_xlate) || > + !of_device_is_available(iommu_spec->np) || > (!ops && !of_iommu_driver_present(iommu_spec->np))) > return NULL; Without this patch, v4.12-rc1 hangs on my Juno waiting to mount the root filesystem. The problem is that the USB controller is behind an SMMU which is marked as 'status = "disabled"' in the devicetree. Whilst there was a separate thread with Ard about exactly what this means in terms of the DMA ops used by upstream devices, your patch above fixes the regression and I think should go in regardless. The DMA ops issue will likely require an additional DT binding anyway, to advertise the behaviour of the IOMMU when it is disabled. Tested-by: Will Deacon Acked-by: Will Deacon Could you resend it as a proper patch, please? Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/arm-smmu-v3: Increase CMDQ drain timeout value
Hi Sunil, On Mon, May 29, 2017 at 02:41:59PM +0530, Sunil Kovvuri wrote: > On Fri, May 5, 2017 at 4:47 PM, wrote: > > From: Sunil Goutham > > > > Processing queue full of TLB invalidation commands might > > take more time on some platforms than current timeout > > of 100us. So increased drain timeout value. > > > > Also now udelay time is increased exponentially for each poll. > > > > Signed-off-by: Sunil Goutham [...] > Gentle reminder for patch review. Sorry, I forgot to push this out. Should be on my iommu/devel branch now. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Device address specific mapping of arm,mmu-500
On Mon, May 29, 2017 at 06:18:45PM -0700, Ray Jui wrote: > I'm writing to check with you to see if the latest arm-smmu.c driver in > v4.12-rc Linux for smmu-500 can support mapping that is only specific to > a particular physical address range while leave the rest still to be > handled by the client device. I believe this can already be supported by > the device tree binding of the generic IOMMU framework; however, it is > not clear to me whether or not the arm-smmu.c driver can support it. > > To give you some background information: > > We have a SoC that has PCIe root complex that has a build-in logic block > to forward MSI writes to ARM GICv3 ITS. Unfortunately, this logic block > has a HW bug that causes the MSI writes not parsed properly and can > potentially corrupt data in the internal FIFO. A workaround is to have > ARM MMU-500 takes care of all inbound transactions. I found that is > working after hooking up our PCIe root complex to MMU-500; however, even > with this optimized arm-smmu driver in v4.12, I'm still seeing a > significant Ethernet throughput drop in both the TX and RX directions. > The throughput drop is very significant at around 50% (but is already > much improved compared to other prior kernel versions at 70~90%). Did Robin's experiments help at all with this? http://www.linux-arm.org/git?p=linux-rm.git;a=shortlog;h=refs/heads/iommu/perf > One alternative is to only use MMU-500 for MSI writes towards > GITS_TRANSLATER register in the GICv3, i.e., if I can define a specific > region of physical address that I want MMU-500 to act on and leave the > rest of inbound transactions to be handled directly by our PCIe > controller, it can potentially work around the HW bug we have and at the > same time achieve optimal throughput. I don't think you can bypass the SMMU for MSIs unless you give them their own StreamIDs, which is likely to break things horribly in the kernel. You could try to create an identity mapping, but you'll still have the translation overhead and you'd probably end up having to supply your own DMA ops to manage the address space. I'm assuming that you need to prevent the physical address of the ITS from being allocated as an IOVA? > Any feedback from you is greatly appreciated! Fix the hardware ;) Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Device address specific mapping of arm,mmu-500
On Tue, May 30, 2017 at 11:13:36PM -0700, Ray Jui wrote: > I did a little more digging myself and I think I now understand what you > meant by identity mapping, i.e., configuring the MMU-500 with 1:1 mapping > between the DMA address and the IOVA address. > > I think that should work. In the end, due to this MSI write parsing issue in > our PCIe controller, the reason to use IOMMU is to allow the cache > attributes (AxCACHE) of the MSI writes towards GICv3 ITS to be modified by > the IOMMU to be device type, while leaving the rest of inbound reads/writes > from/to DDR with more optimized cache attributes setting, to allow I/O > coherency to be still enabled for the PCIe controller. In fact, the PCIe > controller itself is fully capable of DMA to/from the full address space of > our SoC including both DDR and any device memory. > > The 1:1 mapping will still pose some translation overhead like you > suggested; however, the overhead of allocating page tables and locking will > be gone. This sounds like the best possible option I have currently. It might end up being pretty invasive to work around a hardware bug, so we'll have to see what it looks like. Ideally, we could just use the SMMU for everything as-is and work on clawing back the lost performance (it should be possible to get ~95% of the perf if we sort out the locking, which we *are* working on). > May I ask, how do I start to try to get this identity mapping to work as an > experiment and proof of concept? Any pointer or advise is highly appreciated > as you can see I'm not very experienced with this. I found Will recently > added the IOMMU_DOMAIN_IDENTITY support to the arm-smmu driver. But I > suppose that is to bypass the SMMU completely, instead of still going > through the MMU with 1:1 translation. Is my understanding correct? Yes, I don't think IOMMU_DOMAIN_IDENTITY is what you need because you actally need per-page control of memory attributes. Robin might have a better idea, but I think you'll have to hack dma-iommu.c so that you can have a version of the DMA ops that: * Initialises the identity map (I guess as normal WB cacheable?) * Reserves and maps the MSI region appropriately * Just returns the physical address for the dma address for map requests (return error for the MSI region) * Does nothing for unmap requests But my strong preference would be to fix the locking overhead from the SMMU so that the perf hit is acceptable. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 3/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126
Hi Geetha,
On Tue, May 30, 2017 at 05:33:41PM +0530, Geetha sowjanya wrote:
> From: Geetha Sowjanya
>
> Cavium ThunderX2 SMMU doesn't support MSI and also doesn't have unique irq
> lines for gerror, eventq and cmdq-sync.
>
> This patch addresses the issue by checking if any interrupt sources are
> using same irq number, then they are registered as shared irqs.
>
> Signed-off-by: Geetha Sowjanya
> ---
> Documentation/arm64/silicon-errata.txt |1 +
> drivers/iommu/arm-smmu-v3.c| 29 +
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/arm64/silicon-errata.txt
> b/Documentation/arm64/silicon-errata.txt
> index 4693a32..42422f6 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -63,6 +63,7 @@ stable kernels.
> | Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456
> |
> | Cavium | ThunderX SMMUv2 | #27704 | N/A
> |
> | Cavium | ThunderX2 SMMUv3| #74 | N/A
> |
> +| Cavium | ThunderX2 SMMUv3| #126| N/A
> |
> || | |
> |
> | Freescale/NXP | LS2080A/LS1043A | A-008585| FSL_ERRATUM_A008585
> |
> || | |
> |
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 4e80205..d2db01f 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2232,6 +2232,25 @@ static void arm_smmu_setup_msis(struct arm_smmu_device
> *smmu)
> devm_add_action(dev, arm_smmu_free_msis, dev);
> }
>
> +static int get_irq_flags(struct arm_smmu_device *smmu, int irq)
> +{
> + int match_count = 0;
> +
> + if (irq == smmu->evtq.q.irq)
> + match_count++;
> + if (irq == smmu->cmdq.q.irq)
> + match_count++;
> + if (irq == smmu->gerr_irq)
> + match_count++;
> + if (irq == smmu->priq.q.irq)
> + match_count++;
> +
> + if (match_count > 1)
> + return IRQF_SHARED | IRQF_ONESHOT;
> +
> + return IRQF_ONESHOT;
> +}
I really think this is the wrong way of solving the problem: using
IRQF_SHARED has implications elsewhere in the driver (for example, we must
then pass a unique dev_id otherwise freeing the IRQs won't work properly)
and I don't want to have to worry about these constraints just because of
this broken platform.
Please do what I suggested instead: register a single threaded interrupt
handler that acts as a multiplexer and manually calls the other routines.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 3/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126
On Tue, Jun 20, 2017 at 07:47:39PM +0530, Geetha sowjanya wrote:
> From: Geetha Sowjanya
>
> Cavium ThunderX2 SMMU doesn't support MSI and also doesn't have unique irq
> lines for gerror, eventq and cmdq-sync.
>
> SHARED_IRQ option is set as a errata workaround, which allows to share the irq
> line by register single irq handler for all the interrupts.
>
> Signed-off-by: Geetha sowjanya
> ---
> .../devicetree/bindings/iommu/arm,smmu-v3.txt |5 ++
> drivers/iommu/arm-smmu-v3.c| 73
>
> 2 files changed, 64 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> index 6ecc48c..44b40e0 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> @@ -55,6 +55,11 @@ the PCIe specification.
> Set for Caviun ThunderX2 silicon that doesn't support
> SMMU page1 register space.
>
> +- cavium,cn9900-broken-unique-irqline
> +: Use single irq line for all the SMMUv3 interrupts.
> + Set for Caviun ThunderX2 silicon that doesn't support
> + MSI and also doesn't have unique irq lines for gerror,
> + eventq and cmdq-sync.
I think we're better off just supporting a new (optional) named interrupt
as "combined", and then allowing that to be used instead of the others.
> ** Example
>
> smmu@2b40 {
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 2dea4a9..6c0c632 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -598,6 +598,7 @@ struct arm_smmu_device {
>
> #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0)
> #define ARM_SMMU_OPT_PAGE0_REGS_ONLY (1 << 1)
> +#define ARM_SMMU_OPT_SHARED_IRQ (1 << 2)
Please call this COMBINED instead of SHARED (similarly elsewhere). That
said, not sure we need this.
> u32 options;
>
> struct arm_smmu_cmdqcmdq;
> @@ -665,6 +666,7 @@ struct arm_smmu_option_prop {
> static struct arm_smmu_option_prop arm_smmu_options[] = {
> { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
> { ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
> + { ARM_SMMU_OPT_SHARED_IRQ, "cavium,cn9900-broken-unique-irqline"},
> { 0, NULL},
> };
>
> @@ -1313,6 +1315,21 @@ static irqreturn_t arm_smmu_gerror_handler(int irq,
> void *dev)
> writel(gerror, smmu->base + ARM_SMMU_GERRORN);
> return IRQ_HANDLED;
> }
> +/* Shared irq handler*/
> +static irqreturn_t arm_smmu_shared_irq_thread(int irq, void *dev)
> +{
> + struct arm_smmu_device *smmu = dev;
> + irqreturn_t ret;
> +
> + ret = arm_smmu_gerror_handler(irq, dev);
> + if (ret == IRQ_NONE) {
> + arm_smmu_evtq_thread(irq, dev);
> + arm_smmu_cmdq_sync_handler(irq, dev);
> + if (smmu->features & ARM_SMMU_FEAT_PRI)
> + arm_smmu_priq_thread(irq, dev);
> + }
> + return IRQ_HANDLED;
> +}
This isn't quite right, because you're now running low-level handlers (like
the gerror handler) in threaded context. You're better off registering a
low-level handler too (see below) which can kick gerror and cmdq_sync
before unconditionally returning IRQ_WAKE_THREAD.
>
> /* IO_PGTABLE API */
> static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
> @@ -2230,18 +2247,9 @@ static void arm_smmu_setup_msis(struct arm_smmu_device
> *smmu)
> devm_add_action(dev, arm_smmu_free_msis, dev);
> }
>
> -static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> +static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
> {
> - int ret, irq;
> - u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> -
> - /* Disable IRQs first */
> - ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> - ARM_SMMU_IRQ_CTRLACK);
> - if (ret) {
> - dev_err(smmu->dev, "failed to disable irqs\n");
> - return ret;
> - }
> + int irq, ret;
>
> arm_smmu_setup_msis(smmu);
>
> @@ -2284,10 +2292,46 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device
> *smmu)
> if (ret < 0)
> dev_warn(smmu->dev,
>"failed to enable priq irq\n");
> - else
> - irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
> }
> }
> +}
> +
> +static void arm_smmu_setup_shared_irqs(struct arm_smmu_device *smmu)
> +{
> + int ret, irq;
> +
> + /* Single irq is used for all queues, request single interrupt lines */
> + irq = smmu->evtq.q.irq;
> + if (irq) {
> + ret = devm_request_threaded_irq(smmu->dev,
Re: [PATCH v8 1/3] ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3 model
On Tue, Jun 20, 2017 at 07:47:37PM +0530, Geetha sowjanya wrote: > From: Linu Cherian > > Cavium ThunderX2 implementation doesn't support second page in SMMU > register space. Hence, resource size is set as 64k for this model. > > Signed-off-by: Linu Cherian > Signed-off-by: Geetha Sowjanya > --- > drivers/acpi/arm64/iort.c | 15 ++- > 1 files changed, 14 insertions(+), 1 deletions(-) Looks fine to me, but I need Lorenzo's ack on this. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
