* [PATCH] arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168
2016-10-22 4:54 ` [PATCH] arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168 Geetha sowjanya
@ 2016-10-24 11:28 ` Robin Murphy
2016-10-24 13:44 ` Marc Zyngier
1 sibling, 0 replies; 4+ messages in thread
From: Robin Murphy @ 2016-10-24 11:28 UTC (permalink / raw)
To: linux-arm-kernel
[+Thomas, Jason, Marc]
On 22/10/16 05:54, Geetha sowjanya wrote:
> From: Tirumalesh Chalamarla <Tirumalesh.Chalamarla@cavium.com>
>
> This patch implements Cavium ThunderX erratum 28168.
>
> PCI requires stores complete in order. 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 transactions are
> completed.
>
> Signed-off-by: Tirumalesh Chalamarla <Tirumalesh.Chalamarla@cavium.com>
> Signed-off-by: Geetha sowjanya <gakula@caviumnetworks.com>
> ---
> arch/arm64/Kconfig | 11 +++++++++++
> drivers/iommu/arm-smmu.c | 38 ++++++++++++++++++++++++++++++++++++++
> drivers/irqchip/irq-gic-common.h | 1 +
> drivers/irqchip/irq-gic-v3-its.c | 22 ++++++++++++++++++++++
> kernel/irq/chip.c | 4 ++++
The irqchip maintainers should absolutely be CC'ed on this. Please use
get_maintainer.pl to check.
> 5 files changed, 76 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 30398db..57f5c9b 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -474,6 +474,17 @@ config CAVIUM_ERRATUM_27456
>
> If unsure, say Y.
>
> +config CAVIUM_ERRATUM_28168
> + bool "Cavium erratum 28168: Make sure ITS and SMMU TLB are in sync"
> + default y
> + help
> + 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 transactions are completed.
Ouch! Is there definitely no other possible workaround, like overriding
transaction attributes, disabling hit-under-miss handling, or somesuch?
Does it have to be a global sync, or is syncing the relevant context
bank sufficient? (I have some half-finished patches to split up the TLB
maintenance ops, including finer-grained syncs wherever possible, which
I can resurrect).
> +
> + If unsure, say Y.
> +
> endmenu
>
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 9740846..20a61c6 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -378,6 +378,7 @@ struct arm_smmu_device {
> unsigned int *irqs;
>
> u32 cavium_id_base; /* Specific to Cavium */
> + spinlock_t tlb_lock;
> };
>
> enum arm_smmu_context_fmt {
> @@ -576,9 +577,39 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
> static void arm_smmu_tlb_sync(void *cookie)
> {
> struct arm_smmu_domain *smmu_domain = cookie;
> + struct arm_smmu_device *smmu = smmu_domain->smmu;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&smmu->tlb_lock, flags);
So *every* unmap operation on any SMMU ever now has to poll some slow
hardware for up to a second with IRQs disabled unconditionally? I don't
think that's acceptable for the general case.
> __arm_smmu_tlb_sync(smmu_domain->smmu);
> + spin_unlock_irqrestore(&smmu->tlb_lock, flags)
> }
>
> +/*
> + * 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 transactions are completed.
> + *
> + */
> +void cavium_smmu_tlb_sync(struct device *dev)
> +{
> + struct arm_smmu_device *smmu;
> + struct arm_smmu_master_cfg *cfg;
> + unsigned long flags;
> +
> + smmu = find_smmu_for_device(dev);
Won't compile - that function no longer exists.
> + if (!smmu)
> + return;
> +
> + spin_lock_irqsave(&smmu->tlb_lock, flags);
> + __arm_smmu_tlb_sync(smmu);
> + spin_unlock_irqrestore(&smmu->tlb_lock, flags)
> +}
> +EXPORT_SYMBOL(cavium_smmu_tlb_sync);
> +
> static void arm_smmu_tlb_inv_context(void *cookie)
> {
> struct arm_smmu_domain *smmu_domain = cookie;
> @@ -586,6 +617,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
> struct arm_smmu_device *smmu = smmu_domain->smmu;
> bool stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
> void __iomem *base;
> + unsigned long flags;
>
> if (stage1) {
> base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx);
> @@ -597,7 +629,9 @@ static void arm_smmu_tlb_inv_context(void *cookie)
> base + ARM_SMMU_GR0_TLBIVMID);
> }
>
> + spin_lock_irqsave(&smmu->tlb_lock, flags);
> __arm_smmu_tlb_sync(smmu);
> + spin_unlock_irqrestore(&smmu->tlb_lock, flags)
> }
>
> static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
> @@ -1562,6 +1596,7 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> void __iomem *cb_base;
> int i;
> + unsigned long flags;
> u32 reg, major;
>
> /* clear global FSR */
> @@ -1633,7 +1668,9 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> reg |= sCR0_VMID16EN;
>
> /* Push the button */
> + spin_lock_irqsave(&smmu->tlb_lock, flags)
> __arm_smmu_tlb_sync(smmu);
> + spin_unlock_irqrestore(&smmu->tlb_lock, flags)
After the fourth identical wrapping of the same function call, does it
not start to seem a better idea to implement a workaround *inside*
__arm_smmu_tlb_sync()?
> writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> }
>
> @@ -2001,6 +2038,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
> }
> }
>
> + spin_lock_init(&smmu->tlb_lock);
> of_iommu_set_ops(dev->of_node, &arm_smmu_ops);
> platform_set_drvdata(pdev, smmu);
> arm_smmu_device_reset(smmu);
> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> index 205e5fd..0228ba0 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -38,4 +38,5 @@ void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>
> void gic_set_kvm_info(const struct gic_kvm_info *info);
>
> +void cavium_smmu_tlb_sync(void *iommu);
> #endif /* _IRQ_GIC_COMMON_H */
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 003495d..88e9958 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -112,6 +112,7 @@ struct its_device {
> struct its_node *its;
> struct event_lpi_map event_map;
> void *itt;
> + struct device *dev;
> u32 nr_ites;
> u32 device_id;
> };
> @@ -664,10 +665,29 @@ static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
> iommu_dma_map_msi_msg(d->irq, msg);
> }
>
> +/**
> + * Due to erratum in ThunderX,
> + * we need to make sure SMMU is in sync with ITS translations.
> + **/
> +static void its_ack_irq(struct irq_data *d)
> +{
> + struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> + struct pci_dev *pdev;
> +
> + if (!dev_is_pci(its_dev->dev))
> + return;
> +
> + pdev = to_pci_dev(its_dev->dev);
> + if (pdev->vendor != 0x177d)
> + cavium_smmu_tlb_sync(its_dev->dev);
I'm pretty sure that makes any kernel built without CONFIG_ARM_SMMU fail
to link.
Robin.
> +
> +}
> +
> static struct irq_chip its_irq_chip = {
> .name = "ITS",
> .irq_mask = its_mask_irq,
> .irq_unmask = its_unmask_irq,
> + .irq_ack = its_ack_irq,
> .irq_eoi = irq_chip_eoi_parent,
> .irq_set_affinity = its_set_affinity,
> .irq_compose_msi_msg = its_irq_compose_msi_msg,
> @@ -1422,6 +1442,8 @@ static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
> if (!its_dev)
> return -ENOMEM;
>
> + its_dev->dev = dev;
> +
> pr_debug("ITT %d entries, %d bits\n", nvec, ilog2(nvec));
> out:
> info->scratchpad[0].ptr = its_dev;
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index be3c34e..6add8da 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -585,6 +585,10 @@ void handle_fasteoi_irq(struct irq_desc *desc)
> goto out;
> }
>
> +#ifdef CONFIG_CAVIUM_ERRATUM_28168
> + if (chip->irq_ack)
> + chip->irq_ack(&desc->irq_data);
> +#endif
> kstat_incr_irqs_this_cpu(desc);
> if (desc->istate & IRQS_ONESHOT)
> mask_irq(desc);
>
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168
2016-10-22 4:54 ` [PATCH] arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168 Geetha sowjanya
2016-10-24 11:28 ` Robin Murphy
@ 2016-10-24 13:44 ` Marc Zyngier
2016-10-24 20:54 ` Thomas Gleixner
1 sibling, 1 reply; 4+ messages in thread
From: Marc Zyngier @ 2016-10-24 13:44 UTC (permalink / raw)
To: linux-arm-kernel
Geetha,
On 22/10/16 05:54, Geetha sowjanya wrote:
> From: Tirumalesh Chalamarla <Tirumalesh.Chalamarla@cavium.com>
>
> This patch implements Cavium ThunderX erratum 28168.
>
> PCI requires stores complete in order. 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 transactions are
> completed.
>
> Signed-off-by: Tirumalesh Chalamarla <Tirumalesh.Chalamarla@cavium.com>
> Signed-off-by: Geetha sowjanya <gakula@caviumnetworks.com>
> ---
> arch/arm64/Kconfig | 11 +++++++++++
> drivers/iommu/arm-smmu.c | 38 ++++++++++++++++++++++++++++++++++++++
> drivers/irqchip/irq-gic-common.h | 1 +
> drivers/irqchip/irq-gic-v3-its.c | 22 ++++++++++++++++++++++
> kernel/irq/chip.c | 4 ++++
Thanks Robin for looping me in. Geetha, please use get_maintainers.pl to
keep the relevant people on CC, specially as you're touching some of the
core infrastructure.
> 5 files changed, 76 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 30398db..57f5c9b 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -474,6 +474,17 @@ config CAVIUM_ERRATUM_27456
>
> If unsure, say Y.
>
> +config CAVIUM_ERRATUM_28168
> + bool "Cavium erratum 28168: Make sure ITS and SMMU TLB are in sync"
> + default y
> + help
> + 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 transactions are completed.
> +
> + If unsure, say Y.
Please add an entry to Documentation/arm64/silicon-errata.txt.
> +
> endmenu
>
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 9740846..20a61c6 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
I'll skip the SMMU code on which Robin has commented already, and move
to the irq part, which is equally entertaining.
[...]
> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> index 205e5fd..0228ba0 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -38,4 +38,5 @@ void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>
> void gic_set_kvm_info(const struct gic_kvm_info *info);
>
> +void cavium_smmu_tlb_sync(void *iommu);
> #endif /* _IRQ_GIC_COMMON_H */
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 003495d..88e9958 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -112,6 +112,7 @@ struct its_device {
> struct its_node *its;
> struct event_lpi_map event_map;
> void *itt;
> + struct device *dev;
This doesn't work in the presence of anything that will multiplex
multiple RequesterIDs onto a single DeviceID (non transparent PCI
bridge, for example).
> u32 nr_ites;
> u32 device_id;
> };
> @@ -664,10 +665,29 @@ static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
> iommu_dma_map_msi_msg(d->irq, msg);
> }
>
> +/**
> + * Due to erratum in ThunderX,
> + * we need to make sure SMMU is in sync with ITS translations.
> + **/
> +static void its_ack_irq(struct irq_data *d)
> +{
> + struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> + struct pci_dev *pdev;
> +
> + if (!dev_is_pci(its_dev->dev))
> + return;
How about non PCI devices?
> +
> + pdev = to_pci_dev(its_dev->dev);
> + if (pdev->vendor != 0x177d)
> + cavium_smmu_tlb_sync(its_dev->dev);
What makes Cavium devices so special that they do not need to respect
the PCI memory ordering with respect to MSI delivery?
> +
> +}
> +
> static struct irq_chip its_irq_chip = {
> .name = "ITS",
> .irq_mask = its_mask_irq,
> .irq_unmask = its_unmask_irq,
> + .irq_ack = its_ack_irq,
Nice try, but no, thank you. If you really want to go down that road,
have a look at CONFIG_IRQ_PREFLOW_FASTEOI, and make this workaround a
per interrupt thing. At least, you won't pollute the core code with
another hack.
Also, it would be good to find a way for that hack to be confined to the
SMMU driver, since that's where the oddity is being handled. Something
that would occur when the device is mapping memory is probably on good spot.
> .irq_eoi = irq_chip_eoi_parent,
> .irq_set_affinity = its_set_affinity,
> .irq_compose_msi_msg = its_irq_compose_msi_msg,
> @@ -1422,6 +1442,8 @@ static int its_msi_prepare(struct irq_domain *domain, struct device *dev,
> if (!its_dev)
> return -ENOMEM;
>
> + its_dev->dev = dev;
> +
> pr_debug("ITT %d entries, %d bits\n", nvec, ilog2(nvec));
> out:
> info->scratchpad[0].ptr = its_dev;
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index be3c34e..6add8da 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -585,6 +585,10 @@ void handle_fasteoi_irq(struct irq_desc *desc)
> goto out;
> }
>
> +#ifdef CONFIG_CAVIUM_ERRATUM_28168
> + if (chip->irq_ack)
> + chip->irq_ack(&desc->irq_data);
> +#endif
> kstat_incr_irqs_this_cpu(desc);
> if (desc->istate & IRQS_ONESHOT)
> mask_irq(desc);
>
Overall, this workaround is not acceptable as it is. You need to find
ways to make it less invasive, and hopefully the above pointers will
help. Please keep the current distribution list posted once you update
this patch.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 4+ messages in thread