[PATCH 00/50] PS2 device QOMification - part 1

[PATCH 00/50] PS2 device QOMification - part 1

[Mark Cave-Ayland]

This series came about when looking at improving the LASI PS2 device for
the HPPA machine: there were improvements that I was keen to make, but
was restricted because the PS2 device(s) weren't QOMified.

Trying to do everything in a single patchset would be a very large series
indeed, so here is part 1 of the series which does the basic QOMification
process and consists of:

- QOMifying the basic PS2, PS2 keyboard and PS2 mouse types

- Moving any functionality that exists in a global device init function
  directly into the relevant device, so that all device behaviour is
  configured using qdev properties and QOM

- Introducing a new I8042_MMIO type for use by the MIPS magnum machine

- Switch all PS2 devices to use qdev gpios for IRQs instead of using the
  update_irq() callback function along with the update_arg opaque

Once this work has been done, a follow-up part 2 series will finish the
remainder of the work which involves i) improving the QOM object model
now QOMification is complete and ii) removing the legacy global device
init functions for PS2 and related devices.

Testing for this series has comprised of booting a machine with each type
of PS2 device and confirming that i) the machine responds to keypresses
when using a graphical console and ii) completing a successful migration
from a machine with this series applies back to a machine running latest
git master. The test machines I used were:

- qemu-system-x86_64 -M pc for the I8042 device
- qemu-system-hppa for the LASIPS2 device
- qemu-system-arm -M versatilepb for the PL050 device
- qemu-system-mips64el -M magnum for the I8042_MMIO device

Finally the QOM tree changes caused by QOMification of the PS2 devices
trigger a failure due to a bug in the bios-tables-test qtest for subtest
/x86_64/acpi/q35/viot. This can be fixed by applying the series at
https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04266.html
"hw/acpi/viot: generate stable VIOT ACPI tables" first.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (50):
  ps2: checkpatch fixes
  ps2: QOMify PS2State
  ps2: QOMify PS2KbdState
  ps2: QOMify PS2MouseState
  ps2: move QOM type definitions from ps2.c to ps2.h
  ps2: improve function prototypes in ps2.c and ps2.h
  ps2: introduce PS2DeviceClass
  ps2: implement ps2_reset() for the PS2_DEVICE QOM type based upon
    ps2_common_reset()
  ps2: remove duplicate setting of scancode_set in ps2_kbd_init()
  ps2: implement ps2_kbd_realize() and use it to register
    ps2_keyboard_handler
  ps2: implement ps2_mouse_realize() and use it to register
    ps2_mouse_handler
  ps2: don't use vmstate_register() in ps2_kbd_init()
  ps2: don't use vmstate_register() in ps2_mouse_init()
  pl050: checkpatch fixes
  pl050: split pl050_update_irq() into separate pl050_set_irq() and
    pl050_update_irq() functions
  lasips2: spacing fixes
  lasips2: rename ps2dev_update_irq() to lasips2_port_set_irq()
  pckbd: checkpatch fixes
  pckbd: move KBDState from pckbd.c to i8042.h
  pckbd: move ISAKBDState from pckbd.c to i8042.h
  pckbd: introduce new I8042_MMIO QOM type
  pckbd: implement i8042_mmio_reset() for I8042_MMIO device
  pckbd: add mask qdev property to I8042_MMIO device
  pckbd: add size qdev property to I8042_MMIO device
  pckbd: implement i8042_mmio_realize() function
  pckbd: implement i8042_mmio_init() function
  pckbd: alter i8042_mm_init() to return a I8042_MMIO device
  pckbd: move mapping of I8042_MMIO registers to MIPS magnum machine
  pckbd: more vmstate_register() from i8042_mm_init() to
    i8042_mmio_realize()
  pckbd: move ps2_kbd_init() and ps2_mouse_init() to
    i8042_mmio_realize()
  ps2: make ps2_raise_irq() function static
  ps2: use ps2_raise_irq() instead of calling update_irq() directly
  ps2: introduce ps2_lower_irq() instead of calling update_irq()
    directly
  ps2: add gpio for output IRQ and optionally use it in ps2_raise_irq()
    and ps2_lower_irq()
  pckbd: replace irq_kbd and irq_mouse with qemu_irq array in KBDState
  pl050: switch over from update_irq() function to PS2 device gpio
  lasips2: QOMify LASIPS2State
  lasips2: move lasips2 QOM types from lasips2.c to lasips2.h
  lasips2: rename lasips2_init() to lasips2_initfn() and update it to
    return the LASIPS2 device
  lasips2: implement lasips2_init() function
  lasips2: move mapping of LASIPS2 registers to HPPA machine
  lasips2: move initialisation of PS2 ports from lasi_initfn() to
    lasi_init()
  lasips2: add base property
  lasips2: implement lasips2_realize()
  lasips2: use qdev gpio for output IRQ
  lasips2: switch over from update_irq() function to PS2 device gpio
  pckbd: switch I8042_MMIO device from update_irq() function to PS2
    device gpio
  pckbd: add i8042_reset() function to I8042 device
  pckbd: switch I8042 device from update_irq() function to PS2 device
    gpio
  ps2: remove update_irq() function and update_arg parameter

 hw/hppa/machine.c          |  11 +-
 hw/input/lasips2.c         | 110 +++++---
 hw/input/pckbd.c           | 321 ++++++++++++++++--------
 hw/input/pl050.c           |  50 ++--
 hw/input/ps2.c             | 500 +++++++++++++++++++++----------------
 hw/mips/jazz.c             |  11 +-
 include/hw/input/i8042.h   |  54 +++-
 include/hw/input/lasips2.h |  27 +-
 include/hw/input/ps2.h     |  79 +++++-
 9 files changed, 775 insertions(+), 388 deletions(-)

-- 
2.20.1

[PATCH 01/50] ps2: checkpatch fixes

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c | 154 +++++++++++++++++++++++++++----------------------
 1 file changed, 86 insertions(+), 68 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index c16df1de7a..67dd2eca84 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -34,41 +34,41 @@
 #include "trace.h"
 
 /* Keyboard Commands */
-#define KBD_CMD_SET_LEDS	0xED	/* Set keyboard leds */
-#define KBD_CMD_ECHO     	0xEE
-#define KBD_CMD_SCANCODE	0xF0	/* Get/set scancode set */
-#define KBD_CMD_GET_ID 	        0xF2	/* get keyboard ID */
-#define KBD_CMD_SET_RATE	0xF3	/* Set typematic rate */
-#define KBD_CMD_ENABLE		0xF4	/* Enable scanning */
-#define KBD_CMD_RESET_DISABLE	0xF5	/* reset and disable scanning */
-#define KBD_CMD_RESET_ENABLE   	0xF6    /* reset and enable scanning */
-#define KBD_CMD_RESET		0xFF	/* Reset */
+#define KBD_CMD_SET_LEDS        0xED    /* Set keyboard leds */
+#define KBD_CMD_ECHO            0xEE
+#define KBD_CMD_SCANCODE        0xF0    /* Get/set scancode set */
+#define KBD_CMD_GET_ID          0xF2    /* get keyboard ID */
+#define KBD_CMD_SET_RATE        0xF3    /* Set typematic rate */
+#define KBD_CMD_ENABLE          0xF4    /* Enable scanning */
+#define KBD_CMD_RESET_DISABLE   0xF5    /* reset and disable scanning */
+#define KBD_CMD_RESET_ENABLE    0xF6    /* reset and enable scanning */
+#define KBD_CMD_RESET           0xFF    /* Reset */
 #define KBD_CMD_SET_MAKE_BREAK  0xFC    /* Set Make and Break mode */
 #define KBD_CMD_SET_TYPEMATIC   0xFA    /* Set Typematic Make and Break mode */
 
 /* Keyboard Replies */
-#define KBD_REPLY_POR		0xAA	/* Power on reset */
-#define KBD_REPLY_ID		0xAB	/* Keyboard ID */
-#define KBD_REPLY_ACK		0xFA	/* Command ACK */
-#define KBD_REPLY_RESEND	0xFE	/* Command NACK, send the cmd again */
+#define KBD_REPLY_POR       0xAA    /* Power on reset */
+#define KBD_REPLY_ID        0xAB    /* Keyboard ID */
+#define KBD_REPLY_ACK       0xFA    /* Command ACK */
+#define KBD_REPLY_RESEND    0xFE    /* Command NACK, send the cmd again */
 
 /* Mouse Commands */
-#define AUX_SET_SCALE11		0xE6	/* Set 1:1 scaling */
-#define AUX_SET_SCALE21		0xE7	/* Set 2:1 scaling */
-#define AUX_SET_RES		0xE8	/* Set resolution */
-#define AUX_GET_SCALE		0xE9	/* Get scaling factor */
-#define AUX_SET_STREAM		0xEA	/* Set stream mode */
-#define AUX_POLL		0xEB	/* Poll */
-#define AUX_RESET_WRAP		0xEC	/* Reset wrap mode */
-#define AUX_SET_WRAP		0xEE	/* Set wrap mode */
-#define AUX_SET_REMOTE		0xF0	/* Set remote mode */
-#define AUX_GET_TYPE		0xF2	/* Get type */
-#define AUX_SET_SAMPLE		0xF3	/* Set sample rate */
-#define AUX_ENABLE_DEV		0xF4	/* Enable aux device */
-#define AUX_DISABLE_DEV		0xF5	/* Disable aux device */
-#define AUX_SET_DEFAULT		0xF6
-#define AUX_RESET		0xFF	/* Reset aux device */
-#define AUX_ACK			0xFA	/* Command byte ACK. */
+#define AUX_SET_SCALE11     0xE6    /* Set 1:1 scaling */
+#define AUX_SET_SCALE21     0xE7    /* Set 2:1 scaling */
+#define AUX_SET_RES         0xE8    /* Set resolution */
+#define AUX_GET_SCALE       0xE9    /* Get scaling factor */
+#define AUX_SET_STREAM      0xEA    /* Set stream mode */
+#define AUX_POLL            0xEB    /* Poll */
+#define AUX_RESET_WRAP      0xEC    /* Reset wrap mode */
+#define AUX_SET_WRAP        0xEE    /* Set wrap mode */
+#define AUX_SET_REMOTE      0xF0    /* Set remote mode */
+#define AUX_GET_TYPE        0xF2    /* Get type */
+#define AUX_SET_SAMPLE      0xF3    /* Set sample rate */
+#define AUX_ENABLE_DEV      0xF4    /* Enable aux device */
+#define AUX_DISABLE_DEV     0xF5    /* Disable aux device */
+#define AUX_SET_DEFAULT     0xF6
+#define AUX_RESET           0xFF    /* Reset aux device */
+#define AUX_ACK             0xFA    /* Command byte ACK. */
 
 #define MOUSE_STATUS_REMOTE     0x40
 #define MOUSE_STATUS_ENABLED    0x20
