Re: [PATCH v9] mac_oldworld: Allow loading binary ROM image

[BALATON Zoltan via <qemu-devel@nongnu.org>]

[-- Attachment #1: Type: text/plain, Size: 5870 bytes --]

On Sat, 17 Oct 2020, Philippe Mathieu-Daudé wrote:
> On 10/17/20 6:31 PM, BALATON Zoltan via wrote:
>> On Sat, 17 Oct 2020, Philippe Mathieu-Daudé wrote:
>>> +Alistair for loader
>>> 
>>> On 10/17/20 5:47 PM, BALATON Zoltan via 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>
>>>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> v9: Revert change from v8, back to the same as v7 rebased on latest
>>>> 
>>>>   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 05e46ee6fe..0117ae17f5 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)
>>>> @@ -100,6 +102,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;
>>>> @@ -128,24 +131,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);
>>>> +        /* Unfortunately, load_elf sign-extends reading elf32 */
>>> 
>>> Maybe this is what translate_fn() is for?
>>> 
>>> uint64_t oldworld_phys(void *opaque, uint64_t addr)
>>> {
>>>    return addr & UINT32_MAX;
>>> }
>>> 
>>> Using as (untested):
>>> 
>>>        bios_size = load_elf(filename, NULL, oldworld_phys, NULL,
>>>                             NULL, &bios_addr, NULL,
>>>                             NULL, 1, PPC_ELF_MACHINE, 0, 0);
>> 
>> Please don't come up with any more great ideas for this patch unless you 
>> also propose a replacement and test it. This one works and we could just 
>> get this in as it is until the real problem with load_elf is fixed at which 
>> point all this can be removed so no need to be more sophisticated as the 
>> simple cast I have.
>
> Zoltan, I'm not trying to block your patch to get merged,

I didn't say you're trying to do that but based on my past experience any 
slightest doubt could result in that so I did not want it to miss another 
freeze now that it's almost got in.

> I'm asking because I'm trying to understand how this API
> is expected to be used.

Likely nobody knows, this seems to have been evolved into this mess which 
could be cleaned up but unrelated to this series. Probably nobody dared to 
touch it so far as it's used by almost every board so breling something is 
easy and testing it is difficult.

Regards,
BALATON Zoltan

>> As you can see in the original discussion:
>> 
>> http://patchwork.ozlabs.org/project/qemu-devel/patch/c69a791c7cad1246f3f34b3993dee4f549b75aa2.1593456926.git.balaton@eik.bme.hu/ 
>> 
>> problem is really in include/hw/elf_ops.h this is just a work around for 
>> that as I did not want to break anything I can't test so I'd rather fix it 
>> up here and let you fix load_elf then drop this cast. But unless you can do 
>> that before the freeze please don't hold up this patch any more.
>> 
>> Regards,
>> BALATON Zoltan
>> 
>>>> +        bios_addr = (uint32_t)bios_addr;
>>>> +
>>>> +        if (bios_size <= 0) {
>>>> +            /* or load binary ROM image */
>>>> +            bios_size = load_image_targphys(filename, PROM_BASE, 
>>>> PROM_SIZE);
>>>> +            bios_addr = PROM_BASE;
>>>> +        }
>>>>           g_free(filename);
>>>>       } else {
>>>>           bios_size = -1;
>>>>       }
>>>> -    if (bios_size < 0 || bios_size > BIOS_SIZE) {
>>>> +    if (bios_size < 0 || bios_addr - PROM_BASE + bios_size > PROM_SIZE) 
>>>> {
>>>>           error_report("could not load PowerPC bios '%s'", bios_name);
>>>>           exit(1);
>>>>       }
>>>> 
>>> 
>>> 
>>> 
>
>