Re: [PATCH v3 11/20] nubus-device: add romfile property for loading declaration ROMs

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 9/16/21 12:05 PM, Mark Cave-Ayland wrote:
>> The declaration ROM is located at the top-most address of the standard slot
>> space.
>> 
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/nubus/nubus-device.c  | 43 +++++++++++++++++++++++++++++++++++++++-
>>  include/hw/nubus/nubus.h |  6 ++++++
>>  2 files changed, 48 insertions(+), 1 deletion(-)
>
>> @@ -38,10 +43,46 @@ static void nubus_device_realize(DeviceState *dev, Error 
>> **errp)
>>      memory_region_add_subregion(&nubus->slot_io, slot_offset,
>>                                  &nd->slot_mem);
>>      g_free(name);
>> +
>> +    /* Declaration ROM */
>> +    if (nd->romfile != NULL) {
>> +        path = qemu_find_file(QEMU_FILE_TYPE_BIOS, nd->romfile);
>> +        if (path == NULL) {
>> +            path = g_strdup(nd->romfile);
>> +        }
>> +
>> +        size = get_image_size(path);
>> +        if (size < 0) {
>> +            error_setg(errp, "failed to find romfile \"%s\"", nd->romfile);
>> +            g_free(path);
>> +            return;
>> +        } else if (size == 0) {
>> +            error_setg(errp, "romfile \"%s\" is empty", nd->romfile);
>> +            g_free(path);
>> +            return;
>> +        } else if (size > NUBUS_DECL_ROM_MAX_SIZE) {
>> +            error_setg(errp, "romfile \"%s\" too large (maximum size 128K)",
>> +                       nd->romfile);
>> +            g_free(path);
>> +            return;
>> +        }
>> +
>> +        name = g_strdup_printf("nubus-slot-%x-declaration-rom", nd->slot);
>> +        memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, size,
>> +                               &error_fatal);

Is this error expected to happen?

If yes, you should quite probably propagate it.

If no, &error_abort.

>> +        ret = load_image_mr(path, &nd->decl_rom);
>
> load_image_mr() already calls get_image_size(), rom_add_file() and
> qemu_find_file(). *But* it doesn't takes and Error handle, and report
> error using fprintf()...

... except when they don't:

    int load_image_mr(const char *filename, MemoryRegion *mr)
    {
        int size;

        if (!memory_access_is_direct(mr, false)) {
            /* Can only load an image into RAM or ROM */
--->        return -1;
        }

        size = get_image_size(filename);

        if (size < 0 || size > memory_region_size(mr)) {
            return -1;
        }
        if (size > 0) {
            if (rom_add_file_mr(filename, mr, -1) < 0) {
                return -1;
            }
        }
        return size;
    }

Hot mess!

>                          So unfortunately rom_add*() functions are
> kinda outdated and you are doing the right thing to propagate detailled
> errors.

I can't see errors being propagated, only a warn_report()...

>         Therefore:
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>> +        g_free(path);
>> +        if (ret < 0) {
>> +            warn_report("nubus-device: could not load prom '%s'", 
>> nd->romfile);

... here.

>> +        }
>> +        memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - size,
>> +                                    &nd->decl_rom);
>> +    }
>>  }