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

[Damien Hedde]

On 5/24/22 19:44, Mark Cave-Ayland wrote:
> On 24/05/2022 14:48, Damien Hedde wrote:
>
> > Hi all,
> >
> > This series is about enabling to plug sysbus devices with
> > device_add QAPI command. I've put RFC because, there are several
> > options and I would like to know if you think the current version
> > is ok to be added in qemu.
> >
> > Right now only a few sysbus device can be plugged using "-device"
> > CLI option and a custom plugging mechanism. A machine defines a
> > list of allowed/supported sysbus devices and provides some code to
> > handle the plug. For example, it sets up the memory map and irq
> > connections.
> >
> > In order to configure a machine from scratch with qapi, we want to
> > cold plug sysbus devices to the _none_ machine with qapi commands
> > without requiring the machine to provide some specific per-device
> > support.
> >
> > There are mostly 2 options (option 1 is in these patches). Note that
> > in any case this only applies to "user-creatable" device.
> >
> > + Option 1: Use the current plug mechanism by allowing any sysbus
> > device, without adding handle code in the machine.
> >
> > + Option 2: Add a boolean flag in the machine to allow to plug any
> > sysbus device. We can additionally differentiate the sysbus devices
> > requiring the legacy plug mechanism (with a flag, for example) and
> > the others.
> >
> > The setup of the memory map and irq connections is not related to
> > the option choice above. We planned to rely on qapi commands to do
> > these operations. A new _sysbus-mmio-map_ command is proposed in this
> > series to setup the mapping. Irqs can already be connected using the
> > _qom-set_ command.
> > Alternatively we could probably add parameters/properties to device_add
> > to handle the memory map, but it looks more complicated to achieve.
> >
> > Based-on: <20220519153402.41540-1-damien.hedde@greensocs.com>
> >      ( QAPI support for device cold-plug )
> > Note that it does not stricly require this to be merged, but this series
> > does not make much sense without the ability to cold plug with device_add
> > first.
> >
> > Thanks for your feedback,
> > --
> > Damien
> >
> > Damien Hedde (3):
> >    none-machine: allow cold plugging sysbus devices
> >    softmmu/memory: add memory_region_try_add_subregion function
> >    add sysbus-mmio-map qapi command
> >
> >   qapi/qdev.json         | 31 ++++++++++++++++++++++++++
> >   include/exec/memory.h  | 22 +++++++++++++++++++
> >   hw/core/null-machine.c |  4 ++++
> >   hw/core/sysbus.c       | 49 ++++++++++++++++++++++++++++++++++++++++++
> >   softmmu/memory.c       | 26 ++++++++++++++--------
> >   5 files changed, 123 insertions(+), 9 deletions(-)
>
> 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).

> No objection to the concept of being able to plug devices into the none 
> machine, but I'm wondering if the "sysbus-mmio-map" QAPI command should 
> be a generic "device-map" instead so that the concept of SYS_BUS_DEVICE 
> doesn't leak into QAPI.

It is possible to change this command to a more generic command if 
people feel better with it.
Instead of providing a mmio index we just need to provide an argument to 
identify the memory region in the device (by it's name/path maybe ?).

> Can you give a bit more detail as to the sequence of QMP transactions 
> that would be used to map a SYS_BUS_DEVICE with this series applied? I'm 
> particularly interested to understand how SYS_BUS_DEVICEs are 
> identified, and how their memory regions and gpios are enumerated by the 
> client to enable it to generate the final "sysbus-mmio-map" and 
> "qom-set" commands.

Here's a typical example of commands to create and connect an uart (here 
"plic" is the id of the interrupt controller created before):
> 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]
> qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2]
> qom-set path=uart property=sysbus-irq[2] value=plic/unnamed-gpio-in[3]
> qom-set path=uart property=sysbus-irq[3] value=plic/unnamed-gpio-in[4]

This comes from one of my example here (it needs more patches than this 
series): 
https://github.com/GreenSocs/qemu-qmp-machines/blob/master/riscv-opentitan/machine.qmp

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.

Thanks,
--
Damien