[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

The current code in reserve_noncached() has two issues:

1) The first update of gd->start_addr_sp always rounds down to a section
start. However, the equivalent calculation in cache.c:noncached_init()
always first rounds up to a section start, then subtracts a section size.
These two calculations differ if the initial value is already rounded to
section alignment.

2) The second update of gd->start_addr_sp subtracts exactly
CONFIG_SYS_NONCACHED_MEMORY, whereas the equivalent calculation in
cache.c:noncached_init() rounds the noncached size up to section
alignment before subtracting it. The two calculations differ if the
noncached region size is not a multiple of the MMU section size.

In practice, one/both of those issues causes a practical problem on
Jetson TX1; U-Boot triggers a synchronous abort during initialization,
likely due to overlapping use of some memory region.

This change fixes both these issues by duplicating the exact calculations
from noncached_init() into reserve_noncached().

However, this fix assumes that gd->start_addr_sp on entry to
reserve_noncached() exactly matches mem_malloc_start on entry to
noncached_init(). I haven't traced the code to see whether it absolutely
guarantees this in all (or indeed any!) cases. Consequently, I added some
comments in the hope that this condition will continue to be true.

Fixes: 5f7adb5b1c02 ("board_f: reserve noncached space below malloc area")
Cc: Vikas Manocha <vikas.manocha@st.com>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/lib/cache.c |  1 +
 common/board_f.c     | 15 ++++++++++++---
 common/board_r.c     |  4 ++++
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
index 449544d11cff..463d283cb768 100644
--- a/arch/arm/lib/cache.c
+++ b/arch/arm/lib/cache.c
@@ -77,6 +77,7 @@ void noncached_init(void)
 	phys_addr_t start, end;
 	size_t size;
 
+	/* If this calculation changes, update board_f.c:reserve_noncached() */
 	end = ALIGN(mem_malloc_start, MMU_SECTION_SIZE) - MMU_SECTION_SIZE;
 	size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY, MMU_SECTION_SIZE);
 	start = end - size;
diff --git a/common/board_f.c b/common/board_f.c
index 6867abc8e679..591f18f391e2 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -470,9 +470,18 @@ static int reserve_uboot(void)
 #ifdef CONFIG_SYS_NONCACHED_MEMORY
 static int reserve_noncached(void)
 {
-	/* round down to SECTION SIZE (typicaly 1MB) limit */
-	gd->start_addr_sp &= ~(MMU_SECTION_SIZE - 1);
-	gd->start_addr_sp -= CONFIG_SYS_NONCACHED_MEMORY;
+	/*
+	 * The value of gd->start_addr_sp must match the value of malloc_start
+	 * calculated in boatrd_f.c:initr_malloc(), which is passed to
+	 * board_r.c:mem_malloc_init() and then used by
+	 * cache.c:noncached_init()
+	 *
+	 * These calculations must match the code in cache.c:noncached_init()
+	 */
+	gd->start_addr_sp = ALIGN(gd->start_addr_sp, MMU_SECTION_SIZE) -
+		MMU_SECTION_SIZE;
+	gd->start_addr_sp -= ALIGN(CONFIG_SYS_NONCACHED_MEMORY,
+				   MMU_SECTION_SIZE);
 	debug("Reserving %dM for noncached_alloc() at: %08lx\n",
 	      CONFIG_SYS_NONCACHED_MEMORY >> 20, gd->start_addr_sp);
 
diff --git a/common/board_r.c b/common/board_r.c
index b7f68bba4a7e..d6fb5047a265 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -247,6 +247,10 @@ static int initr_malloc(void)
 	      gd->malloc_ptr / 1024);
 #endif
 	/* The malloc area is immediately below the monitor copy in DRAM */
+	/*
+	 * This value MUST match the value of gd->start_addr_sp in board_f.c:
+	 * reserve_noncached().
+	 */
 	malloc_start = gd->relocaddr - TOTAL_MALLOC_LEN;
 	mem_malloc_init((ulong)map_sysmem(malloc_start, TOTAL_MALLOC_LEN),
 			TOTAL_MALLOC_LEN);
-- 
2.22.1

[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[Vikas MANOCHA]

Hi Stephen,

> -----Original Message-----
> From: Stephen Warren <swarren@wwwdotorg.org>
> Sent: Tuesday, August 27, 2019 10:55 AM
> To: Tom Rini <trini@konsulko.com>
> Cc: twarren at wwwdotorg.org; u-boot at lists.denx.de; Stephen Warren
> <swarren@nvidia.com>; Vikas MANOCHA <vikas.manocha@st.com>
> Subject: [PATCH] board_f: fix noncached reservation calculation
> 
> From: Stephen Warren <swarren@nvidia.com>
> 
> The current code in reserve_noncached() has two issues:
> 
> 1) The first update of gd->start_addr_sp always rounds down to a section
> start. However, the equivalent calculation in cache.c:noncached_init()
> always first rounds up to a section start, then subtracts a section size.
> These two calculations differ if the initial value is already rounded to section
> alignment.

