Re: [Minios-devel] [UNIKRAFT PATCHv4 35/43] plat/common: Implement CPU suspend for arm64

[Julien Grall <julien.grall@xxxxxxx>]

On 18/07/18 10:11, Wei Chen wrote:
> Hi Julien,
Hi Wei,

> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: 2018年7月16日 21:48
> > To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
> > simon.kuenzer@xxxxxxxxx
> > Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> > Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 35/43] plat/common: Implement
> > CPU suspend for arm64
> >
> > Hi Wei,
> >
> > On 06/07/18 10:03, Wei Chen wrote:
> > > Using PSCI despatch function to call PSCI_CPU_SUSPEND
> > > function to halt CPU.
> > >
> > > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > > ---
> > >    plat/common/arm/cpu_native.c        | 44 +++++++++++++++++++++++++++++
> > >    plat/common/include/arm/arm64/cpu.h |  2 ++
> > >    2 files changed, 46 insertions(+)
> > >    create mode 100644 plat/common/arm/cpu_native.c
> > >
> > > diff --git a/plat/common/arm/cpu_native.c b/plat/common/arm/cpu_native.c
> > > new file mode 100644
> > > index 0000000..07548bf
> > > --- /dev/null
> > > +++ b/plat/common/arm/cpu_native.c
> > > @@ -0,0 +1,44 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause */
> > > +/*
> > > + * Authors: Wei Chen <wei.chen@xxxxxxx>
> > > + *
> > > + * Copyright (c) 2018, Arm Ltd., All rights reserved.
> > > + *
> > > + * Redistribution and use in source and binary forms, with or without
> > > + * modification, are permitted provided that the following conditions
> > > + * are met:
> > > + *
> > > + * 1. Redistributions of source code must retain the above copyright
> > > + *    notice, this list of conditions and the following disclaimer.
> > > + * 2. Redistributions in binary form must reproduce the above copyright
> > > + *    notice, this list of conditions and the following disclaimer in the
> > > + *    documentation and/or other materials provided with the distribution.
> > > + * 3. Neither the name of the copyright holder nor the names of its
> > > + *    contributors may be used to endorse or promote products derived from
> > > + *    this software without specific prior written permission.
> > > + *
> > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> > IS"
> > > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> > THE
> > > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> > PURPOSE
> > > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
> > BE
> > > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> > > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> > > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> > > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
> > THE
> > > + * POSSIBILITY OF SUCH DAMAGE.
> > > + *
> > > + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > > + */
> > > +
> > > +#include <arm/cpu.h>
> > > +#include <arm/cpu_defs.h>
> > > +
> > > +void halt(void)
> > Looking at the usage of halt, I think PSCI CPU suspend is too expensive
> > for brief idling.
> >
> > I am not entirely sure of the semantic expected, but it looks like "wfi"
> > would be more suitable here.
> Does timer interrupt can wake up wfi? If yes, I think I can't use wfi here.
Yes interrupt will wake-up the CPU. This is the same with PSCI suspend.

The main difference with the two is the latter will allow you to go in 
deeper state (depending on the arguments you passed).
You don't need that at the moment.

> > > +{
> > > +       if (psci_method == PSCI_METHOD_HVC)
> > > +               psci_hvc_call(PSCI_FNID_CPU_SUSPEND);
> > > +       else if (psci_method == PSCI_METHOD_SMC)
> > > +               psci_smc_call(PSCI_FNID_CPU_SUSPEND);
> > Regardless my comment above, I don't think this code follow the PSCI
> > spec. CPU_SUSPEND takes another parameter that is left unknown. Also,
> Yes, I have ignored other parameters. I will use another patch series to
> improve the PSCI library. Let's record it in TODO list.
I don't think this should be a TODO. That function takes 3 arguments, 
the first one will be used to decide how deep the processor will sleep.
If you don't specify it, you will get whatever value will be in w1 when 
calling CPU_SUSPEND. This could be anything from standby to powerdown 
the CPU.
So you should at least pass a default values to all the parameters of 
PSCI calls.

> > Furthermore, you may want to introduce a function pointer and detect at
> > initialization which call you want to use rather than checking in every
> > function.
> That's sensible. I will use such a function pointer.
>
> > Lastly, in case of non-PSCI platform function will just do nothing. This
> > looks slightly strange.
> Yes, because in the code context, while cpu_suspend/cpu_halt could not take
> effect, the code will enter dead loop.
There are multiple place where halt can be called. Not all of them will 
end up with a dead loop. I would at least at a comment explaining 
nothing is done with non-PSCI platform.
Cheers,

--
Julien Grall

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