[PATCH] mm: fix LLVM code-generation issue

[PATCH] mm: fix LLVM code-generation issue

[Roger Pau Monne]

LLVM code generation can attempt to perform a load from a variable in
the next condition of an expression under certain circumstances, thus
turning the following condition:

if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )

Into:

0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
0xffff82d08022396e <+110>: setb   -0x29(%rbp)
0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>

Such code will trigger a page fault if system_state >=
SYS_STATE_active.

In order to prevent such optimization signal to the compiler that
accessing opt_bootscrub can have side effects by using ACCESS_ONCE.
This has been reported and discussed with upstream LLVM:

https://bugs.llvm.org/show_bug.cgi?id=39707

I haven't been able to find any other instances of such conditional
expression that uses system_state together with an init variable or
function.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/common/page_alloc.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 08ee8cfbb9..60adf6f64b 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1772,7 +1772,17 @@ static void init_heap_pages(
     first_valid_mfn = mfn_min(page_to_mfn(pg), first_valid_mfn);
     spin_unlock(&heap_lock);
 
-    if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
+    if ( system_state < SYS_STATE_active &&
+         /*
+          * Use ACCESS_ONCE in order to let the compiler know accessing
+          * opt_bootscrub in this context can have side-effects (since it
+          * might be unmapped depending on the value of system_state).
+          * This prevents the compiler from attempting a load of
+          * opt_bootscrub before checking the value of system_state. See:
+          *
+          * https://bugs.llvm.org/show_bug.cgi?id=39707
+          */
+         ACCESS_ONCE(opt_bootscrub) == BOOTSCRUB_IDLE )
         idle_scrub = true;
 
     for ( i = 0; i < nr_pages; i++ )
-- 
2.19.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Wei Liu]

On Thu, Nov 22, 2018 at 01:03:27PM +0100, Roger Pau Monne wrote:
> LLVM code generation can attempt to perform a load from a variable in
> the next condition of an expression under certain circumstances, thus
> turning the following condition:
> 
> if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
> 
> Into:
> 
> 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
> 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
> 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>
> 
> Such code will trigger a page fault if system_state >=
> SYS_STATE_active.
> 
> In order to prevent such optimization signal to the compiler that
> accessing opt_bootscrub can have side effects by using ACCESS_ONCE.
> This has been reported and discussed with upstream LLVM:
> 
> https://bugs.llvm.org/show_bug.cgi?id=39707
> 
> I haven't been able to find any other instances of such conditional
> expression that uses system_state together with an init variable or
> function.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

I think long term we should invent / adopt some systematical approach,
but this will do for now.

I have written a QEMU based smoke test which will hopefully help us
catch further issues quickly.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Andrew Cooper]

On 22/11/2018 12:03, Roger Pau Monne wrote:
> LLVM code generation can attempt to perform a load from a variable in
> the next condition of an expression under certain circumstances, thus
> turning the following condition:
>
> if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
>
> Into:
>
> 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
> 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
> 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>

You're actually missing two pieces here.  If I recall the disassembly
correctly, there was a `sete %al`, and an `and %al, -0x29(%rbp)` which
had the cumulative effect of calculating `idle_scrub` branchlessly
(which is no doubt the intended optimisation).

>
> Such code will trigger a page fault if system_state >=
> SYS_STATE_active.
>
> In order to prevent such optimization signal to the compiler that
> accessing opt_bootscrub can have side effects by using ACCESS_ONCE.
> This has been reported and discussed with upstream LLVM:
>
> https://bugs.llvm.org/show_bug.cgi?id=39707
>
> I haven't been able to find any other instances of such conditional
> expression that uses system_state together with an init variable or
> function.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

To unblock the Clang build, Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com>, although ideally with the expanded disassembly.

I still think that longterm, we need to reconsider our position of
referencing __init things from non-__init functions.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Julien Grall]

Hi Andrew,

On 11/22/18 12:23 PM, Andrew Cooper wrote:
> On 22/11/2018 12:03, Roger Pau Monne wrote:
>> LLVM code generation can attempt to perform a load from a variable in
>> the next condition of an expression under certain circumstances, thus
>> turning the following condition:
>>
>> if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
>>
>> Into:
>>
>> 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
>> 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
>> 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>
> 
> You're actually missing two pieces here.  If I recall the disassembly
> correctly, there was a `sete %al`, and an `and %al, -0x29(%rbp)` which
> had the cumulative effect of calculating `idle_scrub` branchlessly
> (which is no doubt the intended optimisation).
> 
>>
>> Such code will trigger a page fault if system_state >=
>> SYS_STATE_active.
>>
>> In order to prevent such optimization signal to the compiler that
>> accessing opt_bootscrub can have side effects by using ACCESS_ONCE.
>> This has been reported and discussed with upstream LLVM:
>>
>> https://bugs.llvm.org/show_bug.cgi?id=39707
>>
>> I haven't been able to find any other instances of such conditional
>> expression that uses system_state together with an init variable or
>> function.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> To unblock the Clang build, Acked-by: Andrew Cooper
> <andrew.cooper3@citrix.com>, although ideally with the expanded disassembly.

Actually is it enough in all the case? We are only preventing the 
re-ordering for the compiler. The processor is still free to re-order it 
and load opt_bootscrub before loading system_state.

Furthermore, this is relying on system_state to always be visible before 
the init section is freed. I am not entirely whether we have the 
explicit barrier for that.

> 
> I still think that longterm, we need to reconsider our position of
> referencing __init things from non-__init functions.

+1.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Jan Beulich]

>>> On 22.11.18 at 13:38, <julien.grall@arm.com> wrote:
> Hi Andrew,
> 
> On 11/22/18 12:23 PM, Andrew Cooper wrote:
>> On 22/11/2018 12:03, Roger Pau Monne wrote:
>>> LLVM code generation can attempt to perform a load from a variable in
>>> the next condition of an expression under certain circumstances, thus
>>> turning the following condition:
>>>
>>> if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
>>>
>>> Into:
>>>
>>> 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 
> <system_state>
>>> 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
>>> 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 
> <opt_bootscrub>
>> 
>> You're actually missing two pieces here.  If I recall the disassembly
>> correctly, there was a `sete %al`, and an `and %al, -0x29(%rbp)` which
>> had the cumulative effect of calculating `idle_scrub` branchlessly
>> (which is no doubt the intended optimisation).
>> 
>>>
>>> Such code will trigger a page fault if system_state >=
>>> SYS_STATE_active.
>>>
>>> In order to prevent such optimization signal to the compiler that
>>> accessing opt_bootscrub can have side effects by using ACCESS_ONCE.
>>> This has been reported and discussed with upstream LLVM:
>>>
>>> https://bugs.llvm.org/show_bug.cgi?id=39707 
>>>
>>> I haven't been able to find any other instances of such conditional
>>> expression that uses system_state together with an init variable or
>>> function.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> 
>> To unblock the Clang build, Acked-by: Andrew Cooper
>> <andrew.cooper3@citrix.com>, although ideally with the expanded disassembly.
> 
> Actually is it enough in all the case? We are only preventing the 
> re-ordering for the compiler. The processor is still free to re-order it 
> and load opt_bootscrub before loading system_state.

