[Qemu-devel] [PATCH] hw/arm/boot: Increase compliance with kernel arm64 boot protocol.

[Qemu-devel] [PATCH] hw/arm/boot: Increase compliance with kernel arm64 boot protocol.

[Stewart Hildebrand]

"The Image must be placed text_offset bytes from a 2MB aligned base
address anywhere in usable system RAM and called there."

For the virt board, we write our startup bootloader at the very
bottom of RAM, so that bit can't be used for the image. To avoid
overlap in case the image requests to be loaded at an offset
smaller than our bootloader, we increment the load offset to the
next 2MB.

This fixes a boot failure for Xen AArch64.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
---
 hw/arm/boot.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 20c71d7d96..559ddbcd53 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -919,6 +919,16 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
         memcpy(&hdrvals, buffer + ARM64_TEXT_OFFSET_OFFSET, sizeof(hdrvals));
         if (hdrvals[1] != 0) {
             kernel_load_offset = le64_to_cpu(hdrvals[0]);
+
+            /* For the virt board, we write our startup "bootloader" at the very
+             * bottom of RAM, so that bit can't be used for the image. To avoid
+             * overlap in case the image requests to be loaded at an offset
+             * smaller than our bootloader, we increment the load offset to the
+             * next 2MB.
+             */
+            if (kernel_load_offset < FIXUP_MAX) {
+                kernel_load_offset += 2 << 20;
+            }
         }
     }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH] hw/arm/boot: Increase compliance with kernel arm64 boot protocol.

[Philippe Mathieu-Daudé]

Hi Stewart,

On 15/10/2018 23:26, Stewart Hildebrand wrote:
> "The Image must be placed text_offset bytes from a 2MB aligned base
> address anywhere in usable system RAM and called there."
> 
> For the virt board, we write our startup bootloader at the very
> bottom of RAM, so that bit can't be used for the image. To avoid
> overlap in case the image requests to be loaded at an offset
> smaller than our bootloader, we increment the load offset to the
> next 2MB.
> 
> This fixes a boot failure for Xen AArch64.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> ---
>  hw/arm/boot.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 20c71d7d96..559ddbcd53 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -919,6 +919,16 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
>          memcpy(&hdrvals, buffer + ARM64_TEXT_OFFSET_OFFSET, sizeof(hdrvals));
>          if (hdrvals[1] != 0) {
>              kernel_load_offset = le64_to_cpu(hdrvals[0]);
> +
> +            /* For the virt board, we write our startup "bootloader" at the very
> +             * bottom of RAM, so that bit can't be used for the image. To avoid
> +             * overlap in case the image requests to be loaded at an offset
> +             * smaller than our bootloader, we increment the load offset to the
> +             * next 2MB.
> +             */
> +            if (kernel_load_offset < FIXUP_MAX) {

I don't understand how this is related to FIXUP_MAX...

> +                kernel_load_offset += 2 << 20;

You can use += 2 * MiB; which is easier to review.

> +            }
>          }
>      }
>  
> 

Re: [Qemu-devel] [PATCH] hw/arm/boot: Increase compliance with kernel arm64 boot protocol.

[Stewart Hildebrand]

Hi Philippe,

