[PATCH v2 0/2] board: sifive: unmatched: use zero copy for initrd

[PATCH v2 0/2] board: sifive: unmatched: use zero copy for initrd

[Heinrich Schuchardt]

Booting Ubuntu Impish showed the following output:

    relocaddr   = 0x00000000fff60000

    Loading Ramdisk to fa118000, end fffff19d ...

The initrd is overwriting the U-Boot binary. Booting fails.

There is no need to copy the initrd from $ramdisk_addr_r.
Set init_high = ~0UL to use zero copy. Do the same for the device tree.

Rearrange all load addresses:

* put the small files below $kernel_addr_r to not limit the kernel size.
* ramdisk_addr_r should be the highest of all loading addresses to
  avoid limiting the size of the initial RAM disk.

v2:
	rearrange all more addresses

Heinrich Schuchardt (2):
  board: sifive: unmatched: use zero copy for initrd
  board: sifive: unmatched: rearrange load addresses

 include/configs/sifive-unmatched.h | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 
2.32.0

[PATCH v2 1/2] board: sifive: unmatched: use zero copy for initrd

[Heinrich Schuchardt]

Booting Ubuntu Impish showed the following output:

    relocaddr   = 0x00000000fff60000

    Loading Ramdisk to fa118000, end fffff19d ...

The initrd is overwriting the U-Boot binary. Booting fails.

There is no need to copy the initrd from $ramdisk_addr_r.
Set init_high = ~0UL to use zero copy. Do the same for the device tree.

Same for the devicetree.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	Don't copy fdt either.
---
 include/configs/sifive-unmatched.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
index f68d7d7676..69a4eb2f2a 100644
--- a/include/configs/sifive-unmatched.h
+++ b/include/configs/sifive-unmatched.h
@@ -59,6 +59,8 @@
 	"name=system,size=-,bootable,type=${type_guid_gpt_system};"
 
 #define CONFIG_EXTRA_ENV_SETTINGS \
+	"fdt_high=0xffffffffffffffff\0" \
+	"initrd_high=0xffffffffffffffff\0" \
 	"kernel_addr_r=0x84000000\0" \
 	"fdt_addr_r=0x88000000\0" \
 	"scriptaddr=0x88100000\0" \
-- 
2.32.0

[PATCH v2 2/2] board: sifive: unmatched: rearrange load addresses

[Heinrich Schuchardt]

* Put the small files below $kernel_addr_r to not limit the kernel size.
* ramdisk_addr_r should be the highest of all loading addresses to
  avoid limiting the size of the initial RAM disk.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	new patch
---
 include/configs/sifive-unmatched.h | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
index 69a4eb2f2a..7a6f4acf4d 100644
--- a/include/configs/sifive-unmatched.h
+++ b/include/configs/sifive-unmatched.h
@@ -61,13 +61,14 @@
 #define CONFIG_EXTRA_ENV_SETTINGS \
 	"fdt_high=0xffffffffffffffff\0" \
 	"initrd_high=0xffffffffffffffff\0" \
-	"kernel_addr_r=0x84000000\0" \
-	"fdt_addr_r=0x88000000\0" \
-	"scriptaddr=0x88100000\0" \
-	"pxefile_addr_r=0x88200000\0" \
-	"ramdisk_addr_r=0x88300000\0" \
-	"kernel_comp_addr_r=0x90000000\0" \
-	"kernel_comp_size=0x4000000\0" \
+	"fdt_addr_r=0x84000000\0" \
+	"scriptaddr=0x84100000\0" \
+	"pxefile_addr_r=0x84200000\0" \
+	"kernel_addr_r=0x85000000\0" \
+	"loadaddr=0x85000000\0" \
+	"kernel_comp_addr_r=0x8b000000\0" \
+	"kernel_comp_size=0x05000000\0" \
+	"ramdisk_addr_r=0x90000000\0" \
 	"type_guid_gpt_loader1=" TYPE_GUID_LOADER1 "\0" \
 	"type_guid_gpt_loader2=" TYPE_GUID_LOADER2 "\0" \
 	"type_guid_gpt_system=" TYPE_GUID_SYSTEM "\0" \
