Xen devel - Dec 2010 - regression from c/s 22071:c5aed2e049bc (ept: Put locks around ept_get_entry) ?

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

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
>

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

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

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

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

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
>

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

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
>>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


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

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


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

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


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

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


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

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



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

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


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

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


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

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
> 


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

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


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

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

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

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 }


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

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


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

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

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