@@ -436,8 +436,9 @@ static void ps2_keyboard_event(DeviceState *dev, QemuConsole *src,
                 }
             }
         } else {
-            if (qcode < qemu_input_map_qcode_to_atset1_len)
+            if (qcode < qemu_input_map_qcode_to_atset1_len) {
                 keycode = qemu_input_map_qcode_to_atset1[qcode];
+            }
             if (keycode) {
                 if (keycode & 0xff00) {
                     ps2_put_keycode(s, keycode >> 8);
@@ -530,8 +531,9 @@ static void ps2_keyboard_event(DeviceState *dev, QemuConsole *src,
                 }
             }
         } else {
-            if (qcode < qemu_input_map_qcode_to_atset2_len)
+            if (qcode < qemu_input_map_qcode_to_atset2_len) {
                 keycode = qemu_input_map_qcode_to_atset2[qcode];
+            }
             if (keycode) {
                 if (keycode & 0xff00) {
                     ps2_put_keycode(s, keycode >> 8);
@@ -546,8 +548,9 @@ static void ps2_keyboard_event(DeviceState *dev, QemuConsole *src,
             }
         }
     } else if (s->scancode_set == 3) {
-        if (qcode < qemu_input_map_qcode_to_atset3_len)
+        if (qcode < qemu_input_map_qcode_to_atset3_len) {
             keycode = qemu_input_map_qcode_to_atset3[qcode];
+        }
         if (keycode) {
             /* FIXME: break code should be configured on a key by key basis */
             if (!key->down) {
@@ -569,8 +572,10 @@ uint32_t ps2_read_data(PS2State *s)
     trace_ps2_read_data(s);
     q = &s->queue;
     if (q->count == 0) {
-        /* NOTE: if no data left, we return the last keyboard one
-           (needed for EMM386) */
+        /*
+         * NOTE: if no data left, we return the last keyboard one
+         * (needed for EMM386)
+         */
         /* XXX: need a timer to do things correctly */
         index = q->rptr - 1;
         if (index < 0) {
@@ -619,10 +624,10 @@ void ps2_write_keyboard(void *opaque, int val)
 
     trace_ps2_write_keyboard(opaque, val);
     ps2_cqueue_reset(&s->common);
-    switch(s->common.write_cmd) {
+    switch (s->common.write_cmd) {
     default:
     case -1:
-        switch(val) {
+        switch (val) {
         case 0x00:
             ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
             break;
@@ -632,7 +637,7 @@ void ps2_write_keyboard(void *opaque, int val)
         case KBD_CMD_GET_ID:
             /* We emulate a MF2 AT keyboard here */
             ps2_cqueue_3(&s->common, KBD_REPLY_ACK, KBD_REPLY_ID,
-                s->translate ? 0x41 : 0x83);
+                         s->translate ? 0x41 : 0x83);
             break;
         case KBD_CMD_ECHO:
             ps2_cqueue_1(&s->common, KBD_CMD_ECHO);
@@ -661,8 +666,8 @@ void ps2_write_keyboard(void *opaque, int val)
         case KBD_CMD_RESET:
             ps2_reset_keyboard(s);
             ps2_cqueue_2(&s->common,
-                KBD_REPLY_ACK,
-                KBD_REPLY_POR);
+                         KBD_REPLY_ACK,
+                         KBD_REPLY_POR);
             break;
         case KBD_CMD_SET_TYPEMATIC:
             ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
@@ -700,9 +705,11 @@ void ps2_write_keyboard(void *opaque, int val)
     }
 }
 
-/* Set the scancode translation mode.
-   0 = raw scancodes.
-   1 = translated scancodes (used by qemu internally).  */
+/*
+ * Set the scancode translation mode.
+ * 0 = raw scancodes.
+ * 1 = translated scancodes (used by qemu internally).
+ */
 
 void ps2_keyboard_set_translation(void *opaque, int mode)
 {
@@ -727,30 +734,33 @@ static int ps2_mouse_send_packet(PS2MouseState *s)
     dz1 = s->mouse_dz;
     dw1 = s->mouse_dw;
     /* XXX: increase range to 8 bits ? */
-    if (dx1 > 127)
+    if (dx1 > 127) {
         dx1 = 127;
-    else if (dx1 < -127)
+    } else if (dx1 < -127) {
         dx1 = -127;
-    if (dy1 > 127)
+    }
+    if (dy1 > 127) {
         dy1 = 127;
-    else if (dy1 < -127)
+    } else if (dy1 < -127) {
         dy1 = -127;
+    }
     b = 0x08 | ((dx1 < 0) << 4) | ((dy1 < 0) << 5) | (s->mouse_buttons & 0x07);
     ps2_queue_noirq(&s->common, b);
     ps2_queue_noirq(&s->common, dx1 & 0xff);
     ps2_queue_noirq(&s->common, dy1 & 0xff);
     /* extra byte for IMPS/2 or IMEX */
-    switch(s->mouse_type) {
+    switch (s->mouse_type) {
     default:
         /* Just ignore the wheels if not supported */
         s->mouse_dz = 0;
         s->mouse_dw = 0;
         break;
     case 3:
-        if (dz1 > 127)
+        if (dz1 > 127) {
             dz1 = 127;
-        else if (dz1 < -127)
-                dz1 = -127;
+        } else if (dz1 < -127) {
+            dz1 = -127;
+        }
         ps2_queue_noirq(&s->common, dz1 & 0xff);
         s->mouse_dz -= dz1;
         s->mouse_dw = 0;
@@ -816,8 +826,9 @@ static void ps2_mouse_event(DeviceState *dev, QemuConsole *src,
     InputBtnEvent *btn;
 
     /* check if deltas are recorded when disabled */
-    if (!(s->mouse_status & MOUSE_STATUS_ENABLED))
+    if (!(s->mouse_status & MOUSE_STATUS_ENABLED)) {
         return;
+    }
 
     switch (evt->type) {
     case INPUT_EVENT_KIND_REL:
@@ -868,8 +879,10 @@ static void ps2_mouse_sync(DeviceState *dev)
         qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
     }
     if (!(s->mouse_status & MOUSE_STATUS_REMOTE)) {
-        /* if not remote, send event. Multiple events are sent if
-           too big deltas */
+        /*
+         * if not remote, send event. Multiple events are sent if
+         * too big deltas
+         */
         while (ps2_mouse_send_packet(s)) {
             if (s->mouse_dx == 0 && s->mouse_dy == 0
                     && s->mouse_dz == 0 && s->mouse_dw == 0) {
@@ -892,7 +905,7 @@ void ps2_write_mouse(void *opaque, int val)
     PS2MouseState *s = (PS2MouseState *)opaque;
 
     trace_ps2_write_mouse(opaque, val);
-    switch(s->common.write_cmd) {
+    switch (s->common.write_cmd) {
     default:
     case -1:
         /* mouse command */
@@ -906,7 +919,7 @@ void ps2_write_mouse(void *opaque, int val)
                 return;
             }
         }
-        switch(val) {
+        switch (val) {
         case AUX_SET_SCALE11:
             s->mouse_status &= ~MOUSE_STATUS_SCALE21;
             ps2_queue(&s->common, AUX_ACK);
@@ -980,28 +993,32 @@ void ps2_write_mouse(void *opaque, int val)
     case AUX_SET_SAMPLE:
         s->mouse_sample_rate = val;
         /* detect IMPS/2 or IMEX */
-        switch(s->mouse_detect_state) {
+        switch (s->mouse_detect_state) {
         default:
         case 0:
-            if (val == 200)
+            if (val == 200) {
                 s->mouse_detect_state = 1;
+            }
             break;
         case 1:
-            if (val == 100)
+            if (val == 100) {
                 s->mouse_detect_state = 2;
-            else if (val == 200)
+            } else if (val == 200) {
                 s->mouse_detect_state = 3;
-            else
+            } else {
                 s->mouse_detect_state = 0;
+            }
             break;
         case 2:
-            if (val == 80)
+            if (val == 80) {
                 s->mouse_type = 3; /* IMPS/2 */
+            }
             s->mouse_detect_state = 0;
             break;
         case 3:
-            if (val == 80)
+            if (val == 80) {
                 s->mouse_type = 4; /* IMEX */
+            }
             s->mouse_detect_state = 0;
             break;
         }
@@ -1154,13 +1171,14 @@ static const VMStateDescription vmstate_ps2_keyboard_cqueue = {
     }
 };
 
-static int ps2_kbd_post_load(void* opaque, int version_id)
+static int ps2_kbd_post_load(void *opaque, int version_id)
 {
-    PS2KbdState *s = (PS2KbdState*)opaque;
+    PS2KbdState *s = (PS2KbdState *)opaque;
     PS2State *ps2 = &s->common;
 
-    if (version_id == 2)
-        s->scancode_set=2;
+    if (version_id == 2) {
+        s->scancode_set = 2;
+    }
 
     ps2_common_post_load(ps2);
 
@@ -1176,10 +1194,10 @@ static const VMStateDescription vmstate_ps2_keyboard = {
         VMSTATE_STRUCT(common, PS2KbdState, 0, vmstate_ps2_common, PS2State),
         VMSTATE_INT32(scan_enabled, PS2KbdState),
         VMSTATE_INT32(translate, PS2KbdState),
-        VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
+        VMSTATE_INT32_V(scancode_set, PS2KbdState, 3),
         VMSTATE_END_OF_LIST()
     },
-    .subsections = (const VMStateDescription*[]) {
+    .subsections = (const VMStateDescription * []) {
         &vmstate_ps2_keyboard_ledstate,
         &vmstate_ps2_keyboard_need_high_bit,
         &vmstate_ps2_keyboard_cqueue,
-- 
2.20.1

[PATCH 02/50] ps2: QOMify PS2State

[Mark Cave-Ayland]

Make PS2State a new abstract PS2_DEVICE QOM type to represent the common
functionality shared between PS2 keyboard and mouse devices.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 67dd2eca84..514e55cbb6 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
+#include "hw/sysbus.h"
 #include "hw/input/ps2.h"
 #include "migration/vmstate.h"
 #include "ui/console.h"
@@ -96,12 +97,17 @@ typedef struct {
 } PS2Queue;
 
 struct PS2State {
+    SysBusDevice parent_obj;
+
     PS2Queue queue;
     int32_t write_cmd;
     void (*update_irq)(void *, int);
     void *update_arg;
 };
 
+#define TYPE_PS2_DEVICE "ps2-device"
+OBJECT_DECLARE_SIMPLE_TYPE(PS2State, PS2_DEVICE)
+
 typedef struct {
     PS2State common;
     int scan_enabled;
@@ -1277,3 +1283,25 @@ void *ps2_mouse_init(void (*update_irq)(void *, int), void *update_arg)
     qemu_register_reset(ps2_mouse_reset, s);
     return s;
 }
+
+static void ps2_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+}
+
+static const TypeInfo ps2_info = {
+    .name          = TYPE_PS2_DEVICE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(PS2State),
+    .class_init    = ps2_class_init,
+    .abstract      = true
+};
+
+static void ps2_register_types(void)
+{
+    type_register_static(&ps2_info);
+}
+
+type_init(ps2_register_types)
-- 
2.20.1

[PATCH 03/50] ps2: QOMify PS2KbdState

[Mark Cave-Ayland]

Make PS2KbdState into a new PS2_KBD_DEVICE QOM type which inherits from the
abstract PS2_DEVICE type.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c | 104 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 65 insertions(+), 39 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 514e55cbb6..14eb777c3f 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -31,6 +31,7 @@
 #include "ui/input.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
+#include "qapi/error.h"
 
 #include "trace.h"
 
@@ -108,15 +109,19 @@ struct PS2State {
 #define TYPE_PS2_DEVICE "ps2-device"
 OBJECT_DECLARE_SIMPLE_TYPE(PS2State, PS2_DEVICE)
 
-typedef struct {
-    PS2State common;
+struct PS2KbdState {
+    PS2State parent_obj;
+
     int scan_enabled;
     int translate;
     int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
     int ledstate;
     bool need_high_bit;
     unsigned int modifiers; /* bitmask of MOD_* constants above */
-} PS2KbdState;
+};
+
+#define TYPE_PS2_KBD_DEVICE "ps2-kbd"
+OBJECT_DECLARE_SIMPLE_TYPE(PS2KbdState, PS2_KBD_DEVICE)
 
 typedef struct {
     PS2State common;
@@ -330,6 +335,7 @@ static void ps2_cqueue_reset(PS2State *s)
 static void ps2_put_keycode(void *opaque, int keycode)
 {
     PS2KbdState *s = opaque;
+    PS2State *ps = PS2_DEVICE(s);
 
     trace_ps2_put_keycode(opaque, keycode);
     qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
@@ -338,13 +344,13 @@ static void ps2_put_keycode(void *opaque, int keycode)
         if (keycode == 0xf0) {
             s->need_high_bit = true;
         } else if (s->need_high_bit) {
-            ps2_queue(&s->common, translate_table[keycode] | 0x80);
+            ps2_queue(ps, translate_table[keycode] | 0x80);
             s->need_high_bit = false;
         } else {
-            ps2_queue(&s->common, translate_table[keycode]);
+            ps2_queue(ps, translate_table[keycode]);
         }
     } else {
-        ps2_queue(&s->common, keycode);
+        ps2_queue(ps, keycode);
     }
 }
 
@@ -617,96 +623,99 @@ static void ps2_set_ledstate(PS2KbdState *s, int ledstate)
 
 static void ps2_reset_keyboard(PS2KbdState *s)
 {
+    PS2State *ps2 = PS2_DEVICE(s);
+
     trace_ps2_reset_keyboard(s);
     s->scan_enabled = 1;
     s->scancode_set = 2;
-    ps2_reset_queue(&s->common);
+    ps2_reset_queue(ps2);
     ps2_set_ledstate(s, 0);
 }
 
 void ps2_write_keyboard(void *opaque, int val)
 {
     PS2KbdState *s = (PS2KbdState *)opaque;
+    PS2State *ps2 = PS2_DEVICE(s);
 
     trace_ps2_write_keyboard(opaque, val);
-    ps2_cqueue_reset(&s->common);
-    switch (s->common.write_cmd) {
+    ps2_cqueue_reset(ps2);
+    switch (ps2->write_cmd) {
     default:
     case -1:
         switch (val) {
         case 0x00:
-            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(ps2, KBD_REPLY_ACK);
             break;
         case 0x05:
-            ps2_cqueue_1(&s->common, KBD_REPLY_RESEND);
+            ps2_cqueue_1(ps2, KBD_REPLY_RESEND);
             break;
         case KBD_CMD_GET_ID:
             /* We emulate a MF2 AT keyboard here */
-            ps2_cqueue_3(&s->common, KBD_REPLY_ACK, KBD_REPLY_ID,
+            ps2_cqueue_3(ps2, KBD_REPLY_ACK, KBD_REPLY_ID,
                          s->translate ? 0x41 : 0x83);
             break;
         case KBD_CMD_ECHO:
-            ps2_cqueue_1(&s->common, KBD_CMD_ECHO);
+            ps2_cqueue_1(ps2, KBD_CMD_ECHO);
             break;
         case KBD_CMD_ENABLE:
             s->scan_enabled = 1;
-            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(ps2, KBD_REPLY_ACK);
             break;
         case KBD_CMD_SCANCODE:
         case KBD_CMD_SET_LEDS:
         case KBD_CMD_SET_RATE:
         case KBD_CMD_SET_MAKE_BREAK:
-            s->common.write_cmd = val;
-            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
+            ps2->write_cmd = val;
+            ps2_cqueue_1(ps2, KBD_REPLY_ACK);
             break;
         case KBD_CMD_RESET_DISABLE:
             ps2_reset_keyboard(s);
             s->scan_enabled = 0;
-            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(ps2, KBD_REPLY_ACK);
             break;
         case KBD_CMD_RESET_ENABLE:
             ps2_reset_keyboard(s);
             s->scan_enabled = 1;
-            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(ps2, KBD_REPLY_ACK);
             break;
         case KBD_CMD_RESET:
             ps2_reset_keyboard(s);
-            ps2_cqueue_2(&s->common,
+            ps2_cqueue_2(ps2,
                          KBD_REPLY_ACK,
                          KBD_REPLY_POR);
             break;
         case KBD_CMD_SET_TYPEMATIC:
-            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(ps2, KBD_REPLY_ACK);
             break;
         default:
-            ps2_cqueue_1(&s->common, KBD_REPLY_RESEND);
+            ps2_cqueue_1(ps2, KBD_REPLY_RESEND);
             break;
         }
         break;
     case KBD_CMD_SET_MAKE_BREAK:
-        ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
-        s->common.write_cmd = -1;
+        ps2_cqueue_1(ps2, KBD_REPLY_ACK);
+        ps2->write_cmd = -1;
         break;
     case KBD_CMD_SCANCODE:
         if (val == 0) {
-            ps2_cqueue_2(&s->common, KBD_REPLY_ACK, s->translate ?
+            ps2_cqueue_2(ps2, KBD_REPLY_ACK, s->translate ?
                 translate_table[s->scancode_set] : s->scancode_set);
         } else if (val >= 1 && val <= 3) {
             s->scancode_set = val;
-            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(ps2, KBD_REPLY_ACK);
         } else {
-            ps2_cqueue_1(&s->common, KBD_REPLY_RESEND);
+            ps2_cqueue_1(ps2, KBD_REPLY_RESEND);
         }
-        s->common.write_cmd = -1;
+        ps2->write_cmd = -1;
         break;
     case KBD_CMD_SET_LEDS:
         ps2_set_ledstate(s, val);
-        ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
-        s->common.write_cmd = -1;
+        ps2_cqueue_1(ps2, KBD_REPLY_ACK);
+        ps2->write_cmd = -1;
         break;
     case KBD_CMD_SET_RATE:
-        ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
-        s->common.write_cmd = -1;
+        ps2_cqueue_1(ps2, KBD_REPLY_ACK);
+        ps2->write_cmd = -1;
         break;
     }
 }
@@ -1075,9 +1084,10 @@ static void ps2_common_post_load(PS2State *s)
 static void ps2_kbd_reset(void *opaque)
 {
     PS2KbdState *s = (PS2KbdState *) opaque;
+    PS2State *ps2 = PS2_DEVICE(s);
 
     trace_ps2_kbd_reset(opaque);
-    ps2_common_reset(&s->common);
+    ps2_common_reset(ps2);
     s->scan_enabled = 1;
     s->translate = 0;
     s->scancode_set = 2;
@@ -1164,15 +1174,16 @@ static const VMStateDescription vmstate_ps2_keyboard_need_high_bit = {
 static bool ps2_keyboard_cqueue_needed(void *opaque)
 {
     PS2KbdState *s = opaque;
+    PS2State *ps2 = PS2_DEVICE(s);
 
-    return s->common.queue.cwptr != -1; /* the queue is mostly empty */
+    return ps2->queue.cwptr != -1; /* the queue is mostly empty */
 }
 
 static const VMStateDescription vmstate_ps2_keyboard_cqueue = {
     .name = "ps2kbd/command_reply_queue",
     .needed = ps2_keyboard_cqueue_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_INT32(common.queue.cwptr, PS2KbdState),
+        VMSTATE_INT32(parent_obj.queue.cwptr, PS2KbdState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -1180,7 +1191,7 @@ static const VMStateDescription vmstate_ps2_keyboard_cqueue = {
 static int ps2_kbd_post_load(void *opaque, int version_id)
 {
     PS2KbdState *s = (PS2KbdState *)opaque;
-    PS2State *ps2 = &s->common;
+    PS2State *ps2 = PS2_DEVICE(s);
 
     if (version_id == 2) {
         s->scancode_set = 2;
@@ -1197,7 +1208,8 @@ static const VMStateDescription vmstate_ps2_keyboard = {
     .minimum_version_id = 2,
     .post_load = ps2_kbd_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_STRUCT(common, PS2KbdState, 0, vmstate_ps2_common, PS2State),
+        VMSTATE_STRUCT(parent_obj, PS2KbdState, 0, vmstate_ps2_common,
+                       PS2State),
         VMSTATE_INT32(scan_enabled, PS2KbdState),
         VMSTATE_INT32(translate, PS2KbdState),
         VMSTATE_INT32_V(scancode_set, PS2KbdState, 3),
@@ -1250,11 +1262,18 @@ static QemuInputHandler ps2_keyboard_handler = {
 
 void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
 {
-    PS2KbdState *s = g_new0(PS2KbdState, 1);
+    DeviceState *dev;
+    PS2KbdState *s;
+    PS2State *ps2;
+
+    dev = qdev_new(TYPE_PS2_KBD_DEVICE);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    s = PS2_KBD_DEVICE(dev);
+    ps2 = PS2_DEVICE(s);
 
     trace_ps2_kbd_init(s);
-    s->common.update_irq = update_irq;
-    s->common.update_arg = update_arg;
+    ps2->update_irq = update_irq;
+    ps2->update_arg = update_arg;
     s->scancode_set = 2;
     vmstate_register(NULL, 0, &vmstate_ps2_keyboard, s);
     qemu_input_handler_register((DeviceState *)s,
@@ -1284,6 +1303,12 @@ void *ps2_mouse_init(void (*update_irq)(void *, int), void *update_arg)
     return s;
 }
 
+static const TypeInfo ps2_kbd_info = {
+    .name          = TYPE_PS2_KBD_DEVICE,
+    .parent        = TYPE_PS2_DEVICE,
+    .instance_size = sizeof(PS2KbdState),
+};
+
 static void ps2_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1302,6 +1327,7 @@ static const TypeInfo ps2_info = {
 static void ps2_register_types(void)
 {
     type_register_static(&ps2_info);
+    type_register_static(&ps2_kbd_info);
 }
 
 type_init(ps2_register_types)
-- 
2.20.1

[PATCH 04/50] ps2: QOMify PS2MouseState

[Mark Cave-Ayland]

Make PS2MouseState into a new PS2_MOUSE_DEVICE QOM type which inherits from the
abstract PS2_DEVICE type.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c | 98 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 38 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 14eb777c3f..ee7c36d4f2 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -123,8 +123,9 @@ struct PS2KbdState {
 #define TYPE_PS2_KBD_DEVICE "ps2-kbd"
 OBJECT_DECLARE_SIMPLE_TYPE(PS2KbdState, PS2_KBD_DEVICE)
 
-typedef struct {
-    PS2State common;
+struct PS2MouseState {
+    PS2State parent_obj;
+
     uint8_t mouse_status;
     uint8_t mouse_resolution;
     uint8_t mouse_sample_rate;
@@ -136,7 +137,10 @@ typedef struct {
     int mouse_dz;
     int mouse_dw;
     uint8_t mouse_buttons;
-} PS2MouseState;
+};
+
+#define TYPE_PS2_MOUSE_DEVICE "ps2-mouse"
+OBJECT_DECLARE_SIMPLE_TYPE(PS2MouseState, PS2_MOUSE_DEVICE)
 
 static uint8_t translate_table[256] = {
     0xff, 0x43, 0x41, 0x3f, 0x3d, 0x3b, 0x3c, 0x58,
@@ -735,12 +739,13 @@ void ps2_keyboard_set_translation(void *opaque, int mode)
 
 static int ps2_mouse_send_packet(PS2MouseState *s)
 {
+    PS2State *ps2 = PS2_DEVICE(s);
     /* IMPS/2 and IMEX send 4 bytes, PS2 sends 3 bytes */
     const int needed = s->mouse_type ? 4 : 3;
     unsigned int b;
     int dx1, dy1, dz1, dw1;
 
-    if (PS2_QUEUE_SIZE - s->common.queue.count < needed) {
+    if (PS2_QUEUE_SIZE - ps2->queue.count < needed) {
         return 0;
     }
 
@@ -760,9 +765,9 @@ static int ps2_mouse_send_packet(PS2MouseState *s)
         dy1 = -127;
     }
     b = 0x08 | ((dx1 < 0) << 4) | ((dy1 < 0) << 5) | (s->mouse_buttons & 0x07);
-    ps2_queue_noirq(&s->common, b);
-    ps2_queue_noirq(&s->common, dx1 & 0xff);
-    ps2_queue_noirq(&s->common, dy1 & 0xff);
+    ps2_queue_noirq(ps2, b);
+    ps2_queue_noirq(ps2, dx1 & 0xff);
+    ps2_queue_noirq(ps2, dy1 & 0xff);
     /* extra byte for IMPS/2 or IMEX */
     switch (s->mouse_type) {
     default:
@@ -776,7 +781,7 @@ static int ps2_mouse_send_packet(PS2MouseState *s)
         } else if (dz1 < -127) {
             dz1 = -127;
         }
-        ps2_queue_noirq(&s->common, dz1 & 0xff);
+        ps2_queue_noirq(ps2, dz1 & 0xff);
         s->mouse_dz -= dz1;
         s->mouse_dw = 0;
         break;
@@ -812,11 +817,11 @@ static int ps2_mouse_send_packet(PS2MouseState *s)
             b = (dz1 & 0x0f) | ((s->mouse_buttons & 0x18) << 1);
             s->mouse_dz -= dz1;
         }
-        ps2_queue_noirq(&s->common, b);
+        ps2_queue_noirq(ps2, b);
         break;
     }
 
-    ps2_raise_irq(&s->common);
+    ps2_raise_irq(ps2);
 
     trace_ps2_mouse_send_packet(s, dx1, dy1, dz1, b);
     /* update deltas */
@@ -918,85 +923,86 @@ void ps2_mouse_fake_event(void *opaque)
 void ps2_write_mouse(void *opaque, int val)
 {
     PS2MouseState *s = (PS2MouseState *)opaque;
+    PS2State *ps2 = PS2_DEVICE(s);
 
     trace_ps2_write_mouse(opaque, val);
-    switch (s->common.write_cmd) {
+    switch (ps2->write_cmd) {
     default:
     case -1:
         /* mouse command */
         if (s->mouse_wrap) {
             if (val == AUX_RESET_WRAP) {
                 s->mouse_wrap = 0;
-                ps2_queue(&s->common, AUX_ACK);
+                ps2_queue(ps2, AUX_ACK);
                 return;
             } else if (val != AUX_RESET) {
-                ps2_queue(&s->common, val);
+                ps2_queue(ps2, val);
                 return;
             }
         }
         switch (val) {
         case AUX_SET_SCALE11:
             s->mouse_status &= ~MOUSE_STATUS_SCALE21;
-            ps2_queue(&s->common, AUX_ACK);
+            ps2_queue(ps2, AUX_ACK);
             break;
         case AUX_SET_SCALE21:
             s->mouse_status |= MOUSE_STATUS_SCALE21;
-            ps2_queue(&s->common, AUX_ACK);
+            ps2_queue(ps2, AUX_ACK);
             break;
         case AUX_SET_STREAM:
             s->mouse_status &= ~MOUSE_STATUS_REMOTE;
-            ps2_queue(&s->common, AUX_ACK);
+            ps2_queue(ps2, AUX_ACK);
             break;
         case AUX_SET_WRAP:
             s->mouse_wrap = 1;
-            ps2_queue(&s->common, AUX_ACK);
+            ps2_queue(ps2, AUX_ACK);
             break;
         case AUX_SET_REMOTE:
             s->mouse_status |= MOUSE_STATUS_REMOTE;
-            ps2_queue(&s->common, AUX_ACK);
+            ps2_queue(ps2, AUX_ACK);
             break;
         case AUX_GET_TYPE:
-            ps2_queue_2(&s->common,
+            ps2_queue_2(ps2,
                 AUX_ACK,
                 s->mouse_type);
             break;
         case AUX_SET_RES:
         case AUX_SET_SAMPLE:
-            s->common.write_cmd = val;
-            ps2_queue(&s->common, AUX_ACK);
+            ps2->write_cmd = val;
+            ps2_queue(ps2, AUX_ACK);
             break;
         case AUX_GET_SCALE:
-            ps2_queue_4(&s->common,
+            ps2_queue_4(ps2,
                 AUX_ACK,
                 s->mouse_status,
                 s->mouse_resolution,
                 s->mouse_sample_rate);
             break;
         case AUX_POLL:
-            ps2_queue(&s->common, AUX_ACK);
+            ps2_queue(ps2, AUX_ACK);
             ps2_mouse_send_packet(s);
             break;
         case AUX_ENABLE_DEV:
             s->mouse_status |= MOUSE_STATUS_ENABLED;
-            ps2_queue(&s->common, AUX_ACK);
+            ps2_queue(ps2, AUX_ACK);
             break;
         case AUX_DISABLE_DEV:
             s->mouse_status &= ~MOUSE_STATUS_ENABLED;
-            ps2_queue(&s->common, AUX_ACK);
+            ps2_queue(ps2, AUX_ACK);
             break;
         case AUX_SET_DEFAULT:
             s->mouse_sample_rate = 100;
             s->mouse_resolution = 2;
             s->mouse_status = 0;
-            ps2_queue(&s->common, AUX_ACK);
+            ps2_queue(ps2, AUX_ACK);
             break;
         case AUX_RESET:
             s->mouse_sample_rate = 100;
             s->mouse_resolution = 2;
             s->mouse_status = 0;
             s->mouse_type = 0;
-            ps2_reset_queue(&s->common);
-            ps2_queue_3(&s->common,
+            ps2_reset_queue(ps2);
+            ps2_queue_3(ps2,
                 AUX_ACK,
                 0xaa,
                 s->mouse_type);
@@ -1037,13 +1043,13 @@ void ps2_write_mouse(void *opaque, int val)
             s->mouse_detect_state = 0;
             break;
         }
-        ps2_queue(&s->common, AUX_ACK);
-        s->common.write_cmd = -1;
+        ps2_queue(ps2, AUX_ACK);
+        ps2->write_cmd = -1;
         break;
     case AUX_SET_RES:
         s->mouse_resolution = val;
-        ps2_queue(&s->common, AUX_ACK);
-        s->common.write_cmd = -1;
+        ps2_queue(ps2, AUX_ACK);
+        ps2->write_cmd = -1;
         break;
     }
 }
@@ -1097,9 +1103,10 @@ static void ps2_kbd_reset(void *opaque)
 static void ps2_mouse_reset(void *opaque)
 {
     PS2MouseState *s = (PS2MouseState *) opaque;
+    PS2State *ps2 = PS2_DEVICE(s);
 
     trace_ps2_mouse_reset(opaque);
-    ps2_common_reset(&s->common);
+    ps2_common_reset(ps2);
     s->mouse_status = 0;
     s->mouse_resolution = 0;
     s->mouse_sample_rate = 0;
@@ -1226,7 +1233,7 @@ static const VMStateDescription vmstate_ps2_keyboard = {
 static int ps2_mouse_post_load(void *opaque, int version_id)
 {
     PS2MouseState *s = (PS2MouseState *)opaque;
-    PS2State *ps2 = &s->common;
+    PS2State *ps2 = PS2_DEVICE(s);
 
     ps2_common_post_load(ps2);
 
@@ -1239,7 +1246,8 @@ static const VMStateDescription vmstate_ps2_mouse = {
     .minimum_version_id = 2,
     .post_load = ps2_mouse_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_STRUCT(common, PS2MouseState, 0, vmstate_ps2_common, PS2State),
+        VMSTATE_STRUCT(parent_obj, PS2MouseState, 0, vmstate_ps2_common,
+                       PS2State),
         VMSTATE_UINT8(mouse_status, PS2MouseState),
         VMSTATE_UINT8(mouse_resolution, PS2MouseState),
         VMSTATE_UINT8(mouse_sample_rate, PS2MouseState),
@@ -1291,11 +1299,18 @@ static QemuInputHandler ps2_mouse_handler = {
 
 void *ps2_mouse_init(void (*update_irq)(void *, int), void *update_arg)
 {
-    PS2MouseState *s = g_new0(PS2MouseState, 1);
+    DeviceState *dev;
+    PS2MouseState *s;
+    PS2State *ps2;
+
+    dev = qdev_new(TYPE_PS2_MOUSE_DEVICE);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    s = PS2_MOUSE_DEVICE(dev);
+    ps2 = PS2_DEVICE(s);
 
     trace_ps2_mouse_init(s);
-    s->common.update_irq = update_irq;
-    s->common.update_arg = update_arg;
+    ps2->update_irq = update_irq;
+    ps2->update_arg = update_arg;
     vmstate_register(NULL, 0, &vmstate_ps2_mouse, s);
     qemu_input_handler_register((DeviceState *)s,
                                 &ps2_mouse_handler);
@@ -1309,6 +1324,12 @@ static const TypeInfo ps2_kbd_info = {
     .instance_size = sizeof(PS2KbdState),
 };
 
+static const TypeInfo ps2_mouse_info = {
+    .name          = TYPE_PS2_MOUSE_DEVICE,
+    .parent        = TYPE_PS2_DEVICE,
+    .instance_size = sizeof(PS2MouseState),
+};
+
 static void ps2_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1328,6 +1349,7 @@ static void ps2_register_types(void)
 {
     type_register_static(&ps2_info);
     type_register_static(&ps2_kbd_info);
+    type_register_static(&ps2_mouse_info);
 }
 
 type_init(ps2_register_types)
-- 
2.20.1

[PATCH 05/50] ps2: move QOM type definitions from ps2.c to ps2.h

[Mark Cave-Ayland]

Move the QOM type definitions into the ps2.h header file to allow the new QOM
types to be used by other devices.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c         | 55 ---------------------------------------
 include/hw/input/ps2.h | 58 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 56 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index ee7c36d4f2..f4bad9876a 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -76,11 +76,6 @@
 #define MOUSE_STATUS_ENABLED    0x20
 #define MOUSE_STATUS_SCALE21    0x10
 
-/*
- * PS/2 buffer size. Keep 256 bytes for compatibility with
- * older QEMU versions.
- */
-#define PS2_BUFFER_SIZE     256
 #define PS2_QUEUE_SIZE      16  /* Queue size required by PS/2 protocol */
 #define PS2_QUEUE_HEADROOM  8   /* Queue size for keyboard command replies */
 
@@ -92,56 +87,6 @@
 #define MOD_SHIFT_R (1 << 4)
 #define MOD_ALT_R   (1 << 5)
 
-typedef struct {
-    uint8_t data[PS2_BUFFER_SIZE];
-    int rptr, wptr, cwptr, count;
-} PS2Queue;
-
-struct PS2State {
-    SysBusDevice parent_obj;
-
-    PS2Queue queue;
-    int32_t write_cmd;
-    void (*update_irq)(void *, int);
-    void *update_arg;
-};
-
-#define TYPE_PS2_DEVICE "ps2-device"
-OBJECT_DECLARE_SIMPLE_TYPE(PS2State, PS2_DEVICE)
-
-struct PS2KbdState {
-    PS2State parent_obj;
-
-    int scan_enabled;
-    int translate;
-    int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
-    int ledstate;
-    bool need_high_bit;
-    unsigned int modifiers; /* bitmask of MOD_* constants above */
-};
-
-#define TYPE_PS2_KBD_DEVICE "ps2-kbd"
-OBJECT_DECLARE_SIMPLE_TYPE(PS2KbdState, PS2_KBD_DEVICE)
-
-struct PS2MouseState {
-    PS2State parent_obj;
-
-    uint8_t mouse_status;
-    uint8_t mouse_resolution;
-    uint8_t mouse_sample_rate;
-    uint8_t mouse_wrap;
-    uint8_t mouse_type; /* 0 = PS2, 3 = IMPS/2, 4 = IMEX */
-    uint8_t mouse_detect_state;
-    int mouse_dx; /* current values, needed for 'poll' mode */
-    int mouse_dy;
-    int mouse_dz;
-    int mouse_dw;
-    uint8_t mouse_buttons;
-};
-
-#define TYPE_PS2_MOUSE_DEVICE "ps2-mouse"
-OBJECT_DECLARE_SIMPLE_TYPE(PS2MouseState, PS2_MOUSE_DEVICE)
-
 static uint8_t translate_table[256] = {
     0xff, 0x43, 0x41, 0x3f, 0x3d, 0x3b, 0x3c, 0x58,
     0x64, 0x44, 0x42, 0x40, 0x3e, 0x0f, 0x29, 0x59,
diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h
index 35d983897a..7f2c3f6090 100644
--- a/include/hw/input/ps2.h
+++ b/include/hw/input/ps2.h
@@ -25,13 +25,69 @@
 #ifndef HW_PS2_H
 #define HW_PS2_H
 
+#include "hw/sysbus.h"
+
 #define PS2_MOUSE_BUTTON_LEFT   0x01
 #define PS2_MOUSE_BUTTON_RIGHT  0x02
 #define PS2_MOUSE_BUTTON_MIDDLE 0x04
 #define PS2_MOUSE_BUTTON_SIDE   0x08
 #define PS2_MOUSE_BUTTON_EXTRA  0x10
 
-typedef struct PS2State PS2State;
+/*
+ * PS/2 buffer size. Keep 256 bytes for compatibility with
+ * older QEMU versions.
+ */
+#define PS2_BUFFER_SIZE     256
+
+typedef struct {
+    uint8_t data[PS2_BUFFER_SIZE];
+    int rptr, wptr, cwptr, count;
+} PS2Queue;
+
+struct PS2State {
+    SysBusDevice parent_obj;
+
+    PS2Queue queue;
+    int32_t write_cmd;
+    void (*update_irq)(void *, int);
+    void *update_arg;
+};
+
+#define TYPE_PS2_DEVICE "ps2-device"
+OBJECT_DECLARE_SIMPLE_TYPE(PS2State, PS2_DEVICE)
+
+struct PS2KbdState {
+    PS2State parent_obj;
+
+    int scan_enabled;
+    int translate;
+    int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
+    int ledstate;
+    bool need_high_bit;
+    unsigned int modifiers; /* bitmask of MOD_* constants above */
+};
+
+#define TYPE_PS2_KBD_DEVICE "ps2-kbd"
+OBJECT_DECLARE_SIMPLE_TYPE(PS2KbdState, PS2_KBD_DEVICE)
+
+struct PS2MouseState {
+    PS2State parent_obj;
+
+    uint8_t mouse_status;
+    uint8_t mouse_resolution;
+    uint8_t mouse_sample_rate;
+    uint8_t mouse_wrap;
+    uint8_t mouse_type; /* 0 = PS2, 3 = IMPS/2, 4 = IMEX */
+    uint8_t mouse_detect_state;
+    int mouse_dx; /* current values, needed for 'poll' mode */
+    int mouse_dy;
+    int mouse_dz;
+    int mouse_dw;
+    uint8_t mouse_buttons;
+};
+
+#define TYPE_PS2_MOUSE_DEVICE "ps2-mouse"
+OBJECT_DECLARE_SIMPLE_TYPE(PS2MouseState, PS2_MOUSE_DEVICE)
 
 /* ps2.c */
 void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg);
-- 
2.20.1

[PATCH 06/50] ps2: improve function prototypes in ps2.c and ps2.h

[Mark Cave-Ayland]

With the latest changes it is now possible to improve some of the function
prototypes in ps2.c and ps.h to use the appropriate PS2KbdState or
PS2MouseState type instead of being a void opaque.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c         | 22 +++++++++-------------
 include/hw/input/ps2.h |  8 ++++----
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index f4bad9876a..3a770f3b78 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -581,12 +581,11 @@ static void ps2_reset_keyboard(PS2KbdState *s)
     ps2_set_ledstate(s, 0);
 }
 
-void ps2_write_keyboard(void *opaque, int val)
+void ps2_write_keyboard(PS2KbdState *s, int val)
 {
-    PS2KbdState *s = (PS2KbdState *)opaque;
     PS2State *ps2 = PS2_DEVICE(s);
 
-    trace_ps2_write_keyboard(opaque, val);
+    trace_ps2_write_keyboard(s, val);
     ps2_cqueue_reset(ps2);
     switch (ps2->write_cmd) {
     default:
@@ -675,10 +674,9 @@ void ps2_write_keyboard(void *opaque, int val)
  * 1 = translated scancodes (used by qemu internally).
  */
 
-void ps2_keyboard_set_translation(void *opaque, int mode)
+void ps2_keyboard_set_translation(PS2KbdState *s, int mode)
 {
-    PS2KbdState *s = (PS2KbdState *)opaque;
-    trace_ps2_keyboard_set_translation(opaque, mode);
+    trace_ps2_keyboard_set_translation(s, mode);
     s->translate = mode;
 }
 
@@ -857,20 +855,18 @@ static void ps2_mouse_sync(DeviceState *dev)
     }
 }
 
-void ps2_mouse_fake_event(void *opaque)
+void ps2_mouse_fake_event(PS2MouseState *s)
 {
-    PS2MouseState *s = opaque;
-    trace_ps2_mouse_fake_event(opaque);
+    trace_ps2_mouse_fake_event(s);
     s->mouse_dx++;
-    ps2_mouse_sync(opaque);
+    ps2_mouse_sync(DEVICE(s));
 }
 
-void ps2_write_mouse(void *opaque, int val)
+void ps2_write_mouse(PS2MouseState *s, int val)
 {
-    PS2MouseState *s = (PS2MouseState *)opaque;
     PS2State *ps2 = PS2_DEVICE(s);
 
-    trace_ps2_write_mouse(opaque, val);
+    trace_ps2_write_mouse(s, val);
     switch (ps2->write_cmd) {
     default:
     case -1:
diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h
index 7f2c3f6090..1a3321d77e 100644
--- a/include/hw/input/ps2.h
+++ b/include/hw/input/ps2.h
@@ -92,8 +92,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(PS2MouseState, PS2_MOUSE_DEVICE)
 /* ps2.c */
 void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg);
 void *ps2_mouse_init(void (*update_irq)(void *, int), void *update_arg);
-void ps2_write_mouse(void *, int val);
-void ps2_write_keyboard(void *, int val);
+void ps2_write_mouse(PS2MouseState *s, int val);
+void ps2_write_keyboard(PS2KbdState *s, int val);
 uint32_t ps2_read_data(PS2State *s);
 void ps2_queue_noirq(PS2State *s, int b);
 void ps2_raise_irq(PS2State *s);
@@ -101,8 +101,8 @@ void ps2_queue(PS2State *s, int b);
 void ps2_queue_2(PS2State *s, int b1, int b2);
 void ps2_queue_3(PS2State *s, int b1, int b2, int b3);
 void ps2_queue_4(PS2State *s, int b1, int b2, int b3, int b4);
-void ps2_keyboard_set_translation(void *opaque, int mode);
-void ps2_mouse_fake_event(void *opaque);
+void ps2_keyboard_set_translation(PS2KbdState *s, int mode);
+void ps2_mouse_fake_event(PS2MouseState *s);
 int ps2_queue_empty(PS2State *s);
 
 #endif /* HW_PS2_H */
-- 
2.20.1

[PATCH 07/50] ps2: introduce PS2DeviceClass

[Mark Cave-Ayland]

This is in preparation for allowing the new PS2_KBD_DEVICE and PS2_MOUSE_DEVICE
QOM types to reference the parent PS2_DEVICE device reset() function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 include/hw/input/ps2.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h
index 1a3321d77e..0493b78812 100644
--- a/include/hw/input/ps2.h
+++ b/include/hw/input/ps2.h
@@ -33,6 +33,10 @@
 #define PS2_MOUSE_BUTTON_SIDE   0x08
 #define PS2_MOUSE_BUTTON_EXTRA  0x10
 
+struct PS2DeviceClass {
+    DeviceClass parent_class;
+};
+
 /*
  * PS/2 buffer size. Keep 256 bytes for compatibility with
  * older QEMU versions.
@@ -54,7 +58,7 @@ struct PS2State {
 };
 
 #define TYPE_PS2_DEVICE "ps2-device"
-OBJECT_DECLARE_SIMPLE_TYPE(PS2State, PS2_DEVICE)
+OBJECT_DECLARE_TYPE(PS2State, PS2DeviceClass, PS2_DEVICE)
 
 struct PS2KbdState {
     PS2State parent_obj;
-- 
2.20.1

[PATCH 08/50] ps2: implement ps2_reset() for the PS2_DEVICE QOM type based upon ps2_common_reset()

[Mark Cave-Ayland]

The functionality of ps2_common_reset() can be moved into a new ps2_reset() function
for the PS2_DEVICE QOM type. Update PS2DeviceClass to hold a reference to the parent
reset function and update the PS2_KBD_DEVICE and PS2_MOUSE_DEVICE types to use
device_class_set_parent_reset() accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c         | 48 ++++++++++++++++++++++++++++++------------
 include/hw/input/ps2.h |  2 ++
 2 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 3a770f3b78..5990eb6f79 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -995,8 +995,10 @@ void ps2_write_mouse(PS2MouseState *s, int val)
     }
 }
 
-static void ps2_common_reset(PS2State *s)
+static void ps2_reset(DeviceState *dev)
 {
+    PS2State *s = PS2_DEVICE(dev);
+
     s->write_cmd = -1;
     ps2_reset_queue(s);
     s->update_irq(s->update_arg, 0);
@@ -1028,26 +1030,28 @@ static void ps2_common_post_load(PS2State *s)
     q->cwptr = ccount ? (q->rptr + ccount) & (PS2_BUFFER_SIZE - 1) : -1;
 }
 
-static void ps2_kbd_reset(void *opaque)
+static void ps2_kbd_reset(DeviceState *dev)
 {
-    PS2KbdState *s = (PS2KbdState *) opaque;
-    PS2State *ps2 = PS2_DEVICE(s);
+    PS2DeviceClass *ps2dc = PS2_DEVICE_GET_CLASS(dev);
+    PS2KbdState *s = PS2_KBD_DEVICE(dev);
+
+    trace_ps2_kbd_reset(s);
+    ps2dc->parent_reset(dev);
 
-    trace_ps2_kbd_reset(opaque);
-    ps2_common_reset(ps2);
     s->scan_enabled = 1;
     s->translate = 0;
     s->scancode_set = 2;
     s->modifiers = 0;
 }
 
-static void ps2_mouse_reset(void *opaque)
+static void ps2_mouse_reset(DeviceState *dev)
 {
-    PS2MouseState *s = (PS2MouseState *) opaque;
-    PS2State *ps2 = PS2_DEVICE(s);
+    PS2DeviceClass *ps2dc = PS2_DEVICE_GET_CLASS(dev);
+    PS2MouseState *s = PS2_MOUSE_DEVICE(dev);
+
+    trace_ps2_mouse_reset(s);
+    ps2dc->parent_reset(dev);
 
-    trace_ps2_mouse_reset(opaque);
-    ps2_common_reset(ps2);
     s->mouse_status = 0;
     s->mouse_resolution = 0;
     s->mouse_sample_rate = 0;
@@ -1227,7 +1231,6 @@ void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
     vmstate_register(NULL, 0, &vmstate_ps2_keyboard, s);
     qemu_input_handler_register((DeviceState *)s,
                                 &ps2_keyboard_handler);
-    qemu_register_reset(ps2_kbd_reset, s);
     return s;
 }
 
@@ -1255,26 +1258,45 @@ void *ps2_mouse_init(void (*update_irq)(void *, int), void *update_arg)
     vmstate_register(NULL, 0, &vmstate_ps2_mouse, s);
     qemu_input_handler_register((DeviceState *)s,
                                 &ps2_mouse_handler);
-    qemu_register_reset(ps2_mouse_reset, s);
     return s;
 }
 
+static void ps2_kbd_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PS2DeviceClass *ps2dc = PS2_DEVICE_CLASS(klass);
+
+    device_class_set_parent_reset(dc, ps2_kbd_reset, &ps2dc->parent_reset);
+}
+
 static const TypeInfo ps2_kbd_info = {
     .name          = TYPE_PS2_KBD_DEVICE,
     .parent        = TYPE_PS2_DEVICE,
     .instance_size = sizeof(PS2KbdState),
+    .class_init    = ps2_kbd_class_init
 };
 
+static void ps2_mouse_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PS2DeviceClass *ps2dc = PS2_DEVICE_CLASS(klass);
+
+    device_class_set_parent_reset(dc, ps2_mouse_reset,
+                                  &ps2dc->parent_reset);
+}
+
 static const TypeInfo ps2_mouse_info = {
     .name          = TYPE_PS2_MOUSE_DEVICE,
     .parent        = TYPE_PS2_DEVICE,
     .instance_size = sizeof(PS2MouseState),
+    .class_init    = ps2_mouse_class_init
 };
 
 static void ps2_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->reset = ps2_reset;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
 
diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h
index 0493b78812..4e8de912c6 100644
--- a/include/hw/input/ps2.h
+++ b/include/hw/input/ps2.h
@@ -35,6 +35,8 @@
 
 struct PS2DeviceClass {
     DeviceClass parent_class;
+
+    DeviceReset parent_reset;
 };
 
 /*
-- 
2.20.1

[PATCH 09/50] ps2: remove duplicate setting of scancode_set in ps2_kbd_init()

[Mark Cave-Ayland]

The default value for scancode_set is already set in ps2_reset() so there is no
need to duplicate this in ps2_kbd_init().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 5990eb6f79..555abb5392 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -1227,7 +1227,6 @@ void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
     trace_ps2_kbd_init(s);
     ps2->update_irq = update_irq;
     ps2->update_arg = update_arg;
-    s->scancode_set = 2;
     vmstate_register(NULL, 0, &vmstate_ps2_keyboard, s);
     qemu_input_handler_register((DeviceState *)s,
                                 &ps2_keyboard_handler);
-- 
2.20.1

Re: [PATCH 09/50] ps2: remove duplicate setting of scancode_set in ps2_kbd_init()

[Philippe Mathieu-Daudé via]

On 22/5/22 20:17, Mark Cave-Ayland wrote:
> The default value for scancode_set is already set in ps2_reset() so there is no

ps2_reset -> ps2_reset_keyboard ?

> need to duplicate this in ps2_kbd_init().
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/input/ps2.c | 1 -
>   1 file changed, 1 deletion(-)

Re: [PATCH 09/50] ps2: remove duplicate setting of scancode_set in ps2_kbd_init()

[Mark Cave-Ayland]

On 22/05/2022 23:22, Philippe Mathieu-Daudé via wrote:

> On 22/5/22 20:17, Mark Cave-Ayland wrote:
>> The default value for scancode_set is already set in ps2_reset() so there is no
> 
> ps2_reset -> ps2_reset_keyboard ?

Ah yes, it's just a typo in the commit message where ps2_reset() should be changed to 
ps2_kbd_reset(). I can fix this for v2.

>> need to duplicate this in ps2_kbd_init().
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/input/ps2.c | 1 -
>>   1 file changed, 1 deletion(-)

ATB,

Mark.

[PATCH 10/50] ps2: implement ps2_kbd_realize() and use it to register ps2_keyboard_handler

[Mark Cave-Ayland]

Move the registration of ps2_keyboard_handler from ps2_kbd_init() to a new
ps2_kbd_realize() function. Since the abstract PS2_DEVICE parent class doesn't
have a realize() function then it is not necessary to store the reference to
it in PS2DeviceClass and use device_class_set_parent_realize().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 555abb5392..2abd6510ab 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -1213,6 +1213,11 @@ static QemuInputHandler ps2_keyboard_handler = {
     .event = ps2_keyboard_event,
 };
 
+static void ps2_kbd_realize(DeviceState *dev, Error **errp)
+{
+    qemu_input_handler_register(dev, &ps2_keyboard_handler);
+}
+
 void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
 {
     DeviceState *dev;
@@ -1228,8 +1233,7 @@ void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
     ps2->update_irq = update_irq;
     ps2->update_arg = update_arg;
     vmstate_register(NULL, 0, &vmstate_ps2_keyboard, s);
-    qemu_input_handler_register((DeviceState *)s,
-                                &ps2_keyboard_handler);
+
     return s;
 }
 
@@ -1265,6 +1269,7 @@ static void ps2_kbd_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_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);
 }
 
-- 
2.20.1

[PATCH 11/50] ps2: implement ps2_mouse_realize() and use it to register ps2_mouse_handler

[Mark Cave-Ayland]

Move the registration of ps2_mouse_handler from ps2_mouse_init() to a new
ps2_mouse_realize() function. Since the abstract PS2_DEVICE parent class doesn't
have a realize() function then it is not necessary to store the reference to
it in PS2DeviceClass and use device_class_set_parent_realize().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 2abd6510ab..1d223de59f 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -1244,6 +1244,11 @@ static QemuInputHandler ps2_mouse_handler = {
     .sync  = ps2_mouse_sync,
 };
 
+static void ps2_mouse_realize(DeviceState *dev, Error **errp)
+{
+    qemu_input_handler_register(dev, &ps2_mouse_handler);
+}
+
 void *ps2_mouse_init(void (*update_irq)(void *, int), void *update_arg)
 {
     DeviceState *dev;
@@ -1259,8 +1264,6 @@ void *ps2_mouse_init(void (*update_irq)(void *, int), void *update_arg)
     ps2->update_irq = update_irq;
     ps2->update_arg = update_arg;
     vmstate_register(NULL, 0, &vmstate_ps2_mouse, s);
-    qemu_input_handler_register((DeviceState *)s,
-                                &ps2_mouse_handler);
     return s;
 }
 
@@ -1285,6 +1288,7 @@ static void ps2_mouse_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_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);
 }
-- 
2.20.1

[PATCH 12/50] ps2: don't use vmstate_register() in ps2_kbd_init()

[Mark Cave-Ayland]

Since PS2_KBD_DEVICE is a qdev device then vmstate_ps2_keyboard can be registered
using the DeviceClass vmsd field instead. There is no need to use
qdev_set_legacy_instance_id() to ensure migration compatibility since the first 2
parameters to vmstate_register() are NULL and 0 respectively.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 1d223de59f..04360c7f74 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -1232,7 +1232,6 @@ void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
     trace_ps2_kbd_init(s);
     ps2->update_irq = update_irq;
     ps2->update_arg = update_arg;
-    vmstate_register(NULL, 0, &vmstate_ps2_keyboard, s);
 
     return s;
 }
@@ -1274,6 +1273,7 @@ static void ps2_kbd_class_init(ObjectClass *klass, void *data)
 
     dc->realize = ps2_kbd_realize;
     device_class_set_parent_reset(dc, ps2_kbd_reset, &ps2dc->parent_reset);
+    dc->vmsd = &vmstate_ps2_keyboard;
 }
 
 static const TypeInfo ps2_kbd_info = {
-- 
2.20.1

[PATCH 13/50] ps2: don't use vmstate_register() in ps2_mouse_init()

[Mark Cave-Ayland]

Since PS2_MOUSE_DEVICE is a qdev device then vmstate_ps2_mouse can be registered
using the DeviceClass vmsd field instead. There is no need to use
qdev_set_legacy_instance_id() to ensure migration compatibility since the first 2
parameters to vmstate_register() are NULL and 0 respectively.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 04360c7f74..e3ee69870b 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -1262,7 +1262,6 @@ void *ps2_mouse_init(void (*update_irq)(void *, int), void *update_arg)
     trace_ps2_mouse_init(s);
     ps2->update_irq = update_irq;
     ps2->update_arg = update_arg;
-    vmstate_register(NULL, 0, &vmstate_ps2_mouse, s);
     return s;
 }
 
@@ -1291,6 +1290,7 @@ static void ps2_mouse_class_init(ObjectClass *klass, void *data)
     dc->realize = ps2_mouse_realize;
     device_class_set_parent_reset(dc, ps2_mouse_reset,
                                   &ps2dc->parent_reset);
+    dc->vmsd = &vmstate_ps2_mouse;
 }
 
 static const TypeInfo ps2_mouse_info = {
-- 
2.20.1

[PATCH 14/50] pl050: checkpatch fixes

[Mark Cave-Ayland]

This patch also includes a couple of minor spacing updates.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pl050.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/input/pl050.c b/hw/input/pl050.c
index d279b6c148..889a0674d3 100644
--- a/hw/input/pl050.c
+++ b/hw/input/pl050.c
@@ -53,8 +53,9 @@ static const VMStateDescription vmstate_pl050 = {
 #define PL050_KMIC            (1 << 1)
 #define PL050_KMID            (1 << 0)
 
-static const unsigned char pl050_id[] =
-{ 0x50, 0x10, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 };
+static const unsigned char pl050_id[] = {
+    0x50, 0x10, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
+};
 
 static void pl050_update(void *opaque, int level)
 {
@@ -71,8 +72,10 @@ static uint64_t pl050_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
     PL050State *s = (PL050State *)opaque;
-    if (offset >= 0xfe0 && offset < 0x1000)
+
+    if (offset >= 0xfe0 && offset < 0x1000) {
         return pl050_id[(offset - 0xfe0) >> 2];
+    }
 
     switch (offset >> 2) {
     case 0: /* KMICR */
@@ -88,16 +91,19 @@ static uint64_t pl050_read(void *opaque, hwaddr offset,
             val = (val ^ (val >> 1)) & 1;
 
             stat = PL050_TXEMPTY;
-            if (val)
+            if (val) {
                 stat |= PL050_RXPARITY;
-            if (s->pending)
+            }
+            if (s->pending) {
                 stat |= PL050_RXFULL;
+            }
 
             return stat;
         }
     case 2: /* KMIDATA */
-        if (s->pending)
+        if (s->pending) {
             s->last = ps2_read_data(s->dev);
+        }
         return s->last;
     case 3: /* KMICLKDIV */
         return s->clk;
@@ -114,6 +120,7 @@ static void pl050_write(void *opaque, hwaddr offset,
                         uint64_t value, unsigned size)
 {
     PL050State *s = (PL050State *)opaque;
+
     switch (offset >> 2) {
     case 0: /* KMICR */
         s->cr = value;
-- 
2.20.1

[PATCH 15/50] pl050: split pl050_update_irq() into separate pl050_set_irq() and pl050_update_irq() functions

[Mark Cave-Ayland]

This will soon allow pl050_set_irq() to be used as a GPIO input function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pl050.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/input/pl050.c b/hw/input/pl050.c
index 889a0674d3..66f8c20d9f 100644
--- a/hw/input/pl050.c
+++ b/hw/input/pl050.c
@@ -57,15 +57,20 @@ static const unsigned char pl050_id[] = {
     0x50, 0x10, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
 };
 
-static void pl050_update(void *opaque, int level)
+static void pl050_update_irq(PL050State *s)
+{
+    int level = (s->pending && (s->cr & 0x10) != 0)
+                 || (s->cr & 0x08) != 0;
+
+    qemu_set_irq(s->irq, level);
+}
+
+static void pl050_set_irq(void *opaque, int level)
 {
     PL050State *s = (PL050State *)opaque;
-    int raise;
 
     s->pending = level;
-    raise = (s->pending && (s->cr & 0x10) != 0)
-            || (s->cr & 0x08) != 0;
-    qemu_set_irq(s->irq, raise);
+    pl050_update_irq(s);
 }
 
 static uint64_t pl050_read(void *opaque, hwaddr offset,
@@ -124,7 +129,7 @@ static void pl050_write(void *opaque, hwaddr offset,
     switch (offset >> 2) {
     case 0: /* KMICR */
         s->cr = value;
-        pl050_update(s, s->pending);
+        pl050_update_irq(s);
         /* ??? Need to implement the enable/disable bit.  */
         break;
     case 2: /* KMIDATA */
@@ -159,9 +164,9 @@ static void pl050_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->iomem);
     sysbus_init_irq(sbd, &s->irq);
     if (s->is_mouse) {
-        s->dev = ps2_mouse_init(pl050_update, s);
+        s->dev = ps2_mouse_init(pl050_set_irq, s);
     } else {
-        s->dev = ps2_kbd_init(pl050_update, s);
+        s->dev = ps2_kbd_init(pl050_set_irq, s);
     }
 }
 
-- 
2.20.1

[PATCH 16/50] lasips2: spacing fixes

[Mark Cave-Ayland]

This helps improve the readability of lasips2.c.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/lasips2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index 94f18be4cd..2ac3433014 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -205,7 +205,6 @@ static uint64_t lasips2_reg_read(void *opaque, hwaddr addr, unsigned size)
         break;
 
     case REG_PS2_STATUS:
-
         ret = LASIPS2_STATUS_DATSHD | LASIPS2_STATUS_CLKSHD;
 
         if (port->control & LASIPS2_CONTROL_DIAG) {
@@ -238,9 +237,9 @@ static uint64_t lasips2_reg_read(void *opaque, hwaddr addr, unsigned size)
                       __func__, addr);
         break;
     }
+
     trace_lasips2_reg_read(size, port->id, addr,
                            lasips2_read_reg_name(addr), ret);
-
     return ret;
 }
 
@@ -257,6 +256,7 @@ static const MemoryRegionOps lasips2_reg_ops = {
 static void ps2dev_update_irq(void *opaque, int level)
 {
     LASIPS2Port *port = opaque;
+
     port->irq = level;
     lasips2_update_irq(port->parent);
 }
-- 
2.20.1

[PATCH 17/50] lasips2: rename ps2dev_update_irq() to lasips2_port_set_irq()

[Mark Cave-Ayland]

This better reflects that the IRQ input opaque is a LASIPS2Port structure
and not a PS2_DEVICE.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/lasips2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index 2ac3433014..adfde1684f 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -253,7 +253,7 @@ static const MemoryRegionOps lasips2_reg_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void ps2dev_update_irq(void *opaque, int level)
+static void lasips2_port_set_irq(void *opaque, int level)
 {
     LASIPS2Port *port = opaque;
 
@@ -275,8 +275,8 @@ void lasips2_init(MemoryRegion *address_space,
 
     vmstate_register(NULL, base, &vmstate_lasips2, s);
 
-    s->kbd.dev = ps2_kbd_init(ps2dev_update_irq, &s->kbd);
-    s->mouse.dev = ps2_mouse_init(ps2dev_update_irq, &s->mouse);
+    s->kbd.dev = ps2_kbd_init(lasips2_port_set_irq, &s->kbd);
+    s->mouse.dev = ps2_mouse_init(lasips2_port_set_irq, &s->mouse);
 
     memory_region_init_io(&s->kbd.reg, NULL, &lasips2_reg_ops, &s->kbd,
                           "lasips2-kbd", 0x100);
-- 
2.20.1

[PATCH 18/50] pckbd: checkpatch fixes

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c | 150 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 97 insertions(+), 53 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 4efdf75620..ab0d66d00d 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -39,49 +39,86 @@
 
 #include "trace.h"
 
-/*	Keyboard Controller Commands */
-#define KBD_CCMD_READ_MODE	0x20	/* Read mode bits */
-#define KBD_CCMD_WRITE_MODE	0x60	/* Write mode bits */
-#define KBD_CCMD_GET_VERSION	0xA1	/* Get controller version */
-#define KBD_CCMD_MOUSE_DISABLE	0xA7	/* Disable mouse interface */
-#define KBD_CCMD_MOUSE_ENABLE	0xA8	/* Enable mouse interface */
-#define KBD_CCMD_TEST_MOUSE	0xA9	/* Mouse interface test */
-#define KBD_CCMD_SELF_TEST	0xAA	/* Controller self test */
-#define KBD_CCMD_KBD_TEST	0xAB	/* Keyboard interface test */
-#define KBD_CCMD_KBD_DISABLE	0xAD	/* Keyboard interface disable */
-#define KBD_CCMD_KBD_ENABLE	0xAE	/* Keyboard interface enable */
-#define KBD_CCMD_READ_INPORT    0xC0    /* read input port */
-#define KBD_CCMD_READ_OUTPORT	0xD0    /* read output port */
-#define KBD_CCMD_WRITE_OUTPORT	0xD1    /* write output port */
-#define KBD_CCMD_WRITE_OBUF	0xD2
-#define KBD_CCMD_WRITE_AUX_OBUF	0xD3    /* Write to output buffer as if
-                                           initiated by the auxiliary device */
-#define KBD_CCMD_WRITE_MOUSE	0xD4	/* Write the following byte to the mouse */
-#define KBD_CCMD_DISABLE_A20    0xDD    /* HP vectra only ? */
-#define KBD_CCMD_ENABLE_A20     0xDF    /* HP vectra only ? */
-#define KBD_CCMD_PULSE_BITS_3_0 0xF0    /* Pulse bits 3-0 of the output port P2. */
-#define KBD_CCMD_RESET          0xFE    /* Pulse bit 0 of the output port P2 = CPU reset. */
-#define KBD_CCMD_NO_OP          0xFF    /* Pulse no bits of the output port P2. */
+/* Keyboard Controller Commands */
+
+/* Read mode bits */
+#define KBD_CCMD_READ_MODE         0x20
+/* Write mode bits */
+#define KBD_CCMD_WRITE_MODE        0x60
+/* Get controller version */
+#define KBD_CCMD_GET_VERSION       0xA1
+/* Disable mouse interface */
+#define KBD_CCMD_MOUSE_DISABLE     0xA7
+/* Enable mouse interface */
+#define KBD_CCMD_MOUSE_ENABLE      0xA8
+/* Mouse interface test */
+#define KBD_CCMD_TEST_MOUSE        0xA9
+/* Controller self test */
+#define KBD_CCMD_SELF_TEST         0xAA
+/* Keyboard interface test */
+#define KBD_CCMD_KBD_TEST          0xAB
+/* Keyboard interface disable */
+#define KBD_CCMD_KBD_DISABLE       0xAD
+/* Keyboard interface enable */
+#define KBD_CCMD_KBD_ENABLE        0xAE
+/* read input port */
+#define KBD_CCMD_READ_INPORT       0xC0
+/* read output port */
+#define KBD_CCMD_READ_OUTPORT      0xD0
+/* write output port */
+#define KBD_CCMD_WRITE_OUTPORT     0xD1
+#define KBD_CCMD_WRITE_OBUF        0xD2
+/* Write to output buffer as if initiated by the auxiliary device */
+#define KBD_CCMD_WRITE_AUX_OBUF    0xD3
+/* Write the following byte to the mouse */
+#define KBD_CCMD_WRITE_MOUSE       0xD4
+/* HP vectra only ? */
+#define KBD_CCMD_DISABLE_A20       0xDD
+/* HP vectra only ? */
+#define KBD_CCMD_ENABLE_A20        0xDF
+/* Pulse bits 3-0 of the output port P2. */
+#define KBD_CCMD_PULSE_BITS_3_0    0xF0
+/* Pulse bit 0 of the output port P2 = CPU reset. */
+#define KBD_CCMD_RESET             0xFE
+/* Pulse no bits of the output port P2. */
+#define KBD_CCMD_NO_OP             0xFF
 
 /* Status Register Bits */
-#define KBD_STAT_OBF 		0x01	/* Keyboard output buffer full */
-#define KBD_STAT_IBF 		0x02	/* Keyboard input buffer full */
-#define KBD_STAT_SELFTEST	0x04	/* Self test successful */
-#define KBD_STAT_CMD		0x08	/* Last write was a command write (0=data) */
-#define KBD_STAT_UNLOCKED	0x10	/* Zero if keyboard locked */
-#define KBD_STAT_MOUSE_OBF	0x20	/* Mouse output buffer full */
-#define KBD_STAT_GTO 		0x40	/* General receive/xmit timeout */
-#define KBD_STAT_PERR 		0x80	/* Parity error */
+
+/* Keyboard output buffer full */
+#define KBD_STAT_OBF           0x01
+/* Keyboard input buffer full */
+#define KBD_STAT_IBF           0x02
+/* Self test successful */
+#define KBD_STAT_SELFTEST      0x04
+/* Last write was a command write (0=data) */
+#define KBD_STAT_CMD           0x08
+/* Zero if keyboard locked */
+#define KBD_STAT_UNLOCKED      0x10
+/* Mouse output buffer full */
+#define KBD_STAT_MOUSE_OBF     0x20
+/* General receive/xmit timeout */
+#define KBD_STAT_GTO           0x40
+/* Parity error */
+#define KBD_STAT_PERR          0x80
 
 /* Controller Mode Register Bits */
-#define KBD_MODE_KBD_INT	0x01	/* Keyboard data generate IRQ1 */
-#define KBD_MODE_MOUSE_INT	0x02	/* Mouse data generate IRQ12 */
-#define KBD_MODE_SYS 		0x04	/* The system flag (?) */
-#define KBD_MODE_NO_KEYLOCK	0x08	/* The keylock doesn't affect the keyboard if set */
-#define KBD_MODE_DISABLE_KBD	0x10	/* Disable keyboard interface */
-#define KBD_MODE_DISABLE_MOUSE	0x20	/* Disable mouse interface */
-#define KBD_MODE_KCC 		0x40	/* Scan code conversion to PC format */
-#define KBD_MODE_RFU		0x80
+
+/* Keyboard data generate IRQ1 */
+#define KBD_MODE_KBD_INT       0x01
+/* Mouse data generate IRQ12 */
+#define KBD_MODE_MOUSE_INT     0x02
+/* The system flag (?) */
+#define KBD_MODE_SYS           0x04
+/* The keylock doesn't affect the keyboard if set */
+#define KBD_MODE_NO_KEYLOCK    0x08
+/* Disable keyboard interface */
+#define KBD_MODE_DISABLE_KBD   0x10
+/* Disable mouse interface */
+#define KBD_MODE_DISABLE_MOUSE 0x20
+/* Scan code conversion to PC format */
+#define KBD_MODE_KCC           0x40
+#define KBD_MODE_RFU           0x80
 
 /* Output Port Bits */
 #define KBD_OUT_RESET           0x01    /* 1=normal mode, 0=reset */
@@ -89,7 +126,8 @@
 #define KBD_OUT_OBF             0x10    /* Keyboard output buffer full */
 #define KBD_OUT_MOUSE_OBF       0x20    /* Mouse output buffer full */
 
-/* OSes typically write 0xdd/0xdf to turn the A20 line off and on.
+/*
+ * OSes typically write 0xdd/0xdf to turn the A20 line off and on.
  * We make the default value of the outport include these four bits,
  * so that the subsection is rarely necessary.
  */
@@ -133,8 +171,10 @@ typedef struct KBDState {
     hwaddr mask;
 } KBDState;
 
-/* XXX: not generating the irqs if KBD_MODE_DISABLE_KBD is set may be
-   incorrect, but it avoids having to simulate exact delays */
+/*
+ * XXX: not generating the irqs if KBD_MODE_DISABLE_KBD is set may be
+ * incorrect, but it avoids having to simulate exact delays
+ */
 static void kbd_update_irq_lines(KBDState *s)
 {
     int irq_kbd_level, irq_mouse_level;
@@ -302,21 +342,23 @@ static void kbd_write_command(void *opaque, hwaddr addr,
 
     trace_pckbd_kbd_write_command(val);
 
-    /* Bits 3-0 of the output port P2 of the keyboard controller may be pulsed
+    /*
+     * Bits 3-0 of the output port P2 of the keyboard controller may be pulsed
      * low for approximately 6 micro seconds. Bits 3-0 of the KBD_CCMD_PULSE
      * command specify the output port bits to be pulsed.
      * 0: Bit should be pulsed. 1: Bit should not be modified.
      * The only useful version of this command is pulsing bit 0,
      * which does a CPU reset.
      */
-    if((val & KBD_CCMD_PULSE_BITS_3_0) == KBD_CCMD_PULSE_BITS_3_0) {
-        if(!(val & 1))
+    if ((val & KBD_CCMD_PULSE_BITS_3_0) == KBD_CCMD_PULSE_BITS_3_0) {
+        if (!(val & 1)) {
             val = KBD_CCMD_RESET;
-        else
+        } else {
             val = KBD_CCMD_NO_OP;
+        }
     }
 
-    switch(val) {
+    switch (val) {
     case KBD_CCMD_READ_MODE:
         kbd_queue(s, s->mode, 0);
         break;
@@ -409,7 +451,7 @@ static void kbd_write_data(void *opaque, hwaddr addr,
 
     trace_pckbd_kbd_write_data(val);
 
-    switch(s->write_cmd) {
+    switch (s->write_cmd) {
     case 0:
         ps2_write_keyboard(s->kbd, val);
         /* sending data to the keyboard reenables PS/2 communication */
@@ -607,7 +649,7 @@ static const VMStateDescription vmstate_kbd = {
         VMSTATE_UINT8(pending_tmp, KBDState),
         VMSTATE_END_OF_LIST()
     },
-    .subsections = (const VMStateDescription*[]) {
+    .subsections = (const VMStateDescription * []) {
         &vmstate_kbd_outport,
         &vmstate_kbd_extended_state,
         NULL
@@ -619,10 +661,11 @@ static uint64_t kbd_mm_readfn(void *opaque, hwaddr addr, unsigned size)
 {
     KBDState *s = opaque;
 
-    if (addr & s->mask)
+    if (addr & s->mask) {
         return kbd_read_status(s, 0, 1) & 0xff;
-    else
+    } else {
         return kbd_read_data(s, 0, 1) & 0xff;
+    }
 }
 
 static void kbd_mm_writefn(void *opaque, hwaddr addr,
@@ -630,10 +673,11 @@ static void kbd_mm_writefn(void *opaque, hwaddr addr,
 {
     KBDState *s = opaque;
 
-    if (addr & s->mask)
+    if (addr & s->mask) {
         kbd_write_command(s, 0, value & 0xff, 1);
-    else
+    } else {
         kbd_write_data(s, 0, value & 0xff, 1);
+    }
 }
 
 
-- 
2.20.1

[PATCH 19/50] pckbd: move KBDState from pckbd.c to i8042.h

[Mark Cave-Ayland]

This allows the QOM types in pckbd.c to be used elsewhere by simply including
i8042.h.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c         | 24 ------------------------
 include/hw/input/i8042.h | 25 +++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index ab0d66d00d..87e015f243 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -146,30 +146,6 @@
 #define KBD_OBSRC_MOUSE         0x02
 #define KBD_OBSRC_CTRL          0x04
 
-typedef struct KBDState {
-    uint8_t write_cmd; /* if non zero, write data to port 60 is expected */
-    uint8_t status;
-    uint8_t mode;
-    uint8_t outport;
-    uint32_t migration_flags;
-    uint32_t obsrc;
-    bool outport_present;
-    bool extended_state;
-    bool extended_state_loaded;
-    /* Bitmask of devices with data available.  */
-    uint8_t pending;
-    uint8_t obdata;
-    uint8_t cbdata;
-    uint8_t pending_tmp;
-    void *kbd;
-    void *mouse;
-    QEMUTimer *throttle_timer;
-
-    qemu_irq irq_kbd;
-    qemu_irq irq_mouse;
-    qemu_irq a20_out;
-    hwaddr mask;
-} KBDState;
 
 /*
  * XXX: not generating the irqs if KBD_MODE_DISABLE_KBD is set may be
diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h
index e070f546e4..84b5aa7f23 100644
--- a/include/hw/input/i8042.h
+++ b/include/hw/input/i8042.h
@@ -11,6 +11,31 @@
 #include "hw/isa/isa.h"
 #include "qom/object.h"
 
+typedef struct KBDState {
+    uint8_t write_cmd; /* if non zero, write data to port 60 is expected */
+    uint8_t status;
+    uint8_t mode;
+    uint8_t outport;
+    uint32_t migration_flags;
+    uint32_t obsrc;
+    bool outport_present;
+    bool extended_state;
+    bool extended_state_loaded;
+    /* Bitmask of devices with data available.  */
+    uint8_t pending;
+    uint8_t obdata;
+    uint8_t cbdata;
+    uint8_t pending_tmp;
+    void *kbd;
+    void *mouse;
+    QEMUTimer *throttle_timer;
+
+    qemu_irq irq_kbd;
+    qemu_irq irq_mouse;
+    qemu_irq a20_out;
+    hwaddr mask;
+} KBDState;
+
 #define TYPE_I8042 "i8042"
 OBJECT_DECLARE_SIMPLE_TYPE(ISAKBDState, I8042)
 
-- 
2.20.1

[PATCH 20/50] pckbd: move ISAKBDState from pckbd.c to i8042.h

[Mark Cave-Ayland]

This allows the QOM types in pckbd.c to be used elsewhere by simply including
i8042.h.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c         | 10 ----------
 include/hw/input/i8042.h | 10 ++++++++++
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 87e015f243..3a7411f799 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -686,16 +686,6 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
     qemu_register_reset(kbd_reset, s);
 }
 
-struct ISAKBDState {
-    ISADevice parent_obj;
-
-    KBDState kbd;
-    bool kbd_throttle;
-    MemoryRegion io[2];
-    uint8_t kbd_irq;
-    uint8_t mouse_irq;
-};
-
 void i8042_isa_mouse_fake_event(ISAKBDState *isa)
 {
     KBDState *s = &isa->kbd;
diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h
index 84b5aa7f23..a246250d1f 100644
--- a/include/hw/input/i8042.h
+++ b/include/hw/input/i8042.h
@@ -39,6 +39,16 @@ typedef struct KBDState {
 #define TYPE_I8042 "i8042"
 OBJECT_DECLARE_SIMPLE_TYPE(ISAKBDState, I8042)
 
+struct ISAKBDState {
+    ISADevice parent_obj;
+
+    KBDState kbd;
+    bool kbd_throttle;
+    MemoryRegion io[2];
+    uint8_t kbd_irq;
+    uint8_t mouse_irq;
+};
+
 #define I8042_A20_LINE "a20"
 
 
-- 
2.20.1

[PATCH 21/50] pckbd: introduce new I8042_MMIO QOM type

[Mark Cave-Ayland]

Currently i8042_mm_init() creates a new KBDState directly which is used by the MIPS
magnum machine. Introduce a new I8042_MMIO QOM type that will soon be used to
allow the MIPS magnum machine to be wired up using standard qdev GPIOs.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c         | 22 +++++++++++++++++++++-
 include/hw/input/i8042.h | 10 ++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 3a7411f799..bbdd3bc568 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -665,11 +665,23 @@ static const MemoryRegionOps i8042_mmio_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static void i8042_mmio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+}
+
 void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
                    MemoryRegion *region, ram_addr_t size,
                    hwaddr mask)
 {
-    KBDState *s = g_new0(KBDState, 1);
+    DeviceState *dev;
+    KBDState *s;
+
+    dev = qdev_new(TYPE_I8042_MMIO);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    s = &I8042_MMIO(dev)->kbd;
 
     s->irq_kbd = kbd_irq;
     s->irq_mouse = mouse_irq;
@@ -686,6 +698,13 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
     qemu_register_reset(kbd_reset, s);
 }
 
+static const TypeInfo i8042_mmio_info = {
+    .name          = TYPE_I8042_MMIO,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(MMIOKBDState),
+    .class_init    = i8042_mmio_class_init
+};
+
 void i8042_isa_mouse_fake_event(ISAKBDState *isa)
 {
     KBDState *s = &isa->kbd;
@@ -837,6 +856,7 @@ static const TypeInfo i8042_info = {
 static void i8042_register_types(void)
 {
     type_register_static(&i8042_info);
+    type_register_static(&i8042_mmio_info);
 }
 
 type_init(i8042_register_types)
diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h
index a246250d1f..b7df9ace6e 100644
--- a/include/hw/input/i8042.h
+++ b/include/hw/input/i8042.h
@@ -9,6 +9,7 @@
 #define HW_INPUT_I8042_H
 
 #include "hw/isa/isa.h"
+#include "hw/sysbus.h"
 #include "qom/object.h"
 
 typedef struct KBDState {
@@ -49,6 +50,15 @@ struct ISAKBDState {
     uint8_t mouse_irq;
 };
 
+#define TYPE_I8042_MMIO "i8042-mmio"
+OBJECT_DECLARE_SIMPLE_TYPE(MMIOKBDState, I8042_MMIO)
+
+struct MMIOKBDState {
+    SysBusDevice parent_obj;
+
+    KBDState kbd;
+};
+
 #define I8042_A20_LINE "a20"
 
 
-- 
2.20.1

Re: [PATCH 21/50] pckbd: introduce new I8042_MMIO QOM type

[Peter Maydell]

On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Currently i8042_mm_init() creates a new KBDState directly which is used by the MIPS
> magnum machine. Introduce a new I8042_MMIO QOM type that will soon be used to
> allow the MIPS magnum machine to be wired up using standard qdev GPIOs.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 22/50] pckbd: implement i8042_mmio_reset() for I8042_MMIO device

[Mark Cave-Ayland]

This allows the I8042_MMIO reset function to be registered directly within the
DeviceClass rather than using qemu_register_reset() directly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index bbdd3bc568..df443aaff2 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -665,10 +665,20 @@ static const MemoryRegionOps i8042_mmio_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static void i8042_mmio_reset(DeviceState *dev)
+{
+    MMIOKBDState *s = I8042_MMIO(dev);
+    KBDState *ks = &s->kbd;
+
+    ks->extended_state = true;
+    kbd_reset(ks);
+}
+
 static void i8042_mmio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->reset = i8042_mmio_reset;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
 
@@ -687,15 +697,12 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
     s->irq_mouse = mouse_irq;
     s->mask = mask;
 
-    s->extended_state = true;
-
     vmstate_register(NULL, 0, &vmstate_kbd, s);
 
     memory_region_init_io(region, NULL, &i8042_mmio_ops, s, "i8042", size);
 
     s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
     s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);
-    qemu_register_reset(kbd_reset, s);
 }
 
 static const TypeInfo i8042_mmio_info = {
-- 
2.20.1

Re: [PATCH 22/50] pckbd: implement i8042_mmio_reset() for I8042_MMIO device

[Peter Maydell]

On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This allows the I8042_MMIO reset function to be registered directly within the
> DeviceClass rather than using qemu_register_reset() directly.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/pckbd.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index bbdd3bc568..df443aaff2 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -665,10 +665,20 @@ static const MemoryRegionOps i8042_mmio_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> +static void i8042_mmio_reset(DeviceState *dev)
> +{
> +    MMIOKBDState *s = I8042_MMIO(dev);
> +    KBDState *ks = &s->kbd;
> +
> +    ks->extended_state = true;
> +    kbd_reset(ks);
> +}

extended_state is not runtime guest-changeable state, it's a
device property that's set at device creation time. So we
shouldn't be setting it to 'true' here, but instead in the
device init or realize function.

thanks
-- PMM

Re: [PATCH 22/50] pckbd: implement i8042_mmio_reset() for I8042_MMIO device

[Mark Cave-Ayland]

On 09/06/2022 11:49, Peter Maydell wrote:

> On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> This allows the I8042_MMIO reset function to be registered directly within the
>> DeviceClass rather than using qemu_register_reset() directly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/input/pckbd.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
>> index bbdd3bc568..df443aaff2 100644
>> --- a/hw/input/pckbd.c
>> +++ b/hw/input/pckbd.c
>> @@ -665,10 +665,20 @@ static const MemoryRegionOps i8042_mmio_ops = {
>>       .endianness = DEVICE_NATIVE_ENDIAN,
>>   };
>>
>> +static void i8042_mmio_reset(DeviceState *dev)
>> +{
>> +    MMIOKBDState *s = I8042_MMIO(dev);
>> +    KBDState *ks = &s->kbd;
>> +
>> +    ks->extended_state = true;
>> +    kbd_reset(ks);
>> +}
> 
> extended_state is not runtime guest-changeable state, it's a
> device property that's set at device creation time. So we
> shouldn't be setting it to 'true' here, but instead in the
> device init or realize function.

Ah oops. I'll move it to a corresponding device init function in v2.


ATB,

Mark.

[PATCH 23/50] pckbd: add mask qdev property to I8042_MMIO device

[Mark Cave-Ayland]

This allows the KBDState mask value to be set using a qdev property rather
than directly in i8042_mm_init().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index df443aaff2..9da602fb47 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -674,11 +674,17 @@ static void i8042_mmio_reset(DeviceState *dev)
     kbd_reset(ks);
 }
 
+static Property i8042_mmio_properties[] = {
+    DEFINE_PROP_UINT64("mask", MMIOKBDState, kbd.mask, UINT64_MAX),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void i8042_mmio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->reset = i8042_mmio_reset;
+    device_class_set_props(dc, i8042_mmio_properties);
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
 
@@ -690,12 +696,12 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
     KBDState *s;
 
     dev = qdev_new(TYPE_I8042_MMIO);
+    qdev_prop_set_uint64(dev, "mask", mask);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     s = &I8042_MMIO(dev)->kbd;
 
     s->irq_kbd = kbd_irq;
     s->irq_mouse = mouse_irq;
-    s->mask = mask;
 
     vmstate_register(NULL, 0, &vmstate_kbd, s);
 
-- 
2.20.1

Re: [PATCH 23/50] pckbd: add mask qdev property to I8042_MMIO device

[Peter Maydell]

On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This allows the KBDState mask value to be set using a qdev property rather
> than directly in i8042_mm_init().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 24/50] pckbd: add size qdev property to I8042_MMIO device

[Mark Cave-Ayland]

This will soon be used to set the size of the register memory region using a
qdev property.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c         | 2 ++
 include/hw/input/i8042.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 9da602fb47..e13a62bd4f 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -676,6 +676,7 @@ static void i8042_mmio_reset(DeviceState *dev)
 
 static Property i8042_mmio_properties[] = {
     DEFINE_PROP_UINT64("mask", MMIOKBDState, kbd.mask, UINT64_MAX),
+    DEFINE_PROP_UINT32("size", MMIOKBDState, size, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -697,6 +698,7 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
 
     dev = qdev_new(TYPE_I8042_MMIO);
     qdev_prop_set_uint64(dev, "mask", mask);
+    qdev_prop_set_uint32(dev, "size", size);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     s = &I8042_MMIO(dev)->kbd;
 
diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h
index b7df9ace6e..ac4098b957 100644
--- a/include/hw/input/i8042.h
+++ b/include/hw/input/i8042.h
@@ -57,6 +57,7 @@ struct MMIOKBDState {
     SysBusDevice parent_obj;
 
     KBDState kbd;
+    uint32_t size;
 };
 
 #define I8042_A20_LINE "a20"
-- 
2.20.1

Re: [PATCH 24/50] pckbd: add size qdev property to I8042_MMIO device

[Peter Maydell]

On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This will soon be used to set the size of the register memory region using a
> qdev property.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/pckbd.c         | 2 ++
>  include/hw/input/i8042.h | 1 +

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 25/50] pckbd: implement i8042_mmio_realize() function

[Mark Cave-Ayland]

Move the initialisation of the register memory region to the I8042_MMIO device
realize function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c         | 12 +++++++++++-
 include/hw/input/i8042.h |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index e13a62bd4f..addd1f058a 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -674,6 +674,15 @@ static void i8042_mmio_reset(DeviceState *dev)
     kbd_reset(ks);
 }
 
+static void i8042_mmio_realize(DeviceState *dev, Error **errp)
+{
+    MMIOKBDState *s = I8042_MMIO(dev);
+    KBDState *ks = &s->kbd;
+
+    memory_region_init_io(&s->region, OBJECT(dev), &i8042_mmio_ops, ks,
+                          "i8042", s->size);
+}
+
 static Property i8042_mmio_properties[] = {
     DEFINE_PROP_UINT64("mask", MMIOKBDState, kbd.mask, UINT64_MAX),
     DEFINE_PROP_UINT32("size", MMIOKBDState, size, -1),
@@ -684,6 +693,7 @@ static void i8042_mmio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->realize = i8042_mmio_realize;
     dc->reset = i8042_mmio_reset;
     device_class_set_props(dc, i8042_mmio_properties);
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
@@ -707,7 +717,7 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
 
     vmstate_register(NULL, 0, &vmstate_kbd, s);
 
-    memory_region_init_io(region, NULL, &i8042_mmio_ops, s, "i8042", size);
+    region = &I8042_MMIO(dev)->region;
 
     s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
     s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);
diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h
index ac4098b957..59d695a9dd 100644
--- a/include/hw/input/i8042.h
+++ b/include/hw/input/i8042.h
@@ -58,6 +58,7 @@ struct MMIOKBDState {
 
     KBDState kbd;
     uint32_t size;
+    MemoryRegion region;
 };
 
 #define I8042_A20_LINE "a20"
-- 
2.20.1

Re: [PATCH 25/50] pckbd: implement i8042_mmio_realize() function

[Peter Maydell]

On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Move the initialisation of the register memory region to the I8042_MMIO device
> realize function.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/pckbd.c         | 12 +++++++++++-
>  include/hw/input/i8042.h |  1 +
>  2 files changed, 12 insertions(+), 1 deletion(-)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 26/50] pckbd: implement i8042_mmio_init() function

[Mark Cave-Ayland]

This enables use to expose the register memory region of the I8042_MMIO device
using sysbus_init_mmio().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index addd1f058a..c407c082e2 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -683,6 +683,13 @@ static void i8042_mmio_realize(DeviceState *dev, Error **errp)
                           "i8042", s->size);
 }
 
+static void i8042_mmio_init(Object *obj)
+{
+    MMIOKBDState *s = I8042_MMIO(obj);
+
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->region);
+}
+
 static Property i8042_mmio_properties[] = {
     DEFINE_PROP_UINT64("mask", MMIOKBDState, kbd.mask, UINT64_MAX),
     DEFINE_PROP_UINT32("size", MMIOKBDState, size, -1),
@@ -726,6 +733,7 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
 static const TypeInfo i8042_mmio_info = {
     .name          = TYPE_I8042_MMIO,
     .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_init = i8042_mmio_init,
     .instance_size = sizeof(MMIOKBDState),
     .class_init    = i8042_mmio_class_init
 };
-- 
2.20.1

Re: [PATCH 26/50] pckbd: implement i8042_mmio_init() function

[Peter Maydell]

On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This enables use to expose the register memory region of the I8042_MMIO device
> using sysbus_init_mmio().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/pckbd.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index addd1f058a..c407c082e2 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -683,6 +683,13 @@ static void i8042_mmio_realize(DeviceState *dev, Error **errp)
>                            "i8042", s->size);
>  }
>
> +static void i8042_mmio_init(Object *obj)
> +{
> +    MMIOKBDState *s = I8042_MMIO(obj);
> +
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->region);
> +}

You don't initialize s->region until your device realize function;
you should only call sysbus_init_mmio() after that, so this line
belongs in the realize function too.

thanks
-- PMM

Re: [PATCH 26/50] pckbd: implement i8042_mmio_init() function

[Mark Cave-Ayland]

On 09/06/2022 11:53, Peter Maydell wrote:

> On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> This enables use to expose the register memory region of the I8042_MMIO device
>> using sysbus_init_mmio().
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/input/pckbd.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
>> index addd1f058a..c407c082e2 100644
>> --- a/hw/input/pckbd.c
>> +++ b/hw/input/pckbd.c
>> @@ -683,6 +683,13 @@ static void i8042_mmio_realize(DeviceState *dev, Error **errp)
>>                             "i8042", s->size);
>>   }
>>
>> +static void i8042_mmio_init(Object *obj)
>> +{
>> +    MMIOKBDState *s = I8042_MMIO(obj);
>> +
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->region);
>> +}
> 
> You don't initialize s->region until your device realize function;
> you should only call sysbus_init_mmio() after that, so this line
> belongs in the realize function too.

Yes indeed, I can fix this fairly easily.


ATB,

Mark.

[PATCH 27/50] pckbd: alter i8042_mm_init() to return a I8042_MMIO device

[Mark Cave-Ayland]

This exposes the I8042_MMIO device to the caller to allow the register memory
region to be mapped outside of i8042_mm_init().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c         | 8 +++++---
 include/hw/input/i8042.h | 6 +++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index c407c082e2..a7f1439994 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -706,9 +706,9 @@ static void i8042_mmio_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
 
-void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
-                   MemoryRegion *region, ram_addr_t size,
-                   hwaddr mask)
+MMIOKBDState *i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
+                            MemoryRegion *region, ram_addr_t size,
+                            hwaddr mask)
 {
     DeviceState *dev;
     KBDState *s;
@@ -728,6 +728,8 @@ void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
 
     s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
     s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);
+
+    return I8042_MMIO(dev);
 }
 
 static const TypeInfo i8042_mmio_info = {
diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h
index 59d695a9dd..d05cd33e3c 100644
--- a/include/hw/input/i8042.h
+++ b/include/hw/input/i8042.h
@@ -64,9 +64,9 @@ struct MMIOKBDState {
 #define I8042_A20_LINE "a20"
 
 
-void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
-                   MemoryRegion *region, ram_addr_t size,
-                   hwaddr mask);
+MMIOKBDState *i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
+                            MemoryRegion *region, ram_addr_t size,
+                            hwaddr mask);
 void i8042_isa_mouse_fake_event(ISAKBDState *isa);
 void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out);
 
-- 
2.20.1

Re: [PATCH 27/50] pckbd: alter i8042_mm_init() to return a I8042_MMIO device

[Peter Maydell]

On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This exposes the I8042_MMIO device to the caller to allow the register memory
> region to be mapped outside of i8042_mm_init().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I'm not sure where best to put this review comment in the patchseries,
so I'll just note it here: I see that at the end of the series we end
up with an i8042_mm_init() which is just "create a device with qdev_new,
set properties, realize it, and connect its gpio lines". Since that
function has exactly one callsite (in hw/mips/jazz.c) I think we should
add a patch on the end that gets rid of i8042_mm_init() entirely in
favour of the board doing all the "create and wire up device" code inline.

thanks
-- PMM

Re: [PATCH 27/50] pckbd: alter i8042_mm_init() to return a I8042_MMIO device

[Mark Cave-Ayland]

On 09/06/2022 11:58, Peter Maydell wrote:

> On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> This exposes the I8042_MMIO device to the caller to allow the register memory
>> region to be mapped outside of i8042_mm_init().
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I'm not sure where best to put this review comment in the patchseries,
> so I'll just note it here: I see that at the end of the series we end
> up with an i8042_mm_init() which is just "create a device with qdev_new,
> set properties, realize it, and connect its gpio lines". Since that
> function has exactly one callsite (in hw/mips/jazz.c) I think we should
> add a patch on the end that gets rid of i8042_mm_init() entirely in
> favour of the board doing all the "create and wire up device" code inline.

I actually have a patch to do this in the follow-up part 2 series locally. The aim 
was to get part 1 merged first (which is removing the update callback and the 
opaque), and then finish tidying up the QOM modelling of the devices in part 2.

However since part 1 has been posted, both Hervé and Helge have mentioned that they 
wouldn't mind if there was a migration break for the hppa and MIPS magnum machines. 
As this does save a bit of work for the I8042_MMIO device, I'll see if it is easy 
enough to bring forward into part 1.


ATB,

Mark.

Re: [PATCH 27/50] pckbd: alter i8042_mm_init() to return a I8042_MMIO device

[Mark Cave-Ayland]

On 10/06/2022 08:04, Mark Cave-Ayland wrote:

> On 09/06/2022 11:58, Peter Maydell wrote:
> 
>> On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>
>>> This exposes the I8042_MMIO device to the caller to allow the register memory
>>> region to be mapped outside of i8042_mm_init().
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> I'm not sure where best to put this review comment in the patchseries,
>> so I'll just note it here: I see that at the end of the series we end
>> up with an i8042_mm_init() which is just "create a device with qdev_new,
>> set properties, realize it, and connect its gpio lines". Since that
>> function has exactly one callsite (in hw/mips/jazz.c) I think we should
>> add a patch on the end that gets rid of i8042_mm_init() entirely in
>> favour of the board doing all the "create and wire up device" code inline.
> 
> I actually have a patch to do this in the follow-up part 2 series locally. The aim 
> was to get part 1 merged first (which is removing the update callback and the 
> opaque), and then finish tidying up the QOM modelling of the devices in part 2.
> 
> However since part 1 has been posted, both Hervé and Helge have mentioned that they 
> wouldn't mind if there was a migration break for the hppa and MIPS magnum machines. 
> As this does save a bit of work for the I8042_MMIO device, I'll see if it is easy 
> enough to bring forward into part 1.

After a bit of experimentation I think deferring this to part 2 is the easiest 
solution here: even though there is a patch to remove the "base" property again for 
the I8042_MMIO_DEVICE, that solution still feels cleaner than scattering the 
migration compatibility across multiple series.


ATB,

Mark.

[PATCH 28/50] pckbd: move mapping of I8042_MMIO registers to MIPS magnum machine

[Mark Cave-Ayland]

Now that the register memory region is exposed as a SysBus memory region, move
the mapping of the I8042_MMIO registers from i8042_mm_init() to the MIPS magnum
machine (which is its only user).

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c         |  5 +----
 hw/mips/jazz.c           | 11 +++++++----
 include/hw/input/i8042.h |  3 +--
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index a7f1439994..eb77ad193e 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -707,8 +707,7 @@ static void i8042_mmio_class_init(ObjectClass *klass, void *data)
 }
 
 MMIOKBDState *i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
-                            MemoryRegion *region, ram_addr_t size,
-                            hwaddr mask)
+                            ram_addr_t size, hwaddr mask)
 {
     DeviceState *dev;
     KBDState *s;
@@ -724,8 +723,6 @@ MMIOKBDState *i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
 
     vmstate_register(NULL, 0, &vmstate_kbd, s);
 
-    region = &I8042_MMIO(dev)->region;
-
     s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
     s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);
 
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 6598d7dddd..80c9206230 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -136,11 +136,11 @@ static void mips_jazz_init(MachineState *machine,
     MemoryRegion *isa_mem = g_new(MemoryRegion, 1);
     MemoryRegion *isa_io = g_new(MemoryRegion, 1);
     MemoryRegion *rtc = g_new(MemoryRegion, 1);
-    MemoryRegion *i8042 = g_new(MemoryRegion, 1);
     MemoryRegion *dma_dummy = g_new(MemoryRegion, 1);
     MemoryRegion *dp8393x_prom = g_new(MemoryRegion, 1);
     NICInfo *nd;
     DeviceState *dev, *rc4030;
+    MMIOKBDState *i8042;
     SysBusDevice *sysbus;
     ISABus *isa_bus;
     ISADevice *pit;
@@ -361,9 +361,12 @@ static void mips_jazz_init(MachineState *machine,
     memory_region_add_subregion(address_space, 0x80004000, rtc);
 
     /* Keyboard (i8042) */
-    i8042_mm_init(qdev_get_gpio_in(rc4030, 6), qdev_get_gpio_in(rc4030, 7),
-                  i8042, 0x1000, 0x1);
-    memory_region_add_subregion(address_space, 0x80005000, i8042);
+    i8042 = i8042_mm_init(qdev_get_gpio_in(rc4030, 6),
+                          qdev_get_gpio_in(rc4030, 7),
+                          0x1000, 0x1);
+    memory_region_add_subregion(address_space, 0x80005000,
+                                sysbus_mmio_get_region(SYS_BUS_DEVICE(i8042),
+                                                       0));
 
     /* Serial ports */
     serial_mm_init(address_space, 0x80006000, 0,
diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h
index d05cd33e3c..9d1f8af964 100644
--- a/include/hw/input/i8042.h
+++ b/include/hw/input/i8042.h
@@ -65,8 +65,7 @@ struct MMIOKBDState {
 
 
 MMIOKBDState *i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
-                            MemoryRegion *region, ram_addr_t size,
-                            hwaddr mask);
+                            ram_addr_t size, hwaddr mask);
 void i8042_isa_mouse_fake_event(ISAKBDState *isa);
 void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out);
 
-- 
2.20.1

Re: [PATCH 28/50] pckbd: move mapping of I8042_MMIO registers to MIPS magnum machine

[Peter Maydell]

On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Now that the register memory region is exposed as a SysBus memory region, move
> the mapping of the I8042_MMIO registers from i8042_mm_init() to the MIPS magnum
> machine (which is its only user).
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 29/50] pckbd: more vmstate_register() from i8042_mm_init() to i8042_mmio_realize()

[Mark Cave-Ayland]

Note in this case it is not possible to register a (new) VMStateDescription in
the DeviceClass without breaking migration compatibility for the MIPS magnum
machine.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index eb77ad193e..7f3578aa4d 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -681,6 +681,9 @@ static void i8042_mmio_realize(DeviceState *dev, Error **errp)
 
     memory_region_init_io(&s->region, OBJECT(dev), &i8042_mmio_ops, ks,
                           "i8042", s->size);
+
+    /* Note we can't use dc->vmsd without breaking migration compatibility */
+    vmstate_register(NULL, 0, &vmstate_kbd, ks);
 }
 
 static void i8042_mmio_init(Object *obj)
@@ -721,8 +724,6 @@ MMIOKBDState *i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
     s->irq_kbd = kbd_irq;
     s->irq_mouse = mouse_irq;
 
-    vmstate_register(NULL, 0, &vmstate_kbd, s);
-
     s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
     s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);
 
-- 
2.20.1

Re: [PATCH 29/50] pckbd: more vmstate_register() from i8042_mm_init() to i8042_mmio_realize()

[Hervé Poussineau]

Hello,

If you want, you can break migration compatibility. I don't see it as a problem.

Hervé

Le 22/05/2022 à 20:18, Mark Cave-Ayland a écrit :
> Note in this case it is not possible to register a (new) VMStateDescription in
> the DeviceClass without breaking migration compatibility for the MIPS magnum
> machine.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/input/pckbd.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index eb77ad193e..7f3578aa4d 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -681,6 +681,9 @@ static void i8042_mmio_realize(DeviceState *dev, Error **errp)
>   
>       memory_region_init_io(&s->region, OBJECT(dev), &i8042_mmio_ops, ks,
>                             "i8042", s->size);
> +
> +    /* Note we can't use dc->vmsd without breaking migration compatibility */
> +    vmstate_register(NULL, 0, &vmstate_kbd, ks);
>   }
>   
>   static void i8042_mmio_init(Object *obj)
> @@ -721,8 +724,6 @@ MMIOKBDState *i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
>       s->irq_kbd = kbd_irq;
>       s->irq_mouse = mouse_irq;
>   
> -    vmstate_register(NULL, 0, &vmstate_kbd, s);
> -
>       s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
>       s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);
>   

Re: [PATCH 29/50] pckbd: more vmstate_register() from i8042_mm_init() to i8042_mmio_realize()

[Mark Cave-Ayland]

On 23/05/2022 07:34, Hervé Poussineau wrote:

> Hello,
> 
> If you want, you can break migration compatibility. I don't see it as a problem.
> 
> Hervé

Thanks for suggesting this. Given that migration of the magnum machine was broken for 
a long time until I fixed it recently, I don't think it would be a problem either.

I was considering a similar proposal for lasips2 but was planning to do the migration 
break in the part 2 series as part of the QOM modelling improvements rather than in 
this initial series. I'll update the I8042_MMIO accordingly when my current working 
branch for part 2.

> Le 22/05/2022 à 20:18, Mark Cave-Ayland a écrit :
>> Note in this case it is not possible to register a (new) VMStateDescription in
>> the DeviceClass without breaking migration compatibility for the MIPS magnum
>> machine.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/input/pckbd.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
>> index eb77ad193e..7f3578aa4d 100644
>> --- a/hw/input/pckbd.c
>> +++ b/hw/input/pckbd.c
>> @@ -681,6 +681,9 @@ static void i8042_mmio_realize(DeviceState *dev, Error **errp)
>>       memory_region_init_io(&s->region, OBJECT(dev), &i8042_mmio_ops, ks,
>>                             "i8042", s->size);
>> +
>> +    /* Note we can't use dc->vmsd without breaking migration compatibility */
>> +    vmstate_register(NULL, 0, &vmstate_kbd, ks);
>>   }
>>   static void i8042_mmio_init(Object *obj)
>> @@ -721,8 +724,6 @@ MMIOKBDState *i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
>>       s->irq_kbd = kbd_irq;
>>       s->irq_mouse = mouse_irq;
>> -    vmstate_register(NULL, 0, &vmstate_kbd, s);
>> -
>>       s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
>>       s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);


ATB,

Mark.

[PATCH 30/50] pckbd: move ps2_kbd_init() and ps2_mouse_init() to i8042_mmio_realize()

[Mark Cave-Ayland]

Move ps2_kbd_init() and ps2_mouse_init() from i8042_mm_init() to
i8042_mmio_realize() to further reduce the initialisation logic done in
i8042_mm_init().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 7f3578aa4d..1c61c55123 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -684,6 +684,9 @@ static void i8042_mmio_realize(DeviceState *dev, Error **errp)
 
     /* Note we can't use dc->vmsd without breaking migration compatibility */
     vmstate_register(NULL, 0, &vmstate_kbd, ks);
+
+    ks->kbd = ps2_kbd_init(kbd_update_kbd_irq, ks);
+    ks->mouse = ps2_mouse_init(kbd_update_aux_irq, ks);
 }
 
 static void i8042_mmio_init(Object *obj)
@@ -724,9 +727,6 @@ MMIOKBDState *i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
     s->irq_kbd = kbd_irq;
     s->irq_mouse = mouse_irq;
 
-    s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
-    s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);
-
     return I8042_MMIO(dev);
 }
 
-- 
2.20.1

Re: [PATCH 30/50] pckbd: move ps2_kbd_init() and ps2_mouse_init() to i8042_mmio_realize()

[Peter Maydell]

On Sun, 22 May 2022 at 19:19, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Move ps2_kbd_init() and ps2_mouse_init() from i8042_mm_init() to
> i8042_mmio_realize() to further reduce the initialisation logic done in
> i8042_mm_init().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 31/50] ps2: make ps2_raise_irq() function static

[Mark Cave-Ayland]

This function is no longer used outside of ps2.c and so can be declared static.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c         | 2 +-
 include/hw/input/ps2.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index e3ee69870b..8b76ce1cdc 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -172,7 +172,7 @@ void ps2_queue_noirq(PS2State *s, int b)
     q->count++;
 }
 
-void ps2_raise_irq(PS2State *s)
+static void ps2_raise_irq(PS2State *s)
 {
     s->update_irq(s->update_arg, 1);
 }
diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h
index 4e8de912c6..d04d291287 100644
--- a/include/hw/input/ps2.h
+++ b/include/hw/input/ps2.h
@@ -102,7 +102,6 @@ void ps2_write_mouse(PS2MouseState *s, int val);
 void ps2_write_keyboard(PS2KbdState *s, int val);
 uint32_t ps2_read_data(PS2State *s);
 void ps2_queue_noirq(PS2State *s, int b);
-void ps2_raise_irq(PS2State *s);
 void ps2_queue(PS2State *s, int b);
 void ps2_queue_2(PS2State *s, int b1, int b2);
 void ps2_queue_3(PS2State *s, int b1, int b2, int b3);
-- 
2.20.1

Re: [PATCH 31/50] ps2: make ps2_raise_irq() function static

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This function is no longer used outside of ps2.c and so can be declared static.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 32/50] ps2: use ps2_raise_irq() instead of calling update_irq() directly

[Mark Cave-Ayland]

This consolidates the logic of raising the PS2 IRQ into one single function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 8b76ce1cdc..55a2ac08c2 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -557,7 +557,7 @@ uint32_t ps2_read_data(PS2State *s)
         s->update_irq(s->update_arg, 0);
         /* reassert IRQs if data left */
         if (q->count) {
-            s->update_irq(s->update_arg, 1);
+            ps2_raise_irq(s);
         }
     }
     return val;
-- 
2.20.1

Re: [PATCH 32/50] ps2: use ps2_raise_irq() instead of calling update_irq() directly

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This consolidates the logic of raising the PS2 IRQ into one single function.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/ps2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 33/50] ps2: introduce ps2_lower_irq() instead of calling update_irq() directly

[Mark Cave-Ayland]

This consolidates the logic of lowering the PS2 IRQ into one single function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 55a2ac08c2..214dda60bf 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -177,6 +177,11 @@ static void ps2_raise_irq(PS2State *s)
     s->update_irq(s->update_arg, 1);
 }
 
+static void ps2_lower_irq(PS2State *s)
+{
+    s->update_irq(s->update_arg, 0);
+}
+
 void ps2_queue(PS2State *s, int b)
 {
     if (PS2_QUEUE_SIZE - s->queue.count < 1) {
@@ -554,7 +559,7 @@ uint32_t ps2_read_data(PS2State *s)
             q->cwptr = -1;
         }
         /* reading deasserts IRQ */
-        s->update_irq(s->update_arg, 0);
+        ps2_lower_irq(s);
         /* reassert IRQs if data left */
         if (q->count) {
             ps2_raise_irq(s);
@@ -1001,7 +1006,7 @@ static void ps2_reset(DeviceState *dev)
 
     s->write_cmd = -1;
     ps2_reset_queue(s);
-    s->update_irq(s->update_arg, 0);
+    ps2_lower_irq(s);
 }
 
 static void ps2_common_post_load(PS2State *s)
-- 
2.20.1

Re: [PATCH 33/50] ps2: introduce ps2_lower_irq() instead of calling update_irq() directly

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This consolidates the logic of lowering the PS2 IRQ into one single function.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 34/50] ps2: add gpio for output IRQ and optionally use it in ps2_raise_irq() and ps2_lower_irq()

[Mark Cave-Ayland]

Define the gpio for the PS2 output IRQ in ps2_init() and add logic to optionally
use it in ps2_raise_irq() and ps2_lower_irq() if the gpio is connected. If the
gpio is not connected then call the legacy update_irq() function as before.

This allows the incremental conversion of devices from the legacy update_irq()
function to use gpios instead.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/ps2.c         | 21 +++++++++++++++++++--
 include/hw/input/ps2.h |  4 ++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 214dda60bf..891eb7181c 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
+#include "hw/irq.h"
 #include "hw/sysbus.h"
 #include "hw/input/ps2.h"
 #include "migration/vmstate.h"
@@ -174,12 +175,20 @@ void ps2_queue_noirq(PS2State *s, int b)
 
 static void ps2_raise_irq(PS2State *s)
 {
-    s->update_irq(s->update_arg, 1);
+    if (s->irq) {
+        qemu_set_irq(s->irq, 1);
+    } else {
+        s->update_irq(s->update_arg, 1);
+    }
 }
 
 static void ps2_lower_irq(PS2State *s)
 {
-    s->update_irq(s->update_arg, 0);
+    if (s->irq) {
+        qemu_set_irq(s->irq, 0);
+    } else {
+        s->update_irq(s->update_arg, 0);
+    }
 }
 
 void ps2_queue(PS2State *s, int b)
@@ -1305,6 +1314,13 @@ static const TypeInfo ps2_mouse_info = {
     .class_init    = ps2_mouse_class_init
 };
 
+static void ps2_init(Object *obj)
+{
+    PS2State *s = PS2_DEVICE(obj);
+
+    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
+}
+
 static void ps2_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1316,6 +1332,7 @@ static void ps2_class_init(ObjectClass *klass, void *data)
 static const TypeInfo ps2_info = {
     .name          = TYPE_PS2_DEVICE,
     .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_init = ps2_init,
     .instance_size = sizeof(PS2State),
     .class_init    = ps2_class_init,
     .abstract      = true
diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h
index d04d291287..6e5c404cb5 100644
--- a/include/hw/input/ps2.h
+++ b/include/hw/input/ps2.h
@@ -50,11 +50,15 @@ typedef struct {
     int rptr, wptr, cwptr, count;
 } PS2Queue;
 
+/* Output IRQ */
+#define PS2_DEVICE_IRQ      0
+
 struct PS2State {
     SysBusDevice parent_obj;
 
     PS2Queue queue;
     int32_t write_cmd;
+    qemu_irq irq;
     void (*update_irq)(void *, int);
     void *update_arg;
 };
-- 
2.20.1

Re: [PATCH 34/50] ps2: add gpio for output IRQ and optionally use it in ps2_raise_irq() and ps2_lower_irq()

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Define the gpio for the PS2 output IRQ in ps2_init() and add logic to optionally
> use it in ps2_raise_irq() and ps2_lower_irq() if the gpio is connected. If the
> gpio is not connected then call the legacy update_irq() function as before.
>
> This allows the incremental conversion of devices from the legacy update_irq()
> function to use gpios instead.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/ps2.c         | 21 +++++++++++++++++++--
>  include/hw/input/ps2.h |  4 ++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index 214dda60bf..891eb7181c 100644
> --- a/hw/input/ps2.c
> +++ b/hw/input/ps2.c
> @@ -24,6 +24,7 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
> +#include "hw/irq.h"
>  #include "hw/sysbus.h"
>  #include "hw/input/ps2.h"
>  #include "migration/vmstate.h"
> @@ -174,12 +175,20 @@ void ps2_queue_noirq(PS2State *s, int b)
>
>  static void ps2_raise_irq(PS2State *s)
>  {
> -    s->update_irq(s->update_arg, 1);
> +    if (s->irq) {

I know this code is going to go away in patch 50, but cleaner to write
 if (qemu_irq_is_connected(s->irq))
rather than directly testing for NULL.

> +        qemu_set_irq(s->irq, 1);
> +    } else {
> +        s->update_irq(s->update_arg, 1);
> +    }
>  }

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 34/50] ps2: add gpio for output IRQ and optionally use it in ps2_raise_irq() and ps2_lower_irq()

[Mark Cave-Ayland]

On 09/06/2022 12:05, Peter Maydell wrote:

> On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> Define the gpio for the PS2 output IRQ in ps2_init() and add logic to optionally
>> use it in ps2_raise_irq() and ps2_lower_irq() if the gpio is connected. If the
>> gpio is not connected then call the legacy update_irq() function as before.
>>
>> This allows the incremental conversion of devices from the legacy update_irq()
>> function to use gpios instead.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/input/ps2.c         | 21 +++++++++++++++++++--
>>   include/hw/input/ps2.h |  4 ++++
>>   2 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
>> index 214dda60bf..891eb7181c 100644
>> --- a/hw/input/ps2.c
>> +++ b/hw/input/ps2.c
>> @@ -24,6 +24,7 @@
>>
>>   #include "qemu/osdep.h"
>>   #include "qemu/log.h"
>> +#include "hw/irq.h"
>>   #include "hw/sysbus.h"
>>   #include "hw/input/ps2.h"
>>   #include "migration/vmstate.h"
>> @@ -174,12 +175,20 @@ void ps2_queue_noirq(PS2State *s, int b)
>>
>>   static void ps2_raise_irq(PS2State *s)
>>   {
>> -    s->update_irq(s->update_arg, 1);
>> +    if (s->irq) {
> 
> I know this code is going to go away in patch 50, but cleaner to write
>   if (qemu_irq_is_connected(s->irq))
> rather than directly testing for NULL.

Thanks, I wasn't aware of that function. I'll update it for v2.

>> +        qemu_set_irq(s->irq, 1);
>> +    } else {
>> +        s->update_irq(s->update_arg, 1);
>> +    }
>>   }
> 
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM


ATB,

Mark.

[PATCH 35/50] pckbd: replace irq_kbd and irq_mouse with qemu_irq array in KBDState

[Mark Cave-Ayland]

This allows both IRQs to be declared as a single qdev gpio array.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c         | 12 ++++++------
 include/hw/input/i8042.h |  6 ++++--
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 1c61c55123..90acf0dd6a 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -170,8 +170,8 @@ static void kbd_update_irq_lines(KBDState *s)
             }
         }
     }
-    qemu_set_irq(s->irq_kbd, irq_kbd_level);
-    qemu_set_irq(s->irq_mouse, irq_mouse_level);
+    qemu_set_irq(s->irqs[I8042_KBD_IRQ], irq_kbd_level);
+    qemu_set_irq(s->irqs[I8042_MOUSE_IRQ], irq_mouse_level);
 }
 
 static void kbd_deassert_irq(KBDState *s)
@@ -724,8 +724,8 @@ MMIOKBDState *i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     s = &I8042_MMIO(dev)->kbd;
 
-    s->irq_kbd = kbd_irq;
-    s->irq_mouse = mouse_irq;
+    s->irqs[I8042_KBD_IRQ] = kbd_irq;
+    s->irqs[I8042_MOUSE_IRQ] = mouse_irq;
 
     return I8042_MMIO(dev);
 }
@@ -811,8 +811,8 @@ static void i8042_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    s->irq_kbd = isa_get_irq(isadev, isa_s->kbd_irq);
-    s->irq_mouse = isa_get_irq(isadev, isa_s->mouse_irq);
+    s->irqs[I8042_KBD_IRQ] = isa_get_irq(isadev, isa_s->kbd_irq);
+    s->irqs[I8042_MOUSE_IRQ] = isa_get_irq(isadev, isa_s->mouse_irq);
 
     isa_register_ioport(isadev, isa_s->io + 0, 0x60);
     isa_register_ioport(isadev, isa_s->io + 1, 0x64);
diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h
index 9d1f8af964..4ba2664377 100644
--- a/include/hw/input/i8042.h
+++ b/include/hw/input/i8042.h
@@ -12,6 +12,9 @@
 #include "hw/sysbus.h"
 #include "qom/object.h"
 
+#define I8042_KBD_IRQ      0
+#define I8042_MOUSE_IRQ    1
+
 typedef struct KBDState {
     uint8_t write_cmd; /* if non zero, write data to port 60 is expected */
     uint8_t status;
@@ -31,8 +34,7 @@ typedef struct KBDState {
     void *mouse;
     QEMUTimer *throttle_timer;
 
-    qemu_irq irq_kbd;
-    qemu_irq irq_mouse;
+    qemu_irq irqs[2];
     qemu_irq a20_out;
     hwaddr mask;
 } KBDState;
-- 
2.20.1

Re: [PATCH 35/50] pckbd: replace irq_kbd and irq_mouse with qemu_irq array in KBDState

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This allows both IRQs to be declared as a single qdev gpio array.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 36/50] pl050: switch over from update_irq() function to PS2 device gpio

[Mark Cave-Ayland]

Add a new pl050_init() function which initialises a qdev input gpio for handling
incoming PS2 IRQs, and then wire up the PS2 device to use it. At the same time
set update_irq() and update_arg to NULL in ps2_kbd_init() and ps2_mouse_init()
to ensure that any accidental attempt to use the legacy update_irq() function will
cause a NULL pointer dereference.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pl050.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/input/pl050.c b/hw/input/pl050.c
index 66f8c20d9f..fe6a281307 100644
--- a/hw/input/pl050.c
+++ b/hw/input/pl050.c
@@ -32,6 +32,8 @@ struct PL050State {
     bool is_mouse;
 };
 
+#define PL050_INPUT_IRQ    0
+
 static const VMStateDescription vmstate_pl050 = {
     .name = "pl050",
     .version_id = 2,
@@ -65,7 +67,7 @@ static void pl050_update_irq(PL050State *s)
     qemu_set_irq(s->irq, level);
 }
 
-static void pl050_set_irq(void *opaque, int level)
+static void pl050_set_irq(void *opaque, int n, int level)
 {
     PL050State *s = (PL050State *)opaque;
 
@@ -164,10 +166,12 @@ static void pl050_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->iomem);
     sysbus_init_irq(sbd, &s->irq);
     if (s->is_mouse) {
-        s->dev = ps2_mouse_init(pl050_set_irq, s);
+        s->dev = ps2_mouse_init(NULL, NULL);
     } else {
-        s->dev = ps2_kbd_init(pl050_set_irq, s);
+        s->dev = ps2_kbd_init(NULL, NULL);
     }
+    qdev_connect_gpio_out(DEVICE(s->dev), PS2_DEVICE_IRQ,
+                          qdev_get_gpio_in(dev, PL050_INPUT_IRQ));
 }
 
 static void pl050_keyboard_init(Object *obj)
@@ -196,6 +200,11 @@ static const TypeInfo pl050_mouse_info = {
     .instance_init = pl050_mouse_init,
 };
 
+static void pl050_init(Object *obj)
+{
+    qdev_init_gpio_in(DEVICE(obj), pl050_set_irq, 1);
+}
+
 static void pl050_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
@@ -207,6 +216,7 @@ static void pl050_class_init(ObjectClass *oc, void *data)
 static const TypeInfo pl050_type_info = {
     .name          = TYPE_PL050,
     .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_init = pl050_init,
     .instance_size = sizeof(PL050State),
     .abstract      = true,
     .class_init    = pl050_class_init,
-- 
2.20.1

Re: [PATCH 36/50] pl050: switch over from update_irq() function to PS2 device gpio

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Add a new pl050_init() function which initialises a qdev input gpio for handling
> incoming PS2 IRQs, and then wire up the PS2 device to use it. At the same time
> set update_irq() and update_arg to NULL in ps2_kbd_init() and ps2_mouse_init()
> to ensure that any accidental attempt to use the legacy update_irq() function will
> cause a NULL pointer dereference.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/pl050.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/hw/input/pl050.c b/hw/input/pl050.c
> index 66f8c20d9f..fe6a281307 100644
> --- a/hw/input/pl050.c
> +++ b/hw/input/pl050.c
> @@ -32,6 +32,8 @@ struct PL050State {
>      bool is_mouse;
>  };
>
> +#define PL050_INPUT_IRQ

> +
>  static const VMStateDescription vmstate_pl050 = {
>      .name = "pl050",
>      .version_id = 2,
> @@ -65,7 +67,7 @@ static void pl050_update_irq(PL050State *s)
>      qemu_set_irq(s->irq, level);
>  }
>
> -static void pl050_set_irq(void *opaque, int level)
> +static void pl050_set_irq(void *opaque, int n, int level)
>  {
>      PL050State *s = (PL050State *)opaque;
>
> @@ -164,10 +166,12 @@ static void pl050_realize(DeviceState *dev, Error **errp)
>      sysbus_init_mmio(sbd, &s->iomem);
>      sysbus_init_irq(sbd, &s->irq);
>      if (s->is_mouse) {
> -        s->dev = ps2_mouse_init(pl050_set_irq, s);
> +        s->dev = ps2_mouse_init(NULL, NULL);
>      } else {
> -        s->dev = ps2_kbd_init(pl050_set_irq, s);
> +        s->dev = ps2_kbd_init(NULL, NULL);
>      }
> +    qdev_connect_gpio_out(DEVICE(s->dev), PS2_DEVICE_IRQ,
> +                          qdev_get_gpio_in(dev, PL050_INPUT_IRQ));
>  }
>
>  static void pl050_keyboard_init(Object *obj)
> @@ -196,6 +200,11 @@ static const TypeInfo pl050_mouse_info = {
>      .instance_init = pl050_mouse_init,
>  };
>
> +static void pl050_init(Object *obj)
> +{
> +    qdev_init_gpio_in(DEVICE(obj), pl050_set_irq, 1);
> +}

Better to use a named GPIO input here I think, especially
since it's kind of an internal implementation detail of the device.

thanks
-- PMM

[PATCH 37/50] lasips2: QOMify LASIPS2State

[Mark Cave-Ayland]

Currently lasip2_init() creates a new LASIPS2State directly which is used by the HPPA
machine. Introduce a new LASIPS2 QOM type that will soon be used to allow the HPPA
machine to be wired up using standard qdev GPIOs.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/lasips2.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index adfde1684f..db0a791e6c 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -24,6 +24,7 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "hw/qdev-properties.h"
+#include "hw/sysbus.h"
 #include "hw/input/ps2.h"
 #include "hw/input/lasips2.h"
 #include "exec/hwaddr.h"
@@ -31,6 +32,7 @@
 #include "exec/address-spaces.h"
 #include "migration/vmstate.h"
 #include "hw/irq.h"
+#include "qapi/error.h"
 
 
 struct LASIPS2State;
@@ -45,11 +47,16 @@ typedef struct LASIPS2Port {
     bool irq;
 } LASIPS2Port;
 
-typedef struct LASIPS2State {
+struct LASIPS2State {
+    SysBusDevice parent_obj;
+
     LASIPS2Port kbd;
     LASIPS2Port mouse;
     qemu_irq irq;
-} LASIPS2State;
+};
+
+#define TYPE_LASIPS2 "lasips2"
+OBJECT_DECLARE_SIMPLE_TYPE(LASIPS2State, LASIPS2)
 
 static const VMStateDescription vmstate_lasips2 = {
     .name = "lasips2",
@@ -265,8 +272,11 @@ void lasips2_init(MemoryRegion *address_space,
                   hwaddr base, qemu_irq irq)
 {
     LASIPS2State *s;
+    DeviceState *dev;
 
-    s = g_new0(LASIPS2State, 1);
+    dev = qdev_new(TYPE_LASIPS2);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    s = LASIPS2(dev);
 
     s->irq = irq;
     s->mouse.id = 1;
@@ -286,3 +296,16 @@ void lasips2_init(MemoryRegion *address_space,
                           "lasips2-mouse", 0x100);
     memory_region_add_subregion(address_space, base + 0x100, &s->mouse.reg);
 }
+
+static const TypeInfo lasips2_info = {
+    .name          = TYPE_LASIPS2,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(LASIPS2State)
+};
+
+static void lasips2_register_types(void)
+{
+    type_register_static(&lasips2_info);
+}
+
+type_init(lasips2_register_types)
-- 
2.20.1

Re: [PATCH 37/50] lasips2: QOMify LASIPS2State

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Currently lasip2_init() creates a new LASIPS2State directly which is used by the HPPA
> machine. Introduce a new LASIPS2 QOM type that will soon be used to allow the HPPA
> machine to be wired up using standard qdev GPIOs.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 38/50] lasips2: move lasips2 QOM types from lasips2.c to lasips2.h

[Mark Cave-Ayland]

This allows the QOM types in lasips2.c to be used elsewhere by simply including
lasips2.h.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/lasips2.c         | 23 -----------------------
 include/hw/input/lasips2.h | 22 ++++++++++++++++++++++
 2 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index db0a791e6c..2caa80bd3c 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -35,29 +35,6 @@
 #include "qapi/error.h"
 
 
-struct LASIPS2State;
-typedef struct LASIPS2Port {
-    struct LASIPS2State *parent;
-    MemoryRegion reg;
-    void *dev;
-    uint8_t id;
-    uint8_t control;
-    uint8_t buf;
-    bool loopback_rbne;
-    bool irq;
-} LASIPS2Port;
-
-struct LASIPS2State {
-    SysBusDevice parent_obj;
-
-    LASIPS2Port kbd;
-    LASIPS2Port mouse;
-    qemu_irq irq;
-};
-
-#define TYPE_LASIPS2 "lasips2"
-OBJECT_DECLARE_SIMPLE_TYPE(LASIPS2State, LASIPS2)
-
 static const VMStateDescription vmstate_lasips2 = {
     .name = "lasips2",
     .version_id = 0,
diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h
index 0cd7b59064..ddcea74c14 100644
--- a/include/hw/input/lasips2.h
+++ b/include/hw/input/lasips2.h
@@ -8,8 +8,30 @@
 #define HW_INPUT_LASIPS2_H
 
 #include "exec/hwaddr.h"
+#include "hw/sysbus.h"
+
+struct LASIPS2State;
+typedef struct LASIPS2Port {
+    struct LASIPS2State *parent;
+    MemoryRegion reg;
+    void *dev;
+    uint8_t id;
+    uint8_t control;
+    uint8_t buf;
+    bool loopback_rbne;
+    bool irq;
+} LASIPS2Port;
+
+struct LASIPS2State {
+    SysBusDevice parent_obj;
+
+    LASIPS2Port kbd;
+    LASIPS2Port mouse;
+    qemu_irq irq;
+};
 
 #define TYPE_LASIPS2 "lasips2"
+OBJECT_DECLARE_SIMPLE_TYPE(LASIPS2State, LASIPS2)
 
 void lasips2_init(MemoryRegion *address_space, hwaddr base, qemu_irq irq);
 
-- 
2.20.1

Re: [PATCH 38/50] lasips2: move lasips2 QOM types from lasips2.c to lasips2.h

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This allows the QOM types in lasips2.c to be used elsewhere by simply including
> lasips2.h.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 39/50] lasips2: rename lasips2_init() to lasips2_initfn() and update it to return the LASIPS2 device

[Mark Cave-Ayland]

When QOMifying a device it is typical to use _init() as the suffix for an
instance_init function, however this name is already in use by the legacy LASIPS2
wrapper function. Eventually the wrapper function will be removed, but for now
rename it to lasips2_initfn() to avoid a naming collision.

At the same time update lasips2_initfn() return the LASIPS2 device so that it
can later be accessed using qdev APIs by the HPPA machine.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/hppa/machine.c          | 4 ++--
 hw/input/lasips2.c         | 6 ++++--
 include/hw/input/lasips2.h | 3 ++-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index d1e174b1f4..1fceaf2072 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -286,8 +286,8 @@ static void machine_hppa_init(MachineState *machine)
     }
 
     /* PS/2 Keyboard/Mouse */
-    lasips2_init(addr_space, LASI_PS2KBD_HPA,
-                 qdev_get_gpio_in(lasi_dev, LASI_IRQ_PS2KBD_HPA));
+    lasips2_initfn(addr_space, LASI_PS2KBD_HPA,
+                   qdev_get_gpio_in(lasi_dev, LASI_IRQ_PS2KBD_HPA));
 
     /* register power switch emulation */
     qemu_register_powerdown_notifier(&hppa_system_powerdown_notifier);
diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index 2caa80bd3c..85da4081e3 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -245,8 +245,8 @@ static void lasips2_port_set_irq(void *opaque, int level)
     lasips2_update_irq(port->parent);
 }
 
-void lasips2_init(MemoryRegion *address_space,
-                  hwaddr base, qemu_irq irq)
+LASIPS2State *lasips2_initfn(MemoryRegion *address_space,
+                             hwaddr base, qemu_irq irq)
 {
     LASIPS2State *s;
     DeviceState *dev;
@@ -272,6 +272,8 @@ void lasips2_init(MemoryRegion *address_space,
     memory_region_init_io(&s->mouse.reg, NULL, &lasips2_reg_ops, &s->mouse,
                           "lasips2-mouse", 0x100);
     memory_region_add_subregion(address_space, base + 0x100, &s->mouse.reg);
+
+    return s;
 }
 
 static const TypeInfo lasips2_info = {
diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h
index ddcea74c14..5a35c22f73 100644
--- a/include/hw/input/lasips2.h
+++ b/include/hw/input/lasips2.h
@@ -33,6 +33,7 @@ struct LASIPS2State {
 #define TYPE_LASIPS2 "lasips2"
 OBJECT_DECLARE_SIMPLE_TYPE(LASIPS2State, LASIPS2)
 
-void lasips2_init(MemoryRegion *address_space, hwaddr base, qemu_irq irq);
+LASIPS2State *lasips2_initfn(MemoryRegion *address_space, hwaddr base,
+                             qemu_irq irq);
 
 #endif /* HW_INPUT_LASIPS2_H */
-- 
2.20.1

Re: [PATCH 39/50] lasips2: rename lasips2_init() to lasips2_initfn() and update it to return the LASIPS2 device

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> When QOMifying a device it is typical to use _init() as the suffix for an
> instance_init function, however this name is already in use by the legacy LASIPS2
> wrapper function. Eventually the wrapper function will be removed, but for now
> rename it to lasips2_initfn() to avoid a naming collision.
>
> At the same time update lasips2_initfn() return the LASIPS2 device so that it
> can later be accessed using qdev APIs by the HPPA machine.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 40/50] lasips2: implement lasips2_init() function

[Mark Cave-Ayland]

Move the initialisation of the keyboard and mouse memory regions to lasips2_init()
and expose them as SysBus memory regions.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/lasips2.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index 85da4081e3..8d3a2d88e8 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -265,20 +265,30 @@ LASIPS2State *lasips2_initfn(MemoryRegion *address_space,
     s->kbd.dev = ps2_kbd_init(lasips2_port_set_irq, &s->kbd);
     s->mouse.dev = ps2_mouse_init(lasips2_port_set_irq, &s->mouse);
 
-    memory_region_init_io(&s->kbd.reg, NULL, &lasips2_reg_ops, &s->kbd,
-                          "lasips2-kbd", 0x100);
     memory_region_add_subregion(address_space, base, &s->kbd.reg);
 
-    memory_region_init_io(&s->mouse.reg, NULL, &lasips2_reg_ops, &s->mouse,
-                          "lasips2-mouse", 0x100);
     memory_region_add_subregion(address_space, base + 0x100, &s->mouse.reg);
 
     return s;
 }
 
+static void lasips2_init(Object *obj)
+{
+    LASIPS2State *s = LASIPS2(obj);
+
+    memory_region_init_io(&s->kbd.reg, obj, &lasips2_reg_ops, &s->kbd,
+                          "lasips2-kbd", 0x100);
+    memory_region_init_io(&s->mouse.reg, obj, &lasips2_reg_ops, &s->mouse,
+                          "lasips2-mouse", 0x100);
+
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->kbd.reg);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg);
+}
+
 static const TypeInfo lasips2_info = {
     .name          = TYPE_LASIPS2,
     .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_init = lasips2_init,
     .instance_size = sizeof(LASIPS2State)
 };
 
-- 
2.20.1

Re: [PATCH 40/50] lasips2: implement lasips2_init() function

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Move the initialisation of the keyboard and mouse memory regions to lasips2_init()
> and expose them as SysBus memory regions.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/lasips2.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 41/50] lasips2: move mapping of LASIPS2 registers to HPPA machine

[Mark Cave-Ayland]

Now that the register memory regions are exposed as SysBus memory regions, move
the mapping of the LASIPS2 registers from lasips2_initfn() to the HPPA machine
(which is its only user).

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/hppa/machine.c          | 11 +++++++++--
 hw/input/lasips2.c         |  7 +------
 include/hw/input/lasips2.h |  3 +--
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 1fceaf2072..f6dd79e211 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -286,8 +286,15 @@ static void machine_hppa_init(MachineState *machine)
     }
 
     /* PS/2 Keyboard/Mouse */
-    lasips2_initfn(addr_space, LASI_PS2KBD_HPA,
-                   qdev_get_gpio_in(lasi_dev, LASI_IRQ_PS2KBD_HPA));
+    dev = DEVICE(lasips2_initfn(LASI_PS2KBD_HPA,
+                                qdev_get_gpio_in(lasi_dev,
+                                                 LASI_IRQ_PS2KBD_HPA)));
+    memory_region_add_subregion(addr_space, LASI_PS2KBD_HPA,
+                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev),
+                                                       0));
+    memory_region_add_subregion(addr_space, LASI_PS2KBD_HPA + 0x100,
+                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev),
+                                                       1));
 
     /* register power switch emulation */
     qemu_register_powerdown_notifier(&hppa_system_powerdown_notifier);
diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index 8d3a2d88e8..84e7a1feee 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -245,8 +245,7 @@ static void lasips2_port_set_irq(void *opaque, int level)
     lasips2_update_irq(port->parent);
 }
 
-LASIPS2State *lasips2_initfn(MemoryRegion *address_space,
-                             hwaddr base, qemu_irq irq)
+LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq)
 {
     LASIPS2State *s;
     DeviceState *dev;
@@ -265,10 +264,6 @@ LASIPS2State *lasips2_initfn(MemoryRegion *address_space,
     s->kbd.dev = ps2_kbd_init(lasips2_port_set_irq, &s->kbd);
     s->mouse.dev = ps2_mouse_init(lasips2_port_set_irq, &s->mouse);
 
-    memory_region_add_subregion(address_space, base, &s->kbd.reg);
-
-    memory_region_add_subregion(address_space, base + 0x100, &s->mouse.reg);
-
     return s;
 }
 
diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h
index 5a35c22f73..b9723073e1 100644
--- a/include/hw/input/lasips2.h
+++ b/include/hw/input/lasips2.h
@@ -33,7 +33,6 @@ struct LASIPS2State {
 #define TYPE_LASIPS2 "lasips2"
 OBJECT_DECLARE_SIMPLE_TYPE(LASIPS2State, LASIPS2)
 
-LASIPS2State *lasips2_initfn(MemoryRegion *address_space, hwaddr base,
-                             qemu_irq irq);
+LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq);
 
 #endif /* HW_INPUT_LASIPS2_H */
-- 
2.20.1

Re: [PATCH 41/50] lasips2: move mapping of LASIPS2 registers to HPPA machine

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Now that the register memory regions are exposed as SysBus memory regions, move
> the mapping of the LASIPS2 registers from lasips2_initfn() to the HPPA machine
> (which is its only user).
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 42/50] lasips2: move initialisation of PS2 ports from lasi_initfn() to lasi_init()

[Mark Cave-Ayland]

This can be improved once the ps2_kbd_init() and ps2_mouse_init() functions have
been removed, but for now move the existing logic from lasi_initfn() to
lasi_init(). At the same time explicitly set keyboard port id to 0, even if it
isn't technically required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/lasips2.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index 84e7a1feee..bd89c03996 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -255,9 +255,6 @@ LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq)
     s = LASIPS2(dev);
 
     s->irq = irq;
-    s->mouse.id = 1;
-    s->kbd.parent = s;
-    s->mouse.parent = s;
 
     vmstate_register(NULL, base, &vmstate_lasips2, s);
 
@@ -271,6 +268,11 @@ static void lasips2_init(Object *obj)
 {
     LASIPS2State *s = LASIPS2(obj);
 
+    s->kbd.id = 0;
+    s->mouse.id = 1;
+    s->kbd.parent = s;
+    s->mouse.parent = s;
+
     memory_region_init_io(&s->kbd.reg, obj, &lasips2_reg_ops, &s->kbd,
                           "lasips2-kbd", 0x100);
     memory_region_init_io(&s->mouse.reg, obj, &lasips2_reg_ops, &s->mouse,
-- 
2.20.1

Re: [PATCH 42/50] lasips2: move initialisation of PS2 ports from lasi_initfn() to lasi_init()

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This can be improved once the ps2_kbd_init() and ps2_mouse_init() functions have
> been removed, but for now move the existing logic from lasi_initfn() to
> lasi_init(). At the same time explicitly set keyboard port id to 0, even if it
> isn't technically required.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/lasips2.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 43/50] lasips2: add base property

[Mark Cave-Ayland]

This is in preparation for handling vmstate_register() within the device.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/lasips2.c         | 17 ++++++++++++++++-
 include/hw/input/lasips2.h |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index bd89c03996..a7c7192c07 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -251,6 +251,7 @@ LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq)
     DeviceState *dev;
 
     dev = qdev_new(TYPE_LASIPS2);
+    qdev_prop_set_uint64(dev, "base", base);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     s = LASIPS2(dev);
 
@@ -282,11 +283,25 @@ static void lasips2_init(Object *obj)
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg);
 }
 
+static Property lasips2_properties[] = {
+    DEFINE_PROP_UINT64("base", LASIPS2State, base, UINT64_MAX),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void lasips2_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    device_class_set_props(dc, lasips2_properties);
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+}
+
 static const TypeInfo lasips2_info = {
     .name          = TYPE_LASIPS2,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_init = lasips2_init,
-    .instance_size = sizeof(LASIPS2State)
+    .instance_size = sizeof(LASIPS2State),
+    .class_init    = lasips2_class_init
 };
 
 static void lasips2_register_types(void)
diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h
index b9723073e1..7e4437b925 100644
--- a/include/hw/input/lasips2.h
+++ b/include/hw/input/lasips2.h
@@ -25,6 +25,7 @@ typedef struct LASIPS2Port {
 struct LASIPS2State {
     SysBusDevice parent_obj;
 
+    hwaddr base;
     LASIPS2Port kbd;
     LASIPS2Port mouse;
     qemu_irq irq;
-- 
2.20.1

Re: [PATCH 43/50] lasips2: add base property

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This is in preparation for handling vmstate_register() within the device.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/lasips2.c         | 17 ++++++++++++++++-
>  include/hw/input/lasips2.h |  1 +
>  2 files changed, 17 insertions(+), 1 deletion(-)


>  static const TypeInfo lasips2_info = {
>      .name          = TYPE_LASIPS2,
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_init = lasips2_init,
> -    .instance_size = sizeof(LASIPS2State)
> +    .instance_size = sizeof(LASIPS2State),
> +    .class_init    = lasips2_class_init

If you put the trailing comma at the end of this last line, then
the next person who comes along to add a new line at the end of
the struct won't have to edit the preceding line to add the comma.

Anyway, as a code change this is fine, so
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
but as I note in review of patch 44 it would be better to
avoid vmstate_register(), in which case we don't need the property.

thanks
-- PMM

[PATCH 44/50] lasips2: implement lasips2_realize()

[Mark Cave-Ayland]

Move ps2_kbd_init() and ps2_mouse_init() from lasips2_initfn() to lasips2_realize.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/lasips2.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index a7c7192c07..6849b71e5c 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -256,13 +256,17 @@ LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq)
     s = LASIPS2(dev);
 
     s->irq = irq;
+    return s;
+}
+
+static void lasips2_realize(DeviceState *dev, Error **errp)
+{
+    LASIPS2State *s = LASIPS2(dev);
 
-    vmstate_register(NULL, base, &vmstate_lasips2, s);
+    vmstate_register(NULL, s->base, &vmstate_lasips2, s);
 
     s->kbd.dev = ps2_kbd_init(lasips2_port_set_irq, &s->kbd);
     s->mouse.dev = ps2_mouse_init(lasips2_port_set_irq, &s->mouse);
-
-    return s;
 }
 
 static void lasips2_init(Object *obj)
@@ -292,6 +296,7 @@ static void lasips2_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->realize = lasips2_realize;
     device_class_set_props(dc, lasips2_properties);
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
-- 
2.20.1

Re: [PATCH 44/50] lasips2: implement lasips2_realize()

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Move ps2_kbd_init() and ps2_mouse_init() from lasips2_initfn() to lasips2_realize.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/lasips2.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
> index a7c7192c07..6849b71e5c 100644
> --- a/hw/input/lasips2.c
> +++ b/hw/input/lasips2.c
> @@ -256,13 +256,17 @@ LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq)
>      s = LASIPS2(dev);
>
>      s->irq = irq;
> +    return s;
> +}
> +
> +static void lasips2_realize(DeviceState *dev, Error **errp)
> +{
> +    LASIPS2State *s = LASIPS2(dev);
>
> -    vmstate_register(NULL, base, &vmstate_lasips2, s);
> +    vmstate_register(NULL, s->base, &vmstate_lasips2, s);
>
>      s->kbd.dev = ps2_kbd_init(lasips2_port_set_irq, &s->kbd);
>      s->mouse.dev = ps2_mouse_init(lasips2_port_set_irq, &s->mouse);
> -
> -    return s;
>  }
>
>  static void lasips2_init(Object *obj)
> @@ -292,6 +296,7 @@ static void lasips2_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
> +    dc->realize = lasips2_realize;
>      device_class_set_props(dc, lasips2_properties);
>      set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }

This is a valid refactoring, so in that sense
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

but I'd prefer it if we just broke migration compat for this machine
and converted it to use dc->vmsd rather than vmstate_register().

thanks
-- PMM

[PATCH 45/50] lasips2: use qdev gpio for output IRQ

[Mark Cave-Ayland]

This enables the IRQ to be wired up using qdev_connect_gpio_out() in
lasips2_initfn().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/lasips2.c         | 8 ++++----
 include/hw/input/lasips2.h | 2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index 6849b71e5c..644cf70955 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -247,16 +247,14 @@ static void lasips2_port_set_irq(void *opaque, int level)
 
 LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq)
 {
-    LASIPS2State *s;
     DeviceState *dev;
 
     dev = qdev_new(TYPE_LASIPS2);
     qdev_prop_set_uint64(dev, "base", base);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    s = LASIPS2(dev);
+    qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq);
 
-    s->irq = irq;
-    return s;
+    return LASIPS2(dev);
 }
 
 static void lasips2_realize(DeviceState *dev, Error **errp)
@@ -285,6 +283,8 @@ static void lasips2_init(Object *obj)
 
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->kbd.reg);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg);
+
+    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
 }
 
 static Property lasips2_properties[] = {
diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h
index 7e4437b925..d3e9719d65 100644
--- a/include/hw/input/lasips2.h
+++ b/include/hw/input/lasips2.h
@@ -22,6 +22,8 @@ typedef struct LASIPS2Port {
     bool irq;
 } LASIPS2Port;
 
+#define LASIPS2_IRQ    0
+
 struct LASIPS2State {
     SysBusDevice parent_obj;
 
-- 
2.20.1

Re: [PATCH 45/50] lasips2: use qdev gpio for output IRQ

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This enables the IRQ to be wired up using qdev_connect_gpio_out() in
> lasips2_initfn().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/lasips2.c         | 8 ++++----
>  include/hw/input/lasips2.h | 2 ++
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
> index 6849b71e5c..644cf70955 100644
> --- a/hw/input/lasips2.c
> +++ b/hw/input/lasips2.c
> @@ -247,16 +247,14 @@ static void lasips2_port_set_irq(void *opaque, int level)
>
>  LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq)
>  {
> -    LASIPS2State *s;
>      DeviceState *dev;
>
>      dev = qdev_new(TYPE_LASIPS2);
>      qdev_prop_set_uint64(dev, "base", base);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> -    s = LASIPS2(dev);
> +    qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq);
>
> -    s->irq = irq;
> -    return s;
> +    return LASIPS2(dev);
>  }
>
>  static void lasips2_realize(DeviceState *dev, Error **errp)
> @@ -285,6 +283,8 @@ static void lasips2_init(Object *obj)
>
>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->kbd.reg);
>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg);
> +
> +    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
>  }
>
>  static Property lasips2_properties[] = {
> diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h
> index 7e4437b925..d3e9719d65 100644
> --- a/include/hw/input/lasips2.h
> +++ b/include/hw/input/lasips2.h
> @@ -22,6 +22,8 @@ typedef struct LASIPS2Port {
>      bool irq;
>  } LASIPS2Port;
>
> +#define LASIPS2_IRQ    0

If you find yourself #defining names for IRQ lines then this is
probably a sign you should be using named GPIO lines :-)

Alternatively, maybe use sysbus_init_irq()? By convention the
only sysbus IRQ on a device is generally "its IRQ line".

-- PMM

Re: [PATCH 45/50] lasips2: use qdev gpio for output IRQ

[Mark Cave-Ayland]

On 09/06/2022 12:18, Peter Maydell wrote:

> On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> This enables the IRQ to be wired up using qdev_connect_gpio_out() in
>> lasips2_initfn().
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/input/lasips2.c         | 8 ++++----
>>   include/hw/input/lasips2.h | 2 ++
>>   2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
>> index 6849b71e5c..644cf70955 100644
>> --- a/hw/input/lasips2.c
>> +++ b/hw/input/lasips2.c
>> @@ -247,16 +247,14 @@ static void lasips2_port_set_irq(void *opaque, int level)
>>
>>   LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq)
>>   {
>> -    LASIPS2State *s;
>>       DeviceState *dev;
>>
>>       dev = qdev_new(TYPE_LASIPS2);
>>       qdev_prop_set_uint64(dev, "base", base);
>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> -    s = LASIPS2(dev);
>> +    qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq);
>>
>> -    s->irq = irq;
>> -    return s;
>> +    return LASIPS2(dev);
>>   }
>>
>>   static void lasips2_realize(DeviceState *dev, Error **errp)
>> @@ -285,6 +283,8 @@ static void lasips2_init(Object *obj)
>>
>>       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->kbd.reg);
>>       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg);
>> +
>> +    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
>>   }
>>
>>   static Property lasips2_properties[] = {
>> diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h
>> index 7e4437b925..d3e9719d65 100644
>> --- a/include/hw/input/lasips2.h
>> +++ b/include/hw/input/lasips2.h
>> @@ -22,6 +22,8 @@ typedef struct LASIPS2Port {
>>       bool irq;
>>   } LASIPS2Port;
>>
>> +#define LASIPS2_IRQ    0
> 
> If you find yourself #defining names for IRQ lines then this is
> probably a sign you should be using named GPIO lines :-)

