Re: [PATCH v4 12/12] hw/sparc64/cpu: Initialize GPIO before realizing CPU devices
On 2/13/24 14:03, Philippe Mathieu-Daudé wrote: Inline cpu_create() in order to call qdev_init_gpio_in_named_with_opaque() before the CPU is realized. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Peter Maydell Reviewed-by: Mark Cave-Ayland --- hw/sparc64/sparc64.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c index 72f0849f50..3091cde586 100644 --- a/hw/sparc64/sparc64.c +++ b/hw/sparc64/sparc64.c @@ -24,6 +24,7 @@ #include "qemu/osdep.h" +#include "qapi/error.h" #include "cpu.h" #include "hw/boards.h" #include "hw/sparc/sparc64.h" @@ -271,9 +272,10 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr) uint32_t stick_frequency = 100 * 100; uint32_t hstick_frequency = 100 * 100; -cpu = SPARC_CPU(cpu_create(cpu_type)); +cpu = SPARC_CPU(object_new(cpu_type)); qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq, "ivec-irq", IVEC_MAX); +qdev_realize(DEVICE(cpu), NULL, _fatal); env = >env; env->tick = cpu_timer_create("tick", cpu, tick_irq, Reviewed-by: Damien Hedde
Re: [PATCH v4 04/12] hw/i386/q35: Realize LPC PCI function before accessing it
On 2/13/24 14:03, Philippe Mathieu-Daudé wrote: We should not wire IRQs on unrealized device. Signed-off-by: Philippe Mathieu-Daudé > --- hw/i386/pc_q35.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 7ca3f465e0..b7c69d55d6 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -248,13 +248,13 @@ static void pc_q35_init(MachineState *machine) /* create ISA bus */ lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC), TYPE_ICH9_LPC_DEVICE); -qdev_prop_set_bit(DEVICE(lpc), "smm-enabled", - x86_machine_is_smm_enabled(x86ms)); lpc_dev = DEVICE(lpc); +qdev_prop_set_bit(lpc_dev, "smm-enabled", + x86_machine_is_smm_enabled(x86ms)); +pci_realize_and_unref(lpc, host_bus, _fatal); for (i = 0; i < IOAPIC_NUM_PINS; i++) { qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]); } -pci_realize_and_unref(lpc, host_bus, _fatal); rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc")); Reviewed-by: Damien Hedde
Re: NVME hotplug support ?
On 1/29/24 16:35, Hannes Reinecke wrote: On 1/29/24 14:13, Damien Hedde wrote: On 1/24/24 08:47, Hannes Reinecke wrote: On 1/24/24 07:52, Philippe Mathieu-Daudé wrote: Hi Hannes, [+Markus as QOM/QDev rubber duck] On 23/1/24 13:40, Hannes Reinecke wrote: On 1/23/24 11:59, Damien Hedde wrote: Hi all, We are currently looking into hotplugging nvme devices and it is currently not possible: When nvme was introduced 2 years ago, the feature was disabled. commit cc6fb6bc506e6c47ed604fcb7b7413dff0b7d845 Author: Klaus Jensen Date: Tue Jul 6 10:48:40 2021 +0200 hw/nvme: mark nvme-subsys non-hotpluggable We currently lack the infrastructure to handle subsystem hotplugging, so disable it. Do someone know what's lacking or anyone have some tips/idea of what we should develop to add the support ? Problem is that the object model is messed up. In qemu namespaces are attached to controllers, which in turn are children of the PCI device. There are subsystems, but these just reference the controller. So if you hotunplug the PCI device you detach/destroy the controller and detach the namespaces from the controller. But if you hotplug the PCI device again the NVMe controller will be attached to the PCI device, but the namespace are still be detached. Klaus said he was going to fix that, and I dimly remember some patches floating around. But apparently it never went anywhere. Fundamental problem is that the NVMe hierarchy as per spec is incompatible with the qemu object model; qemu requires a strict tree model where every object has exactly _one_ parent. The modelling problem is not clear to me. Do you have an example of how the NVMe hierarchy should be? Sure. As per NVMe spec we have this hierarchy: ---> subsys --- | | | V controller namespaces There can be several controllers, and several namespaces. The initiator (ie the linux 'nvme' driver) connects to a controller, queries the subsystem for the attached namespaces, and presents each namespace as a block device. For Qemu we have the problem that every device _must_ be a direct descendant of the parent (expressed by the fact that each 'parent' object is embedded in the device object). So if we were to present a NVMe PCI device, the controller must be derived from the PCI device: pci -> controller but now we have to express the NVMe hierarchy, too: pci -> ctrl1 -> subsys1 -> namespace1 which actually works. We can easily attach several namespaces: pci -> ctrl1 ->subsys1 -> namespace2 For a single controller and a single subsystem. However, as mentioned above, there can be _several_ controllers attached to the same subsystem. So we can express the second controller: pci -> ctrl2 but we cannot attach the controller to 'subsys1' as then 'subsys1' would need to be derived from 'ctrl2', and not (as it is now) from 'ctrl1'. The most logical step would be to have 'subsystems' their own entity, independent of any controllers. But then the block devices (which are derived from the namespaces) could not be traced back to the PCI device, and a PCI hotplug would not 'automatically' disconnect the nvme block devices. Plus the subsystem would be independent from the NVMe PCI devices, so you could have a subsystem with no controllers attached. And one would wonder who should be responsible for cleaning up that. Thanks for the details ! My use case is the simple one with no nvme subsystem/namespaces: - hotplug a pci nvme device (nvme controller) as in the following CLI (which automatically put the drive into a default namespace) ./qemu-system-aarch64 -nographic -M virt \ -drive file=nvme0.disk,if=none,id=nvme-drive0 \ -device nvme,serial=nvme0,id=nvmedev0,drive=nvme-drive0 In the simple tree approach where subsystems and namespaces are not shared by controllers. We could delete the whole nvme hiearchy under the controller while unplugging it ? In your first message, you said > So if you hotunplug the PCI device you detach/destroy the controller > and detach the namespaces from the controller. > But if you hotplug the PCI device again the NVMe controller will be > attached to the PCI device, but the namespace are still be detached. Do you mean that if we unplug the pci device we HAVE to keep some nvme objects so that if we plug the device back we can recover them ? Or just that it's hard to unplug nvme objects if they are not real qom children of pci device ? Key point for trying on PCI hotplug with qemu is to attach the PCI device to it's own PCI root port. Cf the mail from Klaus Jensen for details. Cheers, Hannes Thanks a lot from both of you. I missed that. Damien
Re: NVME hotplug support ?
On 1/24/24 08:47, Hannes Reinecke wrote: On 1/24/24 07:52, Philippe Mathieu-Daudé wrote: Hi Hannes, [+Markus as QOM/QDev rubber duck] On 23/1/24 13:40, Hannes Reinecke wrote: On 1/23/24 11:59, Damien Hedde wrote: Hi all, We are currently looking into hotplugging nvme devices and it is currently not possible: When nvme was introduced 2 years ago, the feature was disabled. commit cc6fb6bc506e6c47ed604fcb7b7413dff0b7d845 Author: Klaus Jensen Date: Tue Jul 6 10:48:40 2021 +0200 hw/nvme: mark nvme-subsys non-hotpluggable We currently lack the infrastructure to handle subsystem hotplugging, so disable it. Do someone know what's lacking or anyone have some tips/idea of what we should develop to add the support ? Problem is that the object model is messed up. In qemu namespaces are attached to controllers, which in turn are children of the PCI device. There are subsystems, but these just reference the controller. So if you hotunplug the PCI device you detach/destroy the controller and detach the namespaces from the controller. But if you hotplug the PCI device again the NVMe controller will be attached to the PCI device, but the namespace are still be detached. Klaus said he was going to fix that, and I dimly remember some patches floating around. But apparently it never went anywhere. Fundamental problem is that the NVMe hierarchy as per spec is incompatible with the qemu object model; qemu requires a strict tree model where every object has exactly _one_ parent. The modelling problem is not clear to me. Do you have an example of how the NVMe hierarchy should be? Sure. As per NVMe spec we have this hierarchy: ---> subsys --- | | | V controller namespaces There can be several controllers, and several namespaces. The initiator (ie the linux 'nvme' driver) connects to a controller, queries the subsystem for the attached namespaces, and presents each namespace as a block device. For Qemu we have the problem that every device _must_ be a direct descendant of the parent (expressed by the fact that each 'parent' object is embedded in the device object). So if we were to present a NVMe PCI device, the controller must be derived from the PCI device: pci -> controller but now we have to express the NVMe hierarchy, too: pci -> ctrl1 -> subsys1 -> namespace1 which actually works. We can easily attach several namespaces: pci -> ctrl1 ->subsys1 -> namespace2 For a single controller and a single subsystem. However, as mentioned above, there can be _several_ controllers attached to the same subsystem. So we can express the second controller: pci -> ctrl2 but we cannot attach the controller to 'subsys1' as then 'subsys1' would need to be derived from 'ctrl2', and not (as it is now) from 'ctrl1'. The most logical step would be to have 'subsystems' their own entity, independent of any controllers. But then the block devices (which are derived from the namespaces) could not be traced back to the PCI device, and a PCI hotplug would not 'automatically' disconnect the nvme block devices. Plus the subsystem would be independent from the NVMe PCI devices, so you could have a subsystem with no controllers attached. And one would wonder who should be responsible for cleaning up that. Thanks for the details ! My use case is the simple one with no nvme subsystem/namespaces: - hotplug a pci nvme device (nvme controller) as in the following CLI (which automatically put the drive into a default namespace) ./qemu-system-aarch64 -nographic -M virt \ -drive file=nvme0.disk,if=none,id=nvme-drive0 \ -device nvme,serial=nvme0,id=nvmedev0,drive=nvme-drive0 In the simple tree approach where subsystems and namespaces are not shared by controllers. We could delete the whole nvme hiearchy under the controller while unplugging it ? In your first message, you said > So if you hotunplug the PCI device you detach/destroy the controller > and detach the namespaces from the controller. > But if you hotplug the PCI device again the NVMe controller will be > attached to the PCI device, but the namespace are still be detached. Do you mean that if we unplug the pci device we HAVE to keep some nvme objects so that if we plug the device back we can recover them ? Or just that it's hard to unplug nvme objects if they are not real qom children of pci device ? Thanks, Damien
NVME hotplug support ?
Hi all, We are currently looking into hotplugging nvme devices and it is currently not possible: When nvme was introduced 2 years ago, the feature was disabled. > commit cc6fb6bc506e6c47ed604fcb7b7413dff0b7d845 > Author: Klaus Jensen > Date: Tue Jul 6 10:48:40 2021 +0200 > >hw/nvme: mark nvme-subsys non-hotpluggable > >We currently lack the infrastructure to handle subsystem hotplugging, so >disable it. Do someone know what's lacking or anyone have some tips/idea of what we should develop to add the support ? Regards, -- Damien
[PATCH] MAINTAINERS: update my email address for the clock framework
Also update mailmap Signed-off-by: Damien Hedde --- MAINTAINERS | 2 +- .mailmap| 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 96e25f62ac..ceeda49d49 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3321,7 +3321,7 @@ F: .gitlab-ci.d/opensbi/ Clock framework M: Luc Michel -R: Damien Hedde +R: Damien Hedde S: Maintained F: include/hw/clock.h F: include/hw/qdev-clock.h diff --git a/.mailmap b/.mailmap index fad2aff5aa..7677047950 100644 --- a/.mailmap +++ b/.mailmap @@ -56,6 +56,7 @@ Aleksandar Rikalo Alexander Graf Anthony Liguori Anthony Liguori Christian Borntraeger +Damien Hedde Filip Bozuta Frederic Konrad Frederic Konrad -- 2.37.0
Re: [RFC PATCH v5 0/3] Sysbus device generic QAPI plug support
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 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_B
Re: [RFC PATCH v5 0/3] Sysbus device generic QAPI plug support
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 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
Re: [RFC PATCH v5 0/3] Sysbus device generic QAPI plug support
On 5/30/22 12:25, Peter Maydell wrote: On Mon, 30 May 2022 at 10:50, Damien Hedde 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
Re: [RFC PATCH v5 0/3] Sysbus device generic QAPI plug support
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 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) hotplugged (bool) hotpluggable (bool) realized (bool) diagnostic[0] (child) unnamed-gpio-in[0] (child) modem[0] (child) leds[0] (child) misc-system-functions[0] (child) sysbus-irq[1] (link) sysbus-irq[0] (link) system-control[0] (child) configuration[0] (child) software-powerdown-control[0] (child) 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 - looks up the QOM path for device "id" and calls qom-list on the result You can already do qom_list . 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[] - 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 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
Re: [RFC PATCH] python: add qmp-send program to send raw qmp commands to qemu
On 5/25/22 18:06, Daniel P. Berrangé wrote: On Wed, Mar 16, 2022 at 10:54:55AM +0100, Damien Hedde wrote: +def raw_load(file: TextIO) -> List[QMPMessage]: +"""parse a raw qmp command file. + +JSON formatted commands can expand on several lines but must +be separated by an end-of-line (two commands can not share the +same line). +File must not end with empty lines. +""" +cmds: List[QMPMessage] = [] +linecnt = 0 +while True: +buf = file.readline() +if not buf: +return cmds If you change this to 'break'... +prev_err_pos = None +buf_linecnt = 1 +while True: +try: +cmds.append(json.loads(buf)) ...and this to yield json.loads(buf) then +break +except json.JSONDecodeError as err: +if prev_err_pos == err.pos: +# adding a line made no progress so +# + either we're at EOF and json data is truncated +# + or the parsing error is before +raise QmpRawDecodeError(err.msg, linecnt + err.lineno, +err.colno) from err +prev_err_pos = err.pos +buf += file.readline() +buf_linecnt += 1 +linecnt += buf_linecnt + + +def report_error(msg: str) -> None: +"""Write an error to stderr.""" +sys.stderr.write('ERROR: %s\n' % msg) + + +def main() -> None: +""" +qmp-send entry point: parse command line arguments and start the REPL. +""" +parser = argparse.ArgumentParser( +description=""" +Send raw qmp commands to qemu as long as they succeed. It either +connects to a remote qmp server using the provided socket or wrap +the qemu process. It stops sending the provided commands when a +command fails (disconnection or error response). +""", +epilog=""" +When qemu wrap option is used, this script waits for qemu +to terminate but never send any quit or kill command. This +needs to be done manually. +""") + +parser.add_argument('-f', '--file', action='store', +help='Input file containing the commands') +parser.add_argument('-s', '--socket', action='store', +help='< UNIX socket path | TCP address:port >') +parser.add_argument('-v', '--verbose', action='store_true', +help='Verbose (echo commands sent and received)') +parser.add_argument('-p', '--pretty', action='store_true', +help='Pretty-print JSON') + +parser.add_argument('--wrap', nargs=argparse.REMAINDER, +help='QEMU command line to invoke') + +args = parser.parse_args() + +socket = args.socket +wrap_qemu = args.wrap is not None + +if wrap_qemu: +if len(args.wrap) != 0: +qemu_cmdline = args.wrap +else: +qemu_cmdline = ["qemu-system-x86_64"] +if socket is None: +socket = "qmp-send-wrap-%d" % os.getpid() +qemu_cmdline += ["-qmp", "unix:%s" % socket] + +try: +address = QMPSend.parse_address(socket) +except QMPBadPortError: +parser.error(f"Bad port number: {socket}") +return # pycharm doesn't know error() is noreturn + +try: +with open(args.file, mode='rt', encoding='utf8') as file: +qmp_cmds = raw_load(file) +except QmpRawDecodeError as err: +report_error(str(err)) +sys.exit(1) ...change this to fh = sys.stdin if args.file is not None and args.file != '-': fh = open(args.file, mode='rt', encoding='utf8') + +try: +with QMPSend(address, args.pretty, args.verbose, + server=wrap_qemu) as qmp: +# starting with python 3.7 we could use contextlib.nullcontext +qemu = Popen(qemu_cmdline) if wrap_qemu else contextlib.suppress() +with qemu: +try: +qmp.setup_connection() +except ConnectError as err: +if isinstance(err.exc, OSError): +report_error(f"Couldn't connect to {socket}: {err!s}") +else: +report_error(str(err)) +sys.exit(1) +try: +for cmd in qmp_cmds: ...finally this to for cmd in raw_load(fh) This means we can use qmp-send in a pipeline with commands sent to QEMU on the fly as they arrive, rather than having to read all the commands upfront before QEMU is started. Yes. I was not su
Re: [RFC PATCH v5 0/3] Sysbus device generic QAPI plug support
On 5/25/22 13:45, Peter Maydell wrote: On Wed, 25 May 2022 at 10:51, Damien Hedde 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
Re: [RFC PATCH v5 0/3] Sysbus device generic QAPI plug support
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.he...@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
[RFC PATCH v5 1/3] none-machine: allow cold plugging sysbus devices
Allow plugging any sysbus device on this machine (the sysbus devices still need to be 'user-creatable'). This commit is needed to use the 'none' machine as a base, and subsequently to dynamically populate it with sysbus devices using qapi commands. Note that this only concern cold-plug: sysbus devices can not be hot-plugged because the sysbus bus does not support it. Signed-off-by: Damien Hedde Reviewed-by: Philippe Mathieu-Daudé --- v5: + fix commit message (Philippe) --- hw/core/null-machine.c | 4 1 file changed, 4 insertions(+) diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c index f586a4bef5..21bc2aca23 100644 --- a/hw/core/null-machine.c +++ b/hw/core/null-machine.c @@ -16,6 +16,7 @@ #include "hw/boards.h" #include "exec/address-spaces.h" #include "hw/core/cpu.h" +#include "hw/sysbus.h" static void machine_none_init(MachineState *mch) { @@ -54,6 +55,9 @@ static void machine_none_machine_init(MachineClass *mc) mc->no_floppy = 1; mc->no_cdrom = 1; mc->no_sdcard = 1; + +/* allow cold plugging any any "user-creatable" sysbus device */ +machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SYS_BUS_DEVICE); } DEFINE_MACHINE("none", machine_none_machine_init) -- 2.36.1
[RFC PATCH v5 2/3] softmmu/memory: add memory_region_try_add_subregion function
It allows adding a subregion to a memory region with error handling. Like memory_region_add_subregion_overlap(), it handles priority as well. Apart from the error handling, the behavior is the same. It can be used to do the simple memory_region_add_subregion() (with no overlap) by setting the priority parameter to 0. This commit is a preparation to further use of this function in the context of qapi command which needs error handling support. Signed-off-by: Damien Hedde Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: David Hildenbrand Reviewed-by: Alistair Francis --- v5: + rebase, new callsite --- include/exec/memory.h | 22 ++ softmmu/memory.c | 26 +- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/include/exec/memory.h b/include/exec/memory.h index f1c19451bc..36f2e86be5 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -2215,6 +2215,28 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr, MemoryRegion *subregion, int priority); +/** + * memory_region_try_add_subregion: Add a subregion to a container + * with error handling. + * + * Behaves like memory_region_add_subregion_overlap(), but errors are + * reported if the subregion cannot be added. + * + * @mr: the region to contain the new subregion; must be a container + * initialized with memory_region_init(). + * @offset: the offset relative to @mr where @subregion is added. + * @subregion: the subregion to be added. + * @priority: used for resolving overlaps; highest priority wins. + * @errp: pointer to Error*, to store an error if it happens. + * + * Returns: True in case of success, false otherwise. + */ +bool memory_region_try_add_subregion(MemoryRegion *mr, + hwaddr offset, + MemoryRegion *subregion, + int priority, + Error **errp); + /** * memory_region_get_ram_addr: Get the ram address associated with a memory * region diff --git a/softmmu/memory.c b/softmmu/memory.c index 7ba2048836..5ea4000830 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -2541,27 +2541,34 @@ done: memory_region_transaction_commit(); } -static void memory_region_add_subregion_common(MemoryRegion *mr, - hwaddr offset, - MemoryRegion *subregion) +bool memory_region_try_add_subregion(MemoryRegion *mr, + hwaddr offset, + MemoryRegion *subregion, + int priority, + Error **errp) { MemoryRegion *alias; -assert(!subregion->container); +if (subregion->container) { +error_setg(errp, "The memory region is already in another region"); +return false; +} + +subregion->priority = priority; subregion->container = mr; for (alias = subregion->alias; alias; alias = alias->alias) { alias->mapped_via_alias++; } subregion->addr = offset; memory_region_update_container_subregions(subregion); +return true; } void memory_region_add_subregion(MemoryRegion *mr, hwaddr offset, MemoryRegion *subregion) { -subregion->priority = 0; -memory_region_add_subregion_common(mr, offset, subregion); +memory_region_try_add_subregion(mr, offset, subregion, 0, _abort); } void memory_region_add_subregion_overlap(MemoryRegion *mr, @@ -2569,8 +2576,8 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr, MemoryRegion *subregion, int priority) { -subregion->priority = priority; -memory_region_add_subregion_common(mr, offset, subregion); +memory_region_try_add_subregion(mr, offset, subregion, priority, +_abort); } void memory_region_del_subregion(MemoryRegion *mr, @@ -2626,7 +2633,8 @@ static void memory_region_readd_subregion(MemoryRegion *mr) memory_region_transaction_begin(); memory_region_ref(mr); memory_region_del_subregion(container, mr); -memory_region_add_subregion_common(container, mr->addr, mr); +memory_region_try_add_subregion(container, mr->addr, mr, mr->priority, +_abort); memory_region_unref(mr); memory_region_transaction_commit(); } -- 2.36.1
[RFC PATCH v5 3/3] add sysbus-mmio-map qapi command
This command allows to map an mmio region of sysbus device onto the system memory. Its behavior mimics the sysbus_mmio_map() function apart from the automatic unmap (the C function unmaps the region if it is already mapped). For the qapi function we consider it is an error to try to map an already mapped function. If unmapping is required, it is probably better to add a sysbus-mmio-unmap command. This command is still experimental (hence the 'unstable' feature), as it is related to the sysbus device creation through qapi commands. This command is required to be able to dynamically build a machine from scratch as there is no qapi-way of doing a memory mapping. Signed-off-by: Damien Hedde --- Cc: Alistair Francis v5: + bump version to 7.1 v4: + integrate priority parameter + use 'unstable' feature flag instead of 'x-' prefix + bump version to 7.0 + dropped Alistair's reviewed-by as a consequence --- qapi/qdev.json | 31 ++ hw/core/sysbus.c | 49 2 files changed, 80 insertions(+) diff --git a/qapi/qdev.json b/qapi/qdev.json index 2e2de41499..787d1ebf81 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -160,3 +160,34 @@ ## { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': { '*device': 'str', 'path': 'str' } } + +## +# @sysbus-mmio-map: +# +# Map a sysbus device mmio onto the main system bus. +# +# @device: the device's QOM path +# +# @mmio: The mmio number to be mapped (defaults to 0). +# +# @addr: The base address for the mapping. +# +# @priority: The priority of the mapping (defaults to 0). +# +# Features: +# @unstable: Command is meant to map sysbus devices +#while in preconfig mode. +# +# Since: 7.1 +# +# Returns: Nothing on success +# +## + +{ 'command': 'sysbus-mmio-map', + 'data': { 'device': 'str', +'*mmio': 'uint8', +'addr': 'uint64', +'*priority': 'int32' }, + 'features': ['unstable'], + 'allow-preconfig' : true } diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c index 05c1da3d31..df1f1f43a5 100644 --- a/hw/core/sysbus.c +++ b/hw/core/sysbus.c @@ -23,6 +23,7 @@ #include "hw/sysbus.h" #include "monitor/monitor.h" #include "exec/address-spaces.h" +#include "qapi/qapi-commands-qdev.h" static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent); static char *sysbus_get_fw_dev_path(DeviceState *dev); @@ -154,6 +155,54 @@ static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr, } } +void qmp_sysbus_mmio_map(const char *device, + bool has_mmio, uint8_t mmio, + uint64_t addr, + bool has_priority, int32_t priority, + Error **errp) +{ +Object *obj = object_resolve_path_type(device, TYPE_SYS_BUS_DEVICE, NULL); +SysBusDevice *dev; + +if (phase_get() != PHASE_MACHINE_INITIALIZED) { +error_setg(errp, "The command is permitted only when " + "the machine is in initialized phase"); +return; +} + +if (obj == NULL) { +error_setg(errp, "Device '%s' not found", device); +return; +} +dev = SYS_BUS_DEVICE(obj); + +if (!has_mmio) { +mmio = 0; +} +if (!has_priority) { +priority = 0; +} + +if (mmio >= dev->num_mmio) { +error_setg(errp, "MMIO index '%u' does not exist in '%s'", + mmio, device); +return; +} + +if (dev->mmio[mmio].addr != (hwaddr)-1) { +error_setg(errp, "MMIO index '%u' is already mapped", mmio); +return; +} + +if (!memory_region_try_add_subregion(get_system_memory(), addr, + dev->mmio[mmio].memory, priority, + errp)) { +return; +} + +dev->mmio[mmio].addr = addr; +} + void sysbus_mmio_unmap(SysBusDevice *dev, int n) { assert(n >= 0 && n < dev->num_mmio); -- 2.36.1
[RFC PATCH v5 0/3] Sysbus device generic QAPI plug support
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.he...@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
[PATCH v5 5/6] RFC qapi/device_add: handle the rom_order_override when cold-plugging
rom_set_order_override() and rom_reset_order_override() were called in qemu_create_cli_devices() to set the rom_order_override value once and for all when creating the devices added on CLI. Unfortunately this won't work with qapi commands. Move the calls inside device_add so that it will be done in every case: + CLI option: -device + QAPI command: device_add rom_[set|reset]_order_override() are implemented in hw/core/loader.c They either do nothing or call fw_cfg_[set|reset]_order_override(). The later functions are implemented in hw/nvram/fw_cfg.c and only change an integer value of a "global" variable. In consequence, there are no complex side effects involved and we can safely move them from outside the -device option loop to the inner function. Signed-off-by: Damien Hedde --- I see 2 other ways to handle this: 1. Adding a new option to device_add. We could add a new boolean (_rom_order_override_ for example) option tot he qapi command. This flag would then be forced to "true" when handling "-device" parameter from CLI. The flag default could be: - always false - false on hot-plug, true on cold-plug. 2. Adding a new qapi command We could add one or two commands to do the rom_[set|reset]_order_override() operation. --- softmmu/qdev-monitor.c | 11 +++ softmmu/vl.c | 2 -- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index d68ef883b5..7cbee2b0d8 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -43,6 +43,7 @@ #include "hw/qdev-properties.h" #include "hw/clock.h" #include "hw/boards.h" +#include "hw/loader.h" /* * Aliases were a bad idea from the start. Let's keep them @@ -673,6 +674,10 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, return NULL; } +if (!is_hotplug) { +rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE); +} + /* create device */ dev = qdev_new(driver); @@ -714,6 +719,9 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, if (!qdev_realize(DEVICE(dev), bus, errp)) { goto err_del_dev; } +if (!is_hotplug) { +rom_reset_order_override(); +} return dev; err_del_dev: @@ -721,6 +729,9 @@ err_del_dev: object_unparent(OBJECT(dev)); object_unref(OBJECT(dev)); } +if (!is_hotplug) { +rom_reset_order_override(); +} return NULL; } diff --git a/softmmu/vl.c b/softmmu/vl.c index ea15e37973..5a0d54b595 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2635,7 +2635,6 @@ static void qemu_create_cli_devices(void) } /* init generic devices */ -rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE); qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, _fatal); QTAILQ_FOREACH(opt, _opts, next) { @@ -2652,7 +2651,6 @@ static void qemu_create_cli_devices(void) object_unref(OBJECT(dev)); loc_pop(>loc); } -rom_reset_order_override(); } static void qemu_machine_creation_done(void) -- 2.36.1
[PATCH v5 1/6] machine: add phase_get() and document phase_check()/advance()
phase_get() returns the current phase, we'll use it in next commit. Signed-off-by: Damien Hedde Reviewed-by: Philippe Mathieu-Daudé --- include/hw/qdev-core.h | 19 +++ hw/core/qdev.c | 5 + 2 files changed, 24 insertions(+) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 92c3d65208..e29c705b74 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -887,7 +887,26 @@ typedef enum MachineInitPhase { PHASE_MACHINE_READY, } MachineInitPhase; +/* + * phase_get: + * Returns the current phase + */ +MachineInitPhase phase_get(void); + +/** + * phase_check: + * Test if current phase is at least @phase. + * + * Returns true if this is the case. + */ extern bool phase_check(MachineInitPhase phase); + +/** + * @phase_advance: + * Update the current phase to @phase. + * + * Must only be used to make a single phase step. + */ extern void phase_advance(MachineInitPhase phase); #endif diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 84f3019440..632dc0a4be 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -910,6 +910,11 @@ Object *qdev_get_machine(void) static MachineInitPhase machine_phase; +MachineInitPhase phase_get(void) +{ +return machine_phase; +} + bool phase_check(MachineInitPhase phase) { return machine_phase >= phase; -- 2.36.1
[PATCH v5 2/6] machine: introduce phase_until() to handle phase transitions
phase_until() is implemented in vl.c and is meant to be used to make startup progress up to a specified phase being reached(). At this point, no behavior change is introduced: phase_until() only supports a single double transition corresponding to the functionality of qmp_exit_preconfig(): + accel-created -> machine-initialized -> machine-ready As a result qmp_exit_preconfig() now uses phase_until(). This commit is a preparation to support cold plugging a device using qapi command (which will be introduced in a following commit). For this we need fine grain control of the phase. Signed-off-by: Damien Hedde --- v5: + refactor to avoid indentation change --- include/hw/qdev-core.h | 14 + softmmu/vl.c | 46 ++ 2 files changed, 60 insertions(+) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index e29c705b74..5f73d06408 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -909,4 +909,18 @@ extern bool phase_check(MachineInitPhase phase); */ extern void phase_advance(MachineInitPhase phase); +/** + * @phase_until: + * @phase: the target phase + * @errp: error report + * + * Make the machine init progress until the target phase is reached. + * + * Its is a no-op is the target phase is the current or an earlier + * phase. + * + * Returns true in case of success. + */ +extern bool phase_until(MachineInitPhase phase, Error **errp); + #endif diff --git a/softmmu/vl.c b/softmmu/vl.c index 84a31eba76..7f8d15b5b8 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2702,11 +2702,17 @@ void qmp_x_exit_preconfig(Error **errp) error_setg(errp, "The command is permitted only before machine initialization"); return; } +phase_until(PHASE_MACHINE_READY, errp); +} +static void qemu_phase_ready(Error **errp) +{ qemu_init_board(); +/* phase is now PHASE_MACHINE_INITIALIZED. */ qemu_create_cli_devices(); cxl_fixed_memory_window_link_targets(errp); qemu_machine_creation_done(); +/* Phase is now PHASE_MACHINE_READY. */ if (loadvm) { load_snapshot(loadvm, NULL, false, NULL, _fatal); @@ -2729,6 +2735,46 @@ void qmp_x_exit_preconfig(Error **errp) } } +bool phase_until(MachineInitPhase phase, Error **errp) +{ +ERRP_GUARD(); +if (!phase_check(PHASE_ACCEL_CREATED)) { +error_setg(errp, "Phase transition is not supported until accelerator" + " is created"); +return false; +} + +while (!phase_check(phase)) { +MachineInitPhase cur_phase = phase_get(); + +switch (cur_phase) { +case PHASE_ACCEL_CREATED: +qemu_phase_ready(errp); +break; + +default: +/* + * If we end up here, it is because we miss a case above. + */ +error_setg(_abort, "Requested phase transition is not" + " implemented"); +return false; +} + +if (*errp) { +return false; +} + +/* + * Ensure we made some progress. + * With the default case above, it should be enough to prevent + * any infinite loop. + */ +assert(cur_phase < phase_get()); +} +return true; +} + void qemu_init(int argc, char **argv, char **envp) { QemuOpts *opts; -- 2.36.1
[PATCH v5 6/6] qapi/device_add: Allow execution in machine initialized phase
From: Mirela Grujic This commit allows to use the QMP command to add a cold-plugged device like we can do with the CLI option -device. Note: for device_add command in qdev.json adding the 'allow-preconfig' option has no effect because the command appears to bypass QAPI (see TODO at qapi/qdev.json:61). The option is added there solely to document the intent. For the same reason, the flags have to be explicitly set in monitor_init_qmp_commands() when the device_add command is registered. Signed-off-by: Mirela Grujic Signed-off-by: Damien Hedde --- v4: + use phase_until() + add missing flag in hmp-commands.hx --- qapi/qdev.json | 3 ++- monitor/misc.c | 2 +- softmmu/qdev-monitor.c | 4 hmp-commands.hx| 1 + 4 files changed, 8 insertions(+), 2 deletions(-) diff --git a/qapi/qdev.json b/qapi/qdev.json index 26cd10106b..2e2de41499 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -77,7 +77,8 @@ { 'command': 'device_add', 'data': {'driver': 'str', '*bus': 'str', '*id': 'str'}, 'gen': false, # so we can get the additional arguments - 'features': ['json-cli', 'json-cli-hotplug'] } + 'features': ['json-cli', 'json-cli-hotplug'], + 'allow-preconfig': true } ## # @device_del: diff --git a/monitor/misc.c b/monitor/misc.c index 6c5bb82d3b..d3d413d70c 100644 --- a/monitor/misc.c +++ b/monitor/misc.c @@ -233,7 +233,7 @@ static void monitor_init_qmp_commands(void) qmp_init_marshal(_commands); qmp_register_command(_commands, "device_add", - qmp_device_add, 0, 0); + qmp_device_add, QCO_ALLOW_PRECONFIG, 0); QTAILQ_INIT(_cap_negotiation_commands); qmp_register_command(_cap_negotiation_commands, "qmp_capabilities", diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 7cbee2b0d8..c53f62be51 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -855,6 +855,10 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) QemuOpts *opts; DeviceState *dev; +if (!phase_until(PHASE_MACHINE_INITIALIZED, errp)) { +return; +} + opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp); if (!opts) { return; diff --git a/hmp-commands.hx b/hmp-commands.hx index 03e6a73d1f..0091b8e2dd 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -672,6 +672,7 @@ ERST .help = "add device, like -device on the command line", .cmd= hmp_device_add, .command_completion = device_add_completion, +.flags = "p", }, SRST -- 2.36.1
[PATCH v5 0/6] QAPI support for device cold-plug
Hi all, As of now dynamic cold plug of device is only possible using the CLI "-device" option. This series add support for device cold-plug using QAPI. Patches 2, 5 and 6 are not reviewed yet. It relies on the use of the "preconfig" mode (only way to stop QEMU early enough) and requires more control on the machine phase than we had before. This work is part of our work towards to build a machine from scratch using QAPI (see v4 or [1]). But this is an independent part which can already be used to add devices on any machine using QAPI instead of having to use the CLI to pass some options. For example, in this command the network interface could be added using qapi: > $ qemu-system-aarch64 -display none -M virt -cpu cortex-a53 \ > -drive file=./images/rootfs.ext4,if=none,format=raw,id=hd0 \ > -device virtio-blk-device,drive=hd0 \ > -kernel ./images/Image -append "rootwait root=/dev/vda console=ttyAMA0" \ > -netdev user,id=eth0 -device virtio-net-device,netdev=eth0 \ > -serial stdio By using the following command line: > $ qemu-system-aarch64 -display none -M virt -cpu cortex-a53 \ >-drive file=./images/rootfs.ext4,if=none,format=raw,id=hd0 \ >-device virtio-blk-device,drive=hd0 \ >-kernel ./images/Image -append "rootwait root=/dev/vda console=ttyAMA0" \ >-serial stdio -preconfig -qmp socket,path=./qmpsocket,server and then qmp-shell (or any other qmp tool) to add the network interface and device: > $ qmp-shell ./qmpsocket > (QEMU) netdev_add type=user id=eth0 > {"return": {}} > (QEMU) device_add driver=virtio-net-device netdev=eth0 > {"return": {}} > (QEMU) x-exit-preconfig > {"return": {}} Thanks, -- Damien v5: + refactor patch 2 to avoid indentation changes v4: https://lore.kernel.org/qemu-devel/20220223090706.4888-1-damien.he...@greensocs.com/ [1]: https://github.com/GreenSocs/qemu-qmp-machines Damien Hedde (5): machine: add phase_get() and document phase_check()/advance() machine: introduce phase_until() to handle phase transitions vl: support machine-initialized target in phase_until() qapi/device_add: compute is_hotplug flag RFC qapi/device_add: handle the rom_order_override when cold-plugging Mirela Grujic (1): qapi/device_add: Allow execution in machine initialized phase qapi/qdev.json | 3 +- include/hw/qdev-core.h | 33 +++ hw/core/qdev.c | 5 +++ monitor/misc.c | 2 +- softmmu/qdev-monitor.c | 20 ++-- softmmu/vl.c | 72 ++ hmp-commands.hx| 1 + 7 files changed, 126 insertions(+), 10 deletions(-) -- 2.36.1
[PATCH v5 4/6] qapi/device_add: compute is_hotplug flag
Instead of checking the phase everytime, just store the result in a flag. We will use more of it in the following commit. Signed-off-by: Damien Hedde Reviewed-by: Philippe Mathieu-Daudé --- softmmu/qdev-monitor.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 12fe60c467..d68ef883b5 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -619,6 +619,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, char *id; DeviceState *dev = NULL; BusState *bus = NULL; +bool is_hotplug = phase_check(PHASE_MACHINE_READY); driver = qdict_get_try_str(opts, "driver"); if (!driver) { @@ -662,7 +663,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, return NULL; } -if (phase_check(PHASE_MACHINE_READY) && bus && !qbus_is_hotpluggable(bus)) { +if (is_hotplug && bus && !qbus_is_hotpluggable(bus)) { error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name); return NULL; } @@ -676,7 +677,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, dev = qdev_new(driver); /* Check whether the hotplug is allowed by the machine */ -if (phase_check(PHASE_MACHINE_READY)) { +if (is_hotplug) { if (!qdev_hotplug_allowed(dev, errp)) { goto err_del_dev; } -- 2.36.1
[PATCH v5 3/6] vl: support machine-initialized target in phase_until()
phase_until() now supports the following transitions: + accel-created -> machine-initialized + machine-initialized -> machine-ready As a consequence we can now support the use of qmp_exit_preconfig() from phases _accel-created_ and _machine-initialized_. This commit is a preparation to support cold plugging a device using qapi (which will be introduced in a following commit). For this we need fine grain control of the phase. Signed-off-by: Damien Hedde Reviewed-by: Philippe Mathieu-Daudé --- v5: update due to refactor of previous commit --- softmmu/vl.c | 26 +- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/softmmu/vl.c b/softmmu/vl.c index 7f8d15b5b8..ea15e37973 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2698,8 +2698,9 @@ static void qemu_machine_creation_done(void) void qmp_x_exit_preconfig(Error **errp) { -if (phase_check(PHASE_MACHINE_INITIALIZED)) { -error_setg(errp, "The command is permitted only before machine initialization"); +if (phase_check(PHASE_MACHINE_READY)) { +error_setg(errp, "The command is permitted only before" + " machine is ready"); return; } phase_until(PHASE_MACHINE_READY, errp); @@ -2707,9 +2708,6 @@ void qmp_x_exit_preconfig(Error **errp) static void qemu_phase_ready(Error **errp) { -qemu_init_board(); -/* phase is now PHASE_MACHINE_INITIALIZED. */ -qemu_create_cli_devices(); cxl_fixed_memory_window_link_targets(errp); qemu_machine_creation_done(); /* Phase is now PHASE_MACHINE_READY. */ @@ -2749,6 +2747,24 @@ bool phase_until(MachineInitPhase phase, Error **errp) switch (cur_phase) { case PHASE_ACCEL_CREATED: +qemu_init_board(); +/* Phase is now PHASE_MACHINE_INITIALIZED. */ +/* + * Handle CLI devices now in order leave this case in a state + * where we can cold plug devices with QMP. The following call + * handles the CLI options: + * + -fw_cfg (has side effects on device cold plug) + * + -device + */ +qemu_create_cli_devices(); +/* + * At this point all CLI options are handled apart: + * + -S (autostart) + * + -incoming + */ +break; + +case PHASE_MACHINE_INITIALIZED: qemu_phase_ready(errp); break; -- 2.36.1
Re: [PATCH] qom/object: Remove circular include dependency
On 5/9/22 10:46, Philippe Mathieu-Daudé wrote: From: Philippe Mathieu-Daudé "qom/object.h" doesn't need to include itself. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Damien Hedde --- include/qom/object.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/qom/object.h b/include/qom/object.h index 5f3d5b5bf5..ef7258a5e1 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -16,7 +16,6 @@ #include "qapi/qapi-builtin-types.h" #include "qemu/module.h" -#include "qom/object.h" struct TypeImpl; typedef struct TypeImpl *Type;
Re: [PATCH v3 56/60] target/arm: Enable FEAT_CSV2_2 for -cpu max
On 4/17/22 19:44, Richard Henderson wrote: There is no branch prediction in TCG, therefore there is no need to actually include the context number into the predictor. Therefore all we need to do is add the state for SCXTNUM_ELx. > Signed-off-by: Richard Henderson --- v2: Update emulation.rst; clear CSV2_FRAC; use decimal; tidy access_scxtnum. v3: Rely on EL3-no-EL2 squashing during registration. --- docs/system/arm/emulation.rst | 3 ++ target/arm/cpu.h | 16 + target/arm/cpu64.c| 3 +- target/arm/helper.c | 66 ++- 4 files changed, 86 insertions(+), 2 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c @@ -7233,7 +7243,57 @@ static const ARMCPRegInfo mte_el0_cacheop_reginfo[] = { }, }; Hi Richard, I tried to compare with the pseudocode from arm doc and I've a few interrogations. It seems to me there are missing cases in the access checks, but I lack the background to know if these are not checked somewhere else or guaranteed to never happen. -#endif > +static CPAccessResult access_scxtnum(CPUARMState *env, const ARMCPRegInfo *ri, + bool isread) +{ The following checks are missing: + for HFG[W/R]TR_EL2.SCXTNUM_EL0/1 + HCR_EL2. when accessing SCXTNUM_EL1, but maybe these are always guaranteed to fail because we don't support the features ? + HCR_EL2.NV when accessing SCXTNUM_EL2 +int el = arm_current_el(env); + +if (el == 0) { +uint64_t hcr = arm_hcr_el2_eff(env); +if ((hcr & (HCR_TGE | HCR_E2H)) != (HCR_TGE | HCR_E2H)) { +if (env->cp15.sctlr_el[1] & SCTLR_TSCXT) { +if (hcr & HCR_TGE) { +return CP_ACCESS_TRAP_EL2; +} +return CP_ACCESS_TRAP; +} +if (arm_is_el2_enabled(env) && !(hcr & HCR_ENSCXT)) { This case is also present when accessing SCXTNUM_EL0 from el1 (but without "(hcr & (HCR_TGE | HCR_E2H)) != (HCR_TGE | HCR_E2H)" precondition) +return CP_ACCESS_TRAP_EL2; +} +goto no_sctlr_el2; +} +} +if (el < 2 && (env->cp15.sctlr_el[2] & SCTLR_TSCXT)) { +return CP_ACCESS_TRAP_EL2; +} + no_sctlr_el2: +if (el < 3 +&& arm_feature(env, ARM_FEATURE_EL3) +&& !(env->cp15.scr_el3 & SCR_ENSCXT)) { +return CP_ACCESS_TRAP_EL3; +} +return CP_ACCESS_OK; +} Regards, -- Damien
Re: [PATCH] docs/devel: add doc about device life cycles
Any feedback ? -- Thanks, On 4/22/22 16:28, Damien Hedde wrote: Document the 3 life cycles cases that can happen with devices. Signed-off-by: Damien Hedde --- Hi all, It's been a few weeks I wanted to propose this in order to sort out what should be done to make a 'user-creatable' device. This is a follow up of [1] in which Peter asked for this point to be clarified. [1]: https://lore.kernel.org/qemu-devel/a2967493-fd00-8f9b-29bd-56baaae9f...@greensocs.com/ Thanks, Damien --- docs/devel/device.rst | 111 + docs/devel/index-internals.rst | 1 + MAINTAINERS| 1 + 3 files changed, 113 insertions(+) create mode 100644 docs/devel/device.rst diff --git a/docs/devel/device.rst b/docs/devel/device.rst new file mode 100644 index 00..80e3016e80 --- /dev/null +++ b/docs/devel/device.rst @@ -0,0 +1,111 @@ +QEMU device life-cycle +== + +This document details the specifics of devices. + +Devices can be created in two ways: either internally by code or through a +user interface: + ++ command line interface provides ``-device`` option ++ QAPI interface provides ``device_add`` command + +Error handling is most important for the user interfaces. Internal code is +generally designed so that errors do not happen and if some happen, the error +is probably fatal (and QEMU will exit or abort). + +Devices are a particular type of QEMU objects. In addition of the +``instance_init``, ``instance_post_init``, ``unparent`` and +``instance_finalize`` methods (common to all QOM objects), they have the +additional methods: + ++ ``realize`` ++ ``unrealize`` + +In the following we will ignore ``instance_post_init`` and consider is +associated with ``instance_init``. + +``realize`` is the only method that can fail. In that case it should +return an adequate error. Some devices does not do this and should +not be created by means of user interfaces. + +Device succesfully realized +--- + +The normal use case for device is the following: + +1. ``instance_init`` +2. ``realize`` with success +3. The device takes part in emulation +4. ``unrealize`` and ``unparent`` +5. ``instance_finalize`` + +``instance_init`` and ``realize`` are part of the device creation procedure, whereas +``unrealize`` and ``instance_finalize`` are part of the device deletion procedure. + +In case of an object created by code, ``realize`` has to be done explicitly +(eg: by calling ``qdev_realize(...)``). This is done automatically in case of a +device created via a user interface. + +On the other hand ``unrealize`` is done automatically. +``unparent`` will take care of unrealizing the device then undoing any bus +relationships (children and parent). + +Note that ``instance_finalize`` may not occur just after ``unrealize`` because +other objects than the parent can hold references to a device. It may even not +happen at all if a reference is never released. + +Device realize failure +-- + +This use case is most important when the device is created through a user +interface. The user might for example invalid properties and in that case +realize will fail and the device should then be deleted. + +1. ``instance_init`` +2. ``realize`` failure +3. ``unparent`` +4. ``instance_finalize`` + +Failure to create a device should not leave traces. The emulation state after +that should be as if the device has not be created. ``realize`` and +``instance_finalize`` must take care of this. + +Device help +--- + +Last use case is only a user interface case. When requesting help about a device +type, the following life cycle exists: + +1. ``instance_init`` +2. Introspect device properties +3. ``unparent`` +4. ``instance_finalize`` + +This use case is simple but it means that ``instance_finalize`` cannot assume that +``realize`` has been called. + +Implementation consequences +--- + +A device developer should ensure the above use cases are +supported so that the device is *user-creatable*. + +In particular, fail cases must checked in realize and reported using the error +parameter. One should particularly take care of cleaning correctly whatever has +been previously done if realize fails. Cleaning tasks (eg: memory freeing) can +be done in ``realize`` or ``instance_finalize`` as they will be called in a row by +the user interface. + +To this end ``realize`` must ensure than no additional reference to the device is +dangling when it fails. Otherwise the device will never be finalized and deleted. + +If a device has created some children, they should be deleted as well in the +cleaning process. If ``object_initialize_child()`` was used to create a child +hosted into the device structure, the child memory space will disappear with the +parent. No reference to such child must be dangling to ensure the child will +not survive its parent deletion. + +Although it is not asserted by code, one can assume
[PATCH v2] target/arm: Disable cryptographic instructions when neon is disabled
As of now, cryptographic instructions ISAR fields are never cleared so we can end up with a cpu with cryptographic instructions but no floating-point/neon instructions which is not a possible configuration according to ARM specifications. In QEMU, we have 3 kinds of cpus regarding cryptographic instructions: + no support + cortex-a57/a72: cryptographic extension is optional, floating-point/neon is not. + cortex-a53: crytographic extension is optional as well as floationg-point/neon. But cryptographic requires floating-point/neon support. Therefore we can safely clear the ISAR fields when neon is disabled. Note that other arm cpus seem to follow this. For example cortex-a55 is like cortex-a53 and cortex-a76/cortex-a710 are like cortex-a57/a72. Signed-off-by: Damien Hedde --- v2: also clear SHA3 / SM3 / SM4 (Richard) --- target/arm/cpu.c | 9 + 1 file changed, 9 insertions(+) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index e3f8215203..e46a766d77 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1587,6 +1587,12 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) unset_feature(env, ARM_FEATURE_NEON); t = cpu->isar.id_aa64isar0; +t = FIELD_DP64(t, ID_AA64ISAR0, AES, 0); +t = FIELD_DP64(t, ID_AA64ISAR0, SHA1, 0); +t = FIELD_DP64(t, ID_AA64ISAR0, SHA2, 0); +t = FIELD_DP64(t, ID_AA64ISAR0, SHA3, 0); +t = FIELD_DP64(t, ID_AA64ISAR0, SM3, 0); +t = FIELD_DP64(t, ID_AA64ISAR0, SM4, 0); t = FIELD_DP64(t, ID_AA64ISAR0, DP, 0); cpu->isar.id_aa64isar0 = t; @@ -1601,6 +1607,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) cpu->isar.id_aa64pfr0 = t; u = cpu->isar.id_isar5; +u = FIELD_DP32(u, ID_ISAR5, AES, 0); +u = FIELD_DP32(u, ID_ISAR5, SHA1, 0); +u = FIELD_DP32(u, ID_ISAR5, SHA2, 0); u = FIELD_DP32(u, ID_ISAR5, RDM, 0); u = FIELD_DP32(u, ID_ISAR5, VCMA, 0); cpu->isar.id_isar5 = u; -- 2.35.1
[PATCH] target/arm: Disable cryptographic instructions when neon is disabled
As of now, cryptographic instructions ISAR fields are never cleared so we can end up with a cpu with cryptographic instructions but no floating-point/neon instructions which is impossible according to ARM specifications. In QEMU, we have 3 kinds of cpus regarding cryptographic instructions: + no support + cortex-a57/a72: cryptographic extension is optional, floating-point/neon is not. + cortex-a53: crytographic extension is optional as well as floationg-point/neon. But cryptographic requires floating-point/neon support. Therefore we can safely clear the ISAR fields when neon is disabled. Note that other arm cpus seem to follow this. For example cortex-a55 is like cortex-a53 and cortex-a76 is like cortex-57/72. Signed-off-by: Damien Hedde --- Note: if we wanted to provide such configuration, we could have a has_crypto property/feature to represent this. --- target/arm/cpu.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index e3f8215203..e789d18851 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1587,6 +1587,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) unset_feature(env, ARM_FEATURE_NEON); t = cpu->isar.id_aa64isar0; +t = FIELD_DP64(t, ID_AA64ISAR0, AES, 0); +t = FIELD_DP64(t, ID_AA64ISAR0, SHA1, 0); +t = FIELD_DP64(t, ID_AA64ISAR0, SHA2, 0); t = FIELD_DP64(t, ID_AA64ISAR0, DP, 0); cpu->isar.id_aa64isar0 = t; @@ -1601,6 +1604,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) cpu->isar.id_aa64pfr0 = t; u = cpu->isar.id_isar5; +u = FIELD_DP32(u, ID_ISAR5, AES, 0); +u = FIELD_DP32(u, ID_ISAR5, SHA1, 0); +u = FIELD_DP32(u, ID_ISAR5, SHA2, 0); u = FIELD_DP32(u, ID_ISAR5, RDM, 0); u = FIELD_DP32(u, ID_ISAR5, VCMA, 0); cpu->isar.id_isar5 = u; -- 2.35.1
[PATCH] docs/devel: add doc about device life cycles
Document the 3 life cycles cases that can happen with devices. Signed-off-by: Damien Hedde --- Hi all, It's been a few weeks I wanted to propose this in order to sort out what should be done to make a 'user-creatable' device. This is a follow up of [1] in which Peter asked for this point to be clarified. [1]: https://lore.kernel.org/qemu-devel/a2967493-fd00-8f9b-29bd-56baaae9f...@greensocs.com/ Thanks, Damien --- docs/devel/device.rst | 111 + docs/devel/index-internals.rst | 1 + MAINTAINERS| 1 + 3 files changed, 113 insertions(+) create mode 100644 docs/devel/device.rst diff --git a/docs/devel/device.rst b/docs/devel/device.rst new file mode 100644 index 00..80e3016e80 --- /dev/null +++ b/docs/devel/device.rst @@ -0,0 +1,111 @@ +QEMU device life-cycle +== + +This document details the specifics of devices. + +Devices can be created in two ways: either internally by code or through a +user interface: + ++ command line interface provides ``-device`` option ++ QAPI interface provides ``device_add`` command + +Error handling is most important for the user interfaces. Internal code is +generally designed so that errors do not happen and if some happen, the error +is probably fatal (and QEMU will exit or abort). + +Devices are a particular type of QEMU objects. In addition of the +``instance_init``, ``instance_post_init``, ``unparent`` and +``instance_finalize`` methods (common to all QOM objects), they have the +additional methods: + ++ ``realize`` ++ ``unrealize`` + +In the following we will ignore ``instance_post_init`` and consider is +associated with ``instance_init``. + +``realize`` is the only method that can fail. In that case it should +return an adequate error. Some devices does not do this and should +not be created by means of user interfaces. + +Device succesfully realized +--- + +The normal use case for device is the following: + +1. ``instance_init`` +2. ``realize`` with success +3. The device takes part in emulation +4. ``unrealize`` and ``unparent`` +5. ``instance_finalize`` + +``instance_init`` and ``realize`` are part of the device creation procedure, whereas +``unrealize`` and ``instance_finalize`` are part of the device deletion procedure. + +In case of an object created by code, ``realize`` has to be done explicitly +(eg: by calling ``qdev_realize(...)``). This is done automatically in case of a +device created via a user interface. + +On the other hand ``unrealize`` is done automatically. +``unparent`` will take care of unrealizing the device then undoing any bus +relationships (children and parent). + +Note that ``instance_finalize`` may not occur just after ``unrealize`` because +other objects than the parent can hold references to a device. It may even not +happen at all if a reference is never released. + +Device realize failure +-- + +This use case is most important when the device is created through a user +interface. The user might for example invalid properties and in that case +realize will fail and the device should then be deleted. + +1. ``instance_init`` +2. ``realize`` failure +3. ``unparent`` +4. ``instance_finalize`` + +Failure to create a device should not leave traces. The emulation state after +that should be as if the device has not be created. ``realize`` and +``instance_finalize`` must take care of this. + +Device help +--- + +Last use case is only a user interface case. When requesting help about a device +type, the following life cycle exists: + +1. ``instance_init`` +2. Introspect device properties +3. ``unparent`` +4. ``instance_finalize`` + +This use case is simple but it means that ``instance_finalize`` cannot assume that +``realize`` has been called. + +Implementation consequences +--- + +A device developer should ensure the above use cases are +supported so that the device is *user-creatable*. + +In particular, fail cases must checked in realize and reported using the error +parameter. One should particularly take care of cleaning correctly whatever has +been previously done if realize fails. Cleaning tasks (eg: memory freeing) can +be done in ``realize`` or ``instance_finalize`` as they will be called in a row by +the user interface. + +To this end ``realize`` must ensure than no additional reference to the device is +dangling when it fails. Otherwise the device will never be finalized and deleted. + +If a device has created some children, they should be deleted as well in the +cleaning process. If ``object_initialize_child()`` was used to create a child +hosted into the device structure, the child memory space will disappear with the +parent. No reference to such child must be dangling to ensure the child will +not survive its parent deletion. + +Although it is not asserted by code, one can assume ``realize`` will not be tried +again in case of failure
Re: [RFC PATCH 00/18] user-creatable cpu clusters
On 4/21/22 17:51, Peter Maydell wrote: On Wed, 30 Mar 2022 at 13:56, Damien Hedde wrote: Hi, This series add devices to be able to user-create (coldplug) cpu clusters. The existing cpu cluster dictates how cpus are exposed in gdb, but it does not handle the cpu objects creation. This series adds a new device to handle both issues and adds support for two architectures: arm and riscv. Please look at patches 2 and 3 for more details about the new device. Last part concerning the riscv is rfc as I do non-backward compatible updates. I'm not sure what migration (or other) constraints we have on these machines and I probably need to make some changes to cope with them. This series almost deprecates the cpu-cluster type as all uses but one are replaced. I don't think we should have two different concepts which are "a group of CPUs". It means we wind up with two different ways to do something (which we have too many examples of already), only one of which gets to use the nicer, more obvious name. I was not sure that I could merge the cpu-cluster into the new object and agree having both is not ideal. I can 1. delete the last cpu-cluster usecase and the object type 2. manage to update in place the cpu-cluster instead of adding the new type. In the end, the object will be the same. I am also confused by the "cluster" naming. The original cpu-cluster is used only to set cluster_index of cpus which is used in the gdbstub to group cpus in inferiors. So it is just a container to expose cpus in a specific gdb inferior. But in most machines (which don't use explicit cpu-clusters) this cluster[_index] has nothing to do with the topology's cluster. In practice, topology's cluster is not a 1-1 match of gdb inferior in gsbtub ("cpu-cluster") and I'm not sure what to do with that... I'm happy to keep the "cpu-cluster" if everyone feel the same. The stated motivation is to allow user creation of CPU clusters, but I'm not sure how this would work -- CPUs need a lot of things wiring up like interrupt lines and memory regions, which you can't do on the command line anyway. Do you have an example of what the new code lets you do? This simplify the place where cpu-cluster was used (see patch 9 for example) as you don't need to create both the cpus and the cluster. But mostly the goal is to provide cpu cold plug functionality with the qapi interface (not the cli interface). With this series + my other series adding commands to cold-plug devices and memory-map sysbus devices, I reproduced the arm virt board starting from the none machine. Interrupts are wired using qom_set. Thanks, -- Damien
Re: [PATCH v2 1/5] qdev: add user_creatable_requires_machine_allowance class flag
On 4/21/22 17:59, Peter Maydell wrote: On Thu, 31 Mar 2022 at 13:19, Damien Hedde wrote: This flag will be used in device_add to check if the device needs special allowance from the machine model. It will replace the current check based only on the device being a TYPE_SYB_BUS_DEVICE. Signed-off-by: Damien Hedde --- v2: + change the flag name and put it just below user_creatable --- include/hw/qdev-core.h | 9 + hw/core/qdev.c | 1 + hw/core/sysbus.c | 1 + 3 files changed, 11 insertions(+) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 92c3d65208..6a040fcd3b 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -122,6 +122,15 @@ struct DeviceClass { * TODO remove once we're there */ bool user_creatable; +/* + * Some devices can be user created under certain conditions (eg: + * specific machine support for sysbus devices), but it is + * preferable to prevent global allowance for the reasons + * described above. + * This flag is an additional constraint over user_creatable: + * user_creatable still needs to be set to true. + */ +bool user_creatable_requires_machine_allowance; "allowance" doesn't have the meaning you seem to be trying to give it here. (It means "the amount of something you're allowed to have", like a baggage allowance, or "an amount of money you're given for something", like a travel allowance.) Do you mean "user creatable only if the machine permits it" ? Yes. More generally, the pluggable-sysbus stuff is an awful hack that I wish we didn't have to have. I'm not sure I want to see us expanding the use of it to other device types... I not really trying to trigger code when adding devices. I'm just trying to use the related per-machine allow-list as a way to prevent the user to add such devices (specifically cpu group/cluster) on any random machine which would most probably not "work". Thanks, Damien
Re: [RFC PATCH 02/18] hw/cpu/cpus: introduce _cpus_ device
On 4/16/22 19:52, Philippe Mathieu-Daudé wrote: On 30/3/22 14:56, Damien Hedde wrote: This object will be a _cpu-cluster_ generalization and is meant to allow create cpus of the same type. The main goal is that this object, on contrary to _cpu-cluster-_, can be used to dynamically create cpus: it does not rely on external code to populate the object with cpus. Allowing the user to create a cpu cluster and each _cpu_ separately would be hard because of the following reasons: + cpu reset need to be handled + instantiation and realize of cpu-cluster and the cpus are interleaved + cpu cluster must contains only identical cpus and it seems difficult to check that at runtime. Therefore we add a new type solving all this constraints. _cpu-cluster_ will be updated to inherit from this class in following commits. Signed-off-by: Damien Hedde --- include/hw/cpu/cpus.h | 71 +++ hw/cpu/cpus.c | 127 ++ hw/cpu/meson.build | 2 +- 3 files changed, 199 insertions(+), 1 deletion(-) create mode 100644 include/hw/cpu/cpus.h create mode 100644 hw/cpu/cpus.c diff --git a/include/hw/cpu/cpus.h b/include/hw/cpu/cpus.h new file mode 100644 index 00..c65f568ef8 --- /dev/null +++ b/include/hw/cpu/cpus.h @@ -0,0 +1,71 @@ +/* + * QEMU CPUs type + * + * Copyright (c) 2022 GreenSocs + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef HW_CPU_CPUS_H +#define HW_CPU_CPUS_H + +#include "qemu/typedefs.h" +#include "hw/qdev-core.h" +#include "qom/object.h" + +/* + * This object represent several CPUs which are all identical. Typo "represents". + * + * If CPUs are not identical (for example, Cortex-A53 and Cortex-A57 CPUs in an + * Arm big.LITTLE system) they should be in different groups. If the CPUs do + * not have the same view of memory (for example the main CPU and a management + * controller processor) they should be in different groups. This description calls for a clearer CpusGroupState name instead of CpusState (which confuses me with CPUState). Alternatively CpusArrayState. Your are right, I'll add the "group" suffix. Thanks, Damien
Re: [RFC PATCH 00/18] user-creatable cpu clusters
Ping ! It would be nice to have some rough feedback about this series. I plan to submit it in 2 sub-series (riscv-array in a separate part) but would like to know first if this direction looks ok to you ? Note that I'm a bit confused by the terminology. Should a "cluster" as in "machine topology's cluster" be a 1-1 match with a gdb inferior ? Thanks, Damien On 3/30/22 14:56, Damien Hedde wrote: Hi, This series add devices to be able to user-create (coldplug) cpu clusters. The existing cpu cluster dictates how cpus are exposed in gdb, but it does not handle the cpu objects creation. This series adds a new device to handle both issues and adds support for two architectures: arm and riscv. Please look at patches 2 and 3 for more details about the new device. Last part concerning the riscv is rfc as I do non-backward compatible updates. I'm not sure what migration (or other) constraints we have on these machines and I probably need to make some changes to cope with them. This series almost deprecates the cpu-cluster type as all uses but one are replaced. It is organized as follows: + Patches 1 to 7 adds a new base device to replace cpu-cluster + Patches 8 and 9 adds an arm specific version and replace existing clusters in the xlnx-zynqmp machine. + patches 10 to 17 updates the riscv_array. It was already used to create cpus but was not a cpu cluster. Thanks for any comments, -- Damien Damien Hedde (18): define MAX_CLUSTERS in cpu.h instead of cluster.h hw/cpu/cpus: introduce _cpus_ device hw/cpu/cpus: prepare to handle cpu clusters hw/cpu/cluster: make _cpu-cluster_ a subclass of _cpus_ gdbstub: deal with _cpus_ object instead of _cpu-cluster_ hw/cpu/cluster: remove cluster_id now that gdbstub is updated hw/cpu/cpus: add a common start-powered-off property hw/arm/arm_cpus: add arm_cpus device hw/arm/xlnx-zynqmp: convert cpu clusters to arm_cpus hw/riscv/riscv_hart: prepare transition to cpus hw/riscv: prepare riscv_hart transition to cpus hw/riscv/virt: prepare riscv_hart transition to cpus hw/riscv/spike: prepare riscv_hart transition to cpus hw/riscv/riscv_hart: use cpus as base class hw/riscv/sifive_u_pfsoc: apply riscv_hart_array update hw/riscv: update remaining machines due to riscv_hart_array update hw/riscv/riscv_hart: remove temporary features add myself as reviewer of the newly added _cpus_ include/hw/arm/arm_cpus.h | 45 +++ include/hw/arm/xlnx-zynqmp.h | 8 +- include/hw/core/cpu.h | 6 + include/hw/cpu/cluster.h | 26 ++-- include/hw/cpu/cpus.h | 93 ++ include/hw/riscv/microchip_pfsoc.h | 2 - include/hw/riscv/riscv_hart.h | 25 +++- include/hw/riscv/sifive_u.h| 2 - gdbstub.c | 12 +- hw/arm/arm_cpus.c | 63 ++ hw/arm/xlnx-zynqmp.c | 121 +++--- hw/cpu/cluster.c | 53 hw/cpu/cpus.c | 195 + hw/riscv/boot.c| 2 +- hw/riscv/microchip_pfsoc.c | 28 + hw/riscv/opentitan.c | 4 +- hw/riscv/riscv_hart.c | 44 ++- hw/riscv/shakti_c.c| 4 +- hw/riscv/sifive_e.c| 4 +- hw/riscv/sifive_u.c| 31 ++--- hw/riscv/spike.c | 18 +-- hw/riscv/virt.c| 79 +++- MAINTAINERS| 3 + hw/arm/meson.build | 1 + hw/cpu/meson.build | 2 +- 25 files changed, 612 insertions(+), 259 deletions(-) create mode 100644 include/hw/arm/arm_cpus.h create mode 100644 include/hw/cpu/cpus.h create mode 100644 hw/arm/arm_cpus.c create mode 100644 hw/cpu/cpus.c
Re: [PATCH] hw/vfio/common: Fix a small boundary issue of a trace
On 4/6/22 10:14, chenxiang via wrote: From: Xiang Chen Right now the trace of vfio_region_sparse_mmap_entry is as follows: vfio_region_sparse_mmap_entry sparse entry 0 [0x1000 - 0x9000] Actually the range it wants to show is [0x1000 - 0x8fff],so fix it. Signed-off-by: Xiang Chen --- hw/vfio/common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 080046e3f5..0b3808caf8 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1546,7 +1546,7 @@ static int vfio_setup_region_sparse_mmaps(VFIORegion *region, for (i = 0, j = 0; i < sparse->nr_areas; i++) { trace_vfio_region_sparse_mmap_entry(i, sparse->areas[i].offset, sparse->areas[i].offset + -sparse->areas[i].size); +sparse->areas[i].size - 1); if (sparse->areas[i].size) { region->mmaps[j].offset = sparse->areas[i].offset; If the size if zero, the trace will be weird with an underflow if offset is zero as well. Maybe just change the trace by inverting the right bracket ? eg: [0x1000 - 0x9000[ Or don't trace in that case ? (but I am not maintainer of this, so maybe that does not make sense). -- Damien
Re: [PATCH 12/32] qga: replace deprecated g_get_current_time()
On 3/23/22 16:57, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau According to GLib API: g_get_current_time has been deprecated since version 2.62 and should not be used in newly-written code. GTimeVal is not year-2038-safe. Use g_get_real_time() instead. Signed-off-by: Marc-André Lureau --- qga/main.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/qga/main.c b/qga/main.c index b9dd19918e47..1deb0ee2fbfe 100644 --- a/qga/main.c +++ b/qga/main.c @@ -314,7 +314,6 @@ static void ga_log(const gchar *domain, GLogLevelFlags level, const gchar *msg, gpointer opaque) { GAState *s = opaque; -GTimeVal time; const char *level_str = ga_log_level_str(level); if (!ga_logging_enabled(s)) { @@ -329,9 +328,11 @@ static void ga_log(const gchar *domain, GLogLevelFlags level, #else if (level & s->log_level) { #endif -g_get_current_time(); +gint64 t = g_get_real_time(); fprintf(s->log_file, -"%lu.%lu: %s: %s\n", time.tv_sec, time.tv_usec, level_str, msg); +"%" G_GINT64_FORMAT ".%" G_GINT64_FORMAT +": %s: %s\n", t / G_USEC_PER_SEC, t % G_USEC_PER_SEC, +level_str, msg); fflush(s->log_file); } } Reviewed-by: Damien Hedde -- Damien
Re: [PATCH] docs/ccid: convert to restructuredText
On 4/5/22 16:29, oxr...@gmx.us wrote: From: Lucas Ramage Buglink: https://gitlab.com/qemu-project/qemu/-/issues/527 Signed-off-by: Lucas Ramage Provided 2 minors tweaks (see below: missing empty line, and empty line at EOF), Reviewed-by: Damien Hedde Note that I'm not competent regarding the content of this doc. But it corresponds to the previous version and the doc generation works. --- docs/ccid.txt| 182 --- docs/system/device-emulation.rst | 1 + docs/system/devices/ccid.rst | 171 + 3 files changed, 172 insertions(+), 182 deletions(-) delete mode 100644 docs/ccid.txt create mode 100644 docs/system/devices/ccid.rst diff --git a/docs/ccid.txt b/docs/ccid.txt deleted file mode 100644 index 2b85b1bd42..00 --- a/docs/ccid.txt +++ /dev/null @@ -1,182 +0,0 @@ -QEMU CCID Device Documentation. - -Contents -1. USB CCID device -2. Building -3. Using ccid-card-emulated with hardware -4. Using ccid-card-emulated with certificates -5. Using ccid-card-passthru with client side hardware -6. Using ccid-card-passthru with client side certificates -7. Passthrough protocol scenario -8. libcacard - -1. USB CCID device - -The USB CCID device is a USB device implementing the CCID specification, which -lets one connect smart card readers that implement the same spec. For more -information see the specification: - - Universal Serial Bus - Device Class: Smart Card - CCID - Specification for - Integrated Circuit(s) Cards Interface Devices - Revision 1.1 - April 22rd, 2005 - -Smartcards are used for authentication, single sign on, decryption in -public/private schemes and digital signatures. A smartcard reader on the client -cannot be used on a guest with simple usb passthrough since it will then not be -available on the client, possibly locking the computer when it is "removed". On -the other hand this device can let you use the smartcard on both the client and -the guest machine. It is also possible to have a completely virtual smart card -reader and smart card (i.e. not backed by a physical device) using this device. - -2. Building - -The cryptographic functions and access to the physical card is done via the -libcacard library, whose development package must be installed prior to -building QEMU: - -In redhat/fedora: -yum install libcacard-devel -In ubuntu: -apt-get install libcacard-dev - -Configuring and building: -./configure --enable-smartcard && make - - -3. Using ccid-card-emulated with hardware - -Assuming you have a working smartcard on the host with the current -user, using libcacard, QEMU acts as another client using ccid-card-emulated: - -qemu -usb -device usb-ccid -device ccid-card-emulated - - -4. Using ccid-card-emulated with certificates stored in files - -You must create the CA and card certificates. This is a one time process. -We use NSS certificates: - -mkdir fake-smartcard -cd fake-smartcard -certutil -N -d sql:$PWD -certutil -S -d sql:$PWD -s "CN=Fake Smart Card CA" -x -t TC,TC,TC -n fake-smartcard-ca -certutil -S -d sql:$PWD -t ,, -s "CN=John Doe" -n id-cert -c fake-smartcard-ca -certutil -S -d sql:$PWD -t ,, -s "CN=John Doe (signing)" --nsCertType smime -n signing-cert -c fake-smartcard-ca -certutil -S -d sql:$PWD -t ,, -s "CN=John Doe (encryption)" --nsCertType sslClient -n encryption-cert -c fake-smartcard-ca - -Note: you must have exactly three certificates. - -You can use the emulated card type with the certificates backend: - -qemu -usb -device usb-ccid -device ccid-card-emulated,backend=certificates,db=sql:$PWD,cert1=id-cert,cert2=signing-cert,cert3=encryption-cert - -To use the certificates in the guest, export the CA certificate: - -certutil -L -r -d sql:$PWD -o fake-smartcard-ca.cer -n fake-smartcard-ca - -and import it in the guest: - -certutil -A -d /etc/pki/nssdb -i fake-smartcard-ca.cer -t TC,TC,TC -n fake-smartcard-ca - -In a Linux guest you can then use the CoolKey PKCS #11 module to access -the card: - -certutil -d /etc/pki/nssdb -L -h all - -It will prompt you for the PIN (which is the password you assigned to the -certificate database early on), and then show you all three certificates -together with the manually imported CA cert: - -Certificate NicknameTrust Attributes -fake-smartcard-ca CT,C,C -John Doe:CAC ID Certificate u,u,u -John Doe:CAC Email Signature Certificateu,u,u -John Doe:CAC Email Encryption Certificate u,u,u - -If this does not happen, CoolKey is not installed or not registered with -NSS. Registration can be done from Firefox or the command line: - -modutil -dbdir /etc/pki/nssdb -add "CAC Module" -libfile /usr/lib64/pkcs11/libcoolkeypk11.so -modutil -dbdir /etc/pki/nssdb -list - - -5. Using ccid-card-passthru with clien
Re: [RFC PATCH] python: add qmp-send program to send raw qmp commands to qemu
On 4/5/22 07:41, Markus Armbruster wrote: Daniel P. Berrangé writes: On Wed, Mar 16, 2022 at 10:54:55AM +0100, Damien Hedde wrote: It takes an input file containing raw qmp commands (concatenated json dicts) and send all commands one by one to a qmp server. When one command fails, it exits. As a convenience, it can also wrap the qemu process to avoid having to start qemu in background. When wrapping qemu, the program returns only when the qemu process terminates. Signed-off-by: Damien Hedde [...] I name that qmp-send as Daniel proposed, maybe qmp-test matches better what I'm doing there ? 'qmp-test' is a use case specific name. I think it is better to name it based on functionality provided rather than anticipated use case, since use cases evolve over time, hence 'qmp-send'. Well, it doesn't just send, it also receives. qmpcat, like netcat and socat? anyone against qmpcat ? -- Damien
Re: [qemu.qmp PATCH 10/13] docs: add versioning policy to README
On 3/30/22 20:24, John Snow wrote: The package is in an alpha state, but there's a method to the madness. Signed-off-by: John Snow --- README.rst | 21 + 1 file changed, 21 insertions(+) diff --git a/README.rst b/README.rst index 8593259..88efe84 100644 --- a/README.rst +++ b/README.rst @@ -154,6 +154,27 @@ fail. These checks use their own virtual environments and won't pollute your working space. +Stability and Versioning + + +This package uses a major.minor.micro SemVer versioning, with the +following additional semantics during the alpha/beta period (Major +version 0): + +This package treats 0.0.z versions as "alpha" versions. Each micro +version update may change the API incompatibly. Early users are advised +to pin against explicit versions, but check for updates often. + +A planned 0.1.z version will introduce the first "beta", whereafter each +micro update will be backwards compatible, but each minor update will +not be. The first beta version will be released after legacy.py is +removed, and the API is tentatively "stable". + +Thereafter, normal SemVer/PEP440 rules will apply; micro updates will +always be bugfixes, and minor updates will be reserved for backwards +compatible feature changes. + + Changelog - Looks reasonable to me. Reviewed-by: Damien Hedde
Re: [RFC PATCH] python: add qmp-send program to send raw qmp commands to qemu
On 4/4/22 22:34, John Snow wrote: On Wed, Mar 16, 2022 at 5:55 AM Damien Hedde wrote: It takes an input file containing raw qmp commands (concatenated json dicts) and send all commands one by one to a qmp server. When one command fails, it exits. As a convenience, it can also wrap the qemu process to avoid having to start qemu in background. When wrapping qemu, the program returns only when the qemu process terminates. Signed-off-by: Damien Hedde --- Hi all, Following our discussion, I've started this. What do you think ? I tried to follow Daniel's qmp-shell-wrap. I think it is better to have similar options (eg: logging). There is also room for factorizing code if we want to keep them aligned and ease maintenance. There are still some pylint issues (too many branches in main and it does not like my context manager if else line). But it's kind of a mess to fix theses so I think it's enough for a first version. Yeah, don't worry about these. You can just tell pylint to shut up while you prototype. Sometimes it's just not worth spending more time on a more beautiful factoring. Oh well. I name that qmp-send as Daniel proposed, maybe qmp-test matches better what I'm doing there ? I think I agree with Dan's response. Thanks, Damien --- python/qemu/aqmp/qmp_send.py | 229 +++ I recommend putting this in qemu/util/qmp_send.py instead. I'm in the process of pulling out the AQMP lib and hosting it separately. Scripts like this I think should stay in the QEMU tree, so moving it to util instead is probably best. Otherwise, I'll *really* have to commit to the syntax, and that's probably a bigger hurdle than you want to deal with. If it stays in QEMU tree, what licensing should I use ? LGPL does not hurt, no ? scripts/qmp/qmp-send | 11 ++ 2 files changed, 240 insertions(+) create mode 100644 python/qemu/aqmp/qmp_send.py create mode 100755 scripts/qmp/qmp-send diff --git a/python/qemu/aqmp/qmp_send.py b/python/qemu/aqmp/qmp_send.py new file mode 100644 index 00..cbca1d0205 --- /dev/null +++ b/python/qemu/aqmp/qmp_send.py Seems broadly fine to me, but I didn't review closely this time. If it works for you, it works for me. As for making QEMU hang: there's a few things you could do, take a look at iotests and see how they handle timeout blocks in synchronous code -- iotests.py line 696 or so, "class Timeout". When writing async code, you can also do stuff like this: async def foo(): await asyncio.wait_for(qmp.execute("some-command", args_etc), timeout=30) See https://docs.python.org/3/library/asyncio-task.html#asyncio.wait_for --js Thanks for the tip, -- Damien
Re: [PATCH] plugins: Assert mmu_idx in range before use in qemu_plugin_get_hwaddr
Reviewed-by: Damien Hedde On 4/1/22 21:02, Richard Henderson wrote: Coverity reports out-of-bound accesses here. This should be a false positive due to how the index is decoded from MemOpIdx. Fixes: Coverity CID 1487201 Signed-off-by: Richard Henderson --- plugins/api.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/api.c b/plugins/api.c index 7bf71b189d..2078b16edb 100644 --- a/plugins/api.c +++ b/plugins/api.c @@ -289,6 +289,8 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info, enum qemu_plugin_mem_rw rw = get_plugin_meminfo_rw(info); hwaddr_info.is_store = (rw & QEMU_PLUGIN_MEM_W) != 0; +assert(mmu_idx < NB_MMU_MODES); + if (!tlb_plugin_lookup(cpu, vaddr, mmu_idx, hwaddr_info.is_store, _info)) { error_report("invalid use of qemu_plugin_get_hwaddr");
Re: [PATCH] target/i386: Suppress coverity warning on fsave/frstor
On 4/1/22 20:46, Richard Henderson wrote: Coverity warns that 14 << data32 may overflow with respect to the target_ulong to which it is subsequently added. We know this wasn't true because data32 is in [1,2], but the suggested fix is perfectly fine. Fixes: Coverity CID 1487135, 1487256 Signed-off-by: Richard Henderson --- target/i386/tcg/fpu_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c index ebf5e73df9..30bc44fcf8 100644 --- a/target/i386/tcg/fpu_helper.c +++ b/target/i386/tcg/fpu_helper.c @@ -2466,7 +2466,7 @@ static void do_fsave(CPUX86State *env, target_ulong ptr, int data32, do_fstenv(env, ptr, data32, retaddr); -ptr += (14 << data32); +ptr += (target_ulong)14 << data32; for (i = 0; i < 8; i++) { tmp = ST(i); do_fstt(env, tmp, ptr, retaddr); @@ -2488,7 +2488,7 @@ static void do_frstor(CPUX86State *env, target_ulong ptr, int data32, int i; do_fldenv(env, ptr, data32, retaddr); -ptr += (14 << data32); +ptr += (target_ulong)14 << data32; for (i = 0; i < 8; i++) { tmp = do_fldt(env, ptr, retaddr); Reviewed-by: Damien Hedde
Re: [PATCH v5 3/4] hw/intc/exynos4210: replace 'qemu_split_irq' in combiner and gic
On 3/24/22 19:15, Zongyuan Li wrote: Signed-off-by: Zongyuan Li --- hw/arm/exynos4210.c | 26 +++ hw/intc/exynos4210_combiner.c | 81 +++ hw/intc/exynos4210_gic.c | 36 +--- include/hw/arm/exynos4210.h | 11 ++--- include/hw/core/split-irq.h | 5 +-- 5 files changed, 126 insertions(+), 33 deletions(-) diff --git a/include/hw/core/split-irq.h b/include/hw/core/split-irq.h index ff8852f407..eb485dd7a6 100644 --- a/include/hw/core/split-irq.h +++ b/include/hw/core/split-irq.h @@ -42,9 +42,6 @@ #define MAX_SPLIT_LINES 16 - -OBJECT_DECLARE_SIMPLE_TYPE(SplitIRQ, SPLIT_IRQ) - struct SplitIRQ { DeviceState parent_obj; @@ -52,4 +49,6 @@ struct SplitIRQ { uint16_t num_lines; }; +OBJECT_DECLARE_SIMPLE_TYPE(SplitIRQ, SPLIT_IRQ) + #endif Is there a reason to move the OBJECT_DECLARE_SIMPLE_TYPE line ? -- Damien
Re: [RFC PATCH 0/4] hw/i2c: i2c slave mode support
On 4/1/22 08:29, Klaus Jensen wrote: On Mar 31 15:32, Corey Minyard wrote: On Thu, Mar 31, 2022 at 06:57:33PM +0200, Klaus Jensen wrote: From: Klaus Jensen Hi all, This RFC series adds I2C "slave mode" support for the Aspeed I2C controller as well as the necessary infrastructure in the i2c core to support this. I've been wondering when this would happen :). I had put some thought into how this would work, but hadn't come up with anything good. The big disadvantage of this is you are adding an interface that is incompatible with the current masters and slaves. So you are using the same I2C bus, but slaves written this way cannot talk to existing masters, and masters written this way cannot talk to existing slave. You could adapt the masters to be able to work either way, and I suppose some slaves that could do it could have both an async send and a normal send. Would it make sense to introduce a QOM Interface to differentiate between the slave/master types? Probably. I expect a normal slave-only I2C device will be compatible with any master (having or having not this feature) in real life ? It would be great if the compatibility between "a I2C slave requiring the slave-mode from the bus" and the bus could be checked during the device plug. -- Damien
Re: [PATCH 1/2] doc/style: CLang -> Clang
Reviewed-by: Damien Hedde On 3/31/22 15:56, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau It's not the way it is usually written (see https://clang.llvm.org/). Signed-off-by: Marc-André Lureau --- docs/devel/style.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/devel/style.rst b/docs/devel/style.rst index 9e66d133e15b..7ddd42b6c2c8 100644 --- a/docs/devel/style.rst +++ b/docs/devel/style.rst @@ -522,7 +522,7 @@ documented in the GNU Compiler Collection manual starting at version 4.0. Automatic memory deallocation = -QEMU has a mandatory dependency either the GCC or CLang compiler. As +QEMU has a mandatory dependency on either the GCC or the Clang compiler. As such it has the freedom to make use of a C language extension for automatically running a cleanup function when a stack variable goes out of scope. This can be used to simplify function cleanup paths,
Re: [PATCH 2/2] doc/build-platforms: document supported compilers
Reviewed-by: Damien Hedde On 3/31/22 15:56, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau According to our configure checks, this is the list of supported compilers. Signed-off-by: Marc-André Lureau --- docs/about/build-platforms.rst | 10 ++ 1 file changed, 10 insertions(+) diff --git a/docs/about/build-platforms.rst b/docs/about/build-platforms.rst index c29a4b8fe649..1980c5d2476f 100644 --- a/docs/about/build-platforms.rst +++ b/docs/about/build-platforms.rst @@ -92,6 +92,16 @@ hosted on Linux (Debian/Fedora). The version of the Windows API that's currently targeted is Vista / Server 2008. +Supported compilers +--- + +To compile, QEMU requires either: + +- GCC >= 7.4.0 +- Clang >= 6.0 +- XCode Clang >= 10.0 + + .. _HomeBrew: https://brew.sh/ .. _MacPorts: https://www.macports.org/ .. _Repology: https://repology.org/
[PATCH v2 4/5] rename machine_class_allow_dynamic_sysbus_dev
All callsite are updated to the new function name "machine_class_allow_dynamic_device" Signed-off-by: Damien Hedde Reviewed-by: Philippe Mathieu-Daudé --- hw/arm/virt.c | 10 +- hw/i386/microvm.c | 2 +- hw/i386/pc_piix.c | 4 ++-- hw/i386/pc_q35.c| 8 hw/ppc/e500plat.c | 2 +- hw/ppc/spapr.c | 2 +- hw/riscv/virt.c | 2 +- hw/xen/xen-legacy-backend.c | 2 +- 8 files changed, 16 insertions(+), 16 deletions(-) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index d2e5ecd234..1442b8840b 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -2829,12 +2829,12 @@ static void virt_machine_class_init(ObjectClass *oc, void *data) * configuration of the particular instance. */ mc->max_cpus = 512; -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC); -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE); -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE); -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM); +machine_class_allow_dynamic_device(mc, TYPE_VFIO_CALXEDA_XGMAC); +machine_class_allow_dynamic_device(mc, TYPE_VFIO_AMD_XGBE); +machine_class_allow_dynamic_device(mc, TYPE_RAMFB_DEVICE); +machine_class_allow_dynamic_device(mc, TYPE_VFIO_PLATFORM); #ifdef CONFIG_TPM -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS); +machine_class_allow_dynamic_device(mc, TYPE_TPM_TIS_SYSBUS); #endif mc->block_default_type = IF_VIRTIO; mc->no_cdrom = 1; diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c index 4b3b1dd262..4f8f423d31 100644 --- a/hw/i386/microvm.c +++ b/hw/i386/microvm.c @@ -756,7 +756,7 @@ static void microvm_class_init(ObjectClass *oc, void *data) MICROVM_MACHINE_AUTO_KERNEL_CMDLINE, "Set off to disable adding virtio-mmio devices to the kernel cmdline"); -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE); +machine_class_allow_dynamic_device(mc, TYPE_RAMFB_DEVICE); } static const TypeInfo microvm_machine_info = { diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index b72c03d0a6..27373cb16a 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -411,8 +411,8 @@ static void pc_i440fx_machine_options(MachineClass *m) m->desc = "Standard PC (i440FX + PIIX, 1996)"; m->default_machine_opts = "firmware=bios-256k.bin"; m->default_display = "std"; -machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE); -machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE); +machine_class_allow_dynamic_device(m, TYPE_RAMFB_DEVICE); +machine_class_allow_dynamic_device(m, TYPE_VMBUS_BRIDGE); } static void pc_i440fx_7_0_machine_options(MachineClass *m) diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 1780f79bc1..8221615fa4 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -353,10 +353,10 @@ static void pc_q35_machine_options(MachineClass *m) m->default_display = "std"; m->default_kernel_irqchip_split = false; m->no_floppy = 1; -machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE); -machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE); -machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE); -machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE); +machine_class_allow_dynamic_device(m, TYPE_AMD_IOMMU_DEVICE); +machine_class_allow_dynamic_device(m, TYPE_INTEL_IOMMU_DEVICE); +machine_class_allow_dynamic_device(m, TYPE_RAMFB_DEVICE); +machine_class_allow_dynamic_device(m, TYPE_VMBUS_BRIDGE); m->max_cpus = 288; } diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c index fc911bbb7b..273cde9d06 100644 --- a/hw/ppc/e500plat.c +++ b/hw/ppc/e500plat.c @@ -102,7 +102,7 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data) mc->max_cpus = 32; mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30"); mc->default_ram_id = "mpc8544ds.ram"; -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ETSEC_COMMON); +machine_class_allow_dynamic_device(mc, TYPE_ETSEC_COMMON); } static const TypeInfo e500plat_info = { diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index a4372ba189..70e12d9037 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -4586,7 +4586,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) mc->default_ram_id = "ppc_spapr.ram"; mc->default_display = "std"; mc->kvm_type = spapr_kvm_type; -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SPAPR_PCI_HOST_BRIDGE); +machine_class_allow_dynamic_device(mc, TYPE_SPAPR_PCI_HOST_BRIDGE); mc->pci_allow_0_address = true; assert(!mc->get_hotplug_handler); mc->get_hotplug_handler = spapr_get_hotplug_handler; diff --git a/hw/riscv/virt.
[PATCH v2 1/5] qdev: add user_creatable_requires_machine_allowance class flag
This flag will be used in device_add to check if the device needs special allowance from the machine model. It will replace the current check based only on the device being a TYPE_SYB_BUS_DEVICE. Signed-off-by: Damien Hedde --- v2: + change the flag name and put it just below user_creatable --- include/hw/qdev-core.h | 9 + hw/core/qdev.c | 1 + hw/core/sysbus.c | 1 + 3 files changed, 11 insertions(+) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 92c3d65208..6a040fcd3b 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -122,6 +122,15 @@ struct DeviceClass { * TODO remove once we're there */ bool user_creatable; +/* + * Some devices can be user created under certain conditions (eg: + * specific machine support for sysbus devices), but it is + * preferable to prevent global allowance for the reasons + * described above. + * This flag is an additional constraint over user_creatable: + * user_creatable still needs to be set to true. + */ +bool user_creatable_requires_machine_allowance; bool hotpluggable; /* callbacks */ diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 84f3019440..0844c85a21 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -833,6 +833,7 @@ static void device_class_init(ObjectClass *class, void *data) */ dc->hotpluggable = true; dc->user_creatable = true; +dc->user_creatable_requires_machine_allowance = false; vc->get_id = device_vmstate_if_get_id; rc->get_state = device_get_reset_state; rc->child_foreach = device_reset_child_foreach; diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c index 05c1da3d31..5f771ed1e9 100644 --- a/hw/core/sysbus.c +++ b/hw/core/sysbus.c @@ -325,6 +325,7 @@ static void sysbus_device_class_init(ObjectClass *klass, void *data) * subclass needs to override it and set user_creatable=true. */ k->user_creatable = false; +k->user_creatable_requires_machine_allowance = true; } static const TypeInfo sysbus_device_type_info = { -- 2.35.1
[PATCH v2 5/5] machine: remove temporary inline functions
Now we have renamed all calls to these old functions, we can delete the temporary inline we've defined. Signed-off-by: Damien Hedde Reviewed-by: Philippe Mathieu-Daudé --- include/hw/boards.h | 10 -- 1 file changed, 10 deletions(-) diff --git a/include/hw/boards.h b/include/hw/boards.h index 1814793175..7efba048e9 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -52,11 +52,6 @@ void machine_parse_smp_config(MachineState *ms, * it will get an error message. */ void machine_class_allow_dynamic_device(MachineClass *mc, const char *type); -static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, - const char *type) -{ -machine_class_allow_dynamic_device(mc, type); -} /** * device_type_is_dynamic_allowed: Check if type is an allowed device @@ -72,11 +67,6 @@ static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, * Note that if @type has a parent type in the list, it is allowed too. */ bool device_type_is_dynamic_allowed(MachineClass *mc, const char *type); -static inline bool device_type_is_dynamic_sysbus(MachineClass *mc, - const char *type) -{ -return device_type_is_dynamic_allowed(mc, type); -} /** * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device -- 2.35.1
[PATCH v2 0/5] Generalize the sysbus device machine allowance
Hi all, This series transforms the TYPE_SYSBUS_DEVICE allowed list that exists in machine class model into a TYPE_DEVICE allowed list. This will allow to add non-sysbus device into this list to prevent the user to create them on most machines. Typical use case will be for example cpu related devices like these developed in the following series: https://lore.kernel.org/qemu-devel/20220330125639.201937-1-damien.he...@greensocs.com/ Patches 1 and 3 lack a review. Thanks, -- Damien v2: + update the flag name and put it just below user_creatable (Philippe) Damien Hedde (5): qdev: add user_creatable_requires_machine_allowance class flag machine: update machine allowed list related functions/fields qdev-monitor: use the new user_creatable_requires_machine_allowance rename machine_class_allow_dynamic_sysbus_dev machine: remove temporary inline functions include/hw/boards.h | 40 ++--- include/hw/qdev-core.h | 9 + hw/arm/virt.c | 10 +- hw/core/machine.c | 10 +- hw/core/qdev.c | 1 + hw/core/sysbus.c| 1 + hw/i386/microvm.c | 2 +- hw/i386/pc_piix.c | 4 ++-- hw/i386/pc_q35.c| 8 hw/ppc/e500plat.c | 2 +- hw/ppc/spapr.c | 2 +- hw/riscv/virt.c | 2 +- hw/xen/xen-legacy-backend.c | 2 +- softmmu/qdev-monitor.c | 8 14 files changed, 56 insertions(+), 45 deletions(-) -- 2.35.1
[PATCH v2 2/5] machine: update machine allowed list related functions/fields
The list will now accept any device (not only sysbus devices) so we rename the related code and documentation. Create some temporary inline functions with old names until we've udpated callsites as well. Signed-off-by: Damien Hedde Reviewed-by: Philippe Mathieu-Daudé --- include/hw/boards.h | 50 +++-- hw/core/machine.c | 10 - 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/include/hw/boards.h b/include/hw/boards.h index c92ac8815c..1814793175 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -38,35 +38,45 @@ void machine_parse_smp_config(MachineState *ms, const SMPConfiguration *config, Error **errp); /** - * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices + * machine_class_allow_dynamic_device: Add type to list of valid devices * @mc: Machine class - * @type: type to allow (should be a subtype of TYPE_SYS_BUS_DEVICE) + * @type: type to allow (should be a subtype of TYPE_DEVICE having the + *uc_requires_machine_allowance flag) * * Add the QOM type @type to the list of devices of which are subtypes - * of TYPE_SYS_BUS_DEVICE but which are still permitted to be dynamically - * created (eg by the user on the command line with -device). - * By default if the user tries to create any devices on the command line - * that are subtypes of TYPE_SYS_BUS_DEVICE they will get an error message; - * for the special cases which are permitted for this machine model, the - * machine model class init code must call this function to add them - * to the list of specifically permitted devices. + * of TYPE_DEVICE but which are only permitted to be dynamically + * created (eg by the user on the command line with -device) if the + * machine allowed it. + * + * Otherwise if the user tries to create such a device on the command line, + * it will get an error message. */ -void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type); +void machine_class_allow_dynamic_device(MachineClass *mc, const char *type); +static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, + const char *type) +{ +machine_class_allow_dynamic_device(mc, type); +} /** - * device_type_is_dynamic_sysbus: Check if type is an allowed sysbus device + * device_type_is_dynamic_allowed: Check if type is an allowed device * type for the machine class. * @mc: Machine class - * @type: type to check (should be a subtype of TYPE_SYS_BUS_DEVICE) + * @type: type to check (should be a subtype of TYPE_DEVICE) * * Returns: true if @type is a type in the machine's list of - * dynamically pluggable sysbus devices; otherwise false. + * dynamically pluggable devices; otherwise false. * - * Check if the QOM type @type is in the list of allowed sysbus device - * types (see machine_class_allowed_dynamic_sysbus_dev()). + * Check if the QOM type @type is in the list of allowed device + * types (see machine_class_allowed_dynamic_device()). * Note that if @type has a parent type in the list, it is allowed too. */ -bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type); +bool device_type_is_dynamic_allowed(MachineClass *mc, const char *type); +static inline bool device_type_is_dynamic_sysbus(MachineClass *mc, + const char *type) +{ +return device_type_is_dynamic_allowed(mc, type); +} /** * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device @@ -74,12 +84,12 @@ bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type); * @dev: device to check * * Returns: true if @dev is a sysbus device on the machine's list - * of dynamically pluggable sysbus devices; otherwise false. + * of dynamically pluggable devices; otherwise false. * * This function checks whether @dev is a valid dynamic sysbus device, * by first confirming that it is a sysbus device and then checking it - * against the list of permitted dynamic sysbus devices which has been - * set up by the machine using machine_class_allow_dynamic_sysbus_dev(). + * against the list of permitted dynamic devices which has been + * set up by the machine using machine_class_allow_dynamic_device(). * * It is valid to call this with something that is not a subclass of * TYPE_SYS_BUS_DEVICE; the function will return false in this case. @@ -263,7 +273,7 @@ struct MachineClass { bool ignore_memory_transaction_failures; int numa_mem_align_shift; const char **valid_cpu_types; -strList *allowed_dynamic_sysbus_devices; +strList *allowed_dynamic_devices; bool auto_enable_numa_with_memhp; bool auto_enable_numa_with_memdev; bool ignore_boot_device_suffixes; diff --git a/hw/core/machine.c b/hw/core/machine.c index d856485cb4..fb1f7c8e5a 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -545,9 +545,9 @@ static void
[PATCH v2 3/5] qdev-monitor: use the new user_creatable_requires_machine_allowance
Instead of checking if the device is a sysbus device, just check the newly added flag in device class. Signed-off-by: Damien Hedde --- v2: update the flag name --- softmmu/qdev-monitor.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 12fe60c467..77f468358d 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -258,12 +258,12 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp) return NULL; } -if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) { -/* sysbus devices need to be allowed by the machine */ +if (dc->user_creatable_requires_machine_allowance) { +/* some devices need to be allowed by the machine */ MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine())); -if (!device_type_is_dynamic_sysbus(mc, *driver)) { +if (!device_type_is_dynamic_allowed(mc, *driver)) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver", - "a dynamic sysbus device type for the machine"); + "the device type is not allowed for this machine"); return NULL; } } -- 2.35.1
Re: [PATCH 1/5] qdev: add uc_requires_machine_allowance flag
On 3/31/22 12:21, Philippe Mathieu-Daudé wrote: On 30/3/22 18:12, Damien Hedde wrote: This flag will be used in device_add to check if the device needs special allowance from the machine model. It will replace the current check based only on the device being a TYPE_SYB_BUS_DEVICE. Signed-off-by: Damien Hedde --- include/hw/qdev-core.h | 6 ++ hw/core/qdev.c | 1 + hw/core/sysbus.c | 1 + 3 files changed, 8 insertions(+) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 92c3d65208..f5a05ced39 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -123,6 +123,12 @@ struct DeviceClass { */ bool user_creatable; bool hotpluggable; + /* + * Some devices (eg: sysbus devices) are only user-creatable if + * the machine allowed it. user_creatable need still to be set to + * true, this is an additional constraint. + */ + bool uc_requires_machine_allowance; Why not name it user_creatable_requires_machine_allowance? Also I'd put it just after user_creatable. I was worried about the length being too long when initially writing this patch. Since we use it only doing the following: > if (dc->user_creatable_requires_machine_allowance) or > dc->user_creatable_requires_machine_allowance = false; The length will be ok anyway, so I'll rename it, it is less confusing. I'll also move it ahead and respin. Thanks, Damien
[PATCH 5/5] machine: remove temporary inline functions
Now we have renamed all call to these old functions, we can delete the temporary inline we've defined. Signed-off-by: Damien Hedde --- include/hw/boards.h | 10 -- 1 file changed, 10 deletions(-) diff --git a/include/hw/boards.h b/include/hw/boards.h index 9ce7d705c9..7efba048e9 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -52,11 +52,6 @@ void machine_parse_smp_config(MachineState *ms, * it will get an error message. */ void machine_class_allow_dynamic_device(MachineClass *mc, const char *type); -static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, - const char *type) -{ -machine_class_allow_dynamic_device(mc, type); -} /** * device_type_is_dynamic_allowed: Check if type is an allowed device @@ -72,11 +67,6 @@ static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, * Note that if @type has a parent type in the list, it is allowed too. */ bool device_type_is_dynamic_allowed(MachineClass *mc, const char *type); -static inline void device_type_is_dynamic_sysbus(MachineClass *mc, - const char *type) -{ -device_type_is_dynamic_allowed(mc, type); -} /** * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device -- 2.35.1
[PATCH 2/5] machine: update machine allowed list related functions/fields
The list will now accept any device (not only sysbus devices) so we rename the related code and documentation. Create some temporary inline functions with old names until we've udpated callsites as well. Signed-off-by: Damien Hedde --- include/hw/boards.h | 50 +++-- hw/core/machine.c | 10 - 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/include/hw/boards.h b/include/hw/boards.h index c92ac8815c..9ce7d705c9 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -38,35 +38,45 @@ void machine_parse_smp_config(MachineState *ms, const SMPConfiguration *config, Error **errp); /** - * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices + * machine_class_allow_dynamic_device: Add type to list of valid devices * @mc: Machine class - * @type: type to allow (should be a subtype of TYPE_SYS_BUS_DEVICE) + * @type: type to allow (should be a subtype of TYPE_DEVICE having the + *uc_requires_machine_allowance flag) * * Add the QOM type @type to the list of devices of which are subtypes - * of TYPE_SYS_BUS_DEVICE but which are still permitted to be dynamically - * created (eg by the user on the command line with -device). - * By default if the user tries to create any devices on the command line - * that are subtypes of TYPE_SYS_BUS_DEVICE they will get an error message; - * for the special cases which are permitted for this machine model, the - * machine model class init code must call this function to add them - * to the list of specifically permitted devices. + * of TYPE_DEVICE but which are only permitted to be dynamically + * created (eg by the user on the command line with -device) if the + * machine allowed it. + * + * Otherwise if the user tries to create such a device on the command line, + * it will get an error message. */ -void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type); +void machine_class_allow_dynamic_device(MachineClass *mc, const char *type); +static inline void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, + const char *type) +{ +machine_class_allow_dynamic_device(mc, type); +} /** - * device_type_is_dynamic_sysbus: Check if type is an allowed sysbus device + * device_type_is_dynamic_allowed: Check if type is an allowed device * type for the machine class. * @mc: Machine class - * @type: type to check (should be a subtype of TYPE_SYS_BUS_DEVICE) + * @type: type to check (should be a subtype of TYPE_DEVICE) * * Returns: true if @type is a type in the machine's list of - * dynamically pluggable sysbus devices; otherwise false. + * dynamically pluggable devices; otherwise false. * - * Check if the QOM type @type is in the list of allowed sysbus device - * types (see machine_class_allowed_dynamic_sysbus_dev()). + * Check if the QOM type @type is in the list of allowed device + * types (see machine_class_allowed_dynamic_device()). * Note that if @type has a parent type in the list, it is allowed too. */ -bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type); +bool device_type_is_dynamic_allowed(MachineClass *mc, const char *type); +static inline void device_type_is_dynamic_sysbus(MachineClass *mc, + const char *type) +{ +device_type_is_dynamic_allowed(mc, type); +} /** * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device @@ -74,12 +84,12 @@ bool device_type_is_dynamic_sysbus(MachineClass *mc, const char *type); * @dev: device to check * * Returns: true if @dev is a sysbus device on the machine's list - * of dynamically pluggable sysbus devices; otherwise false. + * of dynamically pluggable devices; otherwise false. * * This function checks whether @dev is a valid dynamic sysbus device, * by first confirming that it is a sysbus device and then checking it - * against the list of permitted dynamic sysbus devices which has been - * set up by the machine using machine_class_allow_dynamic_sysbus_dev(). + * against the list of permitted dynamic devices which has been + * set up by the machine using machine_class_allow_dynamic_device(). * * It is valid to call this with something that is not a subclass of * TYPE_SYS_BUS_DEVICE; the function will return false in this case. @@ -263,7 +273,7 @@ struct MachineClass { bool ignore_memory_transaction_failures; int numa_mem_align_shift; const char **valid_cpu_types; -strList *allowed_dynamic_sysbus_devices; +strList *allowed_dynamic_devices; bool auto_enable_numa_with_memhp; bool auto_enable_numa_with_memdev; bool ignore_boot_device_suffixes; diff --git a/hw/core/machine.c b/hw/core/machine.c index d856485cb4..fb1f7c8e5a 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -545,9 +545,9 @@ static void machine_set_nvdimm_persistence(Object *obj
[PATCH 1/5] qdev: add uc_requires_machine_allowance flag
This flag will be used in device_add to check if the device needs special allowance from the machine model. It will replace the current check based only on the device being a TYPE_SYB_BUS_DEVICE. Signed-off-by: Damien Hedde --- include/hw/qdev-core.h | 6 ++ hw/core/qdev.c | 1 + hw/core/sysbus.c | 1 + 3 files changed, 8 insertions(+) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 92c3d65208..f5a05ced39 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -123,6 +123,12 @@ struct DeviceClass { */ bool user_creatable; bool hotpluggable; +/* + * Some devices (eg: sysbus devices) are only user-creatable if + * the machine allowed it. user_creatable need still to be set to + * true, this is an additional constraint. + */ +bool uc_requires_machine_allowance; /* callbacks */ /* diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 84f3019440..0825277521 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -833,6 +833,7 @@ static void device_class_init(ObjectClass *class, void *data) */ dc->hotpluggable = true; dc->user_creatable = true; +dc->uc_requires_machine_allowance = false; vc->get_id = device_vmstate_if_get_id; rc->get_state = device_get_reset_state; rc->child_foreach = device_reset_child_foreach; diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c index 05c1da3d31..462eb1116c 100644 --- a/hw/core/sysbus.c +++ b/hw/core/sysbus.c @@ -325,6 +325,7 @@ static void sysbus_device_class_init(ObjectClass *klass, void *data) * subclass needs to override it and set user_creatable=true. */ k->user_creatable = false; +k->uc_requires_machine_allowance = true; } static const TypeInfo sysbus_device_type_info = { -- 2.35.1
[PATCH 3/5] qdev-monitor: use the newly uc_requires_machine_allowance
Signed-off-by: Damien Hedde --- softmmu/qdev-monitor.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 12fe60c467..94e5f35127 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -258,12 +258,12 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp) return NULL; } -if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) { -/* sysbus devices need to be allowed by the machine */ +if (dc->uc_requires_machine_allowance) { +/* some devices need to be allowed by the machine */ MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine())); -if (!device_type_is_dynamic_sysbus(mc, *driver)) { +if (!device_type_is_dynamic_allowed(mc, *driver)) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver", - "a dynamic sysbus device type for the machine"); + "the device type is not allowed for this machine"); return NULL; } } -- 2.35.1
[PATCH 4/5] rename machine_class_allow_dynamic_sysbus_dev
All callsite are updated to the new function name "machine_class_allow_dynamic_device" Signed-off-by: Damien Hedde --- hw/arm/virt.c | 10 +- hw/i386/microvm.c | 2 +- hw/i386/pc_piix.c | 4 ++-- hw/i386/pc_q35.c| 8 hw/ppc/e500plat.c | 2 +- hw/ppc/spapr.c | 2 +- hw/riscv/virt.c | 2 +- hw/xen/xen-legacy-backend.c | 2 +- 8 files changed, 16 insertions(+), 16 deletions(-) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index d2e5ecd234..1442b8840b 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -2829,12 +2829,12 @@ static void virt_machine_class_init(ObjectClass *oc, void *data) * configuration of the particular instance. */ mc->max_cpus = 512; -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC); -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE); -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE); -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM); +machine_class_allow_dynamic_device(mc, TYPE_VFIO_CALXEDA_XGMAC); +machine_class_allow_dynamic_device(mc, TYPE_VFIO_AMD_XGBE); +machine_class_allow_dynamic_device(mc, TYPE_RAMFB_DEVICE); +machine_class_allow_dynamic_device(mc, TYPE_VFIO_PLATFORM); #ifdef CONFIG_TPM -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS); +machine_class_allow_dynamic_device(mc, TYPE_TPM_TIS_SYSBUS); #endif mc->block_default_type = IF_VIRTIO; mc->no_cdrom = 1; diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c index 4b3b1dd262..4f8f423d31 100644 --- a/hw/i386/microvm.c +++ b/hw/i386/microvm.c @@ -756,7 +756,7 @@ static void microvm_class_init(ObjectClass *oc, void *data) MICROVM_MACHINE_AUTO_KERNEL_CMDLINE, "Set off to disable adding virtio-mmio devices to the kernel cmdline"); -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE); +machine_class_allow_dynamic_device(mc, TYPE_RAMFB_DEVICE); } static const TypeInfo microvm_machine_info = { diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index b72c03d0a6..27373cb16a 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -411,8 +411,8 @@ static void pc_i440fx_machine_options(MachineClass *m) m->desc = "Standard PC (i440FX + PIIX, 1996)"; m->default_machine_opts = "firmware=bios-256k.bin"; m->default_display = "std"; -machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE); -machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE); +machine_class_allow_dynamic_device(m, TYPE_RAMFB_DEVICE); +machine_class_allow_dynamic_device(m, TYPE_VMBUS_BRIDGE); } static void pc_i440fx_7_0_machine_options(MachineClass *m) diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 1780f79bc1..8221615fa4 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -353,10 +353,10 @@ static void pc_q35_machine_options(MachineClass *m) m->default_display = "std"; m->default_kernel_irqchip_split = false; m->no_floppy = 1; -machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE); -machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE); -machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE); -machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE); +machine_class_allow_dynamic_device(m, TYPE_AMD_IOMMU_DEVICE); +machine_class_allow_dynamic_device(m, TYPE_INTEL_IOMMU_DEVICE); +machine_class_allow_dynamic_device(m, TYPE_RAMFB_DEVICE); +machine_class_allow_dynamic_device(m, TYPE_VMBUS_BRIDGE); m->max_cpus = 288; } diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c index fc911bbb7b..273cde9d06 100644 --- a/hw/ppc/e500plat.c +++ b/hw/ppc/e500plat.c @@ -102,7 +102,7 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data) mc->max_cpus = 32; mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30"); mc->default_ram_id = "mpc8544ds.ram"; -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ETSEC_COMMON); +machine_class_allow_dynamic_device(mc, TYPE_ETSEC_COMMON); } static const TypeInfo e500plat_info = { diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index a4372ba189..70e12d9037 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -4586,7 +4586,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) mc->default_ram_id = "ppc_spapr.ram"; mc->default_display = "std"; mc->kvm_type = spapr_kvm_type; -machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SPAPR_PCI_HOST_BRIDGE); +machine_class_allow_dynamic_device(mc, TYPE_SPAPR_PCI_HOST_BRIDGE); mc->pci_allow_0_address = true; assert(!mc->get_hotplug_handler); mc->get_hotplug_handler = spapr_get_hotplug_handler; diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index da5
[PATCH 0/5] Generalize the sysbus device machine allowance
Hi all, This series transforms the TYPE_SYSBUS_DEVICE allowed list that exists in machine class model into a TYPE_DEVICE allowed list. This will allow to add non-sysbus device into this list to prevent the user to create them on most machines. Typical use case will be for example cpu related devices like these developed in the following series: https://lore.kernel.org/qemu-devel/20220330125639.201937-1-damien.he...@greensocs.com/ Thanks, -- Damien Damien Hedde (5): qdev: add uc_requires_machine_allowance flag machine: update machine allowed list related functions/fields qdev-monitor: use the newly uc_requires_machine_allowance rename machine_class_allow_dynamic_sysbus_dev machine: remove temporary inline functions include/hw/boards.h | 40 ++--- include/hw/qdev-core.h | 6 ++ hw/arm/virt.c | 10 +- hw/core/machine.c | 10 +- hw/core/qdev.c | 1 + hw/core/sysbus.c| 1 + hw/i386/microvm.c | 2 +- hw/i386/pc_piix.c | 4 ++-- hw/i386/pc_q35.c| 8 hw/ppc/e500plat.c | 2 +- hw/ppc/spapr.c | 2 +- hw/riscv/virt.c | 2 +- hw/xen/xen-legacy-backend.c | 2 +- softmmu/qdev-monitor.c | 8 14 files changed, 53 insertions(+), 45 deletions(-) -- 2.35.1
[RFC PATCH 17/18] hw/riscv/riscv_hart: remove temporary features
Now that we updated all riscv machines, we can remove the temporary realize helper and the alias property. Signed-off-by: Damien Hedde --- include/hw/riscv/riscv_hart.h | 3 --- hw/riscv/riscv_hart.c | 14 -- 2 files changed, 17 deletions(-) diff --git a/include/hw/riscv/riscv_hart.h b/include/hw/riscv/riscv_hart.h index 65ac0d2bc4..acf5ee8575 100644 --- a/include/hw/riscv/riscv_hart.h +++ b/include/hw/riscv/riscv_hart.h @@ -56,7 +56,4 @@ static inline unsigned riscv_array_get_num_harts(RISCVHartArrayState *s) return CPUS(s)->topology.cpus; } -/* Temporary function until we migrated the riscv hart array to simple device */ -void riscv_hart_array_realize(RISCVHartArrayState *state, Error **errp); - #endif diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c index 1b4ff7e3c6..ea798de5d5 100644 --- a/hw/riscv/riscv_hart.c +++ b/hw/riscv/riscv_hart.c @@ -27,13 +27,6 @@ #include "hw/riscv/riscv_hart.h" #include "hw/cpu/cpus.h" -void riscv_hart_array_realize(RISCVHartArrayState *state, Error **errp) -{ -/* disable the clustering */ -cpus_disable_clustering(CPUS(state)); -qdev_realize(DEVICE(state), NULL, errp); -} - static Property riscv_harts_props[] = { DEFINE_PROP_UINT32("hartid-base", RISCVHartArrayState, hartid_base, 0), DEFINE_PROP_UINT64("resetvec", RISCVHartArrayState, resetvec, @@ -52,12 +45,6 @@ static void riscv_harts_configure_cpu(CpusState *base, CPUState *cpu, cpuenv->mhartid = s->hartid_base + i; } -static void riscv_harts_init(Object *obj) -{ -/* add a temporary property to keep num-harts */ -object_property_add_alias(obj, "num-harts", obj, "num-cpus"); -} - static void riscv_harts_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -73,7 +60,6 @@ static const TypeInfo riscv_harts_info = { .name = TYPE_RISCV_HART_ARRAY, .parent= TYPE_CPUS, .instance_size = sizeof(RISCVHartArrayState), -.instance_init = riscv_harts_init, .class_init= riscv_harts_class_init, }; -- 2.35.1
[RFC PATCH 18/18] add myself as reviewer of the newly added _cpus_
Add cpus into the "machine core" subset along with cpu cluster and add myself as reviewer. Signed-off-by: Damien Hedde --- Should we put all cpu groups in the same subset ? hw/cpu/cluster hw/cpu/cpus hw/arm/arm_cpus hw/riscv/riscv_array --- MAINTAINERS | 3 +++ 1 file changed, 3 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index cc364afef7..80b9331d02 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1678,6 +1678,7 @@ M: Eduardo Habkost M: Marcel Apfelbaum R: Philippe Mathieu-Daudé R: Yanan Wang +R: Damien Hedde S: Supported F: cpu.c F: hw/core/cpu.c @@ -1687,11 +1688,13 @@ F: hw/core/machine-smp.c F: hw/core/null-machine.c F: hw/core/numa.c F: hw/cpu/cluster.c +F: hw/cpu/cpus.c F: qapi/machine.json F: qapi/machine-target.json F: include/hw/boards.h F: include/hw/core/cpu.h F: include/hw/cpu/cluster.h +F: include/hw/cpu/cpus.h F: include/sysemu/numa.h F: tests/unit/test-smp-parse.c T: git https://gitlab.com/ehabkost/qemu.git machine-next -- 2.35.1
[RFC PATCH 16/18] hw/riscv: update remaining machines due to riscv_hart_array update
virt & spike machines have multiple sockets (but they still put all cpus in the same default cluster). For these 2 machines we disable the clustering in the array in order to keep the behavior. opentitan, sifive_e and shakti_c, were using the default cluster too. But we can use the embedded cluster as it is equivalent. Signed-off-by: Damien Hedde --- hw/riscv/opentitan.c | 4 ++-- hw/riscv/shakti_c.c | 4 ++-- hw/riscv/sifive_e.c | 4 ++-- hw/riscv/spike.c | 5 +++-- hw/riscv/virt.c | 5 +++-- 5 files changed, 12 insertions(+), 10 deletions(-) diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c index 2eb7454d8a..4e90610f1d 100644 --- a/hw/riscv/opentitan.c +++ b/hw/riscv/opentitan.c @@ -132,10 +132,10 @@ static void lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp) object_property_set_str(OBJECT(>cpus), "cpu-type", ms->cpu_type, _abort); -object_property_set_int(OBJECT(>cpus), "num-harts", ms->smp.cpus, +object_property_set_int(OBJECT(>cpus), "num-cpus", ms->smp.cpus, _abort); object_property_set_int(OBJECT(>cpus), "resetvec", 0x8080, _abort); -riscv_hart_array_realize(>cpus, _abort); +qdev_realize(DEVICE(>cpus), NULL, _abort); /* Boot ROM */ memory_region_init_rom(>rom, OBJECT(dev_soc), "riscv.lowrisc.ibex.rom", diff --git a/hw/riscv/shakti_c.c b/hw/riscv/shakti_c.c index 93e0c8dd68..5158ea6e8b 100644 --- a/hw/riscv/shakti_c.c +++ b/hw/riscv/shakti_c.c @@ -108,7 +108,7 @@ static void shakti_c_soc_state_realize(DeviceState *dev, Error **errp) ShaktiCSoCState *sss = RISCV_SHAKTI_SOC(dev); MemoryRegion *system_memory = get_system_memory(); -riscv_hart_array_realize(>cpus, _abort); +qdev_realize(DEVICE(>cpus), NULL, _abort); sss->plic = sifive_plic_create(shakti_c_memmap[SHAKTI_C_PLIC].base, (char *)SHAKTI_C_PLIC_HART_CONFIG, ms->smp.cpus, 0, @@ -171,7 +171,7 @@ static void shakti_c_soc_instance_init(Object *obj) */ object_property_set_str(OBJECT(>cpus), "cpu-type", TYPE_RISCV_CPU_SHAKTI_C, _abort); -object_property_set_int(OBJECT(>cpus), "num-harts", 1, +object_property_set_int(OBJECT(>cpus), "num-cpus", 1, _abort); } diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c index 25ba0a8c85..c1e67c0f78 100644 --- a/hw/riscv/sifive_e.c +++ b/hw/riscv/sifive_e.c @@ -179,7 +179,7 @@ static void sifive_e_soc_init(Object *obj) SiFiveESoCState *s = RISCV_E_SOC(obj); object_initialize_child(obj, "cpus", >cpus, TYPE_RISCV_HART_ARRAY); -object_property_set_int(OBJECT(>cpus), "num-harts", ms->smp.cpus, +object_property_set_int(OBJECT(>cpus), "num-cpus", ms->smp.cpus, _abort); object_property_set_int(OBJECT(>cpus), "resetvec", 0x1004, _abort); object_initialize_child(obj, "riscv.sifive.e.gpio0", >gpio, @@ -195,7 +195,7 @@ static void sifive_e_soc_realize(DeviceState *dev, Error **errp) object_property_set_str(OBJECT(>cpus), "cpu-type", ms->cpu_type, _abort); -riscv_hart_array_realize(>cpus, _abort); +qdev_realize(DEVICE(>cpus), NULL, _abort); /* Mask ROM */ memory_region_init_rom(>mask_rom, OBJECT(dev), "riscv.sifive.e.mrom", diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index b75e3656e1..17bba3c7fa 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -228,9 +228,10 @@ static void spike_board_init(MachineState *machine) machine->cpu_type, _abort); object_property_set_int(OBJECT(>soc[i]), "hartid-base", base_hartid, _abort); -object_property_set_int(OBJECT(>soc[i]), "num-harts", +object_property_set_int(OBJECT(>soc[i]), "num-cpus", hart_count, _abort); -riscv_hart_array_realize(>soc[i], _abort); +cpus_disable_clustering(CPUS(>soc[i])); +qdev_realize(DEVICE(>soc[i]), NULL, _abort); /* Core Local Interruptor (timer and IPI) for each socket */ riscv_aclint_swi_create( diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 12036aa95b..c51b124330 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1221,9 +1221,10 @@ static void virt_machine_init(MachineState *machine) machine->cpu_type, _abort); object_property_set_int(OBJECT(>soc[i]), "hartid-base", base_hartid, _abort); -object_property_set_int(OBJECT(>soc[i]), "num-harts", +object_property_set_int(OBJECT(>soc[i]), "num-cpus", hart_count, _abort); -riscv_hart_array_realize(>soc[i], _abort); +cpus_disable_clustering(CPUS(>soc[i])); +qdev_realize(DEVICE(>soc[i]), NULL, _abort); if (!kvm_enabled()) { if (s->have_aclint) { -- 2.35.1
[RFC PATCH 12/18] hw/riscv/virt: prepare riscv_hart transition to cpus
+ Use riscv_array_get_num_harts instead of accessing num_harts field. + Use riscv_array_get_hart instead of accessing harts field. + Use riscv_hart_array_realize instead of sysbus_realize. Signed-off-by: Damien Hedde --- hw/riscv/virt.c | 76 ++--- 1 file changed, 46 insertions(+), 30 deletions(-) diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index da50cbed43..12036aa95b 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -223,8 +223,11 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket, uint32_t cpu_phandle; MachineState *mc = MACHINE(s); char *name, *cpu_name, *core_name, *intc_name; +unsigned num_harts; -for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) { +num_harts = riscv_array_get_num_harts(>soc[socket]); + +for (cpu = num_harts - 1; cpu >= 0; cpu--) { cpu_phandle = (*phandle)++; cpu_name = g_strdup_printf("/cpus/cpu@%d", @@ -232,7 +235,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket, qemu_fdt_add_subnode(mc->fdt, cpu_name); qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type", (is_32_bit) ? "riscv,sv32" : "riscv,sv48"); -name = riscv_isa_string(>soc[socket].harts[cpu]); +name = riscv_isa_string(riscv_array_get_hart(>soc[socket], cpu)); qemu_fdt_setprop_string(mc->fdt, cpu_name, "riscv,isa", name); g_free(name); qemu_fdt_setprop_string(mc->fdt, cpu_name, "compatible", "riscv"); @@ -249,7 +252,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket, qemu_fdt_add_subnode(mc->fdt, intc_name); qemu_fdt_setprop_cell(mc->fdt, intc_name, "phandle", intc_phandles[cpu]); -if (riscv_feature(>soc[socket].harts[cpu].env, +if (riscv_feature(_array_get_hart(>soc[socket], cpu)->env, RISCV_FEATURE_AIA)) { static const char * const compat[2] = { "riscv,cpu-intc-aia", "riscv,cpu-intc" @@ -299,14 +302,16 @@ static void create_fdt_socket_clint(RISCVVirtState *s, char *clint_name; uint32_t *clint_cells; unsigned long clint_addr; +unsigned num_harts; MachineState *mc = MACHINE(s); static const char * const clint_compat[2] = { "sifive,clint0", "riscv,clint0" }; -clint_cells = g_new0(uint32_t, s->soc[socket].num_harts * 4); +num_harts = riscv_array_get_num_harts(>soc[socket]); +clint_cells = g_new0(uint32_t, num_harts * 4); -for (cpu = 0; cpu < s->soc[socket].num_harts; cpu++) { +for (cpu = 0; cpu < num_harts; cpu++) { clint_cells[cpu * 4 + 0] = cpu_to_be32(intc_phandles[cpu]); clint_cells[cpu * 4 + 1] = cpu_to_be32(IRQ_M_SOFT); clint_cells[cpu * 4 + 2] = cpu_to_be32(intc_phandles[cpu]); @@ -322,7 +327,7 @@ static void create_fdt_socket_clint(RISCVVirtState *s, qemu_fdt_setprop_cells(mc->fdt, clint_name, "reg", 0x0, clint_addr, 0x0, memmap[VIRT_CLINT].size); qemu_fdt_setprop(mc->fdt, clint_name, "interrupts-extended", -clint_cells, s->soc[socket].num_harts * sizeof(uint32_t) * 4); +clint_cells, num_harts * sizeof(uint32_t) * 4); riscv_socket_fdt_write_id(mc, mc->fdt, clint_name, socket); g_free(clint_name); @@ -340,13 +345,15 @@ static void create_fdt_socket_aclint(RISCVVirtState *s, uint32_t *aclint_mswi_cells; uint32_t *aclint_sswi_cells; uint32_t *aclint_mtimer_cells; +unsigned num_harts; MachineState *mc = MACHINE(s); -aclint_mswi_cells = g_new0(uint32_t, s->soc[socket].num_harts * 2); -aclint_mtimer_cells = g_new0(uint32_t, s->soc[socket].num_harts * 2); -aclint_sswi_cells = g_new0(uint32_t, s->soc[socket].num_harts * 2); +num_harts = riscv_array_get_num_harts(>soc[socket]); +aclint_mswi_cells = g_new0(uint32_t, num_harts * 2); +aclint_mtimer_cells = g_new0(uint32_t, num_harts * 2); +aclint_sswi_cells = g_new0(uint32_t, num_harts * 2); -for (cpu = 0; cpu < s->soc[socket].num_harts; cpu++) { +for (cpu = 0; cpu < num_harts; cpu++) { aclint_mswi_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]); aclint_mswi_cells[cpu * 2 + 1] = cpu_to_be32(IRQ_M_SOFT); aclint_mtimer_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]); @@ -354,7 +361,7 @@ static void create_fdt_socket_aclint(RISCVVirtState *s, aclint_sswi_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]); aclint_sswi_cells[cpu * 2 + 1] = cpu_to_be32(IRQ_S_SOFT); } -aclint_cells_size = s->soc[socket].num_harts * sizeof(uint32_t) * 2; +aclint_cells_size = num_harts * sizeof(uint32_t) * 2; if (s->aia_type != VIRT_AIA_TY
[RFC PATCH 09/18] hw/arm/xlnx-zynqmp: convert cpu clusters to arm_cpus
qom-path of cpus are changed: + "apu-cluster/apu-cpu[n]" to "apu/cpu[n]" + "rpu-cluster/rpu-cpu[n]" to "rpu/cpu[n]" Signed-off-by: Damien Hedde --- include/hw/arm/xlnx-zynqmp.h | 8 +-- hw/arm/xlnx-zynqmp.c | 121 +-- 2 files changed, 48 insertions(+), 81 deletions(-) diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h index 9d9a9d0bf9..1bcc3f9356 100644 --- a/include/hw/arm/xlnx-zynqmp.h +++ b/include/hw/arm/xlnx-zynqmp.h @@ -31,7 +31,7 @@ #include "hw/display/xlnx_dp.h" #include "hw/intc/xlnx-zynqmp-ipi.h" #include "hw/rtc/xlnx-zynqmp-rtc.h" -#include "hw/cpu/cluster.h" +#include "hw/arm/arm_cpus.h" #include "target/arm/cpu.h" #include "qom/object.h" #include "net/can_emu.h" @@ -94,10 +94,8 @@ struct XlnxZynqMPState { DeviceState parent_obj; /*< public >*/ -CPUClusterState apu_cluster; -CPUClusterState rpu_cluster; -ARMCPU apu_cpu[XLNX_ZYNQMP_NUM_APU_CPUS]; -ARMCPU rpu_cpu[XLNX_ZYNQMP_NUM_RPU_CPUS]; +ArmCpusState apu; +ArmCpusState rpu; GICState gic; MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS][XLNX_ZYNQMP_GIC_ALIASES]; diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c index 5bfe285a19..fa0c093733 100644 --- a/hw/arm/xlnx-zynqmp.c +++ b/hw/arm/xlnx-zynqmp.c @@ -25,6 +25,7 @@ #include "sysemu/kvm.h" #include "sysemu/sysemu.h" #include "kvm_arm.h" +#include "hw/arm/arm_cpus.h" #define GIC_NUM_SPI_INTR 160 @@ -201,7 +202,7 @@ static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index) static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s, const char *boot_cpu, Error **errp) { -int i; +unsigned boot_idx; int num_rpus = MIN(ms->smp.cpus - XLNX_ZYNQMP_NUM_APU_CPUS, XLNX_ZYNQMP_NUM_RPU_CPUS); @@ -210,36 +211,21 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s, return; } -object_initialize_child(OBJECT(s), "rpu-cluster", >rpu_cluster, -TYPE_CPU_CLUSTER); -qdev_prop_set_uint32(DEVICE(>rpu_cluster), "cluster-id", 1); - -for (i = 0; i < num_rpus; i++) { -const char *name; - -object_initialize_child(OBJECT(>rpu_cluster), "rpu-cpu[*]", ->rpu_cpu[i], -ARM_CPU_TYPE_NAME("cortex-r5f")); - -name = object_get_canonical_path_component(OBJECT(>rpu_cpu[i])); -if (strcmp(name, boot_cpu)) { -/* - * Secondary CPUs start in powered-down state. - */ -object_property_set_bool(OBJECT(>rpu_cpu[i]), - "start-powered-off", true, _abort); -} else { -s->boot_cpu_ptr = >rpu_cpu[i]; -} - -object_property_set_bool(OBJECT(>rpu_cpu[i]), "reset-hivecs", true, - _abort); -if (!qdev_realize(DEVICE(>rpu_cpu[i]), NULL, errp)) { -return; -} +/* apus are already created, rpus will have cluster-id 1 */ +object_initialize_child(OBJECT(s), "rpu", >rpu, TYPE_ARM_CPUS); +qdev_prop_set_uint32(DEVICE(>rpu), "num-cpus", num_rpus); +qdev_prop_set_string(DEVICE(>rpu), "cpu-type", + ARM_CPU_TYPE_NAME("cortex-r5f")); +qdev_prop_set_bit(DEVICE(>rpu), "reset-hivecs", true); +qdev_prop_set_bit(DEVICE(>rpu), "start-powered-off", true); + +qdev_realize(DEVICE(>rpu), NULL, _fatal); + +if (sscanf(boot_cpu, "rpu-cpu[%u]", _idx) && boot_idx < num_rpus) { +s->boot_cpu_ptr = arm_cpus_get_cpu(>rpu, boot_idx); +object_property_set_bool(OBJECT(s->boot_cpu_ptr), "start-powered-on", + true, _abort); } - -qdev_realize(DEVICE(>rpu_cluster), NULL, _fatal); } static void xlnx_zynqmp_create_bbram(XlnxZynqMPState *s, qemu_irq *gic) @@ -296,7 +282,8 @@ static void xlnx_zynqmp_create_apu_ctrl(XlnxZynqMPState *s, qemu_irq *gic) g_autofree gchar *name = g_strdup_printf("cpu%d", i); object_property_set_link(OBJECT(>apu_ctrl), name, - OBJECT(>apu_cpu[i]), _abort); + OBJECT(arm_cpus_get_cpu(>apu, i)), + _abort); } sysbus_realize(sbd, _fatal); @@ -349,15 +336,10 @@ static void xlnx_zynqmp_init(Object *obj) int i; int num_apus = MIN(ms->smp.cpus, XLNX_ZYNQMP_NUM_APU_CPUS); -object_initialize_child(obj, "apu-cluster", &g
[RFC PATCH 15/18] hw/riscv/sifive_u_pfsoc: apply riscv_hart_array update
These machines were creating 2 explicit clusters of different cpus (a cluster of 1 cpu and a cluster of N cpus). These are removed as they are now embedded in the riscv array. Note: The qom-path of the riscv hart arrays are changed, the cluster level is removed: + "/path/to/e-cluster/e-cpus" to "/path/to/e-cpus" + "/path/to/u-cluster/u-cpus" to "/path/to/u-cpus" Signed-off-by: Damien Hedde --- If keeping the qom-paths is necessary we can add a container as a replacement of the cluster. --- include/hw/riscv/microchip_pfsoc.h | 2 -- include/hw/riscv/sifive_u.h| 2 -- hw/riscv/microchip_pfsoc.c | 28 ++-- hw/riscv/sifive_u.c| 27 ++- 4 files changed, 12 insertions(+), 47 deletions(-) diff --git a/include/hw/riscv/microchip_pfsoc.h b/include/hw/riscv/microchip_pfsoc.h index a0673f5f59..9101c94978 100644 --- a/include/hw/riscv/microchip_pfsoc.h +++ b/include/hw/riscv/microchip_pfsoc.h @@ -35,8 +35,6 @@ typedef struct MicrochipPFSoCState { DeviceState parent_obj; /*< public >*/ -CPUClusterState e_cluster; -CPUClusterState u_cluster; RISCVHartArrayState e_cpus; RISCVHartArrayState u_cpus; DeviceState *plic; diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h index 8f63a183c4..5439e0d0c3 100644 --- a/include/hw/riscv/sifive_u.h +++ b/include/hw/riscv/sifive_u.h @@ -38,8 +38,6 @@ typedef struct SiFiveUSoCState { DeviceState parent_obj; /*< public >*/ -CPUClusterState e_cluster; -CPUClusterState u_cluster; RISCVHartArrayState e_cpus; RISCVHartArrayState u_cpus; DeviceState *plic; diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c index 82547a53e6..f4b1400ba5 100644 --- a/hw/riscv/microchip_pfsoc.c +++ b/hw/riscv/microchip_pfsoc.c @@ -133,23 +133,15 @@ static void microchip_pfsoc_soc_instance_init(Object *obj) MachineState *ms = MACHINE(qdev_get_machine()); MicrochipPFSoCState *s = MICROCHIP_PFSOC(obj); -object_initialize_child(obj, "e-cluster", >e_cluster, TYPE_CPU_CLUSTER); -qdev_prop_set_uint32(DEVICE(>e_cluster), "cluster-id", 0); - -object_initialize_child(OBJECT(>e_cluster), "e-cpus", >e_cpus, -TYPE_RISCV_HART_ARRAY); -qdev_prop_set_uint32(DEVICE(>e_cpus), "num-harts", 1); +object_initialize_child(obj, "e-cpus", >e_cpus, TYPE_RISCV_HART_ARRAY); +qdev_prop_set_uint32(DEVICE(>e_cpus), "num-cpus", 1); qdev_prop_set_uint32(DEVICE(>e_cpus), "hartid-base", 0); qdev_prop_set_string(DEVICE(>e_cpus), "cpu-type", TYPE_RISCV_CPU_SIFIVE_E51); qdev_prop_set_uint64(DEVICE(>e_cpus), "resetvec", RESET_VECTOR); -object_initialize_child(obj, "u-cluster", >u_cluster, TYPE_CPU_CLUSTER); -qdev_prop_set_uint32(DEVICE(>u_cluster), "cluster-id", 1); - -object_initialize_child(OBJECT(>u_cluster), "u-cpus", >u_cpus, -TYPE_RISCV_HART_ARRAY); -qdev_prop_set_uint32(DEVICE(>u_cpus), "num-harts", ms->smp.cpus - 1); +object_initialize_child(obj, "u-cpus", >u_cpus, TYPE_RISCV_HART_ARRAY); +qdev_prop_set_uint32(DEVICE(>u_cpus), "num-cpus", ms->smp.cpus - 1); qdev_prop_set_uint32(DEVICE(>u_cpus), "hartid-base", 1); qdev_prop_set_string(DEVICE(>u_cpus), "cpu-type", TYPE_RISCV_CPU_SIFIVE_U54); @@ -190,16 +182,8 @@ static void microchip_pfsoc_soc_realize(DeviceState *dev, Error **errp) NICInfo *nd; int i; -riscv_hart_array_realize(>e_cpus, _abort); -riscv_hart_array_realize(>u_cpus, _abort); -/* - * The cluster must be realized after the RISC-V hart array container, - * as the container's CPU object is only created on realize, and the - * CPU must exist and have been parented into the cluster before the - * cluster is realized. - */ -qdev_realize(DEVICE(>e_cluster), NULL, _abort); -qdev_realize(DEVICE(>u_cluster), NULL, _abort); +qdev_realize(DEVICE(>e_cpus), NULL, _abort); +qdev_realize(DEVICE(>u_cpus), NULL, _abort); /* Reserved Memory at address 0 */ memory_region_init_ram(rsvd0_mem, NULL, "microchip.pfsoc.rsvd0_mem", diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index c99e92a7eb..1d9a7c5bf1 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -44,7 +44,6 @@ #include "hw/loader.h" #include "hw/sysbus.h" #include "hw/char/serial.h" -#include "hw/cpu/cluster.h" #include "hw/misc/unimp.h" #include "hw/sd/sd.h" #include "hw/ssi/ssi.h" @@ -786,20 +785,14 @@ static void sifive_u_soc_instance
[RFC PATCH 14/18] hw/riscv/riscv_hart: use cpus as base class
This is a drastic update: qom-path of riscv harts are changed by this patch from "/path/to/array/hart[n]" to "/path/to/array/cpu[n]". This object is now not anymore a SYS_BUS_DEVICE. Signed-off-by: Damien Hedde --- I expect it is an issue regarding migration of riscv machines. It is possible to keep an "hart[n]" alias to "cpu[n]" or even to be able to configure the cpu child basename to "hart". I'm not sure what we need to keep migration working: Does qom-path has to stay identical ? Or having aliases is ok ? Any ideas or comments ? Any of these changes could be fixed by adding support in the base class (child name, sysbus device, ...) if needed. --- include/hw/riscv/riscv_hart.h | 17 ++-- hw/riscv/riscv_hart.c | 49 ++- 2 files changed, 28 insertions(+), 38 deletions(-) diff --git a/include/hw/riscv/riscv_hart.h b/include/hw/riscv/riscv_hart.h index 71747bf37c..65ac0d2bc4 100644 --- a/include/hw/riscv/riscv_hart.h +++ b/include/hw/riscv/riscv_hart.h @@ -21,7 +21,7 @@ #ifndef HW_RISCV_HART_H #define HW_RISCV_HART_H -#include "hw/sysbus.h" +#include "hw/cpu/cpus.h" #include "target/riscv/cpu.h" #include "qom/object.h" @@ -31,30 +31,29 @@ OBJECT_DECLARE_SIMPLE_TYPE(RISCVHartArrayState, RISCV_HART_ARRAY) struct RISCVHartArrayState { /*< private >*/ -SysBusDevice parent_obj; +CpusState parent_obj; /*< public >*/ -uint32_t num_harts; uint32_t hartid_base; -char *cpu_type; uint64_t resetvec; -RISCVCPU *harts; }; /** * riscv_array_get_hart: + * Helper to get an hart from the container. */ -static inline RISCVCPU *riscv_array_get_hart(RISCVHartArrayState *harts, int i) +static inline RISCVCPU *riscv_array_get_hart(RISCVHartArrayState *s, int i) { -return >harts[i]; +return RISCV_CPU(CPUS(s)->cpus[i]); } /** * riscv_array_get_num_harts: + * Helper to get the number of harts in the container. */ -static inline unsigned riscv_array_get_num_harts(RISCVHartArrayState *harts) +static inline unsigned riscv_array_get_num_harts(RISCVHartArrayState *s) { -return harts->num_harts; +return CPUS(s)->topology.cpus; } /* Temporary function until we migrated the riscv hart array to simple device */ diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c index 780fd3a59a..1b4ff7e3c6 100644 --- a/hw/riscv/riscv_hart.c +++ b/hw/riscv/riscv_hart.c @@ -22,67 +22,58 @@ #include "qapi/error.h" #include "qemu/module.h" #include "sysemu/reset.h" -#include "hw/sysbus.h" #include "target/riscv/cpu.h" #include "hw/qdev-properties.h" #include "hw/riscv/riscv_hart.h" +#include "hw/cpu/cpus.h" void riscv_hart_array_realize(RISCVHartArrayState *state, Error **errp) { -sysbus_realize(SYS_BUS_DEVICE(state), errp); +/* disable the clustering */ +cpus_disable_clustering(CPUS(state)); +qdev_realize(DEVICE(state), NULL, errp); } static Property riscv_harts_props[] = { -DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts, 1), DEFINE_PROP_UINT32("hartid-base", RISCVHartArrayState, hartid_base, 0), -DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type), DEFINE_PROP_UINT64("resetvec", RISCVHartArrayState, resetvec, DEFAULT_RSTVEC), DEFINE_PROP_END_OF_LIST(), }; -static void riscv_harts_cpu_reset(void *opaque) +static void riscv_harts_configure_cpu(CpusState *base, CPUState *cpu, + unsigned i) { -RISCVCPU *cpu = opaque; -cpu_reset(CPU(cpu)); -} +RISCVHartArrayState *s = RISCV_HART_ARRAY(base); +DeviceState *cpudev = DEVICE(cpu); +CPURISCVState *cpuenv = cpu->env_ptr; -static bool riscv_hart_realize(RISCVHartArrayState *s, int idx, - char *cpu_type, Error **errp) -{ -object_initialize_child(OBJECT(s), "harts[*]", >harts[idx], cpu_type); -qdev_prop_set_uint64(DEVICE(>harts[idx]), "resetvec", s->resetvec); -s->harts[idx].env.mhartid = s->hartid_base + idx; -qemu_register_reset(riscv_harts_cpu_reset, >harts[idx]); -return qdev_realize(DEVICE(>harts[idx]), NULL, errp); +qdev_prop_set_uint64(cpudev, "resetvec", s->resetvec); +cpuenv->mhartid = s->hartid_base + i; } -static void riscv_harts_realize(DeviceState *dev, Error **errp) +static void riscv_harts_init(Object *obj) { -RISCVHartArrayState *s = RISCV_HART_ARRAY(dev); -int n; - -s->harts = g_new0(RISCVCPU, s->num_harts); - -for (n = 0; n < s->num_harts; n++) { -if (!riscv_hart_realize(s, n, s->cpu_type, errp)) { -return; -} -} +/* add a temporary property to keep num-harts */ +ob
[RFC PATCH 11/18] hw/riscv: prepare riscv_hart transition to cpus
+ Use riscv_array_get_hart instead of accessing harts field. + Use riscv_hart_array_realize instead of sysbus_realize. spike and virt machines will be handled separately in following commits. Signed-off-by: Damien Hedde --- hw/riscv/boot.c| 2 +- hw/riscv/microchip_pfsoc.c | 4 ++-- hw/riscv/opentitan.c | 2 +- hw/riscv/shakti_c.c| 2 +- hw/riscv/sifive_e.c| 2 +- hw/riscv/sifive_u.c| 8 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index cae74fcbcd..b21c8f6488 100644 --- a/hw/riscv/boot.c +++ b/hw/riscv/boot.c @@ -36,7 +36,7 @@ bool riscv_is_32bit(RISCVHartArrayState *harts) { -return harts->harts[0].env.misa_mxl_max == MXL_RV32; +return riscv_array_get_hart(harts, 0)->env.misa_mxl_max == MXL_RV32; } /* diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c index cafd1fc9ae..82547a53e6 100644 --- a/hw/riscv/microchip_pfsoc.c +++ b/hw/riscv/microchip_pfsoc.c @@ -190,8 +190,8 @@ static void microchip_pfsoc_soc_realize(DeviceState *dev, Error **errp) NICInfo *nd; int i; -sysbus_realize(SYS_BUS_DEVICE(>e_cpus), _abort); -sysbus_realize(SYS_BUS_DEVICE(>u_cpus), _abort); +riscv_hart_array_realize(>e_cpus, _abort); +riscv_hart_array_realize(>u_cpus, _abort); /* * The cluster must be realized after the RISC-V hart array container, * as the container's CPU object is only created on realize, and the diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c index 833624d66c..2eb7454d8a 100644 --- a/hw/riscv/opentitan.c +++ b/hw/riscv/opentitan.c @@ -135,7 +135,7 @@ static void lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp) object_property_set_int(OBJECT(>cpus), "num-harts", ms->smp.cpus, _abort); object_property_set_int(OBJECT(>cpus), "resetvec", 0x8080, _abort); -sysbus_realize(SYS_BUS_DEVICE(>cpus), _abort); +riscv_hart_array_realize(>cpus, _abort); /* Boot ROM */ memory_region_init_rom(>rom, OBJECT(dev_soc), "riscv.lowrisc.ibex.rom", diff --git a/hw/riscv/shakti_c.c b/hw/riscv/shakti_c.c index 90e2cf609f..93e0c8dd68 100644 --- a/hw/riscv/shakti_c.c +++ b/hw/riscv/shakti_c.c @@ -108,7 +108,7 @@ static void shakti_c_soc_state_realize(DeviceState *dev, Error **errp) ShaktiCSoCState *sss = RISCV_SHAKTI_SOC(dev); MemoryRegion *system_memory = get_system_memory(); -sysbus_realize(SYS_BUS_DEVICE(>cpus), _abort); +riscv_hart_array_realize(>cpus, _abort); sss->plic = sifive_plic_create(shakti_c_memmap[SHAKTI_C_PLIC].base, (char *)SHAKTI_C_PLIC_HART_CONFIG, ms->smp.cpus, 0, diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c index dcb87b6cfd..25ba0a8c85 100644 --- a/hw/riscv/sifive_e.c +++ b/hw/riscv/sifive_e.c @@ -195,7 +195,7 @@ static void sifive_e_soc_realize(DeviceState *dev, Error **errp) object_property_set_str(OBJECT(>cpus), "cpu-type", ms->cpu_type, _abort); -sysbus_realize(SYS_BUS_DEVICE(>cpus), _abort); +riscv_hart_array_realize(>cpus, _abort); /* Mask ROM */ memory_region_init_rom(>mask_rom, OBJECT(dev), "riscv.sifive.e.mrom", diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index 7fbc7dea42..c99e92a7eb 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -188,9 +188,9 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap, } else { qemu_fdt_setprop_string(fdt, nodename, "mmu-type", "riscv,sv48"); } -isa = riscv_isa_string(>soc.u_cpus.harts[cpu - 1]); +isa = riscv_isa_string(riscv_array_get_hart(>soc.u_cpus, cpu - 1)); } else { -isa = riscv_isa_string(>soc.e_cpus.harts[0]); +isa = riscv_isa_string(riscv_array_get_hart(>soc.e_cpus, 0)); } qemu_fdt_setprop_string(fdt, nodename, "riscv,isa", isa); qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv"); @@ -830,8 +830,8 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp) qdev_prop_set_string(DEVICE(>u_cpus), "cpu-type", s->cpu_type); qdev_prop_set_uint64(DEVICE(>u_cpus), "resetvec", 0x1004); -sysbus_realize(SYS_BUS_DEVICE(>e_cpus), _abort); -sysbus_realize(SYS_BUS_DEVICE(>u_cpus), _abort); +riscv_hart_array_realize(>e_cpus, _abort); +riscv_hart_array_realize(>u_cpus, _abort); /* * The cluster must be realized after the RISC-V hart array container, * as the container's CPU object is only created on realize, and the -- 2.35.1
[RFC PATCH 13/18] hw/riscv/spike: prepare riscv_hart transition to cpus
+ Use riscv_array_get_num_harts instead of accessing num_harts field. + Use riscv_array_get_hart instead of accessing harts field. + Use riscv_hart_array_realize instead of sysbus_realize. Signed-off-by: Damien Hedde --- hw/riscv/spike.c | 15 --- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index d059a67f9b..b75e3656e1 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -54,6 +54,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, uint64_t addr, size; unsigned long clint_addr; int cpu, socket; +unsigned num_harts; MachineState *mc = MACHINE(s); uint32_t *clint_cells; uint32_t cpu_phandle, intc_phandle, phandle = 1; @@ -97,10 +98,10 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, for (socket = (riscv_socket_count(mc) - 1); socket >= 0; socket--) { clust_name = g_strdup_printf("/cpus/cpu-map/cluster%d", socket); qemu_fdt_add_subnode(fdt, clust_name); +num_harts = riscv_array_get_num_harts(>soc[socket]); +clint_cells = g_new0(uint32_t, num_harts * 4); -clint_cells = g_new0(uint32_t, s->soc[socket].num_harts * 4); - -for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) { +for (cpu = num_harts - 1; cpu >= 0; cpu--) { cpu_phandle = phandle++; cpu_name = g_strdup_printf("/cpus/cpu@%d", @@ -111,7 +112,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, } else { qemu_fdt_setprop_string(fdt, cpu_name, "mmu-type", "riscv,sv48"); } -name = riscv_isa_string(>soc[socket].harts[cpu]); +name = riscv_isa_string(riscv_array_get_hart(>soc[socket], cpu)); qemu_fdt_setprop_string(fdt, cpu_name, "riscv,isa", name); g_free(name); qemu_fdt_setprop_string(fdt, cpu_name, "compatible", "riscv"); @@ -164,7 +165,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, qemu_fdt_setprop_cells(fdt, clint_name, "reg", 0x0, clint_addr, 0x0, memmap[SPIKE_CLINT].size); qemu_fdt_setprop(fdt, clint_name, "interrupts-extended", -clint_cells, s->soc[socket].num_harts * sizeof(uint32_t) * 4); +clint_cells, num_harts * sizeof(uint32_t) * 4); riscv_socket_fdt_write_id(mc, fdt, clint_name, socket); g_free(clint_name); @@ -229,7 +230,7 @@ static void spike_board_init(MachineState *machine) base_hartid, _abort); object_property_set_int(OBJECT(>soc[i]), "num-harts", hart_count, _abort); -sysbus_realize(SYS_BUS_DEVICE(>soc[i]), _abort); +riscv_hart_array_realize(>soc[i], _abort); /* Core Local Interruptor (timer and IPI) for each socket */ riscv_aclint_swi_create( @@ -311,7 +312,7 @@ static void spike_board_init(MachineState *machine) /* initialize HTIF using symbols found in load_kernel */ htif_mm_init(system_memory, mask_rom, - >soc[0].harts[0].env, serial_hd(0), + _array_get_hart(>soc[0], 0)->env, serial_hd(0), memmap[SPIKE_HTIF].base); } -- 2.35.1
[RFC PATCH 08/18] hw/arm/arm_cpus: add arm_cpus device
This object can be used to create a group of homogeneous arm cpus. Signed-off-by: Damien Hedde --- include/hw/arm/arm_cpus.h | 45 hw/arm/arm_cpus.c | 63 +++ hw/arm/meson.build| 1 + 3 files changed, 109 insertions(+) create mode 100644 include/hw/arm/arm_cpus.h create mode 100644 hw/arm/arm_cpus.c diff --git a/include/hw/arm/arm_cpus.h b/include/hw/arm/arm_cpus.h new file mode 100644 index 00..8c540d9853 --- /dev/null +++ b/include/hw/arm/arm_cpus.h @@ -0,0 +1,45 @@ +/* + * ARM CPUs + * + * Copyright (c) 2022 Greensocs + * + * This work is licensed under the terms of the GNU GPLv2 or later. + * See the COPYING file in the top-level directory. + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef HW_ARM_CPUS_H +#define HW_ARM_CPUS_H + +#include "hw/cpu/cpus.h" +#include "target/arm/cpu.h" + +#define TYPE_ARM_CPUS "arm-cpus" +OBJECT_DECLARE_SIMPLE_TYPE(ArmCpusState, ARM_CPUS) + +/** + * ArmCpusState: + * @reset_hivecs: use to initialize cpu's reset-hivecs + * @has_el3: use to initialize cpu's has_el3 + * @has_el2: use to initialize cpu's has_el2 + * @reset_cbar: use to initialize cpu's reset-cbar + */ +struct ArmCpusState { +CpusState parent_obj; + +bool reset_hivecs; +bool has_el3; +bool has_el2; +uint64_t reset_cbar; +}; + +/* + * arm_cpus_get_cpu: + * Helper to get an ArmCpu from the container. + */ +static inline ARMCPU *arm_cpus_get_cpu(ArmCpusState *s, unsigned i) +{ +return ARM_CPU(CPUS(s)->cpus[i]); +} + +#endif /* HW_ARM_CPUS_H */ diff --git a/hw/arm/arm_cpus.c b/hw/arm/arm_cpus.c new file mode 100644 index 00..ed6483f45b --- /dev/null +++ b/hw/arm/arm_cpus.c @@ -0,0 +1,63 @@ +/* + * ARM CPUs + * + * Copyright (c) 2022 Greensocs + * + * This work is licensed under the terms of the GNU GPLv2 or later. + * See the COPYING file in the top-level directory. + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "hw/arm/arm_cpus.h" +#include "hw/cpu/cpus.h" +#include "hw/qdev-properties.h" +#include "cpu.h" + +static Property arm_cpus_props[] = { +/* FIXME: get the default values from the arm cpu object */ +DEFINE_PROP_BOOL("reset-hivecs", ArmCpusState, reset_hivecs, false), +DEFINE_PROP_BOOL("has_el3", ArmCpusState, has_el3, false), +DEFINE_PROP_BOOL("has_el2", ArmCpusState, has_el2, false), +DEFINE_PROP_UINT64("reset-cbar", ArmCpusState, reset_cbar, 0), +DEFINE_PROP_END_OF_LIST() +}; + +static void arm_cpus_configure_cpu(CpusState *base, CPUState *cpu, + unsigned i) +{ +ArmCpusState *s = ARM_CPUS(base); +DeviceState *cpudev = DEVICE(cpu); + +qdev_prop_set_uint32(cpudev, "core-count", base->topology.cpus); +qdev_prop_set_bit(cpudev, "reset-hivecs", s->reset_hivecs); +qdev_prop_set_bit(cpudev, "has_el3", s->has_el3); +qdev_prop_set_bit(cpudev, "has_el2", s->has_el2); +qdev_prop_set_uint64(cpudev, "reset-cbar", s->reset_cbar); +} + +static void arm_cpus_class_init(ObjectClass *klass, void *data) +{ +DeviceClass *dc = DEVICE_CLASS(klass); +CpusClass *cc = CPUS_CLASS(klass); + +device_class_set_props(dc, arm_cpus_props); + +cc->configure_cpu = arm_cpus_configure_cpu; +cc->base_cpu_type = TYPE_ARM_CPU; +} + +static const TypeInfo arm_cpus_info = { +.name = TYPE_ARM_CPUS, +.parent= TYPE_CPUS, +.instance_size = sizeof(ArmCpusState), +.class_init= arm_cpus_class_init, +}; + +static void arm_cpus_register_types(void) +{ +type_register_static(_cpus_info); +} + +type_init(arm_cpus_register_types) diff --git a/hw/arm/meson.build b/hw/arm/meson.build index 721a8eb8be..feeb301c09 100644 --- a/hw/arm/meson.build +++ b/hw/arm/meson.build @@ -58,5 +58,6 @@ arm_ss.add(when: 'CONFIG_FSL_IMX7', if_true: files('fsl-imx7.c', 'mcimx7d-sabre. arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmu-common.c', 'smmuv3.c')) arm_ss.add(when: 'CONFIG_FSL_IMX6UL', if_true: files('fsl-imx6ul.c', 'mcimx6ul-evk.c')) arm_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_soc.c')) +arm_ss.add(files('arm_cpus.c')) hw_arch += {'arm': arm_ss} -- 2.35.1
[RFC PATCH 10/18] hw/riscv/riscv_hart: prepare transition to cpus
riscv_hart_array does not need to be a sysbus device: it does not have any mmio or sysbus irq. We want to make it inherit the new cpus class so we need a few tweaks: + a temporary helper realize so we can switch from sysbus_realize to qdev_realize (will be removed afer the transition is done). + a helper function to get an hart from the array (the current storage array field will be removed). + a helper function to get the number of harts in an array (the current field will be removed). Signed-off-by: Damien Hedde --- include/hw/riscv/riscv_hart.h | 19 +++ hw/riscv/riscv_hart.c | 5 + 2 files changed, 24 insertions(+) diff --git a/include/hw/riscv/riscv_hart.h b/include/hw/riscv/riscv_hart.h index bbc21cdc9a..71747bf37c 100644 --- a/include/hw/riscv/riscv_hart.h +++ b/include/hw/riscv/riscv_hart.h @@ -41,4 +41,23 @@ struct RISCVHartArrayState { RISCVCPU *harts; }; +/** + * riscv_array_get_hart: + */ +static inline RISCVCPU *riscv_array_get_hart(RISCVHartArrayState *harts, int i) +{ +return >harts[i]; +} + +/** + * riscv_array_get_num_harts: + */ +static inline unsigned riscv_array_get_num_harts(RISCVHartArrayState *harts) +{ +return harts->num_harts; +} + +/* Temporary function until we migrated the riscv hart array to simple device */ +void riscv_hart_array_realize(RISCVHartArrayState *state, Error **errp); + #endif diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c index 613ea2aaa0..780fd3a59a 100644 --- a/hw/riscv/riscv_hart.c +++ b/hw/riscv/riscv_hart.c @@ -27,6 +27,11 @@ #include "hw/qdev-properties.h" #include "hw/riscv/riscv_hart.h" +void riscv_hart_array_realize(RISCVHartArrayState *state, Error **errp) +{ +sysbus_realize(SYS_BUS_DEVICE(state), errp); +} + static Property riscv_harts_props[] = { DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts, 1), DEFINE_PROP_UINT32("hartid-base", RISCVHartArrayState, hartid_base, 0), -- 2.35.1
[RFC PATCH 05/18] gdbstub: deal with _cpus_ object instead of _cpu-cluster_
The gdbstub will only handle _cpus_ object having set the _is_cluster_ flag. Signed-off-by: Damien Hedde --- gdbstub.c | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/gdbstub.c b/gdbstub.c index c8375e3c3f..0b3e943f95 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -38,7 +38,7 @@ #include "monitor/monitor.h" #include "chardev/char.h" #include "chardev/char-fe.h" -#include "hw/cpu/cluster.h" +#include "hw/cpu/cpus.h" #include "hw/boards.h" #endif @@ -3458,9 +3458,10 @@ static const TypeInfo char_gdb_type_info = { static int find_cpu_clusters(Object *child, void *opaque) { -if (object_dynamic_cast(child, TYPE_CPU_CLUSTER)) { +if (object_dynamic_cast(child, TYPE_CPUS) && +CPUS(child)->is_cluster) { GDBState *s = (GDBState *) opaque; -CPUClusterState *cluster = CPU_CLUSTER(child); +CpusState *cluster = CPUS(child); GDBProcess *process; s->processes = g_renew(GDBProcess, s->processes, ++s->process_num); @@ -3472,8 +3473,9 @@ static int find_cpu_clusters(Object *child, void *opaque) * runtime, we enforce here that the machine does not use a cluster ID * that would lead to PID 0. */ -assert(cluster->cluster_id != UINT32_MAX); -process->pid = cluster->cluster_id + 1; +assert(cluster->cluster_index >= 0 && + cluster->cluster_index < UINT32_MAX); +process->pid = cluster->cluster_index + 1; process->attached = false; process->target_xml[0] = '\0'; -- 2.35.1
[RFC PATCH 06/18] hw/cpu/cluster: remove cluster_id now that gdbstub is updated
Signed-off-by: Damien Hedde --- include/hw/cpu/cluster.h | 7 --- hw/cpu/cluster.c | 7 --- 2 files changed, 14 deletions(-) diff --git a/include/hw/cpu/cluster.h b/include/hw/cpu/cluster.h index 2125765f21..6704434cc0 100644 --- a/include/hw/cpu/cluster.h +++ b/include/hw/cpu/cluster.h @@ -60,17 +60,10 @@ OBJECT_DECLARE_TYPE(CPUClusterState, CPUClusterClass, CPU_CLUSTER) /** * CPUClusterState: - * @cluster_id: The cluster ID. This value is for internal use only and should - * not be exposed directly to the user or to the guest. - * - * State of a CPU cluster. */ struct CPUClusterState { /*< private >*/ CpusState parent_obj; - -/*< public >*/ -uint32_t cluster_id; }; /** diff --git a/hw/cpu/cluster.c b/hw/cpu/cluster.c index 3daf897bd9..51da6ce3a9 100644 --- a/hw/cpu/cluster.c +++ b/hw/cpu/cluster.c @@ -42,7 +42,6 @@ static int add_cpu_to_cluster(Object *obj, void *opaque) static void cpu_cluster_realize(DeviceState *dev, Error **errp) { CPUClusterClass *ccc = CPU_CLUSTER_GET_CLASS(dev); -CPUClusterState *cluster = CPU_CLUSTER(dev); CpusState *base = CPUS(dev); Object *cluster_obj = OBJECT(dev); @@ -64,12 +63,6 @@ static void cpu_cluster_realize(DeviceState *dev, Error **errp) /* realize base class (will set cluster field to true) */ ccc->parent_realize(dev, errp); - -/* - * Temporarily copy the cluster id from the base class as - * gdbstub still uses our field. - */ -cluster->cluster_id = base->cluster_index; } static void cpu_cluster_class_init(ObjectClass *klass, void *data) -- 2.35.1
[RFC PATCH 07/18] hw/cpu/cpus: add a common start-powered-off property
Can be used to initialize the same property on all cpus. Signed-off-by: Damien Hedde --- include/hw/cpu/cpus.h | 3 +++ hw/cpu/cpus.c | 5 + 2 files changed, 8 insertions(+) diff --git a/include/hw/cpu/cpus.h b/include/hw/cpu/cpus.h index 295d7537e2..7e89a0d018 100644 --- a/include/hw/cpu/cpus.h +++ b/include/hw/cpu/cpus.h @@ -46,6 +46,8 @@ OBJECT_DECLARE_TYPE(CpusState, CpusClass, CPUS) * written before realize in order to enable/disable clustering. * @cluster_index: The cluster ID. This value is for internal use only and * should not be exposed directly to the user or to the guest. + * @start_powered_off: Default start power state of all cpus + * (can be modified on a per-cpu basis after realize). */ struct CpusState { /*< private >*/ @@ -59,6 +61,7 @@ struct CpusState { struct { uint16_t cpus; } topology; +bool start_powered_off; CPUState **cpus; }; diff --git a/hw/cpu/cpus.c b/hw/cpu/cpus.c index ed9402c100..d1fe80f0ab 100644 --- a/hw/cpu/cpus.c +++ b/hw/cpu/cpus.c @@ -27,6 +27,7 @@ static Property cpus_properties[] = { * FIXME: remove this property to keep it internal ? */ DEFINE_PROP_INT32("cluster-id", CpusState, cluster_index, -1), +DEFINE_PROP_BOOL("start-powered-off", CpusState, start_powered_off, false), DEFINE_PROP_END_OF_LIST() }; @@ -71,6 +72,10 @@ static void cpus_create_cpus(CpusState *s, Error **errp) cpu->cluster_index = s->cluster_index; } +/* set power start state */ +qdev_prop_set_bit(DEVICE(cpu), "start-powered-off", + s->start_powered_off); + /* let subclass configure the cpu */ if (cgc->configure_cpu) { cgc->configure_cpu(s, cpu, i); -- 2.35.1
[RFC PATCH 02/18] hw/cpu/cpus: introduce _cpus_ device
This object will be a _cpu-cluster_ generalization and is meant to allow create cpus of the same type. The main goal is that this object, on contrary to _cpu-cluster-_, can be used to dynamically create cpus: it does not rely on external code to populate the object with cpus. Allowing the user to create a cpu cluster and each _cpu_ separately would be hard because of the following reasons: + cpu reset need to be handled + instantiation and realize of cpu-cluster and the cpus are interleaved + cpu cluster must contains only identical cpus and it seems difficult to check that at runtime. Therefore we add a new type solving all this constraints. _cpu-cluster_ will be updated to inherit from this class in following commits. Signed-off-by: Damien Hedde --- include/hw/cpu/cpus.h | 71 +++ hw/cpu/cpus.c | 127 ++ hw/cpu/meson.build| 2 +- 3 files changed, 199 insertions(+), 1 deletion(-) create mode 100644 include/hw/cpu/cpus.h create mode 100644 hw/cpu/cpus.c diff --git a/include/hw/cpu/cpus.h b/include/hw/cpu/cpus.h new file mode 100644 index 00..c65f568ef8 --- /dev/null +++ b/include/hw/cpu/cpus.h @@ -0,0 +1,71 @@ +/* + * QEMU CPUs type + * + * Copyright (c) 2022 GreenSocs + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef HW_CPU_CPUS_H +#define HW_CPU_CPUS_H + +#include "qemu/typedefs.h" +#include "hw/qdev-core.h" +#include "qom/object.h" + +/* + * This object represent several CPUs which are all identical. + * + * If CPUs are not identical (for example, Cortex-A53 and Cortex-A57 CPUs in an + * Arm big.LITTLE system) they should be in different groups. If the CPUs do + * not have the same view of memory (for example the main CPU and a management + * controller processor) they should be in different groups. + * + * This is an abstract class, subclasses are supposed to be created on + * per-architecture basis to handle the specifics of the cpu architecture. + * Subclasses are meant to be user-creatable (for cold-plug). + */ + +#define TYPE_CPUS "cpus" +OBJECT_DECLARE_TYPE(CpusState, CpusClass, CPUS) + +/** + * CpusState: + * @cpu_type: The type of cpu. + * @topology.cpus: The number of cpus in this group. + * Explicity put this field into a topology structure in + * order to eventually update this smoothly with a full + * CpuTopology structure in the future. + * @cpus: Array of pointer to cpu objects. + */ +struct CpusState { +/*< private >*/ +DeviceState parent_obj; + +/*< public >*/ +char *cpu_type; +struct { +uint16_t cpus; +} topology; +CPUState **cpus; +}; + +typedef void (*CpusConfigureCpu)(CpusState *s, CPUState *cpu, unsigned idx); + +/** + * CpusClass: + * @base_cpu_type: base cpu type accepted by this cpu group + * (the state cpu_type will be tested against it). + * @configure_cpu: method to configure a cpu (called between + * cpu init and realize) + * @skip_cpus_creation: CPUCLuster do not rely on creating + * cpus internally. This flag disables this feature. + */ +struct CpusClass { +DeviceClass parent_class; +const char *base_cpu_type; +CpusConfigureCpu configure_cpu; +bool skip_cpus_creation; +}; + +#endif /* HW_CPU_CPUS_H */ diff --git a/hw/cpu/cpus.c b/hw/cpu/cpus.c new file mode 100644 index 00..5fad1de2c7 --- /dev/null +++ b/hw/cpu/cpus.c @@ -0,0 +1,127 @@ +/* + * QEMU CPUs type + * + * Copyright (c) 2022 GreenSocs + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "hw/qdev-properties.h" +#include "qapi/error.h" +#include "qemu/module.h" +#include "qemu/cutils.h" +#include "hw/cpu/cpus.h" +#include "hw/core/cpu.h" +#include "hw/resettable.h" +#include "sysemu/reset.h" + +static Property cpus_properties[] = { +DEFINE_PROP_STRING("cpu-type", CpusState, cpu_type), +DEFINE_PROP_UINT16("num-cpus", CpusState, topology.cpus, 0), +DEFINE_PROP_END_OF_LIST() +}; + +static void cpus_reset(Object *obj) +{ +CpusState *s = CPUS(obj); +for (unsigned i = 0; i < s->topology.cpus; i++) { +cpu_reset(s->cpus[i]); +} +} + +static void cpus_create_cpus(CpusState *s, Error **errp) +{ +Error *err = NULL; +CpusClass *cgc = CPUS_GET_CLASS(s); +s->cpus = g_new0(CPUState *, s->topology.cpus); + +for (unsigned i = 0; i < s->topology.cpus; i++) { +CPUState *cpu = CPU(object_new(s->cpu_type)); +s->cpus[i] = cpu; + +/* set child property and release the initial ref */ +object_property_add_child(OBJECT(s), "cpu[*]", OBJECT(cpu)); +object_unref(OBJECT(cpu)); + +/* let subclass configure the cpu */ +if (cgc->configure_cpu) { +cgc->configure_cpu(s, cpu, i); +} + +
[RFC PATCH 03/18] hw/cpu/cpus: prepare to handle cpu clusters
Some group of cpus need to form a cpu cluster to be exposed in gdb as inferiors. Note: 'is_cluster' field is required at least to transition the riscv_hart_array (see following commits about that) Signed-off-by: Damien Hedde --- include/hw/cpu/cpus.h | 19 + hw/cpu/cpus.c | 65 ++- 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/include/hw/cpu/cpus.h b/include/hw/cpu/cpus.h index c65f568ef8..295d7537e2 100644 --- a/include/hw/cpu/cpus.h +++ b/include/hw/cpu/cpus.h @@ -24,6 +24,10 @@ * This is an abstract class, subclasses are supposed to be created on * per-architecture basis to handle the specifics of the cpu architecture. * Subclasses are meant to be user-creatable (for cold-plug). + * + * Optionnaly a group of cpus may correspond to a cpu cluster and be + * exposed as a gdbstub's inferior. In that case cpus must have the + * same memory view. */ #define TYPE_CPUS "cpus" @@ -37,10 +41,18 @@ OBJECT_DECLARE_TYPE(CpusState, CpusClass, CPUS) * order to eventually update this smoothly with a full * CpuTopology structure in the future. * @cpus: Array of pointer to cpu objects. + * @cluster_node: node in the global cluster list. + * @is_cluster: true if the object corresponds to a cpu cluster. It can be + * written before realize in order to enable/disable clustering. + * @cluster_index: The cluster ID. This value is for internal use only and + * should not be exposed directly to the user or to the guest. */ struct CpusState { /*< private >*/ DeviceState parent_obj; +bool is_cluster; +int32_t cluster_index; +QLIST_ENTRY(CpusState) cluster_node; /*< public >*/ char *cpu_type; @@ -68,4 +80,11 @@ struct CpusClass { bool skip_cpus_creation; }; +/** + * cpus_disable_clustering: + * Disable clustering for this object. + * Has to be called before realize step. + */ +void cpus_disable_clustering(CpusState *s); + #endif /* HW_CPU_CPUS_H */ diff --git a/hw/cpu/cpus.c b/hw/cpu/cpus.c index 5fad1de2c7..ed9402c100 100644 --- a/hw/cpu/cpus.c +++ b/hw/cpu/cpus.c @@ -16,9 +16,17 @@ #include "hw/resettable.h" #include "sysemu/reset.h" +static QLIST_HEAD(, CpusState) clusters = +QLIST_HEAD_INITIALIZER(clusters); + static Property cpus_properties[] = { DEFINE_PROP_STRING("cpu-type", CpusState, cpu_type), DEFINE_PROP_UINT16("num-cpus", CpusState, topology.cpus, 0), +/* + * Default behavior is to automatically compute a valid index. + * FIXME: remove this property to keep it internal ? + */ +DEFINE_PROP_INT32("cluster-id", CpusState, cluster_index, -1), DEFINE_PROP_END_OF_LIST() }; @@ -30,6 +38,20 @@ static void cpus_reset(Object *obj) } } +static void cpus_init(Object *obj) +{ +CpusState *s = CPUS(obj); + +/* may be overriden by subclasses or code to disable clustering */ +s->is_cluster = true; +} + +void cpus_disable_clustering(CpusState *s) +{ +assert(!DEVICE(s)->realized); +s->is_cluster = false; +} + static void cpus_create_cpus(CpusState *s, Error **errp) { Error *err = NULL; @@ -44,6 +66,11 @@ static void cpus_create_cpus(CpusState *s, Error **errp) object_property_add_child(OBJECT(s), "cpu[*]", OBJECT(cpu)); object_unref(OBJECT(cpu)); +/* set index in case of cluster */ +if (s->is_cluster) { +cpu->cluster_index = s->cluster_index; +} + /* let subclass configure the cpu */ if (cgc->configure_cpu) { cgc->configure_cpu(s, cpu, i); @@ -60,7 +87,7 @@ static void cpus_create_cpus(CpusState *s, Error **errp) static void cpus_realize(DeviceState *dev, Error **errp) { -CpusState *s = CPUS(dev); +CpusState *item, *s = CPUS(dev); CpusClass *cgc = CPUS_GET_CLASS(s); /* if subclass defined a base type, let's check it */ @@ -77,6 +104,38 @@ static void cpus_realize(DeviceState *dev, Error **errp) return; } +if (s->is_cluster) { +if (s->cluster_index < 0) { +int32_t max = -1; +QLIST_FOREACH(item, , cluster_node) { +if (max < item->cluster_index) { +max = item->cluster_index; +} +} +s->cluster_index = max + 1; +} else { +/* + * Check the index is not already taken. + */ +QLIST_FOREACH(item, , cluster_node) { +if (s->cluster_index == item->cluster_index) { +error_setg(errp, "cluster index %d already exists", + s->cluster_index); +return; +} +} +} + +if (s->cluster_index >= MAX_CLUSTERS) { +error_setg(errp, &quo
[RFC PATCH 04/18] hw/cpu/cluster: make _cpu-cluster_ a subclass of _cpus_
Drop the cluster-id property as base class defines one. cluster_id field is temporarily kept until gdbstub is updated. Signed-off-by: Damien Hedde --- include/hw/cpu/cluster.h | 17 ++-- hw/cpu/cluster.c | 58 +--- 2 files changed, 46 insertions(+), 29 deletions(-) diff --git a/include/hw/cpu/cluster.h b/include/hw/cpu/cluster.h index 09a2b86832..2125765f21 100644 --- a/include/hw/cpu/cluster.h +++ b/include/hw/cpu/cluster.h @@ -22,6 +22,7 @@ #include "hw/qdev-core.h" #include "qom/object.h" +#include "hw/cpu/cpus.h" /* * CPU Cluster type @@ -55,7 +56,7 @@ */ #define TYPE_CPU_CLUSTER "cpu-cluster" -OBJECT_DECLARE_SIMPLE_TYPE(CPUClusterState, CPU_CLUSTER) +OBJECT_DECLARE_TYPE(CPUClusterState, CPUClusterClass, CPU_CLUSTER) /** * CPUClusterState: @@ -66,10 +67,22 @@ OBJECT_DECLARE_SIMPLE_TYPE(CPUClusterState, CPU_CLUSTER) */ struct CPUClusterState { /*< private >*/ -DeviceState parent_obj; +CpusState parent_obj; /*< public >*/ uint32_t cluster_id; }; +/** + * CPUClusterClass: + * @parent_realize: to store base class realize method + */ +struct CPUClusterClass { +/*< private >*/ +CpusClass parent_class; + +/*< public >*/ +DeviceRealize parent_realize; +}; + #endif diff --git a/hw/cpu/cluster.c b/hw/cpu/cluster.c index e444b7c29d..3daf897bd9 100644 --- a/hw/cpu/cluster.c +++ b/hw/cpu/cluster.c @@ -20,50 +20,39 @@ #include "qemu/osdep.h" #include "hw/cpu/cluster.h" +#include "hw/cpu/cpus.h" #include "hw/qdev-properties.h" #include "hw/core/cpu.h" #include "qapi/error.h" #include "qemu/module.h" #include "qemu/cutils.h" -static Property cpu_cluster_properties[] = { -DEFINE_PROP_UINT32("cluster-id", CPUClusterState, cluster_id, 0), -DEFINE_PROP_END_OF_LIST() -}; - -typedef struct CallbackData { -CPUClusterState *cluster; -int cpu_count; -} CallbackData; - static int add_cpu_to_cluster(Object *obj, void *opaque) { -CallbackData *cbdata = opaque; +CpusState *base = CPUS(opaque); CPUState *cpu = (CPUState *)object_dynamic_cast(obj, TYPE_CPU); if (cpu) { -cpu->cluster_index = cbdata->cluster->cluster_id; -cbdata->cpu_count++; +cpu->cluster_index = base->cluster_index; +base->topology.cpus++; } return 0; } static void cpu_cluster_realize(DeviceState *dev, Error **errp) { -/* Iterate through all our CPU children and set their cluster_index */ +CPUClusterClass *ccc = CPU_CLUSTER_GET_CLASS(dev); CPUClusterState *cluster = CPU_CLUSTER(dev); +CpusState *base = CPUS(dev); Object *cluster_obj = OBJECT(dev); -CallbackData cbdata = { -.cluster = cluster, -.cpu_count = 0, -}; -if (cluster->cluster_id >= MAX_CLUSTERS) { -error_setg(errp, "cluster-id must be less than %d", MAX_CLUSTERS); -return; -} +/* This is a special legacy case */ +assert(base->topology.cpus == 0); +assert(base->cpu_type == NULL); +assert(base->is_cluster); -object_child_foreach_recursive(cluster_obj, add_cpu_to_cluster, ); +/* Iterate through all our CPU children and set their cluster_index */ +object_child_foreach_recursive(cluster_obj, add_cpu_to_cluster, base); /* * A cluster with no CPUs is a bug in the board/SoC code that created it; @@ -71,24 +60,39 @@ static void cpu_cluster_realize(DeviceState *dev, Error **errp) * created the CPUs and parented them into the cluster object before * realizing the cluster object. */ -assert(cbdata.cpu_count > 0); +assert(base->topology.cpus > 0); + +/* realize base class (will set cluster field to true) */ +ccc->parent_realize(dev, errp); + +/* + * Temporarily copy the cluster id from the base class as + * gdbstub still uses our field. + */ +cluster->cluster_id = base->cluster_index; } static void cpu_cluster_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); +CPUClusterClass *ccc = CPU_CLUSTER_CLASS(klass); +CpusClass *cc = CPUS_CLASS(klass); -device_class_set_props(dc, cpu_cluster_properties); -dc->realize = cpu_cluster_realize; +device_class_set_parent_realize(dc, cpu_cluster_realize, +>parent_realize); /* This is not directly for users, CPU children must be attached by code */ dc->user_creatable = false; + +/* Cpus are created by external code */ +cc->skip_cpus_creation = true; } static const TypeInfo cpu_cluster_type_info = { .name = TYPE_CPU_CLUSTER, -.parent = TYPE_DEVICE, +.parent = TYPE_CPUS, .instance_size = sizeof(CPUClusterState), +.class_size = sizeof(CPUClusterClass), .class_init = cpu_cluster_class_init, }; -- 2.35.1
[RFC PATCH 01/18] define MAX_CLUSTERS in cpu.h instead of cluster.h
Signed-off-by: Damien Hedde --- include/hw/core/cpu.h| 6 ++ include/hw/cpu/cluster.h | 6 -- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 136973655c..38c72cbda1 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -1054,5 +1054,11 @@ extern const VMStateDescription vmstate_cpu_common; #define UNASSIGNED_CPU_INDEX -1 #define UNASSIGNED_CLUSTER_INDEX -1 +/* + * This limit is imposed by TCG, which puts the cluster ID into an + * 8 bit field. Default being all-1s, custom index cannot be set to + * more than 254. + */ +#define MAX_CLUSTERS 255 #endif diff --git a/include/hw/cpu/cluster.h b/include/hw/cpu/cluster.h index 53fbf36af5..09a2b86832 100644 --- a/include/hw/cpu/cluster.h +++ b/include/hw/cpu/cluster.h @@ -57,12 +57,6 @@ #define TYPE_CPU_CLUSTER "cpu-cluster" OBJECT_DECLARE_SIMPLE_TYPE(CPUClusterState, CPU_CLUSTER) -/* - * This limit is imposed by TCG, which puts the cluster ID into an - * 8 bit field (and uses all-1s for the default "not in any cluster"). - */ -#define MAX_CLUSTERS 255 - /** * CPUClusterState: * @cluster_id: The cluster ID. This value is for internal use only and should -- 2.35.1
[RFC PATCH 00/18] user-creatable cpu clusters
Hi, This series add devices to be able to user-create (coldplug) cpu clusters. The existing cpu cluster dictates how cpus are exposed in gdb, but it does not handle the cpu objects creation. This series adds a new device to handle both issues and adds support for two architectures: arm and riscv. Please look at patches 2 and 3 for more details about the new device. Last part concerning the riscv is rfc as I do non-backward compatible updates. I'm not sure what migration (or other) constraints we have on these machines and I probably need to make some changes to cope with them. This series almost deprecates the cpu-cluster type as all uses but one are replaced. It is organized as follows: + Patches 1 to 7 adds a new base device to replace cpu-cluster + Patches 8 and 9 adds an arm specific version and replace existing clusters in the xlnx-zynqmp machine. + patches 10 to 17 updates the riscv_array. It was already used to create cpus but was not a cpu cluster. Thanks for any comments, -- Damien Damien Hedde (18): define MAX_CLUSTERS in cpu.h instead of cluster.h hw/cpu/cpus: introduce _cpus_ device hw/cpu/cpus: prepare to handle cpu clusters hw/cpu/cluster: make _cpu-cluster_ a subclass of _cpus_ gdbstub: deal with _cpus_ object instead of _cpu-cluster_ hw/cpu/cluster: remove cluster_id now that gdbstub is updated hw/cpu/cpus: add a common start-powered-off property hw/arm/arm_cpus: add arm_cpus device hw/arm/xlnx-zynqmp: convert cpu clusters to arm_cpus hw/riscv/riscv_hart: prepare transition to cpus hw/riscv: prepare riscv_hart transition to cpus hw/riscv/virt: prepare riscv_hart transition to cpus hw/riscv/spike: prepare riscv_hart transition to cpus hw/riscv/riscv_hart: use cpus as base class hw/riscv/sifive_u_pfsoc: apply riscv_hart_array update hw/riscv: update remaining machines due to riscv_hart_array update hw/riscv/riscv_hart: remove temporary features add myself as reviewer of the newly added _cpus_ include/hw/arm/arm_cpus.h | 45 +++ include/hw/arm/xlnx-zynqmp.h | 8 +- include/hw/core/cpu.h | 6 + include/hw/cpu/cluster.h | 26 ++-- include/hw/cpu/cpus.h | 93 ++ include/hw/riscv/microchip_pfsoc.h | 2 - include/hw/riscv/riscv_hart.h | 25 +++- include/hw/riscv/sifive_u.h| 2 - gdbstub.c | 12 +- hw/arm/arm_cpus.c | 63 ++ hw/arm/xlnx-zynqmp.c | 121 +++--- hw/cpu/cluster.c | 53 hw/cpu/cpus.c | 195 + hw/riscv/boot.c| 2 +- hw/riscv/microchip_pfsoc.c | 28 + hw/riscv/opentitan.c | 4 +- hw/riscv/riscv_hart.c | 44 ++- hw/riscv/shakti_c.c| 4 +- hw/riscv/sifive_e.c| 4 +- hw/riscv/sifive_u.c| 31 ++--- hw/riscv/spike.c | 18 +-- hw/riscv/virt.c| 79 +++- MAINTAINERS| 3 + hw/arm/meson.build | 1 + hw/cpu/meson.build | 2 +- 25 files changed, 612 insertions(+), 259 deletions(-) create mode 100644 include/hw/arm/arm_cpus.h create mode 100644 include/hw/cpu/cpus.h create mode 100644 hw/arm/arm_cpus.c create mode 100644 hw/cpu/cpus.c -- 2.35.1
Re: [PATCH v4 02/14] machine: introduce phase_until() to handle phase transitions
Hi ! It would nice to have some feedback about this solution. Basically it just introduces a new function 'qemu_until' which implement the startup fsm. It's bit like what Markus proposed in december (hence the name), only it is is only internal so we can change it if we find out it should be done otherwise regarding startup. In practice it is just some code move from qmp_exit_preconfig() to phase_until() with more indentation (not sure if there is a way to make that easier to read). In the following patches I use it in device_add() to ensure the startup is advanced. Thanks, -- Damien On 2/23/22 10:06, Damien Hedde wrote: phase_until() is implemented in vl.c and is meant to be used to make startup progress up to a specified phase being reached(). At this point, no behavior change is introduced: phase_until() only supports a single double transition corresponding to the functionality of qmp_exit_preconfig(): + accel-created -> machine-initialized -> machine-ready As a result qmp_exit_preconfig() now uses phase_until(). This commit is a preparation to support cold plugging a device using qapi command (which will be introduced in a following commit). For this we need fine grain control of the phase. Signed-off-by: Damien Hedde --- include/hw/qdev-core.h | 14 softmmu/vl.c | 78 -- 2 files changed, 74 insertions(+), 18 deletions(-) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index e29c705b74..5f73d06408 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -909,4 +909,18 @@ extern bool phase_check(MachineInitPhase phase); */ extern void phase_advance(MachineInitPhase phase); +/** + * @phase_until: + * @phase: the target phase + * @errp: error report + * + * Make the machine init progress until the target phase is reached. + * + * Its is a no-op is the target phase is the current or an earlier + * phase. + * + * Returns true in case of success. + */ +extern bool phase_until(MachineInitPhase phase, Error **errp); + #endif diff --git a/softmmu/vl.c b/softmmu/vl.c index 5e1b35ba48..5689d0be88 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2741,30 +2741,72 @@ void qmp_x_exit_preconfig(Error **errp) error_setg(errp, "The command is permitted only before machine initialization"); return; } +phase_until(PHASE_MACHINE_READY, errp); +} -qemu_init_board(); -qemu_create_cli_devices(); -qemu_machine_creation_done(); - -if (loadvm) { -load_snapshot(loadvm, NULL, false, NULL, _fatal); -} -if (replay_mode != REPLAY_MODE_NONE) { -replay_vmstate_init(); +bool phase_until(MachineInitPhase phase, Error **errp) +{ +if (!phase_check(PHASE_ACCEL_CREATED)) { +error_setg(errp, "Phase transition is not supported until accelerator" + " is created"); +return false; } -if (incoming) { -Error *local_err = NULL; -if (strcmp(incoming, "defer") != 0) { -qmp_migrate_incoming(incoming, _err); -if (local_err) { -error_reportf_err(local_err, "-incoming %s: ", incoming); -exit(1); +while (!phase_check(phase)) { +MachineInitPhase cur_phase = phase_get(); + +switch (cur_phase) { +case PHASE_ACCEL_CREATED: +qemu_init_board(); +/* We are now in PHASE_MACHINE_INITIALIZED. */ +qemu_create_cli_devices(); +/* + * At this point all CLI options are handled apart: + * + -S (autostart) + * + -incoming + */ +qemu_machine_creation_done(); +/* We are now in PHASE_MACHINE_READY. */ + +if (loadvm) { +load_snapshot(loadvm, NULL, false, NULL, _fatal); } +if (replay_mode != REPLAY_MODE_NONE) { +replay_vmstate_init(); +} + +if (incoming) { +Error *local_err = NULL; +if (strcmp(incoming, "defer") != 0) { +qmp_migrate_incoming(incoming, _err); +if (local_err) { +error_reportf_err(local_err, "-incoming %s: ", + incoming); +exit(1); +} +} +} else if (autostart) { +qmp_cont(NULL); +} +break; + +default: +/* + * If we end up here, it is because we miss a case above. + */ +error_setg(_abort, "Requested phase transition is not" + " implemented"); +return false; } -} else if (autostart) { -qmp_cont(NULL); + +/* + * Ensure we made some progress. +
Re: [RFC PATCH] python: add qmp-send program to send raw qmp commands to qemu
On 3/16/22 11:24, Daniel P. Berrangé wrote: On Wed, Mar 16, 2022 at 10:54:55AM +0100, Damien Hedde wrote: It takes an input file containing raw qmp commands (concatenated json dicts) and send all commands one by one to a qmp server. When one command fails, it exits. As a convenience, it can also wrap the qemu process to avoid having to start qemu in background. When wrapping qemu, the program returns only when the qemu process terminates. Signed-off-by: Damien Hedde --- Hi all, Following our discussion, I've started this. What do you think ? I tried to follow Daniel's qmp-shell-wrap. I think it is better to have similar options (eg: logging). There is also room for factorizing code if we want to keep them aligned and ease maintenance. Having CLI similarity to the existing scripts is a good idea. As a proof of usefulness, it might be worth trying to illustrate this qmp-send command by converting an I/O test. Quite a few I/O tests have code that look like: do_run_qemu() { echo Testing: "$@" | _filter_imgfmt $QEMU -nographic -qmp stdio -serial none "$@" echo } run_qemu() { do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp | _filter_qemu_io } run_qemu < I need to add stdin handling, but it should be straightforward. I'm more worried by what should happen if there is a failure that makes qemu hang, because then run_qemu won't exit. I'll take a look at the iotest. I expect the test will be killed at some point, I need to ensure that part is handled properly by qmp-send. Thanks, Damien
[RFC PATCH] python: add qmp-send program to send raw qmp commands to qemu
It takes an input file containing raw qmp commands (concatenated json dicts) and send all commands one by one to a qmp server. When one command fails, it exits. As a convenience, it can also wrap the qemu process to avoid having to start qemu in background. When wrapping qemu, the program returns only when the qemu process terminates. Signed-off-by: Damien Hedde --- Hi all, Following our discussion, I've started this. What do you think ? I tried to follow Daniel's qmp-shell-wrap. I think it is better to have similar options (eg: logging). There is also room for factorizing code if we want to keep them aligned and ease maintenance. There are still some pylint issues (too many branches in main and it does not like my context manager if else line). But it's kind of a mess to fix theses so I think it's enough for a first version. I name that qmp-send as Daniel proposed, maybe qmp-test matches better what I'm doing there ? Thanks, Damien --- python/qemu/aqmp/qmp_send.py | 229 +++ scripts/qmp/qmp-send | 11 ++ 2 files changed, 240 insertions(+) create mode 100644 python/qemu/aqmp/qmp_send.py create mode 100755 scripts/qmp/qmp-send diff --git a/python/qemu/aqmp/qmp_send.py b/python/qemu/aqmp/qmp_send.py new file mode 100644 index 00..cbca1d0205 --- /dev/null +++ b/python/qemu/aqmp/qmp_send.py @@ -0,0 +1,229 @@ +# +# Copyright (C) 2022 Greensocs +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. +# + +""" +usage: qmp-send [-h] [-f FILE] [-s SOCKET] [-v] [-p] [--wrap ...] + +Send raw qmp commands to qemu as long as they succeed. It either connects to a +remote qmp server using the provided socket or wrap the qemu process. It stops +sending the provided commands when a command fails (disconnection or error +response). + +optional arguments: + -h, --helpshow this help message and exit + -f FILE, --file FILE Input file containing the commands + -s SOCKET, --socket SOCKET +< UNIX socket path | TCP address:port > + -v, --verbose Verbose (echo commands sent and received) + -p, --pretty Pretty-print JSON + --wrap ...QEMU command line to invoke + +When qemu wrap option is used, this script waits for qemu to terminate but +never send any quit or kill command. This needs to be done manually. +""" + +import argparse +import contextlib +import json +import logging +import os +from subprocess import Popen +import sys +from typing import List, TextIO + +from qemu.aqmp import ConnectError, QMPError, SocketAddrT +from qemu.aqmp.legacy import ( +QEMUMonitorProtocol, +QMPBadPortError, +QMPMessage, +) + + +LOG = logging.getLogger(__name__) + + +class QmpRawDecodeError(Exception): +""" +Exception for raw qmp decoding + +msg: exception message +lineno: input line of the error +colno: input column of the error +""" +def __init__(self, msg: str, lineno: int, colno: int): +self.msg = msg +self.lineno = lineno +self.colno = colno +super().__init__(f"{msg}: line {lineno} column {colno}") + + +class QMPSendError(QMPError): +""" +QMP Send Base error class. +""" + + +class QMPSend(QEMUMonitorProtocol): +""" +QMP Send class. +""" +def __init__(self, address: SocketAddrT, + pretty: bool = False, + verbose: bool = False, + server: bool = False): +super().__init__(address, server=server) +self._verbose = verbose +self._pretty = pretty +self._server = server + +def setup_connection(self) -> None: +"""Setup the connetion with the remote client/server.""" +if self._server: +self.accept() +else: +self.connect() + +def _print(self, qmp_message: object) -> None: +jsobj = json.dumps(qmp_message, + indent=4 if self._pretty else None, + sort_keys=self._pretty) +print(str(jsobj)) + +def execute_cmd(self, cmd: QMPMessage) -> None: +"""Execute a qmp command.""" +if self._verbose: +self._print(cmd) +resp = self.cmd_obj(cmd) +if resp is None: +raise QMPSendError("Disconnected") +if self._verbose: +self._print(resp) +if 'error' in resp: +raise QMPSendError(f"Command failed: {resp['error']}") + + +def raw_load(file: TextIO) -> List[QMPMessage]: +"""parse a raw qmp command file. + +JSON formatted commands can expand on several lines but must +be separated by an end-of-line (two commands can not share
Re: [PATCH v4 11/14] softmmu/memory: add memory_region_try_add_subregion function
On 3/3/22 14:32, Philippe Mathieu-Daudé wrote: On 23/2/22 10:12, Damien Hedde wrote: Hi Philippe, I suppose it is ok if I change your mail in the reviewed by ? No, the email is fine (git tools should take care of using the correct email via the .mailmap entry, see commit 90f285fd83). Thanks, Damien ok. Looks like git keeps as-is the "*-by:" entries untouched when cc-ing them. -- Damien
Re: [PATCH v4 12/14] add sysbus-mmio-map qapi command
On 3/3/22 15:59, Philippe Mathieu-Daudé wrote: On 23/2/22 10:07, Damien Hedde wrote: This command allows to map an mmio region of sysbus device onto the system memory. Its behavior mimics the sysbus_mmio_map() function apart from the automatic unmap (the C function unmaps the region if it is already mapped). For the qapi function we consider it is an error to try to map an already mapped function. If unmapping is required, it is probably better to add a sysbus-mmip-unmap command. "sysbus-mmio-unmap" typo I presume. This command is still experimental (hence the 'unstable' feature), as it is related to the sysbus device creation through qapi commands. This command is required to be able to dynamically build a machine from scratch as there is no qapi-way of doing a memory mapping. Signed-off-by: Damien Hedde --- Cc: Alistair Francis v4: + integrate priority parameter + use 'unstable' feature flag instead of 'x-' prefix + bump version to 7.0 + dropped Alistair's reviewed-by as a consequence --- qapi/qdev.json | 31 ++ hw/core/sysbus.c | 49 2 files changed, 80 insertions(+) diff --git a/qapi/qdev.json b/qapi/qdev.json index 2e2de41499..4830e87a90 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -160,3 +160,34 @@ ## { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': { '*device': 'str', 'path': 'str' } } + +## +# @sysbus-mmio-map: +# +# Map a sysbus device mmio onto the main system bus. +# +# @device: the device's QOM path +# +# @mmio: The mmio number to be mapped (defaults to 0). +# +# @addr: The base address for the mapping. +# +# @priority: The priority of the mapping (defaults to 0). +# +# Features: +# @unstable: Command is meant to map sysbus devices +# while in preconfig mode. +# +# Since: 7.0 +# +# Returns: Nothing on success +# +## + +{ 'command': 'sysbus-mmio-map', + 'data': { 'device': 'str', + '*mmio': 'uint8', + 'addr': 'uint64', + '*priority': 'int32' }, I wonder if not providing the explicit parent container now could be problematic later, and if we shouldn't start with a QOM MR path (default to 'system_memory'). Anyway, sysbus are currently restricted to system_memory so as you described, this mimics well sysbus_mmio_map(). I think we ended-up adding such a parameter to handle complex xilinx machines having several cpu clusters. I wanted to stay simple in this series here as there are probably several way to address this issue. (we could also add a bus parameter, and create more sysbus). We can still add the parameter later, with an appropriate default value (or even make the parameter mandatory). If everybody agree to go for the bus-less approach. I can add the MR parameter right now. CCing Igor +void qmp_sysbus_mmio_map(const char *device, + bool has_mmio, uint8_t mmio, + uint64_t addr, + bool has_priority, int32_t priority, + Error **errp) +{ + Object *obj = object_resolve_path_type(device, TYPE_SYS_BUS_DEVICE, NULL); + SysBusDevice *dev; + + if (phase_get() != PHASE_MACHINE_INITIALIZED) { + error_setg(errp, "The command is permitted only when " + "the machine is in initialized phase"); "command only permitted during the " #PHASE_MACHINE_INITIALIZED "phase"? What do you mean by '#', this is not a macro parameter. PHASE_MACHINE_INITIALIZED is just an enum value and there is no human/qapi exposed name. "when the machine is initialized/initializing" ? I think all the machine phase error message are bit like that (not mentioning the phase).
Re: [PATCH v4 08/14] none-machine: add 'ram-addr' property
On 3/3/22 15:41, Philippe Mathieu-Daudé wrote: On 23/2/22 10:07, Damien Hedde wrote: Add the property to configure a the base address of the ram. The default value remains zero. This commit is needed to use the 'none' machine as a base, and subsequently to dynamically populate it using qapi commands. Having a non null 'ram' is really hard to workaround because of the actual constraints on the generic loader: it prevents loading binaries bigger than ram_size (with a null ram, we cannot load anything). For now we need to be able to use the existing ram creation feature of the none machine with a configurable base address. Signed-off-by: Damien Hedde --- hw/core/null-machine.c | 34 -- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c index 7eb258af07..5fd1cc0218 100644 --- a/hw/core/null-machine.c +++ b/hw/core/null-machine.c @@ -16,9 +16,11 @@ #include "hw/boards.h" #include "exec/address-spaces.h" #include "hw/core/cpu.h" +#include "qapi/visitor.h" struct NoneMachineState { MachineState parent; + uint64_t ram_addr; }; #define TYPE_NONE_MACHINE MACHINE_TYPE_NAME("none") @@ -26,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(NoneMachineState, NONE_MACHINE) static void machine_none_init(MachineState *mch) { + NoneMachineState *nms = NONE_MACHINE(mch); CPUState *cpu = NULL; /* Initialize CPU (if user asked for it) */ @@ -37,9 +40,13 @@ static void machine_none_init(MachineState *mch) } } - /* RAM at address zero */ + /* RAM at configured address (default: 0) */ if (mch->ram) { - memory_region_add_subregion(get_system_memory(), 0, mch->ram); + memory_region_add_subregion(get_system_memory(), nms->ram_addr, + mch->ram); + } else if (nms->ram_addr) { + error_report("'ram-addr' has been specified but the size is zero"); I'm not sure about this error message, IIUC we can get here if no ram backend is provided, not if we have one zero-sized. Otherwise LGTM. You're most probably right. Keeping the ram_size to 0 is just one way of getting here. I can replace the message by a more generic formulation "'ram-addr' has been specified but the machine has no ram"
Re: [PATCH v4 00/14] Initial support for machine creation via QMP
Ping ! It would be good to have some feedback on 1st and 2nd part. Thanks, Damien On 2/23/22 10:06, Damien Hedde wrote: Hi, This series adds initial support to build a machine using QMP/QAPI commands. With this series, one can start from the 'none' machine, create cpus, sysbus devices, memory map them and wire interrupts. Sorry for the huge cc list on this cover-letter. Apart from people who attended the kvm call about this topic, I've cc'ed you only according to MAINTAINERS file. The series is divided in 4 parts which are independent of each other, but we need the 4 parts to be able to use this mechanism: + Patches 1 to 6 allow to use the qapi command device_add to cold plug devices (like CLI -device do) + Patches 7 to 10 modify the 'none' machine which serves as base machine. + Patches 11 to 13 handle memory mapping and memory creation + Patches 14 allows dynamic cold plug of opentitan/sifive_e machine to build some example. This last patch is based on a cleanup series: it probably works without it, but some config errors are not handled (see based-on below). Only patch 11 is reviewed-by. v4: + cold plugging approach changed in order not to conflict with startup. I do not add additional command to handle this so that we can change everything easily. + device_add in cold plug context is also now equivalent to -device CLI regarding -fw_cfg. I also added patches to modify the 'none' machine. + reworked most of the none machine part + updated the sybus-mmio-map command patch Note that there are still lot of limitations (for example if you try to create more cpus than the _max_cpus_, tcg will abort()). Basically all tasks done by machine init reading some parameters are really tricky: for example, loading complex firmware. But we have to start by something and all this is not accessible unless the user asked for none machine and -preconfig. I can maintain the code introduced here. I'm not sure what's the process. Is there something else to do than propose a patch to MAINTAINERS ? If there is a global agreement on moving on with these feature, it would be great to have a login on qemu wiki so I can document limitations and the work being done to solve them. A simple test can be done with the following scenario which build a machine subset of the opentitan. $ cat commands.qmp // RAM 0x1000 device_add driver=sysbus-memory id=ram size=0x4000 readonly=false sysbus-mmio-map device=ram addr=268435456 // CPUS device_add driver=riscv.hart_array id=cpus cpu-type=lowrisc-ibex-riscv-cpu num-harts=1 resetvec=0x8080 // ROM 0x8000 device_add driver=sysbus-memory id=rom size=0x4000 readonly=true sysbus-mmio-map device=rom addr=32768 // PLIC 0x4800 device_add driver=riscv.sifive.plic id=plic hart-config=M hartid-base=0 num-sources=180 num-priorities=3 priority-base=0x0 pending-base=0x1000 enable-base=0x2000 enable-stride=32 context-base=0x20 context-stride=8 aperture-size=0x4005000 sysbus-mmio-map device=plic addr=1207959552 qom-set path=plic property=unnamed-gpio-out[1] value=cpus/harts[0]/unnamed-gpio-in[11] // UART 0x4000 device_add driver=ibex-uart id=uart chardev=serial0 sysbus-mmio-map device=uart addr=1073741824 qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2] // FIRMWARE device_add driver=loader cpu-num=0 file=/path/to/firmware.elf x-exit-preconfig $ qemu-system-riscv32 -display none -M none -preconfig -serial stdio -qmp unix:/tmp/qmp-sock,server In another terminal, you'll need to send the commands with, for example: $ grep -v '^//' commands.qmp | qmp-shell /tmp/qmp-sock -v It is the same as running $ qemu-system-riscv32 -display none -M opentitan -serial stdio -kernel path/to/firmware.elf If you need a firmware, you can pick this one https://github.com/GreenSocs/qemu-qmp-machines/blob/master/opentitan-echo.elf This firmware is just a small interrupt-based bare-metal program echoing back whatever is sent in the uart. This repo contains also sifive_e machine example. Based-on: <20220218164646.132112-1-damien.he...@greensocs.com> "RiscV cleanups for user-related life cycles" Thanks for your comments, -- Damien Damien Hedde (13): machine: add phase_get() and document phase_check()/advance() machine: introduce phase_until() to handle phase transitions vl: support machine-initialized target in phase_until() qapi/device_add: compute is_hotplug flag qapi/device_add: handle the rom_order_override when cold-plugging none-machine: add the NoneMachineState structure none-machine: add 'ram-addr' property none-machine: allow cold plugging sysbus devices none-machine: allow several cpus softmmu/memory: add memory_region_try_add_subregion function add sysbus-mmio-map qapi command hw/mem/system-memory: add a memory sysbus device hw: set user_creatable on opentitan/sifive_e devices Mirela Grujic (1): qapi/device_add: Allow execution in machine initialized phase
Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device
On 2/25/22 12:38, Igor Mammedov wrote: On Thu, 24 Feb 2022 12:43:21 +0100 Damien Hedde wrote: On 2/24/22 10:55, Igor Mammedov wrote: On Wed, 23 Feb 2022 11:19:49 +0100 Damien Hedde wrote: On 2/23/22 10:44, Igor Mammedov wrote: On Wed, 23 Feb 2022 10:07:05 +0100 Damien Hedde wrote: This device can be used to create a memory wrapped into a sysbus device. This device has one property 'readonly' which allows to choose between a ram or a rom. The purpose for this device is to be used with qapi command device_add. that's the way to add a device to QEMU but a don't actual purpose described here, i.e. how board will use this device/actual usecase and how it will be wired to board and why it does have to be a sysbus device. Sorry, this was unclear. It is a sysbus device in order to use it like any other sysbus device. The memory region it contains is exposed as a sysbus mmio. aside of that sysbus is legacy fictional bus (albeit widely used), it doesn't scale to non sysbus devices (me thinking about buss-less pc-dimm & co) since eventually we would like to create mainstream machine types via QMP as well. I can replace the commit message by the following paragraph: Boards instantiate memories by creating memory region objects which is not possible using QAPI commands. That's not entirely true, you can use object-add with hostmem backends which do provide a means to allocate memory_region. (there is no rom hostmem backend probably (i.e. one that return rom memory region) but that could be added). Another benefit of approach is that one can replace backing memory any other backend (file/memfd/pmem...) without affecting device model. I'm not familiar with memory backends. I need to take a look at them. To create a memory, the user can instantiate and map this device by issuing the following commands: device_add driver=sysbus-memory size=0x1000 id=someram sysbus-mmio-map device=someram addr=0 I'd imagine more generic approach would look like: object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks device_add memdevice_frontend,memdev=mem1,addr=0x0 where [pre]plug hooks in machine can map device to an appropriate address space/place at device realize time. (see memory_device_[pre_]plug() for starters). We cannot rely on hooks the machine would define, because we start from an empty machine. So anything must come from qapi and we would need to do something like that I suppose: object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks device_add sysbus-memory-frontend,memdev=mem1,id=memdev_fe sysbus-mmio-map device=memdev_fe addr=0 As I pointed out using legacy sysbus doesn't scale, also I'd avoid spawning more new device based on it if it can be helped. I'm not sure to get the issue with adding sysbus devices, there is a lot of them. And for most them, they'll never be put on anything else than a simple memory bus. This one is trivial. Right now there is a sysbus and the whole buses tree starts from it, it propagates reset. Everything is based on it. with bus-less design, machine is still empty, in advance prepared plug callbacks, is practically meta-data saying which device class map into which address space which is quite generic. It helps to avoid having extra QMP command for mapping. AFAIK the sysbus is the only bus type, on which we cannot specify the mapping/addresses with some device_add command parameter (this is probably doable, but hard). That's why I proposed to add sysbus-mmio-map several months ago. I didn't look again since, it's probably easier now with the modification done to device_add. However if prebuilt mapping is problematic, maybe have an alternative QMP command that would do mapping, just not limited to sysbus, something like map_at_as device=1 as={parent_mr_name,system,io} [addr=x overlap=y prio=z] which should give you full control where and how to map device. sysbus-mmio-map is not introduced by this series just for this memory device. It is here to solve mapping of any existing sysbus devices. By bus-less. You mean mapping a sysbus device on another address space than the main one exposed by the sysbus ? We can support this by adding an 'as' parameter to the mapping function. You mentioned non sysbus devices above. I don't think we need to try to do super-commands to solve all use cases. I think there is a need to map a sysbus device on a sysbus. Maybe there is also a need to map a non-sysbus device (a memory region then ?) to an address space. -- Damien
Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device
On 2/24/22 10:55, Igor Mammedov wrote: On Wed, 23 Feb 2022 11:19:49 +0100 Damien Hedde wrote: On 2/23/22 10:44, Igor Mammedov wrote: On Wed, 23 Feb 2022 10:07:05 +0100 Damien Hedde wrote: This device can be used to create a memory wrapped into a sysbus device. This device has one property 'readonly' which allows to choose between a ram or a rom. The purpose for this device is to be used with qapi command device_add. that's the way to add a device to QEMU but a don't actual purpose described here, i.e. how board will use this device/actual usecase and how it will be wired to board and why it does have to be a sysbus device. Sorry, this was unclear. It is a sysbus device in order to use it like any other sysbus device. The memory region it contains is exposed as a sysbus mmio. aside of that sysbus is legacy fictional bus (albeit widely used), it doesn't scale to non sysbus devices (me thinking about buss-less pc-dimm & co) since eventually we would like to create mainstream machine types via QMP as well. I can replace the commit message by the following paragraph: Boards instantiate memories by creating memory region objects which is not possible using QAPI commands. That's not entirely true, you can use object-add with hostmem backends which do provide a means to allocate memory_region. (there is no rom hostmem backend probably (i.e. one that return rom memory region) but that could be added). Another benefit of approach is that one can replace backing memory any other backend (file/memfd/pmem...) without affecting device model. I'm not familiar with memory backends. I need to take a look at them. To create a memory, the user can instantiate and map this device by issuing the following commands: device_add driver=sysbus-memory size=0x1000 id=someram sysbus-mmio-map device=someram addr=0 I'd imagine more generic approach would look like: object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks device_add memdevice_frontend,memdev=mem1,addr=0x0 where [pre]plug hooks in machine can map device to an appropriate address space/place at device realize time. (see memory_device_[pre_]plug() for starters). We cannot rely on hooks the machine would define, because we start from an empty machine. So anything must come from qapi and we would need to do something like that I suppose: object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks device_add sysbus-memory-frontend,memdev=mem1,id=memdev_fe sysbus-mmio-map device=memdev_fe addr=0 Thanks, -- Damien
Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option
On 2/23/22 19:20, John Snow wrote: On Wed, Feb 23, 2022 at 12:09 PM Damien Hedde wrote: On 2/23/22 17:18, John Snow wrote: On Wed, Feb 23, 2022 at 10:44 AM Daniel P. Berrangé wrote: On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote: On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé wrote: On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote: On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde wrote: This option makes qmp_shell exit (with error code 1) as soon as one of the following error occurs: + command parsing error + disconnection + command failure (response is an error) _execute_cmd() method now returns None or the response so that read_exec_command() can do the last check. This is meant to be used in combination with an input file redirection. It allows to store a list of commands into a file and try to run them by qmp_shell and easily see if it failed or not. Signed-off-by: Damien Hedde Based on this patch, it looks like you really want something scriptable, so I think the qemu-send idea that Dan has suggested might be the best way to go. Are you still hoping to use the interactive "short" QMP command format? That might be a bad idea, given how flaky the parsing is -- and how we don't actually have a published standard for that format. We've *never* liked the bad parsing here, so I have a reluctance to use it in more places. I'm having the naive idea that a script file could be as simple as a list of QMP commands to send: [ {"execute": "block-dirty-bitmap-add", "arguments": { ... }}, ... ] I'd really recommend against creating a new format for the script file, especially one needing opening & closing [] like this, as that isn't so amenable to dynamic usage/creation. ie you can't just append an extcra command to an existing file. IMHO, the "file" format should be identical to the result of capturing the socket data off the wire. ie just a concatenation of QMP commands, with no extra wrapping / change in format. Eugh. That's just so hard to parse, because there's no off-the-shelf tooling for "load a sequence of JSON documents". Nothing in Python does it. :\ It isn't that hard if you require each JSON doc to be followed by a newline. Feed one line at a time to the JSON parser, until you get a complete JSON doc, process that, then re-init the parser and carry on feeding it lines until it emits the next JSON doc, and so on. There's two interfaces in Python: (1) json.load(), which takes a file pointer and either returns a single, complete JSON document or it raises an Exception. It's not useful here at all. (2) json.JSONDecoder().raw_decode(strbuf), which takes a string buffer and returns a 2-tuple of a JSON Document and the position at which it stopped decoding. The second is what we need here, but it does require buffering the entire file into a string first, and then iteratively calling it. It feels like working against the grain a little bit. We also can't use the QAPI parser, as that parser has intentionally removed support for constructs we don't use in the qapi schema language. Boo. (Not that I want more non-standard configuration files like that propagating, either.) It would be possible to generate a JSON-Schema document to describe a script file that used a containing list construct, but impossible for a concatenation of JSON documents. This is one of the reasons I instinctively shy away from non-standard file formats, they tend to cut off support for this sort of thing. Wanting to keep the script easy to append to is legitimate. I'm keen to hear a bit more about the use case here before I press extremely hard in any given direction, but those are my impulses here. The use case is to be able to feed qemu with a bunch of commands we expect to succeed and let qemu continue (unlike Daniel's wrap use case, we don't want to quit qemu after the last command). Typically it's the use case I present in the following cover-letter: https://lore.kernel.org/qemu-devel/20220223090706.4888-1-damien.he...@greensocs.com/ OK (Sorry for blowing this out into a bigger ordeal than you had maybe hoped for. I want to get you happy and on your way ASAP, I promise) So, I see comments and simple QMP commands using the short-hand format. If I understand correctly, you want this script upstream so that you don't have to re-engineer the hack every time I shift something around in qmp-shell, and so that examples can be easily shared and reproduced between parties. Good reasons, so I want to help you out and get something merged. (An aside: In the past I have just copy-pasted stuff into my qmp-shell terminal. It's less reproducible for people who aren't used to using the tool, but it's been just enough juice for me in the past. I empathize with wanting to give someone a single-shot command they can copy-paste and have it Just Work.) Some observations: (1) Comments we don't have in JS
Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option
On 2/23/22 17:18, John Snow wrote: On Wed, Feb 23, 2022 at 10:44 AM Daniel P. Berrangé wrote: On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote: On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé wrote: On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote: On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde wrote: This option makes qmp_shell exit (with error code 1) as soon as one of the following error occurs: + command parsing error + disconnection + command failure (response is an error) _execute_cmd() method now returns None or the response so that read_exec_command() can do the last check. This is meant to be used in combination with an input file redirection. It allows to store a list of commands into a file and try to run them by qmp_shell and easily see if it failed or not. Signed-off-by: Damien Hedde Based on this patch, it looks like you really want something scriptable, so I think the qemu-send idea that Dan has suggested might be the best way to go. Are you still hoping to use the interactive "short" QMP command format? That might be a bad idea, given how flaky the parsing is -- and how we don't actually have a published standard for that format. We've *never* liked the bad parsing here, so I have a reluctance to use it in more places. I'm having the naive idea that a script file could be as simple as a list of QMP commands to send: [ {"execute": "block-dirty-bitmap-add", "arguments": { ... }}, ... ] I'd really recommend against creating a new format for the script file, especially one needing opening & closing [] like this, as that isn't so amenable to dynamic usage/creation. ie you can't just append an extcra command to an existing file. IMHO, the "file" format should be identical to the result of capturing the socket data off the wire. ie just a concatenation of QMP commands, with no extra wrapping / change in format. Eugh. That's just so hard to parse, because there's no off-the-shelf tooling for "load a sequence of JSON documents". Nothing in Python does it. :\ It isn't that hard if you require each JSON doc to be followed by a newline. Feed one line at a time to the JSON parser, until you get a complete JSON doc, process that, then re-init the parser and carry on feeding it lines until it emits the next JSON doc, and so on. There's two interfaces in Python: (1) json.load(), which takes a file pointer and either returns a single, complete JSON document or it raises an Exception. It's not useful here at all. (2) json.JSONDecoder().raw_decode(strbuf), which takes a string buffer and returns a 2-tuple of a JSON Document and the position at which it stopped decoding. The second is what we need here, but it does require buffering the entire file into a string first, and then iteratively calling it. It feels like working against the grain a little bit. We also can't use the QAPI parser, as that parser has intentionally removed support for constructs we don't use in the qapi schema language. Boo. (Not that I want more non-standard configuration files like that propagating, either.) It would be possible to generate a JSON-Schema document to describe a script file that used a containing list construct, but impossible for a concatenation of JSON documents. This is one of the reasons I instinctively shy away from non-standard file formats, they tend to cut off support for this sort of thing. Wanting to keep the script easy to append to is legitimate. I'm keen to hear a bit more about the use case here before I press extremely hard in any given direction, but those are my impulses here. The use case is to be able to feed qemu with a bunch of commands we expect to succeed and let qemu continue (unlike Daniel's wrap use case, we don't want to quit qemu after the last command). Typically it's the use case I present in the following cover-letter: https://lore.kernel.org/qemu-devel/20220223090706.4888-1-damien.he...@greensocs.com/ -- Damien
Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option
On 2/23/22 17:43, Damien Hedde wrote: On 2/23/22 16:44, Daniel P. Berrangé wrote: On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote: On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé wrote: On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote: On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde wrote: This option makes qmp_shell exit (with error code 1) as soon as one of the following error occurs: + command parsing error + disconnection + command failure (response is an error) _execute_cmd() method now returns None or the response so that read_exec_command() can do the last check. This is meant to be used in combination with an input file redirection. It allows to store a list of commands into a file and try to run them by qmp_shell and easily see if it failed or not. Signed-off-by: Damien Hedde Based on this patch, it looks like you really want something scriptable, so I think the qemu-send idea that Dan has suggested might be the best way to go. Are you still hoping to use the interactive "short" QMP command format? That might be a bad idea, given how flaky the parsing is -- and how we don't actually have a published standard for that format. We've *never* liked the bad parsing here, so I have a reluctance to use it in more places. I don't really care of the command format. I was just using the one expected by qmp-shell to avoid providing another script. I think it's better not to use some standard syntax like json. I wanted to say the opposite: it's best to use json. As long as we can store the commands into a file and tests them easily, it's ok. The crucial feature is the "stop as soon something unexpected happens" so that we can easily spot an issue. I'm having the naive idea that a script file could be as simple as a list of QMP commands to send: [ {"execute": "block-dirty-bitmap-add", "arguments": { ... }}, ... ] I used this format at some point because it's so trivial to feed into the QMP tools. Even used a yaml version of that to get the "human readability" that goes with it. I'd really recommend against creating a new format for the script file, especially one needing opening & closing [] like this, as that isn't so amenable to dynamic usage/creation. ie you can't just append an extcra command to an existing file. IMHO, the "file" format should be identical to the result of capturing the socket data off the wire. ie just a concatenation of QMP commands, with no extra wrapping / change in format. >> Eugh. That's just so hard to parse, because there's no off-the-shelf tooling for "load a sequence of JSON documents". Nothing in Python does it. :\ It isn't that hard if you require each JSON doc to be followed by a newline. Feed one line at a time to the JSON parser, until you get a complete JSON doc, process that, then re-init the parser and carry on feeding it lines until it emits the next JSON doc, and so on. I agree it's doable. I can look into that. It makes me think that I've managed to modify the chardev 'file' backend a few months ago so that it can be used with an input file on the cli. This allowed to give such raw qmp file directly with the -qmp option instead of using an intermediate socket and a script issuing the same file. But I gave up with this approach because then it can't stop if a command failed without hacking into the receiving side in qemu. -- Damien
Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option
On 2/23/22 16:44, Daniel P. Berrangé wrote: On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote: On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé wrote: On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote: On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde wrote: This option makes qmp_shell exit (with error code 1) as soon as one of the following error occurs: + command parsing error + disconnection + command failure (response is an error) _execute_cmd() method now returns None or the response so that read_exec_command() can do the last check. This is meant to be used in combination with an input file redirection. It allows to store a list of commands into a file and try to run them by qmp_shell and easily see if it failed or not. Signed-off-by: Damien Hedde Based on this patch, it looks like you really want something scriptable, so I think the qemu-send idea that Dan has suggested might be the best way to go. Are you still hoping to use the interactive "short" QMP command format? That might be a bad idea, given how flaky the parsing is -- and how we don't actually have a published standard for that format. We've *never* liked the bad parsing here, so I have a reluctance to use it in more places. I don't really care of the command format. I was just using the one expected by qmp-shell to avoid providing another script. I think it's better not to use some standard syntax like json. As long as we can store the commands into a file and tests them easily, it's ok. The crucial feature is the "stop as soon something unexpected happens" so that we can easily spot an issue. I'm having the naive idea that a script file could be as simple as a list of QMP commands to send: [ {"execute": "block-dirty-bitmap-add", "arguments": { ... }}, ... ] I used this format at some point because it's so trivial to feed into the QMP tools. Even used a yaml version of that to get the "human readability" that goes with it. I'd really recommend against creating a new format for the script file, especially one needing opening & closing [] like this, as that isn't so amenable to dynamic usage/creation. ie you can't just append an extcra command to an existing file. IMHO, the "file" format should be identical to the result of capturing the socket data off the wire. ie just a concatenation of QMP commands, with no extra wrapping / change in format. >> Eugh. That's just so hard to parse, because there's no off-the-shelf tooling for "load a sequence of JSON documents". Nothing in Python does it. :\ It isn't that hard if you require each JSON doc to be followed by a newline. Feed one line at a time to the JSON parser, until you get a complete JSON doc, process that, then re-init the parser and carry on feeding it lines until it emits the next JSON doc, and so on. I agree it's doable. I can look into that. It makes me think that I've managed to modify the chardev 'file' backend a few months ago so that it can be used with an input file on the cli. This allowed to give such raw qmp file directly with the -qmp option instead of using an intermediate socket and a script issuing the same file. But I gave up with this approach because then it can't stop if a command failed without hacking into the receiving side in qemu. -- Damien
Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device
On 2/23/22 10:44, Igor Mammedov wrote: On Wed, 23 Feb 2022 10:07:05 +0100 Damien Hedde wrote: This device can be used to create a memory wrapped into a sysbus device. This device has one property 'readonly' which allows to choose between a ram or a rom. The purpose for this device is to be used with qapi command device_add. that's the way to add a device to QEMU but a don't actual purpose described here, i.e. how board will use this device/actual usecase and how it will be wired to board and why it does have to be a sysbus device. Sorry, this was unclear. It is a sysbus device in order to use it like any other sysbus device. The memory region it contains is exposed as a sysbus mmio. I can replace the commit message by the following paragraph: Boards instantiate memories by creating memory region objects which is not possible using QAPI commands. To create a memory, the user can instantiate and map this device by issuing the following commands: device_add driver=sysbus-memory size=0x1000 id=someram sysbus-mmio-map device=someram addr=0 Signed-off-by: Damien Hedde --- include/hw/mem/sysbus-memory.h | 28 hw/mem/sysbus-memory.c | 80 ++ hw/mem/meson.build | 2 + 3 files changed, 110 insertions(+) create mode 100644 include/hw/mem/sysbus-memory.h create mode 100644 hw/mem/sysbus-memory.c diff --git a/include/hw/mem/sysbus-memory.h b/include/hw/mem/sysbus-memory.h new file mode 100644 index 00..5c596f8b4f --- /dev/null +++ b/include/hw/mem/sysbus-memory.h @@ -0,0 +1,28 @@ +/* + * SPDX-License-Identifier: GPL-2.0-or-later + * + * SysBusDevice Memory + * + * Copyright (c) 2021 Greensocs + */ + +#ifndef HW_SYSBUS_MEMORY_H +#define HW_SYSBUS_MEMORY_H + +#include "hw/sysbus.h" +#include "qom/object.h" + +#define TYPE_SYSBUS_MEMORY "sysbus-memory" +OBJECT_DECLARE_SIMPLE_TYPE(SysBusMemoryState, SYSBUS_MEMORY) + +struct SysBusMemoryState { +/* */ +SysBusDevice parent_obj; +uint64_t size; +bool readonly; + +/* */ +MemoryRegion mem; +}; + +#endif /* HW_SYSBUS_MEMORY_H */ diff --git a/hw/mem/sysbus-memory.c b/hw/mem/sysbus-memory.c new file mode 100644 index 00..f1ad7ba7ec --- /dev/null +++ b/hw/mem/sysbus-memory.c @@ -0,0 +1,80 @@ +/* + * SPDX-License-Identifier: GPL-2.0-or-later + * + * SysBusDevice Memory + * + * Copyright (c) 2021 Greensocs + */ + +#include "qemu/osdep.h" +#include "hw/mem/sysbus-memory.h" +#include "hw/qdev-properties.h" +#include "qemu/log.h" +#include "qemu/module.h" +#include "qapi/error.h" + +static Property sysbus_memory_properties[] = { +DEFINE_PROP_UINT64("size", SysBusMemoryState, size, 0), +DEFINE_PROP_BOOL("readonly", SysBusMemoryState, readonly, false), +DEFINE_PROP_END_OF_LIST(), +}; + +static void sysbus_memory_realize(DeviceState *dev, Error **errp) +{ +SysBusMemoryState *s = SYSBUS_MEMORY(dev); +gchar *name; + +if (!s->size) { +error_setg(errp, "'size' must be non-zero."); +return; +} + +/* + * We impose having an id (which is unique) because we need to generate + * a unique name for the memory region. + * memory_region_init_ram/rom() will abort() (in qemu_ram_set_idstr() + * function if 2 system-memory devices are created with the same name + * for the memory region). + */ +if (!dev->id) { +error_setg(errp, "system-memory device must have an id."); +return; +} +name = g_strdup_printf("%s.region", dev->id); + +if (s->readonly) { +memory_region_init_rom(>mem, OBJECT(dev), name, s->size, errp); +} else { +memory_region_init_ram(>mem, OBJECT(dev), name, s->size, errp); +} + +g_free(name); +if (*errp) { +return; +} + +sysbus_init_mmio(SYS_BUS_DEVICE(s), >mem); +} + +static void sysbus_memory_class_init(ObjectClass *klass, void *data) +{ +DeviceClass *dc = DEVICE_CLASS(klass); + +dc->user_creatable = true; +dc->realize = sysbus_memory_realize; +device_class_set_props(dc, sysbus_memory_properties); +} + +static const TypeInfo sysbus_memory_info = { +.name = TYPE_SYSBUS_MEMORY, +.parent= TYPE_SYS_BUS_DEVICE, +.instance_size = sizeof(SysBusMemoryState), +.class_init= sysbus_memory_class_init, +}; + +static void sysbus_memory_register_types(void) +{ +type_register_static(_memory_info); +} + +type_init(sysbus_memory_register_types) diff --git a/hw/mem/meson.build b/hw/mem/meson.build index 82f86d117e..04c74e12f2 100644 --- a/hw/mem/meson.build +++ b/hw/mem/meson.build @@ -7,3 +7,5 @@ mem_ss.add(when: 'CONFIG_NVDIMM', if_true: files('nvdimm.c')) softmmu_ss.add_all(when: 'CONFIG_MEM_DEVICE', if_true: mem_ss) softmmu_ss.add(when: 'CONFIG_SPARSE_MEM', if_true: files('sparse-mem.c')) + +softmmu_ss.add(files('sysbus-memory.c'))
Re: [PATCH 0/5] qmp-shell modifications for non-interactive use
On 2/22/22 11:31, Daniel P. Berrangé wrote: On Tue, Feb 22, 2022 at 10:38:09AM +0100, Damien Hedde wrote: Here I just wanted to propose a simple way to just send a bunch of commands from a source file and stop if something unexpected happens. Only goal is to be able to share a file on the ml and allow people to reproduce easily. We can already redirect the input, but it is almost impossible to see if something failed. Yes, I see what you mean. So the problem with using 'socat' or similar is that we fill the input with commands and response appear asynchronously, so we can't match them up easily. This is actually a problem seen in the block I/O tests which just send QMP stuff in a batch. While you could do this by invoking socat once for each command, that gets silly with the repeated QMP handshake for each command. The thing about using qmp-shell is that it does a bunch of extra stuff targetted at humans on top, and history tells us it isn't a good idea to mix stuff for humans and machines in the same tool/interface. How about instead creating a separate 'qmp-send' command that is not much more than a "QMP-aware socat". By which I mean, it just reads raw QMP commands from stdin, sends each one to the server, but crucially waits for a reply after sending each, and stops on first error reponse. By 'qmp-send' command, you mean another script in scripts/qmp ? Yes If we go for another script, I would rather do one with wrap feature (like your series) to start qemu as well. -- Damien
[PATCH v4 07/14] none-machine: add the NoneMachineState structure
The none machine was using the parent state structure. We'll need a custom state to add a field in the following commit. Signed-off-by: Damien Hedde --- hw/core/null-machine.c | 24 ++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c index f586a4bef5..7eb258af07 100644 --- a/hw/core/null-machine.c +++ b/hw/core/null-machine.c @@ -17,6 +17,13 @@ #include "exec/address-spaces.h" #include "hw/core/cpu.h" +struct NoneMachineState { +MachineState parent; +}; + +#define TYPE_NONE_MACHINE MACHINE_TYPE_NAME("none") +OBJECT_DECLARE_SIMPLE_TYPE(NoneMachineState, NONE_MACHINE) + static void machine_none_init(MachineState *mch) { CPUState *cpu = NULL; @@ -42,8 +49,10 @@ static void machine_none_init(MachineState *mch) } } -static void machine_none_machine_init(MachineClass *mc) +static void machine_none_class_init(ObjectClass *oc, void *data) { +MachineClass *mc = MACHINE_CLASS(oc); + mc->desc = "empty machine"; mc->init = machine_none_init; mc->max_cpus = 1; @@ -56,4 +65,15 @@ static void machine_none_machine_init(MachineClass *mc) mc->no_sdcard = 1; } -DEFINE_MACHINE("none", machine_none_machine_init) +static const TypeInfo none_machine_info = { +.name = TYPE_NONE_MACHINE, +.parent= TYPE_MACHINE, +.instance_size = sizeof(NoneMachineState), +.class_init= machine_none_class_init, +}; + +static void none_machine_register_types(void) +{ +type_register_static(_machine_info); +} +type_init(none_machine_register_types); -- 2.35.1
[PATCH v4 01/14] machine: add phase_get() and document phase_check()/advance()
phase_get() returns the current phase, we'll use it in next commit. Signed-off-by: Damien Hedde --- include/hw/qdev-core.h | 19 +++ hw/core/qdev.c | 5 + 2 files changed, 24 insertions(+) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 92c3d65208..e29c705b74 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -887,7 +887,26 @@ typedef enum MachineInitPhase { PHASE_MACHINE_READY, } MachineInitPhase; +/* + * phase_get: + * Returns the current phase + */ +MachineInitPhase phase_get(void); + +/** + * phase_check: + * Test if current phase is at least @phase. + * + * Returns true if this is the case. + */ extern bool phase_check(MachineInitPhase phase); + +/** + * @phase_advance: + * Update the current phase to @phase. + * + * Must only be used to make a single phase step. + */ extern void phase_advance(MachineInitPhase phase); #endif diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 84f3019440..632dc0a4be 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -910,6 +910,11 @@ Object *qdev_get_machine(void) static MachineInitPhase machine_phase; +MachineInitPhase phase_get(void) +{ +return machine_phase; +} + bool phase_check(MachineInitPhase phase) { return machine_phase >= phase; -- 2.35.1
[PATCH v4 11/14] softmmu/memory: add memory_region_try_add_subregion function
It allows adding a subregion to a memory region with error handling. Like memory_region_add_subregion_overlap(), it handles priority as well. Apart from the error handling, the behavior is the same. It can be used to do the simple memory_region_add_subregion() (with no overlap) by setting the priority parameter to 0. This commit is a preparation to further use of this function in the context of qapi command which needs error handling support. Signed-off-by: Damien Hedde Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: David Hildenbrand Reviewed-by: Alistair Francis --- include/exec/memory.h | 22 ++ softmmu/memory.c | 23 +++ 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/include/exec/memory.h b/include/exec/memory.h index 4d5997e6bb..070dcb5255 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -2215,6 +2215,28 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr, MemoryRegion *subregion, int priority); +/** + * memory_region_try_add_subregion: Add a subregion to a container + * with error handling. + * + * Behaves like memory_region_add_subregion_overlap(), but errors are + * reported if the subregion cannot be added. + * + * @mr: the region to contain the new subregion; must be a container + * initialized with memory_region_init(). + * @offset: the offset relative to @mr where @subregion is added. + * @subregion: the subregion to be added. + * @priority: used for resolving overlaps; highest priority wins. + * @errp: pointer to Error*, to store an error if it happens. + * + * Returns: True in case of success, false otherwise. + */ +bool memory_region_try_add_subregion(MemoryRegion *mr, + hwaddr offset, + MemoryRegion *subregion, + int priority, + Error **errp); + /** * memory_region_get_ram_addr: Get the ram address associated with a memory * region diff --git a/softmmu/memory.c b/softmmu/memory.c index 678dc62f06..6bc76bf6da 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -2541,27 +2541,34 @@ done: memory_region_transaction_commit(); } -static void memory_region_add_subregion_common(MemoryRegion *mr, - hwaddr offset, - MemoryRegion *subregion) +bool memory_region_try_add_subregion(MemoryRegion *mr, + hwaddr offset, + MemoryRegion *subregion, + int priority, + Error **errp) { MemoryRegion *alias; -assert(!subregion->container); +if (subregion->container) { +error_setg(errp, "The memory region is already in another region"); +return false; +} + +subregion->priority = priority; subregion->container = mr; for (alias = subregion->alias; alias; alias = alias->alias) { alias->mapped_via_alias++; } subregion->addr = offset; memory_region_update_container_subregions(subregion); +return true; } void memory_region_add_subregion(MemoryRegion *mr, hwaddr offset, MemoryRegion *subregion) { -subregion->priority = 0; -memory_region_add_subregion_common(mr, offset, subregion); +memory_region_try_add_subregion(mr, offset, subregion, 0, _abort); } void memory_region_add_subregion_overlap(MemoryRegion *mr, @@ -2569,8 +2576,8 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr, MemoryRegion *subregion, int priority) { -subregion->priority = priority; -memory_region_add_subregion_common(mr, offset, subregion); +memory_region_try_add_subregion(mr, offset, subregion, priority, +_abort); } void memory_region_del_subregion(MemoryRegion *mr, -- 2.35.1
[PATCH v4 03/14] vl: support machine-initialized target in phase_until()
phase_until() now supports the following transitions: + accel-created -> machine-initialized + machine-initialized -> machine-ready As a consequence we can now support the use of qmp_exit_preconfig() from phases _accel-created_ and _machine-initialized_. This commit is a preparation to support cold plugging a device using qapi (which will be introduced in a following commit). For this we need fine grain control of the phase. Signed-off-by: Damien Hedde --- softmmu/vl.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/softmmu/vl.c b/softmmu/vl.c index 5689d0be88..50337d68b9 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2737,8 +2737,8 @@ static void qemu_machine_creation_done(void) void qmp_x_exit_preconfig(Error **errp) { -if (phase_check(PHASE_MACHINE_INITIALIZED)) { -error_setg(errp, "The command is permitted only before machine initialization"); +if (phase_check(PHASE_MACHINE_READY)) { +error_setg(errp, "The command is permitted only before machine is ready"); return; } phase_until(PHASE_MACHINE_READY, errp); @@ -2759,7 +2759,17 @@ bool phase_until(MachineInitPhase phase, Error **errp) case PHASE_ACCEL_CREATED: qemu_init_board(); /* We are now in PHASE_MACHINE_INITIALIZED. */ +/* + * Handle CLI devices now in order leave the function in a state + * where we can cold plug devices with QMP. The following call + * handles the CLI options: + * + -fw_cfg (has side effects on device cold plug) + * + -device + */ qemu_create_cli_devices(); +break; + +case PHASE_MACHINE_INITIALIZED: /* * At this point all CLI options are handled apart: * + -S (autostart) -- 2.35.1
Re: [PATCH v4 11/14] softmmu/memory: add memory_region_try_add_subregion function
Hi Philippe, I suppose it is ok if I change your mail in the reviewed by ? Thanks, Damien On 2/23/22 10:07, Damien Hedde wrote: It allows adding a subregion to a memory region with error handling. Like memory_region_add_subregion_overlap(), it handles priority as well. Apart from the error handling, the behavior is the same. It can be used to do the simple memory_region_add_subregion() (with no overlap) by setting the priority parameter to 0. This commit is a preparation to further use of this function in the context of qapi command which needs error handling support. Signed-off-by: Damien Hedde Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: David Hildenbrand Reviewed-by: Alistair Francis --- include/exec/memory.h | 22 ++ softmmu/memory.c | 23 +++ 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/include/exec/memory.h b/include/exec/memory.h index 4d5997e6bb..070dcb5255 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -2215,6 +2215,28 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr, MemoryRegion *subregion, int priority); +/** + * memory_region_try_add_subregion: Add a subregion to a container + * with error handling. + * + * Behaves like memory_region_add_subregion_overlap(), but errors are + * reported if the subregion cannot be added. + * + * @mr: the region to contain the new subregion; must be a container + * initialized with memory_region_init(). + * @offset: the offset relative to @mr where @subregion is added. + * @subregion: the subregion to be added. + * @priority: used for resolving overlaps; highest priority wins. + * @errp: pointer to Error*, to store an error if it happens. + * + * Returns: True in case of success, false otherwise. + */ +bool memory_region_try_add_subregion(MemoryRegion *mr, + hwaddr offset, + MemoryRegion *subregion, + int priority, + Error **errp); + /** * memory_region_get_ram_addr: Get the ram address associated with a memory * region diff --git a/softmmu/memory.c b/softmmu/memory.c index 678dc62f06..6bc76bf6da 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -2541,27 +2541,34 @@ done: memory_region_transaction_commit(); } -static void memory_region_add_subregion_common(MemoryRegion *mr, - hwaddr offset, - MemoryRegion *subregion) +bool memory_region_try_add_subregion(MemoryRegion *mr, + hwaddr offset, + MemoryRegion *subregion, + int priority, + Error **errp) { MemoryRegion *alias; -assert(!subregion->container); +if (subregion->container) { +error_setg(errp, "The memory region is already in another region"); +return false; +} + +subregion->priority = priority; subregion->container = mr; for (alias = subregion->alias; alias; alias = alias->alias) { alias->mapped_via_alias++; } subregion->addr = offset; memory_region_update_container_subregions(subregion); +return true; } void memory_region_add_subregion(MemoryRegion *mr, hwaddr offset, MemoryRegion *subregion) { -subregion->priority = 0; -memory_region_add_subregion_common(mr, offset, subregion); +memory_region_try_add_subregion(mr, offset, subregion, 0, _abort); } void memory_region_add_subregion_overlap(MemoryRegion *mr, @@ -2569,8 +2576,8 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr, MemoryRegion *subregion, int priority) { -subregion->priority = priority; -memory_region_add_subregion_common(mr, offset, subregion); +memory_region_try_add_subregion(mr, offset, subregion, priority, +_abort); } void memory_region_del_subregion(MemoryRegion *mr,
[PATCH v4 12/14] add sysbus-mmio-map qapi command
This command allows to map an mmio region of sysbus device onto the system memory. Its behavior mimics the sysbus_mmio_map() function apart from the automatic unmap (the C function unmaps the region if it is already mapped). For the qapi function we consider it is an error to try to map an already mapped function. If unmapping is required, it is probably better to add a sysbus-mmip-unmap command. This command is still experimental (hence the 'unstable' feature), as it is related to the sysbus device creation through qapi commands. This command is required to be able to dynamically build a machine from scratch as there is no qapi-way of doing a memory mapping. Signed-off-by: Damien Hedde --- Cc: Alistair Francis v4: + integrate priority parameter + use 'unstable' feature flag instead of 'x-' prefix + bump version to 7.0 + dropped Alistair's reviewed-by as a consequence --- qapi/qdev.json | 31 ++ hw/core/sysbus.c | 49 2 files changed, 80 insertions(+) diff --git a/qapi/qdev.json b/qapi/qdev.json index 2e2de41499..4830e87a90 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -160,3 +160,34 @@ ## { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': { '*device': 'str', 'path': 'str' } } + +## +# @sysbus-mmio-map: +# +# Map a sysbus device mmio onto the main system bus. +# +# @device: the device's QOM path +# +# @mmio: The mmio number to be mapped (defaults to 0). +# +# @addr: The base address for the mapping. +# +# @priority: The priority of the mapping (defaults to 0). +# +# Features: +# @unstable: Command is meant to map sysbus devices +#while in preconfig mode. +# +# Since: 7.0 +# +# Returns: Nothing on success +# +## + +{ 'command': 'sysbus-mmio-map', + 'data': { 'device': 'str', +'*mmio': 'uint8', +'addr': 'uint64', +'*priority': 'int32' }, + 'features': ['unstable'], + 'allow-preconfig' : true } diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c index 05c1da3d31..df1f1f43a5 100644 --- a/hw/core/sysbus.c +++ b/hw/core/sysbus.c @@ -23,6 +23,7 @@ #include "hw/sysbus.h" #include "monitor/monitor.h" #include "exec/address-spaces.h" +#include "qapi/qapi-commands-qdev.h" static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent); static char *sysbus_get_fw_dev_path(DeviceState *dev); @@ -154,6 +155,54 @@ static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr, } } +void qmp_sysbus_mmio_map(const char *device, + bool has_mmio, uint8_t mmio, + uint64_t addr, + bool has_priority, int32_t priority, + Error **errp) +{ +Object *obj = object_resolve_path_type(device, TYPE_SYS_BUS_DEVICE, NULL); +SysBusDevice *dev; + +if (phase_get() != PHASE_MACHINE_INITIALIZED) { +error_setg(errp, "The command is permitted only when " + "the machine is in initialized phase"); +return; +} + +if (obj == NULL) { +error_setg(errp, "Device '%s' not found", device); +return; +} +dev = SYS_BUS_DEVICE(obj); + +if (!has_mmio) { +mmio = 0; +} +if (!has_priority) { +priority = 0; +} + +if (mmio >= dev->num_mmio) { +error_setg(errp, "MMIO index '%u' does not exist in '%s'", + mmio, device); +return; +} + +if (dev->mmio[mmio].addr != (hwaddr)-1) { +error_setg(errp, "MMIO index '%u' is already mapped", mmio); +return; +} + +if (!memory_region_try_add_subregion(get_system_memory(), addr, + dev->mmio[mmio].memory, priority, + errp)) { +return; +} + +dev->mmio[mmio].addr = addr; +} + void sysbus_mmio_unmap(SysBusDevice *dev, int n) { assert(n >= 0 && n < dev->num_mmio); -- 2.35.1
[PATCH v4 04/14] qapi/device_add: compute is_hotplug flag
Instead of checking the phase everytime, just store the result in a flag. We will use more of it in the following commit. Signed-off-by: Damien Hedde --- softmmu/qdev-monitor.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 01f3834db5..47a89aee20 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -617,6 +617,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, char *id; DeviceState *dev = NULL; BusState *bus = NULL; +bool is_hotplug = phase_check(PHASE_MACHINE_READY); driver = qdict_get_try_str(opts, "driver"); if (!driver) { @@ -660,7 +661,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, return NULL; } -if (phase_check(PHASE_MACHINE_READY) && bus && !qbus_is_hotpluggable(bus)) { +if (is_hotplug && bus && !qbus_is_hotpluggable(bus)) { error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name); return NULL; } @@ -674,7 +675,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, dev = qdev_new(driver); /* Check whether the hotplug is allowed by the machine */ -if (phase_check(PHASE_MACHINE_READY)) { +if (is_hotplug) { if (!qdev_hotplug_allowed(dev, errp)) { goto err_del_dev; } -- 2.35.1