The processor is fine to issue the load early, but it is not permitted
to raise an exception resulting from that read attempt before the
reading insn is the next one to retire.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Andrew Cooper]

On 22/11/2018 13:27, Jan Beulich wrote:
>>>> On 22.11.18 at 13:38, <julien.grall@arm.com> wrote:
>> Hi Andrew,
>>
>> On 11/22/18 12:23 PM, Andrew Cooper wrote:
>>> On 22/11/2018 12:03, Roger Pau Monne wrote:
>>>> LLVM code generation can attempt to perform a load from a variable in
>>>> the next condition of an expression under certain circumstances, thus
>>>> turning the following condition:
>>>>
>>>> if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
>>>>
>>>> Into:
>>>>
>>>> 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 
>> <system_state>
>>>> 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
>>>> 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 
>> <opt_bootscrub>
>>> You're actually missing two pieces here.  If I recall the disassembly
>>> correctly, there was a `sete %al`, and an `and %al, -0x29(%rbp)` which
>>> had the cumulative effect of calculating `idle_scrub` branchlessly
>>> (which is no doubt the intended optimisation).
>>>
>>>> Such code will trigger a page fault if system_state >=
>>>> SYS_STATE_active.
>>>>
>>>> In order to prevent such optimization signal to the compiler that
>>>> accessing opt_bootscrub can have side effects by using ACCESS_ONCE.
>>>> This has been reported and discussed with upstream LLVM:
>>>>
>>>> https://bugs.llvm.org/show_bug.cgi?id=39707 
>>>>
>>>> I haven't been able to find any other instances of such conditional
>>>> expression that uses system_state together with an init variable or
>>>> function.
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> To unblock the Clang build, Acked-by: Andrew Cooper
>>> <andrew.cooper3@citrix.com>, although ideally with the expanded disassembly.
>> Actually is it enough in all the case? We are only preventing the 
>> re-ordering for the compiler. The processor is still free to re-order it 
>> and load opt_bootscrub before loading system_state.
> The processor is fine to issue the load early, but it is not permitted
> to raise an exception resulting from that read attempt before the
> reading insn is the next one to retire.

I think Julien's point is that without explicitly barriers, CPU0's
update to system_state may not be visible on CPU1, even though the
mappings have been shot down.

Therefore, from the processors point of view, it did everything
correctly, and hit a real pagefault.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Jan Beulich]

>>> On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
> I think Julien's point is that without explicitly barriers, CPU0's
> update to system_state may not be visible on CPU1, even though the
> mappings have been shot down.
> 
> Therefore, from the processors point of view, it did everything
> correctly, and hit a real pagefault.

Boot time updates of system_state should be of no interest here,
as at that time the APs are all idling. The only other update is
during S3 suspend/resume, in that case the mappings have been
removed long ago.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Julien Grall]

Hi Jan,

On 11/22/18 1:36 PM, Jan Beulich wrote:
>>>> On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
>> I think Julien's point is that without explicitly barriers, CPU0's
>> update to system_state may not be visible on CPU1, even though the
>> mappings have been shot down.
>>
>> Therefore, from the processors point of view, it did everything
>> correctly, and hit a real pagefault.
> 
> Boot time updates of system_state should be of no interest here,
> as at that time the APs are all idling.

That's probably true today. But this code looks rather fragile as you 
don't know how this is going to be used in the future.

If you decide to gate init code with system_state, then you need a 
barrier to ensure the code is future proof.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Roger Pau Monné]

On Thu, Nov 22, 2018 at 12:23:22PM +0000, Andrew Cooper wrote:
> On 22/11/2018 12:03, Roger Pau Monne wrote:
> > LLVM code generation can attempt to perform a load from a variable in
> > the next condition of an expression under certain circumstances, thus
> > turning the following condition:
> >
> > if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
> >
> > Into:
> >
> > 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
> > 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
> > 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>
> 
> You're actually missing two pieces here.  If I recall the disassembly
> correctly, there was a `sete %al`, and an `and %al, -0x29(%rbp)` which
> had the cumulative effect of calculating `idle_scrub` branchlessly
> (which is no doubt the intended optimisation).

Right, my point here was to merely show that opt_bootscrub is accessed
regardless of the value of system_state. I can expand the asm chunk,
but the bug reference below already has the disassembly of the whole
function.

> >
> > Such code will trigger a page fault if system_state >=
> > SYS_STATE_active.
> >
> > In order to prevent such optimization signal to the compiler that
> > accessing opt_bootscrub can have side effects by using ACCESS_ONCE.
> > This has been reported and discussed with upstream LLVM:
> >
> > https://bugs.llvm.org/show_bug.cgi?id=39707

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Roger Pau Monné]

On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
> Hi Jan,
> 
> On 11/22/18 1:36 PM, Jan Beulich wrote:
> > > > > On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
> > > I think Julien's point is that without explicitly barriers, CPU0's
> > > update to system_state may not be visible on CPU1, even though the
> > > mappings have been shot down.
> > > 
> > > Therefore, from the processors point of view, it did everything
> > > correctly, and hit a real pagefault.
> > 
> > Boot time updates of system_state should be of no interest here,
> > as at that time the APs are all idling.
> 
> That's probably true today. But this code looks rather fragile as you don't
> know how this is going to be used in the future.
> 
> If you decide to gate init code with system_state, then you need a barrier
> to ensure the code is future proof.

Wouldn't it be enough to declare system_state as volatile?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Andrew Cooper]

On 22/11/2018 15:20, Roger Pau Monné wrote:
> On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
>> Hi Jan,
>>
>> On 11/22/18 1:36 PM, Jan Beulich wrote:
>>>>>> On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
>>>> I think Julien's point is that without explicitly barriers, CPU0's
>>>> update to system_state may not be visible on CPU1, even though the
>>>> mappings have been shot down.
>>>>
>>>> Therefore, from the processors point of view, it did everything
>>>> correctly, and hit a real pagefault.
>>> Boot time updates of system_state should be of no interest here,
>>> as at that time the APs are all idling.
>> That's probably true today. But this code looks rather fragile as you don't
>> know how this is going to be used in the future.
>>
>> If you decide to gate init code with system_state, then you need a barrier
>> to ensure the code is future proof.
> Wouldn't it be enough to declare system_state as volatile?

No.  volatility (or lack thereof) is a compiler level construct.

ARM has weaker cache coherency than x86, so a write which has completed
on one CPU0 in the past may legitimately not be visible on CPU1 yet.

If you need guarantees about the visibility of updated, you must use
appropriate barriers.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Roger Pau Monné]

