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

["Woodhouse, David via iommu" <iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>]

[-- 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 --]