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

[Markus Armbruster <armbru@redhat.com>]

Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:

> On 16/09/2021 14:06, Markus Armbruster wrote:
>
>> 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.
>
> (goes and looks)
>
> Ultimately this gets set from
> memory_region_init_rom_device_nomigrate() where err is returned from
> qemu_ram_alloc() which is fairly fatal. So I guess this should be
> &error_abort then?

There are two schools of thought on handling out-of-memory conditions.

One school argues that attempting to recover by failing the operation is
expensive and futile.  It's expensive, because it creates a huge number
of failure paths that wouldn't otherwise exists, and won't be tested.
It's futile, because by the time malloc() fails, the process is doomed
anyway.  That's g_malloc().  It aborts on OOM.

The other school disagrees, and writes the error paths.  In this case,
propagate to caller.

In QEMU, we of course do both, and with no clear guidance on when to do
what.  All we have is talk about aborting only on "small" allocations,
whatever "small" may be.

I'm cool with &error_abort here.

> Note that I copied that part of the logic from hw/pci/pci.c's
> pci_add_option_rom() so it may also need to be adjusted there.

We're quite prone to use &error_fatal or NULL where we should use
&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.
>
> Looking again at pci_add_option_rom() then perhaps this should be
> error_setg() instead: if you are explicitly trying to load a ROM
> image, then you should at least be able to get the filename correct.

Makes sense to me.

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