| Message ID | 20180208192203.9556-3-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: PSCI 1.1 and SMCCC-1.1 support and XSA-254 variant 2 update | expand |
Hi Julien, On 08.02.18 21:21, Julien Grall wrote: > At the moment, Xen provides virtual PSCI interface compliant with 0.1 > and 0.2. Since them, the specification has been updated and the latest > version is 1.1 (see ARM DEN 0022D). > > From an implementation point of view, only PSCI_FEATURES is mandatory. > The rest is optional and can be left unimplemented for now. > > At the same time, the compatible for PSCI node have been updated to > expose "arm,psci-1.0". > > Signed-off-by: Julien Grall <julien.grall@arm.com> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: mirela.simonovic@aggios.com > > --- > We may want to provide a way for the toolstack to specify a PSCI > version. This could be useful if a guest is expecting a given > version. > > Changes in v2: > - Return v1.1 on GET_VERSION call as claimed by this patch > - Order by function ID the calls in FEATURES call > --- > tools/libxl/libxl_arm.c | 3 ++- > xen/arch/arm/domain_build.c | 1 + > xen/arch/arm/vpsci.c | 39 ++++++++++++++++++++++++++++++++++++++- > xen/include/asm-arm/perfc_defn.h | 1 + > xen/include/asm-arm/psci.h | 1 + > xen/include/asm-arm/vpsci.h | 2 +- > 6 files changed, 44 insertions(+), 3 deletions(-) > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > index 3e46554301..86f59c0d80 100644 > --- a/tools/libxl/libxl_arm.c > +++ b/tools/libxl/libxl_arm.c > @@ -410,7 +410,8 @@ static int make_psci_node(libxl__gc *gc, void *fdt) > res = fdt_begin_node(fdt, "psci"); > if (res) return res; > > - res = fdt_property_compat(gc, fdt, 2, "arm,psci-0.2","arm,psci"); > + res = fdt_property_compat(gc, fdt, 3, "arm,psci-1.0", > + "arm,psci-0.2", "arm,psci"); What about this place? Should it be "arm,psci-1.1"? > if (res) return res; > > res = fdt_property_string(fdt, "method", "hvc"); > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 155c952349..941688a2ce 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -637,6 +637,7 @@ static int make_psci_node(void *fdt, const struct dt_device_node *parent) > { > int res; > const char compat[] = > + "arm,psci-1.0""\0" > "arm,psci-0.2""\0" > "arm,psci"; > > diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c > index 6ab8ab64d0..e82b62db1a 100644 > --- a/xen/arch/arm/vpsci.c > +++ b/xen/arch/arm/vpsci.c > @@ -106,7 +106,11 @@ static int32_t do_psci_cpu_off(uint32_t power_state) > > static uint32_t do_psci_0_2_version(void) > { > - return PSCI_VERSION(0, 2); > + /* > + * PSCI is backward compatible from 0.2. So we can bump the version > + * without any issue. > + */ > + return PSCI_VERSION(1, 1); > } > > static register_t do_psci_0_2_cpu_suspend(uint32_t power_state, > @@ -191,6 +195,29 @@ static void do_psci_0_2_system_reset(void) > domain_shutdown(d,SHUTDOWN_reboot); > } > > +static int32_t do_psci_1_0_features(uint32_t psci_func_id) > +{ > + /* /!\ Ordered by function ID and not name */ > + switch ( psci_func_id ) > + { > + case PSCI_0_2_FN32_PSCI_VERSION: > + case PSCI_0_2_FN32_CPU_SUSPEND: > + case PSCI_0_2_FN64_CPU_SUSPEND: > + case PSCI_0_2_FN32_CPU_OFF: > + case PSCI_0_2_FN32_CPU_ON: > + case PSCI_0_2_FN64_CPU_ON: > + case PSCI_0_2_FN32_AFFINITY_INFO: > + case PSCI_0_2_FN64_AFFINITY_INFO: > + case PSCI_0_2_FN32_MIGRATE_INFO_TYPE: > + case PSCI_0_2_FN32_SYSTEM_OFF: > + case PSCI_0_2_FN32_SYSTEM_RESET: > + case PSCI_1_0_FN32_PSCI_FEATURES: > + return 0; > + default: > + return PSCI_NOT_SUPPORTED; > + } > +} > + > #define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val) > #define PSCI_ARG(reg, n) get_user_reg(reg, n) > > @@ -304,6 +331,16 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid) > PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); > return true; > } > + > + case PSCI_1_0_FN32_PSCI_FEATURES: > + { > + uint32_t psci_func_id = PSCI_ARG32(regs, 1); > + > + perfc_incr(vpsci_features); > + PSCI_SET_RESULT(regs, do_psci_1_0_features(psci_func_id)); > + return true; > + } > + > default: > return false; > } > diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h > index a7acb7d21c..87866264ca 100644 > --- a/xen/include/asm-arm/perfc_defn.h > +++ b/xen/include/asm-arm/perfc_defn.h > @@ -31,6 +31,7 @@ PERFCOUNTER(vpsci_system_off, "vpsci: system_off") > PERFCOUNTER(vpsci_system_reset, "vpsci: system_reset") > PERFCOUNTER(vpsci_cpu_suspend, "vpsci: cpu_suspend") > PERFCOUNTER(vpsci_cpu_affinity_info, "vpsci: cpu_affinity_info") > +PERFCOUNTER(vpsci_features, "vpsci: features") > > PERFCOUNTER(vgicd_reads, "vgicd: read") > PERFCOUNTER(vgicd_writes, "vgicd: write") > diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h > index becc9f9ded..e2629eed01 100644 > --- a/xen/include/asm-arm/psci.h > +++ b/xen/include/asm-arm/psci.h > @@ -40,6 +40,7 @@ void call_psci_system_reset(void); > #define PSCI_0_2_FN32_MIGRATE_INFO_TYPE PSCI_0_2_FN32(6) > #define PSCI_0_2_FN32_SYSTEM_OFF PSCI_0_2_FN32(8) > #define PSCI_0_2_FN32_SYSTEM_RESET PSCI_0_2_FN32(9) > +#define PSCI_1_0_FN32_PSCI_FEATURES PSCI_0_2_FN32(10) > > #define PSCI_0_2_FN64_CPU_SUSPEND PSCI_0_2_FN64(1) > #define PSCI_0_2_FN64_CPU_ON PSCI_0_2_FN64(3) > diff --git a/xen/include/asm-arm/vpsci.h b/xen/include/asm-arm/vpsci.h > index 035a41e812..0cca5e6830 100644 > --- a/xen/include/asm-arm/vpsci.h > +++ b/xen/include/asm-arm/vpsci.h > @@ -23,7 +23,7 @@ > #include <asm/psci.h> > > /* Number of function implemented by virtual PSCI (only 0.2 or later) */ > -#define VPSCI_NR_FUNCS 11 > +#define VPSCI_NR_FUNCS 12 > > /* Functions handle PSCI calls from the guests */ > bool do_vpsci_0_1_call(struct cpu_user_regs *regs, uint32_t fid); >
On 02/09/2018 04:07 PM, Volodymyr Babchuk wrote: > Hi Julien, Hi Volodymyr, > On 08.02.18 21:21, Julien Grall wrote: >> At the moment, Xen provides virtual PSCI interface compliant with 0.1 >> and 0.2. Since them, the specification has been updated and the latest >> version is 1.1 (see ARM DEN 0022D). >> >> From an implementation point of view, only PSCI_FEATURES is mandatory. >> The rest is optional and can be left unimplemented for now. >> >> At the same time, the compatible for PSCI node have been updated to >> expose "arm,psci-1.0". >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> Cc: Wei Liu <wei.liu2@citrix.com> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: mirela.simonovic@aggios.com >> >> --- >> We may want to provide a way for the toolstack to specify a PSCI >> version. This could be useful if a guest is expecting a given >> version. >> >> Changes in v2: >> - Return v1.1 on GET_VERSION call as claimed by this patch >> - Order by function ID the calls in FEATURES call >> --- >> tools/libxl/libxl_arm.c | 3 ++- >> xen/arch/arm/domain_build.c | 1 + >> xen/arch/arm/vpsci.c | 39 >> ++++++++++++++++++++++++++++++++++++++- >> xen/include/asm-arm/perfc_defn.h | 1 + >> xen/include/asm-arm/psci.h | 1 + >> xen/include/asm-arm/vpsci.h | 2 +- >> 6 files changed, 44 insertions(+), 3 deletions(-) >> >> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c >> index 3e46554301..86f59c0d80 100644 >> --- a/tools/libxl/libxl_arm.c >> +++ b/tools/libxl/libxl_arm.c >> @@ -410,7 +410,8 @@ static int make_psci_node(libxl__gc *gc, void *fdt) >> res = fdt_begin_node(fdt, "psci"); >> if (res) return res; >> - res = fdt_property_compat(gc, fdt, 2, "arm,psci-0.2","arm,psci"); >> + res = fdt_property_compat(gc, fdt, 3, "arm,psci-1.0", >> + "arm,psci-0.2", "arm,psci"); > What about this place? Should it be "arm,psci-1.1"? arm,psci-1.1 compatible string does not exist. Technically after 0.2 you should discover the PSCI version through GET_VERSION. So I am not entirely sure why arm,psci-1.0 compatible was added. From the documentation (Documentation/devicetree/bindings/arm/psci.txt), the compatibles means the PSCI implementation comply to a given version. Our implementation complies to 0.1, 0.2 and 1.0. So I have added 1.0 just in case a guest decides to check the compatible. Cheers,
On 09.02.18 18:13, Julien Grall wrote: > > > On 02/09/2018 04:07 PM, Volodymyr Babchuk wrote: >> Hi Julien, > > Hi Volodymyr, > >> On 08.02.18 21:21, Julien Grall wrote: >>> At the moment, Xen provides virtual PSCI interface compliant with 0.1 >>> and 0.2. Since them, the specification has been updated and the latest >>> version is 1.1 (see ARM DEN 0022D). >>> >>> From an implementation point of view, only PSCI_FEATURES is mandatory. >>> The rest is optional and can be left unimplemented for now. >>> >>> At the same time, the compatible for PSCI node have been updated to >>> expose "arm,psci-1.0". >>> >>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> Cc: Wei Liu <wei.liu2@citrix.com> >>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>> Cc: mirela.simonovic@aggios.com Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >>> --- >>> We may want to provide a way for the toolstack to specify a PSCI >>> version. This could be useful if a guest is expecting a given >>> version. >>> >>> Changes in v2: >>> - Return v1.1 on GET_VERSION call as claimed by this patch >>> - Order by function ID the calls in FEATURES call >>> --- >>> tools/libxl/libxl_arm.c | 3 ++- >>> xen/arch/arm/domain_build.c | 1 + >>> xen/arch/arm/vpsci.c | 39 >>> ++++++++++++++++++++++++++++++++++++++- >>> xen/include/asm-arm/perfc_defn.h | 1 + >>> xen/include/asm-arm/psci.h | 1 + >>> xen/include/asm-arm/vpsci.h | 2 +- >>> 6 files changed, 44 insertions(+), 3 deletions(-) >>> >>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c >>> index 3e46554301..86f59c0d80 100644 >>> --- a/tools/libxl/libxl_arm.c >>> +++ b/tools/libxl/libxl_arm.c >>> @@ -410,7 +410,8 @@ static int make_psci_node(libxl__gc *gc, void *fdt) >>> res = fdt_begin_node(fdt, "psci"); >>> if (res) return res; >>> - res = fdt_property_compat(gc, fdt, 2, "arm,psci-0.2","arm,psci"); >>> + res = fdt_property_compat(gc, fdt, 3, "arm,psci-1.0", >>> + "arm,psci-0.2", "arm,psci"); >> What about this place? Should it be "arm,psci-1.1"? > > arm,psci-1.1 compatible string does not exist. Technically after 0.2 you > should discover the PSCI version through GET_VERSION. So I am not > entirely sure why arm,psci-1.0 compatible was added. > > From the documentation > (Documentation/devicetree/bindings/arm/psci.txt), the compatibles means > the PSCI implementation comply to a given version. Our implementation > complies to 0.1, 0.2 and 1.0. So I have added 1.0 just in case a guest > decides to check the compatible. Okay, makes sense
On Thu, Feb 08, 2018 at 07:21:50PM +0000, Julien Grall wrote: > At the moment, Xen provides virtual PSCI interface compliant with 0.1 > and 0.2. Since them, the specification has been updated and the latest > version is 1.1 (see ARM DEN 0022D). > > From an implementation point of view, only PSCI_FEATURES is mandatory. > The rest is optional and can be left unimplemented for now. > > At the same time, the compatible for PSCI node have been updated to > expose "arm,psci-1.0". > > Signed-off-by: Julien Grall <julien.grall@arm.com> Acked-by: Wei Liu <wei.liu2@citrix.com> I will leave this patch to ARM committers.
Hi Julien, I've done pretty much the same work in parallel, but there are few additional minor changes I've made. Briefly, the difference is in return values that some already implemented functions should return starting from v1.0 (and even v0.2 errata). Please let me know whether you omitted that intentionally. I can submit these patches if you want. Currently I have few - one for each fix, easier to review. I guess all of them should be squashed with the patch you submitted. One more note - starting from v1.0, PSCI_NOT_SUPPORTED error should be returned for all optional functions that are not implemented. Is that the case? I.e. when there is no case for a particular function ID in do_vpsci_0_2_call the PSCI_NOT_SUPPORTED error will be returned? On 02/08/2018 08:21 PM, Julien Grall wrote: > At the moment, Xen provides virtual PSCI interface compliant with 0.1 > and 0.2. Since them, the specification has been updated and the latest > version is 1.1 (see ARM DEN 0022D). > > From an implementation point of view, only PSCI_FEATURES is mandatory. > The rest is optional and can be left unimplemented for now. > > At the same time, the compatible for PSCI node have been updated to > expose "arm,psci-1.0". > > Signed-off-by: Julien Grall <julien.grall@arm.com> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: mirela.simonovic@aggios.com > > --- > We may want to provide a way for the toolstack to specify a PSCI > version. This could be useful if a guest is expecting a given > version. > > Changes in v2: > - Return v1.1 on GET_VERSION call as claimed by this patch > - Order by function ID the calls in FEATURES call > --- > tools/libxl/libxl_arm.c | 3 ++- > xen/arch/arm/domain_build.c | 1 + > xen/arch/arm/vpsci.c | 39 ++++++++++++++++++++++++++++++++++++++- > xen/include/asm-arm/perfc_defn.h | 1 + > xen/include/asm-arm/psci.h | 1 + > xen/include/asm-arm/vpsci.h | 2 +- > 6 files changed, 44 insertions(+), 3 deletions(-) > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > index 3e46554301..86f59c0d80 100644 > --- a/tools/libxl/libxl_arm.c > +++ b/tools/libxl/libxl_arm.c > @@ -410,7 +410,8 @@ static int make_psci_node(libxl__gc *gc, void *fdt) > res = fdt_begin_node(fdt, "psci"); > if (res) return res; > > - res = fdt_property_compat(gc, fdt, 2, "arm,psci-0.2","arm,psci"); > + res = fdt_property_compat(gc, fdt, 3, "arm,psci-1.0", > + "arm,psci-0.2", "arm,psci"); > if (res) return res; > > res = fdt_property_string(fdt, "method", "hvc"); > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 155c952349..941688a2ce 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -637,6 +637,7 @@ static int make_psci_node(void *fdt, const struct dt_device_node *parent) > { > int res; > const char compat[] = > + "arm,psci-1.0""\0" > "arm,psci-0.2""\0" > "arm,psci"; > > diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c > index 6ab8ab64d0..e82b62db1a 100644 > --- a/xen/arch/arm/vpsci.c > +++ b/xen/arch/arm/vpsci.c > @@ -106,7 +106,11 @@ static int32_t do_psci_cpu_off(uint32_t power_state) > > static uint32_t do_psci_0_2_version(void) > { > - return PSCI_VERSION(0, 2); > + /* > + * PSCI is backward compatible from 0.2. So we can bump the version > + * without any issue. > + */ > + return PSCI_VERSION(1, 1); > } > > static register_t do_psci_0_2_cpu_suspend(uint32_t power_state, > @@ -191,6 +195,29 @@ static void do_psci_0_2_system_reset(void) > domain_shutdown(d,SHUTDOWN_reboot); > } > > +static int32_t do_psci_1_0_features(uint32_t psci_func_id) > +{ > + /* /!\ Ordered by function ID and not name */ > + switch ( psci_func_id ) > + { > + case PSCI_0_2_FN32_PSCI_VERSION: > + case PSCI_0_2_FN32_CPU_SUSPEND: > + case PSCI_0_2_FN64_CPU_SUSPEND: Just a note here - PSCI_FEATURES should return additional information just for CPU_SUSPEND (supported power state and mode). AFAIU, that value is also 0, so the return code should be fine. > + case PSCI_0_2_FN32_CPU_OFF: > + case PSCI_0_2_FN32_CPU_ON: > + case PSCI_0_2_FN64_CPU_ON: > + case PSCI_0_2_FN32_AFFINITY_INFO: > + case PSCI_0_2_FN64_AFFINITY_INFO: > + case PSCI_0_2_FN32_MIGRATE_INFO_TYPE: > + case PSCI_0_2_FN32_SYSTEM_OFF: > + case PSCI_0_2_FN32_SYSTEM_RESET: > + case PSCI_1_0_FN32_PSCI_FEATURES: > + return 0; > + default: > + return PSCI_NOT_SUPPORTED; > + } > +} > + > #define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val) > #define PSCI_ARG(reg, n) get_user_reg(reg, n) > > @@ -304,6 +331,16 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid) > PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); > return true; > } > + > + case PSCI_1_0_FN32_PSCI_FEATURES: > + { > + uint32_t psci_func_id = PSCI_ARG32(regs, 1); > + > + perfc_incr(vpsci_features); > + PSCI_SET_RESULT(regs, do_psci_1_0_features(psci_func_id)); > + return true; > + } > + > default: > return false; > } > diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h > index a7acb7d21c..87866264ca 100644 > --- a/xen/include/asm-arm/perfc_defn.h > +++ b/xen/include/asm-arm/perfc_defn.h > @@ -31,6 +31,7 @@ PERFCOUNTER(vpsci_system_off, "vpsci: system_off") > PERFCOUNTER(vpsci_system_reset, "vpsci: system_reset") > PERFCOUNTER(vpsci_cpu_suspend, "vpsci: cpu_suspend") > PERFCOUNTER(vpsci_cpu_affinity_info, "vpsci: cpu_affinity_info") > +PERFCOUNTER(vpsci_features, "vpsci: features") > > PERFCOUNTER(vgicd_reads, "vgicd: read") > PERFCOUNTER(vgicd_writes, "vgicd: write") > diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h > index becc9f9ded..e2629eed01 100644 > --- a/xen/include/asm-arm/psci.h > +++ b/xen/include/asm-arm/psci.h > @@ -40,6 +40,7 @@ void call_psci_system_reset(void); > #define PSCI_0_2_FN32_MIGRATE_INFO_TYPE PSCI_0_2_FN32(6) > #define PSCI_0_2_FN32_SYSTEM_OFF PSCI_0_2_FN32(8) > #define PSCI_0_2_FN32_SYSTEM_RESET PSCI_0_2_FN32(9) > +#define PSCI_1_0_FN32_PSCI_FEATURES PSCI_0_2_FN32(10) > > #define PSCI_0_2_FN64_CPU_SUSPEND PSCI_0_2_FN64(1) > #define PSCI_0_2_FN64_CPU_ON PSCI_0_2_FN64(3) > diff --git a/xen/include/asm-arm/vpsci.h b/xen/include/asm-arm/vpsci.h > index 035a41e812..0cca5e6830 100644 > --- a/xen/include/asm-arm/vpsci.h > +++ b/xen/include/asm-arm/vpsci.h > @@ -23,7 +23,7 @@ > #include <asm/psci.h> > > /* Number of function implemented by virtual PSCI (only 0.2 or later) */ > -#define VPSCI_NR_FUNCS 11 > +#define VPSCI_NR_FUNCS 12 > > /* Functions handle PSCI calls from the guests */ > bool do_vpsci_0_1_call(struct cpu_user_regs *regs, uint32_t fid);
On 12/02/2018 20:12, Mirela Simonovic wrote: > Hi Julien, Hi Mirela, Thank you for the review. > I've done pretty much the same work in parallel, but there are few > additional minor changes I've made. Briefly, the difference is in return > values that some already implemented functions should return starting > from v1.0 (and even v0.2 errata). Please let me know whether you omitted > that intentionally. Could you give a bit more details here? From a brief look we don't seem to implement correctly: - CPU_OFF: PSCI_DENY should be return on failure (though it should never fail in Xen case) and the check on the vCPU state is pointless. - MIGRATE_INFO_TYPE: should technically return int32_t instead of uint32_t. That not really matter for now. If you speak about denying SMC64 call from AArch32, then this is already done in vsmccc.c (see vsmccc_call). > I can submit these patches if you want. Currently I have few - one for > each fix, easier to review. I guess all of them should be squashed with > the patch you submitted. > > One more note - starting from v1.0, PSCI_NOT_SUPPORTED error should be > returned for all optional functions that are not implemented. Is that > the case? I.e. when there is no case for a particular function ID in > do_vpsci_0_2_call the PSCI_NOT_SUPPORTED error will be returned? This is done by vmsccc_handle_call(). See set_user_regs(regs, 0, ARM_SMCC_ERR_UNKNOWN_FUNCTION) which is equivalent to PSCI_NOT_SUPPORTED. [...] >> +static int32_t do_psci_1_0_features(uint32_t psci_func_id) >> +{ >> + /* /!\ Ordered by function ID and not name */ >> + switch ( psci_func_id ) >> + { >> + case PSCI_0_2_FN32_PSCI_VERSION: >> + case PSCI_0_2_FN32_CPU_SUSPEND: >> + case PSCI_0_2_FN64_CPU_SUSPEND: > > Just a note here - PSCI_FEATURES should return additional information > just for CPU_SUSPEND (supported power state and mode). AFAIU, that value > is also 0, so the return code should be fine. I think so, from what I understood this is inline with CPU_SUSPEND only supports 0.2 format. Cheers,
Hi Julien, On 02/12/2018 10:41 PM, Julien Grall wrote: > > > On 12/02/2018 20:12, Mirela Simonovic wrote: >> Hi Julien, > > Hi Mirela, > > Thank you for the review. > >> I've done pretty much the same work in parallel, but there are few >> additional minor changes I've made. Briefly, the difference is in >> return values that some already implemented functions should return >> starting from v1.0 (and even v0.2 errata). Please let me know whether >> you omitted that intentionally. > > Could you give a bit more details here? From a brief look we don't > seem to implement correctly: > - CPU_OFF: PSCI_DENY should be return on failure (though it should > never fail in Xen case) and the check on the vCPU state is pointless. I believe CPU_OFF is fine today, it never returns. > - MIGRATE_INFO_TYPE: should technically return int32_t instead of > uint32_t. That not really matter for now. > > If you speak about denying SMC64 call from AArch32, then this is > already done in vsmccc.c (see vsmccc_call). Agreed on above, there are 2 more: 1. MIGRATE_INFO_TYPE should return PSCI_NOT_SUPPORTED instead PSCI_0_2_TOS_MP_OR_NOT_PRESENT. The function is effectively not implemented, but in v0.2 it was mandatory, so it couldn't return PSCI_NOT_SUPPORTED (I guess this was some kind of a workaround). Since v0.2 errata and v1.0 release the function is made optional and it should return "not supported" error - just removing the function should be fine (and mismatching return type issue would be gone). 2. A new error code has been introduced in PSCI v1.0: PSCI_INVALID_ADDRESS. This error should be returned by PSCI functions which receive an address as the argument when the provided address is incorrect. In implementation in Xen this affects CPU_ON and CPU_SUSPEND. CPU_ON today returns invalid parameter error and that needs to be replaced with invalid address error. I'm not sure for CPU_SUSPEND since its implementation doesn't use/check any of the arguments today... Thanks, Mirela > >> I can submit these patches if you want. Currently I have few - one >> for each fix, easier to review. I guess all of them should be >> squashed with the patch you submitted. >> >> One more note - starting from v1.0, PSCI_NOT_SUPPORTED error should >> be returned for all optional functions that are not implemented. Is >> that the case? I.e. when there is no case for a particular function >> ID in do_vpsci_0_2_call the PSCI_NOT_SUPPORTED error will be returned? > > This is done by vmsccc_handle_call(). > See set_user_regs(regs, 0, ARM_SMCC_ERR_UNKNOWN_FUNCTION) which is > equivalent to PSCI_NOT_SUPPORTED. > > [...] > >>> +static int32_t do_psci_1_0_features(uint32_t psci_func_id) >>> +{ >>> + /* /!\ Ordered by function ID and not name */ >>> + switch ( psci_func_id ) >>> + { >>> + case PSCI_0_2_FN32_PSCI_VERSION: >>> + case PSCI_0_2_FN32_CPU_SUSPEND: >>> + case PSCI_0_2_FN64_CPU_SUSPEND: >> >> Just a note here - PSCI_FEATURES should return additional information >> just for CPU_SUSPEND (supported power state and mode). AFAIU, that >> value is also 0, so the return code should be fine. > > I think so, from what I understood this is inline with CPU_SUSPEND > only supports 0.2 format. > > Cheers, >
On 12/02/2018 23:16, Mirela Simonovic wrote: > Hi Julien, Hi, > On 02/12/2018 10:41 PM, Julien Grall wrote: >> >> >> On 12/02/2018 20:12, Mirela Simonovic wrote: >>> Hi Julien, >> >> Hi Mirela, >> >> Thank you for the review. >> >>> I've done pretty much the same work in parallel, but there are few >>> additional minor changes I've made. Briefly, the difference is in >>> return values that some already implemented functions should return >>> starting from v1.0 (and even v0.2 errata). Please let me know whether >>> you omitted that intentionally. >> >> Could you give a bit more details here? From a brief look we don't >> seem to implement correctly: >> - CPU_OFF: PSCI_DENY should be return on failure (though it should >> never fail in Xen case) and the check on the vCPU state is pointless. > > I believe CPU_OFF is fine today, it never returns. > >> - MIGRATE_INFO_TYPE: should technically return int32_t instead of >> uint32_t. That not really matter for now. >> >> If you speak about denying SMC64 call from AArch32, then this is >> already done in vsmccc.c (see vsmccc_call). > > Agreed on above, there are 2 more: > > 1. MIGRATE_INFO_TYPE should return PSCI_NOT_SUPPORTED instead > PSCI_0_2_TOS_MP_OR_NOT_PRESENT. The function is effectively not > implemented, but in v0.2 it was mandatory, so it couldn't return > PSCI_NOT_SUPPORTED (I guess this was some kind of a workaround). Since > v0.2 errata and v1.0 release the function is made optional and it should > return "not supported" error - just removing the function should be fine > (and mismatching return type issue would be gone). Looking at the spec: "2 Trusted OS is either not present or does not require migration. A system of this type does not require the caller to use the MIGRATE function. MIGRATE function calls return NOT_SUPPORTED." So returning 2 in our case seems to be valid. > > 2. A new error code has been introduced in PSCI v1.0: > PSCI_INVALID_ADDRESS. This error should be returned by PSCI functions > which receive an address as the argument when the provided address is > incorrect. In implementation in Xen this affects CPU_ON and CPU_SUSPEND. > CPU_ON today returns invalid parameter error and that needs to be > replaced with invalid address error. I'm not sure for CPU_SUSPEND since > its implementation doesn't use/check any of the arguments today... I disagree, not all PSCI_INVALID_PARAMETERS should be replaced by PSCI_INVALID_ADDRESS. They have two distinct meaning. However, I am not sure where we would need to use it in Xen. The error is described as "INVALID_ADDRESS is returned when the entry point address is known by the implementation to be invalid, because it is in a range that is known not to be available to the caller." The only potential one would be the check on is_thumb, but even there it does not match the description. The range is still available to the guest. I think that check should just be dropped. Cheers,
Hi Julien, On 02/13/2018 12:44 AM, Julien Grall wrote: > > > On 12/02/2018 23:16, Mirela Simonovic wrote: >> Hi Julien, > > Hi, > >> On 02/12/2018 10:41 PM, Julien Grall wrote: >>> >>> >>> On 12/02/2018 20:12, Mirela Simonovic wrote: >>>> Hi Julien, >>> >>> Hi Mirela, >>> >>> Thank you for the review. >>> >>>> I've done pretty much the same work in parallel, but there are few >>>> additional minor changes I've made. Briefly, the difference is in >>>> return values that some already implemented functions should return >>>> starting from v1.0 (and even v0.2 errata). Please let me know >>>> whether you omitted that intentionally. >>> >>> Could you give a bit more details here? From a brief look we don't >>> seem to implement correctly: >>> - CPU_OFF: PSCI_DENY should be return on failure (though it >>> should never fail in Xen case) and the check on the vCPU state is >>> pointless. >> >> I believe CPU_OFF is fine today, it never returns. >> >>> - MIGRATE_INFO_TYPE: should technically return int32_t instead >>> of uint32_t. That not really matter for now. >>> >>> If you speak about denying SMC64 call from AArch32, then this is >>> already done in vsmccc.c (see vsmccc_call). >> >> Agreed on above, there are 2 more: >> >> 1. MIGRATE_INFO_TYPE should return PSCI_NOT_SUPPORTED instead >> PSCI_0_2_TOS_MP_OR_NOT_PRESENT. The function is effectively not >> implemented, but in v0.2 it was mandatory, so it couldn't return >> PSCI_NOT_SUPPORTED (I guess this was some kind of a workaround). >> Since v0.2 errata and v1.0 release the function is made optional and >> it should return "not supported" error - just removing the function >> should be fine (and mismatching return type issue would be gone). > > Looking at the spec: > > "2 Trusted OS is either not present or does not require migration. A > system of this type does not require the caller to use the MIGRATE > function. MIGRATE function calls return NOT_SUPPORTED." > > So returning 2 in our case seems to be valid. > >> >> 2. A new error code has been introduced in PSCI v1.0: >> PSCI_INVALID_ADDRESS. This error should be returned by PSCI functions >> which receive an address as the argument when the provided address is >> incorrect. In implementation in Xen this affects CPU_ON and >> CPU_SUSPEND. CPU_ON today returns invalid parameter error and that >> needs to be replaced with invalid address error. I'm not sure for >> CPU_SUSPEND since its implementation doesn't use/check any of the >> arguments today... > I disagree, not all PSCI_INVALID_PARAMETERS should be replaced by > PSCI_INVALID_ADDRESS. They have two distinct meaning. However, I am > not sure where we would need to use it in Xen. The error is described > as "INVALID_ADDRESS is returned when the entry point address is known > by the implementation to be invalid, because it is in a range that is > known not to be available to the caller." > > The only potential one would be the check on is_thumb, but even there > it does not match the description. The range is still available to the > guest. I think that check should just be dropped. To be more specific, I was thinking that in xen/arch/arm/vpsci.c line 41 for psci version other than 0.1 the PSCI_INVALID_ADDRESS error should be returned instead PSCI_INVALID_PARAMETERS. Cheers, Mirela > > Cheers, >
On 14/02/18 19:14, Mirela Simonovic wrote: > Hi Julien, Hi, > On 02/13/2018 12:44 AM, Julien Grall wrote: >> >> >> On 12/02/2018 23:16, Mirela Simonovic wrote: >>> Hi Julien, >> >> Hi, >> >>> On 02/12/2018 10:41 PM, Julien Grall wrote: >>>> >>>> >>>> On 12/02/2018 20:12, Mirela Simonovic wrote: >>>>> Hi Julien, >>>> >>>> Hi Mirela, >>>> >>>> Thank you for the review. >>>> >>>>> I've done pretty much the same work in parallel, but there are few >>>>> additional minor changes I've made. Briefly, the difference is in >>>>> return values that some already implemented functions should return >>>>> starting from v1.0 (and even v0.2 errata). Please let me know >>>>> whether you omitted that intentionally. >>>> >>>> Could you give a bit more details here? From a brief look we don't >>>> seem to implement correctly: >>>> - CPU_OFF: PSCI_DENY should be return on failure (though it >>>> should never fail in Xen case) and the check on the vCPU state is >>>> pointless. >>> >>> I believe CPU_OFF is fine today, it never returns. >>> >>>> - MIGRATE_INFO_TYPE: should technically return int32_t instead >>>> of uint32_t. That not really matter for now. >>>> >>>> If you speak about denying SMC64 call from AArch32, then this is >>>> already done in vsmccc.c (see vsmccc_call). >>> >>> Agreed on above, there are 2 more: >>> >>> 1. MIGRATE_INFO_TYPE should return PSCI_NOT_SUPPORTED instead >>> PSCI_0_2_TOS_MP_OR_NOT_PRESENT. The function is effectively not >>> implemented, but in v0.2 it was mandatory, so it couldn't return >>> PSCI_NOT_SUPPORTED (I guess this was some kind of a workaround). >>> Since v0.2 errata and v1.0 release the function is made optional and >>> it should return "not supported" error - just removing the function >>> should be fine (and mismatching return type issue would be gone). >> >> Looking at the spec: >> >> "2 Trusted OS is either not present or does not require migration. A >> system of this type does not require the caller to use the MIGRATE >> function. MIGRATE function calls return NOT_SUPPORTED." >> >> So returning 2 in our case seems to be valid. >> >>> >>> 2. A new error code has been introduced in PSCI v1.0: >>> PSCI_INVALID_ADDRESS. This error should be returned by PSCI functions >>> which receive an address as the argument when the provided address is >>> incorrect. In implementation in Xen this affects CPU_ON and >>> CPU_SUSPEND. CPU_ON today returns invalid parameter error and that >>> needs to be replaced with invalid address error. I'm not sure for >>> CPU_SUSPEND since its implementation doesn't use/check any of the >>> arguments today... >> I disagree, not all PSCI_INVALID_PARAMETERS should be replaced by >> PSCI_INVALID_ADDRESS. They have two distinct meaning. However, I am >> not sure where we would need to use it in Xen. The error is described >> as "INVALID_ADDRESS is returned when the entry point address is known >> by the implementation to be invalid, because it is in a range that is >> known not to be available to the caller." >> >> The only potential one would be the check on is_thumb, but even there >> it does not match the description. The range is still available to the >> guest. I think that check should just be dropped. > > To be more specific, I was thinking that in xen/arch/arm/vpsci.c line 41 > for psci version other than 0.1 the PSCI_INVALID_ADDRESS error should be > returned instead PSCI_INVALID_PARAMETERS. This is exactly the place I was speaking in my previous e-mail. I am not entirely convinced we should keep the check or even switch the return to PSCI_INVALID_PARAMETERS as the usage does not entirely match the error description. Cheers,
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 3e46554301..86f59c0d80 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -410,7 +410,8 @@ static int make_psci_node(libxl__gc *gc, void *fdt) res = fdt_begin_node(fdt, "psci"); if (res) return res; - res = fdt_property_compat(gc, fdt, 2, "arm,psci-0.2","arm,psci"); + res = fdt_property_compat(gc, fdt, 3, "arm,psci-1.0", + "arm,psci-0.2", "arm,psci"); if (res) return res; res = fdt_property_string(fdt, "method", "hvc"); diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 155c952349..941688a2ce 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -637,6 +637,7 @@ static int make_psci_node(void *fdt, const struct dt_device_node *parent) { int res; const char compat[] = + "arm,psci-1.0""\0" "arm,psci-0.2""\0" "arm,psci"; diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c index 6ab8ab64d0..e82b62db1a 100644 --- a/xen/arch/arm/vpsci.c +++ b/xen/arch/arm/vpsci.c @@ -106,7 +106,11 @@ static int32_t do_psci_cpu_off(uint32_t power_state) static uint32_t do_psci_0_2_version(void) { - return PSCI_VERSION(0, 2); + /* + * PSCI is backward compatible from 0.2. So we can bump the version + * without any issue. + */ + return PSCI_VERSION(1, 1); } static register_t do_psci_0_2_cpu_suspend(uint32_t power_state, @@ -191,6 +195,29 @@ static void do_psci_0_2_system_reset(void) domain_shutdown(d,SHUTDOWN_reboot); } +static int32_t do_psci_1_0_features(uint32_t psci_func_id) +{ + /* /!\ Ordered by function ID and not name */ + switch ( psci_func_id ) + { + case PSCI_0_2_FN32_PSCI_VERSION: + case PSCI_0_2_FN32_CPU_SUSPEND: + case PSCI_0_2_FN64_CPU_SUSPEND: + case PSCI_0_2_FN32_CPU_OFF: + case PSCI_0_2_FN32_CPU_ON: + case PSCI_0_2_FN64_CPU_ON: + case PSCI_0_2_FN32_AFFINITY_INFO: + case PSCI_0_2_FN64_AFFINITY_INFO: + case PSCI_0_2_FN32_MIGRATE_INFO_TYPE: + case PSCI_0_2_FN32_SYSTEM_OFF: + case PSCI_0_2_FN32_SYSTEM_RESET: + case PSCI_1_0_FN32_PSCI_FEATURES: + return 0; + default: + return PSCI_NOT_SUPPORTED; + } +} + #define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val) #define PSCI_ARG(reg, n) get_user_reg(reg, n) @@ -304,6 +331,16 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid) PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); return true; } + + case PSCI_1_0_FN32_PSCI_FEATURES: + { + uint32_t psci_func_id = PSCI_ARG32(regs, 1); + + perfc_incr(vpsci_features); + PSCI_SET_RESULT(regs, do_psci_1_0_features(psci_func_id)); + return true; + } + default: return false; } diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h index a7acb7d21c..87866264ca 100644 --- a/xen/include/asm-arm/perfc_defn.h +++ b/xen/include/asm-arm/perfc_defn.h @@ -31,6 +31,7 @@ PERFCOUNTER(vpsci_system_off, "vpsci: system_off") PERFCOUNTER(vpsci_system_reset, "vpsci: system_reset") PERFCOUNTER(vpsci_cpu_suspend, "vpsci: cpu_suspend") PERFCOUNTER(vpsci_cpu_affinity_info, "vpsci: cpu_affinity_info") +PERFCOUNTER(vpsci_features, "vpsci: features") PERFCOUNTER(vgicd_reads, "vgicd: read") PERFCOUNTER(vgicd_writes, "vgicd: write") diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h index becc9f9ded..e2629eed01 100644 --- a/xen/include/asm-arm/psci.h +++ b/xen/include/asm-arm/psci.h @@ -40,6 +40,7 @@ void call_psci_system_reset(void); #define PSCI_0_2_FN32_MIGRATE_INFO_TYPE PSCI_0_2_FN32(6) #define PSCI_0_2_FN32_SYSTEM_OFF PSCI_0_2_FN32(8) #define PSCI_0_2_FN32_SYSTEM_RESET PSCI_0_2_FN32(9) +#define PSCI_1_0_FN32_PSCI_FEATURES PSCI_0_2_FN32(10) #define PSCI_0_2_FN64_CPU_SUSPEND PSCI_0_2_FN64(1) #define PSCI_0_2_FN64_CPU_ON PSCI_0_2_FN64(3) diff --git a/xen/include/asm-arm/vpsci.h b/xen/include/asm-arm/vpsci.h index 035a41e812..0cca5e6830 100644 --- a/xen/include/asm-arm/vpsci.h +++ b/xen/include/asm-arm/vpsci.h @@ -23,7 +23,7 @@ #include <asm/psci.h> /* Number of function implemented by virtual PSCI (only 0.2 or later) */ -#define VPSCI_NR_FUNCS 11 +#define VPSCI_NR_FUNCS 12 /* Functions handle PSCI calls from the guests */ bool do_vpsci_0_1_call(struct cpu_user_regs *regs, uint32_t fid);
At the moment, Xen provides virtual PSCI interface compliant with 0.1 and 0.2. Since them, the specification has been updated and the latest version is 1.1 (see ARM DEN 0022D). From an implementation point of view, only PSCI_FEATURES is mandatory. The rest is optional and can be left unimplemented for now. At the same time, the compatible for PSCI node have been updated to expose "arm,psci-1.0". Signed-off-by: Julien Grall <julien.grall@arm.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: mirela.simonovic@aggios.com --- We may want to provide a way for the toolstack to specify a PSCI version. This could be useful if a guest is expecting a given version. Changes in v2: - Return v1.1 on GET_VERSION call as claimed by this patch - Order by function ID the calls in FEATURES call --- tools/libxl/libxl_arm.c | 3 ++- xen/arch/arm/domain_build.c | 1 + xen/arch/arm/vpsci.c | 39 ++++++++++++++++++++++++++++++++++++++- xen/include/asm-arm/perfc_defn.h | 1 + xen/include/asm-arm/psci.h | 1 + xen/include/asm-arm/vpsci.h | 2 +- 6 files changed, 44 insertions(+), 3 deletions(-)