[PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging

[PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging

[Mark Cave-Ayland]

Here is another patchset taken from my series to enable MacOS to boot on the q800
machine.

Patches 1-3 define the IFR bit flags in terms of the physical control lines and
update mac_via to use them.

Patch 4 does the main switch from custom methods in MOS6522DeviceClass to using
standard gpios whilst patch 5 removes these now-obsolete methods.

Patch 6 updates mos6522 instances to use the recommended method of calling
device_class_set_parent_reset() to propagate the device reset to the parent.

Patches 7 and 8 add more support for debugging guests using the mos6522 devices
by adding register names into the trace-event output and implementing a new
"info via" HMP command to give detailed information about the registers and timer
states.

Patches 9 and 10 implement edge-triggering for the CA1/2 and CB1/2 control lines as
documented in the datasheet, including updating the relevant inputs for negative
edge-triggering if required. Note that the introduction of last_irq_levels to enable
detection of the edge transition causes a migration break for the q800 and
g3beige/mac99 machines.

Finally patch 11 removes some old code in the PMU mos6522 instance which is now no
longer required with these latest changes.

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


Mark Cave-Ayland (11):
  mos6522: add defines for IFR bit flags
  mac_via: use IFR bit flag constants for VIA1 IRQs
  mac_via: use IFR bit flag constants for VIA2 IRQs
  mos6522: switch over to use qdev gpios for IRQs
  mos6522: remove update_irq() and set_sr_int() methods from
    MOS6522DeviceClass
  mos6522: use device_class_set_parent_reset() to propagate reset to
    parent
  mos6522: add register names to register read/write trace events
  mos6522: add "info via" HMP command for debugging
  mos6522: record last_irq_levels in mos6522_set_irq()
  mos6522: implement edge-triggering for CA1/2 and CB1/2 control line
    IRQs
  macio/pmu.c: remove redundant code

 hmp-commands-info.hx        |  12 ++
 hw/m68k/q800.c              |   9 +-
 hw/misc/mac_via.c           |  68 ++++--------
 hw/misc/macio/cuda.c        |   8 +-
 hw/misc/macio/pmu.c         |  40 +------
 hw/misc/mos6522.c           | 212 +++++++++++++++++++++++++++++++++---
 hw/misc/trace-events        |   4 +-
 include/hw/misc/mac_via.h   |  25 ++---
 include/hw/misc/macio/pmu.h |   2 -
 include/hw/misc/mos6522.h   |  42 +++++--
 10 files changed, 288 insertions(+), 134 deletions(-)

-- 
2.20.1

[PATCH 01/11] mos6522: add defines for IFR bit flags

[Mark Cave-Ayland]

These are intended to make it easier to see how the physical control lines
are wired for each instance.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 include/hw/misc/mos6522.h | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index fc95d22b0f..12abd8b8d2 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -41,13 +41,21 @@
 #define IER_SET            0x80    /* set bits in IER */
 #define IER_CLR            0       /* clear bits in IER */
 
-#define CA2_INT            0x01
-#define CA1_INT            0x02
-#define SR_INT             0x04    /* Shift register full/empty */
-#define CB2_INT            0x08
-#define CB1_INT            0x10
-#define T2_INT             0x20    /* Timer 2 interrupt */
-#define T1_INT             0x40    /* Timer 1 interrupt */
+#define CA2_INT_BIT        0
+#define CA1_INT_BIT        1
+#define SR_INT_BIT         2       /* Shift register full/empty */
+#define CB2_INT_BIT        3
+#define CB1_INT_BIT        4
+#define T2_INT_BIT         5       /* Timer 2 interrupt */
+#define T1_INT_BIT         6       /* Timer 1 interrupt */
+
+#define CA2_INT            (1 << CA2_INT_BIT)
+#define CA1_INT            (1 << CA1_INT_BIT)
+#define SR_INT             (1 << SR_INT_BIT)
+#define CB2_INT            (1 << CB2_INT_BIT)
+#define CB1_INT            (1 << CB1_INT_BIT)
+#define T2_INT             (1 << T2_INT_BIT)
+#define T1_INT             (1 << T1_INT_BIT)
 
 /* Bits in ACR */
 #define T1MODE             0xc0    /* Timer 1 mode */
-- 
2.20.1

[PATCH 02/11] mac_via: use IFR bit flag constants for VIA1 IRQs

[Mark Cave-Ayland]

This allows us to easily see how the physical control lines are mapped to the
IFR bit flags.

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

diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
index b445565866..b0c3825c9b 100644
--- a/include/hw/misc/mac_via.h
+++ b/include/hw/misc/mac_via.h
@@ -18,11 +18,11 @@
 #define VIA_SIZE   0x2000
 
 /* VIA 1 */
-#define VIA1_IRQ_ONE_SECOND_BIT 0
-#define VIA1_IRQ_60HZ_BIT       1
-#define VIA1_IRQ_ADB_READY_BIT  2
-#define VIA1_IRQ_ADB_DATA_BIT   3
-#define VIA1_IRQ_ADB_CLOCK_BIT  4
+#define VIA1_IRQ_ONE_SECOND_BIT CA2_INT_BIT
+#define VIA1_IRQ_60HZ_BIT       CA1_INT_BIT
+#define VIA1_IRQ_ADB_READY_BIT  SR_INT_BIT
+#define VIA1_IRQ_ADB_DATA_BIT   CB2_INT_BIT
+#define VIA1_IRQ_ADB_CLOCK_BIT  CB1_INT_BIT
 
 #define VIA1_IRQ_NB             8
 
-- 
2.20.1

[PATCH 03/11] mac_via: use IFR bit flag constants for VIA2 IRQs

[Mark Cave-Ayland]

This allows us to easily see how the physical control lines are mapped to the
IFR bit flags.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 include/hw/misc/mac_via.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
index b0c3825c9b..2df1ab01b6 100644
--- a/include/hw/misc/mac_via.h
+++ b/include/hw/misc/mac_via.h
@@ -80,11 +80,10 @@ struct MOS6522Q800VIA1State {
 
 
 /* VIA 2 */
-#define VIA2_IRQ_SCSI_DATA_BIT  0
-#define VIA2_IRQ_NUBUS_BIT      1
-#define VIA2_IRQ_UNUSED_BIT     2
-#define VIA2_IRQ_SCSI_BIT       3
-#define VIA2_IRQ_ASC_BIT        4
+#define VIA2_IRQ_SCSI_DATA_BIT  CA2_INT_BIT
+#define VIA2_IRQ_NUBUS_BIT      CA1_INT_BIT
+#define VIA2_IRQ_SCSI_BIT       CB2_INT_BIT
+#define VIA2_IRQ_ASC_BIT        CB1_INT_BIT
 
 #define VIA2_IRQ_NB             8
 
-- 
2.20.1

[PATCH 04/11] mos6522: switch over to use qdev gpios for IRQs

[Mark Cave-Ayland]

For historical reasons each mos6522 instance implements its own setting and
update of the IFR flag bits using methods exposed by MOS6522DeviceClass. As
of today this is no longer required, and it is now possible to implement
the mos6522 IRQs as standard qdev gpios.

Switch over to use qdev gpios for the mos6522 device and update all instances
accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/mac_via.c         | 56 +++++++--------------------------------
 hw/misc/macio/cuda.c      |  5 ++--
 hw/misc/macio/pmu.c       |  4 +--
 hw/misc/mos6522.c         | 15 +++++++++++
 include/hw/misc/mac_via.h |  6 +----
 include/hw/misc/mos6522.h |  2 ++
 6 files changed, 32 insertions(+), 56 deletions(-)

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index b378e6b305..0756374f1b 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -325,10 +325,9 @@ static void via1_sixty_hz(void *opaque)
 {
     MOS6522Q800VIA1State *v1s = opaque;
     MOS6522State *s = MOS6522(v1s);
-    MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
+    qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA1_IRQ_60HZ_BIT);
 
-    s->ifr |= VIA1_IRQ_60HZ;
-    mdc->update_irq(s);
+    qemu_set_irq(irq, 1);
 
     via1_sixty_hz_update(v1s);
 }
@@ -337,44 +336,13 @@ static void via1_one_second(void *opaque)
 {
     MOS6522Q800VIA1State *v1s = opaque;
     MOS6522State *s = MOS6522(v1s);
-    MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
+    qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA1_IRQ_ONE_SECOND_BIT);
 
-    s->ifr |= VIA1_IRQ_ONE_SECOND;
-    mdc->update_irq(s);
+    qemu_set_irq(irq, 1);
 
     via1_one_second_update(v1s);
 }
 
-static void via1_irq_request(void *opaque, int irq, int level)
-{
-    MOS6522Q800VIA1State *v1s = opaque;
-    MOS6522State *s = MOS6522(v1s);
-    MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
-
-    if (level) {
-        s->ifr |= 1 << irq;
-    } else {
-        s->ifr &= ~(1 << irq);
-    }
-
-    mdc->update_irq(s);
-}
-
-static void via2_irq_request(void *opaque, int irq, int level)
-{
-    MOS6522Q800VIA2State *v2s = opaque;
-    MOS6522State *s = MOS6522(v2s);
-    MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
-
-    if (level) {
-        s->ifr |= 1 << irq;
-    } else {
-        s->ifr &= ~(1 << irq);
-    }
-
-    mdc->update_irq(s);
-}
-
 
 static void pram_update(MOS6522Q800VIA1State *v1s)
 {
@@ -1061,8 +1029,6 @@ static void mos6522_q800_via1_init(Object *obj)
     qbus_init((BusState *)&v1s->adb_bus, sizeof(v1s->adb_bus),
               TYPE_ADB_BUS, DEVICE(v1s), "adb.0");
 
-    qdev_init_gpio_in(DEVICE(obj), via1_irq_request, VIA1_IRQ_NB);
-
     /* A/UX mode */
     qdev_init_gpio_out(DEVICE(obj), &v1s->auxmode_irq, 1);
 }
@@ -1150,22 +1116,20 @@ static void mos6522_q800_via2_reset(DeviceState *dev)
     ms->a = 0x7f;
 }
 
-static void via2_nubus_irq_request(void *opaque, int irq, int level)
+static void via2_nubus_irq_request(void *opaque, int n, int level)
 {
     MOS6522Q800VIA2State *v2s = opaque;
     MOS6522State *s = MOS6522(v2s);
-    MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
+    qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA2_IRQ_NUBUS_BIT);
 
     if (level) {
         /* Port A nubus IRQ inputs are active LOW */
-        s->a &= ~(1 << irq);
-        s->ifr |= 1 << VIA2_IRQ_NUBUS_BIT;
+        s->a &= ~(1 << n);
     } else {
-        s->a |= (1 << irq);
-        s->ifr &= ~(1 << VIA2_IRQ_NUBUS_BIT);
+        s->a |= (1 << n);
     }
 
-    mdc->update_irq(s);
+    qemu_set_irq(irq, level);
 }
 
 static void mos6522_q800_via2_init(Object *obj)
@@ -1177,8 +1141,6 @@ static void mos6522_q800_via2_init(Object *obj)
                           "via2", VIA_SIZE);
     sysbus_init_mmio(sbd, &v2s->via_mem);
 
-    qdev_init_gpio_in(DEVICE(obj), via2_irq_request, VIA2_IRQ_NB);
-
     qdev_init_gpio_in_named(DEVICE(obj), via2_nubus_irq_request, "nubus-irq",
                             VIA2_NUBUS_IRQ_NB);
 }
diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index e917a6a095..f76d9227d3 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -25,6 +25,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu-common.h"
+#include "hw/irq.h"
 #include "hw/ppc/mac.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
@@ -96,9 +97,9 @@ static void cuda_set_sr_int(void *opaque)
     CUDAState *s = opaque;
     MOS6522CUDAState *mcs = &s->mos6522_cuda;
     MOS6522State *ms = MOS6522(mcs);
-    MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(ms);
+    qemu_irq irq = qdev_get_gpio_in(DEVICE(ms), SR_INT_BIT);
 
-    mdc->set_sr_int(ms);
+    qemu_set_irq(irq, 1);
 }
 
 static void cuda_delay_set_sr_int(CUDAState *s)
diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
index eb39c64694..6e80fe1cfa 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -75,9 +75,9 @@ static void via_set_sr_int(void *opaque)
     PMUState *s = opaque;
     MOS6522PMUState *mps = MOS6522_PMU(&s->mos6522_pmu);
     MOS6522State *ms = MOS6522(mps);
-    MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(ms);
+    qemu_irq irq = qdev_get_gpio_in(DEVICE(ms), SR_INT_BIT);
 
-    mdc->set_sr_int(ms);
+    qemu_set_irq(irq, 1);
 }
 
 static void pmu_update_extirq(PMUState *s)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 1c57332b40..6be6853dc2 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -52,6 +52,19 @@ static void mos6522_update_irq(MOS6522State *s)
     }
 }
 
+static void mos6522_set_irq(void *opaque, int n, int level)
+{
+    MOS6522State *s = MOS6522(opaque);
+
+    if (level) {
+        s->ifr |= 1 << n;
+    } else {
+        s->ifr &= ~(1 << n);
+    }
+
+    mos6522_update_irq(s);
+}
+
 static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti)
 {
     MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
@@ -488,6 +501,8 @@ static void mos6522_init(Object *obj)
 
     s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
     s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
+
+    qdev_init_gpio_in(DEVICE(obj), mos6522_set_irq, VIA_NUM_INTS);
 }
 
 static void mos6522_finalize(Object *obj)
diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
index 2df1ab01b6..bffeba38ee 100644
--- a/include/hw/misc/mac_via.h
+++ b/include/hw/misc/mac_via.h
@@ -24,8 +24,6 @@
 #define VIA1_IRQ_ADB_DATA_BIT   CB2_INT_BIT
 #define VIA1_IRQ_ADB_CLOCK_BIT  CB1_INT_BIT
 
-#define VIA1_IRQ_NB             8
-
 #define VIA1_IRQ_ONE_SECOND     (1 << VIA1_IRQ_ONE_SECOND_BIT)
 #define VIA1_IRQ_60HZ           (1 << VIA1_IRQ_60HZ_BIT)
 #define VIA1_IRQ_ADB_READY      (1 << VIA1_IRQ_ADB_READY_BIT)
@@ -42,7 +40,7 @@ struct MOS6522Q800VIA1State {
 
     MemoryRegion via_mem;
 
-    qemu_irq irqs[VIA1_IRQ_NB];
+    qemu_irq irqs[VIA_NUM_INTS];
     qemu_irq auxmode_irq;
     uint8_t last_b;
 
@@ -85,8 +83,6 @@ struct MOS6522Q800VIA1State {
 #define VIA2_IRQ_SCSI_BIT       CB2_INT_BIT
 #define VIA2_IRQ_ASC_BIT        CB1_INT_BIT
 
-#define VIA2_IRQ_NB             8
-
 #define VIA2_IRQ_SCSI_DATA      (1 << VIA2_IRQ_SCSI_DATA_BIT)
 #define VIA2_IRQ_NUBUS          (1 << VIA2_IRQ_NUBUS_BIT)
 #define VIA2_IRQ_UNUSED         (1 << VIA2_IRQ_SCSI_BIT)
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index 12abd8b8d2..ced8a670bf 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -57,6 +57,8 @@
 #define T2_INT             (1 << T2_INT_BIT)
 #define T1_INT             (1 << T1_INT_BIT)
 
+#define VIA_NUM_INTS       5
+
 /* Bits in ACR */
 #define T1MODE             0xc0    /* Timer 1 mode */
 #define T1MODE_CONT        0x40    /*  continuous interrupts */
-- 
2.20.1

[PATCH 05/11] mos6522: remove update_irq() and set_sr_int() methods from MOS6522DeviceClass

[Mark Cave-Ayland]

Now that the mos6522 IRQs are managed using standard qdev gpios these methods
are no longer required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/mos6522.c         | 9 ---------
 include/hw/misc/mos6522.h | 2 --
 2 files changed, 11 deletions(-)

diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 6be6853dc2..4c3147a7d1 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -208,13 +208,6 @@ static void mos6522_timer2(void *opaque)
     mos6522_update_irq(s);
 }
 
-static void mos6522_set_sr_int(MOS6522State *s)
-{
-    trace_mos6522_set_sr_int();
-    s->ifr |= SR_INT;
-    mos6522_update_irq(s);
-}
-
 static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
 {
     return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
@@ -527,10 +520,8 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
     dc->vmsd = &vmstate_mos6522;
     device_class_set_props(dc, mos6522_properties);
     mdc->parent_reset = dc->reset;
-    mdc->set_sr_int = mos6522_set_sr_int;
     mdc->portB_write = mos6522_portB_write;
     mdc->portA_write = mos6522_portA_write;
-    mdc->update_irq = mos6522_update_irq;
     mdc->get_timer1_counter_value = mos6522_get_counter_value;
     mdc->get_timer2_counter_value = mos6522_get_counter_value;
     mdc->get_timer1_load_time = mos6522_get_load_time;
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index ced8a670bf..f6b513a3a2 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -140,10 +140,8 @@ struct MOS6522DeviceClass {
     DeviceClass parent_class;
 
     DeviceReset parent_reset;
-    void (*set_sr_int)(MOS6522State *dev);
     void (*portB_write)(MOS6522State *dev);
     void (*portA_write)(MOS6522State *dev);
-    void (*update_irq)(MOS6522State *dev);
     /* These are used to influence the CUDA MacOS timebase calibration */
     uint64_t (*get_timer1_counter_value)(MOS6522State *dev, MOS6522Timer *ti);
     uint64_t (*get_timer2_counter_value)(MOS6522State *dev, MOS6522Timer *ti);
-- 
2.20.1

[PATCH 06/11] mos6522: use device_class_set_parent_reset() to propagate reset to parent

[Mark Cave-Ayland]

Switch from using a legacy approach to the more formal approach for propagating
device reset to the parent.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/mac_via.c    | 7 +++++--
 hw/misc/macio/cuda.c | 3 ++-
 hw/misc/macio/pmu.c  | 3 ++-
 hw/misc/mos6522.c    | 1 -
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index 0756374f1b..95cf2e03d9 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -1076,9 +1076,11 @@ static Property mos6522_q800_via1_properties[] = {
 static void mos6522_q800_via1_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
+    MOS6522DeviceClass *mdc = MOS6522_CLASS(oc);
 
     dc->realize = mos6522_q800_via1_realize;
-    dc->reset = mos6522_q800_via1_reset;
+    device_class_set_parent_reset(dc, mos6522_q800_via1_reset,
+                                  &mdc->parent_reset);
     dc->vmsd = &vmstate_q800_via1;
     device_class_set_props(dc, mos6522_q800_via1_properties);
 }
@@ -1161,7 +1163,8 @@ static void mos6522_q800_via2_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     MOS6522DeviceClass *mdc = MOS6522_CLASS(oc);
 
-    dc->reset = mos6522_q800_via2_reset;
+    device_class_set_parent_reset(dc, mos6522_q800_via2_reset,
+                                  &mdc->parent_reset);
     dc->vmsd = &vmstate_q800_via2;
     mdc->portB_write = mos6522_q800_via2_portB_write;
 }
diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index f76d9227d3..4ced31c2c5 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -606,7 +606,8 @@ static void mos6522_cuda_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     MOS6522DeviceClass *mdc = MOS6522_CLASS(oc);
 
-    dc->reset = mos6522_cuda_reset;
+    device_class_set_parent_reset(dc, mos6522_cuda_reset,
+                                  &mdc->parent_reset);
     mdc->portB_write = mos6522_cuda_portB_write;
     mdc->get_timer1_counter_value = cuda_get_counter_value;
     mdc->get_timer2_counter_value = cuda_get_counter_value;
diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
index 6e80fe1cfa..a5dd0a4734 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -850,7 +850,8 @@ static void mos6522_pmu_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     MOS6522DeviceClass *mdc = MOS6522_CLASS(oc);
 
-    dc->reset = mos6522_pmu_reset;
+    device_class_set_parent_reset(dc, mos6522_pmu_reset,
+                                  &mdc->parent_reset);
     mdc->portB_write = mos6522_pmu_portB_write;
     mdc->portA_write = mos6522_pmu_portA_write;
 }
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 4c3147a7d1..093cc83dcf 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -519,7 +519,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
     dc->reset = mos6522_reset;
     dc->vmsd = &vmstate_mos6522;
     device_class_set_props(dc, mos6522_properties);
-    mdc->parent_reset = dc->reset;
     mdc->portB_write = mos6522_portB_write;
     mdc->portA_write = mos6522_portA_write;
     mdc->get_timer1_counter_value = mos6522_get_counter_value;
-- 
2.20.1

[PATCH 07/11] mos6522: add register names to register read/write trace events

[Mark Cave-Ayland]

This helps to follow how the guest is programming the mos6522 when debugging.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/mos6522.c    | 10 ++++++++--
 hw/misc/trace-events |  4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 093cc83dcf..aaae195d63 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -36,6 +36,12 @@
 #include "qemu/module.h"
 #include "trace.h"
 
+
+static const char *mos6522_reg_names[16] = {
+    "ORB", "ORA", "DDRB", "DDRA", "T1CL", "T1CH", "T1LL", "T1LH",
+    "T2CL", "T2CH", "SR", "ACR", "PCR", "IFR", "IER", "ANH"
+};
+
 /* XXX: implement all timer modes */
 
 static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
@@ -310,7 +316,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
     }
 
     if (addr != VIA_REG_IFR || val != 0) {
-        trace_mos6522_read(addr, val);
+        trace_mos6522_read(addr, mos6522_reg_names[addr], val);
     }
 
     return val;