Yeah that's something I've done a few times here, mainly to have just one "set IRQ" 
function rather a separate one for both keyboard and mouse. It takes a bit more work, 
but I can certainly separate them out.

> Alternatively, maybe use sysbus_init_irq()? By convention the
> only sysbus IRQ on a device is generally "its IRQ line".

Thinking longer term about sysbus, I can see that sysbus_init_irq() would be one of 
the top entries on my list of things to go. For that reason I'd like to stick to 
using gpios here :)


ATB,

Mark.

Re: [PATCH 45/50] lasips2: use qdev gpio for output IRQ

[Mark Cave-Ayland]

On 10/06/2022 08:17, Mark Cave-Ayland wrote:

> On 09/06/2022 12:18, Peter Maydell wrote:
> 
>> On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>
>>> This enables the IRQ to be wired up using qdev_connect_gpio_out() in
>>> lasips2_initfn().
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/input/lasips2.c         | 8 ++++----
>>>   include/hw/input/lasips2.h | 2 ++
>>>   2 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
>>> index 6849b71e5c..644cf70955 100644
>>> --- a/hw/input/lasips2.c
>>> +++ b/hw/input/lasips2.c
>>> @@ -247,16 +247,14 @@ static void lasips2_port_set_irq(void *opaque, int level)
>>>
>>>   LASIPS2State *lasips2_initfn(hwaddr base, qemu_irq irq)
>>>   {
>>> -    LASIPS2State *s;
>>>       DeviceState *dev;
>>>
>>>       dev = qdev_new(TYPE_LASIPS2);
>>>       qdev_prop_set_uint64(dev, "base", base);
>>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>> -    s = LASIPS2(dev);
>>> +    qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq);
>>>
>>> -    s->irq = irq;
>>> -    return s;
>>> +    return LASIPS2(dev);
>>>   }
>>>
>>>   static void lasips2_realize(DeviceState *dev, Error **errp)
>>> @@ -285,6 +283,8 @@ static void lasips2_init(Object *obj)
>>>
>>>       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->kbd.reg);
>>>       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg);
>>> +
>>> +    qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
>>>   }
>>>
>>>   static Property lasips2_properties[] = {
>>> diff --git a/include/hw/input/lasips2.h b/include/hw/input/lasips2.h
>>> index 7e4437b925..d3e9719d65 100644
>>> --- a/include/hw/input/lasips2.h
>>> +++ b/include/hw/input/lasips2.h
>>> @@ -22,6 +22,8 @@ typedef struct LASIPS2Port {
>>>       bool irq;
>>>   } LASIPS2Port;
>>>
>>> +#define LASIPS2_IRQ    0
>>
>> If you find yourself #defining names for IRQ lines then this is
>> probably a sign you should be using named GPIO lines :-)
> 
> Yeah that's something I've done a few times here, mainly to have just one "set IRQ" 
> function rather a separate one for both keyboard and mouse. It takes a bit more work, 
> but I can certainly separate them out.

