Re: [PATCH v2] arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168

2016-11-16 Thread Marc Zyngier 
On 15/11/16 18:24, David Daney wrote:
> On 11/15/2016 01:26 AM, Marc Zyngier wrote:
>> On 15/11/16 07:00, Geetha sowjanya wrote:
>>> From: Tirumalesh Chalamarla 
>>>
>>>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 data transfers are
>>>completed before invoking ISR.
>>>
>>> Signed-off-by: Tirumalesh Chalamarla 
>>> Signed-off-by: Geetha sowjanya 
> [...]
>>> --- a/drivers/irqchip/irq-gic-v3.c
>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>> @@ -28,6 +28,8 @@
>>>   #include 
>>>   #include 
>>>   #include 
>>> +#include 
>>> +#include 
>>>
>>>   #include 
>>>   #include 
>>> @@ -736,6 +738,20 @@ static inline void gic_cpu_pm_init(void) { }
>>>
>>>   #define GIC_ID_NR (1U << gic_data.rdists.id_bits)
>>>
>>> +/*
>>> + * Due to #28168 erratum in ThunderX,
>>> + * we need to make sure DMA data transfer is done before MSIX.
>>> + */
>>> +static void cavium_irq_perflow_handler(struct irq_data *data)
>>> +{
>>> +   struct pci_dev *pdev;
>>> +
>>> +   pdev = msi_desc_to_pci_dev(irq_data_get_msi_desc(data));
>>
>> What happens if this is not a PCI device?
>>
>>> +   if ((pdev->vendor != 0x177d) &&
>>> +   ((pdev->device & 0xA000) != 0xA000))
>>> +   cavium_arm_smmu_tlb_sync(>dev);
>>
>> I've asked that before. What makes Cavium devices so special that they
>> are not sensitive to this bug?
> 
> 
> This is a heuristic for devices connected to external PCIe buses as 
> opposed to on-SoC devices (which don't suffer from the erratum).
> 
> In any event what would happen if we got rid of this check and ...
> 
> 
>>
>>> +}
>>> +
>>>   static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>>>   irq_hw_number_t hw)
>>>   {
>>> @@ -773,6 +789,9 @@ static int gic_irq_domain_map(struct irq_domain *d, 
>>> unsigned int irq,
>>> return -EPERM;
>>> irq_domain_set_info(d, irq, hw, chip, d->host_data,
>>> handle_fasteoi_irq, NULL, NULL);
>>> +   if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_28168))
>>> +   __irq_set_preflow_handler(irq,
>>> + cavium_irq_perflow_handler);
>>
> 
> ... move the registration of the preflow_handler into a 
> msi_domain_ops.msi_finish() handler in irq-git-v3-its-pic-msi.c?

That's the kind of thing I was angling for. You'll have to store the
device pointer into the scratchpad (we still have plenty of space there)
so that msi_finish() can have a peek.

> There we will know that it is a pci device, and can walk up the bus 
> hierarchy to see if there is a Cavium PCIe root port present.  If such a 
> port is found, we know we are on an external Cavium PCIe bus, and can 
> register the preflow_handler without having to check the device identifiers.

