sysbus_create_simple Vs qdev_create

sysbus_create_simple Vs qdev_create

[Pratik Parvati]

[-- Attachment #1: Type: text/plain, Size: 266 bytes --]

Hi Support team,

Can someone please guide me to understand the difference between
*sysbus_create_simple
*and *qdev_create*?.

I understand that these two methods are used to create a new device. But,
when to use these functions is not clear to me.

Regards,
Pratik

[-- Attachment #2: Type: text/html, Size: 491 bytes --]

Re: sysbus_create_simple Vs qdev_create

[Pratik Parvati]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

Here is a brief context that might help you.
I am referring hw/arm/versatilepb.c

The ARM PrimeCell UART (PL011) device created as follows

    dev = qdev_create(NULL, "pl011");
    s = SYS_BUS_DEVICE(dev);
    qdev_prop_set_chr(dev, "chardev", chr);
    qdev_init_nofail(dev);
    sysbus_mmio_map(s, 0, addr);
    sysbus_connect_irq(s, 0, irq);

Whereas the PL031 RTC device is created as

    /* Add PL031 Real Time Clock. */
    sysbus_create_simple("pl031", 0x101e8000, pic[10]);

What is the difference between these two devices creation? How do I know
which method to use while creating an object?

Thanks & Regards,

Pratik

On Tue, Jul 14, 2020 at 9:39 PM Pratik Parvati <pratikp@vayavyalabs.com>
wrote:

> Hi Support team,
>
> Can someone please guide me to understand the difference between *sysbus_create_simple
> *and *qdev_create*?.
>
> I understand that these two methods are used to create a new device. But,
> when to use these functions is not clear to me.
>
> Regards,
> Pratik
>

[-- Attachment #2: Type: text/html, Size: 5430 bytes --]

Re: sysbus_create_simple Vs qdev_create

[Philippe Mathieu-Daudé]

Hi Pratik,

On 7/14/20 6:17 PM, Pratik Parvati wrote:
> Here is a brief context that might help you.
> I am referring hw/arm/versatilepb.c
> 
> The ARM PrimeCell UART (PL011) device created as follows
> 
> |dev = qdev_create(NULL, "pl011"); s = SYS_BUS_DEVICE(dev);
> qdev_prop_set_chr(dev, "chardev", chr); qdev_init_nofail(dev);
> sysbus_mmio_map(s, 0, addr); sysbus_connect_irq(s, 0, irq); |
> 
> Whereas the PL031 RTC device is created as
> 
> |/* Add PL031 Real Time Clock. */ sysbus_create_simple("pl031",
> 0x101e8000, pic[10]); |
> 
> What is the difference between these two devices creation?

Both devices inherit SysBusDevice, which itself inherits QDev.

You can create QDev objects with the qdev API, and
SysBusDevice objects with the sysbus API.

sysbus_create_simple() is a condensed helper, but only allow you
to pass qemu_irq objects, not a 'chardev' property. So for this
case you have to use the qdev API instead.

> How do I know
> which method to use while creating an object?

SysBusDevice are plugged onto a bus. QDev aren't.
The sysbus API results in smaller code, easier to review.

> 
> Thanks & Regards,
> 
> Pratik
> 
> 
> On Tue, Jul 14, 2020 at 9:39 PM Pratik Parvati <pratikp@vayavyalabs.com
> <mailto:pratikp@vayavyalabs.com>> wrote:
> 
>     Hi Support team,
> 
>     Can someone please guide me to understand the difference between
>     *sysbus_create_simple *and *qdev_create*?.
> 
>     I understand that these two methods are used to create a new device.
>     But, when to use these functions is not clear to me.
> 
>     Regards,
>     Pratik
> 

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Hi Pratik,
>
> On 7/14/20 6:17 PM, Pratik Parvati wrote:
>> Here is a brief context that might help you.
>> I am referring hw/arm/versatilepb.c
>> 
>> The ARM PrimeCell UART (PL011) device created as follows
>> 
>>     dev = qdev_create(NULL, "pl011");
>>     s = SYS_BUS_DEVICE(dev);
>>     qdev_prop_set_chr(dev, "chardev", chr);
>>     qdev_init_nofail(dev);
>>     sysbus_mmio_map(s, 0, addr);
>>     sysbus_connect_irq(s, 0, irq);

This is pl011_create().

Since recent merge commit 6675a653d2e, it's

       dev = qdev_new("pl011");
       s = SYS_BUS_DEVICE(dev);
       qdev_prop_set_chr(dev, "chardev", chr);
       sysbus_realize_and_unref(s, &error_fatal);
       sysbus_mmio_map(s, 0, addr);
       sysbus_connect_irq(s, 0, irq);

>>
>> Whereas the PL031 RTC device is created as
>> 
>>     /* Add PL031 Real Time Clock. */
>>     sysbus_create_simple("pl031", 0x101e8000, pic[10]);
>> 
>> What is the difference between these two devices creation?
>
> Both devices inherit SysBusDevice, which itself inherits QDev.

Yes: TYPE_SYS_BUS_DEVICE is a subtype of TYPE_DEVICE.

> You can create QDev objects with the qdev API, and
> SysBusDevice objects with the sysbus API.

Yes.

qdev_new(), qdev_realize_and_unref(), ... work with DeviceState * (the C
type of an instance of QOM TYPE_DEVICE).

sysbus_realize_and_unref(), ... work with SysBusDevice * (the C type of
an instance of QOM TYPE_SYS_BUS_DEVICE).

Since TYPE_SYS_BUS_DEVICE is a subtype of TYPE_DEVICE, you can safely
use qdev_ functions with sysbus devices.  Example: pl011_create() uses
qdev_new() to create a sysbus device.  That's fine.

> sysbus_create_simple() is a condensed helper, but only allow you
> to pass qemu_irq objects, not a 'chardev' property. So for this
> case you have to use the qdev API instead.

Yes.  It's a helper that combines creating a sysbus device, wiring up
one MMIO region and one IRQ, and realizing.  If you need to configure or
wire up more than that, you can't use it.

>> How do I know
>> which method to use while creating an object?
>
> SysBusDevice are plugged onto a bus. QDev aren't.
> The sysbus API results in smaller code, easier to review.

The general pattern for a stand-alone device is

    dev = qdev_new(type_name);
    set properties and wire up stuff...
    qdev_realize_and_unref(dev, bus, &err);

When this is to be done in device code, say to create a component
device, the split between .instance_init() and .realize() complicates
things.  If interested, ask and I'll explain.

There are quite a few wrappers around qdev_ functions for various
subtypes of TYPE_DEVICE.  Use them to make your code more concise and
easier to understand.  Example: sysbus_realize_and_unref().

There are also convenience functions that capture special cases of the
entire general pattern.  Example: sysbus_create_simple().

Hope this helps!

Re: sysbus_create_simple Vs qdev_create

[Pratik Parvati]

[-- Attachment #1: Type: text/plain, Size: 4707 bytes --]

Hi Markus and Philippe,

Thanks for your reply. Now I am pretty clear about Qdev and sysbus helper
function.

Can you please explain to me in brief on buses and device hierarchies (i.e.
BusState and DeviceState) and how they are related to each other? As I can
see, the DeviceState class inherits the BusState

struct DeviceState {
    /*< private >*/
    Object parent_obj;
    /*< public >*/

    const char *id;
    char *canonical_path;
    bool realized;
    bool pending_deleted_event;
    QemuOpts *opts;
    int hotplugged;
    bool allow_unplug_during_migration;
    BusState *parent_bus; \\ BusState is inherited here
    QLIST_HEAD(, NamedGPIOList) gpios;
    QLIST_HEAD(, BusState) child_bus;
    int num_child_bus;
    int instance_id_alias;
    int alias_required_for_version;
    ResettableState reset;
};

and BusState, in turn, inherits the DeviceState as

/**
 * BusState:
 * @hotplug_handler: link to a hotplug handler associated with bus.
 * @reset: ResettableState for the bus; handled by Resettable interface.
 */struct BusState {
    Object obj;
    DeviceState *parent; \\ DeviceState is inherited here
    char *name;
    HotplugHandler *hotplug_handler;
    int max_index;
    bool realized;
    int num_children;
    QTAILQ_HEAD(, BusChild) children;
    QLIST_ENTRY(BusState) sibling;
    ResettableState reset;
};

I am a bit confused. Can you brief me this relation!

Thanks and Regards,
Pratik

On Wed, Jul 15, 2020 at 2:02 PM Markus Armbruster <armbru@redhat.com> wrote:

> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
> > Hi Pratik,
> >
> > On 7/14/20 6:17 PM, Pratik Parvati wrote:
> >> Here is a brief context that might help you.
> >> I am referring hw/arm/versatilepb.c
> >>
> >> The ARM PrimeCell UART (PL011) device created as follows
> >>
> >>     dev = qdev_create(NULL, "pl011");
> >>     s = SYS_BUS_DEVICE(dev);
> >>     qdev_prop_set_chr(dev, "chardev", chr);
> >>     qdev_init_nofail(dev);
> >>     sysbus_mmio_map(s, 0, addr);
> >>     sysbus_connect_irq(s, 0, irq);
>
> This is pl011_create().
>
> Since recent merge commit 6675a653d2e, it's
>
>        dev = qdev_new("pl011");
>        s = SYS_BUS_DEVICE(dev);
>        qdev_prop_set_chr(dev, "chardev", chr);
>        sysbus_realize_and_unref(s, &error_fatal);
>        sysbus_mmio_map(s, 0, addr);
>        sysbus_connect_irq(s, 0, irq);
>
> >>
> >> Whereas the PL031 RTC device is created as
> >>
> >>     /* Add PL031 Real Time Clock. */
> >>     sysbus_create_simple("pl031", 0x101e8000, pic[10]);
> >>
> >> What is the difference between these two devices creation?
> >
> > Both devices inherit SysBusDevice, which itself inherits QDev.
>
> Yes: TYPE_SYS_BUS_DEVICE is a subtype of TYPE_DEVICE.
>
> > You can create QDev objects with the qdev API, and
> > SysBusDevice objects with the sysbus API.
>
> Yes.
>
> qdev_new(), qdev_realize_and_unref(), ... work with DeviceState * (the C
> type of an instance of QOM TYPE_DEVICE).
>
> sysbus_realize_and_unref(), ... work with SysBusDevice * (the C type of
> an instance of QOM TYPE_SYS_BUS_DEVICE).
>
> Since TYPE_SYS_BUS_DEVICE is a subtype of TYPE_DEVICE, you can safely
> use qdev_ functions with sysbus devices.  Example: pl011_create() uses
> qdev_new() to create a sysbus device.  That's fine.
>
> > sysbus_create_simple() is a condensed helper, but only allow you
> > to pass qemu_irq objects, not a 'chardev' property. So for this
> > case you have to use the qdev API instead.
>
> Yes.  It's a helper that combines creating a sysbus device, wiring up
> one MMIO region and one IRQ, and realizing.  If you need to configure or
> wire up more than that, you can't use it.
>
> >> How do I know
> >> which method to use while creating an object?
> >
> > SysBusDevice are plugged onto a bus. QDev aren't.
> > The sysbus API results in smaller code, easier to review.
>
> The general pattern for a stand-alone device is
>
>     dev = qdev_new(type_name);
>     set properties and wire up stuff...
>     qdev_realize_and_unref(dev, bus, &err);
>
> When this is to be done in device code, say to create a component
> device, the split between .instance_init() and .realize() complicates
> things.  If interested, ask and I'll explain.
>
> There are quite a few wrappers around qdev_ functions for various
> subtypes of TYPE_DEVICE.  Use them to make your code more concise and
> easier to understand.  Example: sysbus_realize_and_unref().
>
> There are also convenience functions that capture special cases of the
> entire general pattern.  Example: sysbus_create_simple().
>
> Hope this helps!
>
>

[-- Attachment #2: Type: text/html, Size: 11998 bytes --]

Re: sysbus_create_simple Vs qdev_create

[Peter Maydell]

On Wed, 15 Jul 2020 at 14:59, Pratik Parvati <pratikp@vayavyalabs.com> wrote:
> Can you please explain to me in brief on buses and device hierarchies (i.e. BusState and DeviceState) and how they are related to each other? As I can see, the DeviceState class inherits the BusState
>
> struct DeviceState {
>     /*< private >*/
>     Object parent_obj;
>     /*< public >*/
>
>     const char *id;
>     char *canonical_path;
>     bool realized;
>     bool pending_deleted_event;
>     QemuOpts *opts;
>     int hotplugged;
>     bool allow_unplug_during_migration;
>     BusState *parent_bus; \\ BusState is inherited here

This is not inheritance. The DeviceState has-a BusState parent_bus.
Inheritance is the parent_obj at the top: a DeviceState is-a
Object.

>     QLIST_HEAD(, NamedGPIOList) gpios;
>     QLIST_HEAD(, BusState) child_bus;
>     int num_child_bus;
>     int instance_id_alias;
>     int alias_required_for_version;
>     ResettableState reset;
> };
>
> and BusState, in turn, inherits the DeviceState as
>
> /**
>  * BusState:
>  * @hotplug_handler: link to a hotplug handler associated with bus.
>  * @reset: ResettableState for the bus; handled by Resettable interface.
>  */
> struct BusState {
>     Object obj;
>     DeviceState *parent; \\ DeviceState is inherited here

This isn't inheritance either. A BusState is-a
Object (which is the inheritance for this class),
and it has-a DeviceState parent.

Anyway, the two form a tree: every Device may
be on exactly one Bus (that's the parent_bus link),
and may have one or more child Buses (that's the
child_bus list). Every Bus is owned by exactly
one Device (its parent in the tree), may have
multiple siblings (if its parent has more than
one child bus), and has children (any Devices
which are plugged into the bus). These parent-and-child
links form the qdev or qbus tree. Note that this is
an entirely separate thing from the QOM hierarchy
of parent-and-child object relationships. It is
also entirely separate from the class hierarchy
of classes and subclasses.

thanks
-- PMM

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Pratik Parvati <pratikp@vayavyalabs.com> writes:

> Hi Markus and Philippe,
>
> Thanks for your reply. Now I am pretty clear about Qdev and sysbus helper
> function.
>
> Can you please explain to me in brief on buses and device hierarchies (i.e.
> BusState and DeviceState) and how they are related to each other? As I can
> see, the DeviceState class inherits the BusState
>
> struct DeviceState {
>     /*< private >*/
>     Object parent_obj;
>     /*< public >*/
>
>     const char *id;
>     char *canonical_path;
>     bool realized;
>     bool pending_deleted_event;
>     QemuOpts *opts;
>     int hotplugged;
>     bool allow_unplug_during_migration;
>     BusState *parent_bus; \\ BusState is inherited here
>     QLIST_HEAD(, NamedGPIOList) gpios;
>     QLIST_HEAD(, BusState) child_bus;
>     int num_child_bus;
>     int instance_id_alias;
>     int alias_required_for_version;
>     ResettableState reset;
> };
>
> and BusState, in turn, inherits the DeviceState as
>
> /**
>  * BusState:
>  * @hotplug_handler: link to a hotplug handler associated with bus.
>  * @reset: ResettableState for the bus; handled by Resettable interface.
>  */struct BusState {
>     Object obj;
>     DeviceState *parent; \\ DeviceState is inherited here
>     char *name;
>     HotplugHandler *hotplug_handler;
>     int max_index;
>     bool realized;
>     int num_children;
>     QTAILQ_HEAD(, BusChild) children;
>     QLIST_ENTRY(BusState) sibling;
>     ResettableState reset;
> };
>
> I am a bit confused. Can you brief me this relation!

We sorely lack introductory documentation on both qdev and QOM.  I
believe most developers are more or less confused about them most of the
time.  I've done a bit of work on both, so I'm probably less confused
than average.  I'm cc'ing maintainers in the hope of reducing average
confusion among participants in this thread.

DeviceState does not inherit from BusState, and BusState does not
inherit from DeviceState.  The relation you marked in the code is
actually "has a".

A DeviceState may have a BusState, namely the bus the device is plugged
into.  "May", because some devices are bus-less (their
DEVICE_GET_CLASS(dev)->bus_type is null), and the others get plugged
into their bus only at realize time.

Example: PCI device "pci-serial" plugs into a PCI bus.

Example: device "serial" does not plug into a bus (its used as component
device of "pci-serial" and other devices).

Example: device "pc-dimm" does not plug into a bus.

A bus has a DeviceState, namely the device providing this bus.

Example: device "i440FX-pcihost" provides PCI bus "pci.0".

Both DeviceState and BusState are QOM subtypes of Object.  I prefer to
avoid use of "inherit" for that, because it can mean different things to
different people.

Re: sysbus_create_simple Vs qdev_create

[Eduardo Habkost]

On Wed, Jul 15, 2020 at 04:37:18PM +0200, Markus Armbruster wrote:
> Pratik Parvati <pratikp@vayavyalabs.com> writes:
> 
> > Hi Markus and Philippe,
> >
> > Thanks for your reply. Now I am pretty clear about Qdev and sysbus helper
> > function.
> >
> > Can you please explain to me in brief on buses and device hierarchies (i.e.
> > BusState and DeviceState) and how they are related to each other? As I can
> > see, the DeviceState class inherits the BusState
> >
> > struct DeviceState {
> >     /*< private >*/
> >     Object parent_obj;
> >     /*< public >*/
> >
> >     const char *id;
> >     char *canonical_path;
> >     bool realized;
> >     bool pending_deleted_event;
> >     QemuOpts *opts;
> >     int hotplugged;
> >     bool allow_unplug_during_migration;
> >     BusState *parent_bus; \\ BusState is inherited here
> >     QLIST_HEAD(, NamedGPIOList) gpios;
> >     QLIST_HEAD(, BusState) child_bus;
> >     int num_child_bus;
> >     int instance_id_alias;
> >     int alias_required_for_version;
> >     ResettableState reset;
> > };
> >
> > and BusState, in turn, inherits the DeviceState as
> >
> > /**
> >  * BusState:
> >  * @hotplug_handler: link to a hotplug handler associated with bus.
> >  * @reset: ResettableState for the bus; handled by Resettable interface.
> >  */struct BusState {
> >     Object obj;
> >     DeviceState *parent; \\ DeviceState is inherited here
> >     char *name;
> >     HotplugHandler *hotplug_handler;
> >     int max_index;
> >     bool realized;
> >     int num_children;
> >     QTAILQ_HEAD(, BusChild) children;
> >     QLIST_ENTRY(BusState) sibling;
> >     ResettableState reset;
> > };
> >
> > I am a bit confused. Can you brief me this relation!
> 
> We sorely lack introductory documentation on both qdev and QOM.  I
> believe most developers are more or less confused about them most of the
> time.  I've done a bit of work on both, so I'm probably less confused
> than average.  I'm cc'ing maintainers in the hope of reducing average
> confusion among participants in this thread.
> 
> DeviceState does not inherit from BusState, and BusState does not
> inherit from DeviceState.  The relation you marked in the code is
> actually "has a".
> 
> A DeviceState may have a BusState, namely the bus the device is plugged
> into.  "May", because some devices are bus-less (their
> DEVICE_GET_CLASS(dev)->bus_type is null), and the others get plugged
> into their bus only at realize time.
> 
> Example: PCI device "pci-serial" plugs into a PCI bus.
> 
> Example: device "serial" does not plug into a bus (its used as component
> device of "pci-serial" and other devices).
> 
> Example: device "pc-dimm" does not plug into a bus.
> 
> A bus has a DeviceState, namely the device providing this bus.
> 
> Example: device "i440FX-pcihost" provides PCI bus "pci.0".
> 
> Both DeviceState and BusState are QOM subtypes of Object.  I prefer to
> avoid use of "inherit" for that, because it can mean different things to
> different people.

I'd also note that the use of "parent" in the code is also
ambiguous.  It can mean:

* QOM parent type, i.e. TypeInfo.parent.  Related fields:
  * parent_class members of class structs
  * parent_obj members of object structs
* QOM composition tree parent object, i.e. Object::parent
* qdev device parent bus, i.e. DeviceState::parent_bus
* parent device of of qdev bus, i.e. BusState::parent

-- 
Eduardo

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> I'd also note that the use of "parent" in the code is also
> ambiguous.  It can mean:
>
> * QOM parent type, i.e. TypeInfo.parent.  Related fields:
>   * parent_class members of class structs
>   * parent_obj members of object structs

I hate the use of "parent" and "child" for a super- / subtype relation.

Correcting the terminology there would be short term pain for long term
gain.  Worthwhile?

> * QOM composition tree parent object, i.e. Object::parent
> * qdev device parent bus, i.e. DeviceState::parent_bus
> * parent device of of qdev bus, i.e. BusState::parent

These are tree relations.  Use of "parent" and "child" is perfectly
fine.

Re: sysbus_create_simple Vs qdev_create

[Eduardo Habkost]

On Fri, Jul 17, 2020 at 07:10:57AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > I'd also note that the use of "parent" in the code is also
> > ambiguous.  It can mean:
> >
> > * QOM parent type, i.e. TypeInfo.parent.  Related fields:
> >   * parent_class members of class structs
> >   * parent_obj members of object structs
> 
> I hate the use of "parent" and "child" for a super- / subtype relation.
> 
> Correcting the terminology there would be short term pain for long term
> gain.  Worthwhile?

I don't know.  It looks like the terminology came from GObject.

> 
> > * QOM composition tree parent object, i.e. Object::parent
> > * qdev device parent bus, i.e. DeviceState::parent_bus
> > * parent device of of qdev bus, i.e. BusState::parent
> 
> These are tree relations.  Use of "parent" and "child" is perfectly
> fine.

The terms are fine but still ambiguous, as devices belong to two
separate trees at the same time (the QOM composition tree, and
the qdev tree).

I never understood why we have two separate independent object
trees.

-- 
Eduardo

Re: sysbus_create_simple Vs qdev_create

[Daniel P. Berrangé]

On Fri, Jul 17, 2020 at 12:23:12PM -0400, Eduardo Habkost wrote:
> On Fri, Jul 17, 2020 at 07:10:57AM +0200, Markus Armbruster wrote:
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > 
> > > I'd also note that the use of "parent" in the code is also
> > > ambiguous.  It can mean:
> > >
> > > * QOM parent type, i.e. TypeInfo.parent.  Related fields:
> > >   * parent_class members of class structs
> > >   * parent_obj members of object structs
> > 
> > I hate the use of "parent" and "child" for a super- / subtype relation.
> > 
> > Correcting the terminology there would be short term pain for long term
> > gain.  Worthwhile?
> 
> I don't know.  It looks like the terminology came from GObject.

One day I would love it if we got QOM to actually use GObject, so
from that POV I'd be inclined to stick with the "parent" term.

Personally I've not seen a problem with the term "parent" in
this scenario. The class inheritance metaphor maps reasonably
clearly to a parent/child metaphor. 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: sysbus_create_simple Vs qdev_create

[Peter Maydell]

On Fri, 17 Jul 2020 at 17:32, Daniel P. Berrangé <berrange@redhat.com> wrote:
> Personally I've not seen a problem with the term "parent" in
> this scenario. The class inheritance metaphor maps reasonably
> clearly to a parent/child metaphor.

It's not bad in itself; it's just that it means almost all
of our objects are in three different kinds of parent-child
relationship simultaneously, which is confusing if you're
not used to it...

thanks
-- PMM

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 17 Jul 2020 at 17:32, Daniel P. Berrangé <berrange@redhat.com> wrote:
>> Personally I've not seen a problem with the term "parent" in
>> this scenario. The class inheritance metaphor maps reasonably
>> clearly to a parent/child metaphor.
>
> It's not bad in itself; it's just that it means almost all
> of our objects are in three different kinds of parent-child
> relationship simultaneously, which is confusing if you're
> not used to it...

It's confusing even when you're used to it.

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Jul 17, 2020 at 07:10:57AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > I'd also note that the use of "parent" in the code is also
>> > ambiguous.  It can mean:
>> >
>> > * QOM parent type, i.e. TypeInfo.parent.  Related fields:
>> >   * parent_class members of class structs
>> >   * parent_obj members of object structs
>> 
>> I hate the use of "parent" and "child" for a super- / subtype relation.
>> 
>> Correcting the terminology there would be short term pain for long term
>> gain.  Worthwhile?
>
> I don't know.  It looks like the terminology came from GObject.
>
>> 
>> > * QOM composition tree parent object, i.e. Object::parent
>> > * qdev device parent bus, i.e. DeviceState::parent_bus
>> > * parent device of of qdev bus, i.e. BusState::parent
>> 
>> These are tree relations.  Use of "parent" and "child" is perfectly
>> fine.
>
> The terms are fine but still ambiguous, as devices belong to two
> separate trees at the same time (the QOM composition tree, and
> the qdev tree).
>
> I never understood why we have two separate independent object
> trees.

When we rebased qdev onto QOM, we left the qdev tree alone, we did not
embed it into the QOM composition tree.

The qdev tree edge from bus to device providing the bus is commonly
mirrored in the QOM composition tree: both are QOM objects, and the bus
is commonly a QOM composition child of the device providing it.  I hedge
with "commonly", because nothing enforces this as far as I know.

We do not mirror the edge from device to the bus it's plugged into.  I
believe we could have.  I guess we could mirror it as a link even now
(but note links are not children).

I don't know why the rebase of qdev onto QOM was done that way.  Perhaps
Paolo remembers.

Re: sysbus_create_simple Vs qdev_create

[Eduardo Habkost]

On Mon, Jul 20, 2020 at 09:38:24AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Fri, Jul 17, 2020 at 07:10:57AM +0200, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> 
> >> > I'd also note that the use of "parent" in the code is also
> >> > ambiguous.  It can mean:
> >> >
> >> > * QOM parent type, i.e. TypeInfo.parent.  Related fields:
> >> >   * parent_class members of class structs
> >> >   * parent_obj members of object structs
> >> 
> >> I hate the use of "parent" and "child" for a super- / subtype relation.
> >> 
> >> Correcting the terminology there would be short term pain for long term
> >> gain.  Worthwhile?
> >
> > I don't know.  It looks like the terminology came from GObject.
> >
> >> 
> >> > * QOM composition tree parent object, i.e. Object::parent
> >> > * qdev device parent bus, i.e. DeviceState::parent_bus
> >> > * parent device of of qdev bus, i.e. BusState::parent
> >> 
> >> These are tree relations.  Use of "parent" and "child" is perfectly
> >> fine.
> >
> > The terms are fine but still ambiguous, as devices belong to two
> > separate trees at the same time (the QOM composition tree, and
> > the qdev tree).
> >
> > I never understood why we have two separate independent object
> > trees.
> 
> When we rebased qdev onto QOM, we left the qdev tree alone, we did not
> embed it into the QOM composition tree.
> 
> The qdev tree edge from bus to device providing the bus is commonly
> mirrored in the QOM composition tree: both are QOM objects, and the bus
> is commonly a QOM composition child of the device providing it.  I hedge
> with "commonly", because nothing enforces this as far as I know.
> 
> We do not mirror the edge from device to the bus it's plugged into.  I
> believe we could have.  I guess we could mirror it as a link even now
> (but note links are not children).

They are already mirrored as links, and guess what's the link
name: "child[...]".

> 
> I don't know why the rebase of qdev onto QOM was done that way.  Perhaps
> Paolo remembers.

I'm guessing this is because QOM parent/child relationships
represent ownership, while there's no ownership relationship
between buses and devices.

-- 
Eduardo

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Jul 20, 2020 at 09:38:24AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Fri, Jul 17, 2020 at 07:10:57AM +0200, Markus Armbruster wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >> 
>> >> > I'd also note that the use of "parent" in the code is also
>> >> > ambiguous.  It can mean:
>> >> >
>> >> > * QOM parent type, i.e. TypeInfo.parent.  Related fields:
>> >> >   * parent_class members of class structs
>> >> >   * parent_obj members of object structs
>> >> 
>> >> I hate the use of "parent" and "child" for a super- / subtype relation.
>> >> 
>> >> Correcting the terminology there would be short term pain for long term
>> >> gain.  Worthwhile?
>> >
>> > I don't know.  It looks like the terminology came from GObject.
>> >
>> >> 
>> >> > * QOM composition tree parent object, i.e. Object::parent
>> >> > * qdev device parent bus, i.e. DeviceState::parent_bus
>> >> > * parent device of of qdev bus, i.e. BusState::parent
>> >> 
>> >> These are tree relations.  Use of "parent" and "child" is perfectly
>> >> fine.
>> >
>> > The terms are fine but still ambiguous, as devices belong to two
>> > separate trees at the same time (the QOM composition tree, and
>> > the qdev tree).
>> >
>> > I never understood why we have two separate independent object
>> > trees.
>> 
>> When we rebased qdev onto QOM, we left the qdev tree alone, we did not
>> embed it into the QOM composition tree.
>> 
>> The qdev tree edge from bus to device providing the bus is commonly
>> mirrored in the QOM composition tree: both are QOM objects, and the bus
>> is commonly a QOM composition child of the device providing it.  I hedge
>> with "commonly", because nothing enforces this as far as I know.
>> 
>> We do not mirror the edge from device to the bus it's plugged into.  I
>> believe we could have.  I guess we could mirror it as a link even now
>> (but note links are not children).
>
> They are already mirrored as links, and guess what's the link
> name: "child[...]".

You're right, except for the link name: it's parent_bus.

So the qtree is actually embedded in the QOM graph: it's the device and
bus nodes connected by the child edges from device to provided bus and
parent_bus link egdes from device to bus it's plugged into, except the
latter are backward rather than forward.

Strange: even bus-less devices have this parent_bus link, and its value
is "" (the underlying pointer is null, and null gets mapped to "", for
better or worse).

Should the property be limited to devices that actually have a parent
bus?

>> I don't know why the rebase of qdev onto QOM was done that way.  Perhaps
>> Paolo remembers.
>
> I'm guessing this is because QOM parent/child relationships
> represent ownership, while there's no ownership relationship
> between buses and devices.

Plausible.  I guess the separate qtree was kept even though it's
redundant with the QOM graph because switching its users to the QOM
graph would be work.

Re: sysbus_create_simple Vs qdev_create

[Paolo Bonzini]

On 21/07/20 08:00, Markus Armbruster wrote:
>> They are already mirrored as links, and guess what's the link
>> name: "child[...]".
> You're right, except for the link name: it's parent_bus.

There's links in both directions.

> So the qtree is actually embedded in the QOM graph: it's the device and
> bus nodes connected by the child edges from device to provided bus and
> parent_bus link egdes from device to bus it's plugged into, except the
> latter are backward rather than forward.
> 
> Strange: even bus-less devices have this parent_bus link, and its value
> is "" (the underlying pointer is null, and null gets mapped to "", for
> better or worse).
> 
> Should the property be limited to devices that actually have a parent
> bus?

Yes, it could be done.

>>> I don't know why the rebase of qdev onto QOM was done that way.  Perhaps
>>> Paolo remembers.
>> I'm guessing this is because QOM parent/child relationships
>> represent ownership, while there's no ownership relationship
>> between buses and devices.
>
> Plausible.  I guess the separate qtree was kept even though it's
> redundant with the QOM graph because switching its users to the QOM
> graph would be work.

No, it was kept because:

1) the QOM graph wasn't embedding the qdev tree at the time.  That was
added later.

2) the composition tree generally mirrors things that are born and die
at the same time, and creating children is generally reserved to the
object itself.  Children are usually embedded directly in a struct, for
example.  Instead, peripherals are not created by the bus, they are
created by the device_add monitor command and the like.

3) accessing the QOM graph is slow (it requires hash table lookups,
string comparisons and all that), so the pointers that cache the
parent-child links are needed for use in hot paths.

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 21/07/20 08:00, Markus Armbruster wrote:
>>> They are already mirrored as links, and guess what's the link
>>> name: "child[...]".
>> You're right, except for the link name: it's parent_bus.
>
> There's links in both directions.
>
>> So the qtree is actually embedded in the QOM graph: it's the device and
>> bus nodes connected by the child edges from device to provided bus and
>> parent_bus link egdes from device to bus it's plugged into, except the
>> latter are backward rather than forward.
>> 
>> Strange: even bus-less devices have this parent_bus link, and its value
>> is "" (the underlying pointer is null, and null gets mapped to "", for
>> better or worse).
>> 
>> Should the property be limited to devices that actually have a parent
>> bus?
>
> Yes, it could be done.
>
>>>> I don't know why the rebase of qdev onto QOM was done that way.  Perhaps
>>>> Paolo remembers.
>>> I'm guessing this is because QOM parent/child relationships
>>> represent ownership, while there's no ownership relationship
>>> between buses and devices.
>>
>> Plausible.  I guess the separate qtree was kept even though it's
>> redundant with the QOM graph because switching its users to the QOM
>> graph would be work.
>
> No, it was kept because:
>
> 1) the QOM graph wasn't embedding the qdev tree at the time.  That was
> added later.
>
> 2) the composition tree generally mirrors things that are born and die
> at the same time, and creating children is generally reserved to the
> object itself.

Yes.  Notable exceptions: containers /machine/peripheral,
/machine/peripheral-anon, /machine/unattached.

>                 Children are usually embedded directly in a struct, for
> example.

We sometimes use object_new() + object_property_add_child() instead.
Extra indirection.  I guess we'd be better off without the extra
indirection most of the time.  Implementation detail.

We sometimes use object_new() without object_property_add_child(), and
have qdev_realize() put the device in the /machine/unattached orphanage.
Meh.  I guess the orphanage feature exists to make conversion to QOM
slightly easier.  Could we ban its use for new boards at least?

>           Instead, peripherals are not created by the bus, they are
> created by the device_add monitor command and the like.

Plugged devices (the ones created with -device / device_add) have
/machine/peripheral or /machine/peripheral-anon as QOM parent.

> 3) accessing the QOM graph is slow (it requires hash table lookups,
> string comparisons and all that), so the pointers that cache the
> parent-child links are needed for use in hot paths.

True, but only because QOM's design opts for generality, efficiency be
damned :)

The QOM graph's edges are properties.

The child edges form the QOM composition tree.  The property contains
the child in ObjectProperty member @opaque.  To go from parent to child,
you have to get the property by name (hash table lookup), then follow
@opaque.  Except that would be too simple (and way too efficient), so we
convert pointers to QOM path names and back with visitors for accessing
a single child, or iterate over all properties (hash table iteration)
for accessing all children.

There is also a pointer from child back to parent, which is not a
property.  That one is actually fast and easy to use from C.

The remaining QOM graph edges are link edges.  Compared to child edges,
there's a further indirection through the LinkProperty struct, and no
support for iterating over link edge targets.

This design supports adding arbitrary children and links at run time,
with the actual object none the wiser.

A less general and more efficient design would put pointers right into
the objects' structs, then wrap what we call class properties around the
pointers.  C code for specific objects could then simply follow the
pointers.  Generic code could still use properties.  If efficiency
matters there, we could avoid the detour through path names.

Properties backed by a (pointer-valued) struct member are less general,
of course: you can't add this kind of property without first adding the
struct member.  The property needs to be declared at compile time rather
than built at run time.

I never quite understood why we need to build properties at run time.
It's makes reasoning about properties harder, and introspection brittle.

Re: sysbus_create_simple Vs qdev_create

[Paolo Bonzini]

