[PATCH] intel-iommu: Don't be too aggressive when clearing one context entry

[PATCH] intel-iommu: Don't be too aggressive when clearing one context entry

[Filippo Sironi]

Previously, we were invalidating context cache and IOTLB globally when
clearing one context entry.  This is a tad too aggressive.
Invalidate the context cache and IOTLB for the interested device only.

Signed-off-by: Filippo Sironi <sironi@amazon.de>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/iommu/intel-iommu.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3e8636f1220e..4bf3e59b0929 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2351,13 +2351,32 @@ static inline int domain_pfn_mapping(struct dmar_domain *domain, unsigned long i
 
 static void domain_context_clear_one(struct intel_iommu *iommu, u8 bus, u8 devfn)
 {
+	unsigned long flags;
+	struct context_entry *context;
+	u16 did_old;
+
 	if (!iommu)
 		return;
 
+	spin_lock_irqsave(&iommu->lock, flags);
+	context = iommu_context_addr(iommu, bus, devfn, 0);
+	if (!context) {
+		spin_unlock_irqrestore(&iommu->lock, flags);
+		return;
+	}
+	did_old = context_domain_id(context);
+	spin_unlock_irqrestore(&iommu->lock, flags);
 	clear_context_table(iommu, bus, devfn);
-	iommu->flush.flush_context(iommu, 0, 0, 0,
-					   DMA_CCMD_GLOBAL_INVL);
-	iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH);
+	iommu->flush.flush_context(iommu,
+				   did_old,
+				   (((u16)bus) << 8) | devfn,
+				   DMA_CCMD_MASK_NOBIT,
+				   DMA_CCMD_DEVICE_INVL);
+	iommu->flush.flush_iotlb(iommu,
+				 did_old,
+				 0,
+				 0,
+				 DMA_TLB_DSI_FLUSH);
 }
 
 static inline void unlink_domain_info(struct device_domain_info *info)
-- 
2.7.4

Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry

[Joerg Roedel]

Hi Filippo,

please change the subject to:

	iommu/vt-d: Don't be too aggressive when clearing one context entry

to follow the convention used in the iommu-tree. Another comment below.