On Thu, Nov 22, 2018 at 03:23:41PM +0000, Andrew Cooper wrote:
> On 22/11/2018 15:20, Roger Pau Monné wrote:
> > On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
> >> Hi Jan,
> >>
> >> On 11/22/18 1:36 PM, Jan Beulich wrote:
> >>>>>> On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
> >>>> I think Julien's point is that without explicitly barriers, CPU0's
> >>>> update to system_state may not be visible on CPU1, even though the
> >>>> mappings have been shot down.
> >>>>
> >>>> Therefore, from the processors point of view, it did everything
> >>>> correctly, and hit a real pagefault.
> >>> Boot time updates of system_state should be of no interest here,
> >>> as at that time the APs are all idling.
> >> That's probably true today. But this code looks rather fragile as you don't
> >> know how this is going to be used in the future.
> >>
> >> If you decide to gate init code with system_state, then you need a barrier
> >> to ensure the code is future proof.
> > Wouldn't it be enough to declare system_state as volatile?
> 
> No.  volatility (or lack thereof) is a compiler level construct.
> 
> ARM has weaker cache coherency than x86, so a write which has completed
> on one CPU0 in the past may legitimately not be visible on CPU1 yet.
> 
> If you need guarantees about the visibility of updated, you must use
> appropriate barriers.

Right. There's some differences between ARM and x86, ARM sets
SYS_STATE_active and continues to make use of init functions. In any
case I have the following diff:

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index e83221ab79..cf50d05620 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -966,6 +966,7 @@ void __init start_xen(unsigned long boot_phys_offset,
     serial_endboot();
 
     system_state = SYS_STATE_active;
+    smp_wmb();
 
     create_domUs();
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 9cbff22fb3..41044c0b6f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -593,6 +593,7 @@ static void noinline init_done(void)
     unsigned long start, end;
 
     system_state = SYS_STATE_active;
+    smp_wmb();
 
     domain_unpause_by_systemcontroller(dom0);
 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Andrew Cooper]

On 22/11/2018 16:07, Roger Pau Monné wrote:
> On Thu, Nov 22, 2018 at 03:23:41PM +0000, Andrew Cooper wrote:
>> On 22/11/2018 15:20, Roger Pau Monné wrote:
>>> On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
>>>> Hi Jan,
>>>>
>>>> On 11/22/18 1:36 PM, Jan Beulich wrote:
>>>>>>>> On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
>>>>>> I think Julien's point is that without explicitly barriers, CPU0's
>>>>>> update to system_state may not be visible on CPU1, even though the
>>>>>> mappings have been shot down.
>>>>>>
>>>>>> Therefore, from the processors point of view, it did everything
>>>>>> correctly, and hit a real pagefault.
>>>>> Boot time updates of system_state should be of no interest here,
>>>>> as at that time the APs are all idling.
>>>> That's probably true today. But this code looks rather fragile as you don't
>>>> know how this is going to be used in the future.
>>>>
>>>> If you decide to gate init code with system_state, then you need a barrier
>>>> to ensure the code is future proof.
>>> Wouldn't it be enough to declare system_state as volatile?
>> No.  volatility (or lack thereof) is a compiler level construct.
>>
>> ARM has weaker cache coherency than x86, so a write which has completed
>> on one CPU0 in the past may legitimately not be visible on CPU1 yet.
>>
>> If you need guarantees about the visibility of updated, you must use
>> appropriate barriers.
> Right. There's some differences between ARM and x86, ARM sets
> SYS_STATE_active and continues to make use of init functions. In any
> case I have the following diff:
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index e83221ab79..cf50d05620 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -966,6 +966,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>      serial_endboot();
>  
>      system_state = SYS_STATE_active;
> +    smp_wmb();
>  
>      create_domUs();
>  
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 9cbff22fb3..41044c0b6f 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -593,6 +593,7 @@ static void noinline init_done(void)
>      unsigned long start, end;
>  
>      system_state = SYS_STATE_active;
> +    smp_wmb();
>  
>      domain_unpause_by_systemcontroller(dom0);
>  
>

I'm afraid that that won't do anything to help at all.

smp_{wmb,rmb}() must be in matched pairs, and mb() must be matched with
itself.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Roger Pau Monné]

On Thu, Nov 22, 2018 at 04:22:34PM +0000, Andrew Cooper wrote:
> On 22/11/2018 16:07, Roger Pau Monné wrote:
> > On Thu, Nov 22, 2018 at 03:23:41PM +0000, Andrew Cooper wrote:
> >> On 22/11/2018 15:20, Roger Pau Monné wrote:
> >>> On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
> >>>> Hi Jan,
> >>>>
> >>>> On 11/22/18 1:36 PM, Jan Beulich wrote:
> >>>>>>>> On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
> >>>>>> I think Julien's point is that without explicitly barriers, CPU0's
> >>>>>> update to system_state may not be visible on CPU1, even though the
> >>>>>> mappings have been shot down.
> >>>>>>
> >>>>>> Therefore, from the processors point of view, it did everything
> >>>>>> correctly, and hit a real pagefault.
> >>>>> Boot time updates of system_state should be of no interest here,
> >>>>> as at that time the APs are all idling.
> >>>> That's probably true today. But this code looks rather fragile as you don't
> >>>> know how this is going to be used in the future.
> >>>>
> >>>> If you decide to gate init code with system_state, then you need a barrier
> >>>> to ensure the code is future proof.
> >>> Wouldn't it be enough to declare system_state as volatile?
> >> No.  volatility (or lack thereof) is a compiler level construct.
> >>
> >> ARM has weaker cache coherency than x86, so a write which has completed
> >> on one CPU0 in the past may legitimately not be visible on CPU1 yet.
> >>
> >> If you need guarantees about the visibility of updated, you must use
> >> appropriate barriers.
> > Right. There's some differences between ARM and x86, ARM sets
> > SYS_STATE_active and continues to make use of init functions. In any
> > case I have the following diff:
> >
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index e83221ab79..cf50d05620 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -966,6 +966,7 @@ void __init start_xen(unsigned long boot_phys_offset,
> >      serial_endboot();
> >  
> >      system_state = SYS_STATE_active;
> > +    smp_wmb();
> >  
> >      create_domUs();
> >  
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 9cbff22fb3..41044c0b6f 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -593,6 +593,7 @@ static void noinline init_done(void)
> >      unsigned long start, end;
> >  
> >      system_state = SYS_STATE_active;
> > +    smp_wmb();
> >  
> >      domain_unpause_by_systemcontroller(dom0);
> >  
> >
> 
> I'm afraid that that won't do anything to help at all.
> 
> smp_{wmb,rmb}() must be in matched pairs, and mb() must be matched with
> itself.

Then I'm not sure about whether our previous plan still stands, are we
OK with using ACCESS_ONCE here and forgetting about the memory
barriers given the current usage?

If we have to add memory barriers I think I prefer to just make
opt_bootscrub non-init.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Julien Grall]

Hi Roger,

