| Message ID | 20241218101142.1552618-2-andrei.cherechesu@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Add support for S32CC platforms and LINFlexD UART | expand |
On 18/12/2024 11:11, Andrei Cherechesu (OSS) wrote: > > > From: Andrei Cherechesu <andrei.cherechesu@nxp.com> > > Introduce the SCMI-SMC layer to have some basic degree of > awareness about SCMI calls that are based on the ARM System > Control and Management Interface (SCMI) specification (DEN0056E). > > The SCMI specification includes various protocols for managing > system-level resources, such as: clocks, pins, reset, system power, > power domains, performance domains, etc. The clients are named > "SCMI agents" and the server is named "SCMI platform". > > Only support the shared-memory based transport with SMCs as > the doorbell mechanism for notifying the platform. Also, this > implementation only handles the "arm,scmi-smc" compatible, > requiring the following properties: > - "arm,smc-id" (unique SMC ID) > - "shmem" (one or more phandles pointing to shmem zones > for each channel) > > The initialization is done as initcall, since we need > SMCs, and PSCI should already probe EL3 FW for SMCCC support. > If no "arm,scmi-smc" compatible node is found in the host > DT, the initialization fails silently, as it's not mandatory. > Otherwise, we get the 'arm,smc-id' DT property from the node, > to know the SCMI SMC ID we handle. The 'shmem' memory ranges > are not validated, as the SMC calls are only passed through > to EL3 FW if coming from the hardware domain. > > Create a new 'firmware' folder to keep the SCMI code separate > from the generic ARM code. > > Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/arm/Kconfig | 2 + > xen/arch/arm/Makefile | 1 + > xen/arch/arm/firmware/Kconfig | 13 ++ > xen/arch/arm/firmware/Makefile | 1 + > xen/arch/arm/firmware/scmi-smc.c | 166 +++++++++++++++++++ > xen/arch/arm/include/asm/firmware/scmi-smc.h | 46 +++++ > 6 files changed, 229 insertions(+) > create mode 100644 xen/arch/arm/firmware/Kconfig > create mode 100644 xen/arch/arm/firmware/Makefile > create mode 100644 xen/arch/arm/firmware/scmi-smc.c > create mode 100644 xen/arch/arm/include/asm/firmware/scmi-smc.h > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index 604aba4996..23dc7162a7 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -271,6 +271,8 @@ config PARTIAL_EMULATION > not been emulated to their complete functionality. Enabling this might > result in unwanted/non-spec compliant behavior. > > +source "arch/arm/firmware/Kconfig" > + > endmenu > > menu "ARM errata workaround via the alternative framework" > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index e4ad1ce851..8c696c2011 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -6,6 +6,7 @@ obj-$(CONFIG_HAS_PCI) += pci/ > ifneq ($(CONFIG_NO_PLAT),y) > obj-y += platforms/ > endif > +obj-y += firmware/ > obj-$(CONFIG_TEE) += tee/ > obj-$(CONFIG_HAS_VPCI) += vpci.o > > diff --git a/xen/arch/arm/firmware/Kconfig b/xen/arch/arm/firmware/Kconfig > new file mode 100644 > index 0000000000..817da745fd > --- /dev/null > +++ b/xen/arch/arm/firmware/Kconfig > @@ -0,0 +1,13 @@ > +menu "Firmware Drivers" > + > +config SCMI_SMC > + bool "Forward SCMI over SMC calls from hwdom to EL3 firmware" > + default y > + help > + This option enables basic awareness for SCMI calls using SMC as > + doorbell mechanism and Shared Memory for transport ("arm,scmi-smc" > + compatible only). The value of "arm,smc-id" DT property from SCMI > + firmware node is used to trap and forward corresponding SCMI SMCs > + to firmware running at EL3, for calls coming from the hardware domain. > + > +endmenu > diff --git a/xen/arch/arm/firmware/Makefile b/xen/arch/arm/firmware/Makefile > new file mode 100644 > index 0000000000..a5e4542666 > --- /dev/null > +++ b/xen/arch/arm/firmware/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o > diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c > new file mode 100644 > index 0000000000..62657308d6 > --- /dev/null > +++ b/xen/arch/arm/firmware/scmi-smc.c > @@ -0,0 +1,166 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * xen/arch/arm/firmware/scmi-smc.c > + * > + * ARM System Control and Management Interface (SCMI) over SMC > + * Generic handling layer > + * > + * Andrei Cherechesu <andrei.cherechesu@nxp.com> > + * Copyright 2024 NXP > + */ > + > +#include <xen/acpi.h> > +#include <xen/device_tree.h> > +#include <xen/errno.h> > +#include <xen/init.h> > +#include <xen/sched.h> > +#include <xen/types.h> > + > +#include <asm/smccc.h> > +#include <asm/firmware/scmi-smc.h> > + > +#define SCMI_SMC_ID_PROP "arm,smc-id" > + > +static bool __ro_after_init scmi_support; I don't understand the need for this variable. IMO it's useless, given that in next patch you do: ... if ( scmi_is_enabled() ) return scmi_handle_smc(regs); return false; which could simply be changed to: ... return scmi_handle_smc(regs); and the variable, functions for it, etc. could be removed which would simplify the code. [...] > +err: CODING_STYLE: this should be indented with one space. ~Michal
Hi Michal, Thank you for the review. On 18/12/2024 16:26, Michal Orzel wrote: > > On 18/12/2024 11:11, Andrei Cherechesu (OSS) wrote: >> >> From: Andrei Cherechesu <andrei.cherechesu@nxp.com> >> >> Introduce the SCMI-SMC layer to have some basic degree of >> awareness about SCMI calls that are based on the ARM System >> Control and Management Interface (SCMI) specification (DEN0056E). >> >> The SCMI specification includes various protocols for managing >> system-level resources, such as: clocks, pins, reset, system power, >> power domains, performance domains, etc. The clients are named >> "SCMI agents" and the server is named "SCMI platform". >> >> Only support the shared-memory based transport with SMCs as >> the doorbell mechanism for notifying the platform. Also, this >> implementation only handles the "arm,scmi-smc" compatible, >> requiring the following properties: >> - "arm,smc-id" (unique SMC ID) >> - "shmem" (one or more phandles pointing to shmem zones >> for each channel) >> >> The initialization is done as initcall, since we need >> SMCs, and PSCI should already probe EL3 FW for SMCCC support. >> If no "arm,scmi-smc" compatible node is found in the host >> DT, the initialization fails silently, as it's not mandatory. >> Otherwise, we get the 'arm,smc-id' DT property from the node, >> to know the SCMI SMC ID we handle. The 'shmem' memory ranges >> are not validated, as the SMC calls are only passed through >> to EL3 FW if coming from the hardware domain. >> >> Create a new 'firmware' folder to keep the SCMI code separate >> from the generic ARM code. >> >> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com> >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >> --- >> xen/arch/arm/Kconfig | 2 + >> xen/arch/arm/Makefile | 1 + >> xen/arch/arm/firmware/Kconfig | 13 ++ >> xen/arch/arm/firmware/Makefile | 1 + >> xen/arch/arm/firmware/scmi-smc.c | 166 +++++++++++++++++++ >> xen/arch/arm/include/asm/firmware/scmi-smc.h | 46 +++++ >> 6 files changed, 229 insertions(+) >> create mode 100644 xen/arch/arm/firmware/Kconfig >> create mode 100644 xen/arch/arm/firmware/Makefile >> create mode 100644 xen/arch/arm/firmware/scmi-smc.c >> create mode 100644 xen/arch/arm/include/asm/firmware/scmi-smc.h >> >> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig >> index 604aba4996..23dc7162a7 100644 >> --- a/xen/arch/arm/Kconfig >> +++ b/xen/arch/arm/Kconfig >> @@ -271,6 +271,8 @@ config PARTIAL_EMULATION >> not been emulated to their complete functionality. Enabling this might >> result in unwanted/non-spec compliant behavior. >> >> +source "arch/arm/firmware/Kconfig" >> + >> endmenu >> >> menu "ARM errata workaround via the alternative framework" >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >> index e4ad1ce851..8c696c2011 100644 >> --- a/xen/arch/arm/Makefile >> +++ b/xen/arch/arm/Makefile >> @@ -6,6 +6,7 @@ obj-$(CONFIG_HAS_PCI) += pci/ >> ifneq ($(CONFIG_NO_PLAT),y) >> obj-y += platforms/ >> endif >> +obj-y += firmware/ >> obj-$(CONFIG_TEE) += tee/ >> obj-$(CONFIG_HAS_VPCI) += vpci.o >> >> diff --git a/xen/arch/arm/firmware/Kconfig b/xen/arch/arm/firmware/Kconfig >> new file mode 100644 >> index 0000000000..817da745fd >> --- /dev/null >> +++ b/xen/arch/arm/firmware/Kconfig >> @@ -0,0 +1,13 @@ >> +menu "Firmware Drivers" >> + >> +config SCMI_SMC >> + bool "Forward SCMI over SMC calls from hwdom to EL3 firmware" >> + default y >> + help >> + This option enables basic awareness for SCMI calls using SMC as >> + doorbell mechanism and Shared Memory for transport ("arm,scmi-smc" >> + compatible only). The value of "arm,smc-id" DT property from SCMI >> + firmware node is used to trap and forward corresponding SCMI SMCs >> + to firmware running at EL3, for calls coming from the hardware domain. >> + >> +endmenu >> diff --git a/xen/arch/arm/firmware/Makefile b/xen/arch/arm/firmware/Makefile >> new file mode 100644 >> index 0000000000..a5e4542666 >> --- /dev/null >> +++ b/xen/arch/arm/firmware/Makefile >> @@ -0,0 +1 @@ >> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o >> diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c >> new file mode 100644 >> index 0000000000..62657308d6 >> --- /dev/null >> +++ b/xen/arch/arm/firmware/scmi-smc.c >> @@ -0,0 +1,166 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * xen/arch/arm/firmware/scmi-smc.c >> + * >> + * ARM System Control and Management Interface (SCMI) over SMC >> + * Generic handling layer >> + * >> + * Andrei Cherechesu <andrei.cherechesu@nxp.com> >> + * Copyright 2024 NXP >> + */ >> + >> +#include <xen/acpi.h> >> +#include <xen/device_tree.h> >> +#include <xen/errno.h> >> +#include <xen/init.h> >> +#include <xen/sched.h> >> +#include <xen/types.h> >> + >> +#include <asm/smccc.h> >> +#include <asm/firmware/scmi-smc.h> >> + >> +#define SCMI_SMC_ID_PROP "arm,smc-id" >> + >> +static bool __ro_after_init scmi_support; > I don't understand the need for this variable. IMO it's useless, given that in next patch you do: > > ... > if ( scmi_is_enabled() ) > return scmi_handle_smc(regs); > > return false; > > which could simply be changed to: > ... > return scmi_handle_smc(regs); > > and the variable, functions for it, etc. could be removed which would simplify the code. > > [...] Well, I agree that the code would maybe be simpler, but that means we would call `scmi_handle_smc()` both when SCMI-SMC layer is initialized and when it is not. That then means that if `scmi_handle_smc()` returns false, we won't know in the caller if the SCMI-SMC layer is not initialized at all or if it is initialized, but the SMC request is invalid (it does not have the SMC ID we expect). Do we need to, though? But even if we're not exporting the internal status of the SCMI layer (enabled/disabled), the driver itself needs to have an internal state to to know if it's been correctly initialized or not, right? Otherwise, how would we know internally if we can actually handle a SCMI request or not? Well, we could manage to base this mechanism entirely on the `scmi_smc_id` variable (i.e., if `scmi_smc_id` is not 0, then we know the SCMI-SMC layer is initialized). This could work as long as this handler is not called in a situation where the ID of the SMC we're trapping could actually be 0. And it currently would be not, since FID == 0 is reserved for Arm Architecture SMCs. So, I agree that it makes the code simpler in some sense, but it will add some underlying assumptions and make it less explicit. I'm happy to implement your suggested changes. But I need to know that I understood correctly your comments and that you agree with the implications I've stated above. > >> +err: > CODING_STYLE: this should be indented with one space. Will fix. > > ~Michal > Regards, Andrei Cherechesu
On 18/12/2024 10:11 am, Andrei Cherechesu (OSS) wrote: > diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c > new file mode 100644 > index 0000000000..62657308d6 > --- /dev/null > +++ b/xen/arch/arm/firmware/scmi-smc.c > +bool scmi_handle_smc(struct cpu_user_regs *regs) > +{ > + uint32_t fid = (uint32_t)get_user_reg(regs, 0); > + struct arm_smccc_res res; > + > + if ( !scmi_is_valid_smc_id(fid) ) > + return false; > + > + /* Only the hardware domain should use SCMI calls */ > + if ( !is_hardware_domain(current->domain) ) > + { > + gprintk(XENLOG_ERR, "SCMI: Unprivileged %pd cannot use SCMI.\n", > + current->domain); Minor points. gprintk() already renders current at the start of the line, so you don't need the extra %pd. I'd suggest simply XENLOG_WARNING "SCMI: Unprivileged access attempt\n". A double "SCMI", and trailing punctuation is not useful in the log. However, I'd also suggest using gdprintk() because this is guest trigger-able and not interesting to see in a release build. > diff --git a/xen/arch/arm/include/asm/firmware/scmi-smc.h b/xen/arch/arm/include/asm/firmware/scmi-smc.h > new file mode 100644 > index 0000000000..57cc9eef86 > --- /dev/null > +++ b/xen/arch/arm/include/asm/firmware/scmi-smc.h > +#ifndef __ASM_SCMI_SMC_H__ > +#define __ASM_SCMI_SMC_H__ > + > +#include <xen/types.h> > +#include <asm/regs.h> You can remove the include of asm/regs.h, and simply declare: struct cpu_user_regs; here. Nothing in this file needs to deference the pointer. ~Andrew
On 18/12/2024 15:58, Andrei Cherechesu wrote: > > > Hi Michal, > > Thank you for the review. > > On 18/12/2024 16:26, Michal Orzel wrote: >> >> On 18/12/2024 11:11, Andrei Cherechesu (OSS) wrote: >>> >>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com> >>> >>> Introduce the SCMI-SMC layer to have some basic degree of >>> awareness about SCMI calls that are based on the ARM System >>> Control and Management Interface (SCMI) specification (DEN0056E). >>> >>> The SCMI specification includes various protocols for managing >>> system-level resources, such as: clocks, pins, reset, system power, >>> power domains, performance domains, etc. The clients are named >>> "SCMI agents" and the server is named "SCMI platform". >>> >>> Only support the shared-memory based transport with SMCs as >>> the doorbell mechanism for notifying the platform. Also, this >>> implementation only handles the "arm,scmi-smc" compatible, >>> requiring the following properties: >>> - "arm,smc-id" (unique SMC ID) >>> - "shmem" (one or more phandles pointing to shmem zones >>> for each channel) >>> >>> The initialization is done as initcall, since we need >>> SMCs, and PSCI should already probe EL3 FW for SMCCC support. >>> If no "arm,scmi-smc" compatible node is found in the host >>> DT, the initialization fails silently, as it's not mandatory. >>> Otherwise, we get the 'arm,smc-id' DT property from the node, >>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges >>> are not validated, as the SMC calls are only passed through >>> to EL3 FW if coming from the hardware domain. >>> >>> Create a new 'firmware' folder to keep the SCMI code separate >>> from the generic ARM code. >>> >>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>> --- >>> xen/arch/arm/Kconfig | 2 + >>> xen/arch/arm/Makefile | 1 + >>> xen/arch/arm/firmware/Kconfig | 13 ++ >>> xen/arch/arm/firmware/Makefile | 1 + >>> xen/arch/arm/firmware/scmi-smc.c | 166 +++++++++++++++++++ >>> xen/arch/arm/include/asm/firmware/scmi-smc.h | 46 +++++ >>> 6 files changed, 229 insertions(+) >>> create mode 100644 xen/arch/arm/firmware/Kconfig >>> create mode 100644 xen/arch/arm/firmware/Makefile >>> create mode 100644 xen/arch/arm/firmware/scmi-smc.c >>> create mode 100644 xen/arch/arm/include/asm/firmware/scmi-smc.h >>> >>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig >>> index 604aba4996..23dc7162a7 100644 >>> --- a/xen/arch/arm/Kconfig >>> +++ b/xen/arch/arm/Kconfig >>> @@ -271,6 +271,8 @@ config PARTIAL_EMULATION >>> not been emulated to their complete functionality. Enabling this might >>> result in unwanted/non-spec compliant behavior. >>> >>> +source "arch/arm/firmware/Kconfig" >>> + >>> endmenu >>> >>> menu "ARM errata workaround via the alternative framework" >>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >>> index e4ad1ce851..8c696c2011 100644 >>> --- a/xen/arch/arm/Makefile >>> +++ b/xen/arch/arm/Makefile >>> @@ -6,6 +6,7 @@ obj-$(CONFIG_HAS_PCI) += pci/ >>> ifneq ($(CONFIG_NO_PLAT),y) >>> obj-y += platforms/ >>> endif >>> +obj-y += firmware/ >>> obj-$(CONFIG_TEE) += tee/ >>> obj-$(CONFIG_HAS_VPCI) += vpci.o >>> >>> diff --git a/xen/arch/arm/firmware/Kconfig b/xen/arch/arm/firmware/Kconfig >>> new file mode 100644 >>> index 0000000000..817da745fd >>> --- /dev/null >>> +++ b/xen/arch/arm/firmware/Kconfig >>> @@ -0,0 +1,13 @@ >>> +menu "Firmware Drivers" >>> + >>> +config SCMI_SMC >>> + bool "Forward SCMI over SMC calls from hwdom to EL3 firmware" >>> + default y >>> + help >>> + This option enables basic awareness for SCMI calls using SMC as >>> + doorbell mechanism and Shared Memory for transport ("arm,scmi-smc" >>> + compatible only). The value of "arm,smc-id" DT property from SCMI >>> + firmware node is used to trap and forward corresponding SCMI SMCs >>> + to firmware running at EL3, for calls coming from the hardware domain. >>> + >>> +endmenu >>> diff --git a/xen/arch/arm/firmware/Makefile b/xen/arch/arm/firmware/Makefile >>> new file mode 100644 >>> index 0000000000..a5e4542666 >>> --- /dev/null >>> +++ b/xen/arch/arm/firmware/Makefile >>> @@ -0,0 +1 @@ >>> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o >>> diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c >>> new file mode 100644 >>> index 0000000000..62657308d6 >>> --- /dev/null >>> +++ b/xen/arch/arm/firmware/scmi-smc.c >>> @@ -0,0 +1,166 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >>> +/* >>> + * xen/arch/arm/firmware/scmi-smc.c >>> + * >>> + * ARM System Control and Management Interface (SCMI) over SMC >>> + * Generic handling layer >>> + * >>> + * Andrei Cherechesu <andrei.cherechesu@nxp.com> >>> + * Copyright 2024 NXP >>> + */ >>> + >>> +#include <xen/acpi.h> >>> +#include <xen/device_tree.h> >>> +#include <xen/errno.h> >>> +#include <xen/init.h> >>> +#include <xen/sched.h> >>> +#include <xen/types.h> >>> + >>> +#include <asm/smccc.h> >>> +#include <asm/firmware/scmi-smc.h> >>> + >>> +#define SCMI_SMC_ID_PROP "arm,smc-id" >>> + >>> +static bool __ro_after_init scmi_support; >> I don't understand the need for this variable. IMO it's useless, given that in next patch you do: >> >> ... >> if ( scmi_is_enabled() ) >> return scmi_handle_smc(regs); >> >> return false; >> >> which could simply be changed to: >> ... >> return scmi_handle_smc(regs); >> >> and the variable, functions for it, etc. could be removed which would simplify the code. >> >> [...] > > Well, I agree that the code would maybe be simpler, but > that means we would call `scmi_handle_smc()` both when > SCMI-SMC layer is initialized and when it is not. > > That then means that if `scmi_handle_smc()` returns false, > we won't know in the caller if the SCMI-SMC layer is not > initialized at all or if it is initialized, but the SMC > request is invalid (it does not have the SMC ID we expect). > Do we need to, though? Let me explain more: scmi_handle_smc() is called from within handle_sip() that can result true/false. If SCMI is disabled, we need to return false. If SCMI is enabled but request is invalid we need to return false as well. If SCMI is enabled but not initialized we also need to return false. I suggest to drop scmi_is_enabled() as exported function and only use scmi_handle_smc() as global like I did in my example. Now, this solves the part where SCMI is disabled since you have a stub returning false and also the part where SCMI request is invalid. However, this does not solve the part where SCMI is enabled but layer not initialized. To fix it, you simply need to check inside scmi_handle_smc() if it's initialized. That's much simpler than requiring to use another global function which is not nice. Diff below: diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c index 62657308d61d..b3f34bdbb89b 100644 --- a/xen/arch/arm/firmware/scmi-smc.c +++ b/xen/arch/arm/firmware/scmi-smc.c @@ -24,12 +24,6 @@ static bool __ro_after_init scmi_support; static uint32_t __ro_after_init scmi_smc_id; -/* Check if SCMI layer correctly initialized and can be used. */ -bool scmi_is_enabled(void) -{ - return scmi_support; -} - /* * Check if provided SMC Function Identifier matches the one known by the SCMI * layer, as read from DT prop 'arm,smc-id' during initialiation. @@ -52,6 +46,9 @@ bool scmi_handle_smc(struct cpu_user_regs *regs) uint32_t fid = (uint32_t)get_user_reg(regs, 0); struct arm_smccc_res res; + if ( !scmi_support ) + return false; + if ( !scmi_is_valid_smc_id(fid) ) return false; diff --git a/xen/arch/arm/include/asm/firmware/scmi-smc.h b/xen/arch/arm/include/asm/firmware/scmi-smc.h index 57cc9eef8676..58730a8037c5 100644 --- a/xen/arch/arm/include/asm/firmware/scmi-smc.h +++ b/xen/arch/arm/include/asm/firmware/scmi-smc.h @@ -17,16 +17,10 @@ #ifdef CONFIG_SCMI_SMC -bool scmi_is_enabled(void); bool scmi_handle_smc(struct cpu_user_regs *regs); #else -static inline bool scmi_is_enabled(void) -{ - return false; -} - static inline bool scmi_handle_smc(struct cpu_user_regs *regs) { return false; diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c index c4d225c45cd3..62d8117a120c 100644 --- a/xen/arch/arm/vsmc.c +++ b/xen/arch/arm/vsmc.c @@ -232,10 +232,7 @@ static bool handle_sip(struct cpu_user_regs *regs) if ( platform_smc(regs) ) return true; - if ( scmi_is_enabled() ) - return scmi_handle_smc(regs); - - return false; + return scmi_handle_smc(regs); } /* ~Michal
Hi Michal, On 19/12/2024 10:35, Michal Orzel wrote: > > On 18/12/2024 15:58, Andrei Cherechesu wrote: >> >> Hi Michal, >> >> Thank you for the review. >> >> On 18/12/2024 16:26, Michal Orzel wrote: >>> On 18/12/2024 11:11, Andrei Cherechesu (OSS) wrote: >>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com> >>>> >>>> Introduce the SCMI-SMC layer to have some basic degree of >>>> awareness about SCMI calls that are based on the ARM System >>>> Control and Management Interface (SCMI) specification (DEN0056E). >>>> >>>> The SCMI specification includes various protocols for managing >>>> system-level resources, such as: clocks, pins, reset, system power, >>>> power domains, performance domains, etc. The clients are named >>>> "SCMI agents" and the server is named "SCMI platform". >>>> >>>> Only support the shared-memory based transport with SMCs as >>>> the doorbell mechanism for notifying the platform. Also, this >>>> implementation only handles the "arm,scmi-smc" compatible, >>>> requiring the following properties: >>>> - "arm,smc-id" (unique SMC ID) >>>> - "shmem" (one or more phandles pointing to shmem zones >>>> for each channel) >>>> >>>> The initialization is done as initcall, since we need >>>> SMCs, and PSCI should already probe EL3 FW for SMCCC support. >>>> If no "arm,scmi-smc" compatible node is found in the host >>>> DT, the initialization fails silently, as it's not mandatory. >>>> Otherwise, we get the 'arm,smc-id' DT property from the node, >>>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges >>>> are not validated, as the SMC calls are only passed through >>>> to EL3 FW if coming from the hardware domain. >>>> >>>> Create a new 'firmware' folder to keep the SCMI code separate >>>> from the generic ARM code. >>>> >>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com> >>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>>> --- >>>> xen/arch/arm/Kconfig | 2 + >>>> xen/arch/arm/Makefile | 1 + >>>> xen/arch/arm/firmware/Kconfig | 13 ++ >>>> xen/arch/arm/firmware/Makefile | 1 + >>>> xen/arch/arm/firmware/scmi-smc.c | 166 +++++++++++++++++++ >>>> xen/arch/arm/include/asm/firmware/scmi-smc.h | 46 +++++ >>>> 6 files changed, 229 insertions(+) >>>> create mode 100644 xen/arch/arm/firmware/Kconfig >>>> create mode 100644 xen/arch/arm/firmware/Makefile >>>> create mode 100644 xen/arch/arm/firmware/scmi-smc.c >>>> create mode 100644 xen/arch/arm/include/asm/firmware/scmi-smc.h >>>> >>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig >>>> index 604aba4996..23dc7162a7 100644 >>>> --- a/xen/arch/arm/Kconfig >>>> +++ b/xen/arch/arm/Kconfig >>>> @@ -271,6 +271,8 @@ config PARTIAL_EMULATION >>>> not been emulated to their complete functionality. Enabling this might >>>> result in unwanted/non-spec compliant behavior. >>>> >>>> +source "arch/arm/firmware/Kconfig" >>>> + >>>> endmenu >>>> >>>> menu "ARM errata workaround via the alternative framework" >>>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >>>> index e4ad1ce851..8c696c2011 100644 >>>> --- a/xen/arch/arm/Makefile >>>> +++ b/xen/arch/arm/Makefile >>>> @@ -6,6 +6,7 @@ obj-$(CONFIG_HAS_PCI) += pci/ >>>> ifneq ($(CONFIG_NO_PLAT),y) >>>> obj-y += platforms/ >>>> endif >>>> +obj-y += firmware/ >>>> obj-$(CONFIG_TEE) += tee/ >>>> obj-$(CONFIG_HAS_VPCI) += vpci.o >>>> >>>> diff --git a/xen/arch/arm/firmware/Kconfig b/xen/arch/arm/firmware/Kconfig >>>> new file mode 100644 >>>> index 0000000000..817da745fd >>>> --- /dev/null >>>> +++ b/xen/arch/arm/firmware/Kconfig >>>> @@ -0,0 +1,13 @@ >>>> +menu "Firmware Drivers" >>>> + >>>> +config SCMI_SMC >>>> + bool "Forward SCMI over SMC calls from hwdom to EL3 firmware" >>>> + default y >>>> + help >>>> + This option enables basic awareness for SCMI calls using SMC as >>>> + doorbell mechanism and Shared Memory for transport ("arm,scmi-smc" >>>> + compatible only). The value of "arm,smc-id" DT property from SCMI >>>> + firmware node is used to trap and forward corresponding SCMI SMCs >>>> + to firmware running at EL3, for calls coming from the hardware domain. >>>> + >>>> +endmenu >>>> diff --git a/xen/arch/arm/firmware/Makefile b/xen/arch/arm/firmware/Makefile >>>> new file mode 100644 >>>> index 0000000000..a5e4542666 >>>> --- /dev/null >>>> +++ b/xen/arch/arm/firmware/Makefile >>>> @@ -0,0 +1 @@ >>>> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o >>>> diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c >>>> new file mode 100644 >>>> index 0000000000..62657308d6 >>>> --- /dev/null >>>> +++ b/xen/arch/arm/firmware/scmi-smc.c >>>> @@ -0,0 +1,166 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >>>> +/* >>>> + * xen/arch/arm/firmware/scmi-smc.c >>>> + * >>>> + * ARM System Control and Management Interface (SCMI) over SMC >>>> + * Generic handling layer >>>> + * >>>> + * Andrei Cherechesu <andrei.cherechesu@nxp.com> >>>> + * Copyright 2024 NXP >>>> + */ >>>> + >>>> +#include <xen/acpi.h> >>>> +#include <xen/device_tree.h> >>>> +#include <xen/errno.h> >>>> +#include <xen/init.h> >>>> +#include <xen/sched.h> >>>> +#include <xen/types.h> >>>> + >>>> +#include <asm/smccc.h> >>>> +#include <asm/firmware/scmi-smc.h> >>>> + >>>> +#define SCMI_SMC_ID_PROP "arm,smc-id" >>>> + >>>> +static bool __ro_after_init scmi_support; >>> I don't understand the need for this variable. IMO it's useless, given that in next patch you do: >>> >>> ... >>> if ( scmi_is_enabled() ) >>> return scmi_handle_smc(regs); >>> >>> return false; >>> >>> which could simply be changed to: >>> ... >>> return scmi_handle_smc(regs); >>> >>> and the variable, functions for it, etc. could be removed which would simplify the code. >>> >>> [...] >> Well, I agree that the code would maybe be simpler, but >> that means we would call `scmi_handle_smc()` both when >> SCMI-SMC layer is initialized and when it is not. >> >> That then means that if `scmi_handle_smc()` returns false, >> we won't know in the caller if the SCMI-SMC layer is not >> initialized at all or if it is initialized, but the SMC >> request is invalid (it does not have the SMC ID we expect). >> Do we need to, though? > Let me explain more: > scmi_handle_smc() is called from within handle_sip() that can result true/false. > If SCMI is disabled, we need to return false. If SCMI is enabled but request is > invalid we need to return false as well. If SCMI is enabled but not initialized > we also need to return false. Right, that's what I expressed my concern about too: that we're grouping up all of these cases under a single return code of the same function. I'm fine with that, though. > I suggest to drop scmi_is_enabled() as exported > function and only use scmi_handle_smc() as global like I did in my example. > Now, this solves the part where SCMI is disabled since you have a stub returning > false and also the part where SCMI request is invalid. However, this does not > solve the part where SCMI is enabled but layer not initialized. To fix it, you > simply need to check inside scmi_handle_smc() if it's initialized. That's much > simpler than requiring to use another global function which is not nice. Got it, my understanding was that you suggested to completely drop the `scmi_support` variable and its associated function (`scmi_is_enabled()`), and decide internally whether the SCMI layer is initialized based on another mechanism. That's why I suggested `scmi_smc_id != 0` as mechanism and said that it simplifies the code, but requires maybe some non-trivial assumptions (if `scmi_smc_id == 0` => SCMI not initialized yet). ... > > Diff below: > diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c > index 62657308d61d..b3f34bdbb89b 100644 > --- a/xen/arch/arm/firmware/scmi-smc.c > +++ b/xen/arch/arm/firmware/scmi-smc.c > @@ -24,12 +24,6 @@ > static bool __ro_after_init scmi_support; > static uint32_t __ro_after_init scmi_smc_id; > > -/* Check if SCMI layer correctly initialized and can be used. */ > -bool scmi_is_enabled(void) > -{ > - return scmi_support; > -} > - > /* > * Check if provided SMC Function Identifier matches the one known by the SCMI > * layer, as read from DT prop 'arm,smc-id' during initialiation. > @@ -52,6 +46,9 @@ bool scmi_handle_smc(struct cpu_user_regs *regs) > uint32_t fid = (uint32_t)get_user_reg(regs, 0); > struct arm_smccc_res res; > > + if ( !scmi_support ) > + return false; > + > if ( !scmi_is_valid_smc_id(fid) ) > return false; > > diff --git a/xen/arch/arm/include/asm/firmware/scmi-smc.h b/xen/arch/arm/include/asm/firmware/scmi-smc.h > index 57cc9eef8676..58730a8037c5 100644 > --- a/xen/arch/arm/include/asm/firmware/scmi-smc.h > +++ b/xen/arch/arm/include/asm/firmware/scmi-smc.h > @@ -17,16 +17,10 @@ > > #ifdef CONFIG_SCMI_SMC > > -bool scmi_is_enabled(void); > bool scmi_handle_smc(struct cpu_user_regs *regs); > > #else > > -static inline bool scmi_is_enabled(void) > -{ > - return false; > -} > - > static inline bool scmi_handle_smc(struct cpu_user_regs *regs) > { > return false; > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c > index c4d225c45cd3..62d8117a120c 100644 > --- a/xen/arch/arm/vsmc.c > +++ b/xen/arch/arm/vsmc.c > @@ -232,10 +232,7 @@ static bool handle_sip(struct cpu_user_regs *regs) > if ( platform_smc(regs) ) > return true; > > - if ( scmi_is_enabled() ) > - return scmi_handle_smc(regs); > - > - return false; > + return scmi_handle_smc(regs); > } > > /* ... I understand now that your suggestions were not that restrictive :). Thank you for the clarification! v4 is on the way. > ~Michal > Regards, Andrei Cherechesu
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 604aba4996..23dc7162a7 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -271,6 +271,8 @@ config PARTIAL_EMULATION not been emulated to their complete functionality. Enabling this might result in unwanted/non-spec compliant behavior. +source "arch/arm/firmware/Kconfig" + endmenu menu "ARM errata workaround via the alternative framework" diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index e4ad1ce851..8c696c2011 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_HAS_PCI) += pci/ ifneq ($(CONFIG_NO_PLAT),y) obj-y += platforms/ endif +obj-y += firmware/ obj-$(CONFIG_TEE) += tee/ obj-$(CONFIG_HAS_VPCI) += vpci.o diff --git a/xen/arch/arm/firmware/Kconfig b/xen/arch/arm/firmware/Kconfig new file mode 100644 index 0000000000..817da745fd --- /dev/null +++ b/xen/arch/arm/firmware/Kconfig @@ -0,0 +1,13 @@ +menu "Firmware Drivers" + +config SCMI_SMC + bool "Forward SCMI over SMC calls from hwdom to EL3 firmware" + default y + help + This option enables basic awareness for SCMI calls using SMC as + doorbell mechanism and Shared Memory for transport ("arm,scmi-smc" + compatible only). The value of "arm,smc-id" DT property from SCMI + firmware node is used to trap and forward corresponding SCMI SMCs + to firmware running at EL3, for calls coming from the hardware domain. + +endmenu diff --git a/xen/arch/arm/firmware/Makefile b/xen/arch/arm/firmware/Makefile new file mode 100644 index 0000000000..a5e4542666 --- /dev/null +++ b/xen/arch/arm/firmware/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c new file mode 100644 index 0000000000..62657308d6 --- /dev/null +++ b/xen/arch/arm/firmware/scmi-smc.c @@ -0,0 +1,166 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * xen/arch/arm/firmware/scmi-smc.c + * + * ARM System Control and Management Interface (SCMI) over SMC + * Generic handling layer + * + * Andrei Cherechesu <andrei.cherechesu@nxp.com> + * Copyright 2024 NXP + */ + +#include <xen/acpi.h> +#include <xen/device_tree.h> +#include <xen/errno.h> +#include <xen/init.h> +#include <xen/sched.h> +#include <xen/types.h> + +#include <asm/smccc.h> +#include <asm/firmware/scmi-smc.h> + +#define SCMI_SMC_ID_PROP "arm,smc-id" + +static bool __ro_after_init scmi_support; +static uint32_t __ro_after_init scmi_smc_id; + +/* Check if SCMI layer correctly initialized and can be used. */ +bool scmi_is_enabled(void) +{ + return scmi_support; +} + +/* + * Check if provided SMC Function Identifier matches the one known by the SCMI + * layer, as read from DT prop 'arm,smc-id' during initialiation. + */ +static bool scmi_is_valid_smc_id(uint32_t fid) +{ + return (fid == scmi_smc_id); +} + +/* + * Generic handler for SCMI-SMC requests, currently only forwarding the + * request to FW running at EL3 if it came from the hardware domain. + * Called from the vSMC layer for SiP SMCs, since SCMI calls are usually + * provided this way. + * + * Returns true if SMC was handled (regardless of response), false otherwise. + */ +bool scmi_handle_smc(struct cpu_user_regs *regs) +{ + uint32_t fid = (uint32_t)get_user_reg(regs, 0); + struct arm_smccc_res res; + + if ( !scmi_is_valid_smc_id(fid) ) + return false; + + /* Only the hardware domain should use SCMI calls */ + if ( !is_hardware_domain(current->domain) ) + { + gprintk(XENLOG_ERR, "SCMI: Unprivileged %pd cannot use SCMI.\n", + current->domain); + return false; + } + + /* For the moment, forward the SCMI Request to FW running at EL3 */ + arm_smccc_1_1_smc(fid, + get_user_reg(regs, 1), + get_user_reg(regs, 2), + get_user_reg(regs, 3), + get_user_reg(regs, 4), + get_user_reg(regs, 5), + get_user_reg(regs, 6), + get_user_reg(regs, 7), + &res); + + set_user_reg(regs, 0, res.a0); + set_user_reg(regs, 1, res.a1); + set_user_reg(regs, 2, res.a2); + set_user_reg(regs, 3, res.a3); + + return true; +} + +static int __init scmi_check_smccc_ver(void) +{ + if ( smccc_ver < ARM_SMCCC_VERSION_1_1 ) + { + printk(XENLOG_WARNING + "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n"); + return -ENOSYS; + } + + return 0; +} + +static int __init scmi_dt_init_smccc(void) +{ + static const struct dt_device_match scmi_ids[] __initconst = + { + /* We only support "arm,scmi-smc" binding for now */ + DT_MATCH_COMPATIBLE("arm,scmi-smc"), + { /* sentinel */ }, + }; + const struct dt_device_node *scmi_node; + int ret; + + /* If no SCMI firmware node found, fail silently as it's not mandatory */ + scmi_node = dt_find_matching_node(NULL, scmi_ids); + if ( !scmi_node ) + return -EOPNOTSUPP; + + ret = dt_property_read_u32(scmi_node, SCMI_SMC_ID_PROP, &scmi_smc_id); + if ( !ret ) + { + printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n", + SCMI_SMC_ID_PROP, scmi_node->full_name); + return -ENOENT; + } + + scmi_support = true; + + return 0; +} + +/* Initialize the SCMI layer based on SMCs and Device-tree */ +static int __init scmi_init(void) +{ + int ret; + + if ( !acpi_disabled ) + { + printk(XENLOG_WARNING "SCMI is not supported when using ACPI\n"); + return -EINVAL; + } + + ret = scmi_check_smccc_ver(); + if ( ret ) + return ret; + + ret = scmi_dt_init_smccc(); + if ( ret == -EOPNOTSUPP ) + return ret; + if ( ret ) + goto err; + + printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", scmi_smc_id); + + return 0; + +err: + printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret); + return ret; +} + +__initcall(scmi_init); + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/arm/include/asm/firmware/scmi-smc.h b/xen/arch/arm/include/asm/firmware/scmi-smc.h new file mode 100644 index 0000000000..57cc9eef86 --- /dev/null +++ b/xen/arch/arm/include/asm/firmware/scmi-smc.h @@ -0,0 +1,46 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * xen/arch/arm/include/asm/firmware/scmi-smc.h + * + * ARM System Control and Management Interface (SCMI) over SMC + * Generic handling layer + * + * Andrei Cherechesu <andrei.cherechesu@nxp.com> + * Copyright 2024 NXP + */ + +#ifndef __ASM_SCMI_SMC_H__ +#define __ASM_SCMI_SMC_H__ + +#include <xen/types.h> +#include <asm/regs.h> + +#ifdef CONFIG_SCMI_SMC + +bool scmi_is_enabled(void); +bool scmi_handle_smc(struct cpu_user_regs *regs); + +#else + +static inline bool scmi_is_enabled(void) +{ + return false; +} + +static inline bool scmi_handle_smc(struct cpu_user_regs *regs) +{ + return false; +} + +#endif /* CONFIG_SCMI_SMC */ + +#endif /* __ASM_SCMI_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */