Re: [PATCH] xen/arm: Ensure the vCPU context is seen before clearing the _VPF_down

[Julien Grall <julien@xxxxxxx>]

(+ Andrew and Jan)

Hi Stefano,

On 20/04/2021 20:38, Stefano Stabellini wrote:
> On Fri, 16 Apr 2021, Julien Grall wrote:
> > > I think your explanation is correct. However, don't we need a smp_rmb()
> > > barrier after reading v->is_initialised in xen/common/domctl.c:do_domctl
> > > ? That would be the barrier that pairs with smp_wmb in regards to
> > > v->is_initialised.
> > There is already a smp_mb() in sync_vcpu_exec_state() which is called from
> > vcpu_pause() -> vcpu_sleep_sync().
> But that's too late, isn't? The v->is_initialized check is done before
> the vcpu_pause() call. We might end up taking the wrong code path:
>
> https://gitlab.com/xen-project/xen/-/blob/staging/xen/common/domctl.c#L587
> https://gitlab.com/xen-project/xen/-/blob/staging/xen/common/domctl.c#L598
I am a bit confused what you mean by "wrong path" here. There is no 
timing guarantee with a memory barrier. What the barrier will guarantee 
you is v->is_initialized will be read *before* anything after the barrier.
Are you worried that some variables in vcpu_pause() may be read before 
v->is_initialized?
> > I don't think we can ever remove the memory barrier in sync_vcpu_exec_state()
> > because the vCPU paused may have run (or initialized) on a different pCPU. So
> > I would like to rely on the barrier rather than adding an extra one (even
> > thought it is not a fast path).
> >
> > I am thinking to add a comment on top of vcpu_pause() to clarify that after
> > the call, the vCPU context will be observed without extra synchronization
> > required.
> Given that an if.. goto is involved, even if both code paths lead to
> good results, I think it would be best to have the smp_rmb() barrier
> above after the first v->is_initialised read to make sure we take the
> right path.
I don't understand what you are referring by "wrong" and "right" path. 
The processor will always execute/commit the path based on the value of 
v->is_initialized. What may happen is the processor may start to 
speculate the wrong path and then backtrack if it discovers the value is 
not the expected one. But, AFAIK, smp_rmb() is not going to protect you 
against speculation... smp_rmb() is only going to enforce a memory 
ordering between read.
> Instead of having to make sure that even the "wrong" path
> behaves correctly.
Just for clarification, I think you meant writing the following code:

if ( !v->is_initialized )
  goto getvcpucontext_out;

smp_rmb();

...

vcpu_pause();

AFAICT, there is nothing in the implementation of 
XEN_DOMCTL_getvcpucontext that justify the extra barrier (assuming we 
consider vcpu_pause() as a full memory barrier).
From your e-mail, I also could not infer what is your exact concern 
regarding the memory ordering. If you have something in mind, then would 
you mind to provide a sketch showing what could go wrong?
Cheers,

--
Julien Grall