On 28/07/20 09:19, Markus Armbruster wrote:
>> the composition tree generally mirrors things that are born and die
>> at the same time, and creating children is generally reserved to the
>> object itself.
>
> Yes.  Notable exceptions: containers /machine/peripheral,
> /machine/peripheral-anon, /machine/unattached.

And /objects too.  Apart from /machine/unattached, all these dynamic
objects are created by the monitor or the command line.

>>                 Children are usually embedded directly in a struct, for
>> example.
> 
> We sometimes use object_new() + object_property_add_child() instead.
> Extra indirection.  I guess we'd be better off without the extra
> indirection most of the time.  Implementation detail.
> 
> We sometimes use object_new() without object_property_add_child(), and
> have qdev_realize() put the device in the /machine/unattached orphanage.
> Meh.  I guess the orphanage feature exists to make conversion to QOM
> slightly easier.  Could we ban its use for new boards at least?

Banning perhaps is too strong, but yes /machine/unattached is an
anti-pattern.

>> 3) accessing the QOM graph is slow (it requires hash table lookups,
>> string comparisons and all that), so the pointers that cache the
>> parent-child links are needed for use in hot paths.
> 
> True, but only because QOM's design opts for generality, efficiency be
> damned :)

Remember that QOM's essential feature is the visitors: unlike GObject,
QOM is not targeted at programming languages but rather at CLI and RPC.

