Re: [RFC PATCH v5 0/3] Sysbus device generic QAPI plug support

[Mark Cave-Ayland]

On 25/05/2022 12:45, Peter Maydell wrote:

> On Wed, 25 May 2022 at 10:51, Damien Hedde <damien.hedde@greensocs.com> wrote:
> > On 5/24/22 19:44, Mark Cave-Ayland wrote:
> > > Sorry for coming late into this series, however one of the things I've
> > > been thinking about a lot recently is that with the advent of QOM and
> > > qdev, is there really any distinction between TYPE_DEVICE and
> > > TYPE_SYS_BUS_DEVICE anymore, and whether it makes sense to keep
> > > TYPE_SYS_BUS_DEVICE long term.
> >
> > On QAPI/CLI level there is a huge difference since TYPE_SYS_BUS_DEVICE
> > is the only subtype of TYPE_DEVICE which is subject to special
> > treatment. It prevents to plug a sysbus device which has not be allowed
> > by code and that's what I want to get rid of (or workaround by allowing
> > all of them).
>
> Yes, but the fact that TYPE_SYS_BUS_DEVICE is a special subclass
> is an accident of history. At some point we really ought to tidy
> this up so that any TYPE_DEVICE can have MMIO regions and IRQs,
> and get rid of the subclass entirely. This isn't trivial, for
> reasons including problems with reset handling, but I would
> prefer it if we didn't bake "sysbus is special" into places like
> the QMP commands.

Right, and in fact we can already do this today using QOM regardless of whether 
something is a SysBusDevice or not. As an example here is the output of 
qemu-system-sparc's "info qom-tree" for the slavio_misc device:

    /device[20] (slavio_misc)
      /configuration[0] (memory-region)
      /diagnostic[0] (memory-region)
      /leds[0] (memory-region)
      /misc-system-functions[0] (memory-region)
      /modem[0] (memory-region)
      /software-powerdown-control[0] (memory-region)
      /system-control[0] (memory-region)
      /unnamed-gpio-in[0] (irq)

Now imagine that I instantiate a device with qdev_new():

    DeviceState *dev = qdev_new("slavio_misc");

I can obtain a reference to the "configuration" memory-region using something 
like:

    MemoryRegion *config_mr = MEMORY_REGION(object_resolve_path_component(
                              OBJECT(dev), "configuration[0]"));

and for the IRQ I can do either:

    qemu_irq *irq = IRQ(object_resolve_path_component(
                        OBJECT(dev), "unnamed-gpio-in[0]"));

or simply:

    qemu_irq *irq = qdev_get_gpio_in(dev, 0);

Maybe for simplicity we could even add a qdev wrapper function to obtain a reference 
for memory regions similar to qdev gpios i.e. qdev_get_mmio(dev, "configuration", 0) 
based upon the above example?

Now from the monitor we can already enumerate this information using qom-list if we 
have the QOM path:

    (qemu) qom-list /machine/unattached/device[20]
    type (string)
    parent_bus (link<bus>)
    hotplugged (bool)
    hotpluggable (bool)
    realized (bool)
    diagnostic[0] (child<memory-region>)
    unnamed-gpio-in[0] (child<irq>)
    modem[0] (child<memory-region>)
    leds[0] (child<memory-region>)
    misc-system-functions[0] (child<memory-region>)
    sysbus-irq[1] (link<irq>)
    sysbus-irq[0] (link<irq>)
    system-control[0] (child<memory-region>)
    configuration[0] (child<memory-region>)
    software-powerdown-control[0] (child<memory-region>)

From this I think we're missing just 2 things: i) a method to look up properties 
from a device id which can be used to facilitate introspection, and ii) a function to 
map a memory region from a device (similar to Damien's patch). Those could be 
something like:

   device_list <id>
     - looks up the QOM path for device "id" and calls qom-list on the result

   device_map <id> <mr> <offset> [<parent_mr>]
     - map device "id" region named mr at given offset. If parent_mr is
       unspecified, assume it is the root address space (get_system_memory()).

It may also be worth adding a device_connect wrapper to simplify your qom-set 
example:

   device_connect <out-id> <out-irq> <in-id> <in-irq>

The only thing I see here that SYS_BUS_DEVICE offers that we don't have is the 
ability to restrict which memory regions/irqs are available for mapping - but does 
this matter if we have introspection and don't mind addressing everything by name?

> More generally, I don't think that the correct answer to "is this
> device OK to cold-plug via commandline and QMP is "is it a sysbus
> device?". I don't know what the right way to identify cold-pluggable
> devices is but I suspect it needs to be more complicated.

I think that connecting devices like this can only work if there is no additional bus 
logic, in which case could we say a device is cold-pluggable if it has no bus 
specified, or the bus is the root sysbus?

> > I'm note sure what you mean by identification and enumeration. I do not
> > do any introspection and rely on knowing which mmio or irq index
> > corresponds to what. The "id" in `device_add` allows to reference the
> > device in following commands.
>
> This is then baking in a device's choices of MMIO region
> ordering and arrangement and its IRQ numbering into a
> user-facing ABI. I can't say I'm very keen on that -- it
> would block us from being able to do a variety of
> refactorings and cleanups.

Absolutely agree. The main reason we need something like qom-find-device-path is 
because QOM paths are not stable: there are a large number of legacy devices still 
out there, and QOMifying them often changes the QOM paths and child object ordering.


ATB,

Mark.