@@ -321,7 +327,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     MOS6522State *s = opaque;
     MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
 
-    trace_mos6522_write(addr, val);
+    trace_mos6522_write(addr, mos6522_reg_names[addr], val);
 
     switch (addr) {
     case VIA_REG_B:
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 1c373dd0a4..c1ea57de31 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -95,8 +95,8 @@ imx7_gpr_write(uint64_t offset, uint64_t value) "addr 0x%08" PRIx64 "value 0x%08
 mos6522_set_counter(int index, unsigned int val) "T%d.counter=%d"
 mos6522_get_next_irq_time(uint16_t latch, int64_t d, int64_t delta) "latch=%d counter=0x%"PRId64 " delta_next=0x%"PRId64
 mos6522_set_sr_int(void) "set sr_int"
-mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64
-mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x"
+mos6522_write(uint64_t addr, const char *name, uint64_t val) "reg=0x%"PRIx64 " [%s] val=0x%"PRIx64
+mos6522_read(uint64_t addr, const char *name, unsigned val) "reg=0x%"PRIx64 " [%s] val=0x%x"
 
 # npcm7xx_clk.c
 npcm7xx_clk_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
-- 
2.20.1

[PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

This displays detailed information about the device registers and timers to aid
debugging problems with timers and interrupts.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hmp-commands-info.hx | 12 ++++++
 hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index e90f20a107..4e714e79a2 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -879,3 +879,15 @@ SRST
   ``info sgx``
     Show intel SGX information.
 ERST
+
+    {
+        .name       = "via",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show guest 6522 VIA devices",
+    },
+
+SRST
+  ``info via``
+    Show guest 6522 VIA devices.
+ERST
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index aaae195d63..cfa6a9c44b 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -30,6 +30,8 @@
 #include "hw/misc/mos6522.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
+#include "monitor/monitor.h"
+#include "qapi/type-helpers.h"
 #include "qemu/timer.h"
 #include "qemu/cutils.h"
 #include "qemu/log.h"
@@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     }
 }
 
+static int qmp_x_query_via_foreach(Object *obj, void *opaque)
+{
+    GString *buf = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_MOS6522)) {
+        MOS6522State *s = MOS6522(obj);
+        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        uint16_t t1counter = get_counter(s, &s->timers[0]);
+        uint16_t t2counter = get_counter(s, &s->timers[1]);
+
+        g_string_append_printf(buf, "%s:\n", object_get_typename(obj));
+
+        g_string_append_printf(buf, "  Registers:\n");
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[0], s->b);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[1], s->a);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[2], s->dirb);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[3], s->dira);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[4], t1counter & 0xff);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[5], t1counter >> 8);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[6],
+                               s->timers[0].latch & 0xff);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[7],
+                               s->timers[0].latch >> 8);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[8], t2counter & 0xff);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[9], t2counter >> 8);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[10], s->sr);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[11], s->acr);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[12], s->pcr);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[13], s->ifr);
+        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
+                               mos6522_reg_names[14], s->ier);
+
+        g_string_append_printf(buf, "  Timers:\n");
+        g_string_append_printf(buf, "    Using current time now(ns)=%"PRId64
+                                    "\n", now);
+        g_string_append_printf(buf, "    T1 freq(hz)=%"PRId64
+                               " mode=%s"
+                               " counter=0x%x"
+                               " latch=0x%x\n"
+                               "       load_time(ns)=%"PRId64
+                               " next_irq_time(ns)=%"PRId64 "\n",
+                               s->timers[0].frequency,
+                               ((s->acr & T1MODE) == T1MODE_CONT) ? "continuous"
+                                                                  : "one-shot",
+                               t1counter,
+                               s->timers[0].latch,
+                               s->timers[0].load_time,
+                               get_next_irq_time(s, &s->timers[0], now));
+        g_string_append_printf(buf, "    T2 freq(hz)=%"PRId64
+                               " mode=%s"
+                               " counter=0x%x"
+                               " latch=0x%x\n"
+                               "       load_time(ns)=%"PRId64
+                               " next_irq_time(ns)=%"PRId64 "\n",
+                               s->timers[1].frequency,
+                               "one-shot",
+                               t2counter,
+                               s->timers[1].latch,
+                               s->timers[1].load_time,
+                               get_next_irq_time(s, &s->timers[1], now));
+    }
+
+    return 0;
+}
+
+static HumanReadableText *qmp_x_query_via(Error **errp)
+{
+    g_autoptr(GString) buf = g_string_new("");
+
+    object_child_foreach_recursive(object_get_root(),
+                                   qmp_x_query_via_foreach, buf);
+
+    return human_readable_text_from_str(buf);
+}
+
 static const MemoryRegionOps mos6522_ops = {
     .read = mos6522_read,
     .write = mos6522_write,
@@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = {
 static void mos6522_register_types(void)
 {
     type_register_static(&mos6522_type_info);
+    monitor_register_hmp_info_hrt("via", qmp_x_query_via);
 }
 
 type_init(mos6522_register_types)
-- 
2.20.1

[PATCH 09/11] mos6522: record last_irq_levels in mos6522_set_irq()

[Mark Cave-Ayland]

To detect edge-triggered IRQs it is necessary to store the last state of each
IRQ in a last_irq_levels bitmap.

Note: this is a migration break for machines which use mos6522 instances which
are g3beige/mac99 (PPC) and q800 (m68k).

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

diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index cfa6a9c44b..6eed751726 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -71,6 +71,12 @@ static void mos6522_set_irq(void *opaque, int n, int level)
     }
 
     mos6522_update_irq(s);
+
+    if (level) {
+        s->last_irq_levels |= 1 << n;
+    } else {
+        s->last_irq_levels &= ~(1 << n);
+    }
 }
 
 static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti)
@@ -532,8 +538,8 @@ static const VMStateDescription vmstate_mos6522_timer = {
 
 const VMStateDescription vmstate_mos6522 = {
     .name = "mos6522",
-    .version_id = 0,
-    .minimum_version_id = 0,
+    .version_id = 1,
+    .minimum_version_id = 1,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(a, MOS6522State),
         VMSTATE_UINT8(b, MOS6522State),
@@ -544,6 +550,7 @@ const VMStateDescription vmstate_mos6522 = {
         VMSTATE_UINT8(pcr, MOS6522State),
         VMSTATE_UINT8(ifr, MOS6522State),
         VMSTATE_UINT8(ier, MOS6522State),
+        VMSTATE_UINT8(last_irq_levels, MOS6522State),
         VMSTATE_STRUCT_ARRAY(timers, MOS6522State, 2, 0,
                              vmstate_mos6522_timer, MOS6522Timer),
         VMSTATE_END_OF_LIST()
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index f6b513a3a2..6c40a8fcd3 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -131,6 +131,7 @@ struct MOS6522State {
     uint64_t frequency;
 
     qemu_irq irq;
+    uint8_t last_irq_levels;
 };
 
 #define TYPE_MOS6522 "mos6522"
-- 
2.20.1

[PATCH 10/11] mos6522: implement edge-triggering for CA1/2 and CB1/2 control line IRQs

[Mark Cave-Ayland]

The mos6522 datasheet describes how the control lines IRQs are edge-triggered
according to the configuration in the PCR register. Implement the logic according
to the datasheet so that the interrupt bits in IFR are latched when the edge is
detected, and cleared when reading portA/portB or writing to IFR as necessary.

To maintain bisectibility this change also updates the SCSI, SCSI data, Nubus
and VIA2 60Hz/1Hz clocks in the q800 machine to be negative edge-triggered as
confirmed by the PCR programming in all of Linux, NetBSD and MacOS.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c            |  9 +++--
 hw/misc/mac_via.c         | 11 ++++--
 hw/misc/mos6522.c         | 82 +++++++++++++++++++++++++++++++++++++--
 include/hw/misc/mos6522.h | 15 +++++++
 4 files changed, 106 insertions(+), 11 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 55dfe5036f..66ca5c0df6 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -533,10 +533,11 @@ static void q800_init(MachineState *machine)
 
     sysbus = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(sysbus, &error_fatal);
-    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(via2_dev,
-                                                   VIA2_IRQ_SCSI_BIT));
-    sysbus_connect_irq(sysbus, 1, qdev_get_gpio_in(via2_dev,
-                                                   VIA2_IRQ_SCSI_DATA_BIT));
+    /* SCSI and SCSI data IRQs are negative edge triggered */
+    sysbus_connect_irq(sysbus, 0, qemu_irq_invert(qdev_get_gpio_in(via2_dev,
+                                                  VIA2_IRQ_SCSI_BIT)));
+    sysbus_connect_irq(sysbus, 1, qemu_irq_invert(qdev_get_gpio_in(via2_dev,
+                                                  VIA2_IRQ_SCSI_DATA_BIT)));
     sysbus_mmio_map(sysbus, 0, ESP_BASE);
     sysbus_mmio_map(sysbus, 1, ESP_PDMA);
 
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index 95cf2e03d9..d9ab5b3839 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -327,7 +327,9 @@ static void via1_sixty_hz(void *opaque)
     MOS6522State *s = MOS6522(v1s);
     qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA1_IRQ_60HZ_BIT);
 
-    qemu_set_irq(irq, 1);
+    /* Negative edge trigger */
+    qemu_irq_lower(irq);
+    qemu_irq_raise(irq);
 
     via1_sixty_hz_update(v1s);
 }
@@ -338,7 +340,9 @@ static void via1_one_second(void *opaque)
     MOS6522State *s = MOS6522(v1s);
     qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA1_IRQ_ONE_SECOND_BIT);
 
-    qemu_set_irq(irq, 1);
+    /* Negative edge trigger */
+    qemu_irq_lower(irq);
+    qemu_irq_raise(irq);
 
     via1_one_second_update(v1s);
 }
@@ -1131,7 +1135,8 @@ static void via2_nubus_irq_request(void *opaque, int n, int level)
         s->a |= (1 << n);
     }
 
