[PATCH 1/2] ARC: U-boot: check arguments paranoidly
Handle U-boot arguments paranoidly:
* don't allow to pass unknown tag.
* try to use external device tree blob only if corresponding tag
(TAG_DTB) is set.
* check that magic number is correct.
* don't check uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
NOTE:
If U-boot args are invalid we skip them and try to use embedded device
tree blob. We can't panic on invalid U-boot args as we really pass
invalid args due to bug in U-boot code.
This happens if we don't provide external DTB to U-boot and
don't set 'bootargs' U-boot environment variable (which is default
case at least for HSDK board) In that case we will pass
{r0 = 1 (bootargs in r2); r1 = 0; r2 = 0;} to linux which is invalid.
NOTE:
We can safely check U-boot magic value (0x0) in linux passed via
r1 register as U-boot pass it from the beginning.
While I'm at it refactor U-boot arguments handling code.
Signed-off-by: Eugeniy Paltsev
---
arch/arc/kernel/head.S | 5 +--
arch/arc/kernel/setup.c | 92 +++--
2 files changed, 69 insertions(+), 28 deletions(-)
diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
index 8b90d25a15cc..fccea361e896 100644
--- a/arch/arc/kernel/head.S
+++ b/arch/arc/kernel/head.S
@@ -93,10 +93,11 @@ ENTRY(stext)
#ifdef CONFIG_ARC_UBOOT_SUPPORT
; Uboot - kernel ABI
;r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
- ;r1 = magic number (board identity, unused as of now
+ ;r1 = magic number (always zero as of now)
;r2 = pointer to uboot provided cmdline or external DTB in mem
- ; These are handled later in setup_arch()
+ ; These are handled later in handle_uboot_args()
st r0, [@uboot_tag]
+ st r1, [@uboot_magic]
st r2, [@uboot_arg]
#endif
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index feb90093e6b1..84d394a37e79 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -36,7 +36,8 @@ unsigned int intr_to_DE_cnt;
/* Part of U-boot ABI: see head.S */
int __initdata uboot_tag;
-char __initdata *uboot_arg;
+int __initdata uboot_magic;
+unsigned int __initdata uboot_arg;
const struct machine_desc *machine_desc;
@@ -462,43 +463,82 @@ void setup_processor(void)
arc_chk_core_config();
}
-static inline int is_kernel(unsigned long addr)
+static inline bool uboot_arg_invalid(unsigned int addr)
{
- if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
- return 1;
- return 0;
+ /*
+* Check that it is a untranslated address (although MMU is not enabled
+* yet, it being a high address ensures this is not by fluke)
+*/
+ if (addr < PAGE_OFFSET)
+ return true;
+
+ /* Check that address doesn't clobber resident kernel image */
+ return addr >= (unsigned int)_stext && addr <= (unsigned int)_end;
}
-void __init setup_arch(char **cmdline_p)
+#define IGNORE_ARGS"Ignore U-boot args: "
+
+/* uboot_{tag, magic} values for U-boot - kernel ABI revision 0; see head.S */
+#define UBOOT_TAG_NONE 0
+#define UBOOT_TAG_CMDLINE 1
+#define UBOOT_TAG_DTB 2
+/* We always pass 0 as magic from U-boot */
+#define UBOOT_MAGIC_VAL0
+
+void __init handle_uboot_args(void)
{
+ bool use_embedded_dtb = true;
+ bool append_cmdline = false;
+
#ifdef CONFIG_ARC_UBOOT_SUPPORT
- /* make sure that uboot passed pointer to cmdline/dtb is valid */
- if (uboot_tag && is_kernel((unsigned long)uboot_arg))
- panic("Invalid uboot arg\n");
+ /* check that we know this tag */
+ if (uboot_tag != UBOOT_TAG_NONE &&
+ uboot_tag != UBOOT_TAG_CMDLINE &&
+ uboot_tag != UBOOT_TAG_DTB) {
+ pr_warn(IGNORE_ARGS "invalid uboot tag: '%08x'\n", uboot_tag);
+ goto ignore_uboot_args;
+ }
+
+ if (uboot_magic != UBOOT_MAGIC_VAL) {
+ pr_warn(IGNORE_ARGS "non zero uboot magic\n");
+ goto ignore_uboot_args;
+ }
+
+ if (uboot_tag != UBOOT_TAG_NONE && uboot_arg_invalid(uboot_arg)) {
+ pr_warn(IGNORE_ARGS "invalid uboot arg: '%08x'\n", uboot_arg);
+ goto ignore_uboot_args;
+ }
+
+ /* see if U-boot passed an external Device Tree blob */
+ if (uboot_tag == UBOOT_TAG_DTB) {
+ machine_desc = setup_machine_fdt((void *)uboot_arg);
+
+ /* external Device Tree blob is invalid - use embedded one */
+ use_embedded_dtb = !machine_desc;
+ }
+
+ if (uboot_tag == UBOOT_TAG_CMDLINE)
+ append_cmdline = true;
- /* See if u-boot passed an external Device Tree blob */
- machine_desc = setup_machine_fdt(uboot_arg);/* uboot_tag == 2 */
- if (!machine_desc)
+ignore_uboot_args:
#endif
- {
- /* No, so try the embedded one */
+
+ if (use_em
[PATCH 0/2] RC: rework U-boot arguments handling
Reworking U-boot args handling and enable uboot support unconditionally. Changes RFC->v1: * Don't add new ABI contract between kernel and uboot * Eliminate CONFIG_ARC_UBOOT_SUPPORT Kconfig option and enable uboot support unconditionally * Skip invalid U-boot args instead of panic * Check existing U-boot magic value * Improve uboot_arg validating * Minor code changes Eugeniy Paltsev (2): ARC: U-boot: check arguments paranoidly ARC: enable uboot support unconditionally arch/arc/Kconfig| 12 - arch/arc/configs/nps_defconfig | 1 - arch/arc/configs/vdk_hs38_defconfig | 1 - arch/arc/configs/vdk_hs38_smp_defconfig | 2 - arch/arc/kernel/head.S | 7 ++- arch/arc/kernel/setup.c | 96 +++-- 6 files changed, 70 insertions(+), 49 deletions(-) -- 2.14.5 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH 2/2] ARC: enable uboot support unconditionally
After reworking U-boot args handling code and adding paranoid
arguments check we can eliminate CONFIG_ARC_UBOOT_SUPPORT and
enable uboot support unconditionally.
For JTAG case we can assume that core registers will come up
reset value of 0 or in worst case we rely on user passing
'-on=clear_regs' to Metaware debugger.
Signed-off-by: Eugeniy Paltsev
---
arch/arc/Kconfig| 12
arch/arc/configs/nps_defconfig | 1 -
arch/arc/configs/vdk_hs38_defconfig | 1 -
arch/arc/configs/vdk_hs38_smp_defconfig | 2 --
arch/arc/kernel/head.S | 2 --
arch/arc/kernel/setup.c | 2 --
6 files changed, 20 deletions(-)
diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
index 376366a7db81..f9534417b201 100644
--- a/arch/arc/Kconfig
+++ b/arch/arc/Kconfig
@@ -191,7 +191,6 @@ config NR_CPUS
config ARC_SMP_HALT_ON_RESET
bool "Enable Halt-on-reset boot mode"
- default y if ARC_UBOOT_SUPPORT
help
In SMP configuration cores can be configured as Halt-on-reset
or they could all start at same time. For Halt-on-reset, non
@@ -515,17 +514,6 @@ config ARC_DBG_TLB_PARANOIA
endif
-config ARC_UBOOT_SUPPORT
- bool "Support uboot arg Handling"
- help
- ARC Linux by default checks for uboot provided args as pointers to
- external cmdline or DTB. This however breaks in absence of uboot,
- when booting from Metaware debugger directly, as the registers are
- not zeroed out on reset by mdb and/or ARCv2 based cores. The bogus
- registers look like uboot args to kernel which then chokes.
- So only enable the uboot arg checking/processing if users are sure
- of uboot being in play.
-
config ARC_BUILTIN_DTB_NAME
string "Built in DTB"
help
diff --git a/arch/arc/configs/nps_defconfig b/arch/arc/configs/nps_defconfig
index 6e84060e7c90..621f59407d76 100644
--- a/arch/arc/configs/nps_defconfig
+++ b/arch/arc/configs/nps_defconfig
@@ -31,7 +31,6 @@ CONFIG_ARC_CACHE_LINE_SHIFT=5
# CONFIG_ARC_HAS_LLSC is not set
CONFIG_ARC_KVADDR_SIZE=402
CONFIG_ARC_EMUL_UNALIGNED=y
-CONFIG_ARC_UBOOT_SUPPORT=y
CONFIG_PREEMPT=y
CONFIG_NET=y
CONFIG_UNIX=y
diff --git a/arch/arc/configs/vdk_hs38_defconfig
b/arch/arc/configs/vdk_hs38_defconfig
index 1e59a2e9c602..e447ace6fa1c 100644
--- a/arch/arc/configs/vdk_hs38_defconfig
+++ b/arch/arc/configs/vdk_hs38_defconfig
@@ -13,7 +13,6 @@ CONFIG_PARTITION_ADVANCED=y
CONFIG_ARC_PLAT_AXS10X=y
CONFIG_AXS103=y
CONFIG_ISA_ARCV2=y
-CONFIG_ARC_UBOOT_SUPPORT=y
CONFIG_ARC_BUILTIN_DTB_NAME="vdk_hs38"
CONFIG_PREEMPT=y
CONFIG_NET=y
diff --git a/arch/arc/configs/vdk_hs38_smp_defconfig
b/arch/arc/configs/vdk_hs38_smp_defconfig
index b5c3f6c54b03..c82cdb10aaf4 100644
--- a/arch/arc/configs/vdk_hs38_smp_defconfig
+++ b/arch/arc/configs/vdk_hs38_smp_defconfig
@@ -15,8 +15,6 @@ CONFIG_AXS103=y
CONFIG_ISA_ARCV2=y
CONFIG_SMP=y
# CONFIG_ARC_TIMERS_64BIT is not set
-# CONFIG_ARC_SMP_HALT_ON_RESET is not set
-CONFIG_ARC_UBOOT_SUPPORT=y
CONFIG_ARC_BUILTIN_DTB_NAME="vdk_hs38_smp"
CONFIG_PREEMPT=y
CONFIG_NET=y
diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
index fccea361e896..4b0deaff001c 100644
--- a/arch/arc/kernel/head.S
+++ b/arch/arc/kernel/head.S
@@ -90,7 +90,6 @@ ENTRY(stext)
st.ab 0, [r5, 4]
1:
-#ifdef CONFIG_ARC_UBOOT_SUPPORT
; Uboot - kernel ABI
;r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
;r1 = magic number (always zero as of now)
@@ -99,7 +98,6 @@ ENTRY(stext)
st r0, [@uboot_tag]
st r1, [@uboot_magic]
st r2, [@uboot_arg]
-#endif
; setup "current" tsk and optionally cache it in dedicated r25
mov r9, @init_task
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index 84d394a37e79..fff946b0ab4f 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -490,7 +490,6 @@ void __init handle_uboot_args(void)
bool use_embedded_dtb = true;
bool append_cmdline = false;
-#ifdef CONFIG_ARC_UBOOT_SUPPORT
/* check that we know this tag */
if (uboot_tag != UBOOT_TAG_NONE &&
uboot_tag != UBOOT_TAG_CMDLINE &&
@@ -521,7 +520,6 @@ void __init handle_uboot_args(void)
append_cmdline = true;
ignore_uboot_args:
-#endif
if (use_embedded_dtb) {
machine_desc = setup_machine_fdt(__dtb_start);
--
2.14.5
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 1/2] ARC: U-boot: check arguments paranoidly
On Tue, Feb 12, 2019 at 06:39:31PM +0300, Eugeniy Paltsev wrote:
> Handle U-boot arguments paranoidly:
> * don't allow to pass unknown tag.
> * try to use external device tree blob only if corresponding tag
>(TAG_DTB) is set.
> * check that magic number is correct.
> * don't check uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
>
> NOTE:
> If U-boot args are invalid we skip them and try to use embedded device
> tree blob. We can't panic on invalid U-boot args as we really pass
> invalid args due to bug in U-boot code.
> This happens if we don't provide external DTB to U-boot and
> don't set 'bootargs' U-boot environment variable (which is default
> case at least for HSDK board) In that case we will pass
> {r0 = 1 (bootargs in r2); r1 = 0; r2 = 0;} to linux which is invalid.
>
> NOTE:
> We can safely check U-boot magic value (0x0) in linux passed via
> r1 register as U-boot pass it from the beginning.
>
> While I'm at it refactor U-boot arguments handling code.
>
Hello
I have tried to test this serie, but this patch does not apply anymore on
current next tree.
It conflicts with "ARC: boot: robustify u-boot arg referencing".
Regards
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 1/2] ARC: U-boot: check arguments paranoidly
On 2/12/19 8:39 AM, LABBE Corentin wrote: >> While I'm at it refactor U-boot arguments handling code. >> > Hello > > I have tried to test this serie, but this patch does not apply anymore on > current next tree. > It conflicts with "ARC: boot: robustify u-boot arg referencing". I was carrying that patch in the interim - now dropped and pushed. -Vineet ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 1/2] ARC: U-boot: check arguments paranoidly
On 2/12/19 7:39 AM, Eugeniy Paltsev wrote:
> Handle U-boot arguments paranoidly:
> * don't allow to pass unknown tag.
> * try to use external device tree blob only if corresponding tag
>(TAG_DTB) is set.
> * check that magic number is correct.
> * don't check uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
>
> NOTE:
> If U-boot args are invalid we skip them and try to use embedded device
> tree blob. We can't panic on invalid U-boot args as we really pass
> invalid args due to bug in U-boot code.
> This happens if we don't provide external DTB to U-boot and
> don't set 'bootargs' U-boot environment variable (which is default
> case at least for HSDK board) In that case we will pass
> {r0 = 1 (bootargs in r2); r1 = 0; r2 = 0;} to linux which is invalid.
>
> NOTE:
> We can safely check U-boot magic value (0x0) in linux passed via
> r1 register as U-boot pass it from the beginning.
>
> While I'm at it refactor U-boot arguments handling code.
>
> Signed-off-by: Eugeniy Paltsev
> ---
> arch/arc/kernel/head.S | 5 +--
> arch/arc/kernel/setup.c | 92
> +++--
> 2 files changed, 69 insertions(+), 28 deletions(-)
>
> diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
> index 8b90d25a15cc..fccea361e896 100644
> --- a/arch/arc/kernel/head.S
> +++ b/arch/arc/kernel/head.S
> @@ -93,10 +93,11 @@ ENTRY(stext)
> #ifdef CONFIG_ARC_UBOOT_SUPPORT
> ; Uboot - kernel ABI
> ;r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
> - ;r1 = magic number (board identity, unused as of now
> + ;r1 = magic number (always zero as of now)
This is technically changing the ABI - I think we don't need to enforce this -
keep ignoring this
> ;r2 = pointer to uboot provided cmdline or external DTB in mem
> - ; These are handled later in setup_arch()
> + ; These are handled later in handle_uboot_args()
> st r0, [@uboot_tag]
> + st r1, [@uboot_magic]
> st r2, [@uboot_arg]
> #endif
>
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index feb90093e6b1..84d394a37e79 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -36,7 +36,8 @@ unsigned int intr_to_DE_cnt;
>
> /* Part of U-boot ABI: see head.S */
> int __initdata uboot_tag;
> -char __initdata *uboot_arg;
> +int __initdata uboot_magic;
> +unsigned int __initdata uboot_arg;
>
> const struct machine_desc *machine_desc;
>
> @@ -462,43 +463,82 @@ void setup_processor(void)
> arc_chk_core_config();
> }
>
> -static inline int is_kernel(unsigned long addr)
> +static inline bool uboot_arg_invalid(unsigned int addr)
> {
> - if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
> - return 1;
> - return 0;
> + /*
> + * Check that it is a untranslated address (although MMU is not enabled
> + * yet, it being a high address ensures this is not by fluke)
> + */
> + if (addr < PAGE_OFFSET)
> + return true;
> +
> + /* Check that address doesn't clobber resident kernel image */
> + return addr >= (unsigned int)_stext && addr <= (unsigned int)_end;
> }
>
> -void __init setup_arch(char **cmdline_p)
> +#define IGNORE_ARGS "Ignore U-boot args: "
> +
> +/* uboot_{tag, magic} values for U-boot - kernel ABI revision 0; see head.S
> */
> +#define UBOOT_TAG_NONE 0
> +#define UBOOT_TAG_CMDLINE1
> +#define UBOOT_TAG_DTB2
> +/* We always pass 0 as magic from U-boot */
> +#define UBOOT_MAGIC_VAL 0
> +
> +void __init handle_uboot_args(void)
> {
> + bool use_embedded_dtb = true;
> + bool append_cmdline = false;
> +
> #ifdef CONFIG_ARC_UBOOT_SUPPORT
> - /* make sure that uboot passed pointer to cmdline/dtb is valid */
> - if (uboot_tag && is_kernel((unsigned long)uboot_arg))
> - panic("Invalid uboot arg\n");
> + /* check that we know this tag */
> + if (uboot_tag != UBOOT_TAG_NONE &&
> + uboot_tag != UBOOT_TAG_CMDLINE &&
> + uboot_tag != UBOOT_TAG_DTB) {
> + pr_warn(IGNORE_ARGS "invalid uboot tag: '%08x'\n", uboot_tag);
> + goto ignore_uboot_args;
> + }
> +
> + if (uboot_magic != UBOOT_MAGIC_VAL) {
> + pr_warn(IGNORE_ARGS "non zero uboot magic\n");
> + goto ignore_uboot_args;
> + }
Not needed per above.
> +
> + if (uboot_tag != UBOOT_TAG_NONE && uboot_arg_invalid(uboot_arg)) {
> + pr_warn(IGNORE_ARGS "invalid uboot arg: '%08x'\n", uboot_arg);
> + goto ignore_uboot_args;
> + }
> +
> + /* see if U-boot passed an external Device Tree blob */
> + if (uboot_tag == UBOOT_TAG_DTB) {
> + machine_desc = setup_machine_fdt((void *)uboot_arg);
> +
> + /* external Device Tree blob is invalid - use embedded one */
> + use_embedded_dtb = !machine_desc;
> + }
> +
> + if (uboot_tag == UBOOT_
Re: [PATCH] ARC: Explicitly set ARCH_SLAB_MINALIGN = 8
On 2/8/19 2:55 AM, Alexey Brodkin wrote: > By default ARCH_SLAB_MINALIGN is defined in "include/linux/slab.h" as > "__alignof__(unsigned long long)" which looks fine but not for ARC. Just for the record, the issue happens because a LLOCKD (exclusive 64-bit load) was trying to use a 32-bit aligned effective address (for atomic64_t), not allowed by ISA (LLOCKD can only take 64-bit aligned address, even when the CPU has unaligned access enabled). This in turn was happening because this word is embedded in some other struct and happens to be 4 byte aligned > ARC tools ABI sets align of "long long" the same as for "long" = 4 > instead of 8 one may think of. Right, this was indeed unexpected and not like most other arches. ARCv2 ISA allows regular 64-bit loads/stores (LDD/STD) to take 32-bit aligned addresses. Thus ABI relaxing the alignment for 64-bit data potentially causes more packing and less space waste. But on the flip side we need to waste space at arbitrary places liek this. So this is all good and theory, but I'm not 100% sure how slab alignment helps here (and is future proof). So the outer struct with embedded atomic64_t was allocated via slab and your patch ensures that outer struct is 64-bit aligned ? But how does that guarantee that all embedded atomic64_t in there will be 64-bit aligned (in future say) in the light of ARC ABI and the gcc bug/feature which Peter alluded to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10360 > Thus slab allocator may easily allocate a buffer which is 32-bit aligned. > And most of the time it's OK until we start dealing with 64-bit atomics > with special LLOCKD/SCONDD instructions which (as opposed to their 32-bit > counterparts LLOCK/SCOND) operate with full 64-bit words but those words > must be 64-bit aligned. Some of this text needed to go above to give more context. > > Fixes Ext4 folder removal: > ->8--- > [4.015732] EXT4-fs (mmcblk0p2): re-mounted. Opts: (null) > [4.167881] .. > > Signed-off-by: Alexey Brodkin > Cc: # 4.8+ > --- > arch/arc/include/asm/cache.h | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/arc/include/asm/cache.h b/arch/arc/include/asm/cache.h > index f393b663413e..74f8fcaaef5c 100644 > --- a/arch/arc/include/asm/cache.h > +++ b/arch/arc/include/asm/cache.h > @@ -52,6 +52,16 @@ > #define cache_line_size()SMP_CACHE_BYTES > #define ARCH_DMA_MINALIGNSMP_CACHE_BYTES > > +/* > + * Make sure slab-allocated buffers are 64-bit aligned. > + * This is required for llockd/scondd to deal with 64-bit aligned dwords. > + * By default ARCH_SLAB_MINALIGN is __alignof__(long long) which in > + * case of ARC is 4 instead of 8! > + */ > +#ifdef CONFIG_ARC_HAS_LL64 > +#define ARCH_SLAB_MINALIGN 8 > +#endif > + > extern void arc_cache_init(void); > extern char *arc_cache_mumbojumbo(int cpu_id, char *buf, int len); > extern void read_decode_cache_bcr(void); > ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 1/2] ARC: U-boot: check arguments paranoidly
On Tue, 2019-02-12 at 16:45 +, Vineet Gupta wrote:
> On 2/12/19 7:39 AM, Eugeniy Paltsev wrote:
> > Handle U-boot arguments paranoidly:
> > * don't allow to pass unknown tag.
> > * try to use external device tree blob only if corresponding tag
> >(TAG_DTB) is set.
> > * check that magic number is correct.
> > * don't check uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
> >
> > NOTE:
> > If U-boot args are invalid we skip them and try to use embedded device
> > tree blob. We can't panic on invalid U-boot args as we really pass
> > invalid args due to bug in U-boot code.
> > This happens if we don't provide external DTB to U-boot and
> > don't set 'bootargs' U-boot environment variable (which is default
> > case at least for HSDK board) In that case we will pass
> > {r0 = 1 (bootargs in r2); r1 = 0; r2 = 0;} to linux which is invalid.
> >
> > NOTE:
> > We can safely check U-boot magic value (0x0) in linux passed via
> > r1 register as U-boot pass it from the beginning.
> >
> > While I'm at it refactor U-boot arguments handling code.
> >
> > Signed-off-by: Eugeniy Paltsev
> > ---
> > arch/arc/kernel/head.S | 5 +--
> > arch/arc/kernel/setup.c | 92
> > +++--
> > 2 files changed, 69 insertions(+), 28 deletions(-)
> >
> > diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
> > index 8b90d25a15cc..fccea361e896 100644
> > --- a/arch/arc/kernel/head.S
> > +++ b/arch/arc/kernel/head.S
> > @@ -93,10 +93,11 @@ ENTRY(stext)
> > #ifdef CONFIG_ARC_UBOOT_SUPPORT
> > ; Uboot - kernel ABI
> > ;r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
> > - ;r1 = magic number (board identity, unused as of now
> > + ;r1 = magic number (always zero as of now)
>
> This is technically changing the ABI - I think we don't need to enforce this -
> keep ignoring this
I think it's better to add this check:
* This check doesn't break backward compatibility. ARC U-boot pass zero to r1
from the beginnings, I specially checked this. So we doesn't change ABI,
we only document it ;)
* By adding this check we can cheap and easily minimize problems in JTAG case.
> > +
> > + if (use_embedded_dtb) {
> > machine_desc = setup_machine_fdt(__dtb_start);
> > if (!machine_desc)
> > panic("Embedded DT invalid\n");
> > + }
> >
> > - /*
> > -* If we are here, it is established that @uboot_arg didn't
> > -* point to DT blob. Instead if u-boot says it is cmdline,
> > -* append to embedded DT cmdline.
> > -* setup_machine_fdt() would have populated @boot_command_line
> > -*/
>
> Don't drop this comment, specially the last line. If was tempted to move the
> cmd
> line processing before but this saved me since we rely on setup_machine_fdt()
> being called aprioiri.
Ok, will fix in v2
> > - if (uboot_tag == 1) {
> > - /* Ensure a whitespace between the 2 cmdlines */
> > - strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> > - strlcat(boot_command_line, uboot_arg,
> > - COMMAND_LINE_SIZE);
> > - }
> > + if (append_cmdline) {
> > + /* Ensure a whitespace between the 2 cmdlines */
> > + strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> > + strlcat(boot_command_line, (char *)uboot_arg,
> > COMMAND_LINE_SIZE);
> > }
> > +}
> > +
> > +void __init setup_arch(char **cmdline_p)
> > +{
> > + handle_uboot_args();
> >
> > /* Save unparsed command line copy for /proc/cmdline */
> > *cmdline_p = boot_command_line;
>
>
--
Eugeniy Paltsev
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
RE: [PATCH] ARC: Explicitly set ARCH_SLAB_MINALIGN = 8
From: Vineet Gupta > Sent: 12 February 2019 17:17 > > On 2/8/19 2:55 AM, Alexey Brodkin wrote: > > By default ARCH_SLAB_MINALIGN is defined in "include/linux/slab.h" as > > "__alignof__(unsigned long long)" which looks fine but not for ARC. > > Just for the record, the issue happens because a LLOCKD (exclusive 64-bit > load) > was trying to use a 32-bit aligned effective address (for atomic64_t), not > allowed > by ISA (LLOCKD can only take 64-bit aligned address, even when the CPU has > unaligned access enabled). > > This in turn was happening because this word is embedded in some other struct > and > happens to be 4 byte aligned > > > > ARC tools ABI sets align of "long long" the same as for "long" = 4 > > instead of 8 one may think of. Right, but __alignof__() doesn't have to return the alignment that would be used for a data item of the specified type. (Read the gcc 'bug' info for gory details.) On i386 __alignof__(long long) is 8, but structure members of type 'long long' are 4 byte aligned and the alignment of a structure with a 'long long' member is only 4. (Although the microsoft compiler returns 4.) > Right, this was indeed unexpected and not like most other arches. ARCv2 ISA > allows > regular 64-bit loads/stores (LDD/STD) to take 32-bit aligned addresses. Thus > ABI > relaxing the alignment for 64-bit data potentially causes more packing and > less > space waste. But on the flip side we need to waste space at arbitrary places > liek > this. > > So this is all good and theory, but I'm not 100% sure how slab alignment helps > here (and is future proof). So the outer struct with embedded atomic64_t was > allocated via slab and your patch ensures that outer struct is 64-bit aligned > ? Presumable 'atomic64_t' has an alignment attribute to force 8 byte alignment. > But how does that guarantee that all embedded atomic64_t in there will be > 64-bit > aligned (in future say) in the light of ARC ABI and the gcc bug/feature which > Peter alluded to > >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188 >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10360 > > > Thus slab allocator may easily allocate a buffer which is 32-bit aligned. > > And most of the time it's OK until we start dealing with 64-bit atomics > > with special LLOCKD/SCONDD instructions which (as opposed to their 32-bit > > counterparts LLOCK/SCOND) operate with full 64-bit words but those words > > must be 64-bit aligned. > > Some of this text needed to go above to give more context. I suspect the slab allocator should be returning 8 byte aligned addresses on all systems David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 1/2] ARC: U-boot: check arguments paranoidly
On 2/12/19 9:25 AM, Eugeniy Paltsev wrote: >> This is technically changing the ABI - I think we don't need to enforce this >> - >> keep ignoring this > I think it's better to add this check: > * This check doesn't break backward compatibility. ARC U-boot pass zero to r1 >from the beginnings, I specially checked this. So we doesn't change ABI, >we only document it ;) OK good. > * By adding this check we can cheap and easily minimize problems in JTAG > case. Prior to your patch this register was irrelevant - it didn't matter for jtag or uboot cause what its value was since it was not being checked at all. Now you enforce this be 0. Simple reasoning tells me it will likely cause problems, if any, but won't reduce them at all. So I'd insist we keep ignoring it even if uboot was zeroing it out. Atleast this leaves the door open any future changes. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] ARC: Explicitly set ARCH_SLAB_MINALIGN = 8
+CC some folks interested in alignment stuff in the past.
On 2/12/19 9:30 AM, David Laight wrote:
> From: Vineet Gupta
>> Sent: 12 February 2019 17:17
>>
>> On 2/8/19 2:55 AM, Alexey Brodkin wrote:
>>> By default ARCH_SLAB_MINALIGN is defined in "include/linux/slab.h" as
>>> "__alignof__(unsigned long long)" which looks fine but not for ARC.
>>
>> Just for the record, the issue happens because a LLOCKD (exclusive 64-bit
>> load)
>> was trying to use a 32-bit aligned effective address (for atomic64_t), not
>> allowed
>> by ISA (LLOCKD can only take 64-bit aligned address, even when the CPU has
>> unaligned access enabled).
>>
>> This in turn was happening because this word is embedded in some other
>> struct and
>> happens to be 4 byte aligned
>>
>>
>>> ARC tools ABI sets align of "long long" the same as for "long" = 4
>>> instead of 8 one may think of.
>
> Right, but __alignof__() doesn't have to return the alignment that would
> be used for a data item of the specified type.
> (Read the gcc 'bug' info for gory details.)
>
> On i386 __alignof__(long long) is 8, but structure members of type 'long long'
> are 4 byte aligned and the alignment of a structure with a 'long long' member
> is only 4.
> (Although the microsoft compiler returns 4.)
Exactly my point that this fudging of outer alignment is no magic bullet.
>
>> Right, this was indeed unexpected and not like most other arches. ARCv2 ISA
>> allows
>> regular 64-bit loads/stores (LDD/STD) to take 32-bit aligned addresses. Thus
>> ABI
>> relaxing the alignment for 64-bit data potentially causes more packing and
>> less
>> space waste. But on the flip side we need to waste space at arbitrary places
>> liek
>> this.
>>
>> So this is all good and theory, but I'm not 100% sure how slab alignment
>> helps
>> here (and is future proof). So the outer struct with embedded atomic64_t was
>> allocated via slab and your patch ensures that outer struct is 64-bit
>> aligned ?
>
> Presumable 'atomic64_t' has an alignment attribute to force 8 byte alignment.
It does for ARC
typedef struct {
aligned_u64 counter;
} atomic64_t;
But what was your point ?
>
>> But how does that guarantee that all embedded atomic64_t in there will be
>> 64-bit
>> aligned (in future say) in the light of ARC ABI and the gcc bug/feature which
>> Peter alluded to
>>
>>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188
>>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10360
>>
>>> Thus slab allocator may easily allocate a buffer which is 32-bit aligned.
>>> And most of the time it's OK until we start dealing with 64-bit atomics
>>> with special LLOCKD/SCONDD instructions which (as opposed to their 32-bit
>>> counterparts LLOCK/SCOND) operate with full 64-bit words but those words
>>> must be 64-bit aligned.
>>
>> Some of this text needed to go above to give more context.
>
> I suspect the slab allocator should be returning 8 byte aligned addresses
> on all systems
why ? As I understand it is still not fool proof against the expected alignment
of
inner members. There ought to be a better way to enforce all this.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 04/12] of: select OF_RESERVED_MEM automatically
On Mon, Feb 11, 2019 at 7:37 AM Christoph Hellwig wrote: > > The OF_RESERVED_MEM can be used if we have either CMA or the generic > declare coherent code built and we support the early flattened DT. > > So don't bother making it a user visible options that is selected > by most configs that fit the above category, but just select it when > the requirements are met. > > Signed-off-by: Christoph Hellwig > --- > arch/arc/Kconfig | 1 - > arch/arm/Kconfig | 1 - > arch/arm64/Kconfig | 1 - > arch/csky/Kconfig| 1 - > arch/powerpc/Kconfig | 1 - > arch/xtensa/Kconfig | 1 - > drivers/of/Kconfig | 5 ++--- > 7 files changed, 2 insertions(+), 9 deletions(-) Reviewed-by: Rob Herring ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 03/12] of: mark early_init_dt_alloc_reserved_memory_arch static
On Mon, Feb 11, 2019 at 7:36 AM Christoph Hellwig wrote: > > This function is only used in of_reserved_mem.c, and never overridden > despite the __weak marker. > > Signed-off-by: Christoph Hellwig > --- > drivers/of/of_reserved_mem.c| 2 +- > include/linux/of_reserved_mem.h | 7 --- > 2 files changed, 1 insertion(+), 8 deletions(-) Reviewed-by: Rob Herring Looks like this one isn't a dependency, so I can take it if you want. Rob ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 06/12] dma-mapping: improve selection of dma_declare_coherent availability
On Mon, Feb 11, 2019 at 7:37 AM Christoph Hellwig wrote: > > This API is primarily used through DT entries, but two architectures > and two drivers call it directly. So instead of selecting the config > symbol for random architectures pull it in implicitly for the actual > users. Also rename the Kconfig option to describe the feature better. > > Signed-off-by: Christoph Hellwig > --- > arch/arc/Kconfig| 1 - > arch/arm/Kconfig| 2 +- > arch/arm64/Kconfig | 1 - > arch/csky/Kconfig | 1 - > arch/mips/Kconfig | 1 - > arch/riscv/Kconfig | 1 - > arch/sh/Kconfig | 2 +- > arch/unicore32/Kconfig | 1 - > arch/x86/Kconfig| 1 - > drivers/mfd/Kconfig | 2 ++ > drivers/of/Kconfig | 3 ++- > include/linux/device.h | 2 +- > include/linux/dma-mapping.h | 8 > kernel/dma/Kconfig | 2 +- > kernel/dma/Makefile | 2 +- > 15 files changed, 13 insertions(+), 17 deletions(-) > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > index 3607fd2810e4..f8c66a9472a4 100644 > --- a/drivers/of/Kconfig > +++ b/drivers/of/Kconfig > @@ -43,6 +43,7 @@ config OF_FLATTREE > > config OF_EARLY_FLATTREE > bool > + select DMA_DECLARE_COHERENT Is selecting DMA_DECLARE_COHERENT okay on UML? We run the unittests with UML. Maybe we should just get rid of OF_RESERVED_MEM. If we support booting from DT, then it should always be enabled anyways. > select OF_FLATTREE > > config OF_PROMTREE > @@ -83,7 +84,7 @@ config OF_MDIO > config OF_RESERVED_MEM > bool > depends on OF_EARLY_FLATTREE > - default y if HAVE_GENERIC_DMA_COHERENT || DMA_CMA > + default y if DMA_DECLARE_COHERENT || DMA_CMA > > config OF_RESOLVE > bool ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Semantics of symbol address in perf report -v
On 1/23/19 11:55 AM, Vineet Gupta wrote:
> Hi,
>
> I noticed a small anomaly in perf report -v output on ARC and x86 as well.
>
> A simple program which sits in tight loop, compiled for x86_64
>
> void main() { while(1) {} }
>
> $ gcc -g tight.c
> $ ./a.out &
> $ perf record -e cycles -p 26703
> $ perf report -n -v --stdio | egrep "(main|Symbol)"
>
> |# Overhead Samples Command Shared Object Symbol
>
> | 99.93%55753 a.out /home/arc/test/a.out 0x4da B [.] main
>
> | ^
>
> The printer address for Symbols is *not* the actual address in elf, but
> rather VMA
> start offset.
>
> 0x4da = 0x4004da - 0x0040
>
> $ objdump -d ./a.out
>
> | 004004d6 :
> | 4004d6:55 push %rbp
> | 4004d7:48 89 e5mov%rsp,%rbp
> | 4004da:eb fe jmp4004da
> | 4004dc:0f 1f 40 00 nopl 0x0(%rax)
>
> $ readelf -a ./a.out
>
> | Program Headers:
> | Type Offset VirtAddr PhysAddr
> | FileSizMemSiz Flags Align
> | Program Headers:
> | LOAD 0x 0x0040 0x0040
> 0x068c 0x068c R E20
>
>
> This is problematic in narrowing down the hotspot instruction, when the binary
> itself is. One needs to do the offset addition manually to find the actual
> hotspot
> location.
>
> | 99.79% 100064 a.out /home/arc/test/a.out 0x4da B [.]
> 0x04da
>
>
>
>
> Is this considered an issue ? Would the fix to print the actual symbol address
> (and recorded in raw perf event data) break some existing tooling etc.
>
@Arnaldo any ideas ?
this is being done in tools/perf/util/symbol-elf.c
symsrc__init
if (dso->kernel == DSO_TYPE_USER)
ss->adjust_symbols = true;
dso__load_sym
dso->adjust_symbols = runtime_ss->adjust_symbols || ref_reloc(kmap);
} else if ((used_opd && runtime_ss->adjust_symbols) ||
(!used_opd && syms_ss->adjust_symbols)) {
*sym.st_value -= shdr.sh_addr - shdr.sh_offset;*
}
I investigated a bit more and this goes back to 2009:
Initially the adj was done for prelink binaries: commit f5812a7a336fb
("perf_counter tools: Adjust only prelinked symbol's addresses")
+ self->prelinked = elf_section_by_name(elf, &ehdr, &shdr,
+ ".gnu.prelink_undo",
+ NULL) != NULL;
+ if (self->prelinked) {
if (verbose >= 2)
printf("adjusting symbol: ...
sym.st_value -= shdr.sh_addr - shdr.sh_offset;
+ }
commit 30d7a77dd5a97 ("perf_counter tools: Adjust symbols in ET_EXEC files too")
started doing this for any executable
- self->prelinked = elf_section_by_name(elf, &ehdr, &shdr,
- ".gnu.prelink_undo",
- NULL) != NULL;
+ self->adjust_symbols = (ehdr.e_type == ET_EXEC ||
+ elf_section_by_name(elf, &ehdr, &shdr,
+".gnu.prelink_undo",
+
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 06/12] dma-mapping: improve selection of dma_declare_coherent availability
On Mon, 11 Feb 2019, Christoph Hellwig wrote: > This API is primarily used through DT entries, but two architectures > and two drivers call it directly. So instead of selecting the config > symbol for random architectures pull it in implicitly for the actual > users. Also rename the Kconfig option to describe the feature better. > > Signed-off-by: Christoph Hellwig > --- > arch/arc/Kconfig| 1 - > arch/arm/Kconfig| 2 +- > arch/arm64/Kconfig | 1 - > arch/csky/Kconfig | 1 - > arch/mips/Kconfig | 1 - > arch/riscv/Kconfig | 1 - > arch/sh/Kconfig | 2 +- > arch/unicore32/Kconfig | 1 - > arch/x86/Kconfig| 1 - > drivers/mfd/Kconfig | 2 ++ If everyone else is happy with these changes, then so am I. Acked-by: Lee Jones > drivers/of/Kconfig | 3 ++- > include/linux/device.h | 2 +- > include/linux/dma-mapping.h | 8 > kernel/dma/Kconfig | 2 +- > kernel/dma/Makefile | 2 +- > 15 files changed, 13 insertions(+), 17 deletions(-) -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 01/12] mfd/sm501: depend on HAS_DMA
On Mon, 11 Feb 2019, Christoph Hellwig wrote: > Currently the sm501 mfd driver can be compiled without any dependencies, > but through the use of dma_declare_coherent it really depends on > having DMA and iomem support. Normally we don't explicitly require DMA > support as we have stubs for it if on UML, but in this case the driver > selects support for dma_declare_coherent and thus also requires > memmap support. Guard this by an explicit dependency. > > Signed-off-by: Christoph Hellwig > --- > drivers/mfd/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index f461460a2aeb..f15f6489803d 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1066,6 +1066,7 @@ config MFD_SI476X_CORE > > config MFD_SM501 > tristate "Silicon Motion SM501" > + depends on HAS_DMA >---help--- > This is the core driver for the Silicon Motion SM501 multimedia > companion chip. This device is a multifunction device which may I would normally have taken this, but I fear it will conflict with [PATCH 06/12]. For that reason, just take my: Acked-by: Lee Jones -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
