| Message ID | 20210916100554.10963-12-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | nubus: bus, device, bridge, IRQ and address space improvements | expand |
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); > + 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()... So unfortunately rom_add*() functions are kinda outdated and you are doing the right thing to propagate detailled errors. 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); > + } > + memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - size, > + &nd->decl_rom); > + } > }
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); >> + } >> }
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? 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. >>> + 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. >>> + } >>> + memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - size, >>> + &nd->decl_rom); >>> + } >>> } ATB, Mark.
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.
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c index 9c1992ceb0..dbb3bb7efd 100644 --- a/hw/nubus/nubus-device.c +++ b/hw/nubus/nubus-device.c @@ -9,16 +9,21 @@ */ #include "qemu/osdep.h" +#include "qemu/datadir.h" +#include "hw/loader.h" #include "hw/nubus/nubus.h" #include "qapi/error.h" +#include "qemu/error-report.h" static void nubus_device_realize(DeviceState *dev, Error **errp) { NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(dev)); NubusDevice *nd = NUBUS_DEVICE(dev); - char *name; + char *name, *path; hwaddr slot_offset; + int64_t size; + int ret; /* Super */ slot_offset = (nd->slot - 6) * NUBUS_SUPER_SLOT_SIZE; @@ -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); + ret = load_image_mr(path, &nd->decl_rom); + g_free(path); + if (ret < 0) { + warn_report("nubus-device: could not load prom '%s'", nd->romfile); + } + memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - size, + &nd->decl_rom); + } } static Property nubus_device_properties[] = { DEFINE_PROP_INT32("slot", NubusDevice, slot, -1), + DEFINE_PROP_STRING("romfile", NubusDevice, romfile), DEFINE_PROP_END_OF_LIST() }; diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h index 87a97516c7..0c9f50c32e 100644 --- a/include/hw/nubus/nubus.h +++ b/include/hw/nubus/nubus.h @@ -12,6 +12,7 @@ #include "hw/qdev-properties.h" #include "exec/address-spaces.h" #include "qom/object.h" +#include "qemu/units.h" #define NUBUS_SUPER_SLOT_SIZE 0x10000000U #define NUBUS_SUPER_SLOT_NB 0x9 @@ -39,12 +40,17 @@ struct NubusBus { uint32_t slot_available_mask; }; +#define NUBUS_DECL_ROM_MAX_SIZE (128 * KiB) + struct NubusDevice { DeviceState qdev; int32_t slot; MemoryRegion super_slot_mem; MemoryRegion slot_mem; + + char *romfile; + MemoryRegion decl_rom; }; #endif
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(-)