On 11/22/18 4:39 PM, Roger Pau Monné wrote:
> On Thu, Nov 22, 2018 at 04:22:34PM +0000, Andrew Cooper wrote:
>> On 22/11/2018 16:07, Roger Pau Monné wrote:
>>> On Thu, Nov 22, 2018 at 03:23:41PM +0000, Andrew Cooper wrote:
>>>> On 22/11/2018 15:20, Roger Pau Monné wrote:
>>>>> On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
>>>>>> Hi Jan,
>>>>>>
>>>>>> On 11/22/18 1:36 PM, Jan Beulich wrote:
>>>>>>>>>> On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>> I think Julien's point is that without explicitly barriers, CPU0's
>>>>>>>> update to system_state may not be visible on CPU1, even though the
>>>>>>>> mappings have been shot down.
>>>>>>>>
>>>>>>>> Therefore, from the processors point of view, it did everything
>>>>>>>> correctly, and hit a real pagefault.
>>>>>>> Boot time updates of system_state should be of no interest here,
>>>>>>> as at that time the APs are all idling.
>>>>>> That's probably true today. But this code looks rather fragile as you don't
>>>>>> know how this is going to be used in the future.
>>>>>>
>>>>>> If you decide to gate init code with system_state, then you need a barrier
>>>>>> to ensure the code is future proof.
>>>>> Wouldn't it be enough to declare system_state as volatile?
>>>> No.  volatility (or lack thereof) is a compiler level construct.
>>>>
>>>> ARM has weaker cache coherency than x86, so a write which has completed
>>>> on one CPU0 in the past may legitimately not be visible on CPU1 yet.
>>>>
>>>> If you need guarantees about the visibility of updated, you must use
>>>> appropriate barriers.
>>> Right. There's some differences between ARM and x86, ARM sets
>>> SYS_STATE_active and continues to make use of init functions. In any
>>> case I have the following diff:
>>>
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index e83221ab79..cf50d05620 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -966,6 +966,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>>>       serial_endboot();
>>>   
>>>       system_state = SYS_STATE_active;
>>> +    smp_wmb();
>>>   
>>>       create_domUs();
>>>   
>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>> index 9cbff22fb3..41044c0b6f 100644
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -593,6 +593,7 @@ static void noinline init_done(void)
>>>       unsigned long start, end;
>>>   
>>>       system_state = SYS_STATE_active;
>>> +    smp_wmb();
>>>   
>>>       domain_unpause_by_systemcontroller(dom0);
>>>   
>>>
>>
>> I'm afraid that that won't do anything to help at all.
>>
>> smp_{wmb,rmb}() must be in matched pairs, and mb() must be matched with
>> itself.
> 
> Then I'm not sure about whether our previous plan still stands, are we
> OK with using ACCESS_ONCE here and forgetting about the memory
> barriers given the current usage?

The problem is not the current usage but how it could be used. Debugging 
memory ordering is quite a pain so I would prefer this to be fixed 
correctly.

> 
> If we have to add memory barriers I think I prefer to just make
> opt_bootscrub non-init.

The memory barriers would not be too bad as smp_wmb() on Arm only ensure 
that the write to system_state will be observed before any write after.

But opt_bootscrub in non-init is also a solution.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[George Dunlap]

On 11/22/18 4:45 PM, Julien Grall wrote:
> Hi Roger,
> 
> On 11/22/18 4:39 PM, Roger Pau Monné wrote:
>> On Thu, Nov 22, 2018 at 04:22:34PM +0000, Andrew Cooper wrote:
>>> On 22/11/2018 16:07, Roger Pau Monné wrote:
>>>> On Thu, Nov 22, 2018 at 03:23:41PM +0000, Andrew Cooper wrote:
>>>>> On 22/11/2018 15:20, Roger Pau Monné wrote:
>>>>>> On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
>>>>>>> Hi Jan,
>>>>>>>
>>>>>>> On 11/22/18 1:36 PM, Jan Beulich wrote:
>>>>>>>>>>> On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>>> I think Julien's point is that without explicitly barriers, CPU0's
>>>>>>>>> update to system_state may not be visible on CPU1, even though the
>>>>>>>>> mappings have been shot down.
>>>>>>>>>
>>>>>>>>> Therefore, from the processors point of view, it did everything
>>>>>>>>> correctly, and hit a real pagefault.
>>>>>>>> Boot time updates of system_state should be of no interest here,
>>>>>>>> as at that time the APs are all idling.
>>>>>>> That's probably true today. But this code looks rather fragile as
>>>>>>> you don't
>>>>>>> know how this is going to be used in the future.
>>>>>>>
>>>>>>> If you decide to gate init code with system_state, then you need
>>>>>>> a barrier
>>>>>>> to ensure the code is future proof.
>>>>>> Wouldn't it be enough to declare system_state as volatile?
>>>>> No.  volatility (or lack thereof) is a compiler level construct.
>>>>>
>>>>> ARM has weaker cache coherency than x86, so a write which has
>>>>> completed
>>>>> on one CPU0 in the past may legitimately not be visible on CPU1 yet.
>>>>>
>>>>> If you need guarantees about the visibility of updated, you must use
>>>>> appropriate barriers.
>>>> Right. There's some differences between ARM and x86, ARM sets
>>>> SYS_STATE_active and continues to make use of init functions. In any
>>>> case I have the following diff:
>>>>
>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>> index e83221ab79..cf50d05620 100644
>>>> --- a/xen/arch/arm/setup.c
>>>> +++ b/xen/arch/arm/setup.c
>>>> @@ -966,6 +966,7 @@ void __init start_xen(unsigned long
>>>> boot_phys_offset,
>>>>       serial_endboot();
>>>>         system_state = SYS_STATE_active;
>>>> +    smp_wmb();
>>>>         create_domUs();
>>>>   diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>>> index 9cbff22fb3..41044c0b6f 100644
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -593,6 +593,7 @@ static void noinline init_done(void)
>>>>       unsigned long start, end;
>>>>         system_state = SYS_STATE_active;
>>>> +    smp_wmb();
>>>>         domain_unpause_by_systemcontroller(dom0);
>>>>  
>>>
>>> I'm afraid that that won't do anything to help at all.
>>>
>>> smp_{wmb,rmb}() must be in matched pairs, and mb() must be matched with
>>> itself.
>>
>> Then I'm not sure about whether our previous plan still stands, are we
>> OK with using ACCESS_ONCE here and forgetting about the memory
>> barriers given the current usage?
> 
> The problem is not the current usage but how it could be used. Debugging
> memory ordering is quite a pain so I would prefer this to be fixed
> correctly.

But in this case it wouldn't be a pain, because the only possible
failure mode is if the processor faults trying to read opt_bootscrub, right?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Julien Grall]

