[PATCH for-8.0 0/2] hw/input/ps2: Convert to 3-phase reset

[PATCH for-8.0 0/2] hw/input/ps2: Convert to 3-phase reset

[Peter Maydell]

This patchset converts the ps2 keyboard and mouse devices to 3-phase
reset. The rationale here is that it would be nice to get rid of the
device_class_set_parent_reset() function, which is used by
legacy-reset subclasses which want to chain to their parent's reset
function. There aren't very many of these devices in total, and if we
convert them all to 3-phase reset they can use the 3-phase-reset
equivalent (resettable_class_set_parent_phases()).  Eventually this
will then let us simplify the transitional code for handling old-style
device reset.

This is one of a number of patchsets to do this that I'm planning to
write and send out over the next few weeks. It's all 8.0 material.

thanks
-- PMM

Peter Maydell (2):
  hw/input/ps2: Convert TYPE_PS2_DEVICE to 3-phase reset
  hw/input/ps2.c: Convert TYPE_PS2_{KBD,MOUSE}_DEVICE to 3-phase reset

 include/hw/input/ps2.h |  2 +-
 hw/input/ps2.c         | 45 +++++++++++++++++++++++++++++-------------
 2 files changed, 32 insertions(+), 15 deletions(-)

-- 
2.25.1

[PATCH for-8.0 1/2] hw/input/ps2: Convert TYPE_PS2_DEVICE to 3-phase reset

[Peter Maydell]

Convert the parent class TYPE_PS2_DEVICE to 3-phase reset.  Note that
we need an 'exit' phase function as well as the usual 'hold' phase
function, because changing outbound IRQ line state is only permitted
in 'exit'.  (Strictly speaking it's not supposed to be done in a
legacy reset handler either, but you can often get away with it.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/input/ps2.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 05cf7111e31..47a5d68e300 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -1001,12 +1001,18 @@ void ps2_write_mouse(PS2MouseState *s, int val)
     }
 }
 
-static void ps2_reset(DeviceState *dev)
+static void ps2_reset_hold(Object *obj)
 {
-    PS2State *s = PS2_DEVICE(dev);
+    PS2State *s = PS2_DEVICE(obj);
 
     s->write_cmd = -1;
     ps2_reset_queue(s);
+}
+
+static void ps2_reset_exit(Object *obj)
+{
+    PS2State *s = PS2_DEVICE(obj);
+
     ps2_lower_irq(s);
 }
 
@@ -1281,8 +1287,10 @@ static void ps2_init(Object *obj)
 static void ps2_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
 
-    dc->reset = ps2_reset;
+    rc->phases.hold = ps2_reset_hold;
+    rc->phases.exit = ps2_reset_exit;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
 
-- 
2.25.1

[PATCH for-8.0 2/2] hw/input/ps2.c: Convert TYPE_PS2_{KBD, MOUSE}_DEVICE to 3-phase reset

[Peter Maydell]

Convert the child classes TYPE_PS2_KBD_DEVICE and
TYPE_PS2_MOUSE_DEVICE to the 3-phase reset system.  This allows us to
stop using the old device_class_set_parent_reset() function.