Looking at this again, the gpio being defined here actually is the (only) lasips2 
output IRQ, and so should be left unnamed.

The reason for adding LASIPS2_IRQ was so that the gpio connection process looked like:

     qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq);

instead of:

     qdev_connect_gpio_out(dev, 0, irq);

Would you still prefer for me to simply hardcode 0 here and drop the LASIPS2_IRQ 
define in this case since there is only one output IRQ?

>> Alternatively, maybe use sysbus_init_irq()? By convention the
>> only sysbus IRQ on a device is generally "its IRQ line".
> 
> Thinking longer term about sysbus, I can see that sysbus_init_irq() would be one of 
> the top entries on my list of things to go. For that reason I'd like to stick to 
> using gpios here :)


ATB,

Mark.

Re: [PATCH 45/50] lasips2: use qdev gpio for output IRQ

[Peter Maydell]

On Sat, 11 Jun 2022 at 16:44, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 10/06/2022 08:17, Mark Cave-Ayland wrote:
>
> > On 09/06/2022 12:18, Peter Maydell wrote:
> >> If you find yourself #defining names for IRQ lines then this is
> >> probably a sign you should be using named GPIO lines :-)
> >
> > Yeah that's something I've done a few times here, mainly to have just one "set IRQ"
> > function rather a separate one for both keyboard and mouse. It takes a bit more work,
> > but I can certainly separate them out.
>
> Looking at this again, the gpio being defined here actually is the (only) lasips2
> output IRQ, and so should be left unnamed.
>
> The reason for adding LASIPS2_IRQ was so that the gpio connection process looked like:
>
>      qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq);
>
> instead of:
>
>      qdev_connect_gpio_out(dev, 0, irq);
>
> Would you still prefer for me to simply hardcode 0 here and drop the LASIPS2_IRQ
> define in this case since there is only one output IRQ?

