[PULL 0/7] lsi, vga fixes for 2024-04-02

[PULL 0/7] lsi, vga fixes for 2024-04-02

[Paolo Bonzini]

The following changes since commit b9dbf6f9bf533564f6a4277d03906fcd32bb0245:

  Merge tag 'pull-tcg-20240329' of https://gitlab.com/rth7680/qemu into staging (2024-03-30 14:54:57 +0000)

are available in the Git repository at:

  https://gitlab.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to eac4af186f6db46fc90ec571a855bd6fa4cb7841:

  pc_q35: remove unnecessary m->alias assignment (2024-04-02 15:14:02 +0200)

----------------------------------------------------------------
* lsi53c895a: fix assertion failure with invalid Block Move
* vga: fix assertion failure with 4- and 16-color modes
* remove unnecessary assignment

----------------------------------------------------------------
Paolo Bonzini (7):
      vga: merge conditionals on shift control register
      vga: move computation of dirty memory region later
      vga: adjust dirty memory region if pel panning is active
      vga: do not treat horiz pel panning value of 8 as "enabled"
      lsi53c895a: avoid out of bounds access to s->msg[]
      lsi53c895a: detect invalid Block Move instruction
      pc_q35: remove unnecessary m->alias assignment

 hw/display/vga.c     | 146 +++++++++++++++++++++++++--------------------------
 hw/i386/pc_q35.c     |   1 -
 hw/scsi/lsi53c895a.c |  28 +++++++---
 3 files changed, 94 insertions(+), 81 deletions(-)
-- 
2.44.0

[PULL 1/7] vga: merge conditionals on shift control register

[Paolo Bonzini]

There are two sets of conditionals using the shift control bits: one to
verify the palette and adjust disp_width, one to compute the "v" and
"bits" variables.  Merge them into one, with the extra benefit that
we now have the "bits" value available early and can use it to
compute region_end.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 89 +++++++++++++++++++++++-------------------------
 1 file changed, 42 insertions(+), 47 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index bc5b83421bf..4795a0012e2 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1546,12 +1546,54 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     }
 
     if (shift_control == 0) {
+        full_update |= update_palette16(s);
         if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) {
             disp_width <<= 1;
+            v = VGA_DRAW_LINE4D2;
+        } else {
+            v = VGA_DRAW_LINE4;
         }
+        bits = 4;
+
     } else if (shift_control == 1) {
+        full_update |= update_palette16(s);
         if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) {
             disp_width <<= 1;
+            v = VGA_DRAW_LINE2D2;
+        } else {
+            v = VGA_DRAW_LINE2;
+        }
+        bits = 4;
+
+    } else {
+        switch (depth) {
+        default:
+        case 0:
+            full_update |= update_palette256(s);
+            v = VGA_DRAW_LINE8D2;
+            bits = 4;
+            break;
+        case 8:
+            full_update |= update_palette256(s);
+            v = VGA_DRAW_LINE8;
+            bits = 8;
+            break;
+        case 15:
+            v = s->big_endian_fb ? VGA_DRAW_LINE15_BE : VGA_DRAW_LINE15_LE;
+            bits = 16;
+            break;
+        case 16:
+            v = s->big_endian_fb ? VGA_DRAW_LINE16_BE : VGA_DRAW_LINE16_LE;
+            bits = 16;
+            break;
+        case 24:
+            v = s->big_endian_fb ? VGA_DRAW_LINE24_BE : VGA_DRAW_LINE24_LE;
+            bits = 24;
+            break;
+        case 32:
+            v = s->big_endian_fb ? VGA_DRAW_LINE32_BE : VGA_DRAW_LINE32_LE;
+            bits = 32;
+            break;
         }
     }
 
@@ -1607,53 +1649,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         }
     }
 