-- 
2.32.0

Re: [PATCH v2 2/2] board: sifive: unmatched: rearrange load addresses

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

On Tue, Nov 09, 2021 at 03:46:01PM +0100, Heinrich Schuchardt wrote:

> * Put the small files below $kernel_addr_r to not limit the kernel size.
> * ramdisk_addr_r should be the highest of all loading addresses to
>   avoid limiting the size of the initial RAM disk.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	new patch
> ---
>  include/configs/sifive-unmatched.h | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> index 69a4eb2f2a..7a6f4acf4d 100644
> --- a/include/configs/sifive-unmatched.h
> +++ b/include/configs/sifive-unmatched.h
> @@ -61,13 +61,14 @@
>  #define CONFIG_EXTRA_ENV_SETTINGS \
>  	"fdt_high=0xffffffffffffffff\0" \
>  	"initrd_high=0xffffffffffffffff\0" \
> -	"kernel_addr_r=0x84000000\0" \
> -	"fdt_addr_r=0x88000000\0" \
> -	"scriptaddr=0x88100000\0" \
> -	"pxefile_addr_r=0x88200000\0" \
> -	"ramdisk_addr_r=0x88300000\0" \
> -	"kernel_comp_addr_r=0x90000000\0" \
> -	"kernel_comp_size=0x4000000\0" \
> +	"fdt_addr_r=0x84000000\0" \
> +	"scriptaddr=0x84100000\0" \
> +	"pxefile_addr_r=0x84200000\0" \
> +	"kernel_addr_r=0x85000000\0" \
> +	"loadaddr=0x85000000\0" \
> +	"kernel_comp_addr_r=0x8b000000\0" \
> +	"kernel_comp_size=0x05000000\0" \
> +	"ramdisk_addr_r=0x90000000\0" \
>  	"type_guid_gpt_loader1=" TYPE_GUID_LOADER1 "\0" \
>  	"type_guid_gpt_loader2=" TYPE_GUID_LOADER2 "\0" \
>  	"type_guid_gpt_system=" TYPE_GUID_SYSTEM "\0" \

NAK.  Now that riscv finally has arch_lmb support, we need to stop
disabling relocation of the device tree.  We should also stop disabling
relocation of the initrd, and instead (a) do like you're doing and set
sane default locations and then (b) set bootm_low/bootm_size such that
we won't possibly be moving things outside of the acceptable to the
kernel range for everything.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 2/2] board: sifive: unmatched: rearrange load addresses

[Heinrich Schuchardt]

On 11/9/21 16:43, Tom Rini wrote:
> On Tue, Nov 09, 2021 at 03:46:01PM +0100, Heinrich Schuchardt wrote:
> 
>> * Put the small files below $kernel_addr_r to not limit the kernel size.
>> * ramdisk_addr_r should be the highest of all loading addresses to
>>    avoid limiting the size of the initial RAM disk.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> v2:
>> 	new patch
>> ---
>>   include/configs/sifive-unmatched.h | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
>> index 69a4eb2f2a..7a6f4acf4d 100644
>> --- a/include/configs/sifive-unmatched.h
>> +++ b/include/configs/sifive-unmatched.h
>> @@ -61,13 +61,14 @@
>>   #define CONFIG_EXTRA_ENV_SETTINGS \
>>   	"fdt_high=0xffffffffffffffff\0" \
>>   	"initrd_high=0xffffffffffffffff\0" \
>> -	"kernel_addr_r=0x84000000\0" \
>> -	"fdt_addr_r=0x88000000\0" \
>> -	"scriptaddr=0x88100000\0" \
>> -	"pxefile_addr_r=0x88200000\0" \
>> -	"ramdisk_addr_r=0x88300000\0" \
>> -	"kernel_comp_addr_r=0x90000000\0" \
>> -	"kernel_comp_size=0x4000000\0" \
>> +	"fdt_addr_r=0x84000000\0" \
>> +	"scriptaddr=0x84100000\0" \
>> +	"pxefile_addr_r=0x84200000\0" \
>> +	"kernel_addr_r=0x85000000\0" \
>> +	"loadaddr=0x85000000\0" \
>> +	"kernel_comp_addr_r=0x8b000000\0" \
>> +	"kernel_comp_size=0x05000000\0" \
>> +	"ramdisk_addr_r=0x90000000\0" \
>>   	"type_guid_gpt_loader1=" TYPE_GUID_LOADER1 "\0" \
>>   	"type_guid_gpt_loader2=" TYPE_GUID_LOADER2 "\0" \
>>   	"type_guid_gpt_system=" TYPE_GUID_SYSTEM "\0" \
> 
> NAK.  Now that riscv finally has arch_lmb support, we need to stop
> disabling relocation of the device tree.  We should also stop disabling
> relocation of the initrd, and instead (a) do like you're doing and set
> sane default locations and then (b) set bootm_low/bootm_size such that
> we won't possibly be moving things outside of the acceptable to the
> kernel range for everything.
> 