> I never quite understood why we need to build properties at run time.

I think it was (for once) Anthony reasoning that good is better than
perfect.  You do need run-time properties for /machine/unattached,
/machine/peripheral, etc., so he decided to only have run-time
properties.  Introspection wasn't considered a primary design goal.

Also, even though child properties are quite often the same for all
objects after initialization (and possibly realization) is complete,
this does not cover in two cases:

1) before the corresponding objects are created---so static child
properties would only be possible if creation of all children is moved
to instance_init, which is guaranteed not to fail.

2) there are cases in which a property (e.g. an int) governs how many of
those children exist, so you cannot create all children in instance_init.

Paolo

> It's makes reasoning about properties harder, and introspection brittle.

Re: sysbus_create_simple Vs qdev_create

[Eduardo Habkost]

On Tue, Jul 28, 2020 at 07:38:27PM +0200, Paolo Bonzini wrote:
> On 28/07/20 09:19, Markus Armbruster wrote:
> >> the composition tree generally mirrors things that are born and die
> >> at the same time, and creating children is generally reserved to the
> >> object itself.
> >
> > Yes.  Notable exceptions: containers /machine/peripheral,
> > /machine/peripheral-anon, /machine/unattached.
> 
> And /objects too.  Apart from /machine/unattached, all these dynamic
> objects are created by the monitor or the command line.
> 
> >>                 Children are usually embedded directly in a struct, for
> >> example.
> > 
> > We sometimes use object_new() + object_property_add_child() instead.
> > Extra indirection.  I guess we'd be better off without the extra
> > indirection most of the time.  Implementation detail.
> > 
> > We sometimes use object_new() without object_property_add_child(), and
> > have qdev_realize() put the device in the /machine/unattached orphanage.
> > Meh.  I guess the orphanage feature exists to make conversion to QOM
> > slightly easier.  Could we ban its use for new boards at least?
> 
> Banning perhaps is too strong, but yes /machine/unattached is an
> anti-pattern.
> 
> >> 3) accessing the QOM graph is slow (it requires hash table lookups,
> >> string comparisons and all that), so the pointers that cache the
> >> parent-child links are needed for use in hot paths.
> > 
> > True, but only because QOM's design opts for generality, efficiency be
> > damned :)
> 
> Remember that QOM's essential feature is the visitors: unlike GObject,
> QOM is not targeted at programming languages but rather at CLI and RPC.