-    if (shift_control == 0) {
-        full_update |= update_palette16(s);
-        if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) {
-            v = VGA_DRAW_LINE4D2;
-        } else {
-            v = VGA_DRAW_LINE4;
-        }
-        bits = 4;
-    } else if (shift_control == 1) {
-        full_update |= update_palette16(s);
-        if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) {
-            v = VGA_DRAW_LINE2D2;
-        } else {
-            v = VGA_DRAW_LINE2;
-        }
-        bits = 4;
-    } else {
-        switch(s->get_bpp(s)) {
-        default:
-        case 0:
-            full_update |= update_palette256(s);
-            v = VGA_DRAW_LINE8D2;
-            bits = 4;
-            break;
-        case 8:
-            full_update |= update_palette256(s);
-            v = VGA_DRAW_LINE8;
-            bits = 8;
-            break;
-        case 15:
-            v = s->big_endian_fb ? VGA_DRAW_LINE15_BE : VGA_DRAW_LINE15_LE;
-            bits = 16;
-            break;
-        case 16:
-            v = s->big_endian_fb ? VGA_DRAW_LINE16_BE : VGA_DRAW_LINE16_LE;
-            bits = 16;
-            break;
-        case 24:
-            v = s->big_endian_fb ? VGA_DRAW_LINE24_BE : VGA_DRAW_LINE24_LE;
-            bits = 24;
-            break;
-        case 32:
-            v = s->big_endian_fb ? VGA_DRAW_LINE32_BE : VGA_DRAW_LINE32_LE;
-            bits = 32;
-            break;
-        }
-    }
     vga_draw_line = vga_draw_line_table[v];
 
     if (!is_buffer_shared(surface) && s->cursor_invalidate) {
-- 
2.44.0

[PULL 2/7] vga: move computation of dirty memory region later

[Paolo Bonzini]

Move the computation of region_start and region_end after the value of
"bits" is known.  This makes it possible to distinguish modes that
support horizontal pel panning from modes that do not.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 50 ++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 4795a0012e2..b4ceff70eb8 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1501,31 +1501,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     disp_width = width;
     depth = s->get_bpp(s);
 
-    region_start = (s->params.start_addr * 4);
-    region_end = region_start + (ram_addr_t)s->params.line_offset * height;
-    region_end += width * depth / 8; /* scanline length */
-    region_end -= s->params.line_offset;
-    if (region_end > s->vbe_size || depth == 0 || depth == 15) {
-        /*
-         * We land here on:
-         *  - wraps around (can happen with cirrus vbe modes)
-         *  - depth == 0 (256 color palette video mode)
-         *  - depth == 15
-         *
-         * Take the safe and slow route:
-         *   - create a dirty bitmap snapshot for all vga memory.
-         *   - force shadowing (so all vga memory access goes
-         *     through vga_read_*() helpers).
-         *
-         * Given this affects only vga features which are pretty much
-         * unused by modern guests there should be no performance
-         * impact.
-         */
-        region_start = 0;
-        region_end = s->vbe_size;
-        force_shadow = true;
-    }
-
     /* bits 5-6: 0 = 16-color mode, 1 = 4-color mode, 2 = 256-color mode.  */
     shift_control = (s->gr[VGA_GFX_MODE] >> 5) & 3;
     double_scan = (s->cr[VGA_CRTC_MAX_SCAN] >> 7);
@@ -1597,6 +1572,31 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         }
     }
 
+    region_start = (s->params.start_addr * 4);
+    region_end = region_start + (ram_addr_t)s->params.line_offset * height;
+    region_end += width * depth / 8; /* scanline length */
+    region_end -= s->params.line_offset;
+    if (region_end > s->vbe_size || depth == 0 || depth == 15) {
+        /*
+         * We land here on:
+         *  - wraps around (can happen with cirrus vbe modes)
+         *  - depth == 0 (256 color palette video mode)
+         *  - depth == 15
+         *
+         * Take the safe and slow route:
+         *   - create a dirty bitmap snapshot for all vga memory.
+         *   - force shadowing (so all vga memory access goes
+         *     through vga_read_*() helpers).
+         *
+         * Given this affects only vga features which are pretty much
+         * unused by modern guests there should be no performance
+         * impact.
+         */
+        region_start = 0;
+        region_end = s->vbe_size;
+        force_shadow = true;
+    }
+
     /*
      * Check whether we can share the surface with the backend
      * or whether we need a shadow surface. We share native
-- 
2.44.0

[PULL 3/7] vga: adjust dirty memory region if pel panning is active

[Paolo Bonzini]

When pel panning is active, one more byte is read from each of the VGA
memory planes.  This has to be accounted in the computation of region_end,
otherwise vga_draw_graphic() fails an assertion:

qemu-system-i386: ../system/physmem.c:946: cpu_physical_memory_snapshot_get_dirty: Assertion `start + length <= snap->end' failed.

Reported-by: Helge Konetzka <hk@zapateado.de>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2244
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index b4ceff70eb8..40acd19e72a 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1571,11 +1571,15 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
             break;
         }
     }
+    hpel = bits <= 8 ? s->params.hpel : 0;
 
     region_start = (s->params.start_addr * 4);
     region_end = region_start + (ram_addr_t)s->params.line_offset * height;
     region_end += width * depth / 8; /* scanline length */
     region_end -= s->params.line_offset;