-    qemu_set_irq(irq, level);
+    /* Negative edge trigger */
+    qemu_set_irq(irq, !level);
 }
 
 static void mos6522_q800_via2_init(Object *obj)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 6eed751726..84b2d54ef3 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -63,14 +63,62 @@ static void mos6522_update_irq(MOS6522State *s)
 static void mos6522_set_irq(void *opaque, int n, int level)
 {
     MOS6522State *s = MOS6522(opaque);
+    int last_level = !!(s->last_irq_levels & (1 << n));
+    uint8_t last_ifr = s->ifr;
+    bool positive_edge = true;
+    int ctrl;
+
+    /*
+     * SR_INT is managed by mos6522 instances and cleared upon SR
+     * read. It is only the external CA1/2 and CB1/2 lines that
+     * are edge-triggered and latched in IFR
+     */
+    if (n != SR_INT_BIT && level == last_level) {
+        return;
+    }
 
-    if (level) {
+    /* Detect negative edge trigger */
+    if (last_level == 1 && level == 0) {
+        positive_edge = false;
+    }
+
+    switch (n) {
+    case CA2_INT_BIT:
+        ctrl = (s->pcr & CA2_CTRL_MASK) >> CA2_CTRL_SHIFT;
+        if ((positive_edge && (ctrl & C2_POS)) ||
+             (!positive_edge && !(ctrl & C2_POS))) {
+            s->ifr |= 1 << n;
+        }
+        break;
+    case CA1_INT_BIT:
+        ctrl = (s->pcr & CA1_CTRL_MASK) >> CA1_CTRL_SHIFT;
+        if ((positive_edge && (ctrl & C1_POS)) ||
+             (!positive_edge && !(ctrl & C1_POS))) {
+            s->ifr |= 1 << n;
+        }
+        break;
+    case SR_INT_BIT:
         s->ifr |= 1 << n;
-    } else {
-        s->ifr &= ~(1 << n);
+        break;
+    case CB2_INT_BIT:
+        ctrl = (s->pcr & CB2_CTRL_MASK) >> CB2_CTRL_SHIFT;
+        if ((positive_edge && (ctrl & C2_POS)) ||
+             (!positive_edge && !(ctrl & C2_POS))) {
+            s->ifr |= 1 << n;
+        }
+        break;
+    case CB1_INT_BIT:
+        ctrl = (s->pcr & CB1_CTRL_MASK) >> CB1_CTRL_SHIFT;
+        if ((positive_edge && (ctrl & C1_POS)) ||
+             (!positive_edge && !(ctrl & C1_POS))) {
+            s->ifr |= 1 << n;
+        }
+        break;
     }
 
-    mos6522_update_irq(s);
+    if (s->ifr != last_ifr) {
+        mos6522_update_irq(s);
+    }
 
     if (level) {
         s->last_irq_levels |= 1 << n;
@@ -249,6 +297,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
 {
     MOS6522State *s = opaque;
     uint32_t val;
+    int ctrl;
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
     if (now >= s->timers[0].next_irq_time) {
@@ -262,12 +311,24 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
     switch (addr) {
     case VIA_REG_B:
         val = s->b;
+        ctrl = (s->pcr & CB2_CTRL_MASK) >> CB2_CTRL_SHIFT;
+        if (!(ctrl & C2_IND)) {
+            s->ifr &= ~CB2_INT;
+        }
+        s->ifr &= ~CB1_INT;
+        mos6522_update_irq(s);
         break;
     case VIA_REG_A:
        qemu_log_mask(LOG_UNIMP, "Read access to register A with handshake");
        /* fall through */
     case VIA_REG_ANH:
         val = s->a;
+        ctrl = (s->pcr & CA2_CTRL_MASK) >> CA2_CTRL_SHIFT;
+        if (!(ctrl & C2_IND)) {
+            s->ifr &= ~CA2_INT;
+        }
+        s->ifr &= ~CA1_INT;
+        mos6522_update_irq(s);
         break;
     case VIA_REG_DIRB:
         val = s->dirb;
@@ -334,6 +395,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
 {
     MOS6522State *s = opaque;
     MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
+    int ctrl;
 
     trace_mos6522_write(addr, mos6522_reg_names[addr], val);
 
@@ -341,6 +403,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     case VIA_REG_B:
         s->b = (s->b & ~s->dirb) | (val & s->dirb);
         mdc->portB_write(s);
+        ctrl = (s->pcr & CB2_CTRL_MASK) >> CB2_CTRL_SHIFT;
+        if (!(ctrl & C2_IND)) {
+            s->ifr &= ~CB2_INT;
+        }
+        s->ifr &= ~CB1_INT;
+        mos6522_update_irq(s);
         break;
     case VIA_REG_A:
        qemu_log_mask(LOG_UNIMP, "Write access to register A with handshake");
@@ -348,6 +416,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     case VIA_REG_ANH:
         s->a = (s->a & ~s->dira) | (val & s->dira);
         mdc->portA_write(s);
+        ctrl = (s->pcr & CA2_CTRL_MASK) >> CA2_CTRL_SHIFT;
+        if (!(ctrl & C2_IND)) {
+            s->ifr &= ~CA2_INT;
+        }
+        s->ifr &= ~CA1_INT;
+        mos6522_update_irq(s);
         break;
     case VIA_REG_DIRB:
         s->dirb = val;
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index 6c40a8fcd3..a9afb539b2 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -63,6 +63,21 @@
 #define T1MODE             0xc0    /* Timer 1 mode */
 #define T1MODE_CONT        0x40    /*  continuous interrupts */
 
+/* Bits in PCR */
+#define CB2_CTRL_MASK      0xe0
+#define CB2_CTRL_SHIFT     5
+#define CB1_CTRL_MASK      0x10
+#define CB1_CTRL_SHIFT     4
+#define CA2_CTRL_MASK      0x0e
+#define CA2_CTRL_SHIFT     1
+#define CA1_CTRL_MASK      0x1
+#define CA1_CTRL_SHIFT     0
+
+#define C2_POS             0x2
+#define C2_IND             0x1
+
+#define C1_POS             0x1
+
 /* VIA registers */
 #define VIA_REG_B       0x00
 #define VIA_REG_A       0x01
-- 
2.20.1

[PATCH 11/11] macio/pmu.c: remove redundant code

[Mark Cave-Ayland]

Now that the logic related to edge-triggered interrupts is all contained within
the mos6522 device the redundant implementation for the mac99 PMU device can
be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/pmu.c         | 33 ---------------------------------
 include/hw/misc/macio/pmu.h |  2 --
 2 files changed, 35 deletions(-)

diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
index a5dd0a4734..a16841e758 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -57,19 +57,6 @@
 
 #define VIA_TIMER_FREQ (4700000 / 6)
 
-static void via_update_irq(PMUState *s)
-{
-    MOS6522PMUState *mps = MOS6522_PMU(&s->mos6522_pmu);
-    MOS6522State *ms = MOS6522(mps);
-
-    bool new_state = !!(ms->ifr & ms->ier & (SR_INT | T1_INT | T2_INT));
-
-    if (new_state != s->via_irq_state) {
-        s->via_irq_state = new_state;
-        qemu_set_irq(s->via_irq, new_state);
-    }
-}
-
 static void via_set_sr_int(void *opaque)
 {
     PMUState *s = opaque;
@@ -808,28 +795,9 @@ static void mos6522_pmu_portB_write(MOS6522State *s)
     MOS6522PMUState *mps = container_of(s, MOS6522PMUState, parent_obj);
     PMUState *ps = container_of(mps, PMUState, mos6522_pmu);
 
-    if ((s->pcr & 0xe0) == 0x20 || (s->pcr & 0xe0) == 0x60) {
-        s->ifr &= ~CB2_INT;
-    }
-    s->ifr &= ~CB1_INT;
-
-    via_update_irq(ps);
     pmu_update(ps);
 }
 
-static void mos6522_pmu_portA_write(MOS6522State *s)
-{
-    MOS6522PMUState *mps = container_of(s, MOS6522PMUState, parent_obj);
-    PMUState *ps = container_of(mps, PMUState, mos6522_pmu);
-
-    if ((s->pcr & 0x0e) == 0x02 || (s->pcr & 0x0e) == 0x06) {
-        s->ifr &= ~CA2_INT;
-    }
-    s->ifr &= ~CA1_INT;
-
-    via_update_irq(ps);
-}
-
 static void mos6522_pmu_reset(DeviceState *dev)
 {
     MOS6522State *ms = MOS6522(dev);
@@ -853,7 +821,6 @@ static void mos6522_pmu_class_init(ObjectClass *oc, void *data)
     device_class_set_parent_reset(dc, mos6522_pmu_reset,
                                   &mdc->parent_reset);
     mdc->portB_write = mos6522_pmu_portB_write;
-    mdc->portA_write = mos6522_pmu_portA_write;
 }
 
 static const TypeInfo mos6522_pmu_type_info = {
diff --git a/include/hw/misc/macio/pmu.h b/include/hw/misc/macio/pmu.h
index 78237d99a2..00fcdd23f5 100644
--- a/include/hw/misc/macio/pmu.h
+++ b/include/hw/misc/macio/pmu.h
@@ -193,8 +193,6 @@ struct PMUState {
 
     MemoryRegion mem;
     uint64_t frequency;
-    qemu_irq via_irq;
-    bool via_irq_state;
 
     /* PMU state */
     MOS6522PMUState mos6522_pmu;
-- 
2.20.1

Re: [PATCH 01/11] mos6522: add defines for IFR bit flags

[BALATON Zoltan]

On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
> These are intended to make it easier to see how the physical control lines
> are wired for each instance.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
> index fc95d22b0f..12abd8b8d2 100644
> --- a/include/hw/misc/mos6522.h
> +++ b/include/hw/misc/mos6522.h
> @@ -41,13 +41,21 @@
> #define IER_SET            0x80    /* set bits in IER */
> #define IER_CLR            0       /* clear bits in IER */
>
> -#define CA2_INT            0x01
> -#define CA1_INT            0x02
> -#define SR_INT             0x04    /* Shift register full/empty */
> -#define CB2_INT            0x08
> -#define CB1_INT            0x10
> -#define T2_INT             0x20    /* Timer 2 interrupt */
> -#define T1_INT             0x40    /* Timer 1 interrupt */
> +#define CA2_INT_BIT        0
> +#define CA1_INT_BIT        1
> +#define SR_INT_BIT         2       /* Shift register full/empty */
> +#define CB2_INT_BIT        3
> +#define CB1_INT_BIT        4
> +#define T2_INT_BIT         5       /* Timer 2 interrupt */
> +#define T1_INT_BIT         6       /* Timer 1 interrupt */
> +
> +#define CA2_INT            (1 << CA2_INT_BIT)
> +#define CA1_INT            (1 << CA1_INT_BIT)
> +#define SR_INT             (1 << SR_INT_BIT)
> +#define CB2_INT            (1 << CB2_INT_BIT)
> +#define CB1_INT            (1 << CB1_INT_BIT)
> +#define T2_INT             (1 << T2_INT_BIT)
> +#define T1_INT             (1 << T1_INT_BIT)

Maybe you could leave the #defines called XX_INT and then use BIT(XX_INT) 
instead of the second set of #defines which would provide same readability 
but with less #defines needed.

Regards,
BALATON Zoltan

> /* Bits in ACR */
> #define T1MODE             0xc0    /* Timer 1 mode */
>

Re: [PATCH 01/11] mos6522: add defines for IFR bit flags

[Mark Cave-Ayland]

On 27/01/2022 23:16, BALATON Zoltan wrote:

> On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
>> These are intended to make it easier to see how the physical control lines
>> are wired for each instance.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>> index fc95d22b0f..12abd8b8d2 100644
>> --- a/include/hw/misc/mos6522.h
>> +++ b/include/hw/misc/mos6522.h
>> @@ -41,13 +41,21 @@
>> #define IER_SET            0x80    /* set bits in IER */
>> #define IER_CLR            0       /* clear bits in IER */
>>
>> -#define CA2_INT            0x01
>> -#define CA1_INT            0x02
>> -#define SR_INT             0x04    /* Shift register full/empty */
>> -#define CB2_INT            0x08
>> -#define CB1_INT            0x10
>> -#define T2_INT             0x20    /* Timer 2 interrupt */
>> -#define T1_INT             0x40    /* Timer 1 interrupt */
>> +#define CA2_INT_BIT        0
>> +#define CA1_INT_BIT        1
>> +#define SR_INT_BIT         2       /* Shift register full/empty */
>> +#define CB2_INT_BIT        3
>> +#define CB1_INT_BIT        4
>> +#define T2_INT_BIT         5       /* Timer 2 interrupt */
>> +#define T1_INT_BIT         6       /* Timer 1 interrupt */
>> +
>> +#define CA2_INT            (1 << CA2_INT_BIT)
>> +#define CA1_INT            (1 << CA1_INT_BIT)
>> +#define SR_INT             (1 << SR_INT_BIT)
>> +#define CB2_INT            (1 << CB2_INT_BIT)
>> +#define CB1_INT            (1 << CB1_INT_BIT)
>> +#define T2_INT             (1 << T2_INT_BIT)
>> +#define T1_INT             (1 << T1_INT_BIT)
> 
> Maybe you could leave the #defines called XX_INT and then use BIT(XX_INT) instead of 
> the second set of #defines which would provide same readability but with less 
> #defines needed.

I'm not so keen on removing the _INT defines since that would require updating all 
users to use BIT() everywhere which I don't think gains us much. I could certainly 
replace these definitions with BIT(FOO) instead of (1 << FOO) if that helps 
readability though.


ATB,

Mark.

Re: [PATCH 01/11] mos6522: add defines for IFR bit flags

[Philippe Mathieu-Daudé via]

On 5/2/22 11:51, Mark Cave-Ayland wrote:
> On 27/01/2022 23:16, BALATON Zoltan wrote:
> 
>> On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
>>> These are intended to make it easier to see how the physical control 
>>> lines
>>> are wired for each instance.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
>>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>>> index fc95d22b0f..12abd8b8d2 100644
>>> --- a/include/hw/misc/mos6522.h
>>> +++ b/include/hw/misc/mos6522.h
>>> @@ -41,13 +41,21 @@
>>> #define IER_SET            0x80    /* set bits in IER */
>>> #define IER_CLR            0       /* clear bits in IER */
>>>
>>> -#define CA2_INT            0x01
>>> -#define CA1_INT            0x02
>>> -#define SR_INT             0x04    /* Shift register full/empty */
>>> -#define CB2_INT            0x08
>>> -#define CB1_INT            0x10
>>> -#define T2_INT             0x20    /* Timer 2 interrupt */
>>> -#define T1_INT             0x40    /* Timer 1 interrupt */
>>> +#define CA2_INT_BIT        0
>>> +#define CA1_INT_BIT        1
>>> +#define SR_INT_BIT         2       /* Shift register full/empty */
>>> +#define CB2_INT_BIT        3
>>> +#define CB1_INT_BIT        4
>>> +#define T2_INT_BIT         5       /* Timer 2 interrupt */
>>> +#define T1_INT_BIT         6       /* Timer 1 interrupt */
>>> +
>>> +#define CA2_INT            (1 << CA2_INT_BIT)
>>> +#define CA1_INT            (1 << CA1_INT_BIT)
>>> +#define SR_INT             (1 << SR_INT_BIT)
>>> +#define CB2_INT            (1 << CB2_INT_BIT)
>>> +#define CB1_INT            (1 << CB1_INT_BIT)
>>> +#define T2_INT             (1 << T2_INT_BIT)
>>> +#define T1_INT             (1 << T1_INT_BIT)
>>
>> Maybe you could leave the #defines called XX_INT and then use 
>> BIT(XX_INT) instead of the second set of #defines which would provide 
>> same readability but with less #defines needed.
> 
> I'm not so keen on removing the _INT defines since that would require 
> updating all users to use BIT() everywhere which I don't think gains us 
> much. I could certainly replace these definitions with BIT(FOO) instead 
> of (1 << FOO) if that helps readability though.

Do you mean simply doing this?

-#define T1_INT             0x40    /* Timer 1 interrupt */
+#define T1_INT             BIT(6)  /* Timer 1 interrupt */

Re: [PATCH 01/11] mos6522: add defines for IFR bit flags

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3449 bytes --]

On Sat, 5 Feb 2022, Philippe Mathieu-Daudé wrote:
> On 5/2/22 11:51, Mark Cave-Ayland wrote:
>> On 27/01/2022 23:16, BALATON Zoltan wrote:
>> 
>>> On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
>>>> These are intended to make it easier to see how the physical control 
>>>> lines
>>>> are wired for each instance.
>>>> 
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
>>>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>>> 
>>>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>>>> index fc95d22b0f..12abd8b8d2 100644
>>>> --- a/include/hw/misc/mos6522.h
>>>> +++ b/include/hw/misc/mos6522.h
>>>> @@ -41,13 +41,21 @@
>>>> #define IER_SET            0x80    /* set bits in IER */
>>>> #define IER_CLR            0       /* clear bits in IER */
>>>> 
>>>> -#define CA2_INT            0x01
>>>> -#define CA1_INT            0x02
>>>> -#define SR_INT             0x04    /* Shift register full/empty */
>>>> -#define CB2_INT            0x08
>>>> -#define CB1_INT            0x10
>>>> -#define T2_INT             0x20    /* Timer 2 interrupt */
>>>> -#define T1_INT             0x40    /* Timer 1 interrupt */
>>>> +#define CA2_INT_BIT        0
>>>> +#define CA1_INT_BIT        1
>>>> +#define SR_INT_BIT         2       /* Shift register full/empty */
>>>> +#define CB2_INT_BIT        3
>>>> +#define CB1_INT_BIT        4
>>>> +#define T2_INT_BIT         5       /* Timer 2 interrupt */
>>>> +#define T1_INT_BIT         6       /* Timer 1 interrupt */
>>>> +
>>>> +#define CA2_INT            (1 << CA2_INT_BIT)
>>>> +#define CA1_INT            (1 << CA1_INT_BIT)
>>>> +#define SR_INT             (1 << SR_INT_BIT)
>>>> +#define CB2_INT            (1 << CB2_INT_BIT)
>>>> +#define CB1_INT            (1 << CB1_INT_BIT)
>>>> +#define T2_INT             (1 << T2_INT_BIT)
>>>> +#define T1_INT             (1 << T1_INT_BIT)
>>> 
>>> Maybe you could leave the #defines called XX_INT and then use BIT(XX_INT) 
>>> instead of the second set of #defines which would provide same readability 
>>> but with less #defines needed.
>> 
>> I'm not so keen on removing the _INT defines since that would require 
>> updating all users to use BIT() everywhere which I don't think gains us 
>> much. I could certainly replace these definitions with BIT(FOO) instead of 
>> (1 << FOO) if that helps readability though.
>
> Do you mean simply doing this?
>
> -#define T1_INT             0x40    /* Timer 1 interrupt */
> +#define T1_INT             BIT(6)  /* Timer 1 interrupt */

I meant:

#define T1_INT 6

and then replace current usage of T1_INT with BIT(T1_INT) that way we 
don't need both T1_INT_BIT and T1_INT defines which seems redundant as 
BIT(T1_INT) is not much longer and still clear what it refers to. It's 
true that this needs more changes but the result is more readable IMO than 
introducing another set of defines that ome has to look up when encounters 
them as the meaning might not be clear. That's why I think one set of 
defines for bit numbers and using existing BIT(num) for values is better 
but it's just an idea, I don't care that much.

Regards,
BALATON Zoltan

Re: [PATCH 04/11] mos6522: switch over to use qdev gpios for IRQs

[Peter Maydell]

On Thu, 27 Jan 2022 at 21:01, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> For historical reasons each mos6522 instance implements its own setting and
> update of the IFR flag bits using methods exposed by MOS6522DeviceClass. As
> of today this is no longer required, and it is now possible to implement
> the mos6522 IRQs as standard qdev gpios.
>
> Switch over to use qdev gpios for the mos6522 device and update all instances
> accordingly.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/mac_via.c         | 56 +++++++--------------------------------
>  hw/misc/macio/cuda.c      |  5 ++--
>  hw/misc/macio/pmu.c       |  4 +--
>  hw/misc/mos6522.c         | 15 +++++++++++
>  include/hw/misc/mac_via.h |  6 +----
>  include/hw/misc/mos6522.h |  2 ++
>  6 files changed, 32 insertions(+), 56 deletions(-)


> -static void via2_nubus_irq_request(void *opaque, int irq, int level)
> +static void via2_nubus_irq_request(void *opaque, int n, int level)
>  {
>      MOS6522Q800VIA2State *v2s = opaque;
>      MOS6522State *s = MOS6522(v2s);
> -    MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
> +    qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA2_IRQ_NUBUS_BIT);
>
>      if (level) {
>          /* Port A nubus IRQ inputs are active LOW */
> -        s->a &= ~(1 << irq);
> -        s->ifr |= 1 << VIA2_IRQ_NUBUS_BIT;
> +        s->a &= ~(1 << n);
>      } else {
> -        s->a |= (1 << irq);
> -        s->ifr &= ~(1 << VIA2_IRQ_NUBUS_BIT);
> +        s->a |= (1 << n);
>      }
>
> -    mdc->update_irq(s);
> +    qemu_set_irq(irq, level);
>  }

It feels a bit inconsistent here that we're still reaching into
the MOS6522State to set s->a, but I guess this is still
better than what we had before.

> -#define VIA1_IRQ_NB             8
> -
>  #define VIA1_IRQ_ONE_SECOND     (1 << VIA1_IRQ_ONE_SECOND_BIT)
>  #define VIA1_IRQ_60HZ           (1 << VIA1_IRQ_60HZ_BIT)
>  #define VIA1_IRQ_ADB_READY      (1 << VIA1_IRQ_ADB_READY_BIT)
> @@ -42,7 +40,7 @@ struct MOS6522Q800VIA1State {
>
>      MemoryRegion via_mem;
>
> -    qemu_irq irqs[VIA1_IRQ_NB];
> +    qemu_irq irqs[VIA_NUM_INTS];

This irqs[] array appears to be entirely unused. You could
delete it as a separate patch before this one.

>      qemu_irq auxmode_irq;
>      uint8_t last_b;
>
> @@ -85,8 +83,6 @@ struct MOS6522Q800VIA1State {
>  #define VIA2_IRQ_SCSI_BIT       CB2_INT_BIT
>  #define VIA2_IRQ_ASC_BIT        CB1_INT_BIT
>
> -#define VIA2_IRQ_NB             8
> -
>  #define VIA2_IRQ_SCSI_DATA      (1 << VIA2_IRQ_SCSI_DATA_BIT)
>  #define VIA2_IRQ_NUBUS          (1 << VIA2_IRQ_NUBUS_BIT)
>  #define VIA2_IRQ_UNUSED         (1 << VIA2_IRQ_SCSI_BIT)
> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
> index 12abd8b8d2..ced8a670bf 100644
> --- a/include/hw/misc/mos6522.h
> +++ b/include/hw/misc/mos6522.h
> @@ -57,6 +57,8 @@
>  #define T2_INT             (1 << T2_INT_BIT)
>  #define T1_INT             (1 << T1_INT_BIT)
>
> +#define VIA_NUM_INTS       5

Were we not using 5,6,7 previously ?

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

thanks
-- PMM

Re: [PATCH 05/11] mos6522: remove update_irq() and set_sr_int() methods from MOS6522DeviceClass

[Peter Maydell]

On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Now that the mos6522 IRQs are managed using standard qdev gpios these methods
> are no longer required.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

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

thanks
-- PMM

Re: [PATCH 06/11] mos6522: use device_class_set_parent_reset() to propagate reset to parent

[Peter Maydell]

On Thu, 27 Jan 2022 at 21:04, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Switch from using a legacy approach to the more formal approach for propagating
> device reset to the parent.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/mac_via.c    | 7 +++++--
>  hw/misc/macio/cuda.c | 3 ++-
>  hw/misc/macio/pmu.c  | 3 ++-
>  hw/misc/mos6522.c    | 1 -
>  4 files changed, 9 insertions(+), 5 deletions(-)
>

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

thanks
-- PMM

Re: [PATCH 07/11] mos6522: add register names to register read/write trace events

[Peter Maydell]

On Thu, 27 Jan 2022 at 21:11, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This helps to follow how the guest is programming the mos6522 when debugging.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/mos6522.c    | 10 ++++++++--
>  hw/misc/trace-events |  4 ++--
>  2 files changed, 10 insertions(+), 4 deletions(-)

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

thanks
-- PMM

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Peter Maydell]

On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This displays detailed information about the device registers and timers to aid
> debugging problems with timers and interrupts.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hmp-commands-info.hx | 12 ++++++
>  hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)


I'm not sure how keen we are on adding new device-specific
HMP info commands, but it's not my area of expertise. Markus ?

(patch below for context)

thanks
-- PMM

>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index e90f20a107..4e714e79a2 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -879,3 +879,15 @@ SRST
>    ``info sgx``
>      Show intel SGX information.
>  ERST
> +
> +    {
> +        .name       = "via",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show guest 6522 VIA devices",
> +    },
> +
> +SRST
> +  ``info via``
> +    Show guest 6522 VIA devices.
> +ERST
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index aaae195d63..cfa6a9c44b 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -30,6 +30,8 @@
>  #include "hw/misc/mos6522.h"
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
> +#include "monitor/monitor.h"
> +#include "qapi/type-helpers.h"
>  #include "qemu/timer.h"
>  #include "qemu/cutils.h"
>  #include "qemu/log.h"
> @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>      }
>  }
>
> +static int qmp_x_query_via_foreach(Object *obj, void *opaque)
> +{
> +    GString *buf = opaque;
> +
> +    if (object_dynamic_cast(obj, TYPE_MOS6522)) {
> +        MOS6522State *s = MOS6522(obj);
> +        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        uint16_t t1counter = get_counter(s, &s->timers[0]);
> +        uint16_t t2counter = get_counter(s, &s->timers[1]);
> +
> +        g_string_append_printf(buf, "%s:\n", object_get_typename(obj));
> +
> +        g_string_append_printf(buf, "  Registers:\n");
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[0], s->b);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[1], s->a);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[2], s->dirb);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[3], s->dira);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[4], t1counter & 0xff);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[5], t1counter >> 8);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[6],
> +                               s->timers[0].latch & 0xff);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[7],
> +                               s->timers[0].latch >> 8);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[8], t2counter & 0xff);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[9], t2counter >> 8);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[10], s->sr);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[11], s->acr);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[12], s->pcr);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[13], s->ifr);
> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
> +                               mos6522_reg_names[14], s->ier);
> +
> +        g_string_append_printf(buf, "  Timers:\n");
> +        g_string_append_printf(buf, "    Using current time now(ns)=%"PRId64
> +                                    "\n", now);
> +        g_string_append_printf(buf, "    T1 freq(hz)=%"PRId64
> +                               " mode=%s"
> +                               " counter=0x%x"
> +                               " latch=0x%x\n"
> +                               "       load_time(ns)=%"PRId64
> +                               " next_irq_time(ns)=%"PRId64 "\n",
> +                               s->timers[0].frequency,
> +                               ((s->acr & T1MODE) == T1MODE_CONT) ? "continuous"
> +                                                                  : "one-shot",
> +                               t1counter,
> +                               s->timers[0].latch,
> +                               s->timers[0].load_time,
> +                               get_next_irq_time(s, &s->timers[0], now));
> +        g_string_append_printf(buf, "    T2 freq(hz)=%"PRId64
> +                               " mode=%s"
> +                               " counter=0x%x"
> +                               " latch=0x%x\n"
> +                               "       load_time(ns)=%"PRId64
> +                               " next_irq_time(ns)=%"PRId64 "\n",
> +                               s->timers[1].frequency,
> +                               "one-shot",
> +                               t2counter,
> +                               s->timers[1].latch,
> +                               s->timers[1].load_time,
> +                               get_next_irq_time(s, &s->timers[1], now));
> +    }
> +
> +    return 0;
> +}
> +
> +static HumanReadableText *qmp_x_query_via(Error **errp)
> +{
> +    g_autoptr(GString) buf = g_string_new("");
> +
> +    object_child_foreach_recursive(object_get_root(),
> +                                   qmp_x_query_via_foreach, buf);
> +
> +    return human_readable_text_from_str(buf);
> +}
> +
>  static const MemoryRegionOps mos6522_ops = {
>      .read = mos6522_read,
>      .write = mos6522_write,
> @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = {
>  static void mos6522_register_types(void)
>  {
>      type_register_static(&mos6522_type_info);
> +    monitor_register_hmp_info_hrt("via", qmp_x_query_via);
>  }
>
>  type_init(mos6522_register_types)
> --
> 2.20.1

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Philippe Mathieu-Daudé via]

On 7/2/22 20:34, Peter Maydell wrote:
> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> This displays detailed information about the device registers and timers to aid
>> debugging problems with timers and interrupts.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hmp-commands-info.hx | 12 ++++++
>>   hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 104 insertions(+)
> 
> 
> I'm not sure how keen we are on adding new device-specific
> HMP info commands, but it's not my area of expertise. Markus ?

HMP is David :) IIRC it is OK as long as HMP is a QMP wrapper.