It shouldn't cause any issue, first one round down to section size. Second one(cache.c: noncached_init()) rounds up, so needs section size subtraction.

> 
> 2) The second update of gd->start_addr_sp subtracts exactly
> CONFIG_SYS_NONCACHED_MEMORY, whereas the equivalent calculation in
> cache.c:noncached_init() rounds the noncached size up to section alignment
> before subtracting it. The two calculations differ if the noncached region size
> is not a multiple of the MMU section size.

Never thought CONFIG_SYS_NON_CACACHED_MEMORY could be non-multiple of MMU section size for basic MMU setup in u-boot. It has granularity of section size.
Is it the case with Jetson TX1 ?

> 
> In practice, one/both of those issues causes a practical problem on Jetson
> TX1; U-Boot triggers a synchronous abort during initialization, likely due to
> overlapping use of some memory region.
> 
> This change fixes both these issues by duplicating the exact calculations from
> noncached_init() into reserve_noncached().
> 
> However, this fix assumes that gd->start_addr_sp on entry to
> reserve_noncached() exactly matches mem_malloc_start on entry to
> noncached_init(). I haven't traced the code to see whether it absolutely
> guarantees this in all (or indeed any!) cases. Consequently, I added some
> comments in the hope that this condition will continue to be true.

It is enforced it in the code, reserve_noncached is called from reserve_malloc() after malloc area reservation.

> 
> Fixes: 5f7adb5b1c02 ("board_f: reserve noncached space below malloc
> area")
> Cc: Vikas Manocha <vikas.manocha@st.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  arch/arm/lib/cache.c |  1 +
>  common/board_f.c     | 15 ++++++++++++---
>  common/board_r.c     |  4 ++++
>  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c index
> 449544d11cff..463d283cb768 100644
> --- a/arch/arm/lib/cache.c
> +++ b/arch/arm/lib/cache.c
> @@ -77,6 +77,7 @@ void noncached_init(void)
>  	phys_addr_t start, end;
>  	size_t size;
> 
> +	/* If this calculation changes, update board_f.c:reserve_noncached()
> +*/
>  	end = ALIGN(mem_malloc_start, MMU_SECTION_SIZE) -
> MMU_SECTION_SIZE;
>  	size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY,
> MMU_SECTION_SIZE);
>  	start = end - size;
> diff --git a/common/board_f.c b/common/board_f.c index
> 6867abc8e679..591f18f391e2 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -470,9 +470,18 @@ static int reserve_uboot(void)  #ifdef
> CONFIG_SYS_NONCACHED_MEMORY  static int reserve_noncached(void)  {
> -	/* round down to SECTION SIZE (typicaly 1MB) limit */
> -	gd->start_addr_sp &= ~(MMU_SECTION_SIZE - 1);
> -	gd->start_addr_sp -= CONFIG_SYS_NONCACHED_MEMORY;
> +	/*
> +	 * The value of gd->start_addr_sp must match the value of
> malloc_start
> +	 * calculated in boatrd_f.c:initr_malloc(), which is passed to
> +	 * board_r.c:mem_malloc_init() and then used by
> +	 * cache.c:noncached_init()
> +	 *
> +	 * These calculations must match the code in
> cache.c:noncached_init()
> +	 */
> +	gd->start_addr_sp = ALIGN(gd->start_addr_sp, MMU_SECTION_SIZE)
> -
> +		MMU_SECTION_SIZE;
> +	gd->start_addr_sp -= ALIGN(CONFIG_SYS_NONCACHED_MEMORY,
> +				   MMU_SECTION_SIZE);
>  	debug("Reserving %dM for noncached_alloc() at: %08lx\n",
>  	      CONFIG_SYS_NONCACHED_MEMORY >> 20, gd->start_addr_sp);
> 
> diff --git a/common/board_r.c b/common/board_r.c index
> b7f68bba4a7e..d6fb5047a265 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -247,6 +247,10 @@ static int initr_malloc(void)
>  	      gd->malloc_ptr / 1024);
>  #endif
>  	/* The malloc area is immediately below the monitor copy in DRAM
> */
> +	/*
> +	 * This value MUST match the value of gd->start_addr_sp in
> board_f.c:
> +	 * reserve_noncached().
> +	 */

minor cosmetic suggestion:
gd->start_addr_sp is moving pointer, difficult to comprehend sometimes here, same is true for malloc
area also, how about merging two comments like:

	/* The malloc area is immediately below the monitor copy in DRAM followed by noncached
	*/

Cheers,
Vikas
>  	malloc_start = gd->relocaddr - TOTAL_MALLOC_LEN;
>  	mem_malloc_init((ulong)map_sysmem(malloc_start,
> TOTAL_MALLOC_LEN),
>  			TOTAL_MALLOC_LEN);
> --
> 2.22.1

