[Qemu-devel] [PATCH] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Qemu-devel] [PATCH] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Ard Biesheuvel]

If we are running the 'virt' machine, we may have a device tree blob but no
kernel to supply it to if no -kernel option was passed. In that case, copy it
to the base of DRAM where it can be picked up by a bootloader executing from
NOR flash.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/boot.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 3d1f4a255b48..ff6a727613aa 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -455,6 +455,14 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 
     /* Load the kernel.  */
     if (!info->kernel_filename) {
+        /*
+         * If we have a device tree blob, but no kernel to supply it to, copy it
+         * to the base of DRAM for a bootloader executing from NOR flash to
+         * pick up.
+         */
+        if (have_dtb(info))
+            load_dtb(info->loader_start, info);
+
         /* If no kernel specified, do nothing; we will start from address 0
          * (typically a boot ROM image) in the same way as hardware.
          */
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Peter Maydell]

On 26 August 2014 16:31, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> If we are running the 'virt' machine, we may have a device tree blob but no
> kernel to supply it to if no -kernel option was passed. In that case, copy it
> to the base of DRAM where it can be picked up by a bootloader executing from
> NOR flash.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

In general I like this approach for providing a BIOS/bootloader
with information about the platform it's running on.
(For the benefit of readers who may be missing context, this
bootloader is likely to be UEFI.) Since we already have DTB for
telling the guest about the shape of the platform this makes
more sense to me than having a separate fw_cfg style
interface to repeat the same information.

I should dig out the NOR-flash-in-virt patches and get them
through review/commit so this code has a more immediately
obvious purpose.

A couple of style nits below.

> ---
>  hw/arm/boot.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 3d1f4a255b48..ff6a727613aa 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -455,6 +455,14 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>
>      /* Load the kernel.  */
>      if (!info->kernel_filename) {
> +        /*
> +         * If we have a device tree blob, but no kernel to supply it to, copy it
> +         * to the base of DRAM for a bootloader executing from NOR flash to
> +         * pick up.
> +         */
> +        if (have_dtb(info))
> +            load_dtb(info->loader_start, info);

Our coding style demands braces even on single-statement
if()s. Also, we should check the return value from load_dtb()
and exit(1) on failure (compare the existing callsite).

> +
>          /* If no kernel specified, do nothing; we will start from address 0
>           * (typically a boot ROM image) in the same way as hardware.
>           */

A style nit so minuscule I wouldn't point it out if you weren't
going to reroll this patch anyway: notice how this comment
differs from yours slightly in style...

> --
> 1.8.3.2

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Ard Biesheuvel]

On 1 September 2014 19:36, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 26 August 2014 16:31, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> If we are running the 'virt' machine, we may have a device tree blob but no
>> kernel to supply it to if no -kernel option was passed. In that case, copy it
>> to the base of DRAM where it can be picked up by a bootloader executing from
>> NOR flash.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> In general I like this approach for providing a BIOS/bootloader
> with information about the platform it's running on.
> (For the benefit of readers who may be missing context, this
> bootloader is likely to be UEFI.) Since we already have DTB for
> telling the guest about the shape of the platform this makes
> more sense to me than having a separate fw_cfg style
> interface to repeat the same information.
>

I agree. But perhaps we should address the reset issue we discussed
over IRC last Friday?
Currently, reset does not work at all when using -bios (and your
patch), but we should fix that in a sane way, i.e., the DTB should be
reloaded into DRAM, and this patch currently does not cover that.

> I should dig out the NOR-flash-in-virt patches and get them
> through review/commit so this code has a more immediately
> obvious purpose.
>
> A couple of style nits below.
>
>> ---
>>  hw/arm/boot.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 3d1f4a255b48..ff6a727613aa 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -455,6 +455,14 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>
>>      /* Load the kernel.  */
>>      if (!info->kernel_filename) {
>> +        /*
>> +         * If we have a device tree blob, but no kernel to supply it to, copy it
>> +         * to the base of DRAM for a bootloader executing from NOR flash to
>> +         * pick up.
>> +         */
>> +        if (have_dtb(info))
>> +            load_dtb(info->loader_start, info);
>
> Our coding style demands braces even on single-statement
> if()s. Also, we should check the return value from load_dtb()
> and exit(1) on failure (compare the existing callsite).
>

OK

>> +
>>          /* If no kernel specified, do nothing; we will start from address 0
>>           * (typically a boot ROM image) in the same way as hardware.
>>           */
>
> A style nit so minuscule I wouldn't point it out if you weren't
> going to reroll this patch anyway: notice how this comment
> differs from yours slightly in style...
>

OK

>> --
>> 1.8.3.2
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Peter Maydell]

On 1 September 2014 18:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 1 September 2014 19:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 26 August 2014 16:31, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> If we are running the 'virt' machine, we may have a device tree blob but no
>>> kernel to supply it to if no -kernel option was passed. In that case, copy it
>>> to the base of DRAM where it can be picked up by a bootloader executing from
>>> NOR flash.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> In general I like this approach for providing a BIOS/bootloader
>> with information about the platform it's running on.
>> (For the benefit of readers who may be missing context, this
>> bootloader is likely to be UEFI.) Since we already have DTB for
>> telling the guest about the shape of the platform this makes
>> more sense to me than having a separate fw_cfg style
>> interface to repeat the same information.
>>
>
> I agree. But perhaps we should address the reset issue we discussed
> over IRC last Friday?

