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

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

[Jan Beulich]

George,

with ept_get_entry() calling ept_pod_check_and_populate(), which in
turn unconditionally calls p2m_lock(), we got a report of the BUG() in
p2m_lock() triggering (in a backport of the patch on top of 4.0.1 - the
logic seems unchanged in -unstable, and hence the issue ought to
similarly exist there). Wouldn't therefore ept_pod_check_and_populate(),
only ever called from ept_get_entry(), not need to do any locking on
its own at all?

Thanks, Jan

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

[George Dunlap]

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

Yes, I have to apologize: I have a queue of PoD, p2m, and ept-related
fixes that I haven't pushed to the list because:
* they require non-negligible reworking
* it's been really difficult for me to set up an OSS-based system to test them

It actually turns out that doing locking in ept_get_entry() is the
wrong thing to do anyway; it can cause the following deadlock:

p2m_change_type [grabs p2m lock] -> set_p2m_entry -> ept_set_entry ->
ept_set_middle_level -> p2m_alloc [grabs hap lock]

write cr4 -> hap_update_paging_modes [grabes hap lock] ->
hap_update_cr3 -> gfn_to_mfn -> ept_get_entry -> [grabs p2m lock]

Attached is a ported patch that removes locking in ept_get_entry(),
and implements access-once semantics for reading and writing.  This
solves the original problem (a race between reading and writing the
table) without causing deadlocks.  I haven't had a chance to test it
-- can you give it a spin?

Thanks,
 -George

On Tue, Dec 14, 2010 at 8:39 AM, Jan Beulich <JBeulich@novell.com> wrote:
> George,
>
> with ept_get_entry() calling ept_pod_check_and_populate(), which in
> turn unconditionally calls p2m_lock(), we got a report of the BUG() in
> p2m_lock() triggering (in a backport of the patch on top of 4.0.1 - the
> logic seems unchanged in -unstable, and hence the issue ought to
> similarly exist there). Wouldn't therefore ept_pod_check_and_populate(),
> only ever called from ept_get_entry(), not need to do any locking on
> its own at all?
>
> Thanks, Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

[-- Attachment #2: ept-access-once.diff --]
[-- Type: text/x-diff, Size: 5154 bytes --]

ept: Remove lock in ept_get_entry, replace with access-once semantics.

This mirrors the RVI/shadow situation, where p2m read access is lockless
because it's done in the hardware (linear map of the p2m table).

This fixes the original bug (call it bug A) without introducing bug B (a deadlock).

Bug A was caused by a race when updating p2m entries: between testing if it's valid,
and testing if it's populate-on-demand, it may have been changed from populate-on-demand to valid.

My original patch simply introduced a lock into ept_get_entry, but that
caused bug B, caused by circular locking order:
p2m_change_type [grabs p2m lock] -> set_p2m_entry -> ept_set_entry -> ept_set_middle_level -> p2m_alloc [grabs hap lock]
write cr4 -> hap_update_paging_modes [grabes hap lock] -> hap_update_cr3 -> gfn_to_mfn -> ept_get_entry -> [grabs p2m lock]

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 49d2aa5cee4e xen/arch/x86/mm/hap/p2m-ept.c
--- a/xen/arch/x86/mm/hap/p2m-ept.c	Thu Dec 09 16:17:33 2010 +0000
+++ b/xen/arch/x86/mm/hap/p2m-ept.c	Tue Dec 14 10:36:43 2010 +0000
@@ -211,7 +211,7 @@
                           int next_level)
 {
     unsigned long mfn;
-    ept_entry_t *ept_entry;
+    ept_entry_t *ept_entry, e;
     u32 shift, index;
 
     shift = next_level * EPT_TABLE_ORDER;
@@ -223,9 +223,14 @@
 
     ept_entry = (*table) + index;
 
-    if ( !is_epte_present(ept_entry) )
+    /* 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;
+
+    if ( !is_epte_present(&e) )
     {
-        if ( ept_entry->avail1 == p2m_populate_on_demand )
+        if ( e.avail1 == p2m_populate_on_demand )
             return GUEST_TABLE_POD_PAGE;
 
         if ( read_only )
@@ -233,13 +238,15 @@
 
         if ( !ept_set_middle_entry(p2m, ept_entry) )
             return GUEST_TABLE_MAP_FAILED;
+        else
+            e=*ept_entry; /* Refresh */
     }
 
     /* The only time sp would be set here is if we had hit a superpage */
