* [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry @ 2017-08-28 14:16 Filippo Sironi 2017-08-30 13:31 ` Joerg Roedel ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Filippo Sironi @ 2017-08-28 14:16 UTC (permalink / raw) To: iommu, linux-kernel Cc: Filippo Sironi, David Woodhouse, David Woodhouse, Joerg Roedel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry @ 2017-08-30 13:31 ` Joerg Roedel 0 siblings, 0 replies; 12+ messages in thread From: Joerg Roedel @ 2017-08-30 13:31 UTC (permalink / raw) To: Filippo Sironi; +Cc: iommu, linux-kernel, David Woodhouse, David Woodhouse 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry @ 2017-08-30 13:31 ` Joerg Roedel 0 siblings, 0 replies; 12+ messages in thread From: Joerg Roedel @ 2017-08-30 13:31 UTC (permalink / raw) To: Filippo Sironi Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, David Woodhouse, linux-kernel-u79uwXL29TY76Z2rM5mHXA, David Woodhouse 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry @ 2017-08-31 8:41 ` Sironi, Filippo via iommu 0 siblings, 0 replies; 12+ messages in thread From: Sironi, Filippo @ 2017-08-31 8:41 UTC (permalink / raw) To: Joerg Roedel; +Cc: iommu, linux-kernel, Woodhouse, David, David Woodhouse 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry @ 2017-08-31 8:41 ` Sironi, Filippo via iommu 0 siblings, 0 replies; 12+ messages in thread From: Sironi, Filippo via iommu @ 2017-08-31 8:41 UTC (permalink / raw) To: Joerg Roedel Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, David Woodhouse, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Woodhouse, David 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry @ 2017-08-30 15:50 ` Jacob Pan 0 siblings, 0 replies; 12+ messages in thread From: Jacob Pan @ 2017-08-30 15:50 UTC (permalink / raw) To: Filippo Sironi via iommu Cc: linux-kernel, Filippo Sironi, David Woodhouse, David Woodhouse, jacob.jun.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] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry @ 2017-08-30 15:50 ` Jacob Pan 0 siblings, 0 replies; 12+ messages in thread From: Jacob Pan @ 2017-08-30 15:50 UTC (permalink / raw) To: Filippo Sironi via iommu Cc: Filippo Sironi, David Woodhouse, linux-kernel-u79uwXL29TY76Z2rM5mHXA, David Woodhouse 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] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry 2017-08-30 15:50 ` Jacob Pan (?) @ 2017-08-31 8:47 ` Sironi, Filippo -1 siblings, 0 replies; 12+ messages in thread From: Sironi, Filippo @ 2017-08-31 8:47 UTC (permalink / raw) To: Jacob Pan Cc: Filippo Sironi via iommu, linux-kernel, David Woodhouse, Woodhouse, David 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] iommu/vt-d: Don't be too aggressive when clearing one context entry @ 2017-08-31 8:58 ` Filippo Sironi via iommu 0 siblings, 0 replies; 12+ messages in thread From: Filippo Sironi @ 2017-08-31 8:58 UTC (permalink / raw) To: iommu, linux-kernel Cc: Filippo Sironi, David Woodhouse, David Woodhouse, Joerg Roedel, Jacob Pan 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2] iommu/vt-d: Don't be too aggressive when clearing one context entry @ 2017-08-31 8:58 ` Filippo Sironi via iommu 0 siblings, 0 replies; 12+ messages in thread From: Filippo Sironi via iommu @ 2017-08-31 8:58 UTC (permalink / raw) To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-kernel-u79uwXL29TY76Z2rM5mHXA Cc: Filippo Sironi, David Woodhouse, David Woodhouse 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
[parent not found: <1504169891-9480-1-git-send-email-sironi-ebkRAfMGSJGzQB+pC5nmwQ@public.gmane.org>]
* Re: [PATCH v2] iommu/vt-d: Don't be too aggressive when clearing one context entry [not found] ` <1504169891-9480-1-git-send-email-sironi-ebkRAfMGSJGzQB+pC5nmwQ@public.gmane.org> @ 2017-08-31 10:21 ` Woodhouse, David via iommu 0 siblings, 0 replies; 12+ messages in thread From: Woodhouse, David via iommu @ 2017-08-31 10:21 UTC (permalink / raw) To: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Sironi, Filippo, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] iommu/vt-d: Don't be too aggressive when clearing one context entry 2017-08-31 8:58 ` Filippo Sironi via iommu (?) (?) @ 2017-09-01 9:31 ` Joerg Roedel -1 siblings, 0 replies; 12+ messages in thread From: Joerg Roedel @ 2017-09-01 9:31 UTC (permalink / raw) To: Filippo Sironi Cc: iommu, linux-kernel, David Woodhouse, David Woodhouse, Jacob Pan 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-09-01 9:31 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-28 14:16 [PATCH] intel-iommu: Don't be too aggressive when clearing one context entry Filippo Sironi 2017-08-30 13:31 ` Joerg Roedel 2017-08-30 13:31 ` Joerg Roedel 2017-08-31 8:41 ` Sironi, Filippo 2017-08-31 8:41 ` Sironi, Filippo via iommu 2017-08-30 15:50 ` Jacob Pan 2017-08-30 15:50 ` Jacob Pan 2017-08-31 8:47 ` Sironi, Filippo 2017-08-31 8:58 ` [PATCH v2] iommu/vt-d: " Filippo Sironi 2017-08-31 8:58 ` Filippo Sironi via iommu [not found] ` <1504169891-9480-1-git-send-email-sironi-ebkRAfMGSJGzQB+pC5nmwQ@public.gmane.org> 2017-08-31 10:21 ` Woodhouse, David via iommu 2017-09-01 9:31 ` Joerg Roedel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.