qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v7 1/8] mac_oldworld: Allow loading binary ROM image


From: Mark Cave-Ayland
Subject: Re: [PATCH v7 1/8] mac_oldworld: Allow loading binary ROM image
Date: Mon, 6 Jul 2020 21:24:04 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0

On 05/07/2020 08:31, David Gibson wrote:

> On Tue, Jun 30, 2020 at 11:45:42PM +0200, BALATON Zoltan wrote:
>> On Tue, 30 Jun 2020, Mark Cave-Ayland wrote:
>>> On 29/06/2020 19:55, BALATON Zoltan wrote:
>>>> The beige G3 Power Macintosh has a 4MB firmware ROM. Fix the size of
>>>> the rom region and fall back to loading a binary image with -bios if
>>>> loading ELF image failed. This allows testing emulation with a ROM
>>>> image from real hardware as well as using an ELF OpenBIOS image.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> v4: use load address from ELF to check if ROM is too big
>>>>
>>>>  hw/ppc/mac_oldworld.c | 29 ++++++++++++++++++++---------
>>>>  1 file changed, 20 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
>>>> index f8c204ead7..baf3da6f90 100644
>>>> --- a/hw/ppc/mac_oldworld.c
>>>> +++ b/hw/ppc/mac_oldworld.c
>>>> @@ -59,6 +59,8 @@
>>>>  #define NDRV_VGA_FILENAME "qemu_vga.ndrv"
>>>>
>>>>  #define GRACKLE_BASE 0xfec00000
>>>> +#define PROM_BASE 0xffc00000
>>>> +#define PROM_SIZE (4 * MiB)
>>>>
>>>>  static void fw_cfg_boot_set(void *opaque, const char *boot_device,
>>>>                              Error **errp)
>>>> @@ -99,6 +101,7 @@ static void ppc_heathrow_init(MachineState *machine)
>>>>      SysBusDevice *s;
>>>>      DeviceState *dev, *pic_dev;
>>>>      BusState *adb_bus;
>>>> +    uint64_t bios_addr;
>>>>      int bios_size;
>>>>      unsigned int smp_cpus = machine->smp.cpus;
>>>>      uint16_t ppc_boot_device;
>>>> @@ -127,24 +130,32 @@ static void ppc_heathrow_init(MachineState *machine)
>>>>
>>>>      memory_region_add_subregion(sysmem, 0, machine->ram);
>>>>
>>>> -    /* allocate and load BIOS */
>>>> -    memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE,
>>>> +    /* allocate and load firmware ROM */
>>>> +    memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE,
>>>>                             &error_fatal);
>>>> +    memory_region_add_subregion(sysmem, PROM_BASE, bios);
>>>>
>>>> -    if (bios_name == NULL)
>>>> +    if (!bios_name) {
>>>>          bios_name = PROM_FILENAME;
>>>> +    }
>>>>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>>>> -    memory_region_add_subregion(sysmem, PROM_ADDR, bios);
>>>> -
>>>> -    /* Load OpenBIOS (ELF) */
>>>>      if (filename) {
>>>> -        bios_size = load_elf(filename, NULL, 0, NULL, NULL, NULL, NULL, 
>>>> NULL,
>>>> -                             1, PPC_ELF_MACHINE, 0, 0);
>>>> +        /* Load OpenBIOS (ELF) */
>>>> +        bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr,
>>>> +                             NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0);
>>>> +        if (bios_size <= 0) {
>>>> +            /* or load binary ROM image */
>>>> +            bios_size = load_image_targphys(filename, PROM_BASE, 
>>>> PROM_SIZE);
>>>> +            bios_addr = PROM_BASE;
>>>> +        } else {
>>>> +            /* load_elf sets high 32 bits for some reason, strip those */
>>>> +            bios_addr &= 0xffffffffULL;
>>>
>>> Repeating my earlier comment from v5: something is wrong here if you need 
>>> to manually
>>> strip the high bits. If you compare with SPARC32 which uses the same 
>>> approach, there
>>> is no such strip required - have a look there to try and figure out what's 
>>> going on here.
>>
>> OK, the problem here is this:
>>
>> $ gdb qemu-system-ppc
>> (gdb) b mac_oldworld.c:146
>> Breakpoint 1 at 0x416770: file hw/ppc/mac_oldworld.c, line 146.
>> (gdb) r
>> Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init 
>> (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146
>> 146      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>> (gdb) n
>> 147      if (filename) {
>> 149          bios_size = load_elf(filename, NULL, NULL, NULL, NULL, 
>> &bios_addr,
>> 151          if (bios_size <= 0) {
>> (gdb) p bios_size
>> $1 = 755500
>> (gdb) p/x bios_addr
>> $2 = 0xfffffffffff00000
>>
>> this happens within load_elf that I don't feel like wanting to debug but
>> causes problem when we use it to calculate bios size later here:
> 
> I think the problem is here, in include/hw/elf_ops.h:
> 
>     if (lowaddr)
>         *lowaddr = (uint64_t)(elf_sword)low;
> 
> "low" is a u64, but for a 32-bit ELF file, which is what I'm guessing
> you're dealing with here, elf_sword is an int32_t.  So the first cast
> truncates the high bits, but makes it a signed value, so the second
> cast sign extends, resulting in those high bits.
> 
> Sign extending rather than zero-extending seems a dubious choice here,
> so I wonder if that should be (elf_word) instead of (elf_sword).  But
> maybe there's some weird other case where we do want the sign
> extension here.

I agree that sign-extending here feels odd since it will cause problems with 
32-bit
values on 64-bit systems like we see here, however I'm not really familiar 
enough
with the QEMU ELF loader to know what the intent was here.

The original reference for this was SPARC32 which does a similar thing, but
ultimately the resulting address is never used and the ROM is loaded at a fixed
address which is why it doesn't matter here.

Given that this needs to work with both qemu-system-ppc and qemu-system-ppc64 
would
casting bios_addr to target_ulong work?


ATB,

Mark.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]