* [PATCH v2 0/2] common/memsize.c: Fix get_ram_size() when cache is enabled
@ 2023-05-30 13:33 Francesco Dolcini
2023-05-30 13:33 ` [PATCH v2 1/2] sandbox: Add a dummy dcache_status() function Francesco Dolcini
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Francesco Dolcini @ 2023-05-30 13:33 UTC (permalink / raw)
To: Pali Rohár, u-boot, Tom Rini, Simon Glass; +Cc: Francesco Dolcini
From: Francesco Dolcini <francesco.dolcini@toradex.com>
Ensure that every write is flushed to memory and afterward reads are
from memory.
Since the algorithm rely on the fact that accessing to not existent
memory lead to write at addr / 2 without this modification accesses
to aliased (not physically present) addresses are cached and
wrong size is returned.
This was discovered while working on a TI AM625 based board
where cache is normally enabled, see commit c02712a74849 ("arm: mach-k3: Enable dcache in SPL").
Test results here: https://github.com/u-boot/u-boot/pull/293
v2:
* added additional patch to fix sandbox build
* ensure that the changes are fine with every arch/build,
check if cache is enabled and check for cache line size define
Emanuele Ghidoli (2):
sandbox: Add a dummy dcache_status() function
common/memsize.c: Fix get_ram_size() when cache is enabled
arch/sandbox/cpu/cpu.c | 5 +++++
common/memsize.c | 24 ++++++++++++++++++++++++
2 files changed, 29 insertions(+)
--
2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 1/2] sandbox: Add a dummy dcache_status() function 2023-05-30 13:33 [PATCH v2 0/2] common/memsize.c: Fix get_ram_size() when cache is enabled Francesco Dolcini @ 2023-05-30 13:33 ` Francesco Dolcini 2023-06-01 3:28 ` Simon Glass 2023-05-30 13:33 ` [PATCH v2 2/2] common/memsize.c: Fix get_ram_size() when cache is enabled Francesco Dolcini 2023-06-22 14:00 ` [PATCH v2 0/2] " Tom Rini 2 siblings, 1 reply; 8+ messages in thread From: Francesco Dolcini @ 2023-05-30 13:33 UTC (permalink / raw) To: u-boot, Tom Rini, Simon Glass; +Cc: Emanuele Ghidoli, Francesco Dolcini From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> This adds dcache_status() so that code using it can build without error on sandbox. This is required in preparation of adding cache handling into get_ram_size function. Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> --- arch/sandbox/cpu/cpu.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c index 51496338ad60..a1c5c7c4311a 100644 --- a/arch/sandbox/cpu/cpu.c +++ b/arch/sandbox/cpu/cpu.c @@ -286,6 +286,11 @@ void sandbox_set_enable_pci_map(int enable) enable_pci_map = enable; } +int dcache_status(void) +{ + return 1; +} + void flush_dcache_range(unsigned long start, unsigned long stop) { } -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] sandbox: Add a dummy dcache_status() function 2023-05-30 13:33 ` [PATCH v2 1/2] sandbox: Add a dummy dcache_status() function Francesco Dolcini @ 2023-06-01 3:28 ` Simon Glass 0 siblings, 0 replies; 8+ messages in thread From: Simon Glass @ 2023-06-01 3:28 UTC (permalink / raw) To: Francesco Dolcini; +Cc: u-boot, Tom Rini, Emanuele Ghidoli, Francesco Dolcini On Tue, 30 May 2023 at 07:33, Francesco Dolcini <francesco@dolcini.it> wrote: > > From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> > > This adds dcache_status() so that code using it can build > without error on sandbox. This is required in preparation > of adding cache handling into get_ram_size function. > > Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > --- > arch/sandbox/cpu/cpu.c | 5 +++++ > 1 file changed, 5 insertions(+) > Reviewed-by: Simon Glass <sjg@chromium.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] common/memsize.c: Fix get_ram_size() when cache is enabled 2023-05-30 13:33 [PATCH v2 0/2] common/memsize.c: Fix get_ram_size() when cache is enabled Francesco Dolcini 2023-05-30 13:33 ` [PATCH v2 1/2] sandbox: Add a dummy dcache_status() function Francesco Dolcini @ 2023-05-30 13:33 ` Francesco Dolcini 2023-05-30 13:42 ` Michael Nazzareno Trimarchi 2023-06-22 14:00 ` [PATCH v2 0/2] " Tom Rini 2 siblings, 1 reply; 8+ messages in thread From: Francesco Dolcini @ 2023-05-30 13:33 UTC (permalink / raw) To: Pali Rohár, u-boot, Tom Rini, Simon Glass Cc: Emanuele Ghidoli, Francesco Dolcini From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> Ensure that every write is flushed to memory and afterward reads are from memory. Since the algorithm rely on the fact that accessing to not existent memory lead to write at addr / 2 without this modification accesses to aliased (not physically present) addresses are cached and wrong size is returned. This was discovered while working on a TI AM625 based board where cache is normally enabled, see commit c02712a74849 ("arm: mach-k3: Enable dcache in SPL"). Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> --- v2: * check if CONFIG_SYS_CACHELINE_SIZE is defined * do flush only when cache is enabled --- common/memsize.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/common/memsize.c b/common/memsize.c index 66d5be6a1ff3..d646df8b04cb 100644 --- a/common/memsize.c +++ b/common/memsize.c @@ -7,9 +7,18 @@ #include <common.h> #include <init.h> #include <asm/global_data.h> +#include <cpu_func.h> +#include <stdint.h> DECLARE_GLOBAL_DATA_PTR; +#ifdef CONFIG_SYS_CACHELINE_SIZE +# define MEMSIZE_CACHELINE_SIZE CONFIG_SYS_CACHELINE_SIZE +#else +/* Just use the greatest cache flush alignment requirement I'm aware of */ +# define MEMSIZE_CACHELINE_SIZE 128 +#endif + #ifdef __PPC__ /* * At least on G2 PowerPC cores, sequential accesses to non-existent @@ -20,6 +29,15 @@ DECLARE_GLOBAL_DATA_PTR; # define sync() /* nothing */ #endif +static void dcache_flush_invalidate(volatile long *p) +{ + uintptr_t start, stop; + start = ALIGN_DOWN((uintptr_t)p, MEMSIZE_CACHELINE_SIZE); + stop = start + MEMSIZE_CACHELINE_SIZE; + flush_dcache_range(start, stop); + invalidate_dcache_range(start, stop); +} + /* * Check memory range for valid RAM. A simple memory test determines * the actually available RAM size between addresses `base' and @@ -34,6 +52,7 @@ long get_ram_size(long *base, long maxsize) long val; long size; int i = 0; + int dcache_en = dcache_status(); for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) { addr = base + cnt; /* pointer arith! */ @@ -41,6 +60,8 @@ long get_ram_size(long *base, long maxsize) save[i++] = *addr; sync(); *addr = ~cnt; + if (dcache_en) + dcache_flush_invalidate(addr); } addr = base; @@ -50,6 +71,9 @@ long get_ram_size(long *base, long maxsize) *addr = 0; sync(); + if (dcache_en) + dcache_flush_invalidate(addr); + if ((val = *addr) != 0) { /* Restore the original data before leaving the function. */ sync(); -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] common/memsize.c: Fix get_ram_size() when cache is enabled 2023-05-30 13:33 ` [PATCH v2 2/2] common/memsize.c: Fix get_ram_size() when cache is enabled Francesco Dolcini @ 2023-05-30 13:42 ` Michael Nazzareno Trimarchi 2023-05-30 13:48 ` Francesco Dolcini 0 siblings, 1 reply; 8+ messages in thread From: Michael Nazzareno Trimarchi @ 2023-05-30 13:42 UTC (permalink / raw) To: Francesco Dolcini Cc: Pali Rohár, u-boot, Tom Rini, Simon Glass, Emanuele Ghidoli, Francesco Dolcini Hi Few questions On Tue, May 30, 2023 at 3:34 PM Francesco Dolcini <francesco@dolcini.it> wrote: > > From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> > > Ensure that every write is flushed to memory and afterward reads are > from memory. > Since the algorithm rely on the fact that accessing to not existent > memory lead to write at addr / 2 without this modification accesses > to aliased (not physically present) addresses are cached and > wrong size is returned. > > This was discovered while working on a TI AM625 based board > where cache is normally enabled, see commit c02712a74849 ("arm: mach-k3: Enable dcache in SPL"). > > Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > --- > v2: > * check if CONFIG_SYS_CACHELINE_SIZE is defined > * do flush only when cache is enabled > --- > common/memsize.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/common/memsize.c b/common/memsize.c > index 66d5be6a1ff3..d646df8b04cb 100644 > --- a/common/memsize.c > +++ b/common/memsize.c > @@ -7,9 +7,18 @@ > #include <common.h> > #include <init.h> > #include <asm/global_data.h> > +#include <cpu_func.h> > +#include <stdint.h> > > DECLARE_GLOBAL_DATA_PTR; > > +#ifdef CONFIG_SYS_CACHELINE_SIZE > +# define MEMSIZE_CACHELINE_SIZE CONFIG_SYS_CACHELINE_SIZE > +#else > +/* Just use the greatest cache flush alignment requirement I'm aware of */ > +# define MEMSIZE_CACHELINE_SIZE 128 > +#endif > + > #ifdef __PPC__ > /* > * At least on G2 PowerPC cores, sequential accesses to non-existent > @@ -20,6 +29,15 @@ DECLARE_GLOBAL_DATA_PTR; > # define sync() /* nothing */ > #endif > > +static void dcache_flush_invalidate(volatile long *p) > +{ > + uintptr_t start, stop; > + start = ALIGN_DOWN((uintptr_t)p, MEMSIZE_CACHELINE_SIZE); > + stop = start + MEMSIZE_CACHELINE_SIZE; > + flush_dcache_range(start, stop); > + invalidate_dcache_range(start, stop); > +} > + > /* > * Check memory range for valid RAM. A simple memory test determines > * the actually available RAM size between addresses `base' and > @@ -34,6 +52,7 @@ long get_ram_size(long *base, long maxsize) > long val; > long size; > int i = 0; > + int dcache_en = dcache_status(); > > for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) { > addr = base + cnt; /* pointer arith! */ > @@ -41,6 +60,8 @@ long get_ram_size(long *base, long maxsize) > save[i++] = *addr; > sync(); > *addr = ~cnt; > + if (dcache_en) > + dcache_flush_invalidate(addr); Why this should be done on every increment if the invalidate keep a range of address? > } > > addr = base; > @@ -50,6 +71,9 @@ long get_ram_size(long *base, long maxsize) > *addr = 0; > > sync(); > + if (dcache_en) > + dcache_flush_invalidate(addr); > + > if ((val = *addr) != 0) { > /* Restore the original data before leaving the function. */ > sync(); Can be possible just to enable/disable cache around memory test? Michael > -- > 2.25.1 > -- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com __________________________________ Amarula Solutions BV Joop Geesinkweg 125, 1114 AB, Amsterdam, NL T. +31 (0)85 111 9172 info@amarulasolutions.com www.amarulasolutions.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] common/memsize.c: Fix get_ram_size() when cache is enabled 2023-05-30 13:42 ` Michael Nazzareno Trimarchi @ 2023-05-30 13:48 ` Francesco Dolcini 2023-05-30 14:04 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 8+ messages in thread From: Francesco Dolcini @ 2023-05-30 13:48 UTC (permalink / raw) To: Michael Nazzareno Trimarchi Cc: Francesco Dolcini, Pali Rohár, u-boot, Tom Rini, Simon Glass, Emanuele Ghidoli, Francesco Dolcini On Tue, May 30, 2023 at 03:42:18PM +0200, Michael Nazzareno Trimarchi wrote: > On Tue, May 30, 2023 at 3:34 PM Francesco Dolcini <francesco@dolcini.it> wrote: > > > > From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> > > > > Ensure that every write is flushed to memory and afterward reads are > > from memory. > > Since the algorithm rely on the fact that accessing to not existent > > memory lead to write at addr / 2 without this modification accesses > > to aliased (not physically present) addresses are cached and > > wrong size is returned. > > > > This was discovered while working on a TI AM625 based board > > where cache is normally enabled, see commit c02712a74849 ("arm: mach-k3: Enable dcache in SPL"). > > > > Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > --- > > v2: > > * check if CONFIG_SYS_CACHELINE_SIZE is defined > > * do flush only when cache is enabled > > --- > > common/memsize.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/common/memsize.c b/common/memsize.c > > index 66d5be6a1ff3..d646df8b04cb 100644 > > --- a/common/memsize.c > > +++ b/common/memsize.c > > @@ -7,9 +7,18 @@ > > #include <common.h> > > #include <init.h> > > #include <asm/global_data.h> > > +#include <cpu_func.h> > > +#include <stdint.h> > > > > DECLARE_GLOBAL_DATA_PTR; > > > > +#ifdef CONFIG_SYS_CACHELINE_SIZE > > +# define MEMSIZE_CACHELINE_SIZE CONFIG_SYS_CACHELINE_SIZE > > +#else > > +/* Just use the greatest cache flush alignment requirement I'm aware of */ > > +# define MEMSIZE_CACHELINE_SIZE 128 > > +#endif > > + > > #ifdef __PPC__ > > /* > > * At least on G2 PowerPC cores, sequential accesses to non-existent > > @@ -20,6 +29,15 @@ DECLARE_GLOBAL_DATA_PTR; > > # define sync() /* nothing */ > > #endif > > > > +static void dcache_flush_invalidate(volatile long *p) > > +{ > > + uintptr_t start, stop; > > + start = ALIGN_DOWN((uintptr_t)p, MEMSIZE_CACHELINE_SIZE); > > + stop = start + MEMSIZE_CACHELINE_SIZE; > > + flush_dcache_range(start, stop); > > + invalidate_dcache_range(start, stop); > > +} > > + > > /* > > * Check memory range for valid RAM. A simple memory test determines > > * the actually available RAM size between addresses `base' and > > @@ -34,6 +52,7 @@ long get_ram_size(long *base, long maxsize) > > long val; > > long size; > > int i = 0; > > + int dcache_en = dcache_status(); > > > > for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) { > > addr = base + cnt; /* pointer arith! */ > > @@ -41,6 +60,8 @@ long get_ram_size(long *base, long maxsize) > > save[i++] = *addr; > > sync(); > > *addr = ~cnt; > > + if (dcache_en) > > + dcache_flush_invalidate(addr); > > Why this should be done on every increment if the invalidate keep a > range of address? We do invalidate/flush the current write, the granularity of the flush/invalidate is the page size. This is required since we need to ensure ordering of the writes. How would you know where the aliasing is going to happen if you flush all at once at the end? > Can be possible just to enable/disable cache around memory test? In theory yes. In practice this proved some architecture to just crash badly because the stack was "corrupted" after re-enabling the cache. We'll submit a separate bug fix for that, bug given that this pattern (disable AND enable) is normally not done in U-Boot it seems like looking for trouble doing it in such a commonly used routine. Francesco ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] common/memsize.c: Fix get_ram_size() when cache is enabled 2023-05-30 13:48 ` Francesco Dolcini @ 2023-05-30 14:04 ` Michael Nazzareno Trimarchi 0 siblings, 0 replies; 8+ messages in thread From: Michael Nazzareno Trimarchi @ 2023-05-30 14:04 UTC (permalink / raw) To: Francesco Dolcini Cc: Pali Rohár, u-boot, Tom Rini, Simon Glass, Emanuele Ghidoli, Francesco Dolcini Hi On Tue, May 30, 2023 at 3:49 PM Francesco Dolcini <francesco@dolcini.it> wrote: > > On Tue, May 30, 2023 at 03:42:18PM +0200, Michael Nazzareno Trimarchi wrote: > > On Tue, May 30, 2023 at 3:34 PM Francesco Dolcini <francesco@dolcini.it> wrote: > > > > > > From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> > > > > > > Ensure that every write is flushed to memory and afterward reads are > > > from memory. > > > Since the algorithm rely on the fact that accessing to not existent > > > memory lead to write at addr / 2 without this modification accesses > > > to aliased (not physically present) addresses are cached and > > > wrong size is returned. > > > > > > This was discovered while working on a TI AM625 based board > > > where cache is normally enabled, see commit c02712a74849 ("arm: mach-k3: Enable dcache in SPL"). > > > > > > Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > --- > > > v2: > > > * check if CONFIG_SYS_CACHELINE_SIZE is defined > > > * do flush only when cache is enabled > > > --- > > > common/memsize.c | 24 ++++++++++++++++++++++++ > > > 1 file changed, 24 insertions(+) > > > > > > diff --git a/common/memsize.c b/common/memsize.c > > > index 66d5be6a1ff3..d646df8b04cb 100644 > > > --- a/common/memsize.c > > > +++ b/common/memsize.c > > > @@ -7,9 +7,18 @@ > > > #include <common.h> > > > #include <init.h> > > > #include <asm/global_data.h> > > > +#include <cpu_func.h> > > > +#include <stdint.h> > > > > > > DECLARE_GLOBAL_DATA_PTR; > > > > > > +#ifdef CONFIG_SYS_CACHELINE_SIZE > > > +# define MEMSIZE_CACHELINE_SIZE CONFIG_SYS_CACHELINE_SIZE > > > +#else > > > +/* Just use the greatest cache flush alignment requirement I'm aware of */ > > > +# define MEMSIZE_CACHELINE_SIZE 128 > > > +#endif > > > + > > > #ifdef __PPC__ > > > /* > > > * At least on G2 PowerPC cores, sequential accesses to non-existent > > > @@ -20,6 +29,15 @@ DECLARE_GLOBAL_DATA_PTR; > > > # define sync() /* nothing */ > > > #endif > > > > > > +static void dcache_flush_invalidate(volatile long *p) > > > +{ > > > + uintptr_t start, stop; > > > + start = ALIGN_DOWN((uintptr_t)p, MEMSIZE_CACHELINE_SIZE); > > > + stop = start + MEMSIZE_CACHELINE_SIZE; > > > + flush_dcache_range(start, stop); > > > + invalidate_dcache_range(start, stop); > > > +} > > > + > > > /* > > > * Check memory range for valid RAM. A simple memory test determines > > > * the actually available RAM size between addresses `base' and > > > @@ -34,6 +52,7 @@ long get_ram_size(long *base, long maxsize) > > > long val; > > > long size; > > > int i = 0; > > > + int dcache_en = dcache_status(); > > > > > > for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) { > > > addr = base + cnt; /* pointer arith! */ > > > @@ -41,6 +60,8 @@ long get_ram_size(long *base, long maxsize) > > > save[i++] = *addr; > > > sync(); > > > *addr = ~cnt; > > > + if (dcache_en) > > > + dcache_flush_invalidate(addr); > > > > Why this should be done on every increment if the invalidate keep a > > range of address? > > We do invalidate/flush the current write, the granularity of the > flush/invalidate is the page size. > > This is required since we need to ensure ordering of the writes. How > would you know where the aliasing is going to happen if you flush all at > once at the end? > I see, I read the code properly of get_mem_size now. > > Can be possible just to enable/disable cache around memory test? > In theory yes. In practice this proved some architecture to just crash > badly because the stack was "corrupted" after re-enabling the cache. > > We'll submit a separate bug fix for that, bug given that this pattern > (disable AND enable) is normally not done in U-Boot it seems like > looking for trouble doing it in such a commonly used routine. > Thank you ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/2] common/memsize.c: Fix get_ram_size() when cache is enabled 2023-05-30 13:33 [PATCH v2 0/2] common/memsize.c: Fix get_ram_size() when cache is enabled Francesco Dolcini 2023-05-30 13:33 ` [PATCH v2 1/2] sandbox: Add a dummy dcache_status() function Francesco Dolcini 2023-05-30 13:33 ` [PATCH v2 2/2] common/memsize.c: Fix get_ram_size() when cache is enabled Francesco Dolcini @ 2023-06-22 14:00 ` Tom Rini 2 siblings, 0 replies; 8+ messages in thread From: Tom Rini @ 2023-06-22 14:00 UTC (permalink / raw) To: u-boot, Pali Rohár, Simon Glass, Francesco Dolcini; +Cc: Francesco Dolcini On Tue, 30 May 2023 15:33:25 +0200, Francesco Dolcini wrote: > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > Ensure that every write is flushed to memory and afterward reads are > from memory. > Since the algorithm rely on the fact that accessing to not existent > memory lead to write at addr / 2 without this modification accesses > to aliased (not physically present) addresses are cached and > wrong size is returned. > > [...] Applied to u-boot/next, thanks! -- Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-06-22 14:01 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-30 13:33 [PATCH v2 0/2] common/memsize.c: Fix get_ram_size() when cache is enabled Francesco Dolcini 2023-05-30 13:33 ` [PATCH v2 1/2] sandbox: Add a dummy dcache_status() function Francesco Dolcini 2023-06-01 3:28 ` Simon Glass 2023-05-30 13:33 ` [PATCH v2 2/2] common/memsize.c: Fix get_ram_size() when cache is enabled Francesco Dolcini 2023-05-30 13:42 ` Michael Nazzareno Trimarchi 2023-05-30 13:48 ` Francesco Dolcini 2023-05-30 14:04 ` Michael Nazzareno Trimarchi 2023-06-22 14:00 ` [PATCH v2 0/2] " Tom Rini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.