Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On Thu, 2018-11-15 at 11:38 +, Julien Grall wrote: > On 11/15/18 11:10 AM, Mirela Simonovic wrote: > I don't think we are discussing the same thing. The discussion was > around other vCPUs, not the vCPU calling SYSTEM_SUSPEND. > > Most likely in the future, we would want to allow the toolstack to > request resuming the domain. This can be considered as an event. > > > Given the assumption, my understanding is that Xen itself will not > > unblock vCPU, except due to an interrupt targeted to the guest. > > Am I missing something? An example would be appreciated. > > At least on Arm, the current semantics of vcpu_block/vcpu_unblock is > to > block until you receive an events. > > I don't much want to restrict the definition of events to only > interrupts. To clarify my point, if you want to wake-up for any > events > then fine. > So, I certainly lack deep knowledge of PSCI, as well as other aspects of how this suspend/resume logic will work, but just to clarify on this. vcpu_unblock() may indeed be called from a number of places, but it actually wakes-up the vcpu _iff_ the vcpu is runnable. If it is not --e.g., because pause_count is not zero, or any vcpu or domain pause_flags are set-- the vcpu stays blocked (check the implementation of vcpu_wake() and of vcpu_runnable()). So, it looks to me that what you want is to be sure that when an event arrives, but the vcpus need to remain suspended, vcpu_runnable() returns false for them. OTOH, when the events that you want to wake them up arrives, you want it to return true. Whether that is better done by using pause_count (plus some other flag, as Stefano is saying) or with either an existing or new vcpu or domain pause flag, I don't know, but it looks like it could work to me. Regards, Dario -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli Software Engineer @ SUSE https://www.suse.com/ signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Mirela,
On 11/15/18 11:42 AM, Mirela Simonovic wrote:
Hi Julien,
On Thu, Nov 15, 2018 at 12:38 PM Julien Grall wrote:
Hi,
On 11/15/18 11:10 AM, Mirela Simonovic wrote:
Hi Julien,
On Thu, Nov 15, 2018 at 11:59 AM Julien Grall wrote:
Hi Mirela,
On 11/15/18 10:33 AM, Mirela Simonovic wrote:
On Thu, Nov 15, 2018 at 11:26 AM Andrew Cooper
wrote:
On 15/11/2018 10:13, Julien Grall wrote:
(+ Andre)
On 11/15/18 12:47 AM, Andrew Cooper wrote:
On 14/11/2018 12:49, Julien Grall wrote:
Hi Mirela,
On 14/11/2018 12:08, Mirela Simonovic wrote:
On 11/13/2018 09:32 AM, Andrew Cooper wrote:
On 12/11/2018 19:56, Julien Grall wrote:
Hi Andrew,
On 11/12/18 4:41 PM, Andrew Cooper wrote:
On 12/11/18 16:35, Mirela Simonovic wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e594b48d81..7f8105465c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+/* VCPU's context should not be saved if its domain is
suspended */
+if ( p->domain->is_shut_down &&
+(p->domain->shutdown_code == SHUTDOWN_suspend) )
+return;
SHUTDOWN_suspend is used in Xen for other purpose (see
SCHEDOP_shutdown). The other user of that code relies on all the
state
to be saved on suspend.
We just need a flag to mark a domain as suspended, and I do
believe
SHUTDOWN_suspend is not used anywhere else.
Let's come back on this.
SHUTDOWN_suspend is used for migration. Grep for it through the
Xen
tree and you'll find several pieces of documentation, including the
description of what this shutdown code means>>>
What you are introducing here is not a shutdown - it is a suspend
with
the intent to resume executing later. As such, it shouldn't use
Xen's
shutdown infrastructure, which exists mainly to communicate with
the
toolstack.
Would domain pause/unpause be a better solution?
Actually yes - that sounds like a very neat solution.
I believe domain pause will not work here - a domain cannot pause
itself, i.e. current domain cannot be paused. This functionality
seems to assume that a domain is pausing another domain. Or I missed
the point.
Yes domain pause/unpause will not work. However, you can introduce a
boolean to tell you whether the domain was suspend.
I actually quite like how suspend work for x86 HVM. This is based on
pause/unpause. Have a look at hvm_s3_{suspend/resume}.
That only exists because the ACPI controller is/was implemented in
QEMU. I wouldn't recommend copying it.
May I ask why? I don't think the properties are very different from
Arm here.
If you observe, that can only be actioned by a hypercall from qemu. It
can't be actioned by an ACPI controller emulated by Xen.
Having spent some more time thinking about this problem, what properties
does the PSCI call have?
I gather from other parts of this thread that there may be a partial
reset of state? Beyond that, what else?
I ask, because conceptually, domU suspend to RAM is literally just
"de-schedule until we want to wake up", and in this case, it appears to
be "wake up on any of the still-active interrupts". We've already got a
scheduler API for that, and its called vcpu_block(). This already
exists with "wait until a new event occurs" semantics.
Is there anything else I've missed?
That's correct, and I agree. BTW that is what's implemented in this series.
All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
only events on that vCPU can trigger a resume. All the other event
should not be taken into account.
What other events are talking about here?
vcpu_unblock is not only called when you receive interrupts. It can be
called in other place when you receive an events.
From the Arm Arm, an event can be anything. So do we really want to
wake-up on any events?
My worry with vcpu_block() is we don't prevent the other vCPUs to run.
Technically they should be off, but I would like some safety to avoid
any potential corner case (i.e other way to turn a vCPU on).
Other vCPUs are hot-unplugged (offlined) by the guest. If that is not
the case, SYSTEM_SUSPEND will return an error.
Could you please clarify what a potential corner case would be?
PSCI CPU_ON is not the only way to online a vCPU. I merely want to
prevent other path to play with the vCPU when it is not necessary.
[...]
If instead of waiting for any event, you need to wait for a specific
event, there is also vcpu_poll() which is a related scheduler API.
~Andrew
Some content disappeared, so I'll write here to avoid thread branching.
The semantic of vCPU block and domain_pause is not the same. When
guest suspends the domain is not (and should not be) paused, instead
its last online vCPU is blocked waiting on an interrupt. That's it.
Well no, you will block until you receive an event. Interrupts are one
of them.
Do we want to consider all events as wakeup event?
I
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On Thu, 15 Nov 2018, Mirela Simonovic wrote:
> Hi Julien,
>
> On Thu, Nov 15, 2018 at 12:38 PM Julien Grall wrote:
> >
> > Hi,
> >
> > On 11/15/18 11:10 AM, Mirela Simonovic wrote:
> > > Hi Julien,
> > >
> > > On Thu, Nov 15, 2018 at 11:59 AM Julien Grall
> > > wrote:
> > >>
> > >> Hi Mirela,
> > >>
> > >> On 11/15/18 10:33 AM, Mirela Simonovic wrote:
> > >>> On Thu, Nov 15, 2018 at 11:26 AM Andrew Cooper
> > >>> wrote:
> >
> > On 15/11/2018 10:13, Julien Grall wrote:
> > > (+ Andre)
> > >
> > > On 11/15/18 12:47 AM, Andrew Cooper wrote:
> > >> On 14/11/2018 12:49, Julien Grall wrote:
> > >>> Hi Mirela,
> > >>>
> > >>> On 14/11/2018 12:08, Mirela Simonovic wrote:
> >
> >
> > On 11/13/2018 09:32 AM, Andrew Cooper wrote:
> > > On 12/11/2018 19:56, Julien Grall wrote:
> > >> Hi Andrew,
> > >>
> > >> On 11/12/18 4:41 PM, Andrew Cooper wrote:
> > >>> On 12/11/18 16:35, Mirela Simonovic wrote:
> > >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > >> index e594b48d81..7f8105465c 100644
> > >> --- a/xen/arch/arm/domain.c
> > >> +++ b/xen/arch/arm/domain.c
> > >> @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu
> > >> *p)
> > >>if ( is_idle_vcpu(p) )
> > >>return;
> > >>
> > >> +/* VCPU's context should not be saved if its domain is
> > >> suspended */
> > >> +if ( p->domain->is_shut_down &&
> > >> +(p->domain->shutdown_code == SHUTDOWN_suspend) )
> > >> +return;
> > > SHUTDOWN_suspend is used in Xen for other purpose (see
> > > SCHEDOP_shutdown). The other user of that code relies on all
> > > the
> > > state
> > > to be saved on suspend.
> > >
> > We just need a flag to mark a domain as suspended, and I do
> > believe
> > SHUTDOWN_suspend is not used anywhere else.
> > Let's come back on this.
> > >>> SHUTDOWN_suspend is used for migration. Grep for it through the
> > >>> Xen
> > >>> tree and you'll find several pieces of documentation, including
> > >>> the
> > >>> description of what this shutdown code means>>>
> > >>> What you are introducing here is not a shutdown - it is a
> > >>> suspend
> > >>> with
> > >>> the intent to resume executing later. As such, it shouldn't use
> > >>> Xen's
> > >>> shutdown infrastructure, which exists mainly to communicate with
> > >>> the
> > >>> toolstack.
> > >> Would domain pause/unpause be a better solution?
> > > Actually yes - that sounds like a very neat solution.
> >
> > I believe domain pause will not work here - a domain cannot pause
> > itself, i.e. current domain cannot be paused. This functionality
> > seems to assume that a domain is pausing another domain. Or I
> > missed
> > the point.
> > >>>
> > >>> Yes domain pause/unpause will not work. However, you can introduce a
> > >>> boolean to tell you whether the domain was suspend.
> > >>>
> > >>> I actually quite like how suspend work for x86 HVM. This is based on
> > >>> pause/unpause. Have a look at hvm_s3_{suspend/resume}.
> > >>
> > >> That only exists because the ACPI controller is/was implemented in
> > >> QEMU. I wouldn't recommend copying it.
> > >
> > > May I ask why? I don't think the properties are very different from
> > > Arm here.
> >
> > If you observe, that can only be actioned by a hypercall from qemu. It
> > can't be actioned by an ACPI controller emulated by Xen.
> >
> > >
> > >>
> > >> Having spent some more time thinking about this problem, what
> > >> properties
> > >> does the PSCI call have?
> > >>
> > >> I gather from other parts of this thread that there may be a partial
> > >> reset of state? Beyond that, what else?
> > >>
> > >> I ask, because conceptually, domU suspend to RAM is literally just
> > >> "de-schedule until we want to wake up", and in this case, it appears
> > >> to
> > >> be "wake up on any of the still-active interrupts". We've already
> > >> got a
> > >> scheduler API for that, and its called vcpu_block(). This already
> > >> exists with "wait until a new event occurs" semantics.
> > >>
> > >> Is there anything else I've missed?
> > >
> > >>>
> > >>> That's correct, and I agree. BTW that is what's implemented in this
> > >>> series.
> > >>>
> > > All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
> > > only events on that vCPU can trigger a
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Julien,
On Thu, Nov 15, 2018 at 12:38 PM Julien Grall wrote:
>
> Hi,
>
> On 11/15/18 11:10 AM, Mirela Simonovic wrote:
> > Hi Julien,
> >
> > On Thu, Nov 15, 2018 at 11:59 AM Julien Grall wrote:
> >>
> >> Hi Mirela,
> >>
> >> On 11/15/18 10:33 AM, Mirela Simonovic wrote:
> >>> On Thu, Nov 15, 2018 at 11:26 AM Andrew Cooper
> >>> wrote:
>
> On 15/11/2018 10:13, Julien Grall wrote:
> > (+ Andre)
> >
> > On 11/15/18 12:47 AM, Andrew Cooper wrote:
> >> On 14/11/2018 12:49, Julien Grall wrote:
> >>> Hi Mirela,
> >>>
> >>> On 14/11/2018 12:08, Mirela Simonovic wrote:
>
>
> On 11/13/2018 09:32 AM, Andrew Cooper wrote:
> > On 12/11/2018 19:56, Julien Grall wrote:
> >> Hi Andrew,
> >>
> >> On 11/12/18 4:41 PM, Andrew Cooper wrote:
> >>> On 12/11/18 16:35, Mirela Simonovic wrote:
> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >> index e594b48d81..7f8105465c 100644
> >> --- a/xen/arch/arm/domain.c
> >> +++ b/xen/arch/arm/domain.c
> >> @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
> >>if ( is_idle_vcpu(p) )
> >>return;
> >>
> >> +/* VCPU's context should not be saved if its domain is
> >> suspended */
> >> +if ( p->domain->is_shut_down &&
> >> +(p->domain->shutdown_code == SHUTDOWN_suspend) )
> >> +return;
> > SHUTDOWN_suspend is used in Xen for other purpose (see
> > SCHEDOP_shutdown). The other user of that code relies on all the
> > state
> > to be saved on suspend.
> >
> We just need a flag to mark a domain as suspended, and I do
> believe
> SHUTDOWN_suspend is not used anywhere else.
> Let's come back on this.
> >>> SHUTDOWN_suspend is used for migration. Grep for it through the
> >>> Xen
> >>> tree and you'll find several pieces of documentation, including
> >>> the
> >>> description of what this shutdown code means>>>
> >>> What you are introducing here is not a shutdown - it is a suspend
> >>> with
> >>> the intent to resume executing later. As such, it shouldn't use
> >>> Xen's
> >>> shutdown infrastructure, which exists mainly to communicate with
> >>> the
> >>> toolstack.
> >> Would domain pause/unpause be a better solution?
> > Actually yes - that sounds like a very neat solution.
>
> I believe domain pause will not work here - a domain cannot pause
> itself, i.e. current domain cannot be paused. This functionality
> seems to assume that a domain is pausing another domain. Or I missed
> the point.
> >>>
> >>> Yes domain pause/unpause will not work. However, you can introduce a
> >>> boolean to tell you whether the domain was suspend.
> >>>
> >>> I actually quite like how suspend work for x86 HVM. This is based on
> >>> pause/unpause. Have a look at hvm_s3_{suspend/resume}.
> >>
> >> That only exists because the ACPI controller is/was implemented in
> >> QEMU. I wouldn't recommend copying it.
> >
> > May I ask why? I don't think the properties are very different from
> > Arm here.
>
> If you observe, that can only be actioned by a hypercall from qemu. It
> can't be actioned by an ACPI controller emulated by Xen.
>
> >
> >>
> >> Having spent some more time thinking about this problem, what
> >> properties
> >> does the PSCI call have?
> >>
> >> I gather from other parts of this thread that there may be a partial
> >> reset of state? Beyond that, what else?
> >>
> >> I ask, because conceptually, domU suspend to RAM is literally just
> >> "de-schedule until we want to wake up", and in this case, it appears to
> >> be "wake up on any of the still-active interrupts". We've already got
> >> a
> >> scheduler API for that, and its called vcpu_block(). This already
> >> exists with "wait until a new event occurs" semantics.
> >>
> >> Is there anything else I've missed?
> >
> >>>
> >>> That's correct, and I agree. BTW that is what's implemented in this
> >>> series.
> >>>
> > All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
> > only events on that vCPU can trigger a resume. All the other event
> > should not be taken into account.
> >
> >>>
> >>> What other events are talking about here?
> >>
> >> vcpu_unblock is not only called when you receive interrupts. It can be
> >> called in other place when you receive an events.
> >>
> >> From the Arm Arm, an event can be anything. So do we really want to
> >> wake-up
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi,
On 11/15/18 11:10 AM, Mirela Simonovic wrote:
Hi Julien,
On Thu, Nov 15, 2018 at 11:59 AM Julien Grall wrote:
Hi Mirela,
On 11/15/18 10:33 AM, Mirela Simonovic wrote:
On Thu, Nov 15, 2018 at 11:26 AM Andrew Cooper
wrote:
On 15/11/2018 10:13, Julien Grall wrote:
(+ Andre)
On 11/15/18 12:47 AM, Andrew Cooper wrote:
On 14/11/2018 12:49, Julien Grall wrote:
Hi Mirela,
On 14/11/2018 12:08, Mirela Simonovic wrote:
On 11/13/2018 09:32 AM, Andrew Cooper wrote:
On 12/11/2018 19:56, Julien Grall wrote:
Hi Andrew,
On 11/12/18 4:41 PM, Andrew Cooper wrote:
On 12/11/18 16:35, Mirela Simonovic wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e594b48d81..7f8105465c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+/* VCPU's context should not be saved if its domain is
suspended */
+if ( p->domain->is_shut_down &&
+(p->domain->shutdown_code == SHUTDOWN_suspend) )
+return;
SHUTDOWN_suspend is used in Xen for other purpose (see
SCHEDOP_shutdown). The other user of that code relies on all the
state
to be saved on suspend.
We just need a flag to mark a domain as suspended, and I do
believe
SHUTDOWN_suspend is not used anywhere else.
Let's come back on this.
SHUTDOWN_suspend is used for migration. Grep for it through the
Xen
tree and you'll find several pieces of documentation, including the
description of what this shutdown code means>>>
What you are introducing here is not a shutdown - it is a suspend
with
the intent to resume executing later. As such, it shouldn't use
Xen's
shutdown infrastructure, which exists mainly to communicate with
the
toolstack.
Would domain pause/unpause be a better solution?
Actually yes - that sounds like a very neat solution.
I believe domain pause will not work here - a domain cannot pause
itself, i.e. current domain cannot be paused. This functionality
seems to assume that a domain is pausing another domain. Or I missed
the point.
Yes domain pause/unpause will not work. However, you can introduce a
boolean to tell you whether the domain was suspend.
I actually quite like how suspend work for x86 HVM. This is based on
pause/unpause. Have a look at hvm_s3_{suspend/resume}.
That only exists because the ACPI controller is/was implemented in
QEMU. I wouldn't recommend copying it.
May I ask why? I don't think the properties are very different from
Arm here.
If you observe, that can only be actioned by a hypercall from qemu. It
can't be actioned by an ACPI controller emulated by Xen.
Having spent some more time thinking about this problem, what properties
does the PSCI call have?
I gather from other parts of this thread that there may be a partial
reset of state? Beyond that, what else?
I ask, because conceptually, domU suspend to RAM is literally just
"de-schedule until we want to wake up", and in this case, it appears to
be "wake up on any of the still-active interrupts". We've already got a
scheduler API for that, and its called vcpu_block(). This already
exists with "wait until a new event occurs" semantics.
Is there anything else I've missed?
That's correct, and I agree. BTW that is what's implemented in this series.
All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
only events on that vCPU can trigger a resume. All the other event
should not be taken into account.
What other events are talking about here?
vcpu_unblock is not only called when you receive interrupts. It can be
called in other place when you receive an events.
From the Arm Arm, an event can be anything. So do we really want to
wake-up on any events?
My worry with vcpu_block() is we don't prevent the other vCPUs to run.
Technically they should be off, but I would like some safety to avoid
any potential corner case (i.e other way to turn a vCPU on).
Other vCPUs are hot-unplugged (offlined) by the guest. If that is not
the case, SYSTEM_SUSPEND will return an error.
Could you please clarify what a potential corner case would be?
PSCI CPU_ON is not the only way to online a vCPU. I merely want to
prevent other path to play with the vCPU when it is not necessary.
[...]
If instead of waiting for any event, you need to wait for a specific
event, there is also vcpu_poll() which is a related scheduler API.
~Andrew
Some content disappeared, so I'll write here to avoid thread branching.
The semantic of vCPU block and domain_pause is not the same. When
guest suspends the domain is not (and should not be) paused, instead
its last online vCPU is blocked waiting on an interrupt. That's it.
Well no, you will block until you receive an event. Interrupts are one
of them.
Do we want to consider all events as wakeup event?
I think we need to assume that events are not triggered via toolstack,
Andrew made a good point.
I don't think we are discussing
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Julien,
On Thu, Nov 15, 2018 at 11:59 AM Julien Grall wrote:
>
> Hi Mirela,
>
> On 11/15/18 10:33 AM, Mirela Simonovic wrote:
> > On Thu, Nov 15, 2018 at 11:26 AM Andrew Cooper
> > wrote:
> >>
> >> On 15/11/2018 10:13, Julien Grall wrote:
> >>> (+ Andre)
> >>>
> >>> On 11/15/18 12:47 AM, Andrew Cooper wrote:
> On 14/11/2018 12:49, Julien Grall wrote:
> > Hi Mirela,
> >
> > On 14/11/2018 12:08, Mirela Simonovic wrote:
> >>
> >>
> >> On 11/13/2018 09:32 AM, Andrew Cooper wrote:
> >>> On 12/11/2018 19:56, Julien Grall wrote:
> Hi Andrew,
>
> On 11/12/18 4:41 PM, Andrew Cooper wrote:
> > On 12/11/18 16:35, Mirela Simonovic wrote:
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index e594b48d81..7f8105465c 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
> if ( is_idle_vcpu(p) )
> return;
>
> +/* VCPU's context should not be saved if its domain is
> suspended */
> +if ( p->domain->is_shut_down &&
> +(p->domain->shutdown_code == SHUTDOWN_suspend) )
> +return;
> >>> SHUTDOWN_suspend is used in Xen for other purpose (see
> >>> SCHEDOP_shutdown). The other user of that code relies on all the
> >>> state
> >>> to be saved on suspend.
> >>>
> >> We just need a flag to mark a domain as suspended, and I do
> >> believe
> >> SHUTDOWN_suspend is not used anywhere else.
> >> Let's come back on this.
> > SHUTDOWN_suspend is used for migration. Grep for it through the
> > Xen
> > tree and you'll find several pieces of documentation, including the
> > description of what this shutdown code means.
> >
> > What you are introducing here is not a shutdown - it is a suspend
> > with
> > the intent to resume executing later. As such, it shouldn't use
> > Xen's
> > shutdown infrastructure, which exists mainly to communicate with
> > the
> > toolstack.
> Would domain pause/unpause be a better solution?
> >>> Actually yes - that sounds like a very neat solution.
> >>
> >> I believe domain pause will not work here - a domain cannot pause
> >> itself, i.e. current domain cannot be paused. This functionality
> >> seems to assume that a domain is pausing another domain. Or I missed
> >> the point.
> >
> > Yes domain pause/unpause will not work. However, you can introduce a
> > boolean to tell you whether the domain was suspend.
> >
> > I actually quite like how suspend work for x86 HVM. This is based on
> > pause/unpause. Have a look at hvm_s3_{suspend/resume}.
>
> That only exists because the ACPI controller is/was implemented in
> QEMU. I wouldn't recommend copying it.
> >>>
> >>> May I ask why? I don't think the properties are very different from
> >>> Arm here.
> >>
> >> If you observe, that can only be actioned by a hypercall from qemu. It
> >> can't be actioned by an ACPI controller emulated by Xen.
> >>
> >>>
>
> Having spent some more time thinking about this problem, what properties
> does the PSCI call have?
>
> I gather from other parts of this thread that there may be a partial
> reset of state? Beyond that, what else?
>
> I ask, because conceptually, domU suspend to RAM is literally just
> "de-schedule until we want to wake up", and in this case, it appears to
> be "wake up on any of the still-active interrupts". We've already got a
> scheduler API for that, and its called vcpu_block(). This already
> exists with "wait until a new event occurs" semantics.
>
> Is there anything else I've missed?
> >>>
> >
> > That's correct, and I agree. BTW that is what's implemented in this series.
> >
> >>> All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
> >>> only events on that vCPU can trigger a resume. All the other event
> >>> should not be taken into account.
> >>>
> >
> > What other events are talking about here?
>
> vcpu_unblock is not only called when you receive interrupts. It can be
> called in other place when you receive an events.
>
> From the Arm Arm, an event can be anything. So do we really want to
> wake-up on any events?
>
> >
> >>> My worry with vcpu_block() is we don't prevent the other vCPUs to run.
> >>> Technically they should be off, but I would like some safety to avoid
> >>> any potential corner case (i.e other way to turn a vCPU on).
> >>
> >
> > Other vCPUs are hot-unplugged (offlined) by the guest. If that is not
> > the case, SYSTEM_SUSPEND will return an error.
> > Could you please clarify what
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Mirela,
On 11/15/18 10:33 AM, Mirela Simonovic wrote:
On Thu, Nov 15, 2018 at 11:26 AM Andrew Cooper
wrote:
On 15/11/2018 10:13, Julien Grall wrote:
(+ Andre)
On 11/15/18 12:47 AM, Andrew Cooper wrote:
On 14/11/2018 12:49, Julien Grall wrote:
Hi Mirela,
On 14/11/2018 12:08, Mirela Simonovic wrote:
On 11/13/2018 09:32 AM, Andrew Cooper wrote:
On 12/11/2018 19:56, Julien Grall wrote:
Hi Andrew,
On 11/12/18 4:41 PM, Andrew Cooper wrote:
On 12/11/18 16:35, Mirela Simonovic wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e594b48d81..7f8105465c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+/* VCPU's context should not be saved if its domain is
suspended */
+if ( p->domain->is_shut_down &&
+(p->domain->shutdown_code == SHUTDOWN_suspend) )
+return;
SHUTDOWN_suspend is used in Xen for other purpose (see
SCHEDOP_shutdown). The other user of that code relies on all the
state
to be saved on suspend.
We just need a flag to mark a domain as suspended, and I do
believe
SHUTDOWN_suspend is not used anywhere else.
Let's come back on this.
SHUTDOWN_suspend is used for migration. Grep for it through the
Xen
tree and you'll find several pieces of documentation, including the
description of what this shutdown code means.
What you are introducing here is not a shutdown - it is a suspend
with
the intent to resume executing later. As such, it shouldn't use
Xen's
shutdown infrastructure, which exists mainly to communicate with
the
toolstack.
Would domain pause/unpause be a better solution?
Actually yes - that sounds like a very neat solution.
I believe domain pause will not work here - a domain cannot pause
itself, i.e. current domain cannot be paused. This functionality
seems to assume that a domain is pausing another domain. Or I missed
the point.
Yes domain pause/unpause will not work. However, you can introduce a
boolean to tell you whether the domain was suspend.
I actually quite like how suspend work for x86 HVM. This is based on
pause/unpause. Have a look at hvm_s3_{suspend/resume}.
That only exists because the ACPI controller is/was implemented in
QEMU. I wouldn't recommend copying it.
May I ask why? I don't think the properties are very different from
Arm here.
If you observe, that can only be actioned by a hypercall from qemu. It
can't be actioned by an ACPI controller emulated by Xen.
Having spent some more time thinking about this problem, what properties
does the PSCI call have?
I gather from other parts of this thread that there may be a partial
reset of state? Beyond that, what else?
I ask, because conceptually, domU suspend to RAM is literally just
"de-schedule until we want to wake up", and in this case, it appears to
be "wake up on any of the still-active interrupts". We've already got a
scheduler API for that, and its called vcpu_block(). This already
exists with "wait until a new event occurs" semantics.
Is there anything else I've missed?
That's correct, and I agree. BTW that is what's implemented in this series.
All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
only events on that vCPU can trigger a resume. All the other event
should not be taken into account.
What other events are talking about here?
vcpu_unblock is not only called when you receive interrupts. It can be
called in other place when you receive an events.
From the Arm Arm, an event can be anything. So do we really want to
wake-up on any events?
My worry with vcpu_block() is we don't prevent the other vCPUs to run.
Technically they should be off, but I would like some safety to avoid
any potential corner case (i.e other way to turn a vCPU on).
Other vCPUs are hot-unplugged (offlined) by the guest. If that is not
the case, SYSTEM_SUSPEND will return an error.
Could you please clarify what a potential corner case would be?
PSCI CPU_ON is not the only way to online a vCPU. I merely want to
prevent other path to play with the vCPU when it is not necessary.
[...]
If instead of waiting for any event, you need to wait for a specific
event, there is also vcpu_poll() which is a related scheduler API.
~Andrew
Some content disappeared, so I'll write here to avoid thread branching.
The semantic of vCPU block and domain_pause is not the same. When
guest suspends the domain is not (and should not be) paused, instead
its last online vCPU is blocked waiting on an interrupt. That's it.
Well no, you will block until you receive an event. Interrupts are one
of them.
Do we want to consider all events as wakeup event?
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 15/11/2018 10:36, Julien Grall wrote:
> Hi Andrew,
>
> On 11/15/18 10:26 AM, Andrew Cooper wrote:
>> On 15/11/2018 10:13, Julien Grall wrote:
>>> (+ Andre)
>>>
>>> On 11/15/18 12:47 AM, Andrew Cooper wrote:
On 14/11/2018 12:49, Julien Grall wrote:
> Hi Mirela,
>
> On 14/11/2018 12:08, Mirela Simonovic wrote:
>>
>>
>> On 11/13/2018 09:32 AM, Andrew Cooper wrote:
>>> On 12/11/2018 19:56, Julien Grall wrote:
Hi Andrew,
On 11/12/18 4:41 PM, Andrew Cooper wrote:
> On 12/11/18 16:35, Mirela Simonovic wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e594b48d81..7f8105465c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu
*p)
if ( is_idle_vcpu(p) )
return;
+ /* VCPU's context should not be saved if its domain is
suspended */
+ if ( p->domain->is_shut_down &&
+ (p->domain->shutdown_code == SHUTDOWN_suspend) )
+ return;
>>> SHUTDOWN_suspend is used in Xen for other purpose (see
>>> SCHEDOP_shutdown). The other user of that code relies on all
>>> the
>>> state
>>> to be saved on suspend.
>>>
>> We just need a flag to mark a domain as suspended, and I do
>> believe
>> SHUTDOWN_suspend is not used anywhere else.
>> Let's come back on this.
> SHUTDOWN_suspend is used for migration. Grep for it through the
> Xen
> tree and you'll find several pieces of documentation,
> including the
> description of what this shutdown code means.
>
> What you are introducing here is not a shutdown - it is a suspend
> with
> the intent to resume executing later. As such, it shouldn't use
> Xen's
> shutdown infrastructure, which exists mainly to communicate with
> the
> toolstack.
Would domain pause/unpause be a better solution?
>>> Actually yes - that sounds like a very neat solution.
>>
>> I believe domain pause will not work here - a domain cannot pause
>> itself, i.e. current domain cannot be paused. This functionality
>> seems to assume that a domain is pausing another domain. Or I
>> missed vC
>> the point.
>
> Yes domain pause/unpause will not work. However, you can introduce a
> boolean to tell you whether the domain was suspend.
>
> I actually quite like how suspend work for x86 HVM. This is based on
> pause/unpause. Have a look at hvm_s3_{suspend/resume}.
That only exists because the ACPI controller is/was implemented in
QEMU. I wouldn't recommend copying it.
>>>
>>> May I ask why? I don't think the properties are very different from
>>> Arm here.
>>
>> If you observe, that can only be actioned by a hypercall from qemu. It
>> can't be actioned by an ACPI controller emulated by Xen.
>
> How does the ACPI controller emulated by Xen deal with suspend/resume?
> Do you have any pointer?
It doesn't, and that is one of the outstanding issues for trying to make
it work sensibly. In the end it wasn't merged, and is still on
someone’s TODO list.
>
>>
>>>
Having spent some more time thinking about this problem, what
properties
does the PSCI call have?
I gather from other parts of this thread that there may be a partial
reset of state? Beyond that, what else?
I ask, because conceptually, domU suspend to RAM is literally just
"de-schedule until we want to wake up", and in this case, it
appears to
be "wake up on any of the still-active interrupts". We've already
got a
scheduler API for that, and its called vcpu_block(). This already
exists with "wait until a new event occurs" semantics.
Is there anything else I've missed?
>>>
>>> All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
>>> only events on that vCPU can trigger a resume. All the other event
>>> should not be taken into account.
>>>
>>> My worry with vcpu_block() is we don't prevent the other vCPUs to run.
>>> Technically they should be off, but I would like some safety to avoid
>>> any potential corner case (i.e other way to turn a vCPU on).
>>
>> Why not have the SYSTEM_SUSPEND check that all other vCPUs are DOWN
>> first, and fail the call if not?
>
> The code already check for that. My concern is there might (today or
> in the future) other bits of Xen that can potentially turn on the vCPU
> (e.g the toolstack).
I wouldn't be concerned - there is nothing you can do about it.
Because of things like domain construction / migration / emulation /
etc, the toolstack(/control domain) has all the low level hooks into Xen
to do
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Andrew,
On 11/15/18 10:26 AM, Andrew Cooper wrote:
On 15/11/2018 10:13, Julien Grall wrote:
(+ Andre)
On 11/15/18 12:47 AM, Andrew Cooper wrote:
On 14/11/2018 12:49, Julien Grall wrote:
Hi Mirela,
On 14/11/2018 12:08, Mirela Simonovic wrote:
On 11/13/2018 09:32 AM, Andrew Cooper wrote:
On 12/11/2018 19:56, Julien Grall wrote:
Hi Andrew,
On 11/12/18 4:41 PM, Andrew Cooper wrote:
On 12/11/18 16:35, Mirela Simonovic wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e594b48d81..7f8105465c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+ /* VCPU's context should not be saved if its domain is
suspended */
+ if ( p->domain->is_shut_down &&
+ (p->domain->shutdown_code == SHUTDOWN_suspend) )
+ return;
SHUTDOWN_suspend is used in Xen for other purpose (see
SCHEDOP_shutdown). The other user of that code relies on all the
state
to be saved on suspend.
We just need a flag to mark a domain as suspended, and I do
believe
SHUTDOWN_suspend is not used anywhere else.
Let's come back on this.
SHUTDOWN_suspend is used for migration. Grep for it through the
Xen
tree and you'll find several pieces of documentation, including the
description of what this shutdown code means.
What you are introducing here is not a shutdown - it is a suspend
with
the intent to resume executing later. As such, it shouldn't use
Xen's
shutdown infrastructure, which exists mainly to communicate with
the
toolstack.
Would domain pause/unpause be a better solution?
Actually yes - that sounds like a very neat solution.
I believe domain pause will not work here - a domain cannot pause
itself, i.e. current domain cannot be paused. This functionality
seems to assume that a domain is pausing another domain. Or I missed vC
the point.
Yes domain pause/unpause will not work. However, you can introduce a
boolean to tell you whether the domain was suspend.
I actually quite like how suspend work for x86 HVM. This is based on
pause/unpause. Have a look at hvm_s3_{suspend/resume}.
That only exists because the ACPI controller is/was implemented in
QEMU. I wouldn't recommend copying it.
May I ask why? I don't think the properties are very different from
Arm here.
If you observe, that can only be actioned by a hypercall from qemu. It
can't be actioned by an ACPI controller emulated by Xen.
How does the ACPI controller emulated by Xen deal with suspend/resume?
Do you have any pointer?
Having spent some more time thinking about this problem, what properties
does the PSCI call have?
I gather from other parts of this thread that there may be a partial
reset of state? Beyond that, what else?
I ask, because conceptually, domU suspend to RAM is literally just
"de-schedule until we want to wake up", and in this case, it appears to
be "wake up on any of the still-active interrupts". We've already got a
scheduler API for that, and its called vcpu_block(). This already
exists with "wait until a new event occurs" semantics.
Is there anything else I've missed?
All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
only events on that vCPU can trigger a resume. All the other event
should not be taken into account.
My worry with vcpu_block() is we don't prevent the other vCPUs to run.
Technically they should be off, but I would like some safety to avoid
any potential corner case (i.e other way to turn a vCPU on).
Why not have the SYSTEM_SUSPEND check that all other vCPUs are DOWN
first, and fail the call if not?
The code already check for that. My concern is there might (today or in
the future) other bits of Xen that can potentially turn on the vCPU (e.g
the toolstack).
If instead of waiting for any event, you need to wait for a specific
event, there is also vcpu_poll() which is a related scheduler API.
I guess we need to agree how what kind of event can resume the guest
from suspend/resume. I am not convinced that all events should be equal
here.
By vcpu_poll, do you mean do_poll?
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Andrew,
On 11/15/18 10:26 AM, Andrew Cooper wrote:
On 15/11/2018 10:13, Julien Grall wrote:
(+ Andre)
On 11/15/18 12:47 AM, Andrew Cooper wrote:
On 14/11/2018 12:49, Julien Grall wrote:
Hi Mirela,
On 14/11/2018 12:08, Mirela Simonovic wrote:
On 11/13/2018 09:32 AM, Andrew Cooper wrote:
On 12/11/2018 19:56, Julien Grall wrote:
Hi Andrew,
On 11/12/18 4:41 PM, Andrew Cooper wrote:
On 12/11/18 16:35, Mirela Simonovic wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e594b48d81..7f8105465c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+ /* VCPU's context should not be saved if its domain is
suspended */
+ if ( p->domain->is_shut_down &&
+ (p->domain->shutdown_code == SHUTDOWN_suspend) )
+ return;
SHUTDOWN_suspend is used in Xen for other purpose (see
SCHEDOP_shutdown). The other user of that code relies on all the
state
to be saved on suspend.
We just need a flag to mark a domain as suspended, and I do
believe
SHUTDOWN_suspend is not used anywhere else.
Let's come back on this.
SHUTDOWN_suspend is used for migration. Grep for it through the
Xen
tree and you'll find several pieces of documentation, including the
description of what this shutdown code means.
What you are introducing here is not a shutdown - it is a suspend
with
the intent to resume executing later. As such, it shouldn't use
Xen's
shutdown infrastructure, which exists mainly to communicate with
the
toolstack.
Would domain pause/unpause be a better solution?
Actually yes - that sounds like a very neat solution.
I believe domain pause will not work here - a domain cannot pause
itself, i.e. current domain cannot be paused. This functionality
seems to assume that a domain is pausing another domain. Or I missed vC
the point.
Yes domain pause/unpause will not work. However, you can introduce a
boolean to tell you whether the domain was suspend.
I actually quite like how suspend work for x86 HVM. This is based on
pause/unpause. Have a look at hvm_s3_{suspend/resume}.
That only exists because the ACPI controller is/was implemented in
QEMU. I wouldn't recommend copying it.
May I ask why? I don't think the properties are very different from
Arm here.
If you observe, that can only be actioned by a hypercall from qemu. It
can't be actioned by an ACPI controller emulated by Xen.
Having spent some more time thinking about this problem, what properties
does the PSCI call have?
I gather from other parts of this thread that there may be a partial
reset of state? Beyond that, what else?
I ask, because conceptually, domU suspend to RAM is literally just
"de-schedule until we want to wake up", and in this case, it appears to
be "wake up on any of the still-active interrupts". We've already got a
scheduler API for that, and its called vcpu_block(). This already
exists with "wait until a new event occurs" semantics.
Is there anything else I've missed?
All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
only events on that vCPU can trigger a resume. All the other event
should not be taken into account.
My worry with vcpu_block() is we don't prevent the other vCPUs to run.
Technically they should be off, but I would like some safety to avoid
any potential corner case (i.e other way to turn a vCPU on).
Why not have the SYSTEM_SUSPEND check that all other vCPUs are DOWN
first, and fail the call if not?
The code already check for that. My concern is there might (today or in
the future) other bits of Xen that can potentially turn on the vCPU (e.g
the toolstack).
If instead of waiting for any event, you need to wait for a specific
event, there is also vcpu_poll() which is a related scheduler API.
I guess we need to agree how what kind of event can resume the guest
from suspend/resume. I am not convinced that all events should be equal
here.
By vcpu_poll, do you mean do_poll?
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On Thu, Nov 15, 2018 at 11:26 AM Andrew Cooper
wrote:
>
> On 15/11/2018 10:13, Julien Grall wrote:
> > (+ Andre)
> >
> > On 11/15/18 12:47 AM, Andrew Cooper wrote:
> >> On 14/11/2018 12:49, Julien Grall wrote:
> >>> Hi Mirela,
> >>>
> >>> On 14/11/2018 12:08, Mirela Simonovic wrote:
>
>
> On 11/13/2018 09:32 AM, Andrew Cooper wrote:
> > On 12/11/2018 19:56, Julien Grall wrote:
> >> Hi Andrew,
> >>
> >> On 11/12/18 4:41 PM, Andrew Cooper wrote:
> >>> On 12/11/18 16:35, Mirela Simonovic wrote:
> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >> index e594b48d81..7f8105465c 100644
> >> --- a/xen/arch/arm/domain.c
> >> +++ b/xen/arch/arm/domain.c
> >> @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
> >> if ( is_idle_vcpu(p) )
> >> return;
> >>
> >> +/* VCPU's context should not be saved if its domain is
> >> suspended */
> >> +if ( p->domain->is_shut_down &&
> >> +(p->domain->shutdown_code == SHUTDOWN_suspend) )
> >> +return;
> > SHUTDOWN_suspend is used in Xen for other purpose (see
> > SCHEDOP_shutdown). The other user of that code relies on all the
> > state
> > to be saved on suspend.
> >
> We just need a flag to mark a domain as suspended, and I do
> believe
> SHUTDOWN_suspend is not used anywhere else.
> Let's come back on this.
> >>> SHUTDOWN_suspend is used for migration. Grep for it through the
> >>> Xen
> >>> tree and you'll find several pieces of documentation, including the
> >>> description of what this shutdown code means.
> >>>
> >>> What you are introducing here is not a shutdown - it is a suspend
> >>> with
> >>> the intent to resume executing later. As such, it shouldn't use
> >>> Xen's
> >>> shutdown infrastructure, which exists mainly to communicate with
> >>> the
> >>> toolstack.
> >> Would domain pause/unpause be a better solution?
> > Actually yes - that sounds like a very neat solution.
>
> I believe domain pause will not work here - a domain cannot pause
> itself, i.e. current domain cannot be paused. This functionality
> seems to assume that a domain is pausing another domain. Or I missed
> the point.
> >>>
> >>> Yes domain pause/unpause will not work. However, you can introduce a
> >>> boolean to tell you whether the domain was suspend.
> >>>
> >>> I actually quite like how suspend work for x86 HVM. This is based on
> >>> pause/unpause. Have a look at hvm_s3_{suspend/resume}.
> >>
> >> That only exists because the ACPI controller is/was implemented in
> >> QEMU. I wouldn't recommend copying it.
> >
> > May I ask why? I don't think the properties are very different from
> > Arm here.
>
> If you observe, that can only be actioned by a hypercall from qemu. It
> can't be actioned by an ACPI controller emulated by Xen.
>
> >
> >>
> >> Having spent some more time thinking about this problem, what properties
> >> does the PSCI call have?
> >>
> >> I gather from other parts of this thread that there may be a partial
> >> reset of state? Beyond that, what else?
> >>
> >> I ask, because conceptually, domU suspend to RAM is literally just
> >> "de-schedule until we want to wake up", and in this case, it appears to
> >> be "wake up on any of the still-active interrupts". We've already got a
> >> scheduler API for that, and its called vcpu_block(). This already
> >> exists with "wait until a new event occurs" semantics.
> >>
> >> Is there anything else I've missed?
> >
That's correct, and I agree. BTW that is what's implemented in this series.
> > All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
> > only events on that vCPU can trigger a resume. All the other event
> > should not be taken into account.
> >
What other events are talking about here?
> > My worry with vcpu_block() is we don't prevent the other vCPUs to run.
> > Technically they should be off, but I would like some safety to avoid
> > any potential corner case (i.e other way to turn a vCPU on).
>
Other vCPUs are hot-unplugged (offlined) by the guest. If that is not
the case, SYSTEM_SUSPEND will return an error.
Could you please clarify what a potential corner case would be?
> Why not have the SYSTEM_SUSPEND check that all other vCPUs are DOWN
> first, and fail the call if not?
>
This is also already done.
> If instead of waiting for any event, you need to wait for a specific
> event, there is also vcpu_poll() which is a related scheduler API.
>
> ~Andrew
Some content disappeared, so I'll write here to avoid thread branching.
The semantic of vCPU block and domain_pause is not the same. When
guest suspends the domain is not (and should not be) paused, instead
its last online vCPU is blocked waiting on an interrupt. That's it.
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 15/11/2018 10:13, Julien Grall wrote:
> (+ Andre)
>
> On 11/15/18 12:47 AM, Andrew Cooper wrote:
>> On 14/11/2018 12:49, Julien Grall wrote:
>>> Hi Mirela,
>>>
>>> On 14/11/2018 12:08, Mirela Simonovic wrote:
On 11/13/2018 09:32 AM, Andrew Cooper wrote:
> On 12/11/2018 19:56, Julien Grall wrote:
>> Hi Andrew,
>>
>> On 11/12/18 4:41 PM, Andrew Cooper wrote:
>>> On 12/11/18 16:35, Mirela Simonovic wrote:
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index e594b48d81..7f8105465c 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
>> if ( is_idle_vcpu(p) )
>> return;
>>
>> + /* VCPU's context should not be saved if its domain is
>> suspended */
>> + if ( p->domain->is_shut_down &&
>> + (p->domain->shutdown_code == SHUTDOWN_suspend) )
>> + return;
> SHUTDOWN_suspend is used in Xen for other purpose (see
> SCHEDOP_shutdown). The other user of that code relies on all the
> state
> to be saved on suspend.
>
We just need a flag to mark a domain as suspended, and I do
believe
SHUTDOWN_suspend is not used anywhere else.
Let's come back on this.
>>> SHUTDOWN_suspend is used for migration. Grep for it through the
>>> Xen
>>> tree and you'll find several pieces of documentation, including the
>>> description of what this shutdown code means.
>>>
>>> What you are introducing here is not a shutdown - it is a suspend
>>> with
>>> the intent to resume executing later. As such, it shouldn't use
>>> Xen's
>>> shutdown infrastructure, which exists mainly to communicate with
>>> the
>>> toolstack.
>> Would domain pause/unpause be a better solution?
> Actually yes - that sounds like a very neat solution.
I believe domain pause will not work here - a domain cannot pause
itself, i.e. current domain cannot be paused. This functionality
seems to assume that a domain is pausing another domain. Or I missed
the point.
>>>
>>> Yes domain pause/unpause will not work. However, you can introduce a
>>> boolean to tell you whether the domain was suspend.
>>>
>>> I actually quite like how suspend work for x86 HVM. This is based on
>>> pause/unpause. Have a look at hvm_s3_{suspend/resume}.
>>
>> That only exists because the ACPI controller is/was implemented in
>> QEMU. I wouldn't recommend copying it.
>
> May I ask why? I don't think the properties are very different from
> Arm here.
If you observe, that can only be actioned by a hypercall from qemu. It
can't be actioned by an ACPI controller emulated by Xen.
>
>>
>> Having spent some more time thinking about this problem, what properties
>> does the PSCI call have?
>>
>> I gather from other parts of this thread that there may be a partial
>> reset of state? Beyond that, what else?
>>
>> I ask, because conceptually, domU suspend to RAM is literally just
>> "de-schedule until we want to wake up", and in this case, it appears to
>> be "wake up on any of the still-active interrupts". We've already got a
>> scheduler API for that, and its called vcpu_block(). This already
>> exists with "wait until a new event occurs" semantics.
>>
>> Is there anything else I've missed?
>
> All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
> only events on that vCPU can trigger a resume. All the other event
> should not be taken into account.
>
> My worry with vcpu_block() is we don't prevent the other vCPUs to run.
> Technically they should be off, but I would like some safety to avoid
> any potential corner case (i.e other way to turn a vCPU on).
Why not have the SYSTEM_SUSPEND check that all other vCPUs are DOWN
first, and fail the call if not?
If instead of waiting for any event, you need to wait for a specific
event, there is also vcpu_poll() which is a related scheduler API.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
(+ Andre)
On 11/15/18 12:47 AM, Andrew Cooper wrote:
On 14/11/2018 12:49, Julien Grall wrote:
Hi Mirela,
On 14/11/2018 12:08, Mirela Simonovic wrote:
On 11/13/2018 09:32 AM, Andrew Cooper wrote:
On 12/11/2018 19:56, Julien Grall wrote:
Hi Andrew,
On 11/12/18 4:41 PM, Andrew Cooper wrote:
On 12/11/18 16:35, Mirela Simonovic wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e594b48d81..7f8105465c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+ /* VCPU's context should not be saved if its domain is
suspended */
+ if ( p->domain->is_shut_down &&
+ (p->domain->shutdown_code == SHUTDOWN_suspend) )
+ return;
SHUTDOWN_suspend is used in Xen for other purpose (see
SCHEDOP_shutdown). The other user of that code relies on all the
state
to be saved on suspend.
We just need a flag to mark a domain as suspended, and I do believe
SHUTDOWN_suspend is not used anywhere else.
Let's come back on this.
SHUTDOWN_suspend is used for migration. Grep for it through the Xen
tree and you'll find several pieces of documentation, including the
description of what this shutdown code means.
What you are introducing here is not a shutdown - it is a suspend
with
the intent to resume executing later. As such, it shouldn't use
Xen's
shutdown infrastructure, which exists mainly to communicate with the
toolstack.
Would domain pause/unpause be a better solution?
Actually yes - that sounds like a very neat solution.
I believe domain pause will not work here - a domain cannot pause
itself, i.e. current domain cannot be paused. This functionality
seems to assume that a domain is pausing another domain. Or I missed
the point.
Yes domain pause/unpause will not work. However, you can introduce a
boolean to tell you whether the domain was suspend.
I actually quite like how suspend work for x86 HVM. This is based on
pause/unpause. Have a look at hvm_s3_{suspend/resume}.
That only exists because the ACPI controller is/was implemented in
QEMU. I wouldn't recommend copying it.
May I ask why? I don't think the properties are very different from Arm
here.
Having spent some more time thinking about this problem, what properties
does the PSCI call have?
I gather from other parts of this thread that there may be a partial
reset of state? Beyond that, what else?
I ask, because conceptually, domU suspend to RAM is literally just
"de-schedule until we want to wake up", and in this case, it appears to
be "wake up on any of the still-active interrupts". We've already got a
scheduler API for that, and its called vcpu_block(). This already
exists with "wait until a new event occurs" semantics.
Is there anything else I've missed?
All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also, only
events on that vCPU can trigger a resume. All the other event should not
be taken into account.
My worry with vcpu_block() is we don't prevent the other vCPUs to run.
Technically they should be off, but I would like some safety to avoid
any potential corner case (i.e other way to turn a vCPU on).
That's why I think domain_pause() is more suitable in that case. We
could unpause the domain either when receiving an interrupt or when
requested by the toolstack).
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 14/11/2018 12:49, Julien Grall wrote:
> Hi Mirela,
>
> On 14/11/2018 12:08, Mirela Simonovic wrote:
>>
>>
>> On 11/13/2018 09:32 AM, Andrew Cooper wrote:
>>> On 12/11/2018 19:56, Julien Grall wrote:
Hi Andrew,
On 11/12/18 4:41 PM, Andrew Cooper wrote:
> On 12/11/18 16:35, Mirela Simonovic wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e594b48d81..7f8105465c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+ /* VCPU's context should not be saved if its domain is
suspended */
+ if ( p->domain->is_shut_down &&
+ (p->domain->shutdown_code == SHUTDOWN_suspend) )
+ return;
>>> SHUTDOWN_suspend is used in Xen for other purpose (see
>>> SCHEDOP_shutdown). The other user of that code relies on all the
>>> state
>>> to be saved on suspend.
>>>
>> We just need a flag to mark a domain as suspended, and I do believe
>> SHUTDOWN_suspend is not used anywhere else.
>> Let's come back on this.
> SHUTDOWN_suspend is used for migration. Grep for it through the Xen
> tree and you'll find several pieces of documentation, including the
> description of what this shutdown code means.
>
> What you are introducing here is not a shutdown - it is a suspend
> with
> the intent to resume executing later. As such, it shouldn't use
> Xen's
> shutdown infrastructure, which exists mainly to communicate with the
> toolstack.
Would domain pause/unpause be a better solution?
>>> Actually yes - that sounds like a very neat solution.
>>
>> I believe domain pause will not work here - a domain cannot pause
>> itself, i.e. current domain cannot be paused. This functionality
>> seems to assume that a domain is pausing another domain. Or I missed
>> the point.
>
> Yes domain pause/unpause will not work. However, you can introduce a
> boolean to tell you whether the domain was suspend.
>
> I actually quite like how suspend work for x86 HVM. This is based on
> pause/unpause. Have a look at hvm_s3_{suspend/resume}.
That only exists because the ACPI controller is/was implemented in
QEMU. I wouldn't recommend copying it.
Having spent some more time thinking about this problem, what properties
does the PSCI call have?
I gather from other parts of this thread that there may be a partial
reset of state? Beyond that, what else?
I ask, because conceptually, domU suspend to RAM is literally just
"de-schedule until we want to wake up", and in this case, it appears to
be "wake up on any of the still-active interrupts". We've already got a
scheduler API for that, and its called vcpu_block(). This already
exists with "wait until a new event occurs" semantics.
Is there anything else I've missed?
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On Wed, 14 Nov 2018, Julien Grall wrote: > On 13/11/2018 20:39, Stefano Stabellini wrote: > > On Tue, 13 Nov 2018, Julien Grall wrote: > > > > If we mark the domU simply as "paused" it will be difficult to implement > > correctly "xl restore" / "xl trigger s3resume". We should be able to > > distinguish a domain which is simply not running or paused (as in "xl > > pause") from one that has been put to sleep. Otherwise we won't be able > > to use "xl pause" in its original sense anymore. "xl pause" will become > > effectively "xl trigger sleep" on ARM. Which is something to consider > > but I don't think that is what we want. > > I didn't suggested to only use those 2 helpers. But you can build > suspend/resume on top of it. AFAICT, this is how s3 suspend/resume works on > x86 HVM. Sounds like we are saying the same thing. > > The most similar feature is ACPI S3 in x86-land but the current calls > > are so ACPI/x86 specific that don't make much sense on a ACPI-less ARM > > implementation. If I am not mistaken, on x86 there is a special struct > > domain flag for this: d->arch.hvm.is_s3_suspended. > > > > > > Let's leave aside the "which commands should we use" discussion for now > > because it doesn't related to this patch series. It seems to me that the > > best option is to introduce a new ARM specific struct domain flag, > > something akin to d->arch.hvm.is_s3_suspended but ARM PSCI specific. > > See above. > > -- > Julien Grall > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Mirela,
On 14/11/2018 15:36, Mirela Simonovic wrote:
On Wed, Nov 14, 2018 at 3:49 PM Julien Grall wrote:
On 14/11/2018 13:05, Julien Grall wrote:
On 14/11/2018 12:35, Mirela Simonovic wrote:
On 11/14/2018 11:45 AM, Julien Grall wrote:
Hi,
On 13/11/2018 20:39, Stefano Stabellini wrote:
On Mon, 12 Nov 2018, Julien Grall wrote:
However, what is the issue with saving all the registers here?
We need to save arguments that are provided by a guest with system
suspend PSCI call. These arguments are the entry point that needs to
be saved in program counter and context ID that needs to be saved in
x0/r0. We don't have these arguments here. Context switch happens
after processing the system suspend PSCI call, so it's too late.
It does not feel right to modify ctxt_switch{from,to} for suspend/resume. If
you want to reset the vCPU state before blocking the vCPU, then you should
instead
I think it's not clear what problem are we discussing here, at least it's not
to me. So I'll make an assumption, and please correct me if I'm wrong.
In the patches we submitted, the VCPU context is not reset in
ctxt_switch{from,to}. My understanding is that you suggested/asked to reset
the VCPU context when switch happens, and I explained why is that not possible
- at least not without additional code changes, that may not be so small. I
agree with Andrew's comment in this perspective - reset of VCPU should not
(and right now cannot) be done when the context is switched.
I didn't ask to reset the vCPU context in the switch. Instead we should make
sure the vCPU context is synced to memory before modifying it.
It seems that solution works on x86 using domain_pause (see
hvm_s3_{resume,suspend}). So I am not sure why it cannot be use on Arm. Note
that it may require more work.
You missed the end of the suggestion here
Whoops. I meant that instead you should save the context of the vCPU in
advance or reset the vCPU using the system registers directly.
But my preference is to reset the vCPU when you receive the wake-up interrupt.
Without you presenting more details how would that work I cannot really
provide any comment, nor say that your preference could work or be better
compared to what is in this series. Honestly, I don't understand what exactly
you're proposing, because more things needs to be think-through beyond the
place to put a code.
We submitted a code that works, which is very elegant and nice in my opinion
(fair to say we may not share opinions here), and does not require lots of
code changes. So there's the reference.
Could you please clarify why do you think the proposed solution is not good?
The context switch is about saving/restore the context from the hardware. We can
decide to optimize it in the suspend case (though it might be premature), but it
is clearly the wrong place to decide to resume a domain.
Actually, I just found a good example of why I think it is wrong and broken. You
rely on a context switch to always happen after suspending and on resuming the
guest. There are path where context/switch will not happen. An example is if you
have interrupt pending, you may return to the guest directly if the scheduler
does not have anything else to schedule.
Can we check whether the vcpu blocked, right? Let me be more specific
- after calling vcpu_block_unless_event_pending we can check whether
the vcpu is indeed blocked?
This would be racy. The vCPU might be blocked when you check it. However, it can
get unblocked before you reached the schedule softirq (where schedule is done).
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On Wed, Nov 14, 2018 at 4:36 PM Mirela Simonovic
wrote:
>
> Hi Julien,
>
> On Wed, Nov 14, 2018 at 3:49 PM Julien Grall wrote:
> >
> >
> >
> > On 14/11/2018 13:05, Julien Grall wrote:
> > >
> > >
> > > On 14/11/2018 12:35, Mirela Simonovic wrote:
> > >> Hi Julien,
> > >
> > > Hi,
> > >
> > >>
> > >> On 11/14/2018 11:45 AM, Julien Grall wrote:
> > >>> Hi,
> > >>>
> > >>> On 13/11/2018 20:39, Stefano Stabellini wrote:
> > On Mon, 12 Nov 2018, Julien Grall wrote:
> > >>> However, what is the issue with saving all the registers here?
> > >>>
> > >>
> > >> We need to save arguments that are provided by a guest with system
> > >> suspend PSCI call. These arguments are the entry point that needs to
> > >> be saved in program counter and context ID that needs to be saved in
> > >> x0/r0. We don't have these arguments here. Context switch happens
> > >> after processing the system suspend PSCI call, so it's too late.
> > >
> > > It does not feel right to modify ctxt_switch{from,to} for
> > > suspend/resume. If
> > > you want to reset the vCPU state before blocking the vCPU, then you
> > > should
> > > instead
> >
> > >>
> > >> I think it's not clear what problem are we discussing here, at least
> > >> it's not
> > >> to me. So I'll make an assumption, and please correct me if I'm wrong.
> > >> In the patches we submitted, the VCPU context is not reset in
> > >> ctxt_switch{from,to}. My understanding is that you suggested/asked to
> > >> reset
> > >> the VCPU context when switch happens, and I explained why is that not
> > >> possible
> > >> - at least not without additional code changes, that may not be so
> > >> small. I
> > >> agree with Andrew's comment in this perspective - reset of VCPU should
> > >> not
> > >> (and right now cannot) be done when the context is switched.
> > >
> > > I didn't ask to reset the vCPU context in the switch. Instead we should
> > > make
> > > sure the vCPU context is synced to memory before modifying it.
> > >
> > > It seems that solution works on x86 using domain_pause (see
> > > hvm_s3_{resume,suspend}). So I am not sure why it cannot be use on Arm.
> > > Note
> > > that it may require more work.
> > >
> > >>
> > You missed the end of the suggestion here
> > >>>
> > >>> Whoops. I meant that instead you should save the context of the vCPU in
> > >>> advance or reset the vCPU using the system registers directly.
> > >>>
> > >>> But my preference is to reset the vCPU when you receive the wake-up
> > >>> interrupt.
> > >>>
> > >>
> > >> Without you presenting more details how would that work I cannot really
> > >> provide any comment, nor say that your preference could work or be better
> > >> compared to what is in this series. Honestly, I don't understand what
> > >> exactly
> > >> you're proposing, because more things needs to be think-through beyond
> > >> the
> > >> place to put a code.
> > >> We submitted a code that works, which is very elegant and nice in my
> > >> opinion
> > >> (fair to say we may not share opinions here), and does not require lots
> > >> of
> > >> code changes. So there's the reference.
> > >> Could you please clarify why do you think the proposed solution is not
> > >> good?
> > >
> > > The context switch is about saving/restore the context from the hardware.
> > > We can
> > > decide to optimize it in the suspend case (though it might be premature),
> > > but it
> > > is clearly the wrong place to decide to resume a domain.
> >
> > Actually, I just found a good example of why I think it is wrong and
> > broken. You
> > rely on a context switch to always happen after suspending and on resuming
> > the
> > guest. There are path where context/switch will not happen. An example is
> > if you
> > have interrupt pending, you may return to the guest directly if the
> > scheduler
> > does not have anything else to schedule.
> >
>
> Can we check whether the vcpu blocked, right? Let me be more specific - after
> calling vcpu_block_unless_event_pending we can check whether the vcpu is
> indeed blocked?
>
> > The problem is the variable is_shut_down and shutdown_code are only be
> > reset on
> > restoring the vCPU. This means the next context switch will skip the saving
> > part
> > and result to vCPU corruption.
> >
> > In general, you cannot rely on the context/switch code as it may or may not
> > happen (that's up to the scheduler).
> >
>
I wanted to write also that this is a very good example, thank you for
putting it together
>
>
> > Cheers,
> >
> > --
> > Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Julien,
On Wed, Nov 14, 2018 at 3:49 PM Julien Grall wrote:
>
>
>
> On 14/11/2018 13:05, Julien Grall wrote:
> >
> >
> > On 14/11/2018 12:35, Mirela Simonovic wrote:
> >> Hi Julien,
> >
> > Hi,
> >
> >>
> >> On 11/14/2018 11:45 AM, Julien Grall wrote:
> >>> Hi,
> >>>
> >>> On 13/11/2018 20:39, Stefano Stabellini wrote:
> On Mon, 12 Nov 2018, Julien Grall wrote:
> >>> However, what is the issue with saving all the registers here?
> >>>
> >>
> >> We need to save arguments that are provided by a guest with system
> >> suspend PSCI call. These arguments are the entry point that needs to
> >> be saved in program counter and context ID that needs to be saved in
> >> x0/r0. We don't have these arguments here. Context switch happens
> >> after processing the system suspend PSCI call, so it's too late.
> >
> > It does not feel right to modify ctxt_switch{from,to} for
> > suspend/resume. If
> > you want to reset the vCPU state before blocking the vCPU, then you
> > should
> > instead
>
> >>
> >> I think it's not clear what problem are we discussing here, at least it's
> >> not
> >> to me. So I'll make an assumption, and please correct me if I'm wrong.
> >> In the patches we submitted, the VCPU context is not reset in
> >> ctxt_switch{from,to}. My understanding is that you suggested/asked to reset
> >> the VCPU context when switch happens, and I explained why is that not
> >> possible
> >> - at least not without additional code changes, that may not be so small. I
> >> agree with Andrew's comment in this perspective - reset of VCPU should not
> >> (and right now cannot) be done when the context is switched.
> >
> > I didn't ask to reset the vCPU context in the switch. Instead we should make
> > sure the vCPU context is synced to memory before modifying it.
> >
> > It seems that solution works on x86 using domain_pause (see
> > hvm_s3_{resume,suspend}). So I am not sure why it cannot be use on Arm. Note
> > that it may require more work.
> >
> >>
> You missed the end of the suggestion here
> >>>
> >>> Whoops. I meant that instead you should save the context of the vCPU in
> >>> advance or reset the vCPU using the system registers directly.
> >>>
> >>> But my preference is to reset the vCPU when you receive the wake-up
> >>> interrupt.
> >>>
> >>
> >> Without you presenting more details how would that work I cannot really
> >> provide any comment, nor say that your preference could work or be better
> >> compared to what is in this series. Honestly, I don't understand what
> >> exactly
> >> you're proposing, because more things needs to be think-through beyond the
> >> place to put a code.
> >> We submitted a code that works, which is very elegant and nice in my
> >> opinion
> >> (fair to say we may not share opinions here), and does not require lots of
> >> code changes. So there's the reference.
> >> Could you please clarify why do you think the proposed solution is not
> >> good?
> >
> > The context switch is about saving/restore the context from the hardware.
> > We can
> > decide to optimize it in the suspend case (though it might be premature),
> > but it
> > is clearly the wrong place to decide to resume a domain.
>
> Actually, I just found a good example of why I think it is wrong and broken.
> You
> rely on a context switch to always happen after suspending and on resuming the
> guest. There are path where context/switch will not happen. An example is if
> you
> have interrupt pending, you may return to the guest directly if the scheduler
> does not have anything else to schedule.
>
Can we check whether the vcpu blocked, right? Let me be more specific
- after calling vcpu_block_unless_event_pending we can check whether
the vcpu is indeed blocked?
> The problem is the variable is_shut_down and shutdown_code are only be reset
> on
> restoring the vCPU. This means the next context switch will skip the saving
> part
> and result to vCPU corruption.
>
> In general, you cannot rely on the context/switch code as it may or may not
> happen (that's up to the scheduler).
>
> Cheers,
>
> --
> Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 14/11/2018 13:05, Julien Grall wrote:
On 14/11/2018 12:35, Mirela Simonovic wrote:
Hi Julien,
Hi,
On 11/14/2018 11:45 AM, Julien Grall wrote:
Hi,
On 13/11/2018 20:39, Stefano Stabellini wrote:
On Mon, 12 Nov 2018, Julien Grall wrote:
However, what is the issue with saving all the registers here?
We need to save arguments that are provided by a guest with system
suspend PSCI call. These arguments are the entry point that needs to
be saved in program counter and context ID that needs to be saved in
x0/r0. We don't have these arguments here. Context switch happens
after processing the system suspend PSCI call, so it's too late.
It does not feel right to modify ctxt_switch{from,to} for suspend/resume. If
you want to reset the vCPU state before blocking the vCPU, then you should
instead
I think it's not clear what problem are we discussing here, at least it's not
to me. So I'll make an assumption, and please correct me if I'm wrong.
In the patches we submitted, the VCPU context is not reset in
ctxt_switch{from,to}. My understanding is that you suggested/asked to reset
the VCPU context when switch happens, and I explained why is that not possible
- at least not without additional code changes, that may not be so small. I
agree with Andrew's comment in this perspective - reset of VCPU should not
(and right now cannot) be done when the context is switched.
I didn't ask to reset the vCPU context in the switch. Instead we should make
sure the vCPU context is synced to memory before modifying it.
It seems that solution works on x86 using domain_pause (see
hvm_s3_{resume,suspend}). So I am not sure why it cannot be use on Arm. Note
that it may require more work.
You missed the end of the suggestion here
Whoops. I meant that instead you should save the context of the vCPU in
advance or reset the vCPU using the system registers directly.
But my preference is to reset the vCPU when you receive the wake-up interrupt.
Without you presenting more details how would that work I cannot really
provide any comment, nor say that your preference could work or be better
compared to what is in this series. Honestly, I don't understand what exactly
you're proposing, because more things needs to be think-through beyond the
place to put a code.
We submitted a code that works, which is very elegant and nice in my opinion
(fair to say we may not share opinions here), and does not require lots of
code changes. So there's the reference.
Could you please clarify why do you think the proposed solution is not good?
The context switch is about saving/restore the context from the hardware. We can
decide to optimize it in the suspend case (though it might be premature), but it
is clearly the wrong place to decide to resume a domain.
Actually, I just found a good example of why I think it is wrong and broken. You
rely on a context switch to always happen after suspending and on resuming the
guest. There are path where context/switch will not happen. An example is if you
have interrupt pending, you may return to the guest directly if the scheduler
does not have anything else to schedule.
The problem is the variable is_shut_down and shutdown_code are only be reset on
restoring the vCPU. This means the next context switch will skip the saving part
and result to vCPU corruption.
In general, you cannot rely on the context/switch code as it may or may not
happen (that's up to the scheduler).
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 13/11/2018 20:39, Stefano Stabellini wrote: On Tue, 13 Nov 2018, Julien Grall wrote: If we mark the domU simply as "paused" it will be difficult to implement correctly "xl restore" / "xl trigger s3resume". We should be able to distinguish a domain which is simply not running or paused (as in "xl pause") from one that has been put to sleep. Otherwise we won't be able to use "xl pause" in its original sense anymore. "xl pause" will become effectively "xl trigger sleep" on ARM. Which is something to consider but I don't think that is what we want. I didn't suggested to only use those 2 helpers. But you can build suspend/resume on top of it. AFAICT, this is how s3 suspend/resume works on x86 HVM. The most similar feature is ACPI S3 in x86-land but the current calls are so ACPI/x86 specific that don't make much sense on a ACPI-less ARM implementation. If I am not mistaken, on x86 there is a special struct domain flag for this: d->arch.hvm.is_s3_suspended. Let's leave aside the "which commands should we use" discussion for now because it doesn't related to this patch series. It seems to me that the best option is to introduce a new ARM specific struct domain flag, something akin to d->arch.hvm.is_s3_suspended but ARM PSCI specific. See above. -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 14/11/2018 12:35, Mirela Simonovic wrote:
Hi Julien,
Hi,
On 11/14/2018 11:45 AM, Julien Grall wrote:
Hi,
On 13/11/2018 20:39, Stefano Stabellini wrote:
On Mon, 12 Nov 2018, Julien Grall wrote:
However, what is the issue with saving all the registers here?
We need to save arguments that are provided by a guest with system
suspend PSCI call. These arguments are the entry point that needs to
be saved in program counter and context ID that needs to be saved in
x0/r0. We don't have these arguments here. Context switch happens
after processing the system suspend PSCI call, so it's too late.
It does not feel right to modify ctxt_switch{from,to} for suspend/resume. If
you want to reset the vCPU state before blocking the vCPU, then you should
instead
I think it's not clear what problem are we discussing here, at least it's not to
me. So I'll make an assumption, and please correct me if I'm wrong.
In the patches we submitted, the VCPU context is not reset in
ctxt_switch{from,to}. My understanding is that you suggested/asked to reset the
VCPU context when switch happens, and I explained why is that not possible - at
least not without additional code changes, that may not be so small. I agree
with Andrew's comment in this perspective - reset of VCPU should not (and right
now cannot) be done when the context is switched.
I didn't ask to reset the vCPU context in the switch. Instead we should make
sure the vCPU context is synced to memory before modifying it.
It seems that solution works on x86 using domain_pause (see
hvm_s3_{resume,suspend}). So I am not sure why it cannot be use on Arm. Note
that it may require more work.
You missed the end of the suggestion here
Whoops. I meant that instead you should save the context of the vCPU in
advance or reset the vCPU using the system registers directly.
But my preference is to reset the vCPU when you receive the wake-up interrupt.
Without you presenting more details how would that work I cannot really provide
any comment, nor say that your preference could work or be better compared to
what is in this series. Honestly, I don't understand what exactly you're
proposing, because more things needs to be think-through beyond the place to put
a code.
We submitted a code that works, which is very elegant and nice in my opinion
(fair to say we may not share opinions here), and does not require lots of code
changes. So there's the reference.
Could you please clarify why do you think the proposed solution is not good?
The context switch is about saving/restore the context from the hardware. We can
decide to optimize it in the suspend case (though it might be premature), but it
is clearly the wrong place to decide to resume a domain.
If saving the context happens to late, then we should look at making sure it
will happen earlier on (see my comment above).
And why do you think that what you're proposing is better? Lets be more clear
here - how exactly you propose to implement that?
The same way as hvm_s3_{suspend/resume} works on x86.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Mirela,
On 14/11/2018 12:08, Mirela Simonovic wrote:
On 11/13/2018 09:32 AM, Andrew Cooper wrote:
On 12/11/2018 19:56, Julien Grall wrote:
Hi Andrew,
On 11/12/18 4:41 PM, Andrew Cooper wrote:
On 12/11/18 16:35, Mirela Simonovic wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e594b48d81..7f8105465c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+ /* VCPU's context should not be saved if its domain is
suspended */
+ if ( p->domain->is_shut_down &&
+ (p->domain->shutdown_code == SHUTDOWN_suspend) )
+ return;
SHUTDOWN_suspend is used in Xen for other purpose (see
SCHEDOP_shutdown). The other user of that code relies on all the state
to be saved on suspend.
We just need a flag to mark a domain as suspended, and I do believe
SHUTDOWN_suspend is not used anywhere else.
Let's come back on this.
SHUTDOWN_suspend is used for migration. Grep for it through the Xen
tree and you'll find several pieces of documentation, including the
description of what this shutdown code means.
What you are introducing here is not a shutdown - it is a suspend with
the intent to resume executing later. As such, it shouldn't use Xen's
shutdown infrastructure, which exists mainly to communicate with the
toolstack.
Would domain pause/unpause be a better solution?
Actually yes - that sounds like a very neat solution.
I believe domain pause will not work here - a domain cannot pause itself, i.e.
current domain cannot be paused. This functionality seems to assume that a
domain is pausing another domain. Or I missed the point.
Yes domain pause/unpause will not work. However, you can introduce a boolean to
tell you whether the domain was suspend.
I actually quite like how suspend work for x86 HVM. This is based on
pause/unpause. Have a look at hvm_s3_{suspend/resume}.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 11/13/2018 04:09 PM, Julien Grall wrote:
On 13/11/2018 10:23, Julien Grall wrote:
Hi,
On 12/11/2018 11:30, Mirela Simonovic wrote:
+/*
+ * This function sets the context of current VCPU to the state
which is expected
+ * by the guest on resume. The expected VCPU state is:
+ * 1) pc to contain resume entry point (1st argument of PSCI
SYSTEM_SUSPEND)
+ * 2) r0/x0 to contain context ID (2nd argument of PSCI
SYSTEM_SUSPEND)
+ * 3) All other general purpose and system registers should have
reset values
+ *
+ * Note: this function has to return void because it has to always
succeed. In
+ * other words, this function is called from virtual PSCI
SYSTEM_SUSPEND
+ * implementation, which can return only a limited number of
possible errors,
+ * none of which could represent the fact that an error occurred
when preparing
+ * the domain for suspend.
+ * Consequently, dynamic memory allocation cannot be done within
this function,
+ * because if malloc fails the error has nowhere to propagate.
+ */
+static void vcpu_suspend(register_t epoint, register_t cid)
+{
+/* Static allocation because dynamic would need a non-void
return */
+static struct vcpu_guest_context ctxt;
+struct vcpu *v = current;
+
+/* Make sure that VCPU guest regs are zeroied */
+memset(, 0, sizeof(ctxt));
+
+/* Set non-zero values to the registers prior to copying */
+ctxt.user_regs.pc64 = (u64)epoint;
+
+if ( is_32bit_domain(current->domain) )
+{
+ctxt.user_regs.r0_usr = cid;
+ctxt.user_regs.cpsr = PSR_GUEST32_INIT;
This is going to disable the MMU and Cache as requested by the PSCI
spec. As the guest is not required to clean the cache when turning
off the CPU/suspending, the data may not have reached the main memory.
So do you need to perform cache maintenance to avoid stale information?
Answering to myself, I have discussed about the cache with others Arm
folks today. SYSTEM_SUSPEND may exit early due to a pending event (BTW
you don't seem to handle it) and could jump to the specified entry
point address.
I believe it is handled - with the vcpu_block_unless_event_pending in
domain_suspend
In that case, the only guarantee is the data would not be lost, yet
can still be in the cache. This means that any data used before the
MMU & cache are enabled in the entry point should have been cleaned to
PoC and if you modify data make sure the cache is invalidated.
Linux is doing that properly. Looking at Xen, we don't really properly
handle the cache in the boot path. So you may randomly crash.
Cheers,
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Julien,
On 11/14/2018 11:45 AM, Julien Grall wrote:
Hi,
On 13/11/2018 20:39, Stefano Stabellini wrote:
On Mon, 12 Nov 2018, Julien Grall wrote:
However, what is the issue with saving all the registers here?
We need to save arguments that are provided by a guest with system
suspend PSCI call. These arguments are the entry point that needs to
be saved in program counter and context ID that needs to be saved in
x0/r0. We don't have these arguments here. Context switch happens
after processing the system suspend PSCI call, so it's too late.
It does not feel right to modify ctxt_switch{from,to} for
suspend/resume. If
you want to reset the vCPU state before blocking the vCPU, then you
should
instead
I think it's not clear what problem are we discussing here, at least
it's not to me. So I'll make an assumption, and please correct me if I'm
wrong.
In the patches we submitted, the VCPU context is not reset in
ctxt_switch{from,to}. My understanding is that you suggested/asked to
reset the VCPU context when switch happens, and I explained why is that
not possible - at least not without additional code changes, that may
not be so small. I agree with Andrew's comment in this perspective -
reset of VCPU should not (and right now cannot) be done when the context
is switched.
You missed the end of the suggestion here
Whoops. I meant that instead you should save the context of the vCPU
in advance or reset the vCPU using the system registers directly.
But my preference is to reset the vCPU when you receive the wake-up
interrupt.
Without you presenting more details how would that work I cannot really
provide any comment, nor say that your preference could work or be
better compared to what is in this series. Honestly, I don't understand
what exactly you're proposing, because more things needs to be
think-through beyond the place to put a code.
We submitted a code that works, which is very elegant and nice in my
opinion (fair to say we may not share opinions here), and does not
require lots of code changes. So there's the reference.
Could you please clarify why do you think the proposed solution is not good?
And why do you think that what you're proposing is better? Lets be more
clear here - how exactly you propose to implement that?
I haven't understood so far why do you think that the proposed approach
is not good. Maybe the whole discussion drifted a bit for no reason.
Thanks,
Mirela
Cheers,
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 11/13/2018 09:32 AM, Andrew Cooper wrote: On 12/11/2018 19:56, Julien Grall wrote: Hi Andrew, On 11/12/18 4:41 PM, Andrew Cooper wrote: On 12/11/18 16:35, Mirela Simonovic wrote: diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index e594b48d81..7f8105465c 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p) if ( is_idle_vcpu(p) ) return; +/* VCPU's context should not be saved if its domain is suspended */ +if ( p->domain->is_shut_down && +(p->domain->shutdown_code == SHUTDOWN_suspend) ) +return; SHUTDOWN_suspend is used in Xen for other purpose (see SCHEDOP_shutdown). The other user of that code relies on all the state to be saved on suspend. We just need a flag to mark a domain as suspended, and I do believe SHUTDOWN_suspend is not used anywhere else. Let's come back on this. SHUTDOWN_suspend is used for migration. Grep for it through the Xen tree and you'll find several pieces of documentation, including the description of what this shutdown code means. What you are introducing here is not a shutdown - it is a suspend with the intent to resume executing later. As such, it shouldn't use Xen's shutdown infrastructure, which exists mainly to communicate with the toolstack. Would domain pause/unpause be a better solution? Actually yes - that sounds like a very neat solution. I believe domain pause will not work here - a domain cannot pause itself, i.e. current domain cannot be paused. This functionality seems to assume that a domain is pausing another domain. Or I missed the point. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi,
On 13/11/2018 20:39, Stefano Stabellini wrote:
On Mon, 12 Nov 2018, Julien Grall wrote:
However, what is the issue with saving all the registers here?
We need to save arguments that are provided by a guest with system
suspend PSCI call. These arguments are the entry point that needs to
be saved in program counter and context ID that needs to be saved in
x0/r0. We don't have these arguments here. Context switch happens
after processing the system suspend PSCI call, so it's too late.
It does not feel right to modify ctxt_switch{from,to} for suspend/resume. If
you want to reset the vCPU state before blocking the vCPU, then you should
instead
You missed the end of the suggestion here
Whoops. I meant that instead you should save the context of the vCPU in advance
or reset the vCPU using the system registers directly.
But my preference is to reset the vCPU when you receive the wake-up interrupt.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On Tue, 13 Nov 2018, Julien Grall wrote: > > > > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > > > > > > index e594b48d81..7f8105465c 100644 > > > > > > --- a/xen/arch/arm/domain.c > > > > > > +++ b/xen/arch/arm/domain.c > > > > > > @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p) > > > > > >if ( is_idle_vcpu(p) ) > > > > > >return; > > > > > > > > > > > > +/* VCPU's context should not be saved if its domain is > > > > > > suspended */ > > > > > > +if ( p->domain->is_shut_down && > > > > > > +(p->domain->shutdown_code == SHUTDOWN_suspend) ) > > > > > > +return; > > > > > SHUTDOWN_suspend is used in Xen for other purpose (see > > > > > SCHEDOP_shutdown). The other user of that code relies on all the state > > > > > to be saved on suspend. > > > > > > > > > We just need a flag to mark a domain as suspended, and I do believe > > > > SHUTDOWN_suspend is not used anywhere else. > > > > Let's come back on this. > > > > > > SHUTDOWN_suspend is used for migration. > > > > That is true, but it is not only used for migration. It is also used > > for suspending a guest and saving its state to file with the intention > > of resuming it later from file. > > Which is some sort of migration at the end. However, they don't have the same > semantics as suspend/resume regarding the state of the vCPU. Right > > > Grep for it through the Xen > > > tree and you'll find several pieces of documentation, including the > > > description of what this shutdown code means. > > > > > > What you are introducing here is not a shutdown - it is a suspend with > > > the intent to resume executing later. As such, it shouldn't use Xen's > > > shutdown infrastructure, which exists mainly to communicate with the > > > toolstack. > > > > Future work will need toolstack support for suspending/resuming guests. > > SHUTDOWN_suspend is the most natural fit today; we don't want to hijack > > domain pause, because if we do, then we can't normally pause a domain > > anymore. > > Why? suspend/resume is like pausing the domain but will be resumed on event > (e.g interrupts) rather than user request. I meant to say two things. 1) Which domain state should we use for suspended guests? This patch is using SHUTDOWN_suspend. Taken on its own, a regular "pause state" sounds like an option, in fact it is necessary to set the domain as paused otherwise the scheduler will schedule it. But in addition we need to distinguish a normal paused state from a PSCI system suspend state. We need to know that the domain has been system-suspended with PSCI, not just paused. 2) This ties into the discussion of what xl commands we want to use to request a domU to suspend. We don't need to talk about it now, but at some point we'll want something equivalent to "xl save" or "xl trigger sleep" and "xl restore" or "xl trigger s3resume" for this suspended state. If we mark the domU simply as "paused" it will be difficult to implement correctly "xl restore" / "xl trigger s3resume". We should be able to distinguish a domain which is simply not running or paused (as in "xl pause") from one that has been put to sleep. Otherwise we won't be able to use "xl pause" in its original sense anymore. "xl pause" will become effectively "xl trigger sleep" on ARM. Which is something to consider but I don't think that is what we want. The most similar feature is ACPI S3 in x86-land but the current calls are so ACPI/x86 specific that don't make much sense on a ACPI-less ARM implementation. If I am not mistaken, on x86 there is a special struct domain flag for this: d->arch.hvm.is_s3_suspended. Let's leave aside the "which commands should we use" discussion for now because it doesn't related to this patch series. It seems to me that the best option is to introduce a new ARM specific struct domain flag, something akin to d->arch.hvm.is_s3_suspended but ARM PSCI specific.___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On Mon, 12 Nov 2018, Julien Grall wrote:
> > > However, what is the issue with saving all the registers here?
> > >
> >
> > We need to save arguments that are provided by a guest with system
> > suspend PSCI call. These arguments are the entry point that needs to
> > be saved in program counter and context ID that needs to be saved in
> > x0/r0. We don't have these arguments here. Context switch happens
> > after processing the system suspend PSCI call, so it's too late.
>
> It does not feel right to modify ctxt_switch{from,to} for suspend/resume. If
> you want to reset the vCPU state before blocking the vCPU, then you should
> instead
You missed the end of the suggestion here
> Another way would be to reset the vCPU once you receive the interrupt.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 13/11/2018 10:23, Julien Grall wrote:
Hi,
On 12/11/2018 11:30, Mirela Simonovic wrote:
+/*
+ * This function sets the context of current VCPU to the state which is
expected
+ * by the guest on resume. The expected VCPU state is:
+ * 1) pc to contain resume entry point (1st argument of PSCI SYSTEM_SUSPEND)
+ * 2) r0/x0 to contain context ID (2nd argument of PSCI SYSTEM_SUSPEND)
+ * 3) All other general purpose and system registers should have reset values
+ *
+ * Note: this function has to return void because it has to always succeed. In
+ * other words, this function is called from virtual PSCI SYSTEM_SUSPEND
+ * implementation, which can return only a limited number of possible errors,
+ * none of which could represent the fact that an error occurred when preparing
+ * the domain for suspend.
+ * Consequently, dynamic memory allocation cannot be done within this function,
+ * because if malloc fails the error has nowhere to propagate.
+ */
+static void vcpu_suspend(register_t epoint, register_t cid)
+{
+ /* Static allocation because dynamic would need a non-void return */
+ static struct vcpu_guest_context ctxt;
+ struct vcpu *v = current;
+
+ /* Make sure that VCPU guest regs are zeroied */
+ memset(, 0, sizeof(ctxt));
+
+ /* Set non-zero values to the registers prior to copying */
+ ctxt.user_regs.pc64 = (u64)epoint;
+
+ if ( is_32bit_domain(current->domain) )
+ {
+ ctxt.user_regs.r0_usr = cid;
+ ctxt.user_regs.cpsr = PSR_GUEST32_INIT;
This is going to disable the MMU and Cache as requested by the PSCI spec. As the
guest is not required to clean the cache when turning off the CPU/suspending,
the data may not have reached the main memory.
So do you need to perform cache maintenance to avoid stale information?
Answering to myself, I have discussed about the cache with others Arm folks
today. SYSTEM_SUSPEND may exit early due to a pending event (BTW you don't seem
to handle it) and could jump to the specified entry point address.
In that case, the only guarantee is the data would not be lost, yet can still be
in the cache. This means that any data used before the MMU & cache are enabled
in the entry point should have been cleaned to PoC and if you modify data make
sure the cache is invalidated.
Linux is doing that properly. Looking at Xen, we don't really properly handle
the cache in the boot path. So you may randomly crash.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi,
On 12/11/2018 11:30, Mirela Simonovic wrote:
+/*
+ * This function sets the context of current VCPU to the state which is
expected
+ * by the guest on resume. The expected VCPU state is:
+ * 1) pc to contain resume entry point (1st argument of PSCI SYSTEM_SUSPEND)
+ * 2) r0/x0 to contain context ID (2nd argument of PSCI SYSTEM_SUSPEND)
+ * 3) All other general purpose and system registers should have reset values
+ *
+ * Note: this function has to return void because it has to always succeed. In
+ * other words, this function is called from virtual PSCI SYSTEM_SUSPEND
+ * implementation, which can return only a limited number of possible errors,
+ * none of which could represent the fact that an error occurred when preparing
+ * the domain for suspend.
+ * Consequently, dynamic memory allocation cannot be done within this function,
+ * because if malloc fails the error has nowhere to propagate.
+ */
+static void vcpu_suspend(register_t epoint, register_t cid)
+{
+/* Static allocation because dynamic would need a non-void return */
+static struct vcpu_guest_context ctxt;
+struct vcpu *v = current;
+
+/* Make sure that VCPU guest regs are zeroied */
+memset(, 0, sizeof(ctxt));
+
+/* Set non-zero values to the registers prior to copying */
+ctxt.user_regs.pc64 = (u64)epoint;
+
+if ( is_32bit_domain(current->domain) )
+{
+ctxt.user_regs.r0_usr = cid;
+ctxt.user_regs.cpsr = PSR_GUEST32_INIT;
This is going to disable the MMU and Cache as requested by the PSCI spec. As the
guest is not required to clean the cache when turning off the CPU/suspending,
the data may not have reached the main memory.
So do you need to perform cache maintenance to avoid stale information?
+
+/* Thumb set is allowed only for 32-bit domain */
+if ( epoint & 1 )
+{
+ctxt.user_regs.cpsr |= PSR_THUMB;
+ctxt.user_regs.pc64 &= ~(u64)1;
+}
+}
+#ifdef CONFIG_ARM_64
+else
+{
+ctxt.user_regs.x0 = cid;
+ctxt.user_regs.cpsr = PSR_GUEST64_INIT;
Same here.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Stefano, On 13/11/2018 01:53, Stefano Stabellini wrote: On Mon, 12 Nov 2018, Andrew Cooper wrote: On 12/11/18 16:35, Mirela Simonovic wrote: Hi Julien, Thanks for your feedback, I'll need to answer in iterations. On Mon, Nov 12, 2018 at 4:27 PM Julien Grall wrote: Hi Mirela, On 11/12/18 11:30 AM, Mirela Simonovic wrote: The implementation consists of: -Adding PSCI system suspend call as new PSCI function -Trapping PSCI system_suspend HVC -Implementing PSCI system suspend call (virtual interface that allows guests to suspend themselves) The PSCI system suspend should be called by a guest from its boot VCPU. Non-boot VCPUs of the guest should be hot-unplugged using PSCI CPU_OFF call prior to issuing PSCI system suspend. Interrupts that are left enabled by the guest are assumed to be its wake-up interrupts. Therefore, a wake-up interrupt triggers the resume of the guest. Guest should resume regardless of the state of Xen (suspended or not). When a guest calls PSCI system suspend the respective domain will be suspended if the following conditions are met: 1) Given resume entry point is not invalid 2) Other (if any) VCPUs of the calling guest are hot-unplugged If the conditions above are met the calling domain is labeled as suspended and the calling VCPU is blocked. If nothing else wouldn't be done the suspended domain would resume from the place where it called PSCI system suspend. This is expected if processing of the PSCI system suspend call fails. However, in the case of success the calling guest should resume (continue execution after the wake-up) from the entry point which is given as the first argument of the PSCI system suspend call. In addition to the entry point, the guest expects to start within the environment whose state matches the state after reset. This means that the guest should find reset register values, MMU disabled, etc. Thereby, the context of VCPU should be 'reset' (as if the system is comming out of reset), the program counter should contain entry point, which is 1st argument, and r0/x0 should contain context ID which is 2nd argument of PSCI system suspend call. The context of VCPU is set accordingly when the PSCI system suspend is processed, so that nothing needs to be done on resume/wake-up path. However, in order to ensure that this context doesn't get overwritten by the scheduler when scheduling out this VCPU (would normally happen after the calling CPU is blocked), we need to check whether to return early from ctxt_switch_from(). There are variables in domain structure to keep track of domain shutdown. One of existing shutdown reason is 'suspend' which this patch is using to track the suspend state of a domain. Those variables are used to determine whether to early return from ctxt_switch_from() or not. A suspended domain will resume after the Xen receives an interrupt which is targeted to the domain, unblocks the domain's VCPU, and schedules it in. When the VCPU is scheduled in, the VCPU context is already reset, and contains the right resume entry point in program counter that will be restored in ctxt_switch_to(). The only thing that needs to be done at this point is to clear the variables that marked the domain state as suspended. Signed-off-by: Mirela Simonovic Signed-off-by: Saeed Nowshadi --- Changes in v2: -Fix print to compile for arm32 and to align with Xen coding style --- xen/arch/arm/Makefile| 1 + xen/arch/arm/domain.c| 13 +++ xen/arch/arm/suspend.c | 166 +++ xen/arch/arm/vpsci.c | 19 + xen/include/asm-arm/perfc_defn.h | 1 + xen/include/asm-arm/psci.h | 2 + xen/include/asm-arm/suspend.h| 16 xen/include/xen/sched.h | 1 + 8 files changed, 219 insertions(+) create mode 100644 xen/arch/arm/suspend.c create mode 100644 xen/include/asm-arm/suspend.h diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 23c5d9adbc..744b1a4dc8 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -43,6 +43,7 @@ obj-y += setup.o obj-y += shutdown.o obj-y += smp.o obj-y += smpboot.o +obj-y += suspend.o obj-y += sysctl.o obj-y += time.o obj-y += traps.o diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index e594b48d81..7f8105465c 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p) if ( is_idle_vcpu(p) ) return; +/* VCPU's context should not be saved if its domain is suspended */ +if ( p->domain->is_shut_down && +(p->domain->shutdown_code == SHUTDOWN_suspend) ) +return; SHUTDOWN_suspend is used in Xen for other purpose (see SCHEDOP_shutdown). The other user of that code relies on all the state to be saved on suspend. We just need a flag to mark a domain as suspended, and I do believe SHUTDOWN_suspend is not used anywhere else. Let's come back on
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 12/11/2018 19:56, Julien Grall wrote: > Hi Andrew, > > On 11/12/18 4:41 PM, Andrew Cooper wrote: >> On 12/11/18 16:35, Mirela Simonovic wrote: > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index e594b48d81..7f8105465c 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p) > if ( is_idle_vcpu(p) ) > return; > > + /* VCPU's context should not be saved if its domain is > suspended */ > + if ( p->domain->is_shut_down && > + (p->domain->shutdown_code == SHUTDOWN_suspend) ) > + return; SHUTDOWN_suspend is used in Xen for other purpose (see SCHEDOP_shutdown). The other user of that code relies on all the state to be saved on suspend. >>> We just need a flag to mark a domain as suspended, and I do believe >>> SHUTDOWN_suspend is not used anywhere else. >>> Let's come back on this. >> >> SHUTDOWN_suspend is used for migration. Grep for it through the Xen >> tree and you'll find several pieces of documentation, including the >> description of what this shutdown code means. >> >> What you are introducing here is not a shutdown - it is a suspend with >> the intent to resume executing later. As such, it shouldn't use Xen's >> shutdown infrastructure, which exists mainly to communicate with the >> toolstack. > > Would domain pause/unpause be a better solution? Actually yes - that sounds like a very neat solution. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On Mon, 12 Nov 2018, Andrew Cooper wrote: > On 12/11/18 16:35, Mirela Simonovic wrote: > > Hi Julien, > > > > Thanks for your feedback, I'll need to answer in iterations. > > > > On Mon, Nov 12, 2018 at 4:27 PM Julien Grall wrote: > >> Hi Mirela, > >> > >> On 11/12/18 11:30 AM, Mirela Simonovic wrote: > >>> The implementation consists of: > >>> -Adding PSCI system suspend call as new PSCI function > >>> -Trapping PSCI system_suspend HVC > >>> -Implementing PSCI system suspend call (virtual interface that allows > >>> guests to suspend themselves) > >>> > >>> The PSCI system suspend should be called by a guest from its boot > >>> VCPU. Non-boot VCPUs of the guest should be hot-unplugged using PSCI > >>> CPU_OFF call prior to issuing PSCI system suspend. Interrupts that > >>> are left enabled by the guest are assumed to be its wake-up interrupts. > >>> Therefore, a wake-up interrupt triggers the resume of the guest. Guest > >>> should resume regardless of the state of Xen (suspended or not). > >>> > >>> When a guest calls PSCI system suspend the respective domain will be > >>> suspended if the following conditions are met: > >>> 1) Given resume entry point is not invalid > >>> 2) Other (if any) VCPUs of the calling guest are hot-unplugged > >>> > >>> If the conditions above are met the calling domain is labeled as > >>> suspended and the calling VCPU is blocked. If nothing else wouldn't > >>> be done the suspended domain would resume from the place where it > >>> called PSCI system suspend. This is expected if processing of the PSCI > >>> system suspend call fails. However, in the case of success the calling > >>> guest should resume (continue execution after the wake-up) from the entry > >>> point which is given as the first argument of the PSCI system suspend > >>> call. In addition to the entry point, the guest expects to start within > >>> the environment whose state matches the state after reset. This means > >>> that the guest should find reset register values, MMU disabled, etc. > >>> Thereby, the context of VCPU should be 'reset' (as if the system is > >>> comming out of reset), the program counter should contain entry point, > >>> which is 1st argument, and r0/x0 should contain context ID which is 2nd > >>> argument of PSCI system suspend call. The context of VCPU is set > >>> accordingly when the PSCI system suspend is processed, so that nothing > >>> needs to be done on resume/wake-up path. However, in order to ensure that > >>> this context doesn't get overwritten by the scheduler when scheduling out > >>> this VCPU (would normally happen after the calling CPU is blocked), we > >>> need > >>> to check whether to return early from ctxt_switch_from(). > >>> > >>> There are variables in domain structure to keep track of domain shutdown. > >>> One of existing shutdown reason is 'suspend' which this patch is using to > >>> track the suspend state of a domain. Those variables are used to determine > >>> whether to early return from ctxt_switch_from() or not. > >>> > >>> A suspended domain will resume after the Xen receives an interrupt which > >>> is > >>> targeted to the domain, unblocks the domain's VCPU, and schedules it in. > >>> When the VCPU is scheduled in, the VCPU context is already reset, and > >>> contains the right resume entry point in program counter that will be > >>> restored in ctxt_switch_to(). The only thing that needs to be done at this > >>> point is to clear the variables that marked the domain state as suspended. > >>> > >>> Signed-off-by: Mirela Simonovic > >>> Signed-off-by: Saeed Nowshadi > >>> > >>> --- > >>> Changes in v2: > >>> > >>> -Fix print to compile for arm32 and to align with Xen coding style > >>> --- > >>> xen/arch/arm/Makefile| 1 + > >>> xen/arch/arm/domain.c| 13 +++ > >>> xen/arch/arm/suspend.c | 166 > >>> +++ > >>> xen/arch/arm/vpsci.c | 19 + > >>> xen/include/asm-arm/perfc_defn.h | 1 + > >>> xen/include/asm-arm/psci.h | 2 + > >>> xen/include/asm-arm/suspend.h| 16 > >>> xen/include/xen/sched.h | 1 + > >>> 8 files changed, 219 insertions(+) > >>> create mode 100644 xen/arch/arm/suspend.c > >>> create mode 100644 xen/include/asm-arm/suspend.h > >>> > >>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > >>> index 23c5d9adbc..744b1a4dc8 100644 > >>> --- a/xen/arch/arm/Makefile > >>> +++ b/xen/arch/arm/Makefile > >>> @@ -43,6 +43,7 @@ obj-y += setup.o > >>> obj-y += shutdown.o > >>> obj-y += smp.o > >>> obj-y += smpboot.o > >>> +obj-y += suspend.o > >>> obj-y += sysctl.o > >>> obj-y += time.o > >>> obj-y += traps.o > >>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > >>> index e594b48d81..7f8105465c 100644 > >>> --- a/xen/arch/arm/domain.c > >>> +++ b/xen/arch/arm/domain.c > >>> @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p) > >>> if ( is_idle_vcpu(p) ) >
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 11/12/18 4:35 PM, Mirela Simonovic wrote: Hi Julien, Thanks for your feedback, I'll need to answer in iterations. On Mon, Nov 12, 2018 at 4:27 PM Julien Grall wrote: Hi Mirela, On 11/12/18 11:30 AM, Mirela Simonovic wrote: The implementation consists of: -Adding PSCI system suspend call as new PSCI function -Trapping PSCI system_suspend HVC -Implementing PSCI system suspend call (virtual interface that allows guests to suspend themselves) The PSCI system suspend should be called by a guest from its boot VCPU. Non-boot VCPUs of the guest should be hot-unplugged using PSCI CPU_OFF call prior to issuing PSCI system suspend. Interrupts that are left enabled by the guest are assumed to be its wake-up interrupts. Therefore, a wake-up interrupt triggers the resume of the guest. Guest should resume regardless of the state of Xen (suspended or not). When a guest calls PSCI system suspend the respective domain will be suspended if the following conditions are met: 1) Given resume entry point is not invalid 2) Other (if any) VCPUs of the calling guest are hot-unplugged If the conditions above are met the calling domain is labeled as suspended and the calling VCPU is blocked. If nothing else wouldn't be done the suspended domain would resume from the place where it called PSCI system suspend. This is expected if processing of the PSCI system suspend call fails. However, in the case of success the calling guest should resume (continue execution after the wake-up) from the entry point which is given as the first argument of the PSCI system suspend call. In addition to the entry point, the guest expects to start within the environment whose state matches the state after reset. This means that the guest should find reset register values, MMU disabled, etc. Thereby, the context of VCPU should be 'reset' (as if the system is comming out of reset), the program counter should contain entry point, which is 1st argument, and r0/x0 should contain context ID which is 2nd argument of PSCI system suspend call. The context of VCPU is set accordingly when the PSCI system suspend is processed, so that nothing needs to be done on resume/wake-up path. However, in order to ensure that this context doesn't get overwritten by the scheduler when scheduling out this VCPU (would normally happen after the calling CPU is blocked), we need to check whether to return early from ctxt_switch_from(). There are variables in domain structure to keep track of domain shutdown. One of existing shutdown reason is 'suspend' which this patch is using to track the suspend state of a domain. Those variables are used to determine whether to early return from ctxt_switch_from() or not. A suspended domain will resume after the Xen receives an interrupt which is targeted to the domain, unblocks the domain's VCPU, and schedules it in. When the VCPU is scheduled in, the VCPU context is already reset, and contains the right resume entry point in program counter that will be restored in ctxt_switch_to(). The only thing that needs to be done at this point is to clear the variables that marked the domain state as suspended. Signed-off-by: Mirela Simonovic Signed-off-by: Saeed Nowshadi --- Changes in v2: -Fix print to compile for arm32 and to align with Xen coding style --- xen/arch/arm/Makefile| 1 + xen/arch/arm/domain.c| 13 +++ xen/arch/arm/suspend.c | 166 +++ xen/arch/arm/vpsci.c | 19 + xen/include/asm-arm/perfc_defn.h | 1 + xen/include/asm-arm/psci.h | 2 + xen/include/asm-arm/suspend.h| 16 xen/include/xen/sched.h | 1 + 8 files changed, 219 insertions(+) create mode 100644 xen/arch/arm/suspend.c create mode 100644 xen/include/asm-arm/suspend.h diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 23c5d9adbc..744b1a4dc8 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -43,6 +43,7 @@ obj-y += setup.o obj-y += shutdown.o obj-y += smp.o obj-y += smpboot.o +obj-y += suspend.o obj-y += sysctl.o obj-y += time.o obj-y += traps.o diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index e594b48d81..7f8105465c 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p) if ( is_idle_vcpu(p) ) return; +/* VCPU's context should not be saved if its domain is suspended */ +if ( p->domain->is_shut_down && +(p->domain->shutdown_code == SHUTDOWN_suspend) ) +return; SHUTDOWN_suspend is used in Xen for other purpose (see SCHEDOP_shutdown). The other user of that code relies on all the state to be saved on suspend. We just need a flag to mark a domain as suspended, and I do believe SHUTDOWN_suspend is not used anywhere else. Let's come back on this. See Andrew's comment here. However, what is the issue with saving all the registers here?
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Andrew, On 11/12/18 4:41 PM, Andrew Cooper wrote: On 12/11/18 16:35, Mirela Simonovic wrote: diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index e594b48d81..7f8105465c 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p) if ( is_idle_vcpu(p) ) return; +/* VCPU's context should not be saved if its domain is suspended */ +if ( p->domain->is_shut_down && +(p->domain->shutdown_code == SHUTDOWN_suspend) ) +return; SHUTDOWN_suspend is used in Xen for other purpose (see SCHEDOP_shutdown). The other user of that code relies on all the state to be saved on suspend. We just need a flag to mark a domain as suspended, and I do believe SHUTDOWN_suspend is not used anywhere else. Let's come back on this. SHUTDOWN_suspend is used for migration. Grep for it through the Xen tree and you'll find several pieces of documentation, including the description of what this shutdown code means. What you are introducing here is not a shutdown - it is a suspend with the intent to resume executing later. As such, it shouldn't use Xen's shutdown infrastructure, which exists mainly to communicate with the toolstack. Would domain pause/unpause be a better solution? Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On 12/11/18 16:35, Mirela Simonovic wrote: > Hi Julien, > > Thanks for your feedback, I'll need to answer in iterations. > > On Mon, Nov 12, 2018 at 4:27 PM Julien Grall wrote: >> Hi Mirela, >> >> On 11/12/18 11:30 AM, Mirela Simonovic wrote: >>> The implementation consists of: >>> -Adding PSCI system suspend call as new PSCI function >>> -Trapping PSCI system_suspend HVC >>> -Implementing PSCI system suspend call (virtual interface that allows >>> guests to suspend themselves) >>> >>> The PSCI system suspend should be called by a guest from its boot >>> VCPU. Non-boot VCPUs of the guest should be hot-unplugged using PSCI >>> CPU_OFF call prior to issuing PSCI system suspend. Interrupts that >>> are left enabled by the guest are assumed to be its wake-up interrupts. >>> Therefore, a wake-up interrupt triggers the resume of the guest. Guest >>> should resume regardless of the state of Xen (suspended or not). >>> >>> When a guest calls PSCI system suspend the respective domain will be >>> suspended if the following conditions are met: >>> 1) Given resume entry point is not invalid >>> 2) Other (if any) VCPUs of the calling guest are hot-unplugged >>> >>> If the conditions above are met the calling domain is labeled as >>> suspended and the calling VCPU is blocked. If nothing else wouldn't >>> be done the suspended domain would resume from the place where it >>> called PSCI system suspend. This is expected if processing of the PSCI >>> system suspend call fails. However, in the case of success the calling >>> guest should resume (continue execution after the wake-up) from the entry >>> point which is given as the first argument of the PSCI system suspend >>> call. In addition to the entry point, the guest expects to start within >>> the environment whose state matches the state after reset. This means >>> that the guest should find reset register values, MMU disabled, etc. >>> Thereby, the context of VCPU should be 'reset' (as if the system is >>> comming out of reset), the program counter should contain entry point, >>> which is 1st argument, and r0/x0 should contain context ID which is 2nd >>> argument of PSCI system suspend call. The context of VCPU is set >>> accordingly when the PSCI system suspend is processed, so that nothing >>> needs to be done on resume/wake-up path. However, in order to ensure that >>> this context doesn't get overwritten by the scheduler when scheduling out >>> this VCPU (would normally happen after the calling CPU is blocked), we need >>> to check whether to return early from ctxt_switch_from(). >>> >>> There are variables in domain structure to keep track of domain shutdown. >>> One of existing shutdown reason is 'suspend' which this patch is using to >>> track the suspend state of a domain. Those variables are used to determine >>> whether to early return from ctxt_switch_from() or not. >>> >>> A suspended domain will resume after the Xen receives an interrupt which is >>> targeted to the domain, unblocks the domain's VCPU, and schedules it in. >>> When the VCPU is scheduled in, the VCPU context is already reset, and >>> contains the right resume entry point in program counter that will be >>> restored in ctxt_switch_to(). The only thing that needs to be done at this >>> point is to clear the variables that marked the domain state as suspended. >>> >>> Signed-off-by: Mirela Simonovic >>> Signed-off-by: Saeed Nowshadi >>> >>> --- >>> Changes in v2: >>> >>> -Fix print to compile for arm32 and to align with Xen coding style >>> --- >>> xen/arch/arm/Makefile| 1 + >>> xen/arch/arm/domain.c| 13 +++ >>> xen/arch/arm/suspend.c | 166 >>> +++ >>> xen/arch/arm/vpsci.c | 19 + >>> xen/include/asm-arm/perfc_defn.h | 1 + >>> xen/include/asm-arm/psci.h | 2 + >>> xen/include/asm-arm/suspend.h| 16 >>> xen/include/xen/sched.h | 1 + >>> 8 files changed, 219 insertions(+) >>> create mode 100644 xen/arch/arm/suspend.c >>> create mode 100644 xen/include/asm-arm/suspend.h >>> >>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >>> index 23c5d9adbc..744b1a4dc8 100644 >>> --- a/xen/arch/arm/Makefile >>> +++ b/xen/arch/arm/Makefile >>> @@ -43,6 +43,7 @@ obj-y += setup.o >>> obj-y += shutdown.o >>> obj-y += smp.o >>> obj-y += smpboot.o >>> +obj-y += suspend.o >>> obj-y += sysctl.o >>> obj-y += time.o >>> obj-y += traps.o >>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >>> index e594b48d81..7f8105465c 100644 >>> --- a/xen/arch/arm/domain.c >>> +++ b/xen/arch/arm/domain.c >>> @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p) >>> if ( is_idle_vcpu(p) ) >>> return; >>> >>> +/* VCPU's context should not be saved if its domain is suspended */ >>> +if ( p->domain->is_shut_down && >>> +(p->domain->shutdown_code == SHUTDOWN_suspend) ) >>> +return; >> SHUTDOWN_suspend is used
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Julien, Thanks for your feedback, I'll need to answer in iterations. On Mon, Nov 12, 2018 at 4:27 PM Julien Grall wrote: > > Hi Mirela, > > On 11/12/18 11:30 AM, Mirela Simonovic wrote: > > The implementation consists of: > > -Adding PSCI system suspend call as new PSCI function > > -Trapping PSCI system_suspend HVC > > -Implementing PSCI system suspend call (virtual interface that allows > > guests to suspend themselves) > > > > The PSCI system suspend should be called by a guest from its boot > > VCPU. Non-boot VCPUs of the guest should be hot-unplugged using PSCI > > CPU_OFF call prior to issuing PSCI system suspend. Interrupts that > > are left enabled by the guest are assumed to be its wake-up interrupts. > > Therefore, a wake-up interrupt triggers the resume of the guest. Guest > > should resume regardless of the state of Xen (suspended or not). > > > > When a guest calls PSCI system suspend the respective domain will be > > suspended if the following conditions are met: > > 1) Given resume entry point is not invalid > > 2) Other (if any) VCPUs of the calling guest are hot-unplugged > > > > If the conditions above are met the calling domain is labeled as > > suspended and the calling VCPU is blocked. If nothing else wouldn't > > be done the suspended domain would resume from the place where it > > called PSCI system suspend. This is expected if processing of the PSCI > > system suspend call fails. However, in the case of success the calling > > guest should resume (continue execution after the wake-up) from the entry > > point which is given as the first argument of the PSCI system suspend > > call. In addition to the entry point, the guest expects to start within > > the environment whose state matches the state after reset. This means > > that the guest should find reset register values, MMU disabled, etc. > > Thereby, the context of VCPU should be 'reset' (as if the system is > > comming out of reset), the program counter should contain entry point, > > which is 1st argument, and r0/x0 should contain context ID which is 2nd > > argument of PSCI system suspend call. The context of VCPU is set > > accordingly when the PSCI system suspend is processed, so that nothing > > needs to be done on resume/wake-up path. However, in order to ensure that > > this context doesn't get overwritten by the scheduler when scheduling out > > this VCPU (would normally happen after the calling CPU is blocked), we need > > to check whether to return early from ctxt_switch_from(). > > > > There are variables in domain structure to keep track of domain shutdown. > > One of existing shutdown reason is 'suspend' which this patch is using to > > track the suspend state of a domain. Those variables are used to determine > > whether to early return from ctxt_switch_from() or not. > > > > A suspended domain will resume after the Xen receives an interrupt which is > > targeted to the domain, unblocks the domain's VCPU, and schedules it in. > > When the VCPU is scheduled in, the VCPU context is already reset, and > > contains the right resume entry point in program counter that will be > > restored in ctxt_switch_to(). The only thing that needs to be done at this > > point is to clear the variables that marked the domain state as suspended. > > > > Signed-off-by: Mirela Simonovic > > Signed-off-by: Saeed Nowshadi > > > > --- > > Changes in v2: > > > > -Fix print to compile for arm32 and to align with Xen coding style > > --- > > xen/arch/arm/Makefile| 1 + > > xen/arch/arm/domain.c| 13 +++ > > xen/arch/arm/suspend.c | 166 > > +++ > > xen/arch/arm/vpsci.c | 19 + > > xen/include/asm-arm/perfc_defn.h | 1 + > > xen/include/asm-arm/psci.h | 2 + > > xen/include/asm-arm/suspend.h| 16 > > xen/include/xen/sched.h | 1 + > > 8 files changed, 219 insertions(+) > > create mode 100644 xen/arch/arm/suspend.c > > create mode 100644 xen/include/asm-arm/suspend.h > > > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > > index 23c5d9adbc..744b1a4dc8 100644 > > --- a/xen/arch/arm/Makefile > > +++ b/xen/arch/arm/Makefile > > @@ -43,6 +43,7 @@ obj-y += setup.o > > obj-y += shutdown.o > > obj-y += smp.o > > obj-y += smpboot.o > > +obj-y += suspend.o > > obj-y += sysctl.o > > obj-y += time.o > > obj-y += traps.o > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > > index e594b48d81..7f8105465c 100644 > > --- a/xen/arch/arm/domain.c > > +++ b/xen/arch/arm/domain.c > > @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p) > > if ( is_idle_vcpu(p) ) > > return; > > > > +/* VCPU's context should not be saved if its domain is suspended */ > > +if ( p->domain->is_shut_down && > > +(p->domain->shutdown_code == SHUTDOWN_suspend) ) > > +return; > SHUTDOWN_suspend is used in Xen for other purpose (see > SCHEDOP_shutdown). The
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Mirela,
On 11/12/18 11:30 AM, Mirela Simonovic wrote:
The implementation consists of:
-Adding PSCI system suspend call as new PSCI function
-Trapping PSCI system_suspend HVC
-Implementing PSCI system suspend call (virtual interface that allows
guests to suspend themselves)
The PSCI system suspend should be called by a guest from its boot
VCPU. Non-boot VCPUs of the guest should be hot-unplugged using PSCI
CPU_OFF call prior to issuing PSCI system suspend. Interrupts that
are left enabled by the guest are assumed to be its wake-up interrupts.
Therefore, a wake-up interrupt triggers the resume of the guest. Guest
should resume regardless of the state of Xen (suspended or not).
When a guest calls PSCI system suspend the respective domain will be
suspended if the following conditions are met:
1) Given resume entry point is not invalid
2) Other (if any) VCPUs of the calling guest are hot-unplugged
If the conditions above are met the calling domain is labeled as
suspended and the calling VCPU is blocked. If nothing else wouldn't
be done the suspended domain would resume from the place where it
called PSCI system suspend. This is expected if processing of the PSCI
system suspend call fails. However, in the case of success the calling
guest should resume (continue execution after the wake-up) from the entry
point which is given as the first argument of the PSCI system suspend
call. In addition to the entry point, the guest expects to start within
the environment whose state matches the state after reset. This means
that the guest should find reset register values, MMU disabled, etc.
Thereby, the context of VCPU should be 'reset' (as if the system is
comming out of reset), the program counter should contain entry point,
which is 1st argument, and r0/x0 should contain context ID which is 2nd
argument of PSCI system suspend call. The context of VCPU is set
accordingly when the PSCI system suspend is processed, so that nothing
needs to be done on resume/wake-up path. However, in order to ensure that
this context doesn't get overwritten by the scheduler when scheduling out
this VCPU (would normally happen after the calling CPU is blocked), we need
to check whether to return early from ctxt_switch_from().
There are variables in domain structure to keep track of domain shutdown.
One of existing shutdown reason is 'suspend' which this patch is using to
track the suspend state of a domain. Those variables are used to determine
whether to early return from ctxt_switch_from() or not.
A suspended domain will resume after the Xen receives an interrupt which is
targeted to the domain, unblocks the domain's VCPU, and schedules it in.
When the VCPU is scheduled in, the VCPU context is already reset, and
contains the right resume entry point in program counter that will be
restored in ctxt_switch_to(). The only thing that needs to be done at this
point is to clear the variables that marked the domain state as suspended.
Signed-off-by: Mirela Simonovic
Signed-off-by: Saeed Nowshadi
---
Changes in v2:
-Fix print to compile for arm32 and to align with Xen coding style
---
xen/arch/arm/Makefile| 1 +
xen/arch/arm/domain.c| 13 +++
xen/arch/arm/suspend.c | 166 +++
xen/arch/arm/vpsci.c | 19 +
xen/include/asm-arm/perfc_defn.h | 1 +
xen/include/asm-arm/psci.h | 2 +
xen/include/asm-arm/suspend.h| 16
xen/include/xen/sched.h | 1 +
8 files changed, 219 insertions(+)
create mode 100644 xen/arch/arm/suspend.c
create mode 100644 xen/include/asm-arm/suspend.h
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 23c5d9adbc..744b1a4dc8 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -43,6 +43,7 @@ obj-y += setup.o
obj-y += shutdown.o
obj-y += smp.o
obj-y += smpboot.o
+obj-y += suspend.o
obj-y += sysctl.o
obj-y += time.o
obj-y += traps.o
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e594b48d81..7f8105465c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+/* VCPU's context should not be saved if its domain is suspended */
+if ( p->domain->is_shut_down &&
+(p->domain->shutdown_code == SHUTDOWN_suspend) )
+return;
SHUTDOWN_suspend is used in Xen for other purpose (see
SCHEDOP_shutdown). The other user of that code relies on all the state
to be saved on suspend.
However, what is the issue with saving all the registers here?
+
p2m_save_state(p);
/* CP 15 */
@@ -181,6 +186,14 @@ static void ctxt_switch_to(struct vcpu *n)
if ( is_idle_vcpu(n) )
return;
+/* If the domain was suspended, it is resuming now */
+if ( n->domain->is_shut_down &&
+(n->domain->shutdown_code == SHUTDOWN_suspend) )
+{
+n->domain->is_shut_down = 0;
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
>>> On 12.11.18 at 12:50, wrote: > What? Please don't top post. > On Mon, Nov 12, 2018 at 12:42 PM Jan Beulich wrote: > >> >>> On 12.11.18 at 12:30, wrote: >> > --- a/xen/include/xen/sched.h >> > +++ b/xen/include/xen/sched.h >> > @@ -24,6 +24,7 @@ >> > #include >> > #include >> > #include >> > +#include >> > #include >> > #include >> > #include >> >> Why? I think given the context the question, even if terse, is pretty clear: Why do you add this #include as the only change in this header? Nothing in the header previously required it, and if you have a sudden new requirement elsewhere, then please include the extra header just there. Also please accept that given the amount of time needed to deal with incoming patches, it is not generally scalable to always give long explanations to what seems pretty obvious anyway. (I do realize that "obvious" is a rather subjective attribute to things.) Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
What? On Mon, Nov 12, 2018 at 12:42 PM Jan Beulich wrote: > >>> On 12.11.18 at 12:30, wrote: > > --- a/xen/include/xen/sched.h > > +++ b/xen/include/xen/sched.h > > @@ -24,6 +24,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > Why? > > Jan. > > > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
>>> On 12.11.18 at 12:30, wrote: > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > #include > #include > #include Why? Jan. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