-    if ( is_epte_superpage(ept_entry) )
+    if ( is_epte_superpage(&e) )
         return GUEST_TABLE_SUPER_PAGE;
 
-    mfn = ept_entry->mfn;
+    mfn = e.mfn;
     unmap_domain_page(*table);
     *table = map_domain_page(mfn);
     *gfn_remainder &= (1UL << shift) - 1;
@@ -318,19 +325,24 @@
         if ( mfn_valid(mfn_x(mfn)) || direct_mmio || p2m_is_paged(p2mt) ||
              (p2mt == p2m_ram_paging_in_start) )
         {
-            ept_entry->emt = epte_get_entry_emt(p2m->domain, gfn, mfn, &ipat,
+            ept_entry_t new_entry;
+
+            /* Construct the new entry, and then write it once */
+            new_entry.emt = epte_get_entry_emt(p2m->domain, gfn, mfn, &ipat,
                                                 direct_mmio);
-            ept_entry->ipat = ipat;
-            ept_entry->sp = order ? 1 : 0;
-            ept_entry->avail1 = p2mt;
-            ept_entry->avail2 = 0;
+            new_entry.ipat = ipat;
+            new_entry.sp = order ? 1 : 0;
+            new_entry.avail1 = p2mt;
+            new_entry.avail2 = 0;
 
-            if ( ept_entry->mfn == mfn_x(mfn) )
+            if ( new_entry.mfn == mfn_x(mfn) )
                 need_modify_vtd_table = 0;
             else
-                ept_entry->mfn = mfn_x(mfn);
+                new_entry.mfn = mfn_x(mfn);
 
-            ept_p2m_type_to_flags(ept_entry, p2mt);
+            ept_p2m_type_to_flags(&new_entry, p2mt);
+
+            ept_entry->epte = new_entry.epte;
         }
         else
             ept_entry->epte = 0;
@@ -339,6 +351,7 @@
     {
         /* We need to split the original page. */
         ept_entry_t split_ept_entry;
+        ept_entry_t new_entry;
 
         ASSERT(is_epte_superpage(ept_entry));
 
@@ -365,18 +378,20 @@
 
         ept_entry = table + index;
 
-        ept_entry->emt = epte_get_entry_emt(d, gfn, mfn, &ipat, direct_mmio);
-        ept_entry->ipat = ipat;
-        ept_entry->sp = i ? 1 : 0;
-        ept_entry->avail1 = p2mt;
-        ept_entry->avail2 = 0;
+        new_entry.emt = epte_get_entry_emt(d, gfn, mfn, &ipat, direct_mmio);
+        new_entry.ipat = ipat;
+        new_entry.sp = i ? 1 : 0;
+        new_entry.avail1 = p2mt;
+        new_entry.avail2 = 0;
 
-        if ( ept_entry->mfn == mfn_x(mfn) )
+        if ( new_entry.mfn == mfn_x(mfn) )
              need_modify_vtd_table = 0;
         else /* the caller should take care of the previous page */
-            ept_entry->mfn = mfn_x(mfn);
+            new_entry.mfn = mfn_x(mfn);
 
-        ept_p2m_type_to_flags(ept_entry, p2mt);
+        ept_p2m_type_to_flags(&new_entry, p2mt);
+
+        ept_entry->epte = new_entry.epte;
     }
 
     /* Track the highest gfn for which we have ever had a valid mapping */
@@ -437,10 +452,6 @@
     int i;
     int ret = 0;
     mfn_t mfn = _mfn(INVALID_MFN);
-    int do_locking = !p2m_locked_by_me(p2m);
-
-    if ( do_locking )
-        p2m_lock(p2m);
 
     *t = p2m_mmio_dm;
 
@@ -517,8 +528,6 @@
     }
 
 out:
-    if ( do_locking )
-        p2m_unlock(p2m);
     unmap_domain_page(table);
     return mfn;
 }

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

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

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

[George Dunlap]

On Tue, Dec 14, 2010 at 10:47 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> I haven't had a chance to test it
> -- can you give it a spin?

