[RFC 1/1] board: sifive: unmatched: use zero copy for initrd

[RFC 1/1] 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.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
Generally copying to another memory location than $ramdisk_addr_r provides
no benefit whatsoever.

But we still should find out why the initrd relocation fails so badly.
---
 include/configs/sifive-unmatched.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
index d63a5f62fb..8dcfffedbe 100644
--- a/include/configs/sifive-unmatched.h
+++ b/include/configs/sifive-unmatched.h
@@ -67,6 +67,7 @@
 	"scriptaddr=0x88100000\0" \
 	"pxefile_addr_r=0x88200000\0" \
 	"ramdisk_addr_r=0x88300000\0" \
+	"initrd_high=0xffffffffffffffff\0" \
 	"kernel_comp_addr_r=0x90000000\0" \
 	"kernel_comp_size=0x4000000\0" \
 	"type_guid_gpt_loader1=" TYPE_GUID_LOADER1 "\0" \
--
2.30.2

Re: [RFC 1/1] board: sifive: unmatched: use zero copy for initrd

[Heinrich Schuchardt]

On 19.07.21 23:38, 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.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> Generally copying to another memory location than $ramdisk_addr_r provides
> no benefit whatsoever.
>
> But we still should find out why the initrd relocation fails so badly.
> ---
>   include/configs/sifive-unmatched.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> index d63a5f62fb..8dcfffedbe 100644
> --- a/include/configs/sifive-unmatched.h
> +++ b/include/configs/sifive-unmatched.h
> @@ -67,6 +67,7 @@
>   	"scriptaddr=0x88100000\0" \
>   	"pxefile_addr_r=0x88200000\0" \
>   	"ramdisk_addr_r=0x88300000\0" \
> +	"initrd_high=0xffffffffffffffff\0" \
>   	"kernel_comp_addr_r=0x90000000\0" \

Tom remarked that kernel_comp_addr_r is poorly chosen as it will overlap
with initrd if initrd exceeds 125 MiB. Current RISC-V Linux thinks
initrd should fit into the first 128 MiB of RAM but that is an
unnecessary restriction in Linux.

Best regards

Heinrich

>   	"kernel_comp_size=0x4000000\0" \
>   	"type_guid_gpt_loader1=" TYPE_GUID_LOADER1 "\0" \
> --
> 2.30.2
>

Re: [RFC 1/1] board: sifive: unmatched: use zero copy for initrd

[Tom Rini]

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

On Mon, Jul 19, 2021 at 11:52:09PM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 19.07.21 23:38, 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.
> > 
> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > ---
> > Generally copying to another memory location than $ramdisk_addr_r provides
> > no benefit whatsoever.
> > 
> > But we still should find out why the initrd relocation fails so badly.
> > ---
> >   include/configs/sifive-unmatched.h | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> > index d63a5f62fb..8dcfffedbe 100644
> > --- a/include/configs/sifive-unmatched.h
> > +++ b/include/configs/sifive-unmatched.h
> > @@ -67,6 +67,7 @@
> >   	"scriptaddr=0x88100000\0" \
> >   	"pxefile_addr_r=0x88200000\0" \
> >   	"ramdisk_addr_r=0x88300000\0" \
> > +	"initrd_high=0xffffffffffffffff\0" \
> >   	"kernel_comp_addr_r=0x90000000\0" \
> 
> Tom remarked that kernel_comp_addr_r is poorly chosen as it will overlap
> with initrd if initrd exceeds 125 MiB. Current RISC-V Linux thinks
> initrd should fit into the first 128 MiB of RAM but that is an
> unnecessary restriction in Linux.

I'll repeat what I said on IRC as well.  I lament that there does not
seem to be as detailed "booting" requirements in Linux for RISC-V as
there is for arm/aarch64 because we REALLY need something like:
https://source.denx.de/u-boot/u-boot/-/blob/master/include/configs/ti_armv7_common.h#L32
but for one or more RISC-V platforms (and well, MIPS and everyone else
using device trees) that can be referenced when bringing up a new board
to get good and always safe addresses to use.

And I'll even say now that the ti_armv7_common.h example could be
improved upon because I'm not super thrilled with the DTBO storage spot,
but I don't have a better answer off the top of my head.

-- 
Tom

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

Re: [RFC 1/1] board: sifive: unmatched: use zero copy for initrd

[David Abdurachmanov]