On Mon, Aug 28, 2017 at 04:16:29PM +0200, Filippo Sironi wrote:
>  static void domain_context_clear_one(struct intel_iommu *iommu, u8 bus, u8 devfn)
>  {
> +	unsigned long flags;
> +	struct context_entry *context;
> +	u16 did_old;
> +
>  	if (!iommu)
>  		return;
>  
> +	spin_lock_irqsave(&iommu->lock, flags);
> +	context = iommu_context_addr(iommu, bus, devfn, 0);
> +	if (!context) {
> +		spin_unlock_irqrestore(&iommu->lock, flags);
> +		return;
> +	}
> +	did_old = context_domain_id(context);
> +	spin_unlock_irqrestore(&iommu->lock, flags);
>  	clear_context_table(iommu, bus, devfn);

This function is the only caller of clear_context_table(), which does
similar things (like fetching the context-entry) as you are adding
above.

So you can either make clear_context_table() return the old domain-id
so that you don't need to do it here, or you get rid of the function
entirely and add the context_clear_entry() and __iommu_flush_cache()
calls into this code-path.

Regards,

	Joerg

Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry

[Joerg Roedel]

Hi Filippo,

please change the subject to:

	iommu/vt-d: Don't be too aggressive when clearing one context entry

to follow the convention used in the iommu-tree. Another comment below.

On Mon, Aug 28, 2017 at 04:16:29PM +0200, Filippo Sironi wrote:
>  static void domain_context_clear_one(struct intel_iommu *iommu, u8 bus, u8 devfn)
>  {
> +	unsigned long flags;
> +	struct context_entry *context;
> +	u16 did_old;
> +
>  	if (!iommu)
>  		return;
>  
> +	spin_lock_irqsave(&iommu->lock, flags);
> +	context = iommu_context_addr(iommu, bus, devfn, 0);
> +	if (!context) {
> +		spin_unlock_irqrestore(&iommu->lock, flags);
> +		return;
> +	}
> +	did_old = context_domain_id(context);
> +	spin_unlock_irqrestore(&iommu->lock, flags);
>  	clear_context_table(iommu, bus, devfn);

This function is the only caller of clear_context_table(), which does
similar things (like fetching the context-entry) as you are adding
above.

So you can either make clear_context_table() return the old domain-id
so that you don't need to do it here, or you get rid of the function
entirely and add the context_clear_entry() and __iommu_flush_cache()
calls into this code-path.

Regards,

	Joerg

Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry

[Jacob Pan]

On Mon, 28 Aug 2017 16:16:29 +0200
Filippo Sironi via iommu <iommu@lists.linux-foundation.org> wrote:

> Previously, we were invalidating context cache and IOTLB globally when
> clearing one context entry.  This is a tad too aggressive.
> Invalidate the context cache and IOTLB for the interested device only.
> 
> Signed-off-by: Filippo Sironi <sironi@amazon.de>
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: iommu@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/iommu/intel-iommu.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3e8636f1220e..4bf3e59b0929 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2351,13 +2351,32 @@ static inline int domain_pfn_mapping(struct
> dmar_domain *domain, unsigned long i 
>  static void domain_context_clear_one(struct intel_iommu *iommu, u8
> bus, u8 devfn) {
> +	unsigned long flags;
> +	struct context_entry *context;
> +	u16 did_old;
> +
>  	if (!iommu)
>  		return;
>  
> +	spin_lock_irqsave(&iommu->lock, flags);
> +	context = iommu_context_addr(iommu, bus, devfn, 0);
> +	if (!context) {
> +		spin_unlock_irqrestore(&iommu->lock, flags);
> +		return;
> +	}
perhaps check with device_context_mapped()?

> +	did_old = context_domain_id(context);
> +	spin_unlock_irqrestore(&iommu->lock, flags);
>  	clear_context_table(iommu, bus, devfn);
> -	iommu->flush.flush_context(iommu, 0, 0, 0,
> -					   DMA_CCMD_GLOBAL_INVL);
> -	iommu->flush.flush_iotlb(iommu, 0, 0, 0,
> DMA_TLB_GLOBAL_FLUSH);
> +	iommu->flush.flush_context(iommu,
> +				   did_old,
> +				   (((u16)bus) << 8) | devfn,
> +				   DMA_CCMD_MASK_NOBIT,
> +				   DMA_CCMD_DEVICE_INVL);
> +	iommu->flush.flush_iotlb(iommu,
> +				 did_old,
> +				 0,
> +				 0,
> +				 DMA_TLB_DSI_FLUSH);
>  }
>  
>  static inline void unlink_domain_info(struct device_domain_info
> *info)

[Jacob Pan]

Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry

[Jacob Pan]

On Mon, 28 Aug 2017 16:16:29 +0200
Filippo Sironi via iommu <iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org> wrote:

> Previously, we were invalidating context cache and IOTLB globally when
> clearing one context entry.  This is a tad too aggressive.
> Invalidate the context cache and IOTLB for the interested device only.
> 
> Signed-off-by: Filippo Sironi <sironi-ebkRAfMGSJGzQB+pC5nmwQ@public.gmane.org>
> Cc: David Woodhouse <dwmw-vV1OtcyAfmbQXOPxS62xeg@public.gmane.org>
> Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  drivers/iommu/intel-iommu.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3e8636f1220e..4bf3e59b0929 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2351,13 +2351,32 @@ static inline int domain_pfn_mapping(struct
> dmar_domain *domain, unsigned long i 
>  static void domain_context_clear_one(struct intel_iommu *iommu, u8
> bus, u8 devfn) {
> +	unsigned long flags;
> +	struct context_entry *context;
> +	u16 did_old;
> +
>  	if (!iommu)
>  		return;
>  
> +	spin_lock_irqsave(&iommu->lock, flags);
> +	context = iommu_context_addr(iommu, bus, devfn, 0);
> +	if (!context) {
> +		spin_unlock_irqrestore(&iommu->lock, flags);
> +		return;
> +	}
perhaps check with device_context_mapped()?

> +	did_old = context_domain_id(context);
> +	spin_unlock_irqrestore(&iommu->lock, flags);
>  	clear_context_table(iommu, bus, devfn);
> -	iommu->flush.flush_context(iommu, 0, 0, 0,
> -					   DMA_CCMD_GLOBAL_INVL);
> -	iommu->flush.flush_iotlb(iommu, 0, 0, 0,
> DMA_TLB_GLOBAL_FLUSH);
> +	iommu->flush.flush_context(iommu,
> +				   did_old,
> +				   (((u16)bus) << 8) | devfn,
> +				   DMA_CCMD_MASK_NOBIT,
> +				   DMA_CCMD_DEVICE_INVL);
> +	iommu->flush.flush_iotlb(iommu,
> +				 did_old,
> +				 0,
> +				 0,
> +				 DMA_TLB_DSI_FLUSH);
>  }
>  
>  static inline void unlink_domain_info(struct device_domain_info
> *info)

[Jacob Pan]

Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry

[Sironi, Filippo]

Hi Joerg,

> On 30. Aug 2017, at 15:31, Joerg Roedel <joro@8bytes.org> wrote:
> 
> Hi Filippo,
> 
> please change the subject to:
> 
> 	iommu/vt-d: Don't be too aggressive when clearing one context entry
> 
> to follow the convention used in the iommu-tree. Another comment below.

Will do.

> On Mon, Aug 28, 2017 at 04:16:29PM +0200, Filippo Sironi wrote:
>> static void domain_context_clear_one(struct intel_iommu *iommu, u8 bus, u8 devfn)
>> {
>> +	unsigned long flags;
>> +	struct context_entry *context;
>> +	u16 did_old;
>> +
>> 	if (!iommu)
>> 		return;
>> 
>> +	spin_lock_irqsave(&iommu->lock, flags);
>> +	context = iommu_context_addr(iommu, bus, devfn, 0);
>> +	if (!context) {
>> +		spin_unlock_irqrestore(&iommu->lock, flags);
>> +		return;
>> +	}
>> +	did_old = context_domain_id(context);
>> +	spin_unlock_irqrestore(&iommu->lock, flags);
>> 	clear_context_table(iommu, bus, devfn);
> 
> This function is the only caller of clear_context_table(), which does
> similar things (like fetching the context-entry) as you are adding
> above.
> 
> So you can either make clear_context_table() return the old domain-id
> so that you don't need to do it here, or you get rid of the function
> entirely and add the context_clear_entry() and __iommu_flush_cache()
> calls into this code-path.
> 
> Regards,
> 
> 	Joerg

I went for merging domain_context_clear_one() with context_clear_one().

Regards,
Filippo


Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry

[Sironi, Filippo via iommu]

Hi Joerg,

> On 30. Aug 2017, at 15:31, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> 
> Hi Filippo,
> 
> please change the subject to:
> 
> 	iommu/vt-d: Don't be too aggressive when clearing one context entry
> 
> to follow the convention used in the iommu-tree. Another comment below.

Will do.

> On Mon, Aug 28, 2017 at 04:16:29PM +0200, Filippo Sironi wrote:
>> static void domain_context_clear_one(struct intel_iommu *iommu, u8 bus, u8 devfn)
>> {
>> +	unsigned long flags;
>> +	struct context_entry *context;
>> +	u16 did_old;
>> +
>> 	if (!iommu)
>> 		return;
>> 
>> +	spin_lock_irqsave(&iommu->lock, flags);
>> +	context = iommu_context_addr(iommu, bus, devfn, 0);
>> +	if (!context) {
>> +		spin_unlock_irqrestore(&iommu->lock, flags);
>> +		return;
>> +	}
>> +	did_old = context_domain_id(context);
>> +	spin_unlock_irqrestore(&iommu->lock, flags);
>> 	clear_context_table(iommu, bus, devfn);
> 
> This function is the only caller of clear_context_table(), which does
> similar things (like fetching the context-entry) as you are adding
> above.
> 
> So you can either make clear_context_table() return the old domain-id
> so that you don't need to do it here, or you get rid of the function
> entirely and add the context_clear_entry() and __iommu_flush_cache()
> calls into this code-path.
> 
> Regards,
> 
> 	Joerg

I went for merging domain_context_clear_one() with context_clear_one().

Regards,
Filippo


Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry

[Sironi, Filippo]

Hi Jacob,

> On 30. Aug 2017, at 17:50, Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> 
> On Mon, 28 Aug 2017 16:16:29 +0200
> Filippo Sironi via iommu <iommu@lists.linux-foundation.org> wrote:
> 
>> Previously, we were invalidating context cache and IOTLB globally when
>> clearing one context entry.  This is a tad too aggressive.
>> Invalidate the context cache and IOTLB for the interested device only.
>> 
>> Signed-off-by: Filippo Sironi <sironi@amazon.de>
>> Cc: David Woodhouse <dwmw@amazon.co.uk>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: iommu@lists.linux-foundation.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> drivers/iommu/intel-iommu.c | 25 ++++++++++++++++++++++---
>> 1 file changed, 22 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index 3e8636f1220e..4bf3e59b0929 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -2351,13 +2351,32 @@ static inline int domain_pfn_mapping(struct
>> dmar_domain *domain, unsigned long i 
>> static void domain_context_clear_one(struct intel_iommu *iommu, u8
>> bus, u8 devfn) {
>> +	unsigned long flags;
>> +	struct context_entry *context;
>> +	u16 did_old;
>> +
>> 	if (!iommu)
>> 		return;
>> 
>> +	spin_lock_irqsave(&iommu->lock, flags);
>> +	context = iommu_context_addr(iommu, bus, devfn, 0);
>> +	if (!context) {
>> +		spin_unlock_irqrestore(&iommu->lock, flags);
>> +		return;
>> +	}
> perhaps check with device_context_mapped()?

Using device_context_mapped() wouldn't simplify the code since it
would just tell me that at the time of check there was a context.
I would still need to lock, get the context, check if the context
is valid, do the work, and unlock.

Modifying device_context_mapped() to return the context isn't
going to work either because it may go away in the meantime since
I wouldn't hold the lock.

>> +	did_old = context_domain_id(context);
>> +	spin_unlock_irqrestore(&iommu->lock, flags);
>> 	clear_context_table(iommu, bus, devfn);
>> -	iommu->flush.flush_context(iommu, 0, 0, 0,
>> -					   DMA_CCMD_GLOBAL_INVL);
>> -	iommu->flush.flush_iotlb(iommu, 0, 0, 0,
>> DMA_TLB_GLOBAL_FLUSH);
>> +	iommu->flush.flush_context(iommu,
>> +				   did_old,
>> +				   (((u16)bus) << 8) | devfn,
>> +				   DMA_CCMD_MASK_NOBIT,
>> +				   DMA_CCMD_DEVICE_INVL);
>> +	iommu->flush.flush_iotlb(iommu,
>> +				 did_old,
>> +				 0,
>> +				 0,
>> +				 DMA_TLB_DSI_FLUSH);
>> }
>> 
>> static inline void unlink_domain_info(struct device_domain_info
>> *info)
> 
> [Jacob Pan]

Regards,
Filippo


Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH v2] iommu/vt-d: Don't be too aggressive when clearing one context entry

[Filippo Sironi]

Previously, we were invalidating context cache and IOTLB globally when
clearing one context entry.  This is a tad too aggressive.
Invalidate the context cache and IOTLB for the interested device only.

Signed-off-by: Filippo Sironi <sironi@amazon.de>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/iommu/intel-iommu.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3e8636f1220e..1aa4ad7974b9 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -974,20 +974,6 @@ static int device_context_mapped(struct intel_iommu *iommu, u8 bus, u8 devfn)
 	return ret;
 }
 
-static void clear_context_table(struct intel_iommu *iommu, u8 bus, u8 devfn)
-{
-	struct context_entry *context;
-	unsigned long flags;
-
-	spin_lock_irqsave(&iommu->lock, flags);
-	context = iommu_context_addr(iommu, bus, devfn, 0);
-	if (context) {
-		context_clear_entry(context);
-		__iommu_flush_cache(iommu, context, sizeof(*context));
-	}
-	spin_unlock_irqrestore(&iommu->lock, flags);
-}
-
 static void free_context_table(struct intel_iommu *iommu)
 {
 	int i;
@@ -2351,13 +2337,33 @@ static inline int domain_pfn_mapping(struct dmar_domain *domain, unsigned long i
 
 static void domain_context_clear_one(struct intel_iommu *iommu, u8 bus, u8 devfn)
 {
+	unsigned long flags;
+	struct context_entry *context;
+	u16 did_old;
+
 	if (!iommu)
 		return;
 
-	clear_context_table(iommu, bus, devfn);
-	iommu->flush.flush_context(iommu, 0, 0, 0,
-					   DMA_CCMD_GLOBAL_INVL);
-	iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH);
+	spin_lock_irqsave(&iommu->lock, flags);
+	context = iommu_context_addr(iommu, bus, devfn, 0);
+	if (!context) {
+		spin_unlock_irqrestore(&iommu->lock, flags);
+		return;
+	}
+	did_old = context_domain_id(context);
+	context_clear_entry(context);
+	__iommu_flush_cache(iommu, context, sizeof(*context));
+	spin_unlock_irqrestore(&iommu->lock, flags);
+	iommu->flush.flush_context(iommu,
+				   did_old,
+				   (((u16)bus) << 8) | devfn,
+				   DMA_CCMD_MASK_NOBIT,
+				   DMA_CCMD_DEVICE_INVL);
+	iommu->flush.flush_iotlb(iommu,
+				 did_old,
+				 0,
+				 0,
+				 DMA_TLB_DSI_FLUSH);
 }
 
 static inline void unlink_domain_info(struct device_domain_info *info)
-- 
2.7.4

[PATCH v2] iommu/vt-d: Don't be too aggressive when clearing one context entry

[Filippo Sironi via iommu]

Previously, we were invalidating context cache and IOTLB globally when
clearing one context entry.  This is a tad too aggressive.
Invalidate the context cache and IOTLB for the interested device only.

Signed-off-by: Filippo Sironi <sironi-ebkRAfMGSJGzQB+pC5nmwQ@public.gmane.org>
Cc: David Woodhouse <dwmw-vV1OtcyAfmbQXOPxS62xeg@public.gmane.org>
Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Cc: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/iommu/intel-iommu.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3e8636f1220e..1aa4ad7974b9 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -974,20 +974,6 @@ static int device_context_mapped(struct intel_iommu *iommu, u8 bus, u8 devfn)
 	return ret;
 }
 
-static void clear_context_table(struct intel_iommu *iommu, u8 bus, u8 devfn)
-{
-	struct context_entry *context;
-	unsigned long flags;
-
-	spin_lock_irqsave(&iommu->lock, flags);
-	context = iommu_context_addr(iommu, bus, devfn, 0);
-	if (context) {
-		context_clear_entry(context);
-		__iommu_flush_cache(iommu, context, sizeof(*context));
-	}
-	spin_unlock_irqrestore(&iommu->lock, flags);
-}
-
 static void free_context_table(struct intel_iommu *iommu)
 {
 	int i;
@@ -2351,13 +2337,33 @@ static inline int domain_pfn_mapping(struct dmar_domain *domain, unsigned long i
 
 static void domain_context_clear_one(struct intel_iommu *iommu, u8 bus, u8 devfn)
 {
+	unsigned long flags;
+	struct context_entry *context;
+	u16 did_old;
+
 	if (!iommu)
 		return;
 
-	clear_context_table(iommu, bus, devfn);
-	iommu->flush.flush_context(iommu, 0, 0, 0,
-					   DMA_CCMD_GLOBAL_INVL);
-	iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH);
+	spin_lock_irqsave(&iommu->lock, flags);
+	context = iommu_context_addr(iommu, bus, devfn, 0);
+	if (!context) {
+		spin_unlock_irqrestore(&iommu->lock, flags);
+		return;
+	}
+	did_old = context_domain_id(context);
+	context_clear_entry(context);
+	__iommu_flush_cache(iommu, context, sizeof(*context));
+	spin_unlock_irqrestore(&iommu->lock, flags);
+	iommu->flush.flush_context(iommu,
+				   did_old,
+				   (((u16)bus) << 8) | devfn,
+				   DMA_CCMD_MASK_NOBIT,
+				   DMA_CCMD_DEVICE_INVL);
+	iommu->flush.flush_iotlb(iommu,
+				 did_old,
+				 0,
+				 0,
+				 DMA_TLB_DSI_FLUSH);
 }
 
 static inline void unlink_domain_info(struct device_domain_info *info)
-- 
2.7.4

Re: [PATCH v2] iommu/vt-d: Don't be too aggressive when clearing one context entry

[Woodhouse, David via iommu]

[-- Attachment #1.1: Type: text/plain, Size: 3888 bytes --]

On Thu, 2017-08-31 at 10:58 +0200, Filippo Sironi wrote:
> Previously, we were invalidating context cache and IOTLB globally when
> clearing one context entry.  This is a tad too aggressive.
> Invalidate the context cache and IOTLB for the interested device only.
> 
> Signed-off-by: Filippo Sironi <sironi-ebkRAfMGSJGzQB+pC5nmwQ@public.gmane.org>
> Cc: David Woodhouse <dwmw-vV1OtcyAfmbQXOPxS62xeg@public.gmane.org>
> Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> Cc: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Acked-by: David Woodhouse <dwmw-vV1OtcyAfmbQXOPxS62xeg@public.gmane.org>

I'd actually quite like to see us tidy this up some more. If there are
aliases, we're doing the flush multiple times. It might be nicer to do
no flushing in domain_contact_clear_one(), which is invoked for each
alias, and just to update a (u16 *)did. If *did was empty, set it. If
it was set (i.e. if there are aliases), then set it to a magic value
like 0xFFFF. 

Then we can do the actual flush — either device-specific, or global —
just once in domain_context_clear().

We'd probably just move domain_context_clear_one() into its only caller
domain_context_clear_one_cb() while we're at it.

But in the meantime I'm more than happy with this patch which turns
multiple global flushes into multiple domain-specific flushes, and does
the right thing in the common case.

> ---

v2: Changed subject, killed clear_context_table(). Anything else?

>  drivers/iommu/intel-iommu.c | 42 ++++++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3e8636f1220e..1aa4ad7974b9 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -974,20 +974,6 @@ static int device_context_mapped(struct intel_iommu *iommu, u8 bus, u8 devfn)
>  	return ret;
>  }
>  
> -static void clear_context_table(struct intel_iommu *iommu, u8 bus, u8 devfn)
> -{
> -	struct context_entry *context;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&iommu->lock, flags);
> -	context = iommu_context_addr(iommu, bus, devfn, 0);
> -	if (context) {
> -		context_clear_entry(context);
> -		__iommu_flush_cache(iommu, context, sizeof(*context));
> -	}
> -	spin_unlock_irqrestore(&iommu->lock, flags);
> -}
> -
>  static void free_context_table(struct intel_iommu *iommu)
>  {
>  	int i;
> @@ -2351,13 +2337,33 @@ static inline int domain_pfn_mapping(struct dmar_domain *domain, unsigned long i
>  
>  static void domain_context_clear_one(struct intel_iommu *iommu, u8 bus, u8 devfn)
>  {
> +	unsigned long flags;
> +	struct context_entry *context;
> +	u16 did_old;
> +
>  	if (!iommu)
>  		return;
>  
> -	clear_context_table(iommu, bus, devfn);
> -	iommu->flush.flush_context(iommu, 0, 0, 0,
> -					   DMA_CCMD_GLOBAL_INVL);
> -	iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH);
> +	spin_lock_irqsave(&iommu->lock, flags);
> +	context = iommu_context_addr(iommu, bus, devfn, 0);
> +	if (!context) {
> +		spin_unlock_irqrestore(&iommu->lock, flags);
> +		return;
> +	}
> +	did_old = context_domain_id(context);
> +	context_clear_entry(context);
> +	__iommu_flush_cache(iommu, context, sizeof(*context));
> +	spin_unlock_irqrestore(&iommu->lock, flags);
> +	iommu->flush.flush_context(iommu,
> +				   did_old,
> +				   (((u16)bus) << 8) | devfn,

Arguably we ought to be PCI_DEVID() there, but I feel bad pointing that
out when you're adding about the fifth instance of open-coding it... :)

-- 
dwmw2

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4783 bytes --]

[-- Attachment #2.1: Type: text/plain, Size: 197 bytes --]




Amazon Web Services UK Limited. Registered in England and Wales with registration number 08650665 and which has its registered office at 60 Holborn Viaduct, London EC1A 2FD, United Kingdom.

[-- Attachment #2.2: Type: text/html, Size: 197 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2] iommu/vt-d: Don't be too aggressive when clearing one context entry

[Joerg Roedel]

On Thu, Aug 31, 2017 at 10:58:11AM +0200, Filippo Sironi wrote:
> Previously, we were invalidating context cache and IOTLB globally when
> clearing one context entry.  This is a tad too aggressive.
> Invalidate the context cache and IOTLB for the interested device only.
> 
> Signed-off-by: Filippo Sironi <sironi@amazon.de>
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: iommu@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/iommu/intel-iommu.c | 42 ++++++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 18 deletions(-)

Applied, thanks.