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.he...@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 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.
Thanks,
Damien
