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

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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.