Hi Julien,
> On 22 Sep 2020, at 20:31, Julien Grall <jul...@xen.org> wrote:
>
> From: Julien Grall <jgr...@amazon.com>
>
> 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.
The function vcpu_context_saved does contain a memory barrier already.
Shouldn’t we make sure instead that any time is_running is modified to
false there is a barrier before (which is the case in vcpu_context_saved) ?
I understand the goal here but the barrier seem very far from the modification
of is_running.
Cheers,
Bertrand
>
> 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 <jgr...@amazon.com>
>
> ---
>
> Cc: Andrew Cooper <andrew.coop...@citrix.com>
> Cc: Jan Beulich <jbeul...@suse.com>
> Cc: George Dunlap <george.dun...@citrix.com>
> Cc: Dario Faggioli <dfaggi...@suse.com>
> Cc: Bertrand Marquis <bertrand.marq...@arm.com>
>
> 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
>