Why should we relocate initrd or ftd?
This never should have been enabled on RISC-V.

Best regards

Heinrich

Re: [PATCH v2 1/2] board: sifive: unmatched: use zero copy for initrd

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

On Tue, Nov 09, 2021 at 03:46:00PM +0100, Heinrich Schuchardt wrote:

> Booting Ubuntu Impish showed the following output:
> 
>     relocaddr   = 0x00000000fff60000
> 
>     Loading Ramdisk to fa118000, end fffff19d ...
> 
> The initrd is overwriting the U-Boot binary. Booting fails.
> 
> There is no need to copy the initrd from $ramdisk_addr_r.
> Set init_high = ~0UL to use zero copy. Do the same for the device tree.
> 
> Same for the devicetree.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	Don't copy fdt either.
> ---
>  include/configs/sifive-unmatched.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> index f68d7d7676..69a4eb2f2a 100644
> --- a/include/configs/sifive-unmatched.h
> +++ b/include/configs/sifive-unmatched.h
> @@ -59,6 +59,8 @@
>  	"name=system,size=-,bootable,type=${type_guid_gpt_system};"
>  
>  #define CONFIG_EXTRA_ENV_SETTINGS \
> +	"fdt_high=0xffffffffffffffff\0" \
> +	"initrd_high=0xffffffffffffffff\0" \
>  	"kernel_addr_r=0x84000000\0" \
>  	"fdt_addr_r=0x88000000\0" \
>  	"scriptaddr=0x88100000\0" \

I know I'm doing this out of order, but I'm going to repeat myself here
too.  You cannot disable device tree relocation.  I cannot count the
number of hours that have been wasted because of this mis-feature due to
misalignment of the device tree or overwriting of the device tree and
then U-Boot not fixing it because it was told not to, and then people
and projects wasting countless hours over it.  It's why checkpatch.pl
throws out an ERROR on this now.  I didn't yell even more loudly
previously at riscv because as it was missing the arch_lmb portion to
avoid overwriting U-Boot at run-time, it still was a problem.  But
that's been fixed.  So, no.  NAK.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 2/2] board: sifive: unmatched: rearrange load addresses

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2904 bytes --]