On 11/22/18 5:04 PM, George Dunlap wrote:
> On 11/22/18 4:45 PM, Julien Grall wrote:
>> Hi Roger,
>>
>> On 11/22/18 4:39 PM, Roger Pau Monné wrote:
>>> On Thu, Nov 22, 2018 at 04:22:34PM +0000, Andrew Cooper wrote:
>>>> On 22/11/2018 16:07, Roger Pau Monné wrote:
>>>>> On Thu, Nov 22, 2018 at 03:23:41PM +0000, Andrew Cooper wrote:
>>>>>> On 22/11/2018 15:20, Roger Pau Monné wrote:
>>>>>>> On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
>>>>>>>> Hi Jan,
>>>>>>>>
>>>>>>>> On 11/22/18 1:36 PM, Jan Beulich wrote:
>>>>>>>>>>>> On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>>>> I think Julien's point is that without explicitly barriers, CPU0's
>>>>>>>>>> update to system_state may not be visible on CPU1, even though the
>>>>>>>>>> mappings have been shot down.
>>>>>>>>>>
>>>>>>>>>> Therefore, from the processors point of view, it did everything
>>>>>>>>>> correctly, and hit a real pagefault.
>>>>>>>>> Boot time updates of system_state should be of no interest here,
>>>>>>>>> as at that time the APs are all idling.
>>>>>>>> That's probably true today. But this code looks rather fragile as
>>>>>>>> you don't
>>>>>>>> know how this is going to be used in the future.
>>>>>>>>
>>>>>>>> If you decide to gate init code with system_state, then you need
>>>>>>>> a barrier
>>>>>>>> to ensure the code is future proof.
>>>>>>> Wouldn't it be enough to declare system_state as volatile?
>>>>>> No.  volatility (or lack thereof) is a compiler level construct.
>>>>>>
>>>>>> ARM has weaker cache coherency than x86, so a write which has
>>>>>> completed
>>>>>> on one CPU0 in the past may legitimately not be visible on CPU1 yet.
>>>>>>
>>>>>> If you need guarantees about the visibility of updated, you must use
>>>>>> appropriate barriers.
>>>>> Right. There's some differences between ARM and x86, ARM sets
>>>>> SYS_STATE_active and continues to make use of init functions. In any
>>>>> case I have the following diff:
>>>>>
>>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>>> index e83221ab79..cf50d05620 100644
>>>>> --- a/xen/arch/arm/setup.c
>>>>> +++ b/xen/arch/arm/setup.c
>>>>> @@ -966,6 +966,7 @@ void __init start_xen(unsigned long
>>>>> boot_phys_offset,
>>>>>        serial_endboot();
>>>>>          system_state = SYS_STATE_active;
>>>>> +    smp_wmb();
>>>>>          create_domUs();
>>>>>    diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>>>> index 9cbff22fb3..41044c0b6f 100644
>>>>> --- a/xen/arch/x86/setup.c
>>>>> +++ b/xen/arch/x86/setup.c
>>>>> @@ -593,6 +593,7 @@ static void noinline init_done(void)
>>>>>        unsigned long start, end;
>>>>>          system_state = SYS_STATE_active;
>>>>> +    smp_wmb();
>>>>>          domain_unpause_by_systemcontroller(dom0);
>>>>>   
>>>>
>>>> I'm afraid that that won't do anything to help at all.
>>>>
>>>> smp_{wmb,rmb}() must be in matched pairs, and mb() must be matched with
>>>> itself.
>>>
>>> Then I'm not sure about whether our previous plan still stands, are we
>>> OK with using ACCESS_ONCE here and forgetting about the memory
>>> barriers given the current usage?
>>
>> The problem is not the current usage but how it could be used. Debugging
>> memory ordering is quite a pain so I would prefer this to be fixed
>> correctly.
> 
> But in this case it wouldn't be a pain, because the only possible
> failure mode is if the processor faults trying to read opt_bootscrub, right?

Possibly. But I don't see any reason to defer the fix until someone 
comes up with unreliable crash.

Cheers,


-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Roger Pau Monné]

On Thu, Nov 22, 2018 at 05:46:19PM +0000, Julien Grall wrote:
> 
> 
> On 11/22/18 5:04 PM, George Dunlap wrote:
> > On 11/22/18 4:45 PM, Julien Grall wrote:
> > > Hi Roger,
> > > 
> > > On 11/22/18 4:39 PM, Roger Pau Monné wrote:
> > > > On Thu, Nov 22, 2018 at 04:22:34PM +0000, Andrew Cooper wrote:
> > > > > On 22/11/2018 16:07, Roger Pau Monné wrote:
> > > > > > On Thu, Nov 22, 2018 at 03:23:41PM +0000, Andrew Cooper wrote:
> > > > > > > On 22/11/2018 15:20, Roger Pau Monné wrote:
> > > > > > > > On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
> > > > > > > > > Hi Jan,
> > > > > > > > > 
> > > > > > > > > On 11/22/18 1:36 PM, Jan Beulich wrote:
> > > > > > > > > > > > > On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
> > > > > > > > > > > I think Julien's point is that without explicitly barriers, CPU0's
> > > > > > > > > > > update to system_state may not be visible on CPU1, even though the
> > > > > > > > > > > mappings have been shot down.
> > > > > > > > > > > 
> > > > > > > > > > > Therefore, from the processors point of view, it did everything
> > > > > > > > > > > correctly, and hit a real pagefault.
> > > > > > > > > > Boot time updates of system_state should be of no interest here,
> > > > > > > > > > as at that time the APs are all idling.
> > > > > > > > > That's probably true today. But this code looks rather fragile as
> > > > > > > > > you don't
> > > > > > > > > know how this is going to be used in the future.
> > > > > > > > > 
> > > > > > > > > If you decide to gate init code with system_state, then you need
> > > > > > > > > a barrier
> > > > > > > > > to ensure the code is future proof.
> > > > > > > > Wouldn't it be enough to declare system_state as volatile?
> > > > > > > No.  volatility (or lack thereof) is a compiler level construct.
> > > > > > > 
> > > > > > > ARM has weaker cache coherency than x86, so a write which has
> > > > > > > completed
> > > > > > > on one CPU0 in the past may legitimately not be visible on CPU1 yet.
> > > > > > > 
> > > > > > > If you need guarantees about the visibility of updated, you must use
> > > > > > > appropriate barriers.
> > > > > > Right. There's some differences between ARM and x86, ARM sets
> > > > > > SYS_STATE_active and continues to make use of init functions. In any
> > > > > > case I have the following diff:
> > > > > > 
> > > > > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > > > > > index e83221ab79..cf50d05620 100644
> > > > > > --- a/xen/arch/arm/setup.c
> > > > > > +++ b/xen/arch/arm/setup.c
> > > > > > @@ -966,6 +966,7 @@ void __init start_xen(unsigned long
> > > > > > boot_phys_offset,
> > > > > >        serial_endboot();
> > > > > >          system_state = SYS_STATE_active;
> > > > > > +    smp_wmb();
> > > > > >          create_domUs();
> > > > > >    diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > > > > > index 9cbff22fb3..41044c0b6f 100644
> > > > > > --- a/xen/arch/x86/setup.c
> > > > > > +++ b/xen/arch/x86/setup.c
> > > > > > @@ -593,6 +593,7 @@ static void noinline init_done(void)
> > > > > >        unsigned long start, end;
> > > > > >          system_state = SYS_STATE_active;
> > > > > > +    smp_wmb();
> > > > > >          domain_unpause_by_systemcontroller(dom0);
> > > > > 
> > > > > I'm afraid that that won't do anything to help at all.
> > > > > 
> > > > > smp_{wmb,rmb}() must be in matched pairs, and mb() must be matched with
> > > > > itself.
> > > > 
> > > > Then I'm not sure about whether our previous plan still stands, are we
> > > > OK with using ACCESS_ONCE here and forgetting about the memory
> > > > barriers given the current usage?
> > > 
> > > The problem is not the current usage but how it could be used. Debugging
> > > memory ordering is quite a pain so I would prefer this to be fixed
> > > correctly.
> > 
> > But in this case it wouldn't be a pain, because the only possible
> > failure mode is if the processor faults trying to read opt_bootscrub, right?
> 
> Possibly. But I don't see any reason to defer the fix until someone comes up
> with unreliable crash.

If we have to go down that route, shouldn't we also protect
system_state with a lock so that it cannot be modified by a CPU while
it's being read from another?

Image an AP reads system_state < SYS_STATE_active, then BSP sets
system_state = SYS_STATE_active and clears the init mappings, then
when the AP tries to read an init variable it would get a page fault.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Jan Beulich]