Something like that (though I'm unclear why other devices wouldn't see a
root port, but that's probably me lacking some PCIe foo).

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168

2016-11-15 Thread David Daney 

On 11/15/2016 01:26 AM, Marc Zyngier wrote:

On 15/11/16 07:00, Geetha sowjanya wrote:

From: Tirumalesh Chalamarla 

   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 data transfers are
   completed before invoking ISR.

Signed-off-by: Tirumalesh Chalamarla 
Signed-off-by: Geetha sowjanya 

[...]

--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -28,6 +28,8 @@
  #include 
  #include 
  #include 
+#include 
+#include 

  #include 
  #include 
@@ -736,6 +738,20 @@ static inline void gic_cpu_pm_init(void) { }

  #define GIC_ID_NR (1U << gic_data.rdists.id_bits)

+/*
+ * Due to #28168 erratum in ThunderX,
+ * we need to make sure DMA data transfer is done before MSIX.
+ */
+static void cavium_irq_perflow_handler(struct irq_data *data)
+{
+   struct pci_dev *pdev;
+
+   pdev = msi_desc_to_pci_dev(irq_data_get_msi_desc(data));


What happens if this is not a PCI device?


+   if ((pdev->vendor != 0x177d) &&
+   ((pdev->device & 0xA000) != 0xA000))
+   cavium_arm_smmu_tlb_sync(>dev);


I've asked that before. What makes Cavium devices so special that they
are not sensitive to this bug?



This is a heuristic for devices connected to external PCIe buses as 
opposed to on-SoC devices (which don't suffer from the erratum).


In any event what would happen if we got rid of this check and ...





+}
+
  static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
  irq_hw_number_t hw)
  {
@@ -773,6 +789,9 @@ static int gic_irq_domain_map(struct irq_domain *d, 
unsigned int irq,
return -EPERM;
irq_domain_set_info(d, irq, hw, chip, d->host_data,
handle_fasteoi_irq, NULL, NULL);
+   if (cpus_have_cap(ARM64_WORKAROUND_CAVIUM_28168))
+   __irq_set_preflow_handler(irq,
+ cavium_irq_perflow_handler);




... move the registration of the preflow_handler into a 
msi_domain_ops.msi_finish() handler in irq-git-v3-its-pic-msi.c?


There we will know that it is a pci device, and can walk up the bus 
hierarchy to see if there is a Cavium PCIe root port present.  If such a 
port is found, we know we are on an external Cavium PCIe bus, and can 
register the preflow_handler without having to check the device identifiers.





What happens if SMMUv2 is not compiled in? Also, since this only affects
LPI signaling, why is this in the core GICv3 code and not in the ITS.
And more specifically, in the PCI part of the ITS, since you seem to
exclusively consider PCI?


}

return 0;



Thanks,

M.



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

Re: [PATCH v2] arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168

2016-11-15 Thread Robin Murphy 
On 15/11/16 09:26, Marc Zyngier wrote:
> On 15/11/16 07:00, Geetha sowjanya wrote:
>> From: Tirumalesh Chalamarla 
>>
>>   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 data transfers are
>>   completed before invoking ISR.
>>
>> Signed-off-by: Tirumalesh Chalamarla 
>> Signed-off-by: Geetha sowjanya 
>> ---
>>  arch/arm64/Kconfig  |   11 +++
>>  arch/arm64/Kconfig.platforms|1 +
>>  arch/arm64/include/asm/cpufeature.h |3 ++-
>>  arch/arm64/kernel/cpu_errata.c  |   16 
>>  drivers/iommu/arm-smmu.c|   24 
>>  drivers/irqchip/irq-gic-common.h|1 +
>>  drivers/irqchip/irq-gic-v3.c|   19 +++
>>  7 files changed, 74 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 30398db..751972c 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 DMA data transfer is done 
>> before MSIX"
>> +   depends on ARCH_THUNDER && ARM64
>> +   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 data transfers are done before invoking 
>> ISR.
>> +
>> +If unsure, say Y.
> 
> Where is the entry in Documentation/arm64/silicon-errata.txt?
> 
>>  endmenu
>>  
>>  
>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>> index cfbdf02..2ac4ac6 100644
>> --- a/arch/arm64/Kconfig.platforms
>> +++ b/arch/arm64/Kconfig.platforms
>> @@ -185,6 +185,7 @@ config ARCH_SPRD
>>  
>>  config ARCH_THUNDER
>>  bool "Cavium Inc. Thunder SoC Family"
>> +select IRQ_PREFLOW_FASTEOI
>>  help
>>This enables support for Cavium's Thunder Family of SoCs.
>>  
>> diff --git a/arch/arm64/include/asm/cpufeature.h 
>> b/arch/arm64/include/asm/cpufeature.h
>> index 758d74f..821fc3c 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -40,8 +40,9 @@
>>  #define ARM64_HAS_32BIT_EL0 13
>>  #define ARM64_HYP_OFFSET_LOW14
>>  #define ARM64_MISMATCHED_CACHE_LINE_SIZE15
>> +#define ARM64_WORKAROUND_CAVIUM_28168   16
>>  
>> -#define ARM64_NCAPS 16
>> +#define ARM64_NCAPS 17
>>  
>>  #ifndef __ASSEMBLY__
>>  
>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>> index 0150394..0841a12 100644
>> --- a/arch/arm64/kernel/cpu_errata.c
>> +++ b/arch/arm64/kernel/cpu_errata.c
>> @@ -122,6 +122,22 @@ static void cpu_enable_trap_ctr_access(void *__unused)
>>  MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x00),
>>  },
>>  #endif
>> +#ifdef CONFIG_CAVIUM_ERRATUM_28168
>> +{
>> +/* Cavium ThunderX, T88 pass 1.x - 2.1 */
>> +.desc = "Cavium erratum 28168",
>> +.capability = ARM64_WORKAROUND_CAVIUM_28168,
>> +MIDR_RANGE(MIDR_THUNDERX, 0x00,
>> +   (1 << MIDR_VARIANT_SHIFT) | 1),
>> +},
>> +{
>> +/* Cavium ThunderX, T81 pass 1.0 */
>> +.desc = "Cavium erratum 28168",
>> +.capability = ARM64_WORKAROUND_CAVIUM_28168,
>> +MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x00),
>> +},
>> +#endif
> 
> How is that a CPU bug? Shouldn't that be keyed on the SMMU version or
> the ITs version?