On Tue, Nov 09, 2021 at 04:46:00PM +0100, Heinrich Schuchardt wrote:
> On 11/9/21 16:43, Tom Rini wrote:
> > On Tue, Nov 09, 2021 at 03:46:01PM +0100, Heinrich Schuchardt wrote:
> > 
> > > * Put the small files below $kernel_addr_r to not limit the kernel size.
> > > * ramdisk_addr_r should be the highest of all loading addresses to
> > >    avoid limiting the size of the initial RAM disk.
> > > 
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > > v2:
> > > 	new patch
> > > ---
> > >   include/configs/sifive-unmatched.h | 15 ++++++++-------
> > >   1 file changed, 8 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> > > index 69a4eb2f2a..7a6f4acf4d 100644
> > > --- a/include/configs/sifive-unmatched.h
> > > +++ b/include/configs/sifive-unmatched.h
> > > @@ -61,13 +61,14 @@
> > >   #define CONFIG_EXTRA_ENV_SETTINGS \
> > >   	"fdt_high=0xffffffffffffffff\0" \
> > >   	"initrd_high=0xffffffffffffffff\0" \
> > > -	"kernel_addr_r=0x84000000\0" \
> > > -	"fdt_addr_r=0x88000000\0" \
> > > -	"scriptaddr=0x88100000\0" \
> > > -	"pxefile_addr_r=0x88200000\0" \
> > > -	"ramdisk_addr_r=0x88300000\0" \
> > > -	"kernel_comp_addr_r=0x90000000\0" \
> > > -	"kernel_comp_size=0x4000000\0" \
> > > +	"fdt_addr_r=0x84000000\0" \
> > > +	"scriptaddr=0x84100000\0" \
> > > +	"pxefile_addr_r=0x84200000\0" \
> > > +	"kernel_addr_r=0x85000000\0" \
> > > +	"loadaddr=0x85000000\0" \
> > > +	"kernel_comp_addr_r=0x8b000000\0" \
> > > +	"kernel_comp_size=0x05000000\0" \
> > > +	"ramdisk_addr_r=0x90000000\0" \
> > >   	"type_guid_gpt_loader1=" TYPE_GUID_LOADER1 "\0" \
> > >   	"type_guid_gpt_loader2=" TYPE_GUID_LOADER2 "\0" \
> > >   	"type_guid_gpt_system=" TYPE_GUID_SYSTEM "\0" \
> > 
> > NAK.  Now that riscv finally has arch_lmb support, we need to stop
> > disabling relocation of the device tree.  We should also stop disabling
> > relocation of the initrd, and instead (a) do like you're doing and set
> > sane default locations and then (b) set bootm_low/bootm_size such that
> > we won't possibly be moving things outside of the acceptable to the
> > kernel range for everything.
> 
> Why should we relocate initrd or ftd?

You MUST relocate the device tree for all of the times it will be loaded
in at an incorrectly aligned address.  Further, for all of the times it
may well end up being overwritten as it doesn't end up at $fdt_addr_r
but somewhere else.

Disabling relocation of the initrd is strongly discouraged as it's still
quite possible to end up with "Well, we're overlapping the kernel and
initrd, but told to ignore that and just proceed...".

> This never should have been enabled on RISC-V.

Yes, RISC-V was missing the lmb code to avoid overwriting U-Boot at
run-time until recently.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/2] board: sifive: unmatched: use zero copy for initrd

[Heinrich Schuchardt]

On 11/9/21 16:46, Tom Rini wrote:
> On Tue, Nov 09, 2021 at 03:46:00PM +0100, Heinrich Schuchardt wrote:
> 
>> Booting Ubuntu Impish showed the following output:
>>
>>      relocaddr   = 0x00000000fff60000
>>
>>      Loading Ramdisk to fa118000, end fffff19d ...
>>
>> The initrd is overwriting the U-Boot binary. Booting fails.
>>
>> There is no need to copy the initrd from $ramdisk_addr_r.
>> Set init_high = ~0UL to use zero copy. Do the same for the device tree.
>>
>> Same for the devicetree.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> v2:
>> 	Don't copy fdt either.
>> ---
>>   include/configs/sifive-unmatched.h | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
>> index f68d7d7676..69a4eb2f2a 100644
>> --- a/include/configs/sifive-unmatched.h
>> +++ b/include/configs/sifive-unmatched.h
>> @@ -59,6 +59,8 @@
>>   	"name=system,size=-,bootable,type=${type_guid_gpt_system};"
>>   
>>   #define CONFIG_EXTRA_ENV_SETTINGS \
>> +	"fdt_high=0xffffffffffffffff\0" \
>> +	"initrd_high=0xffffffffffffffff\0" \
>>   	"kernel_addr_r=0x84000000\0" \
>>   	"fdt_addr_r=0x88000000\0" \
>>   	"scriptaddr=0x88100000\0" \
> 
> I know I'm doing this out of order, but I'm going to repeat myself here
> too.  You cannot disable device tree relocation.  I cannot count the
> number of hours that have been wasted because of this mis-feature due to
> misalignment of the device tree or overwriting of the device tree and
> then U-Boot not fixing it because it was told not to, and then people
> and projects wasting countless hours over it.  It's why checkpatch.pl
> throws out an ERROR on this now.  I didn't yell even more loudly
> previously at riscv because as it was missing the arch_lmb portion to
> avoid overwriting U-Boot at run-time, it still was a problem.  But
> that's been fixed.  So, no.  NAK.

