Re: [Qemu-block] [Qemu-devel] [PATCH v3 05/33] Switch to new api in qdev/bus
On Fri, Aug 09, 2019 at 12:08:43PM +0100, Peter Maydell wrote:
> On Fri, 9 Aug 2019 at 01:10, David Gibson wrote:
> >
> > On Wed, Jul 31, 2019 at 01:31:28PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 7/31/19 11:29 AM, Damien Hedde wrote:
> > > > On 7/31/19 8:05 AM, David Gibson wrote:
> > > >> On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote:
> > > >>> @@ -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.
>
> So, I don't think that actually is the established meaning of
> device_reset(). I think our current semantics for this function are
> "reset of some sort, probably cold, but in practice called in
> lots of places which really wanted some other kind of reset,
> because our current reset semantics are not well-defined".
>
> For instance in s390-pci-inst.c the handling of CLP_SET_DISABLE_PCI_FN
> calls device_reset() on a device. I bet that's not really intentionally
> trying to model "we powered it off and on again".
> hw/scsi/vmw_pvscsi.c uses device_reset() in its handling of
> the guest "reset the SCSI bus" command. I know that isn't literally
> powering off the SCSI disks and powering them on again.
>
> The advantage of an actual API change here is that it means we go
> and look at all the call sites and find out what the semantics
> they actually wanted were, and hopefully that then feeds into the
> design of the new API and we make sure we can handle those
> semantics in a non-confused way.
That's a fair point.
> > > >> 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).
> > >
> > > If the boolean is not meaningful, we can use an enum...
> >
> > That's certainly better, but I'm not seeing a compelling reason not to
> > have separate function names. It's just as clear and means less churn.
>
> One advantage of an enum is that we have an extendable API,
> so we could say something like "all devices support reset types
> 'cold' and 'warm', but individual devices or families of devices
> can also support other kinds of reset". So the relevant s390
> devices could directly support the kinds of reset currently
> enumerated by the enum s390_reset, and then if you know you're
> dealing with an s390 device you can ask it to reset with the
> right semantics rather than fudging it with one of the generic ones.
> Or devices with "if I'm reset by the guest writing to a register then
> I reset less stuff than a reset via external reset line" have a
> way to model that.
Ok, I can see the value in that. I guess the way I'd prefer to
approach it though, is to start out with just a single-value enum for
(roughly) a cold reset. Then when we find a subset of devices for
which we can consistently define a warm reset of some type, we add an
enum value for it.
I guess we'd also need some way of introspecting what reset types are
supported by a device.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH v3 05/33] Switch to new api in qdev/bus
On Fri, 9 Aug 2019 at 01:10, David Gibson wrote:
>
> On Wed, Jul 31, 2019 at 01:31:28PM +0200, Philippe Mathieu-Daudé wrote:
> > On 7/31/19 11:29 AM, Damien Hedde wrote:
> > > On 7/31/19 8:05 AM, David Gibson wrote:
> > >> On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote:
> > >>> @@ -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.
So, I don't think that actually is the established meaning of
device_reset(). I think our current semantics for this function are
"reset of some sort, probably cold, but in practice called in
lots of places which really wanted some other kind of reset,
because our current reset semantics are not well-defined".
For instance in s390-pci-inst.c the handling of CLP_SET_DISABLE_PCI_FN
calls device_reset() on a device. I bet that's not really intentionally
trying to model "we powered it off and on again".
hw/scsi/vmw_pvscsi.c uses device_reset() in its handling of
the guest "reset the SCSI bus" command. I know that isn't literally
powering off the SCSI disks and powering them on again.
The advantage of an actual API change here is that it means we go
and look at all the call sites and find out what the semantics
they actually wanted were, and hopefully that then feeds into the
design of the new API and we make sure we can handle those
semantics in a non-confused way.
> > >> 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).
> >
> > If the boolean is not meaningful, we can use an enum...
>
> That's certainly better, but I'm not seeing a compelling reason not to
> have separate function names. It's just as clear and means less churn.
One advantage of an enum is that we have an extendable API,
so we could say something like "all devices support reset types
'cold' and 'warm', but individual devices or families of devices
can also support other kinds of reset". So the relevant s390
devices could directly support the kinds of reset currently
enumerated by the enum s390_reset, and then if you know you're
dealing with an s390 device you can ask it to reset with the
right semantics rather than fudging it with one of the generic ones.
Or devices with "if I'm reset by the guest writing to a register then
I reset less stuff than a reset via external reset line" have a
way to model that.
thanks
-- PMM
Re: [Qemu-block] [Qemu-devel] [PATCH v3 05/33] Switch to new api in qdev/bus
On Wed, Jul 31, 2019 at 01:31:28PM +0200, Philippe Mathieu-Daudé wrote:
> On 7/31/19 11:29 AM, Damien Hedde wrote:
> > 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).
>
> If the boolean is not meaningful, we can use an enum...
That's certainly better, but I'm not seeing a compelling reason not to
have separate function names. It's just as clear and means less churn.
>
> > 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
> >
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH v3 05/33] Switch to new api in qdev/bus
On 7/31/19 11:29 AM, Damien Hedde wrote:
> 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).
If the boolean is not meaningful, we can use an enum...
> 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
>