[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[Stephen Warren]

On 8/27/19 4:10 PM, Vikas MANOCHA wrote:
> Hi Stephen,
> 
>> -----Original Message-----
>> From: Stephen Warren <swarren@wwwdotorg.org>
>> Sent: Tuesday, August 27, 2019 10:55 AM
>> To: Tom Rini <trini@konsulko.com>
>> Cc: twarren at wwwdotorg.org; u-boot at lists.denx.de; Stephen Warren
>> <swarren@nvidia.com>; Vikas MANOCHA <vikas.manocha@st.com>
>> Subject: [PATCH] board_f: fix noncached reservation calculation
>>
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> The current code in reserve_noncached() has two issues:
>>
>> 1) The first update of gd->start_addr_sp always rounds down to a section
>> start. However, the equivalent calculation in cache.c:noncached_init()
>> always first rounds up to a section start, then subtracts a section size.
>> These two calculations differ if the initial value is already rounded to section
>> alignment.
> 
> It shouldn't cause any issue, first one round down to section size. Second
> one(cache.c: noncached_init()) rounds up, so needs section size subtraction.

Here's an example where it fails, based on code before my patch:

Assume that MMU section size is 2, and that mem_malloc_start and 
gd->start_addr_sp are both 1000M on entry to the functions, and the 
noncached region is 1 (what Jetson TX1 uses). The example uses values 
assumed to be multiples of 1M to make the numbers easier to read.

noncached_init:

// mem_malloc_start = 1000
end = ALIGN(mem_malloc_start, MMU_SECTION_SIZE) - MMU_SECTION_SIZE;
// end = 1000 - 2 = 998 // was already aligned, so 1000 not 1002
size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY, MMU_SECTION_SIZE);
// size = 2
start = end - size;
// start = 998 - 2 = 996
// region is 996...998

reserve_noncached:

// gd->start_addr_sp = 1000
gd->start_addr_sp &= ~(MMU_SECTION_SIZE - 1);
// gd->start_addr_sp = 1000
gd->start_addr_sp -= CONFIG_SYS_NONCACHED_MEMORY;
// gd->start_addr_sp = 1000 - 1 = 999
// region is 999...1000

So, the end of the region that's been reserved is 1000, yet the code 
that sets up the noncached region believes the end of the region is at 
998. Even ignoring the difference in size calculation due to issue (2) 
below, that still means the reservation is in the wrong place, and the 
stack can end up overlaid with the noncached reservation, or even other 
data below it.

>> 2) The second update of gd->start_addr_sp subtracts exactly
>> CONFIG_SYS_NONCACHED_MEMORY, whereas the equivalent calculation in
>> cache.c:noncached_init() rounds the noncached size up to section alignment
>> before subtracting it. The two calculations differ if the noncached region size
>> is not a multiple of the MMU section size.
> 
> Never thought CONFIG_SYS_NON_CACACHED_MEMORY could be non-multiple of
> MMU section size for basic MMU setup in u-boot. It has granularity of section size.
> Is it the case with Jetson TX1 ?

Yes, on Jetson TX1, the MMU section size is 2M, yet the noncached region 
is 1M. Nothing in the README docs for the nocached region state or imply 
that the noncached region needs to be a multiple of the MMU section 
size, and all code that uses the config symbol before your patch rounds 
the config symbol to MMU section size, implying that its value doens't 
need to be rounded already.

>> In practice, one/both of those issues causes a practical problem on Jetson
>> TX1; U-Boot triggers a synchronous abort during initialization, likely due to
>> overlapping use of some memory region.
>>
>> This change fixes both these issues by duplicating the exact calculations from
>> noncached_init() into reserve_noncached().
>>
>> However, this fix assumes that gd->start_addr_sp on entry to
>> reserve_noncached() exactly matches mem_malloc_start on entry to
>> noncached_init(). I haven't traced the code to see whether it absolutely
>> guarantees this in all (or indeed any!) cases. Consequently, I added some
>> comments in the hope that this condition will continue to be true.
> 
> It is enforced it in the code, reserve_noncached is called from 
> reserve_malloc()after malloc area reservation.

That's a bit implicit still; nothing in reserve_malloc sets or uses the 
value of mem_malloc_start, so the two could easily become decoupled if 
the reservation calculations don't match the code which actually sets up 
the region usage, which incidentally is exactly what happened here, and 
hence why I found this bug.

