Re: [Xen-devel] [PATCH 05/18] xen/arm: Trigger Xen suspend when Dom0 completes suspend

[Julien Grall <julien.grall@xxxxxxx>]

On 13/11/2018 11:26, Mirela Simonovic wrote:
> Hi,
>
> On Tue, Nov 13, 2018 at 10:43 AM Julien Grall <julien.grall@xxxxxxx> wrote:
> > Hi Stefano,
> >
> > On 12/11/2018 23:46, Stefano Stabellini wrote:
> > > On Mon, 12 Nov 2018, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > On 11/12/18 11:30 AM, Mirela Simonovic wrote:
> > > > > When Dom0 finalizes its suspend procedure the suspend of Xen is
> > > > > triggered by calling system_suspend(). Dom0 finalizes the suspend from
> > > > > its boot core (VCPU#0), which could be mapped to any physical CPU,
> > > > > i.e. the system_suspend() function could be executed by any physical
> > > > > CPU. Since Xen suspend procedure has to be run by the boot CPU
> > > > > (non-boot CPUs will be disabled at some point in suspend procedure),
> > > > > system_suspend() execution has to continue on CPU#0.
> > > > Nothing in the domain_suspend code checks that domain_suspend is called by
> > > > vCPU0. I also can't find any restriction in the PSCI spec to run on pCPU0. Can
> > > > you provide more details why this required?
> > > The spec says that "to use this API, a calling OS must power down all
> > > but one core through calls to CPU_OFF". It is natural to think that the
> > > remaining core would be (physical or virtual) cpu0, but actually it is
> > > not clearly stated by the spec. For dom0, we could simply check that
> > > only 1 vcpu is left ON.
> > It is what we already do in the code. The comment and commit message does not
> > match the code.
> >
> > > For Xen and the physical system_suspend call,
> > > it makes sense to make the call on pcpu0.
> > This needs some rationale. What's wrong with issue the suspend from the current
> > pCPU?
> The guest doesn't have to finalize its suspend procedure from VCPU0.
> That is not required by the spec, although it is commonly the case in
> implementations. We don't make any assumptions on CPU ID until Dom0
> suspends. In the context of Dom0 and comments in this patch, linux
> will finalize the suspend procedure from its boot core/CPU0, so the
> comment is correct.
The hypervisor is not tie to a specific Operating System. So comment should 
*not* assume that Dom0 is always Linux.
The comment should be as generic as possible and match the code. If you say 
"Dom0 will suspend from vCPU#0", then I expect a check "if vcpu_id != 0 then 
return PSCI_DENIED".
Example are always fine, but this should be treated as such. In that context, we 
don't really care that Dom0 called from CPU0 or CPU1. What matters is you have 
only one vCPU left.
> When it comes to suspending Xen, we use generic, existing functions to
> disable secondary cpus (disable_nonboot_cpus) later in the suspend
> procedure. This function makes the assumption that non-boot CPUs are
> the ones whose index is not zero, and the pCPU0 will not be disabled
> in this process. So we have to switch the execution to physical CPU0
> before we move on with suspend. The same is done for x86 suspend.
Please clarify it in the commit message.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel