[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 11/20] nubus-device: add romfile property for loading decl
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 11/20] nubus-device: add romfile property for loading declaration ROMs |
Date: |
Fri, 17 Sep 2021 11:53:43 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
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.
- Re: [PATCH v3 05/20] nubus: move slot bitmap checks from NubusDevice realize() to BusClass check_address(), (continued)
- [PATCH v3 06/20] nubus: implement BusClass get_dev_path(), Mark Cave-Ayland, 2021/09/16
- [PATCH v3 07/20] nubus: add trace-events for empty slot accesses, Mark Cave-Ayland, 2021/09/16
- [PATCH v3 09/20] macfb: don't register declaration ROM, Mark Cave-Ayland, 2021/09/16
- [PATCH v3 08/20] nubus: generate bus error when attempting to access empty slots, Mark Cave-Ayland, 2021/09/16
- [PATCH v3 10/20] nubus-device: remove nubus_register_rom() and nubus_register_format_block(), Mark Cave-Ayland, 2021/09/16
- [PATCH v3 11/20] nubus-device: add romfile property for loading declaration ROMs, Mark Cave-Ayland, 2021/09/16
[PATCH v3 12/20] nubus: move nubus to its own 32-bit address space, Mark Cave-Ayland, 2021/09/16
[PATCH v3 13/20] nubus-bridge: introduce separate NubusBridge structure, Mark Cave-Ayland, 2021/09/16
[PATCH v3 14/20] mac-nubus-bridge: rename MacNubusState to MacNubusBridge, Mark Cave-Ayland, 2021/09/16
[PATCH v3 15/20] nubus: move NubusBus from mac-nubus-bridge to nubus-bridge, Mark Cave-Ayland, 2021/09/16
[PATCH v3 16/20] nubus-bridge: embed the NubusBus object directly within nubus-bridge, Mark Cave-Ayland, 2021/09/16
[PATCH v3 17/20] nubus-bridge: make slot_available_mask a qdev property, Mark Cave-Ayland, 2021/09/16
[PATCH v3 18/20] nubus: add support for slot IRQs, Mark Cave-Ayland, 2021/09/16