Well, I think that "unnamed GPIO out" lines should be for
actual GPIO lines, ie on a GPIO controller or similar.
If you want an outbound IRQ line and don't want to name it,
that's what sysbus IRQ lines do. Otherwise, name the GPIO line.

thanks
-- PMM

Re: [PATCH 45/50] lasips2: use qdev gpio for output IRQ

[Mark Cave-Ayland]

On 20/06/2022 11:17, Peter Maydell wrote:

> On Sat, 11 Jun 2022 at 16:44, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 10/06/2022 08:17, Mark Cave-Ayland wrote:
>>
>>> On 09/06/2022 12:18, Peter Maydell wrote:
>>>> If you find yourself #defining names for IRQ lines then this is
>>>> probably a sign you should be using named GPIO lines :-)
>>>
>>> Yeah that's something I've done a few times here, mainly to have just one "set IRQ"
>>> function rather a separate one for both keyboard and mouse. It takes a bit more work,
>>> but I can certainly separate them out.
>>
>> Looking at this again, the gpio being defined here actually is the (only) lasips2
>> output IRQ, and so should be left unnamed.
>>
>> The reason for adding LASIPS2_IRQ was so that the gpio connection process looked like:
>>
>>       qdev_connect_gpio_out(dev, LASIPS2_IRQ, irq);
>>
>> instead of:
>>
>>       qdev_connect_gpio_out(dev, 0, irq);
>>
>> Would you still prefer for me to simply hardcode 0 here and drop the LASIPS2_IRQ
>> define in this case since there is only one output IRQ?
> 
> Well, I think that "unnamed GPIO out" lines should be for
> actual GPIO lines, ie on a GPIO controller or similar.
> If you want an outbound IRQ line and don't want to name it,
> that's what sysbus IRQ lines do. Otherwise, name the GPIO line.

