iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/1] iommu/vt-d: Serialize IOMMU GCMD register modifications
@ 2020-08-27  4:25 Lu Baolu
  2020-08-27  5:39 ` Tian, Kevin
  0 siblings, 1 reply; 3+ messages in thread
From: Lu Baolu @ 2020-08-27  4:25 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: Kevin Tian, Ashok Raj, linux-kernel, Andy Lutomirski, iommu

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 <luto@amacapital.net>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/irq_remapping.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Change log:
v1->v2:
  - v1 posted here
    https://lore.kernel.org/linux-iommu/20200826025825.2322-1-baolu.lu@linux.intel.com/;
  - Add status check before disabling CFI. (Kevin)

diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c
index 9564d23d094f..7552bb7e92c8 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -507,12 +507,19 @@ 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 */
+	sts = readl(iommu->reg + DMAR_GSTS_REG);
+	if (sts & DMA_GSTS_CFIS) {
+		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

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* RE: [PATCH v2 1/1] iommu/vt-d: Serialize IOMMU GCMD register modifications
  2020-08-27  4:25 [PATCH v2 1/1] iommu/vt-d: Serialize IOMMU GCMD register modifications Lu Baolu
@ 2020-08-27  5:39 ` Tian, Kevin
  2020-08-28  0:00   ` Lu Baolu
  0 siblings, 1 reply; 3+ messages in thread
From: Tian, Kevin @ 2020-08-27  5:39 UTC (permalink / raw)
  To: Lu Baolu, Joerg Roedel; +Cc: Raj, Ashok, iommu, linux-kernel, Andy Lutomirski

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, August 27, 2020 12:25 PM
> 
> 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 <luto@amacapital.net>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/irq_remapping.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> Change log:
> v1->v2:
>   - v1 posted here
>     https://lore.kernel.org/linux-iommu/20200826025825.2322-1-
> baolu.lu@linux.intel.com/;
>   - Add status check before disabling CFI. (Kevin)
> 
> diff --git a/drivers/iommu/intel/irq_remapping.c
> b/drivers/iommu/intel/irq_remapping.c
> index 9564d23d094f..7552bb7e92c8 100644
> --- a/drivers/iommu/intel/irq_remapping.c
> +++ b/drivers/iommu/intel/irq_remapping.c
> @@ -507,12 +507,19 @@ 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 */
> +	sts = readl(iommu->reg + DMAR_GSTS_REG);

no need of this readl as the status is already three in IOMMU_WAIT_OP.

> +	if (sts & DMA_GSTS_CFIS) {
> +		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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1] iommu/vt-d: Serialize IOMMU GCMD register modifications
  2020-08-27  5:39 ` Tian, Kevin
@ 2020-08-28  0:00   ` Lu Baolu
  0 siblings, 0 replies; 3+ messages in thread
From: Lu Baolu @ 2020-08-28  0:00 UTC (permalink / raw)
  To: Tian, Kevin, Joerg Roedel
  Cc: Raj, Ashok, linux-kernel, Andy Lutomirski, iommu

Hi,

On 8/27/20 1:39 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, August 27, 2020 12:25 PM
>>
>> 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 <luto@amacapital.net>
>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Cc: Ashok Raj <ashok.raj@intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/irq_remapping.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> Change log:
>> v1->v2:
>>    - v1 posted here
>>      https://lore.kernel.org/linux-iommu/20200826025825.2322-1-
>> baolu.lu@linux.intel.com/;
>>    - Add status check before disabling CFI. (Kevin)
>>
>> diff --git a/drivers/iommu/intel/irq_remapping.c
>> b/drivers/iommu/intel/irq_remapping.c
>> index 9564d23d094f..7552bb7e92c8 100644
>> --- a/drivers/iommu/intel/irq_remapping.c
>> +++ b/drivers/iommu/intel/irq_remapping.c
>> @@ -507,12 +507,19 @@ 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 */
>> +	sts = readl(iommu->reg + DMAR_GSTS_REG);
> 
> no need of this readl as the status is already three in IOMMU_WAIT_OP.

Yes. Really!

Best regards,
baolu

> 
>> +	if (sts & DMA_GSTS_CFIS) {
>> +		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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-28  0:05 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-27  4:25 [PATCH v2 1/1] iommu/vt-d: Serialize IOMMU GCMD register modifications Lu Baolu
2020-08-27  5:39 ` Tian, Kevin
2020-08-28  0:00   ` Lu Baolu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).