QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v9] mac_oldworld: Allow loading binary ROM image
@ 2020-10-17 15:47 BALATON Zoltan via
  2020-10-17 16:05 ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 5+ messages in thread
From: BALATON Zoltan via @ 2020-10-17 15:47 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland

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 */
+        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);
     }
-- 
2.21.3



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v9] mac_oldworld: Allow loading binary ROM image
  2020-10-17 15:47 [PATCH v9] mac_oldworld: Allow loading binary ROM image BALATON Zoltan via
@ 2020-10-17 16:05 ` Philippe Mathieu-Daudé
  2020-10-17 16:31   ` BALATON Zoltan via
  0 siblings, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-17 16:05 UTC (permalink / raw)
  To: BALATON Zoltan, qemu-devel, qemu-ppc, Alistair Francis; +Cc: Mark Cave-Ayland

+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);

> +        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);
>       }
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v9] mac_oldworld: Allow loading binary ROM image
  2020-10-17 16:05 ` Philippe Mathieu-Daudé
@ 2020-10-17 16:31   ` BALATON Zoltan via
  2020-10-17 17:38     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 5+ messages in thread
From: BALATON Zoltan via @ 2020-10-17 16:31 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: Alistair Francis, qemu-ppc, qemu-devel


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

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. 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);
>>       }
>> 
>
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v9] mac_oldworld: Allow loading binary ROM image
  2020-10-17 16:31   ` BALATON Zoltan via
@ 2020-10-17 17:38     ` Philippe Mathieu-Daudé
  2020-10-17 21:57       ` BALATON Zoltan via
  0 siblings, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-17 17:38 UTC (permalink / raw)
  To: BALATON Zoltan; +Cc: Alistair Francis, qemu-ppc, qemu-devel

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'm asking because I'm trying to understand how this API
is expected to be used.

> 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);
>>>       }
>>>
>>
>>
>>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v9] mac_oldworld: Allow loading binary ROM image
  2020-10-17 17:38     ` Philippe Mathieu-Daudé
@ 2020-10-17 21:57       ` BALATON Zoltan via
  0 siblings, 0 replies; 5+ messages in thread
From: BALATON Zoltan via @ 2020-10-17 21:57 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: Alistair Francis, qemu-ppc, qemu-devel


[-- 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);
>>>>       }
>>>> 
>>> 
>>> 
>>> 
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-17 15:47 [PATCH v9] mac_oldworld: Allow loading binary ROM image BALATON Zoltan via
2020-10-17 16:05 ` Philippe Mathieu-Daudé
2020-10-17 16:31   ` BALATON Zoltan via
2020-10-17 17:38     ` Philippe Mathieu-Daudé
2020-10-17 21:57       ` BALATON Zoltan via

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git