Re: [RESEND][PATCH] xen/arm: sched: Ensure the vCPU context is seen before vcpu_pause() returns

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 23 Sep 2020, Bertrand Marquis wrote:
> Hi,
> 
> > On 22 Sep 2020, at 20:31, Julien Grall <julien@xxxxxxx> wrote:
> > 
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> > 
> > Some callers of vcpu_pause() will expect to access the latest vcpu
> > context when the function returns (see XENDOMCTL_{set,get}vcpucontext}.
> > 
> > However, the latest vCPU context can only be observed after
> > v->is_running has been observed to be false.
> > 
> > As there is no memory barrier instruction generated, a processor could
> > try to speculatively access the vCPU context before it was observed.
> > 
> > To prevent the corruption of the vCPU context, we need to insert a
> > memory barrier instruction after v->is_running is observed and before
> > the context is accessed. This barrier is added in sync_vcpu_execstate()
> > as it seems to be the place where we expect the synchronization to
> > happen.
> > 
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> > ---
> > 
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
> > Cc: Dario Faggioli <dfaggioli@xxxxxxxx>
> > Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> > 
> > I am also adding the x86 and scheduler maintainers because I am not sure
> > whether this barrier should be part of the common code instead.
> > ---
> > xen/arch/arm/domain.c | 15 ++++++++++++++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 9258f6d3faa2..3b37f899b9da 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -371,7 +371,20 @@ void sync_local_execstate(void)
> > 
> > void sync_vcpu_execstate(struct vcpu *v)
> > {
> > -    /* Nothing to do -- no lazy switching */
> > +    /*
> > +     * We don't support lazy switching.
> > +     *
> > +     * However the context may have been saved from a remote pCPU so we
> > +     * need a barrier to ensure it is observed before continuing.
> > +     *
> > +     * Per vcpu_context_saved(), the context can be observed when
> > +     * v->is_running is false (the caller should check it before calling
> > +     * this function).
> > +     *
> > +     * Note this is a full barrier to also prevent update of the context
> > +     * to happen before it was observed.
> > +     */
> > +    smp_mb();
> > }
> > 
> > #define NEXT_ARG(fmt, args)                                                 
> > \
> > -- 
> > 2.17.1
> > 
>