Also true; I thought about mentioning those but decided they
were orthogonal. We should probably pull together a list
of all the UEFI related QEMU patches and required work.

> Currently, reset does not work at all when using -bios (and your
> patch), but we should fix that in a sane way, i.e., the DTB should be
> reloaded into DRAM, and this patch currently does not cover that.

Yep. That's also a bug with the -kernel support, though:
currently we rely on the guest kernel never writing over the
dtb we pass it since we don't reinstate it on reset...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Ard Biesheuvel]

On 1 September 2014 19:50, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 September 2014 18:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 1 September 2014 19:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 26 August 2014 16:31, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>> If we are running the 'virt' machine, we may have a device tree blob but no
>>>> kernel to supply it to if no -kernel option was passed. In that case, copy it
>>>> to the base of DRAM where it can be picked up by a bootloader executing from
>>>> NOR flash.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>
>>> In general I like this approach for providing a BIOS/bootloader
>>> with information about the platform it's running on.
>>> (For the benefit of readers who may be missing context, this
>>> bootloader is likely to be UEFI.) Since we already have DTB for
>>> telling the guest about the shape of the platform this makes
>>> more sense to me than having a separate fw_cfg style
>>> interface to repeat the same information.
>>>
>>
>> I agree. But perhaps we should address the reset issue we discussed
>> over IRC last Friday?
>
> Also true; I thought about mentioning those but decided they
> were orthogonal. We should probably pull together a list
> of all the UEFI related QEMU patches and required work.
>

By orthogonal, do you mean this bit still belongs in
arm_load_kernel(), and the reset handling should be adapted to call
arm_load_kernel() again in its entirety?

>> Currently, reset does not work at all when using -bios (and your
>> patch), but we should fix that in a sane way, i.e., the DTB should be
>> reloaded into DRAM, and this patch currently does not cover that.
>
> Yep. That's also a bug with the -kernel support, though:
> currently we rely on the guest kernel never writing over the
> dtb we pass it since we don't reinstate it on reset...
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Peter Maydell]

On 1 September 2014 19:04, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 1 September 2014 19:50, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Also true; I thought about mentioning those but decided they
>> were orthogonal. We should probably pull together a list
>> of all the UEFI related QEMU patches and required work.

> By orthogonal, do you mean this bit still belongs in
> arm_load_kernel(), and the reset handling should be adapted to call
> arm_load_kernel() again in its entirety?

I mean I think this bit is fine, that load_dtb() should
put the DTB in a rom-blob, and that then there's no
need to do anything on reset because the rom-blob's
reset handling does it for us. (As I say, this also gets
us correct handling of the DTB on reset in the -kernel
case too.)

-- PMM

[Qemu-devel] [PATCH] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Ard Biesheuvel]

If we are running the 'virt' machine, we may have a device tree blob but no
kernel to supply it to if no -kernel option was passed. In that case, copy it
to the base of DRAM where it can be picked up by a bootloader executing from
NOR flash.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
v2: comment style, call exit(1) on load_dtb() failure

 hw/arm/boot.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index e32f2f415885..a32ff28d9be1 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -459,6 +459,17 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 
     /* Load the kernel.  */
     if (!info->kernel_filename) {
+
+        if (have_dtb(info)) {
+            /* If we have a device tree blob, but no kernel to supply it to,
+             * copy it to the base of DRAM for a bootloader executing from
+             * NOR flash to pick up.
+             */
+            if (load_dtb(info->loader_start, info)) {
+                exit(1);
+            }
+        }
+
         /* If no kernel specified, do nothing; we will start from address 0
          * (typically a boot ROM image) in the same way as hardware.
          */
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Peter Crosthwaite]

On Wed, Sep 3, 2014 at 10:01 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> If we are running the 'virt' machine, we may have a device tree blob but no
> kernel to supply it to if no -kernel option was passed. In that case, copy it
> to the base of DRAM where it can be picked up by a bootloader executing from
> NOR flash.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: comment style, call exit(1) on load_dtb() failure

On respins, please add "Vx" to git subject prefix.

git fomat-patch --subject-prefix "PATCH v2"

>
>  hw/arm/boot.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index e32f2f415885..a32ff28d9be1 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -459,6 +459,17 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>
>      /* Load the kernel.  */
>      if (!info->kernel_filename) {
> +
> +        if (have_dtb(info)) {
> +            /* If we have a device tree blob, but no kernel to supply it to,
> +             * copy it to the base of DRAM for a bootloader executing from
> +             * NOR flash to pick up.

arm/boot is generic code so you shouldn't mention the specifics of
DRAM/NOR implementation. s/DRAM/RAM and can remove "executing from NOR
flash" completely.

> +             */
> +            if (load_dtb(info->loader_start, info)) {
> +                exit(1);
> +            }
> +        }
> +

Can we get this functionality in the elf loading path too? With this
patch, you now get:

1: -kernel Linux : dtb
2: -kernel elf : no dtb
3: nothing : dtb

It would be nice and consistent if you always got a dtb. If the elf
sections overlap the dts, then the elf should take precedence. Not
sure if the ROM loading framework supports this natively just yet
though.

This would allow for easier debugging of your bootloader as an elf
guest, rather that having to do build cycles all the way down to
firmware blob image.

Regards,
Peter

>          /* If no kernel specified, do nothing; we will start from address 0
>           * (typically a boot ROM image) in the same way as hardware.
>           */
> --
> 1.8.3.2
>
>