[PATCH v2 00/10] q800: migration fixes

[PATCH v2 00/10] q800: migration fixes

[Mark Cave-Ayland]

This patchset contains fixes for the macfb and esp devices which enable
migration of the q800 machine to succeed here in local testing.

Patches 1-5 contain fixes and improvements for migrating the macfb device
whilst patches 6-9 change the ESPState pdma_cb field from being a
function pointer to an integer index that can be included in the migration
stream.

Finally patch 10 ensures that any in-flight SCSI requests active during
migration are resumed correctly post-migration. This is required because
PDMA requires the guest to read/write DMA data and hence an active
SCSI request cannot run to completion before migration starts.

NOTE: this patchset is based upon my previous mos6522 patchset posted at
https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg05248.html.

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

v2:
- Rebase onto master
- Add R-B tags from Phil
- Update patch 8 to use an unsigned type for pdma_cb along with an enum
- Squash fixes into patch 3 (missing default in switch)


Mark Cave-Ayland (10):
  macfb: add VMStateDescription for MacfbNubusState and MacfbSysBusState
  macfb: don't use special irq_state and irq_mask variables in
    MacfbState
  macfb: increase number of registers saved in MacfbState
  macfb: add VMStateDescription fields for display type and VBL timer
  macfb: set initial value of mode control registers in
    macfb_common_realize()
  esp: introduce esp_set_pdma_cb() function
  esp: introduce esp_pdma_cb() function
  esp: convert ESPState pdma_cb from a function pointer to an integer
  esp: include the current PDMA callback in the migration stream
  esp: recreate ESPState current_req after migration

 hw/display/macfb.c         | 57 ++++++++++++++++++++++-----
 hw/scsi/esp.c              | 79 +++++++++++++++++++++++++++++++++-----
 include/hw/display/macfb.h |  5 +--
 include/hw/scsi/esp.h      | 11 +++++-
 4 files changed, 129 insertions(+), 23 deletions(-)

-- 
2.20.1

[PATCH v2 01/10] macfb: add VMStateDescription for MacfbNubusState and MacfbSysBusState

[Mark Cave-Ayland]

Currently when QEMU tries to migrate the macfb framebuffer it crashes randomly
because the opaque provided by the DeviceClass vmsd property for both devices
is set to MacfbState rather than MacfbNubusState or MacfbSysBusState as
appropriate.

