Re: [Minios-devel] [UNIKRAFT PATCHv4 33/43] plat/kvm: Get PSCI conduit method from DTB for arm64

[Julien Grall <julien.grall@xxxxxxx>]

On 18/07/18 09:31, Wei Chen wrote:
> Hi Julien,
Hi Wei,

> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: 2018年7月16日 21:14
> > 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 33/43] plat/kvm: Get PSCI
> > conduit method from DTB for arm64
> >
> > Hi Wei,
> >
> > On 06/07/18 10:03, Wei Chen wrote:
> > > PSCI supports two conduit method: HVC and SMC. The method
> > > would be set value in device tree's PSCI node. This value
> > > would decide which instruction we should use to call PSCI
> > > functions.
> > >
> > > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > > ---
> > >    plat/common/include/arm/arm64/cpu.h |  6 +++++
> > >    plat/kvm/arm/setup.c                | 39 +++++++++++++++++++++++++++++
> > >    2 files changed, 45 insertions(+)
> > >
> > > diff --git a/plat/common/include/arm/arm64/cpu.h
> > b/plat/common/include/arm/arm64/cpu.h
> > > index 7c79462..cf0f6a2 100644
> > > --- a/plat/common/include/arm/arm64/cpu.h
> > > +++ b/plat/common/include/arm/arm64/cpu.h
> > > @@ -70,3 +70,9 @@
> > >    #define      SYSREG_WRITE(reg, val)      \
> > >         __asm__ __volatile__("msr " __STRINGIFY(reg) ", %0" \
> > >                                         : : "r" ((uint64_t)(val)))
> > > +
> > > +
> > No need for 2 newline here.
> I will remove one.
>
> > > +/* PSCI conduit types */
> > > +#define PSCI_METHOD_NONE       0x0
> > > +#define PSCI_METHOD_HVC                0x1
> > > +#define PSCI_METHOD_SMC                0x2
> > It feels like this should be an enum.
> Ok.
>
> > > diff --git a/plat/kvm/arm/setup.c b/plat/kvm/arm/setup.c
> > > index 685308c..b7f3e47 100644
> > > --- a/plat/kvm/arm/setup.c
> > > +++ b/plat/kvm/arm/setup.c
> > > @@ -35,6 +35,7 @@
> > >    #include <uk/plat/console.h>
> > >    #include <uk/assert.h>
> > >    #include <uk/essentials.h>
> > > +#include <arm/cpu.h>
> > >    #include <arm/cpu_defs.h>
> > >
> > >    #define MAX_CMDLINE_SIZE 1024
> > > @@ -46,6 +47,8 @@ void *_libkvmplat_stack_top;
> > >    void *_libkvmplat_mem_end;
> > >    void *_libkvmplat_dtb;
> > >
> > > +int psci_method;
> > > +
> > >    static void _init_dtb(void *dtb_pointer)
> > >    {
> > >         int ret;
> > > @@ -83,6 +86,39 @@ enocmdl:
> > >         strcpy(cmdline, CONFIG_UK_NAME);
> > >    }
> > >
> > > +static void _dtb_get_psci_method(void)
> > > +{
> > > +       int fdtpsci, len;
> > > +       const char *fdtmethod;
> > > +
> > > +       fdtpsci = fdt_node_offset_by_compatible(_libkvmplat_dtb,
> > > +                                               0, "arm,psci-0.2");
> > > +       if (fdtpsci < 0)
> > > +               fdtpsci = fdt_node_offset_by_compatible(_libkvmplat_dtb,
> > > +                                                       0, "arm,psci-0.1");
> > Looking at the bindings, I can't find the compatible arm,psci-0.1. Where
> > does it come from?
> >
> > Also, you may want to check the compatible "arm,psci-1.0".
> Oh, yes, it's typo, for PSCI we have 3 compatible string:
> "arm,psci-1.0",
> "arm,psci-0.2",
> "arm,psci, ==> this one is PSCI-0.1
>
> And in this code, I should use "arm,psci-1.0".
Do you plan to support PSCI 0.1? FWIW, I think this is fine to not 
support it for now.
But I would spell it out and make sure you can either still boot with 
limited functionality or crash.
Cheers,

--
Julien Grall

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