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

[Alexander Graf]

On 23.12.2013, at 22:54, Hervé Poussineau <address@hidden> wrote:

> 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

I like this option the best.


Alex

>> 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
>