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(-)

-- 
2.36.1

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.

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.

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.


ATB,

Mark.

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

On Wed, 25 May 2022 at 10:51, Damien Hedde <damien.hedde@greensocs.com> wrote:
> On 5/24/22 19:44, Mark Cave-Ayland wrote:
> > 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).

Yes, but the fact that TYPE_SYS_BUS_DEVICE is a special subclass
is an accident of history. At some point we really ought to tidy
this up so that any TYPE_DEVICE can have MMIO regions and IRQs,
and get rid of the subclass entirely. This isn't trivial, for
reasons including problems with reset handling, but I would
prefer it if we didn't bake "sysbus is special" into places like
the QMP commands.

More generally, I don't think that the correct answer to "is this
device OK to cold-plug via commandline and QMP is "is it a sysbus
device?". I don't know what the right way to identify cold-pluggable
devices is but I suspect it needs to be more complicated.

> 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.

This is then baking in a device's choices of MMIO region
ordering and arrangement and its IRQ numbering into a
user-facing ABI. I can't say I'm very keen on that -- it
would block us from being able to do a variety of
refactorings and cleanups.

-- PMM

On 25/05/2022 12:45, Peter Maydell wrote:

> On Wed, 25 May 2022 at 10:51, Damien Hedde <damien.hedde@greensocs.com> wrote:
>> On 5/24/22 19:44, Mark Cave-Ayland wrote:
>>> 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).
> 
> Yes, but the fact that TYPE_SYS_BUS_DEVICE is a special subclass
> is an accident of history. At some point we really ought to tidy
> this up so that any TYPE_DEVICE can have MMIO regions and IRQs,
> and get rid of the subclass entirely. This isn't trivial, for
> reasons including problems with reset handling, but I would
> prefer it if we didn't bake "sysbus is special" into places like
> the QMP commands.

Right, and in fact we can already do this today using QOM regardless of whether 
something is a SysBusDevice or not. As an example here is the output of 
qemu-system-sparc's "info qom-tree" for the slavio_misc device:

     /device[20] (slavio_misc)
       /configuration[0] (memory-region)
       /diagnostic[0] (memory-region)
       /leds[0] (memory-region)
       /misc-system-functions[0] (memory-region)
       /modem[0] (memory-region)
       /software-powerdown-control[0] (memory-region)
       /system-control[0] (memory-region)
       /unnamed-gpio-in[0] (irq)

Now imagine that I instantiate a device with qdev_new():

     DeviceState *dev = qdev_new("slavio_misc");

I can obtain a reference to the "configuration" memory-region using something like:

     MemoryRegion *config_mr = MEMORY_REGION(object_resolve_path_component(
                               OBJECT(dev), "configuration[0]"));

and for the IRQ I can do either:

     qemu_irq *irq = IRQ(object_resolve_path_component(
                         OBJECT(dev), "unnamed-gpio-in[0]"));

or simply:

     qemu_irq *irq = qdev_get_gpio_in(dev, 0);

Maybe for simplicity we could even add a qdev wrapper function to obtain a reference 
for memory regions similar to qdev gpios i.e. qdev_get_mmio(dev, "configuration", 0) 
based upon the above example?

Now from the monitor we can already enumerate this information using qom-list if we 
have the QOM path:

     (qemu) qom-list /machine/unattached/device[20]
     type (string)
     parent_bus (link<bus>)
     hotplugged (bool)
     hotpluggable (bool)
     realized (bool)
     diagnostic[0] (child<memory-region>)
     unnamed-gpio-in[0] (child<irq>)
     modem[0] (child<memory-region>)
     leds[0] (child<memory-region>)
     misc-system-functions[0] (child<memory-region>)
     sysbus-irq[1] (link<irq>)
     sysbus-irq[0] (link<irq>)
     system-control[0] (child<memory-region>)
     configuration[0] (child<memory-region>)
     software-powerdown-control[0] (child<memory-region>)

 From this I think we're missing just 2 things: i) a method to look up properties 