>>> On 23.11.18 at 12:23, <roger.pau@citrix.com> wrote:
> Image an AP reads system_state < SYS_STATE_active, then BSP sets
> system_state = SYS_STATE_active and clears the init mappings, then
> when the AP tries to read an init variable it would get a page fault.

As said before - APs are supposed to be quiescent during this
particular transition of system_state, and the idle scrubbing added
an exception to that rule. I don't see why such an exception may
not involve exceptional treatment on the particular reading path.
Furthermore APs, which may also be brought up at run time, may
never ever touch init mappings of any kind.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Julien Grall]

On 23/11/2018 11:23, Roger Pau Monné wrote:
> On Thu, Nov 22, 2018 at 05:46:19PM +0000, Julien Grall wrote:
>>
>>
>> On 11/22/18 5:04 PM, George Dunlap wrote:
>>> On 11/22/18 4:45 PM, Julien Grall wrote:
>>>> Hi Roger,
>>>>
>>>> On 11/22/18 4:39 PM, Roger Pau Monné wrote:
>>>>> On Thu, Nov 22, 2018 at 04:22:34PM +0000, Andrew Cooper wrote:
>>>>>> On 22/11/2018 16:07, Roger Pau Monné wrote:
>>>>>>> On Thu, Nov 22, 2018 at 03:23:41PM +0000, Andrew Cooper wrote:
>>>>>>>> On 22/11/2018 15:20, Roger Pau Monné wrote:
>>>>>>>>> On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
>>>>>>>>>> Hi Jan,
>>>>>>>>>>
>>>>>>>>>> On 11/22/18 1:36 PM, Jan Beulich wrote:
>>>>>>>>>>>>>> On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>>>>>> I think Julien's point is that without explicitly barriers, CPU0's
>>>>>>>>>>>> update to system_state may not be visible on CPU1, even though the
>>>>>>>>>>>> mappings have been shot down.
>>>>>>>>>>>>
>>>>>>>>>>>> Therefore, from the processors point of view, it did everything
>>>>>>>>>>>> correctly, and hit a real pagefault.
>>>>>>>>>>> Boot time updates of system_state should be of no interest here,
>>>>>>>>>>> as at that time the APs are all idling.
>>>>>>>>>> That's probably true today. But this code looks rather fragile as
>>>>>>>>>> you don't
>>>>>>>>>> know how this is going to be used in the future.
>>>>>>>>>>
>>>>>>>>>> If you decide to gate init code with system_state, then you need
>>>>>>>>>> a barrier
>>>>>>>>>> to ensure the code is future proof.
>>>>>>>>> Wouldn't it be enough to declare system_state as volatile?
>>>>>>>> No.  volatility (or lack thereof) is a compiler level construct.
>>>>>>>>
>>>>>>>> ARM has weaker cache coherency than x86, so a write which has
>>>>>>>> completed
>>>>>>>> on one CPU0 in the past may legitimately not be visible on CPU1 yet.
>>>>>>>>
>>>>>>>> If you need guarantees about the visibility of updated, you must use
>>>>>>>> appropriate barriers.
>>>>>>> Right. There's some differences between ARM and x86, ARM sets
>>>>>>> SYS_STATE_active and continues to make use of init functions. In any
>>>>>>> case I have the following diff:
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>>>>> index e83221ab79..cf50d05620 100644
>>>>>>> --- a/xen/arch/arm/setup.c
>>>>>>> +++ b/xen/arch/arm/setup.c
>>>>>>> @@ -966,6 +966,7 @@ void __init start_xen(unsigned long
>>>>>>> boot_phys_offset,
>>>>>>>         serial_endboot();
>>>>>>>           system_state = SYS_STATE_active;
>>>>>>> +    smp_wmb();
>>>>>>>           create_domUs();
>>>>>>>     diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>>>>>> index 9cbff22fb3..41044c0b6f 100644
>>>>>>> --- a/xen/arch/x86/setup.c
>>>>>>> +++ b/xen/arch/x86/setup.c
>>>>>>> @@ -593,6 +593,7 @@ static void noinline init_done(void)
>>>>>>>         unsigned long start, end;
>>>>>>>           system_state = SYS_STATE_active;
>>>>>>> +    smp_wmb();
>>>>>>>           domain_unpause_by_systemcontroller(dom0);
>>>>>>
>>>>>> I'm afraid that that won't do anything to help at all.
>>>>>>
>>>>>> smp_{wmb,rmb}() must be in matched pairs, and mb() must be matched with
>>>>>> itself.
>>>>>
>>>>> Then I'm not sure about whether our previous plan still stands, are we
>>>>> OK with using ACCESS_ONCE here and forgetting about the memory
>>>>> barriers given the current usage?
>>>>
>>>> The problem is not the current usage but how it could be used. Debugging
>>>> memory ordering is quite a pain so I would prefer this to be fixed
>>>> correctly.
>>>
>>> But in this case it wouldn't be a pain, because the only possible
>>> failure mode is if the processor faults trying to read opt_bootscrub, right?
>>
>> Possibly. But I don't see any reason to defer the fix until someone comes up
>> with unreliable crash.
> 
> If we have to go down that route, shouldn't we also protect
> system_state with a lock so that it cannot be modified by a CPU while
> it's being read from another?

The locking might be a bit too much. Modifying the system_state should not be an 
issue if you put the correct barrier in place.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Julien Grall]

Hi Jan,

