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
Re: [PATCH v2 08/15] qdev: Make DeviceState.id independent of QemuOpts
On 10/8/21 15:34, Kevin Wolf wrote: DeviceState.id is a pointer to a string that is stored in the QemuOpts object DeviceState.opts and freed together with it. We want to create devices without going through QemuOpts in the future, so make this a separately allocated string. Signed-off-by: Kevin Wolf Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Damien Hedde
Re: [PATCH v2 02/15] net/vhost-user: Fix device compatibility check
On 10/8/21 15:34, Kevin Wolf wrote:
vhost-user works only with specific devices. At startup, it second
guesses what the command line option handling will do and error out if
it thinks a non-virtio device will attach to them.
This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.
Drop the old checks and implement .check_peer_type() instead to fix
this. As a nice side effect, it also removes one more dependency on the
legacy QemuOpts infrastructure and even reduces the code size.
Signed-off-by: Kevin Wolf
Reviewed-by: Damien Hedde
Re: [PATCH v2 03/15] net/vhost-vdpa: Fix device compatibility check
On 10/8/21 15:34, Kevin Wolf wrote:
vhost-vdpa works only with specific devices. At startup, it second
guesses what the command line option handling will do and error out if
it thinks a non-virtio device will attach to them.
This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.
Drop the old checks and implement .check_peer_type() instead to fix
this. As a nice side effect, it also removes one more dependency on the
legacy QemuOpts infrastructure and even reduces the code size.
Signed-off-by: Kevin Wolf
Reviewed-by: Damien Hedde
Re: [PATCH v2 01/15] net: Introduce NetClientInfo.check_peer_type()
On 10/8/21 15:34, Kevin Wolf wrote:
Some network backends (vhost-user and vhost-vdpa) work only with
specific devices. At startup, they second guess what the command line
option handling will do and error out if they think a non-virtio device
will attach to them.
This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.
Add a callback where backends can check compatibility with a device when
it actually tries to attach, even on hotplug.
Signed-off-by: Kevin Wolf
Reviewed-by: Damien Hedde
---
include/net/net.h| 2 ++
hw/core/qdev-properties-system.c | 6 ++
2 files changed, 8 insertions(+)
diff --git a/include/net/net.h b/include/net/net.h
index 5d1508081f..986288eb07 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -62,6 +62,7 @@ typedef struct SocketReadState SocketReadState;
typedef void (SocketReadStateFinalize)(SocketReadState *rs);
typedef void (NetAnnounce)(NetClientState *);
typedef bool (SetSteeringEBPF)(NetClientState *, int);
+typedef bool (NetCheckPeerType)(NetClientState *, ObjectClass *, Error **);
typedef struct NetClientInfo {
NetClientDriver type;
@@ -84,6 +85,7 @@ typedef struct NetClientInfo {
SetVnetBE *set_vnet_be;
NetAnnounce *announce;
SetSteeringEBPF *set_steering_ebpf;
+NetCheckPeerType *check_peer_type;
} NetClientInfo;
struct NetClientState {
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index e71f5d64d1..a91f60567a 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -431,6 +431,12 @@ static void set_netdev(Object *obj, Visitor *v, const char
*name,
goto out;
}
+if (peers[i]->info->check_peer_type) {
+if (!peers[i]->info->check_peer_type(peers[i], obj->class, errp)) {
+goto out;
+}
+}
+
ncs[i] = peers[i];
ncs[i]->queue_index = i;
}
Re: [PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
On 10/11/21 23:00, Eric Blake wrote:
On Fri, Oct 08, 2021 at 03:34:36PM +0200, Kevin Wolf wrote:
From: Damien Hedde
qdev_set_id() is mostly used when the user adds a device (using
-device cli option or device_add qmp command). This commit adds
an error parameter to handle the case where the given id is
already taken.
Also document the function and add a return value in order to
be able to capture success/failure: the function now returns the
id in case of success, or NULL in case of failure.
The commit modifies the 2 calling places (qdev-monitor and
xen-legacy-backend) to add the error object parameter.
Note that the id is, right now, guaranteed to be unique because
all ids came from the "device" QemuOptsList where the id is used
as key. This addition is a preparation for a future commit which
will relax the uniqueness.
Signed-off-by: Damien Hedde
Signed-off-by: Kevin Wolf
---
+} else {
+error_setg(errp, "Duplicate device ID '%s'", id);
+return NULL;
id is not freed on this error path...
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
@@ -691,7 +703,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
}
}
-qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+/*
+ * set dev's parent and register its id.
+ * If it fails it means the id is already taken.
+ */
+if (!qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp)) {
+goto err_del_dev;
...nor on this, which means the g_strdup() leaks on failure.
Since we strdup the id just before calling qdev_set_id.
Maybe we should do the the strdup in qdev_set_id (and free things on
error there too). It seems simplier than freeing things on the callee
side just in case of an error.
Damien
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
On 10/5/21 16:37, Kevin Wolf wrote:
Am 27.09.2021 um 13:39 hat Kevin Wolf geschrieben:
Am 27.09.2021 um 13:06 hat Damien Hedde geschrieben:
On 9/24/21 11:04, Kevin Wolf wrote:
Directly call qdev_device_add_from_qdict() for QMP device_add instead of
first going through QemuOpts and converting back to QDict.
Note that this changes the behaviour of device_add, though in ways that
should be considered bug fixes:
QemuOpts ignores differences between data types, so you could
successfully pass a string "123" for an integer property, or a string
"on" for a boolean property (and vice versa). After this change, the
correct data type for the property must be used in the JSON input.
qemu_opts_from_qdict() also silently ignores any options whose value is
a QDict, QList or QNull.
To illustrate, the following QMP command was accepted before and is now
rejected for both reasons:
{ "execute": "device_add",
"arguments": { "driver": "scsi-cd",
"drive": { "completely": "invalid" },
"physical_block_size": "4096" } }
Signed-off-by: Kevin Wolf
---
softmmu/qdev-monitor.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index c09b7430eb..8622ccade6 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
qdev_print_devinfos(true);
}
-void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
+static void monitor_device_add(QDict *qdict, QObject **ret_data,
+ bool from_json, Error **errp)
{
QemuOpts *opts;
DeviceState *dev;
@@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp)
qemu_opts_del(opts);
return;
}
-dev = qdev_device_add(opts, errp);
+qemu_opts_del(opts);
+
+dev = qdev_device_add_from_qdict(qdict, from_json, errp);
Hi Kevin,
I'm wandering if deleting the opts (which remove it from the "device" opts
list) is really a no-op ?
It's not exactly a no-op. Previously, the QemuOpts would only be freed
when the device is destroying, now we delete it immediately after
creating the device. This could matter in some cases.
The one case I was aware of is that QemuOpts used to be responsible for
checking for duplicate IDs. Obviously, it can't do this job any more
when we call qemu_opts_del() right after creating the device. This is
the reason for patch 6.
The opts list is, eg, traversed in hw/net/virtio-net.c in the function
failover_find_primary_device_id() which may be called during the
virtio_net_set_features() (a TYPE_VIRTIO_NET method).
I do not have the knowledge to tell when this method is called. But If this
is after we create the devices. Then the list will be empty at this point
now.
It seems, there are 2 other calling sites of
"qemu_opts_foreach(qemu_find_opts("device"), [...]" in net/vhost-user.c and
net/vhost-vdpa.c
Yes, you are right. These callers probably need to be changed. Going
through the command line options rather than looking at the actual
device objects that exist doesn't feel entirely clean anyway.
So I tried to have a look at the virtio-net case, and ended up very
confused.
Obviously looking at command line options (even of a differrent device)
from within a device is very unclean. With a non-broken, i.e. type safe,
device-add (as well as with the JSON CLI option introduced by this
series), we can't have a QemuOpts any more that is by definition unsafe.
So this code needs a replacement.
My naive idea was that we just need to look at runtime state instead.
Don't search the options for a device with a matching 'failover_pair_id'
(which, by the way, would fail as soon as any other device introduces a
property with the same name), but search for actual PCIDevices in qdev
that have pci_dev->failover_pair_id set accordingly.
However, the logic in failover_add_primary() suggests that we can have a
state where QemuOpts for a device exist, but the device doesn't, and
then it hotplugs the device from the command line options. How would we
ever get into such an inconsistent state where QemuOpts contains a
device that doesn't exist? Normally devices get their QemuOpts when they
are created and device_finalize() deletes the QemuOpts again. >
Just read the following from docs/system/virtio-net-failover.rst
> Usage
> -
>
> The primary device can be hotplugged or be part of the startup
> configuration
>
> -device virtio-net-pci,netdev=hostnet1,id=net1,
> mac=52:54:00:6f:55:cc,bus=root2,failover=on
>
> With the parameter failover=on the VIRTIO_NET_F_STANDBY feature
> will be enabled.
>
> -device vfio-pci,host=5e:00.2,id=hostdev
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
On 10/1/21 16:42, Peter Krempa wrote: On Fri, Sep 24, 2021 at 11:04:25 +0200, Kevin Wolf wrote: Directly call qdev_device_add_from_qdict() for QMP device_add instead of first going through QemuOpts and converting back to QDict. Note that this changes the behaviour of device_add, though in ways that should be considered bug fixes: QemuOpts ignores differences between data types, so you could successfully pass a string "123" for an integer property, or a string "on" for a boolean property (and vice versa). After this change, the correct data type for the property must be used in the JSON input. qemu_opts_from_qdict() also silently ignores any options whose value is a QDict, QList or QNull. Sorry for chiming in a bit late, but preferrably this commit should be postponed to at least the next release so that we decrease the amount of libvirt users broken by this. Granted users are supposed to use new libvirt with new qemu but that's not always the case. Anyways, libvirt is currently mangling all the types to strings with device_add. I'm currently working on fixing it and it will hopefully be done until next-month's release, but preferrably we increase the window of working combinations by postponing this until the next release. Switching to qdict is really an improvement I think. If we choose to keep legacy behavior working for now, I think we should find a way to still do this switch. Maybe we can temporarily keep the str_to_int and str_to_bool conversion when converting the qdict to the qdev properties afterward ? Damien
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
On 9/24/21 11:04, Kevin Wolf wrote:
Directly call qdev_device_add_from_qdict() for QMP device_add instead of
first going through QemuOpts and converting back to QDict.
Note that this changes the behaviour of device_add, though in ways that
should be considered bug fixes:
QemuOpts ignores differences between data types, so you could
successfully pass a string "123" for an integer property, or a string
"on" for a boolean property (and vice versa). After this change, the
correct data type for the property must be used in the JSON input.
qemu_opts_from_qdict() also silently ignores any options whose value is
a QDict, QList or QNull.
To illustrate, the following QMP command was accepted before and is now
rejected for both reasons:
{ "execute": "device_add",
"arguments": { "driver": "scsi-cd",
"drive": { "completely": "invalid" },
"physical_block_size": "4096" } }
Signed-off-by: Kevin Wolf
---
softmmu/qdev-monitor.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index c09b7430eb..8622ccade6 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
qdev_print_devinfos(true);
}
-void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
+static void monitor_device_add(QDict *qdict, QObject **ret_data,
+ bool from_json, Error **errp)
{
QemuOpts *opts;
DeviceState *dev;
@@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp)
qemu_opts_del(opts);
return;
}
-dev = qdev_device_add(opts, errp);
+qemu_opts_del(opts);
+
+dev = qdev_device_add_from_qdict(qdict, from_json, errp);
Hi Kevin,
I'm wandering if deleting the opts (which remove it from the "device"
opts list) is really a no-op ?
The opts list is, eg, traversed in hw/net/virtio-net.c in the function
failover_find_primary_device_id() which may be called during the
virtio_net_set_features() (a TYPE_VIRTIO_NET method).
I do not have the knowledge to tell when this method is called. But If
this is after we create the devices. Then the list will be empty at this
point now.
It seems, there are 2 other calling sites of
"qemu_opts_foreach(qemu_find_opts("device"), [...]" in net/vhost-user.c
and net/vhost-vdpa.c
--
Damien
Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()
Hi Kevin,
I proposed a very similar patch in our rfc series because we needed some
of the cleaning you do here.
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05679.html
I've added a bit of doc for the function, feel free to take it if you want.
On 9/24/21 16:09, Vladimir Sementsov-Ogievskiy wrote:
24.09.2021 12:04, Kevin Wolf wrote:
object_property_add_child() fails (with _abort) if an object with
the same name already exists. As long as QemuOpts is in use for -device
and device_add, it catches duplicate IDs before qdev_set_id() is even
called. However, for enabling non-QemuOpts code paths, we need to make
sure that the condition doesn't cause a crash, but fails gracefully.
Signed-off-by: Kevin Wolf
---
include/monitor/qdev.h | 2 +-
hw/xen/xen-legacy-backend.c | 3 ++-
softmmu/qdev-monitor.c | 16 ++--
3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..7961308c75 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
Error **errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, char *id);
+void qdev_set_id(DeviceState *dev, char *id, Error **errp);
#endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index dd8ae1452d..17aca85ddc 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice
*xen_be_get_xendev(const char *type, int dom,
xendev = g_malloc0(ops->size);
object_initialize(>qdev, ops->size, TYPE_XENBACKEND);
OBJECT(xendev)->free = g_free;
- qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type,
dev));
+ qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+ _abort);
qdev_realize(DEVICE(xendev), xen_sysbus, _fatal);
object_unref(OBJECT(xendev));
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1207e57a46..c2af906df0 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path,
Error **errp)
}
/* Takes ownership of @id, will be freed when deleting the device */
-void qdev_set_id(DeviceState *dev, char *id)
+void qdev_set_id(DeviceState *dev, char *id, Error **errp)
According to recommendations in error.h, worth adding also return value
(for example true=success false=failure), so [..]
{
if (id) {
dev->id = id;
}
Unrelated but.. What's the strange logic?
Is it intended that with passed id=NULL we don't update dev->id variable
but try to do following logic with old dev->id?
dev->id is expected to be NULL. The object is created just before
calling this function so it is always the case. We could probably assert
this.
if (dev->id) {
- object_property_add_child(qdev_get_peripheral(), dev->id,
- OBJECT(dev));
+ object_property_try_add_child(qdev_get_peripheral(), dev->id,
+ OBJECT(dev), errp);
} else {
static int anon_count;
gchar *name = g_strdup_printf("device[%d]", anon_count++);
- object_property_add_child(qdev_get_peripheral_anon(), name,
- OBJECT(dev));
+ object_property_try_add_child(qdev_get_peripheral_anon(), name,
+ OBJECT(dev), errp);
g_free(name);
}
}
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
{
+ ERRP_GUARD();
DeviceClass *dc;
const char *driver, *path;
DeviceState *dev = NULL;
@@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts,
Error **errp)
}
}
- qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+ qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp);
+ if (*errp) {
+ goto err_del_dev;
+ }
[..] here we'll have
if (!qdev_set_id(...)) {
goto err_del_dev;
}
and no need for ERRP_GUARD.
/* set properties */
if (qemu_opt_foreach(opts, set_property, dev, errp)) {
[Qemu-block] [PATCH v4 01/10] add device_legacy_reset function to prepare for reset api change
Provide a temporary device_legacy_reset function doing what
device_reset does to prepare for the transition with Resettable
API.
All occurrence of device_reset in the code tree are also replaced
by device_legacy_reset.
The new resettable API has different prototype and semantics
(resetting child buses as well as the specified device). Subsequent
commits will make the changeover for each call site individually; once
that is complete device_legacy_reset() will be removed.
Signed-off-by: Damien Hedde
Reviewed-by: Peter Maydell
---
Cc: Gerd Hoffmann
Cc: Paolo Bonzini
Cc: "Daniel P. Berrangé"
Cc: Eduardo Habkost
Cc: Richard Henderson
Cc: "Michael S. Tsirkin"
Cc: Marcel Apfelbaum
Cc: John Snow
Cc: "Cédric Le Goater"
Cc: Collin Walling
Cc: Cornelia Huck
Cc: David Hildenbrand
Cc: Halil Pasic
Cc: Christian Borntraeger
Cc: Dmitry Fleytman
Cc: Fam Zheng
Cc: qemu-block@nongnu.org
Cc: qemu-...@nongnu.org
Cc: qemu-s3...@nongnu.org
Cc: qemu-...@nongnu.org
---
hw/audio/intel-hda.c | 2 +-
hw/core/qdev.c | 6 +++---
hw/hyperv/hyperv.c | 2 +-
hw/i386/pc.c | 2 +-
hw/ide/microdrive.c | 8
hw/intc/spapr_xive.c | 2 +-
hw/ppc/pnv_psi.c | 2 +-
hw/ppc/spapr_pci.c | 2 +-
hw/ppc/spapr_vio.c | 2 +-
hw/s390x/s390-pci-inst.c | 2 +-
hw/scsi/vmw_pvscsi.c | 2 +-
hw/sd/omap_mmc.c | 2 +-
hw/sd/pl181.c| 2 +-
include/hw/qdev-core.h | 4 ++--
14 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 6ecd383540..27b71c57cf 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1087,7 +1087,7 @@ static void intel_hda_reset(DeviceState *dev)
QTAILQ_FOREACH(kid, >codecs.qbus.children, sibling) {
DeviceState *qdev = kid->child;
cdev = HDA_CODEC_DEVICE(qdev);
-device_reset(DEVICE(cdev));
+device_legacy_reset(DEVICE(cdev));
d->state_sts |= (1 << cdev->cad);
}
intel_hda_update_irq(d);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 60d66c2f39..a95d4fa87d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -257,7 +257,7 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
static int qdev_reset_one(DeviceState *dev, void *opaque)
{
-device_reset(dev);
+device_legacy_reset(dev);
return 0;
}
@@ -865,7 +865,7 @@ static void device_set_realized(Object *obj, bool value,
Error **errp)
}
}
if (dev->hotplugged) {
-device_reset(dev);
+device_legacy_reset(dev);
}
dev->pending_deleted_event = false;
@@ -1087,7 +1087,7 @@ void device_class_set_parent_unrealize(DeviceClass *dc,
dc->unrealize = dev_unrealize;
}
-void device_reset(DeviceState *dev)
+void device_legacy_reset(DeviceState *dev)
{
DeviceClass *klass = DEVICE_GET_CLASS(dev);
diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index 6ebf31c310..cd9db3cb5c 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -140,7 +140,7 @@ void hyperv_synic_reset(CPUState *cs)
SynICState *synic = get_synic(cs);
if (synic) {
-device_reset(DEVICE(synic));
+device_legacy_reset(DEVICE(synic));
}
}
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3ab4bcb3ca..f33a8de42f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2826,7 +2826,7 @@ static void pc_machine_reset(MachineState *machine)
cpu = X86_CPU(cs);
if (cpu->apic_state) {
-device_reset(cpu->apic_state);
+device_legacy_reset(cpu->apic_state);
}
}
}
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index b0272ea14b..6b30e36ed8 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -173,7 +173,7 @@ static void md_attr_write(PCMCIACardState *card, uint32_t
at, uint8_t value)
case 0x00: /* Configuration Option Register */
s->opt = value & 0xcf;
if (value & OPT_SRESET) {
-device_reset(DEVICE(s));
+device_legacy_reset(DEVICE(s));
}
md_interrupt_update(s);
break;
@@ -316,7 +316,7 @@ static void md_common_write(PCMCIACardState *card, uint32_t
at, uint16_t value)
case 0xe: /* Device Control */
s->ctrl = value;
if (value & CTRL_SRST) {
-device_reset(DEVICE(s));
+device_legacy_reset(DEVICE(s));
}
md_interrupt_update(s);
break;
@@ -541,7 +541,7 @@ static int dscm1_attach(PCMCIACardState *card)
md->attr_base = pcc->cis[0x74] | (pcc->cis[0x76] << 8);
md->io_base = 0x0;
-device_reset(DEVICE(md));
+device_legacy_reset(DEVICE(md));
md_interrupt_update(md);
return 0;
@@ -551,7 +551,7 @@ static int dscm1_detach(PCMCIACardState *card)
{
MicroDriveState *md = MICRODRIVE(card);
-device_reset(DEVICE(md));
Re: [Qemu-block] [PATCH v3 04/33] make Device and Bus Resettable
On 8/7/19 4:41 PM, Peter Maydell wrote: > On Mon, 29 Jul 2019 at 15:58, Damien Hedde wrote: >> >> >> +/** >> + * device_reset: >> + * Resets the device @dev, @cold tell whether to do a cold or warm reset. >> + * Base behavior is to reset the device and its qdev/qbus subtree. > > What do you mean by "base behavior" here? When would this ever > do anything else? > Oops, just noticed I missed this comment. Since I had to use a method "foreach_child" to call reset on children, the behavior depends on it. Default implementation in base classes follows the qdev tree. But a specialization can change that. That's more a side-effect than a wanted feature.
Re: [Qemu-block] [PATCH v3 07/33] automatically add vmstate for reset support in devices
On 8/9/19 12:32 PM, Peter Maydell wrote: > On Fri, 9 Aug 2019 at 11:29, Damien Hedde wrote: >> >> One way to keep the feature without copy-pasting vmsd would be to add >> a new vmstate_register with an additional argument to pass the base >> class vmsd section and handle the whole thing there. > > If we have a vmstate section which contains no actual data, > only subsections with 'needed' functions, is it migration > compatible with previous versions in the same way that > tacking a subsection onto an existing function is? I don't think so because of the naming schema. I had to forge the correct name for the reset subsection for every device. Each subsection must be named after its parent section plus '/something'. -- Damien
Re: [Qemu-block] [PATCH v3 07/33] automatically add vmstate for reset support in devices
On 8/9/19 12:07 PM, Peter Maydell wrote: > On Thu, 8 Aug 2019 at 16:42, Dr. David Alan Gilbert > wrote: >> >> * Peter Maydell (peter.mayd...@linaro.org) wrote: >>> On Mon, 29 Jul 2019 at 15:59, Damien Hedde >>> wrote: >>>> >>>> This add the reset related sections for every QOM >>>> device. >>> >>> A bit more detail in the commit message would help, I think -- >>> this is adding extra machinery which has to copy and modify >>> the VMStateDescription passed in by the device in order to >>> add the subsection that handles reset. >>> >>> I've added Dave Gilbert to the already long cc list since this >>> is migration related. >> >> I don't like dynamically modifying all the vmsds. > > Yeah, I'm not a huge fan of it either, but it does mean that > the state gets migrated and we don't have a pile of really > easy to miss bugs where we forgot to say "this device needs to > migrate the reset state" and it means we don't have every > device we ever write in the future needing to say "this device > needs to migrate reset state"... > >> Aren't you going to have to understand each devices reset behaviour >> and make sure it does something sane? e.g. it might have a postload >> that registers a timer or something that you wouldn't want to do if it's >> in reset. >> >> The easiest way is to write a macro that you can easily add to devices >> you've checked subsection list (like the way we have a >> VMSTATE_USB_DEVICE). > > One problem is that as soon as somebody writes a USB controller > or PCI controller model that can be held in reset, every > device that could sat behind it automatically now could find > that it's migrated while it's in reset. > One way to keep the feature without copy-pasting vmsd would be to add a new vmstate_register with an additional argument to pass the base class vmsd section and handle the whole thing there. -- Damien
Re: [Qemu-block] [Qemu-devel] [PATCH v3 12/33] hw/pci/: remove qdev/qbus_reset_all call
On 8/7/19 5:31 PM, Peter Maydell wrote: > On Mon, 29 Jul 2019 at 15:59, Damien Hedde wrote: >> >> Replace deprecated qdev/bus_reset_all by device/bus_reset_warm. >> >> This does not impact the behavior. >> >> Signed-off-by: Damien Hedde > > I'll come back to patches 12-28 later. They're all ok > in principle, we just need to check that in each individual > case: > * we've made the right choice of cold vs warm reset > * we're ok to switch to 'reset including children' from >the legacy 'reset not including children' semantics I'm working on a patch reroll to fix what's been reviewed so far. Should I put aside the patches 12-28 for now ? They could be part of 1 or 2 separate following series or I can re-add them later on when we agree on the api. -- Damien
Re: [Qemu-block] [Qemu-devel] [PATCH v3 02/33] add temporary device_legacy_reset function to replace device_reset
On 8/7/19 4:27 PM, Peter Maydell wrote: > On Mon, 29 Jul 2019 at 15:58, Damien Hedde wrote: >> >> Provide a temporary function doing what device_reset does to do the >> transition with Resettable API which will trigger a prototype change >> of device_reset. > > The other point here is that device_legacy_reset() resets > only that device, not any of its qbus children, right? > So the new function which we eventually replace the callsites > with also has different semantics, which is why we do the > changes one by one in patches 10-28. Yes, for device_reset there is a change of scope. > > So you could add: > > The new resettable API function also has different semantics > (resetting child buses as well as the specified device). > Subsequent commits will make the changeover for each callsite > individually; once that is complete device_legacy_reset() will be > removed. sure > >> Signed-off-by: Damien Hedde > > I agree with David that patch 3 could be squashed into this one. ok > > If you do that and tweak the commit message you can have > Reviewed-by: Peter Maydell > > thanks > -- PMM >
Re: [Qemu-block] [PATCH v3 08/33] Add function to control reset with gpio inputs
On 8/9/19 7:51 AM, David Gibson wrote: > On Wed, Aug 07, 2019 at 11:37:51AM +0100, Peter Maydell wrote: >> On Wed, 31 Jul 2019 at 07:33, David Gibson >> wrote: >>> >>> On Mon, Jul 29, 2019 at 04:56:29PM +0200, Damien Hedde wrote: >>>> It adds the possibility to add 2 gpios to control the warm and cold reset. >>>> With theses ios, the reset can be maintained during some time. >>>> Each io is associated with a state to detect level changes. >>>> >>>> Vmstate subsections are also added to the existsing device_reset >>>> subsection. >>> >>> This doesn't seem like a thing that should be present on every single >>> DeviceState. >> >> It's a facility that's going to be useful to multiple different >> subclasses of DeviceState, so it seems to me cleaner to >> have base class support for the common feature rather than >> to reimplement it entirely from scratch in every subclass >> that wants it. > > Hm, I suppose so. Would it really have to be from scratch, though? > Couldn't some suitable helper functions make adding such GPIOs to a > device pretty straightforward? > This patch does that. A device does have to use the helper to add the gpio. Either qdev_init_warm_reset_gpio(...) or qdev_init_cold_reset_gpio(...) , like any another gpio. The mechanics to control the reset with gpio change is done in the base class and there is some state pre-allocated (and associated vmstate description) to it. If that's a problem I can only provide helpers and let devices handle state allocation and vmstate addition. Damien
Re: [Qemu-block] [PATCH v3 14/33] hw/s390x/s390-virtio-ccw.c: remove qdev_reset_all call
On 8/8/19 12:50 PM, Cornelia Huck wrote: > On Mon, 29 Jul 2019 16:56:35 +0200 > Damien Hedde wrote: > >> Replace deprecated qdev_reset_all by device_reset_warm. >> >> This does not impact the behavior. > > Not so sure about that; see below. In this case, qdev_reset_all is used. The qdev subtree is already reset. device_reset_* does keep the same children-then-parent call order for the legacy reset method. This is why the behavior is unchanged. I will add this point in the commit message of this patch (and also in other qdev/qbus_reset_all replacement patches) so it will be more clear. -- Damien
Re: [Qemu-block] [PATCH v3 07/33] automatically add vmstate for reset support in devices
On 8/7/19 5:07 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:59, Damien Hedde wrote:
>>
>> This add the reset related sections for every QOM
>> device.
>
> A bit more detail in the commit message would help, I think --
> this is adding extra machinery which has to copy and modify
> the VMStateDescription passed in by the device in order to
> add the subsection that handles reset.
Sorry for that, thought I've added some...
I've kept this patch separate from previous one because this it is
awkward. I'm not sure this is the right place (I mean in qdev files) do
this kind of stuff. It basically replaces every static vmsd by dynamic
ones, so it makes it harder to follow when debugging since there is no
symbol associated to them. But on the other hand, I don't see an
alternative.
I copy there what I've put in the cover-letter:
For devices, I've added a patch to automate the addition of reset
related subsection. In consequence it is not necessary to explicitly add
the reset subsection in every device specialization requiring it.
Right know this is kind of a hack into qdev to dynamically modify the
vmsd before the registration. There is probably a much cleaner way to do
this but I prefered to demonstrate it by keeping modification local to qdev.
As far as I can tell it's ok to dynamically add subsections at the end.
This does not prevent from further adding subsections in the orignal
vmsd: the subsections order in the array is irrelevant from migration
point-of-view. The loading part just lookup each subsection in the array
by name uppon reception.
>
> I've added Dave Gilbert to the already long cc list since this
> is migration related.
>
>> Signed-off-by: Damien Hedde
>> ---
>> hw/core/qdev-vmstate.c | 41 +
>> hw/core/qdev.c | 12 +++-
>> include/hw/qdev-core.h | 3 +++
>> stubs/Makefile.objs| 1 +
>> stubs/device.c | 7 +++
>> 5 files changed, 63 insertions(+), 1 deletion(-)
>> create mode 100644 stubs/device.c
>>
>> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
>> index 07b010811f..24f8465c61 100644
>> --- a/hw/core/qdev-vmstate.c
>> +++ b/hw/core/qdev-vmstate.c
>> @@ -43,3 +43,44 @@ const struct VMStateDescription device_vmstate_reset = {
>> VMSTATE_END_OF_LIST()
>> },
>> };
>> +
>> +static VMStateDescription *vmsd_duplicate_and_append(
>> +const VMStateDescription *old_vmsd,
>> +const VMStateDescription *new_subsection)
>> +{
>> +VMStateDescription *vmsd;
>> +int n = 0;
>> +
>> +assert(old_vmsd && new_subsection);
>> +
>> +vmsd = (VMStateDescription *) g_memdup(old_vmsd, sizeof(*vmsd));
>> +
>> +if (old_vmsd->subsections) {
>> +while (old_vmsd->subsections[n]) {
>> +n += 1;
>> +}
>> +}
>> +vmsd->subsections = g_new(const VMStateDescription *, n + 2);
>> +
>> +if (old_vmsd->subsections) {
>> +memcpy(vmsd->subsections, old_vmsd->subsections,
>> + sizeof(VMStateDescription *) * n);
>> +}
>> +vmsd->subsections[n] = new_subsection;
>> +vmsd->subsections[n + 1] = NULL;
>> +
>> +return vmsd;
>> +}
>> +
>> +void device_class_build_extended_vmsd(DeviceClass *dc)
>> +{
>> +assert(dc->vmsd);
>> +assert(!dc->vmsd_ext);
>> +
>> +/* forge a subsection with proper name */
>> +VMStateDescription *reset;
>> +reset = g_memdup(_vmstate_reset, sizeof(*reset));
>> +reset->name = g_strdup_printf("%s/device_reset", dc->vmsd->name);
>> +
>> +dc->vmsd_ext = vmsd_duplicate_and_append(dc->vmsd, reset);
>> +}
>
> This will allocate memory, but there is no corresponding
> code which frees it. This means you'll have a memory leak
> across device realize->unrealize for hotplug devices.
Right. I'll handle this along with the existing vmsd_unregister
in realize/unrealize method.
>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index e9e5f2d5f9..88387d3743 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -45,7 +45,17 @@ bool qdev_hot_removed = false;
>> const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
>> {
>> DeviceClass *dc = DEVICE_GET_CLASS(dev);
>> -return dc->vmsd;
>> +
>> +if (!dc->vmsd) {
>> +return NULL;
>> +}
>> +
>> +if (!dc->vmsd_ext) {
>> +/* build it first time we need it */
>> +device_class_build_e
Re: [Qemu-block] [PATCH v3 08/33] Add function to control reset with gpio inputs
On 8/7/19 5:18 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:59, Damien Hedde wrote:
>>
>> It adds the possibility to add 2 gpios to control the warm and cold reset.
>> With theses ios, the reset can be maintained during some time.
>> Each io is associated with a state to detect level changes.
>>
>> Vmstate subsections are also added to the existsing device_reset
>> subsection.
>>
>> Signed-off-by: Damien Hedde
>> ---
>> hw/core/qdev-vmstate.c | 15 ++
>> hw/core/qdev.c | 65 ++
>> include/hw/qdev-core.h | 57
>> 3 files changed, 137 insertions(+)
>>
>> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
>> index 24f8465c61..72f84c6cee 100644
>> --- a/hw/core/qdev-vmstate.c
>> +++ b/hw/core/qdev-vmstate.c
>> @@ -24,10 +24,23 @@ static int device_vmstate_reset_post_load(void *opaque,
>> int version_id)
>> {
>> DeviceState *dev = (DeviceState *) opaque;
>> BusState *bus;
>> +uint32_t io_count = 0;
>> +
>> QLIST_FOREACH(bus, >child_bus, sibling) {
>> bus->resetting = dev->resetting;
>> bus->reset_is_cold = dev->reset_is_cold;
>> }
>> +
>> +if (dev->cold_reset_input.state) {
>> +io_count += 1;
>> +}
>> +if (dev->warm_reset_input.state) {
>> +io_count += 1;
>> +}
>> +/* ensure resetting count is coherent with io states */
>> +if (dev->resetting < io_count) {
>> +return -1;
>> +}
>> return 0;
>> }
>>
>> @@ -40,6 +53,8 @@ const struct VMStateDescription device_vmstate_reset = {
>> .fields = (VMStateField[]) {
>> VMSTATE_UINT32(resetting, DeviceState),
>> VMSTATE_BOOL(reset_is_cold, DeviceState),
>> +VMSTATE_BOOL(cold_reset_input.state, DeviceState),
>> +VMSTATE_BOOL(warm_reset_input.state, DeviceState),
>
> If we're just adding these fields to this VMStateDescription
> then this patch should come earlier in the series than the
> patch where we create and start using the fields. Otherwise
> there's a migration compat break between a QEMU just
> before this patch and a QEMU with it. I think the simplest
> fix is to put this patch before patches 6/7 and have a note
> in the commit message that this functionality can't be used
> until after the patch which adds the migration support.
Independently of the compat break you mention, maybe it is better to
have 'conditional' subsection for each input ?
>
>> VMSTATE_END_OF_LIST()
>> },
>> };
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 88387d3743..11a4de55ea 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -450,6 +450,67 @@ void qdev_init_gpio_in(DeviceState *dev,
>> qemu_irq_handler handler, int n)
>> qdev_init_gpio_in_named(dev, handler, NULL, n);
>> }
>>
>> +static DeviceResetInputState *device_get_reset_input_state(DeviceState *dev,
>> +bool cold)
>> +{
>> +return cold ? >cold_reset_input : >warm_reset_input;
>> +}
>> +
>> +static void device_reset_handler(DeviceState *dev, bool cold, bool level)
>> +{
>> +DeviceResetInputState *dris = device_get_reset_input_state(dev, cold);
>> +
>> +if (dris->type == DEVICE_RESET_ACTIVE_LOW) {
>> +level = !level;
>> +}
>> +
>> +if (dris->state == level) {
>> +/* io state has not changed */
>> +return;
>> +}
>> +
>> +dris->state = level;
>> +
>> +if (level) {
>> +resettable_assert_reset(OBJECT(dev), cold);
>> +} else {
>> +resettable_deassert_reset(OBJECT(dev));
>> +}
>> +}
>> +
>> +static void device_cold_reset_handler(void *opaque, int n, int level)
>> +{
>> +device_reset_handler((DeviceState *) opaque, true, level);
>> +}
>> +
>> +static void device_warm_reset_handler(void *opaque, int n, int level)
>> +{
>> +device_reset_handler((DeviceState *) opaque, false, level);
>> +}
>> +
>> +void qdev_init_reset_gpio_in_named(DeviceState *dev, const char *name,
>> + bool cold, DeviceResetActiveType type)
>> +{
>> +DeviceResetInputState *dris = device_get_reset_input_state(dev, cold);
>> +qemu_irq_handler handler;
>> +
>> +switch (type) {
>> +case DEVICE_RESET_ACTIVE_LOW:
>> +case DEVICE_RESET_ACTIVE_HIGH:
>> +break;
>> +default:
>> +assert(false);
>> +break;
>
> The usual way to write this is
> g_assert_not_reached();
> (and no following 'break').
>
>
> But the whole switch statement seems to be a complicated way
> of writing
>assert(type == DEVICE_RESET_ACTIVE_LOW || type ==
> DEVICE_RESET_ACTIVE_HIGH);
>
>> +}
>> +
>> +assert(!dris->exists);
>> +dris->exists = true;
>> +dris->type = type;
>> +
>> +handler = cold ? device_cold_reset_handler : device_warm_reset_handler;
>> +qdev_init_gpio_in_named(dev, handler, name, 1);
>> +}
>
> thanks
> -- PMM
>
Re: [Qemu-block] [PATCH v3 06/33] add the vmstate description for device reset state
On 8/7/19 4:54 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:58, Damien Hedde wrote:
>>
>> It contains the resetting counter and cold flag status.
>>
>> At this point, migration of bus reset related state (counter and cold/warm
>> flag) is handled by parent device. This done using the post_load
>> function in the vmsd subsection.
>>
>> This is last point allow to add an initial support of migration with part of
>> qdev/qbus tree in reset state under the following condition:
>> + time-lasting reset are asserted on Device only
>>
>> Note that if this condition is not respected, migration will succeed and
>> no failure will occurs. The only impact is that the resetting counter
>> of a bus may lower afer a migration.
>>
>> Signed-off-by: Damien Hedde
>
>
>> +const struct VMStateDescription device_vmstate_reset = {
>> +.name = "device_reset",
>> +.version_id = 0,
>> +.minimum_version_id = 0,
>> +.needed = device_vmstate_reset_needed,
>> +.post_load = device_vmstate_reset_post_load,
>> +.fields = (VMStateField[]) {
>> +VMSTATE_UINT32(resetting, DeviceState),
>> +VMSTATE_BOOL(reset_is_cold, DeviceState),
>> +VMSTATE_END_OF_LIST()
>> +},
>> +};
>> -
>
> Forgot to ask -- why don't we migrate the hold_needed flags?
The flag is only used to keep the info between executing the init
and hold phases. We can't interrupt the code in between since this
mess is during resettable_assert_reset method which is atomic.
I can add a comment to explain that.
>
> thanks
> -- PMM
>
Re: [Qemu-block] [PATCH v3 04/33] make Device and Bus Resettable
On 8/7/19 4:41 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:58, Damien Hedde wrote:
>>
>> This add Resettable interface implementation for both Bus and Device.
>>
>> *resetting* counter and *reset_is_cold* flag are added in DeviceState
>> and BusState.
>>
>> Compatibility with existing code base is ensured.
>> The legacy bus or device reset method is called in the new exit phase
>> and the other 2 phases are let empty. Using the exit phase guarantee that
>
> "left". "guarantees"
>
>> legacy resets are called in the "post" order (ie: children then parent)
>> in hierarchical reset. That is the same order as legacy qdev_reset_all
>> or qbus_reset_all were using.
>
> This is true, but on the other hand the semantics of most device
> reset methods match "init", not "exit" -- they just set device
> internal fields to the correct reset state.
I changed from "init" to "exit" due to the change of the init phase call
order to parent-then-children.
This is a consequence of what I found about the raspi reset: it changes
the reset hierarchy during reset. The only way I saw to have a chance
allowing this kind of things cleanly is: do parent init first so that it
can setup its children before they are considered for reset.
I can put the legacy reset method to the hold phase which is part of the
"enter reset state". Otherwise I need to change back the order of init
phase.
My other concern with putting it in init phase is that some device do
things we forbid in it (like setting irq).
>
>> New *device_reset* and *bus_reset* function are proposed with an
>> additional boolean argument telling whether the reset is cold or warm.
>> Helper functions *device_reset_[warm|cold]* and *bus_reset_[warm|cold]*
>> are defined also as helpers.
>>
>> Also add a [device|bus]_is_resetting and [device|bus]_is_reset_cold
>> functions telling respectively whether the object is currently under reset
>> and
>> if the current reset is cold or not.
>
> I was expecting this patch to contain handling for migration
> of the new data fields. That's in a later patch, so the
> commit message here should say something like:
>
> "This commit adds new fields to BusState and DeviceState, but
> does not set up migration handling for them; so the new functions
> can only be called after the subsequent commit which adds the
> migration support."
>
>> Signed-off-by: Damien Hedde
>> ---
>> hw/core/bus.c | 85 ++
>> hw/core/qdev.c | 82
>> include/hw/qdev-core.h | 84 ++---
>> tests/Makefile.include | 1 +
>> 4 files changed, 247 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/core/bus.c b/hw/core/bus.c
>> index 17bc1edcde..08a97addb6 100644
>> --- a/hw/core/bus.c
>> +++ b/hw/core/bus.c
>> @@ -22,6 +22,7 @@
>> #include "qemu/module.h"
>> #include "hw/qdev.h"
>> #include "qapi/error.h"
>> +#include "hw/resettable.h"
>>
>> void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp)
>> {
>> @@ -68,6 +69,75 @@ int qbus_walk_children(BusState *bus,
>> return 0;
>> }
>>
>> +void bus_reset(BusState *bus, bool cold)
>> +{
>> +resettable_reset(OBJECT(bus), cold);
>> +}
>> +
>> +bool bus_is_resetting(BusState *bus)
>> +{
>> +return (bus->resetting != 0);
>> +}
>> +
>> +bool bus_is_reset_cold(BusState *bus)
>> +{
>> +return bus->reset_is_cold;
>> +}
>> +
>> +static uint32_t bus_get_reset_count(Object *obj)
>> +{
>> +BusState *bus = BUS(obj);
>> +return bus->resetting;
>> +}
>> +
>> +static uint32_t bus_increment_reset_count(Object *obj)
>> +{
>> +BusState *bus = BUS(obj);
>> +return ++bus->resetting;
>> +}
>> +
>> +static uint32_t bus_decrement_reset_count(Object *obj)
>> +{
>> +BusState *bus = BUS(obj);
>> +return --bus->resetting;
>> +}
>> +
>> +static bool bus_set_reset_cold(Object *obj, bool cold)
>> +{
>> +BusState *bus = BUS(obj);
>> +bool old = bus->reset_is_cold;
>> +bus->reset_is_cold = cold;
>> +return old;
>> +}
>> +
>> +static bool bus_set_hold_needed(Object *obj, bool hold_needed)
>> +{
>> +BusState *bus = BUS(obj);
>> +bool old = bus->reset_hold_needed;
>>
Re: [Qemu-block] [PATCH v3 01/33] Create Resettable QOM interface
On 8/7/19 4:20 PM, Peter Maydell wrote: > On Mon, 29 Jul 2019 at 15:58, Damien Hedde wrote: >> >> This commit defines an interface allowing multi-phase reset. >> The phases are INIT, HOLD and EXIT. Each phase has an associated method >> in the class. >> >> The reset of a Resettable is controlled with 2 functions: >> - resettable_assert_reset which starts the reset operation. >> - resettable_deassert_reset which ends the reset operation. >> >> There is also a `resettable_reset` helper function which does assert then >> deassert allowing to do a proper reset in one call. >> >> In addition, two functions, *resettable_reset_cold_fn* and >> *resettable_reset_warm_fn*, are defined. They take only an opaque argument >> (for the Resettable object) and can be used as callbacks. >> >> The Resettable interface is "reentrant", _assert_ can be called several >> times and _deasert_ must be called the same number of times so that the > > deassert > >> Resettable leave reset state. It is supported by keeping a counter of >> the current number of _assert_ calls. The counter maintainance is done >> though 3 methods get/increment/decrement_count. A method set_cold is used >> to set the cold flag of the reset. Another method set_host_needed is used >> to ensure hold phase is executed only if required. >> >> Reset hierarchy is also supported. Each Resettable may have >> sub-Resettable objects. When resetting a Resettable, it is propagated to >> its children using the foreach_child method. >> >> When entering reset, init phases are executed parent-to-child order then >> hold phases are executed child-parent order. > > Why do we execute the hold phase in child-to-parent order? I would > have expected this to follow the same order as the init phase. No particular reason, I just thought it was better that way. But I don't have a use case where it matters. If we do only an assert_reset, then top-level phases are called first and last: parent's init child A's init child_B's init child_A's hold child_B's hold parent's hold I can switch it. > >> When leaving reset, exit phases are executed in child-to-parent order. >> This will allow to replace current qdev_reset mechanism by this interface >> without side-effects on reset order. > > It makes sense that the exit phase is child-to-parent. > >> Note: I used an uint32 for the count. This match the type already used >> in the existing resetting counter in hw/scsi/vmw_pvscsi.c for the >> PVSCSIState. >> >> Signed-off-by: Damien Hedde >> --- >> Makefile.objs | 1 + >> hw/core/Makefile.objs | 1 + >> hw/core/resettable.c| 220 >> hw/core/trace-events| 39 +++ >> include/hw/resettable.h | 126 +++ >> 5 files changed, 387 insertions(+) >> create mode 100644 hw/core/resettable.c >> create mode 100644 hw/core/trace-events >> create mode 100644 include/hw/resettable.h >> >> diff --git a/Makefile.objs b/Makefile.objs >> index 6a143dcd57..a723a47e14 100644 >> --- a/Makefile.objs >> +++ b/Makefile.objs >> @@ -191,6 +191,7 @@ trace-events-subdirs += migration >> trace-events-subdirs += net >> trace-events-subdirs += ui >> endif >> +trace-events-subdirs += hw/core >> trace-events-subdirs += hw/display >> trace-events-subdirs += qapi >> trace-events-subdirs += qom >> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs >> index f8481d959f..d9234aa98a 100644 >> --- a/hw/core/Makefile.objs >> +++ b/hw/core/Makefile.objs >> @@ -1,6 +1,7 @@ >> # core qdev-related obj files, also used by *-user: >> common-obj-y += qdev.o qdev-properties.o >> common-obj-y += bus.o reset.o >> +common-obj-y += resettable.o >> common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o >> common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o >> # irq.o needed for qdev GPIO handling: >> diff --git a/hw/core/resettable.c b/hw/core/resettable.c >> new file mode 100644 >> index 00..d7d631ce8b >> --- /dev/null >> +++ b/hw/core/resettable.c >> @@ -0,0 +1,220 @@ >> +/* >> + * Resettable interface. >> + * >> + * Copyright (c) 2019 GreenSocs SAS >> + * >> + * Authors: >> + * Damien Hedde >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. >> + * See the COPYING file in the top-level directory. >> + */ >> + >> +#include "qemu/osdep.h" >> +#include "qemu/module.h" >
Re: [Qemu-block] [PATCH v3 04/33] make Device and Bus Resettable
On 8/6/19 2:35 AM, David Gibson wrote:
> On Wed, Jul 31, 2019 at 11:09:05AM +0200, Damien Hedde wrote:
>>
>>
>> On 7/31/19 7:56 AM, David Gibson wrote:
>>> On Mon, Jul 29, 2019 at 04:56:25PM +0200, Damien Hedde wrote:
>>>> This add Resettable interface implementation for both Bus and Device.
>>>>
>>>> *resetting* counter and *reset_is_cold* flag are added in DeviceState
>>>> and BusState.
>>>>
>>>> Compatibility with existing code base is ensured.
>>>> The legacy bus or device reset method is called in the new exit phase
>>>> and the other 2 phases are let empty. Using the exit phase guarantee that
>>>> legacy resets are called in the "post" order (ie: children then parent)
>>>> in hierarchical reset. That is the same order as legacy qdev_reset_all
>>>> or qbus_reset_all were using.
>>>>
>>>> New *device_reset* and *bus_reset* function are proposed with an
>>>> additional boolean argument telling whether the reset is cold or warm.
>>>> Helper functions *device_reset_[warm|cold]* and *bus_reset_[warm|cold]*
>>>> are defined also as helpers.
>>>>
>>>> Also add a [device|bus]_is_resetting and [device|bus]_is_reset_cold
>>>> functions telling respectively whether the object is currently under reset
>>>> and
>>>> if the current reset is cold or not.
>>>>
>>>> Signed-off-by: Damien Hedde
>>>> ---
>>>> hw/core/bus.c | 85 ++
>>>> hw/core/qdev.c | 82
>>>> include/hw/qdev-core.h | 84 ++---
>>>> tests/Makefile.include | 1 +
>>>> 4 files changed, 247 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/core/bus.c b/hw/core/bus.c
>>>> index 17bc1edcde..08a97addb6 100644
>>>> --- a/hw/core/bus.c
>>>> +++ b/hw/core/bus.c
>>>> @@ -22,6 +22,7 @@
>>>> #include "qemu/module.h"
>>>> #include "hw/qdev.h"
>>>> #include "qapi/error.h"
>>>> +#include "hw/resettable.h"
>>>>
>>>> void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error
>>>> **errp)
>>>> {
>>>> @@ -68,6 +69,75 @@ int qbus_walk_children(BusState *bus,
>>>> return 0;
>>>> }
>>>>
>>>> +void bus_reset(BusState *bus, bool cold)
>>>> +{
>>>> +resettable_reset(OBJECT(bus), cold);
>>>> +}
>>>> +
>>>> +bool bus_is_resetting(BusState *bus)
>>>> +{
>>>> +return (bus->resetting != 0);
>>>> +}
>>>> +
>>>> +bool bus_is_reset_cold(BusState *bus)
>>>> +{
>>>> +return bus->reset_is_cold;
>>>> +}
>>>> +
>>>> +static uint32_t bus_get_reset_count(Object *obj)
>>>> +{
>>>> +BusState *bus = BUS(obj);
>>>> +return bus->resetting;
>>>> +}
>>>> +
>>>> +static uint32_t bus_increment_reset_count(Object *obj)
>>>> +{
>>>> +BusState *bus = BUS(obj);
>>>> +return ++bus->resetting;
>>>> +}
>>>> +
>>>> +static uint32_t bus_decrement_reset_count(Object *obj)
>>>> +{
>>>> +BusState *bus = BUS(obj);
>>>> +return --bus->resetting;
>>>> +}
>>>> +
>>>> +static bool bus_set_reset_cold(Object *obj, bool cold)
>>>> +{
>>>> +BusState *bus = BUS(obj);
>>>> +bool old = bus->reset_is_cold;
>>>> +bus->reset_is_cold = cold;
>>>> +return old;
>>>> +}
>>>> +
>>>> +static bool bus_set_hold_needed(Object *obj, bool hold_needed)
>>>> +{
>>>> +BusState *bus = BUS(obj);
>>>> +bool old = bus->reset_hold_needed;
>>>> +bus->reset_hold_needed = hold_needed;
>>>> +return old;
>>>> +}
>>>> +
>>>> +static void bus_foreach_reset_child(Object *obj, void (*func)(Object *))
>>>> +{
>>>> +BusState *bus = BUS(obj);
>>>> +BusChild *kid;
>>>> +
>>>> +QTAILQ_FOREACH(kid, >children, sibling) {
>>>> +func(OBJECT(kid->child));
>>>> +}
Re: [Qemu-block] [Qemu-devel] [PATCH v3 01/33] Create Resettable QOM interface
On 7/31/19 7:46 AM, David Gibson wrote: > On Tue, Jul 30, 2019 at 04:08:59PM +0200, Damien Hedde wrote: >> >> On 7/30/19 3:59 PM, Peter Maydell wrote: >>> On Tue, 30 Jul 2019 at 14:56, Cornelia Huck wrote: >>>> >>>> On Tue, 30 Jul 2019 14:44:21 +0100 >>>> Peter Maydell wrote: >>>> >>>>> On Tue, 30 Jul 2019 at 14:42, Cornelia Huck wrote: >>>>>> I'm having a hard time figuring out what a 'cold' or a 'warm' reset is >>>>>> supposed to be... can you add a definition/guideline somewhere? >>>>> >>>>> Generally "cold" reset is "power on" and "warm" is "we were already >>>>> powered-on, but somebody flipped a reset line somewhere". >>>> >>>> Ok, that makes sense... my main concern is to distinguish that in a >>>> generic way, as it is a generic interface. What about adding something >>>> like: >>>> >>>> "A 'cold' reset means that the object to be reset is initially reset; a >>>> 'warm' >>>> reset means that the object to be reset has already been initialized." >>>> >>>> Or is that again too generic? >>> >>> I think it doesn't quite capture the idea -- an object can have already >>> been reset and then get a 'cold' reset: this is like having a powered-on >>> machine and then power-cycling it. >>> >>> The 'warm' reset is the vaguer one, because the specific behaviour >>> is somewhat device-dependent (many devices might not have any >>> difference from 'cold' reset, for those that do the exact detail >>> of what doesn't get reset on warm-reset will vary). But every >>> device should have some kind of "as if you power-cycled it" (or >>> for QEMU, "go back to the same state as if you just started QEMU on the >>> command line"). Our current "reset" method is really cold-reset. >>> >> >> Exactly. In the following patches, I've tried to replace existing reset >> calls by cold or warm reset depending on whether: >> + it is called through the main system reset -> cold >> + it is called during normal life-time -> warm >> >> But I definitely can add some docs/comments to better explain that. > > Hrm, that helps, but it still seems pretty vague to me. > > It's not really my call, but building the concept of warm versus cold > resets into such a generic interface seems pretty dubios to me. While > it's moderately common for things to have a notion of warm versus cold > reset it's certainly not universal. There are many devices where > there's no meaningful difference between the two. There are some > devices where there are > 2 different types of reset suitable for > various different situations. > > Is there any way this could work with it usually just presenting the > cold reset (which is the closest to a universal concept), and any warm > or additional resets are handled buy a different instance of the > Resettable interface? > In my current implementation, cold/warm is only a question of setting a flag before calling the reset methods. I rely and the reset methods to check that flag if necessary. The feature can be added/removed without pain (if we stick with device_reset to do a cold one). So if it's better, I can put it aside for now. IMO handling warm reset with another interface is probably not a good idea because it will need another load of methods. -- Damien
Re: [Qemu-block] [Qemu-devel] [PATCH v3 01/33] Create Resettable QOM interface
On 7/31/19 12:17 PM, Christophe de Dinechin wrote: > > Peter Maydell writes: > >> On Tue, 30 Jul 2019 at 14:56, Cornelia Huck wrote: >>> >>> On Tue, 30 Jul 2019 14:44:21 +0100 >>> Peter Maydell wrote: >>> On Tue, 30 Jul 2019 at 14:42, Cornelia Huck wrote: > I'm having a hard time figuring out what a 'cold' or a 'warm' reset is > supposed to be... can you add a definition/guideline somewhere? Generally "cold" reset is "power on" and "warm" is "we were already powered-on, but somebody flipped a reset line somewhere". >>> >>> Ok, that makes sense... my main concern is to distinguish that in a >>> generic way, as it is a generic interface. What about adding something >>> like: >>> >>> "A 'cold' reset means that the object to be reset is initially reset; a >>> 'warm' >>> reset means that the object to be reset has already been initialized." >>> >>> Or is that again too generic? >> >> I think it doesn't quite capture the idea -- an object can have already >> been reset and then get a 'cold' reset: this is like having a powered-on >> machine and then power-cycling it. >> >> The 'warm' reset is the vaguer one, because the specific behaviour >> is somewhat device-dependent (many devices might not have any >> difference from 'cold' reset, for those that do the exact detail >> of what doesn't get reset on warm-reset will vary). But every >> device should have some kind of "as if you power-cycled it" (or >> for QEMU, "go back to the same state as if you just started QEMU on the >> command line"). Our current "reset" method is really cold-reset. > > Is there any concept of locality associated with warm reset? > For example, you'd expect a cold reset to happen on the whole system, > but I guess a warm reset could be restricted to a single bus. > > The documentation should give examples of how warm reset could be > triggered, and what it could do differently from cold reset. > Not sure we really have some locality difference between cold/warm resets. I think, most generally, locality of reset is on a per-device scale with different grouping level (up to the whole system). But buses are not always the way things are grouped. Inside a soc (and microcontrollers in general) it follows more the clock tree than the internal bus tree. For example on the the machine I worked here (xilinx-zynq-a9) and, you can control by software the reset of basically every single device and the bus too (but resetting the bus does not reset devices on it). On the other hand, there is some buses, like pci, which explicitly defines some bus reset mechanism with differences between cold and warm (some registers keep their values). (see https://wiki.qemu.org/Features/ResetAPI) Regarding cold reset, if a soc supports some power gating, you'll probably have non-global cold reset in the process. Do you mean "real world" example ?
Re: [Qemu-block] [PATCH v3 06/33] add the vmstate description for device reset state
On 7/31/19 8:08 AM, David Gibson wrote:
> On Mon, Jul 29, 2019 at 04:56:27PM +0200, Damien Hedde wrote:
>> It contains the resetting counter and cold flag status.
>>
>> At this point, migration of bus reset related state (counter and cold/warm
>> flag) is handled by parent device. This done using the post_load
>> function in the vmsd subsection.
>>
>> This is last point allow to add an initial support of migration with part of
>> qdev/qbus tree in reset state under the following condition:
>> + time-lasting reset are asserted on Device only
>>
>> Note that if this condition is not respected, migration will succeed and
>> no failure will occurs. The only impact is that the resetting counter
>> of a bus may lower afer a migration.
>>
>> Signed-off-by: Damien Hedde
>> ---
>> hw/core/Makefile.objs | 1 +
>> hw/core/qdev-vmstate.c | 45 ++
>> 2 files changed, 46 insertions(+)
>> create mode 100644 hw/core/qdev-vmstate.c
>>
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index d9234aa98a..49e9be0228 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -4,6 +4,7 @@ common-obj-y += bus.o reset.o
>> common-obj-y += resettable.o
>> common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>> common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>> +common-obj-$(CONFIG_SOFTMMU) += qdev-vmstate.o
>> # irq.o needed for qdev GPIO handling:
>> common-obj-y += irq.o
>> common-obj-y += hotplug.o
>> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
>> new file mode 100644
>> index 00..07b010811f
>> --- /dev/null
>> +++ b/hw/core/qdev-vmstate.c
>> @@ -0,0 +1,45 @@
>> +/*
>> + * Device vmstate
>> + *
>> + * Copyright (c) 2019 GreenSocs
>> + *
>> + * Authors:
>> + * Damien Hedde
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/qdev.h"
>> +#include "migration/vmstate.h"
>> +
>> +static bool device_vmstate_reset_needed(void *opaque)
>> +{
>> +DeviceState *dev = (DeviceState *) opaque;
>> +return dev->resetting != 0;
>> +}
>> +
>> +static int device_vmstate_reset_post_load(void *opaque, int version_id)
>> +{
>> +DeviceState *dev = (DeviceState *) opaque;
>> +BusState *bus;
>> +QLIST_FOREACH(bus, >child_bus, sibling) {
>> +bus->resetting = dev->resetting;
>
> Having redundant copies of the resetting bit in the bridge and every
> bus instance seems kind of bogus.
Currently we duplicate the resetting bit of parent into children when we
do the reset propagation into the tree. It means resetting count of an
device/bus contains the value of its parent plus any additional bit
local to the object (due to a reset from an gpio for example).
I'm not sure if we can avoid that. It would require the
"get_resetting_count" somehow to be recursive and fetch parent value and
so on. I need to work on it to know if it's really possible.
>
>> +bus->reset_is_cold = dev->reset_is_cold;
>> +}
>> +return 0;
>> +}
>> +
>> +const struct VMStateDescription device_vmstate_reset = {
>> +.name = "device_reset",
>> +.version_id = 0,
>> +.minimum_version_id = 0,
>> +.needed = device_vmstate_reset_needed,
>> +.post_load = device_vmstate_reset_post_load,
>> +.fields = (VMStateField[]) {
>> +VMSTATE_UINT32(resetting, DeviceState),
>> +VMSTATE_BOOL(reset_is_cold, DeviceState),
>> +VMSTATE_END_OF_LIST()
>> +},
>> +};
>
Re: [Qemu-block] [PATCH v3 08/33] Add function to control reset with gpio inputs
On 7/31/19 8:11 AM, David Gibson wrote: > On Mon, Jul 29, 2019 at 04:56:29PM +0200, Damien Hedde wrote: >> It adds the possibility to add 2 gpios to control the warm and cold reset. >> With theses ios, the reset can be maintained during some time. >> Each io is associated with a state to detect level changes. >> >> Vmstate subsections are also added to the existsing device_reset >> subsection. > > This doesn't seem like a thing that should be present on every single > DeviceState. I can revert to previous version where the io state has to be explicitly added in devices using it. Damien
Re: [Qemu-block] [PATCH v3 09/33] add doc about Resettable interface
On 7/31/19 8:30 AM, David Gibson wrote: > On Mon, Jul 29, 2019 at 04:56:30PM +0200, Damien Hedde wrote: >> Signed-off-by: Damien Hedde >> --- >> docs/devel/reset.txt | 165 +++ >> 1 file changed, 165 insertions(+) >> create mode 100644 docs/devel/reset.txt >> >> diff --git a/docs/devel/reset.txt b/docs/devel/reset.txt >> new file mode 100644 >> index 00..c7a1eb068f >> --- /dev/null >> +++ b/docs/devel/reset.txt >> @@ -0,0 +1,165 @@ >> + >> += >> +Reset >> += >> + >> +The reset of qemu objects is handled using the Resettable interface declared >> +in *include/hw/resettable.h*. >> +As of now DeviceClass and BusClass implement this interface. >> + >> + >> +Triggering reset >> + >> + >> +The function *resettable_reset* is used to trigger a reset on a given >> +object. >> +void resettable_reset(Object *obj, bool cold) >> + >> +The parameter *obj* must implement the Resettable interface. > > And what happens if it doesn't? This function has no way to report an > error. In the function, while retrieving the Resettable class, there is an assert checking the obj is compatible. We could put an error argument there to report that if that's preferable. But then it means an error object should be given for every reset call. > >> +The parameter *cold* is a boolean specifying whether to do a cold or warm >> +reset > > This doc really needs to explain the distinction between cold and warm > reset. ok > >> +For Devices and Buses there is also the corresponding helpers: >> +void device_reset(Device *dev, bool cold) >> +void bus_reset(Device *dev, bool cold) > > What's the semantic difference between resetting a bus and resetting > the bridge device which owns it? I can't speak for specific cases. BusClass has already a reset method and qbus_reset_all is used as well as qdev_reset_all in current code base. Currently both devices and buses are used as reset entry point. I'm just keeping it that way. > >> +If one wants to put an object into a reset state. There is the >> +*resettable_assert_reset* function. >> +void resettable_assert_reset(Object *obj, bool cold) >> + >> +One must eventually call the function *resettable_deassert_reset* to end the >> +reset state: >> +void resettable_deassert_reset(Object *obj, bool cold) >> + >> +Calling *resettable_assert_reset* then *resettable_deassert_reset* is the >> +same as calling *resettable_reset*. >> + >> +It is possible to interleave multiple calls to >> + - resettable_reset, >> + - resettable_assert_reset, and >> + - resettable_deassert_reset. >> +The only constraint is that *resettable_deassert_reset* must be called once >> +per *resettable_assert_reset* call so that the object leaves the reset >> state. >> + >> +Therefore there may be several reset sources/controllers of a given object. >> +The interface handle everything and the controllers do not need to know >> +anything about each others. The object will leave reset state only when all >> +controllers released their reset. >> + >> +All theses functions must called while holding the iothread lock. >> + >> + >> +Implementing reset for a Resettable object : Multi-phase reset >> +-- >> + >> +The Resettable uses a multi-phase mechanism to handle some ordering >> constraints >> +when resetting multiple object at the same time. For a given object the >> reset >> +procedure is split into three different phases executed in order: >> + 1 INIT: This phase should set/reset the state of the Resettable it has >> when is >> + in reset state. Side-effects to others object is forbidden (such as >> + setting IO level). >> + 2 HOLD: This phase corresponds to the external side-effects due to staying >> into >> + the reset state. >> + 3 EXIT: This phase corresponds to leaving the reset state. It have both >> + local and external effects. >> + >> +*resettable_assert_reset* does the INIT and HOLD phases. While >> +*resettable_deassert_reset* does the EXIT phase. >> + >> +When resetting multiple object at the same time. The interface executes the >> +given phase of the objects before going to the next phase. This guarantee >> that >> +all INIT phases are done before any HOLD phase and so on. >> + >> +There is three methods in the interface so must be implemented in an object. >&
Re: [Qemu-block] [PATCH v3 05/33] Switch to new api in qdev/bus
On 7/31/19 8:05 AM, David Gibson wrote:
> On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote:
>> Deprecate old reset apis and make them use the new one while they
>> are still used somewhere.
>>
>> Signed-off-by: Damien Hedde
>> ---
>> hw/core/qdev.c | 22 +++---
>> include/hw/qdev-core.h | 28 ++--
>> 2 files changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 559ced070d..e9e5f2d5f9 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj,
>> void (*func)(Object *))
>> }
>> }
>>
>> -static int qdev_reset_one(DeviceState *dev, void *opaque)
>> -{
>> -device_legacy_reset(dev);
>> -
>> -return 0;
>> -}
>> -
>> -static int qbus_reset_one(BusState *bus, void *opaque)
>> -{
>> -BusClass *bc = BUS_GET_CLASS(bus);
>> -if (bc->reset) {
>> -bc->reset(bus);
>> -}
>> -return 0;
>> -}
>> -
>> void qdev_reset_all(DeviceState *dev)
>> {
>> -qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one,
>> NULL);
>> +device_reset(dev, false);
>> }
>>
>> void qdev_reset_all_fn(void *opaque)
>> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque)
>>
>> void qbus_reset_all(BusState *bus)
>> {
>> -qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one,
>> NULL);
>> +bus_reset(bus, false);
>> }
>>
>> void qbus_reset_all_fn(void *opaque)
>> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value,
>> Error **errp)
>> }
>> }
>> if (dev->hotplugged) {
>> -device_legacy_reset(dev);
>> +device_reset(dev, true);
>
> So.. is this change in the device_reset() signature really necessary?
> Even if there are compelling reasons to handle warm reset in the new
> API, that doesn't been you need to change device_reset() itself from
> its established meaning of a cold (i.e. as per power cycle) reset.
> Warm resets are generally called in rather more specific circumstances
> (often under guest software direction) so it seems likely that users
> would want to engage with the new reset API directly. Or we could
> just create a device_warm_reset() wrapper. That would also avoid the
> bare boolean parameter, which is not great for readability (you have
> to look up the signature to have any idea what it means).
I've added device_reset_cold/warm wrapper functions to avoid having to
pass the boolean parameter. it seems I forgot to use them in qdev.c
I suppose, like you said, we could live with
+ no function with the boolean parameter
+ device_reset doing cold reset
+ device_reset_warm (or device_warm_reset) for the warm version
Damien
Re: [Qemu-block] [PATCH v3 04/33] make Device and Bus Resettable
On 7/31/19 7:56 AM, David Gibson wrote:
> On Mon, Jul 29, 2019 at 04:56:25PM +0200, Damien Hedde wrote:
>> This add Resettable interface implementation for both Bus and Device.
>>
>> *resetting* counter and *reset_is_cold* flag are added in DeviceState
>> and BusState.
>>
>> Compatibility with existing code base is ensured.
>> The legacy bus or device reset method is called in the new exit phase
>> and the other 2 phases are let empty. Using the exit phase guarantee that
>> legacy resets are called in the "post" order (ie: children then parent)
>> in hierarchical reset. That is the same order as legacy qdev_reset_all
>> or qbus_reset_all were using.
>>
>> New *device_reset* and *bus_reset* function are proposed with an
>> additional boolean argument telling whether the reset is cold or warm.
>> Helper functions *device_reset_[warm|cold]* and *bus_reset_[warm|cold]*
>> are defined also as helpers.
>>
>> Also add a [device|bus]_is_resetting and [device|bus]_is_reset_cold
>> functions telling respectively whether the object is currently under reset
>> and
>> if the current reset is cold or not.
>>
>> Signed-off-by: Damien Hedde
>> ---
>> hw/core/bus.c | 85 ++
>> hw/core/qdev.c | 82
>> include/hw/qdev-core.h | 84 ++---
>> tests/Makefile.include | 1 +
>> 4 files changed, 247 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/core/bus.c b/hw/core/bus.c
>> index 17bc1edcde..08a97addb6 100644
>> --- a/hw/core/bus.c
>> +++ b/hw/core/bus.c
>> @@ -22,6 +22,7 @@
>> #include "qemu/module.h"
>> #include "hw/qdev.h"
>> #include "qapi/error.h"
>> +#include "hw/resettable.h"
>>
>> void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp)
>> {
>> @@ -68,6 +69,75 @@ int qbus_walk_children(BusState *bus,
>> return 0;
>> }
>>
>> +void bus_reset(BusState *bus, bool cold)
>> +{
>> +resettable_reset(OBJECT(bus), cold);
>> +}
>> +
>> +bool bus_is_resetting(BusState *bus)
>> +{
>> +return (bus->resetting != 0);
>> +}
>> +
>> +bool bus_is_reset_cold(BusState *bus)
>> +{
>> +return bus->reset_is_cold;
>> +}
>> +
>> +static uint32_t bus_get_reset_count(Object *obj)
>> +{
>> +BusState *bus = BUS(obj);
>> +return bus->resetting;
>> +}
>> +
>> +static uint32_t bus_increment_reset_count(Object *obj)
>> +{
>> +BusState *bus = BUS(obj);
>> +return ++bus->resetting;
>> +}
>> +
>> +static uint32_t bus_decrement_reset_count(Object *obj)
>> +{
>> +BusState *bus = BUS(obj);
>> +return --bus->resetting;
>> +}
>> +
>> +static bool bus_set_reset_cold(Object *obj, bool cold)
>> +{
>> +BusState *bus = BUS(obj);
>> +bool old = bus->reset_is_cold;
>> +bus->reset_is_cold = cold;
>> +return old;
>> +}
>> +
>> +static bool bus_set_hold_needed(Object *obj, bool hold_needed)
>> +{
>> +BusState *bus = BUS(obj);
>> +bool old = bus->reset_hold_needed;
>> +bus->reset_hold_needed = hold_needed;
>> +return old;
>> +}
>> +
>> +static void bus_foreach_reset_child(Object *obj, void (*func)(Object *))
>> +{
>> +BusState *bus = BUS(obj);
>> +BusChild *kid;
>> +
>> +QTAILQ_FOREACH(kid, >children, sibling) {
>> +func(OBJECT(kid->child));
>> +}
>> +}
>
> IIUC, every resettable class would need more or less identical
> implementations of the above. That seems like an awful lot of
> boilerplate.
Do you mean the get/increment_count/decrement_count, set_cold/hold part ?
True, but it's limited to the base classes.
Since Resettable is an interface, we have no state there to store what
we need. Only alternative is to have some kind of single
get_resettable_state method returning a pointer to the state (allowing
us to keep the functions in the interface code).
Beyond Device and Bus, which are done here, there is probably not so
many class candidates for the Resettable interface.
Damien
Re: [Qemu-block] [PATCH v3 01/33] Create Resettable QOM interface
On 7/30/19 3:59 PM, Peter Maydell wrote: > On Tue, 30 Jul 2019 at 14:56, Cornelia Huck wrote: >> >> On Tue, 30 Jul 2019 14:44:21 +0100 >> Peter Maydell wrote: >> >>> On Tue, 30 Jul 2019 at 14:42, Cornelia Huck wrote: I'm having a hard time figuring out what a 'cold' or a 'warm' reset is supposed to be... can you add a definition/guideline somewhere? >>> >>> Generally "cold" reset is "power on" and "warm" is "we were already >>> powered-on, but somebody flipped a reset line somewhere". >> >> Ok, that makes sense... my main concern is to distinguish that in a >> generic way, as it is a generic interface. What about adding something >> like: >> >> "A 'cold' reset means that the object to be reset is initially reset; a >> 'warm' >> reset means that the object to be reset has already been initialized." >> >> Or is that again too generic? > > I think it doesn't quite capture the idea -- an object can have already > been reset and then get a 'cold' reset: this is like having a powered-on > machine and then power-cycling it. > > The 'warm' reset is the vaguer one, because the specific behaviour > is somewhat device-dependent (many devices might not have any > difference from 'cold' reset, for those that do the exact detail > of what doesn't get reset on warm-reset will vary). But every > device should have some kind of "as if you power-cycled it" (or > for QEMU, "go back to the same state as if you just started QEMU on the > command line"). Our current "reset" method is really cold-reset. > Exactly. In the following patches, I've tried to replace existing reset calls by cold or warm reset depending on whether: + it is called through the main system reset -> cold + it is called during normal life-time -> warm But I definitely can add some docs/comments to better explain that. -- Damien
[Qemu-block] [PATCH v3 29/33] hw/misc/zynq_slcr: use standard register definition
Replace the zynq_slcr registers enum and macros using the
hw/registerfields.h macros.
Signed-off-by: Damien Hedde
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Alistair Francis
---
hw/misc/zynq_slcr.c | 472 ++--
1 file changed, 236 insertions(+), 236 deletions(-)
diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index 6b51ae5ff1..dd766a6779 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -21,6 +21,7 @@
#include "sysemu/sysemu.h"
#include "qemu/log.h"
#include "qemu/module.h"
+#include "hw/registerfields.h"
#ifndef ZYNQ_SLCR_ERR_DEBUG
#define ZYNQ_SLCR_ERR_DEBUG 0
@@ -36,138 +37,135 @@
#define XILINX_LOCK_KEY 0x767b
#define XILINX_UNLOCK_KEY 0xdf0d
-#define R_PSS_RST_CTRL_SOFT_RST 0x1
-
-enum {
-SCL = 0x000 / 4,
-LOCK,
-UNLOCK,
-LOCKSTA,
-
-ARM_PLL_CTRL= 0x100 / 4,
-DDR_PLL_CTRL,
-IO_PLL_CTRL,
-PLL_STATUS,
-ARM_PLL_CFG,
-DDR_PLL_CFG,
-IO_PLL_CFG,
-
-ARM_CLK_CTRL= 0x120 / 4,
-DDR_CLK_CTRL,
-DCI_CLK_CTRL,
-APER_CLK_CTRL,
-USB0_CLK_CTRL,
-USB1_CLK_CTRL,
-GEM0_RCLK_CTRL,
-GEM1_RCLK_CTRL,
-GEM0_CLK_CTRL,
-GEM1_CLK_CTRL,
-SMC_CLK_CTRL,
-LQSPI_CLK_CTRL,
-SDIO_CLK_CTRL,
-UART_CLK_CTRL,
-SPI_CLK_CTRL,
-CAN_CLK_CTRL,
-CAN_MIOCLK_CTRL,
-DBG_CLK_CTRL,
-PCAP_CLK_CTRL,
-TOPSW_CLK_CTRL,
+REG32(SCL, 0x000)
+REG32(LOCK, 0x004)
+REG32(UNLOCK, 0x008)
+REG32(LOCKSTA, 0x00c)
+
+REG32(ARM_PLL_CTRL, 0x100)
+REG32(DDR_PLL_CTRL, 0x104)
+REG32(IO_PLL_CTRL, 0x108)
+REG32(PLL_STATUS, 0x10c)
+REG32(ARM_PLL_CFG, 0x110)
+REG32(DDR_PLL_CFG, 0x114)
+REG32(IO_PLL_CFG, 0x118)
+
+REG32(ARM_CLK_CTRL, 0x120)
+REG32(DDR_CLK_CTRL, 0x124)
+REG32(DCI_CLK_CTRL, 0x128)
+REG32(APER_CLK_CTRL, 0x12c)
+REG32(USB0_CLK_CTRL, 0x130)
+REG32(USB1_CLK_CTRL, 0x134)
+REG32(GEM0_RCLK_CTRL, 0x138)
+REG32(GEM1_RCLK_CTRL, 0x13c)
+REG32(GEM0_CLK_CTRL, 0x140)
+REG32(GEM1_CLK_CTRL, 0x144)
+REG32(SMC_CLK_CTRL, 0x148)
+REG32(LQSPI_CLK_CTRL, 0x14c)
+REG32(SDIO_CLK_CTRL, 0x150)
+REG32(UART_CLK_CTRL, 0x154)
+REG32(SPI_CLK_CTRL, 0x158)
+REG32(CAN_CLK_CTRL, 0x15c)
+REG32(CAN_MIOCLK_CTRL, 0x160)
+REG32(DBG_CLK_CTRL, 0x164)
+REG32(PCAP_CLK_CTRL, 0x168)
+REG32(TOPSW_CLK_CTRL, 0x16c)
#define FPGA_CTRL_REGS(n, start) \
-FPGA ## n ## _CLK_CTRL = (start) / 4, \
-FPGA ## n ## _THR_CTRL, \
-FPGA ## n ## _THR_CNT, \
-FPGA ## n ## _THR_STA,
-FPGA_CTRL_REGS(0, 0x170)
-FPGA_CTRL_REGS(1, 0x180)
-FPGA_CTRL_REGS(2, 0x190)
-FPGA_CTRL_REGS(3, 0x1a0)
-
-BANDGAP_TRIP= 0x1b8 / 4,
-PLL_PREDIVISOR = 0x1c0 / 4,
-CLK_621_TRUE,
-
-PSS_RST_CTRL= 0x200 / 4,
-DDR_RST_CTRL,
-TOPSW_RESET_CTRL,
-DMAC_RST_CTRL,
-USB_RST_CTRL,
-GEM_RST_CTRL,
-SDIO_RST_CTRL,
-SPI_RST_CTRL,
-CAN_RST_CTRL,
-I2C_RST_CTRL,
-UART_RST_CTRL,
-GPIO_RST_CTRL,
-LQSPI_RST_CTRL,
-SMC_RST_CTRL,
-OCM_RST_CTRL,
-FPGA_RST_CTRL = 0x240 / 4,
-A9_CPU_RST_CTRL,
-
-RS_AWDT_CTRL= 0x24c / 4,
-RST_REASON,
-
-REBOOT_STATUS = 0x258 / 4,
-BOOT_MODE,
-
-APU_CTRL= 0x300 / 4,
-WDT_CLK_SEL,
-
-TZ_DMA_NS = 0x440 / 4,
-TZ_DMA_IRQ_NS,
-TZ_DMA_PERIPH_NS,
-
-PSS_IDCODE = 0x530 / 4,
-
-DDR_URGENT = 0x600 / 4,
-DDR_CAL_START = 0x60c / 4,
-DDR_REF_START = 0x614 / 4,
-DDR_CMD_STA,
-DDR_URGENT_SEL,
-DDR_DFI_STATUS,
-
-MIO = 0x700 / 4,
+REG32(FPGA ## n ## _CLK_CTRL, (start)) \
+REG32(FPGA ## n ## _THR_CTRL, (start) + 0x4)\
+REG32(FPGA ## n ## _THR_CNT, (start) + 0x8)\
+REG32(FPGA ## n ## _THR_STA, (start) + 0xc)
+FPGA_CTRL_REGS(0, 0x170)
+FPGA_CTRL_REGS(1, 0x180)
+FPGA_CTRL_REGS(2, 0x190)
+FPGA_CTRL_REGS(3, 0x1a0)
+
+REG32(BANDGAP_TRIP, 0x1b8)
+REG32(PLL_PREDIVISOR, 0x1c0)
+REG32(CLK_621_TRUE, 0x1c4)
+
+REG32(PSS_RST_CTRL, 0x200)
+FIELD(PSS_RST_CTRL, SOFT_RST, 0, 1)
+REG32(DDR_RST_CTRL, 0x204)
+REG32(TOPSW_RESET_CTRL, 0x208)
+REG32(DMAC_RST_CTRL, 0x20c)
+REG32(USB_RST_CTRL, 0x210)
+REG32(GEM_RST_CTRL, 0x214)
+REG32(SDIO_RST_CTRL, 0x218)
+REG32(SPI_RST_CTRL, 0x21c)
+REG32(CAN_RST_CTRL, 0x220)
+REG32(I2C_RST_CTRL, 0x224)
+REG32(UART_RST_CTRL, 0x228)
+REG32(GPIO_RST_CTRL, 0x22c)
+REG32(LQSPI_RST_CTRL, 0x230)
+REG32(SMC_RST_CTRL, 0x234)
+REG32(OCM_RST_CTRL, 0x238)
+REG32(FPGA_RST_CTRL, 0x240)
+REG32(A9_CPU_RST_CTRL, 0x244)
+
+REG32(RS_AWDT_CTRL, 0x24c)
+REG32(RST_REASON, 0x250)
+
+REG32(REBOOT_STATUS, 0x258)
+REG32(BOOT_MODE, 0x25c)
+
+REG32(APU_CTRL, 0x300)
+REG32(WDT_CLK_SEL, 0x304)
+
+REG32(TZ_DMA_NS, 0x440)
+REG32(TZ_DMA_IRQ_NS, 0x444)
+REG32(TZ_DMA_PERIPH_NS, 0x448)
+
+REG32(PSS_IDCODE, 0x530)
+
+REG32(DDR_URGENT, 0x600)
+REG32(DDR_CAL_START, 0x60c)
+REG32(DDR_REF_START, 0x614)
+REG32(DDR_CMD_STA, 0x618)
+REG32(DDR_URGENT_SEL, 0x61c)
+REG32(DDR_DFI_STATUS, 0x620)
+
+REG32(MIO, 0x700)
#define MIO_LENGTH 54
-MIO_LOOPBACK= 0x804 / 4,
-
[Qemu-block] [PATCH v3 22/33] hw/ppc/pnv_psi.c: remove device_legacy_reset call
Replace legacy's reset call by device_reset_warm.
The new function propagates also the reset to the sub-buses tree but this has
no impact since XiveSource has no child bus.
Signed-off-by: Damien Hedde
---
hw/ppc/pnv_psi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 78eafa353a..c17e83abe5 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -703,7 +703,7 @@ static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
break;
case PSIHB9_INTERRUPT_CONTROL:
if (val & PSIHB9_IRQ_RESET) {
-device_legacy_reset(DEVICE(>source));
+device_reset_warm(DEVICE(>source));
}
psi->regs[reg] = val;
break;
--
2.22.0
[Qemu-block] [PATCH v3 31/33] Convert zynq's slcr to 3-phases reset
Change the legacy reset function into the init phase and test the
resetting flag in register accesses.
Signed-off-by: Damien Hedde
---
hw/misc/zynq_slcr.c | 39 +++
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index dd766a6779..6fcdbce4f0 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -172,6 +172,17 @@ REG32(DDRIOB, 0xb40)
#define TYPE_ZYNQ_SLCR "xilinx,zynq_slcr"
#define ZYNQ_SLCR(obj) OBJECT_CHECK(ZynqSLCRState, (obj), TYPE_ZYNQ_SLCR)
+#define ZYNQ_SLCR_CLASS(class) \
+OBJECT_CLASS_CHECK(ZynqSLCRClass, (class), TYPE_ZYNQ_SLCR)
+#define ZYNQ_SLCR_GET_CLASS(obj) \
+OBJECT_GET_CLASS(ZynqSLCRClass, (obj), TYPE_ZYNQ_SLCR)
+
+typedef struct ZynqSLCRClass {
+/*< private >*/
+SysBusDeviceClass parent_class;
+
+struct ResettablePhases parent_reset_phases;
+} ZynqSLCRClass;
typedef struct ZynqSLCRState {
SysBusDevice parent_obj;
@@ -181,13 +192,18 @@ typedef struct ZynqSLCRState {
uint32_t regs[ZYNQ_SLCR_NUM_REGS];
} ZynqSLCRState;
-static void zynq_slcr_reset(DeviceState *d)
+static void zynq_slcr_reset_init(Object *obj)
{
-ZynqSLCRState *s = ZYNQ_SLCR(d);
+ZynqSLCRState *s = ZYNQ_SLCR(obj);
+ZynqSLCRClass *zc = ZYNQ_SLCR_GET_CLASS(obj);
int i;
DB_PRINT("RESET\n");
+if (zc->parent_reset_phases.init) {
+zc->parent_reset_phases.init(obj);
+}
+
s->regs[R_LOCKSTA] = 1;
/* 0x100 - 0x11C */
s->regs[R_ARM_PLL_CTRL] = 0x0001A008;
@@ -277,7 +293,6 @@ static void zynq_slcr_reset(DeviceState *d)
s->regs[R_DDRIOB + 12] = 0x0021;
}
-
static bool zynq_slcr_check_offset(hwaddr offset, bool rnw)
{
switch (offset) {
@@ -347,6 +362,10 @@ static uint64_t zynq_slcr_read(void *opaque, hwaddr offset,
offset /= 4;
uint32_t ret = s->regs[offset];
+if (device_is_resetting((DeviceState *) opaque)) {
+return 0;
+}
+
if (!zynq_slcr_check_offset(offset, true)) {
qemu_log_mask(LOG_GUEST_ERROR, "zynq_slcr: Invalid read access to "
" addr %" HWADDR_PRIx "\n", offset * 4);
@@ -362,6 +381,10 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
ZynqSLCRState *s = (ZynqSLCRState *)opaque;
offset /= 4;
+if (device_is_resetting((DeviceState *) opaque)) {
+return;
+}
+
DB_PRINT("addr: %08" HWADDR_PRIx " data: %08" PRIx64 "\n", offset * 4,
val);
if (!zynq_slcr_check_offset(offset, false)) {
@@ -440,9 +463,16 @@ static const VMStateDescription vmstate_zynq_slcr = {
static void zynq_slcr_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
+ZynqSLCRClass *zc = ZYNQ_SLCR_CLASS(klass);
dc->vmsd = _zynq_slcr;
-dc->reset = zynq_slcr_reset;
+
+resettable_class_set_parent_reset_phases(rc,
+ zynq_slcr_reset_init,
+ NULL,
+ NULL,
+ >parent_reset_phases);
}
static const TypeInfo zynq_slcr_info = {
@@ -451,6 +481,7 @@ static const TypeInfo zynq_slcr_info = {
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(ZynqSLCRState),
.instance_init = zynq_slcr_init,
+.class_size = sizeof(ZynqSLCRClass),
};
static void zynq_slcr_register_types(void)
--
2.22.0
[Qemu-block] [PATCH v3 30/33] convert cadence_uart to 3-phases reset
Split the existing reset procedure into 3 phases.
Test the resetting flag to discard register accesses
and character reception.
Also adds a active high reset io.
Signed-off-by: Damien Hedde
---
hw/char/cadence_uart.c | 77 +++---
1 file changed, 73 insertions(+), 4 deletions(-)
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index fa25fe24da..d7bacc44fe 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -39,6 +39,18 @@
#define DB_PRINT(...)
#endif
+#define CADENCE_UART_CLASS(class) \
+OBJECT_CLASS_CHECK(CadenceUartClass, (class), TYPE_CADENCE_UART)
+#define CADENCE_UART_GET_CLASS(obj) \
+OBJECT_GET_CLASS(CadenceUartClass, (obj), TYPE_CADENCE_UART)
+
+typedef struct CadenceUartClass {
+/*< private >*/
+SysBusDeviceClass parent_class;
+
+struct ResettablePhases parent_reset_phases;
+} CadenceUartClass;
+
#define UART_SR_INTR_RTRIG 0x0001
#define UART_SR_INTR_REMPTY0x0002
#define UART_SR_INTR_RFUL 0x0004
@@ -223,6 +235,10 @@ static int uart_can_receive(void *opaque)
int ret = MAX(CADENCE_UART_RX_FIFO_SIZE, CADENCE_UART_TX_FIFO_SIZE);
uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
+if (device_is_resetting((DeviceState *) opaque)) {
+return 0;
+}
+
if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
ret = MIN(ret, CADENCE_UART_RX_FIFO_SIZE - s->rx_count);
}
@@ -338,6 +354,10 @@ static void uart_receive(void *opaque, const uint8_t *buf,
int size)
CadenceUARTState *s = opaque;
uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
+if (device_is_resetting((DeviceState *) opaque)) {
+return;
+}
+
if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
uart_write_rx_fifo(opaque, buf, size);
}
@@ -351,6 +371,10 @@ static void uart_event(void *opaque, int event)
CadenceUARTState *s = opaque;
uint8_t buf = '\0';
+if (device_is_resetting((DeviceState *) opaque)) {
+return;
+}
+
if (event == CHR_EVENT_BREAK) {
uart_write_rx_fifo(opaque, , 1);
}
@@ -383,6 +407,10 @@ static void uart_write(void *opaque, hwaddr offset,
{
CadenceUARTState *s = opaque;
+if (device_is_resetting((DeviceState *)opaque)) {
+return;
+}
+
DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
offset >>= 2;
if (offset >= CADENCE_UART_R_MAX) {
@@ -441,6 +469,10 @@ static uint64_t uart_read(void *opaque, hwaddr offset,
CadenceUARTState *s = opaque;
uint32_t c = 0;
+if (device_is_resetting((DeviceState *)opaque)) {
+return 0;
+}
+
offset >>= 2;
if (offset >= CADENCE_UART_R_MAX) {
c = 0;
@@ -460,9 +492,14 @@ static const MemoryRegionOps uart_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
-static void cadence_uart_reset(DeviceState *dev)
+static void cadence_uart_reset_init(Object *obj)
{
-CadenceUARTState *s = CADENCE_UART(dev);
+CadenceUARTState *s = CADENCE_UART(obj);
+CadenceUartClass *cc = CADENCE_UART_GET_CLASS(obj);
+
+if (cc->parent_reset_phases.init) {
+cc->parent_reset_phases.init(obj);
+}
s->r[R_CR] = 0x0128;
s->r[R_IMR] = 0;
@@ -471,6 +508,28 @@ static void cadence_uart_reset(DeviceState *dev)
s->r[R_BRGR] = 0x028B;
s->r[R_BDIV] = 0x000F;
s->r[R_TTRIG] = 0x0020;
+}
+
+static void cadence_uart_reset_hold(Object *obj)
+{
+CadenceUARTState *s = CADENCE_UART(obj);
+CadenceUartClass *cc = CADENCE_UART_GET_CLASS(obj);
+
+if (cc->parent_reset_phases.hold) {
+cc->parent_reset_phases.hold(obj);
+}
+
+qemu_set_irq(s->irq, 0);
+}
+
+static void cadence_uart_reset_exit(Object *obj)
+{
+CadenceUARTState *s = CADENCE_UART(obj);
+CadenceUartClass *cc = CADENCE_UART_GET_CLASS(obj);
+
+if (cc->parent_reset_phases.exit) {
+cc->parent_reset_phases.exit(obj);
+}
uart_rx_reset(s);
uart_tx_reset(s);
@@ -499,6 +558,8 @@ static void cadence_uart_init(Object *obj)
sysbus_init_irq(sbd, >irq);
s->char_tx_time = (NANOSECONDS_PER_SECOND / 9600) * 10;
+
+qdev_init_warm_reset_gpio(DEVICE(obj), "rst", DEVICE_RESET_ACTIVE_HIGH);
}
static int cadence_uart_post_load(void *opaque, int version_id)
@@ -544,12 +605,19 @@ static Property cadence_uart_properties[] = {
static void cadence_uart_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
+CadenceUartClass *cc = CADENCE_UART_CLASS(klass);
dc->realize = cadence_uart_realize;
dc->vmsd = _cadence_uart;
-dc->reset = cadence_uart_reset;
dc->props = cadence_uart_properties;
- }
+
+resettable_class_set_parent_reset_phases(rc,
+
[Qemu-block] [PATCH v3 25/33] hw/i386/pc.c: remove device_legacy_reset call
Replace additional APIC legacy reset by device_reset_cold.
The new function propagates also the reset to the sub-buses tree.
APIC does not have any so it should have no impact on behavior.
Signed-off-by: Damien Hedde
---
hw/i386/pc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c0f20fe8aa..a175d76819 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2823,7 +2823,7 @@ static void pc_machine_reset(MachineState *machine)
cpu = X86_CPU(cs);
if (cpu->apic_state) {
-device_legacy_reset(cpu->apic_state);
+device_reset_cold(cpu->apic_state);
}
}
}
--
2.22.0
[Qemu-block] [PATCH v3 24/33] hw/ppc/spapr: remove device_legacy_reset call
Replace legacy's reset call by device_reset_warm.
The new function propagates also the reset to the sub-buses tree.
In spapr_vio.c, the function resets a SpaprtceTable which does not seem
to have child bus so it should have no impact.
In Spapr_pci.c the functions resets QOM children devices of a SpaprPhbState.
If there is a device with a child bus, then this bus will now be reset
(and all its qdev tree).
Signed-off-by: Damien Hedde
---
hw/ppc/spapr_pci.c | 2 +-
hw/ppc/spapr_vio.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 3c6cf79a5e..946b2b4483 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2029,7 +2029,7 @@ static int spapr_phb_children_reset(Object *child, void
*opaque)
DeviceState *dev = (DeviceState *) object_dynamic_cast(child, TYPE_DEVICE);
if (dev) {
-device_legacy_reset(dev);
+device_reset_warm(dev);
}
return 0;
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 5a0b5cc35c..41c17cfdd6 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -306,7 +306,7 @@ int spapr_vio_send_crq(SpaprVioDevice *dev, uint8_t *crq)
static void spapr_vio_quiesce_one(SpaprVioDevice *dev)
{
if (dev->tcet) {
-device_legacy_reset(DEVICE(dev->tcet));
+device_reset_warm(DEVICE(dev->tcet));
}
free_crq(dev);
}
--
2.22.0
[Qemu-block] [PATCH v3 26/33] hw/s390x/s390-pci-inst.c: remove device_legacy_reset call
Replace S390PCIBusDevice legacy reset by device_reset_warm. The new function propagates also the reset to the sub-buses tree. I'm not sure whether S390PCIBusDevice has bus children or not. Signed-off-by: Damien Hedde --- hw/s390x/s390-pci-inst.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 93cda37c27..d7bca68245 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -242,7 +242,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra) stw_p(>hdr.rsp, CLP_RC_SETPCIFN_FHOP); goto out; } -device_legacy_reset(DEVICE(pbdev)); +device_reset_warm(DEVICE(pbdev)); pbdev->fh &= ~FH_MASK_ENABLE; pbdev->state = ZPCI_FS_DISABLED; stl_p(>fh, pbdev->fh); -- 2.22.0
[Qemu-block] [PATCH v3 15/33] hw/ide/piix.c: remove qdev_reset_all call
Replace deprecated qdev_reset_all by device_reset_warm. This does not impact the behavior. Signed-off-by: Damien Hedde --- hw/ide/piix.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ide/piix.c b/hw/ide/piix.c index b97e555072..64cb4a52ef 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -196,7 +196,7 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux) blk_unref(blk); } } -qdev_reset_all(DEVICE(dev)); +device_reset_warm(DEVICE(dev)); return 0; } -- 2.22.0
[Qemu-block] [PATCH v3 23/33] hw/scsi/vmw_pvscsi.c: remove device_legacy_reset call
Replace legacy's reset call by device_reset_warm.
The new function propagates also the reset to the sub-buses tree but this has
no impact since SCSIDevices have no child bus (neither generic device nor
disks).
Signed-off-by: Damien Hedde
---
hw/scsi/vmw_pvscsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 40fcf808a7..5be2227cc8 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -835,7 +835,7 @@ pvscsi_on_cmd_reset_device(PVSCSIState *s)
if (sdev != NULL) {
s->resetting++;
-device_legacy_reset(>qdev);
+device_reset_warm(>qdev);
s->resetting--;
return PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
}
--
2.22.0
[Qemu-block] [PATCH v3 11/33] hw/s390x/ipl.c: remove qbus_reset_all registration
Replace deprecated qbus_reset_all by resettable_reset_cold_fn for the ipl registration in the main reset handlers. This does not impact the behavior. Signed-off-by: Damien Hedde --- hw/s390x/ipl.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 60bd081d3e..402770a2c9 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -234,7 +234,11 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) */ ipl->compat_start_addr = ipl->start_addr; ipl->compat_bios_start_addr = ipl->bios_start_addr; -qemu_register_reset(qdev_reset_all_fn, dev); +/* + * TODO: when we add some kind of main reset container / domain + * switch to it to really benefit from multi-phase. + */ +qemu_register_reset(resettable_reset_cold_fn, dev); error: error_propagate(errp, err); } -- 2.22.0
[Qemu-block] [PATCH v3 00/33] Multi-phase reset mechanism
sd/omap_mmc.c -- resetting an SDState, which has no child bus hw/sd/pl181.c -- resetting an SDState, which has no child bus In my opinion only hw/ppc/spapr_pci.c and hw/s390x/s390-pci-inst.c may imply behavior changes. 3. DeviceClass's reset and BusClass's methods This is the major change. The method is deprecated because, methods are now located in the interface class. In the series, I make the exit phase redirect to the original reset method of DeviceClass (or BusClass). There a lot of use of the method and transitioning to 3-phases only reset will need some work I did not address in this series. # Migration For devices, I've added a patch to automate the addition of reset related subsection. In consequence it is not necessary to explicetely add the reset subsection in every device specialization requiring it. Right know this is kind of a hack into qdev to dynamically modify the vmsd before the registration. There is probably a much cleaner way to do this but I prefered to demonstrate it by keeping modification local to qdev. As far as I can tell it's ok to dynamically add subsections at the end. This does not prevent from further adding subsections in the orignal vmsd: the subsections order in the array is irrelevant from migration point-of-view. The loading part just lookup each subsection in the array by name uppon reception. For buses, I handle them in the parent parent post_load. It has some limitation but should work pretty well for most of cases (if we hold reset qdev/qbus subtree starting from a device not a bus). If we consider holding part of a soc under reset goes through gpio, it have to be a device. # Change in qdev/qbus hiearchy It is possible to change the parent bus of a device with the qdev_set_parent_bus function. We could add some code to adapt the resetting count of the device if old and new bus have different reset state. I did not do it right now because it doesnt not fill the right place do this: as long as reset hiearchy follows the qdev hiearchy this would be fine place, but there is nothing enforcing that with Resettable. Given that, there is actually no bus help in reset. The only case when it can happen is during the reset of a bus. Running the qom-test which test every machine (with the initial main reset) gives one case where it happens: the raspi because sd-card is moved during the reset. IMO, during reset, it's hazardous to change qdev/qbus hierarchy since reset follows it so I'll try to address this on a separate patch. # Test I tested this using the xilinx-zynq-a9 machine. Reset gpio and migration during reset works. # Changes since v2 - don't call device implementations of reset phase functions multiple times. - init phase are executed parent-to-children order, legacy reset method is mapped on exit phase in order to keep the legacy order. - migration: poc of automatic addition of subsection. - migration: basic support for sub-buses. - qdev/qbus_reset_all & device_reset replacement The series is organised as follow: - Patch 1 adds Resettable interface - Patches 2 and 3 rename device_reset function by device_legacy_reset to prepare the transition. - Patches 4 to 8 makes the changes in Device and Bus classes. - Patche 9 adds some doc - Patches 10 to 17 replace qdev/qbus_reset calls - Patches 18 to 27 replace device_reset calls - Patch 28 cleans remaining legacy reset API - Patches 29 to 33 modify the xilinx_zynq to add 3-phases reset support in the uart and the slcr (the reset controller of the soc). Thanks, Damien Damien Hedde (33): Create Resettable QOM interface add temporary device_legacy_reset function to replace device_reset Replace all call to device_reset by call to device_legacy_reset make Device and Bus Resettable Switch to new api in qdev/bus add the vmstate description for device reset state automatically add vmstate for reset support in devices Add function to control reset with gpio inputs add doc about Resettable interface vl.c: remove qbus_reset_all registration hw/s390x/ipl.c: remove qbus_reset_all registration hw/pci/: remove qdev/qbus_reset_all call hw/scsi/: remove qdev/qbus_reset_all call hw/s390x/s390-virtio-ccw.c: remove qdev_reset_all call hw/ide/piix.c: remove qdev_reset_all call hw/input/adb.c: remove qdev_reset_all call hw/usb/dev-uas.c: remove qdev_reset_all call hw/audio/intel-hda.c: remove device_legacy_reset call hw/sd/pl181.c & omap_mmc.c: remove device_legacy_reset call hw/hyperv/hyperv.c: remove device_legacy_reset call hw/intc/spapr_xive.c: remove device_legacy_reset call hw/ppc/pnv_psi.c: remove device_legacy_reset call hw/scsi/vmw_pvscsi.c: remove device_legacy_reset call hw/ppc/spapr: remove device_legacy_reset call hw/i386/pc.c: remove device_legacy_reset call hw/s390x/s390-pci-inst.c: remove device_legacy_reset call hw/ide/microdrive.c: remove device_legacy_reset calls qdev: Remove unused deprecated reset funct
[Qemu-block] [PATCH v3 14/33] hw/s390x/s390-virtio-ccw.c: remove qdev_reset_all call
Replace deprecated qdev_reset_all by device_reset_warm.
This does not impact the behavior.
Signed-off-by: Damien Hedde
---
hw/s390x/s390-virtio-ccw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 5b6a9a4e55..1d6b966817 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -104,7 +104,7 @@ static void subsystem_reset(void)
for (i = 0; i < ARRAY_SIZE(reset_dev_types); i++) {
dev = DEVICE(object_resolve_path_type("", reset_dev_types[i], NULL));
if (dev) {
-qdev_reset_all(dev);
+device_reset_warm(dev);
}
}
}
--
2.22.0
[Qemu-block] [PATCH v3 27/33] hw/ide/microdrive.c: remove device_legacy_reset calls
Replace MicroDriveState legacy reset by device_reset_warm.
The new function propagates also the reset to the sub-buses tree.
The MicroDriveState has a child bus so it is now reset automatically
as well as all the qdev sub tree. It seems to me that IDE_BUS and
IDE_DEVICEs reset methods are not implemented so resetting the
qdev/qbus ide tree will have no effect.
Keep the explicit call to ide_bus_reset (in md_reset function) since
it is not called when using the standard reset method of the IDE_BUS
object.
Signed-off-by: Damien Hedde
---
hw/ide/microdrive.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index fc346f5ad5..afe2342da8 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -173,7 +173,7 @@ static void md_attr_write(PCMCIACardState *card, uint32_t
at, uint8_t value)
case 0x00: /* Configuration Option Register */
s->opt = value & 0xcf;
if (value & OPT_SRESET) {
-device_legacy_reset(DEVICE(s));
+device_reset_warm(DEVICE(s));
}
md_interrupt_update(s);
break;
@@ -316,7 +316,7 @@ static void md_common_write(PCMCIACardState *card, uint32_t
at, uint16_t value)
case 0xe: /* Device Control */
s->ctrl = value;
if (value & CTRL_SRST) {
-device_legacy_reset(DEVICE(s));
+device_reset_warm(DEVICE(s));
}
md_interrupt_update(s);
break;
@@ -541,7 +541,7 @@ static int dscm1_attach(PCMCIACardState *card)
md->attr_base = pcc->cis[0x74] | (pcc->cis[0x76] << 8);
md->io_base = 0x0;
-device_legacy_reset(DEVICE(md));
+device_reset_warm(DEVICE(md));
md_interrupt_update(md);
return 0;
@@ -551,7 +551,7 @@ static int dscm1_detach(PCMCIACardState *card)
{
MicroDriveState *md = MICRODRIVE(card);
-device_legacy_reset(DEVICE(md));
+device_reset_warm(DEVICE(md));
return 0;
}
--
2.22.0
[Qemu-block] [PATCH v3 07/33] automatically add vmstate for reset support in devices
This add the reset related sections for every QOM
device.
Signed-off-by: Damien Hedde
---
hw/core/qdev-vmstate.c | 41 +
hw/core/qdev.c | 12 +++-
include/hw/qdev-core.h | 3 +++
stubs/Makefile.objs| 1 +
stubs/device.c | 7 +++
5 files changed, 63 insertions(+), 1 deletion(-)
create mode 100644 stubs/device.c
diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
index 07b010811f..24f8465c61 100644
--- a/hw/core/qdev-vmstate.c
+++ b/hw/core/qdev-vmstate.c
@@ -43,3 +43,44 @@ const struct VMStateDescription device_vmstate_reset = {
VMSTATE_END_OF_LIST()
},
};
+
+static VMStateDescription *vmsd_duplicate_and_append(
+const VMStateDescription *old_vmsd,
+const VMStateDescription *new_subsection)
+{
+VMStateDescription *vmsd;
+int n = 0;
+
+assert(old_vmsd && new_subsection);
+
+vmsd = (VMStateDescription *) g_memdup(old_vmsd, sizeof(*vmsd));
+
+if (old_vmsd->subsections) {
+while (old_vmsd->subsections[n]) {
+n += 1;
+}
+}
+vmsd->subsections = g_new(const VMStateDescription *, n + 2);
+
+if (old_vmsd->subsections) {
+memcpy(vmsd->subsections, old_vmsd->subsections,
+ sizeof(VMStateDescription *) * n);
+}
+vmsd->subsections[n] = new_subsection;
+vmsd->subsections[n + 1] = NULL;
+
+return vmsd;
+}
+
+void device_class_build_extended_vmsd(DeviceClass *dc)
+{
+assert(dc->vmsd);
+assert(!dc->vmsd_ext);
+
+/* forge a subsection with proper name */
+VMStateDescription *reset;
+reset = g_memdup(_vmstate_reset, sizeof(*reset));
+reset->name = g_strdup_printf("%s/device_reset", dc->vmsd->name);
+
+dc->vmsd_ext = vmsd_duplicate_and_append(dc->vmsd, reset);
+}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e9e5f2d5f9..88387d3743 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -45,7 +45,17 @@ bool qdev_hot_removed = false;
const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
{
DeviceClass *dc = DEVICE_GET_CLASS(dev);
-return dc->vmsd;
+
+if (!dc->vmsd) {
+return NULL;
+}
+
+if (!dc->vmsd_ext) {
+/* build it first time we need it */
+device_class_build_extended_vmsd(dc);
+}
+
+return dc->vmsd_ext;
}
static void bus_remove_child(BusState *bus, DeviceState *child)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1670ae41bb..926d4bbcb1 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -120,6 +120,7 @@ typedef struct DeviceClass {
/* device state */
const struct VMStateDescription *vmsd;
+const struct VMStateDescription *vmsd_ext;
/* Private to qdev / bus. */
const char *bus_type;
@@ -520,6 +521,8 @@ void device_class_set_parent_unrealize(DeviceClass *dc,
const struct VMStateDescription *qdev_get_vmsd(DeviceState *dev);
+void device_class_build_extended_vmsd(DeviceClass *dc);
+
const char *qdev_fw_name(DeviceState *dev);
Object *qdev_get_machine(void);
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 9c7393b08c..432b56f290 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -40,4 +40,5 @@ stub-obj-y += pci-host-piix.o
stub-obj-y += ram-block.o
stub-obj-y += ramfb.o
stub-obj-y += fw_cfg.o
+stub-obj-y += device.o
stub-obj-$(CONFIG_SOFTMMU) += semihost.o
diff --git a/stubs/device.c b/stubs/device.c
new file mode 100644
index 00..e9b4f57e5f
--- /dev/null
+++ b/stubs/device.c
@@ -0,0 +1,7 @@
+#include "qemu/osdep.h"
+#include "hw/qdev-core.h"
+
+void device_class_build_extended_vmsd(DeviceClass *dc)
+{
+return;
+}
--
2.22.0
[Qemu-block] [PATCH v3 06/33] add the vmstate description for device reset state
It contains the resetting counter and cold flag status.
At this point, migration of bus reset related state (counter and cold/warm
flag) is handled by parent device. This done using the post_load
function in the vmsd subsection.
This is last point allow to add an initial support of migration with part of
qdev/qbus tree in reset state under the following condition:
+ time-lasting reset are asserted on Device only
Note that if this condition is not respected, migration will succeed and
no failure will occurs. The only impact is that the resetting counter
of a bus may lower afer a migration.
Signed-off-by: Damien Hedde
---
hw/core/Makefile.objs | 1 +
hw/core/qdev-vmstate.c | 45 ++
2 files changed, 46 insertions(+)
create mode 100644 hw/core/qdev-vmstate.c
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index d9234aa98a..49e9be0228 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -4,6 +4,7 @@ common-obj-y += bus.o reset.o
common-obj-y += resettable.o
common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
+common-obj-$(CONFIG_SOFTMMU) += qdev-vmstate.o
# irq.o needed for qdev GPIO handling:
common-obj-y += irq.o
common-obj-y += hotplug.o
diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
new file mode 100644
index 00..07b010811f
--- /dev/null
+++ b/hw/core/qdev-vmstate.c
@@ -0,0 +1,45 @@
+/*
+ * Device vmstate
+ *
+ * Copyright (c) 2019 GreenSocs
+ *
+ * Authors:
+ * Damien Hedde
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev.h"
+#include "migration/vmstate.h"
+
+static bool device_vmstate_reset_needed(void *opaque)
+{
+DeviceState *dev = (DeviceState *) opaque;
+return dev->resetting != 0;
+}
+
+static int device_vmstate_reset_post_load(void *opaque, int version_id)
+{
+DeviceState *dev = (DeviceState *) opaque;
+BusState *bus;
+QLIST_FOREACH(bus, >child_bus, sibling) {
+bus->resetting = dev->resetting;
+bus->reset_is_cold = dev->reset_is_cold;
+}
+return 0;
+}
+
+const struct VMStateDescription device_vmstate_reset = {
+.name = "device_reset",
+.version_id = 0,
+.minimum_version_id = 0,
+.needed = device_vmstate_reset_needed,
+.post_load = device_vmstate_reset_post_load,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(resetting, DeviceState),
+VMSTATE_BOOL(reset_is_cold, DeviceState),
+VMSTATE_END_OF_LIST()
+},
+};
--
2.22.0
[Qemu-block] [PATCH v3 09/33] add doc about Resettable interface
Signed-off-by: Damien Hedde
---
docs/devel/reset.txt | 165 +++
1 file changed, 165 insertions(+)
create mode 100644 docs/devel/reset.txt
diff --git a/docs/devel/reset.txt b/docs/devel/reset.txt
new file mode 100644
index 00..c7a1eb068f
--- /dev/null
+++ b/docs/devel/reset.txt
@@ -0,0 +1,165 @@
+
+=
+Reset
+=
+
+The reset of qemu objects is handled using the Resettable interface declared
+in *include/hw/resettable.h*.
+As of now DeviceClass and BusClass implement this interface.
+
+
+Triggering reset
+
+
+The function *resettable_reset* is used to trigger a reset on a given
+object.
+void resettable_reset(Object *obj, bool cold)
+
+The parameter *obj* must implement the Resettable interface.
+The parameter *cold* is a boolean specifying whether to do a cold or warm
+reset
+
+For Devices and Buses there is also the corresponding helpers:
+void device_reset(Device *dev, bool cold)
+void bus_reset(Device *dev, bool cold)
+
+If one wants to put an object into a reset state. There is the
+*resettable_assert_reset* function.
+void resettable_assert_reset(Object *obj, bool cold)
+
+One must eventually call the function *resettable_deassert_reset* to end the
+reset state:
+void resettable_deassert_reset(Object *obj, bool cold)
+
+Calling *resettable_assert_reset* then *resettable_deassert_reset* is the
+same as calling *resettable_reset*.
+
+It is possible to interleave multiple calls to
+ - resettable_reset,
+ - resettable_assert_reset, and
+ - resettable_deassert_reset.
+The only constraint is that *resettable_deassert_reset* must be called once
+per *resettable_assert_reset* call so that the object leaves the reset state.
+
+Therefore there may be several reset sources/controllers of a given object.
+The interface handle everything and the controllers do not need to know
+anything about each others. The object will leave reset state only when all
+controllers released their reset.
+
+All theses functions must called while holding the iothread lock.
+
+
+Implementing reset for a Resettable object : Multi-phase reset
+--
+
+The Resettable uses a multi-phase mechanism to handle some ordering constraints
+when resetting multiple object at the same time. For a given object the reset
+procedure is split into three different phases executed in order:
+ 1 INIT: This phase should set/reset the state of the Resettable it has when is
+ in reset state. Side-effects to others object is forbidden (such as
+ setting IO level).
+ 2 HOLD: This phase corresponds to the external side-effects due to staying
into
+ the reset state.
+ 3 EXIT: This phase corresponds to leaving the reset state. It have both
+ local and external effects.
+
+*resettable_assert_reset* does the INIT and HOLD phases. While
+*resettable_deassert_reset* does the EXIT phase.
+
+When resetting multiple object at the same time. The interface executes the
+given phase of the objects before going to the next phase. This guarantee that
+all INIT phases are done before any HOLD phase and so on.
+
+There is three methods in the interface so must be implemented in an object.
+The methods corresponds to the three phases:
+```
+typedef void (*ResettableInitPhase)(Object *obj);
+typedef void (*ResettableHoldPhase)(Object *obj);
+typedef void (*ResettableExitPhase)(Object *obj);
+typedef struct ResettableClass {
+InterfaceClass parent_class;
+
+struct ResettablePhases {
+ResettableInitPhase init;
+ResettableHoldPhase hold;
+ResettableExitPhase exit;
+} phases;
+[...]
+} ResettableClass;
+```
+
+Theses methods should be updated when specializing an object. For this the
+helper function *resettable_class_set_parent_reset_phases* can be used to
+backup parent methods while changing the specialized ones.
+void resettable_class_set_parent_reset_phases(ResettableClass *rc,
+ ResettableInitPhase init,
+ ResettableHoldPhase hold,
+ ResettableExitPhase exit,
+
+For Devices and Buses, some helper exists to know if a device/bus is under
+reset and what type of reset it is:
+```
+bool device_is_resetting(DeviceState *dev);
+bool device_is_reset_cold(DeviceState *dev);
+bool bus_is_resetting(BusState *bus);
+bool bus_is_reset_cold(BusState *bus);
+```
+
+
+Implementing the base Resettable behavior : Re-entrance, Hierarchy and
Cold/Warm
+
+
+There is five others methods in the interface to handle the base mechanics
+of the Resettable interface. The methods should be implemented in object
+base class. DeviceClass and BusClass implement them.
+
+```
+typedef bool (*ResettableSetCold)(Object *obj, bool cold);
+typedef bool (*ResettableSetHoldNeeded)(Object *obj, bool
[Qemu-block] [PATCH v3 20/33] hw/hyperv/hyperv.c: remove device_legacy_reset call
Replace legacy's reset call by device_reset_warm in
*hyperv_synic_reset*.
The new function propagates also the reset to the sub-buses tree but this has
no impact since SynICState has no child bus.
Signed-off-by: Damien Hedde
---
hw/hyperv/hyperv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index cd9db3cb5c..ae377934ee 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -140,7 +140,7 @@ void hyperv_synic_reset(CPUState *cs)
SynICState *synic = get_synic(cs);
if (synic) {
-device_legacy_reset(DEVICE(synic));
+device_reset_warm(DEVICE(synic));
}
}
--
2.22.0
[Qemu-block] [PATCH v3 10/33] vl.c: remove qbus_reset_all registration
Replace deprecated qbus_reset_all by resettable_reset_cold_fn for
the sysbus reset registration.
This does not impact the behavior.
Signed-off-by: Damien Hedde
---
vl.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/vl.c b/vl.c
index b426b32134..5a465c8236 100644
--- a/vl.c
+++ b/vl.c
@@ -4421,7 +4421,11 @@ int main(int argc, char **argv, char **envp)
/* TODO: once all bus devices are qdevified, this should be done
* when bus is created by qdev.c */
-qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
+/*
+ * TODO: when we have a main reset container/domain object, use
+ * it to fully benefit from multi-phase reset
+ */
+qemu_register_reset(resettable_reset_cold_fn, sysbus_get_default());
qemu_run_machine_init_done_notifiers();
if (rom_check_and_register_reset() != 0) {
--
2.22.0
[Qemu-block] [PATCH v3 32/33] Add uart reset support in zynq_slcr
Add two gpio outputs to control the uart resets.
Signed-off-by: Damien Hedde
---
hw/misc/zynq_slcr.c | 36 +++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index 6fcdbce4f0..b6c9a281c2 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -97,6 +97,10 @@ REG32(SPI_RST_CTRL, 0x21c)
REG32(CAN_RST_CTRL, 0x220)
REG32(I2C_RST_CTRL, 0x224)
REG32(UART_RST_CTRL, 0x228)
+FIELD(UART_RST_CTRL, UART0_CPU1X_RST, 0, 1)
+FIELD(UART_RST_CTRL, UART1_CPU1X_RST, 1, 1)
+FIELD(UART_RST_CTRL, UART0_REF_RST, 2, 1)
+FIELD(UART_RST_CTRL, UART1_REF_RST, 3, 1)
REG32(GPIO_RST_CTRL, 0x22c)
REG32(LQSPI_RST_CTRL, 0x230)
REG32(SMC_RST_CTRL, 0x234)
@@ -190,8 +194,14 @@ typedef struct ZynqSLCRState {
MemoryRegion iomem;
uint32_t regs[ZYNQ_SLCR_NUM_REGS];
+
+qemu_irq uart0_rst;
+qemu_irq uart1_rst;
} ZynqSLCRState;
+#define ZYNQ_SLCR_REGFIELD_TO_OUT(state, irq, reg, field) \
+qemu_set_irq((state)->irq, ARRAY_FIELD_EX32((state)->regs, reg, field) !=
0)
+
static void zynq_slcr_reset_init(Object *obj)
{
ZynqSLCRState *s = ZYNQ_SLCR(obj);
@@ -293,6 +303,24 @@ static void zynq_slcr_reset_init(Object *obj)
s->regs[R_DDRIOB + 12] = 0x0021;
}
+static void zynq_slcr_compute_uart_reset(ZynqSLCRState *s)
+{
+ZYNQ_SLCR_REGFIELD_TO_OUT(s, uart0_rst, UART_RST_CTRL, UART0_REF_RST);
+ZYNQ_SLCR_REGFIELD_TO_OUT(s, uart1_rst, UART_RST_CTRL, UART1_REF_RST);
+}
+
+static void zynq_slcr_reset_hold(Object *obj)
+{
+ZynqSLCRState *s = ZYNQ_SLCR(obj);
+ZynqSLCRClass *zc = ZYNQ_SLCR_GET_CLASS(obj);
+
+if (zc->parent_reset_phases.hold) {
+zc->parent_reset_phases.hold(obj);
+}
+
+zynq_slcr_compute_uart_reset(s);
+}
+
static bool zynq_slcr_check_offset(hwaddr offset, bool rnw)
{
switch (offset) {
@@ -432,6 +460,9 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
}
break;
+case R_UART_RST_CTRL:
+zynq_slcr_compute_uart_reset(s);
+break;
}
}
@@ -448,6 +479,9 @@ static void zynq_slcr_init(Object *obj)
memory_region_init_io(>iomem, obj, _ops, s, "slcr",
ZYNQ_SLCR_MMIO_SIZE);
sysbus_init_mmio(SYS_BUS_DEVICE(obj), >iomem);
+
+qdev_init_gpio_out_named(DEVICE(obj), >uart0_rst, "uart0_rst", 1);
+qdev_init_gpio_out_named(DEVICE(obj), >uart1_rst, "uart1_rst", 1);
}
static const VMStateDescription vmstate_zynq_slcr = {
@@ -470,7 +504,7 @@ static void zynq_slcr_class_init(ObjectClass *klass, void
*data)
resettable_class_set_parent_reset_phases(rc,
zynq_slcr_reset_init,
- NULL,
+ zynq_slcr_reset_hold,
NULL,
>parent_reset_phases);
}
--
2.22.0
[Qemu-block] [PATCH v3 01/33] Create Resettable QOM interface
This commit defines an interface allowing multi-phase reset.
The phases are INIT, HOLD and EXIT. Each phase has an associated method
in the class.
The reset of a Resettable is controlled with 2 functions:
- resettable_assert_reset which starts the reset operation.
- resettable_deassert_reset which ends the reset operation.
There is also a `resettable_reset` helper function which does assert then
deassert allowing to do a proper reset in one call.
In addition, two functions, *resettable_reset_cold_fn* and
*resettable_reset_warm_fn*, are defined. They take only an opaque argument
(for the Resettable object) and can be used as callbacks.
The Resettable interface is "reentrant", _assert_ can be called several
times and _deasert_ must be called the same number of times so that the
Resettable leave reset state. It is supported by keeping a counter of
the current number of _assert_ calls. The counter maintainance is done
though 3 methods get/increment/decrement_count. A method set_cold is used
to set the cold flag of the reset. Another method set_host_needed is used
to ensure hold phase is executed only if required.
Reset hierarchy is also supported. Each Resettable may have
sub-Resettable objects. When resetting a Resettable, it is propagated to
its children using the foreach_child method.
When entering reset, init phases are executed parent-to-child order then
hold phases are executed child-parent order.
When leaving reset, exit phases are executed in child-to-parent order.
This will allow to replace current qdev_reset mechanism by this interface
without side-effects on reset order.
Note: I used an uint32 for the count. This match the type already used
in the existing resetting counter in hw/scsi/vmw_pvscsi.c for the
PVSCSIState.
Signed-off-by: Damien Hedde
---
Makefile.objs | 1 +
hw/core/Makefile.objs | 1 +
hw/core/resettable.c| 220
hw/core/trace-events| 39 +++
include/hw/resettable.h | 126 +++
5 files changed, 387 insertions(+)
create mode 100644 hw/core/resettable.c
create mode 100644 hw/core/trace-events
create mode 100644 include/hw/resettable.h
diff --git a/Makefile.objs b/Makefile.objs
index 6a143dcd57..a723a47e14 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -191,6 +191,7 @@ trace-events-subdirs += migration
trace-events-subdirs += net
trace-events-subdirs += ui
endif
+trace-events-subdirs += hw/core
trace-events-subdirs += hw/display
trace-events-subdirs += qapi
trace-events-subdirs += qom
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index f8481d959f..d9234aa98a 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -1,6 +1,7 @@
# core qdev-related obj files, also used by *-user:
common-obj-y += qdev.o qdev-properties.o
common-obj-y += bus.o reset.o
+common-obj-y += resettable.o
common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
# irq.o needed for qdev GPIO handling:
diff --git a/hw/core/resettable.c b/hw/core/resettable.c
new file mode 100644
index 00..d7d631ce8b
--- /dev/null
+++ b/hw/core/resettable.c
@@ -0,0 +1,220 @@
+/*
+ * Resettable interface.
+ *
+ * Copyright (c) 2019 GreenSocs SAS
+ *
+ * Authors:
+ * Damien Hedde
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "hw/resettable.h"
+#include "trace.h"
+
+#define RESETTABLE_MAX_COUNT 50
+
+#define RESETTABLE_GET_CLASS(obj) \
+OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE)
+
+static void resettable_init_reset(Object *obj, bool cold);
+
+static bool resettable_class_check(ResettableClass *rc)
+{
+if (!rc->set_cold) {
+return false;
+}
+if (!rc->set_hold_needed) {
+return false;
+}
+if (!rc->increment_count) {
+return false;
+}
+if (!rc->decrement_count) {
+return false;
+}
+if (!rc->get_count) {
+return false;
+}
+return true;
+}
+
+static void resettable_foreach_child(ResettableClass *rc,
+ Object *obj,
+ void (*func)(Object *))
+{
+if (rc->foreach_child) {
+rc->foreach_child(obj, func);
+}
+}
+
+static void resettable_init_cold_reset(Object *obj)
+{
+resettable_init_reset(obj, true);
+}
+
+static void resettable_init_warm_reset(Object *obj)
+{
+resettable_init_reset(obj, false);
+}
+
+static void resettable_init_reset(Object *obj, bool cold)
+{
+void (*func)(Object *);
+ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+uint32_t count;
+bool action_needed = false;
+bool prev_cold;
+
+assert(resettable_class_check(rc));
+
+count = rc->increment_count(obj);
+/* this assert is here to
[Qemu-block] [PATCH v3 12/33] hw/pci/: remove qdev/qbus_reset_all call
Replace deprecated qdev/bus_reset_all by device/bus_reset_warm.
This does not impact the behavior.
Signed-off-by: Damien Hedde
---
hw/pci/pci.c| 6 +++---
hw/pci/pci_bridge.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8076a80ab3..f2b9d37754 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -325,14 +325,14 @@ static void pci_do_device_reset(PCIDevice *dev)
*/
void pci_device_reset(PCIDevice *dev)
{
-qdev_reset_all(>qdev);
+device_reset_warm(>qdev);
pci_do_device_reset(dev);
}
/*
* Trigger pci bus reset under a given bus.
- * Called via qbus_reset_all on RST# assert, after the devices
- * have been reset qdev_reset_all-ed already.
+ * Called via bus_reset on RST# assert, after the devices
+ * have been reset device_reset-ed already.
*/
static void pcibus_reset(BusState *qbus)
{
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 715b9a4fe6..695242149f 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -274,7 +274,7 @@ void pci_bridge_write_config(PCIDevice *d,
newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
/* Trigger hot reset on 0->1 transition. */
-qbus_reset_all(BUS(>sec_bus));
+bus_reset_warm(BUS(>sec_bus));
}
}
--
2.22.0
[Qemu-block] [PATCH v3 03/33] Replace all call to device_reset by call to device_legacy_reset
Signed-off-by: Damien Hedde
---
hw/audio/intel-hda.c | 2 +-
hw/hyperv/hyperv.c | 2 +-
hw/i386/pc.c | 2 +-
hw/ide/microdrive.c | 8
hw/intc/spapr_xive.c | 2 +-
hw/ppc/pnv_psi.c | 2 +-
hw/ppc/spapr_pci.c | 2 +-
hw/ppc/spapr_vio.c | 2 +-
hw/s390x/s390-pci-inst.c | 2 +-
hw/scsi/vmw_pvscsi.c | 2 +-
hw/sd/omap_mmc.c | 2 +-
hw/sd/pl181.c| 2 +-
12 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index b78baac295..f133684b10 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1086,7 +1086,7 @@ static void intel_hda_reset(DeviceState *dev)
QTAILQ_FOREACH(kid, >codecs.qbus.children, sibling) {
DeviceState *qdev = kid->child;
cdev = HDA_CODEC_DEVICE(qdev);
-device_reset(DEVICE(cdev));
+device_legacy_reset(DEVICE(cdev));
d->state_sts |= (1 << cdev->cad);
}
intel_hda_update_irq(d);
diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index 6ebf31c310..cd9db3cb5c 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -140,7 +140,7 @@ void hyperv_synic_reset(CPUState *cs)
SynICState *synic = get_synic(cs);
if (synic) {
-device_reset(DEVICE(synic));
+device_legacy_reset(DEVICE(synic));
}
}
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 549c437050..c0f20fe8aa 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2823,7 +2823,7 @@ static void pc_machine_reset(MachineState *machine)
cpu = X86_CPU(cs);
if (cpu->apic_state) {
-device_reset(cpu->apic_state);
+device_legacy_reset(cpu->apic_state);
}
}
}
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index 92ee6e0af8..fc346f5ad5 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -173,7 +173,7 @@ static void md_attr_write(PCMCIACardState *card, uint32_t
at, uint8_t value)
case 0x00: /* Configuration Option Register */
s->opt = value & 0xcf;
if (value & OPT_SRESET) {
-device_reset(DEVICE(s));
+device_legacy_reset(DEVICE(s));
}
md_interrupt_update(s);
break;
@@ -316,7 +316,7 @@ static void md_common_write(PCMCIACardState *card, uint32_t
at, uint16_t value)
case 0xe: /* Device Control */
s->ctrl = value;
if (value & CTRL_SRST) {
-device_reset(DEVICE(s));
+device_legacy_reset(DEVICE(s));
}
md_interrupt_update(s);
break;
@@ -541,7 +541,7 @@ static int dscm1_attach(PCMCIACardState *card)
md->attr_base = pcc->cis[0x74] | (pcc->cis[0x76] << 8);
md->io_base = 0x0;
-device_reset(DEVICE(md));
+device_legacy_reset(DEVICE(md));
md_interrupt_update(md);
return 0;
@@ -551,7 +551,7 @@ static int dscm1_detach(PCMCIACardState *card)
{
MicroDriveState *md = MICRODRIVE(card);
-device_reset(DEVICE(md));
+device_legacy_reset(DEVICE(md));
return 0;
}
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 3ae311d9ff..22e11ad10c 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -1511,7 +1511,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu,
return H_PARAMETER;
}
-device_reset(DEVICE(xive));
+device_legacy_reset(DEVICE(xive));
if (kvm_irqchip_in_kernel()) {
Error *local_err = NULL;
diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index d7b6f5d75b..78eafa353a 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -703,7 +703,7 @@ static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
break;
case PSIHB9_INTERRUPT_CONTROL:
if (val & PSIHB9_IRQ_RESET) {
-device_reset(DEVICE(>source));
+device_legacy_reset(DEVICE(>source));
}
psi->regs[reg] = val;
break;
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 9003fe9010..3c6cf79a5e 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2029,7 +2029,7 @@ static int spapr_phb_children_reset(Object *child, void
*opaque)
DeviceState *dev = (DeviceState *) object_dynamic_cast(child, TYPE_DEVICE);
if (dev) {
-device_reset(dev);
+device_legacy_reset(dev);
}
return 0;
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 583c13deda..5a0b5cc35c 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -306,7 +306,7 @@ int spapr_vio_send_crq(SpaprVioDevice *dev, uint8_t *crq)
static void spapr_vio_quiesce_one(SpaprVioDevice *dev)
{
if (dev->tcet) {
-device_reset(DEVICE(dev->tcet));
+device_legacy_reset(DEVICE(dev->tcet));
}
free_crq(dev);
}
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 00235148be..93cda37c27 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-i
[Qemu-block] [PATCH v3 16/33] hw/input/adb.c: remove qdev_reset_all call
Replace deprecated qdev_reset_all by device_reset_warm.
This does not impact the behavior.
Signed-off-by: Damien Hedde
---
hw/input/adb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/input/adb.c b/hw/input/adb.c
index 1446f32521..6b35682aba 100644
--- a/hw/input/adb.c
+++ b/hw/input/adb.c
@@ -32,7 +32,7 @@
static void adb_device_reset(ADBDevice *d)
{
-qdev_reset_all(DEVICE(d));
+device_reset_warm(DEVICE(d));
}
int adb_request(ADBBusState *s, uint8_t *obuf, const uint8_t *buf, int len)
--
2.22.0
[Qemu-block] [PATCH v3 08/33] Add function to control reset with gpio inputs
It adds the possibility to add 2 gpios to control the warm and cold reset.
With theses ios, the reset can be maintained during some time.
Each io is associated with a state to detect level changes.
Vmstate subsections are also added to the existsing device_reset
subsection.
Signed-off-by: Damien Hedde
---
hw/core/qdev-vmstate.c | 15 ++
hw/core/qdev.c | 65 ++
include/hw/qdev-core.h | 57
3 files changed, 137 insertions(+)
diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
index 24f8465c61..72f84c6cee 100644
--- a/hw/core/qdev-vmstate.c
+++ b/hw/core/qdev-vmstate.c
@@ -24,10 +24,23 @@ static int device_vmstate_reset_post_load(void *opaque, int
version_id)
{
DeviceState *dev = (DeviceState *) opaque;
BusState *bus;
+uint32_t io_count = 0;
+
QLIST_FOREACH(bus, >child_bus, sibling) {
bus->resetting = dev->resetting;
bus->reset_is_cold = dev->reset_is_cold;
}
+
+if (dev->cold_reset_input.state) {
+io_count += 1;
+}
+if (dev->warm_reset_input.state) {
+io_count += 1;
+}
+/* ensure resetting count is coherent with io states */
+if (dev->resetting < io_count) {
+return -1;
+}
return 0;
}
@@ -40,6 +53,8 @@ const struct VMStateDescription device_vmstate_reset = {
.fields = (VMStateField[]) {
VMSTATE_UINT32(resetting, DeviceState),
VMSTATE_BOOL(reset_is_cold, DeviceState),
+VMSTATE_BOOL(cold_reset_input.state, DeviceState),
+VMSTATE_BOOL(warm_reset_input.state, DeviceState),
VMSTATE_END_OF_LIST()
},
};
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 88387d3743..11a4de55ea 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -450,6 +450,67 @@ void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler
handler, int n)
qdev_init_gpio_in_named(dev, handler, NULL, n);
}
+static DeviceResetInputState *device_get_reset_input_state(DeviceState *dev,
+bool cold)
+{
+return cold ? >cold_reset_input : >warm_reset_input;
+}
+
+static void device_reset_handler(DeviceState *dev, bool cold, bool level)
+{
+DeviceResetInputState *dris = device_get_reset_input_state(dev, cold);
+
+if (dris->type == DEVICE_RESET_ACTIVE_LOW) {
+level = !level;
+}
+
+if (dris->state == level) {
+/* io state has not changed */
+return;
+}
+
+dris->state = level;
+
+if (level) {
+resettable_assert_reset(OBJECT(dev), cold);
+} else {
+resettable_deassert_reset(OBJECT(dev));
+}
+}
+
+static void device_cold_reset_handler(void *opaque, int n, int level)
+{
+device_reset_handler((DeviceState *) opaque, true, level);
+}
+
+static void device_warm_reset_handler(void *opaque, int n, int level)
+{
+device_reset_handler((DeviceState *) opaque, false, level);
+}
+
+void qdev_init_reset_gpio_in_named(DeviceState *dev, const char *name,
+ bool cold, DeviceResetActiveType type)
+{
+DeviceResetInputState *dris = device_get_reset_input_state(dev, cold);
+qemu_irq_handler handler;
+
+switch (type) {
+case DEVICE_RESET_ACTIVE_LOW:
+case DEVICE_RESET_ACTIVE_HIGH:
+break;
+default:
+assert(false);
+break;
+}
+
+assert(!dris->exists);
+dris->exists = true;
+dris->type = type;
+
+handler = cold ? device_cold_reset_handler : device_warm_reset_handler;
+qdev_init_gpio_in_named(dev, handler, name, 1);
+}
+
void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
const char *name, int n)
{
@@ -1007,6 +1068,10 @@ static void device_initfn(Object *obj)
dev->instance_id_alias = -1;
dev->realized = false;
dev->resetting = 0;
+dev->cold_reset_input.exists = false;
+dev->cold_reset_input.state = false;
+dev->warm_reset_input.exists = false;
+dev->warm_reset_input.state = false;
object_property_add_bool(obj, "realized",
device_get_realized, device_set_realized, NULL);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 926d4bbcb1..f724ddc8f4 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -136,6 +136,23 @@ struct NamedGPIOList {
QLIST_ENTRY(NamedGPIOList) node;
};
+typedef enum DeviceResetActiveType {
+DEVICE_RESET_ACTIVE_LOW,
+DEVICE_RESET_ACTIVE_HIGH,
+} DeviceResetActiveType;
+
+/**
+ * DeviceResetInputState:
+ * @exists: tell if io exists
+ * @type: tell whether the io active low or high
+ * @state: true if reset is currently active
+ */
+typedef struct DeviceResetInputState {
+bool exists;
+DeviceResetActiveType type;
+bool state;
+} DeviceResetInputState;
+
/**
* DeviceState:
* @realized: Indicates wheth
[Qemu-block] [PATCH v3 05/33] Switch to new api in qdev/bus
Deprecate old reset apis and make them use the new one while they
are still used somewhere.
Signed-off-by: Damien Hedde
---
hw/core/qdev.c | 22 +++---
include/hw/qdev-core.h | 28 ++--
2 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 559ced070d..e9e5f2d5f9 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, void
(*func)(Object *))
}
}
-static int qdev_reset_one(DeviceState *dev, void *opaque)
-{
-device_legacy_reset(dev);
-
-return 0;
-}
-
-static int qbus_reset_one(BusState *bus, void *opaque)
-{
-BusClass *bc = BUS_GET_CLASS(bus);
-if (bc->reset) {
-bc->reset(bus);
-}
-return 0;
-}
-
void qdev_reset_all(DeviceState *dev)
{
-qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
+device_reset(dev, false);
}
void qdev_reset_all_fn(void *opaque)
@@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque)
void qbus_reset_all(BusState *bus)
{
-qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
+bus_reset(bus, false);
}
void qbus_reset_all_fn(void *opaque)
@@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value,
Error **errp)
}
}
if (dev->hotplugged) {
-device_legacy_reset(dev);
+device_reset(dev, true);
}
dev->pending_deleted_event = false;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index eeb75611c8..1670ae41bb 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -109,6 +109,11 @@ typedef struct DeviceClass {
bool hotpluggable;
/* callbacks */
+/*
+ * Reset method here is deprecated and replaced by methods in the
+ * resettable class interface to implement a multi-phase reset.
+ * TODO: remove once every reset callback is unused
+ */
DeviceReset reset;
DeviceRealize realize;
DeviceUnrealize unrealize;
@@ -455,19 +460,22 @@ bool bus_is_resetting(BusState *bus);
*/
bool bus_is_reset_cold(BusState *bus);
-void qdev_reset_all(DeviceState *dev);
-void qdev_reset_all_fn(void *opaque);
-
/**
- * @qbus_reset_all:
- * @bus: Bus to be reset.
+ * qbus/qdev_reset_all:
+ * @bus/dev: Bus/Device to be reset.
*
- * Reset @bus and perform a bus-level ("hard") reset of all devices connected
+ * Reset @bus/dev and perform a bus-level reset of all devices/buses connected
* to it, including recursive processing of all buses below @bus itself. A
* hard reset means that qbus_reset_all will reset all state of the device.
* For PCI devices, for example, this will include the base address registers
* or configuration space.
+ *
+ * Theses functions are deprecated, please use device/bus_reset or
+ * resettable_reset_* instead
+ * TODO: remove them when all occurence are removed
*/
+void qdev_reset_all(DeviceState *dev);
+void qdev_reset_all_fn(void *opaque);
void qbus_reset_all(BusState *bus);
void qbus_reset_all_fn(void *opaque);
@@ -489,9 +497,17 @@ void qdev_machine_init(void);
* device_legacy_reset:
*
* Reset a single device (by calling the reset method).
+ *
+ * This function is deprecated, please use device_reset() instead.
+ * TODO: remove the function when all occurences are removed.
*/
void device_legacy_reset(DeviceState *dev);
+/**
+ * device_class_set_parent_reset:
+ * TODO: remove the function when DeviceClass's reset method
+ * is not used anymore.
+ */
void device_class_set_parent_reset(DeviceClass *dc,
DeviceReset dev_reset,
DeviceReset *parent_reset);
--
2.22.0
[Qemu-block] [PATCH v3 33/33] Connect the uart reset gpios in the zynq platform
Connect the two uart reset inputs to the slcr corresponding outputs.
Signed-off-by: Damien Hedde
---
hw/arm/xilinx_zynq.c | 14 --
include/hw/char/cadence_uart.h | 10 +-
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 89da34808b..bb56f1e03c 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -165,7 +165,7 @@ static void zynq_init(MachineState *machine)
MemoryRegion *address_space_mem = get_system_memory();
MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
-DeviceState *dev;
+DeviceState *dev, *slcr;
SysBusDevice *busdev;
qemu_irq pic[64];
int n;
@@ -210,9 +210,9 @@ static void zynq_init(MachineState *machine)
1, 0x0066, 0x0022, 0x, 0x, 0x0555, 0x2aa,
0);
-dev = qdev_create(NULL, "xilinx,zynq_slcr");
-qdev_init_nofail(dev);
-sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF800);
+slcr = qdev_create(NULL, "xilinx,zynq_slcr");
+qdev_init_nofail(slcr);
+sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF800);
dev = qdev_create(NULL, TYPE_A9MPCORE_PRIV);
qdev_prop_set_uint32(dev, "num-cpu", 1);
@@ -233,8 +233,10 @@ static void zynq_init(MachineState *machine)
sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[76-IRQ_OFFSET]);
-cadence_uart_create(0xE000, pic[59 - IRQ_OFFSET], serial_hd(0));
-cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
+cadence_uart_create(0xE000, pic[59 - IRQ_OFFSET], serial_hd(0),
+slcr, "uart0_rst", 0);
+cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1),
+slcr, "uart1_rst", 0);
sysbus_create_varargs("cadence_ttc", 0xF8001000,
pic[42-IRQ_OFFSET], pic[43-IRQ_OFFSET], pic[44-IRQ_OFFSET], NULL);
diff --git a/include/hw/char/cadence_uart.h b/include/hw/char/cadence_uart.h
index e1cf33e94c..c03c61a1f2 100644
--- a/include/hw/char/cadence_uart.h
+++ b/include/hw/char/cadence_uart.h
@@ -52,7 +52,10 @@ typedef struct {
static inline DeviceState *cadence_uart_create(hwaddr addr,
qemu_irq irq,
-Chardev *chr)
+Chardev *chr,
+DeviceState *rst_dev,
+const char *rst_name,
+int rst_n)
{
DeviceState *dev;
SysBusDevice *s;
@@ -64,6 +67,11 @@ static inline DeviceState *cadence_uart_create(hwaddr addr,
sysbus_mmio_map(s, 0, addr);
sysbus_connect_irq(s, 0, irq);
+if (rst_dev) {
+qdev_connect_gpio_out_named(rst_dev, rst_name, rst_n,
+qdev_get_gpio_in_named(dev, "rst", 0));
+}
+
return dev;
}
--
2.22.0
[Qemu-block] [PATCH v3 02/33] add temporary device_legacy_reset function to replace device_reset
Provide a temporary function doing what device_reset does to do the
transition with Resettable API which will trigger a prototype change
of device_reset.
Signed-off-by: Damien Hedde
---
hw/core/qdev.c | 6 +++---
include/hw/qdev-core.h | 9 +++--
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 94ebc0a4a1..043e058396 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -256,7 +256,7 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
static int qdev_reset_one(DeviceState *dev, void *opaque)
{
-device_reset(dev);
+device_legacy_reset(dev);
return 0;
}
@@ -864,7 +864,7 @@ static void device_set_realized(Object *obj, bool value,
Error **errp)
}
}
if (dev->hotplugged) {
-device_reset(dev);
+device_legacy_reset(dev);
}
dev->pending_deleted_event = false;
@@ -1086,7 +1086,7 @@ void device_class_set_parent_unrealize(DeviceClass *dc,
dc->unrealize = dev_unrealize;
}
-void device_reset(DeviceState *dev)
+void device_legacy_reset(DeviceState *dev)
{
DeviceClass *klass = DEVICE_GET_CLASS(dev);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e157fc4acd..690ce72433 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -407,11 +407,16 @@ char *qdev_get_own_fw_dev_path_from_handler(BusState
*bus, DeviceState *dev);
void qdev_machine_init(void);
/**
- * @device_reset
+ * device_legacy_reset:
*
* Reset a single device (by calling the reset method).
*/
-void device_reset(DeviceState *dev);
+void device_legacy_reset(DeviceState *dev);
+
+static inline void device_reset(DeviceState *dev)
+{
+device_legacy_reset(dev);
+}
void device_class_set_parent_reset(DeviceClass *dc,
DeviceReset dev_reset,
--
2.22.0
[Qemu-block] [PATCH v3 28/33] qdev: Remove unused deprecated reset functions
Remove the functions now they are unused:
+ device_legacy_reset
+ qdev_reset_all[_fn]
+ qbus_reset_all[_fn]
Signed-off-by: Damien Hedde
---
hw/core/qdev.c | 30 --
include/hw/qdev-core.h | 29 -
2 files changed, 59 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 11a4de55ea..896b55f7ba 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -322,27 +322,6 @@ static void device_foreach_reset_child(Object *obj, void
(*func)(Object *))
}
}
-void qdev_reset_all(DeviceState *dev)
-{
-device_reset(dev, false);
-}
-
-void qdev_reset_all_fn(void *opaque)
-{
-qdev_reset_all(DEVICE(opaque));
-}
-
-void qbus_reset_all(BusState *bus)
-{
-bus_reset(bus, false);
-}
-
-void qbus_reset_all_fn(void *opaque)
-{
-BusState *bus = opaque;
-qbus_reset_all(bus);
-}
-
/* can be used as ->unplug() callback for the simple cases */
void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
@@ -1223,15 +1202,6 @@ void device_class_set_parent_unrealize(DeviceClass *dc,
dc->unrealize = dev_unrealize;
}
-void device_legacy_reset(DeviceState *dev)
-{
-DeviceClass *klass = DEVICE_GET_CLASS(dev);
-
-if (klass->reset) {
-klass->reset(dev);
-}
-}
-
Object *qdev_get_machine(void)
{
static Object *dev;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index f724ddc8f4..eb6370970e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -518,25 +518,6 @@ bool bus_is_resetting(BusState *bus);
*/
bool bus_is_reset_cold(BusState *bus);
-/**
- * qbus/qdev_reset_all:
- * @bus/dev: Bus/Device to be reset.
- *
- * Reset @bus/dev and perform a bus-level reset of all devices/buses connected
- * to it, including recursive processing of all buses below @bus itself. A
- * hard reset means that qbus_reset_all will reset all state of the device.
- * For PCI devices, for example, this will include the base address registers
- * or configuration space.
- *
- * Theses functions are deprecated, please use device/bus_reset or
- * resettable_reset_* instead
- * TODO: remove them when all occurence are removed
- */
-void qdev_reset_all(DeviceState *dev);
-void qdev_reset_all_fn(void *opaque);
-void qbus_reset_all(BusState *bus);
-void qbus_reset_all_fn(void *opaque);
-
/* This should go away once we get rid of the NULL bus hack */
BusState *sysbus_get_default(void);
@@ -551,16 +532,6 @@ char *qdev_get_own_fw_dev_path_from_handler(BusState *bus,
DeviceState *dev);
*/
void qdev_machine_init(void);
-/**
- * device_legacy_reset:
- *
- * Reset a single device (by calling the reset method).
- *
- * This function is deprecated, please use device_reset() instead.
- * TODO: remove the function when all occurences are removed.
- */
-void device_legacy_reset(DeviceState *dev);
-
/**
* device_class_set_parent_reset:
* TODO: remove the function when DeviceClass's reset method
--
2.22.0
[Qemu-block] [PATCH v3 13/33] hw/scsi/: remove qdev/qbus_reset_all call
Replace deprecated qdev/bus_reset_all by device/bus_reset_warm.
This does not impact the behavior.
Signed-off-by: Damien Hedde
---
hw/scsi/lsi53c895a.c | 4 ++--
hw/scsi/megasas.c | 2 +-
hw/scsi/mptsas.c | 8
hw/scsi/spapr_vscsi.c | 2 +-
hw/scsi/virtio-scsi.c | 6 +++---
hw/scsi/vmw_pvscsi.c | 4 ++--
6 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 10468c1ec1..19197d1fc8 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -1861,7 +1861,7 @@ static void lsi_reg_writeb(LSIState *s, int offset,
uint8_t val)
}
if (val & LSI_SCNTL1_RST) {
if (!(s->sstat0 & LSI_SSTAT0_RST)) {
-qbus_reset_all(BUS(>bus));
+bus_reset_warm(BUS(>bus));
s->sstat0 |= LSI_SSTAT0_RST;
lsi_script_scsi_interrupt(s, LSI_SIST0_RST, 0);
}
@@ -1919,7 +1919,7 @@ static void lsi_reg_writeb(LSIState *s, int offset,
uint8_t val)
lsi_execute_script(s);
}
if (val & LSI_ISTAT0_SRST) {
-qdev_reset_all(DEVICE(s));
+device_reset_warm(DEVICE(s));
}
break;
case 0x16: /* MBOX0 */
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 0c4399930a..68c5538865 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1438,7 +1438,7 @@ static int megasas_cluster_reset_ld(MegasasState *s,
MegasasCmd *cmd)
MegasasCmd *tmp_cmd = >frames[i];
if (tmp_cmd->req && tmp_cmd->req->dev->id == target_id) {
SCSIDevice *d = tmp_cmd->req->dev;
-qdev_reset_all(>qdev);
+device_reset_warm(>qdev);
}
}
return MFI_STAT_OK;
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 3f94d5ab55..9ae1ebc0f3 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -519,7 +519,7 @@ reply_maybe_async:
reply.ResponseCode = MPI_SCSITASKMGMT_RSP_TM_INVALID_LUN;
goto out;
}
-qdev_reset_all(>qdev);
+device_reset_warm(>qdev);
break;
case MPI_SCSITASKMGMT_TASKTYPE_TARGET_RESET:
@@ -535,13 +535,13 @@ reply_maybe_async:
QTAILQ_FOREACH(kid, >bus.qbus.children, sibling) {
sdev = SCSI_DEVICE(kid->child);
if (sdev->channel == 0 && sdev->id == req->TargetID) {
-qdev_reset_all(kid->child);
+device_reset_warm(kid->child);
}
}
break;
case MPI_SCSITASKMGMT_TASKTYPE_RESET_BUS:
-qbus_reset_all(BUS(>bus));
+bus_reset_warm(BUS(>bus));
break;
default:
@@ -804,7 +804,7 @@ static void mptsas_soft_reset(MPTSASState *s)
s->intr_mask = MPI_HIM_DIM | MPI_HIM_RIM;
mptsas_update_interrupt(s);
-qbus_reset_all(BUS(>bus));
+bus_reset_warm(BUS(>bus));
s->intr_status = 0;
s->intr_mask = save_mask;
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index 0e491c911d..39c6067f48 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -855,7 +855,7 @@ static int vscsi_process_tsk_mgmt(VSCSIState *s, vscsi_req
*req)
break;
}
-qdev_reset_all(>qdev);
+device_reset_warm(>qdev);
break;
case SRP_TSK_ABORT_TASK_SET:
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 8b9e5e2b49..fcf9e3dbde 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -317,7 +317,7 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq
*req)
goto incorrect_lun;
}
s->resetting++;
-qdev_reset_all(>qdev);
+device_reset_warm(>qdev);
s->resetting--;
break;
@@ -367,7 +367,7 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq
*req)
QTAILQ_FOREACH(kid, >bus.qbus.children, sibling) {
d = SCSI_DEVICE(kid->child);
if (d->channel == 0 && d->id == target) {
-qdev_reset_all(>qdev);
+device_reset_warm(>qdev);
}
}
s->resetting--;
@@ -697,7 +697,7 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
assert(!s->dataplane_started);
s->resetting++;
-qbus_reset_all(BUS(>bus));
+bus_reset_warm(BUS(>bus));
s->resetting--;
vs->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index cda3fc96a0..40fcf808a7 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -441,7 +441,7 @@ static void
pvscsi_reset_adapter(PVSCSIState *s)
{
s->resetting++;
-qbus_reset_all(BUS(>bus));
+bus_reset_warm(BUS(>bus));
s->resetting--;
pvscsi_process_completion_queue(s);
assert(QTAILQ_E
[Qemu-block] [PATCH v3 04/33] make Device and Bus Resettable
This add Resettable interface implementation for both Bus and Device.
*resetting* counter and *reset_is_cold* flag are added in DeviceState
and BusState.
Compatibility with existing code base is ensured.
The legacy bus or device reset method is called in the new exit phase
and the other 2 phases are let empty. Using the exit phase guarantee that
legacy resets are called in the "post" order (ie: children then parent)
in hierarchical reset. That is the same order as legacy qdev_reset_all
or qbus_reset_all were using.
New *device_reset* and *bus_reset* function are proposed with an
additional boolean argument telling whether the reset is cold or warm.
Helper functions *device_reset_[warm|cold]* and *bus_reset_[warm|cold]*
are defined also as helpers.
Also add a [device|bus]_is_resetting and [device|bus]_is_reset_cold
functions telling respectively whether the object is currently under reset and
if the current reset is cold or not.
Signed-off-by: Damien Hedde
---
hw/core/bus.c | 85 ++
hw/core/qdev.c | 82
include/hw/qdev-core.h | 84 ++---
tests/Makefile.include | 1 +
4 files changed, 247 insertions(+), 5 deletions(-)
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 17bc1edcde..08a97addb6 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -22,6 +22,7 @@
#include "qemu/module.h"
#include "hw/qdev.h"
#include "qapi/error.h"
+#include "hw/resettable.h"
void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp)
{
@@ -68,6 +69,75 @@ int qbus_walk_children(BusState *bus,
return 0;
}
+void bus_reset(BusState *bus, bool cold)
+{
+resettable_reset(OBJECT(bus), cold);
+}
+
+bool bus_is_resetting(BusState *bus)
+{
+return (bus->resetting != 0);
+}
+
+bool bus_is_reset_cold(BusState *bus)
+{
+return bus->reset_is_cold;
+}
+
+static uint32_t bus_get_reset_count(Object *obj)
+{
+BusState *bus = BUS(obj);
+return bus->resetting;
+}
+
+static uint32_t bus_increment_reset_count(Object *obj)
+{
+BusState *bus = BUS(obj);
+return ++bus->resetting;
+}
+
+static uint32_t bus_decrement_reset_count(Object *obj)
+{
+BusState *bus = BUS(obj);
+return --bus->resetting;
+}
+
+static bool bus_set_reset_cold(Object *obj, bool cold)
+{
+BusState *bus = BUS(obj);
+bool old = bus->reset_is_cold;
+bus->reset_is_cold = cold;
+return old;
+}
+
+static bool bus_set_hold_needed(Object *obj, bool hold_needed)
+{
+BusState *bus = BUS(obj);
+bool old = bus->reset_hold_needed;
+bus->reset_hold_needed = hold_needed;
+return old;
+}
+
+static void bus_foreach_reset_child(Object *obj, void (*func)(Object *))
+{
+BusState *bus = BUS(obj);
+BusChild *kid;
+
+QTAILQ_FOREACH(kid, >children, sibling) {
+func(OBJECT(kid->child));
+}
+}
+
+static void bus_obj_legacy_reset(Object *obj)
+{
+BusState *bus = BUS(obj);
+BusClass *bc = BUS_GET_CLASS(obj);
+
+if (bc->reset) {
+bc->reset(bus);
+}
+}
+
static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
{
const char *typename = object_get_typename(OBJECT(bus));
@@ -192,6 +262,8 @@ static void qbus_initfn(Object *obj)
NULL);
object_property_add_bool(obj, "realized",
bus_get_realized, bus_set_realized, NULL);
+
+bus->resetting = 0;
}
static char *default_bus_get_fw_dev_path(DeviceState *dev)
@@ -202,9 +274,18 @@ static char *default_bus_get_fw_dev_path(DeviceState *dev)
static void bus_class_init(ObjectClass *class, void *data)
{
BusClass *bc = BUS_CLASS(class);
+ResettableClass *rc = RESETTABLE_CLASS(class);
class->unparent = bus_unparent;
bc->get_fw_dev_path = default_bus_get_fw_dev_path;
+
+rc->phases.exit = bus_obj_legacy_reset;
+rc->get_count = bus_get_reset_count;
+rc->increment_count = bus_increment_reset_count;
+rc->decrement_count = bus_decrement_reset_count;
+rc->foreach_child = bus_foreach_reset_child;
+rc->set_cold = bus_set_reset_cold;
+rc->set_hold_needed = bus_set_hold_needed;
}
static void qbus_finalize(Object *obj)
@@ -223,6 +304,10 @@ static const TypeInfo bus_info = {
.instance_init = qbus_initfn,
.instance_finalize = qbus_finalize,
.class_init = bus_class_init,
+.interfaces = (InterfaceInfo[]) {
+{ TYPE_RESETTABLE },
+{ }
+},
};
static void bus_register_types(void)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 043e058396..559ced070d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -254,6 +254,64 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
return hotplug_ctrl;
}
+void device_reset(DeviceState *dev, bool cold)
+{
+resettable_reset(OBJECT(de
[Qemu-block] [PATCH v3 18/33] hw/audio/intel-hda.c: remove device_legacy_reset call
Replace legacy's reset call by device_reset_warm.
The new function propagates also the reset to the sub-buses tree but this has
no impact since since HDACodecDevice has no child bus.
Signed-off-by: Damien Hedde
---
hw/audio/intel-hda.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index f133684b10..523bb3e2ca 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1086,7 +1086,7 @@ static void intel_hda_reset(DeviceState *dev)
QTAILQ_FOREACH(kid, >codecs.qbus.children, sibling) {
DeviceState *qdev = kid->child;
cdev = HDA_CODEC_DEVICE(qdev);
-device_legacy_reset(DEVICE(cdev));
+device_reset_warm(DEVICE(cdev));
d->state_sts |= (1 << cdev->cad);
}
intel_hda_update_irq(d);
--
2.22.0
[Qemu-block] [PATCH v3 21/33] hw/intc/spapr_xive.c: remove device_legacy_reset call
Replace legacy's reset call by device_reset_warm.
The new function propagates also the reset to the sub-buses tree but this has
no impact since SpaprXive has no child bus.
Signed-off-by: Damien Hedde
---
hw/intc/spapr_xive.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 22e11ad10c..fbd7ddb06e 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -1511,7 +1511,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu,
return H_PARAMETER;
}
-device_legacy_reset(DEVICE(xive));
+device_reset_warm(DEVICE(xive));
if (kvm_irqchip_in_kernel()) {
Error *local_err = NULL;
--
2.22.0
[Qemu-block] [PATCH v3 19/33] hw/sd/pl181.c & omap_mmc.c: remove device_legacy_reset call
Replace legacy's reset call by device_reset_warm. The new function propagates also the reset to the sub-buses tree but this has no impact since SDState has no child bus. Signed-off-by: Damien Hedde --- hw/sd/omap_mmc.c | 2 +- hw/sd/pl181.c| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c index 24a1edc149..3021e54b8d 100644 --- a/hw/sd/omap_mmc.c +++ b/hw/sd/omap_mmc.c @@ -317,7 +317,7 @@ void omap_mmc_reset(struct omap_mmc_s *host) * into any bus, and we must reset it manually. When omap_mmc is * QOMified this must move into the QOM reset function. */ -device_legacy_reset(DEVICE(host->card)); +device_reset_warm(DEVICE(host->card)); } static uint64_t omap_mmc_read(void *opaque, hwaddr offset, diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c index 15b4aaa67f..a59ef7eb2a 100644 --- a/hw/sd/pl181.c +++ b/hw/sd/pl181.c @@ -480,7 +480,7 @@ static void pl181_reset(DeviceState *d) /* Since we're still using the legacy SD API the card is not plugged * into any bus, and we must reset it manually. */ -device_legacy_reset(DEVICE(s->card)); +device_reset_warm(DEVICE(s->card)); } static void pl181_init(Object *obj) -- 2.22.0