Seconded. I assume the actual component at fault is the PCI RC, SMMU, or
interconnect in between - are there no ID registers in those parts that
will indicate a fixed version (or ECO) in future?

>> +
>>  {
>>  .desc = "Mismatched cache line size",
>>  .capability = ARM64_MISMATCHED_CACHE_LINE_SIZE,
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index c841eb7..1b4555c 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -570,6 +570,30 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device 
>> *smmu)
>>  }
>>  }
>>  
>> +/*
>> + * 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

Re: [PATCH v2] arm64: SMMU-v2: Workaround for Cavium ThunderX erratum 28168

2016-11-15 Thread Marc Zyngier 
On 15/11/16 07:00, Geetha sowjanya wrote:
> From: Tirumalesh Chalamarla 
> 
>   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 data transfers are
>   completed before invoking ISR.
> 
> Signed-off-by: Tirumalesh Chalamarla 
> Signed-off-by: Geetha sowjanya 
> ---
>  arch/arm64/Kconfig  |   11 +++
>  arch/arm64/Kconfig.platforms|1 +
>  arch/arm64/include/asm/cpufeature.h |3 ++-
>  arch/arm64/kernel/cpu_errata.c  |   16 
>  drivers/iommu/arm-smmu.c|   24 
>  drivers/irqchip/irq-gic-common.h|1 +
>  drivers/irqchip/irq-gic-v3.c|   19 +++
>  7 files changed, 74 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 30398db..751972c 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 DMA data transfer is done 
> before MSIX"
> +   depends on ARCH_THUNDER && ARM64
> +   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 data transfers are done before invoking ISR.
> +
> +If unsure, say Y.

Where is the entry in Documentation/arm64/silicon-errata.txt?

>  endmenu
>  
>  
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index cfbdf02..2ac4ac6 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -185,6 +185,7 @@ config ARCH_SPRD
>  
>  config ARCH_THUNDER
>   bool "Cavium Inc. Thunder SoC Family"
> + select IRQ_PREFLOW_FASTEOI
>   help
> This enables support for Cavium's Thunder Family of SoCs.
>  
> diff --git a/arch/arm64/include/asm/cpufeature.h 
> b/arch/arm64/include/asm/cpufeature.h
> index 758d74f..821fc3c 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -40,8 +40,9 @@
>  #define ARM64_HAS_32BIT_EL0  13
>  #define ARM64_HYP_OFFSET_LOW 14
>  #define ARM64_MISMATCHED_CACHE_LINE_SIZE 15
> +#define ARM64_WORKAROUND_CAVIUM_2816816
>  
> -#define ARM64_NCAPS  16
> +#define ARM64_NCAPS  17
>  
>  #ifndef __ASSEMBLY__
>  
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 0150394..0841a12 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -122,6 +122,22 @@ static void cpu_enable_trap_ctr_access(void *__unused)
>   MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x00),
>   },
>  #endif
> +#ifdef CONFIG_CAVIUM_ERRATUM_28168
> + {
> + /* Cavium ThunderX, T88 pass 1.x - 2.1 */
> + .desc = "Cavium erratum 28168",
> + .capability = ARM64_WORKAROUND_CAVIUM_28168,
> + MIDR_RANGE(MIDR_THUNDERX, 0x00,
> +(1 << MIDR_VARIANT_SHIFT) | 1),
> + },
> + {
> + /* Cavium ThunderX, T81 pass 1.0 */
> + .desc = "Cavium erratum 28168",
> + .capability = ARM64_WORKAROUND_CAVIUM_28168,
> + MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x00),
> + },
> +#endif

How is that a CPU bug? Shouldn't that be keyed on the SMMU version or
the ITs version?

> +
>   {
>   .desc = "Mismatched cache line size",
>   .capability = ARM64_MISMATCHED_CACHE_LINE_SIZE,
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index c841eb7..1b4555c 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -570,6 +570,30 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device 
> *smmu)
>   }
>  }
>  
> +/*
> + * 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 cavium_arm_smmu_tlb_sync(struct device *dev)
> +{
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> + struct arm_smmu_device *smmu;
> +
> + smmu = fwspec_smmu(fwspec);
> + if (!smmu)
> + return;
> + __arm_smmu_tlb_sync(smmu);
> +
> +}
>