On Monday, October 15, 2018 6:05 PM, Philippe Mathieu-Daudé wrote:
> Hi Stewart,
> 
> On 15/10/2018 23:26, Stewart Hildebrand wrote:
> > +            /* For the virt board, we write our startup "bootloader" at the very
> > +             * bottom of RAM, so that bit can't be used for the image. To avoid
> > +             * overlap in case the image requests to be loaded at an offset
> > +             * smaller than our bootloader, we increment the load offset to the
> > +             * next 2MB.
> > +             */
> > +            if (kernel_load_offset < FIXUP_MAX) {
> 
> I don't understand how this is related to FIXUP_MAX...

You're right, my apologies, it's not directly related.
write_bootloader() calculates the size of the bootloader like so:
    len = 0;
    while (insns[len].fixup != FIXUP_TERMINATOR) {
        len++;
    }

The size of the bootloader then would be len * sizeof(uint32_t)

It would be nice not to have to repeat that logic in load_aarch64_image(). I'll send out a v2 after I take some time to wrap my head around it...
 
> 
> > +                kernel_load_offset += 2 << 20;
> 
> You can use += 2 * MiB; which is easier to review.

OK, I will include this in v2.

Re: [Qemu-devel] [PATCH] hw/arm/boot: Increase compliance with kernel arm64 boot protocol.

[Andre Przywara]

On Tue, 16 Oct 2018 01:19:35 +0000
Stewart Hildebrand <Stewart.Hildebrand@dornerworks.com> wrote:

Hi,

Stewart, thanks a lot for picking this up!

> On Monday, October 15, 2018 6:05 PM, Philippe Mathieu-Daudé wrote:
> > Hi Stewart,
> > 
> > On 15/10/2018 23:26, Stewart Hildebrand wrote:  
> > > +            /* For the virt board, we write our startup
> > > "bootloader" at the very
> > > +             * bottom of RAM, so that bit can't be used for the
> > > image. To avoid
> > > +             * overlap in case the image requests to be loaded
> > > at an offset
> > > +             * smaller than our bootloader, we increment the
> > > load offset to the
> > > +             * next 2MB.
> > > +             */
> > > +            if (kernel_load_offset < FIXUP_MAX) {  
> > 
> > I don't understand how this is related to FIXUP_MAX...  
> 
> You're right, my apologies, it's not directly related.
> write_bootloader() calculates the size of the bootloader like so:
>     len = 0;
>     while (insns[len].fixup != FIXUP_TERMINATOR) {
>         len++;
>     }
> 
> The size of the bootloader then would be len * sizeof(uint32_t)
> 
> It would be nice not to have to repeat that logic in
> load_aarch64_image(). I'll send out a v2 after I take some time to
> wrap my head around it... 

I wonder if this could be done much easier, since TEXT_OFFSET must
actually be page aligned. So all we would need is an *upper bound* for
the bootloader size, as long as that is below 4K we wouldn't even loose
anything.

Cheers,
Andre.

> >   
> > > +                kernel_load_offset += 2 << 20;  
> > 
> > You can use += 2 * MiB; which is easier to review.  
> 
> OK, I will include this in v2.

Re: [Qemu-devel] [PATCH] hw/arm/boot: Increase compliance with kernel arm64 boot protocol.

[Peter Maydell]

On 16 October 2018 at 11:02, Andre Przywara <andre.przywara@arm.com> wrote:
> On Tue, 16 Oct 2018 01:19:35 +0000
> Stewart Hildebrand <Stewart.Hildebrand@dornerworks.com> wrote:
>
> Hi,
>
> Stewart, thanks a lot for picking this up!
>
>> On Monday, October 15, 2018 6:05 PM, Philippe Mathieu-Daudé wrote:
>> > Hi Stewart,
>> >
>> > On 15/10/2018 23:26, Stewart Hildebrand wrote:
>> > > +            /* For the virt board, we write our startup
>> > > "bootloader" at the very
>> > > +             * bottom of RAM, so that bit can't be used for the
>> > > image. To avoid
>> > > +             * overlap in case the image requests to be loaded
>> > > at an offset
>> > > +             * smaller than our bootloader, we increment the
>> > > load offset to the
>> > > +             * next 2MB.
>> > > +             */
>> > > +            if (kernel_load_offset < FIXUP_MAX) {
>> >
>> > I don't understand how this is related to FIXUP_MAX...
>>
>> You're right, my apologies, it's not directly related.
>> write_bootloader() calculates the size of the bootloader like so:
>>     len = 0;
>>     while (insns[len].fixup != FIXUP_TERMINATOR) {
>>         len++;
>>     }
>>
>> The size of the bootloader then would be len * sizeof(uint32_t)
>>
>> It would be nice not to have to repeat that logic in
>> load_aarch64_image(). I'll send out a v2 after I take some time to
>> wrap my head around it...
>
> I wonder if this could be done much easier, since TEXT_OFFSET must
> actually be page aligned. So all we would need is an *upper bound* for
> the bootloader size, as long as that is below 4K we wouldn't even loose
> anything.

Yes, I think it would be fine to just set a max bootloader size of
4K and check against that. (If so, assert() in write_bootloader
that we did stay below the max.)

thanks
-- PMM