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