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

[Mark Cave-Ayland]

On 31/05/2022 10:22, Damien Hedde wrote:

> On 5/31/22 10:00, Mark Cave-Ayland wrote:
> > 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.
>
> I'm a bit afraid calling qemu_register_reset() outside dc->realize might modify the 
> behavior for existing devices. Does any device end up having a non-NULL bus right now 
> when using "-device" CLI ?

If you take a look at "info qtree" then that will show you all devices that are 
attached to a bus i.e. ones where bus is not NULL.

> > > > > 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
>
> For me, a "memory bus" is a bus. But I understand in QEMU, this is modeled by a 
> memory region and we do not want to represent it anymore by a qdev/qbus hierarchy.
>
> > > 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?
>
> Yes. Sorry for being not clear enough. I did not wanted to insist on specific command 
> names. I've no issues regarding the modifications you request about having a 
> device_connect or a device_map.
>
> My question was more about the workflow which does not rely on issuing a single 
> 'device_add' command handling mapping/connection using parameters. Note that since we 
> are talking supporting of map/connect for the base type TYPE_DEVICE, I don't really 
> see how we could have parameters for these without impacting subtypes.

I'm not sure I understand what you are saying here? Can you give an example?


ATB,

Mark.