On 23/11/2018 11:33, Jan Beulich wrote:
>>>> On 23.11.18 at 12:23, <roger.pau@citrix.com> wrote:
>> Image an AP reads system_state < SYS_STATE_active, then BSP sets
>> system_state = SYS_STATE_active and clears the init mappings, then
>> when the AP tries to read an init variable it would get a page fault.
> 
> As said before - APs are supposed to be quiescent during this
> particular transition of system_state, and the idle scrubbing added
> an exception to that rule. I don't see why such an exception may
> not involve exceptional treatment on the particular reading path.
> Furthermore APs, which may also be brought up at run time, may
> never ever touch init mappings of any kind.

The "supposed" is quite worrying because we have no way to verify that 
assumption until we get a random crash.

Also, the cost of the barrier should be quite cheap over the rest of the function.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Roger Pau Monné]

On Fri, Nov 23, 2018 at 11:39:32AM +0000, Julien Grall wrote:
> Hi Jan,
> 
> On 23/11/2018 11:33, Jan Beulich wrote:
> > > > > On 23.11.18 at 12:23, <roger.pau@citrix.com> wrote:
> > > Image an AP reads system_state < SYS_STATE_active, then BSP sets
> > > system_state = SYS_STATE_active and clears the init mappings, then
> > > when the AP tries to read an init variable it would get a page fault.
> > 
> > As said before - APs are supposed to be quiescent during this
> > particular transition of system_state, and the idle scrubbing added
> > an exception to that rule. I don't see why such an exception may
> > not involve exceptional treatment on the particular reading path.
> > Furthermore APs, which may also be brought up at run time, may
> > never ever touch init mappings of any kind.
> 
> The "supposed" is quite worrying because we have no way to verify that
> assumption until we get a random crash.
> 
> Also, the cost of the barrier should be quite cheap over the rest of the function.

Right, but unless I'm missing something that won't completely fix the
issue either?

The only way I see to be completely safe is to either use a mutex or
to make opt_bootscrub non-init.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Wei Liu]

On Fri, Nov 23, 2018 at 11:36:48AM +0000, Julien Grall wrote:
> 
> 
> On 23/11/2018 11:23, Roger Pau Monné wrote:
> > On Thu, Nov 22, 2018 at 05:46:19PM +0000, Julien Grall wrote:
> > > 
> > > 
> > > On 11/22/18 5:04 PM, George Dunlap wrote:
> > > > On 11/22/18 4:45 PM, Julien Grall wrote:
> > > > > Hi Roger,
> > > > > 
> > > > > On 11/22/18 4:39 PM, Roger Pau Monné wrote:
> > > > > > On Thu, Nov 22, 2018 at 04:22:34PM +0000, Andrew Cooper wrote:
> > > > > > > On 22/11/2018 16:07, Roger Pau Monné wrote:
> > > > > > > > On Thu, Nov 22, 2018 at 03:23:41PM +0000, Andrew Cooper wrote:
> > > > > > > > > On 22/11/2018 15:20, Roger Pau Monné wrote:
> > > > > > > > > > On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
> > > > > > > > > > > Hi Jan,
> > > > > > > > > > > 
> > > > > > > > > > > On 11/22/18 1:36 PM, Jan Beulich wrote:
> > > > > > > > > > > > > > > On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
> > > > > > > > > > > > > I think Julien's point is that without explicitly barriers, CPU0's
> > > > > > > > > > > > > update to system_state may not be visible on CPU1, even though the
> > > > > > > > > > > > > mappings have been shot down.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Therefore, from the processors point of view, it did everything
> > > > > > > > > > > > > correctly, and hit a real pagefault.
> > > > > > > > > > > > Boot time updates of system_state should be of no interest here,
> > > > > > > > > > > > as at that time the APs are all idling.
> > > > > > > > > > > That's probably true today. But this code looks rather fragile as
> > > > > > > > > > > you don't
> > > > > > > > > > > know how this is going to be used in the future.
> > > > > > > > > > > 
> > > > > > > > > > > If you decide to gate init code with system_state, then you need
> > > > > > > > > > > a barrier
> > > > > > > > > > > to ensure the code is future proof.
> > > > > > > > > > Wouldn't it be enough to declare system_state as volatile?
> > > > > > > > > No.  volatility (or lack thereof) is a compiler level construct.
> > > > > > > > > 
> > > > > > > > > ARM has weaker cache coherency than x86, so a write which has
> > > > > > > > > completed
> > > > > > > > > on one CPU0 in the past may legitimately not be visible on CPU1 yet.
> > > > > > > > > 
> > > > > > > > > If you need guarantees about the visibility of updated, you must use
> > > > > > > > > appropriate barriers.
> > > > > > > > Right. There's some differences between ARM and x86, ARM sets
> > > > > > > > SYS_STATE_active and continues to make use of init functions. In any
> > > > > > > > case I have the following diff:
> > > > > > > > 
> > > > > > > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > > > > > > > index e83221ab79..cf50d05620 100644
> > > > > > > > --- a/xen/arch/arm/setup.c
> > > > > > > > +++ b/xen/arch/arm/setup.c
> > > > > > > > @@ -966,6 +966,7 @@ void __init start_xen(unsigned long
> > > > > > > > boot_phys_offset,
> > > > > > > >         serial_endboot();
> > > > > > > >           system_state = SYS_STATE_active;
> > > > > > > > +    smp_wmb();
> > > > > > > >           create_domUs();
> > > > > > > >     diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > > > > > > > index 9cbff22fb3..41044c0b6f 100644
> > > > > > > > --- a/xen/arch/x86/setup.c
> > > > > > > > +++ b/xen/arch/x86/setup.c
> > > > > > > > @@ -593,6 +593,7 @@ static void noinline init_done(void)
> > > > > > > >         unsigned long start, end;
> > > > > > > >           system_state = SYS_STATE_active;
> > > > > > > > +    smp_wmb();
> > > > > > > >           domain_unpause_by_systemcontroller(dom0);
> > > > > > > 
> > > > > > > I'm afraid that that won't do anything to help at all.
> > > > > > > 
> > > > > > > smp_{wmb,rmb}() must be in matched pairs, and mb() must be matched with
> > > > > > > itself.
> > > > > > 
> > > > > > Then I'm not sure about whether our previous plan still stands, are we
> > > > > > OK with using ACCESS_ONCE here and forgetting about the memory
> > > > > > barriers given the current usage?
> > > > > 
> > > > > The problem is not the current usage but how it could be used. Debugging
> > > > > memory ordering is quite a pain so I would prefer this to be fixed
> > > > > correctly.
> > > > 
> > > > But in this case it wouldn't be a pain, because the only possible
> > > > failure mode is if the processor faults trying to read opt_bootscrub, right?
> > > 
> > > Possibly. But I don't see any reason to defer the fix until someone comes up
> > > with unreliable crash.
> > 
> > If we have to go down that route, shouldn't we also protect
> > system_state with a lock so that it cannot be modified by a CPU while
> > it's being read from another?
> 
> The locking might be a bit too much. Modifying the system_state should not
> be an issue if you put the correct barrier in place.

What about the following scenario?

BSP               write;wmb();    remove init mapping;
AP   rmb();read                                          read init var;
    -----------time------>

Yes matching barriers are in place, but the result is still wrong. Can
this happen?  Even if we make opt_bootscrub non-init to avoid the fault,
we just defer the error to a later point.

This isn't really about coherency. Maybe we should put reading state
under heap lock?

Wei.

> 
> Cheers,
> 
> -- 
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] mm: fix LLVM code-generation issue