This is surprising to me.  I never thought QOM was targeted at
the CLI or RPC.  (Every single property mentioned in this message
don't seem to be related to the CLI or RPC.)

About the visitors: I always had the impression that usage of
visitors inside QOM is unnecessary and avoidable (compared to
QAPI, where the visitors are an essential feature).


> 
> > I never quite understood why we need to build properties at run time.
> 
> I think it was (for once) Anthony reasoning that good is better than
> perfect.  You do need run-time properties for /machine/unattached,
> /machine/peripheral, etc., so he decided to only have run-time
> properties.  Introspection wasn't considered a primary design goal.
> 
> Also, even though child properties are quite often the same for all
> objects after initialization (and possibly realization) is complete,
> this does not cover in two cases:
> 
> 1) before the corresponding objects are created---so static child
> properties would only be possible if creation of all children is moved
> to instance_init, which is guaranteed not to fail.
> 
> 2) there are cases in which a property (e.g. an int) governs how many of
> those children exist, so you cannot create all children in instance_init.

Do we really need need QOM children to be accessible using the QOM
property API?

Using the same code for both user-configurable properties and for
the list of children of a QOM object might have saved some time
years ago, but I'm not sure this is still a necessary or useful
abstraction.

-- 
Eduardo

Re: sysbus_create_simple Vs qdev_create

[Paolo Bonzini]

