Re: [PATCH 1/1] iommu/vt-d: Serialize IOMMU GCMD register modifications

2020-08-26 Thread Lu Baolu 

Hi Kevin,

On 8/26/20 1:29 PM, Tian, Kevin wrote:

From: Lu Baolu
Sent: Wednesday, August 26, 2020 10:58 AM

The VT-d spec requires (10.4.4 Global Command Register, GCMD_REG
General
Description) that:

If multiple control fields in this register need to be modified, software
must serialize the modifications through multiple writes to this register.

However, in irq_remapping.c, modifications of IRE and CFI are done in one
write. We need to do two separate writes with STS checking after each.

Fixes: af8d102f999a4 ("x86/intel/irq_remapping: Clean up x2apic opt-out
security warning mess")
Cc: Andy Lutomirski 
Cc: Jacob Pan 
Signed-off-by: Lu Baolu 
---
  drivers/iommu/intel/irq_remapping.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/irq_remapping.c
b/drivers/iommu/intel/irq_remapping.c
index 9564d23d094f..19d7e18876fe 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -507,12 +507,16 @@ static void iommu_enable_irq_remapping(struct
intel_iommu *iommu)

/* Enable interrupt-remapping */
iommu->gcmd |= DMA_GCMD_IRE;
-   iommu->gcmd &= ~DMA_GCMD_CFI;  /* Block compatibility-format
MSIs */
writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
-
IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
  readl, (sts & DMA_GSTS_IRES), sts);

+   /* Block compatibility-format MSIs */
+   iommu->gcmd &= ~DMA_GCMD_CFI;
+   writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
+   IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
+ readl, !(sts & DMA_GSTS_CFIS), sts);
+


Better do it only when CFI is actually enabled (by checking sts).


Yes. Make sense. Will send a new version with this changed.

Best regards,
baolu




/*
 * With CFI clear in the Global Command register, we should be
 * protected from dangerous (i.e. compatibility) interrupts
--
2.17.1

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

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

RE: [PATCH 1/1] iommu/vt-d: Serialize IOMMU GCMD register modifications

2020-08-25 Thread Tian, Kevin 
> From: Lu Baolu
> Sent: Wednesday, August 26, 2020 10:58 AM
> 
> The VT-d spec requires (10.4.4 Global Command Register, GCMD_REG
> General
> Description) that:
> 
> If multiple control fields in this register need to be modified, software
> must serialize the modifications through multiple writes to this register.
> 
> However, in irq_remapping.c, modifications of IRE and CFI are done in one
> write. We need to do two separate writes with STS checking after each.
> 
> Fixes: af8d102f999a4 ("x86/intel/irq_remapping: Clean up x2apic opt-out
> security warning mess")
> Cc: Andy Lutomirski 
> Cc: Jacob Pan 
> Signed-off-by: Lu Baolu 
> ---
>  drivers/iommu/intel/irq_remapping.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel/irq_remapping.c
> b/drivers/iommu/intel/irq_remapping.c
> index 9564d23d094f..19d7e18876fe 100644
> --- a/drivers/iommu/intel/irq_remapping.c
> +++ b/drivers/iommu/intel/irq_remapping.c
> @@ -507,12 +507,16 @@ static void iommu_enable_irq_remapping(struct
> intel_iommu *iommu)
> 
>   /* Enable interrupt-remapping */
>   iommu->gcmd |= DMA_GCMD_IRE;
> - iommu->gcmd &= ~DMA_GCMD_CFI;  /* Block compatibility-format
> MSIs */
>   writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
> -
>   IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
> readl, (sts & DMA_GSTS_IRES), sts);
> 
> + /* Block compatibility-format MSIs */
> + iommu->gcmd &= ~DMA_GCMD_CFI;
> + writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
> + IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
> +   readl, !(sts & DMA_GSTS_CFIS), sts);
> +

Better do it only when CFI is actually enabled (by checking sts).

>   /*
>* With CFI clear in the Global Command register, we should be
>* protected from dangerous (i.e. compatibility) interrupts
> --
> 2.17.1
> 
> ___
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/1] iommu/vt-d: Serialize IOMMU GCMD register modifications

2020-08-25 Thread Lu Baolu 
The VT-d spec requires (10.4.4 Global Command Register, GCMD_REG General
Description) that:

If multiple control fields in this register need to be modified, software
must serialize the modifications through multiple writes to this register.

However, in irq_remapping.c, modifications of IRE and CFI are done in one
write. We need to do two separate writes with STS checking after each.

Fixes: af8d102f999a4 ("x86/intel/irq_remapping: Clean up x2apic opt-out 
security warning mess")
Cc: Andy Lutomirski 
Cc: Jacob Pan 
Signed-off-by: Lu Baolu 
---
 drivers/iommu/intel/irq_remapping.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/irq_remapping.c 
b/drivers/iommu/intel/irq_remapping.c
index 9564d23d094f..19d7e18876fe 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -507,12 +507,16 @@ static void iommu_enable_irq_remapping(struct intel_iommu 
*iommu)
 
/* Enable interrupt-remapping */
iommu->gcmd |= DMA_GCMD_IRE;
-   iommu->gcmd &= ~DMA_GCMD_CFI;  /* Block compatibility-format MSIs */
writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
-
IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
  readl, (sts & DMA_GSTS_IRES), sts);
 
+   /* Block compatibility-format MSIs */
+   iommu->gcmd &= ~DMA_GCMD_CFI;
+   writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
+   IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
+ readl, !(sts & DMA_GSTS_CFIS), sts);
+
/*
 * With CFI clear in the Global Command register, we should be
 * protected from dangerous (i.e. compatibility) interrupts
-- 
2.17.1

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