Re: [PATCH qemu v5] spapr: Kill SLOF

[David Gibson]

On Wed, Jan 22, 2020 at 06:14:37PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 22/01/2020 17:32, David Gibson wrote:
> > On Tue, Jan 21, 2020 at 06:25:36PM +1100, Alexey Kardashevskiy wrote:
> >>
> >>
> >> On 21/01/2020 16:11, David Gibson wrote:
> >>> On Fri, Jan 10, 2020 at 01:09:25PM +1100, Alexey Kardashevskiy wrote:
> >>>> The Petitboot bootloader is way more advanced than SLOF is ever going to
> >>>> be as Petitboot comes with the full-featured Linux kernel with all
> >>>> the drivers, and initramdisk with quite user friendly interface.
> >>>> The problem with ditching SLOF is that an unmodified pseries kernel can
> >>>> either start via:
> >>>> 1. kexec, this requires presence of RTAS and skips
> >>>> ibm,client-architecture-support entirely;
> >>>> 2. normal boot, this heavily relies on the OF1275 client interface to
> >>>> fetch the device tree and do early setup (claim memory).
> >>>>
> >>>> This adds a new bios-less mode to the pseries machine: "bios=on|off".
> >>>> When enabled, QEMU does not load SLOF and jumps to the kernel from
> >>>> "-kernel".
> >>>
> >>> I don't love the name "bios" for this flag, since BIOS tends to refer
> >>> to old-school x86 firmware.  Given the various plans we're considering
> >>> the future, I'd suggest "firmware=slof" for the current in-guest SLOF
> >>> mode, and say "firmware=vof" (Virtual Open Firmware) for the new
> >>> model.  We can consider firmware=petitboot or firmware=none (for
> >>> direct kexec-style boot into -kernel) or whatever in the future
> >>
> >> Ok. We could also enforce default loading addresses for SLOF/kernel/grub
> >> and drop "kernel-addr", although it is going to be confusing if it
> >> changes in not so obvious way...
> > 
> > Yes, I think that would be confusing, so I think adding the
> > kernel-addr override is a good idea, I'd just like it split out for
> > clarity.
> > 
> >> In fact, I will ideally need 3 flags:
> >> -bios: on|off to stop loading SLOF;
> >> -kernel-addr: 0x0 for slof/kernel; 0x20000 for grub;
> > 
> > I'm happy for that one to be separate from the "firmware style"
> > option.
> > 
> >> -kernel-translate-hack: on|off - as grub is linked to run from 0x20000
> >> and it only works when placed there, the hack breaks it.
> > 
> > Hrm.  I don't really understand what this one is about.  That doesn't
> > really seem like something the user would ever want to fiddle with
> > directly.
> 
> This allows loading grub, or actually any elf (not that I have anything
> else in mind that just grub but still) which is not capable of
> relocating itself.

Ok, why would we ever not want that?

> >> Or we can pass grub via -bios and not via -kernel but strictly speaking
> >> there is still a firmware - that new 20 bytes blob so it would not be
> >> accurate.
> >>
> >> We can put this all into a single
> >> -firmware slof|vof|grub|linux. Not sure.
> > 
> > I'm not thinking of "grub" as a separate option - that would be the
> > same as "vof".  Using vof + no -kernel we'd need to scan the disks in
> > the same way SLOF does, and look for a boot partition, which will
> > probably contain a GRUB image. 
> 
> I was hoping we can avoid that by allowing
> "-kernel grub" and let grub do filesystems and MBR/GPT.

I don't want that to be the only way, because I want the GRUB
installed by the OS installer to be the GRUB we use.

> > Then we'd need enough faked OF client
> > calls to let GRUB operate.
> 
> v6 does very basic block devices. Now I need to learn how to build grub
> properly, it is 32bit and it is not straight forward how to build it
> 100% properly on ppc64 machine, I see occasional issues such as
> uint32->uint64 extension with a garbage in the top 32bits, things like
> this... But it can definitely read MBR/GPT, parse FS, etc.

Ok.

[snip]
> >>>> +/*
> >>>> + * "claim" claims memory at @virt if @align==0; otherwise it allocates
> >>>> + * memory at the requested alignment.
> >>>> + */
> >>>> +uint64_t spapr_do_of_client_claim(SpaprMachineState *spapr, uint64_t 
> >>>> virt,
> >>>> +                                  uint64_t size, uint64_t align)
> >>>> +{
> >>>> +    uint32_t ret;
> >>>> +
> >>>> +    if (align == 0) {
> >>>> +        if (!of_client_claim_avail(spapr->claimed, virt, size)) {
> >>>> +            return -1;
> >>>> +        }
> >>>> +        ret = virt;
> >>>> +    } else {
> >>>> +        align = pow2ceil(align);
> >>>
> >>> Should this be a pow2ceil, or should it just return an error if align
> >>> is not a power of 2. > Note that aligning something to 4 bytes will
> >>> (probably) make it *not* aligned to 3 bytes.
> >>
> >> I did not see any notes about the specific alignment requirements here,
> >> the idea is that clients may just not expect unaligned memory at all; I
> >> could probably just drop it and see what happens...
> > 
> > I don't follow you.  Isn't the align value coming from the client?
> 
> This code is used by the client and QEMU. So for QEMU users, I'll have
> to align myself everywhere, not a huge deal. And since it is an old
> interface which nobody follows 100%, I can imagine clients (grub/yaboot)
> asking to claim with alignments other than power of two in expectation
> that the firmware will align it, may be.
> 
> 
> > 
> >>>> +        spapr->claimed_base = (spapr->claimed_base + align - 1) & 
> >>>> ~(align - 1);
> >>>> +        while (1) {
> >>>> +            if (spapr->claimed_base >= spapr->rma_size) {
> >>>> +                perror("Out of memory");
> >>>
> >>> error_report() or qemu_log() or something and a message with some more
> >>> specificity, please.
> >>
> >>
> >> What kind of specificity is missing here?
> > 
> > That it's on the OF claim interface specifically, and how much they
> > were trying to claim.
> 
> Changing it to
> 
> error_report("Out of RMA memory for the OF client")
> 
> Thanks for the review! I'll post it when we settle on the new bios/vof
> machine parameter.
> 
> 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature