Re: [Xen-devel] regression from c/s 22071:c5aed2e049bc (ept: Put locks around ept_get_entry) ?

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

At 20:34 +0000 on 16 Dec (1292531690), Keir Fraser wrote:
> On 16/12/2010 17:03, "Keir Fraser" <keir@xxxxxxx> wrote:
> 
> > On 16/12/2010 16:50, "Jan Beulich" <JBeulich@xxxxxxxxxx> 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.


George, I think the underlying logic is still racy - the
check-and-populate function is checking a pointer that was found outside
the lock.  It needs to start again from the beginning to be safe, which
probably means just dropping the "check" part and letting the
p2m_pod_demand_populate handle lost races.  Also, why doesn't
ept_get_entry use a single read at the lowest level?

Sorting out the locking and commenting in this code is on my
ever-expanding list of New Year's resolutions.  In the meantime, 
Keir's patch much should definitely go in:

Acked-by: Tim Deegan <Tim.Deegan@xxxxxxxxxx>

and also this to fix one other very-non-atomic write: 

Signed-off-by: Tim Deegan <Tim.Deegan@xxxxxxxxxx>

diff -r cded713fc3bd xen/arch/x86/mm/hap/p2m-ept.c
--- a/xen/arch/x86/mm/hap/p2m-ept.c     Fri Dec 17 10:16:11 2010 +0000
+++ b/xen/arch/x86/mm/hap/p2m-ept.c     Fri Dec 17 11:11:01 2010 +0000
@@ -713,7 +713,7 @@ void ept_change_entry_emt_with_range(str
 static void ept_change_entry_type_page(mfn_t ept_page_mfn, int ept_page_level,
                                        p2m_type_t ot, p2m_type_t nt)
 {
-    ept_entry_t *epte = map_domain_page(mfn_x(ept_page_mfn));
+    ept_entry_t e, *epte = map_domain_page(mfn_x(ept_page_mfn));
 
     for ( int i = 0; i < EPT_PAGETABLE_ENTRIES; i++ )
     {
@@ -725,11 +725,13 @@ static void ept_change_entry_type_page(m
                                        ept_page_level - 1, ot, nt);
         else
         {
-            if ( epte[i].sa_p2mt != ot )
+            e = epte[i];
+            if ( e.sa_p2mt != ot )
                 continue;
 
-            epte[i].sa_p2mt = nt;
-            ept_p2m_type_to_flags(epte + i, nt);
+            e.sa_p2mt = nt;
+            ept_p2m_type_to_flags(&e, nt);
+            atomic_write_ept_entry(epte + i, e);
         }
     }
 

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