I should say, the XenServer version of the patch (against 3.4) has
been thoroughly tested, but doesn't apply cleanly to -unstable.  So
the concept is tested, it's just this particular patch that's
untested.

 -George

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

[Jan Beulich]

>>> On 14.12.10 at 11:47, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> Yes, I have to apologize: I have a queue of PoD, p2m, and ept-related
> fixes that I haven't pushed to the list because:
> * they require non-negligible reworking
> * it's been really difficult for me to set up an OSS-based system to test 
> them
> 
> It actually turns out that doing locking in ept_get_entry() is the
> wrong thing to do anyway; it can cause the following deadlock:
> 
> p2m_change_type [grabs p2m lock] -> set_p2m_entry -> ept_set_entry ->
> ept_set_middle_level -> p2m_alloc [grabs hap lock]
> 
> write cr4 -> hap_update_paging_modes [grabes hap lock] ->
> hap_update_cr3 -> gfn_to_mfn -> ept_get_entry -> [grabs p2m lock]
> 
> Attached is a ported patch that removes locking in ept_get_entry(),
> and implements access-once semantics for reading and writing.  This
> solves the original problem (a race between reading and writing the
> table) without causing deadlocks.  I haven't had a chance to test it
> -- can you give it a spin?

For really giving this a try I'd have to use it on 4.0, where it
doesn't apply at all. Resolving the rejects is non-obvious for me
in some cases, as I don't know this code well enough. Hence
for the moment we'll just drop the bad backport of your first
attempt.

Jan

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

[George Dunlap]

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

Try this one.

FYI, the logic is pretty simple: Without this patch, ept_next_level()
gets a pointer and the logic goes through and reads the actual entry
piecemeal.  ept_set_entry() gets a pointer and goes through setting
bits in the actual entry piecemeal as well.  The idea is, have
ept_next_level() read the entire entry into a local variable, and then
act on that; and have ept_set_entry() write the new entry into a local
variable and then write the whole entry once.  So it's mostly changing
"ept_entry->" to "new_entry."

 -George

On Tue, Dec 14, 2010 at 12:37 PM, Jan Beulich <JBeulich@novell.com> wrote:
>>>> On 14.12.10 at 11:47, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>> Yes, I have to apologize: I have a queue of PoD, p2m, and ept-related
>> fixes that I haven't pushed to the list because:
>> * they require non-negligible reworking
>> * it's been really difficult for me to set up an OSS-based system to test
>> them
>>
>> It actually turns out that doing locking in ept_get_entry() is the
>> wrong thing to do anyway; it can cause the following deadlock:
>>
>> p2m_change_type [grabs p2m lock] -> set_p2m_entry -> ept_set_entry ->
>> ept_set_middle_level -> p2m_alloc [grabs hap lock]
>>
>> write cr4 -> hap_update_paging_modes [grabes hap lock] ->
>> hap_update_cr3 -> gfn_to_mfn -> ept_get_entry -> [grabs p2m lock]
>>
>> Attached is a ported patch that removes locking in ept_get_entry(),
>> and implements access-once semantics for reading and writing.  This
>> solves the original problem (a race between reading and writing the
>> table) without causing deadlocks.  I haven't had a chance to test it
>> -- can you give it a spin?
>
> For really giving this a try I'd have to use it on 4.0, where it
> doesn't apply at all. Resolving the rejects is non-obvious for me
> in some cases, as I don't know this code well enough. Hence
> for the moment we'll just drop the bad backport of your first
> attempt.
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