Why should the devicetree relocated?
This should never have been enabled on RISC-V.

Best regards

Heinrich

Re: [PATCH v2 1/2] board: sifive: unmatched: use zero copy for initrd

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3011 bytes --]

On Tue, Nov 09, 2021 at 04:50:27PM +0100, Heinrich Schuchardt wrote:
> On 11/9/21 16:46, Tom Rini wrote:
> > On Tue, Nov 09, 2021 at 03:46:00PM +0100, Heinrich Schuchardt wrote:
> > 
> > > Booting Ubuntu Impish showed the following output:
> > > 
> > >      relocaddr   = 0x00000000fff60000
> > > 
> > >      Loading Ramdisk to fa118000, end fffff19d ...
> > > 
> > > The initrd is overwriting the U-Boot binary. Booting fails.
> > > 
> > > There is no need to copy the initrd from $ramdisk_addr_r.
> > > Set init_high = ~0UL to use zero copy. Do the same for the device tree.
> > > 
> > > Same for the devicetree.
> > > 
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > > v2:
> > > 	Don't copy fdt either.
> > > ---
> > >   include/configs/sifive-unmatched.h | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> > > index f68d7d7676..69a4eb2f2a 100644
> > > --- a/include/configs/sifive-unmatched.h
> > > +++ b/include/configs/sifive-unmatched.h
> > > @@ -59,6 +59,8 @@
> > >   	"name=system,size=-,bootable,type=${type_guid_gpt_system};"
> > >   #define CONFIG_EXTRA_ENV_SETTINGS \
> > > +	"fdt_high=0xffffffffffffffff\0" \
> > > +	"initrd_high=0xffffffffffffffff\0" \
> > >   	"kernel_addr_r=0x84000000\0" \
> > >   	"fdt_addr_r=0x88000000\0" \
> > >   	"scriptaddr=0x88100000\0" \
> > 
> > I know I'm doing this out of order, but I'm going to repeat myself here
> > too.  You cannot disable device tree relocation.  I cannot count the
> > number of hours that have been wasted because of this mis-feature due to
> > misalignment of the device tree or overwriting of the device tree and
> > then U-Boot not fixing it because it was told not to, and then people
> > and projects wasting countless hours over it.  It's why checkpatch.pl
> > throws out an ERROR on this now.  I didn't yell even more loudly
> > previously at riscv because as it was missing the arch_lmb portion to
> > avoid overwriting U-Boot at run-time, it still was a problem.  But
> > that's been fixed.  So, no.  NAK.
> 
> Why should the devicetree relocated?
> This should never have been enabled on RISC-V.

To repeat myself, because RISC-V has been broken until very recently and
lacked the parts of lmb to avoid overwriting U-Boot while running, is
why any platforms have been allowed in with fdt/initrd_high set to
disable relocation.  As that problem has now been fixed, fdt relocation
must be re-enabled on the currently wrong platforms, and will not be
allowed on new platforms.

There are specific deployment cases where the developer can choose to
disable relocation because they know that there will never be a way for
things to be done in an overlapping manner because the system is locked
down.  That is very rarely the case for mainline and absolutely not the
case for a general purpose board like the unmatched.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/2] board: sifive: unmatched: use zero copy for initrd

[Heinrich Schuchardt]

