Re: [Qemu-devel] [PATCH v3 11/12] pc: Support firmware configuration with -blockdev

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

Le lun. 11 mars 2019 17:02, Markus Armbruster <armbru@redhat.com> a écrit :

> Paolo Bonzini <pbonzini@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?
>

ovmf_code & ovmf_vars?

>
> >>   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!
>
> [...]
>
>