> 
> (patch below for context)
> 
> thanks
> -- PMM
> 
>>
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index e90f20a107..4e714e79a2 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -879,3 +879,15 @@ SRST
>>     ``info sgx``
>>       Show intel SGX information.
>>   ERST
>> +
>> +    {
>> +        .name       = "via",
>> +        .args_type  = "",
>> +        .params     = "",
>> +        .help       = "show guest 6522 VIA devices",
>> +    },
>> +
>> +SRST
>> +  ``info via``
>> +    Show guest 6522 VIA devices.
>> +ERST
>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>> index aaae195d63..cfa6a9c44b 100644
>> --- a/hw/misc/mos6522.c
>> +++ b/hw/misc/mos6522.c
>> @@ -30,6 +30,8 @@
>>   #include "hw/misc/mos6522.h"
>>   #include "hw/qdev-properties.h"
>>   #include "migration/vmstate.h"
>> +#include "monitor/monitor.h"
>> +#include "qapi/type-helpers.h"
>>   #include "qemu/timer.h"
>>   #include "qemu/cutils.h"
>>   #include "qemu/log.h"
>> @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>>       }
>>   }
>>
>> +static int qmp_x_query_via_foreach(Object *obj, void *opaque)
>> +{
>> +    GString *buf = opaque;
>> +
>> +    if (object_dynamic_cast(obj, TYPE_MOS6522)) {
>> +        MOS6522State *s = MOS6522(obj);
>> +        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> +        uint16_t t1counter = get_counter(s, &s->timers[0]);
>> +        uint16_t t2counter = get_counter(s, &s->timers[1]);
>> +
>> +        g_string_append_printf(buf, "%s:\n", object_get_typename(obj));
>> +
>> +        g_string_append_printf(buf, "  Registers:\n");
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[0], s->b);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[1], s->a);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[2], s->dirb);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[3], s->dira);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[4], t1counter & 0xff);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[5], t1counter >> 8);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[6],
>> +                               s->timers[0].latch & 0xff);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[7],
>> +                               s->timers[0].latch >> 8);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[8], t2counter & 0xff);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[9], t2counter >> 8);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[10], s->sr);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[11], s->acr);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[12], s->pcr);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[13], s->ifr);
>> +        g_string_append_printf(buf, "    %-*s:    0x%x\n", 4,
>> +                               mos6522_reg_names[14], s->ier);
>> +
>> +        g_string_append_printf(buf, "  Timers:\n");
>> +        g_string_append_printf(buf, "    Using current time now(ns)=%"PRId64
>> +                                    "\n", now);
>> +        g_string_append_printf(buf, "    T1 freq(hz)=%"PRId64
>> +                               " mode=%s"
>> +                               " counter=0x%x"
>> +                               " latch=0x%x\n"
>> +                               "       load_time(ns)=%"PRId64
>> +                               " next_irq_time(ns)=%"PRId64 "\n",
>> +                               s->timers[0].frequency,
>> +                               ((s->acr & T1MODE) == T1MODE_CONT) ? "continuous"
>> +                                                                  : "one-shot",
>> +                               t1counter,
>> +                               s->timers[0].latch,
>> +                               s->timers[0].load_time,
>> +                               get_next_irq_time(s, &s->timers[0], now));
>> +        g_string_append_printf(buf, "    T2 freq(hz)=%"PRId64
>> +                               " mode=%s"
>> +                               " counter=0x%x"
>> +                               " latch=0x%x\n"
>> +                               "       load_time(ns)=%"PRId64
>> +                               " next_irq_time(ns)=%"PRId64 "\n",
>> +                               s->timers[1].frequency,
>> +                               "one-shot",
>> +                               t2counter,
>> +                               s->timers[1].latch,
>> +                               s->timers[1].load_time,
>> +                               get_next_irq_time(s, &s->timers[1], now));
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static HumanReadableText *qmp_x_query_via(Error **errp)
>> +{
>> +    g_autoptr(GString) buf = g_string_new("");
>> +
>> +    object_child_foreach_recursive(object_get_root(),
>> +                                   qmp_x_query_via_foreach, buf);
>> +
>> +    return human_readable_text_from_str(buf);
>> +}
>> +
>>   static const MemoryRegionOps mos6522_ops = {
>>       .read = mos6522_read,
>>       .write = mos6522_write,
>> @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = {
>>   static void mos6522_register_types(void)
>>   {
>>       type_register_static(&mos6522_type_info);
>> +    monitor_register_hmp_info_hrt("via", qmp_x_query_via);
>>   }
>>
>>   type_init(mos6522_register_types)
>> --
>> 2.20.1
> 

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Markus Armbruster]

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

> On 7/2/22 20:34, Peter Maydell wrote:
>> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>
>>> This displays detailed information about the device registers and timers to aid
>>> debugging problems with timers and interrupts.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hmp-commands-info.hx | 12 ++++++
>>>   hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 104 insertions(+)
>> 
>> I'm not sure how keen we are on adding new device-specific
>> HMP info commands, but it's not my area of expertise. Markus ?
>
> HMP is David :)

Yes.

>                 IIRC it is OK as long as HMP is a QMP wrapper.

That's "how to do it", and I'll get back to it in a jiffie, but Peter
was wondering about the "whether to do it".

Most HMP commands are always there.

We have a few specific to compile-time configurable features: TCG, VNC,
Spice, Slirp, Linux.  Does not apply here.

We have a few specific to targets, such as dump-skeys and info skeys for
s390.  Target-specific is not quite the same as device-specific.

We have no device-specific commands so far.  However, dump-skeys and
info skeys appear to be about the skeys *device*, not the s390 target.
Perhaps any s390 target has such a device?  I don't know.  My point is
we already have device-specific commands, they're just masquerading as
target-specific commands.

The proposed device-specific command uses a mechanism originally made
for modules instead (more on that below).

I think we should make up our minds which way we want device-specific
commands done, then do *all* of them that way.


On to "how to do it", part 1.

Most of the time, the command handler is declared with the command in
hmp-commands{,-info}.hx, possibly with compile-time conditionals.

But it can also be left null there, and set with monitor_register_hmp()
or monitor_register_hmp_info_hrt().  This is intended for modules; see
commit f0e48cbd791^..bca6eb34f03.

Aside: can modules be unloaded?  If yes, we better zap the handler
then.

The proposed "info via" uses monitor_register_hmp_info_hrt().  No
objection from me, requires David's ACK.


"How to do it", part 2, in reply to Philippe's remark.

Ideally, HMP commands wrap around QMP commands, but we accept exceptions
for certain use cases where the wrapping is more trouble than it's
worth, with justification.  I've explained this several times, and I'm
happy to dig up a reference or explain it again if there's a need.

Justifying an exception is bothersome, too.  Daniel Berrangé recently
created a way to reduce the wrapping trouble (merge commit
e86e00a2493).  The proposed "info via" makes use of it.

>> (patch below for context)
>> thanks
>> -- PMM
>> 
>>>
>>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>>> index e90f20a107..4e714e79a2 100644
>>> --- a/hmp-commands-info.hx
>>> +++ b/hmp-commands-info.hx
>>> @@ -879,3 +879,15 @@ SRST
>>>     ``info sgx``
>>>       Show intel SGX information.
>>>   ERST
>>> +
>>> +    {
>>> +        .name       = "via",
>>> +        .args_type  = "",
>>> +        .params     = "",
>>> +        .help       = "show guest 6522 VIA devices",
>>> +    },
>>> +
>>> +SRST
>>> +  ``info via``
>>> +    Show guest 6522 VIA devices.
>>> +ERST

Should this be conditional on the targets where we actually link the
device, like info skeys?

[...]

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
> > On 7/2/22 20:34, Peter Maydell wrote:
> >> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
> >> <mark.cave-ayland@ilande.co.uk> wrote:
> >>>
> >>> This displays detailed information about the device registers and timers to aid
> >>> debugging problems with timers and interrupts.
> >>>
> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>> ---
> >>>   hmp-commands-info.hx | 12 ++++++
> >>>   hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
> >>>   2 files changed, 104 insertions(+)
> >> 
> >> I'm not sure how keen we are on adding new device-specific
> >> HMP info commands, but it's not my area of expertise. Markus ?
> >
> > HMP is David :)
> 
> Yes.

So let me start with an:

Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
(If it's useful info for the author of the device, then I'm happy for
HMP to have that), but then - (moving the reply around a bit):


> Should this be conditional on the targets where we actually link the
> device, like info skeys?
> 

Yes, I think so; it's a reasonably old/obscure device, there's no reason
everyone having it built in.

> >                 IIRC it is OK as long as HMP is a QMP wrapper.
> 
> That's "how to do it", and I'll get back to it in a jiffie, but Peter
> was wondering about the "whether to do it".
> 
> Most HMP commands are always there.
> 
> We have a few specific to compile-time configurable features: TCG, VNC,
> Spice, Slirp, Linux.  Does not apply here.
> 
> We have a few specific to targets, such as dump-skeys and info skeys for
> s390.  Target-specific is not quite the same as device-specific.
> 
> We have no device-specific commands so far.  However, dump-skeys and
> info skeys appear to be about the skeys *device*, not the s390 target.
> Perhaps any s390 target has such a device?  I don't know.  My point is
> we already have device-specific commands, they're just masquerading as
> target-specific commands.

Yeh we've got info lapic/ioapic as well.

> The proposed device-specific command uses a mechanism originally made
> for modules instead (more on that below).
> 
> I think we should make up our minds which way we want device-specific
> commands done, then do *all* of them that way.

I think device specific commands make sense, but I think it would
probably be better if we had an 'info dev $name' and that a method on
the device rather than registering each one separately.
I'd assume that this would be a QMP level thing that got unwrapped at
HMP.

But that's not a problem for this contribution; someone else can figure
that out later.

Dave


> 
> On to "how to do it", part 1.
> 
> Most of the time, the command handler is declared with the command in
> hmp-commands{,-info}.hx, possibly with compile-time conditionals.
> 
> But it can also be left null there, and set with monitor_register_hmp()
> or monitor_register_hmp_info_hrt().  This is intended for modules; see
> commit f0e48cbd791^..bca6eb34f03.
> 
> Aside: can modules be unloaded?  If yes, we better zap the handler
> then.
> 
> The proposed "info via" uses monitor_register_hmp_info_hrt().  No
> objection from me, requires David's ACK.
> 
> 
> "How to do it", part 2, in reply to Philippe's remark.
> 
> Ideally, HMP commands wrap around QMP commands, but we accept exceptions
> for certain use cases where the wrapping is more trouble than it's
> worth, with justification.  I've explained this several times, and I'm
> happy to dig up a reference or explain it again if there's a need.
> 
> Justifying an exception is bothersome, too.  Daniel Berrangé recently
> created a way to reduce the wrapping trouble (merge commit
> e86e00a2493).  The proposed "info via" makes use of it.
> 
> >> (patch below for context)
> >> thanks
> >> -- PMM
> >> 
> >>>
> >>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> >>> index e90f20a107..4e714e79a2 100644
> >>> --- a/hmp-commands-info.hx
> >>> +++ b/hmp-commands-info.hx
> >>> @@ -879,3 +879,15 @@ SRST
> >>>     ``info sgx``
> >>>       Show intel SGX information.
> >>>   ERST
> >>> +
> >>> +    {
> >>> +        .name       = "via",
> >>> +        .args_type  = "",
> >>> +        .params     = "",
> >>> +        .help       = "show guest 6522 VIA devices",
> >>> +    },
> >>> +
> >>> +SRST
> >>> +  ``info via``
> >>> +    Show guest 6522 VIA devices.
> >>> +ERST
> 
> [...]
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Daniel P. Berrangé]

On Thu, Jan 27, 2022 at 08:54:02PM +0000, Mark Cave-Ayland wrote:
> This displays detailed information about the device registers and timers to aid
> debugging problems with timers and interrupts.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hmp-commands-info.hx | 12 ++++++
>  hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index e90f20a107..4e714e79a2 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -879,3 +879,15 @@ SRST
>    ``info sgx``
>      Show intel SGX information.
>  ERST
> +
> +    {
> +        .name       = "via",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show guest 6522 VIA devices",
> +    },
> +
> +SRST
> +  ``info via``
> +    Show guest 6522 VIA devices.
> +ERST
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index aaae195d63..cfa6a9c44b 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -30,6 +30,8 @@
>  #include "hw/misc/mos6522.h"
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
> +#include "monitor/monitor.h"
> +#include "qapi/type-helpers.h"
>  #include "qemu/timer.h"
>  #include "qemu/cutils.h"
>  #include "qemu/log.h"
> @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>      }
>  }
>  
> +static int qmp_x_query_via_foreach(Object *obj, void *opaque)


> +
> +static HumanReadableText *qmp_x_query_via(Error **errp)
> +{
> +    g_autoptr(GString) buf = g_string_new("");
> +
> +    object_child_foreach_recursive(object_get_root(),
> +                                   qmp_x_query_via_foreach, buf);
> +
> +    return human_readable_text_from_str(buf);
> +}

This provides a code handler for a QMP command which is good,
but doesn't ever define the QMP command in the qapi/ schema.


