From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60503) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VvDSw-0000XN-36 for qemu-devel@nongnu.org; Mon, 23 Dec 2013 16:55:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VvDSr-0004Cr-F2 for qemu-devel@nongnu.org; Mon, 23 Dec 2013 16:55:10 -0500 Message-ID: <52B8B12F.5000104@reactos.org> Date: Mon, 23 Dec 2013 22:54:55 +0100 From: =?ISO-8859-1?Q?Herv=E9_Poussineau?= MIME-Version: 1.0 References: <1383606592-12783-1-git-send-email-hpoussin@reactos.org> <1383606592-12783-4-git-send-email-hpoussin@reactos.org> <52B78C3D.4030703@web.de> <52B7DCC9.7060004@reactos.org> <17FC51C8-FDB4-46D0-8512-A1C626D14054@suse.de> <52B87D3D.8060806@reactos.org> <52B896E8.60903@web.de> In-Reply-To: <52B896E8.60903@web.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 03/10] raven: move BIOS loading from board code to PCI host List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Andreas_F=E4rber?= Cc: Peter Maydell , Mark Cave-Ayland , qemu-ppc , Alexander Graf , QEMU Developers Andreas F=E4rber a =E9crit : > Am 23.12.2013 19:13, schrieb Herv=E9 Poussineau: >> Alexander Graf a =E9crit : >>> On 23.12.2013, at 07:48, Herv=E9 Poussineau wr= ote: >>> >>>> Hi, >>>> >>>> Andreas F=E4rber a =E9crit : >>>>> Hi, >>>>> Am 05.11.2013 00:09, schrieb Herv=E9 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 on= ly >>>>>> 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 =3D get_image_size(filename); >>>>>> - bios_addr =3D (uint32_t)(-bios_size); >>>>>> - bios_size =3D load_image_targphys(filename, bios_= addr, >>>> Ie, bios_addr =3D -512KB (size of OHW blob) =3D 0xfff80000 >>>> and firmware is loaded in the range 0xfff80000-0xffffffff >>>> OHW expects reset instruction pointer to be 0xfffffffc (not valid fo= r >>>> 604, but that's not the point now), which contains a valid instructi= on. >>>> Note that range 0xfff00000-0xfff7ffff is empty. >>>> >>>> Datasheet for raven says that firmware is at 0xfff00000, so I change= d >>>> code to: >>>> +#define BIOS_SIZE (1024 * 1024) >>>> + bios_addr =3D (uint32_t)(-BIOS_SIZE); >>>> + bios_size =3D load_image_targphys(filename, bios_= addr, >>>> + bios_size); >>>> Ie, bios_addr =3D -1MB =3D 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-0xfff7fff= f >>>> range to 0xfff80000-0xffffffff. >>>> >>>> So, this patch is a small functional change, as it adds a copy of th= e >>>> 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 =3D get_image_size(filename); >>> if (bios_size < 0) { >>> // error handling >>> } >>> assert(bios_size <=3D (1*MB)); >>> bios_addr =3D (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 res= et >> vector at 0xfffffffc. That's why I want to put the hack outside raven >> chipset, in prep machine code. >=20 > Let me clarify then that it was me who disabled some checks that used t= o > assure that the loaded binary is in fact 1MB: > http://git.qemu.org/?p=3Dqemu.git;a=3Dcommit;h=3D74145374bfc0b7b0241518= 4606236f0390479deb >=20 > So the issue is actually that the OHW binary is really messed up. > And me, Herv=E9 and Mark have been working on getting OpenBIOS working = for > PReP in its place. >=20 > So I'm currently considering the following options: >=20 > 1) > Revert OHW alias and size/position change > Strip ELF loading and elf-machine > Add back Raven ELF support separately >=20 > 2) > Apply my prep.c ELF support patch first > Apply this patch as pure loading-logic movement >=20 > 3) > Leave broken OHW loading in prep.c > Only implement 1MB / ELF loading support in Raven >=20 > 4) > Accept a 1MB OHW image and drop support for 512KB OHW >=20 > 5) > Move only MemoryRegion into Raven PHB > Leave loading code in prep.c but move into function for reuse > -> avoids ELF machine property >=20 > Opinions? Or maybe: 6) Add the bios loading in Raven like done on this patch (only 1M / ELF=20 loading support), which won't currently be used as long as=20 raven.bios-size is not set, and keep the broken code in prep.c, which will be removed when switching=20 to OpenBIOS ? Herv=E9 >=20 > Another issue that came to my attention is that surely the MemoryRegion > and firmware-loading code should live in the SysBusDevice and not in th= e > PCIDevice? Also some instance_init vs. realize separation would seem > possible. >=20 > Regards, > Andreas >=20