| Message ID | 1409067062-1729-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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. */
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(+)