On 29/07/20 00:47, Eduardo Habkost wrote:
> On Tue, Jul 28, 2020 at 07:38:27PM +0200, Paolo Bonzini wrote:
>> On 28/07/20 09:19, Markus Armbruster wrote:
>>>> the composition tree generally mirrors things that are born and die
>>>> at the same time, and creating children is generally reserved to the
>>>> object itself.
>>>
>>> Yes.  Notable exceptions: containers /machine/peripheral,
>>> /machine/peripheral-anon, /machine/unattached.
>>
>> And /objects too.  Apart from /machine/unattached, all these dynamic
>> objects are created by the monitor or the command line.
>>
>>>>                 Children are usually embedded directly in a struct, for
>>>> example.
>>>
>>> We sometimes use object_new() + object_property_add_child() instead.
>>> Extra indirection.  I guess we'd be better off without the extra
>>> indirection most of the time.  Implementation detail.
>>>
>>> We sometimes use object_new() without object_property_add_child(), and
>>> have qdev_realize() put the device in the /machine/unattached orphanage.
>>> Meh.  I guess the orphanage feature exists to make conversion to QOM
>>> slightly easier.  Could we ban its use for new boards at least?
>>
>> Banning perhaps is too strong, but yes /machine/unattached is an
>> anti-pattern.
>>
>>>> 3) accessing the QOM graph is slow (it requires hash table lookups,
>>>> string comparisons and all that), so the pointers that cache the
>>>> parent-child links are needed for use in hot paths.
>>>
>>> True, but only because QOM's design opts for generality, efficiency be
>>> damned :)
>>
>> Remember that QOM's essential feature is the visitors: unlike GObject,
>> QOM is not targeted at programming languages but rather at CLI and RPC.
> 
> This is surprising to me.  I never thought QOM was targeted at
> the CLI or RPC.  (Every single property mentioned in this message
> don't seem to be related to the CLI or RPC.)

See https://www.mail-archive.com/qemu-devel@nongnu.org/msg674110.html
for an explanation.

> About the visitors: I always had the impression that usage of
> visitors inside QOM is unnecessary and avoidable (compared to
> QAPI, where the visitors are an essential feature).

But as I explained in that other message, the main difference between
QOM and something like GObject is eactly the QAPI integration, and that
is where CLI and RPC enter the game: for example the possibility to
share code between -object and HMP object_add on one side and QMP
object-add on the other side.

Even code riddled by backwards-compatibility special cases, such as
-accel and -machine, can share code between themselves and -object to
some extent; this is thanks to functions such as object_property_parse,
whose parsing is deferred to visitors and hence to QAPI.

> Do we really need need QOM children to be accessible using the QOM
> property API?
> 
> Using the same code for both user-configurable properties and for
> the list of children of a QOM object might have saved some time
> years ago, but I'm not sure this is still a necessary or useful
> abstraction.

The main thing we get from it is that the QOM paths treat children and
links the same, and links are properties.  To be honest it's not a
feature that is very much developed, so perhaps we can remove it but we
need to evaluate the impact of losing it.

Paolo

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 29/07/20 00:47, Eduardo Habkost wrote:
>> On Tue, Jul 28, 2020 at 07:38:27PM +0200, Paolo Bonzini wrote:
>>> On 28/07/20 09:19, Markus Armbruster wrote:
>>>>> the composition tree generally mirrors things that are born and die
>>>>> at the same time, and creating children is generally reserved to the
>>>>> object itself.
>>>>
>>>> Yes.  Notable exceptions: containers /machine/peripheral,
>>>> /machine/peripheral-anon, /machine/unattached.
>>>
>>> And /objects too.  Apart from /machine/unattached, all these dynamic
>>> objects are created by the monitor or the command line.
>>>
>>>>>                 Children are usually embedded directly in a struct, for
>>>>> example.
>>>>
>>>> We sometimes use object_new() + object_property_add_child() instead.
>>>> Extra indirection.  I guess we'd be better off without the extra
>>>> indirection most of the time.  Implementation detail.
>>>>
>>>> We sometimes use object_new() without object_property_add_child(), and
>>>> have qdev_realize() put the device in the /machine/unattached orphanage.
>>>> Meh.  I guess the orphanage feature exists to make conversion to QOM
>>>> slightly easier.  Could we ban its use for new boards at least?
>>>
>>> Banning perhaps is too strong, but yes /machine/unattached is an
>>> anti-pattern.
>>>
>>>>> 3) accessing the QOM graph is slow (it requires hash table lookups,
>>>>> string comparisons and all that), so the pointers that cache the
>>>>> parent-child links are needed for use in hot paths.
>>>>
>>>> True, but only because QOM's design opts for generality, efficiency be
>>>> damned :)
>>>
>>> Remember that QOM's essential feature is the visitors: unlike GObject,
>>> QOM is not targeted at programming languages but rather at CLI and RPC.
>> 
>> This is surprising to me.  I never thought QOM was targeted at
>> the CLI or RPC.  (Every single property mentioned in this message
>> don't seem to be related to the CLI or RPC.)
>
> See https://www.mail-archive.com/qemu-devel@nongnu.org/msg674110.html
> for an explanation.
>
>> About the visitors: I always had the impression that usage of
>> visitors inside QOM is unnecessary and avoidable (compared to
>> QAPI, where the visitors are an essential feature).
>
> But as I explained in that other message, the main difference between
> QOM and something like GObject is eactly the QAPI integration, and that
> is where CLI and RPC enter the game: for example the possibility to
> share code between -object and HMP object_add on one side and QMP
> object-add on the other side.
>
> Even code riddled by backwards-compatibility special cases, such as
> -accel and -machine, can share code between themselves and -object to
> some extent; this is thanks to functions such as object_property_parse,
> whose parsing is deferred to visitors and hence to QAPI.

QOM relies on QAPI visitors to access properties.  There is no
integration with the QAPI schema.

Going through a visitor enables property access from QMP, HMP and CLI.

Access from C *also* goes through a visitor.  We typically go from C
type to QObject and back.  Comically inefficient (which hardly matters),
verbose to use and somewhat hard to understand (which does).

Compare to what QOM replaced: qdev.  Properties are a layer on top of
ordinary C.  From C, you can either use the C layer (struct members,
basically), or the property layer for C (functions taking C types, no
conversion to string and back under the hood), or the "text" layer
(parse from text / format to text).

My point is not that qdev was great and QOM is terrible.  There are
reasons we replaced qdev with QOM.  My point is QOM doesn't *have* to be
the way it is.  It is the way it is because we made it so.

>> Do we really need need QOM children to be accessible using the QOM
>> property API?
>> 
>> Using the same code for both user-configurable properties and for
>> the list of children of a QOM object might have saved some time
>> years ago, but I'm not sure this is still a necessary or useful
>> abstraction.
>
> The main thing we get from it is that the QOM paths treat children and
> links the same, and links are properties.  To be honest it's not a
> feature that is very much developed, so perhaps we can remove it but we
> need to evaluate the impact of losing it.

I've long had the nagging feeling that if we had special-cased
containers, children and links, we could have made a QOM that was easier
to reason about, and much easier to integrate with a QAPI schema.

Re: sysbus_create_simple Vs qdev_create

[Paolo Bonzini]

On 29/07/20 15:18, Markus Armbruster wrote:
>> Even code riddled by backwards-compatibility special cases, such as
>> -accel and -machine, can share code between themselves and -object to
>> some extent; this is thanks to functions such as object_property_parse,
>> whose parsing is deferred to visitors and hence to QAPI.
>
> QOM relies on QAPI visitors to access properties.  There is no
> integration with the QAPI schema.

Indeed it doesn't use _all_ of the QAPI goodies.  It does use visitors
and it's a major feature of QOM.

> Going through a visitor enables property access from QMP, HMP and CLI.
> 
> Access from C *also* goes through a visitor.  We typically go from C
> type to QObject and back.  Comically inefficient (which hardly matters),
> verbose to use and somewhat hard to understand (which does).

It's verbose in the getters/setters, but we have wrappers such as
object_property_set_str, object_property_set_bool etc. that do not make
it too hard to understand.

> Compare to what QOM replaced: qdev.  Properties are a layer on top of
> ordinary C.  From C, you can either use the C layer (struct members,
> basically), or the property layer for C (functions taking C types, no
> conversion to string and back under the hood), or the "text" layer
> (parse from text / format to text).
> 
> My point is not that qdev was great and QOM is terrible.  There are
> reasons we replaced qdev with QOM.  My point is QOM doesn't *have* to be
> the way it is.  It is the way it is because we made it so.

QOM didn't only replace qdev: it also removed the need to have a command
line option du jour for any new concept, e.g. all the TLS stuff, RNG
backends, RAM backends, etc.

It didn't succeed (at all) in deprecating chardev/netdev/device etc.,
but this is a very underappreciated part of QOM, and this is why I think
it's appropriate to say QOM is "C with classes and CLI/RPC
serialization", as opposed for example to "C with classes and multi
programming language interface" that is GObject.

> I've long had the nagging feeling that if we had special-cased
> containers, children and links, we could have made a QOM that was easier
> to reason about, and much easier to integrate with a QAPI schema.

That's at least plausible.  But I have a nagging feeling that it would
only cover 99% of what we're doing with QOM. :)

Paolo

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 29/07/20 15:18, Markus Armbruster wrote:
>>> Even code riddled by backwards-compatibility special cases, such as
>>> -accel and -machine, can share code between themselves and -object to
>>> some extent; this is thanks to functions such as object_property_parse,
>>> whose parsing is deferred to visitors and hence to QAPI.
>>
>> QOM relies on QAPI visitors to access properties.  There is no
>> integration with the QAPI schema.
>
> Indeed it doesn't use _all_ of the QAPI goodies.  It does use visitors
> and it's a major feature of QOM.

No argument.

>> Going through a visitor enables property access from QMP, HMP and CLI.
>> 
>> Access from C *also* goes through a visitor.  We typically go from C
>> type to QObject and back.  Comically inefficient (which hardly matters),
>> verbose to use and somewhat hard to understand (which does).
>
> It's verbose in the getters/setters, but we have wrappers such as
> object_property_set_str, object_property_set_bool etc. that do not make
> it too hard to understand.

qdev C layer:

    frob->prop = 42;

Least cognitive load.

QOM has no C layer.

qdev property layer works even when @frob has incomplete type:

    qdev_prop_set_int32(DEVICE(frob), "prop", 42);

This used to map property name to struct offset & copy the value.
Simple, stupid.

Nowadays, it is the same as

    object_property_set_int(OBJECT(frob), "frob", 42, &error_abort);

which first converts the int to a QObject, then uses a QObject input
visitor with a virtual walk to convert it back to int and store it in
@frob.  It's quite a sight in the debugger.

qdev "text" layer is really a QemuOpts layer (because that's what we had
back then).  If we have prop=42 in a QemuOpts, it calls

    set_property("prop", "42", frob, &err);

Nowadays, this is a thin wrapper around object_property_parse(),
basically

    object_property_parse(frob, "prop", 42, &err);

Fine print: except set_property() does nothing for @prop "driver" and
"bus", which look just like properties in -device / device-add, but
aren't.

object_property_parse() uses the string input visitor, which I loathe.

>> Compare to what QOM replaced: qdev.  Properties are a layer on top of
>> ordinary C.  From C, you can either use the C layer (struct members,
>> basically), or the property layer for C (functions taking C types, no
>> conversion to string and back under the hood), or the "text" layer
>> (parse from text / format to text).
>> 
>> My point is not that qdev was great and QOM is terrible.  There are
>> reasons we replaced qdev with QOM.  My point is QOM doesn't *have* to be
>> the way it is.  It is the way it is because we made it so.
>
> QOM didn't only replace qdev: it also removed the need to have a command
> line option du jour for any new concept, e.g. all the TLS stuff, RNG
> backends, RAM backends, etc.

Yes.  There are good reasons for QOM.

> It didn't succeed (at all) in deprecating chardev/netdev/device etc.,
> but this is a very underappreciated part of QOM, and this is why I think
> it's appropriate to say QOM is "C with classes and CLI/RPC
> serialization", as opposed for example to "C with classes and multi
> programming language interface" that is GObject.

That's fair.

>> I've long had the nagging feeling that if we had special-cased
>> containers, children and links, we could have made a QOM that was easier
>> to reason about, and much easier to integrate with a QAPI schema.
>
> That's at least plausible.  But I have a nagging feeling that it would
> only cover 99% of what we're doing with QOM. :)

The question is whether that 1% really should be done the way it is done
:)