>> diff --git a/common/board_r.c b/common/board_r.c index
>> b7f68bba4a7e..d6fb5047a265 100644
>> --- a/common/board_r.c
>> +++ b/common/board_r.c
>> @@ -247,6 +247,10 @@ static int initr_malloc(void)
>>   	      gd->malloc_ptr / 1024);
>>   #endif
>>   	/* The malloc area is immediately below the monitor copy in DRAM
>> */
>> +	/*
>> +	 * This value MUST match the value of gd->start_addr_sp in
>> board_f.c:
>> +	 * reserve_noncached().
>> +	 */
> 
> minor cosmetic suggestion:
> gd->start_addr_sp is moving pointer, difficult to comprehend sometimes here, same is true for malloc
> area also, how about merging two comments like:
> 
> 	/* The malloc area is immediately below the monitor copy in DRAM followed by noncached
> 	*/

I'd rather have an explicit separate comment which mentions the other 
function and variable names; if someone searches the code later, it's 
more likely they'll find this comment that way. I guess I could replace 
the intermediate /* and */ lines with just * to merge the comments 
without changing the text in them if you want.

[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[Vikas MANOCHA]

Hi Stephen,

> -----Original Message-----
> From: Stephen Warren <swarren@wwwdotorg.org>
> Sent: Tuesday, August 27, 2019 3:50 PM
> To: Vikas MANOCHA <vikas.manocha@st.com>; Tom Rini
> <trini@konsulko.com>
> Cc: twarren at wwwdotorg.org; u-boot at lists.denx.de; Stephen Warren
> <swarren@nvidia.com>
> Subject: Re: [PATCH] board_f: fix noncached reservation calculation
> 
> On 8/27/19 4:10 PM, Vikas MANOCHA wrote:
> > Hi Stephen,
> >
> >> -----Original Message-----
> >> From: Stephen Warren <swarren@wwwdotorg.org>
> >> Sent: Tuesday, August 27, 2019 10:55 AM
> >> To: Tom Rini <trini@konsulko.com>
> >> Cc: twarren at wwwdotorg.org; u-boot at lists.denx.de; Stephen Warren
> >> <swarren@nvidia.com>; Vikas MANOCHA <vikas.manocha@st.com>
> >> Subject: [PATCH] board_f: fix noncached reservation calculation
> >>
> >> From: Stephen Warren <swarren@nvidia.com>
> >>
> >> The current code in reserve_noncached() has two issues:
> >>
> >> 1) The first update of gd->start_addr_sp always rounds down to a
> >> section start. However, the equivalent calculation in
> >> cache.c:noncached_init() always first rounds up to a section start, then
> subtracts a section size.
> >> These two calculations differ if the initial value is already rounded
> >> to section alignment.
> >
> > It shouldn't cause any issue, first one round down to section size.
> > Second
> > one(cache.c: noncached_init()) rounds up, so needs section size
> subtraction.
> 
> Here's an example where it fails, based on code before my patch:
> 
> Assume that MMU section size is 2, and that mem_malloc_start and
> gd->start_addr_sp are both 1000M on entry to the functions, and the
> noncached region is 1 (what Jetson TX1 uses). The example uses values
> assumed to be multiples of 1M to make the numbers easier to read.
> 
> noncached_init:
> 
> // mem_malloc_start = 1000
> end = ALIGN(mem_malloc_start, MMU_SECTION_SIZE) -
> MMU_SECTION_SIZE; // end = 1000 - 2 = 998 // was already aligned, so 1000
> not 1002 size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY,
> MMU_SECTION_SIZE); // size = 2 start = end - size; // start = 998 - 2 = 996 //
> region is 996...998

Thanks for this example, it definitely seems a bug.  Just that we are fixing it by adding this gap in the reserve_noncached() also.
Better would be to fix this subtraction of MMU_SECTION_SIZE by aligning down "end" location, like:

end = ALIGN_DOWN(mem_malloc_start, MMU_SECTION_SIZE); // end = 1000
size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY, MMU_SECTION_SIZE); // size = 2
start = end -size; // start = 998

> 
> reserve_noncached:
> 
> // gd->start_addr_sp = 1000
> gd->start_addr_sp &= ~(MMU_SECTION_SIZE - 1);
> // gd->start_addr_sp = 1000
> gd->start_addr_sp -= CONFIG_SYS_NONCACHED_MEMORY;

Here CONFIG_SYS_NONCACHED_MEMORY needs to be aligned to MMU SECTION SIZE before 
subtracting from start_addr_sp to fix the second issue you highlighted.

gd->start_addr_sp -= ALIGN(CONFIG_SYS_NONCACHED_MEMORY, MMU_SECTION_SIZE);  // start of non cached = 998.

> // gd->start_addr_sp = 1000 - 1 = 999
> // region is 999...1000

