All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] hw/display/artist: cursor & buffer mode fixes
@ 2022-01-21 22:16 Sven Schnelle
  2022-01-21 22:16 ` [PATCH 1/3] hw/display/artist: fix cursor position Sven Schnelle
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Sven Schnelle @ 2022-01-21 22:16 UTC (permalink / raw)
  To: Helge Deller, Gerd Hoffmann, Philippe Mathieu-Daudé; +Cc: qemu-devel

Hi List,

this set contains two fixes too make the cursor work with HP-UX' X-server,
and one large rewrite to make the Linux framebuffer work with artist on hppa.

Sven Schnelle (3):
  hw/display/artist: fix cursor position
  hw/display/artist: allow to disable/enable cursor
  hw/display/artist: rewrite vram access mode handling

 hw/display/artist.c     | 449 ++++++++++++++++------------------------
 hw/display/trace-events |   8 +-
 2 files changed, 184 insertions(+), 273 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] hw/display/artist: fix cursor position
  2022-01-21 22:16 [PATCH 0/3] hw/display/artist: cursor & buffer mode fixes Sven Schnelle
@ 2022-01-21 22:16 ` Sven Schnelle
  2022-01-21 22:16 ` [PATCH 2/3] hw/display/artist: allow to disable/enable cursor Sven Schnelle
  2022-01-21 22:16 ` [PATCH 3/3] hw/display/artist: rewrite vram access mode handling Sven Schnelle
  2 siblings, 0 replies; 8+ messages in thread
From: Sven Schnelle @ 2022-01-21 22:16 UTC (permalink / raw)
  To: Helge Deller, Gerd Hoffmann, Philippe Mathieu-Daudé; +Cc: qemu-devel