Re: sysbus_create_simple Vs qdev_create

[Paolo Bonzini]

On 30/07/20 12:03, Markus Armbruster wrote:
> qdev C layer:
> 
>     frob->prop = 42;
> 
> Least cognitive load.
> 
> QOM has no C layer.

Not really, a QOM object is totally free to do frob->prop = 42.  And
just like we didn't do that outside device implementation in qdev as our
tithe to the Church of Information Hiding; the same applies to QOM.

> qdev property layer works even when @frob has incomplete type:
> 
>     qdev_prop_set_int32(DEVICE(frob), "prop", 42);
> 
> This used to map property name to struct offset & copy the value.
> Simple, stupid.
> 
> Nowadays, it is the same as
> 
>     object_property_set_int(OBJECT(frob), "frob", 42, &error_abort);
> 
> which first converts the int to a QObject, then uses a QObject input
> visitor with a virtual walk to convert it back to int and store it in
> @frob.  It's quite a sight in the debugger.

Yes, but thatt's just because we never bothered to create single-type
visitors.  For a good reason though: I don't think the extra QAPI code
is worth (not even that much) nicer backtraces when we already have a
QObject as a battle-tested variant type.

> qdev "text" layer is really a QemuOpts layer (because that's what we had
> back then).  If we have prop=42 in a QemuOpts, it calls
> 
>     set_property("prop", "42", frob, &err);
> 
> Nowadays, this is a thin wrapper around object_property_parse(),
> basically
> 
>     object_property_parse(frob, "prop", 42, &err);
> 
> Fine print: except set_property() does nothing for @prop "driver" and
> "bus", which look just like properties in -device / device-add, but
> aren't.

Ugly indeed.  They should be special cased up in the caller, probably,
or use the long-discussed "remainder" feature of the QAPI schema.

> object_property_parse() uses the string input visitor, which I loathe.

Apart from the list syntax, the string input visitor is decent I think.

>>> I've long had the nagging feeling that if we had special-cased
>>> containers, children and links, we could have made a QOM that was easier
>>> to reason about, and much easier to integrate with a QAPI schema.
>>
>> That's at least plausible.  But I have a nagging feeling that it would
>> only cover 99% of what we're doing with QOM. :)
> 
> The question is whether that 1% really should be done the way it is done
> :)

