From: Julien Grall <jgrall@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.

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@amazon.com>

---

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Dario Faggioli <dfaggioli@suse.com>
Cc: Bertrand Marquis <Bertrand.Marquis@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

Hi Julien,

> On 22 Sep 2020, at 20:31, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@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 <jgrall@amazon.com>
> 
> ---
> 
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Dario Faggioli <dfaggioli@suse.com>
> Cc: Bertrand Marquis <Bertrand.Marquis@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
> 

On 23/09/2020 12:08, Bertrand Marquis wrote:
> Hi Julien,
> 
>> On 22 Sep 2020, at 20:31, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@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.

Memory barriers usually work in pair. We have a write barrier in 
vcpu_context_saved() but no read barrier in the code relying on the 
v->is_running.

> 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.

That's not what I am trying to fix (see above). Instead, this patch will 
ensure that when a pCPU observe v->is_running = false, then it can rely 
on the context of the vCPU to be valid.

Cheers,

-- 
Julien Grall

> On 23 Sep 2020, at 12:27, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 23/09/2020 12:08, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 22 Sep 2020, at 20:31, Julien Grall <julien@xen.org> wrote:
>>> 
>>> From: Julien Grall <jgrall@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.
> 
> Memory barriers usually work in pair. We have a write barrier in vcpu_context_saved() but no read barrier in the code relying on the v->is_running.

Ok.

> 
>> 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.
> 
> That's not what I am trying to fix (see above). Instead, this patch will ensure that when a pCPU observe v->is_running = false, then it can rely on the context of the vCPU to be valid.

Ok you need a memory barrier after setting is_running to false, got it.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Hi,

> On 22 Sep 2020, at 20:31, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@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.
> 
> 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@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> 
> ---
> 
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Dario Faggioli <dfaggioli@suse.com>
> Cc: Bertrand Marquis <Bertrand.Marquis@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
> 

On Wed, 23 Sep 2020, Bertrand Marquis wrote:
> Hi,
> 
> > On 22 Sep 2020, at 20:31, Julien Grall <julien@xen.org> wrote:
> > 
> > From: Julien Grall <jgrall@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.
> > 
> > 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@amazon.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> > ---
> > 
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: George Dunlap <george.dunlap@citrix.com>
> > Cc: Dario Faggioli <dfaggioli@suse.com>
> > Cc: Bertrand Marquis <Bertrand.Marquis@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
> > 
>