From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86: fix cache flushing condition in map_pages_to_xen() Date: Wed, 17 Jul 2013 16:40:12 +0100 Message-ID: <51E6BADC.7000805@citrix.com> References: <51E6CFC802000078000E5AB8@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3441709017493112920==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1UzTq1-0007z7-DB for xen-devel@lists.xenproject.org; Wed, 17 Jul 2013 15:40:21 +0000 In-Reply-To: <51E6CFC802000078000E5AB8@nat28.tlf.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 Cc: xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org --===============3441709017493112920== Content-Type: multipart/alternative; boundary="------------070003040204010508000604" --------------070003040204010508000604 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 17/07/13 16:09, Jan Beulich wrote: > This fixes yet another shortcoming of the function (exposed by 8bfaa2c2 > ["x86: add locking to map_pages_to_xen()"]'s adjustment to > msix_put_fixmap()): It must not flush caches when transitioning to a > non-present mapping. Doing so causes the CLFLUSH to fault, if used in > favor of WBINVD. > > To help code readability, factor out the whole flush flags updating > in map_pages_to_xen() into a helper macro. > > Signed-off-by: Jan Beulich > > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -5430,6 +5430,15 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned l > flush_area_local((const void *)v, f) : \ > flush_area_all((const void *)v, f)) > > +#define flush_flags(oldf) do { \ > + unsigned int o_ = (oldf); \ > + if ( (o_) & _PAGE_GLOBAL ) \ > + flush_flags |= FLUSH_TLB_GLOBAL; \ > + if ( (flags & _PAGE_PRESENT) && \ > + (((o_) ^ flags) & PAGE_CACHE_ATTRS) ) \ > + flush_flags |= FLUSH_CACHE; \ > +} while (0) > + I have to admit to being surprised that the compiler is even happy with a macro aliasing a variable, but please can it be renamed to something else (perhaps "set_flush_flags" ?) for the sanity of other humans trying to read the code. Furthermore, are we not introducing consistency errors? Previously, we occasionally decided to flush specific cache lines, and are now conditionally not flushing the cache lines depending on the mappings. Should the fix not be "If we need to flush parts of the cache and dont have mappings to what we want to flush, use wbinvd()" ? ~Andrew > int map_pages_to_xen( > unsigned long virt, > unsigned long mfn, > @@ -5465,11 +5474,7 @@ int map_pages_to_xen( > > if ( l3e_get_flags(ol3e) & _PAGE_PSE ) > { > - if ( l3e_get_flags(ol3e) & _PAGE_GLOBAL ) > - flush_flags |= FLUSH_TLB_GLOBAL; > - if ( (lNf_to_l1f(l3e_get_flags(ol3e)) ^ flags) & > - PAGE_CACHE_ATTRS ) > - flush_flags |= FLUSH_CACHE; > + flush_flags(lNf_to_l1f(l3e_get_flags(ol3e))); > flush_area(virt, flush_flags); > } > else > @@ -5481,27 +5486,14 @@ int map_pages_to_xen( > if ( !(l2e_get_flags(ol2e) & _PAGE_PRESENT) ) > continue; > if ( l2e_get_flags(ol2e) & _PAGE_PSE ) > - { > - if ( l2e_get_flags(ol2e) & _PAGE_GLOBAL ) > - flush_flags |= FLUSH_TLB_GLOBAL; > - if ( (lNf_to_l1f(l2e_get_flags(ol2e)) ^ flags) & > - PAGE_CACHE_ATTRS ) > - flush_flags |= FLUSH_CACHE; > - } > + flush_flags(lNf_to_l1f(l2e_get_flags(ol2e))); > else > { > unsigned int j; > > pl1e = l2e_to_l1e(ol2e); > for ( j = 0; j < L1_PAGETABLE_ENTRIES; j++ ) > - { > - ol1e = pl1e[j]; > - if ( l1e_get_flags(ol1e) & _PAGE_GLOBAL ) > - flush_flags |= FLUSH_TLB_GLOBAL; > - if ( (l1e_get_flags(ol1e) ^ flags) & > - PAGE_CACHE_ATTRS ) > - flush_flags |= FLUSH_CACHE; > - } > + flush_flags(l1e_get_flags(pl1e[j])); > } > } > flush_area(virt, flush_flags); > @@ -5595,24 +5587,14 @@ int map_pages_to_xen( > > if ( l2e_get_flags(ol2e) & _PAGE_PSE ) > { > - if ( l2e_get_flags(ol2e) & _PAGE_GLOBAL ) > - flush_flags |= FLUSH_TLB_GLOBAL; > - if ( (lNf_to_l1f(l2e_get_flags(ol2e)) ^ flags) & > - PAGE_CACHE_ATTRS ) > - flush_flags |= FLUSH_CACHE; > + flush_flags(lNf_to_l1f(l2e_get_flags(ol2e))); > flush_area(virt, flush_flags); > } > else > { > pl1e = l2e_to_l1e(ol2e); > for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ ) > - { > - if ( l1e_get_flags(pl1e[i]) & _PAGE_GLOBAL ) > - flush_flags |= FLUSH_TLB_GLOBAL; > - if ( (l1e_get_flags(pl1e[i]) ^ flags) & > - PAGE_CACHE_ATTRS ) > - flush_flags |= FLUSH_CACHE; > - } > + flush_flags(l1e_get_flags(pl1e[i])); > flush_area(virt, flush_flags); > free_xen_pagetable(pl1e); > } > @@ -5687,10 +5669,8 @@ int map_pages_to_xen( > if ( (l1e_get_flags(ol1e) & _PAGE_PRESENT) ) > { > unsigned int flush_flags = FLUSH_TLB | FLUSH_ORDER(0); > - if ( l1e_get_flags(ol1e) & _PAGE_GLOBAL ) > - flush_flags |= FLUSH_TLB_GLOBAL; > - if ( (l1e_get_flags(ol1e) ^ flags) & PAGE_CACHE_ATTRS ) > - flush_flags |= FLUSH_CACHE; > + > + flush_flags(l1e_get_flags(ol1e)); > flush_area(virt, flush_flags); > } > > @@ -5769,6 +5749,8 @@ int map_pages_to_xen( > return 0; > } > > +#undef flush_flags > + > void destroy_xen_mappings(unsigned long s, unsigned long e) > { > bool_t locking = system_state > SYS_STATE_boot; > @@ -5908,6 +5890,8 @@ void destroy_xen_mappings(unsigned long > flush_area(NULL, FLUSH_TLB_GLOBAL); > } > > +#undef flush_area > + > void __set_fixmap( > enum fixed_addresses idx, unsigned long mfn, unsigned long flags) > { > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------070003040204010508000604 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 17/07/13 16:09, Jan Beulich wrote:
This fixes yet another shortcoming of the function (exposed by 8bfaa2c2
["x86: add locking to map_pages_to_xen()"]'s adjustment to
msix_put_fixmap()): It must not flush caches when transitioning to a
non-present mapping. Doing so causes the CLFLUSH to fault, if used in
favor of WBINVD.

To help code readability, factor out the whole flush flags updating
in map_pages_to_xen() into a helper macro.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5430,6 +5430,15 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned l
                          flush_area_local((const void *)v, f) : \
                          flush_area_all((const void *)v, f))
 