> 
> So, the end of the region that's been reserved is 1000, yet the code that sets
> up the noncached region believes the end of the region is at 998. Even
> ignoring the difference in size calculation due to issue (2) below, that still
> means the reservation is in the wrong place, and the stack can end up
> overlaid with the noncached reservation, or even other data below it.
> 
> >> 2) The second update of gd->start_addr_sp subtracts exactly
> >> CONFIG_SYS_NONCACHED_MEMORY, whereas the equivalent
> calculation in
> >> cache.c:noncached_init() rounds the noncached size up to section
> >> alignment before subtracting it. The two calculations differ if the
> >> noncached region size is not a multiple of the MMU section size.
> >
> > Never thought CONFIG_SYS_NON_CACACHED_MEMORY could be non-
> multiple of
> > MMU section size for basic MMU setup in u-boot. It has granularity of
> section size.
> > Is it the case with Jetson TX1 ?
> 
> Yes, on Jetson TX1, the MMU section size is 2M, yet the noncached region is
> 1M. Nothing in the README docs for the nocached region state or imply that
> the noncached region needs to be a multiple of the MMU section size,

MMU setup granularity is section size, configuring any memory area less than 
section size is not possible in this basic mmu setup. Your patch rounds up this
noncached area to SECTION area which makes it robust.

> and
> all code that uses the config symbol before your patch rounds the config
> symbol to MMU section size, implying that its value doens't need to be
> rounded already.

It was using stack area well below the stack pointer, so was working fine. The patch
just didn’t take care of the case where configured noncached area is not multiple of section size.

> 
> >> In practice, one/both of those issues causes a practical problem on
> >> Jetson TX1; U-Boot triggers a synchronous abort during
> >> initialization, likely due to overlapping use of some memory region.
> >>
> >> This change fixes both these issues by duplicating the exact
> >> calculations from
> >> noncached_init() into reserve_noncached().
> >>
> >> However, this fix assumes that gd->start_addr_sp on entry to
> >> reserve_noncached() exactly matches mem_malloc_start on entry to
> >> noncached_init(). I haven't traced the code to see whether it
> >> absolutely guarantees this in all (or indeed any!) cases.
> >> Consequently, I added some comments in the hope that this condition
> will continue to be true.
> >
> > It is enforced it in the code, reserve_noncached is called from
> > reserve_malloc()after malloc area reservation.
> 
> That's a bit implicit still; nothing in reserve_malloc sets or uses the value of
> mem_malloc_start, so the two could easily become decoupled if the
> reservation calculations don't match the code which actually sets up the
> region usage, which incidentally is exactly what happened here, and hence
> why I found this bug.

Ok

> 
> >> diff --git a/common/board_r.c b/common/board_r.c index
> >> b7f68bba4a7e..d6fb5047a265 100644
> >> --- a/common/board_r.c
> >> +++ b/common/board_r.c
> >> @@ -247,6 +247,10 @@ static int initr_malloc(void)
> >>   	      gd->malloc_ptr / 1024);
> >>   #endif
> >>   	/* The malloc area is immediately below the monitor copy in DRAM
> >> */
> >> +	/*
> >> +	 * This value MUST match the value of gd->start_addr_sp in
> >> board_f.c:
> >> +	 * reserve_noncached().
> >> +	 */
> >
> > minor cosmetic suggestion:
> > gd->start_addr_sp is moving pointer, difficult to comprehend sometimes
> > gd->here, same is true for malloc
> > area also, how about merging two comments like:
> >
> > 	/* The malloc area is immediately below the monitor copy in DRAM
> followed by noncached
> > 	*/
> 
> I'd rather have an explicit separate comment which mentions the other
> function and variable names; if someone searches the code later, it's more
> likely they'll find this comment that way. I guess I could replace the
> intermediate /* and */ lines with just * to merge the comments without
> changing the text in them if you want.

Sure.

Cheers,
Vikas

[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[Stephen Warren]

On 8/27/19 6:01 PM, Vikas MANOCHA wrote:
> Stephen Warren wrote at Tuesday, August 27, 2019 3:50 PM
>> On 8/27/19 4:10 PM, Vikas MANOCHA wrote:
>>> Stephen Warren wrote at Tuesday, August 27, 2019 10:55 AM
>>>> The current code in reserve_noncached() has two issues:
>>>>
>>>> 1) The first update of gd->start_addr_sp always rounds down to a
>>>> section start. However, the equivalent calculation in
>>>> cache.c:noncached_init() always first rounds up to a section start, then
>>>> subtracts a section size.
>>>> These two calculations differ if the initial value is already rounded
>>>> to section alignment.
>>>
>>> It shouldn't cause any issue, first one round down to section size.
>>> Second
>>> one(cache.c: noncached_init()) rounds up, so needs section size
>>> subtraction.
>>
>> Here's an example where it fails, based on code before my patch:
>>
>> Assume that MMU section size is 2, and that mem_malloc_start and
>> gd->start_addr_sp are both 1000M on entry to the functions, and the
>> noncached region is 1 (what Jetson TX1 uses). The example uses values
>> assumed to be multiples of 1M to make the numbers easier to read.
>>
>> noncached_init:
>>
>> // mem_malloc_start = 1000
>> end = ALIGN(mem_malloc_start, MMU_SECTION_SIZE) -
>> MMU_SECTION_SIZE; // end = 1000 - 2 = 998 // was already aligned, so 1000
>> not 1002 size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY,
>> MMU_SECTION_SIZE); // size = 2 start = end - size; // start = 998 - 2 = 996 //
>> region is 996...998
> 
> Thanks for this example, it definitely seems a bug.  Just that we are fixing it by adding this gap in the reserve_noncached() also.
> Better would be to fix this subtraction of MMU_SECTION_SIZE by aligning down "end" location, like:
> 
> end = ALIGN_DOWN(mem_malloc_start, MMU_SECTION_SIZE); // end = 1000
> size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY, MMU_SECTION_SIZE); // size = 2
> start = end -size; // start = 998

