Re: [PATCH 8/9] hw/i386/pc_sysfw.c: convert pflash0 and pflash1 object props to class props

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Jul 08, 2026 at 10:40:54AM +0100, Mark Cave-Ayland wrote:
> On 07/07/2026 17:58, Daniel P. Berrangé wrote:
> 
> > On Fri, Jul 03, 2026 at 10:08:01AM +0100, Mark Cave-Ayland wrote:
> > > Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> > > ---
> > >   include/hw/i386/pc.h | 2 ++
> > >   hw/i386/pc.c         | 8 ++++++++
> > >   hw/i386/pc_sysfw.c   | 7 +------
> > >   3 files changed, 11 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 309de2eda1..16cb0ec01f 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -40,6 +40,8 @@ typedef struct PCMachineState {
> > >       Object *alias_pcspk;
> > >       Object *alias_rtc_time;
> > > +    Object *alias_pflash0;
> > > +    Object *alias_pflash1;
> > >       /* Configuration options: */
> > >       uint64_t max_ram_below_4g;
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 13c9c1bfc8..11eef176f4 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1762,6 +1762,14 @@ static void pc_machine_class_init(ObjectClass *oc, const void *data)
> > >                                      offsetof(X86MachineState, acpi_dev),
> > >                                      object_property_allow_set_link,
> > >                                      OBJ_PROP_LINK_STRONG);
> > > +    object_class_property_add_alias(oc, "pflash0",
> > > +                                    offsetof(PCMachineState, alias_pflash0),
> > > +                                    TYPE_PFLASH_CFI01,
> > > +                                    "drive");
> > > +    object_class_property_add_alias(oc, "pflash1",
> > > +                                    offsetof(PCMachineState, alias_pflash1),
> > > +                                    TYPE_PFLASH_CFI01,
> > > +                                    "drive");
> > >   }
> > >   static const TypeInfo pc_machine_info = {
> > > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > > index 1a41a5972b..441e777191 100644
> > > --- a/hw/i386/pc_sysfw.c
> > > +++ b/hw/i386/pc_sysfw.c
> > > @@ -85,8 +85,7 @@ static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms,
> > >       qdev_prop_set_uint8(dev, "width", 1);
> > >       qdev_prop_set_string(dev, "name", name);
> > >       object_property_add_child(OBJECT(pcms), name, OBJECT(dev));
> > > -    object_property_add_alias(OBJECT(pcms), alias_prop_name,
> > > -                              OBJECT(dev), "drive");
> > > +    object_property_set_alias(OBJECT(pcms), alias_prop_name, OBJECT(dev));
> > >       /*
> > >        * The returned reference is tied to the child property and
> > >        * will be removed with object_unparent.
> > > @@ -109,16 +108,12 @@ void pc_system_flash_create(PCMachineState *pcms)
> > >   void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> > >   {
> > > -    char *prop_name;
> > >       int i;
> > >       assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
> > >       for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> > >           if (!qdev_is_realized(DEVICE(pcms->flash[i]))) {
> > > -            prop_name = g_strdup_printf("pflash%d", i);
> > > -            object_property_del(OBJECT(pcms), prop_name);
> > > -            g_free(prop_name);
> > >               object_unparent(OBJECT(pcms->flash[i]));
> > >               pcms->flash[i] = NULL;
> > >           }
> > 
> > Does this externally change visible behaviour ?
> > 
> > The pc_system_flash_cleanup_unused method is run in the instance
> > init method, and will purge the 'pflash0' and 'pflash1' properties
> > if they were never set.
> > 
> > IIUC with the new code pflash0 and pflash1 will always be visible,
> > but would be unset, as object_property_set_alias would not have
> > been called.
> > 
> > THe new behaviour is arguably better - IMHO properties should
> > always be present, and use "unset" as an explicit state.
> 
> Yes, it does change the behaviour to reflect the fact that the pflash0 and
> pflash1 properties always exist:
> 
> Before this series:
> 
> $ ./build/qemu-system-x86_64 -M q35 -monitor stdio
> QEMU 11.0.50 monitor - type 'help' for more information
> (qemu) qom-get /machine pflash0
> Error: Property 'pc-q35-11.1-machine.pflash0' not found
> 
> After this series:
> 
> $ ./build/qemu-system-x86_64 -M q35 -monitor stdio
> QEMU 11.0.50 monitor - type 'help' for more information
> (qemu) qom-get /machine pflash0
> ""

As a behaviour change, please call this out in the commit message,
since this is an example situation that needs a subsystem maintainer
review/ack rather than a general review/ack.



With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|