On 11/9/21 17:07, Tom Rini wrote:
> On Tue, Nov 09, 2021 at 04:50:27PM +0100, Heinrich Schuchardt wrote:
>> On 11/9/21 16:46, Tom Rini wrote:
>>> On Tue, Nov 09, 2021 at 03:46:00PM +0100, Heinrich Schuchardt wrote:
>>>
>>>> Booting Ubuntu Impish showed the following output:
>>>>
>>>>       relocaddr   = 0x00000000fff60000
>>>>
>>>>       Loading Ramdisk to fa118000, end fffff19d ...
>>>>
>>>> The initrd is overwriting the U-Boot binary. Booting fails.
>>>>
>>>> There is no need to copy the initrd from $ramdisk_addr_r.
>>>> Set init_high = ~0UL to use zero copy. Do the same for the device tree.
>>>>
>>>> Same for the devicetree.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>> v2:
>>>> 	Don't copy fdt either.
>>>> ---
>>>>    include/configs/sifive-unmatched.h | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
>>>> index f68d7d7676..69a4eb2f2a 100644
>>>> --- a/include/configs/sifive-unmatched.h
>>>> +++ b/include/configs/sifive-unmatched.h
>>>> @@ -59,6 +59,8 @@
>>>>    	"name=system,size=-,bootable,type=${type_guid_gpt_system};"
>>>>    #define CONFIG_EXTRA_ENV_SETTINGS \
>>>> +	"fdt_high=0xffffffffffffffff\0" \
>>>> +	"initrd_high=0xffffffffffffffff\0" \
>>>>    	"kernel_addr_r=0x84000000\0" \
>>>>    	"fdt_addr_r=0x88000000\0" \
>>>>    	"scriptaddr=0x88100000\0" \
>>>
>>> I know I'm doing this out of order, but I'm going to repeat myself here
>>> too.  You cannot disable device tree relocation.  I cannot count the
>>> number of hours that have been wasted because of this mis-feature due to
>>> misalignment of the device tree or overwriting of the device tree and
>>> then U-Boot not fixing it because it was told not to, and then people
>>> and projects wasting countless hours over it.  It's why checkpatch.pl
>>> throws out an ERROR on this now.  I didn't yell even more loudly
>>> previously at riscv because as it was missing the arch_lmb portion to
>>> avoid overwriting U-Boot at run-time, it still was a problem.  But
>>> that's been fixed.  So, no.  NAK.
>>
>> Why should the devicetree relocated?
>> This should never have been enabled on RISC-V.
> 
> To repeat myself, because RISC-V has been broken until very recently and
> lacked the parts of lmb to avoid overwriting U-Boot while running, is
> why any platforms have been allowed in with fdt/initrd_high set to
> disable relocation.  As that problem has now been fixed, fdt relocation
> must be re-enabled on the currently wrong platforms, and will not be
> allowed on new platforms.
> 
> There are specific deployment cases where the developer can choose to
> disable relocation because they know that there will never be a way for
> things to be done in an overlapping manner because the system is locked
> down.  That is very rarely the case for mainline and absolutely not the
> case for a general purpose board like the unmatched.
> 

__lmb_alloc_base() seems not be integrated with the UEFI sub-system. So 
UEFI might hand out memory marked as reserved in the LMB sub-system.

I guess this is still a topic to be addressed.

Best regards

Heinrich

Re: [PATCH v2 1/2] board: sifive: unmatched: use zero copy for initrd

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3810 bytes --]