On Tue, Jul 20, 2021 at 1:06 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Jul 19, 2021 at 11:52:09PM +0200, Heinrich Schuchardt wrote:
> >
> >
> > On 19.07.21 23:38, 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.
> > >
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > > Generally copying to another memory location than $ramdisk_addr_r provides
> > > no benefit whatsoever.
> > >
> > > But we still should find out why the initrd relocation fails so badly.
> > > ---
> > >   include/configs/sifive-unmatched.h | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> > > index d63a5f62fb..8dcfffedbe 100644
> > > --- a/include/configs/sifive-unmatched.h
> > > +++ b/include/configs/sifive-unmatched.h
> > > @@ -67,6 +67,7 @@
> > >     "scriptaddr=0x88100000\0" \
> > >     "pxefile_addr_r=0x88200000\0" \
> > >     "ramdisk_addr_r=0x88300000\0" \
> > > +   "initrd_high=0xffffffffffffffff\0" \
> > >     "kernel_comp_addr_r=0x90000000\0" \
> >
> > Tom remarked that kernel_comp_addr_r is poorly chosen as it will overlap
> > with initrd if initrd exceeds 125 MiB. Current RISC-V Linux thinks
> > initrd should fit into the first 128 MiB of RAM but that is an
> > unnecessary restriction in Linux.

While I don't expect this to be a major issue we should definitely fix it.
When I originally picked the value I wasn't thinking about it and
initrd images were small.

david

>
> I'll repeat what I said on IRC as well.  I lament that there does not
> seem to be as detailed "booting" requirements in Linux for RISC-V as
> there is for arm/aarch64 because we REALLY need something like:
> https://source.denx.de/u-boot/u-boot/-/blob/master/include/configs/ti_armv7_common.h#L32
> but for one or more RISC-V platforms (and well, MIPS and everyone else
> using device trees) that can be referenced when bringing up a new board
> to get good and always safe addresses to use.
>
> And I'll even say now that the ti_armv7_common.h example could be
> improved upon because I'm not super thrilled with the DTBO storage spot,
> but I don't have a better answer off the top of my head.
>
> --
> Tom

Re: [RFC 1/1] board: sifive: unmatched: use zero copy for initrd

[Tom Rini]

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

On Tue, Jul 20, 2021 at 04:46:23PM +0300, David Abdurachmanov wrote:
> On Tue, Jul 20, 2021 at 1:06 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Jul 19, 2021 at 11:52:09PM +0200, Heinrich Schuchardt wrote:
> > >
> > >
> > > On 19.07.21 23:38, 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.
> > > >
> > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > ---
> > > > Generally copying to another memory location than $ramdisk_addr_r provides
> > > > no benefit whatsoever.
> > > >
> > > > But we still should find out why the initrd relocation fails so badly.
> > > > ---
> > > >   include/configs/sifive-unmatched.h | 1 +
> > > >   1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> > > > index d63a5f62fb..8dcfffedbe 100644
> > > > --- a/include/configs/sifive-unmatched.h
> > > > +++ b/include/configs/sifive-unmatched.h
> > > > @@ -67,6 +67,7 @@
> > > >     "scriptaddr=0x88100000\0" \
> > > >     "pxefile_addr_r=0x88200000\0" \
> > > >     "ramdisk_addr_r=0x88300000\0" \
> > > > +   "initrd_high=0xffffffffffffffff\0" \
> > > >     "kernel_comp_addr_r=0x90000000\0" \
> > >
> > > Tom remarked that kernel_comp_addr_r is poorly chosen as it will overlap
> > > with initrd if initrd exceeds 125 MiB. Current RISC-V Linux thinks
> > > initrd should fit into the first 128 MiB of RAM but that is an
> > > unnecessary restriction in Linux.
> 
> While I don't expect this to be a major issue we should definitely fix it.
> When I originally picked the value I wasn't thinking about it and
> initrd images were small.

Yeah, honestly, I think it should be a high priority thing to fix /
document.  Distro initrds tend to not be small and these overlaps end up
being a huge problem for end users.  A good and documented example of
where to put everything relative to start of DDR so that nothing should
overlap by default makes things so much easier for everyone.

-- 
Tom

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

Re: [RFC 1/1] board: sifive: unmatched: use zero copy for initrd

[Heinrich Schuchardt]

