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

[Alistair Francis]

On Tue, Oct 13, 2020 at 3:11 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
>
> > On Mon, Oct 12, 2020 at 05:02:57PM +0100, Alex Bennée wrote:
> >>
> >> Alistair Francis <alistair23@gmail.com> writes:
> >>
> >> > 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
> <snip>
> >> >> "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 ...
> >>
> >> Well it comes down to how much of a special case this is? Pretty much
> >> all FDT (and ACPI for the matter) is basically down to the board level
> >> models - and not all of them just the funky configurable ones. For other
> >> board models we just expect the user to pass the FDT they got with their
> >> kernel blob.
> >>
> >> For modern VirtIO systems the only thing you really need to expose is
> >> the root PCI bus because the rest of the devices are discover-able
> >> there.
> >>
> >> So the real question is are there any other -devices that we want to be
> >> able to graft FDT entries on or is the generic loader enough of a
> >> special case that we keep all the logic in there?
> >>
> >
> > Hi,
> >
> > Another option is to allow the user to pass along a DTB overlay with the
> > generic loader option (or with a separate option as Alistair suggested).
> > With overlways we wouldn't need all the command-line options to enable
> > construction of dtb fragments, it would be more or less transparent to
> > QEMU. There may be limitations with the overlay flows that I'm not aware
> > of though...
>
> So the problem of adding DTB overlays is it's not that much easier than
> the current options of using -machine dumpdtb and then hand hacking the
> magic values and rebuilding, for example:

I agree with this. If a user is changing a DTB from a command line
they probably only want small changes and are unlikely to need the
full power of an overlay.

>
>   https://medium.com/@denisobrezkov/xen-on-arm-and-qemu-1654f24dea75
>
> Unless we come up with some sort of template support that allows QEMU to
> insert numbers like address and size while processing the template. But
> that seems a little too over engineered and likely introduces complexity
> into the system.

This though sounds interesting :)

>
> Given the generic-loader is so simple I'm leaning towards another
> approach of just c&p'ing generic-loader into a new "magic" device (maybe
> guest-loader) and stripping out the bits we don't need (data, data-len,
> be etc) and making the options more tuned what we are trying to achieve.
> For example:
>
>   -device guest-loader,kernel=path/to/Image,args="command 
> line",addr=0x47000000,hyp=xen
>   -device guest-loader,initrd=path/to/initrd,addr=0x42000000,hyp=xen

At first I'm not thrilled of adding a new loader. In saying that,
there are lots of times where -kernel and friends doesn't do what I
want it to do and I have to fall back to the generic loader and code
changes to QEMU so maybe this is a good idea for image loading.

I'm guessing this would be the same for every platform which would be
a nice change from -kernel.

Alistair

>
> And then we can embed the smarts in the loader to set either DTB or ACPI
> entries as required and if we need additional magic to support different
> hypervisors (which hopefully you don't but...) you can modulate the
> hyp=FOO variable.
>
> There may be an argument for having a -hypervisor as a sugar alias for
> -kernel (which maps to machine.kernel) but currently I see no practical
> differences you need to launch it except maybe forcing the
> virtualisation property to true if it exists - but that seems a little
> ARM focused.
>
> --
> Alex Bennée