That's interesting - I've always been under the impression that this was the other 
way around, i.e. for a TYPE_DEVICE then unnamed gpios are equivalent to IRQs, and 
that gpio lines for any other non-IRQ purpose should be named :/

So... I'm not really sure what to do in the context of this patchset. Would using 
qdev gpios with suitable "QEMU interface" comments be good enough, or do you really 
feel strongly that these should be converted to SysBus IRQs?

Following on from this we should also consider starting a new thread re: the future 
of SysBusDevice and how we would like to model SysBus IRQs and mmio regions going 
forward.


ATB,

Mark.

Re: [PATCH 45/50] lasips2: use qdev gpio for output IRQ

[Peter Maydell]

On Mon, 20 Jun 2022 at 14:22, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 20/06/2022 11:17, Peter Maydell wrote:
> > Well, I think that "unnamed GPIO out" lines should be for
> > actual GPIO lines, ie on a GPIO controller or similar.
> > If you want an outbound IRQ line and don't want to name it,
> > that's what sysbus IRQ lines do. Otherwise, name the GPIO line.
>
> That's interesting - I've always been under the impression that this was the other
> way around, i.e. for a TYPE_DEVICE then unnamed gpios are equivalent to IRQs, and
> that gpio lines for any other non-IRQ purpose should be named :/

Well, named GPIO lines are relatively new, so if you look at older
devices you'll probably find plenty that use unnamed GPIO lines
for various things including IRQ lines. But I think that for clarity
if you create something called "gpio_out" the obvious thing is that
that's a GPIO output, and if you create something called "sysbus_irq"
the obvious thing is that that's an IRQ line, and if you want to
do something that's neither of those then the clearest thing is
to name the GPIO.

-- PMM

Re: [PATCH 45/50] lasips2: use qdev gpio for output IRQ

[Mark Cave-Ayland]

On 20/06/2022 15:13, Peter Maydell wrote:

> On Mon, 20 Jun 2022 at 14:22, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 20/06/2022 11:17, Peter Maydell wrote:
>>> Well, I think that "unnamed GPIO out" lines should be for
>>> actual GPIO lines, ie on a GPIO controller or similar.
>>> If you want an outbound IRQ line and don't want to name it,
>>> that's what sysbus IRQ lines do. Otherwise, name the GPIO line.
>>
>> That's interesting - I've always been under the impression that this was the other
>> way around, i.e. for a TYPE_DEVICE then unnamed gpios are equivalent to IRQs, and
>> that gpio lines for any other non-IRQ purpose should be named :/
> 
> Well, named GPIO lines are relatively new, so if you look at older
> devices you'll probably find plenty that use unnamed GPIO lines
> for various things including IRQ lines. But I think that for clarity
> if you create something called "gpio_out" the obvious thing is that
> that's a GPIO output, and if you create something called "sysbus_irq"
> the obvious thing is that that's an IRQ line, and if you want to
> do something that's neither of those then the clearest thing is
> to name the GPIO.

Ultimately I'm not too concerned about the choice between sysbus IRQs instead of gpio 
outputs, since making the change later is quite trivial. I've gone ahead and updated 
this patch to use a sysbus IRQ instead of an unnamed gpio out for v2.


ATB,

Mark.

Re: [PATCH 45/50] lasips2: use qdev gpio for output IRQ

[Philippe Mathieu-Daudé via]

On 22/5/22 20:18, Mark Cave-Ayland wrote:
> This enables the IRQ to be wired up using qdev_connect_gpio_out() in
> lasips2_initfn().
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/input/lasips2.c         | 8 ++++----
>   include/hw/input/lasips2.h | 2 ++
>   2 files changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 46/50] lasips2: switch over from update_irq() function to PS2 device gpio

[Mark Cave-Ayland]

Add a qdev gpio input in lasips2_init() by taking the existing lasips2_port_set_irq()
function, updating it accordingly and then renaming to lasips2_set_irq(). Use these
new qdev gpio inputs to wire up the PS2 keyboard and mouse devices.

At the same time set update_irq() and update_arg to NULL in ps2_kbd_init() and
ps2_mouse_init() to ensure that any accidental attempt to use the legacy update_irq()
function will cause a NULL pointer dereference.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/lasips2.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index 644cf70955..12ff95a05f 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -117,6 +117,9 @@ static const char *lasips2_write_reg_name(uint64_t addr)
     }
 }
 
+#define LASIPS2_KBD_INPUT_IRQ      0
+#define LASIPS2_MOUSE_INPUT_IRQ    1
+
 static void lasips2_update_irq(LASIPS2State *s)
 {
     trace_lasips2_intr(s->kbd.irq | s->mouse.irq);
@@ -237,9 +240,10 @@ static const MemoryRegionOps lasips2_reg_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void lasips2_port_set_irq(void *opaque, int level)
+static void lasips2_set_irq(void *opaque, int n, int level)
 {
-    LASIPS2Port *port = opaque;
+    LASIPS2State *s = LASIPS2(opaque);
+    LASIPS2Port *port = (n ? &s->mouse : &s->kbd);
 
     port->irq = level;
     lasips2_update_irq(port->parent);
@@ -263,8 +267,12 @@ static void lasips2_realize(DeviceState *dev, Error **errp)
 
     vmstate_register(NULL, s->base, &vmstate_lasips2, s);
 
-    s->kbd.dev = ps2_kbd_init(lasips2_port_set_irq, &s->kbd);
-    s->mouse.dev = ps2_mouse_init(lasips2_port_set_irq, &s->mouse);
+    s->kbd.dev = ps2_kbd_init(NULL, NULL);
+    qdev_connect_gpio_out(DEVICE(s->kbd.dev), PS2_DEVICE_IRQ,
+                          qdev_get_gpio_in(dev, LASIPS2_KBD_INPUT_IRQ));
+    s->mouse.dev = ps2_mouse_init(NULL, NULL);
+    qdev_connect_gpio_out(DEVICE(s->mouse.dev), PS2_DEVICE_IRQ,
+                          qdev_get_gpio_in(dev, LASIPS2_MOUSE_INPUT_IRQ));
 }
 
 static void lasips2_init(Object *obj)
@@ -285,6 +293,7 @@ static void lasips2_init(Object *obj)
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg);
 
     qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