from a device id which can be used to facilitate introspection, and ii) a function to 
map a memory region from a device (similar to Damien's patch). Those could be 
something like:

    device_list <id>
      - looks up the QOM path for device "id" and calls qom-list on the result

    device_map <id> <mr> <offset> [<parent_mr>]
      - map device "id" region named mr at given offset. If parent_mr is
        unspecified, assume it is the root address space (get_system_memory()).

It may also be worth adding a device_connect wrapper to simplify your qom-set example:

    device_connect <out-id> <out-irq> <in-id> <in-irq>

The only thing I see here that SYS_BUS_DEVICE offers that we don't have is the 
ability to restrict which memory regions/irqs are available for mapping - but does 
this matter if we have introspection and don't mind addressing everything by name?

> More generally, I don't think that the correct answer to "is this
> device OK to cold-plug via commandline and QMP is "is it a sysbus
> device?". I don't know what the right way to identify cold-pluggable
> devices is but I suspect it needs to be more complicated.

I think that connecting devices like this can only work if there is no additional bus 
logic, in which case could we say a device is cold-pluggable if it has no bus 
specified, or the bus is the root sysbus?

>> 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.
> 
> This is then baking in a device's choices of MMIO region
> ordering and arrangement and its IRQ numbering into a
> user-facing ABI. I can't say I'm very keen on that -- it
> would block us from being able to do a variety of
> refactorings and cleanups.

Absolutely agree. The main reason we need something like qom-find-device-path is 
because QOM paths are not stable: there are a large number of legacy devices still 
out there, and QOMifying them often changes the QOM paths and child object ordering.


ATB,

Mark.

On 5/25/22 21:20, Mark Cave-Ayland wrote:
> On 25/05/2022 12:45, Peter Maydell wrote:
> 
>> On Wed, 25 May 2022 at 10:51, Damien Hedde 
>> <damien.hedde@greensocs.com> wrote:
>>> On 5/24/22 19:44, Mark Cave-Ayland wrote:
>>>> 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).
>>
>> Yes, but the fact that TYPE_SYS_BUS_DEVICE is a special subclass
>> is an accident of history. At some point we really ought to tidy
>> this up so that any TYPE_DEVICE can have MMIO regions and IRQs,
>> and get rid of the subclass entirely. This isn't trivial, for
>> reasons including problems with reset handling, but I would
>> prefer it if we didn't bake "sysbus is special" into places like
>> the QMP commands.
> 
> Right, and in fact we can already do this today using QOM regardless of 
> whether something is a SysBusDevice or not. As an example here is the 
> output of qemu-system-sparc's "info qom-tree" for the slavio_misc device:
> 
>      /device[20] (slavio_misc)
>        /configuration[0] (memory-region)
>        /diagnostic[0] (memory-region)
>        /leds[0] (memory-region)
>        /misc-system-functions[0] (memory-region)
>        /modem[0] (memory-region)
>        /software-powerdown-control[0] (memory-region)
>        /system-control[0] (memory-region)
>        /unnamed-gpio-in[0] (irq)
> 
> Now imagine that I instantiate a device with qdev_new():
> 
>      DeviceState *dev = qdev_new("slavio_misc");
> 
> I can obtain a reference to the "configuration" memory-region using 
> something like:
> 
>      MemoryRegion *config_mr = MEMORY_REGION(object_resolve_path_component(
>                                OBJECT(dev), "configuration[0]"));
> 
> and for the IRQ I can do either:
> 
>      qemu_irq *irq = IRQ(object_resolve_path_component(
>                          OBJECT(dev), "unnamed-gpio-in[0]"));
> 
> or simply:
> 
>      qemu_irq *irq = qdev_get_gpio_in(dev, 0);
> 
> Maybe for simplicity we could even add a qdev wrapper function to obtain 
> a reference for memory regions similar to qdev gpios i.e. 
> qdev_get_mmio(dev, "configuration", 0) based upon the above example?
> 
> Now from the monitor we can already enumerate this information using 
> qom-list if we have the QOM path:
> 
>      (qemu) qom-list /machine/unattached/device[20]
>      type (string)
>      parent_bus (link<bus>)
>      hotplugged (bool)
>      hotpluggable (bool)
>      realized (bool)
>      diagnostic[0] (child<memory-region>)
>      unnamed-gpio-in[0] (child<irq>)
>      modem[0] (child<memory-region>)
>      leds[0] (child<memory-region>)
>      misc-system-functions[0] (child<memory-region>)
>      sysbus-irq[1] (link<irq>)
>      sysbus-irq[0] (link<irq>)
>      system-control[0] (child<memory-region>)
>      configuration[0] (child<memory-region>)
>      software-powerdown-control[0] (child<memory-region>)
> 
>  From this I think we're missing just 2 things: i) a method to look up 
> properties from a device id which can be used to facilitate 
> introspection, and ii) a function to map a memory region from a device 
> (similar to Damien's patch). Those could be something like:
> 
>     device_list <id>
>       - looks up the QOM path for device "id" and calls qom-list on the 
> result

You can already do qom_list <id>.
It works with non-absolute qom path too (if there is only 1 match for 
the relative path in the qom tree). In your example `qom-list 
device[20]` probably works too.

> 
>     device_map <id> <mr> <offset> [<parent_mr>]
>       - map device "id" region named mr at given offset. If parent_mr is
>         unspecified, assume it is the root address space 
> (get_system_memory()).
> 
> It may also be worth adding a device_connect wrapper to simplify your 
> qom-set example:
> 
>     device_connect <out-id> <out-irq> <in-id> <in-irq>
> 
> The only thing I see here that SYS_BUS_DEVICE offers that we don't have 
> is the ability to restrict which memory regions/irqs are available for 
> mapping - but does this matter if we have introspection and don't mind 
> addressing everything by name?

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.

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 ?

> 
>> More generally, I don't think that the correct answer to "is this
>> device OK to cold-plug via commandline and QMP is "is it a sysbus
>> device?". I don't know what the right way to identify cold-pluggable
>> devices is but I suspect it needs to be more complicated.
> 
> I think that connecting devices like this can only work if there is no 
> additional bus logic, in which case could we say a device is 
> cold-pluggable if it has no bus specified, or the bus is the root sysbus?
> 
>>> 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.
>>
>> This is then baking in a device's choices of MMIO region
>> ordering and arrangement and its IRQ numbering into a
>> user-facing ABI. I can't say I'm very keen on that -- it
>> would block us from being able to do a variety of
>> refactorings and cleanups.
> 
> Absolutely agree. The main reason we need something like 
> qom-find-device-path is because QOM paths are not stable: there are a 
> large number of legacy devices still out there, and QOMifying them often 
> changes the QOM paths and child object ordering.
> 
> 
> ATB,
> 
> Mark.
--
Damien

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.

> 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...

-- PMM

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.
> 
>> 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 ?

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.

Thanks,
Damien

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.

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 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

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.

On 5/31/22 22:43, Mark Cave-Ayland wrote:
> 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?

There are 2 possible workflows:
1. several commands
 > device_add ...
 > device_map ...
 > device_connect ...

2. single command
 > device_add ... map={...} connect={...}

The 2nd one is more like how we connect devices with '-device': all is 
done at once. But if this is supposed to apply to TYPE_DEVICE (versus 
TYPE_SYS_BUS_DEVICE), it becomes IMHO hard to prevent using them on 
devices where it does not makes sense (for example: a virtio or pci 
device for which everything is already handled).

--
Damien

On 01/06/2022 09:39, Damien Hedde wrote:

> On 5/31/22 22:43, Mark Cave-Ayland wrote:
>> 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?
> 
> There are 2 possible workflows:
> 1. several commands
>  > device_add ...
>  > device_map ...
>  > device_connect ...
> 
> 2. single command
>  > device_add ... map={...} connect={...}
> 
> The 2nd one is more like how we connect devices with '-device': all is done at once. 
> But if this is supposed to apply to TYPE_DEVICE (versus TYPE_SYS_BUS_DEVICE), it 
> becomes IMHO hard to prevent using them on devices where it does not makes sense (for 
> example: a virtio or pci device for which everything is already handled).

My initial feeling is that 1) is the better approach, since you can report errors at 
each stage. Once you have the id for a specific device you can them attempt to 
device_map it, reporting an error if there is a region overlap. Similarly for 
device_connect can you report an error if the input gpio is already connected.


