Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 03/10] raven: move BIOS loading from board code to PCI host

[Hervé Poussineau]

Andreas Färber a écrit :
> Am 23.12.2013 19:13, schrieb Hervé Poussineau:
> > Alexander Graf a écrit :
> > > On 23.12.2013, at 07:48, Hervé Poussineau <address@hidden> wrote:
> > >
> > > > Hi,
> > > >
> > > > Andreas Färber a écrit :
> > > > > Hi,
> > > > > Am 05.11.2013 00:09, schrieb Hervé Poussineau:
> > > > > > Raven datasheet explains where firmware lives in system memory, so do
> > > > > > it there instead of in board code. Other boards using the same PCI
> > > > > > host will not have to copy the firmware loading code.
> > > > > This part we had discussed and no one objected to the approach, so OK.
> > > > > > However, add a specific hack for Open Hack'Ware, which provides only
> > > > > > a 512KB blob to be loaded at 0xfff00000, but expects valid code at
> > > > > > 0xfffffffc (specific Open Hack'Ware reset instruction pointer).
> > > > > Was this part explained before? I don't spot the equivalent in the
> > > > > deleted code. If this is a new workaround, I would rather like to
> > > > > put it
> > > > > in a separate patch for bisecting (can offer to do that myself then).
> > > > > What are the symptoms? I am testing all these patches with OHW.
> > > > Old code does (error checking removed):
> > > > > > -            bios_size = get_image_size(filename);
> > > > > > -                bios_addr = (uint32_t)(-bios_size);
> > > > > > -                bios_size = load_image_targphys(filename, bios_addr,
> > > > Ie, bios_addr = -512KB (size of OHW blob) = 0xfff80000
> > > > and firmware is loaded in the range 0xfff80000-0xffffffff
> > > > OHW expects reset instruction pointer to be 0xfffffffc (not valid for
> > > > 604, but that's not the point now), which contains a valid instruction.
> > > > Note that range 0xfff00000-0xfff7ffff is empty.
> > > >
> > > > Datasheet for raven says that firmware is at 0xfff00000, so I changed
> > > > code to:
> > > > +#define BIOS_SIZE (1024 * 1024)
> > > > +                  bios_addr = (uint32_t)(-BIOS_SIZE);
> > > > +                  bios_size = load_image_targphys(filename, bios_addr,
> > > > +                                                  bios_size);
> > > > Ie, bios_addr = -1MB = 0xfff00000
> > > > and firmware is loaded in the range 0xfff00000-0xfff7ffff.
> > > > This doesn't work due to reset instruction pointer which now is
> > > > pointing to empty memory, and symptoms are an empty screen on OHW.
> > > >
> > > > So, I'm adding this hack for OHW, to mirror the 0xfff00000-0xfff7ffff
> > > > range to 0xfff80000-0xffffffff.
> > > >
> > > > So, this patch is a small functional change, as it adds a copy of the
> > > > firmware in a new range 0xfff00000-0xfff7ffff, but I think we can
> > > > live with it.
> > > >
> > > > We'll be able to remove it once we switch to another firmware which
> > > > uses the right reset instruction pointer or whose size is 1MB.
> > > Couldn't we just make the ROM fill the upper part of the 1MB region
> > > when we see it's smaller than 1MB? So that we pad at the bottom, not
> > > the top?
> > >
> > >   bios_size = get_image_size(filename);
> > >   if (bios_size < 0) {
> > >     // error handling
> > >   }
> > >   assert(bios_size <= (1*MB));
> > >   bios_addr = (uint32_t)(-bios_size);
> > I don't think that's a good idea, because the PReP cpus (601/604) have a
> > reset vector at 0xfff00100. So you have to put some firmware at this
> > address, even if firmware is smaller than 1MB.
> >
> > OHW is the problem here, because it is less than 1MB and expects a reset
> > vector at 0xfffffffc. That's why I want to put the hack outside raven
> > chipset, in prep machine code.
>
> Let me clarify then that it was me who disabled some checks that used to
> assure that the loaded binary is in fact 1MB:
> http://git.qemu.org/?p=qemu.git;a=commit;h=74145374bfc0b7b02415184606236f0390479deb
>
> So the issue is actually that the OHW binary is really messed up.
> And me, Hervé and Mark have been working on getting OpenBIOS working for
> PReP in its place.
>
> So I'm currently considering the following options:
>
> 1)
> Revert OHW alias and size/position change
> Strip ELF loading and elf-machine
> Add back Raven ELF support separately
>
> 2)
> Apply my prep.c ELF support patch first
> Apply this patch as pure loading-logic movement
>
> 3)
> Leave broken OHW loading in prep.c
> Only implement 1MB / ELF loading support in Raven
>
> 4)
> Accept a 1MB OHW image and drop support for 512KB OHW
>
> 5)
> Move only MemoryRegion into Raven PHB
> Leave loading code in prep.c but move into function for reuse
> -> avoids ELF machine property
>
> Opinions?

Or maybe:
6) Add the bios loading in Raven like done on this patch (only 1M / ELF 
loading support), which won't currently be used as long as 
raven.bios-size is not set,
and keep the broken code in prep.c, which will be removed when switching 
to OpenBIOS ?

Hervé

> Another issue that came to my attention is that surely the MemoryRegion
> and firmware-loading code should live in the SysBusDevice and not in the
> PCIDevice? Also some instance_init vs. realize separation would seem
> possible.
>
> Regards,
> Andreas