>  static const MemoryRegionOps mos6522_ops = {
>      .read = mos6522_read,
>      .write = mos6522_write,
> @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = {
>  static void mos6522_register_types(void)
>  {
>      type_register_static(&mos6522_type_info);
> +    monitor_register_hmp_info_hrt("via", qmp_x_query_via);

This only registers the HMP counterpart.

The idea of the HumanReadableText handler is that it is calling
a QMP command that is exposed to apps.

>  }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Daniel P. Berrangé]

On Tue, Feb 08, 2022 at 10:29:09AM +0000, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
> > Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> > 
> > > On 7/2/22 20:34, Peter Maydell wrote:
> > >> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
> > >> <mark.cave-ayland@ilande.co.uk> wrote:
> > >>>
> > >>> This displays detailed information about the device registers and timers to aid
> > >>> debugging problems with timers and interrupts.
> > >>>
> > >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > >>> ---
> > >>>   hmp-commands-info.hx | 12 ++++++
> > >>>   hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
> > >>>   2 files changed, 104 insertions(+)
> > >> 
> > >> I'm not sure how keen we are on adding new device-specific
> > >> HMP info commands, but it's not my area of expertise. Markus ?
> > >
> > > HMP is David :)
> > 
> > Yes.
> 
> So let me start with an:
> 
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> (If it's useful info for the author of the device, then I'm happy for
> HMP to have that), but then - (moving the reply around a bit):
> 
> 
> > Should this be conditional on the targets where we actually link the
> > device, like info skeys?
> > 
> 
> Yes, I think so; it's a reasonably old/obscure device, there's no reason
> everyone having it built in.
> 
> > >                 IIRC it is OK as long as HMP is a QMP wrapper.
> > 
> > That's "how to do it", and I'll get back to it in a jiffie, but Peter
> > was wondering about the "whether to do it".
> > 
> > Most HMP commands are always there.
> > 
> > We have a few specific to compile-time configurable features: TCG, VNC,
> > Spice, Slirp, Linux.  Does not apply here.
> > 
> > We have a few specific to targets, such as dump-skeys and info skeys for
> > s390.  Target-specific is not quite the same as device-specific.
> > 
> > We have no device-specific commands so far.  However, dump-skeys and
> > info skeys appear to be about the skeys *device*, not the s390 target.
> > Perhaps any s390 target has such a device?  I don't know.  My point is
> > we already have device-specific commands, they're just masquerading as
> > target-specific commands.
> 
> Yeh we've got info lapic/ioapic as well.
> 
> > The proposed device-specific command uses a mechanism originally made
> > for modules instead (more on that below).
> > 
> > I think we should make up our minds which way we want device-specific
> > commands done, then do *all* of them that way.
> 
> I think device specific commands make sense, but I think it would
> probably be better if we had an 'info dev $name' and that a method on
> the device rather than registering each one separately.
> I'd assume that this would be a QMP level thing that got unwrapped at
> HMP.
> 
> But that's not a problem for this contribution; someone else can figure
> that out later.

Actually I think this would solve a problem with this contribution.
This patch implements a QMP command but never registers it, so it
isn't actually accessible via QMP. It only registers the HMP wrapper
which rather defeats the point of doing it via the QMP HumanReadableText
approach.

I'm guessing this was done because we don't have ability to dynamically
register QMP commands at runtime.  I don't know how hard/easy it is
to address this, and perhaps we don't even want to.  It was a problem
for me when previously converting HMP info commands to QMP and I
didn't get a solution, so didn't convert anything where the command
impl was dynamically registered at runtime. This basically excluded
converting devices that have been split into loadable modules.

If we had a general  'info dev-debug' (HMP) and  'x-query-dev-debug'
commands, then we could side-step the QMP limitation, as both thue
HMP and QMP comamnds could be statically registered. The devices
then only need to register  a callback at runtime which should be
easier to deal with.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

On 08/02/2022 10:29, Dr. David Alan Gilbert wrote:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>
>>> On 7/2/22 20:34, Peter Maydell wrote:
>>>> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
>>>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>>>
>>>>> This displays detailed information about the device registers and timers to aid
>>>>> debugging problems with timers and interrupts.
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>> ---
>>>>>    hmp-commands-info.hx | 12 ++++++
>>>>>    hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>    2 files changed, 104 insertions(+)
>>>>
>>>> I'm not sure how keen we are on adding new device-specific
>>>> HMP info commands, but it's not my area of expertise. Markus ?
>>>
>>> HMP is David :)
>>
>> Yes.
> 
> So let me start with an:
> 
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> (If it's useful info for the author of the device, then I'm happy for
> HMP to have that), but then - (moving the reply around a bit):
> 
> 
>> Should this be conditional on the targets where we actually link the
>> device, like info skeys?
>>
> 
> Yes, I think so; it's a reasonably old/obscure device, there's no reason
> everyone having it built in.

Unfortunately that doesn't seem to work: whilst the target CONFIG_* defines are 
declared when processing hmp-commands-info.hx it seems the the device CONFIG_* 
defines are missing?

>>>                  IIRC it is OK as long as HMP is a QMP wrapper.
>>
>> That's "how to do it", and I'll get back to it in a jiffie, but Peter
>> was wondering about the "whether to do it".
>>
>> Most HMP commands are always there.
>>
>> We have a few specific to compile-time configurable features: TCG, VNC,
>> Spice, Slirp, Linux.  Does not apply here.
>>
>> We have a few specific to targets, such as dump-skeys and info skeys for
>> s390.  Target-specific is not quite the same as device-specific.
>>
>> We have no device-specific commands so far.  However, dump-skeys and
>> info skeys appear to be about the skeys *device*, not the s390 target.
>> Perhaps any s390 target has such a device?  I don't know.  My point is
>> we already have device-specific commands, they're just masquerading as
>> target-specific commands.
> 
> Yeh we've got info lapic/ioapic as well.

... which I suspect is a workaround for only the target CONFIG_* defines being declared.

>> The proposed device-specific command uses a mechanism originally made
>> for modules instead (more on that below).
>>
>> I think we should make up our minds which way we want device-specific
>> commands done, then do *all* of them that way.
> 
> I think device specific commands make sense, but I think it would
> probably be better if we had an 'info dev $name' and that a method on
> the device rather than registering each one separately.
> I'd assume that this would be a QMP level thing that got unwrapped at
> HMP.
> 
> But that's not a problem for this contribution; someone else can figure
> that out later.


ATB,

Mark.

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

On 08/02/2022 11:38, Daniel P. Berrangé wrote:

> On Thu, Jan 27, 2022 at 08:54:02PM +0000, Mark Cave-Ayland wrote:
>> This displays detailed information about the device registers and timers to aid
>> debugging problems with timers and interrupts.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hmp-commands-info.hx | 12 ++++++
>>   hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 104 insertions(+)
>>
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index e90f20a107..4e714e79a2 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -879,3 +879,15 @@ SRST
>>     ``info sgx``
>>       Show intel SGX information.
>>   ERST
>> +
>> +    {
>> +        .name       = "via",
>> +        .args_type  = "",
>> +        .params     = "",
>> +        .help       = "show guest 6522 VIA devices",
>> +    },
>> +
>> +SRST
>> +  ``info via``
>> +    Show guest 6522 VIA devices.
>> +ERST
>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>> index aaae195d63..cfa6a9c44b 100644
>> --- a/hw/misc/mos6522.c
>> +++ b/hw/misc/mos6522.c
>> @@ -30,6 +30,8 @@
>>   #include "hw/misc/mos6522.h"
>>   #include "hw/qdev-properties.h"
>>   #include "migration/vmstate.h"
>> +#include "monitor/monitor.h"
>> +#include "qapi/type-helpers.h"
>>   #include "qemu/timer.h"
>>   #include "qemu/cutils.h"
>>   #include "qemu/log.h"
>> @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>>       }
>>   }
>>   
>> +static int qmp_x_query_via_foreach(Object *obj, void *opaque)
> 
> 
>> +
>> +static HumanReadableText *qmp_x_query_via(Error **errp)
>> +{
>> +    g_autoptr(GString) buf = g_string_new("");
>> +
>> +    object_child_foreach_recursive(object_get_root(),
>> +                                   qmp_x_query_via_foreach, buf);
>> +
>> +    return human_readable_text_from_str(buf);
>> +}
> 
> This provides a code handler for a QMP command which is good,
> but doesn't ever define the QMP command in the qapi/ schema.

First of all, thank you for writing the docs at 
https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writing-a-debugging-aid-returning-unstructured-text 
which were really useful when writing this patch.

I was under the impression that monitor_register_hmp_info_hrt() does all the magic 
here i.e. it declares the underlying QMP command with an x- prefix and effectively 
encapsulates the text field in a way that says "this is an unreliable text opaque for 
humans"?

If a qapi/ schema is needed could you explain what it should look like for this 
example and where it should go? Looking at the existing .json files I can't 
immediately see one which is the right place for this to live.

>>   static const MemoryRegionOps mos6522_ops = {
>>       .read = mos6522_read,
>>       .write = mos6522_write,
>> @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = {
>>   static void mos6522_register_types(void)
>>   {
>>       type_register_static(&mos6522_type_info);
>> +    monitor_register_hmp_info_hrt("via", qmp_x_query_via);
> 
> This only registers the HMP counterpart.
> 
> The idea of the HumanReadableText handler is that it is calling
> a QMP command that is exposed to apps.
> 
>>   }
> 
> Regards,
> Daniel


ATB,

Mark.

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

On 08/02/2022 11:52, Daniel P. Berrangé wrote:

(cut)

>>> The proposed device-specific command uses a mechanism originally made
>>> for modules instead (more on that below).
>>>
>>> I think we should make up our minds which way we want device-specific
>>> commands done, then do *all* of them that way.
>>
>> I think device specific commands make sense, but I think it would
>> probably be better if we had an 'info dev $name' and that a method on
>> the device rather than registering each one separately.
>> I'd assume that this would be a QMP level thing that got unwrapped at
>> HMP.
>>
>> But that's not a problem for this contribution; someone else can figure
>> that out later.
> 
> Actually I think this would solve a problem with this contribution.
> This patch implements a QMP command but never registers it, so it
> isn't actually accessible via QMP. It only registers the HMP wrapper
> which rather defeats the point of doing it via the QMP HumanReadableText
> approach.
> 
> I'm guessing this was done because we don't have ability to dynamically
> register QMP commands at runtime.  I don't know how hard/easy it is
> to address this, and perhaps we don't even want to.  It was a problem
> for me when previously converting HMP info commands to QMP and I
> didn't get a solution, so didn't convert anything where the command
> impl was dynamically registered at runtime. This basically excluded
> converting devices that have been split into loadable modules.
> 
> If we had a general  'info dev-debug' (HMP) and  'x-query-dev-debug'
> commands, then we could side-step the QMP limitation, as both thue
> HMP and QMP comamnds could be statically registered. The devices
> then only need to register  a callback at runtime which should be
> easier to deal with.

That could work, or even just a "debug via" without using "info" for brevity.

You could even add the callback to DeviceClass so that the registration takes place 
as part of class_init() using a function such as device_class_register_debug("name", 
callback).


ATB,

Mark.

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Daniel P. Berrangé]

On Tue, Feb 08, 2022 at 12:39:04PM +0000, Mark Cave-Ayland wrote:
> On 08/02/2022 11:38, Daniel P. Berrangé wrote:
> 
> > On Thu, Jan 27, 2022 at 08:54:02PM +0000, Mark Cave-Ayland wrote:
> > > This displays detailed information about the device registers and timers o aid
> > > debugging problems with timers and interrupts.
> > > 
> > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > ---
> > >   hmp-commands-info.hx | 12 ++++++
> > >   hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 104 insertions(+)
> > > 
> > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > > index e90f20a107..4e714e79a2 100644
> > > --- a/hmp-commands-info.hx
> > > +++ b/hmp-commands-info.hx
> > > @@ -879,3 +879,15 @@ SRST
> > >     ``info sgx``
> > >       Show intel SGX information.
> > >   ERST
> > > +
> > > +    {
> > > +        .name       = "via",
> > > +        .args_type  = "",
> > > +        .params     = "",
> > > +        .help       = "show guest 6522 VIA devices",
> > > +    },
> > > +
> > > +SRST
> > > +  ``info via``
> > > +    Show guest 6522 VIA devices.
> > > +ERST
> > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > > index aaae195d63..cfa6a9c44b 100644
> > > --- a/hw/misc/mos6522.c
> > > +++ b/hw/misc/mos6522.c
> > > @@ -30,6 +30,8 @@
> > >   #include "hw/misc/mos6522.h"
> > >   #include "hw/qdev-properties.h"
> > >   #include "migration/vmstate.h"
> > > +#include "monitor/monitor.h"
> > > +#include "qapi/type-helpers.h"
> > >   #include "qemu/timer.h"
> > >   #include "qemu/cutils.h"
> > >   #include "qemu/log.h"
> > > @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> > >       }
> > >   }
> > > +static int qmp_x_query_via_foreach(Object *obj, void *opaque)
> > 
> > 
> > > +
> > > +static HumanReadableText *qmp_x_query_via(Error **errp)
> > > +{
> > > +    g_autoptr(GString) buf = g_string_new("");
> > > +
> > > +    object_child_foreach_recursive(object_get_root(),
> > > +                                   qmp_x_query_via_foreach, buf);
> > > +
> > > +    return human_readable_text_from_str(buf);
> > > +}
> > 
> > This provides a code handler for a QMP command which is good,
> > but doesn't ever define the QMP command in the qapi/ schema.
> 
> First of all, thank you for writing the docs at https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writing-a-debugging-aid-returning-unstructured-text
> which were really useful when writing this patch.
> 
> I was under the impression that monitor_register_hmp_info_hrt() does all the
> magic here i.e. it declares the underlying QMP command with an x- prefix and
> effectively encapsulates the text field in a way that says "this is an
> unreliable text opaque for humans"?

The monitor_register_hmp_info_hrt only does the HMP glue side, and
that's only needed if you must dynamically register the HMP command.
For statically registered commands set '.cmd_info_hrt' directly in
the hml-commands-info.hx for the HMP side.

> If a qapi/ schema is needed could you explain what it should look like for
> this example and where it should go? Looking at the existing .json files I
> can't immediately see one which is the right place for this to live.

Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
there. The QAPI bit is fairly simple. 

if you want to see an illustration of what's different from a previous
pure HMP impl, look at:

  commit dd98234c059e6bdb05a52998270df6d3d990332e
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Wed Sep 8 10:35:43 2021 +0100

    qapi: introduce x-query-roms QMP command



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Dr. David Alan Gilbert]

* Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote:
> On 08/02/2022 11:52, Daniel P. Berrangé wrote:
> 
> (cut)
> 
> > > > The proposed device-specific command uses a mechanism originally made
> > > > for modules instead (more on that below).
> > > > 
> > > > I think we should make up our minds which way we want device-specific
> > > > commands done, then do *all* of them that way.
> > > 
> > > I think device specific commands make sense, but I think it would
> > > probably be better if we had an 'info dev $name' and that a method on
> > > the device rather than registering each one separately.
> > > I'd assume that this would be a QMP level thing that got unwrapped at
> > > HMP.
> > > 
> > > But that's not a problem for this contribution; someone else can figure
> > > that out later.
> > 
> > Actually I think this would solve a problem with this contribution.
> > This patch implements a QMP command but never registers it, so it
> > isn't actually accessible via QMP. It only registers the HMP wrapper
> > which rather defeats the point of doing it via the QMP HumanReadableText
> > approach.
> > 
> > I'm guessing this was done because we don't have ability to dynamically
> > register QMP commands at runtime.  I don't know how hard/easy it is
> > to address this, and perhaps we don't even want to.  It was a problem
> > for me when previously converting HMP info commands to QMP and I
> > didn't get a solution, so didn't convert anything where the command
> > impl was dynamically registered at runtime. This basically excluded
> > converting devices that have been split into loadable modules.
> > 
> > If we had a general  'info dev-debug' (HMP) and  'x-query-dev-debug'
> > commands, then we could side-step the QMP limitation, as both thue
> > HMP and QMP comamnds could be statically registered. The devices
> > then only need to register  a callback at runtime which should be
> > easier to deal with.
> 
> That could work, or even just a "debug via" without using "info" for brevity.
> 
> You could even add the callback to DeviceClass so that the registration
> takes place as part of class_init() using a function such as
> device_class_register_debug("name", callback).

Yes, that was what I was imagining

Dave

> 
> ATB,
> 
> Mark.
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Dr. David Alan Gilbert]

* Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote:
> On 08/02/2022 10:29, Dr. David Alan Gilbert wrote:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> > > Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> > > 
> > > > On 7/2/22 20:34, Peter Maydell wrote:
> > > > > On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
> > > > > <mark.cave-ayland@ilande.co.uk> wrote:
> > > > > > 
> > > > > > This displays detailed information about the device registers and timers to aid
> > > > > > debugging problems with timers and interrupts.
> > > > > > 
> > > > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > > > > ---
> > > > > >    hmp-commands-info.hx | 12 ++++++
> > > > > >    hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > >    2 files changed, 104 insertions(+)
> > > > > 
> > > > > I'm not sure how keen we are on adding new device-specific
> > > > > HMP info commands, but it's not my area of expertise. Markus ?
> > > > 
> > > > HMP is David :)
> > > 
> > > Yes.
> > 
> > So let me start with an:
> > 
> > Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > (If it's useful info for the author of the device, then I'm happy for
> > HMP to have that), but then - (moving the reply around a bit):
> > 
> > 
> > > Should this be conditional on the targets where we actually link the
> > > device, like info skeys?
> > > 
> > 
> > Yes, I think so; it's a reasonably old/obscure device, there's no reason
> > everyone having it built in.
> 
> Unfortunately that doesn't seem to work: whilst the target CONFIG_* defines
> are declared when processing hmp-commands-info.hx it seems the the device
> CONFIG_* defines are missing?

I'd be happy to take it down to the target level.

Dave

> > > >                  IIRC it is OK as long as HMP is a QMP wrapper.
> > > 
> > > That's "how to do it", and I'll get back to it in a jiffie, but Peter
> > > was wondering about the "whether to do it".
> > > 
> > > Most HMP commands are always there.
> > > 
> > > We have a few specific to compile-time configurable features: TCG, VNC,
> > > Spice, Slirp, Linux.  Does not apply here.
> > > 
> > > We have a few specific to targets, such as dump-skeys and info skeys for
> > > s390.  Target-specific is not quite the same as device-specific.
> > > 
> > > We have no device-specific commands so far.  However, dump-skeys and
> > > info skeys appear to be about the skeys *device*, not the s390 target.
> > > Perhaps any s390 target has such a device?  I don't know.  My point is
> > > we already have device-specific commands, they're just masquerading as
> > > target-specific commands.
> > 
> > Yeh we've got info lapic/ioapic as well.
> 
> ... which I suspect is a workaround for only the target CONFIG_* defines being declared.
> 
> > > The proposed device-specific command uses a mechanism originally made
> > > for modules instead (more on that below).
> > > 
> > > I think we should make up our minds which way we want device-specific
> > > commands done, then do *all* of them that way.
> > 
> > I think device specific commands make sense, but I think it would
> > probably be better if we had an 'info dev $name' and that a method on
> > the device rather than registering each one separately.
> > I'd assume that this would be a QMP level thing that got unwrapped at
> > HMP.
> > 
> > But that's not a problem for this contribution; someone else can figure
> > that out later.
> 
> 
> ATB,
> 
> Mark.
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