That would change yet another piece of code that's been stable for a
while. It's late in the U-Boot release cycle, so I think we should be
conservative, and not change any more code than necessary. Changing lots
of extra code would run the risk of introducing more regressions. I'd
rather (a) apply the original change I posted, which adjusts only the
code that caused the regression, or (b) revert the patch that caused the
regression.

If you want to adjust the code in noncached_init, we can do that
immediately after the release, to give maximum time for any regressions
to be debugged and fixed before the next release.

[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[Vikas MANOCHA]

Hi,

> -----Original Message-----
> From: Stephen Warren <swarren@wwwdotorg.org>
> Sent: Tuesday, August 27, 2019 7:50 PM
> To: Vikas MANOCHA <vikas.manocha@st.com>; Tom Rini
> <trini@konsulko.com>
> Cc: twarren at wwwdotorg.org; u-boot at lists.denx.de; Stephen Warren
> <swarren@nvidia.com>
> Subject: Re: [PATCH] board_f: fix noncached reservation calculation
> 
> On 8/27/19 6:01 PM, Vikas MANOCHA wrote:
> > Stephen Warren wrote at Tuesday, August 27, 2019 3:50 PM
> >> On 8/27/19 4:10 PM, Vikas MANOCHA wrote:
> >>> Stephen Warren wrote at Tuesday, August 27, 2019 10:55 AM
> >>>> The current code in reserve_noncached() has two issues:
> >>>>
> >>>> 1) The first update of gd->start_addr_sp always rounds down to a
> >>>> section start. However, the equivalent calculation in
> >>>> cache.c:noncached_init() always first rounds up to a section start,
> >>>> then subtracts a section size.
> >>>> These two calculations differ if the initial value is already
> >>>> rounded to section alignment.
> >>>
> >>> It shouldn't cause any issue, first one round down to section size.
> >>> Second
> >>> one(cache.c: noncached_init()) rounds up, so needs section size
> >>> subtraction.
> >>
> >> Here's an example where it fails, based on code before my patch:
> >>
> >> Assume that MMU section size is 2, and that mem_malloc_start and
> >> gd->start_addr_sp are both 1000M on entry to the functions, and the
> >> noncached region is 1 (what Jetson TX1 uses). The example uses values
> >> assumed to be multiples of 1M to make the numbers easier to read.
> >>
> >> noncached_init:
> >>
> >> // mem_malloc_start = 1000
> >> end = ALIGN(mem_malloc_start, MMU_SECTION_SIZE) -
> MMU_SECTION_SIZE;
> >> // end = 1000 - 2 = 998 // was already aligned, so 1000 not 1002 size
> >> = ALIGN(CONFIG_SYS_NONCACHED_MEMORY,
> >> MMU_SECTION_SIZE); // size = 2 start = end - size; // start = 998 - 2
> >> = 996 // region is 996...998
> >
> > Thanks for this example, it definitely seems a bug.  Just that we are fixing it
> by adding this gap in the reserve_noncached() also.
> > Better would be to fix this subtraction of MMU_SECTION_SIZE by aligning
> down "end" location, like:
> >
> > end = ALIGN_DOWN(mem_malloc_start, MMU_SECTION_SIZE); // end =
> 1000
> > size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY, MMU_SECTION_SIZE);
> // size =
> > 2 start = end -size; // start = 998
> 
> That would change yet another piece of code that's been stable for a while.
> It's late in the U-Boot release cycle, so I think we should be conservative, and
> not change any more code than necessary. Changing lots of extra code
> would run the risk of introducing more regressions. I'd rather (a) apply the
> original change I posted, which adjusts only the code that caused the
> regression, or (b) revert the patch that caused the regression.

Ok, Either way is fine.

> 
> If you want to adjust the code in noncached_init, we can do that
> immediately after the release, to give maximum time for any regressions to
> be debugged and fixed before the next release.

Ok.

Cheers,
Vikas

