Re: [RFC PATCH 0/4] generic loader FDT support (for direct Xen boot)

[Alistair Francis]

On Fri, Oct 9, 2020 at 10:07 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Hi,
>
> This series adds the ability to append FDT data for blobs loaded by
> the generic loader. My principle use-case was to be able to directly
> boot Xen with a kernel image which avoided having to:
>
>   - get the kernel image into my system image
>   - deal with bugs in FDT munging by -bios firmware and/or grub
>
> as such this currently a developer hack that makes *my* life easy and
> is thus presented as an RFC for discussion. While I've tested it with
> Xen I'm sure the approach would be applicable to other hypervisors or
> firmware which expect to consume FDT data pointing at various blobs.

An interesting idea. I think this comes up enough that it's worth
thinking about.

>
> An example command line that launches this is (magic from -kernel):
>
>   ./aarch64-softmmu/qemu-system-aarch64 -cpu cortex-a57 \
>     -machine type=virt,virtualization=on -display none \
>     -serial mon:stdio \
>     -netdev user,id=unet,hostfwd=tcp::2222-:22 \
>     -device virtio-net-pci,netdev=unet,id=virt-net,disable-legacy=on \
>     -device virtio-scsi-pci,id=virt-scsi,disable-legacy=on \
>     -blockdev 
> driver=raw,node-name=hd,discard=unmap,file.driver=host_device,file.filename=/dev/zen-disk/debian-buster-arm64
>  \
>     -device scsi-hd,drive=hd,id=virt-scsi-hd \
>     -smp 4 -m 4096 \
>     -kernel ~/lsrc/xen.git/xen/xen \
>     -append "dom0_mem=1G,max:1G loglvl=all guest_loglvl=all" \
>     -device 
> loader,addr=0x47000000,file=$HOME/lsrc/linux.git/builds/arm64/arch/arm64/boot/Image,len-fdt-compat=2,fdt-compat[0]='multiboot,,module',fdt-compat[1]='multiboot,,kernel',fdt-bootargs="root=/dev/sda2
>  ro console=hvc0 earlyprintk=xen"

You are loading the kernel `Image` file, but changing the device tree
that was generated by QEMU and is being loaded in memory. As a user
that is really confusing.

>
> So a couple of choices I've made doing this:
>
> Promoting *fdt to MachineState
>
> This seemed the simplest approach to making the fdt available to the
> global state, especially as MachineState already has a *dtb pointer.
> I've converted the ARM virt machine and a RISCV machine but if this is
> acceptable I can convert the others.

This seems fine to me.

>
> "Polluting" the generic loader arguments
>
> This was mainly out of convenience as the loader already knows the
> size of the blob being loaded. However you could certainly argue it
> makes sense to have a more generic "FDT expander" virtual device that
> could for example query the QOM model somehow to find the details it
> needs.

That seems like a better option. Why not have a generic way to modify
the device tree with a specific argument? It could either be -device
loader,file=fdt,... or -fdt ...

Alistair

>
> FDT isn't the only way of passing system information up the boot
> chain. We could reasonably want to do a similar thing with ACPI which
> is what should be being used on SBSA like devices to communicate with
> the hypervisor.
>
> Also relying on ,, in the QemuOpt parser is ugly. It might be worth
> having better quoting support if I press on with this.
>
> So what do people think? Something worth developing?
>
>
> Alex Bennée (4):
>   hw/board: promote fdt from ARM VirtMachineState to MachineState
>   hw/riscv: migrate fdt field to generic MachineState
>   device_tree: add qemu_fdt_setprop_string_array helper
>   generic_loader: allow the insertion of /chosen/module stanzas
>
>  include/hw/arm/virt.h            |   1 -
>  include/hw/boards.h              |   1 +
>  include/hw/core/generic-loader.h |   4 +
>  include/hw/riscv/virt.h          |   1 -
>  include/sysemu/device_tree.h     |  17 ++
>  device_tree.c                    |  26 +++
>  hw/arm/virt.c                    | 322 ++++++++++++++++---------------
>  hw/core/generic-loader.c         |  42 ++++
>  hw/riscv/virt.c                  |  18 +-
>  9 files changed, 268 insertions(+), 164 deletions(-)
>
> --
> 2.20.1
>
>