On 08/02/2022 12:49, Daniel P. Berrangé wrote:

>> I was under the impression that monitor_register_hmp_info_hrt() does all the
>> magic here i.e. it declares the underlying QMP command with an x- prefix and
>> effectively encapsulates the text field in a way that says "this is an
>> unreliable text opaque for humans"?
> 
> The monitor_register_hmp_info_hrt only does the HMP glue side, and
> that's only needed if you must dynamically register the HMP command.
> For statically registered commands set '.cmd_info_hrt' directly in
> the hml-commands-info.hx for the HMP side.
> 
>> If a qapi/ schema is needed could you explain what it should look like for
>> this example and where it should go? Looking at the existing .json files I
>> can't immediately see one which is the right place for this to live.
> 
> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
> there. The QAPI bit is fairly simple.
> 
> if you want to see an illustration of what's different from a previous
> pure HMP impl, look at:
> 
>    commit dd98234c059e6bdb05a52998270df6d3d990332e
>    Author: Daniel P. Berrangé <berrange@redhat.com>
>    Date:   Wed Sep 8 10:35:43 2021 +0100
> 
>      qapi: introduce x-query-roms QMP command

I see, thanks for the reference. So qapi/machine.json would be the right place to 
declare the QMP part even for a specific device?

Even this approach still wouldn't work in its current form though, since as mentioned 
in my previous email it seems that only the target CONFIG_* defines and not the 
device CONFIG_* defines are present when processing hmp-commands-info.hx.


ATB,

Mark.

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Daniel P. Berrangé]

On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
> 
> > > I was under the impression that monitor_register_hmp_info_hrt() does all the
> > > magic here i.e. it declares the underlying QMP command with an x- prefix and
> > > effectively encapsulates the text field in a way that says "this is an
> > > unreliable text opaque for humans"?
> > 
> > The monitor_register_hmp_info_hrt only does the HMP glue side, and
> > that's only needed if you must dynamically register the HMP command.
> > For statically registered commands set '.cmd_info_hrt' directly in
> > the hml-commands-info.hx for the HMP side.
> > 
> > > If a qapi/ schema is needed could you explain what it should look like for
> > > this example and where it should go? Looking at the existing .json files I
> > > can't immediately see one which is the right place for this to live.
> > 
> > Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
> > there. The QAPI bit is fairly simple.
> > 
> > if you want to see an illustration of what's different from a previous
> > pure HMP impl, look at:
> > 
> >    commit dd98234c059e6bdb05a52998270df6d3d990332e
> >    Author: Daniel P. Berrangé <berrange@redhat.com>
> >    Date:   Wed Sep 8 10:35:43 2021 +0100
> > 
> >      qapi: introduce x-query-roms QMP command
> 
> I see, thanks for the reference. So qapi/machine.json would be the right
> place to declare the QMP part even for a specific device?
> 
> Even this approach still wouldn't work in its current form though, since as
> mentioned in my previous email it seems that only the target CONFIG_*
> defines and not the device CONFIG_* defines are present when processing
> hmp-commands-info.hx.

Yeah, that's where the pain comes in.  While QAPI schema can be made
conditional on a few CONFIG_* parameters - basically those derived
from global configure time options, it is impossible for this to be
with with target specific options like the device CONFIG_* defines.

This is why I suggested in my othuer reply that it would need to be
done with a generic 'info dev-debug' / 'x-query-dev-debug' command
that can be registered unconditionally, and then individual devices
plug into it.



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Feb 08, 2022 at 10:29:09AM +0000, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (armbru@redhat.com) wrote:
>> > Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> > 
>> > > On 7/2/22 20:34, Peter Maydell wrote:
>> > >> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
>> > >> <mark.cave-ayland@ilande.co.uk> wrote:
>> > >>>
>> > >>> This displays detailed information about the device registers and timers to aid
>> > >>> debugging problems with timers and interrupts.
>> > >>>
>> > >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> > >>> ---
>> > >>>   hmp-commands-info.hx | 12 ++++++
>> > >>>   hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
>> > >>>   2 files changed, 104 insertions(+)
>> > >> 
>> > >> I'm not sure how keen we are on adding new device-specific
>> > >> HMP info commands, but it's not my area of expertise. Markus ?
>> > >
>> > > HMP is David :)
>> > 
>> > Yes.
>> 
>> So let me start with an:
>> 
>> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> (If it's useful info for the author of the device, then I'm happy for
>> HMP to have that), but then - (moving the reply around a bit):
>> 
>> 
>> > Should this be conditional on the targets where we actually link the
>> > device, like info skeys?
>> > 
>> 
>> Yes, I think so; it's a reasonably old/obscure device, there's no reason
>> everyone having it built in.
>> 
>> > >                 IIRC it is OK as long as HMP is a QMP wrapper.
>> > 
>> > That's "how to do it", and I'll get back to it in a jiffie, but Peter
>> > was wondering about the "whether to do it".
>> > 
>> > Most HMP commands are always there.
>> > 
>> > We have a few specific to compile-time configurable features: TCG, VNC,
>> > Spice, Slirp, Linux.  Does not apply here.
>> > 
>> > We have a few specific to targets, such as dump-skeys and info skeys for
>> > s390.  Target-specific is not quite the same as device-specific.
>> > 
>> > We have no device-specific commands so far.  However, dump-skeys and
>> > info skeys appear to be about the skeys *device*, not the s390 target.
>> > Perhaps any s390 target has such a device?  I don't know.  My point is
>> > we already have device-specific commands, they're just masquerading as
>> > target-specific commands.
>> 
>> Yeh we've got info lapic/ioapic as well.
>> 
>> > The proposed device-specific command uses a mechanism originally made
>> > for modules instead (more on that below).
>> > 
>> > I think we should make up our minds which way we want device-specific
>> > commands done, then do *all* of them that way.
>> 
>> I think device specific commands make sense, but I think it would
>> probably be better if we had an 'info dev $name' and that a method on
>> the device rather than registering each one separately.
>> I'd assume that this would be a QMP level thing that got unwrapped at
>> HMP.
>> 
>> But that's not a problem for this contribution; someone else can figure
>> that out later.
>
> Actually I think this would solve a problem with this contribution.
> This patch implements a QMP command but never registers it, so it
> isn't actually accessible via QMP. It only registers the HMP wrapper
> which rather defeats the point of doing it via the QMP HumanReadableText
> approach.
>
> I'm guessing this was done because we don't have ability to dynamically
> register QMP commands at runtime.

Correct.  I pointed this out in review, actually:

    Message-ID: <87tuk1k0de.fsf@dusky.pond.sub.org>
    https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg01236.html

Gerd replied "when going forward with building more code modular we
might need this for qmp too at some point".

>                                    I don't know how hard/easy it is
> to address this, and perhaps we don't even want to.

Depends on requirements.

Say all we want is "command fails unless the module is loaded" (or
whatever the condition is).  Should be fairly easy.

But if we want introspection to reflect the state of things, that's
harder.

>                                                      It was a problem
> for me when previously converting HMP info commands to QMP and I
> didn't get a solution, so didn't convert anything where the command
> impl was dynamically registered at runtime. This basically excluded
> converting devices that have been split into loadable modules.

Understand; you series was big enough as it was.

> If we had a general  'info dev-debug' (HMP) and  'x-query-dev-debug'
> commands, then we could side-step the QMP limitation, as both thue
> HMP and QMP comamnds could be statically registered. The devices
> then only need to register  a callback at runtime which should be
> easier to deal with.

I like it.

Re: [PATCH 01/11] mos6522: add defines for IFR bit flags

[Mark Cave-Ayland]

On 05/02/2022 12:06, BALATON Zoltan wrote:

> On Sat, 5 Feb 2022, Philippe Mathieu-Daudé wrote:
>> On 5/2/22 11:51, Mark Cave-Ayland wrote:
>>> On 27/01/2022 23:16, BALATON Zoltan wrote:
>>>
>>>> On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
>>>>> These are intended to make it easier to see how the physical control lines
>>>>> are wired for each instance.
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>> ---
>>>>> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
>>>>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>>>>> index fc95d22b0f..12abd8b8d2 100644
>>>>> --- a/include/hw/misc/mos6522.h
>>>>> +++ b/include/hw/misc/mos6522.h
>>>>> @@ -41,13 +41,21 @@
>>>>> #define IER_SET            0x80    /* set bits in IER */
>>>>> #define IER_CLR            0       /* clear bits in IER */
>>>>>
>>>>> -#define CA2_INT            0x01
>>>>> -#define CA1_INT            0x02
>>>>> -#define SR_INT             0x04    /* Shift register full/empty */
>>>>> -#define CB2_INT            0x08
>>>>> -#define CB1_INT            0x10
>>>>> -#define T2_INT             0x20    /* Timer 2 interrupt */
>>>>> -#define T1_INT             0x40    /* Timer 1 interrupt */
>>>>> +#define CA2_INT_BIT        0
>>>>> +#define CA1_INT_BIT        1
>>>>> +#define SR_INT_BIT         2       /* Shift register full/empty */
>>>>> +#define CB2_INT_BIT        3
>>>>> +#define CB1_INT_BIT        4
>>>>> +#define T2_INT_BIT         5       /* Timer 2 interrupt */
>>>>> +#define T1_INT_BIT         6       /* Timer 1 interrupt */
>>>>> +
>>>>> +#define CA2_INT            (1 << CA2_INT_BIT)
>>>>> +#define CA1_INT            (1 << CA1_INT_BIT)
>>>>> +#define SR_INT             (1 << SR_INT_BIT)
>>>>> +#define CB2_INT            (1 << CB2_INT_BIT)
>>>>> +#define CB1_INT            (1 << CB1_INT_BIT)
>>>>> +#define T2_INT             (1 << T2_INT_BIT)
>>>>> +#define T1_INT             (1 << T1_INT_BIT)
>>>>
>>>> Maybe you could leave the #defines called XX_INT and then use BIT(XX_INT) instead 
>>>> of the second set of #defines which would provide same readability but with less 
>>>> #defines needed.
>>>
>>> I'm not so keen on removing the _INT defines since that would require updating all 
>>> users to use BIT() everywhere which I don't think gains us much. I could certainly 
>>> replace these definitions with BIT(FOO) instead of (1 << FOO) if that helps 
>>> readability though.
>>
>> Do you mean simply doing this?
>>
>> -#define T1_INT             0x40    /* Timer 1 interrupt */
>> +#define T1_INT             BIT(6)  /* Timer 1 interrupt */
> 
> I meant:
> 
> #define T1_INT 6
> 
> and then replace current usage of T1_INT with BIT(T1_INT) that way we don't need both 
> T1_INT_BIT and T1_INT defines which seems redundant as BIT(T1_INT) is not much longer 
> and still clear what it refers to. It's true that this needs more changes but the 
> result is more readable IMO than introducing another set of defines that ome has to 
> look up when encounters them as the meaning might not be clear. That's why I think 
> one set of defines for bit numbers and using existing BIT(num) for values is better 
> but it's just an idea, I don't care that much.

I think the best solution here is to just use BIT() for the final shifted values like 
this:

#define CA2_INT_BIT        0
...
...
#define CA2_INT            BIT(CA2_INT_BIT)

Otherwise I can see there being confusion given that the BIT() macro is used for 
defines without a _BIT suffix which are also being used elsewhere. I'll update this 
in v2 accordingly.


ATB,

Mark.

Re: [PATCH 04/11] mos6522: switch over to use qdev gpios for IRQs

[Mark Cave-Ayland]

On 07/02/2022 19:29, Peter Maydell wrote:

> On Thu, 27 Jan 2022 at 21:01, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> For historical reasons each mos6522 instance implements its own setting and
>> update of the IFR flag bits using methods exposed by MOS6522DeviceClass. As
>> of today this is no longer required, and it is now possible to implement
>> the mos6522 IRQs as standard qdev gpios.
>>
>> Switch over to use qdev gpios for the mos6522 device and update all instances
>> accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/misc/mac_via.c         | 56 +++++++--------------------------------
>>   hw/misc/macio/cuda.c      |  5 ++--
>>   hw/misc/macio/pmu.c       |  4 +--
>>   hw/misc/mos6522.c         | 15 +++++++++++
>>   include/hw/misc/mac_via.h |  6 +----
>>   include/hw/misc/mos6522.h |  2 ++
>>   6 files changed, 32 insertions(+), 56 deletions(-)
> 
> 
>> -static void via2_nubus_irq_request(void *opaque, int irq, int level)
>> +static void via2_nubus_irq_request(void *opaque, int n, int level)
>>   {
>>       MOS6522Q800VIA2State *v2s = opaque;
>>       MOS6522State *s = MOS6522(v2s);
>> -    MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
>> +    qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA2_IRQ_NUBUS_BIT);
>>
>>       if (level) {
>>           /* Port A nubus IRQ inputs are active LOW */
>> -        s->a &= ~(1 << irq);
>> -        s->ifr |= 1 << VIA2_IRQ_NUBUS_BIT;
>> +        s->a &= ~(1 << n);
>>       } else {
>> -        s->a |= (1 << irq);
>> -        s->ifr &= ~(1 << VIA2_IRQ_NUBUS_BIT);
>> +        s->a |= (1 << n);
>>       }
>>
>> -    mdc->update_irq(s);
>> +    qemu_set_irq(irq, level);
>>   }
> 
> It feels a bit inconsistent here that we're still reaching into
> the MOS6522State to set s->a, but I guess this is still
> better than what we had before.

Yeah it's a little bit messy. On the Mac the Nubus IRQs are ORd to produce the VIA 
interrupt line, but the individual Nubus IRQ lines are wired to the VIA port A to 
allow the state of each line to be determined. The VIA port IO lines are 
bi-directional, so rather than try and implement this with GPIOs it's easiest to have 
a separate VIA2 Nubus GPIOs that also handle the port IO lines as above.

>> -#define VIA1_IRQ_NB             8
>> -
>>   #define VIA1_IRQ_ONE_SECOND     (1 << VIA1_IRQ_ONE_SECOND_BIT)
>>   #define VIA1_IRQ_60HZ           (1 << VIA1_IRQ_60HZ_BIT)
>>   #define VIA1_IRQ_ADB_READY      (1 << VIA1_IRQ_ADB_READY_BIT)
>> @@ -42,7 +40,7 @@ struct MOS6522Q800VIA1State {
>>
>>       MemoryRegion via_mem;
>>
>> -    qemu_irq irqs[VIA1_IRQ_NB];
>> +    qemu_irq irqs[VIA_NUM_INTS];
> 
> This irqs[] array appears to be entirely unused. You could
> delete it as a separate patch before this one.

Ah yes, before this patch each VIA had its own GPIOs which used the methods in 
MOS6522DeviceClass to manipulate the IFR bit. Since the Q800 VIAs have the MOS6522 
device as a parent class, the GPIOs are inherited from that and so this array should 
actually be removed as part of this patch.

>>       qemu_irq auxmode_irq;
>>       uint8_t last_b;
>>
>> @@ -85,8 +83,6 @@ struct MOS6522Q800VIA1State {
>>   #define VIA2_IRQ_SCSI_BIT       CB2_INT_BIT
>>   #define VIA2_IRQ_ASC_BIT        CB1_INT_BIT
>>
>> -#define VIA2_IRQ_NB             8
>> -
>>   #define VIA2_IRQ_SCSI_DATA      (1 << VIA2_IRQ_SCSI_DATA_BIT)
>>   #define VIA2_IRQ_NUBUS          (1 << VIA2_IRQ_NUBUS_BIT)
>>   #define VIA2_IRQ_UNUSED         (1 << VIA2_IRQ_SCSI_BIT)
>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>> index 12abd8b8d2..ced8a670bf 100644
>> --- a/include/hw/misc/mos6522.h
>> +++ b/include/hw/misc/mos6522.h
>> @@ -57,6 +57,8 @@
>>   #define T2_INT             (1 << T2_INT_BIT)
>>   #define T1_INT             (1 << T1_INT_BIT)
>>
>> +#define VIA_NUM_INTS       5
> 
> Were we not using 5,6,7 previously ?

5 and 6 are for the in-built timers, and 7 is a set/clear flag so they are not 
externally accessible.

> Anyway,
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM


ATB,

Mark.

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

On 08/02/2022 13:10, Daniel P. Berrangé wrote:

> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
>> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>>
>>>> I was under the impression that monitor_register_hmp_info_hrt() does all the
>>>> magic here i.e. it declares the underlying QMP command with an x- prefix and
>>>> effectively encapsulates the text field in a way that says "this is an
>>>> unreliable text opaque for humans"?
>>>
>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and
>>> that's only needed if you must dynamically register the HMP command.
>>> For statically registered commands set '.cmd_info_hrt' directly in
>>> the hml-commands-info.hx for the HMP side.
>>>
>>>> If a qapi/ schema is needed could you explain what it should look like for
>>>> this example and where it should go? Looking at the existing .json files I
>>>> can't immediately see one which is the right place for this to live.
>>>
>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
>>> there. The QAPI bit is fairly simple.
>>>
>>> if you want to see an illustration of what's different from a previous
>>> pure HMP impl, look at:
>>>
>>>     commit dd98234c059e6bdb05a52998270df6d3d990332e
>>>     Author: Daniel P. Berrangé <berrange@redhat.com>
>>>     Date:   Wed Sep 8 10:35:43 2021 +0100
>>>
>>>       qapi: introduce x-query-roms QMP command
>>
>> I see, thanks for the reference. So qapi/machine.json would be the right
>> place to declare the QMP part even for a specific device?
>>
>> Even this approach still wouldn't work in its current form though, since as
>> mentioned in my previous email it seems that only the target CONFIG_*
>> defines and not the device CONFIG_* defines are present when processing
>> hmp-commands-info.hx.
> 
> Yeah, that's where the pain comes in.  While QAPI schema can be made
> conditional on a few CONFIG_* parameters - basically those derived
> from global configure time options, it is impossible for this to be
> with with target specific options like the device CONFIG_* defines.
> 
> This is why I suggested in my othuer reply that it would need to be
> done with a generic 'info dev-debug' / 'x-query-dev-debug' command
> that can be registered unconditionally, and then individual devices
> plug into it.

After some more experiments this afternoon I still seem to be falling through the 
gaps on this one. This is based upon my understanding that all new HMP commands 
should use a QMP HumanReadableText implementation and the new command should be 
restricted according to target.

Currently I am working with this change to hmp-commands-info.hx and 
qapi/misc-target.json:


diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 7e6bd30395..aac86d5473 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -880,15 +880,15 @@ SRST
      Show intel SGX information.
  ERST

#if defined(TARGET_M68K) || defined(TARGET_PPC)
      {
          .name         = "via",
          .args_type    = "",
          .params       = "",
          .help         = "show guest mos6522 VIA devices",
          .cmd_info_hrt = qmp_x_query_via,
      },
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 4bc45d2474..72bf71888e 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -2,6 +2,8 @@
  # vim: filetype=python
  #

+{ 'include': 'common.json' }
+
  ##
  # @RTC_CHANGE:
  #
@@ -424,3 +426,19 @@
  #
  ##
  { 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 'TARGET_I386' }
+
+##
+# @x-query-via:
+#
+# Query information on the registered mos6522 VIAs
+#
+# Features:
+# @unstable: This command is meant for debugging.
+#
+# Returns: internal mos6522 information
+#
+# Since: 7.0
+##
+{ 'command': 'x-query-via',
+  'returns': 'HumanReadableText',
+  'features': [ 'unstable' ], 'if': { 'any': [ 'TARGET_M68K', 'TARGET_PPC' ] } }


The main problem with trying to restrict the new command to a target (or targets) is 
that the autogenerated qapi/qapi-commands-misc-target.h QAPI header cannot be 
included in a device header such as mos6522.h without getting poison errors like 
below (which does actually make sense):


In file included from ./qapi/qapi-commands-misc-target.h:17,
                  from /home/build/src/qemu/git/qemu/include/hw/misc/mos6522.h:35,
                  from ../hw/misc/mos6522.c:30:
./qapi/qapi-types-misc-target.h:19:13: error: attempt to use poisoned "TARGET_ALPHA"


I can work around that by declaring the prototype for qmp_x_query_via() manually i.e.:


diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index 9c21da2ddd..9677293ad0 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -30,7 +30,7 @@
  #include "exec/memory.h"
  #include "hw/sysbus.h"
  #include "hw/input/adb.h"
+#include "qapi/qapi-commands-common.h"
  #include "qom/object.h"

  /* Bits in ACR */
@@ -156,4 +156,6 @@ extern const VMStateDescription vmstate_mos6522;
  uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size);
  void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size);

