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

[Andreas Färber]

Am 24.12.2013 01:32, schrieb 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, are you volunteering to build one? You wanted to look into that
with me since a looong time ago... :)

http://repo.or.cz/w/openhackware.git

As you will remember, Jocelyn fixed an IDE issue binary-only, without
updating pc-bios/ohw.diff:
http://git.qemu.org/?p=qemu.git;a=commit;h=55aa45ddde3283cdd781326d001f7456bf02f684

I dug out the patch René Rebe proposed later for fixing that IDE issue:
http://lists.gnu.org/archive/html/qemu-devel/2008-05/msg00223.html

I've just managed to fix up that patch to finally apply (apart from line
wraps in a patch to a patch - gosh! - there was also an "address@hidden"
from the archive hidden in the patch context), attached, but haven't yet
re-tried to build with it. It includes a linker script fix that might
explain my previous build issues.

Andreas

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

P.S. That's my 3). :)

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

0001-fix-PPC-OpenHackWare-for-Linux-2.6.patch
Description: Text Data