Register 0x300200 and 0x300208 seems to be used as scratch register
by HP-UX for cursor offset data. It writes a calculated value on X
startup, and later reads it back and uses this as offset for all
cursor movements. I couldn't figure how this number is calculated,
but forcing it to a fixed value fixes the cursor position problems
for all HP-UX versions.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 hw/display/artist.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/hw/display/artist.c b/hw/display/artist.c
index 21b7fd1b44..7956a1a5c3 100644
--- a/hw/display/artist.c
+++ b/hw/display/artist.c
@@ -326,15 +326,8 @@ static void artist_rop8(ARTISTState *s, struct vram_buffer *buf,
 
 static void artist_get_cursor_pos(ARTISTState *s, int *x, int *y)
 {
-    /*
-     * Don't know whether these magic offset values are configurable via
-     * some register. They are the same for all resolutions, so don't
-     * bother about it.
-     */
-
-    *y = 0x47a - artist_get_y(s->cursor_pos);
-    *x = ((artist_get_x(s->cursor_pos) - 338) / 2);
-
+    *y = 0x400 - artist_get_y(s->cursor_pos);
+    *x = (artist_get_x(s->cursor_pos) + 16) / 2;
     if (*x > s->width) {
         *x = 0;
     }
@@ -1122,11 +1115,15 @@ static uint64_t artist_reg_read(void *opaque, hwaddr addr, unsigned size)
         break;
 
     case 0x300200:
-        val = s->reg_300200;
-        break;
-
     case 0x300208:
-        val = s->reg_300208;
+        /*
+         * Seems to be relevant to cursor position, likely a scratch register.
+         * HP-UX initializes this with different values depending on version.
+         * Best guess is that this number is generated from STI data or other
+         * registers. I couldn't figure out how this number is generated. For
+         * now hardcode it to a number generating a zero cursor offset.
+         */
+        val = 0x00f01000;
         break;
 
     case 0x300218:
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] hw/display/artist: allow to disable/enable cursor
  2022-01-21 22:16 [PATCH 0/3] hw/display/artist: cursor & buffer mode fixes Sven Schnelle
  2022-01-21 22:16 ` [PATCH 1/3] hw/display/artist: fix cursor position Sven Schnelle
@ 2022-01-21 22:16 ` Sven Schnelle
  2022-01-25  8:11   ` Gerd Hoffmann
  2022-01-21 22:16 ` [PATCH 3/3] hw/display/artist: rewrite vram access mode handling Sven Schnelle
  2 siblings, 1 reply; 8+ messages in thread
From: Sven Schnelle @ 2022-01-21 22:16 UTC (permalink / raw)
  To: Helge Deller, Gerd Hoffmann, Philippe Mathieu-Daudé; +Cc: qemu-devel

Allow to disable/enable the cursor via the cursor control
register.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 hw/display/artist.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/display/artist.c b/hw/display/artist.c
index 7956a1a5c3..cfae92d3e8 100644
--- a/hw/display/artist.c
+++ b/hw/display/artist.c
@@ -80,6 +80,7 @@ struct ARTISTState {
     uint32_t line_pattern_skip;
 
     uint32_t cursor_pos;
+    uint32_t cursor_cntl;
 
     uint32_t cursor_height;
     uint32_t cursor_width;
@@ -1027,6 +1028,8 @@ static void artist_reg_write(void *opaque, hwaddr addr, uint64_t val,
         break;
 
     case CURSOR_CTRL:
+        combine_write_reg(addr, val, size, &s->cursor_cntl);
+        artist_invalidate_cursor(s);
         break;
 
     case IMAGE_BITMAP_OP:
@@ -1320,7 +1323,9 @@ static void artist_update_display(void *opaque)
                                s->width, s->width * 4, 0, 0, artist_draw_line,
                                s, &first, &last);
 
-    artist_draw_cursor(s);
+    if (s->cursor_cntl & 0x80) {
+        artist_draw_cursor(s);
+    }
 
     dpy_gfx_update(s->con, 0, 0, s->width, s->height);
 }
@@ -1419,7 +1424,7 @@ static int vmstate_artist_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_artist = {
     .name = "artist",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .post_load = vmstate_artist_post_load,
     .fields = (VMStateField[]) {
@@ -1440,6 +1445,7 @@ static const VMStateDescription vmstate_artist = {
         VMSTATE_UINT32(line_end, ARTISTState),
         VMSTATE_UINT32(line_xy, ARTISTState),
         VMSTATE_UINT32(cursor_pos, ARTISTState),
+        VMSTATE_UINT32(cursor_cntl, ARTISTState),
         VMSTATE_UINT32(cursor_height, ARTISTState),
         VMSTATE_UINT32(cursor_width, ARTISTState),
         VMSTATE_UINT32(plane_mask, ARTISTState),
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] hw/display/artist: rewrite vram access mode handling
  2022-01-21 22:16 [PATCH 0/3] hw/display/artist: cursor & buffer mode fixes Sven Schnelle
  2022-01-21 22:16 ` [PATCH 1/3] hw/display/artist: fix cursor position Sven Schnelle
  2022-01-21 22:16 ` [PATCH 2/3] hw/display/artist: allow to disable/enable cursor Sven Schnelle
@ 2022-01-21 22:16 ` Sven Schnelle
  2022-01-25  8:25   ` Gerd Hoffmann
  2 siblings, 1 reply; 8+ messages in thread
From: Sven Schnelle @ 2022-01-21 22:16 UTC (permalink / raw)
  To: Helge Deller, Gerd Hoffmann, Philippe Mathieu-Daudé; +Cc: qemu-devel

When writing this code it was assumed that register 0x118000 is the
buffer access mode for color map accesses. It turned out that this
is wrong. Instead register 0x118000 sets both src and dst buffer
access mode at the same time.

This required a larger rewrite of the code. The good thing is that
both the linear framebuffer and the register based vram access can
now be combined into one function.

This makes the linux 'stifb' framebuffer work, and both HP-UX 10.20
and HP-UX 11.11 are still working.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 hw/display/artist.c     | 416 ++++++++++++++++------------------------
 hw/display/trace-events |   8 +-
 2 files changed, 166 insertions(+), 258 deletions(-)

diff --git a/hw/display/artist.c b/hw/display/artist.c
index cfae92d3e8..04db74d240 100644
--- a/hw/display/artist.c
+++ b/hw/display/artist.c
@@ -92,7 +92,6 @@ struct ARTISTState {
     uint32_t reg_300208;
     uint32_t reg_300218;
 
-    uint32_t cmap_bm_access;
     uint32_t dst_bm_access;
     uint32_t src_bm_access;
     uint32_t control_plane;
@@ -135,7 +134,7 @@ typedef enum {
     PATTERN_LINE_START = 0x100ecc,
     LINE_SIZE = 0x100e04,
     LINE_END = 0x100e44,
-    CMAP_BM_ACCESS = 0x118000,
+    DST_SRC_BM_ACCESS = 0x118000,
     DST_BM_ACCESS = 0x118004,
     SRC_BM_ACCESS = 0x118008,
     CONTROL_PLANE = 0x11800c,
@@ -177,7 +176,7 @@ static const char *artist_reg_name(uint64_t addr)
     REG_NAME(TRANSFER_DATA);
     REG_NAME(CONTROL_PLANE);
     REG_NAME(IMAGE_BITMAP_OP);
-    REG_NAME(CMAP_BM_ACCESS);
+    REG_NAME(DST_SRC_BM_ACCESS);
     REG_NAME(DST_BM_ACCESS);
     REG_NAME(SRC_BM_ACCESS);
     REG_NAME(CURSOR_POS);
@@ -223,40 +222,14 @@ static void artist_invalidate_lines(struct vram_buffer *buf,
     }
 }
 
-static int vram_write_pix_per_transfer(ARTISTState *s)
-{
-    if (s->cmap_bm_access) {
-        return 1 << ((s->cmap_bm_access >> 27) & 0x0f);
-    } else {
-        return 1 << ((s->dst_bm_access >> 27) & 0x0f);
-    }
-}
-
-static int vram_pixel_length(ARTISTState *s)
-{
-    if (s->cmap_bm_access) {
-        return (s->cmap_bm_access >> 24) & 0x07;
-    } else {
-        return (s->dst_bm_access >> 24) & 0x07;
-    }
-}
-
 static int vram_write_bufidx(ARTISTState *s)
 {
-    if (s->cmap_bm_access) {
-        return (s->cmap_bm_access >> 12) & 0x0f;
-    } else {
-        return (s->dst_bm_access >> 12) & 0x0f;
-    }
+    return (s->dst_bm_access >> 12) & 0x0f;
 }
 
 static int vram_read_bufidx(ARTISTState *s)
 {
-    if (s->cmap_bm_access) {
-        return (s->cmap_bm_access >> 12) & 0x0f;
-    } else {
-        return (s->src_bm_access >> 12) & 0x0f;
-    }
+    return (s->src_bm_access >> 12) & 0x0f;
 }
 
 static struct vram_buffer *vram_read_buffer(ARTISTState *s)
@@ -346,130 +319,6 @@ static void artist_invalidate_cursor(ARTISTState *s)
                             y, s->cursor_height);
 }
 
-static void vram_bit_write(ARTISTState *s, int posy, bool incr_x,
-                           int size, uint32_t data)
-{
-    struct vram_buffer *buf;
-    uint32_t vram_bitmask = s->vram_bitmask;
-    int mask, i, pix_count, pix_length;
-    unsigned int posx, offset, width;
-    uint8_t *data8, *p;
-
-    pix_count = vram_write_pix_per_transfer(s);
-    pix_length = vram_pixel_length(s);
-
-    buf = vram_write_buffer(s);
-    width = buf->width;
-
-    if (s->cmap_bm_access) {
-        offset = s->vram_pos;
-    } else {
-        posx = ADDR_TO_X(s->vram_pos >> 2);
-        posy += ADDR_TO_Y(s->vram_pos >> 2);
-        offset = posy * width + posx;
-    }
-
-    if (!buf->size || offset >= buf->size) {
-        return;
-    }
-
-    p = buf->data;
-
-    if (pix_count > size * 8) {
-        pix_count = size * 8;
-    }
-
-    switch (pix_length) {
-    case 0:
-        if (s->image_bitmap_op & 0x20000000) {
-            data &= vram_bitmask;
-        }
-
-        for (i = 0; i < pix_count; i++) {
-            uint32_t off = offset + pix_count - 1 - i;
-            if (off < buf->size) {
-                artist_rop8(s, buf, off,
-                            (data & 1) ? (s->plane_mask >> 24) : 0);
-            }
-            data >>= 1;
-        }
-        memory_region_set_dirty(&buf->mr, offset, pix_count);
-        break;
-
-    case 3:
-        if (s->cmap_bm_access) {
-            if (offset + 3 < buf->size) {
-                *(uint32_t *)(p + offset) = data;
-            }
-            break;
-        }
-        data8 = (uint8_t *)&data;
-
-        for (i = 3; i >= 0; i--) {
-            if (!(s->image_bitmap_op & 0x20000000) ||
-                s->vram_bitmask & (1 << (28 + i))) {
-                uint32_t off = offset + 3 - i;
-                if (off < buf->size) {
-                    artist_rop8(s, buf, off, data8[ROP8OFF(i)]);
-                }
-            }
-        }
-        memory_region_set_dirty(&buf->mr, offset, 3);
-        break;
-
-    case 6:
-        switch (size) {
-        default:
-        case 4:
-            vram_bitmask = s->vram_bitmask;
-            break;
-
-        case 2:
-            vram_bitmask = s->vram_bitmask >> 16;
-            break;
-
-        case 1:
-            vram_bitmask = s->vram_bitmask >> 24;
-            break;
-        }
-
-        for (i = 0; i < pix_count && offset + i < buf->size; i++) {
-            mask = 1 << (pix_count - 1 - i);
-
-            if (!(s->image_bitmap_op & 0x20000000) ||
-                (vram_bitmask & mask)) {
-                if (data & mask) {
-                    artist_rop8(s, buf, offset + i, s->fg_color);
-                } else {
-                    if (!(s->image_bitmap_op & 0x10000002)) {
-                        artist_rop8(s, buf, offset + i, s->bg_color);
-                    }
-                }
-            }
-        }
-        memory_region_set_dirty(&buf->mr, offset, pix_count);
-        break;
-
-    default:
-        qemu_log_mask(LOG_UNIMP, "%s: unknown pixel length %d\n",
-                      __func__, pix_length);
-        break;
-    }
-
-    if (incr_x) {
-        if (s->cmap_bm_access) {
-            s->vram_pos += 4;
-        } else {
-            s->vram_pos += pix_count << 2;
-        }
-    }
-
-    if (vram_write_bufidx(s) == ARTIST_BUFFER_CURSOR1 ||
-        vram_write_bufidx(s) == ARTIST_BUFFER_CURSOR2) {
-        artist_invalidate_cursor(s);
-    }
-}
-
 static void block_move(ARTISTState *s,
                        unsigned int source_x, unsigned int source_y,
                        unsigned int dest_x,   unsigned int dest_y,
@@ -854,6 +703,151 @@ static void combine_write_reg(hwaddr addr, uint64_t val, int size, void *out)
     }
 }
 
+static void artist_vram_write4(ARTISTState *s, struct vram_buffer *buf,
+                               uint32_t offset, uint32_t data)
+{
+    int i;
+    int mask = s->vram_bitmask >> 28;
+
+    for (i = 0; i < 4; i++) {
+        if (!(s->image_bitmap_op & 0x20000000) || (mask & 8)) {
+            artist_rop8(s, buf, offset + i, data >> 24);
+            data <<= 8;
+            mask <<= 1;
+        }
+    }
+    memory_region_set_dirty(&buf->mr, offset, 3);
+}
+
+static void artist_vram_write32(ARTISTState *s, struct vram_buffer *buf,
+                                uint32_t offset, int size, uint32_t data,
+                                int fg, int bg)
+{
+    uint32_t mask, vram_bitmask = s->vram_bitmask >> ((4 - size) * 8);
+    int i, pix_count = size * 8;
+
+    for (i = 0; i < pix_count && offset + i < buf->size; i++) {
+        mask = 1 << (pix_count - 1 - i);
+
+        if (!(s->image_bitmap_op & 0x20000000) || (vram_bitmask & mask)) {
+            if (data & mask) {
+                artist_rop8(s, buf, offset + i, fg);
+            } else {
+                if (!(s->image_bitmap_op & 0x10000002)) {
+                    artist_rop8(s, buf, offset + i, bg);
+                }
+            }
+        }
+    }
+    memory_region_set_dirty(&buf->mr, offset, pix_count);
+}
+
+static int get_vram_offset(ARTISTState *s, struct vram_buffer *buf,
+                           int pos, int posy)
+{
+    unsigned int posx, width;
+
+    width = buf->width;
+    posx = ADDR_TO_X(pos);
+    posy += ADDR_TO_Y(pos);
+    return posy * width + posx;
+}
+
+static int vram_bit_write(ARTISTState *s, uint32_t pos, int posy,
+                          uint32_t data, int size)
+{
+    struct vram_buffer *buf = vram_write_buffer(s);
+
+    switch (s->dst_bm_access >> 16) {
+    case 0x3ba0:
+    case 0xbbe0:
+        artist_vram_write4(s, buf, pos, bswap32(data));
+        pos += 4;
+        break;
+
+    case 0x1360: /* linux */
+        artist_vram_write4(s, buf, get_vram_offset(s, buf, pos, posy), data);
+        pos += 4;
+        break;
+
+    case 0x13a0:
+        artist_vram_write4(s, buf, get_vram_offset(s, buf, pos >> 2, posy),
+                           data);
+        pos += 16;
+        break;
+
+    case 0x2ea0:
+        artist_vram_write32(s, buf, get_vram_offset(s, buf, pos >> 2, posy),
+                            size, data, s->fg_color, s->bg_color);
+        pos += 4;
+        break;
+
+    case 0x28a0:
+        artist_vram_write32(s, buf, get_vram_offset(s, buf, pos >> 2, posy),
+                            size, data, 1, 0);
+        pos += 4;
+        break;
+
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: unknown dst bm access %08x\n",
+                      __func__, s->dst_bm_access);
+        break;
+    }
+
+    if (vram_write_bufidx(s) == ARTIST_BUFFER_CURSOR1 ||
+        vram_write_bufidx(s) == ARTIST_BUFFER_CURSOR2) {
+        artist_invalidate_cursor(s);
+    }
+    return pos;
+}
+
+static void artist_vram_write(void *opaque, hwaddr addr, uint64_t val,
+                              unsigned size)
+{
+    ARTISTState *s = opaque;
+    s->vram_char_y = 0;
+    trace_artist_vram_write(size, addr, val);
+    vram_bit_write(opaque, addr, 0, val, size);
+}
+
+static uint64_t artist_vram_read(void *opaque, hwaddr addr, unsigned size)
+{
+    ARTISTState *s = opaque;
+    struct vram_buffer *buf;
+    unsigned int offset;
+    uint64_t val;
+
+    buf = vram_read_buffer(s);
+    if (!buf->size) {
+        return 0;
+    }
+
+    offset = get_vram_offset(s, buf, addr >> 2, 0);
+
+    if (offset > buf->size) {
+        return 0;
+    }
+
+    switch (s->src_bm_access >> 16) {
+    case 0x3ba0:
+        val = *(uint32_t *)(buf->data + offset);
+        break;
+
+    case 0x13a0:
+    case 0x2ea0:
+        val = bswap32(*(uint32_t *)(buf->data + offset));
+        break;
+
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: unknown src bm access %08x\n",
+                      __func__, s->dst_bm_access);
+        val = -1ULL;
+        break;
+    }
+    trace_artist_vram_read(size, addr, val);
+    return val;
+}
+
 static void artist_reg_write(void *opaque, hwaddr addr, uint64_t val,
                              unsigned size)
 {
@@ -880,12 +874,12 @@ static void artist_reg_write(void *opaque, hwaddr addr, uint64_t val,
         break;
 
     case VRAM_WRITE_INCR_Y:
-        vram_bit_write(s, s->vram_char_y++, false, size, val);
+        vram_bit_write(s, s->vram_pos, s->vram_char_y++, val, size);
         break;
 
     case VRAM_WRITE_INCR_X:
     case VRAM_WRITE_INCR_X2:
-        vram_bit_write(s, s->vram_char_y, true, size, val);
+        s->vram_pos = vram_bit_write(s, s->vram_pos, s->vram_char_y, val, size);
         break;
 
     case VRAM_IDX:
@@ -987,18 +981,17 @@ static void artist_reg_write(void *opaque, hwaddr addr, uint64_t val,
         combine_write_reg(addr, val, size, &s->plane_mask);
         break;
 
-    case CMAP_BM_ACCESS:
-        combine_write_reg(addr, val, size, &s->cmap_bm_access);
+    case DST_SRC_BM_ACCESS:
+        combine_write_reg(addr, val, size, &s->dst_bm_access);
+        combine_write_reg(addr, val, size, &s->src_bm_access);
         break;
 
     case DST_BM_ACCESS:
         combine_write_reg(addr, val, size, &s->dst_bm_access);
-        s->cmap_bm_access = 0;
         break;
 
     case SRC_BM_ACCESS:
         combine_write_reg(addr, val, size, &s->src_bm_access);
-        s->cmap_bm_access = 0;
         break;
 
     case CONTROL_PLANE:
@@ -1152,98 +1145,6 @@ static uint64_t artist_reg_read(void *opaque, hwaddr addr, unsigned size)
     return val;
 }
 
-static void artist_vram_write(void *opaque, hwaddr addr, uint64_t val,
-                              unsigned size)
-{
-    ARTISTState *s = opaque;
-    struct vram_buffer *buf;
-    unsigned int posy, posx;
-    unsigned int offset;
-    trace_artist_vram_write(size, addr, val);
-
-    if (s->cmap_bm_access) {
-        buf = &s->vram_buffer[ARTIST_BUFFER_CMAP];
-        if (addr + 3 < buf->size) {
-            *(uint32_t *)(buf->data + addr) = val;
-        }
-        return;
-    }
-
-    buf = vram_write_buffer(s);
-    posy = ADDR_TO_Y(addr >> 2);
-    posx = ADDR_TO_X(addr >> 2);
-
-    if (!buf->size) {
-        return;
-    }
-
-    if (posy > buf->height || posx > buf->width) {
-        return;
-    }
-
-    offset = posy * buf->width + posx;
-    if (offset >= buf->size) {
-        return;
-    }
-
-    switch (size) {
-    case 4:
-        if (offset + 3 < buf->size) {
-            *(uint32_t *)(buf->data + offset) = be32_to_cpu(val);
-            memory_region_set_dirty(&buf->mr, offset, 4);
-        }
-        break;
-    case 2:
-        if (offset + 1 < buf->size) {
-            *(uint16_t *)(buf->data + offset) = be16_to_cpu(val);
-            memory_region_set_dirty(&buf->mr, offset, 2);
-        }
-        break;
-    case 1:
-        if (offset < buf->size) {
-            *(uint8_t *)(buf->data + offset) = val;
-            memory_region_set_dirty(&buf->mr, offset, 1);
-        }
-        break;
-    default:
-        break;
-    }
-}
-
-static uint64_t artist_vram_read(void *opaque, hwaddr addr, unsigned size)
-{
-    ARTISTState *s = opaque;
-    struct vram_buffer *buf;
-    uint64_t val;
-    unsigned int posy, posx;
-
-    if (s->cmap_bm_access) {
-        buf = &s->vram_buffer[ARTIST_BUFFER_CMAP];
-        val = 0;
-        if (addr < buf->size && addr + 3 < buf->size) {
-            val = *(uint32_t *)(buf->data + addr);
-        }
-        trace_artist_vram_read(size, addr, 0, 0, val);
-        return val;
-    }
-
-    buf = vram_read_buffer(s);
-    if (!buf->size) {
-        return 0;
-    }
-
-    posy = ADDR_TO_Y(addr >> 2);
-    posx = ADDR_TO_X(addr >> 2);
-
-    if (posy > buf->height || posx > buf->width) {
-        return 0;
-    }
-
-    val = cpu_to_be32(*(uint32_t *)(buf->data + posy * buf->width + posx));
-    trace_artist_vram_read(size, addr, posx, posy, val);
-    return val;
-}
-
 static const MemoryRegionOps artist_reg_ops = {
     .read = artist_reg_read,
     .write = artist_reg_write,
@@ -1412,6 +1313,14 @@ static void artist_realizefn(DeviceState *dev, Error **errp)
     s->cursor_height = 32;
     s->cursor_width = 32;
 
+    /*
+     * These two registers are not initialized by seabios's STI implementation.
+     * Initialize them here to sane values so artist also works with older
+     * (not-fixed) seabios versions.
+     */
+    s->image_bitmap_op = 0x23000300;
+    s->plane_mask = 0xff;
+
     s->con = graphic_console_init(dev, 0, &artist_ops, s);
     qemu_console_resize(s->con, s->width, s->height);
 }
@@ -1453,7 +1362,6 @@ static const VMStateDescription vmstate_artist = {
         VMSTATE_UINT32(reg_300200, ARTISTState),
         VMSTATE_UINT32(reg_300208, ARTISTState),
         VMSTATE_UINT32(reg_300218, ARTISTState),
-        VMSTATE_UINT32(cmap_bm_access, ARTISTState),
         VMSTATE_UINT32(dst_bm_access, ARTISTState),
         VMSTATE_UINT32(src_bm_access, ARTISTState),
         VMSTATE_UINT32(control_plane, ARTISTState),
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 3a7a2c957f..4a687d1b8e 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -140,10 +140,10 @@ ati_mm_read(unsigned int size, uint64_t addr, const char *name, uint64_t val) "%
 ati_mm_write(unsigned int size, uint64_t addr, const char *name, uint64_t val) "%u 0x%"PRIx64 " %s <- 0x%"PRIx64
 
 # artist.c
-artist_reg_read(unsigned int size, uint64_t addr, const char *name, uint64_t val) "%u 0x%"PRIx64 "%s -> 0x%"PRIx64
-artist_reg_write(unsigned int size, uint64_t addr, const char *name, uint64_t val) "%u 0x%"PRIx64 "%s <- 0x%"PRIx64
-artist_vram_read(unsigned int size, uint64_t addr, int posx, int posy, uint64_t val) "%u 0x%"PRIx64 " %ux%u-> 0x%"PRIx64
-artist_vram_write(unsigned int size, uint64_t addr, uint64_t val) "%u 0x%"PRIx64 " <- 0x%"PRIx64
+artist_reg_read(unsigned int size, uint64_t addr, const char *name, uint64_t val) "%u 0x%"PRIx64 "%s -> 0x%08"PRIx64
+artist_reg_write(unsigned int size, uint64_t addr, const char *name, uint64_t val) "%u 0x%"PRIx64 "%s <- 0x%08"PRIx64
+artist_vram_read(unsigned int size, uint64_t addr, uint64_t val) "%u 0x%08"PRIx64 " -> 0x%08"PRIx64
+artist_vram_write(unsigned int size, uint64_t addr, uint64_t val) "%u 0x%08"PRIx64 " <- 0x%08"PRIx64
 artist_fill_window(unsigned int start_x, unsigned int start_y, unsigned int width, unsigned int height, uint32_t op, uint32_t ctlpln) "start=%ux%u length=%ux%u op=0x%08x ctlpln=0x%08x"
 artist_block_move(unsigned int start_x, unsigned int start_y, unsigned int dest_x, unsigned int dest_y, unsigned int width, unsigned int height) "source %ux%u -> dest %ux%u size %ux%u"
 artist_draw_line(unsigned int start_x, unsigned int start_y, unsigned int end_x, unsigned int end_y) "%ux%u %ux%u"
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] hw/display/artist: allow to disable/enable cursor
  2022-01-21 22:16 ` [PATCH 2/3] hw/display/artist: allow to disable/enable cursor Sven Schnelle
@ 2022-01-25  8:11   ` Gerd Hoffmann
  0 siblings, 0 replies; 8+ messages in thread
From: Gerd Hoffmann @ 2022-01-25  8:11 UTC (permalink / raw)
  To: Sven Schnelle; +Cc: Helge Deller, Philippe Mathieu-Daudé, qemu-devel

> @@ -1419,7 +1424,7 @@ static int vmstate_artist_post_load(void *opaque, int version_id)
>  
>  static const VMStateDescription vmstate_artist = {
>      .name = "artist",
> -    .version_id = 1,
> +    .version_id = 2,

Which machines use that device?  Usually we try avoid bumping the
version as this is a one-way ticket (migration to newer versions works,
but back to older doesn't).  But for machine typess which are not
versioned this isn't that much of a problem.

>      .minimum_version_id = 1,
>      .post_load = vmstate_artist_post_load,
>      .fields = (VMStateField[]) {
> @@ -1440,6 +1445,7 @@ static const VMStateDescription vmstate_artist = {
>          VMSTATE_UINT32(line_end, ARTISTState),
>          VMSTATE_UINT32(line_xy, ARTISTState),
>          VMSTATE_UINT32(cursor_pos, ARTISTState),
> +        VMSTATE_UINT32(cursor_cntl, ARTISTState),

You need another variant of that macro (VMSTATE_UINT32_V() IIRC) to
declare that field as "new in v2".

take care,
  Gerd



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] hw/display/artist: rewrite vram access mode handling
  2022-01-21 22:16 ` [PATCH 3/3] hw/display/artist: rewrite vram access mode handling Sven Schnelle
@ 2022-01-25  8:25   ` Gerd Hoffmann
  2022-01-25 16:29     ` Sven Schnelle
  0 siblings, 1 reply; 8+ messages in thread
From: Gerd Hoffmann @ 2022-01-25  8:25 UTC (permalink / raw)
  To: Sven Schnelle; +Cc: Helge Deller, Philippe Mathieu-Daudé, qemu-devel

  Hi,

> +static void artist_vram_write4(ARTISTState *s, struct vram_buffer *buf,
> +                               uint32_t offset, uint32_t data)

> +static int get_vram_offset(ARTISTState *s, struct vram_buffer *buf,
> +                           int pos, int posy)

> +    case 0x13a0:
> +        artist_vram_write4(s, buf, get_vram_offset(s, buf, pos >> 2, posy),
> +                           data);

That is asking for trouble.

You should pass around offsets not pointers.  An offset can trivially be
checked whenever it is within the valid range (i.e. smaller than vram
size), or it can be masked to strip off high bits when accessing virtual
vram.  You need that for robustness and security reasons (i.e. make sure
the guest can't write to host memory by tricking your get_vram_offset
calculations).

grep cirrus for 'cirrus_addr_mask' to see sample code.

take care,
  Gerd



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] hw/display/artist: rewrite vram access mode handling
  2022-01-25  8:25   ` Gerd Hoffmann
@ 2022-01-25 16:29     ` Sven Schnelle
  2022-01-26  9:08       ` Gerd Hoffmann
  0 siblings, 1 reply; 8+ messages in thread
From: Sven Schnelle @ 2022-01-25 16:29 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Helge Deller, Philippe Mathieu-Daudé, qemu-devel

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> +static void artist_vram_write4(ARTISTState *s, struct vram_buffer *buf,
>> +                               uint32_t offset, uint32_t data)
>
>> +static int get_vram_offset(ARTISTState *s, struct vram_buffer *buf,
>> +                           int pos, int posy)
>
>> +    case 0x13a0:
>> +        artist_vram_write4(s, buf, get_vram_offset(s, buf, pos >> 2, posy),
>> +                           data);
>
> That is asking for trouble.
>
> You should pass around offsets not pointers.  An offset can trivially be
> checked whenever it is within the valid range (i.e. smaller than vram
> size), or it can be masked to strip off high bits when accessing virtual
> vram.  You need that for robustness and security reasons (i.e. make sure
> the guest can't write to host memory by tricking your get_vram_offset
> calculations).

I'm not sure i understand the problem. get_vram_offset() returns an
offset, which is passed to artist_vram_write4() which itself doesn't
do anything on the buffer. artist_rop8() in the end accesses the buffer,
and that function checks whether it's < buf->size. Can you elaborate
a bit more? Maybe it's just so obvious that i don't see it.

Thanks,
Sven


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] hw/display/artist: rewrite vram access mode handling
  2022-01-25 16:29     ` Sven Schnelle
@ 2022-01-26  9:08       ` Gerd Hoffmann
  0 siblings, 0 replies; 8+ messages in thread
From: Gerd Hoffmann @ 2022-01-26  9:08 UTC (permalink / raw)
  To: Sven Schnelle; +Cc: Helge Deller, Philippe Mathieu-Daudé, qemu-devel

On Tue, Jan 25, 2022 at 05:29:53PM +0100, Sven Schnelle wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
> >   Hi,
> >
> >> +static void artist_vram_write4(ARTISTState *s, struct vram_buffer *buf,
> >> +                               uint32_t offset, uint32_t data)
> >
> > You should pass around offsets not pointers.  An offset can trivially be
> > checked whenever it is within the valid range (i.e. smaller than vram
> > size), or it can be masked to strip off high bits when accessing virtual
> > vram.  You need that for robustness and security reasons (i.e. make sure
> > the guest can't write to host memory by tricking your get_vram_offset
> > calculations).
> 
> I'm not sure i understand the problem. get_vram_offset() returns an
> offset, which is passed to artist_vram_write4() which itself doesn't
> do anything on the buffer. artist_rop8() in the end accesses the buffer,
> and that function checks whether it's < buf->size. Can you elaborate
> a bit more? Maybe it's just so obvious that i don't see it.

Oops.  Sorry,  Didn't look carefully enough.  You are passing on both
buffer and offset, that is fine.  Missed that because the offset comes
after the line wrap, and also not enough coffee ...

I just had a flashback to the series of CVEs we had for the cirrus
blitter, which used to calculate pointers from buffer + offset, then
passed on those pointers to other functions which where unable to
sanity-check those pointers because all context was gone.

take care,
  Gerd



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-01-26  9:09 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-21 22:16 [PATCH 0/3] hw/display/artist: cursor & buffer mode fixes Sven Schnelle
2022-01-21 22:16 ` [PATCH 1/3] hw/display/artist: fix cursor position Sven Schnelle
2022-01-21 22:16 ` [PATCH 2/3] hw/display/artist: allow to disable/enable cursor Sven Schnelle
2022-01-25  8:11   ` Gerd Hoffmann
2022-01-21 22:16 ` [PATCH 3/3] hw/display/artist: rewrite vram access mode handling Sven Schnelle
2022-01-25  8:25   ` Gerd Hoffmann
2022-01-25 16:29     ` Sven Schnelle
2022-01-26  9:08       ` Gerd Hoffmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.