Re: [PATCH v1] common/memsize.c: Fix get_ram_size() when cache is enabled

[Francesco Dolcini]

On Fri, May 26, 2023 at 12:15:10PM -0400, Tom Rini wrote:
> On Thu, May 25, 2023 at 03:01:13PM +0200, Francesco Dolcini wrote:
> 
> > From: Emanuele Ghidoli 
> > 
> > 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 
> > Signed-off-by: Francesco Dolcini 
> > ---
> >  common/memsize.c | 14 ++
> >  1 file changed, 14 insertions(+)
> 
> Ah, interesting.  Have you put this through a full CI loop via Azure
> for example, since this is common code?  That's my real concern here,
> thanks.

v2 is coming, your were right on having concerns.

Francesco

Re: [PATCH v1] common/memsize.c: Fix get_ram_size() when cache is enabled

[Tom Rini]

On Thu, May 25, 2023 at 03:01:13PM +0200, Francesco Dolcini wrote:

> From: Emanuele Ghidoli 
> 
> 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 
> Signed-off-by: Francesco Dolcini 
> ---
>  common/memsize.c | 14 ++
>  1 file changed, 14 insertions(+)

Ah, interesting.  Have you put this through a full CI loop via Azure
for example, since this is common code?  That's my real concern here,
thanks.

-- 
Tom


signature.asc
Description: PGP signature

[PATCH v1] common/memsize.c: Fix get_ram_size() when cache is enabled

[Francesco Dolcini]

From: Emanuele Ghidoli 

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 
Signed-off-by: Francesco Dolcini 
---
 common/memsize.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/common/memsize.c b/common/memsize.c
index 66d5be6a1ff3..f3cd28a06cc6 100644
--- a/common/memsize.c
+++ b/common/memsize.c
@@ -7,6 +7,7 @@
 #include 
 #include 
 #include 
+#include 
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -20,6 +21,16 @@ DECLARE_GLOBAL_DATA_PTR;
 # define sync()/* nothing */
 #endif
 
+static void dcache_flush_invalidate(volatile long *p)
+{
+   phys_addr_t start, stop;
+
+   start = ALIGN_DOWN((phys_addr_t)p, CONFIG_SYS_CACHELINE_SIZE);
+   stop = start + CONFIG_SYS_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
@@ -41,6 +52,7 @@ long get_ram_size(long *base, long maxsize)
save[i++] = *addr;
sync();
*addr = ~cnt;
+   dcache_flush_invalidate(addr);
}
 
addr = base;
@@ -50,6 +62,8 @@ long get_ram_size(long *base, long maxsize)
*addr = 0;
 
sync();
+   dcache_flush_invalidate(addr);
+
if ((val = *addr) != 0) {
/* Restore the original data before leaving the function. */
sync();
-- 
2.25.1