ATB,

Mark.

On 01.06.22 10:39, Damien Hedde wrote:
> 
> 
> On 5/31/22 22:43, Mark Cave-Ayland wrote:
>> 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?
> 
> There are 2 possible workflows:
> 1. several commands
>  > device_add ...
>  > device_map ...
>  > device_connect ...
> 
> 2. single command
>  > device_add ... map={...} connect={...}
> 
> The 2nd one is more like how we connect devices with '-device': all is 
> done at once. But if this is supposed to apply to TYPE_DEVICE (versus 
> TYPE_SYS_BUS_DEVICE), it becomes IMHO hard to prevent using them on 
> devices where it does not makes sense (for example: a virtio or pci 
> device for which everything is already handled).

Can't we flag devices for which map/connect is supported in the device
class somehow?


-- 
Thanks,

David / dhildenb

On 01/06/2022 10:07, David Hildenbrand wrote:

> On 01.06.22 10:39, Damien Hedde wrote:
>>
>>
>> On 5/31/22 22:43, Mark Cave-Ayland wrote:
>>> 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?
>>
>> There are 2 possible workflows:
>> 1. several commands
>>   > device_add ...
>>   > device_map ...
>>   > device_connect ...
>>
>> 2. single command
>>   > device_add ... map={...} connect={...}
>>
>> The 2nd one is more like how we connect devices with '-device': all is
>> done at once. But if this is supposed to apply to TYPE_DEVICE (versus
>> TYPE_SYS_BUS_DEVICE), it becomes IMHO hard to prevent using them on
>> devices where it does not makes sense (for example: a virtio or pci
>> device for which everything is already handled).
> 
> Can't we flag devices for which map/connect is supported in the device
> class somehow?

