| Message ID | 4744c26fdfd911c44b58a9b6e1d0effc6cb39594.1455739133.git.alistair.francis@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Alistair Francis <alistair.francis@xilinx.com> writes: > If the device being added when running qdev_device_add() has > a reset function, register it so that it can be called. > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > > qdev-monitor.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/qdev-monitor.c b/qdev-monitor.c > index 81e3ff3..0a99d01 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) > > if (bus) { > qdev_set_parent_bus(dev, bus); > + } else if (dc->reset) { > + qemu_register_reset((void (*)(void *))dc->reset, dev); > } > > id = qemu_opts_id(opts); This looks wrong to me. You stuff all the device reset methods into the global reset_handlers list, where they get called in some semi-random order. This breaks when there are reset order dependencies between devices, e.g. between a device and the bus it plugs into. Propagating the reset signal to all the devices is a qdev problem. Copying Andreas for further insight.
On Thu, Feb 18, 2016 at 1:56 AM, Markus Armbruster <armbru@redhat.com> wrote: > Alistair Francis <alistair.francis@xilinx.com> writes: > >> If the device being added when running qdev_device_add() has >> a reset function, register it so that it can be called. >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> >> qdev-monitor.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> index 81e3ff3..0a99d01 100644 >> --- a/qdev-monitor.c >> +++ b/qdev-monitor.c >> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) >> >> if (bus) { >> qdev_set_parent_bus(dev, bus); >> + } else if (dc->reset) { >> + qemu_register_reset((void (*)(void *))dc->reset, dev); >> } >> >> id = qemu_opts_id(opts); > > This looks wrong to me. > > You stuff all the device reset methods into the global reset_handlers > list, where they get called in some semi-random order. This breaks when > there are reset order dependencies between devices, e.g. between a > device and the bus it plugs into. So I'm not a expert on this, but from what I see this will also be added near the end of the list (before the rom_resets though) and doesn't seem to be a problem. Do you have any other ideas how the reset function could be registered? Thanks, Alistair > > Propagating the reset signal to all the devices is a qdev problem. > Copying Andreas for further insight. >
On 18/02/2016 10:56, Markus Armbruster wrote: > Alistair Francis <alistair.francis@xilinx.com> writes: > >> If the device being added when running qdev_device_add() has >> a reset function, register it so that it can be called. >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> >> qdev-monitor.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> index 81e3ff3..0a99d01 100644 >> --- a/qdev-monitor.c >> +++ b/qdev-monitor.c >> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) >> >> if (bus) { >> qdev_set_parent_bus(dev, bus); >> + } else if (dc->reset) { >> + qemu_register_reset((void (*)(void *))dc->reset, dev); >> } >> >> id = qemu_opts_id(opts); > > This looks wrong to me. > > You stuff all the device reset methods into the global reset_handlers > list, where they get called in some semi-random order. This breaks when > there are reset order dependencies between devices, e.g. between a > device and the bus it plugs into. There is no bus here, see the "if" above the one that's being added. However, what devices have done so far is to register/unregister the reset in the realize/unrealize methods, and I suggest doing the same. If you really want to add the magic qemu_register_reset, you should at least do one of the following: * add a matching unregister (no idea where) * assert that the device is not hot-unpluggable, otherwise hot-unplug would leave a dangling pointer. Paolo > Propagating the reset signal to all the devices is a qdev problem. > Copying Andreas for further insight.
On Thu, Feb 18, 2016 at 1:47 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 18/02/2016 10:56, Markus Armbruster wrote: >> Alistair Francis <alistair.francis@xilinx.com> writes: >> >>> If the device being added when running qdev_device_add() has >>> a reset function, register it so that it can be called. >>> >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>> --- >>> >>> qdev-monitor.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/qdev-monitor.c b/qdev-monitor.c >>> index 81e3ff3..0a99d01 100644 >>> --- a/qdev-monitor.c >>> +++ b/qdev-monitor.c >>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) >>> >>> if (bus) { >>> qdev_set_parent_bus(dev, bus); >>> + } else if (dc->reset) { >>> + qemu_register_reset((void (*)(void *))dc->reset, dev); >>> } >>> >>> id = qemu_opts_id(opts); >> >> This looks wrong to me. >> >> You stuff all the device reset methods into the global reset_handlers >> list, where they get called in some semi-random order. This breaks when >> there are reset order dependencies between devices, e.g. between a >> device and the bus it plugs into. > > There is no bus here, see the "if" above the one that's being added. > > However, what devices have done so far is to register/unregister the > reset in the realize/unrealize methods, and I suggest doing the same. > Does this assume the device itself knows whether it is bus-connected or not? This way has the advantage of catchall-ing devices that have no bus connected and the device may or may not know whether it is bus-connected (nor should it need to know). Probably doesn't have in tree precedent yet, but I thought we wanted to move away from qdev/qbus managing the device-tree. So ideally, the new else should become unconditional long term once we debusify (and properly QOMify) the reset tree (and the if goes away). > If you really want to add the magic qemu_register_reset, you should at > least do one of the following: > > * add a matching unregister (no idea where) > You could add a boolean flag to DeviceState that is set by this registration. It can then be checked at unrealize to remove reset handler. Regards, Peter > * assert that the device is not hot-unpluggable, otherwise hot-unplug > would leave a dangling pointer. > > Paolo > >> Propagating the reset signal to all the devices is a qdev problem. >> Copying Andreas for further insight.
On Thu, Feb 18, 2016 at 3:07 PM, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote: > On Thu, Feb 18, 2016 at 1:47 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 18/02/2016 10:56, Markus Armbruster wrote: >>> Alistair Francis <alistair.francis@xilinx.com> writes: >>> >>>> If the device being added when running qdev_device_add() has >>>> a reset function, register it so that it can be called. >>>> >>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>>> --- >>>> >>>> qdev-monitor.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/qdev-monitor.c b/qdev-monitor.c >>>> index 81e3ff3..0a99d01 100644 >>>> --- a/qdev-monitor.c >>>> +++ b/qdev-monitor.c >>>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) >>>> >>>> if (bus) { >>>> qdev_set_parent_bus(dev, bus); >>>> + } else if (dc->reset) { >>>> + qemu_register_reset((void (*)(void *))dc->reset, dev); >>>> } >>>> >>>> id = qemu_opts_id(opts); >>> >>> This looks wrong to me. >>> >>> You stuff all the device reset methods into the global reset_handlers >>> list, where they get called in some semi-random order. This breaks when >>> there are reset order dependencies between devices, e.g. between a >>> device and the bus it plugs into. >> >> There is no bus here, see the "if" above the one that's being added. >> >> However, what devices have done so far is to register/unregister the >> reset in the realize/unrealize methods, and I suggest doing the same. Ok, I am happy to do it that way. It just seemed dodgy to me registering the reset in the realise. This also seemed like a feature worth having, as I thought this would come up again in the future. >> > > Does this assume the device itself knows whether it is bus-connected > or not? This way has the advantage of catchall-ing devices that have > no bus connected and the device may or may not know whether it is > bus-connected (nor should it need to know). Probably doesn't have in > tree precedent yet, but I thought we wanted to move away from > qdev/qbus managing the device-tree. So ideally, the new else should > become unconditional long term once we debusify (and properly QOMify) > the reset tree (and the if goes away). That was my general thinking as well. Thanks, Alistair > >> If you really want to add the magic qemu_register_reset, you should at >> least do one of the following: >> >> * add a matching unregister (no idea where) >> > > You could add a boolean flag to DeviceState that is set by this > registration. It can then be checked at unrealize to remove reset > handler. > > Regards, > Peter > >> * assert that the device is not hot-unpluggable, otherwise hot-unplug >> would leave a dangling pointer. >> >> Paolo >> >>> Propagating the reset signal to all the devices is a qdev problem. >>> Copying Andreas for further insight. >
On 19/02/2016 00:07, Peter Crosthwaite wrote: > On Thu, Feb 18, 2016 at 1:47 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 18/02/2016 10:56, Markus Armbruster wrote: >>> Alistair Francis <alistair.francis@xilinx.com> writes: >>> >>>> If the device being added when running qdev_device_add() has >>>> a reset function, register it so that it can be called. >>>> >>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>>> --- >>>> >>>> qdev-monitor.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/qdev-monitor.c b/qdev-monitor.c >>>> index 81e3ff3..0a99d01 100644 >>>> --- a/qdev-monitor.c >>>> +++ b/qdev-monitor.c >>>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) >>>> >>>> if (bus) { >>>> qdev_set_parent_bus(dev, bus); >>>> + } else if (dc->reset) { >>>> + qemu_register_reset((void (*)(void *))dc->reset, dev); >>>> } >>>> >>>> id = qemu_opts_id(opts); >>> >>> This looks wrong to me. >>> >>> You stuff all the device reset methods into the global reset_handlers >>> list, where they get called in some semi-random order. This breaks when >>> there are reset order dependencies between devices, e.g. between a >>> device and the bus it plugs into. >> >> There is no bus here, see the "if" above the one that's being added. >> >> However, what devices have done so far is to register/unregister the >> reset in the realize/unrealize methods, and I suggest doing the same. > > Does this assume the device itself knows whether it is bus-connected > or not? This way has the advantage of catchall-ing devices that have > no bus connected and the device may or may not know whether it is > bus-connected (nor should it need to know). A device almost definitely needs to know if it is bus connected. More likely than not, a busless device inherits from DeviceState while a device with a bus inherits from SCSIDevice, PCIDevice, I2CSlave, etc. > Probably doesn't have in > tree precedent yet, but I thought we wanted to move away from > qdev/qbus managing the device-tree. So ideally, the new else should > become unconditional long term once we debusify (and properly QOMify) > the reset tree (and the if goes away). Any abstraction we have in QEMU should have at least a parallel (though it need not be the same) in real hardware. Reset signals _do_ propagate along buses, or at least along some buses, so "debusifying" reset seems like a counterproductive goal to me. For busless devices, I thought the idea was just to have the QOM parent (container) propagate the realize/reset/unrealize signals in the right order. Unfortunately reality is not quite as simple and indeed here however you have a busless device that: - doesn't have a container (the container is just /machine/unattached or similar). - triggers a reset for something else that is not contained in it (the CPU) and even some DMA. >> If you really want to add the magic qemu_register_reset, you should at >> least do one of the following: >> >> * add a matching unregister (no idea where) > > You could add a boolean flag to DeviceState that is set by this > registration. It can then be checked at unrealize to remove reset > handler. Yeah, I guess that would work. A few changes: - you register the callback unconditionally for all busless devices, using qdev_reset_all_fn instead of directly dc->reset - you do it after the "realized" property has been set successfully. Otherwise, a failed -device or device_add will also leave a dangling callback. - add a comment that this is because the callback is registered because this is a busless _and_ container-less device Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 18/02/2016 10:56, Markus Armbruster wrote: >> Alistair Francis <alistair.francis@xilinx.com> writes: >> >>> If the device being added when running qdev_device_add() has >>> a reset function, register it so that it can be called. >>> >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>> --- >>> >>> qdev-monitor.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/qdev-monitor.c b/qdev-monitor.c >>> index 81e3ff3..0a99d01 100644 >>> --- a/qdev-monitor.c >>> +++ b/qdev-monitor.c >>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) >>> >>> if (bus) { >>> qdev_set_parent_bus(dev, bus); >>> + } else if (dc->reset) { >>> + qemu_register_reset((void (*)(void *))dc->reset, dev); >>> } >>> >>> id = qemu_opts_id(opts); >> >> This looks wrong to me. >> >> You stuff all the device reset methods into the global reset_handlers >> list, where they get called in some semi-random order. This breaks when >> there are reset order dependencies between devices, e.g. between a >> device and the bus it plugs into. > > There is no bus here, see the "if" above the one that's being added. > > However, what devices have done so far is to register/unregister the > reset in the realize/unrealize methods, and I suggest doing the same. Shows that our support for bus-less devices is still in its infancy. For me, a device has a number of external connectors that need to be wired up. A qdev bus is merely a standardized package of such connectors. For historical reasons, buses are all qdev has, with a special sysbus for everything that cannot be shoehorned into a single bus. To work with individual connectors, you have to drop into C (ignoring the weird "platform bus" sysbus thing Alex added). Support for doing that at configuration rather than code level would be nice. [...]
On 19 February 2016 at 09:33, Paolo Bonzini <pbonzini@redhat.com> wrote: > Any abstraction we have in QEMU should have at least a parallel (though > it need not be the same) in real hardware. Reset signals _do_ propagate > along buses, or at least along some buses, so "debusifying" reset seems > like a counterproductive goal to me. Reset for some buses propagates along buses, but not in all cases. In any case our current "reset" semantics are "power on reset", not any kind of driven-by-hardware-reset-lines reset. thanks -- PMM
Am 18.02.2016 um 10:56 schrieb Markus Armbruster: > Alistair Francis <alistair.francis@xilinx.com> writes: > >> If the device being added when running qdev_device_add() has >> a reset function, register it so that it can be called. >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> >> qdev-monitor.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> index 81e3ff3..0a99d01 100644 >> --- a/qdev-monitor.c >> +++ b/qdev-monitor.c >> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) >> >> if (bus) { >> qdev_set_parent_bus(dev, bus); >> + } else if (dc->reset) { >> + qemu_register_reset((void (*)(void *))dc->reset, dev); >> } >> >> id = qemu_opts_id(opts); > > This looks wrong to me. > > You stuff all the device reset methods into the global reset_handlers > list, where they get called in some semi-random order. This breaks when > there are reset order dependencies between devices, e.g. between a > device and the bus it plugs into. > > Propagating the reset signal to all the devices is a qdev problem. > Copying Andreas for further insight. We had a similar discussion for s390x, and I had started a big reset refactoring, but we agreed to go for a stop-gap solution for 2.5. The recently posted hw/core/bus.c refactoring originated from that branch. The overall idea was that for buses reset propagates along buses (so yes, NACK to this patch), but where no bus exists it shall propagate to QOM children, too. So Alistair, if you have a device that needs a reset while not sitting on a bus, please register the register hook in your device's realize hook for now. Regards, Andreas
On 19/02/2016 11:55, Peter Maydell wrote: >> Any abstraction we have in QEMU should have at least a parallel (though >> > it need not be the same) in real hardware. Reset signals _do_ propagate >> > along buses, or at least along some buses, so "debusifying" reset seems >> > like a counterproductive goal to me. > Reset for some buses propagates along buses, but not in all > cases. Agreed. But still this doesn't change the fact that "debusifying" reset is a non-goal. > In any case our current "reset" semantics are "power on > reset", not any kind of driven-by-hardware-reset-lines reset. It depends. There are several cases (PCI, SCSI) where qdev_reset_all is used from within the code in order to provide hardware reset semantics. Paolo
On Fri, Feb 19, 2016 at 9:15 AM, Andreas Färber <afaerber@suse.de> wrote: > Am 18.02.2016 um 10:56 schrieb Markus Armbruster: >> Alistair Francis <alistair.francis@xilinx.com> writes: >> >>> If the device being added when running qdev_device_add() has >>> a reset function, register it so that it can be called. >>> >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>> --- >>> >>> qdev-monitor.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/qdev-monitor.c b/qdev-monitor.c >>> index 81e3ff3..0a99d01 100644 >>> --- a/qdev-monitor.c >>> +++ b/qdev-monitor.c >>> @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) >>> >>> if (bus) { >>> qdev_set_parent_bus(dev, bus); >>> + } else if (dc->reset) { >>> + qemu_register_reset((void (*)(void *))dc->reset, dev); >>> } >>> >>> id = qemu_opts_id(opts); >> >> This looks wrong to me. >> >> You stuff all the device reset methods into the global reset_handlers >> list, where they get called in some semi-random order. This breaks when >> there are reset order dependencies between devices, e.g. between a >> device and the bus it plugs into. >> >> Propagating the reset signal to all the devices is a qdev problem. >> Copying Andreas for further insight. > > We had a similar discussion for s390x, and I had started a big reset > refactoring, but we agreed to go for a stop-gap solution for 2.5. The > recently posted hw/core/bus.c refactoring originated from that branch. > > The overall idea was that for buses reset propagates along buses (so > yes, NACK to this patch), but where no bus exists it shall propagate to > QOM children, too. > > So Alistair, if you have a device that needs a reset while not sitting > on a bus, please register the register hook in your device's realize > hook for now. Ok, that is fair. I have moved the reset register/unregister to the realise/unrealise functions in V2. Thanks, Alistair > > Regards, > Andreas > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg) >
diff --git a/qdev-monitor.c b/qdev-monitor.c index 81e3ff3..0a99d01 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -561,6 +561,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) if (bus) { qdev_set_parent_bus(dev, bus); + } else if (dc->reset) { + qemu_register_reset((void (*)(void *))dc->reset, dev); } id = qemu_opts_id(opts);
If the device being added when running qdev_device_add() has a reset function, register it so that it can be called. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- qdev-monitor.c | 2 ++ 1 file changed, 2 insertions(+)