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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 21 Apr 2021, Julien Grall wrote:
> 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.

I prefer my proposal because it is easier to understand its correctness
and, like you wrote, it is OK to have one more barrier than strictly
necessary in this case. But I wouldn't argue over this.