* [RESEND][PATCH] xen/arm: sched: Ensure the vCPU context is seen before vcpu_pause() returns
@ 2020-09-22 19:31 Julien Grall
2020-09-23 11:08 ` Bertrand Marquis
2020-09-23 13:53 ` Bertrand Marquis
0 siblings, 2 replies; 6+ messages in thread
From: Julien Grall @ 2020-09-22 19:31 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
Andrew Cooper, Jan Beulich, George Dunlap, Dario Faggioli,
Bertrand Marquis
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND][PATCH] xen/arm: sched: Ensure the vCPU context is seen before vcpu_pause() returns
2020-09-22 19:31 [RESEND][PATCH] xen/arm: sched: Ensure the vCPU context is seen before vcpu_pause() returns Julien Grall
@ 2020-09-23 11:08 ` Bertrand Marquis
2020-09-23 11:27 ` Julien Grall
2020-09-23 13:53 ` Bertrand Marquis
1 sibling, 1 reply; 6+ messages in thread
From: Bertrand Marquis @ 2020-09-23 11:08 UTC (permalink / raw)
To: Julien Grall
Cc: open list:X86, Julien Grall, Stefano Stabellini,
Volodymyr Babchuk, Andrew Cooper, Jan Beulich, George Dunlap,
Dario Faggioli
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND][PATCH] xen/arm: sched: Ensure the vCPU context is seen before vcpu_pause() returns
2020-09-23 11:08 ` Bertrand Marquis
@ 2020-09-23 11:27 ` Julien Grall
2020-09-23 13:52 ` Bertrand Marquis
0 siblings, 1 reply; 6+ messages in thread
From: Julien Grall @ 2020-09-23 11:27 UTC (permalink / raw)
To: Bertrand Marquis
Cc: open list:X86, Julien Grall, Stefano Stabellini,
Volodymyr Babchuk, Andrew Cooper, Jan Beulich, George Dunlap,
Dario Faggioli
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND][PATCH] xen/arm: sched: Ensure the vCPU context is seen before vcpu_pause() returns
2020-09-23 11:27 ` Julien Grall
@ 2020-09-23 13:52 ` Bertrand Marquis
0 siblings, 0 replies; 6+ messages in thread
From: Bertrand Marquis @ 2020-09-23 13:52 UTC (permalink / raw)
To: Julien Grall
Cc: open list:X86, Julien Grall, Stefano Stabellini,
Volodymyr Babchuk, Andrew Cooper, Jan Beulich, George Dunlap,
Dario Faggioli
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND][PATCH] xen/arm: sched: Ensure the vCPU context is seen before vcpu_pause() returns
2020-09-22 19:31 [RESEND][PATCH] xen/arm: sched: Ensure the vCPU context is seen before vcpu_pause() returns Julien Grall
2020-09-23 11:08 ` Bertrand Marquis
@ 2020-09-23 13:53 ` Bertrand Marquis
2020-09-23 17:45 ` Stefano Stabellini
1 sibling, 1 reply; 6+ messages in thread
From: Bertrand Marquis @ 2020-09-23 13:53 UTC (permalink / raw)
To: Julien Grall
Cc: open list:X86, Julien Grall, Stefano Stabellini,
Volodymyr Babchuk, Andrew Cooper, Jan Beulich, George Dunlap,
Dario Faggioli
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND][PATCH] xen/arm: sched: Ensure the vCPU context is seen before vcpu_pause() returns
2020-09-23 13:53 ` Bertrand Marquis
@ 2020-09-23 17:45 ` Stefano Stabellini
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Stabellini @ 2020-09-23 17:45 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Julien Grall, open list:X86, Julien Grall, Stefano Stabellini,
Volodymyr Babchuk, Andrew Cooper, Jan Beulich, George Dunlap,
Dario Faggioli
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
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-23 17:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-22 19:31 [RESEND][PATCH] xen/arm: sched: Ensure the vCPU context is seen before vcpu_pause() returns Julien Grall
2020-09-23 11:08 ` Bertrand Marquis
2020-09-23 11:27 ` Julien Grall
2020-09-23 13:52 ` Bertrand Marquis
2020-09-23 13:53 ` Bertrand Marquis
2020-09-23 17:45 ` Stefano Stabellini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).