[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[Tom Rini]

On Wed, Aug 28, 2019 at 07:22:36PM +0000, Vikas MANOCHA wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Stephen Warren <swarren@wwwdotorg.org>
> > Sent: Tuesday, August 27, 2019 7:50 PM
> > To: Vikas MANOCHA <vikas.manocha@st.com>; Tom Rini
> > <trini@konsulko.com>
> > Cc: twarren at wwwdotorg.org; u-boot at lists.denx.de; Stephen Warren
> > <swarren@nvidia.com>
> > Subject: Re: [PATCH] board_f: fix noncached reservation calculation
> > 
> > On 8/27/19 6:01 PM, Vikas MANOCHA wrote:
> > > Stephen Warren wrote at Tuesday, August 27, 2019 3:50 PM
> > >> On 8/27/19 4:10 PM, Vikas MANOCHA wrote:
> > >>> Stephen Warren wrote at Tuesday, August 27, 2019 10:55 AM
> > >>>> The current code in reserve_noncached() has two issues:
> > >>>>
> > >>>> 1) The first update of gd->start_addr_sp always rounds down to a
> > >>>> section start. However, the equivalent calculation in
> > >>>> cache.c:noncached_init() always first rounds up to a section start,
> > >>>> then subtracts a section size.
> > >>>> These two calculations differ if the initial value is already
> > >>>> rounded to section alignment.
> > >>>
> > >>> It shouldn't cause any issue, first one round down to section size.
> > >>> Second
> > >>> one(cache.c: noncached_init()) rounds up, so needs section size
> > >>> subtraction.
> > >>
> > >> Here's an example where it fails, based on code before my patch:
> > >>
> > >> Assume that MMU section size is 2, and that mem_malloc_start and
> > >> gd->start_addr_sp are both 1000M on entry to the functions, and the
> > >> noncached region is 1 (what Jetson TX1 uses). The example uses values
> > >> assumed to be multiples of 1M to make the numbers easier to read.
> > >>
> > >> noncached_init:
> > >>
> > >> // mem_malloc_start = 1000
> > >> end = ALIGN(mem_malloc_start, MMU_SECTION_SIZE) -
> > MMU_SECTION_SIZE;
> > >> // end = 1000 - 2 = 998 // was already aligned, so 1000 not 1002 size
> > >> = ALIGN(CONFIG_SYS_NONCACHED_MEMORY,
> > >> MMU_SECTION_SIZE); // size = 2 start = end - size; // start = 998 - 2
> > >> = 996 // region is 996...998
> > >
> > > Thanks for this example, it definitely seems a bug.  Just that we are fixing it
> > by adding this gap in the reserve_noncached() also.
> > > Better would be to fix this subtraction of MMU_SECTION_SIZE by aligning
> > down "end" location, like:
> > >
> > > end = ALIGN_DOWN(mem_malloc_start, MMU_SECTION_SIZE); // end =
> > 1000
> > > size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY, MMU_SECTION_SIZE);
> > // size =
> > > 2 start = end -size; // start = 998
> > 
> > That would change yet another piece of code that's been stable for a while.
> > It's late in the U-Boot release cycle, so I think we should be conservative, and
> > not change any more code than necessary. Changing lots of extra code
> > would run the risk of introducing more regressions. I'd rather (a) apply the
> > original change I posted, which adjusts only the code that caused the
> > regression, or (b) revert the patch that caused the regression.
> 
> Ok, Either way is fine.
> 
> > 
> > If you want to adjust the code in noncached_init, we can do that
> > immediately after the release, to give maximum time for any regressions to
> > be debugged and fixed before the next release.
> 
> Ok.

So this patch keeps your use case working and fixes Stephen's problem,
to be clear?  Thanks guys!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190828/b6fab999/attachment.sig>

