Paolo Bonzini <pbonz...@redhat.com> writes: > On 08/03/19 14:14, Markus Armbruster wrote: >> The PC machines put firmware in ROM by default. To get it put into >> flash memory (required by OVMF), you have to use -drive >> if=pflash,unit=0,... and optionally -drive if=pflash,unit=1,... >> >> Why two -drive? This permits setting up one part of the flash memory >> read-only, and the other part read/write. Below the hood, it creates >> two separate flash devices, because we were too lazy to improve our >> flash device models to support sector protection. > > Not just that, it avoids the need to "upgrade the firmware" of old > virtual machines. Instead, you just need to upgrade the files on the > host and restart QEMU, just like with non-UEFI firmware.
True. I'll insert "It also makes upgrading firmware on the host easier." >> This is actually an instance of a wider problem: our general device >> configuration interface doesn't cover onboard devices. Instead, we >> have a zoo of ad hoc interfaces that are much more limited. Some of >> them we'd rather deprecate (-drive, -net), but can't until we have >> suitable replacements. > > -net already has a better replacement, -nic, which is basically "like > -netdev but also tell board creation code about it". I suppose you'd > prefer that it were also accessible as "-netdev ...,id=foo -machine > net0=foo"? I got confused. Or rather, my mind was stuck in the past. Along with parts of our UI, to be frank. Yes, -net already has suitable replacements (-nic and -netdev hubport). We've kept around -net anyway. If this situation confuses me, chances are it confuses users, too. I think we should deprecate it. I'll drop -net from my parenthesis. Restoring a bit more quoted material for later: +void pc_system_flash_create(PCMachineState *pcms) +{ + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); + + if (pcmc->pci_enabled) { + pcms->flash[0] = pc_pflash_create("system.flash0"); + pcms->flash[1] = pc_pflash_create("system.flash1"); + object_property_add_alias(OBJECT(pcms), "pflash0", + OBJECT(pcms->flash[0]), "drive", + &error_abort); + object_property_add_alias(OBJECT(pcms), "pflash1", + OBJECT(pcms->flash[1]), "drive", + &error_abort); + } +} + >> +static void pc_system_flash_cleanup_unused(PCMachineState *pcms) >> +{ >> + char *prop_name; >> + int i; >> + Object *dev_obj; >> + >> + assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled); >> + >> + for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) { >> + dev_obj = OBJECT(pcms->flash[i]); >> + if (!object_property_get_bool(dev_obj, "realized", &error_abort)) { >> + prop_name = g_strdup_printf("pflash%d", i); >> + object_property_del(OBJECT(pcms), prop_name, &error_abort); > > Why didn't this already call object->class->unparent? Let's see... void object_property_del(Object *obj, const char *name, Error **errp) { ObjectProperty *prop = g_hash_table_lookup(obj->properties, name); if (!prop) { error_setg(errp, "Property '.%s' not found", name); return; } if (prop->release) { prop->release(obj, name, prop->opaque); } g_hash_table_remove(obj->properties, name); } The only thing that could possibly call object->class->unparent() is prop->release(). In gdb: (gdb) p *prop $1 = {name = 0x555556a4aa20 "pflash0", type = 0x555556a4aa40 "str", description = 0x555556a4aa80 "Node name or ID of a block device to use as a backend", get = 0x555555d02734 <property_get_alias>, set = 0x555555d0277a <property_set_alias>, resolve = 0x555555d027c0 <property_resolve_alias>, release = 0x555555d027f8 <property_release_alias>, opaque = 0x555556a4a990} property_release_alias() doesn't: static void property_release_alias(Object *obj, const char *name, void *opaque) { AliasProperty *prop = opaque; g_free(prop->target_name); g_free(prop); } Remember, the machine property is merely an alias that forwards to the pflash device's property. >> + g_free(prop_name); >> + /* >> + * Delete @dev_obj. Straight object_unref() is wrong, >> + * since it leaves dangling references in the parent bus >> + * behind. object_unparent() doesn't work, either: since >> + * @dev_obj hasn't been realized, dev_obj->parent is null, >> + * and object_unparent() does nothing. > > Does it work if you add the device yourself as a child of /machine, > instead of relying on /machine/unattached? I figure you're suggesting something like this incremental patch: diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index ccf2221acb..9d3a80c51a 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -99,6 +99,10 @@ void pc_system_flash_create(PCMachineState *pcms) if (pcmc->pci_enabled) { pcms->flash[0] = pc_pflash_create("system.flash0"); pcms->flash[1] = pc_pflash_create("system.flash1"); + object_property_add_child(OBJECT(pcms), "pfl0", + OBJECT(pcms->flash[0]), &error_abort); + object_property_add_child(OBJECT(pcms), "pfl1", + OBJECT(pcms->flash[1]), &error_abort); object_property_add_alias(OBJECT(pcms), "pflash0", OBJECT(pcms->flash[0]), "drive", &error_abort); @@ -122,19 +126,7 @@ static void pc_system_flash_cleanup_unused(PCMachineState *pcms) prop_name = g_strdup_printf("pflash%d", i); object_property_del(OBJECT(pcms), prop_name, &error_abort); g_free(prop_name); - /* - * Delete @dev_obj. Straight object_unref() is wrong, - * since it leaves dangling references in the parent bus - * behind. object_unparent() doesn't work, either: since - * @dev_obj hasn't been realized, dev_obj->parent is null, - * and object_unparent() does nothing. DeviceClass method - * device_unparent() works, but we have to take a - * temporary reference, or else device_unparent() commits - * a use-after-free error. - */ - object_ref(dev_obj); - object_get_class(dev_obj)->unparent(dev_obj); - object_unref(dev_obj); + object_unparent(dev_obj); pcms->flash[i] = NULL; } } Passes "make check" and "info qtree" looks good both with and without -machine pflash0,... I'm not exactly happy with "pfl0" and "pfl1". Got a favorite color for my bikeshed? >> DeviceClass method >> + * device_unparent() works, but we have to take a >> + * temporary reference, or else device_unparent() commits >> + * a use-after-free error. >> + */ >> + object_ref(dev_obj); >> + object_get_class(dev_obj)->unparent(dev_obj); >> + object_unref(dev_obj); >> + pcms->flash[i] = NULL; >> + } > > Paolo Thank you! [...]