Resolve the issue by adding new VMStateDescriptions for MacfbNubusState and
MacfbSysBusState which embed the existing vmstate_macfb VMStateDescription
within them using VMSTATE_STRUCT.

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

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index c9b468c10e..66ceacf1ae 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -746,6 +746,16 @@ static Property macfb_sysbus_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static const VMStateDescription vmstate_macfb_sysbus = {
+    .name = "macfb-sysbus",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(macfb, MacfbSysBusState, 1, vmstate_macfb, MacfbState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static Property macfb_nubus_properties[] = {
     DEFINE_PROP_UINT32("width", MacfbNubusState, macfb.width, 640),
     DEFINE_PROP_UINT32("height", MacfbNubusState, macfb.height, 480),
@@ -755,6 +765,16 @@ static Property macfb_nubus_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static const VMStateDescription vmstate_macfb_nubus = {
+    .name = "macfb-nubus",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(macfb, MacfbNubusState, 1, vmstate_macfb, MacfbState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void macfb_sysbus_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -762,7 +782,7 @@ static void macfb_sysbus_class_init(ObjectClass *klass, void *data)
     dc->realize = macfb_sysbus_realize;
     dc->desc = "SysBus Macintosh framebuffer";
     dc->reset = macfb_sysbus_reset;
-    dc->vmsd = &vmstate_macfb;
+    dc->vmsd = &vmstate_macfb_sysbus;
     device_class_set_props(dc, macfb_sysbus_properties);
 }
 
@@ -777,7 +797,7 @@ static void macfb_nubus_class_init(ObjectClass *klass, void *data)
                                       &ndc->parent_unrealize);
     dc->desc = "Nubus Macintosh framebuffer";
     dc->reset = macfb_nubus_reset;
-    dc->vmsd = &vmstate_macfb;
+    dc->vmsd = &vmstate_macfb_nubus;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
     device_class_set_props(dc, macfb_nubus_properties);
 }
-- 
2.20.1

[PATCH v2 02/10] macfb: don't use special irq_state and irq_mask variables in MacfbState

[Mark Cave-Ayland]

The current IRQ state and IRQ mask are handled exactly the same as standard
register accesses, so store these values directly in the regs array rather
than having separate variables for them.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/macfb.c         | 15 +++++++--------
 include/hw/display/macfb.h |  2 --
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 66ceacf1ae..fb54b460c1 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -476,7 +476,8 @@ static void macfb_update_display(void *opaque)
 
 static void macfb_update_irq(MacfbState *s)
 {
-    uint32_t irq_state = s->irq_state & s->irq_mask;
+    uint32_t irq_state = s->regs[DAFB_INTR_STAT >> 2] &
+                         s->regs[DAFB_INTR_MASK >> 2];
 
     if (irq_state) {
         qemu_irq_raise(s->irq);
@@ -496,7 +497,7 @@ static void macfb_vbl_timer(void *opaque)
     MacfbState *s = opaque;
     int64_t next_vbl;
 
-    s->irq_state |= DAFB_INTR_VBL;
+    s->regs[DAFB_INTR_STAT >> 2] |= DAFB_INTR_VBL;
     macfb_update_irq(s);
 
     /* 60 Hz irq */
@@ -530,10 +531,8 @@ static uint64_t macfb_ctrl_read(void *opaque,
     case DAFB_MODE_VADDR2:
     case DAFB_MODE_CTRL1:
     case DAFB_MODE_CTRL2:
-        val = s->regs[addr >> 2];
-        break;
     case DAFB_INTR_STAT:
-        val = s->irq_state;
+        val = s->regs[addr >> 2];
         break;
     case DAFB_MODE_SENSE:
         val = macfb_sense_read(s);
@@ -568,7 +567,7 @@ static void macfb_ctrl_write(void *opaque,
         macfb_sense_write(s, val);
         break;
     case DAFB_INTR_MASK:
-        s->irq_mask = val;
+        s->regs[addr >> 2] = val;
         if (val & DAFB_INTR_VBL) {
             next_vbl = macfb_next_vbl();
             timer_mod(s->vbl_timer, next_vbl);
@@ -577,12 +576,12 @@ static void macfb_ctrl_write(void *opaque,
         }
         break;
     case DAFB_INTR_CLEAR:
-        s->irq_state &= ~DAFB_INTR_VBL;
+        s->regs[DAFB_INTR_STAT >> 2] &= ~DAFB_INTR_VBL;
         macfb_update_irq(s);
         break;
     case DAFB_RESET:
         s->palette_current = 0;
-        s->irq_state &= ~DAFB_INTR_VBL;
+        s->regs[DAFB_INTR_STAT >> 2] &= ~DAFB_INTR_VBL;
         macfb_update_irq(s);
         break;
     case DAFB_LUT:
diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
index e52775aa21..6d9f0f7869 100644
--- a/include/hw/display/macfb.h
+++ b/include/hw/display/macfb.h
@@ -66,8 +66,6 @@ typedef struct MacfbState {
     uint32_t regs[MACFB_NUM_REGS];
     MacFbMode *mode;
 
-    uint32_t irq_state;
-    uint32_t irq_mask;
     QEMUTimer *vbl_timer;
     qemu_irq irq;
 } MacfbState;
-- 
2.20.1

[PATCH v2 03/10] macfb: increase number of registers saved in MacfbState

[Mark Cave-Ayland]

The MacOS toolbox ROM accesses a number of addresses between 0x0 and 0x200 during
initialisation and resolution changes. Whilst the function of many of these
registers is unknown, it is worth the minimal cost of saving these extra values as
part of migration to help future-proof the migration stream for the q800 machine
as it starts to stabilise.

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

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index fb54b460c1..dfdae90144 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -537,6 +537,10 @@ static uint64_t macfb_ctrl_read(void *opaque,
     case DAFB_MODE_SENSE:
         val = macfb_sense_read(s);
         break;
+    default:
+        if (addr < MACFB_CTRL_TOPADDR) {
+            val = s->regs[addr >> 2];
+        }
     }
 
     trace_macfb_ctrl_read(addr, val, size);
@@ -592,6 +596,10 @@ static void macfb_ctrl_write(void *opaque,
             macfb_invalidate_display(s);
         }
         break;
+    default:
+        if (addr < MACFB_CTRL_TOPADDR) {
+            s->regs[addr >> 2] = val;
+        }
     }
 
     trace_macfb_ctrl_write(addr, val, size);
diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
index 6d9f0f7869..55a50d3fb0 100644
--- a/include/hw/display/macfb.h
+++ b/include/hw/display/macfb.h
@@ -48,7 +48,8 @@ typedef struct MacFbMode {
     uint32_t offset;
 } MacFbMode;
 
-#define MACFB_NUM_REGS      8
+#define MACFB_CTRL_TOPADDR  0x200
+#define MACFB_NUM_REGS      (MACFB_CTRL_TOPADDR / sizeof(uint32_t))
 
 typedef struct MacfbState {
     MemoryRegion mem_vram;
-- 
2.20.1

[PATCH v2 04/10] macfb: add VMStateDescription fields for display type and VBL timer

[Mark Cave-Ayland]

These fields are required in the migration stream to restore macfb state
correctly.

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

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index dfdae90144..7371986480 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -625,9 +625,11 @@ static const VMStateDescription vmstate_macfb = {
     .minimum_version_id = 1,
     .post_load = macfb_post_load,
     .fields = (VMStateField[]) {
+        VMSTATE_UINT8(type, MacfbState),
         VMSTATE_UINT8_ARRAY(color_palette, MacfbState, 256 * 3),
         VMSTATE_UINT32(palette_current, MacfbState),
         VMSTATE_UINT32_ARRAY(regs, MacfbState, MACFB_NUM_REGS),
+        VMSTATE_TIMER_PTR(vbl_timer, MacfbState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.20.1

[PATCH v2 05/10] macfb: set initial value of mode control registers in macfb_common_realize()

[Mark Cave-Ayland]

If booting Linux directly in the q800 machine using -kernel rather than using a
MacOS toolbox ROM, the mode control registers are never initialised,
causing macfb_mode_write() to fail to determine the current resolution after
migration. Resolve this by always setting the initial values of the mode control
registers based upon the initial macfb properties during realize.

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

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 7371986480..2f8e016566 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -655,6 +655,14 @@ static bool macfb_common_realize(DeviceState *dev, MacfbState *s, Error **errp)
         return false;
     }
 
+    /*
+     * Set mode control registers to match the mode found above so that
+     * macfb_mode_write() does the right thing if no MacOS toolbox ROM
+     * is present to initialise them
+     */
+    s->regs[DAFB_MODE_CTRL1 >> 2] = s->mode->mode_ctrl1;
+    s->regs[DAFB_MODE_CTRL2 >> 2] = s->mode->mode_ctrl2;
+
     s->con = graphic_console_init(dev, 0, &macfb_ops, s);
     surface = qemu_console_surface(s->con);
 
-- 
2.20.1

[PATCH v2 06/10] esp: introduce esp_set_pdma_cb() function

[Mark Cave-Ayland]

This function is to be used to set the current PDMA callback rather than
accessing the ESPState pdma_cb function pointer directly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/scsi/esp.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 58d0edbd56..d1640218c4 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -195,6 +195,11 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
     esp_set_tc(s, dmalen);
 }
 
+static void esp_set_pdma_cb(ESPState *s, void (*cb)(ESPState *))
+{
+    s->pdma_cb = cb;
+}
+
 static int esp_select(ESPState *s)
 {
     int target;
@@ -356,7 +361,7 @@ static void handle_satn(ESPState *s)
         s->dma_cb = handle_satn;
         return;
     }
-    s->pdma_cb = satn_pdma_cb;
+    esp_set_pdma_cb(s, satn_pdma_cb);
     cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
     if (cmdlen > 0) {
         s->cmdfifo_cdb_offset = 1;
@@ -387,7 +392,7 @@ static void handle_s_without_atn(ESPState *s)
         s->dma_cb = handle_s_without_atn;
         return;
     }
-    s->pdma_cb = s_without_satn_pdma_cb;
+    esp_set_pdma_cb(s, s_without_satn_pdma_cb);
     cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
     if (cmdlen > 0) {
         s->cmdfifo_cdb_offset = 0;
@@ -422,7 +427,7 @@ static void handle_satn_stop(ESPState *s)
         s->dma_cb = handle_satn_stop;
         return;
     }
-    s->pdma_cb = satn_stop_pdma_cb;
+    esp_set_pdma_cb(s, satn_stop_pdma_cb);
     cmdlen = get_cmd(s, 1);
     if (cmdlen > 0) {
         trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
@@ -464,7 +469,7 @@ static void write_response(ESPState *s)
             s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
             s->rregs[ESP_RSEQ] = SEQ_CD;
         } else {
-            s->pdma_cb = write_response_pdma_cb;
+            esp_set_pdma_cb(s, write_response_pdma_cb);
             esp_raise_drq(s);
             return;
         }
@@ -604,7 +609,7 @@ static void esp_do_dma(ESPState *s)
             s->dma_memory_read(s->dma_opaque, buf, len);
             fifo8_push_all(&s->cmdfifo, buf, len);
         } else {
-            s->pdma_cb = do_dma_pdma_cb;
+            esp_set_pdma_cb(s, do_dma_pdma_cb);
             esp_raise_drq(s);
             return;
         }
@@ -646,7 +651,7 @@ static void esp_do_dma(ESPState *s)
         if (s->dma_memory_read) {
             s->dma_memory_read(s->dma_opaque, s->async_buf, len);
         } else {
-            s->pdma_cb = do_dma_pdma_cb;
+            esp_set_pdma_cb(s, do_dma_pdma_cb);
             esp_raise_drq(s);
             return;
         }
@@ -678,7 +683,7 @@ static void esp_do_dma(ESPState *s)
             }
 
             esp_set_tc(s, esp_get_tc(s) - len);
-            s->pdma_cb = do_dma_pdma_cb;
+            esp_set_pdma_cb(s, do_dma_pdma_cb);
             esp_raise_drq(s);
 
             /* Indicate transfer to FIFO is complete */
-- 
2.20.1

[PATCH v2 07/10] esp: introduce esp_pdma_cb() function

[Mark Cave-Ayland]

This function is to be used to execute the current PDMA callback rather than
dereferencing the ESPState pdma_cb function pointer directly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/scsi/esp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index d1640218c4..035fb0d1f6 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -200,6 +200,11 @@ static void esp_set_pdma_cb(ESPState *s, void (*cb)(ESPState *))
     s->pdma_cb = cb;
 }
 
+static void esp_pdma_cb(ESPState *s)
+{
+    s->pdma_cb(s);
+}
+
 static int esp_select(ESPState *s)
 {
     int target;
@@ -1268,7 +1273,7 @@ static void sysbus_esp_pdma_write(void *opaque, hwaddr addr,
         esp_pdma_write(s, val);
         break;
     }
-    s->pdma_cb(s);
+    esp_pdma_cb(s);
 }
 
 static uint64_t sysbus_esp_pdma_read(void *opaque, hwaddr addr,
@@ -1290,7 +1295,7 @@ static uint64_t sysbus_esp_pdma_read(void *opaque, hwaddr addr,
         break;
     }
     if (fifo8_num_used(&s->fifo) < 2) {
-        s->pdma_cb(s);
+        esp_pdma_cb(s);
     }
     return val;
 }
-- 
2.20.1

[PATCH v2 08/10] esp: convert ESPState pdma_cb from a function pointer to an integer

[Mark Cave-Ayland]

This prepares for the inclusion of the current PDMA callback in the migration
stream since the callback is referenced by an integer instead of a function
pointer.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/scsi/esp.c         | 44 ++++++++++++++++++++++++++++++-------------
 include/hw/scsi/esp.h | 11 ++++++++++-
 2 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 035fb0d1f6..a818b2b07a 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -195,16 +195,11 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
     esp_set_tc(s, dmalen);
 }
 
-static void esp_set_pdma_cb(ESPState *s, void (*cb)(ESPState *))
+static void esp_set_pdma_cb(ESPState *s, enum pdma_cb cb)
 {
     s->pdma_cb = cb;
 }
 
-static void esp_pdma_cb(ESPState *s)
-{
-    s->pdma_cb(s);
-}
-
 static int esp_select(ESPState *s)
 {
     int target;
@@ -366,7 +361,7 @@ static void handle_satn(ESPState *s)
         s->dma_cb = handle_satn;
         return;
     }
-    esp_set_pdma_cb(s, satn_pdma_cb);
+    esp_set_pdma_cb(s, SATN_PDMA_CB);
     cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
     if (cmdlen > 0) {
         s->cmdfifo_cdb_offset = 1;
@@ -397,7 +392,7 @@ static void handle_s_without_atn(ESPState *s)
         s->dma_cb = handle_s_without_atn;
         return;
     }
-    esp_set_pdma_cb(s, s_without_satn_pdma_cb);
+    esp_set_pdma_cb(s, S_WITHOUT_SATN_PDMA_CB);
     cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
     if (cmdlen > 0) {
         s->cmdfifo_cdb_offset = 0;
@@ -432,7 +427,7 @@ static void handle_satn_stop(ESPState *s)
         s->dma_cb = handle_satn_stop;
         return;
     }
-    esp_set_pdma_cb(s, satn_stop_pdma_cb);
+    esp_set_pdma_cb(s, SATN_STOP_PDMA_CB);
     cmdlen = get_cmd(s, 1);
     if (cmdlen > 0) {
         trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
@@ -474,7 +469,7 @@ static void write_response(ESPState *s)
             s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
             s->rregs[ESP_RSEQ] = SEQ_CD;
         } else {
-            esp_set_pdma_cb(s, write_response_pdma_cb);
+            esp_set_pdma_cb(s, WRITE_RESPONSE_PDMA_CB);
             esp_raise_drq(s);
             return;
         }
@@ -614,7 +609,7 @@ static void esp_do_dma(ESPState *s)
             s->dma_memory_read(s->dma_opaque, buf, len);
             fifo8_push_all(&s->cmdfifo, buf, len);
         } else {
-            esp_set_pdma_cb(s, do_dma_pdma_cb);
+            esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
             return;
         }
@@ -656,7 +651,7 @@ static void esp_do_dma(ESPState *s)
         if (s->dma_memory_read) {
             s->dma_memory_read(s->dma_opaque, s->async_buf, len);
         } else {
-            esp_set_pdma_cb(s, do_dma_pdma_cb);
+            esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
             return;
         }
@@ -688,7 +683,7 @@ static void esp_do_dma(ESPState *s)
             }
 
             esp_set_tc(s, esp_get_tc(s) - len);
-            esp_set_pdma_cb(s, do_dma_pdma_cb);
+            esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
 
             /* Indicate transfer to FIFO is complete */
@@ -787,6 +782,29 @@ static void esp_do_nodma(ESPState *s)
     esp_raise_irq(s);
 }
 
+static void esp_pdma_cb(ESPState *s)
+{
+    switch (s->pdma_cb) {
+    case SATN_PDMA_CB:
+        satn_pdma_cb(s);
+        break;
+    case S_WITHOUT_SATN_PDMA_CB:
+        s_without_satn_pdma_cb(s);
+        break;
+    case SATN_STOP_PDMA_CB:
+        satn_stop_pdma_cb(s);
+        break;
+    case WRITE_RESPONSE_PDMA_CB:
+        write_response_pdma_cb(s);
+        break;
+    case DO_DMA_PDMA_CB:
+        do_dma_pdma_cb(s);
+        break;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 void esp_command_complete(SCSIRequest *req, size_t resid)
 {
     ESPState *s = req->hba_private;
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index b1ec27612f..13b17496f8 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -51,7 +51,7 @@ struct ESPState {
     ESPDMAMemoryReadWriteFunc dma_memory_write;
     void *dma_opaque;
     void (*dma_cb)(ESPState *s);
-    void (*pdma_cb)(ESPState *s);
+    uint8_t pdma_cb;
 
     uint8_t mig_version_id;
 
@@ -150,6 +150,15 @@ struct SysBusESPState {
 #define TCHI_FAS100A 0x4
 #define TCHI_AM53C974 0x12
 
+/* PDMA callbacks */
+enum pdma_cb {
+    SATN_PDMA_CB = 0,
+    S_WITHOUT_SATN_PDMA_CB = 1,
+    SATN_STOP_PDMA_CB = 2,
+    WRITE_RESPONSE_PDMA_CB = 3,
+    DO_DMA_PDMA_CB = 4
+};
+
 void esp_dma_enable(ESPState *s, int irq, int level);
 void esp_request_cancelled(SCSIRequest *req);
 void esp_command_complete(SCSIRequest *req, size_t resid);
-- 
2.20.1

[PATCH v2 09/10] esp: include the current PDMA callback in the migration stream

[Mark Cave-Ayland]

This involves (re)adding a PDMA-specific subsection to hold the reference to the
current PDMA callback.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index a818b2b07a..32926834bc 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1209,6 +1209,25 @@ static int esp_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool esp_pdma_needed(void *opaque)
+{
+    ESPState *s = ESP(opaque);
+
+    return s->dma_memory_read == NULL && s->dma_memory_write == NULL &&
+           s->dma_enabled;
+}
+
+static const VMStateDescription vmstate_esp_pdma = {
+    .name = "esp/pdma",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .needed = esp_pdma_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(pdma_cb, ESPState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_esp = {
     .name = "esp",
     .version_id = 6,
@@ -1243,6 +1262,10 @@ const VMStateDescription vmstate_esp = {
         VMSTATE_UINT8_TEST(lun, ESPState, esp_is_version_6),
         VMSTATE_END_OF_LIST()
     },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_esp_pdma,
+        NULL
+    }
 };
 
 static void sysbus_esp_mem_write(void *opaque, hwaddr addr,
-- 
2.20.1

[PATCH v2 10/10] esp: recreate ESPState current_req after migration

[Mark Cave-Ayland]

Since PDMA reads/writes are driven by the guest, it is possible that migration
can occur whilst a SCSIRequest is still active. Fortunately active SCSIRequests
are already included in the migration stream and restarted post migration but
this still leaves the reference in ESPState uninitialised.

Implement the SCSIBusInfo .load_request callback to obtain a reference to the
currently active SCSIRequest and use it to recreate ESPState current_req
after migration.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 32926834bc..824c345ceb 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1341,6 +1341,15 @@ static uint64_t sysbus_esp_pdma_read(void *opaque, hwaddr addr,
     return val;
 }
 
+static void *esp_load_request(QEMUFile *f, SCSIRequest *req)
+{
+    ESPState *s = container_of(req->bus, ESPState, bus);
+
+    scsi_req_ref(req);
+    s->current_req = req;
+    return s;
+}
+
 static const MemoryRegionOps sysbus_esp_pdma_ops = {
     .read = sysbus_esp_pdma_read,
     .write = sysbus_esp_pdma_write,
@@ -1356,6 +1365,7 @@ static const struct SCSIBusInfo esp_scsi_info = {
     .max_target = ESP_MAX_DEVS,
     .max_lun = 7,
 
+    .load_request = esp_load_request,
     .transfer_data = esp_transfer_data,
     .complete = esp_command_complete,
     .cancel = esp_request_cancelled
-- 
2.20.1

Re: [PATCH v2 01/10] macfb: add VMStateDescription for MacfbNubusState and MacfbSysBusState

[Peter Maydell]

On Wed, 2 Mar 2022 at 21:41, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Currently when QEMU tries to migrate the macfb framebuffer it crashes randomly
> because the opaque provided by the DeviceClass vmsd property for both devices
> is set to MacfbState rather than MacfbNubusState or MacfbSysBusState as
> appropriate.
>
> Resolve the issue by adding new VMStateDescriptions for MacfbNubusState and
> MacfbSysBusState which embed the existing vmstate_macfb VMStateDescription
> within them using VMSTATE_STRUCT.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---

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

thanks
-- PMM

Re: [PATCH v2 03/10] macfb: increase number of registers saved in MacfbState

[Peter Maydell]

On Wed, 2 Mar 2022 at 21:31, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> The MacOS toolbox ROM accesses a number of addresses between 0x0 and 0x200 during
> initialisation and resolution changes. Whilst the function of many of these
> registers is unknown, it is worth the minimal cost of saving these extra values as
> part of migration to help future-proof the migration stream for the q800 machine
> as it starts to stabilise.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c         | 8 ++++++++
>  include/hw/display/macfb.h | 3 ++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index fb54b460c1..dfdae90144 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -537,6 +537,10 @@ static uint64_t macfb_ctrl_read(void *opaque,
>      case DAFB_MODE_SENSE:
>          val = macfb_sense_read(s);
>          break;
> +    default:
> +        if (addr < MACFB_CTRL_TOPADDR) {
> +            val = s->regs[addr >> 2];
> +        }
>      }
>
>      trace_macfb_ctrl_read(addr, val, size);
> @@ -592,6 +596,10 @@ static void macfb_ctrl_write(void *opaque,
>              macfb_invalidate_display(s);
>          }
>          break;
> +    default:
> +        if (addr < MACFB_CTRL_TOPADDR) {
> +            s->regs[addr >> 2] = val;
> +        }
>      }
>
>      trace_macfb_ctrl_write(addr, val, size);
> diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
> index 6d9f0f7869..55a50d3fb0 100644
> --- a/include/hw/display/macfb.h
> +++ b/include/hw/display/macfb.h
> @@ -48,7 +48,8 @@ typedef struct MacFbMode {
>      uint32_t offset;
>  } MacFbMode;
>
> -#define MACFB_NUM_REGS      8
> +#define MACFB_CTRL_TOPADDR  0x200
> +#define MACFB_NUM_REGS      (MACFB_CTRL_TOPADDR / sizeof(uint32_t))

You should either bump the vmstate_macfb version numbers here,
or at least note in the commit message that although it's a
migration break we know nobody's migrating this device because
of the bug we just fixed in the previous commit.

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

thanks
-- PMM

Re: [PATCH v2 03/10] macfb: increase number of registers saved in MacfbState

[Peter Maydell]

On Thu, 3 Mar 2022 at 15:25, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 2 Mar 2022 at 21:31, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
> >
> > The MacOS toolbox ROM accesses a number of addresses between 0x0 and 0x200 during
> > initialisation and resolution changes. Whilst the function of many of these
> > registers is unknown, it is worth the minimal cost of saving these extra values as
> > part of migration to help future-proof the migration stream for the q800 machine
> > as it starts to stabilise.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

> You should either bump the vmstate_macfb version numbers here,
> or at least note in the commit message that although it's a
> migration break we know nobody's migrating this device because
> of the bug we just fixed in the previous commit.

...where by "previous" I mean patch 1.

-- PMM

Re: [PATCH v2 04/10] macfb: add VMStateDescription fields for display type and VBL timer

[Peter Maydell]

On Wed, 2 Mar 2022 at 21:31, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> These fields are required in the migration stream to restore macfb state
> correctly.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index dfdae90144..7371986480 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -625,9 +625,11 @@ static const VMStateDescription vmstate_macfb = {
>      .minimum_version_id = 1,
>      .post_load = macfb_post_load,
>      .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(type, MacfbState),
>          VMSTATE_UINT8_ARRAY(color_palette, MacfbState, 256 * 3),
>          VMSTATE_UINT32(palette_current, MacfbState),
>          VMSTATE_UINT32_ARRAY(regs, MacfbState, MACFB_NUM_REGS),
> +        VMSTATE_TIMER_PTR(vbl_timer, MacfbState),
>          VMSTATE_END_OF_LIST()
>      }
>  };

Same bump-versions-or-explain-why-not as previous patch, otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 05/10] macfb: set initial value of mode control registers in macfb_common_realize()

[Peter Maydell]

On Wed, 2 Mar 2022 at 21:35, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> If booting Linux directly in the q800 machine using -kernel rather than using a
> MacOS toolbox ROM, the mode control registers are never initialised,
> causing macfb_mode_write() to fail to determine the current resolution after
> migration. Resolve this by always setting the initial values of the mode control
> registers based upon the initial macfb properties during realize.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/macfb.c | 8 ++++++++

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

thanks
-- PMM

Re: [PATCH v2 09/10] esp: include the current PDMA callback in the migration stream

[Peter Maydell]

On Wed, 2 Mar 2022 at 21:38, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This involves (re)adding a PDMA-specific subsection to hold the reference to the
> current PDMA callback.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index a818b2b07a..32926834bc 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -1209,6 +1209,25 @@ static int esp_post_load(void *opaque, int version_id)
>      return 0;
>  }
>
> +static bool esp_pdma_needed(void *opaque)
> +{
> +    ESPState *s = ESP(opaque);
> +
> +    return s->dma_memory_read == NULL && s->dma_memory_write == NULL &&
> +           s->dma_enabled;

A comment about why this is the correct condition would be helpful.
If I understand it correctly, something like this ?

 /*
  * pdma_cb is used only by the sysbus ESP device, not the PCI ESP
  * device. The PCI device sets the s->dma_memory_read and
  * s->dma_memory_write function pointers at realize.
  */

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

> +}
> +
> +static const VMStateDescription vmstate_esp_pdma = {
> +    .name = "esp/pdma",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .needed = esp_pdma_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(pdma_cb, ESPState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  const VMStateDescription vmstate_esp = {
>      .name = "esp",
>      .version_id = 6,
> @@ -1243,6 +1262,10 @@ const VMStateDescription vmstate_esp = {
>          VMSTATE_UINT8_TEST(lun, ESPState, esp_is_version_6),
>          VMSTATE_END_OF_LIST()
>      },
> +    .subsections = (const VMStateDescription * []) {
> +        &vmstate_esp_pdma,
> +        NULL
> +    }
>  };

Do we need to do something similar to handle s->dma_cb ?

thanks
-- PMM

Re: [PATCH v2 03/10] macfb: increase number of registers saved in MacfbState

[Mark Cave-Ayland]

On 03/03/2022 15:25, Peter Maydell wrote:

> On Wed, 2 Mar 2022 at 21:31, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> The MacOS toolbox ROM accesses a number of addresses between 0x0 and 0x200 during
>> initialisation and resolution changes. Whilst the function of many of these
>> registers is unknown, it is worth the minimal cost of saving these extra values as
>> part of migration to help future-proof the migration stream for the q800 machine
>> as it starts to stabilise.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/display/macfb.c         | 8 ++++++++
>>   include/hw/display/macfb.h | 3 ++-
>>   2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index fb54b460c1..dfdae90144 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
>> @@ -537,6 +537,10 @@ static uint64_t macfb_ctrl_read(void *opaque,
>>       case DAFB_MODE_SENSE:
>>           val = macfb_sense_read(s);
>>           break;
>> +    default:
>> +        if (addr < MACFB_CTRL_TOPADDR) {
>> +            val = s->regs[addr >> 2];
>> +        }
>>       }
>>
>>       trace_macfb_ctrl_read(addr, val, size);
>> @@ -592,6 +596,10 @@ static void macfb_ctrl_write(void *opaque,
>>               macfb_invalidate_display(s);
>>           }
>>           break;
>> +    default:
>> +        if (addr < MACFB_CTRL_TOPADDR) {
>> +            s->regs[addr >> 2] = val;
>> +        }
>>       }
>>
>>       trace_macfb_ctrl_write(addr, val, size);
>> diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
>> index 6d9f0f7869..55a50d3fb0 100644
>> --- a/include/hw/display/macfb.h
>> +++ b/include/hw/display/macfb.h
>> @@ -48,7 +48,8 @@ typedef struct MacFbMode {
>>       uint32_t offset;
>>   } MacFbMode;
>>
>> -#define MACFB_NUM_REGS      8
>> +#define MACFB_CTRL_TOPADDR  0x200
>> +#define MACFB_NUM_REGS      (MACFB_CTRL_TOPADDR / sizeof(uint32_t))
> 
> You should either bump the vmstate_macfb version numbers here,
> or at least note in the commit message that although it's a
> migration break we know nobody's migrating this device because
> of the bug we just fixed in the previous commit.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I can do this if you like, although until the last 2 patches anything that is using 
the disk will fail, and that's just about everything because DMA requests require 
guest support to move the data from the ESP to the CPU.

In terms of the q800 machine there is an implicit assumption that there will be more 
migration breaks to come, mainly because there are new devices to be added to the 
q800 machine in my outstanding MacOS patches that will break migration again once. So 
until these are finally merged I don't think it's worth trying to stabilise the 
migration stream.


ATB,

Mark.

Re: [PATCH v2 04/10] macfb: add VMStateDescription fields for display type and VBL timer

[Mark Cave-Ayland]

On 03/03/2022 15:26, Peter Maydell wrote:

> On Wed, 2 Mar 2022 at 21:31, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> These fields are required in the migration stream to restore macfb state
>> correctly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/display/macfb.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
>> index dfdae90144..7371986480 100644
>> --- a/hw/display/macfb.c
>> +++ b/hw/display/macfb.c
>> @@ -625,9 +625,11 @@ static const VMStateDescription vmstate_macfb = {
>>       .minimum_version_id = 1,
>>       .post_load = macfb_post_load,
>>       .fields = (VMStateField[]) {
>> +        VMSTATE_UINT8(type, MacfbState),
>>           VMSTATE_UINT8_ARRAY(color_palette, MacfbState, 256 * 3),
>>           VMSTATE_UINT32(palette_current, MacfbState),
>>           VMSTATE_UINT32_ARRAY(regs, MacfbState, MACFB_NUM_REGS),
>> +        VMSTATE_TIMER_PTR(vbl_timer, MacfbState),
>>           VMSTATE_END_OF_LIST()
>>       }
>>   };
> 
> Same bump-versions-or-explain-why-not as previous patch, otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

... and the same explanation applies here too. Should I still mention this in the 
commit messages for both this and the previous patch?


ATB,

Mark.

Re: [PATCH v2 09/10] esp: include the current PDMA callback in the migration stream

[Mark Cave-Ayland]

On 03/03/2022 15:40, Peter Maydell wrote:

> On Wed, 2 Mar 2022 at 21:38, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> This involves (re)adding a PDMA-specific subsection to hold the reference to the
>> current PDMA callback.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index a818b2b07a..32926834bc 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -1209,6 +1209,25 @@ static int esp_post_load(void *opaque, int version_id)
>>       return 0;
>>   }
>>
>> +static bool esp_pdma_needed(void *opaque)
>> +{
>> +    ESPState *s = ESP(opaque);
>> +
>> +    return s->dma_memory_read == NULL && s->dma_memory_write == NULL &&
>> +           s->dma_enabled;
> 
> A comment about why this is the correct condition would be helpful.
> If I understand it correctly, something like this ?
> 
>   /*
>    * pdma_cb is used only by the sysbus ESP device, not the PCI ESP
>    * device. The PCI device sets the s->dma_memory_read and
>    * s->dma_memory_write function pointers at realize.
>    */

Even more specifically, PDMA is only used by the Macintosh which is detected by not 
having DMA memory access functions (because the movement between the SCSI bus and the 
CPU is done by the guest) yet DMA is enabled.

> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
>> +}
>> +
>> +static const VMStateDescription vmstate_esp_pdma = {
>> +    .name = "esp/pdma",
>> +    .version_id = 0,
>> +    .minimum_version_id = 0,
>> +    .needed = esp_pdma_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT8(pdma_cb, ESPState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   const VMStateDescription vmstate_esp = {
>>       .name = "esp",
>>       .version_id = 6,
>> @@ -1243,6 +1262,10 @@ const VMStateDescription vmstate_esp = {
>>           VMSTATE_UINT8_TEST(lun, ESPState, esp_is_version_6),
>>           VMSTATE_END_OF_LIST()
>>       },
>> +    .subsections = (const VMStateDescription * []) {
>> +        &vmstate_esp_pdma,
>> +        NULL
>> +    }
>>   };
> 
> Do we need to do something similar to handle s->dma_cb ?

I don't believe so. From IRC my understanding was that for normal DMA where the SCSI 
data is copied directly to memory, if migration is requested the iothread will run to 
completion first which finishes the SCSIRequest before migration starts. It is only 
in the PDMA case where the guest OS has to do the data movement that a reference to 
the current callback is required to allow the SCSIRequest to continue post-migration.


ATB,

Mark.

Re: [PATCH v2 03/10] macfb: increase number of registers saved in MacfbState

[Peter Maydell]

On Thu, 3 Mar 2022 at 17:44, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 03/03/2022 15:25, Peter Maydell wrote:
>
> > On Wed, 2 Mar 2022 at 21:31, Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> wrote:
> >>
> >> The MacOS toolbox ROM accesses a number of addresses between 0x0 and 0x200 during
> >> initialisation and resolution changes. Whilst the function of many of these
> >> registers is unknown, it is worth the minimal cost of saving these extra values as
> >> part of migration to help future-proof the migration stream for the q800 machine
> >> as it starts to stabilise.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >>   hw/display/macfb.c         | 8 ++++++++
> >>   include/hw/display/macfb.h | 3 ++-
> >>   2 files changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> >> index fb54b460c1..dfdae90144 100644
> >> --- a/hw/display/macfb.c
> >> +++ b/hw/display/macfb.c
> >> @@ -537,6 +537,10 @@ static uint64_t macfb_ctrl_read(void *opaque,
> >>       case DAFB_MODE_SENSE:
> >>           val = macfb_sense_read(s);
> >>           break;
> >> +    default:
> >> +        if (addr < MACFB_CTRL_TOPADDR) {
> >> +            val = s->regs[addr >> 2];
> >> +        }
> >>       }
> >>
> >>       trace_macfb_ctrl_read(addr, val, size);
> >> @@ -592,6 +596,10 @@ static void macfb_ctrl_write(void *opaque,
> >>               macfb_invalidate_display(s);
> >>           }
> >>           break;
> >> +    default:
> >> +        if (addr < MACFB_CTRL_TOPADDR) {
> >> +            s->regs[addr >> 2] = val;
> >> +        }
> >>       }
> >>
> >>       trace_macfb_ctrl_write(addr, val, size);
> >> diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
> >> index 6d9f0f7869..55a50d3fb0 100644
> >> --- a/include/hw/display/macfb.h
> >> +++ b/include/hw/display/macfb.h
> >> @@ -48,7 +48,8 @@ typedef struct MacFbMode {
> >>       uint32_t offset;
> >>   } MacFbMode;
> >>
> >> -#define MACFB_NUM_REGS      8
> >> +#define MACFB_CTRL_TOPADDR  0x200
> >> +#define MACFB_NUM_REGS      (MACFB_CTRL_TOPADDR / sizeof(uint32_t))
> >
> > You should either bump the vmstate_macfb version numbers here,
> > or at least note in the commit message that although it's a
> > migration break we know nobody's migrating this device because
> > of the bug we just fixed in the previous commit.
> >
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> I can do this if you like, although until the last 2 patches anything that is using
> the disk will fail, and that's just about everything because DMA requests require
> guest support to move the data from the ESP to the CPU.
>
> In terms of the q800 machine there is an implicit assumption that there will be more
> migration breaks to come, mainly because there are new devices to be added to the
> q800 machine in my outstanding MacOS patches that will break migration again once. So
> until these are finally merged I don't think it's worth trying to stabilise the
> migration stream.

Yeah, fair enough; just put a note in the commit message to
that effect.  (Mostly bumping the migration version is about making
the error message nicer if somebody does do a mismatched save/load.)

thanks
-- PMM

Re: [PATCH v2 01/10] macfb: add VMStateDescription for MacfbNubusState and MacfbSysBusState

[Laurent Vivier]

Le 02/03/2022 à 22:27, Mark Cave-Ayland a écrit :
> Currently when QEMU tries to migrate the macfb framebuffer it crashes randomly
> because the opaque provided by the DeviceClass vmsd property for both devices
> is set to MacfbState rather than MacfbNubusState or MacfbSysBusState as
> appropriate.
> 
> Resolve the issue by adding new VMStateDescriptions for MacfbNubusState and
> MacfbSysBusState which embed the existing vmstate_macfb VMStateDescription
> within them using VMSTATE_STRUCT.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/display/macfb.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 02/10] macfb: don't use special irq_state and irq_mask variables in MacfbState

[Laurent Vivier]

Le 02/03/2022 à 22:27, Mark Cave-Ayland a écrit :
> The current IRQ state and IRQ mask are handled exactly the same as standard
> register accesses, so store these values directly in the regs array rather
> than having separate variables for them.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/display/macfb.c         | 15 +++++++--------
>   include/hw/display/macfb.h |  2 --
>   2 files changed, 7 insertions(+), 10 deletions(-)

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 03/10] macfb: increase number of registers saved in MacfbState

[Laurent Vivier]

Le 02/03/2022 à 22:27, Mark Cave-Ayland a écrit :
> The MacOS toolbox ROM accesses a number of addresses between 0x0 and 0x200 during
> initialisation and resolution changes. Whilst the function of many of these
> registers is unknown, it is worth the minimal cost of saving these extra values as
> part of migration to help future-proof the migration stream for the q800 machine
> as it starts to stabilise.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/display/macfb.c         | 8 ++++++++
>   include/hw/display/macfb.h | 3 ++-
>   2 files changed, 10 insertions(+), 1 deletion(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 04/10] macfb: add VMStateDescription fields for display type and VBL timer

[Laurent Vivier]

Le 02/03/2022 à 22:27, Mark Cave-Ayland a écrit :
> These fields are required in the migration stream to restore macfb state
> correctly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/display/macfb.c | 2 ++
>   1 file changed, 2 insertions(+)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 05/10] macfb: set initial value of mode control registers in macfb_common_realize()

[Laurent Vivier]

Le 02/03/2022 à 22:27, Mark Cave-Ayland a écrit :
> If booting Linux directly in the q800 machine using -kernel rather than using a
> MacOS toolbox ROM, the mode control registers are never initialised,
> causing macfb_mode_write() to fail to determine the current resolution after
> migration. Resolve this by always setting the initial values of the mode control
> registers based upon the initial macfb properties during realize.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/display/macfb.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 06/10] esp: introduce esp_set_pdma_cb() function

[Laurent Vivier]

Le 02/03/2022 à 22:27, Mark Cave-Ayland a écrit :
> This function is to be used to set the current PDMA callback rather than
> accessing the ESPState pdma_cb function pointer directly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/scsi/esp.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 07/10] esp: introduce esp_pdma_cb() function

[Laurent Vivier]

Le 02/03/2022 à 22:27, Mark Cave-Ayland a écrit :
> This function is to be used to execute the current PDMA callback rather than
> dereferencing the ESPState pdma_cb function pointer directly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/scsi/esp.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 08/10] esp: convert ESPState pdma_cb from a function pointer to an integer

[Laurent Vivier]

Le 02/03/2022 à 22:27, Mark Cave-Ayland a écrit :
> This prepares for the inclusion of the current PDMA callback in the migration
> stream since the callback is referenced by an integer instead of a function
> pointer.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/scsi/esp.c         | 44 ++++++++++++++++++++++++++++++-------------
>   include/hw/scsi/esp.h | 11 ++++++++++-
>   2 files changed, 41 insertions(+), 14 deletions(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 09/10] esp: include the current PDMA callback in the migration stream

[Laurent Vivier]

Le 02/03/2022 à 22:27, Mark Cave-Ayland a écrit :
> This involves (re)adding a PDMA-specific subsection to hold the reference to the
> current PDMA callback.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 10/10] esp: recreate ESPState current_req after migration

[Laurent Vivier]

Le 02/03/2022 à 22:27, Mark Cave-Ayland a écrit :
> Since PDMA reads/writes are driven by the guest, it is possible that migration
> can occur whilst a SCSIRequest is still active. Fortunately active SCSIRequests
> are already included in the migration stream and restarted post migration but
> this still leaves the reference in ESPState uninitialised.
> 
> Implement the SCSIBusInfo .load_request callback to obtain a reference to the
> currently active SCSIRequest and use it to recreate ESPState current_req
> after migration.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>