[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[Vikas MANOCHA]

Hi Tom,

> -----Original Message-----
> From: Tom Rini <trini@konsulko.com>
> Sent: Wednesday, August 28, 2019 12:31 PM
> To: Vikas MANOCHA <vikas.manocha@st.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>; twarren at wwwdotorg.org;
> u-boot at lists.denx.de; Stephen Warren <swarren@nvidia.com>
> Subject: Re: [PATCH] board_f: fix noncached reservation calculation
> 
> On Wed, Aug 28, 2019 at 07:22:36PM +0000, Vikas MANOCHA wrote:
> > Hi,
> >
> > > -----Original Message-----
> > > From: Stephen Warren <swarren@wwwdotorg.org>
> > > Sent: Tuesday, August 27, 2019 7:50 PM
> > > To: Vikas MANOCHA <vikas.manocha@st.com>; Tom Rini
> > > <trini@konsulko.com>
> > > Cc: twarren at wwwdotorg.org; u-boot at lists.denx.de; Stephen Warren
> > > <swarren@nvidia.com>
> > > Subject: Re: [PATCH] board_f: fix noncached reservation calculation
> > >
> > > On 8/27/19 6:01 PM, Vikas MANOCHA wrote:
> > > > Stephen Warren wrote at Tuesday, August 27, 2019 3:50 PM
> > > >> On 8/27/19 4:10 PM, Vikas MANOCHA wrote:
> > > >>> Stephen Warren wrote at Tuesday, August 27, 2019 10:55 AM
> > > >>>> The current code in reserve_noncached() has two issues:
> > > >>>>
> > > >>>> 1) The first update of gd->start_addr_sp always rounds down to
> > > >>>> a section start. However, the equivalent calculation in
> > > >>>> cache.c:noncached_init() always first rounds up to a section
> > > >>>> start, then subtracts a section size.
> > > >>>> These two calculations differ if the initial value is already
> > > >>>> rounded to section alignment.
> > > >>>
> > > >>> It shouldn't cause any issue, first one round down to section size.
> > > >>> Second
> > > >>> one(cache.c: noncached_init()) rounds up, so needs section size
> > > >>> subtraction.
> > > >>
> > > >> Here's an example where it fails, based on code before my patch:
> > > >>
> > > >> Assume that MMU section size is 2, and that mem_malloc_start and
> > > >> gd->start_addr_sp are both 1000M on entry to the functions, and
> > > >> gd->the
> > > >> noncached region is 1 (what Jetson TX1 uses). The example uses
> > > >> values assumed to be multiples of 1M to make the numbers easier to
> read.
> > > >>
> > > >> noncached_init:
> > > >>
> > > >> // mem_malloc_start = 1000
> > > >> end = ALIGN(mem_malloc_start, MMU_SECTION_SIZE) -
> > > MMU_SECTION_SIZE;
> > > >> // end = 1000 - 2 = 998 // was already aligned, so 1000 not 1002
> > > >> size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY,
> > > >> MMU_SECTION_SIZE); // size = 2 start = end - size; // start = 998
> > > >> - 2 = 996 // region is 996...998
> > > >
> > > > Thanks for this example, it definitely seems a bug.  Just that we
> > > > are fixing it
> > > by adding this gap in the reserve_noncached() also.
> > > > Better would be to fix this subtraction of MMU_SECTION_SIZE by
> > > > aligning
> > > down "end" location, like:
> > > >
> > > > end = ALIGN_DOWN(mem_malloc_start, MMU_SECTION_SIZE); // end
> =
> > > 1000
> > > > size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY,
> MMU_SECTION_SIZE);
> > > // size =
> > > > 2 start = end -size; // start = 998
> > >
> > > That would change yet another piece of code that's been stable for a
> while.
> > > It's late in the U-Boot release cycle, so I think we should be
> > > conservative, and not change any more code than necessary. Changing
> > > lots of extra code would run the risk of introducing more
> > > regressions. I'd rather (a) apply the original change I posted,
> > > which adjusts only the code that caused the regression, or (b) revert the
> patch that caused the regression.
> >
> > Ok, Either way is fine.
> >
> > >
> > > If you want to adjust the code in noncached_init, we can do that
> > > immediately after the release, to give maximum time for any
> > > regressions to be debugged and fixed before the next release.
> >
> > Ok.
> 
> So this patch keeps your use case working and fixes Stephen's problem, to be
> clear?  Thanks guys!

Yes, that's correct.

Cheers,
Vikas

> 
> --
> Tom

[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[Tom Rini]

On Tue, Aug 27, 2019 at 11:54:31AM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> The current code in reserve_noncached() has two issues:
> 
> 1) The first update of gd->start_addr_sp always rounds down to a section
> start. However, the equivalent calculation in cache.c:noncached_init()
> always first rounds up to a section start, then subtracts a section size.
> These two calculations differ if the initial value is already rounded to
> section alignment.
> 
> 2) The second update of gd->start_addr_sp subtracts exactly
> CONFIG_SYS_NONCACHED_MEMORY, whereas the equivalent calculation in
> cache.c:noncached_init() rounds the noncached size up to section
> alignment before subtracting it. The two calculations differ if the
> noncached region size is not a multiple of the MMU section size.
> 
> In practice, one/both of those issues causes a practical problem on
> Jetson TX1; U-Boot triggers a synchronous abort during initialization,
> likely due to overlapping use of some memory region.
> 
> This change fixes both these issues by duplicating the exact calculations
> from noncached_init() into reserve_noncached().
> 
> However, this fix assumes that gd->start_addr_sp on entry to
> reserve_noncached() exactly matches mem_malloc_start on entry to
> noncached_init(). I haven't traced the code to see whether it absolutely
> guarantees this in all (or indeed any!) cases. Consequently, I added some
> comments in the hope that this condition will continue to be true.
> 
> Fixes: 5f7adb5b1c02 ("board_f: reserve noncached space below malloc area")
> Cc: Vikas Manocha <vikas.manocha@st.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190902/56a824ee/attachment.sig>