| Message ID | 1582115146-28658-11-git-send-email-chee.hong.ang@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable ARM Trusted Firmware for U-Boot | expand |
On 2/19/20 1:25 PM, chee.hong.ang at intel.com wrote: > From: Chee Hong Ang <chee.hong.ang at intel.com> > > These secure register access functions allow U-Boot proper running > at EL2 (non-secure) to access System Manager's secure registers > by calling the ATF's PSCI runtime services (EL3/secure). If these > helper functions are called from secure mode (EL3), the helper > function will direct access the secure registers without calling > the ATF's PSCI runtime services. > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> > --- > arch/arm/mach-socfpga/Makefile | 6 +++ > .../mach-socfpga/include/mach/secure_reg_helper.h | 20 ++++++++ > arch/arm/mach-socfpga/secure_reg_helper.c | 57 ++++++++++++++++++++++ > 3 files changed, 83 insertions(+) > create mode 100644 arch/arm/mach-socfpga/include/mach/secure_reg_helper.h > create mode 100644 arch/arm/mach-socfpga/secure_reg_helper.c > > diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile > index 3310e92..e59587b 100644 > --- a/arch/arm/mach-socfpga/Makefile > +++ b/arch/arm/mach-socfpga/Makefile > @@ -53,6 +53,12 @@ obj-y += wrap_pinmux_config_s10.o > obj-y += wrap_pll_config_s10.o > endif > > +ifndef CONFIG_SPL_BUILD > +ifdef CONFIG_SPL_ATF > +obj-y += secure_reg_helper.o obj-$(FOO) += bar.o , you don't need the inner ifdef > +endif > +endif [...] > +++ b/arch/arm/mach-socfpga/include/mach/secure_reg_helper.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * Copyright (C) 2019 Intel Corporation <www.intel.com> > + * > + */ > + > +#ifndef _SECURE_REG_HELPER_H_ > +#define _SECURE_REG_HELPER_H_ > + > +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) > +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); > +void socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); > +void socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 val); > +#else > +#define socfpga_secure_reg_read32 readl > +#define socfpga_secure_reg_write32 writel > +#define socfpga_secure_reg_update32 clrsetbits_le32 > +#endif I think I don't understand how this is supposed to work. Would every place in U-Boot have to be patched to call these functions now ? [...]
> On 2/19/20 1:25 PM, chee.hong.ang at intel.com wrote: > > From: Chee Hong Ang <chee.hong.ang at intel.com> > > > > These secure register access functions allow U-Boot proper running at > > EL2 (non-secure) to access System Manager's secure registers by > > calling the ATF's PSCI runtime services (EL3/secure). If these helper > > functions are called from secure mode (EL3), the helper function will > > direct access the secure registers without calling the ATF's PSCI > > runtime services. > > > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> > > --- > > arch/arm/mach-socfpga/Makefile | 6 +++ > > .../mach-socfpga/include/mach/secure_reg_helper.h | 20 ++++++++ > > arch/arm/mach-socfpga/secure_reg_helper.c | 57 > ++++++++++++++++++++++ > > 3 files changed, 83 insertions(+) > > create mode 100644 > > arch/arm/mach-socfpga/include/mach/secure_reg_helper.h > > create mode 100644 arch/arm/mach-socfpga/secure_reg_helper.c > > > > diff --git a/arch/arm/mach-socfpga/Makefile > > b/arch/arm/mach-socfpga/Makefile index 3310e92..e59587b 100644 > > --- a/arch/arm/mach-socfpga/Makefile > > +++ b/arch/arm/mach-socfpga/Makefile > > @@ -53,6 +53,12 @@ obj-y += wrap_pinmux_config_s10.o > > obj-y += wrap_pll_config_s10.o > > endif > > > > +ifndef CONFIG_SPL_BUILD > > +ifdef CONFIG_SPL_ATF > > +obj-y += secure_reg_helper.o > > obj-$(FOO) += bar.o , you don't need the inner ifdef OK. It's cleaner. Thanks. > > > +endif > > +endif > > [...] > > > +++ b/arch/arm/mach-socfpga/include/mach/secure_reg_helper.h > > @@ -0,0 +1,20 @@ > > +/* SPDX-License-Identifier: GPL-2.0 > > + * > > + * Copyright (C) 2019 Intel Corporation <www.intel.com> > > + * > > + */ > > + > > +#ifndef _SECURE_REG_HELPER_H_ > > +#define _SECURE_REG_HELPER_H_ > > + > > +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) > > +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void > > +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void > > +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 val); > > +#else > > +#define socfpga_secure_reg_read32 readl > > +#define socfpga_secure_reg_write32 writel > > +#define socfpga_secure_reg_update32 clrsetbits_le32 > > +#endif > > I think I don't understand how this is supposed to work. Would every place in U- > Boot have to be patched to call these functions now ? Not every register access need this. Only those accessing registers in secure zone such as 'System Manager' registers need to call this. It's basically determine whether the driver should issue SMC/PSCI call if it's running in EL2 (non-secure) or access the registers directly by simply using readl/writel and etc if it's running in EL3 (secure). Accessing those registers in secure zone in non-secure mode (EL2) will cause SError exception. So we can determine this behaviour in compile time: SPL always running in EL3. So it just simply fallback to use readl/writel/clrsetbits_le32. For U-Boot proper (SSBL), there are 2 scenarios: 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It implies that U-Boot proper will be running in EL2 (non-secure), then it will use SMC/PSCI calls to access the secure registers. 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will be running in EL3 which will fall back to simply using the direct access functions (readl/writel and etc). > > [...]
On 2/20/20 3:02 AM, Ang, Chee Hong wrote: [...] >>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) >>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void >>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void >>> +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 val); >>> +#else >>> +#define socfpga_secure_reg_read32 readl >>> +#define socfpga_secure_reg_write32 writel >>> +#define socfpga_secure_reg_update32 clrsetbits_le32 >>> +#endif >> >> I think I don't understand how this is supposed to work. Would every place in U- >> Boot have to be patched to call these functions now ? > > Not every register access need this. Only those accessing registers in secure zone such > as 'System Manager' registers need to call this. It's basically determine whether the driver > should issue SMC/PSCI call if it's running in EL2 (non-secure) or access the registers directly > by simply using readl/writel and etc if it's running in EL3 (secure). > Accessing those registers in secure zone in non-secure mode (EL2) will cause SError exception. > So we can determine this behaviour in compile time: > SPL always running in EL3. So it just simply fallback to use readl/writel/clrsetbits_le32. > > For U-Boot proper (SSBL), there are 2 scenarios: > 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It implies that U-Boot proper will be > running in EL2 (non-secure), then it will use SMC/PSCI calls to access the secure registers. > > 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will be running in EL3 which > will fall back to simply using the direct access functions (readl/writel and etc). I would expect the standard IO accessors would or should handle this stuff ?
> On 2/20/20 3:02 AM, Ang, Chee Hong wrote: > [...] > >>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) > >>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void > >>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void > >>> +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 > >>> +val); #else > >>> +#define socfpga_secure_reg_read32 readl > >>> +#define socfpga_secure_reg_write32 writel > >>> +#define socfpga_secure_reg_update32 clrsetbits_le32 > >>> +#endif > >> > >> I think I don't understand how this is supposed to work. Would every > >> place in U- Boot have to be patched to call these functions now ? > > > > Not every register access need this. Only those accessing registers in > > secure zone such as 'System Manager' registers need to call this. It's > > basically determine whether the driver should issue SMC/PSCI call if > > it's running in EL2 (non-secure) or access the registers directly by simply using > readl/writel and etc if it's running in EL3 (secure). > > Accessing those registers in secure zone in non-secure mode (EL2) will cause > SError exception. > > So we can determine this behaviour in compile time: > > SPL always running in EL3. So it just simply fallback to use > readl/writel/clrsetbits_le32. > > > > For U-Boot proper (SSBL), there are 2 scenarios: > > 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It implies > > that U-Boot proper will be running in EL2 (non-secure), then it will use > SMC/PSCI calls to access the secure registers. > > > > 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will > > be running in EL3 which will fall back to simply using the direct access functions > (readl/writel and etc). > > I would expect the standard IO accessors would or should handle this stuff ? Standard IO accessors are just general memory read/write functions designed to be compatible with general hardware platforms. Not all platforms have secure/non-secure hardware zones. I don't think they should handle this. If it's running in EL3 (secure mode) the standard I/O accessors will work just fine because EL3 can access to all secure/non-secure zones. In the header file, you can see the secure I/O accessors will be replaced by the standard I/O accessors if it's built for SPL and U-Boot proper without ATF. Because both are running in EL3 (secure). If ATF is enabled, SPL will be still running in EL3 but U-Boot proper will be running in EL2 (non-secure). If any code accessing those secure zones without going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError' exceptions and crash the U-Boot.
On 2/20/20 6:54 PM, Ang, Chee Hong wrote: >> On 2/20/20 3:02 AM, Ang, Chee Hong wrote: >> [...] >>>>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) >>>>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void >>>>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void >>>>> +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 >>>>> +val); #else >>>>> +#define socfpga_secure_reg_read32 readl >>>>> +#define socfpga_secure_reg_write32 writel >>>>> +#define socfpga_secure_reg_update32 clrsetbits_le32 >>>>> +#endif >>>> >>>> I think I don't understand how this is supposed to work. Would every >>>> place in U- Boot have to be patched to call these functions now ? >>> >>> Not every register access need this. Only those accessing registers in >>> secure zone such as 'System Manager' registers need to call this. It's >>> basically determine whether the driver should issue SMC/PSCI call if >>> it's running in EL2 (non-secure) or access the registers directly by simply using >> readl/writel and etc if it's running in EL3 (secure). >>> Accessing those registers in secure zone in non-secure mode (EL2) will cause >> SError exception. >>> So we can determine this behaviour in compile time: >>> SPL always running in EL3. So it just simply fallback to use >> readl/writel/clrsetbits_le32. >>> >>> For U-Boot proper (SSBL), there are 2 scenarios: >>> 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It implies >>> that U-Boot proper will be running in EL2 (non-secure), then it will use >> SMC/PSCI calls to access the secure registers. >>> >>> 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will >>> be running in EL3 which will fall back to simply using the direct access functions >> (readl/writel and etc). >> >> I would expect the standard IO accessors would or should handle this stuff ? > Standard IO accessors are just general memory read/write functions designed to be > compatible with general hardware platforms. Not all platforms have secure/non-secure > hardware zones. I don't think they should handle this. > > If it's running in EL3 (secure mode) the standard I/O accessors will work just fine because > EL3 can access to all secure/non-secure zones. In the header file, you can see the secure > I/O accessors will be replaced by the standard I/O accessors if it's built for SPL and U-Boot > proper without ATF. Because both are running in EL3 (secure). > > If ATF is enabled, SPL will be still running in EL3 but U-Boot proper will be running in > EL2 (non-secure). If any code accessing those secure zones without going through ATF > (making SMC/PSCI calls to EL3), it will trigger 'SError' exceptions and crash the U-Boot. Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever access secure zones in the first place ? And if that's really necessary, should the ATF really provide secure-mode register access primitives or should it provide some more high-level interface instead ?
> On 2/20/20 6:54 PM, Ang, Chee Hong wrote: > >> On 2/20/20 3:02 AM, Ang, Chee Hong wrote: > >> [...] > >>>>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) > >>>>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void > >>>>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void > >>>>> +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 > >>>>> +val); #else > >>>>> +#define socfpga_secure_reg_read32 readl > >>>>> +#define socfpga_secure_reg_write32 writel > >>>>> +#define socfpga_secure_reg_update32 clrsetbits_le32 > >>>>> +#endif > >>>> > >>>> I think I don't understand how this is supposed to work. Would > >>>> every place in U- Boot have to be patched to call these functions now ? > >>> > >>> Not every register access need this. Only those accessing registers > >>> in secure zone such as 'System Manager' registers need to call this. > >>> It's basically determine whether the driver should issue SMC/PSCI > >>> call if it's running in EL2 (non-secure) or access the registers > >>> directly by simply using > >> readl/writel and etc if it's running in EL3 (secure). > >>> Accessing those registers in secure zone in non-secure mode (EL2) > >>> will cause > >> SError exception. > >>> So we can determine this behaviour in compile time: > >>> SPL always running in EL3. So it just simply fallback to use > >> readl/writel/clrsetbits_le32. > >>> > >>> For U-Boot proper (SSBL), there are 2 scenarios: > >>> 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It > >>> implies that U-Boot proper will be running in EL2 (non-secure), then > >>> it will use > >> SMC/PSCI calls to access the secure registers. > >>> > >>> 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will > >>> be running in EL3 which will fall back to simply using the direct > >>> access functions > >> (readl/writel and etc). > >> > >> I would expect the standard IO accessors would or should handle this stuff ? > > Standard IO accessors are just general memory read/write functions > > designed to be compatible with general hardware platforms. Not all > > platforms have secure/non-secure hardware zones. I don't think they should > handle this. > > > > If it's running in EL3 (secure mode) the standard I/O accessors will > > work just fine because > > EL3 can access to all secure/non-secure zones. In the header file, you > > can see the secure I/O accessors will be replaced by the standard I/O > > accessors if it's built for SPL and U-Boot proper without ATF. Because both are > running in EL3 (secure). > > > > If ATF is enabled, SPL will be still running in EL3 but U-Boot proper > > will be running in > > EL2 (non-secure). If any code accessing those secure zones without > > going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError' > exceptions and crash the U-Boot. > > Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever access > secure zones in the first place ? SPL and U-Boot reuse the same drivers code. It runs without issue in SPL (EL3) but it crashes if running the same driver code in EL2 which access some secure zone registers. The System Manager (secure zone) contains some register which control the behaviours of EMAC/SDMMC and etc. Clock manager driver rely on those registers in System Manager as well for retrieving clock information. These clock/EMAC/SDMMC drivers access the System Manager (secure zone). > > And if that's really necessary, should the ATF really provide secure-mode register > access primitives or should it provide some more high-level interface instead ? I see your point. I should have mentioned to you that these secure-mode register access provided by ATF actually do more stuffs than just primitive accessors. ATF only allow certain secure registers required by the non-secure driver to be accessed. It will check the secure register address and block access if the register address is not allowed to be accessed by non-secure world (EL2). So these secure register access provided by ATF is not just simple accessor/delegate which simply allow access to any secure zone from non-secure world without any restrictions. I would say the secure register access provided by ATF is a 'middle-level' interface not just some primitive accessors. Currently, we have like 20+ secure registers allowed access by drivers running in non-secure mode (U-Boot proper / Linux). I don't think we want to define and maintain those high level interfaces for each of those secure register accesses in ATF and U-Boot.
On 2/21/20 7:01 PM, Ang, Chee Hong wrote: >> On 2/20/20 6:54 PM, Ang, Chee Hong wrote: >>>> On 2/20/20 3:02 AM, Ang, Chee Hong wrote: >>>> [...] >>>>>>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) >>>>>>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void >>>>>>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void >>>>>>> +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 >>>>>>> +val); #else >>>>>>> +#define socfpga_secure_reg_read32 readl >>>>>>> +#define socfpga_secure_reg_write32 writel >>>>>>> +#define socfpga_secure_reg_update32 clrsetbits_le32 >>>>>>> +#endif >>>>>> >>>>>> I think I don't understand how this is supposed to work. Would >>>>>> every place in U- Boot have to be patched to call these functions now ? >>>>> >>>>> Not every register access need this. Only those accessing registers >>>>> in secure zone such as 'System Manager' registers need to call this. >>>>> It's basically determine whether the driver should issue SMC/PSCI >>>>> call if it's running in EL2 (non-secure) or access the registers >>>>> directly by simply using >>>> readl/writel and etc if it's running in EL3 (secure). >>>>> Accessing those registers in secure zone in non-secure mode (EL2) >>>>> will cause >>>> SError exception. >>>>> So we can determine this behaviour in compile time: >>>>> SPL always running in EL3. So it just simply fallback to use >>>> readl/writel/clrsetbits_le32. >>>>> >>>>> For U-Boot proper (SSBL), there are 2 scenarios: >>>>> 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It >>>>> implies that U-Boot proper will be running in EL2 (non-secure), then >>>>> it will use >>>> SMC/PSCI calls to access the secure registers. >>>>> >>>>> 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will >>>>> be running in EL3 which will fall back to simply using the direct >>>>> access functions >>>> (readl/writel and etc). >>>> >>>> I would expect the standard IO accessors would or should handle this stuff ? >>> Standard IO accessors are just general memory read/write functions >>> designed to be compatible with general hardware platforms. Not all >>> platforms have secure/non-secure hardware zones. I don't think they should >> handle this. >>> >>> If it's running in EL3 (secure mode) the standard I/O accessors will >>> work just fine because >>> EL3 can access to all secure/non-secure zones. In the header file, you >>> can see the secure I/O accessors will be replaced by the standard I/O >>> accessors if it's built for SPL and U-Boot proper without ATF. Because both are >> running in EL3 (secure). >>> >>> If ATF is enabled, SPL will be still running in EL3 but U-Boot proper >>> will be running in >>> EL2 (non-secure). If any code accessing those secure zones without >>> going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError' >> exceptions and crash the U-Boot. >> >> Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever access >> secure zones in the first place ? > SPL and U-Boot reuse the same drivers code. It runs without issue in SPL (EL3) but > it crashes if running the same driver code in EL2 which access some secure zone registers. > The System Manager (secure zone) contains some register which control the behaviours of > EMAC/SDMMC and etc. Clock manager driver rely on those registers in System Manager as well > for retrieving clock information. These clock/EMAC/SDMMC drivers access the System Manager > (secure zone). Maybe those registers should only be configured by the secure OS, so maybe those drivers should be fixed ? >> And if that's really necessary, should the ATF really provide secure-mode register >> access primitives or should it provide some more high-level interface instead ? > I see your point. I should have mentioned to you that these secure-mode register access provided by > ATF actually do more stuffs than just primitive accessors. So socfpga_secure_reg_read32 does not just read a register ? Then the naming is misleading . And documentation is missing. > ATF only allow certain secure registers required by the non-secure driver to be accessed. > It will check the secure register address and block access if the register address is not allowed > to be accessed by non-secure world (EL2). Why don't you configure those secure registers in the secure mode then ? It seems like that's the purpose of those registers being secure only. > So these secure register access provided by ATF is not just simple accessor/delegate which > simply allow access to any secure zone from non-secure world without any restrictions. > I would say the secure register access provided by ATF is a 'middle-level' interface not just > some primitive accessors. > > Currently, we have like 20+ secure registers allowed access by drivers running in > non-secure mode (U-Boot proper / Linux). > I don't think we want to define and maintain those high level interfaces for each of those secure > register accesses in ATF and U-Boot. See above.
> On 2/21/20 7:01 PM, Ang, Chee Hong wrote: > >> On 2/20/20 6:54 PM, Ang, Chee Hong wrote: > >>>> On 2/20/20 3:02 AM, Ang, Chee Hong wrote: > >>>> [...] > >>>>>>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) > >>>>>>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void > >>>>>>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void > >>>>>>> +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 > >>>>>>> +val); #else > >>>>>>> +#define socfpga_secure_reg_read32 readl > >>>>>>> +#define socfpga_secure_reg_write32 writel > >>>>>>> +#define socfpga_secure_reg_update32 clrsetbits_le32 > >>>>>>> +#endif > >>>>>> > >>>>>> I think I don't understand how this is supposed to work. Would > >>>>>> every place in U- Boot have to be patched to call these functions now ? > >>>>> > >>>>> Not every register access need this. Only those accessing > >>>>> registers in secure zone such as 'System Manager' registers need to call > this. > >>>>> It's basically determine whether the driver should issue SMC/PSCI > >>>>> call if it's running in EL2 (non-secure) or access the registers > >>>>> directly by simply using > >>>> readl/writel and etc if it's running in EL3 (secure). > >>>>> Accessing those registers in secure zone in non-secure mode (EL2) > >>>>> will cause > >>>> SError exception. > >>>>> So we can determine this behaviour in compile time: > >>>>> SPL always running in EL3. So it just simply fallback to use > >>>> readl/writel/clrsetbits_le32. > >>>>> > >>>>> For U-Boot proper (SSBL), there are 2 scenarios: > >>>>> 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It > >>>>> implies that U-Boot proper will be running in EL2 (non-secure), > >>>>> then it will use > >>>> SMC/PSCI calls to access the secure registers. > >>>>> > >>>>> 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper > >>>>> will be running in EL3 which will fall back to simply using the > >>>>> direct access functions > >>>> (readl/writel and etc). > >>>> > >>>> I would expect the standard IO accessors would or should handle this stuff ? > >>> Standard IO accessors are just general memory read/write functions > >>> designed to be compatible with general hardware platforms. Not all > >>> platforms have secure/non-secure hardware zones. I don't think they > >>> should > >> handle this. > >>> > >>> If it's running in EL3 (secure mode) the standard I/O accessors will > >>> work just fine because > >>> EL3 can access to all secure/non-secure zones. In the header file, > >>> you can see the secure I/O accessors will be replaced by the > >>> standard I/O accessors if it's built for SPL and U-Boot proper > >>> without ATF. Because both are > >> running in EL3 (secure). > >>> > >>> If ATF is enabled, SPL will be still running in EL3 but U-Boot > >>> proper will be running in > >>> EL2 (non-secure). If any code accessing those secure zones without > >>> going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError' > >> exceptions and crash the U-Boot. > >> > >> Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever > >> access secure zones in the first place ? > > SPL and U-Boot reuse the same drivers code. It runs without issue in > > SPL (EL3) but it crashes if running the same driver code in EL2 which access > some secure zone registers. > > The System Manager (secure zone) contains some register which control > > the behaviours of EMAC/SDMMC and etc. Clock manager driver rely on > > those registers in System Manager as well for retrieving clock > > information. These clock/EMAC/SDMMC drivers access the System Manager > (secure zone). > > Maybe those registers should only be configured by the secure OS, so maybe > those drivers should be fixed ? > > >> And if that's really necessary, should the ATF really provide > >> secure-mode register access primitives or should it provide some more high- > level interface instead ? > > I see your point. I should have mentioned to you that these > > secure-mode register access provided by ATF actually do more stuffs than just > primitive accessors. > > So socfpga_secure_reg_read32 does not just read a register ? Then the naming > is misleading . And documentation is missing. > > > ATF only allow certain secure registers required by the non-secure driver to be > accessed. > > It will check the secure register address and block access if the > > register address is not allowed to be accessed by non-secure world (EL2). > > Why don't you configure those secure registers in the secure mode then ? > It seems like that's the purpose of those registers being secure only. > > > So these secure register access provided by ATF is not just simple > > accessor/delegate which simply allow access to any secure zone from non- > secure world without any restrictions. > > I would say the secure register access provided by ATF is a > > 'middle-level' interface not just some primitive accessors. > > > > Currently, we have like 20+ secure registers allowed access by drivers > > running in non-secure mode (U-Boot proper / Linux). > > I don't think we want to define and maintain those high level > > interfaces for each of those secure register accesses in ATF and U-Boot. > > See above. OK. Then these secure access register should be set up in SPL (EL3). U-Boot drivers shouldn't access them at all because the driver may be running in SPL(EL3) and in U-Boot proper (EL2) too. I can take a look at those drivers accessing secure registers and try to move/decouple those secure access from U-Boot drivers to SPL (EL3) then we no longer need those secure register access functions. I am not sure doing this will impact the functionality of the U-Boot driver running in EL2 or not. I can refactor those drivers and try it out.
On 2/21/20 8:06 PM, Ang, Chee Hong wrote: >> On 2/21/20 7:01 PM, Ang, Chee Hong wrote: >>>> On 2/20/20 6:54 PM, Ang, Chee Hong wrote: >>>>>> On 2/20/20 3:02 AM, Ang, Chee Hong wrote: >>>>>> [...] >>>>>>>>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) >>>>>>>>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void >>>>>>>>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void >>>>>>>>> +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 >>>>>>>>> +val); #else >>>>>>>>> +#define socfpga_secure_reg_read32 readl >>>>>>>>> +#define socfpga_secure_reg_write32 writel >>>>>>>>> +#define socfpga_secure_reg_update32 clrsetbits_le32 >>>>>>>>> +#endif >>>>>>>> >>>>>>>> I think I don't understand how this is supposed to work. Would >>>>>>>> every place in U- Boot have to be patched to call these functions now ? >>>>>>> >>>>>>> Not every register access need this. Only those accessing >>>>>>> registers in secure zone such as 'System Manager' registers need to call >> this. >>>>>>> It's basically determine whether the driver should issue SMC/PSCI >>>>>>> call if it's running in EL2 (non-secure) or access the registers >>>>>>> directly by simply using >>>>>> readl/writel and etc if it's running in EL3 (secure). >>>>>>> Accessing those registers in secure zone in non-secure mode (EL2) >>>>>>> will cause >>>>>> SError exception. >>>>>>> So we can determine this behaviour in compile time: >>>>>>> SPL always running in EL3. So it just simply fallback to use >>>>>> readl/writel/clrsetbits_le32. >>>>>>> >>>>>>> For U-Boot proper (SSBL), there are 2 scenarios: >>>>>>> 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It >>>>>>> implies that U-Boot proper will be running in EL2 (non-secure), >>>>>>> then it will use >>>>>> SMC/PSCI calls to access the secure registers. >>>>>>> >>>>>>> 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper >>>>>>> will be running in EL3 which will fall back to simply using the >>>>>>> direct access functions >>>>>> (readl/writel and etc). >>>>>> >>>>>> I would expect the standard IO accessors would or should handle this stuff ? >>>>> Standard IO accessors are just general memory read/write functions >>>>> designed to be compatible with general hardware platforms. Not all >>>>> platforms have secure/non-secure hardware zones. I don't think they >>>>> should >>>> handle this. >>>>> >>>>> If it's running in EL3 (secure mode) the standard I/O accessors will >>>>> work just fine because >>>>> EL3 can access to all secure/non-secure zones. In the header file, >>>>> you can see the secure I/O accessors will be replaced by the >>>>> standard I/O accessors if it's built for SPL and U-Boot proper >>>>> without ATF. Because both are >>>> running in EL3 (secure). >>>>> >>>>> If ATF is enabled, SPL will be still running in EL3 but U-Boot >>>>> proper will be running in >>>>> EL2 (non-secure). If any code accessing those secure zones without >>>>> going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError' >>>> exceptions and crash the U-Boot. >>>> >>>> Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever >>>> access secure zones in the first place ? >>> SPL and U-Boot reuse the same drivers code. It runs without issue in >>> SPL (EL3) but it crashes if running the same driver code in EL2 which access >> some secure zone registers. >>> The System Manager (secure zone) contains some register which control >>> the behaviours of EMAC/SDMMC and etc. Clock manager driver rely on >>> those registers in System Manager as well for retrieving clock >>> information. These clock/EMAC/SDMMC drivers access the System Manager >> (secure zone). >> >> Maybe those registers should only be configured by the secure OS, so maybe >> those drivers should be fixed ? >> >>>> And if that's really necessary, should the ATF really provide >>>> secure-mode register access primitives or should it provide some more high- >> level interface instead ? >>> I see your point. I should have mentioned to you that these >>> secure-mode register access provided by ATF actually do more stuffs than just >> primitive accessors. >> >> So socfpga_secure_reg_read32 does not just read a register ? Then the naming >> is misleading . And documentation is missing. >> >>> ATF only allow certain secure registers required by the non-secure driver to be >> accessed. >>> It will check the secure register address and block access if the >>> register address is not allowed to be accessed by non-secure world (EL2). >> >> Why don't you configure those secure registers in the secure mode then ? >> It seems like that's the purpose of those registers being secure only. >> >>> So these secure register access provided by ATF is not just simple >>> accessor/delegate which simply allow access to any secure zone from non- >> secure world without any restrictions. >>> I would say the secure register access provided by ATF is a >>> 'middle-level' interface not just some primitive accessors. >>> >>> Currently, we have like 20+ secure registers allowed access by drivers >>> running in non-secure mode (U-Boot proper / Linux). >>> I don't think we want to define and maintain those high level >>> interfaces for each of those secure register accesses in ATF and U-Boot. >> >> See above. > OK. Then these secure access register should be set up in SPL (EL3). > U-Boot drivers shouldn't access them at all because the driver may be running > in SPL(EL3) and in U-Boot proper (EL2) too. > I can take a look at those drivers accessing secure registers and try > to move/decouple those secure access from U-Boot drivers to SPL (EL3) then we no > longer need those secure register access functions. I think that would be great, no ? > I am not sure doing this will impact the functionality of the U-Boot driver > running in EL2 or not. > I can refactor those drivers and try it out. Thanks!
> On 2/21/20 8:06 PM, Ang, Chee Hong wrote: > >> On 2/21/20 7:01 PM, Ang, Chee Hong wrote: > >>>> On 2/20/20 6:54 PM, Ang, Chee Hong wrote: > >>>>>> On 2/20/20 3:02 AM, Ang, Chee Hong wrote: > >>>>>> [...] > >>>>>>>>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) > >>>>>>>>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void > >>>>>>>>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); > >>>>>>>>> +void socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 > >>>>>>>>> +mask, u32 val); #else > >>>>>>>>> +#define socfpga_secure_reg_read32 readl > >>>>>>>>> +#define socfpga_secure_reg_write32 writel > >>>>>>>>> +#define socfpga_secure_reg_update32 clrsetbits_le32 > >>>>>>>>> +#endif > >>>>>>>> > >>>>>>>> I think I don't understand how this is supposed to work. Would > >>>>>>>> every place in U- Boot have to be patched to call these functions now ? > >>>>>>> > >>>>>>> Not every register access need this. Only those accessing > >>>>>>> registers in secure zone such as 'System Manager' registers need > >>>>>>> to call > >> this. > >>>>>>> It's basically determine whether the driver should issue > >>>>>>> SMC/PSCI call if it's running in EL2 (non-secure) or access the > >>>>>>> registers directly by simply using > >>>>>> readl/writel and etc if it's running in EL3 (secure). > >>>>>>> Accessing those registers in secure zone in non-secure mode > >>>>>>> (EL2) will cause > >>>>>> SError exception. > >>>>>>> So we can determine this behaviour in compile time: > >>>>>>> SPL always running in EL3. So it just simply fallback to use > >>>>>> readl/writel/clrsetbits_le32. > >>>>>>> > >>>>>>> For U-Boot proper (SSBL), there are 2 scenarios: > >>>>>>> 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It > >>>>>>> implies that U-Boot proper will be running in EL2 (non-secure), > >>>>>>> then it will use > >>>>>> SMC/PSCI calls to access the secure registers. > >>>>>>> > >>>>>>> 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper > >>>>>>> will be running in EL3 which will fall back to simply using the > >>>>>>> direct access functions > >>>>>> (readl/writel and etc). > >>>>>> > >>>>>> I would expect the standard IO accessors would or should handle this > stuff ? > >>>>> Standard IO accessors are just general memory read/write functions > >>>>> designed to be compatible with general hardware platforms. Not all > >>>>> platforms have secure/non-secure hardware zones. I don't think > >>>>> they should > >>>> handle this. > >>>>> > >>>>> If it's running in EL3 (secure mode) the standard I/O accessors > >>>>> will work just fine because > >>>>> EL3 can access to all secure/non-secure zones. In the header file, > >>>>> you can see the secure I/O accessors will be replaced by the > >>>>> standard I/O accessors if it's built for SPL and U-Boot proper > >>>>> without ATF. Because both are > >>>> running in EL3 (secure). > >>>>> > >>>>> If ATF is enabled, SPL will be still running in EL3 but U-Boot > >>>>> proper will be running in > >>>>> EL2 (non-secure). If any code accessing those secure zones without > >>>>> going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError' > >>>> exceptions and crash the U-Boot. > >>>> > >>>> Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever > >>>> access secure zones in the first place ? > >>> SPL and U-Boot reuse the same drivers code. It runs without issue in > >>> SPL (EL3) but it crashes if running the same driver code in EL2 > >>> which access > >> some secure zone registers. > >>> The System Manager (secure zone) contains some register which > >>> control the behaviours of EMAC/SDMMC and etc. Clock manager driver > >>> rely on those registers in System Manager as well for retrieving > >>> clock information. These clock/EMAC/SDMMC drivers access the System > >>> Manager > >> (secure zone). > >> > >> Maybe those registers should only be configured by the secure OS, so > >> maybe those drivers should be fixed ? > >> > >>>> And if that's really necessary, should the ATF really provide > >>>> secure-mode register access primitives or should it provide some > >>>> more high- > >> level interface instead ? > >>> I see your point. I should have mentioned to you that these > >>> secure-mode register access provided by ATF actually do more stuffs > >>> than just > >> primitive accessors. > >> > >> So socfpga_secure_reg_read32 does not just read a register ? Then the > >> naming is misleading . And documentation is missing. > >> > >>> ATF only allow certain secure registers required by the non-secure > >>> driver to be > >> accessed. > >>> It will check the secure register address and block access if the > >>> register address is not allowed to be accessed by non-secure world (EL2). > >> > >> Why don't you configure those secure registers in the secure mode then ? > >> It seems like that's the purpose of those registers being secure only. > >> > >>> So these secure register access provided by ATF is not just simple > >>> accessor/delegate which simply allow access to any secure zone from > >>> non- > >> secure world without any restrictions. > >>> I would say the secure register access provided by ATF is a > >>> 'middle-level' interface not just some primitive accessors. > >>> > >>> Currently, we have like 20+ secure registers allowed access by > >>> drivers running in non-secure mode (U-Boot proper / Linux). > >>> I don't think we want to define and maintain those high level > >>> interfaces for each of those secure register accesses in ATF and U-Boot. > >> > >> See above. > > OK. Then these secure access register should be set up in SPL (EL3). > > U-Boot drivers shouldn't access them at all because the driver may be > > running in SPL(EL3) and in U-Boot proper (EL2) too. > > I can take a look at those drivers accessing secure registers and try > > to move/decouple those secure access from U-Boot drivers to SPL (EL3) > > then we no longer need those secure register access functions. > > I think that would be great, no ? Since the SDMMC/DWMAC drivers read the device tree to configure the behaviour of the hardware via the secure registers. I think it should still be part of the driver instead of configuring the hardware in different places. I have proposed using ATF's high-level APIs to achieve this when the driver is running in EL2. I have already proposed this in other email threads. Are you OK with this approach ? . > > > I am not sure doing this will impact the functionality of the U-Boot > > driver running in EL2 or not. > > I can refactor those drivers and try it out. > > Thanks!
On 2/24/20 3:21 AM, Ang, Chee Hong wrote: [...] >>>>> Currently, we have like 20+ secure registers allowed access by >>>>> drivers running in non-secure mode (U-Boot proper / Linux). >>>>> I don't think we want to define and maintain those high level >>>>> interfaces for each of those secure register accesses in ATF and U-Boot. >>>> >>>> See above. >>> OK. Then these secure access register should be set up in SPL (EL3). >>> U-Boot drivers shouldn't access them at all because the driver may be >>> running in SPL(EL3) and in U-Boot proper (EL2) too. >>> I can take a look at those drivers accessing secure registers and try >>> to move/decouple those secure access from U-Boot drivers to SPL (EL3) >>> then we no longer need those secure register access functions. >> >> I think that would be great, no ? > Since the SDMMC/DWMAC drivers read the device tree to configure the behaviour > of the hardware via the secure registers. I think it should still be part of the > driver instead of configuring the hardware in different places. I have proposed > using ATF's high-level APIs to achieve this when the driver is running in EL2. > I have already proposed this in other email threads. > Are you OK with this approach ? I think something more high level might be a good idea here.
> On 2/24/20 3:21 AM, Ang, Chee Hong wrote: > [...] > > >>>>> Currently, we have like 20+ secure registers allowed access by > >>>>> drivers running in non-secure mode (U-Boot proper / Linux). > >>>>> I don't think we want to define and maintain those high level > >>>>> interfaces for each of those secure register accesses in ATF and U-Boot. > >>>> > >>>> See above. > >>> OK. Then these secure access register should be set up in SPL (EL3). > >>> U-Boot drivers shouldn't access them at all because the driver may > >>> be running in SPL(EL3) and in U-Boot proper (EL2) too. > >>> I can take a look at those drivers accessing secure registers and > >>> try to move/decouple those secure access from U-Boot drivers to SPL > >>> (EL3) then we no longer need those secure register access functions. > >> > >> I think that would be great, no ? > > Since the SDMMC/DWMAC drivers read the device tree to configure the > > behaviour of the hardware via the secure registers. I think it should > > still be part of the driver instead of configuring the hardware in > > different places. I have proposed using ATF's high-level APIs to achieve this > when the driver is running in EL2. > > I have already proposed this in other email threads. > > Are you OK with this approach ? > > I think something more high level might be a good idea here. What do you mean by 'more high level' ? We handle this in SPL (EL3) ? Since you are the author of this 'drivers/net/dwmac_socfpga.c': https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/net/dwmac_socfpga.c#L101 https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/dts/socfpga_stratix10.dtsi#L98 Your driver selects the PHY interface (RGMII/RMII and etc) using the following register (part of System Manager): https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#topic/jng1505406892594.html I personally think this PHY interface select for EMACx shouldn't be part of System Manager. I don't see the security benefits here by making this PHY interface select as 'secure zone' register. Same applies to DW MMC driver as well: https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mmc/socfpga_dw_mmc.c#L60 It sets the following register in System Manager (secure zone) to configure the SDMMC clocks: https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#topic/gil1505406886282.html Don't you think these things should be part of driver itself as what we are doing now instead of removing these from drivers and place them in SPL (EL3)?
> > On 2/24/20 3:21 AM, Ang, Chee Hong wrote: > > [...] > > > > >>>>> Currently, we have like 20+ secure registers allowed access by > > >>>>> drivers running in non-secure mode (U-Boot proper / Linux). > > >>>>> I don't think we want to define and maintain those high level > > >>>>> interfaces for each of those secure register accesses in ATF and U-Boot. > > >>>> > > >>>> See above. > > >>> OK. Then these secure access register should be set up in SPL (EL3). > > >>> U-Boot drivers shouldn't access them at all because the driver may > > >>> be running in SPL(EL3) and in U-Boot proper (EL2) too. > > >>> I can take a look at those drivers accessing secure registers and > > >>> try to move/decouple those secure access from U-Boot drivers to > > >>> SPL > > >>> (EL3) then we no longer need those secure register access functions. > > >> > > >> I think that would be great, no ? > > > Since the SDMMC/DWMAC drivers read the device tree to configure the > > > behaviour of the hardware via the secure registers. I think it > > > should still be part of the driver instead of configuring the > > > hardware in different places. I have proposed using ATF's high-level > > > APIs to achieve this > > when the driver is running in EL2. > > > I have already proposed this in other email threads. > > > Are you OK with this approach ? > > > > I think something more high level might be a good idea here. > What do you mean by 'more high level' ? > We handle this in SPL (EL3) ? > > Since you are the author of this 'drivers/net/dwmac_socfpga.c': > https://gitlab.denx.de/u-boot/u- > boot/blob/master/drivers/net/dwmac_socfpga.c#L101 > https://gitlab.denx.de/u-boot/u- > boot/blob/master/arch/arm/dts/socfpga_stratix10.dtsi#L98 > > Your driver selects the PHY interface (RGMII/RMII and etc) using the following > register (part of System Manager): > https://www.intel.com/content/www/us/en/programmable/hps/stratix- > 10/hps.html#topic/jng1505406892594.html > > I personally think this PHY interface select for EMACx shouldn't be part of > System Manager. > I don't see the security benefits here by making this PHY interface select as > 'secure zone' register. > > Same applies to DW MMC driver as well: > https://gitlab.denx.de/u-boot/u- > boot/blob/master/drivers/mmc/socfpga_dw_mmc.c#L60 > > It sets the following register in System Manager (secure zone) to configure the > SDMMC clocks: > https://www.intel.com/content/www/us/en/programmable/hps/stratix- > 10/hps.html#topic/gil1505406886282.html > > Don't you think these things should be part of driver itself as what we are doing > now instead of removing these from drivers and place them in SPL (EL3)? These 2 drivers that access the system manager: drivers/mmc/socfpga_dw_mmc.c - MMC driver access System Manager's 'SDMMC' register to set clock phase https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/topic/gil1505406886282.html drivers/net/dwmac_socfpga.c - MAC driver access System Manager's 'EMACx' registers to set MAC's PHY interface: https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#topic/jng1505406892594.html Gen5/Arria10/Stratix10 & Agilex all using these drivers. They do not cause any issue in Gen5/Arria10 because there is no 'secure zone' in System Manager. For Stratix10 & Agilex, it has issue with U-Boot proper running in EL2. I don't think is good idea to separate those System Manager access from these 2 drivers and move them to SPL as they are shared by all SOCFPGA platforms. I think the proper way to resolve this is we add a new System Manager driver under 'drivers/misc'. The device type should be 'UCLASS_MISC'. Then we make changes to those drivers (SDMMC/MAC) to access the System Manager through the System Manager driver by using 'misc_read(dev...)' & 'misc_write(dev...)' The System Manager driver should decide whether to access those 'secure zone' registers directly in EL3 or making SMC call to ATF to access the System Manager if it's running in EL2. Linux has similar MAC driver running in EL1 (non-secure) accessing System Manager: https://elixir.bootlin.com/linux/v5.5/source/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c#L302 Linux MAC driver access System Manager via this 'altr,system_manager' driver: https://elixir.bootlin.com/linux/v5.5/source/drivers/mfd/altera-sysmgr.c#L44 System Manager driver will make SMC/PSCI call to ATF to access the System Manager's registers.
Ang, Chee Hong <chee.hong.ang at intel.com> schrieb am Fr., 28. Feb. 2020, 03:53: > > > On 2/24/20 3:21 AM, Ang, Chee Hong wrote: > > > [...] > > > > > > >>>>> Currently, we have like 20+ secure registers allowed access by > > > >>>>> drivers running in non-secure mode (U-Boot proper / Linux). > > > >>>>> I don't think we want to define and maintain those high level > > > >>>>> interfaces for each of those secure register accesses in ATF and > U-Boot. > > > >>>> > > > >>>> See above. > > > >>> OK. Then these secure access register should be set up in SPL > (EL3). > > > >>> U-Boot drivers shouldn't access them at all because the driver may > > > >>> be running in SPL(EL3) and in U-Boot proper (EL2) too. > > > >>> I can take a look at those drivers accessing secure registers and > > > >>> try to move/decouple those secure access from U-Boot drivers to > > > >>> SPL > > > >>> (EL3) then we no longer need those secure register access > functions. > > > >> > > > >> I think that would be great, no ? > > > > Since the SDMMC/DWMAC drivers read the device tree to configure the > > > > behaviour of the hardware via the secure registers. I think it > > > > should still be part of the driver instead of configuring the > > > > hardware in different places. I have proposed using ATF's high-level > > > > APIs to achieve this > > > when the driver is running in EL2. > > > > I have already proposed this in other email threads. > > > > Are you OK with this approach ? > > > > > > I think something more high level might be a good idea here. > > What do you mean by 'more high level' ? > > We handle this in SPL (EL3) ? > > > > Since you are the author of this 'drivers/net/dwmac_socfpga.c': > > https://gitlab.denx.de/u-boot/u- > > boot/blob/master/drivers/net/dwmac_socfpga.c#L101 > > https://gitlab.denx.de/u-boot/u- > > boot/blob/master/arch/arm/dts/socfpga_stratix10.dtsi#L98 > > > > Your driver selects the PHY interface (RGMII/RMII and etc) using the > following > > register (part of System Manager): > > https://www.intel.com/content/www/us/en/programmable/hps/stratix- > > 10/hps.html#topic/jng1505406892594.html > > > > I personally think this PHY interface select for EMACx shouldn't be part > of > > System Manager. > > I don't see the security benefits here by making this PHY interface > select as > > 'secure zone' register. > > > > Same applies to DW MMC driver as well: > > https://gitlab.denx.de/u-boot/u- > > boot/blob/master/drivers/mmc/socfpga_dw_mmc.c#L60 > > > > It sets the following register in System Manager (secure zone) to > configure the > > SDMMC clocks: > > https://www.intel.com/content/www/us/en/programmable/hps/stratix- > > 10/hps.html#topic/gil1505406886282.html > > > > Don't you think these things should be part of driver itself as what we > are doing > > now instead of removing these from drivers and place them in SPL (EL3)? > > These 2 drivers that access the system manager: > drivers/mmc/socfpga_dw_mmc.c > - MMC driver access System Manager's 'SDMMC' register to set clock phase > > https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/topic/gil1505406886282.html > > drivers/net/dwmac_socfpga.c > - MAC driver access System Manager's 'EMACx' registers to set MAC's PHY > interface: > > https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#topic/jng1505406892594.html > > Gen5/Arria10/Stratix10 & Agilex all using these drivers. > They do not cause any issue in Gen5/Arria10 because there is no 'secure > zone' in System Manager. > For Stratix10 & Agilex, it has issue with U-Boot proper running in EL2. > > I don't think is good idea to separate those System Manager access from > these 2 drivers and move them to SPL as they are shared by all SOCFPGA > platforms. > > I think the proper way to resolve this is we add a new System Manager > driver under 'drivers/misc'. > The device type should be 'UCLASS_MISC'. I have a pending patchset that adds such a sysmgr driver at least for gen5. I haven't published it yet because the whole series as one makes gen5 SRAM overlow, but maybe that part can be split out... Regards, Simon Then we make changes to those drivers (SDMMC/MAC) to access the System > Manager through the System Manager driver by using 'misc_read(dev...)' & > 'misc_write(dev...)' > The System Manager driver should decide whether to access those 'secure > zone' registers directly in EL3 or making SMC call to ATF to access the > System Manager if it's running in EL2. > > Linux has similar MAC driver running in EL1 (non-secure) accessing System > Manager: > > https://elixir.bootlin.com/linux/v5.5/source/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c#L302 > > Linux MAC driver access System Manager via this 'altr,system_manager' > driver: > > https://elixir.bootlin.com/linux/v5.5/source/drivers/mfd/altera-sysmgr.c#L44 > System Manager driver will make SMC/PSCI call to ATF to access the System > Manager's registers. >
Ang, Chee Hong <chee.hong.ang at intel.com<mailto:chee.hong.ang at intel.com>> schrieb am Fr., 28. Feb. 2020, 03:53: > > On 2/24/20 3:21 AM, Ang, Chee Hong wrote: > > [...] > > > > >>>>> Currently, we have like 20+ secure registers allowed access by > > >>>>> drivers running in non-secure mode (U-Boot proper / Linux). > > >>>>> I don't think we want to define and maintain those high level > > >>>>> interfaces for each of those secure register accesses in ATF and U-Boot. > > >>>> > > >>>> See above. > > >>> OK. Then these secure access register should be set up in SPL (EL3). > > >>> U-Boot drivers shouldn't access them at all because the driver may > > >>> be running in SPL(EL3) and in U-Boot proper (EL2) too. > > >>> I can take a look at those drivers accessing secure registers and > > >>> try to move/decouple those secure access from U-Boot drivers to > > >>> SPL > > >>> (EL3) then we no longer need those secure register access functions. > > >> > > >> I think that would be great, no ? > > > Since the SDMMC/DWMAC drivers read the device tree to configure the > > > behaviour of the hardware via the secure registers. I think it > > > should still be part of the driver instead of configuring the > > > hardware in different places. I have proposed using ATF's high-level > > > APIs to achieve this > > when the driver is running in EL2. > > > I have already proposed this in other email threads. > > > Are you OK with this approach ? > > > > I think something more high level might be a good idea here. > What do you mean by 'more high level' ? > We handle this in SPL (EL3) ? > > Since you are the author of this 'drivers/net/dwmac_socfpga.c': > https://gitlab.denx.de/u-boot/u- > boot/blob/master/drivers/net/dwmac_socfpga.c#L101 > https://gitlab.denx.de/u-boot/u- > boot/blob/master/arch/arm/dts/socfpga_stratix10.dtsi#L98 > > Your driver selects the PHY interface (RGMII/RMII and etc) using the following > register (part of System Manager): > https://www.intel.com/content/www/us/en/programmable/hps/stratix- > 10/hps.html#topic/jng1505406892594.html > > I personally think this PHY interface select for EMACx shouldn't be part of > System Manager. > I don't see the security benefits here by making this PHY interface select as > 'secure zone' register. > > Same applies to DW MMC driver as well: > https://gitlab.denx.de/u-boot/u- > boot/blob/master/drivers/mmc/socfpga_dw_mmc.c#L60 > > It sets the following register in System Manager (secure zone) to configure the > SDMMC clocks: > https://www.intel.com/content/www/us/en/programmable/hps/stratix- > 10/hps.html#topic/gil1505406886282.html > > Don't you think these things should be part of driver itself as what we are doing > now instead of removing these from drivers and place them in SPL (EL3)? These 2 drivers that access the system manager: drivers/mmc/socfpga_dw_mmc.c - MMC driver access System Manager's 'SDMMC' register to set clock phase https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/topic/gil1505406886282.html drivers/net/dwmac_socfpga.c - MAC driver access System Manager's 'EMACx' registers to set MAC's PHY interface: https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#topic/jng1505406892594.html Gen5/Arria10/Stratix10 & Agilex all using these drivers. They do not cause any issue in Gen5/Arria10 because there is no 'secure zone' in System Manager. For Stratix10 & Agilex, it has issue with U-Boot proper running in EL2. I don't think is good idea to separate those System Manager access from these 2 drivers and move them to SPL as they are shared by all SOCFPGA platforms. I think the proper way to resolve this is we add a new System Manager driver under 'drivers/misc'. The device type should be 'UCLASS_MISC'. >I have a pending patchset that adds such a sysmgr driver at least for gen5. I haven't published it yet because the whole >series as one makes gen5 SRAM overlow, but maybe that part can be split out... > >Regards, >Simon Thanks. I am working on the changes. I will need to test that out to make sure the new sysmgr driver works on all socfpga platforms. Will let you guys review whether this is a better/cleaner solution to handle the system manager access. Then we make changes to those drivers (SDMMC/MAC) to access the System Manager through the System Manager driver by using 'misc_read(dev...)' & 'misc_write(dev...)' The System Manager driver should decide whether to access those 'secure zone' registers directly in EL3 or making SMC call to ATF to access the System Manager if it's running in EL2. Linux has similar MAC driver running in EL1 (non-secure) accessing System Manager: https://elixir.bootlin.com/linux/v5.5/source/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c#L302 Linux MAC driver access System Manager via this 'altr,system_manager' driver: https://elixir.bootlin.com/linux/v5.5/source/drivers/mfd/altera-sysmgr.c#L44 System Manager driver will make SMC/PSCI call to ATF to access the System Manager's registers.
diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile index 3310e92..e59587b 100644 --- a/arch/arm/mach-socfpga/Makefile +++ b/arch/arm/mach-socfpga/Makefile @@ -53,6 +53,12 @@ obj-y += wrap_pinmux_config_s10.o obj-y += wrap_pll_config_s10.o endif +ifndef CONFIG_SPL_BUILD +ifdef CONFIG_SPL_ATF +obj-y += secure_reg_helper.o +endif +endif + ifdef CONFIG_SPL_BUILD ifdef CONFIG_TARGET_SOCFPGA_GEN5 obj-y += spl_gen5.o diff --git a/arch/arm/mach-socfpga/include/mach/secure_reg_helper.h b/arch/arm/mach-socfpga/include/mach/secure_reg_helper.h new file mode 100644 index 0000000..de581fc --- /dev/null +++ b/arch/arm/mach-socfpga/include/mach/secure_reg_helper.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * Copyright (C) 2019 Intel Corporation <www.intel.com> + * + */ + +#ifndef _SECURE_REG_HELPER_H_ +#define _SECURE_REG_HELPER_H_ + +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); +void socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); +void socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 val); +#else +#define socfpga_secure_reg_read32 readl +#define socfpga_secure_reg_write32 writel +#define socfpga_secure_reg_update32 clrsetbits_le32 +#endif + +#endif /* _SECURE_REG_HELPER_H_ */ diff --git a/arch/arm/mach-socfpga/secure_reg_helper.c b/arch/arm/mach-socfpga/secure_reg_helper.c new file mode 100644 index 0000000..46658a2 --- /dev/null +++ b/arch/arm/mach-socfpga/secure_reg_helper.c @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2019 Intel Corporation <www.intel.com> + * + */ + +#include <common.h> +#include <hang.h> +#include <asm/io.h> +#include <asm/system.h> +#include <asm/arch/misc.h> +#include <linux/intel-smc.h> + +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr) +{ + int ret; + u64 ret_arg; + u64 args[1]; + + args[0] = (u64)reg_addr; + ret = invoke_smc(INTEL_SIP_SMC_REG_READ, args, 1, &ret_arg, 1); + if (ret) { + /* SMC call return error */ + hang(); + } + + return ret_arg; +} + +void socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr) +{ + int ret; + u64 args[2]; + + args[0] = (u64)reg_addr; + args[1] = val; + ret = invoke_smc(INTEL_SIP_SMC_REG_WRITE, args, 2, NULL, 0); + if (ret) { + /* SMC call return error */ + hang(); + } +} + +void socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 val) +{ + int ret; + u64 args[3]; + + args[0] = (u64)reg_addr; + args[1] = mask; + args[2] = val; + ret = invoke_smc(INTEL_SIP_SMC_REG_UPDATE, args, 3, NULL, 0); + if (ret) { + /* SMC call return error */ + hang(); + } +}