On Tue, Nov 09, 2021 at 05:27:52PM +0100, Heinrich Schuchardt wrote:
> On 11/9/21 17:07, Tom Rini wrote:
> > On Tue, Nov 09, 2021 at 04:50:27PM +0100, Heinrich Schuchardt wrote:
> > > On 11/9/21 16:46, Tom Rini wrote:
> > > > On Tue, Nov 09, 2021 at 03:46:00PM +0100, Heinrich Schuchardt wrote:
> > > > 
> > > > > Booting Ubuntu Impish showed the following output:
> > > > > 
> > > > >       relocaddr   = 0x00000000fff60000
> > > > > 
> > > > >       Loading Ramdisk to fa118000, end fffff19d ...
> > > > > 
> > > > > The initrd is overwriting the U-Boot binary. Booting fails.
> > > > > 
> > > > > There is no need to copy the initrd from $ramdisk_addr_r.
> > > > > Set init_high = ~0UL to use zero copy. Do the same for the device tree.
> > > > > 
> > > > > Same for the devicetree.
> > > > > 
> > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > ---
> > > > > v2:
> > > > > 	Don't copy fdt either.
> > > > > ---
> > > > >    include/configs/sifive-unmatched.h | 2 ++
> > > > >    1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> > > > > index f68d7d7676..69a4eb2f2a 100644
> > > > > --- a/include/configs/sifive-unmatched.h
> > > > > +++ b/include/configs/sifive-unmatched.h
> > > > > @@ -59,6 +59,8 @@
> > > > >    	"name=system,size=-,bootable,type=${type_guid_gpt_system};"
> > > > >    #define CONFIG_EXTRA_ENV_SETTINGS \
> > > > > +	"fdt_high=0xffffffffffffffff\0" \
> > > > > +	"initrd_high=0xffffffffffffffff\0" \
> > > > >    	"kernel_addr_r=0x84000000\0" \
> > > > >    	"fdt_addr_r=0x88000000\0" \
> > > > >    	"scriptaddr=0x88100000\0" \
> > > > 
> > > > I know I'm doing this out of order, but I'm going to repeat myself here
> > > > too.  You cannot disable device tree relocation.  I cannot count the
> > > > number of hours that have been wasted because of this mis-feature due to
> > > > misalignment of the device tree or overwriting of the device tree and
> > > > then U-Boot not fixing it because it was told not to, and then people
> > > > and projects wasting countless hours over it.  It's why checkpatch.pl
> > > > throws out an ERROR on this now.  I didn't yell even more loudly
> > > > previously at riscv because as it was missing the arch_lmb portion to
> > > > avoid overwriting U-Boot at run-time, it still was a problem.  But
> > > > that's been fixed.  So, no.  NAK.
> > > 
> > > Why should the devicetree relocated?
> > > This should never have been enabled on RISC-V.
> > 
> > To repeat myself, because RISC-V has been broken until very recently and
> > lacked the parts of lmb to avoid overwriting U-Boot while running, is
> > why any platforms have been allowed in with fdt/initrd_high set to
> > disable relocation.  As that problem has now been fixed, fdt relocation
> > must be re-enabled on the currently wrong platforms, and will not be
> > allowed on new platforms.
> > 
> > There are specific deployment cases where the developer can choose to
> > disable relocation because they know that there will never be a way for
> > things to be done in an overlapping manner because the system is locked
> > down.  That is very rarely the case for mainline and absolutely not the
> > case for a general purpose board like the unmatched.
> 
> __lmb_alloc_base() seems not be integrated with the UEFI sub-system. So UEFI
> might hand out memory marked as reserved in the LMB sub-system.
> 
> I guess this is still a topic to be addressed.

If UEFI can still end up getting U-Boot overwritten, yes, that needs to
be addressed.  Only slightly surprised one of the capture-the-flag or
similar events hasn't come to us yet with some CVEs related to that,
too.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/2] board: sifive: unmatched: use zero copy for initrd

[David Abdurachmanov]

