Re: [PATCH] iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE

2020-09-25 Thread Greg KH
On Fri, Sep 25, 2020 at 11:45:05AM +, Suravee Suthikulpanit wrote:
> When using 128-bit interrupt-remapping table entry (IRTE) (a.k.a GA mode),
> current driver disables interrupt remapping when it updates the IRTE
> so that the upper and lower 64-bit values can be updated safely.
> 
> However, this creates a small window, where the interrupt could
> arrive and result in IO_PAGE_FAULT (for interrupt) as shown below.
> 
>   IOMMU DriverDevice IRQ
>   ===
>   irte.RemapEn=0
>...
>change IRTEIRQ from device ==> IO_PAGE_FAULT !!
>...
>   irte.RemapEn=1
> 
> This scenario has been observed when changing irq affinity on a system
> running I/O-intensive workload, in which the destination APIC ID
> in the IRTE is updated.
> 
> Instead, use cmpxchg_double() to update the 128-bit IRTE at once without
> disabling the interrupt remapping. However, this means several features,
> which require GA (128-bit IRTE) support will also be affected if cmpxchg16b
> is not supported (which is unprecedented for AMD processors w/ IOMMU).
> 
> Cc: sta...@vger.kernel.org
> Fixes: 880ac60e2538 ("iommu/amd: Introduce interrupt remapping ops structure")
> Reported-by: Sean Osborne 
> Signed-off-by: Suravee Suthikulpanit 
> Tested-by: Erik Rockstrom 
> Reviewed-by: Joao Martins 
> Link: 
> https://lore.kernel.org/r/20200903093822.52012-3-suravee.suthikulpa...@amd.com
> Signed-off-by: Joerg Roedel 
> ---
> Note: This patch is the back-port on top of the stable branch linux-5.4.y
> for the upstream commit e52d58d54a32 ("iommu/amd: Use cmpxchg_double() when
> updating 128-bit IRTE") since the original patch does not apply cleanly.

Now queued up, thanks.

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


[PATCH] iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE

2020-09-25 Thread Suravee Suthikulpanit
When using 128-bit interrupt-remapping table entry (IRTE) (a.k.a GA mode),
current driver disables interrupt remapping when it updates the IRTE
so that the upper and lower 64-bit values can be updated safely.

However, this creates a small window, where the interrupt could
arrive and result in IO_PAGE_FAULT (for interrupt) as shown below.

  IOMMU DriverDevice IRQ
  ===
  irte.RemapEn=0
   ...
   change IRTEIRQ from device ==> IO_PAGE_FAULT !!
   ...
  irte.RemapEn=1

This scenario has been observed when changing irq affinity on a system
running I/O-intensive workload, in which the destination APIC ID
in the IRTE is updated.

Instead, use cmpxchg_double() to update the 128-bit IRTE at once without
disabling the interrupt remapping. However, this means several features,
which require GA (128-bit IRTE) support will also be affected if cmpxchg16b
is not supported (which is unprecedented for AMD processors w/ IOMMU).

Cc: sta...@vger.kernel.org
Fixes: 880ac60e2538 ("iommu/amd: Introduce interrupt remapping ops structure")
Reported-by: Sean Osborne 
Signed-off-by: Suravee Suthikulpanit 
Tested-by: Erik Rockstrom 
Reviewed-by: Joao Martins 
Link: 
https://lore.kernel.org/r/20200903093822.52012-3-suravee.suthikulpa...@amd.com
Signed-off-by: Joerg Roedel 
---
Note: This patch is the back-port on top of the stable branch linux-5.4.y
for the upstream commit e52d58d54a32 ("iommu/amd: Use cmpxchg_double() when
updating 128-bit IRTE") since the original patch does not apply cleanly.

 drivers/iommu/Kconfig  |  2 +-
 drivers/iommu/amd_iommu.c  | 17 +
 drivers/iommu/amd_iommu_init.c | 21 +++--
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 390568afee9f..fc0160e8ed33 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -138,7 +138,7 @@ config AMD_IOMMU
select PCI_PASID
select IOMMU_API
select IOMMU_IOVA
-   depends on X86_64 && PCI && ACPI
+   depends on X86_64 && PCI && ACPI && HAVE_CMPXCHG_DOUBLE
---help---
  With this option you can enable support for AMD IOMMU hardware in
  your system. An IOMMU is a hardware component which provides
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index fa91d856a43e..7b724f7b27a9 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3873,6 +3873,7 @@ static int alloc_irq_index(u16 devid, int count, bool 
align,
 static int modify_irte_ga(u16 devid, int index, struct irte_ga *irte,
  struct amd_ir_data *data)
 {
+   bool ret;
struct irq_remap_table *table;
struct amd_iommu *iommu;
unsigned long flags;
@@ -3890,10 +3891,18 @@ static int modify_irte_ga(u16 devid, int index, struct 
irte_ga *irte,
 
entry = (struct irte_ga *)table->table;
entry = [index];
-   entry->lo.fields_remap.valid = 0;
-   entry->hi.val = irte->hi.val;
-   entry->lo.val = irte->lo.val;
-   entry->lo.fields_remap.valid = 1;
+
+   ret = cmpxchg_double(>lo.val, >hi.val,
+entry->lo.val, entry->hi.val,
+irte->lo.val, irte->hi.val);
+   /*
+* We use cmpxchg16 to atomically update the 128-bit IRTE,
+* and it cannot be updated by the hardware or other processors
+* behind us, so the return value of cmpxchg16 should be the
+* same as the old value.
+*/
+   WARN_ON(!ret);
+
if (data)
data->ref = entry;
 
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 135ae5222cf3..31d7e2d4f304 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1522,7 +1522,14 @@ static int __init init_iommu_one(struct amd_iommu 
*iommu, struct ivhd_header *h)
iommu->mmio_phys_end = MMIO_REG_END_OFFSET;
else
iommu->mmio_phys_end = MMIO_CNTR_CONF_OFFSET;
-   if (((h->efr_attr & (0x1 << IOMMU_FEAT_GASUP_SHIFT)) == 0))
+
+   /*
+* Note: GA (128-bit IRTE) mode requires cmpxchg16b supports.
+* GAM also requires GA mode. Therefore, we need to
+* check cmpxchg16b support before enabling it.
+*/
+   if (!boot_cpu_has(X86_FEATURE_CX16) ||
+   ((h->efr_attr & (0x1 << IOMMU_FEAT_GASUP_SHIFT)) == 0))
amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY;
break;
case 0x11:
@@ -1531,8 +1538,18 @@ static int __init init_iommu_one(struct amd_iommu 
*iommu, struct ivhd_header *h)
iommu->mmio_phys_end = MMIO_REG_END_OFFSET;
else
iommu->mmio_phys_end = MMIO_CNTR_CONF_OFFSET;
-   if (((h->efr_reg & (0x1 <<