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

[Damien Hedde <damien.hedde@greensocs.com>]

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