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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 21/04/2021 01:38, Stefano Stabellini wrote:
> On Tue, 20 Apr 2021, Julien Grall wrote:
> > 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?
> Let me start with what I had in mind:
>
> initialized = v->is_initialized;
> smp_rmb();
> if ( !initialized )
>    goto getvcpucontext_out;
>
>
> Without smp_rmb there are no guarantees that we see an up-to-date value
> of v->is_initialized, right?
No. The smp_*mb() barriers only guarantee an ordering, it doesn't say 
anything about when the values will be seen. The write may in fact still 
be in the write buffer of the other CPU.
> This READ of v->is_initialized is supposed
> to pair with the WRITE in arch_set_info_guest.
I am assuming you meant the access and not the barrier, right? 
Regardless, the barriers used, the access will happen in any order. IOW 
the following two ordering is possible:
Sequence 1:

CPU0: v->vcpu_initialized = 1
CPU1: read v->vcpu_initialized

Sequence 2:

CPU1: read v->vcpu_initialized
CPU0: v->vcpu_initialized = 0

In the first sequence, CPU1 will observe 0 and therefore bailout. In the 
second sequence, we will be observing 1 and continue.
> Relying on the the barrier in vcpu_pause is OK only if is_initialised
> was already read as "1".
>
> I think it might be OK in this case, because probably nothing wrong
> happens if we read the old value of v->is_initialized as "0" but it
> doesn't seem to be a good idea.
The smp_rmb() is not going to give you that guarantee. If you need it, 
then you mostly likely want to use a synchronization primitives such as 
spin_lock().
> Theoretically it could pass a very long
> time before we see v->is_initialized as "1" without the smp_rmb.
I have the impression that there might be confusion about what I am 
aiming to achieve.
The code today looks roughly like:

1) if ( v->is_initialized )
2)   return;
3) vcpu_pause();
4) /* Access registers */

My aim is to ensure that a processor will not READ any value for 4) are 
not happening before 1). If this happens, then we may provide garbagge 
to the domctl.
Returning an error is a lot better because the caller of the domctl will 
know the vCPU is not yet setup and can provide the infrastructure to try 
again.
From this PoV, your proposal and my two proposals are functionally 
equivalent. The main differences is whether there is an extra barrier 
and how easy is it to figure out the ordering.
Speaking with Ash, your proposal is probably the easiest one to 
understand. However, it also adds a barrier for the failure path (which 
is pointless).
I would like to avoid barriers when they are not necessary. Hence why I 
prefer the vcpu_pause() solution (we may want to add a comment).
Although, I could live with your proposal if the others are happy with 
it (Andrew? Jan?) because this is not an hot path.
Cheers,

--
Julien Grall