+HumanReadableText *qmp_x_query_via(Error **errp);
+
  #endif /* MOS6522_H */


This works fine for targets where CONFIG_MOS6522 is defined, but if I try a target 
such as x86_64 where the device isn't used then I hit another compilation error:


qapi/qapi-commands-misc-target.c:598:13: error: 
‘qmp_marshal_output_HumanReadableText’ defined but not used [-Werror=unused-function]
  static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in,
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


Looking at the generated qapi/qapi-commands-misc-target.c file in question I see this:


static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in,
                                 QObject **ret_out, Error **errp)
{
     Visitor *v;

     v = qobject_output_visitor_new_qmp(ret_out);
     if (visit_type_HumanReadableText(v, "unused", &ret_in, errp)) {
         visit_complete(v, ret_out);
     }
     visit_free(v);
     v = qapi_dealloc_visitor_new();
     visit_type_HumanReadableText(v, "unused", &ret_in, NULL);
     visit_free(v);
}

#if defined(TARGET_M68K) || defined(TARGET_PPC)
void qmp_marshal_x_query_via(QDict *args, QObject **ret, Error **errp)
{
     ...
     ...
}
#endif

i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if TARGET guards 
and since HumanReadableText is only used by the new qmp_x_query_via() functionality 
then the compiler complains and aborts the compilation.

Possibly this is an error in the QAPI generator for types hidden behind commands 
using "if"? Otherwise I'm not sure what is the best way to proceed, so I'd be 
grateful for some further pointers.


ATB,

Mark.

Re: [PATCH 01/11] mos6522: add defines for IFR bit flags

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 5258 bytes --]

On Sun, 20 Feb 2022, Mark Cave-Ayland wrote:
> On 05/02/2022 12:06, BALATON Zoltan wrote:
>> On Sat, 5 Feb 2022, Philippe Mathieu-Daudé wrote:
>>> On 5/2/22 11:51, Mark Cave-Ayland wrote:
>>>> On 27/01/2022 23:16, BALATON Zoltan wrote:
>>>> 
>>>>> On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
>>>>>> These are intended to make it easier to see how the physical control 
>>>>>> lines
>>>>>> are wired for each instance.
>>>>>> 
>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>> ---
>>>>>> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
>>>>>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>> 
>>>>>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>>>>>> index fc95d22b0f..12abd8b8d2 100644
>>>>>> --- a/include/hw/misc/mos6522.h
>>>>>> +++ b/include/hw/misc/mos6522.h
>>>>>> @@ -41,13 +41,21 @@
>>>>>> #define IER_SET            0x80    /* set bits in IER */
>>>>>> #define IER_CLR            0       /* clear bits in IER */
>>>>>> 
>>>>>> -#define CA2_INT            0x01
>>>>>> -#define CA1_INT            0x02
>>>>>> -#define SR_INT             0x04    /* Shift register full/empty */
>>>>>> -#define CB2_INT            0x08
>>>>>> -#define CB1_INT            0x10
>>>>>> -#define T2_INT             0x20    /* Timer 2 interrupt */
>>>>>> -#define T1_INT             0x40    /* Timer 1 interrupt */
>>>>>> +#define CA2_INT_BIT        0
>>>>>> +#define CA1_INT_BIT        1
>>>>>> +#define SR_INT_BIT         2       /* Shift register full/empty */
>>>>>> +#define CB2_INT_BIT        3
>>>>>> +#define CB1_INT_BIT        4
>>>>>> +#define T2_INT_BIT         5       /* Timer 2 interrupt */
>>>>>> +#define T1_INT_BIT         6       /* Timer 1 interrupt */
>>>>>> +
>>>>>> +#define CA2_INT            (1 << CA2_INT_BIT)
>>>>>> +#define CA1_INT            (1 << CA1_INT_BIT)
>>>>>> +#define SR_INT             (1 << SR_INT_BIT)
>>>>>> +#define CB2_INT            (1 << CB2_INT_BIT)
>>>>>> +#define CB1_INT            (1 << CB1_INT_BIT)
>>>>>> +#define T2_INT             (1 << T2_INT_BIT)
>>>>>> +#define T1_INT             (1 << T1_INT_BIT)
>>>>> 
>>>>> Maybe you could leave the #defines called XX_INT and then use 
>>>>> BIT(XX_INT) instead of the second set of #defines which would provide 
>>>>> same readability but with less #defines needed.
>>>> 
>>>> I'm not so keen on removing the _INT defines since that would require 
>>>> updating all users to use BIT() everywhere which I don't think gains us 
>>>> much. I could certainly replace these definitions with BIT(FOO) instead 
>>>> of (1 << FOO) if that helps readability though.
>>> 
>>> Do you mean simply doing this?
>>> 
>>> -#define T1_INT             0x40    /* Timer 1 interrupt */
>>> +#define T1_INT             BIT(6)  /* Timer 1 interrupt */
>> 
>> I meant:
>> 
>> #define T1_INT 6
>> 
>> and then replace current usage of T1_INT with BIT(T1_INT) that way we don't 
>> need both T1_INT_BIT and T1_INT defines which seems redundant as 
>> BIT(T1_INT) is not much longer and still clear what it refers to. It's true 
>> that this needs more changes but the result is more readable IMO than 
>> introducing another set of defines that ome has to look up when encounters 
>> them as the meaning might not be clear. That's why I think one set of 
>> defines for bit numbers and using existing BIT(num) for values is better 
>> but it's just an idea, I don't care that much.
>
> I think the best solution here is to just use BIT() for the final shifted 
> values like this:
>
> #define CA2_INT_BIT        0
> ...
> ...
> #define CA2_INT            BIT(CA2_INT_BIT)

That does not really help much as the idea was to avoid having two set of 
defines and only have one set for the bit numbers then use the BIT() macro 
instead of the current values. Using the BIT() macro in the second set of 
defines does not help reduce the number of defines in code which the 
reader will have to look up in this header. IMO having defines only for 
bit numbers and always using BIT(whatever) for values is less confusing 
assuming one is familiar with what the BIT() macro does.

> Otherwise I can see there being confusion given that the BIT() macro is used 
> for defines without a _BIT suffix which are also being used elsewhere.

Maybe it's only confusing to you as you've named the bit numbers *_BIT and 
the values without BIT and my proposal was to name the bit numbers as the 
simple names and use BIT(name) for the value which looks kind of opposite 
naming but it's the simplest. I guess you could also have bit numbers 
named *_BIT and then use BIT(CA2_INT_BIT) instead of the second set of 
defines for the values but that looks a bit redundant and maybe more 
confusing than just using BIT(CA2_INT).

> I'll update this in v2 accordingly.

I don't have a strong opinion on this so if you prefer the way it is now 
or using the BIT() macro only for the separate defines then do that. I 
don't mind either way.

Regards,
BALATON Zoltan

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Philippe Mathieu-Daudé]

+Thomas

On 20/2/22 18:18, Mark Cave-Ayland wrote:
> On 08/02/2022 13:10, Daniel P. Berrangé wrote:
> 
>> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
>>> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>>>
>>>>> I was under the impression that monitor_register_hmp_info_hrt() 
>>>>> does all the
>>>>> magic here i.e. it declares the underlying QMP command with an x- 
>>>>> prefix and
>>>>> effectively encapsulates the text field in a way that says "this is an
>>>>> unreliable text opaque for humans"?
>>>>
>>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and
>>>> that's only needed if you must dynamically register the HMP command.
>>>> For statically registered commands set '.cmd_info_hrt' directly in
>>>> the hml-commands-info.hx for the HMP side.
>>>>
>>>>> If a qapi/ schema is needed could you explain what it should look 
>>>>> like for
>>>>> this example and where it should go? Looking at the existing .json 
>>>>> files I
>>>>> can't immediately see one which is the right place for this to live.
>>>>
>>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
>>>> there. The QAPI bit is fairly simple.
>>>>
>>>> if you want to see an illustration of what's different from a previous
>>>> pure HMP impl, look at:
>>>>
>>>>     commit dd98234c059e6bdb05a52998270df6d3d990332e
>>>>     Author: Daniel P. Berrangé <berrange@redhat.com>
>>>>     Date:   Wed Sep 8 10:35:43 2021 +0100
>>>>
>>>>       qapi: introduce x-query-roms QMP command
>>>
>>> I see, thanks for the reference. So qapi/machine.json would be the right
>>> place to declare the QMP part even for a specific device?
>>>
>>> Even this approach still wouldn't work in its current form though, 
>>> since as
>>> mentioned in my previous email it seems that only the target CONFIG_*
>>> defines and not the device CONFIG_* defines are present when processing
>>> hmp-commands-info.hx.
>>
>> Yeah, that's where the pain comes in.  While QAPI schema can be made
>> conditional on a few CONFIG_* parameters - basically those derived
>> from global configure time options, it is impossible for this to be
>> with with target specific options like the device CONFIG_* defines.
>>
>> This is why I suggested in my othuer reply that it would need to be
>> done with a generic 'info dev-debug' / 'x-query-dev-debug' command
>> that can be registered unconditionally, and then individual devices
>> plug into it.
> 
> After some more experiments this afternoon I still seem to be falling 
> through the gaps on this one. This is based upon my understanding that 
> all new HMP commands should use a QMP HumanReadableText implementation 
> and the new command should be restricted according to target.
> 
> Currently I am working with this change to hmp-commands-info.hx and 
> qapi/misc-target.json:
> 
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 7e6bd30395..aac86d5473 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -880,15 +880,15 @@ SRST
>       Show intel SGX information.
>   ERST
> 
> #if defined(TARGET_M68K) || defined(TARGET_PPC)
>       {
>           .name         = "via",
>           .args_type    = "",
>           .params       = "",
>           .help         = "show guest mos6522 VIA devices",
>           .cmd_info_hrt = qmp_x_query_via,
>       },
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 4bc45d2474..72bf71888e 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -2,6 +2,8 @@
>   # vim: filetype=python
>   #
> 
> +{ 'include': 'common.json' }
> +
>   ##
>   # @RTC_CHANGE:
>   #
> @@ -424,3 +426,19 @@
>   #
>   ##
>   { 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 
> 'TARGET_I386' }
> +
> +##
> +# @x-query-via:
> +#
> +# Query information on the registered mos6522 VIAs
> +#
> +# Features:
> +# @unstable: This command is meant for debugging.
> +#
> +# Returns: internal mos6522 information
> +#
> +# Since: 7.0
> +##
> +{ 'command': 'x-query-via',
> +  'returns': 'HumanReadableText',
> +  'features': [ 'unstable' ], 'if': { 'any': [ 'TARGET_M68K', 
> 'TARGET_PPC' ] } }
> 
> 
> The main problem with trying to restrict the new command to a target (or 
> targets) is that the autogenerated qapi/qapi-commands-misc-target.h QAPI 
> header cannot be included in a device header such as mos6522.h without 
> getting poison errors like below (which does actually make sense):
> 
> 
> In file included from ./qapi/qapi-commands-misc-target.h:17,
>                   from 
> /home/build/src/qemu/git/qemu/include/hw/misc/mos6522.h:35,
>                   from ../hw/misc/mos6522.c:30:
> ./qapi/qapi-types-misc-target.h:19:13: error: attempt to use poisoned 
> "TARGET_ALPHA"

Can be kludged by making this device target-specific:

-- >8 --
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 6dcbe044f3..65837b1dfe 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -23 +23 @@ softmmu_ss.add(when: 'CONFIG_ARM_V7M', if_true: 
files('armv7m_ras.c'))
-softmmu_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
+specific_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
---

I'd rather keep devices generic, but sometime we can't. In this case
VIA is only used on m68k so it could be acceptable.

> I can work around that by declaring the prototype for qmp_x_query_via() 
> manually i.e.:
> 
> 
> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
> index 9c21da2ddd..9677293ad0 100644
> --- a/include/hw/misc/mos6522.h
> +++ b/include/hw/misc/mos6522.h
> @@ -30,7 +30,7 @@
>   #include "exec/memory.h"
>   #include "hw/sysbus.h"
>   #include "hw/input/adb.h"
> +#include "qapi/qapi-commands-common.h"
>   #include "qom/object.h"
> 
>   /* Bits in ACR */
> @@ -156,4 +156,6 @@ extern const VMStateDescription vmstate_mos6522;
>   uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size);
>   void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned 
> size);
> 
> +HumanReadableText *qmp_x_query_via(Error **errp);
> +
>   #endif /* MOS6522_H */
> 
> 
> This works fine for targets where CONFIG_MOS6522 is defined, but if I 
> try a target such as x86_64 where the device isn't used then I hit 
> another compilation error:
> 
> 
> qapi/qapi-commands-misc-target.c:598:13: error: 
> ‘qmp_marshal_output_HumanReadableText’ defined but not used 
> [-Werror=unused-function]
>   static void qmp_marshal_output_HumanReadableText(HumanReadableText 
> *ret_in,
>               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> 
> Looking at the generated qapi/qapi-commands-misc-target.c file in 
> question I see this:
> 
> 
> static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in,
>                                  QObject **ret_out, Error **errp)
> {
>      Visitor *v;
> 
>      v = qobject_output_visitor_new_qmp(ret_out);
>      if (visit_type_HumanReadableText(v, "unused", &ret_in, errp)) {
>          visit_complete(v, ret_out);
>      }
>      visit_free(v);
>      v = qapi_dealloc_visitor_new();
>      visit_type_HumanReadableText(v, "unused", &ret_in, NULL);
>      visit_free(v);
> }
> 
> #if defined(TARGET_M68K) || defined(TARGET_PPC)
> void qmp_marshal_x_query_via(QDict *args, QObject **ret, Error **errp)
> {
>      ...
>      ...
> }
> #endif
> 
> i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if 
> TARGET guards and since HumanReadableText is only used by the new 
> qmp_x_query_via() functionality then the compiler complains and aborts 
> the compilation.
> 
> Possibly this is an error in the QAPI generator for types hidden behind 
> commands using "if"? Otherwise I'm not sure what is the best way to 
> proceed, so I'd be grateful for some further pointers.
> 
> 
> ATB,
> 
> Mark.
> 

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Daniel P. Berrangé]

On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote:
> On 08/02/2022 13:10, Daniel P. Berrangé wrote:
> 
> > On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
> > > On 08/02/2022 12:49, Daniel P. Berrangé wrote:
> > > 
> > > > > I was under the impression that monitor_register_hmp_info_hrt() does all the
> > > > > magic here i.e. it declares the underlying QMP command with an x- prefix and
> > > > > effectively encapsulates the text field in a way that says "this is an
> > > > > unreliable text opaque for humans"?
> > > > 
> > > > The monitor_register_hmp_info_hrt only does the HMP glue side, and
> > > > that's only needed if you must dynamically register the HMP command.
> > > > For statically registered commands set '.cmd_info_hrt' directly in
> > > > the hml-commands-info.hx for the HMP side.
> > > > 
> > > > > If a qapi/ schema is needed could you explain what it should look like for
> > > > > this example and where it should go? Looking at the existing .json files I
> > > > > can't immediately see one which is the right place for this to live.
> > > > 
> > > > Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
> > > > there. The QAPI bit is fairly simple.
> > > > 
> > > > if you want to see an illustration of what's different from a previous
> > > > pure HMP impl, look at:
> > > > 
> > > >     commit dd98234c059e6bdb05a52998270df6d3d990332e
> > > >     Author: Daniel P. Berrangé <berrange@redhat.com>
> > > >     Date:   Wed Sep 8 10:35:43 2021 +0100
> > > > 
> > > >       qapi: introduce x-query-roms QMP command
> > > 
> > > I see, thanks for the reference. So qapi/machine.json would be the right
> > > place to declare the QMP part even for a specific device?
> > > 
> > > Even this approach still wouldn't work in its current form though, since as
> > > mentioned in my previous email it seems that only the target CONFIG_*
> > > defines and not the device CONFIG_* defines are present when processing
> > > hmp-commands-info.hx.
> > 
> > Yeah, that's where the pain comes in.  While QAPI schema can be made
> > conditional on a few CONFIG_* parameters - basically those derived
> > from global configure time options, it is impossible for this to be
> > with with target specific options like the device CONFIG_* defines.
> > 
> > This is why I suggested in my othuer reply that it would need to be
> > done with a generic 'info dev-debug' / 'x-query-dev-debug' command
> > that can be registered unconditionally, and then individual devices
> > plug into it.
> 
> After some more experiments this afternoon I still seem to be falling
> through the gaps on this one. This is based upon my understanding that all
> new HMP commands should use a QMP HumanReadableText implementation and the
> new command should be restricted according to target.
> 
> Currently I am working with this change to hmp-commands-info.hx and
> qapi/misc-target.json:

