From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86: don't use VA for cache flush when also flushing TLB Date: Mon, 26 May 2014 17:37:30 +0100 Message-ID: <53836DCA.8090907@citrix.com> References: <538330EF0200007800015B4B@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2529705118835205382==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Woxu3-0005dj-FL for xen-devel@lists.xenproject.org; Mon, 26 May 2014 16:37:35 +0000 In-Reply-To: <538330EF0200007800015B4B@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel Cc: Keir Fraser List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============2529705118835205382== Content-Type: multipart/alternative; boundary="------------060300000102090403050909" This is a multi-part message in MIME format. --------------060300000102090403050909 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit On 26/05/2014 11:17, Jan Beulich wrote: > Doing both flushes at once is a strong indication for the address > mapping to either having got dropped (in which case the cache flush, > when done via INVLPG, would fault) or its physical address having > changed (in which case the cache flush would end up being done on the > wrong address range). There is no adverse effect (other than the > obvious performance one) using WBINVD in this case regardless of the > range's size; only map_pages_to_xen() uses combined flushes at present. > > This problem was observed with the 2nd try backport of d6cb14b3 ("VT-d: > suppress UR signaling for desktop chipsets") to 4.2 (where ioremap() > needs to be replaced with set_fixmap_nocache(); the now commented out > __set_fixmap(, 0, 0) there to undo the mapping resulted in the first of > the above two scenarios). > > Signed-off-by: Jan Beulich Overall, I agree with your final assessment, and it might indeed be the easiest way, so on that basis, Reviewed-by: Andrew Cooper It would be nice to avoid the performance hit if possible. The fault case can be fixed by reording the cache flush with respect to the tlb flush inside flush_area_local. For callers performing a TLB shootdown and a cache flush, it is far more likely that the VAs wanting flushing are under the old TLB mappings rather than the new. This does come with a risk that the old TLB entry has already fallen out of the TLB before the flush happens, at which point we are back into the risk of a fault or flushing the wrong physical area. The only way I can see to safely flush bot the caches and TLB for a given change is to flush the cache by VA, then update the PTE, then shoot down the TLB entry. Despite this being awkward, I think it is preferable to falling back to wbinvd() to cover up bad behaviour in the callers. ~Andrew > > --- a/xen/arch/x86/flushtlb.c > +++ b/xen/arch/x86/flushtlb.c > @@ -152,7 +152,8 @@ void flush_area_local(const void *va, un > if ( order < (BITS_PER_LONG - PAGE_SHIFT) ) > sz = 1UL << (order + PAGE_SHIFT); > > - if ( c->x86_clflush_size && c->x86_cache_size && sz && > + if ( !(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) && > + c->x86_clflush_size && c->x86_cache_size && sz && > ((sz >> 10) < c->x86_cache_size) ) > { > va = (const void *)((unsigned long)va & ~(sz - 1)); > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------060300000102090403050909 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit
On 26/05/2014 11:17, Jan Beulich wrote:
Doing both flushes at once is a strong indication for the address
mapping to either having got dropped (in which case the cache flush,
when done via INVLPG, would fault) or its physical address having
changed (in which case the cache flush would end up being done on the
wrong address range). There is no adverse effect (other than the
obvious performance one) using WBINVD in this case regardless of the
range's size; only map_pages_to_xen() uses combined flushes at present.

This problem was observed with the 2nd try backport of d6cb14b3 ("VT-d:
suppress UR signaling for desktop chipsets") to 4.2 (where ioremap()
needs to be replaced with set_fixmap_nocache(); the now commented out
__set_fixmap(, 0, 0) there to undo the mapping resulted in the first of
the above two scenarios).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Overall, I agree with your final assessment, and it might indeed be the easiest way, so on that basis, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

It would be nice to avoid the performance hit if possible.

The fault case can be fixed by reording the cache flush with respect to the tlb flush inside flush_area_local.  For callers performing a TLB shootdown and a cache flush, it is far more likely that the VAs wanting flushing are under the old TLB mappings rather than the new.

This does come with a risk that the old TLB entry has already fallen out of the TLB before the flush happens, at which point we are back into the risk of a fault or flushing the wrong physical area.

The only way I can see to safely flush bot the caches and TLB for a given change is to flush the cache by VA, then update the PTE, then shoot down the TLB entry.  Despite this being awkward, I think it is preferable to falling back to wbinvd() to cover up bad behaviour in the callers.

~Andrew


--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -152,7 +152,8 @@ void flush_area_local(const void *va, un
         if ( order < (BITS_PER_LONG - PAGE_SHIFT) )
             sz = 1UL << (order + PAGE_SHIFT);
 
-        if ( c->x86_clflush_size && c->x86_cache_size && sz &&
+        if ( !(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) &&
+             c->x86_clflush_size && c->x86_cache_size && sz &&
              ((sz >> 10) < c->x86_cache_size) )
         {
             va = (const void *)((unsigned long)va & ~(sz - 1));






_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--------------060300000102090403050909-- --===============2529705118835205382== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============2529705118835205382==--