And that's a very fair question, but it implies non-trivial design work,
so the smiley changes to a frown. :(

Paolo

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 30/07/20 12:03, Markus Armbruster wrote:
>> qdev C layer:
>> 
>>     frob->prop = 42;
>> 
>> Least cognitive load.
>> 
>> QOM has no C layer.
>
> Not really, a QOM object is totally free to do frob->prop = 42.  And
> just like we didn't do that outside device implementation in qdev as our
> tithe to the Church of Information Hiding; the same applies to QOM.

I screwed up the part of my argument that actually has a hope to be
valid, let me try again.

With qdev, you can always do frob->prop = 42, because properties are
always backed by a struct member.

With QOM, properties are built around visitor-based getters and setters.
This means you can always do (but never actually would do) something
like

    fortytwo = qnum_from_int(42);
    v = qobject_input_visitor_new(fortytwo);
    set_prop(OBJECT(frob), v, "prop", cookie, &err);
    visit_free(v);
    qobject_unref(fortytwo);

where set_prop() is the setter you passed to object_property_add(), and
@cookie is the opaque value you passed along with it.

*Maybe* set_prop() wraps around a simpler setter you can call directly,
or even a struct member you can set directy.  QOM does not care.

And that's my point: QOM does not care for the C layer.

>> qdev property layer works even when @frob has incomplete type:
>> 
>>     qdev_prop_set_int32(DEVICE(frob), "prop", 42);
>> 
>> This used to map property name to struct offset & copy the value.
>> Simple, stupid.
>> 
>> Nowadays, it is the same as
>> 
>>     object_property_set_int(OBJECT(frob), "frob", 42, &error_abort);
>> 
>> which first converts the int to a QObject, then uses a QObject input
>> visitor with a virtual walk to convert it back to int and store it in
>> @frob.  It's quite a sight in the debugger.
>
> Yes, but thatt's just because we never bothered to create single-type
> visitors.  For a good reason though: I don't think the extra QAPI code
> is worth (not even that much) nicer backtraces when we already have a
> QObject as a battle-tested variant type.
>
>> qdev "text" layer is really a QemuOpts layer (because that's what we had
>> back then).  If we have prop=42 in a QemuOpts, it calls
>> 
>>     set_property("prop", "42", frob, &err);
>> 
>> Nowadays, this is a thin wrapper around object_property_parse(),
>> basically
>> 
>>     object_property_parse(frob, "prop", 42, &err);
>> 
>> Fine print: except set_property() does nothing for @prop "driver" and
>> "bus", which look just like properties in -device / device-add, but
>> aren't.
>
> Ugly indeed.  They should be special cased up in the caller, probably,
> or use the long-discussed "remainder" feature of the QAPI schema.

qdev_device_add() is still stuck in the QemuOpts age.

>> object_property_parse() uses the string input visitor, which I loathe.
>
> Apart from the list syntax, the string input visitor is decent I think.

It's a death trap:

/*
 * The string input visitor does not implement support for visiting
 * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
 * of integers (except type "size") are supported.
 */

"Does not implement support for visiting" is polite language for
"crashes when you dare to visit".

>>>> I've long had the nagging feeling that if we had special-cased
>>>> containers, children and links, we could have made a QOM that was easier
>>>> to reason about, and much easier to integrate with a QAPI schema.
>>>
>>> That's at least plausible.  But I have a nagging feeling that it would
>>> only cover 99% of what we're doing with QOM. :)
>> 
>> The question is whether that 1% really should be done the way it is done
>> :)
>
> And that's a very fair question, but it implies non-trivial design work,
> so the smiley changes to a frown. :(

True!

Re: sysbus_create_simple Vs qdev_create

[Paolo Bonzini]

On 30/07/20 14:36, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 30/07/20 12:03, Markus Armbruster wrote:
>>> qdev C layer:
>>>
>>>     frob->prop = 42;
>>>
>>> Least cognitive load.
>>>
>>> QOM has no C layer.
>>
>> Not really, a QOM object is totally free to do frob->prop = 42.  And
>> just like we didn't do that outside device implementation in qdev as our
>> tithe to the Church of Information Hiding; the same applies to QOM.
> 
> I screwed up the part of my argument that actually has a hope to be
> valid, let me try again.
> 
> With qdev, you can always do frob->prop = 42, because properties are
> always backed by a struct member.
> 
> With QOM, properties are built around visitor-based getters and setters.
> This means you can always do (but never actually would do) something
> like
> 
>     fortytwo = qnum_from_int(42);
>     v = qobject_input_visitor_new(fortytwo);
>     set_prop(OBJECT(frob), v, "prop", cookie, &err);
>     visit_free(v);
>     qobject_unref(fortytwo);
> 
> where set_prop() is the setter you passed to object_property_add(), and
> @cookie is the opaque value you passed along with it.
> 
> *Maybe* set_prop() wraps around a simpler setter you can call directly,
> or even a struct member you can set directy.  QOM does not care.
> 
> And that's my point: QOM does not care for the C layer.

Ok, I think I got it.  In practice it depends on how much you want to
hide the implementation of the properties and (especially for
set-only-before-realize properties) the answer will be "not at all
inside the device implementation".  So inside the implementation you can
always break the information hiding layer if needed (usually for
performance or as you point out sanity), and I don't think there is a
substantial difference between qdev and QOM.

But yeah, I'm willing to concede that QOM the QAPI-- layer of QOM comes
first and the C layer comes second. :)

Paolo

Re: sysbus_create_simple Vs qdev_create

[Eduardo Habkost]

On Wed, Jul 29, 2020 at 11:54:35AM +0200, Paolo Bonzini wrote:
> On 29/07/20 00:47, Eduardo Habkost wrote:
[...]
> > Do we really need need QOM children to be accessible using the QOM
> > property API?
> > 
> > Using the same code for both user-configurable properties and for
> > the list of children of a QOM object might have saved some time
> > years ago, but I'm not sure this is still a necessary or useful
> > abstraction.
> 
> The main thing we get from it is that the QOM paths treat children and
> links the same, and links are properties.  To be honest it's not a
> feature that is very much developed, so perhaps we can remove it but we
> need to evaluate the impact of losing it.

Are link properties usable by -device/device_add/-object/object-add?

-- 
Eduardo

Re: sysbus_create_simple Vs qdev_create

[Paolo Bonzini]

On 29/07/20 16:32, Eduardo Habkost wrote:
> On Wed, Jul 29, 2020 at 11:54:35AM +0200, Paolo Bonzini wrote:
>> On 29/07/20 00:47, Eduardo Habkost wrote:
> [...]
>>> Do we really need need QOM children to be accessible using the QOM
>>> property API?
>>>
>>> Using the same code for both user-configurable properties and for
>>> the list of children of a QOM object might have saved some time
>>> years ago, but I'm not sure this is still a necessary or useful
>>> abstraction.
>>
>> The main thing we get from it is that the QOM paths treat children and
>> links the same, and links are properties.  To be honest it's not a
>> feature that is very much developed, so perhaps we can remove it but we
>> need to evaluate the impact of losing it.
> 
> Are link properties usable by -device/device_add/-object/object-add?

Not sure exactly what you mean, but there is DEFINE_PROP_LINK and it's
used to link devices to objects.  Is it ever used with an actual path
rather than just the id of something in /objects?  Probably not.

Paolo

Re: sysbus_create_simple Vs qdev_create

[Eduardo Habkost]

On Wed, Jul 29, 2020 at 06:01:31PM +0200, Paolo Bonzini wrote:
> On 29/07/20 16:32, Eduardo Habkost wrote:
> > On Wed, Jul 29, 2020 at 11:54:35AM +0200, Paolo Bonzini wrote:
> >> On 29/07/20 00:47, Eduardo Habkost wrote:
> > [...]
> >>> Do we really need need QOM children to be accessible using the QOM
> >>> property API?
> >>>
> >>> Using the same code for both user-configurable properties and for
> >>> the list of children of a QOM object might have saved some time
> >>> years ago, but I'm not sure this is still a necessary or useful
> >>> abstraction.
> >>
> >> The main thing we get from it is that the QOM paths treat children and
> >> links the same, and links are properties.  To be honest it's not a
> >> feature that is very much developed, so perhaps we can remove it but we
> >> need to evaluate the impact of losing it.
> > 
> > Are link properties usable by -device/device_add/-object/object-add?
> 
> Not sure exactly what you mean, but there is DEFINE_PROP_LINK and it's
> used to link devices to objects.  Is it ever used with an actual path
> rather than just the id of something in /objects?  Probably not.

I mean: are link properties settable from the command line or
QMP, as an argument to -device/device_add/-object/object-add?

-- 
Eduardo

Re: sysbus_create_simple Vs qdev_create

[Paolo Bonzini]

On 29/07/20 18:08, Eduardo Habkost wrote:
>>>> The main thing we get from it is that the QOM paths treat children and
>>>> links the same, and links are properties.  To be honest it's not a
>>>> feature that is very much developed, so perhaps we can remove it but we
>>>> need to evaluate the impact of losing it.
>>> Are link properties usable by -device/device_add/-object/object-add?
>> Not sure exactly what you mean, but there is DEFINE_PROP_LINK and it's
>> used to link devices to objects.  Is it ever used with an actual path
>> rather than just the id of something in /objects?  Probably not.
> I mean: are link properties settable from the command line or
> QMP, as an argument to -device/device_add/-object/object-add?

Then yes.

Paolo

Re: sysbus_create_simple Vs qdev_create

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/07/20 09:19, Markus Armbruster wrote:
>>> the composition tree generally mirrors things that are born and die
>>> at the same time, and creating children is generally reserved to the
>>> object itself.
>>
>> Yes.  Notable exceptions: containers /machine/peripheral,
>> /machine/peripheral-anon, /machine/unattached.
>
> And /objects too.  Apart from /machine/unattached, all these dynamic
> objects are created by the monitor or the command line.
>
>>>                 Children are usually embedded directly in a struct, for
>>> example.
>> 
>> We sometimes use object_new() + object_property_add_child() instead.
>> Extra indirection.  I guess we'd be better off without the extra
>> indirection most of the time.  Implementation detail.
>> 
>> We sometimes use object_new() without object_property_add_child(), and
>> have qdev_realize() put the device in the /machine/unattached orphanage.
>> Meh.  I guess the orphanage feature exists to make conversion to QOM
>> slightly easier.  Could we ban its use for new boards at least?
>
> Banning perhaps is too strong, but yes /machine/unattached is an
> anti-pattern.

A ban backed by an automated test will be effective.  Writing "but don't
do that" on the list not so much.

The automated test could have a list of exceptions.  Adding a new
instance of the anti-pattern then isn't impossible (you can update the
list of exceptions), but clearly visible to patch submitter and
reviewers.

[...]