Re: [Xen-devel] [PATCH 13/18] xen/arm: Implement PSCI SYSTEM_SUSPEND call (physical interface)

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

On Wed, Nov 14, 2018 at 1:14 AM Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
>
> On Mon, 12 Nov 2018, Mirela Simonovic wrote:
> > PSCI system suspend function shall be invoked to finalize Xen suspend
> > procedure. Resume entry point, which needs to be passed via 1st argument
> > of PSCI system suspend call to the EL3, is hyp_resume. For now, hyp_resume
> > is just a placeholder that will be implemented in assembly. Context ID,
> > which is 2nd argument of system suspend PSCI call, is unused, as in Linux.
> >
> > Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> > Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> >
> > ---
> > Changes in v2:
> >
> > -The commit message was stale - referring to the do_suspend function
> > that has been renamed long time ago. Fixed commit message
> > ---
> >  xen/arch/arm/arm64/entry.S    |  3 +++
> >  xen/arch/arm/psci.c           | 16 ++++++++++++++++
> >  xen/arch/arm/suspend.c        |  4 ++++
> >  xen/include/asm-arm/psci.h    |  1 +
> >  xen/include/asm-arm/suspend.h |  1 +
> >  5 files changed, 25 insertions(+)
> >
> > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> > index 97b05f53ea..dbc4717903 100644
> > --- a/xen/arch/arm/arm64/entry.S
> > +++ b/xen/arch/arm/arm64/entry.S
> > @@ -533,6 +533,9 @@ ENTRY(__context_switch)
> >          mov     sp, x9
> >          ret
> >
> > +ENTRY(hyp_resume)
> > +        b .
> > +
> >  /*
> >   * Local variables:
> >   * mode: ASM
> > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> > index a93121f43b..b100bd8ad2 100644
> > --- a/xen/arch/arm/psci.c
> > +++ b/xen/arch/arm/psci.c
> > @@ -24,6 +24,7 @@
> >  #include <asm/cpufeature.h>
> >  #include <asm/psci.h>
> >  #include <asm/acpi.h>
> > +#include <asm/suspend.h>
> >
> >  /*
> >   * While a 64-bit OS can make calls with SMC32 calling conventions, for
> > @@ -67,6 +68,21 @@ void call_psci_cpu_off(void)
> >      }
> >  }
> >
> > +int call_psci_system_suspend(void)
> > +{
> > +#ifdef CONFIG_ARM_64
> > +    struct arm_smccc_res res;
> > +
> > +    /* 2nd argument (context ID) is not used */
> > +    arm_smccc_smc(PSCI_1_0_FN64_SYSTEM_SUSPEND, __pa(hyp_resume), &res);
> > +
> > +    return PSCI_RET(res);
> > +#else
> > +    /* not supported */
> > +    return 1;
> > +#endif
> > +}
>
> Given that suspend is unimplemented on arm32, the #ifdef is OK. But
> in that case return PSCI_NOT_SUPPORTED for arm32.
>
> Otherwise you should be able to remove this #ifdef by introducing
> something similar to PSCI_0_2_FN_NATIVE, but for PSCI_1_0 calls.
>
>
> >  void call_psci_system_off(void)
> >  {
> >      if ( psci_ver > PSCI_VERSION(0, 1) )
> > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > index d1b48c339a..37926374bc 100644
> > --- a/xen/arch/arm/suspend.c
> > +++ b/xen/arch/arm/suspend.c
> > @@ -141,6 +141,10 @@ static long system_suspend(void *data)
> >          goto resume_irqs;
> >      }
> >
> > +    status = call_psci_system_suspend();
> > +    if ( status )
> > +        dprintk(XENLOG_ERR, "PSCI system suspend failed, err=%d\n", 
> > status);
> > +
> >      system_state = SYS_STATE_resume;
> >
> >      gic_resume();
> > diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> > index 26462d0c47..9f6116a224 100644
> > --- a/xen/include/asm-arm/psci.h
> > +++ b/xen/include/asm-arm/psci.h
> > @@ -20,6 +20,7 @@ extern uint32_t psci_ver;
> >
> >  int psci_init(void);
> >  int call_psci_cpu_on(int cpu);
> > +int call_psci_system_suspend(void);
> >  void call_psci_cpu_off(void);
> >  void call_psci_system_off(void);
> >  void call_psci_system_reset(void);
> > diff --git a/xen/include/asm-arm/suspend.h b/xen/include/asm-arm/suspend.h
> > index de787d296a..7604e2e2e2 100644
> > --- a/xen/include/asm-arm/suspend.h
> > +++ b/xen/include/asm-arm/suspend.h
> > @@ -2,6 +2,7 @@
> >  #define __ASM_ARM_SUSPEND_H__
> >
> >  int32_t domain_suspend(register_t epoint, register_t cid);
> > +void hyp_resume(void);
>
> I think it would be better to separate physical suspend from virtual
> suspend, like Julien suggested. As you separate the C implementation,
> please also introduce separate header files (for instance vsuspend.h and
> suspend.h).
>

AFAIU Julien came back with Andrew's feedback suggesting that this
should rather not be done. Please see discussion "[PATCH 02/18]
xen/arm: Implement PSCI system suspend call (virtual interface)"

>
> >  #endif
> >
> > --
> > 2.13.0
> >

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