| Message ID | 1383606592-12783-4-git-send-email-hpoussin@reactos.org |
|---|---|
| State | New |
| Headers | show |
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. Regards, Andreas > > Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> > --- > hw/pci-host/prep.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > hw/ppc/prep.c | 50 +++++++++++++------------------------------------- > 2 files changed, 64 insertions(+), 37 deletions(-) [...] > diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c > index 9f8538c..8a09e2b 100644 > --- a/hw/ppc/prep.c > +++ b/hw/ppc/prep.c [...] > @@ -510,41 +509,13 @@ static void ppc_prep_init(QEMUMachineInitArgs *args) > memory_region_add_subregion(sysmem, 0, ram); > > /* allocate and load BIOS */ > - memory_region_init_ram(bios, NULL, "ppc_prep.bios", BIOS_SIZE); > - memory_region_set_readonly(bios, true); > - memory_region_add_subregion(sysmem, (uint32_t)(-BIOS_SIZE), bios); > - vmstate_register_ram_global(bios); > - if (bios_name == NULL) > - bios_name = BIOS_FILENAME; > - filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > - if (filename) { > - bios_size = load_elf(filename, NULL, NULL, NULL, > - NULL, NULL, 1, ELF_MACHINE, 0); > - if (bios_size < 0) { > - bios_size = get_image_size(filename); > - if (bios_size > 0 && bios_size <= BIOS_SIZE) { > - hwaddr bios_addr; > - bios_size = (bios_size + 0xfff) & ~0xfff; > - bios_addr = (uint32_t)(-bios_size); > - bios_size = load_image_targphys(filename, bios_addr, bios_size); > - } > - if (bios_size > BIOS_SIZE) { > - fprintf(stderr, "qemu: PReP bios '%s' is too large (0x%x)\n", > - bios_name, bios_size); > - exit(1); > - } > - } > - } else { > - bios_size = -1; > - } > - if (bios_size < 0 && !qtest_enabled()) { > - fprintf(stderr, "qemu: could not load PPC PReP bios '%s'\n", > - bios_name); > - exit(1); > - } > - if (filename) { > - g_free(filename); > - } > + /* Open Hack'Ware hack: bios size is 512K and is loaded at 0xfff00000. > + * However, reset address is 0xfffffffc. Mirror the bios from > + * 0xfff00000 to 0xfff80000. > + */ > + memory_region_init_alias(bios, NULL, "bios-alias", sysmem, 0xfff00000, > + 0x00080000); > + memory_region_add_subregion_overlap(sysmem, 0xfff80000, bios, 1); > > if (linux_boot) { > kernel_base = KERNEL_LOAD_ADDR; [snip]
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. Regards, Hervé >> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> >> --- >> hw/pci-host/prep.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/ppc/prep.c | 50 +++++++++++++------------------------------------- >> 2 files changed, 64 insertions(+), 37 deletions(-) > [...] >> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c >> index 9f8538c..8a09e2b 100644 >> --- a/hw/ppc/prep.c >> +++ b/hw/ppc/prep.c > [...] >> @@ -510,41 +509,13 @@ static void ppc_prep_init(QEMUMachineInitArgs *args) >> memory_region_add_subregion(sysmem, 0, ram); >> >> /* allocate and load BIOS */ >> - memory_region_init_ram(bios, NULL, "ppc_prep.bios", BIOS_SIZE); >> - memory_region_set_readonly(bios, true); >> - memory_region_add_subregion(sysmem, (uint32_t)(-BIOS_SIZE), bios); >> - vmstate_register_ram_global(bios); >> - if (bios_name == NULL) >> - bios_name = BIOS_FILENAME; >> - filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >> - if (filename) { >> - bios_size = load_elf(filename, NULL, NULL, NULL, >> - NULL, NULL, 1, ELF_MACHINE, 0); >> - if (bios_size < 0) { >> - bios_size = get_image_size(filename); >> - if (bios_size > 0 && bios_size <= BIOS_SIZE) { >> - hwaddr bios_addr; >> - bios_size = (bios_size + 0xfff) & ~0xfff; >> - bios_addr = (uint32_t)(-bios_size); >> - bios_size = load_image_targphys(filename, bios_addr, bios_size); >> - } >> - if (bios_size > BIOS_SIZE) { >> - fprintf(stderr, "qemu: PReP bios '%s' is too large (0x%x)\n", >> - bios_name, bios_size); >> - exit(1); >> - } >> - } >> - } else { >> - bios_size = -1; >> - } >> - if (bios_size < 0 && !qtest_enabled()) { >> - fprintf(stderr, "qemu: could not load PPC PReP bios '%s'\n", >> - bios_name); >> - exit(1); >> - } >> - if (filename) { >> - g_free(filename); >> - } >> + /* Open Hack'Ware hack: bios size is 512K and is loaded at 0xfff00000. >> + * However, reset address is 0xfffffffc. Mirror the bios from >> + * 0xfff00000 to 0xfff80000. >> + */ >> + memory_region_init_alias(bios, NULL, "bios-alias", sysmem, 0xfff00000, >> + 0x00080000); >> + memory_region_add_subregion_overlap(sysmem, 0xfff80000, bios, 1); >> >> if (linux_boot) { >> kernel_base = KERNEL_LOAD_ADDR; > [snip] > >
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. Regards, Hervé >> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> >> --- >> hw/pci-host/prep.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/ppc/prep.c | 50 +++++++++++++------------------------------------- >> 2 files changed, 64 insertions(+), 37 deletions(-) > [...] >> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c >> index 9f8538c..8a09e2b 100644 >> --- a/hw/ppc/prep.c >> +++ b/hw/ppc/prep.c > [...] >> @@ -510,41 +509,13 @@ static void ppc_prep_init(QEMUMachineInitArgs *args) >> memory_region_add_subregion(sysmem, 0, ram); >> >> /* allocate and load BIOS */ >> - memory_region_init_ram(bios, NULL, "ppc_prep.bios", BIOS_SIZE); >> - memory_region_set_readonly(bios, true); >> - memory_region_add_subregion(sysmem, (uint32_t)(-BIOS_SIZE), bios); >> - vmstate_register_ram_global(bios); >> - if (bios_name == NULL) >> - bios_name = BIOS_FILENAME; >> - filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >> - if (filename) { >> - bios_size = load_elf(filename, NULL, NULL, NULL, >> - NULL, NULL, 1, ELF_MACHINE, 0); >> - if (bios_size < 0) { >> - bios_size = get_image_size(filename); >> - if (bios_size > 0 && bios_size <= BIOS_SIZE) { >> - hwaddr bios_addr; >> - bios_size = (bios_size + 0xfff) & ~0xfff; >> - bios_addr = (uint32_t)(-bios_size); >> - bios_size = load_image_targphys(filename, bios_addr, bios_size); >> - } >> - if (bios_size > BIOS_SIZE) { >> - fprintf(stderr, "qemu: PReP bios '%s' is too large (0x%x)\n", >> - bios_name, bios_size); >> - exit(1); >> - } >> - } >> - } else { >> - bios_size = -1; >> - } >> - if (bios_size < 0 && !qtest_enabled()) { >> - fprintf(stderr, "qemu: could not load PPC PReP bios '%s'\n", >> - bios_name); >> - exit(1); >> - } >> - if (filename) { >> - g_free(filename); >> - } >> + /* Open Hack'Ware hack: bios size is 512K and is loaded at 0xfff00000. >> + * However, reset address is 0xfffffffc. Mirror the bios from >> + * 0xfff00000 to 0xfff80000. >> + */ >> + memory_region_init_alias(bios, NULL, "bios-alias", sysmem, 0xfff00000, >> + 0x00080000); >> + memory_region_add_subregion_overlap(sysmem, 0xfff80000, bios, 1); >> >> if (linux_boot) { >> kernel_base = KERNEL_LOAD_ADDR; > [snip] > >
On 23.12.2013, at 07:48, Hervé Poussineau <hpoussin@reactos.org> 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); Alex
Alexander Graf a écrit : > On 23.12.2013, at 07:48, Hervé Poussineau <hpoussin@reactos.org> 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. Regards, Hervé
On 23 December 2013 06:48, Hervé Poussineau <hpoussin@reactos.org> wrote: > 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. > >>> + /* Open Hack'Ware hack: bios size is 512K and is loaded at >>> 0xfff00000. >>> + * However, reset address is 0xfffffffc. Mirror the bios from >>> + * 0xfff00000 to 0xfff80000. >>> + */ >>> + memory_region_init_alias(bios, NULL, "bios-alias", sysmem, >>> 0xfff00000, >>> + 0x00080000); >>> + memory_region_add_subregion_overlap(sysmem, 0xfff80000, bios, 1); This code creates the mirrored region regardless of the size of the firmware blob, right? I think that means that if we do supply a 1MB blob it'll do the wrong thing. You probably want to have some "mirror this object as many times as necessary to fill the space" logic. We could probably do with having a generic MemoryRegion API for that, actually -- it's not uncommon behaviour for devices to be accessible every N bytes because they simply don't decode the full set of address lines. memory_region_add_subregion_tiled(MemoryRegion *mr, hwaddr offset, hwaddr tilelen, MemoryRegion *subregion) to add copies of subregion to container mr starting at offset for tilelen bytes, maybe? (we assume subregion to be created at the length that each 'tile' should be, so don't need to pass that too). thanks -- PMM
Peter Maydell a écrit : > On 23 December 2013 06:48, Hervé Poussineau <hpoussin@reactos.org> wrote: >> 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. >> >>>> + /* Open Hack'Ware hack: bios size is 512K and is loaded at >>>> 0xfff00000. >>>> + * However, reset address is 0xfffffffc. Mirror the bios from >>>> + * 0xfff00000 to 0xfff80000. >>>> + */ >>>> + memory_region_init_alias(bios, NULL, "bios-alias", sysmem, >>>> 0xfff00000, >>>> + 0x00080000); >>>> + memory_region_add_subregion_overlap(sysmem, 0xfff80000, bios, 1); > > This code creates the mirrored region regardless of the size of the > firmware blob, right? I think that means that if we do supply a > 1MB blob it'll do the wrong thing. You probably want to have some > "mirror this object as many times as necessary to fill the space" > logic. > > We could probably do with having a generic MemoryRegion > API for that, actually -- it's not uncommon behaviour for devices > to be accessible every N bytes because they simply don't > decode the full set of address lines. > > memory_region_add_subregion_tiled(MemoryRegion *mr, > hwaddr offset, hwaddr tilelen, > MemoryRegion *subregion) > > to add copies of subregion to container mr starting at offset > for tilelen bytes, maybe? (we assume subregion to be created > at the length that each 'tile' should be, so don't need to pass > that too). This hack is meant to exist only as long as OHW has not been replaced by something else. That's a hack which has to be used only for *current OHW firmware* (ie 512KB) and only for *a short time*. I've already patches to replace OHW by OpenBIOS, but some details need some more polish. So, I don't want to invest too much time to polish this hack. Choose whatever you want, but I don't want to take more time to push this patchset. If you really don't like it, I can provide a OHW image which is 1MB, so this hack becomes moot. It will be created by concatenating 2 512KB OHW images in a 1MB image. Regards, Hervé
Am 23.12.2013 19:13, schrieb Hervé Poussineau: > Alexander Graf a écrit : >> On 23.12.2013, at 07:48, Hervé Poussineau <hpoussin@reactos.org> 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? 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
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 <hpoussin@reactos.org> 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 >
On 23.12.2013, at 22:54, Hervé Poussineau <hpoussin@reactos.org> 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 <hpoussin@reactos.org> 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 >
On 23/12/13 20:02, Andreas Färber wrote: > 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. It may be worth adding that when Hervé's latest patches are applied to OpenBIOS SVN trunk (currently frozen while I wait for git.qemu.org to update its mirror) then AFAIK the main issue preventing OHW from being switched for OpenBIOS is that OpenBIOS hangs somewhere in the MMU setup code unless QEMU is launched with -cpu 750. It's probably fairly easy to resolve for someone with knowledge of 600 and 700 series CPUs, but my background is mainly SPARC so it's outside my area of expertise. FWIW what images are people currently booting under -M prep? My general feeling is that OpenBIOS will be able to boot a lot more images straight off, but it would be good to get a list of what images people are actually using to ensure that there are no regressions. ATB, Mark.
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c index 6ca9802..4dc5456 100644 --- a/hw/pci-host/prep.c +++ b/hw/pci-host/prep.c @@ -28,7 +28,9 @@ #include "hw/pci/pci_bus.h" #include "hw/pci/pci_host.h" #include "hw/i386/pc.h" +#include "hw/loader.h" #include "exec/address-spaces.h" +#include "elf.h" #define TYPE_RAVEN_PCI_DEVICE "raven" #define TYPE_RAVEN_PCI_HOST_BRIDGE "raven-pcihost" @@ -38,6 +40,9 @@ typedef struct RavenPCIState { PCIDevice dev; + uint32_t elf_machine; + char *bios_name; + MemoryRegion bios; } RavenPCIState; #define RAVEN_PCI_HOST_BRIDGE(obj) \ @@ -52,6 +57,8 @@ typedef struct PRePPCIState { RavenPCIState pci_dev; } PREPPCIState; +#define BIOS_SIZE (1024 * 1024) + static inline uint32_t PPC_PCIIO_config(hwaddr addr) { int i; @@ -169,10 +176,46 @@ static void raven_pcihost_initfn(Object *obj) static int raven_init(PCIDevice *d) { + Object *obj = OBJECT(d); + RavenPCIState *s = RAVEN_PCI_DEVICE(d); + char *filename; + int bios_size = -1; + d->config[0x0C] = 0x08; // cache_line_size d->config[0x0D] = 0x10; // latency_timer d->config[0x34] = 0x00; // capabilities_pointer + memory_region_init_ram(&s->bios, obj, "bios", BIOS_SIZE); + memory_region_set_readonly(&s->bios, true); + memory_region_add_subregion(get_system_memory(), (uint32_t)(-BIOS_SIZE), + &s->bios); + vmstate_register_ram_global(&s->bios); + if (s->bios_name) { + filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, s->bios_name); + if (filename) { + if (s->elf_machine != EM_NONE) { + bios_size = load_elf(filename, NULL, NULL, NULL, + NULL, NULL, 1, s->elf_machine, 0); + } + if (bios_size < 0) { + bios_size = get_image_size(filename); + if (bios_size > 0 && bios_size <= BIOS_SIZE) { + hwaddr bios_addr; + bios_size = (bios_size + 0xfff) & ~0xfff; + bios_addr = (uint32_t)(-BIOS_SIZE); + bios_size = load_image_targphys(filename, bios_addr, + bios_size); + } + } + } + if (bios_size < 0 || bios_size > BIOS_SIZE) { + hw_error("qemu: could not load bios image '%s'\n", s->bios_name); + } + if (filename) { + g_free(filename); + } + } + return 0; } @@ -208,12 +251,20 @@ static const TypeInfo raven_info = { .class_init = raven_class_init, }; +static Property raven_pcihost_properties[] = { + DEFINE_PROP_UINT32("elf-machine", PREPPCIState, pci_dev.elf_machine, + EM_NONE), + DEFINE_PROP_STRING("bios-name", PREPPCIState, pci_dev.bios_name), + DEFINE_PROP_END_OF_LIST() +}; + static void raven_pcihost_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); dc->realize = raven_pcihost_realizefn; + dc->props = raven_pcihost_properties; dc->fw_name = "pci"; dc->no_user = 1; } diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c index 9f8538c..8a09e2b 100644 --- a/hw/ppc/prep.c +++ b/hw/ppc/prep.c @@ -456,7 +456,6 @@ static void ppc_prep_init(QEMUMachineInitArgs *args) MemoryRegion *sysmem = get_system_memory(); PowerPCCPU *cpu = NULL; CPUPPCState *env = NULL; - char *filename; nvram_t nvram; M48t59State *m48t59; MemoryRegion *PPC_io_memory = g_new(MemoryRegion, 1); @@ -464,7 +463,7 @@ static void ppc_prep_init(QEMUMachineInitArgs *args) #if 0 MemoryRegion *xcsr = g_new(MemoryRegion, 1); #endif - int linux_boot, i, nb_nics1, bios_size; + int linux_boot, i, nb_nics1; MemoryRegion *ram = g_new(MemoryRegion, 1); MemoryRegion *bios = g_new(MemoryRegion, 1); uint32_t kernel_base, initrd_base; @@ -510,41 +509,13 @@ static void ppc_prep_init(QEMUMachineInitArgs *args) memory_region_add_subregion(sysmem, 0, ram); /* allocate and load BIOS */ - memory_region_init_ram(bios, NULL, "ppc_prep.bios", BIOS_SIZE); - memory_region_set_readonly(bios, true); - memory_region_add_subregion(sysmem, (uint32_t)(-BIOS_SIZE), bios); - vmstate_register_ram_global(bios); - if (bios_name == NULL) - bios_name = BIOS_FILENAME; - filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); - if (filename) { - bios_size = load_elf(filename, NULL, NULL, NULL, - NULL, NULL, 1, ELF_MACHINE, 0); - if (bios_size < 0) { - bios_size = get_image_size(filename); - if (bios_size > 0 && bios_size <= BIOS_SIZE) { - hwaddr bios_addr; - bios_size = (bios_size + 0xfff) & ~0xfff; - bios_addr = (uint32_t)(-bios_size); - bios_size = load_image_targphys(filename, bios_addr, bios_size); - } - if (bios_size > BIOS_SIZE) { - fprintf(stderr, "qemu: PReP bios '%s' is too large (0x%x)\n", - bios_name, bios_size); - exit(1); - } - } - } else { - bios_size = -1; - } - if (bios_size < 0 && !qtest_enabled()) { - fprintf(stderr, "qemu: could not load PPC PReP bios '%s'\n", - bios_name); - exit(1); - } - if (filename) { - g_free(filename); - } + /* Open Hack'Ware hack: bios size is 512K and is loaded at 0xfff00000. + * However, reset address is 0xfffffffc. Mirror the bios from + * 0xfff00000 to 0xfff80000. + */ + memory_region_init_alias(bios, NULL, "bios-alias", sysmem, 0xfff00000, + 0x00080000); + memory_region_add_subregion_overlap(sysmem, 0xfff80000, bios, 1); if (linux_boot) { kernel_base = KERNEL_LOAD_ADDR; @@ -593,6 +564,11 @@ static void ppc_prep_init(QEMUMachineInitArgs *args) } dev = qdev_create(NULL, "raven-pcihost"); + if (bios_name == NULL) { + bios_name = BIOS_FILENAME; + } + qdev_prop_set_string(dev, "bios-name", bios_name); + qdev_prop_set_uint32(dev, "elf-machine", ELF_MACHINE); pcihost = PCI_HOST_BRIDGE(dev); object_property_add_child(qdev_get_machine(), "raven", OBJECT(dev), NULL); qdev_init_nofail(dev);
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. 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). Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> --- hw/pci-host/prep.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ hw/ppc/prep.c | 50 +++++++++++++------------------------------------- 2 files changed, 64 insertions(+), 37 deletions(-)