[-- Attachment #2: ept-access-once.4.0.diff --]
[-- Type: text/x-diff, Size: 4227 bytes --]

CA-46536: Remove lock in ept_get_entry, replace with access-once semantics.

This mirrors the RVI/shadow situation, where p2m read access is lockless
because it's done in the hardware (linear map of the p2m table).

This fixes bug CA-44168 without introducing CA-46536.

CA-44168 was caused by a race when updating p2m entries: between testing if it's valid,
and testing if it's populate-on-demand, it may have been changed from populate-on-demand to valid.

My original patch simply introduced a lock into ept_get_entry, but that
caused CA-46536, caused by circular locking order:
p2m_change_type [grabs p2m lock] -> set_p2m_entry -> ept_set_entry -> ept_set_middle_level -> p2m_alloc [grabs hap lock]
write cr4 -> hap_update_paging_modes [grabes hap lock] -> hap_update_cr3 -> gfn_to_mfn -> ept_get_entry -> [grabs p2m lock]

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 116a979f9d0f xen/arch/x86/mm/hap/p2m-ept.c
--- a/xen/arch/x86/mm/hap/p2m-ept.c	Thu Dec 09 16:15:55 2010 +0000
+++ b/xen/arch/x86/mm/hap/p2m-ept.c	Tue Dec 14 14:26:17 2010 +0000
@@ -137,7 +137,7 @@
                           ept_entry_t **table, unsigned long *gfn_remainder,
                           u32 shift)
 {
-    ept_entry_t *ept_entry;
+    ept_entry_t *ept_entry, e;
     ept_entry_t *next;
     u32 index;
 
@@ -145,9 +145,11 @@
 
     ept_entry = (*table) + index;
 
-    if ( !is_epte_present(ept_entry) )
+    e=*ept_entry;
+
+    if ( !is_epte_present(&e) )
     {
-        if ( ept_entry->avail1 == p2m_populate_on_demand )
+        if ( e.avail1 == p2m_populate_on_demand )
             return GUEST_TABLE_POD_PAGE;
 
         if ( read_only )
@@ -155,15 +157,17 @@
 
         if ( !ept_set_middle_entry(d, ept_entry) )
             return GUEST_TABLE_MAP_FAILED;
+        else
+            e=*ept_entry;
     }
 
     /* The only time sp would be set here is if we had hit a superpage */
-    if ( is_epte_superpage(ept_entry) )
+    if ( is_epte_superpage(&e) )
         return GUEST_TABLE_SUPER_PAGE;
     else
     {
         *gfn_remainder &= (1UL << shift) - 1;
-        next = map_domain_page(ept_entry->mfn);
+        next = map_domain_page(e.mfn);
         unmap_domain_page(*table);
         *table = next;
         return GUEST_TABLE_NORMAL_PAGE;
@@ -235,35 +239,39 @@
         if ( mfn_valid(mfn_x(mfn)) || direct_mmio || p2m_is_paged(p2mt) ||
              (p2mt == p2m_ram_paging_in_start) )
         {
-            ept_entry->emt = epte_get_entry_emt(d, gfn, mfn, &ipat,
+            ept_entry_t new_entry;
+
+            new_entry.emt = epte_get_entry_emt(d, gfn, mfn, &ipat,
                                                 direct_mmio);
-            ept_entry->ipat = ipat;
-            ept_entry->sp = order ? 1 : 0;
+            new_entry.ipat = ipat;
+            new_entry.sp = order ? 1 : 0;
 
             if ( ret == GUEST_TABLE_SUPER_PAGE )
             {
-                if ( ept_entry->mfn == (mfn_x(mfn) - offset) )
+                if ( new_entry.mfn == (mfn_x(mfn) - offset) )
                     need_modify_vtd_table = 0;  
                 else                  
-                    ept_entry->mfn = mfn_x(mfn) - offset;
+                    new_entry.mfn = mfn_x(mfn) - offset;
 
-                if ( (ept_entry->avail1 == p2m_ram_logdirty)
+                if ( (new_entry.avail1 == p2m_ram_logdirty)
                      && (p2mt == p2m_ram_rw) )
                     for ( i = 0; i < 512; i++ )
                         paging_mark_dirty(d, mfn_x(mfn) - offset + i);
             }
             else
             {
-                if ( ept_entry->mfn == mfn_x(mfn) )
+                if ( new_entry.mfn == mfn_x(mfn) )
                     need_modify_vtd_table = 0;
                 else
-                    ept_entry->mfn = mfn_x(mfn);
+                    new_entry.mfn = mfn_x(mfn);
             }
 
-            ept_entry->avail1 = p2mt;
-            ept_entry->avail2 = 0;
+            new_entry.avail1 = p2mt;
+            new_entry.avail2 = 0;
 
-            ept_p2m_type_to_flags(ept_entry, p2mt);
+            ept_p2m_type_to_flags(&new_entry, p2mt);
+
+            ept_entry->epte = new_entry.epte;
         }
         else
             ept_entry->epte = 0;

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

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

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

[George Dunlap]

BTW, let me know if you want the other PoD / p2m / ept patches I
haven't upstreamed yet.  Even if you don't end up backporting them,
they may be handy to have if there are problems later.

 -George

On Tue, Dec 14, 2010 at 2:32 PM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> Try this one.
>
> FYI, the logic is pretty simple: Without this patch, ept_next_level()
> gets a pointer and the logic goes through and reads the actual entry
> piecemeal.  ept_set_entry() gets a pointer and goes through setting
> bits in the actual entry piecemeal as well.  The idea is, have
> ept_next_level() read the entire entry into a local variable, and then
> act on that; and have ept_set_entry() write the new entry into a local
> variable and then write the whole entry once.  So it's mostly changing
> "ept_entry->" to "new_entry."
>
>  -George
>
> On Tue, Dec 14, 2010 at 12:37 PM, Jan Beulich <JBeulich@novell.com> wrote:
>>>>> On 14.12.10 at 11:47, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>>> Yes, I have to apologize: I have a queue of PoD, p2m, and ept-related
>>> fixes that I haven't pushed to the list because:
>>> * they require non-negligible reworking
>>> * it's been really difficult for me to set up an OSS-based system to test
>>> them
>>>
>>> It actually turns out that doing locking in ept_get_entry() is the
>>> wrong thing to do anyway; it can cause the following deadlock:
>>>
>>> p2m_change_type [grabs p2m lock] -> set_p2m_entry -> ept_set_entry ->
>>> ept_set_middle_level -> p2m_alloc [grabs hap lock]
>>>
>>> write cr4 -> hap_update_paging_modes [grabes hap lock] ->
>>> hap_update_cr3 -> gfn_to_mfn -> ept_get_entry -> [grabs p2m lock]
>>>
>>> Attached is a ported patch that removes locking in ept_get_entry(),
>>> and implements access-once semantics for reading and writing.  This
>>> solves the original problem (a race between reading and writing the
>>> table) without causing deadlocks.  I haven't had a chance to test it
>>> -- can you give it a spin?
>>
>> For really giving this a try I'd have to use it on 4.0, where it
>> doesn't apply at all. Resolving the rejects is non-obvious for me
>> in some cases, as I don't know this code well enough. Hence
>> for the moment we'll just drop the bad backport of your first
>> attempt.
>>
>> Jan
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>>
>

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

[Jan Beulich]

>>> On 14.12.10 at 15:34, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> BTW, let me know if you want the other PoD / p2m / ept patches I
> haven't upstreamed yet.  Even if you don't end up backporting them,
> they may be handy to have if there are problems later.

Depends on your (time-wise) plans for submitting them for inclusion.
If that's going to take a while, it would certainly be nice to have them
at hand.

Thanks, Jan

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

[Jan Beulich]

>>> On 14.12.10 at 11:47, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> Attached is a ported patch that removes locking in ept_get_entry(),
> and implements access-once semantics for reading and writing.  This
> solves the original problem (a race between reading and writing the
> table) without causing deadlocks.  I haven't had a chance to test it
> -- can you give it a spin?

I think this is missing some barrier() instances (or volatile
qualifiers). Without them, I don't think there's a guarantee
that the single memory access in the source won't be
converted to multiple ones at the compiler's discretion.

Jan

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

[Keir Fraser]

On 16/12/2010 15:51, "Jan Beulich" <JBeulich@novell.com> wrote:

>>>> On 14.12.10 at 11:47, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>> Attached is a ported patch that removes locking in ept_get_entry(),
>> and implements access-once semantics for reading and writing.  This
>> solves the original problem (a race between reading and writing the
>> table) without causing deadlocks.  I haven't had a chance to test it
>> -- can you give it a spin?
> 
> I think this is missing some barrier() instances (or volatile
> qualifiers). Without them, I don't think there's a guarantee
> that the single memory access in the source won't be
> converted to multiple ones at the compiler's discretion.

Probably a similar assumption to what we make in x86_64's pte_write_atomic()
implementation? Possibly pte_{read,write}_atomic() should cast the pte
pointer to volatile, and the EPT reads/writes should be similarly wrapped in
macros which do casting. I'm sure we make various other assumptions about
read/write atomicity in Xen, but aiming to fix them as we find them is maybe
not a bad idea.

If that sounds good, I can propose a patch?

 -- Keir

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

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

[Jan Beulich]

>>> On 16.12.10 at 17:12, Keir Fraser <keir@xen.org> wrote:
> On 16/12/2010 15:51, "Jan Beulich" <JBeulich@novell.com> wrote:
> 
>>>>> On 14.12.10 at 11:47, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>>> Attached is a ported patch that removes locking in ept_get_entry(),
>>> and implements access-once semantics for reading and writing.  This
>>> solves the original problem (a race between reading and writing the
>>> table) without causing deadlocks.  I haven't had a chance to test it
>>> -- can you give it a spin?
>> 
>> I think this is missing some barrier() instances (or volatile
>> qualifiers). Without them, I don't think there's a guarantee
>> that the single memory access in the source won't be
>> converted to multiple ones at the compiler's discretion.
> 
> Probably a similar assumption to what we make in x86_64's pte_write_atomic()
> implementation? Possibly pte_{read,write}_atomic() should cast the pte
> pointer to volatile, and the EPT reads/writes should be similarly wrapped in
> macros which do casting. I'm sure we make various other assumptions about
> read/write atomicity in Xen, but aiming to fix them as we find them is maybe
> not a bad idea.
> 
> If that sounds good, I can propose a patch?

Oh, yes. I didn't even consider there might be more places.

What I'm surprised about is you suggesting to take the "volatile"
route instead of the barrier() one...

Jan

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

[Keir Fraser]

On 16/12/2010 16:22, "Jan Beulich" <JBeulich@novell.com> wrote:

>> Probably a similar assumption to what we make in x86_64's pte_write_atomic()
>> implementation? Possibly pte_{read,write}_atomic() should cast the pte
>> pointer to volatile, and the EPT reads/writes should be similarly wrapped in
>> macros which do casting. I'm sure we make various other assumptions about
>> read/write atomicity in Xen, but aiming to fix them as we find them is maybe
>> not a bad idea.
>> 
>> If that sounds good, I can propose a patch?
> 
> Oh, yes. I didn't even consider there might be more places.
> 
> What I'm surprised about is you suggesting to take the "volatile"
> route instead of the barrier() one...

I don't think barrier() would solve the problem at hand. The idiom we are
dealing with is something like:
 x = *px;
 [barrier()]
 <mess with fields in x>
 [barrier()]
 *px = x;

I don't see that adding the bracketed barrier() calls above ensures that the
access to *px are done in a single atomic instruction. There's nothing
touching non-local variables between the two barrier()s, so for example the
code that messes with x could be moved after the second barrier() and then
the compiler could choose to mess with *px directly if it wishes.

The issue is not one of serialisation or code ordering. It is one of
memory-access atomicity. Thus it seems to me that volatile is the correct
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?

 -- Keir

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

[Jan Beulich]

>>> On 16.12.10 at 17:42, Keir Fraser <keir@xen.org> wrote:
> The issue is not one of serialisation or code ordering. It is one of
> memory-access atomicity. Thus it seems to me that volatile is the correct

Indeed, I agree.

> 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.

Jan

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

[Keir Fraser]

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

> On 16/12/2010 16:22, "Jan Beulich" <JBeulich@novell.com> wrote:
> 
>>> Probably a similar assumption to what we make in x86_64's pte_write_atomic()
>>> implementation? Possibly pte_{read,write}_atomic() should cast the pte
>>> pointer to volatile, and the EPT reads/writes should be similarly wrapped in
>>> macros which do casting. I'm sure we make various other assumptions about
>>> read/write atomicity in Xen, but aiming to fix them as we find them is maybe
>>> not a bad idea.
>>> 
>>> If that sounds good, I can propose a patch?
>> 
>> Oh, yes. I didn't even consider there might be more places.
>> 
>> What I'm surprised about is you suggesting to take the "volatile"
>> route instead of the barrier() one...
> 
> I don't think barrier() would solve the problem at hand. The idiom we are
> dealing with is something like:
>  x = *px;
>  [barrier()]
>  <mess with fields in x>
>  [barrier()]
>  *px = x;
> 
> I don't see that adding the bracketed barrier() calls above ensures that the
> access to *px are done in a single atomic instruction. There's nothing
> touching non-local variables between the two barrier()s, so for example the
> code that messes with x could be moved after the second barrier() and then
> the compiler could choose to mess with *px directly if it wishes.

Or in George's EPT changes, I think the issue was getting an atomic snapshot
of some P2M flags (populate-on-demand vs. valid vs. ...). Again, barrier()
would not help since <mess with fields in x> could be moved before the first
barrier(), and again the compiler can then do a number of direct reads on
*px. So, again, the right fix is to make the memory read properly atomic via
use of volatile, and preferably a snippet of inline asm to guarantee we emit
the desired single mov instruction.

 -- Keir

> The issue is not one of serialisation or code ordering. It is one of
> memory-access atomicity. Thus it seems to me that volatile is the correct
> 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?
> 
>  -- Keir
> 
> 

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

[Keir Fraser]

On 16/12/2010 16:50, "Jan Beulich" <JBeulich@novell.com> wrote:

>>>> On 16.12.10 at 17:42, Keir Fraser <keir@xen.org> wrote:
>> The issue is not one of serialisation or code ordering. It is one of
>> memory-access atomicity. Thus it seems to me that volatile is the correct
> 
> Indeed, I agree.
> 
>> 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.

 -- Keir

> Jan
> 

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

[Keir Fraser]

[-- 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

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

[Tim Deegan]

At 20:34 +0000 on 16 Dec (1292531690), Keir Fraser wrote:
> 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.


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@citrix.com>

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

Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>

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);
         }
     }

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

[Olaf Hering]

On Thu, Dec 16, Keir Fraser wrote:

> 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.

Keir,

this failure may be related to the changes that went just into
xen-unstable, fails in openSuSE 11.2 and 11.3 on 32bit:

make[2]: Entering directory `/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/arch/x86'
gcc -fomit-frame-pointer -fmessage-length=0 -O2 -Wall  -D_FORTIFY_SOURCE=2 -fstack-protector -funwind-tables  -fasynchronous-unwind-tables -O1 -fno-omit-frame-pointer  -fno-optimize-sibling-calls -m32 -march=i686 -g -fno-strict-aliasing  -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-value  -Wdeclaration-after-statement  -nostdinc -fno-builtin -fno-common  -Wredundant-decls -iwithprefix include -Werror -Wno-pointer-arith -pipe  -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include   -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm-x86/mach-generic   -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm-x86/mach-default  -msoft-float -fno-stack-protector -fno-exceptions -g -D__XEN__  -DVERBOSE -DCRASH_DEBUG -fno-omit-frame-pointer -DCONFIG_FRAME_POINTER  -DMAX_PHYS_CPUS=32 -MMD -MF .xen.d -O1 -fno-omit-frame-pointer  -fno-optimize-sibling-calls -m32 -march=i686 -g -fno-strict-aliasing  -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-value  -Wdeclaration-after-statement  -nostdinc -fno-builtin -fno-common  -Wredundant-decls -iwithprefix include -Werror -Wno-pointer-arith -pipe  -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include   -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm-x86/mach-generic   -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm-x86/mach-default  -msoft-float -fno-stack-protector -fno-exceptions -g -D__XEN__  -DVERBOSE -DCRASH_DEBUG -fno-omit-frame-pointer -DCONFIG_FRAME_POINTER  -DMAX_PHYS_CPUS=32 -MMD -MF .asm-offsets.s.d -S -o asm-offsets.s  x86_32/asm-offsets.c
cc1: warnings being treated as errors
In file included from /usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm/spinlock.h:6,
                 from /usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/xen/spinlock.h:6,
                 from /usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/xen/sched.h:7,
                 from x86_32/asm-offsets.c:9:
/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm/atomic.h: In function 'atomic_write64':
/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm/atomic.h:39: error: operation on 'old' may be undefined
make[2]: *** [asm-offsets.s] Error 1
make[2]: Leaving directory `/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/arch/x86'

 36 static inline void atomic_write64(volatile uint64_t *addr, uint64_t val)
 37 {
 38     uint64_t old = *addr, new, *__addr = (uint64_t *)addr;
 39     while ( (old = __cmpxchg8b(__addr, old, val)) != old )
 40         old = new;
 41 }

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

[Keir Fraser]

On 17/12/2010 14:03, "Olaf Hering" <olaf@aepfle.de> wrote:

> On Thu, Dec 16, Keir Fraser wrote:
> 
>> 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.
> 
> Keir,
> 
> this failure may be related to the changes that went just into
> xen-unstable, fails in openSuSE 11.2 and 11.3 on 32bit:

Oops, I stared at the atomic_write64() implementation a while, I had an old
version to basically copy across, and still I got it wrong. It's fixed by
xen-unstable c/s 22572.

 Thanks,
 Keir

> make[2]: Entering directory `/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571
> /xen/arch/x86'
> gcc -fomit-frame-pointer -fmessage-length=0 -O2 -Wall 
> -D_FORTIFY_SOURCE=2 -fstack-protector -funwind-tables 
> -fasynchronous-unwind-tables -O1 -fno-omit-frame-pointer 
> -fno-optimize-sibling-calls -m32 -march=i686 -g -fno-strict-aliasing 
> -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-value 
> -Wdeclaration-after-statement  -nostdinc -fno-builtin -fno-common 
> -Wredundant-decls -iwithprefix include -Werror -Wno-pointer-arith -pipe 
> -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include  
> -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm-x86/mach-g
> eneric   
> -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm-x86/mach-d
> efault  -msoft-float -fno-stack-protector -fno-exceptions -g -D__XEN__ 
> -DVERBOSE -DCRASH_DEBUG -fno-omit-frame-pointer -DCONFIG_FRAME_POINTER 
> -DMAX_PHYS_CPUS=32 -MMD -MF .xen.d -O1 -fno-omit-frame-pointer 
> -fno-optimize-sibling-calls -m32 -march=i686 -g -fno-strict-aliasing 
> -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-value 
> -Wdeclaration-after-statement  -nostdinc -fno-builtin -fno-common 
> -Wredundant-decls -iwithprefix include -Werror -Wno-pointer-arith -pipe 
> -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include  
> -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm-x86/mach-g
> eneric   
> -I/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm-x86/mach-d
> efault  -msoft-float -fno-stack-protector -fno-exceptions -g -D__XEN__ 
> -DVERBOSE -DCRASH_DEBUG -fno-omit-frame-pointer -DCONFIG_FRAME_POINTER 
> -DMAX_PHYS_CPUS=32 -MMD -MF .asm-offsets.s.d -S -o asm-offsets.s 
> x86_32/asm-offsets.c
> cc1: warnings being treated as errors
> In file included from /usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/in
> clude/asm/spinlock.h:6,
>                  from /usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/in
> clude/xen/spinlock.h:6,
>                  from /usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/in
> clude/xen/sched.h:7,
>                  from x86_32/asm-offsets.c:9:
> /usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm/atomic.h: In
>  function 'atomic_write64':
> /usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/xen/include/asm/atomic.h:39:
>  error: operation on 'old' may be undefined
> make[2]: *** [asm-offsets.s] Error 1
> make[2]: Leaving directory `/usr/src/packages/BUILD/xen-unstable.hg-4.1.22571/
> xen/arch/x86'
> 
>  36 static inline void atomic_write64(volatile uint64_t *addr, uint64_t val)
>  37 {
>  38     uint64_t old = *addr, new, *__addr = (uint64_t *)addr;
>  39     while ( (old = __cmpxchg8b(__addr, old, val)) != old )
>  40         old = new;
>  41 }
> 

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

[George Dunlap]

Sorry for the delay in responding; I caught a bad cold, and was in no
state to comment on making lockless races benign. :-)

On Fri, Dec 17, 2010 at 11:15 AM, Tim Deegan <Tim.Deegan@citrix.com> wrote:
> 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?

My main goal when I wrote this patch was to fix a bug our testing
found that was going to slip a release, and then move on to other
pressing issues.  So I'm sure there's more raciness to clean up in
here that just isn't triggered generally.  I think you're right,
dropping the check and doing it in p2m_pod_demand_populate() is
probably the Right Thing to do.  I've got that stack of patches to
deal with in January, I'll work on it then.

 -George Dunlap