We don't need to register an 'exit' phase function for the
subclasses, because they have no work to do in that phase.  Passing
NULL to resettable_class_set_parent_phases() will result in the
parent class method being called for that phase, so we don't need to
register a function purely to chain to the parent 'exit' phase
function.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/input/ps2.h |  2 +-
 hw/input/ps2.c         | 31 ++++++++++++++++++++-----------
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h
index ff777582cd6..cd61a634c39 100644
--- a/include/hw/input/ps2.h
+++ b/include/hw/input/ps2.h
@@ -36,7 +36,7 @@
 struct PS2DeviceClass {
     SysBusDeviceClass parent_class;
 
-    DeviceReset parent_reset;
+    ResettablePhases parent_phases;
 };
 
 /*
diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 47a5d68e300..3253ab6a92c 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -1042,13 +1042,16 @@ static void ps2_common_post_load(PS2State *s)
     q->cwptr = ccount ? (q->rptr + ccount) & (PS2_BUFFER_SIZE - 1) : -1;
 }
 
-static void ps2_kbd_reset(DeviceState *dev)
+static void ps2_kbd_reset_hold(Object *obj)
 {
-    PS2DeviceClass *ps2dc = PS2_DEVICE_GET_CLASS(dev);
-    PS2KbdState *s = PS2_KBD_DEVICE(dev);
+    PS2DeviceClass *ps2dc = PS2_DEVICE_GET_CLASS(obj);
+    PS2KbdState *s = PS2_KBD_DEVICE(obj);
 
     trace_ps2_kbd_reset(s);
-    ps2dc->parent_reset(dev);
+
+    if (ps2dc->parent_phases.hold) {
+        ps2dc->parent_phases.hold(obj);
+    }
 
     s->scan_enabled = 1;
     s->translate = 0;
@@ -1056,13 +1059,16 @@ static void ps2_kbd_reset(DeviceState *dev)
     s->modifiers = 0;
 }
 
-static void ps2_mouse_reset(DeviceState *dev)
+static void ps2_mouse_reset_hold(Object *obj)
 {
-    PS2DeviceClass *ps2dc = PS2_DEVICE_GET_CLASS(dev);
-    PS2MouseState *s = PS2_MOUSE_DEVICE(dev);
+    PS2DeviceClass *ps2dc = PS2_DEVICE_GET_CLASS(obj);
+    PS2MouseState *s = PS2_MOUSE_DEVICE(obj);
 
     trace_ps2_mouse_reset(s);
-    ps2dc->parent_reset(dev);
+
+    if (ps2dc->parent_phases.hold) {
+        ps2dc->parent_phases.hold(obj);
+    }
 
     s->mouse_status = 0;
     s->mouse_resolution = 0;
@@ -1245,10 +1251,12 @@ static void ps2_mouse_realize(DeviceState *dev, Error **errp)
 static void ps2_kbd_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
     PS2DeviceClass *ps2dc = PS2_DEVICE_CLASS(klass);
 
     dc->realize = ps2_kbd_realize;
-    device_class_set_parent_reset(dc, ps2_kbd_reset, &ps2dc->parent_reset);
+    resettable_class_set_parent_phases(rc, NULL, ps2_kbd_reset_hold, NULL,
+                                       &ps2dc->parent_phases);
     dc->vmsd = &vmstate_ps2_keyboard;
 }
 
@@ -1262,11 +1270,12 @@ static const TypeInfo ps2_kbd_info = {
 static void ps2_mouse_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
     PS2DeviceClass *ps2dc = PS2_DEVICE_CLASS(klass);
 
     dc->realize = ps2_mouse_realize;
-    device_class_set_parent_reset(dc, ps2_mouse_reset,
-                                  &ps2dc->parent_reset);
+    resettable_class_set_parent_phases(rc, NULL, ps2_mouse_reset_hold, NULL,
+                                       &ps2dc->parent_phases);
     dc->vmsd = &vmstate_ps2_mouse;
 }
 
-- 
2.25.1

Re: [PATCH for-8.0 1/2] hw/input/ps2: Convert TYPE_PS2_DEVICE to 3-phase reset

[Philippe Mathieu-Daudé]

On 9/11/22 18:00, Peter Maydell wrote:
> Convert the parent class TYPE_PS2_DEVICE to 3-phase reset.  Note that
> we need an 'exit' phase function as well as the usual 'hold' phase
> function, because changing outbound IRQ line state is only permitted
> in 'exit'.  (Strictly speaking it's not supposed to be done in a
> legacy reset handler either, but you can often get away with it.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/input/ps2.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-8.0 1/2] hw/input/ps2: Convert TYPE_PS2_DEVICE to 3-phase reset

[Richard Henderson]

On 11/10/22 04:00, Peter Maydell wrote:
> Convert the parent class TYPE_PS2_DEVICE to 3-phase reset.  Note that
> we need an 'exit' phase function as well as the usual 'hold' phase
> function, because changing outbound IRQ line state is only permitted
> in 'exit'.  (Strictly speaking it's not supposed to be done in a
> legacy reset handler either, but you can often get away with it.)
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/input/ps2.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-8.0 2/2] hw/input/ps2.c: Convert TYPE_PS2_{KBD, MOUSE}_DEVICE to 3-phase reset

[Richard Henderson]

On 11/10/22 04:00, Peter Maydell wrote:
> Convert the child classes TYPE_PS2_KBD_DEVICE and
> TYPE_PS2_MOUSE_DEVICE to the 3-phase reset system.  This allows us to
> stop using the old device_class_set_parent_reset() function.
> 
> We don't need to register an 'exit' phase function for the
> subclasses, because they have no work to do in that phase.  Passing
> NULL to resettable_class_set_parent_phases() will result in the
> parent class method being called for that phase, so we don't need to
> register a function purely to chain to the parent 'exit' phase
> function.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/input/ps2.h |  2 +-
>   hw/input/ps2.c         | 31 ++++++++++++++++++++-----------
>   2 files changed, 21 insertions(+), 12 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-8.0 0/2] hw/input/ps2: Convert to 3-phase reset

[Mark Cave-Ayland]

On 09/11/2022 17:00, Peter Maydell wrote:

> This patchset converts the ps2 keyboard and mouse devices to 3-phase
> reset. The rationale here is that it would be nice to get rid of the
> device_class_set_parent_reset() function, which is used by
> legacy-reset subclasses which want to chain to their parent's reset
> function. There aren't very many of these devices in total, and if we
> convert them all to 3-phase reset they can use the 3-phase-reset
> equivalent (resettable_class_set_parent_phases()).  Eventually this
> will then let us simplify the transitional code for handling old-style
> device reset.
> 
> This is one of a number of patchsets to do this that I'm planning to
> write and send out over the next few weeks. It's all 8.0 material.
> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>    hw/input/ps2: Convert TYPE_PS2_DEVICE to 3-phase reset
>    hw/input/ps2.c: Convert TYPE_PS2_{KBD,MOUSE}_DEVICE to 3-phase reset
> 
>   include/hw/input/ps2.h |  2 +-
>   hw/input/ps2.c         | 45 +++++++++++++++++++++++++++++-------------
>   2 files changed, 32 insertions(+), 15 deletions(-)

I haven't used the new ResettableClass myself previously, however it seems to match 
the excellent documentation at https://qemu.readthedocs.io/en/latest/devel/reset.html 
so feel free to add my Acked-by tag.

One part that did stand out to me in the docs is the part that reads "For now 
migration of a device or bus in reset is not supported. Care must be taken not to 
delay resettable_release_reset() after its resettable_assert_reset() counterpart". Is 
this still a valid concern and something we need to think about? I'm thinking about 
if a guest triggers a SCSI bus or PCI bus reset for example.


ATB,

Mark.

Re: [PATCH for-8.0 0/2] hw/input/ps2: Convert to 3-phase reset

[Peter Maydell]

On Thu, 10 Nov 2022 at 10:36, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> I haven't used the new ResettableClass myself previously, however it seems to match
> the excellent documentation at https://qemu.readthedocs.io/en/latest/devel/reset.html
> so feel free to add my Acked-by tag.
>
> One part that did stand out to me in the docs is the part that reads "For now
> migration of a device or bus in reset is not supported. Care must be taken not to
> delay resettable_release_reset() after its resettable_assert_reset() counterpart". Is
> this still a valid concern and something we need to think about? I'm thinking about
> if a guest triggers a SCSI bus or PCI bus reset for example.

That only matters if there's a way for the guest to hold the device
in reset, as opposed to resetting it as a point event. This is
theoretically possible to do with the new API, and was simply
impossible with the old API because with the old one reset was
always a one-shot point event.

thanks
-- PMM

Re: [PATCH for-8.0 0/2] hw/input/ps2: Convert to 3-phase reset

[Mark Cave-Ayland]

On 10/11/2022 11:22, Peter Maydell wrote:

> On Thu, 10 Nov 2022 at 10:36, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>> I haven't used the new ResettableClass myself previously, however it seems to match
>> the excellent documentation at https://qemu.readthedocs.io/en/latest/devel/reset.html
>> so feel free to add my Acked-by tag.
>>
>> One part that did stand out to me in the docs is the part that reads "For now
>> migration of a device or bus in reset is not supported. Care must be taken not to
>> delay resettable_release_reset() after its resettable_assert_reset() counterpart". Is
>> this still a valid concern and something we need to think about? I'm thinking about
>> if a guest triggers a SCSI bus or PCI bus reset for example.
> 
> That only matters if there's a way for the guest to hold the device
> in reset, as opposed to resetting it as a point event. This is
> theoretically possible to do with the new API, and was simply
> impossible with the old API because with the old one reset was
> always a one-shot point event.

I see, thanks. And the documentation confirms that resettable_reset() is an assert 
immediately followed by a release so that shouldn't be an issue for 
device_cold_reset() and bus_cold_reset() calling the new Resettable inteface.


ATB,

Mark.

Re: [PATCH for-8.0 2/2] hw/input/ps2.c: Convert TYPE_PS2_{KBD, MOUSE}_DEVICE to 3-phase reset

[Philippe Mathieu-Daudé]

On 9/11/22 18:00, Peter Maydell wrote:
> Convert the child classes TYPE_PS2_KBD_DEVICE and
> TYPE_PS2_MOUSE_DEVICE to the 3-phase reset system.  This allows us to
> stop using the old device_class_set_parent_reset() function.
> 
> We don't need to register an 'exit' phase function for the
> subclasses, because they have no work to do in that phase.  Passing
> NULL to resettable_class_set_parent_phases() will result in the
> parent class method being called for that phase, so we don't need to
> register a function purely to chain to the parent 'exit' phase
> function.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/input/ps2.h |  2 +-
>   hw/input/ps2.c         | 31 ++++++++++++++++++++-----------
>   2 files changed, 21 insertions(+), 12 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-8.0 0/2] hw/input/ps2: Convert to 3-phase reset

[Peter Maydell]

On Wed, 9 Nov 2022 at 17:00, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset converts the ps2 keyboard and mouse devices to 3-phase
> reset. The rationale here is that it would be nice to get rid of the
> device_class_set_parent_reset() function, which is used by
> legacy-reset subclasses which want to chain to their parent's reset
> function. There aren't very many of these devices in total, and if we
> convert them all to 3-phase reset they can use the 3-phase-reset
> equivalent (resettable_class_set_parent_phases()).  Eventually this
> will then let us simplify the transitional code for handling old-style
> device reset.


I plan to pick these up and send them in a pullreq together
with various other reset-related patches of mine, unless
you'd prefer them to go in some other way.

thanks
-- PMM