I don't think we actually need to do this: if someone is using the monitor to 
wire/rewire or map/remap a device in an invalid way then ultimately that is their choice.

However given that this is a new feature, we can initially restrict it to 
SYS_BUS_DEVICEs before expanding it out to a generic DEVICE later once the feature 
has been proved. The key part for me is to try and do it in a way that such a change 
won't require future changes to the monitor client.


ATB,

Mark.

On 5/25/22 13:45, Peter Maydell wrote:
> On Wed, 25 May 2022 at 10:51, Damien Hedde <damien.hedde@greensocs.com> wrote:
>> On 5/24/22 19:44, Mark Cave-Ayland wrote:
>>> 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).
> 
> Yes, but the fact that TYPE_SYS_BUS_DEVICE is a special subclass
> is an accident of history. At some point we really ought to tidy
> this up so that any TYPE_DEVICE can have MMIO regions and IRQs,
> and get rid of the subclass entirely. This isn't trivial, for
> reasons including problems with reset handling, but I would
> prefer it if we didn't bake "sysbus is special" into places like
> the QMP commands.
> 
> More generally, I don't think that the correct answer to "is this
> device OK to cold-plug via commandline and QMP is "is it a sysbus
> device?". I don't know what the right way to identify cold-pluggable
> devices is but I suspect it needs to be more complicated.

"sysbus is special" is already into the QMP commands.

Right now, any user-creatable device (but sysbus devices) can be 
cold-plugged by CLI using "-device".
The checks are basically:
+ does this device is "user-creatable" ?
+ do realize succeed ? (check device properties and handle bus connection)
So basically this is down to the bus realize mechanism to deal with any 
non-trivial constraint.

Concretely "user-creatable" means cold-pluggable by CLI.
And the reason why it cannot be done by QMP is because QMP commands are 
not possible at that time (the based-on series fix that).

For sysbus device, it is the same but there is an additional check to 
verify that the device is present in the machine allow list.

> 
>> 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.
> 
> This is then baking in a device's choices of MMIO region
> ordering and arrangement and its IRQ numbering into a
> user-facing ABI. I can't say I'm very keen on that -- it
> would block us from being able to do a variety of
> refactorings and cleanups.
>

It's the same for any device property, we make a choice that is exposed 
to the user.

Whether this is done at TYPE_DEVICE or TYPE_SYS_BUS_DEVICE level does 
not change anything to that matter. We could, for example, choose to 
follow the order/name convention used in device tree specs if the issue 
is having no rules. That would probably ease any fdt generation from a 
device qemu model.

That does not prevent us to make change after. Right now, this would be 
accessible only if using the _none_ machine and with '-preconfig' 
experimental and using the _none_ machine. So I don't think we have to 
follow some deprecation policy.

What would you consider a starting point to allow that kind of plug ?

--
Damien