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

[Mark Cave-Ayland]

On 30/05/2022 15:05, Damien Hedde wrote:

> On 5/30/22 12:25, Peter Maydell wrote:
> > On Mon, 30 May 2022 at 10:50, Damien Hedde <damien.hedde@greensocs.com> wrote:
> > > TYPE_SYS_BUS_DEVICE also comes with reset support.
> > > If a device is on not on any bus it is not reached by the root sysbus
> > > reset which propagates to every device (and other sub-buses).
> > > Even if we move all the mmio/sysbus-irq logic into TYPE_DEVICE, we will
> > > still miss that. The bus is needed to handle the reset.
> > > For devices created in machine init code, we have the option to do it in
> > > the machine reset handler. But for user created device, this is an issue.
> >
> > Yes, the missing reset support in TYPE_DEVICE is a design
> > flaw that we really should try to address.

I think the easiest way to handle this would be just after calling dc->realize; if 
the device has bus == NULL and dc->reset != NULL then manually call 
qemu_register_reset() for dc->reset. In a qdev world dc->reset is intended to be a 
bus-level reset, but I can't see an issue with manual registration for individual 
devices in this way, particularly as there are no reset ordering guarantees for sysbus.

> > > If we end up putting in TYPE_DEVICE support for mmios, interrupts and
> > > some way to do the bus reset. What would be the difference between the
> > > current TYPE_SYS_BUS_DEVICE ?
> >
> > There would be none, and the idea would be to get rid of
> > TYPE_SYS_BUS_DEVICE entirely...
> Do you expect the bus object to disappear in the process (bus-less system) or 
> transforming the sysbus into a ~TYPE_BUS thing ?

I'd probably lean towards removing sysbus completely since in real life devices can 
exist outside of a bus. If a device needs a bus then it should already be modelled in 
QEMU, and anything that requires a hierarchy can already be represented via QOM children.

> Assuming we manage to sort out this does cold plugging using the following scenario 
> looks ok ? (regarding having to issue one command to create the device AND some 
> commands to handle memory-region and interrupt lines)
>
>  > device_add driver=ibex-uart id=uart chardev=serial0
>  > sysbus-mmio-map device=uart addr=1073741824
>  > qom-set path=uart property=sysbus-irq[0] value=plic/unnamed-gpio-in[1]
>
> TYPE_DEVICE or TYPE_SYS_BUS_DEVICE, my goal is still to be able to cold-plug a 
> "ibex-uart" define its memory map and choose which irq I wire where.

Anyhow getting back on topic: my main objection here is that you're adding a command 
"sysbus-mmio-map" when we don't want the concept of SysBusDevice to be exposed 
outside of QEMU at all. Referring back to my last email I think we should extend the 
device concept in the monitor to handle the additional functionality perhaps along 
the lines of:

- A monitor command such as "device_map" which is effectively a wrapper around
  memory_region_add_subregion(). Do we also want a "device_unmap"? We should
  consider allow mapping to other memory regions other than the system root.

- A monitor command such as "device_connect" which can be used to simplify your 
IRQ
  wiring, perhaps also with a "device_disconnect"?

- An outline of the monitor commands showing the complete workflow from 
introspection
  of a device to mapping its memory region(s) and connecting its gpios

Does that give you enough information to come up with a more detailed proposal?


ATB,

Mark.