On 7/20/21 4:06 PM, Tom Rini wrote:
> On Tue, Jul 20, 2021 at 04:46:23PM +0300, David Abdurachmanov wrote:
>> On Tue, Jul 20, 2021 at 1:06 AM Tom Rini <trini@konsulko.com> wrote:
>>>
>>> On Mon, Jul 19, 2021 at 11:52:09PM +0200, Heinrich Schuchardt wrote:
>>>>
>>>>
>>>> On 19.07.21 23:38, 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.
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>>> ---
>>>>> Generally copying to another memory location than $ramdisk_addr_r provides
>>>>> no benefit whatsoever.
>>>>>
>>>>> But we still should find out why the initrd relocation fails so badly.
>>>>> ---
>>>>>    include/configs/sifive-unmatched.h | 1 +
>>>>>    1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
>>>>> index d63a5f62fb..8dcfffedbe 100644
>>>>> --- a/include/configs/sifive-unmatched.h
>>>>> +++ b/include/configs/sifive-unmatched.h
>>>>> @@ -67,6 +67,7 @@
>>>>>      "scriptaddr=0x88100000\0" \
>>>>>      "pxefile_addr_r=0x88200000\0" \
>>>>>      "ramdisk_addr_r=0x88300000\0" \
>>>>> +   "initrd_high=0xffffffffffffffff\0" \
>>>>>      "kernel_comp_addr_r=0x90000000\0" \
>>>>
>>>> Tom remarked that kernel_comp_addr_r is poorly chosen as it will overlap
>>>> with initrd if initrd exceeds 125 MiB. Current RISC-V Linux thinks
>>>> initrd should fit into the first 128 MiB of RAM but that is an
>>>> unnecessary restriction in Linux.
>>
>> While I don't expect this to be a major issue we should definitely fix it.
>> When I originally picked the value I wasn't thinking about it and
>> initrd images were small.
>
> Yeah, honestly, I think it should be a high priority thing to fix /
> document.  Distro initrds tend to not be small and these overlaps end up
> being a huge problem for end users.  A good and documented example of
> where to put everything relative to start of DDR so that nothing should
> overlap by default makes things so much easier for everyone.
>
There is no fits all solution. Boards come with different reserved
areas. I even ran into a problem were you simply could not fit the
intitrd where Linux was expecting it because there was so much reserved
memory in the low 128 MiB. So Linux' EFI stub needed patching.

Best regards

Heinrich

Re: [RFC 1/1] board: sifive: unmatched: use zero copy for initrd

[Tom Rini]

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

On Tue, Jul 20, 2021 at 04:50:57PM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 7/20/21 4:06 PM, Tom Rini wrote:
> > On Tue, Jul 20, 2021 at 04:46:23PM +0300, David Abdurachmanov wrote:
> > > On Tue, Jul 20, 2021 at 1:06 AM Tom Rini <trini@konsulko.com> wrote:
> > > > 
> > > > On Mon, Jul 19, 2021 at 11:52:09PM +0200, Heinrich Schuchardt wrote:
> > > > > 
> > > > > 
> > > > > On 19.07.21 23:38, 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.
> > > > > > 
> > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > > ---
> > > > > > Generally copying to another memory location than $ramdisk_addr_r provides
> > > > > > no benefit whatsoever.
> > > > > > 
> > > > > > But we still should find out why the initrd relocation fails so badly.
> > > > > > ---
> > > > > >    include/configs/sifive-unmatched.h | 1 +
> > > > > >    1 file changed, 1 insertion(+)
> > > > > > 
> > > > > > diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> > > > > > index d63a5f62fb..8dcfffedbe 100644
> > > > > > --- a/include/configs/sifive-unmatched.h
> > > > > > +++ b/include/configs/sifive-unmatched.h
> > > > > > @@ -67,6 +67,7 @@
> > > > > >      "scriptaddr=0x88100000\0" \
> > > > > >      "pxefile_addr_r=0x88200000\0" \
> > > > > >      "ramdisk_addr_r=0x88300000\0" \
> > > > > > +   "initrd_high=0xffffffffffffffff\0" \
> > > > > >      "kernel_comp_addr_r=0x90000000\0" \
> > > > > 
> > > > > Tom remarked that kernel_comp_addr_r is poorly chosen as it will overlap
> > > > > with initrd if initrd exceeds 125 MiB. Current RISC-V Linux thinks
> > > > > initrd should fit into the first 128 MiB of RAM but that is an
> > > > > unnecessary restriction in Linux.
> > > 
> > > While I don't expect this to be a major issue we should definitely fix it.
> > > When I originally picked the value I wasn't thinking about it and
> > > initrd images were small.
> > 
> > Yeah, honestly, I think it should be a high priority thing to fix /
> > document.  Distro initrds tend to not be small and these overlaps end up
> > being a huge problem for end users.  A good and documented example of
> > where to put everything relative to start of DDR so that nothing should
> > overlap by default makes things so much easier for everyone.
> > 
> There is no fits all solution. Boards come with different reserved
> areas. I even ran into a problem were you simply could not fit the
> intitrd where Linux was expecting it because there was so much reserved
> memory in the low 128 MiB. So Linux' EFI stub needed patching.

There's certainly "fits most".  And documenting restrictions /
requirements is important, especially if that's not a bug you're talking
about in that specific case.  For example, wait, the ramdisk needs to be
in the lower 128MiB of memory only for what platforms?

-- 
Tom

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