On Tue, Nov 9, 2021 at 7:11 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Nov 09, 2021 at 05:27:52PM +0100, Heinrich Schuchardt wrote:
> > On 11/9/21 17:07, Tom Rini wrote:
> > > On Tue, Nov 09, 2021 at 04:50:27PM +0100, Heinrich Schuchardt wrote:
> > > > On 11/9/21 16:46, Tom Rini wrote:
> > > > > On Tue, Nov 09, 2021 at 03:46:00PM +0100, Heinrich Schuchardt wrote:
> > > > >
> > > > > > Booting Ubuntu Impish showed the following output:
> > > > > >
> > > > > >       relocaddr   = 0x00000000fff60000
> > > > > >
> > > > > >       Loading Ramdisk to fa118000, end fffff19d ...
> > > > > >
> > > > > > The initrd is overwriting the U-Boot binary. Booting fails.
> > > > > >
> > > > > > There is no need to copy the initrd from $ramdisk_addr_r.
> > > > > > Set init_high = ~0UL to use zero copy. Do the same for the device tree.
> > > > > >
> > > > > > Same for the devicetree.
> > > > > >
> > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > > ---
> > > > > > v2:
> > > > > >       Don't copy fdt either.
> > > > > > ---
> > > > > >    include/configs/sifive-unmatched.h | 2 ++
> > > > > >    1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> > > > > > index f68d7d7676..69a4eb2f2a 100644
> > > > > > --- a/include/configs/sifive-unmatched.h
> > > > > > +++ b/include/configs/sifive-unmatched.h
> > > > > > @@ -59,6 +59,8 @@
> > > > > >       "name=system,size=-,bootable,type=${type_guid_gpt_system};"
> > > > > >    #define CONFIG_EXTRA_ENV_SETTINGS \
> > > > > > +     "fdt_high=0xffffffffffffffff\0" \
> > > > > > +     "initrd_high=0xffffffffffffffff\0" \
> > > > > >       "kernel_addr_r=0x84000000\0" \
> > > > > >       "fdt_addr_r=0x88000000\0" \
> > > > > >       "scriptaddr=0x88100000\0" \
> > > > >
> > > > > I know I'm doing this out of order, but I'm going to repeat myself here
> > > > > too.  You cannot disable device tree relocation.  I cannot count the
> > > > > number of hours that have been wasted because of this mis-feature due to
> > > > > misalignment of the device tree or overwriting of the device tree and
> > > > > then U-Boot not fixing it because it was told not to, and then people
> > > > > and projects wasting countless hours over it.  It's why checkpatch.pl
> > > > > throws out an ERROR on this now.  I didn't yell even more loudly
> > > > > previously at riscv because as it was missing the arch_lmb portion to
> > > > > avoid overwriting U-Boot at run-time, it still was a problem.  But
> > > > > that's been fixed.  So, no.  NAK.
> > > >
> > > > Why should the devicetree relocated?
> > > > This should never have been enabled on RISC-V.
> > >
> > > To repeat myself, because RISC-V has been broken until very recently and
> > > lacked the parts of lmb to avoid overwriting U-Boot while running, is
> > > why any platforms have been allowed in with fdt/initrd_high set to
> > > disable relocation.  As that problem has now been fixed, fdt relocation
> > > must be re-enabled on the currently wrong platforms, and will not be
> > > allowed on new platforms.
> > >
> > > There are specific deployment cases where the developer can choose to
> > > disable relocation because they know that there will never be a way for
> > > things to be done in an overlapping manner because the system is locked
> > > down.  That is very rarely the case for mainline and absolutely not the
> > > case for a general purpose board like the unmatched.
> >
> > __lmb_alloc_base() seems not be integrated with the UEFI sub-system. So UEFI
> > might hand out memory marked as reserved in the LMB sub-system.

Heinrich, do you plan to work on this?

It would be great if we could finally solve this situation with the
2022.01 release.

Right now Unmatched is probably the only board that doesn't turn off
the relocation, and thus some people hit the issues.

Looking at the git history, the LMB issue is fixed in v2022.01-rc1
only right now. Minus UEFI part, which is/will be important for
distributions.

david

> >
> > I guess this is still a topic to be addressed.
>
> If UEFI can still end up getting U-Boot overwritten, yes, that needs to
> be addressed.  Only slightly surprised one of the capture-the-flag or
> similar events hasn't come to us yet with some CVEs related to that,
> too.
>
> --
> Tom

Re: LMB and UEFI memory management not integrated.

[Heinrich Schuchardt]

>>>
>>> __lmb_alloc_base() seems not be integrated with the UEFI sub-system. So UEFI
>>> might hand out memory marked as reserved in the LMB sub-system.
> 
> Heinrich, do you plan to work on this?

I am not sure if I will have enough capacity to work on the LMB/UEFI 
integration. But it is surely an issue to be solved.

Best regards

Heinrich

> 
> It would be great if we could finally solve this situation with the
> 2022.01 release.
> 
> Right now Unmatched is probably the only board that doesn't turn off
> the relocation, and thus some people hit the issues.
> 
> Looking at the git history, the LMB issue is fixed in v2022.01-rc1
> only right now. Minus UEFI part, which is/will be important for
> distributions.
> 
> david
> 
>>>
>>> I guess this is still a topic to be addressed.
>>
>> If UEFI can still end up getting U-Boot overwritten, yes, that needs to
>> be addressed.  Only slightly surprised one of the capture-the-flag or
>> similar events hasn't come to us yet with some CVEs related to that,
>> too.
>>
>> --
>> Tom