+    qdev_init_gpio_in(DEVICE(obj), lasips2_set_irq, 2);
 }
 
 static Property lasips2_properties[] = {
-- 
2.20.1

Re: [PATCH 46/50] lasips2: switch over from update_irq() function to PS2 device gpio

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Add a qdev gpio input in lasips2_init() by taking the existing lasips2_port_set_irq()
> function, updating it accordingly and then renaming to lasips2_set_irq(). Use these
> new qdev gpio inputs to wire up the PS2 keyboard and mouse devices.
>
> At the same time set update_irq() and update_arg to NULL in ps2_kbd_init() and
> ps2_mouse_init() to ensure that any accidental attempt to use the legacy update_irq()
> function will cause a NULL pointer dereference.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/lasips2.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
> index 644cf70955..12ff95a05f 100644
> --- a/hw/input/lasips2.c
> +++ b/hw/input/lasips2.c
> @@ -117,6 +117,9 @@ static const char *lasips2_write_reg_name(uint64_t addr)
>      }
>  }
>
> +#define LASIPS2_KBD_INPUT_IRQ      0
> +#define LASIPS2_MOUSE_INPUT_IRQ    1
> +
>  static void lasips2_update_irq(LASIPS2State *s)
>  {
>      trace_lasips2_intr(s->kbd.irq | s->mouse.irq);
> @@ -237,9 +240,10 @@ static const MemoryRegionOps lasips2_reg_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> -static void lasips2_port_set_irq(void *opaque, int level)
> +static void lasips2_set_irq(void *opaque, int n, int level)
>  {
> -    LASIPS2Port *port = opaque;
> +    LASIPS2State *s = LASIPS2(opaque);
> +    LASIPS2Port *port = (n ? &s->mouse : &s->kbd);
>
>      port->irq = level;
>      lasips2_update_irq(port->parent);
> @@ -263,8 +267,12 @@ static void lasips2_realize(DeviceState *dev, Error **errp)
>
>      vmstate_register(NULL, s->base, &vmstate_lasips2, s);
>
> -    s->kbd.dev = ps2_kbd_init(lasips2_port_set_irq, &s->kbd);
> -    s->mouse.dev = ps2_mouse_init(lasips2_port_set_irq, &s->mouse);
> +    s->kbd.dev = ps2_kbd_init(NULL, NULL);
> +    qdev_connect_gpio_out(DEVICE(s->kbd.dev), PS2_DEVICE_IRQ,
> +                          qdev_get_gpio_in(dev, LASIPS2_KBD_INPUT_IRQ));
> +    s->mouse.dev = ps2_mouse_init(NULL, NULL);
> +    qdev_connect_gpio_out(DEVICE(s->mouse.dev), PS2_DEVICE_IRQ,
> +                          qdev_get_gpio_in(dev, LASIPS2_MOUSE_INPUT_IRQ));
>  }
>
>  static void lasips2_init(Object *obj)
> @@ -285,6 +293,7 @@ static void lasips2_init(Object *obj)
>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg);
>
>      qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
> +    qdev_init_gpio_in(DEVICE(obj), lasips2_set_irq, 2);
>  }

These definitely should be named GPIO inputs, as with the pl050.

Aside, if you felt like adding "QEMU interface" comments to these
devices that summarize what the various sysbus IRQs, MMIOs,
qdev GPIOs, etc do, that would be nice, and then gives you a
place to document that these GPIO lines are part of the internal
implementation rather than externally-facing. include/hw/intc/ppc-uic.h
is one example but you can find lots by grepping for "QEMU interface"
in include/hw/.

thanks
-- PMM

Re: [PATCH 46/50] lasips2: switch over from update_irq() function to PS2 device gpio

[Mark Cave-Ayland]

On 09/06/2022 12:21, Peter Maydell wrote:

> On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> Add a qdev gpio input in lasips2_init() by taking the existing lasips2_port_set_irq()
>> function, updating it accordingly and then renaming to lasips2_set_irq(). Use these
>> new qdev gpio inputs to wire up the PS2 keyboard and mouse devices.
>>
>> At the same time set update_irq() and update_arg to NULL in ps2_kbd_init() and
>> ps2_mouse_init() to ensure that any accidental attempt to use the legacy update_irq()
>> function will cause a NULL pointer dereference.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/input/lasips2.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
>> index 644cf70955..12ff95a05f 100644
>> --- a/hw/input/lasips2.c
>> +++ b/hw/input/lasips2.c
>> @@ -117,6 +117,9 @@ static const char *lasips2_write_reg_name(uint64_t addr)
>>       }
>>   }
>>
>> +#define LASIPS2_KBD_INPUT_IRQ      0
>> +#define LASIPS2_MOUSE_INPUT_IRQ    1
>> +
>>   static void lasips2_update_irq(LASIPS2State *s)
>>   {
>>       trace_lasips2_intr(s->kbd.irq | s->mouse.irq);
>> @@ -237,9 +240,10 @@ static const MemoryRegionOps lasips2_reg_ops = {
>>       .endianness = DEVICE_NATIVE_ENDIAN,
>>   };
>>
>> -static void lasips2_port_set_irq(void *opaque, int level)
>> +static void lasips2_set_irq(void *opaque, int n, int level)
>>   {
>> -    LASIPS2Port *port = opaque;
>> +    LASIPS2State *s = LASIPS2(opaque);
>> +    LASIPS2Port *port = (n ? &s->mouse : &s->kbd);
>>
>>       port->irq = level;
>>       lasips2_update_irq(port->parent);
>> @@ -263,8 +267,12 @@ static void lasips2_realize(DeviceState *dev, Error **errp)
>>
>>       vmstate_register(NULL, s->base, &vmstate_lasips2, s);
>>
>> -    s->kbd.dev = ps2_kbd_init(lasips2_port_set_irq, &s->kbd);
>> -    s->mouse.dev = ps2_mouse_init(lasips2_port_set_irq, &s->mouse);
>> +    s->kbd.dev = ps2_kbd_init(NULL, NULL);
>> +    qdev_connect_gpio_out(DEVICE(s->kbd.dev), PS2_DEVICE_IRQ,
>> +                          qdev_get_gpio_in(dev, LASIPS2_KBD_INPUT_IRQ));
>> +    s->mouse.dev = ps2_mouse_init(NULL, NULL);
>> +    qdev_connect_gpio_out(DEVICE(s->mouse.dev), PS2_DEVICE_IRQ,
>> +                          qdev_get_gpio_in(dev, LASIPS2_MOUSE_INPUT_IRQ));
>>   }
>>
>>   static void lasips2_init(Object *obj)
>> @@ -285,6 +293,7 @@ static void lasips2_init(Object *obj)
>>       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mouse.reg);
>>
>>       qdev_init_gpio_out(DEVICE(obj), &s->irq, 1);
>> +    qdev_init_gpio_in(DEVICE(obj), lasips2_set_irq, 2);
>>   }
> 
> These definitely should be named GPIO inputs, as with the pl050.
> 
> Aside, if you felt like adding "QEMU interface" comments to these
> devices that summarize what the various sysbus IRQs, MMIOs,
> qdev GPIOs, etc do, that would be nice, and then gives you a
> place to document that these GPIO lines are part of the internal
> implementation rather than externally-facing. include/hw/intc/ppc-uic.h
> is one example but you can find lots by grepping for "QEMU interface"
> in include/hw/.

Oh interesting, I wasn't aware of this. I'll see if I can add a summary of the 
modelling in v2.


ATB,

Mark.

[PATCH 47/50] pckbd: switch I8042_MMIO device from update_irq() function to PS2 device gpio

[Mark Cave-Ayland]

Define a new qdev input gpio for handling incoming PS2 IRQs, and then wire up the
PS2 keyboard and mouse devices to use it. At the same time set update_irq() and
update_arg to NULL in ps2_kbd_init() and ps2_mouse_init() to ensure that any
accidental attempt to use the legacy update_irq() function will cause a NULL
pointer dereference.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 90acf0dd6a..d89efddda9 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -665,6 +665,21 @@ static const MemoryRegionOps i8042_mmio_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+#define I8042_KBD_INPUT_IRQ        0
+#define I8042_MOUSE_INPUT_IRQ      1
+
+static void i8042_mmio_set_irq(void *opaque, int n, int level)
+{
+    MMIOKBDState *s = I8042_MMIO(opaque);
+    KBDState *ks = &s->kbd;
+
+    if (n) {
+        kbd_update_aux_irq(ks, level);
+    } else {
+        kbd_update_kbd_irq(ks, level);
+    }
+}
+
 static void i8042_mmio_reset(DeviceState *dev)
 {
     MMIOKBDState *s = I8042_MMIO(dev);
@@ -685,15 +700,23 @@ static void i8042_mmio_realize(DeviceState *dev, Error **errp)
     /* Note we can't use dc->vmsd without breaking migration compatibility */
     vmstate_register(NULL, 0, &vmstate_kbd, ks);
 
-    ks->kbd = ps2_kbd_init(kbd_update_kbd_irq, ks);
-    ks->mouse = ps2_mouse_init(kbd_update_aux_irq, ks);
+    ks->kbd = ps2_kbd_init(NULL, NULL);
+    qdev_connect_gpio_out(DEVICE(ks->kbd), PS2_DEVICE_IRQ,
+                          qdev_get_gpio_in(dev, I8042_KBD_INPUT_IRQ));
+    ks->mouse = ps2_mouse_init(NULL, NULL);
+    qdev_connect_gpio_out(DEVICE(ks->mouse), PS2_DEVICE_IRQ,
+                          qdev_get_gpio_in(dev, I8042_MOUSE_INPUT_IRQ));
 }
 
 static void i8042_mmio_init(Object *obj)
 {
     MMIOKBDState *s = I8042_MMIO(obj);
+    KBDState *ks = &s->kbd;
 
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->region);
+
+    qdev_init_gpio_out(DEVICE(obj), ks->irqs, 2);
+    qdev_init_gpio_in(DEVICE(obj), i8042_mmio_set_irq, 2);
 }
 
 static Property i8042_mmio_properties[] = {
@@ -716,16 +739,14 @@ MMIOKBDState *i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
                             ram_addr_t size, hwaddr mask)
 {
     DeviceState *dev;
-    KBDState *s;
 
     dev = qdev_new(TYPE_I8042_MMIO);
     qdev_prop_set_uint64(dev, "mask", mask);
     qdev_prop_set_uint32(dev, "size", size);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    s = &I8042_MMIO(dev)->kbd;
 
-    s->irqs[I8042_KBD_IRQ] = kbd_irq;
-    s->irqs[I8042_MOUSE_IRQ] = mouse_irq;
+    qdev_connect_gpio_out(dev, I8042_KBD_IRQ, kbd_irq);
+    qdev_connect_gpio_out(dev, I8042_MOUSE_IRQ, mouse_irq);
 
     return I8042_MMIO(dev);
 }
-- 
2.20.1

Re: [PATCH 47/50] pckbd: switch I8042_MMIO device from update_irq() function to PS2 device gpio

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Define a new qdev input gpio for handling incoming PS2 IRQs, and then wire up the
> PS2 keyboard and mouse devices to use it. At the same time set update_irq() and
> update_arg to NULL in ps2_kbd_init() and ps2_mouse_init() to ensure that any
> accidental attempt to use the legacy update_irq() function will cause a NULL
> pointer dereference.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/pckbd.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)

Same remark about preferring named-gpio for the internal-implementation
irqs, but otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 48/50] pckbd: add i8042_reset() function to I8042 device

[Mark Cave-Ayland]

This means that it is no longer necessary to call qemu_register_reset() manually
within i8042_realizefn().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index d89efddda9..b2fdd1c999 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -801,6 +801,14 @@ static const MemoryRegionOps i8042_cmd_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static void i8042_reset(DeviceState *dev)
+{
+    ISAKBDState *s = I8042(dev);
+    KBDState *ks = &s->kbd;
+
+    kbd_reset(ks);
+}
+
 static void i8042_initfn(Object *obj)
 {
     ISAKBDState *isa_s = I8042(obj);
@@ -847,7 +855,6 @@ static void i8042_realizefn(DeviceState *dev, Error **errp)
         s->throttle_timer = timer_new_us(QEMU_CLOCK_VIRTUAL,
                                          kbd_throttle_timeout, s);
     }
-    qemu_register_reset(kbd_reset, s);
 }
 
 static void i8042_build_aml(ISADevice *isadev, Aml *scope)
@@ -893,6 +900,7 @@ static void i8042_class_initfn(ObjectClass *klass, void *data)
     ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
 
     device_class_set_props(dc, i8042_properties);
+    dc->reset = i8042_reset;
     dc->realize = i8042_realizefn;
     dc->vmsd = &vmstate_kbd_isa;
     isa->build_aml = i8042_build_aml;
-- 
2.20.1

Re: [PATCH 48/50] pckbd: add i8042_reset() function to I8042 device

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This means that it is no longer necessary to call qemu_register_reset() manually
> within i8042_realizefn().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/pckbd.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 49/50] pckbd: switch I8042 device from update_irq() function to PS2 device gpio

[Mark Cave-Ayland]

Define a new qdev input gpio for handling incoming PS2 IRQs, and then wire up the
PS2 keyboard and mouse devices to use it. At the same time set update_irq() and
update_arg to NULL in ps2_kbd_init() and ps2_mouse_init() to ensure that any
accidental attempt to use the legacy update_irq() function will cause a NULL
pointer dereference.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/pckbd.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index b2fdd1c999..f1eeab9952 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -801,6 +801,18 @@ static const MemoryRegionOps i8042_cmd_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static void i8042_set_irq(void *opaque, int n, int level)
+{
+    ISAKBDState *s = I8042(opaque);
+    KBDState *ks = &s->kbd;
+
+    if (n) {
+        kbd_update_aux_irq(ks, level);
+    } else {
+        kbd_update_kbd_irq(ks, level);
+    }
+}
+
 static void i8042_reset(DeviceState *dev)
 {
     ISAKBDState *s = I8042(dev);
@@ -820,6 +832,9 @@ static void i8042_initfn(Object *obj)
                           "i8042-cmd", 1);
 
     qdev_init_gpio_out_named(DEVICE(obj), &s->a20_out, I8042_A20_LINE, 1);
+
+    qdev_init_gpio_out(DEVICE(obj), s->irqs, 2);
+    qdev_init_gpio_in(DEVICE(obj), i8042_set_irq, 2);
 }
 
 static void i8042_realizefn(DeviceState *dev, Error **errp)
@@ -840,14 +855,18 @@ static void i8042_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    s->irqs[I8042_KBD_IRQ] = isa_get_irq(isadev, isa_s->kbd_irq);
-    s->irqs[I8042_MOUSE_IRQ] = isa_get_irq(isadev, isa_s->mouse_irq);
+    isa_connect_gpio_out(isadev, I8042_KBD_IRQ, isa_s->kbd_irq);
+    isa_connect_gpio_out(isadev, I8042_MOUSE_IRQ, isa_s->mouse_irq);
 
     isa_register_ioport(isadev, isa_s->io + 0, 0x60);
     isa_register_ioport(isadev, isa_s->io + 1, 0x64);
 
-    s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
-    s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);
+    s->kbd = ps2_kbd_init(NULL, NULL);
+    qdev_connect_gpio_out(DEVICE(s->kbd), PS2_DEVICE_IRQ,
+                          qdev_get_gpio_in(dev, I8042_KBD_INPUT_IRQ));
+    s->mouse = ps2_mouse_init(NULL, NULL);
+    qdev_connect_gpio_out(DEVICE(s->mouse), PS2_DEVICE_IRQ,
+                          qdev_get_gpio_in(dev, I8042_MOUSE_INPUT_IRQ));
     if (isa_s->kbd_throttle && !isa_s->kbd.extended_state) {
         warn_report(TYPE_I8042 ": can't enable kbd-throttle without"
                     " extended-state, disabling kbd-throttle");
-- 
2.20.1

Re: [PATCH 49/50] pckbd: switch I8042 device from update_irq() function to PS2 device gpio

[Peter Maydell]

On Sun, 22 May 2022 at 19:20, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Define a new qdev input gpio for handling incoming PS2 IRQs, and then wire up the
> PS2 keyboard and mouse devices to use it. At the same time set update_irq() and
> update_arg to NULL in ps2_kbd_init() and ps2_mouse_init() to ensure that any
> accidental attempt to use the legacy update_irq() function will cause a NULL
> pointer dereference.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/input/pckbd.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index b2fdd1c999..f1eeab9952 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -801,6 +801,18 @@ static const MemoryRegionOps i8042_cmd_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>
> +static void i8042_set_irq(void *opaque, int n, int level)
> +{
> +    ISAKBDState *s = I8042(opaque);
> +    KBDState *ks = &s->kbd;
> +
> +    if (n) {
> +        kbd_update_aux_irq(ks, level);
> +    } else {
> +        kbd_update_kbd_irq(ks, level);
> +    }
> +}
> +
>  static void i8042_reset(DeviceState *dev)
>  {
>      ISAKBDState *s = I8042(dev);
> @@ -820,6 +832,9 @@ static void i8042_initfn(Object *obj)
>                            "i8042-cmd", 1);
>
>      qdev_init_gpio_out_named(DEVICE(obj), &s->a20_out, I8042_A20_LINE, 1);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, 2);
> +    qdev_init_gpio_in(DEVICE(obj), i8042_set_irq, 2);
>  }

named gpio inputs, otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 50/50] ps2: remove update_irq() function and update_arg parameter

[Mark Cave-Ayland]

Now that all the PS2 devices have been converted to use GPIOs the update_irq()
callback function and the update_arg parameter can be removed.

This allows these arguments to be completely removed from ps2_kbd_init() and
ps2_mouse_init(), along with the transitional logic that was added to
ps2_raise_irq() and ps2_lower_irq().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/lasips2.c     |  4 ++--
 hw/input/pckbd.c       |  8 ++++----
 hw/input/pl050.c       |  4 ++--
 hw/input/ps2.c         | 25 ++++---------------------
 include/hw/input/ps2.h |  6 ++----
 5 files changed, 14 insertions(+), 33 deletions(-)

diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
index 12ff95a05f..80ec9891be 100644
--- a/hw/input/lasips2.c
+++ b/hw/input/lasips2.c
@@ -267,10 +267,10 @@ static void lasips2_realize(DeviceState *dev, Error **errp)
 
     vmstate_register(NULL, s->base, &vmstate_lasips2, s);
 
-    s->kbd.dev = ps2_kbd_init(NULL, NULL);
+    s->kbd.dev = ps2_kbd_init();
     qdev_connect_gpio_out(DEVICE(s->kbd.dev), PS2_DEVICE_IRQ,
                           qdev_get_gpio_in(dev, LASIPS2_KBD_INPUT_IRQ));
-    s->mouse.dev = ps2_mouse_init(NULL, NULL);
+    s->mouse.dev = ps2_mouse_init();
     qdev_connect_gpio_out(DEVICE(s->mouse.dev), PS2_DEVICE_IRQ,
                           qdev_get_gpio_in(dev, LASIPS2_MOUSE_INPUT_IRQ));
 }
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index f1eeab9952..e53285a126 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -700,10 +700,10 @@ static void i8042_mmio_realize(DeviceState *dev, Error **errp)
     /* Note we can't use dc->vmsd without breaking migration compatibility */
     vmstate_register(NULL, 0, &vmstate_kbd, ks);
 
-    ks->kbd = ps2_kbd_init(NULL, NULL);
+    ks->kbd = ps2_kbd_init();
     qdev_connect_gpio_out(DEVICE(ks->kbd), PS2_DEVICE_IRQ,
                           qdev_get_gpio_in(dev, I8042_KBD_INPUT_IRQ));
-    ks->mouse = ps2_mouse_init(NULL, NULL);
+    ks->mouse = ps2_mouse_init();
     qdev_connect_gpio_out(DEVICE(ks->mouse), PS2_DEVICE_IRQ,
                           qdev_get_gpio_in(dev, I8042_MOUSE_INPUT_IRQ));
 }
@@ -861,10 +861,10 @@ static void i8042_realizefn(DeviceState *dev, Error **errp)
     isa_register_ioport(isadev, isa_s->io + 0, 0x60);
     isa_register_ioport(isadev, isa_s->io + 1, 0x64);
 
-    s->kbd = ps2_kbd_init(NULL, NULL);
+    s->kbd = ps2_kbd_init();
     qdev_connect_gpio_out(DEVICE(s->kbd), PS2_DEVICE_IRQ,
                           qdev_get_gpio_in(dev, I8042_KBD_INPUT_IRQ));
-    s->mouse = ps2_mouse_init(NULL, NULL);
+    s->mouse = ps2_mouse_init();
     qdev_connect_gpio_out(DEVICE(s->mouse), PS2_DEVICE_IRQ,
                           qdev_get_gpio_in(dev, I8042_MOUSE_INPUT_IRQ));
     if (isa_s->kbd_throttle && !isa_s->kbd.extended_state) {
diff --git a/hw/input/pl050.c b/hw/input/pl050.c
index fe6a281307..3e48fee92a 100644
--- a/hw/input/pl050.c
+++ b/hw/input/pl050.c
@@ -166,9 +166,9 @@ static void pl050_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->iomem);
     sysbus_init_irq(sbd, &s->irq);
     if (s->is_mouse) {
-        s->dev = ps2_mouse_init(NULL, NULL);
+        s->dev = ps2_mouse_init();
     } else {
-        s->dev = ps2_kbd_init(NULL, NULL);
+        s->dev = ps2_kbd_init();
     }
     qdev_connect_gpio_out(DEVICE(s->dev), PS2_DEVICE_IRQ,
                           qdev_get_gpio_in(dev, PL050_INPUT_IRQ));
diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 891eb7181c..9d109f019a 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -175,20 +175,12 @@ void ps2_queue_noirq(PS2State *s, int b)
 
 static void ps2_raise_irq(PS2State *s)
 {
-    if (s->irq) {
-        qemu_set_irq(s->irq, 1);
-    } else {
-        s->update_irq(s->update_arg, 1);
-    }
+    qemu_set_irq(s->irq, 1);
 }
 
 static void ps2_lower_irq(PS2State *s)
 {
-    if (s->irq) {
-        qemu_set_irq(s->irq, 0);
-    } else {
-        s->update_irq(s->update_arg, 0);
-    }
+    qemu_set_irq(s->irq, 0);
 }
 
 void ps2_queue(PS2State *s, int b)
@@ -1232,21 +1224,16 @@ static void ps2_kbd_realize(DeviceState *dev, Error **errp)
     qemu_input_handler_register(dev, &ps2_keyboard_handler);
 }
 
-void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
+void *ps2_kbd_init(void)
 {
     DeviceState *dev;
     PS2KbdState *s;
-    PS2State *ps2;
 
     dev = qdev_new(TYPE_PS2_KBD_DEVICE);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     s = PS2_KBD_DEVICE(dev);
-    ps2 = PS2_DEVICE(s);
 
     trace_ps2_kbd_init(s);
-    ps2->update_irq = update_irq;
-    ps2->update_arg = update_arg;
-
     return s;
 }
 
@@ -1262,20 +1249,16 @@ static void ps2_mouse_realize(DeviceState *dev, Error **errp)
     qemu_input_handler_register(dev, &ps2_mouse_handler);
 }
 
-void *ps2_mouse_init(void (*update_irq)(void *, int), void *update_arg)
+void *ps2_mouse_init(void)
 {
     DeviceState *dev;
     PS2MouseState *s;
-    PS2State *ps2;
 
     dev = qdev_new(TYPE_PS2_MOUSE_DEVICE);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     s = PS2_MOUSE_DEVICE(dev);
-    ps2 = PS2_DEVICE(s);
 
     trace_ps2_mouse_init(s);
-    ps2->update_irq = update_irq;
-    ps2->update_arg = update_arg;
     return s;
 }
 
diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h
index 6e5c404cb5..df685a54c5 100644
--- a/include/hw/input/ps2.h
+++ b/include/hw/input/ps2.h
@@ -59,8 +59,6 @@ struct PS2State {
     PS2Queue queue;
     int32_t write_cmd;
     qemu_irq irq;
-    void (*update_irq)(void *, int);
-    void *update_arg;
 };
 
 #define TYPE_PS2_DEVICE "ps2-device"
@@ -100,8 +98,8 @@ struct PS2MouseState {
 OBJECT_DECLARE_SIMPLE_TYPE(PS2MouseState, PS2_MOUSE_DEVICE)
 
 /* ps2.c */
-void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg);
-void *ps2_mouse_init(void (*update_irq)(void *, int), void *update_arg);
+void *ps2_kbd_init(void);
+void *ps2_mouse_init(void);
 void ps2_write_mouse(PS2MouseState *s, int val);
 void ps2_write_keyboard(PS2KbdState *s, int val);
 uint32_t ps2_read_data(PS2State *s);
-- 
2.20.1

Re: [PATCH 50/50] ps2: remove update_irq() function and update_arg parameter

[Peter Maydell]

On Sun, 22 May 2022 at 19:23, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Now that all the PS2 devices have been converted to use GPIOs the update_irq()
> callback function and the update_arg parameter can be removed.
>
> This allows these arguments to be completely removed from ps2_kbd_init() and
> ps2_mouse_init(), along with the transitional logic that was added to
> ps2_raise_irq() and ps2_lower_irq().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 00/50] PS2 device QOMification - part 1

[Philippe Mathieu-Daudé via]

On 22/5/22 20:17, Mark Cave-Ayland wrote:

> Mark Cave-Ayland (50):
>    ps2: checkpatch fixes
>    ps2: QOMify PS2State
>    ps2: QOMify PS2KbdState
>    ps2: QOMify PS2MouseState
>    ps2: move QOM type definitions from ps2.c to ps2.h
>    ps2: improve function prototypes in ps2.c and ps2.h
>    ps2: introduce PS2DeviceClass
>    ps2: implement ps2_reset() for the PS2_DEVICE QOM type based upon
>      ps2_common_reset()
>    ps2: remove duplicate setting of scancode_set in ps2_kbd_init()
>    ps2: implement ps2_kbd_realize() and use it to register
>      ps2_keyboard_handler
>    ps2: implement ps2_mouse_realize() and use it to register
>      ps2_mouse_handler
>    ps2: don't use vmstate_register() in ps2_kbd_init()
>    ps2: don't use vmstate_register() in ps2_mouse_init()
>    pl050: checkpatch fixes
>    pl050: split pl050_update_irq() into separate pl050_set_irq() and
>      pl050_update_irq() functions
>    lasips2: spacing fixes
>    lasips2: rename ps2dev_update_irq() to lasips2_port_set_irq()
>    pckbd: checkpatch fixes
>    pckbd: move KBDState from pckbd.c to i8042.h
>    pckbd: move ISAKBDState from pckbd.c to i8042.h

Patches 1-20:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 00/50] PS2 device QOMification - part 1

[Helge Deller]

Hi Mark,

On 5/22/22 20:17, Mark Cave-Ayland wrote:
> This series came about when looking at improving the LASI PS2 device for
> the HPPA machine: there were improvements that I was keen to make, but
> was restricted because the PS2 device(s) weren't QOMified.
>
> Trying to do everything in a single patchset would be a very large series
> indeed, so here is part 1 of the series which does the basic QOMification
> process and consists of:
>
> - QOMifying the basic PS2, PS2 keyboard and PS2 mouse types
>
> - Moving any functionality that exists in a global device init function
>   directly into the relevant device, so that all device behaviour is
>   configured using qdev properties and QOM
>
> - Introducing a new I8042_MMIO type for use by the MIPS magnum machine
>
> - Switch all PS2 devices to use qdev gpios for IRQs instead of using the
>   update_irq() callback function along with the update_arg opaque
>
> Once this work has been done, a follow-up part 2 series will finish the
> remainder of the work which involves i) improving the QOM object model
> now QOMification is complete and ii) removing the legacy global device
> init functions for PS2 and related devices.
>
> Testing for this series has comprised of booting a machine with each type
> of PS2 device and confirming that i) the machine responds to keypresses
> when using a graphical console and ii) completing a successful migration
> from a machine with this series applies back to a machine running latest
> git master. The test machines I used were:
>
> - qemu-system-x86_64 -M pc for the I8042 device
> - qemu-system-hppa for the LASIPS2 device
> - qemu-system-arm -M versatilepb for the PL050 device
> - qemu-system-mips64el -M magnum for the I8042_MMIO device
>
> Finally the QOM tree changes caused by QOMification of the PS2 devices
> trigger a failure due to a bug in the bios-tables-test qtest for subtest
> /x86_64/acpi/q35/viot. This can be fixed by applying the series at
> https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04266.html
> "hw/acpi/viot: generate stable VIOT ACPI tables" first.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Thanks a lot for this QOMifying and cleanup work!
I've sucessfully tested it with qemu-system-hppa for HP-UX 10 and 11.

You may add either or both of:

Acked-by: Helge Deller <deller@gmx.de>
Tested-by: Helge Deller <deller@gmx.de>

Thank you!
Helge