+    if (hpel) {
+        region_end += 4;
+    }
     if (region_end > s->vbe_size || depth == 0 || depth == 15) {
         /*
          * We land here on:
@@ -1660,7 +1664,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
            width, height, v, line_offset, s->cr[9], s->cr[VGA_CRTC_MODE],
            s->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
 #endif
-    hpel = bits <= 8 ? s->params.hpel : 0;
     addr1 = (s->params.start_addr * 4);
     bwidth = DIV_ROUND_UP(width * bits, 8);
     if (hpel) {
-- 
2.44.0

[PULL 4/7] vga: do not treat horiz pel panning value of 8 as "enabled"

[Paolo Bonzini]

Horizontal pel panning bit 3 is only used in text mode.  In graphics
mode, it can be treated as if it was zero, thus not extending the
dirty memory region.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 40acd19e72a..77f59e8c113 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1571,7 +1571,9 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
             break;
         }
     }
-    hpel = bits <= 8 ? s->params.hpel : 0;
+
+    /* Horizontal pel panning bit 3 is only used in text mode.  */
+    hpel = bits <= 8 ? s->params.hpel & 7 : 0;
 
     region_start = (s->params.start_addr * 4);
     region_end = region_start + (ram_addr_t)s->params.line_offset * height;
-- 
2.44.0

[PULL 5/7] lsi53c895a: avoid out of bounds access to s->msg[]

[Paolo Bonzini]

If no bytes are there to process in the message in phase,
the input data latch (s->sidl) is set to s->msg[-1].  Just
do nothing since no DMA is performed.

Reported-by: Chuhong Yuan <hslester96@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/lsi53c895a.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 71f759a59dd..eb9828dd5ef 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -927,13 +927,18 @@ static void lsi_do_msgin(LSIState *s)
     assert(len > 0 && len <= LSI_MAX_MSGIN_LEN);
     if (len > s->dbc)
         len = s->dbc;
-    pci_dma_write(PCI_DEVICE(s), s->dnad, s->msg, len);
-    /* Linux drivers rely on the last byte being in the SIDL.  */
-    s->sidl = s->msg[len - 1];
-    s->msg_len -= len;
-    if (s->msg_len) {
-        memmove(s->msg, s->msg + len, s->msg_len);
-    } else {
+
+    if (len) {
+        pci_dma_write(PCI_DEVICE(s), s->dnad, s->msg, len);
+        /* Linux drivers rely on the last byte being in the SIDL.  */
+        s->sidl = s->msg[len - 1];
+        s->msg_len -= len;
+        if (s->msg_len) {
+            memmove(s->msg, s->msg + len, s->msg_len);
+        }
+    }
+
+    if (!s->msg_len) {
         /* ??? Check if ATN (not yet implemented) is asserted and maybe
            switch to PHASE_MO.  */
         switch (s->msg_action) {
-- 
2.44.0

[PULL 6/7] lsi53c895a: detect invalid Block Move instruction

[Paolo Bonzini]

The spec for the lsi53c895a says: "If the instruction is a Block Move
and a value of 0x000000 is loaded into the DBC register, an illegal
instruction interrupt occurs if the LSI53C895A is not in target mode,
Command phase".

Because QEMU only operates in initiator mode, generate the interrupt
unconditionally if the low 24 bits are 0x000000.

Reported-by: Chuhong Yuan <hslester96@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/lsi53c895a.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index eb9828dd5ef..1e18d88983b 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -1205,6 +1205,15 @@ again:
             break;
         }
         s->dbc = insn & 0xffffff;
+        if (!s->dbc) {
+            /*
+             * If the instruction is a Block Move and a value of 0x000000 is
+             * loaded into the DBC register, an illegal instruction interrupt
+             * occurs if the LSI53C895A is not in target mode, Command phase.
+             */
+            lsi_script_dma_interrupt(s, LSI_DSTAT_IID);
+            break;
+        }
         s->rbc = s->dbc;
         /* ??? Set ESA.  */
         s->ia = s->dsp - 8;
-- 
2.44.0

[PULL 7/7] pc_q35: remove unnecessary m->alias assignment

[Paolo Bonzini]

The assignment is already inherited from pc-q35-8.2.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/pc_q35.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index b5922b44afa..c7bc8a2041f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -393,7 +393,6 @@ static void pc_q35_8_1_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_8_2_machine_options(m);
-    m->alias = NULL;
     pcmc->broken_32bit_mem_addr_check = true;
     compat_props_add(m->compat_props, hw_compat_8_1, hw_compat_8_1_len);
     compat_props_add(m->compat_props, pc_compat_8_1, pc_compat_8_1_len);
-- 
2.44.0

Re: [PULL 0/7] lsi, vga fixes for 2024-04-02

[Peter Maydell]

On Tue, 2 Apr 2024 at 14:20, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The following changes since commit b9dbf6f9bf533564f6a4277d03906fcd32bb0245:
>
>   Merge tag 'pull-tcg-20240329' of https://gitlab.com/rth7680/qemu into staging (2024-03-30 14:54:57 +0000)
>
> are available in the Git repository at:
>
>   https://gitlab.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to eac4af186f6db46fc90ec571a855bd6fa4cb7841:
>
>   pc_q35: remove unnecessary m->alias assignment (2024-04-02 15:14:02 +0200)
>
> ----------------------------------------------------------------
> * lsi53c895a: fix assertion failure with invalid Block Move
> * vga: fix assertion failure with 4- and 16-color modes
> * remove unnecessary assignment
>
> ----------------------------------------------------------------
> Paolo Bonzini (7):
>       vga: merge conditionals on shift control register
>       vga: move computation of dirty memory region later
>       vga: adjust dirty memory region if pel panning is active
>       vga: do not treat horiz pel panning value of 8 as "enabled"
>       lsi53c895a: avoid out of bounds access to s->msg[]
>       lsi53c895a: detect invalid Block Move instruction
>       pc_q35: remove unnecessary m->alias assignment

This seems to break the avocado test
tests/avocado/ppc_prep_40p.py:IbmPrep40pMachine.test_openbios_and_netbsd

and it's consistent even with retrying the job:
https://gitlab.com/qemu-project/qemu/-/jobs/6529626987
https://gitlab.com/qemu-project/qemu/-/jobs/6528696711
https://gitlab.com/qemu-project/qemu/-/jobs/6529196532

The debug log says:

14:23:32 DEBUG| Transitioning from 'Runstate.CONNECTING' to 'Runstate.RUNNING'.
14:23:32 DEBUG| Opening console file
14:23:32 DEBUG| Opening console socket
14:23:32 DEBUG| >> =============================================================
14:23:32 DEBUG| >> OpenBIOS 1.1 [Mar 7 2023 22:21]
14:23:32 DEBUG| >> Configuration device id QEMU version 1 machine id 0
14:23:32 DEBUG| >> CPUs: 0
14:23:32 DEBUG| >> Memory: 128M
14:23:32 DEBUG| >> UUID: 00000000-0000-0000-0000-000000000000
14:23:32 DEBUG| >> CPU type PowerPC,604
14:23:32 DEBUG| milliseconds isn't unique.
14:23:32 DEBUG| Output device screen not found.
14:23:32 DEBUG| Output device screen not found.
14:23:32 DEBUG| Trying cd:,\\:tbxi...
14:23:32 DEBUG| Trying cd:,\ppc\bootinfo.txt...
14:23:32 DEBUG| Trying cd:,%BOOT...
14:23:32 DEBUG| No valid state has been set by load or init-program

and then the test times out because it never sees the NetBSD
console output it's waiting for.

Successful job for a previous pullreq:
https://gitlab.com/qemu-project/qemu/-/jobs/6527774374

Here the debug log says:

12:36:14 DEBUG| >> =============================================================
12:36:14 DEBUG| >> OpenBIOS 1.1 [Mar 7 2023 22:21]
12:36:14 DEBUG| >> Configuration device id QEMU version 1 machine id 0
12:36:14 DEBUG| >> CPUs: 0
12:36:14 DEBUG| >> Memory: 128M
12:36:14 DEBUG| >> UUID: 00000000-0000-0000-0000-000000000000
12:36:14 DEBUG| >> CPU type PowerPC,604
12:36:14 DEBUG| milliseconds isn't unique.
12:36:14 DEBUG| Output device screen not found.
12:36:14 DEBUG| Output device screen not found.
12:36:14 DEBUG| Trying cd:,\\:tbxi...
12:36:14 DEBUG| >> Not a bootable ELF image
12:36:15 DEBUG| >> switching to new context:
12:36:15 DEBUG| >> NetBSD/prep BOOT, Revision 1.9
12:36:15 DEBUG| Shutting down VM appliance; timeout=30
12:36:15 DEBUG| Attempting graceful termination
12:36:15 DEBUG| Closing console file
12:36:15 DEBUG| Closing console socket
12:36:15 DEBUG| Politely asking QEMU to terminate

This machine uses the lsi53c810 SCSI controller, and
it's failing to load from the CDROM, so my guess is the
problem is in one of the two SCSI patches.

thanks
-- PMM