+#define flush_flags(oldf) do {                                  \
+    unsigned int o_ = (oldf);                                   \
+    if ( (o_) & _PAGE_GLOBAL )                                  \
+        flush_flags |= FLUSH_TLB_GLOBAL;                        \
+    if ( (flags & _PAGE_PRESENT) &&                             \
+         (((o_) ^ flags) & PAGE_CACHE_ATTRS) )                  \
+        flush_flags |= FLUSH_CACHE;                             \
+} while (0)
+

I have to admit to being surprised that the compiler is even happy with a macro aliasing a variable, but please can it be renamed to something else (perhaps "set_flush_flags" ?) for the sanity of other humans trying to read the code.

Furthermore, are we not introducing consistency errors?

Previously, we occasionally decided to flush specific cache lines, and are now conditionally not flushing the cache lines depending on the mappings.

Should the fix not be "If we need to flush parts of the cache and dont have mappings to what we want to flush, use wbinvd()" ?

~Andrew

 int map_pages_to_xen(
     unsigned long virt,
     unsigned long mfn,
@@ -5465,11 +5474,7 @@ int map_pages_to_xen(
 
                 if ( l3e_get_flags(ol3e) & _PAGE_PSE )
                 {
-                    if ( l3e_get_flags(ol3e) & _PAGE_GLOBAL )
-                        flush_flags |= FLUSH_TLB_GLOBAL;
-                    if ( (lNf_to_l1f(l3e_get_flags(ol3e)) ^ flags) &
-                         PAGE_CACHE_ATTRS )
-                        flush_flags |= FLUSH_CACHE;
+                    flush_flags(lNf_to_l1f(l3e_get_flags(ol3e)));
                     flush_area(virt, flush_flags);
                 }
                 else
@@ -5481,27 +5486,14 @@ int map_pages_to_xen(
                         if ( !(l2e_get_flags(ol2e) & _PAGE_PRESENT) )
                             continue;
                         if ( l2e_get_flags(ol2e) & _PAGE_PSE )
-                        {
-                            if ( l2e_get_flags(ol2e) & _PAGE_GLOBAL )
-                                flush_flags |= FLUSH_TLB_GLOBAL;
-                            if ( (lNf_to_l1f(l2e_get_flags(ol2e)) ^ flags) &
-                                 PAGE_CACHE_ATTRS )
-                                flush_flags |= FLUSH_CACHE;
-                        }
+                            flush_flags(lNf_to_l1f(l2e_get_flags(ol2e)));
                         else
                         {
                             unsigned int j;
 
                             pl1e = l2e_to_l1e(ol2e);
                             for ( j = 0; j < L1_PAGETABLE_ENTRIES; j++ )
-                            {
-                                ol1e = pl1e[j];
-                                if ( l1e_get_flags(ol1e) & _PAGE_GLOBAL )
-                                    flush_flags |= FLUSH_TLB_GLOBAL;
-                                if ( (l1e_get_flags(ol1e) ^ flags) &
-                                     PAGE_CACHE_ATTRS )
-                                    flush_flags |= FLUSH_CACHE;
-                            }
+                                flush_flags(l1e_get_flags(pl1e[j]));
                         }
                     }
                     flush_area(virt, flush_flags);
@@ -5595,24 +5587,14 @@ int map_pages_to_xen(
 
                 if ( l2e_get_flags(ol2e) & _PAGE_PSE )
                 {
-                    if ( l2e_get_flags(ol2e) & _PAGE_GLOBAL )
-                        flush_flags |= FLUSH_TLB_GLOBAL;
-                    if ( (lNf_to_l1f(l2e_get_flags(ol2e)) ^ flags) &
-                         PAGE_CACHE_ATTRS )
-                        flush_flags |= FLUSH_CACHE;
+                    flush_flags(lNf_to_l1f(l2e_get_flags(ol2e)));
                     flush_area(virt, flush_flags);
                 }
                 else
                 {
                     pl1e = l2e_to_l1e(ol2e);
                     for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
-                    {
-                        if ( l1e_get_flags(pl1e[i]) & _PAGE_GLOBAL )
-                            flush_flags |= FLUSH_TLB_GLOBAL;
-                        if ( (l1e_get_flags(pl1e[i]) ^ flags) &
-                             PAGE_CACHE_ATTRS )
-                            flush_flags |= FLUSH_CACHE;
-                    }
+                        flush_flags(l1e_get_flags(pl1e[i]));
                     flush_area(virt, flush_flags);
                     free_xen_pagetable(pl1e);
                 }
@@ -5687,10 +5669,8 @@ int map_pages_to_xen(
             if ( (l1e_get_flags(ol1e) & _PAGE_PRESENT) )
             {
                 unsigned int flush_flags = FLUSH_TLB | FLUSH_ORDER(0);
-                if ( l1e_get_flags(ol1e) & _PAGE_GLOBAL )
-                    flush_flags |= FLUSH_TLB_GLOBAL;
-                if ( (l1e_get_flags(ol1e) ^ flags) & PAGE_CACHE_ATTRS )
-                    flush_flags |= FLUSH_CACHE;
+
+                flush_flags(l1e_get_flags(ol1e));
                 flush_area(virt, flush_flags);
             }
 
@@ -5769,6 +5749,8 @@ int map_pages_to_xen(
     return 0;
 }
 
+#undef flush_flags
+
 void destroy_xen_mappings(unsigned long s, unsigned long e)
 {
     bool_t locking = system_state > SYS_STATE_boot;
@@ -5908,6 +5890,8 @@ void destroy_xen_mappings(unsigned long 
     flush_area(NULL, FLUSH_TLB_GLOBAL);
 }
 
+#undef flush_area
+
 void __set_fixmap(
     enum fixed_addresses idx, unsigned long mfn, unsigned long flags)
 {





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

--------------070003040204010508000604-- --===============3441709017493112920== 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 --===============3441709017493112920==--