>
>
> Mark Cave-Ayland (50):
>   ps2: checkpatch fixes
>   ps2: QOMify PS2State
>   ps2: QOMify PS2KbdState
>   ps2: QOMify PS2MouseState
>   ps2: move QOM type definitions from ps2.c to ps2.h
>   ps2: improve function prototypes in ps2.c and ps2.h
>   ps2: introduce PS2DeviceClass
>   ps2: implement ps2_reset() for the PS2_DEVICE QOM type based upon
>     ps2_common_reset()
>   ps2: remove duplicate setting of scancode_set in ps2_kbd_init()
>   ps2: implement ps2_kbd_realize() and use it to register
>     ps2_keyboard_handler
>   ps2: implement ps2_mouse_realize() and use it to register
>     ps2_mouse_handler
>   ps2: don't use vmstate_register() in ps2_kbd_init()
>   ps2: don't use vmstate_register() in ps2_mouse_init()
>   pl050: checkpatch fixes
>   pl050: split pl050_update_irq() into separate pl050_set_irq() and
>     pl050_update_irq() functions
>   lasips2: spacing fixes
>   lasips2: rename ps2dev_update_irq() to lasips2_port_set_irq()
>   pckbd: checkpatch fixes
>   pckbd: move KBDState from pckbd.c to i8042.h
>   pckbd: move ISAKBDState from pckbd.c to i8042.h
>   pckbd: introduce new I8042_MMIO QOM type
>   pckbd: implement i8042_mmio_reset() for I8042_MMIO device
>   pckbd: add mask qdev property to I8042_MMIO device
>   pckbd: add size qdev property to I8042_MMIO device
>   pckbd: implement i8042_mmio_realize() function
>   pckbd: implement i8042_mmio_init() function
>   pckbd: alter i8042_mm_init() to return a I8042_MMIO device
>   pckbd: move mapping of I8042_MMIO registers to MIPS magnum machine
>   pckbd: more vmstate_register() from i8042_mm_init() to
>     i8042_mmio_realize()
>   pckbd: move ps2_kbd_init() and ps2_mouse_init() to
>     i8042_mmio_realize()
>   ps2: make ps2_raise_irq() function static
>   ps2: use ps2_raise_irq() instead of calling update_irq() directly
>   ps2: introduce ps2_lower_irq() instead of calling update_irq()
>     directly
>   ps2: add gpio for output IRQ and optionally use it in ps2_raise_irq()
>     and ps2_lower_irq()
>   pckbd: replace irq_kbd and irq_mouse with qemu_irq array in KBDState
>   pl050: switch over from update_irq() function to PS2 device gpio
>   lasips2: QOMify LASIPS2State
>   lasips2: move lasips2 QOM types from lasips2.c to lasips2.h
>   lasips2: rename lasips2_init() to lasips2_initfn() and update it to
>     return the LASIPS2 device
>   lasips2: implement lasips2_init() function
>   lasips2: move mapping of LASIPS2 registers to HPPA machine
>   lasips2: move initialisation of PS2 ports from lasi_initfn() to
>     lasi_init()
>   lasips2: add base property
>   lasips2: implement lasips2_realize()
>   lasips2: use qdev gpio for output IRQ
>   lasips2: switch over from update_irq() function to PS2 device gpio
>   pckbd: switch I8042_MMIO device from update_irq() function to PS2
>     device gpio
>   pckbd: add i8042_reset() function to I8042 device
>   pckbd: switch I8042 device from update_irq() function to PS2 device
>     gpio
>   ps2: remove update_irq() function and update_arg parameter
>
>  hw/hppa/machine.c          |  11 +-
>  hw/input/lasips2.c         | 110 +++++---
>  hw/input/pckbd.c           | 321 ++++++++++++++++--------
>  hw/input/pl050.c           |  50 ++--
>  hw/input/ps2.c             | 500 +++++++++++++++++++++----------------
>  hw/mips/jazz.c             |  11 +-
>  include/hw/input/i8042.h   |  54 +++-
>  include/hw/input/lasips2.h |  27 +-
>  include/hw/input/ps2.h     |  79 +++++-
>  9 files changed, 775 insertions(+), 388 deletions(-)
>

Re: [PATCH 00/50] PS2 device QOMification - part 1

[Mark Cave-Ayland]

On 24/05/2022 19:25, Helge Deller wrote:

> Hi Mark,
> 
> On 5/22/22 20:17, Mark Cave-Ayland wrote:
>> This series came about when looking at improving the LASI PS2 device for
>> the HPPA machine: there were improvements that I was keen to make, but
>> was restricted because the PS2 device(s) weren't QOMified.
>>
>> Trying to do everything in a single patchset would be a very large series
>> indeed, so here is part 1 of the series which does the basic QOMification
>> process and consists of:
>>
>> - QOMifying the basic PS2, PS2 keyboard and PS2 mouse types
>>
>> - Moving any functionality that exists in a global device init function
>>    directly into the relevant device, so that all device behaviour is
>>    configured using qdev properties and QOM
>>
>> - Introducing a new I8042_MMIO type for use by the MIPS magnum machine
>>
>> - Switch all PS2 devices to use qdev gpios for IRQs instead of using the
>>    update_irq() callback function along with the update_arg opaque
>>
>> Once this work has been done, a follow-up part 2 series will finish the
>> remainder of the work which involves i) improving the QOM object model
>> now QOMification is complete and ii) removing the legacy global device
>> init functions for PS2 and related devices.
>>
>> Testing for this series has comprised of booting a machine with each type
>> of PS2 device and confirming that i) the machine responds to keypresses
>> when using a graphical console and ii) completing a successful migration
>> from a machine with this series applies back to a machine running latest
>> git master. The test machines I used were:
>>
>> - qemu-system-x86_64 -M pc for the I8042 device
>> - qemu-system-hppa for the LASIPS2 device
>> - qemu-system-arm -M versatilepb for the PL050 device
>> - qemu-system-mips64el -M magnum for the I8042_MMIO device
>>
>> Finally the QOM tree changes caused by QOMification of the PS2 devices
>> trigger a failure due to a bug in the bios-tables-test qtest for subtest
>> /x86_64/acpi/q35/viot. This can be fixed by applying the series at
>> https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04266.html
>> "hw/acpi/viot: generate stable VIOT ACPI tables" first.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Thanks a lot for this QOMifying and cleanup work!
> I've sucessfully tested it with qemu-system-hppa for HP-UX 10 and 11.

Thanks for testing! As mentioned in the cover letter, in part 1 it's just a very 
basic conversion for now to keep the size of the series down - part 2 is where the 
magic happens :)

> You may add either or both of:
> 
> Acked-by: Helge Deller <deller@gmx.de>
> Tested-by: Helge Deller <deller@gmx.de>
> 
> Thank you!
> Helge
> 
> 
>>
>>
>> Mark Cave-Ayland (50):
>>    ps2: checkpatch fixes
>>    ps2: QOMify PS2State
>>    ps2: QOMify PS2KbdState
>>    ps2: QOMify PS2MouseState
>>    ps2: move QOM type definitions from ps2.c to ps2.h
>>    ps2: improve function prototypes in ps2.c and ps2.h
>>    ps2: introduce PS2DeviceClass
>>    ps2: implement ps2_reset() for the PS2_DEVICE QOM type based upon
>>      ps2_common_reset()
>>    ps2: remove duplicate setting of scancode_set in ps2_kbd_init()
>>    ps2: implement ps2_kbd_realize() and use it to register
>>      ps2_keyboard_handler
>>    ps2: implement ps2_mouse_realize() and use it to register
>>      ps2_mouse_handler
>>    ps2: don't use vmstate_register() in ps2_kbd_init()
>>    ps2: don't use vmstate_register() in ps2_mouse_init()
>>    pl050: checkpatch fixes
>>    pl050: split pl050_update_irq() into separate pl050_set_irq() and
>>      pl050_update_irq() functions
>>    lasips2: spacing fixes
>>    lasips2: rename ps2dev_update_irq() to lasips2_port_set_irq()
>>    pckbd: checkpatch fixes
>>    pckbd: move KBDState from pckbd.c to i8042.h
>>    pckbd: move ISAKBDState from pckbd.c to i8042.h
>>    pckbd: introduce new I8042_MMIO QOM type
>>    pckbd: implement i8042_mmio_reset() for I8042_MMIO device
>>    pckbd: add mask qdev property to I8042_MMIO device
>>    pckbd: add size qdev property to I8042_MMIO device
>>    pckbd: implement i8042_mmio_realize() function
>>    pckbd: implement i8042_mmio_init() function
>>    pckbd: alter i8042_mm_init() to return a I8042_MMIO device
>>    pckbd: move mapping of I8042_MMIO registers to MIPS magnum machine
>>    pckbd: more vmstate_register() from i8042_mm_init() to
>>      i8042_mmio_realize()
>>    pckbd: move ps2_kbd_init() and ps2_mouse_init() to
>>      i8042_mmio_realize()
>>    ps2: make ps2_raise_irq() function static
>>    ps2: use ps2_raise_irq() instead of calling update_irq() directly
>>    ps2: introduce ps2_lower_irq() instead of calling update_irq()
>>      directly
>>    ps2: add gpio for output IRQ and optionally use it in ps2_raise_irq()
>>      and ps2_lower_irq()
>>    pckbd: replace irq_kbd and irq_mouse with qemu_irq array in KBDState
>>    pl050: switch over from update_irq() function to PS2 device gpio
>>    lasips2: QOMify LASIPS2State
>>    lasips2: move lasips2 QOM types from lasips2.c to lasips2.h
>>    lasips2: rename lasips2_init() to lasips2_initfn() and update it to
>>      return the LASIPS2 device
>>    lasips2: implement lasips2_init() function
>>    lasips2: move mapping of LASIPS2 registers to HPPA machine
>>    lasips2: move initialisation of PS2 ports from lasi_initfn() to
>>      lasi_init()
>>    lasips2: add base property
>>    lasips2: implement lasips2_realize()
>>    lasips2: use qdev gpio for output IRQ
>>    lasips2: switch over from update_irq() function to PS2 device gpio
>>    pckbd: switch I8042_MMIO device from update_irq() function to PS2
>>      device gpio
>>    pckbd: add i8042_reset() function to I8042 device
>>    pckbd: switch I8042 device from update_irq() function to PS2 device
>>      gpio
>>    ps2: remove update_irq() function and update_arg parameter
>>
>>   hw/hppa/machine.c          |  11 +-
>>   hw/input/lasips2.c         | 110 +++++---
>>   hw/input/pckbd.c           | 321 ++++++++++++++++--------
>>   hw/input/pl050.c           |  50 ++--
>>   hw/input/ps2.c             | 500 +++++++++++++++++++++----------------
>>   hw/mips/jazz.c             |  11 +-
>>   include/hw/input/i8042.h   |  54 +++-
>>   include/hw/input/lasips2.h |  27 +-
>>   include/hw/input/ps2.h     |  79 +++++-
>>   9 files changed, 775 insertions(+), 388 deletions(-)


ATB,

Mark.

Re: [PATCH 00/50] PS2 device QOMification - part 1

[Mark Cave-Ayland]

On 22/05/2022 19:17, Mark Cave-Ayland wrote:

> This series came about when looking at improving the LASI PS2 device for
> the HPPA machine: there were improvements that I was keen to make, but
> was restricted because the PS2 device(s) weren't QOMified.
> 
> Trying to do everything in a single patchset would be a very large series
> indeed, so here is part 1 of the series which does the basic QOMification
> process and consists of:
> 
> - QOMifying the basic PS2, PS2 keyboard and PS2 mouse types
> 
> - Moving any functionality that exists in a global device init function
>    directly into the relevant device, so that all device behaviour is
>    configured using qdev properties and QOM
> 
> - Introducing a new I8042_MMIO type for use by the MIPS magnum machine
> 
> - Switch all PS2 devices to use qdev gpios for IRQs instead of using the
>    update_irq() callback function along with the update_arg opaque
> 
> Once this work has been done, a follow-up part 2 series will finish the
> remainder of the work which involves i) improving the QOM object model
> now QOMification is complete and ii) removing the legacy global device
> init functions for PS2 and related devices.
> 
> Testing for this series has comprised of booting a machine with each type
> of PS2 device and confirming that i) the machine responds to keypresses
> when using a graphical console and ii) completing a successful migration
> from a machine with this series applies back to a machine running latest
> git master. The test machines I used were:
> 
> - qemu-system-x86_64 -M pc for the I8042 device
> - qemu-system-hppa for the LASIPS2 device
> - qemu-system-arm -M versatilepb for the PL050 device
> - qemu-system-mips64el -M magnum for the I8042_MMIO device
> 
> Finally the QOM tree changes caused by QOMification of the PS2 devices
> trigger a failure due to a bug in the bios-tables-test qtest for subtest
> /x86_64/acpi/q35/viot. This can be fixed by applying the series at
> https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04266.html
> "hw/acpi/viot: generate stable VIOT ACPI tables" first.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (50):
>    ps2: checkpatch fixes
>    ps2: QOMify PS2State
>    ps2: QOMify PS2KbdState
>    ps2: QOMify PS2MouseState
>    ps2: move QOM type definitions from ps2.c to ps2.h
>    ps2: improve function prototypes in ps2.c and ps2.h
>    ps2: introduce PS2DeviceClass
>    ps2: implement ps2_reset() for the PS2_DEVICE QOM type based upon
>      ps2_common_reset()
>    ps2: remove duplicate setting of scancode_set in ps2_kbd_init()
>    ps2: implement ps2_kbd_realize() and use it to register
>      ps2_keyboard_handler
>    ps2: implement ps2_mouse_realize() and use it to register
>      ps2_mouse_handler
>    ps2: don't use vmstate_register() in ps2_kbd_init()
>    ps2: don't use vmstate_register() in ps2_mouse_init()
>    pl050: checkpatch fixes
>    pl050: split pl050_update_irq() into separate pl050_set_irq() and
>      pl050_update_irq() functions
>    lasips2: spacing fixes
>    lasips2: rename ps2dev_update_irq() to lasips2_port_set_irq()
>    pckbd: checkpatch fixes
>    pckbd: move KBDState from pckbd.c to i8042.h
>    pckbd: move ISAKBDState from pckbd.c to i8042.h
>    pckbd: introduce new I8042_MMIO QOM type
>    pckbd: implement i8042_mmio_reset() for I8042_MMIO device
>    pckbd: add mask qdev property to I8042_MMIO device
>    pckbd: add size qdev property to I8042_MMIO device
>    pckbd: implement i8042_mmio_realize() function
>    pckbd: implement i8042_mmio_init() function
>    pckbd: alter i8042_mm_init() to return a I8042_MMIO device
>    pckbd: move mapping of I8042_MMIO registers to MIPS magnum machine
>    pckbd: more vmstate_register() from i8042_mm_init() to
>      i8042_mmio_realize()
>    pckbd: move ps2_kbd_init() and ps2_mouse_init() to
>      i8042_mmio_realize()
>    ps2: make ps2_raise_irq() function static
>    ps2: use ps2_raise_irq() instead of calling update_irq() directly
>    ps2: introduce ps2_lower_irq() instead of calling update_irq()
>      directly
>    ps2: add gpio for output IRQ and optionally use it in ps2_raise_irq()
>      and ps2_lower_irq()
>    pckbd: replace irq_kbd and irq_mouse with qemu_irq array in KBDState
>    pl050: switch over from update_irq() function to PS2 device gpio
>    lasips2: QOMify LASIPS2State
>    lasips2: move lasips2 QOM types from lasips2.c to lasips2.h
>    lasips2: rename lasips2_init() to lasips2_initfn() and update it to
>      return the LASIPS2 device
>    lasips2: implement lasips2_init() function
>    lasips2: move mapping of LASIPS2 registers to HPPA machine
>    lasips2: move initialisation of PS2 ports from lasi_initfn() to
>      lasi_init()
>    lasips2: add base property
>    lasips2: implement lasips2_realize()
>    lasips2: use qdev gpio for output IRQ
>    lasips2: switch over from update_irq() function to PS2 device gpio
>    pckbd: switch I8042_MMIO device from update_irq() function to PS2
>      device gpio
>    pckbd: add i8042_reset() function to I8042 device
>    pckbd: switch I8042 device from update_irq() function to PS2 device
>      gpio
>    ps2: remove update_irq() function and update_arg parameter
> 
>   hw/hppa/machine.c          |  11 +-
>   hw/input/lasips2.c         | 110 +++++---
>   hw/input/pckbd.c           | 321 ++++++++++++++++--------
>   hw/input/pl050.c           |  50 ++--
>   hw/input/ps2.c             | 500 +++++++++++++++++++++----------------
>   hw/mips/jazz.c             |  11 +-
>   include/hw/input/i8042.h   |  54 +++-
>   include/hw/input/lasips2.h |  27 +-
>   include/hw/input/ps2.h     |  79 +++++-
>   9 files changed, 775 insertions(+), 388 deletions(-)

Ping? I've had an R-B from Phil for patches 1-20 along with a Tested-by from Helge 
for the entire series, but that still leaves the last 30 patches unreviewed...


ATB,

Mark.

Re: [PATCH 00/50] PS2 device QOMification - part 1

[Philippe Mathieu-Daudé via]

On 7/6/22 17:54, Mark Cave-Ayland wrote:
> On 22/05/2022 19:17, Mark Cave-Ayland wrote:
> 
>> This series came about when looking at improving the LASI PS2 device for
>> the HPPA machine: there were improvements that I was keen to make, but
>> was restricted because the PS2 device(s) weren't QOMified.
>>
>> Trying to do everything in a single patchset would be a very large series
>> indeed, so here is part 1 of the series which does the basic QOMification
>> process and consists of:
>>
>> - QOMifying the basic PS2, PS2 keyboard and PS2 mouse types
>>
>> - Moving any functionality that exists in a global device init function
>>    directly into the relevant device, so that all device behaviour is
>>    configured using qdev properties and QOM
>>
>> - Introducing a new I8042_MMIO type for use by the MIPS magnum machine
>>
>> - Switch all PS2 devices to use qdev gpios for IRQs instead of using the
>>    update_irq() callback function along with the update_arg opaque
>>
>> Once this work has been done, a follow-up part 2 series will finish the
>> remainder of the work which involves i) improving the QOM object model
>> now QOMification is complete and ii) removing the legacy global device
>> init functions for PS2 and related devices.
>>
>> Testing for this series has comprised of booting a machine with each type
>> of PS2 device and confirming that i) the machine responds to keypresses
>> when using a graphical console and ii) completing a successful migration
>> from a machine with this series applies back to a machine running latest
>> git master. The test machines I used were:
>>
>> - qemu-system-x86_64 -M pc for the I8042 device
>> - qemu-system-hppa for the LASIPS2 device
>> - qemu-system-arm -M versatilepb for the PL050 device
>> - qemu-system-mips64el -M magnum for the I8042_MMIO device
>>
>> Finally the QOM tree changes caused by QOMification of the PS2 devices
>> trigger a failure due to a bug in the bios-tables-test qtest for subtest
>> /x86_64/acpi/q35/viot. This can be fixed by applying the series at
>> https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04266.html
>> "hw/acpi/viot: generate stable VIOT ACPI tables" first.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>>
>> Mark Cave-Ayland (50):
>>    ps2: checkpatch fixes
>>    ps2: QOMify PS2State
>>    ps2: QOMify PS2KbdState
>>    ps2: QOMify PS2MouseState
>>    ps2: move QOM type definitions from ps2.c to ps2.h
>>    ps2: improve function prototypes in ps2.c and ps2.h
>>    ps2: introduce PS2DeviceClass
>>    ps2: implement ps2_reset() for the PS2_DEVICE QOM type based upon
>>      ps2_common_reset()
>>    ps2: remove duplicate setting of scancode_set in ps2_kbd_init()
>>    ps2: implement ps2_kbd_realize() and use it to register
>>      ps2_keyboard_handler
>>    ps2: implement ps2_mouse_realize() and use it to register
>>      ps2_mouse_handler
>>    ps2: don't use vmstate_register() in ps2_kbd_init()
>>    ps2: don't use vmstate_register() in ps2_mouse_init()
>>    pl050: checkpatch fixes
>>    pl050: split pl050_update_irq() into separate pl050_set_irq() and
>>      pl050_update_irq() functions
>>    lasips2: spacing fixes
>>    lasips2: rename ps2dev_update_irq() to lasips2_port_set_irq()
>>    pckbd: checkpatch fixes
>>    pckbd: move KBDState from pckbd.c to i8042.h
>>    pckbd: move ISAKBDState from pckbd.c to i8042.h

> Ping? I've had an R-B from Phil for patches 1-20 along with a Tested-by 
> from Helge for the entire series, but that still leaves the last 30 
> patches unreviewed...

I'm queuing patches 1-20 (fixing the typo in patch 9) via mips-next,
and will review v2.

Re: [PATCH 00/50] PS2 device QOMification - part 1

[Mark Cave-Ayland]

On 10/06/2022 16:35, Philippe Mathieu-Daudé wrote:

> On 7/6/22 17:54, Mark Cave-Ayland wrote:
>> On 22/05/2022 19:17, Mark Cave-Ayland wrote:
>>
>>> This series came about when looking at improving the LASI PS2 device for
>>> the HPPA machine: there were improvements that I was keen to make, but
>>> was restricted because the PS2 device(s) weren't QOMified.
>>>
>>> Trying to do everything in a single patchset would be a very large series
>>> indeed, so here is part 1 of the series which does the basic QOMification
>>> process and consists of:
>>>
>>> - QOMifying the basic PS2, PS2 keyboard and PS2 mouse types
>>>
>>> - Moving any functionality that exists in a global device init function
>>>    directly into the relevant device, so that all device behaviour is
>>>    configured using qdev properties and QOM
>>>
>>> - Introducing a new I8042_MMIO type for use by the MIPS magnum machine
>>>
>>> - Switch all PS2 devices to use qdev gpios for IRQs instead of using the
>>>    update_irq() callback function along with the update_arg opaque
>>>
>>> Once this work has been done, a follow-up part 2 series will finish the
>>> remainder of the work which involves i) improving the QOM object model
>>> now QOMification is complete and ii) removing the legacy global device
>>> init functions for PS2 and related devices.
>>>
>>> Testing for this series has comprised of booting a machine with each type
>>> of PS2 device and confirming that i) the machine responds to keypresses
>>> when using a graphical console and ii) completing a successful migration
>>> from a machine with this series applies back to a machine running latest
>>> git master. The test machines I used were:
>>>
>>> - qemu-system-x86_64 -M pc for the I8042 device
>>> - qemu-system-hppa for the LASIPS2 device
>>> - qemu-system-arm -M versatilepb for the PL050 device
>>> - qemu-system-mips64el -M magnum for the I8042_MMIO device
>>>
>>> Finally the QOM tree changes caused by QOMification of the PS2 devices
>>> trigger a failure due to a bug in the bios-tables-test qtest for subtest
>>> /x86_64/acpi/q35/viot. This can be fixed by applying the series at
>>> https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04266.html
>>> "hw/acpi/viot: generate stable VIOT ACPI tables" first.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>>
>>> Mark Cave-Ayland (50):
>>>    ps2: checkpatch fixes
>>>    ps2: QOMify PS2State
>>>    ps2: QOMify PS2KbdState
>>>    ps2: QOMify PS2MouseState
>>>    ps2: move QOM type definitions from ps2.c to ps2.h
>>>    ps2: improve function prototypes in ps2.c and ps2.h
>>>    ps2: introduce PS2DeviceClass
>>>    ps2: implement ps2_reset() for the PS2_DEVICE QOM type based upon
>>>      ps2_common_reset()
>>>    ps2: remove duplicate setting of scancode_set in ps2_kbd_init()
>>>    ps2: implement ps2_kbd_realize() and use it to register
>>>      ps2_keyboard_handler
>>>    ps2: implement ps2_mouse_realize() and use it to register
>>>      ps2_mouse_handler
>>>    ps2: don't use vmstate_register() in ps2_kbd_init()
>>>    ps2: don't use vmstate_register() in ps2_mouse_init()
>>>    pl050: checkpatch fixes
>>>    pl050: split pl050_update_irq() into separate pl050_set_irq() and
>>>      pl050_update_irq() functions
>>>    lasips2: spacing fixes
>>>    lasips2: rename ps2dev_update_irq() to lasips2_port_set_irq()
>>>    pckbd: checkpatch fixes
>>>    pckbd: move KBDState from pckbd.c to i8042.h
>>>    pckbd: move ISAKBDState from pckbd.c to i8042.h
> 
>> Ping? I've had an R-B from Phil for patches 1-20 along with a Tested-by from Helge 
>> for the entire series, but that still leaves the last 30 patches unreviewed...
> 
> I'm queuing patches 1-20 (fixing the typo in patch 9) via mips-next,
> and will review v2.

Hi Phil,

I've also got a local fix that needs to be squashed into one of these early patches, 
so could you drop this series for now? As well as this extra fix (thanks Asan!) it 
will make it easier to rebase and update the series according to Peter's review comments.


ATB,

Mark.

Re: [PATCH 00/50] PS2 device QOMification - part 1

[Philippe Mathieu-Daudé via]

On Fri, Jun 10, 2022 at 5:43 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 10/06/2022 16:35, Philippe Mathieu-Daudé wrote:
>
> > On 7/6/22 17:54, Mark Cave-Ayland wrote:
> >> On 22/05/2022 19:17, Mark Cave-Ayland wrote:
> >>
> >>> This series came about when looking at improving the LASI PS2 device for
> >>> the HPPA machine: there were improvements that I was keen to make, but
> >>> was restricted because the PS2 device(s) weren't QOMified.
> >>>
> >>> Trying to do everything in a single patchset would be a very large series
> >>> indeed, so here is part 1 of the series which does the basic QOMification
> >>> process and consists of:
> >>>
> >>> - QOMifying the basic PS2, PS2 keyboard and PS2 mouse types
> >>>
> >>> - Moving any functionality that exists in a global device init function
> >>>    directly into the relevant device, so that all device behaviour is
> >>>    configured using qdev properties and QOM
> >>>
> >>> - Introducing a new I8042_MMIO type for use by the MIPS magnum machine
> >>>
> >>> - Switch all PS2 devices to use qdev gpios for IRQs instead of using the
> >>>    update_irq() callback function along with the update_arg opaque
> >>>
> >>> Once this work has been done, a follow-up part 2 series will finish the
> >>> remainder of the work which involves i) improving the QOM object model
> >>> now QOMification is complete and ii) removing the legacy global device
> >>> init functions for PS2 and related devices.
> >>>
> >>> Testing for this series has comprised of booting a machine with each type
> >>> of PS2 device and confirming that i) the machine responds to keypresses
> >>> when using a graphical console and ii) completing a successful migration
> >>> from a machine with this series applies back to a machine running latest
> >>> git master. The test machines I used were:
> >>>
> >>> - qemu-system-x86_64 -M pc for the I8042 device
> >>> - qemu-system-hppa for the LASIPS2 device
> >>> - qemu-system-arm -M versatilepb for the PL050 device
> >>> - qemu-system-mips64el -M magnum for the I8042_MMIO device
> >>>
> >>> Finally the QOM tree changes caused by QOMification of the PS2 devices
> >>> trigger a failure due to a bug in the bios-tables-test qtest for subtest
> >>> /x86_64/acpi/q35/viot. This can be fixed by applying the series at
> >>> https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04266.html
> >>> "hw/acpi/viot: generate stable VIOT ACPI tables" first.
> >>>
> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>>
> >>>
> >>> Mark Cave-Ayland (50):
> >>>    ps2: checkpatch fixes
> >>>    ps2: QOMify PS2State
> >>>    ps2: QOMify PS2KbdState
> >>>    ps2: QOMify PS2MouseState
> >>>    ps2: move QOM type definitions from ps2.c to ps2.h
> >>>    ps2: improve function prototypes in ps2.c and ps2.h
> >>>    ps2: introduce PS2DeviceClass
> >>>    ps2: implement ps2_reset() for the PS2_DEVICE QOM type based upon
> >>>      ps2_common_reset()
> >>>    ps2: remove duplicate setting of scancode_set in ps2_kbd_init()
> >>>    ps2: implement ps2_kbd_realize() and use it to register
> >>>      ps2_keyboard_handler
> >>>    ps2: implement ps2_mouse_realize() and use it to register
> >>>      ps2_mouse_handler
> >>>    ps2: don't use vmstate_register() in ps2_kbd_init()
> >>>    ps2: don't use vmstate_register() in ps2_mouse_init()
> >>>    pl050: checkpatch fixes
> >>>    pl050: split pl050_update_irq() into separate pl050_set_irq() and
> >>>      pl050_update_irq() functions
> >>>    lasips2: spacing fixes
> >>>    lasips2: rename ps2dev_update_irq() to lasips2_port_set_irq()
> >>>    pckbd: checkpatch fixes
> >>>    pckbd: move KBDState from pckbd.c to i8042.h
> >>>    pckbd: move ISAKBDState from pckbd.c to i8042.h
> >
> >> Ping? I've had an R-B from Phil for patches 1-20 along with a Tested-by from Helge
> >> for the entire series, but that still leaves the last 30 patches unreviewed...
> >
> > I'm queuing patches 1-20 (fixing the typo in patch 9) via mips-next,
> > and will review v2.
>
> Hi Phil,
>
> I've also got a local fix that needs to be squashed into one of these early patches,
> so could you drop this series for now? As well as this extra fix (thanks Asan!) it
> will make it easier to rebase and update the series according to Peter's review comments.

Sure, patches removed from mips-next queue.

> ATB,
>
> Mark.