Re: regression from c/s 22071:c5aed2e049bc (ept: Put locks around ept_get_entry) ?

[Keir Fraser <keir@xen.org>]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

On 16/12/2010 17:03, "Keir Fraser" <keir@xen.org> wrote:

> On 16/12/2010 16:50, "Jan Beulich" <JBeulich@novell.com> wrote:
>
>>> approach therefore. Perhaps *(volatile type *)px = x or, really, even better
>>> I should define some {read,write}_atomic{8,16,32,64} accessor functions
>>> which use inline asm to absolutely definitely emit a single atomic 'mov'
>>> instruction.
>>> 
>>> Make sense?
>> 
>> Yes.
> 
> Excellent. I will lay groundwork and fix pte_{read,write}_atomic directly in
> -unstable and -4.0-testing. I will then post a proposed fix for EPT to the
> list. I don't know that code so well and I may not otherwise catch all
> places that require use of the new accessor macros.

Attached is a patch I've knocked up for p2m-ept.c. I don't know how complete
it really is. Perhaps someone (George?) would like to Ack it as is, or
develop it further.

 -- Keir

>  -- Keir
> 
>> Jan
>> 
> 
> 


[-- Attachment #2: 00-ept-atomic --]
[-- Type: application/octet-stream, Size: 3122 bytes --]

diff -r f5f3cf4e001f xen/arch/x86/mm/hap/p2m-ept.c
--- a/xen/arch/x86/mm/hap/p2m-ept.c	Thu Dec 16 20:07:03 2010 +0000
+++ b/xen/arch/x86/mm/hap/p2m-ept.c	Thu Dec 16 20:29:01 2010 +0000
@@ -32,6 +32,11 @@
 #include <xen/keyhandler.h>
 #include <xen/softirq.h>
 
+#define atomic_read_ept_entry(__pepte)                              \
+    ( (ept_entry_t) { .epte = atomic_read64(&(__pepte)->epte) } )
+#define atomic_write_ept_entry(__pepte, __epte)                     \
+    atomic_write64(&(__pepte)->epte, (__epte).epte)
+
 #define is_epte_present(ept_entry)      ((ept_entry)->epte & 0x7)
 #define is_epte_superpage(ept_entry)    ((ept_entry)->sp)
 
@@ -222,7 +227,7 @@
     /* ept_next_level() is called (sometimes) without a lock.  Read
      * the entry once, and act on the "cached" entry after that to
      * avoid races. */
-    e=*ept_entry;
+    e = atomic_read_ept_entry(ept_entry);
 
     if ( !is_epte_present(&e) )
     {
@@ -235,7 +240,7 @@
         if ( !ept_set_middle_entry(p2m, ept_entry) )
             return GUEST_TABLE_MAP_FAILED;
         else
-            e=*ept_entry; /* Refresh */
+            e = atomic_read_ept_entry(ept_entry); /* Refresh */
     }
 
     /* The only time sp would be set here is if we had hit a superpage */
@@ -317,6 +322,7 @@
     if ( i == target )
     {
         /* We reached the target level. */
+        ept_entry_t new_entry = { .epte = 0 };
 
         /* No need to flush if the old entry wasn't valid */
         if ( !is_epte_present(ept_entry) )
@@ -325,8 +331,6 @@
         if ( mfn_valid(mfn_x(mfn)) || direct_mmio || p2m_is_paged(p2mt) ||
              (p2mt == p2m_ram_paging_in_start) )
         {
-            ept_entry_t new_entry = { .epte = 0 };
-
             /* Construct the new entry, and then write it once */
             new_entry.emt = epte_get_entry_emt(p2m->domain, gfn, mfn, &ipat,
                                                 direct_mmio);
@@ -341,11 +345,9 @@
                 new_entry.mfn = mfn_x(mfn);
 
             ept_p2m_type_to_flags(&new_entry, p2mt);
+        }
 
-            ept_entry->epte = new_entry.epte;
-        }
-        else
-            ept_entry->epte = 0;
+        atomic_write_ept_entry(ept_entry, new_entry);
     }
     else
     {
@@ -355,7 +357,7 @@
 
         ASSERT(is_epte_superpage(ept_entry));
 
-        split_ept_entry = *ept_entry;
+        split_ept_entry = atomic_read_ept_entry(ept_entry);
 
         if ( !ept_split_super_page(p2m, &split_ept_entry, i, target) )
         {
@@ -365,7 +367,7 @@
 
         /* now install the newly split ept sub-tree */
         /* NB: please make sure domian is paused and no in-fly VT-d DMA. */
-        *ept_entry = split_ept_entry;
+        atomic_write_ept_entry(ept_entry, split_ept_entry);
 
         /* then move to the level we want to make real changes */
         for ( ; i > target; i-- )
@@ -391,7 +393,7 @@
 
         ept_p2m_type_to_flags(&new_entry, p2mt);
 
-        ept_entry->epte = new_entry.epte;
+        atomic_write_ept_entry(ept_entry, new_entry);
     }
 
     /* Track the highest gfn for which we have ever had a valid mapping */

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

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