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(-)
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
© 2016 - 2024 Red Hat, Inc.