[snip]
 
> i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if
> TARGET guards and since HumanReadableText is only used by the new
> qmp_x_query_via() functionality then the compiler complains and aborts the
> compilation.
> 
> Possibly this is an error in the QAPI generator for types hidden behind
> commands using "if"? Otherwise I'm not sure what is the best way to proceed,
> so I'd be grateful for some further pointers.

Yes, this is pretty much what I expect and exactly what I hit with
other target specific commands.

That's why I suggested something like a general 'x-device-debug' command
that is NOT conditionalized in QAPI, against which dev impls can register
a callback to provide detailed reporting, instead of a device type specific
command.



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

On 21/02/2022 12:20, Philippe Mathieu-Daudé wrote:

> +Thomas
> 
> On 20/2/22 18:18, Mark Cave-Ayland wrote:
>> On 08/02/2022 13:10, Daniel P. Berrangé wrote:
>>
>>> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
>>>> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>>>>
>>>>>> I was under the impression that monitor_register_hmp_info_hrt() does all the
>>>>>> magic here i.e. it declares the underlying QMP command with an x- prefix and
>>>>>> effectively encapsulates the text field in a way that says "this is an
>>>>>> unreliable text opaque for humans"?
>>>>>
>>>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and
>>>>> that's only needed if you must dynamically register the HMP command.
>>>>> For statically registered commands set '.cmd_info_hrt' directly in
>>>>> the hml-commands-info.hx for the HMP side.
>>>>>
>>>>>> If a qapi/ schema is needed could you explain what it should look like for
>>>>>> this example and where it should go? Looking at the existing .json files I
>>>>>> can't immediately see one which is the right place for this to live.
>>>>>
>>>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
>>>>> there. The QAPI bit is fairly simple.
>>>>>
>>>>> if you want to see an illustration of what's different from a previous
>>>>> pure HMP impl, look at:
>>>>>
>>>>>     commit dd98234c059e6bdb05a52998270df6d3d990332e
>>>>>     Author: Daniel P. Berrangé <berrange@redhat.com>
>>>>>     Date:   Wed Sep 8 10:35:43 2021 +0100
>>>>>
>>>>>       qapi: introduce x-query-roms QMP command
>>>>
>>>> I see, thanks for the reference. So qapi/machine.json would be the right
>>>> place to declare the QMP part even for a specific device?
>>>>
>>>> Even this approach still wouldn't work in its current form though, since as
>>>> mentioned in my previous email it seems that only the target CONFIG_*
>>>> defines and not the device CONFIG_* defines are present when processing
>>>> hmp-commands-info.hx.
>>>
>>> Yeah, that's where the pain comes in.  While QAPI schema can be made
>>> conditional on a few CONFIG_* parameters - basically those derived
>>> from global configure time options, it is impossible for this to be
>>> with with target specific options like the device CONFIG_* defines.
>>>
>>> This is why I suggested in my othuer reply that it would need to be
>>> done with a generic 'info dev-debug' / 'x-query-dev-debug' command
>>> that can be registered unconditionally, and then individual devices
>>> plug into it.
>>
>> After some more experiments this afternoon I still seem to be falling through the 
>> gaps on this one. This is based upon my understanding that all new HMP commands 
>> should use a QMP HumanReadableText implementation and the new command should be 
>> restricted according to target.
>>
>> Currently I am working with this change to hmp-commands-info.hx and 
>> qapi/misc-target.json:
>>
>>
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index 7e6bd30395..aac86d5473 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -880,15 +880,15 @@ SRST
>>       Show intel SGX information.
>>   ERST
>>
>> #if defined(TARGET_M68K) || defined(TARGET_PPC)
>>       {
>>           .name         = "via",
>>           .args_type    = "",
>>           .params       = "",
>>           .help         = "show guest mos6522 VIA devices",
>>           .cmd_info_hrt = qmp_x_query_via,
>>       },
>> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
>> index 4bc45d2474..72bf71888e 100644
>> --- a/qapi/misc-target.json
>> +++ b/qapi/misc-target.json
>> @@ -2,6 +2,8 @@
>>   # vim: filetype=python
>>   #
>>
>> +{ 'include': 'common.json' }
>> +
>>   ##
>>   # @RTC_CHANGE:
>>   #
>> @@ -424,3 +426,19 @@
>>   #
>>   ##
>>   { 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 'TARGET_I386' }
>> +
>> +##
>> +# @x-query-via:
>> +#
>> +# Query information on the registered mos6522 VIAs
>> +#
>> +# Features:
>> +# @unstable: This command is meant for debugging.
>> +#
>> +# Returns: internal mos6522 information
>> +#
>> +# Since: 7.0
>> +##
>> +{ 'command': 'x-query-via',
>> +  'returns': 'HumanReadableText',
>> +  'features': [ 'unstable' ], 'if': { 'any': [ 'TARGET_M68K', 'TARGET_PPC' ] } }
>>
>>
>> The main problem with trying to restrict the new command to a target (or targets) 
>> is that the autogenerated qapi/qapi-commands-misc-target.h QAPI header cannot be 
>> included in a device header such as mos6522.h without getting poison errors like 
>> below (which does actually make sense):
>>
>>
>> In file included from ./qapi/qapi-commands-misc-target.h:17,
>>                   from /home/build/src/qemu/git/qemu/include/hw/misc/mos6522.h:35,
>>                   from ../hw/misc/mos6522.c:30:
>> ./qapi/qapi-types-misc-target.h:19:13: error: attempt to use poisoned "TARGET_ALPHA"
> 
> Can be kludged by making this device target-specific:
> 
> -- >8 --
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 6dcbe044f3..65837b1dfe 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -23 +23 @@ softmmu_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('armv7m_ras.c'))
> -softmmu_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
> +specific_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
> ---
> 
> I'd rather keep devices generic, but sometime we can't. In this case
> VIA is only used on m68k so it could be acceptable.

That's not quite true though, it's also used for the Mac machines. From what I can 
see everyone agrees that there needs to be a better way to do this, but it requires 
someone with time and interest to implement the suggested changes.


ATB,

Mark.

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

On 21/02/2022 17:11, Daniel P. Berrangé wrote:

> On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote:
>> On 08/02/2022 13:10, Daniel P. Berrangé wrote:
>>
>>> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
>>>> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>>>>
>>>>>> I was under the impression that monitor_register_hmp_info_hrt() does all the
>>>>>> magic here i.e. it declares the underlying QMP command with an x- prefix and
>>>>>> effectively encapsulates the text field in a way that says "this is an
>>>>>> unreliable text opaque for humans"?
>>>>>
>>>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and
>>>>> that's only needed if you must dynamically register the HMP command.
>>>>> For statically registered commands set '.cmd_info_hrt' directly in
>>>>> the hml-commands-info.hx for the HMP side.
>>>>>
>>>>>> If a qapi/ schema is needed could you explain what it should look like for
>>>>>> this example and where it should go? Looking at the existing .json files I
>>>>>> can't immediately see one which is the right place for this to live.
>>>>>
>>>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
>>>>> there. The QAPI bit is fairly simple.
>>>>>
>>>>> if you want to see an illustration of what's different from a previous
>>>>> pure HMP impl, look at:
>>>>>
>>>>>      commit dd98234c059e6bdb05a52998270df6d3d990332e
>>>>>      Author: Daniel P. Berrangé <berrange@redhat.com>
>>>>>      Date:   Wed Sep 8 10:35:43 2021 +0100
>>>>>
>>>>>        qapi: introduce x-query-roms QMP command
>>>>
>>>> I see, thanks for the reference. So qapi/machine.json would be the right
>>>> place to declare the QMP part even for a specific device?
>>>>
>>>> Even this approach still wouldn't work in its current form though, since as
>>>> mentioned in my previous email it seems that only the target CONFIG_*
>>>> defines and not the device CONFIG_* defines are present when processing
>>>> hmp-commands-info.hx.
>>>
>>> Yeah, that's where the pain comes in.  While QAPI schema can be made
>>> conditional on a few CONFIG_* parameters - basically those derived
>>> from global configure time options, it is impossible for this to be
>>> with with target specific options like the device CONFIG_* defines.
>>>
>>> This is why I suggested in my othuer reply that it would need to be
>>> done with a generic 'info dev-debug' / 'x-query-dev-debug' command
>>> that can be registered unconditionally, and then individual devices
>>> plug into it.
>>
>> After some more experiments this afternoon I still seem to be falling
>> through the gaps on this one. This is based upon my understanding that all
>> new HMP commands should use a QMP HumanReadableText implementation and the
>> new command should be restricted according to target.
>>
>> Currently I am working with this change to hmp-commands-info.hx and
>> qapi/misc-target.json:
> 
> [snip]
>   
>> i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if
>> TARGET guards and since HumanReadableText is only used by the new
>> qmp_x_query_via() functionality then the compiler complains and aborts the
>> compilation.
>>
>> Possibly this is an error in the QAPI generator for types hidden behind
>> commands using "if"? Otherwise I'm not sure what is the best way to proceed,
>> so I'd be grateful for some further pointers.
> 
> Yes, this is pretty much what I expect and exactly what I hit with
> other target specific commands.
> 
> That's why I suggested something like a general 'x-device-debug' command
> that is NOT conditionalized in QAPI, against which dev impls can register
> a callback to provide detailed reporting, instead of a device type specific
> command.

Ah so this is a known issue with this approach then. David mentioned earlier in the 
thread that he'd be okay with a HMP command if it was useful and restricted to the 
required targets, so would it be okay to add "info via" for now as just a (non-QMP 
wrapped) HMP info command if I can get that to work?


ATB,

Mark.

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Dr. David Alan Gilbert]

* Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote:
> On 21/02/2022 17:11, Daniel P. Berrangé wrote:
> 
> > On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote:
> > > On 08/02/2022 13:10, Daniel P. Berrangé wrote:
> > > 
> > > > On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
> > > > > On 08/02/2022 12:49, Daniel P. Berrangé wrote:
> > > > > 
> > > > > > > I was under the impression that monitor_register_hmp_info_hrt() does all the
> > > > > > > magic here i.e. it declares the underlying QMP command with an x- prefix and
> > > > > > > effectively encapsulates the text field in a way that says "this is an
> > > > > > > unreliable text opaque for humans"?
> > > > > > 
> > > > > > The monitor_register_hmp_info_hrt only does the HMP glue side, and
> > > > > > that's only needed if you must dynamically register the HMP command.
> > > > > > For statically registered commands set '.cmd_info_hrt' directly in
> > > > > > the hml-commands-info.hx for the HMP side.
> > > > > > 
> > > > > > > If a qapi/ schema is needed could you explain what it should look like for
> > > > > > > this example and where it should go? Looking at the existing .json files I
> > > > > > > can't immediately see one which is the right place for this to live.
> > > > > > 
> > > > > > Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
> > > > > > there. The QAPI bit is fairly simple.
> > > > > > 
> > > > > > if you want to see an illustration of what's different from a previous
> > > > > > pure HMP impl, look at:
> > > > > > 
> > > > > >      commit dd98234c059e6bdb05a52998270df6d3d990332e
> > > > > >      Author: Daniel P. Berrangé <berrange@redhat.com>
> > > > > >      Date:   Wed Sep 8 10:35:43 2021 +0100
> > > > > > 
> > > > > >        qapi: introduce x-query-roms QMP command
> > > > > 
> > > > > I see, thanks for the reference. So qapi/machine.json would be the right
> > > > > place to declare the QMP part even for a specific device?
> > > > > 
> > > > > Even this approach still wouldn't work in its current form though, since as
> > > > > mentioned in my previous email it seems that only the target CONFIG_*
> > > > > defines and not the device CONFIG_* defines are present when processing
> > > > > hmp-commands-info.hx.
> > > > 
> > > > Yeah, that's where the pain comes in.  While QAPI schema can be made
> > > > conditional on a few CONFIG_* parameters - basically those derived
> > > > from global configure time options, it is impossible for this to be
> > > > with with target specific options like the device CONFIG_* defines.
> > > > 
> > > > This is why I suggested in my othuer reply that it would need to be
> > > > done with a generic 'info dev-debug' / 'x-query-dev-debug' command
> > > > that can be registered unconditionally, and then individual devices
> > > > plug into it.
> > > 
> > > After some more experiments this afternoon I still seem to be falling
> > > through the gaps on this one. This is based upon my understanding that all
> > > new HMP commands should use a QMP HumanReadableText implementation and the
> > > new command should be restricted according to target.
> > > 
> > > Currently I am working with this change to hmp-commands-info.hx and
> > > qapi/misc-target.json:
> > 
> > [snip]
> > > i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if
> > > TARGET guards and since HumanReadableText is only used by the new
> > > qmp_x_query_via() functionality then the compiler complains and aborts the
> > > compilation.
> > > 
> > > Possibly this is an error in the QAPI generator for types hidden behind
> > > commands using "if"? Otherwise I'm not sure what is the best way to proceed,
> > > so I'd be grateful for some further pointers.
> > 
> > Yes, this is pretty much what I expect and exactly what I hit with
> > other target specific commands.
> > 
> > That's why I suggested something like a general 'x-device-debug' command
> > that is NOT conditionalized in QAPI, against which dev impls can register
> > a callback to provide detailed reporting, instead of a device type specific
> > command.
> 
> Ah so this is a known issue with this approach then. David mentioned earlier
> in the thread that he'd be okay with a HMP command if it was useful and
> restricted to the required targets, so would it be okay to add "info via"
> for now as just a (non-QMP wrapped) HMP info command if I can get that to
> work?

I still am from an HMP point of view; it sounds like the right way in
the future is to get the info devices or whatever;  I suggest you keep
it as close to a QMP implementation as possible, still with the
HumanReadableText stuff.
(Others might still be nervous about an HMP special; but I don't see
it's worth holding this trivial stuff up for it).

Dave

> 
> ATB,
> 
> Mark.
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

On 22/02/2022 15:03, Dr. David Alan Gilbert wrote:

> * Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote:
>> On 21/02/2022 17:11, Daniel P. Berrangé wrote:
>>
>>> On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote:
>>>> On 08/02/2022 13:10, Daniel P. Berrangé wrote:
>>>>
>>>>> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
>>>>>> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>>>>>>
>>>>>>>> I was under the impression that monitor_register_hmp_info_hrt() does all the
>>>>>>>> magic here i.e. it declares the underlying QMP command with an x- prefix and
>>>>>>>> effectively encapsulates the text field in a way that says "this is an
>>>>>>>> unreliable text opaque for humans"?
>>>>>>>
>>>>>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and
>>>>>>> that's only needed if you must dynamically register the HMP command.
>>>>>>> For statically registered commands set '.cmd_info_hrt' directly in
>>>>>>> the hml-commands-info.hx for the HMP side.
>>>>>>>
>>>>>>>> If a qapi/ schema is needed could you explain what it should look like for
>>>>>>>> this example and where it should go? Looking at the existing .json files I
>>>>>>>> can't immediately see one which is the right place for this to live.
>>>>>>>
>>>>>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
>>>>>>> there. The QAPI bit is fairly simple.
>>>>>>>
>>>>>>> if you want to see an illustration of what's different from a previous
>>>>>>> pure HMP impl, look at:
>>>>>>>
>>>>>>>       commit dd98234c059e6bdb05a52998270df6d3d990332e
>>>>>>>       Author: Daniel P. Berrangé <berrange@redhat.com>
>>>>>>>       Date:   Wed Sep 8 10:35:43 2021 +0100
>>>>>>>
>>>>>>>         qapi: introduce x-query-roms QMP command
>>>>>>
>>>>>> I see, thanks for the reference. So qapi/machine.json would be the right
>>>>>> place to declare the QMP part even for a specific device?
>>>>>>
>>>>>> Even this approach still wouldn't work in its current form though, since as
>>>>>> mentioned in my previous email it seems that only the target CONFIG_*
>>>>>> defines and not the device CONFIG_* defines are present when processing
>>>>>> hmp-commands-info.hx.
>>>>>
>>>>> Yeah, that's where the pain comes in.  While QAPI schema can be made
>>>>> conditional on a few CONFIG_* parameters - basically those derived
>>>>> from global configure time options, it is impossible for this to be
>>>>> with with target specific options like the device CONFIG_* defines.
>>>>>
>>>>> This is why I suggested in my othuer reply that it would need to be
>>>>> done with a generic 'info dev-debug' / 'x-query-dev-debug' command
>>>>> that can be registered unconditionally, and then individual devices
>>>>> plug into it.
>>>>
>>>> After some more experiments this afternoon I still seem to be falling
>>>> through the gaps on this one. This is based upon my understanding that all
>>>> new HMP commands should use a QMP HumanReadableText implementation and the
>>>> new command should be restricted according to target.
>>>>
>>>> Currently I am working with this change to hmp-commands-info.hx and
>>>> qapi/misc-target.json:
>>>
>>> [snip]
>>>> i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if
>>>> TARGET guards and since HumanReadableText is only used by the new
>>>> qmp_x_query_via() functionality then the compiler complains and aborts the
>>>> compilation.
>>>>
>>>> Possibly this is an error in the QAPI generator for types hidden behind
>>>> commands using "if"? Otherwise I'm not sure what is the best way to proceed,
>>>> so I'd be grateful for some further pointers.
>>>
>>> Yes, this is pretty much what I expect and exactly what I hit with
>>> other target specific commands.
>>>
>>> That's why I suggested something like a general 'x-device-debug' command
>>> that is NOT conditionalized in QAPI, against which dev impls can register
>>> a callback to provide detailed reporting, instead of a device type specific
>>> command.
>>
>> Ah so this is a known issue with this approach then. David mentioned earlier
>> in the thread that he'd be okay with a HMP command if it was useful and
>> restricted to the required targets, so would it be okay to add "info via"
>> for now as just a (non-QMP wrapped) HMP info command if I can get that to
>> work?
> 
> I still am from an HMP point of view; it sounds like the right way in
> the future is to get the info devices or whatever;  I suggest you keep
> it as close to a QMP implementation as possible, still with the
> HumanReadableText stuff.
> (Others might still be nervous about an HMP special; but I don't see
> it's worth holding this trivial stuff up for it).

I've just posted a v2 and what I've done there is to manually add a hmp_info_via() 
wrapper (taken almost verbatim from 
https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#id1) and added 
it to both include/monitor/hmp-target.h and include/hw/misc/mos6522.h which passes a 
Gitlab run.

I think it's worth having as an in-tree reference for when a more formal HMP/QMP 
per-device as opposed to per-target infrastructure arrives.


ATB,

Mark.