[Julien Grall]

On 23/11/2018 12:26, Wei Liu wrote:
> On Fri, Nov 23, 2018 at 11:36:48AM +0000, Julien Grall wrote:
>>
>>
>> On 23/11/2018 11:23, Roger Pau Monné wrote:
>>> On Thu, Nov 22, 2018 at 05:46:19PM +0000, Julien Grall wrote:
>>>>
>>>>
>>>> On 11/22/18 5:04 PM, George Dunlap wrote:
>>>>> On 11/22/18 4:45 PM, Julien Grall wrote:
>>>>>> Hi Roger,
>>>>>>
>>>>>> On 11/22/18 4:39 PM, Roger Pau Monné wrote:
>>>>>>> On Thu, Nov 22, 2018 at 04:22:34PM +0000, Andrew Cooper wrote:
>>>>>>>> On 22/11/2018 16:07, Roger Pau Monné wrote:
>>>>>>>>> On Thu, Nov 22, 2018 at 03:23:41PM +0000, Andrew Cooper wrote:
>>>>>>>>>> On 22/11/2018 15:20, Roger Pau Monné wrote:
>>>>>>>>>>> On Thu, Nov 22, 2018 at 02:03:55PM +0000, Julien Grall wrote:
>>>>>>>>>>>> Hi Jan,
>>>>>>>>>>>>
>>>>>>>>>>>> On 11/22/18 1:36 PM, Jan Beulich wrote:
>>>>>>>>>>>>>>>> On 22.11.18 at 14:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>>>>>>>> I think Julien's point is that without explicitly barriers, CPU0's
>>>>>>>>>>>>>> update to system_state may not be visible on CPU1, even though the
>>>>>>>>>>>>>> mappings have been shot down.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Therefore, from the processors point of view, it did everything
>>>>>>>>>>>>>> correctly, and hit a real pagefault.
>>>>>>>>>>>>> Boot time updates of system_state should be of no interest here,
>>>>>>>>>>>>> as at that time the APs are all idling.
>>>>>>>>>>>> That's probably true today. But this code looks rather fragile as
>>>>>>>>>>>> you don't
>>>>>>>>>>>> know how this is going to be used in the future.
>>>>>>>>>>>>
>>>>>>>>>>>> If you decide to gate init code with system_state, then you need
>>>>>>>>>>>> a barrier
>>>>>>>>>>>> to ensure the code is future proof.
>>>>>>>>>>> Wouldn't it be enough to declare system_state as volatile?
>>>>>>>>>> No.  volatility (or lack thereof) is a compiler level construct.
>>>>>>>>>>
>>>>>>>>>> ARM has weaker cache coherency than x86, so a write which has
>>>>>>>>>> completed
>>>>>>>>>> on one CPU0 in the past may legitimately not be visible on CPU1 yet.
>>>>>>>>>>
>>>>>>>>>> If you need guarantees about the visibility of updated, you must use
>>>>>>>>>> appropriate barriers.
>>>>>>>>> Right. There's some differences between ARM and x86, ARM sets
>>>>>>>>> SYS_STATE_active and continues to make use of init functions. In any
>>>>>>>>> case I have the following diff:
>>>>>>>>>
>>>>>>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>>>>>>> index e83221ab79..cf50d05620 100644
>>>>>>>>> --- a/xen/arch/arm/setup.c
>>>>>>>>> +++ b/xen/arch/arm/setup.c
>>>>>>>>> @@ -966,6 +966,7 @@ void __init start_xen(unsigned long
>>>>>>>>> boot_phys_offset,
>>>>>>>>>          serial_endboot();
>>>>>>>>>            system_state = SYS_STATE_active;
>>>>>>>>> +    smp_wmb();
>>>>>>>>>            create_domUs();
>>>>>>>>>      diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>>>>>>>> index 9cbff22fb3..41044c0b6f 100644
>>>>>>>>> --- a/xen/arch/x86/setup.c
>>>>>>>>> +++ b/xen/arch/x86/setup.c
>>>>>>>>> @@ -593,6 +593,7 @@ static void noinline init_done(void)
>>>>>>>>>          unsigned long start, end;
>>>>>>>>>            system_state = SYS_STATE_active;
>>>>>>>>> +    smp_wmb();
>>>>>>>>>            domain_unpause_by_systemcontroller(dom0);
>>>>>>>>
>>>>>>>> I'm afraid that that won't do anything to help at all.
>>>>>>>>
>>>>>>>> smp_{wmb,rmb}() must be in matched pairs, and mb() must be matched with
>>>>>>>> itself.
>>>>>>>
>>>>>>> Then I'm not sure about whether our previous plan still stands, are we
>>>>>>> OK with using ACCESS_ONCE here and forgetting about the memory
>>>>>>> barriers given the current usage?
>>>>>>
>>>>>> The problem is not the current usage but how it could be used. Debugging
>>>>>> memory ordering is quite a pain so I would prefer this to be fixed
>>>>>> correctly.
>>>>>
>>>>> But in this case it wouldn't be a pain, because the only possible
>>>>> failure mode is if the processor faults trying to read opt_bootscrub, right?
>>>>
>>>> Possibly. But I don't see any reason to defer the fix until someone comes up
>>>> with unreliable crash.
>>>
>>> If we have to go down that route, shouldn't we also protect
>>> system_state with a lock so that it cannot be modified by a CPU while
>>> it's being read from another?
>>
>> The locking might be a bit too much. Modifying the system_state should not
>> be an issue if you put the correct barrier in place.
> 
> What about the following scenario?
> 
> BSP               write;wmb();    remove init mapping;
> AP   rmb();read                                          read init var;
>      -----------time------>

I don't think the barrier would be correctly placed for the AP. You would need 
to do:

read system_state; rmb(); read init var;

> 
> Yes matching barriers are in place, but the result is still wrong.
> Can
> this happen?

Hmmm yes. Because there are still a chance that system_state != ACTIVE but by 
the time we read init it may have been clobbered.

>  Even if we make opt_bootscrub non-init to avoid the fault,
> we just defer the error to a later point.

What would be the error if opt_bootscrub is read one more time?

> 
> This isn't really about coherency. Maybe we should put reading state
> under heap lock?

But to be honest, the problem is we are trying to read init data from core code. 
  Do we really want to keep that option init?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel