* [PATCH 3/6] sm501: Use BIT(x) macro to shorten constant
2020-05-20 13:39 [PATCH 0/6] Misc display/sm501 clean ups and fixes BALATON Zoltan
@ 2020-05-20 13:39 ` BALATON Zoltan
2020-05-21 14:10 ` Philippe Mathieu-Daudé
2020-05-20 13:39 ` [PATCH 6/6] sm501: Remove obsolete changelog and todo comment BALATON Zoltan
` (5 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: BALATON Zoltan @ 2020-05-20 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sebastian Bauer, Magnus Damm, Gerd Hoffmann,
Aurelien Jarno
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/display/sm501.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index f42d05e1e4..97660090bb 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -701,7 +701,7 @@ static void sm501_2d_operation(SM501State *s)
{
/* obtain operation parameters */
int cmd = (s->twoD_control >> 16) & 0x1F;
- int rtl = s->twoD_control & 0x8000000;
+ int rtl = s->twoD_control & BIT(27);
int src_x = (s->twoD_source >> 16) & 0x01FFF;
int src_y = s->twoD_source & 0xFFFF;
int dst_x = (s->twoD_destination >> 16) & 0x01FFF;
@@ -751,8 +751,7 @@ static void sm501_2d_operation(SM501State *s)
}
}
- if ((s->twoD_source_base & 0x08000000) ||
- (s->twoD_destination_base & 0x08000000)) {
+ if (s->twoD_source_base & BIT(27) || s->twoD_destination_base & BIT(27)) {
qemu_log_mask(LOG_UNIMP, "sm501: only local memory is supported.\n");
return;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 3/6] sm501: Use BIT(x) macro to shorten constant
2020-05-20 13:39 ` [PATCH 3/6] sm501: Use BIT(x) macro to shorten constant BALATON Zoltan
@ 2020-05-21 14:10 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-21 14:10 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel
Cc: Peter Maydell, Sebastian Bauer, Magnus Damm, Gerd Hoffmann,
Aurelien Jarno
On 5/20/20 3:39 PM, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/display/sm501.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index f42d05e1e4..97660090bb 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -701,7 +701,7 @@ static void sm501_2d_operation(SM501State *s)
> {
> /* obtain operation parameters */
> int cmd = (s->twoD_control >> 16) & 0x1F;
> - int rtl = s->twoD_control & 0x8000000;
> + int rtl = s->twoD_control & BIT(27);
> int src_x = (s->twoD_source >> 16) & 0x01FFF;
> int src_y = s->twoD_source & 0xFFFF;
> int dst_x = (s->twoD_destination >> 16) & 0x01FFF;
> @@ -751,8 +751,7 @@ static void sm501_2d_operation(SM501State *s)
> }
> }
>
> - if ((s->twoD_source_base & 0x08000000) ||
> - (s->twoD_destination_base & 0x08000000)) {
> + if (s->twoD_source_base & BIT(27) || s->twoD_destination_base & BIT(27)) {
> qemu_log_mask(LOG_UNIMP, "sm501: only local memory is supported.\n");
> return;
> }
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 6/6] sm501: Remove obsolete changelog and todo comment
2020-05-20 13:39 [PATCH 0/6] Misc display/sm501 clean ups and fixes BALATON Zoltan
2020-05-20 13:39 ` [PATCH 3/6] sm501: Use BIT(x) macro to shorten constant BALATON Zoltan
@ 2020-05-20 13:39 ` BALATON Zoltan
2020-05-21 14:08 ` Philippe Mathieu-Daudé
2020-05-20 13:39 ` [PATCH 1/6] sm501: Convert printf + abort to qemu_log_mask BALATON Zoltan
` (4 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: BALATON Zoltan @ 2020-05-20 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sebastian Bauer, Magnus Damm, Gerd Hoffmann,
Aurelien Jarno
Also update copyright year for latest changes
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/display/sm501.c | 19 +------------------
1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 13269cc9f4..b76b691674 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -2,7 +2,7 @@
* QEMU SM501 Device
*
* Copyright (c) 2008 Shin-ichiro KAWASAKI
- * Copyright (c) 2016 BALATON Zoltan
+ * Copyright (c) 2016-2020 BALATON Zoltan
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
@@ -40,23 +40,6 @@
#include "ui/pixel_ops.h"
#include "qemu/bswap.h"
-/*
- * Status: 2010/05/07
- * - Minimum implementation for Linux console : mmio regs and CRT layer.
- * - 2D graphics acceleration partially supported : only fill rectangle.
- *
- * Status: 2016/12/04
- * - Misc fixes: endianness, hardware cursor
- * - Panel support
- *
- * TODO:
- * - Touch panel support
- * - USB support
- * - UART support
- * - More 2D graphics engine support
- * - Performance tuning
- */
-
/*#define DEBUG_SM501*/
/*#define DEBUG_BITBLT*/
--
2.21.3
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 6/6] sm501: Remove obsolete changelog and todo comment
2020-05-20 13:39 ` [PATCH 6/6] sm501: Remove obsolete changelog and todo comment BALATON Zoltan
@ 2020-05-21 14:08 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-21 14:08 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel
Cc: Peter Maydell, Sebastian Bauer, Magnus Damm, Gerd Hoffmann,
Aurelien Jarno
On 5/20/20 3:39 PM, BALATON Zoltan wrote:
> Also update copyright year for latest changes
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/display/sm501.c | 19 +------------------
> 1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 13269cc9f4..b76b691674 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -2,7 +2,7 @@
> * QEMU SM501 Device
> *
> * Copyright (c) 2008 Shin-ichiro KAWASAKI
> - * Copyright (c) 2016 BALATON Zoltan
> + * Copyright (c) 2016-2020 BALATON Zoltan
> *
> * Permission is hereby granted, free of charge, to any person obtaining a copy
> * of this software and associated documentation files (the "Software"), to deal
> @@ -40,23 +40,6 @@
> #include "ui/pixel_ops.h"
> #include "qemu/bswap.h"
>
> -/*
> - * Status: 2010/05/07
> - * - Minimum implementation for Linux console : mmio regs and CRT layer.
> - * - 2D graphics acceleration partially supported : only fill rectangle.
> - *
> - * Status: 2016/12/04
> - * - Misc fixes: endianness, hardware cursor
> - * - Panel support
> - *
> - * TODO:
> - * - Touch panel support
> - * - USB support
> - * - UART support
> - * - More 2D graphics engine support
> - * - Performance tuning
> - */
> -
> /*#define DEBUG_SM501*/
> /*#define DEBUG_BITBLT*/
>
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/6] sm501: Convert printf + abort to qemu_log_mask
2020-05-20 13:39 [PATCH 0/6] Misc display/sm501 clean ups and fixes BALATON Zoltan
2020-05-20 13:39 ` [PATCH 3/6] sm501: Use BIT(x) macro to shorten constant BALATON Zoltan
2020-05-20 13:39 ` [PATCH 6/6] sm501: Remove obsolete changelog and todo comment BALATON Zoltan
@ 2020-05-20 13:39 ` BALATON Zoltan
2020-05-21 14:07 ` Philippe Mathieu-Daudé
2020-05-20 13:39 ` [PATCH 2/6] sm501: Shorten long variable names in sm501_2d_operation BALATON Zoltan
` (3 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: BALATON Zoltan @ 2020-05-20 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sebastian Bauer, Magnus Damm, Gerd Hoffmann,
Aurelien Jarno
Some places already use qemu_log_mask() to log unimplemented features
or errors but some others have printf() then abort(). Convert these to
qemu_log_mask() and avoid aborting to prevent guests to easily cause
denial of service.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/display/sm501.c | 57 ++++++++++++++++++++++------------------------
1 file changed, 27 insertions(+), 30 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index acc692531a..bd3ccfe311 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -727,8 +727,8 @@ static void sm501_2d_operation(SM501State *s)
int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
if (addressing != 0x0) {
- printf("%s: only XY addressing is supported.\n", __func__);
- abort();
+ qemu_log_mask(LOG_UNIMP, "sm501: only XY addressing is supported.\n");
+ return;
}
if (rop_mode == 0) {
@@ -754,8 +754,8 @@ static void sm501_2d_operation(SM501State *s)
if ((s->twoD_source_base & 0x08000000) ||
(s->twoD_destination_base & 0x08000000)) {
- printf("%s: only local memory is supported.\n", __func__);
- abort();
+ qemu_log_mask(LOG_UNIMP, "sm501: only local memory is supported.\n");
+ return;
}
switch (operation) {
@@ -823,9 +823,9 @@ static void sm501_2d_operation(SM501State *s)
break;
default:
- printf("non-implemented SM501 2D operation. %d\n", operation);
- abort();
- break;
+ qemu_log_mask(LOG_UNIMP, "sm501: not implemented 2D operation: %d\n",
+ operation);
+ return;
}
if (dst_base >= get_fb_addr(s, crt) &&
@@ -892,9 +892,8 @@ static uint64_t sm501_system_config_read(void *opaque, hwaddr addr,
break;
default:
- printf("sm501 system config : not implemented register read."
- " addr=%x\n", (int)addr);
- abort();
+ qemu_log_mask(LOG_UNIMP, "sm501: not implemented system config"
+ "register read. addr=%" HWADDR_PRIx "\n", addr);
}
return ret;
@@ -948,15 +947,15 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
break;
case SM501_ENDIAN_CONTROL:
if (value & 0x00000001) {
- printf("sm501 system config : big endian mode not implemented.\n");
- abort();
+ qemu_log_mask(LOG_UNIMP, "sm501: system config big endian mode not"
+ " implemented.\n");
}
break;
default:
- printf("sm501 system config : not implemented register write."
- " addr=%x, val=%x\n", (int)addr, (uint32_t)value);
- abort();
+ qemu_log_mask(LOG_UNIMP, "sm501: not implemented system config"
+ "register write. addr=%" HWADDR_PRIx
+ ", val=%" PRIx64 "\n", addr, value);
}
}
@@ -1207,9 +1206,8 @@ static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
break;
default:
- printf("sm501 disp ctrl : not implemented register read."
- " addr=%x\n", (int)addr);
- abort();
+ qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
+ "read. addr=%" HWADDR_PRIx "\n", addr);
}
return ret;
@@ -1345,9 +1343,9 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr addr,
break;
default:
- printf("sm501 disp ctrl : not implemented register write."
- " addr=%x, val=%x\n", (int)addr, (unsigned)value);
- abort();
+ qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
+ "write. addr=%" HWADDR_PRIx
+ ", val=%" PRIx64 "\n", addr, value);
}
}
@@ -1433,9 +1431,8 @@ static uint64_t sm501_2d_engine_read(void *opaque, hwaddr addr,
ret = 0; /* Should return interrupt status */
break;
default:
- printf("sm501 disp ctrl : not implemented register read."
- " addr=%x\n", (int)addr);
- abort();
+ qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
+ "read. addr=%" HWADDR_PRIx "\n", addr);
}
return ret;
@@ -1520,9 +1517,9 @@ static void sm501_2d_engine_write(void *opaque, hwaddr addr,
/* ignored, writing 0 should clear interrupt status */
break;
default:
- printf("sm501 2d engine : not implemented register write."
- " addr=%x, val=%x\n", (int)addr, (unsigned)value);
- abort();
+ qemu_log_mask(LOG_UNIMP, "sm501: not implemented 2d engine register "
+ "write. addr=%" HWADDR_PRIx
+ ", val=%" PRIx64 "\n", addr, value);
}
}
@@ -1670,9 +1667,9 @@ static void sm501_update_display(void *opaque)
draw_line = draw_line32_funcs[dst_depth_index];
break;
default:
- printf("sm501 update display : invalid control register value.\n");
- abort();
- break;
+ qemu_log_mask(LOG_GUEST_ERROR, "sm501: update display"
+ "invalid control register value.\n");
+ return;
}
/* set up to draw hardware cursor */
--
2.21.3
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/6] sm501: Convert printf + abort to qemu_log_mask
2020-05-20 13:39 ` [PATCH 1/6] sm501: Convert printf + abort to qemu_log_mask BALATON Zoltan
@ 2020-05-21 14:07 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-21 14:07 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel
Cc: Peter Maydell, Sebastian Bauer, Magnus Damm, Gerd Hoffmann,
Aurelien Jarno
On 5/20/20 3:39 PM, BALATON Zoltan wrote:
> Some places already use qemu_log_mask() to log unimplemented features
> or errors but some others have printf() then abort(). Convert these to
> qemu_log_mask() and avoid aborting to prevent guests to easily cause
> denial of service.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/display/sm501.c | 57 ++++++++++++++++++++++------------------------
> 1 file changed, 27 insertions(+), 30 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index acc692531a..bd3ccfe311 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -727,8 +727,8 @@ static void sm501_2d_operation(SM501State *s)
> int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
>
> if (addressing != 0x0) {
> - printf("%s: only XY addressing is supported.\n", __func__);
> - abort();
> + qemu_log_mask(LOG_UNIMP, "sm501: only XY addressing is supported.\n");
> + return;
> }
>
> if (rop_mode == 0) {
> @@ -754,8 +754,8 @@ static void sm501_2d_operation(SM501State *s)
>
> if ((s->twoD_source_base & 0x08000000) ||
> (s->twoD_destination_base & 0x08000000)) {
> - printf("%s: only local memory is supported.\n", __func__);
> - abort();
> + qemu_log_mask(LOG_UNIMP, "sm501: only local memory is supported.\n");
> + return;
> }
>
> switch (operation) {
> @@ -823,9 +823,9 @@ static void sm501_2d_operation(SM501State *s)
> break;
>
> default:
> - printf("non-implemented SM501 2D operation. %d\n", operation);
> - abort();
> - break;
> + qemu_log_mask(LOG_UNIMP, "sm501: not implemented 2D operation: %d\n",
> + operation);
> + return;
> }
>
> if (dst_base >= get_fb_addr(s, crt) &&
> @@ -892,9 +892,8 @@ static uint64_t sm501_system_config_read(void *opaque, hwaddr addr,
> break;
>
> default:
> - printf("sm501 system config : not implemented register read."
> - " addr=%x\n", (int)addr);
> - abort();
> + qemu_log_mask(LOG_UNIMP, "sm501: not implemented system config"
> + "register read. addr=%" HWADDR_PRIx "\n", addr);
> }
>
> return ret;
> @@ -948,15 +947,15 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
> break;
> case SM501_ENDIAN_CONTROL:
> if (value & 0x00000001) {
> - printf("sm501 system config : big endian mode not implemented.\n");
> - abort();
> + qemu_log_mask(LOG_UNIMP, "sm501: system config big endian mode not"
> + " implemented.\n");
> }
> break;
>
> default:
> - printf("sm501 system config : not implemented register write."
> - " addr=%x, val=%x\n", (int)addr, (uint32_t)value);
> - abort();
> + qemu_log_mask(LOG_UNIMP, "sm501: not implemented system config"
> + "register write. addr=%" HWADDR_PRIx
> + ", val=%" PRIx64 "\n", addr, value);
> }
> }
>
> @@ -1207,9 +1206,8 @@ static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
> break;
>
> default:
> - printf("sm501 disp ctrl : not implemented register read."
> - " addr=%x\n", (int)addr);
> - abort();
> + qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
> + "read. addr=%" HWADDR_PRIx "\n", addr);
> }
>
> return ret;
> @@ -1345,9 +1343,9 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr addr,
> break;
>
> default:
> - printf("sm501 disp ctrl : not implemented register write."
> - " addr=%x, val=%x\n", (int)addr, (unsigned)value);
> - abort();
> + qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
> + "write. addr=%" HWADDR_PRIx
> + ", val=%" PRIx64 "\n", addr, value);
> }
> }
>
> @@ -1433,9 +1431,8 @@ static uint64_t sm501_2d_engine_read(void *opaque, hwaddr addr,
> ret = 0; /* Should return interrupt status */
> break;
> default:
> - printf("sm501 disp ctrl : not implemented register read."
> - " addr=%x\n", (int)addr);
> - abort();
> + qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
> + "read. addr=%" HWADDR_PRIx "\n", addr);
> }
>
> return ret;
> @@ -1520,9 +1517,9 @@ static void sm501_2d_engine_write(void *opaque, hwaddr addr,
> /* ignored, writing 0 should clear interrupt status */
> break;
> default:
> - printf("sm501 2d engine : not implemented register write."
> - " addr=%x, val=%x\n", (int)addr, (unsigned)value);
> - abort();
> + qemu_log_mask(LOG_UNIMP, "sm501: not implemented 2d engine register "
> + "write. addr=%" HWADDR_PRIx
> + ", val=%" PRIx64 "\n", addr, value);
> }
> }
>
> @@ -1670,9 +1667,9 @@ static void sm501_update_display(void *opaque)
> draw_line = draw_line32_funcs[dst_depth_index];
> break;
> default:
> - printf("sm501 update display : invalid control register value.\n");
> - abort();
> - break;
> + qemu_log_mask(LOG_GUEST_ERROR, "sm501: update display"
> + "invalid control register value.\n");
> + return;
> }
>
> /* set up to draw hardware cursor */
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/6] sm501: Shorten long variable names in sm501_2d_operation
2020-05-20 13:39 [PATCH 0/6] Misc display/sm501 clean ups and fixes BALATON Zoltan
` (2 preceding siblings ...)
2020-05-20 13:39 ` [PATCH 1/6] sm501: Convert printf + abort to qemu_log_mask BALATON Zoltan
@ 2020-05-20 13:39 ` BALATON Zoltan
2020-05-20 13:39 ` [PATCH 4/6] sm501: Clean up local variables " BALATON Zoltan
` (2 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: BALATON Zoltan @ 2020-05-20 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sebastian Bauer, Magnus Damm, Gerd Hoffmann,
Aurelien Jarno
This increases readability and cleans up some confusing naming.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/display/sm501.c | 45 ++++++++++++++++++++++-----------------------
1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index bd3ccfe311..f42d05e1e4 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -700,17 +700,16 @@ static inline void hwc_invalidate(SM501State *s, int crt)
static void sm501_2d_operation(SM501State *s)
{
/* obtain operation parameters */
- int operation = (s->twoD_control >> 16) & 0x1f;
+ int cmd = (s->twoD_control >> 16) & 0x1F;
int rtl = s->twoD_control & 0x8000000;
int src_x = (s->twoD_source >> 16) & 0x01FFF;
int src_y = s->twoD_source & 0xFFFF;
int dst_x = (s->twoD_destination >> 16) & 0x01FFF;
int dst_y = s->twoD_destination & 0xFFFF;
- int operation_width = (s->twoD_dimension >> 16) & 0x1FFF;
- int operation_height = s->twoD_dimension & 0xFFFF;
+ int width = (s->twoD_dimension >> 16) & 0x1FFF;
+ int height = s->twoD_dimension & 0xFFFF;
uint32_t color = s->twoD_foreground;
- int format_flags = (s->twoD_stretch >> 20) & 0x3;
- int addressing = (s->twoD_stretch >> 16) & 0xF;
+ int format = (s->twoD_stretch >> 20) & 0x3;
int rop_mode = (s->twoD_control >> 15) & 0x1; /* 1 for rop2, else rop3 */
/* 1 if rop2 source is the pattern, otherwise the source is the bitmap */
int rop2_source_is_pattern = (s->twoD_control >> 14) & 0x1;
@@ -721,12 +720,12 @@ static void sm501_2d_operation(SM501State *s)
/* get frame buffer info */
uint8_t *src = s->local_mem + src_base;
uint8_t *dst = s->local_mem + dst_base;
- int src_width = s->twoD_pitch & 0x1FFF;
- int dst_width = (s->twoD_pitch >> 16) & 0x1FFF;
+ int src_pitch = s->twoD_pitch & 0x1FFF;
+ int dst_pitch = (s->twoD_pitch >> 16) & 0x1FFF;
int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
- if (addressing != 0x0) {
+ if ((s->twoD_stretch >> 16) & 0xF) {
qemu_log_mask(LOG_UNIMP, "sm501: only XY addressing is supported.\n");
return;
}
@@ -758,20 +757,20 @@ static void sm501_2d_operation(SM501State *s)
return;
}
- switch (operation) {
+ switch (cmd) {
case 0x00: /* copy area */
#define COPY_AREA(_bpp, _pixel_type, rtl) { \
int y, x, index_d, index_s; \
- for (y = 0; y < operation_height; y++) { \
- for (x = 0; x < operation_width; x++) { \
+ for (y = 0; y < height; y++) { \
+ for (x = 0; x < width; x++) { \
_pixel_type val; \
\
if (rtl) { \
- index_s = ((src_y - y) * src_width + src_x - x) * _bpp; \
- index_d = ((dst_y - y) * dst_width + dst_x - x) * _bpp; \
+ index_s = ((src_y - y) * src_pitch + src_x - x) * _bpp; \
+ index_d = ((dst_y - y) * dst_pitch + dst_x - x) * _bpp; \
} else { \
- index_s = ((src_y + y) * src_width + src_x + x) * _bpp; \
- index_d = ((dst_y + y) * dst_width + dst_x + x) * _bpp; \
+ index_s = ((src_y + y) * src_pitch + src_x + x) * _bpp; \
+ index_d = ((dst_y + y) * dst_pitch + dst_x + x) * _bpp; \
} \
if (rop_mode == 1 && rop == 5) { \
/* Invert dest */ \
@@ -783,7 +782,7 @@ static void sm501_2d_operation(SM501State *s)
} \
} \
}
- switch (format_flags) {
+ switch (format) {
case 0:
COPY_AREA(1, uint8_t, rtl);
break;
@@ -799,15 +798,15 @@ static void sm501_2d_operation(SM501State *s)
case 0x01: /* fill rectangle */
#define FILL_RECT(_bpp, _pixel_type) { \
int y, x; \
- for (y = 0; y < operation_height; y++) { \
- for (x = 0; x < operation_width; x++) { \
- int index = ((dst_y + y) * dst_width + dst_x + x) * _bpp; \
+ for (y = 0; y < height; y++) { \
+ for (x = 0; x < width; x++) { \
+ int index = ((dst_y + y) * dst_pitch + dst_x + x) * _bpp; \
*(_pixel_type *)&dst[index] = (_pixel_type)color; \
} \
} \
}
- switch (format_flags) {
+ switch (format) {
case 0:
FILL_RECT(1, uint8_t);
break;
@@ -824,14 +823,14 @@ static void sm501_2d_operation(SM501State *s)
default:
qemu_log_mask(LOG_UNIMP, "sm501: not implemented 2D operation: %d\n",
- operation);
+ cmd);
return;
}
if (dst_base >= get_fb_addr(s, crt) &&
dst_base <= get_fb_addr(s, crt) + fb_len) {
- int dst_len = MIN(fb_len, ((dst_y + operation_height - 1) * dst_width +
- dst_x + operation_width) * (1 << format_flags));
+ int dst_len = MIN(fb_len, ((dst_y + height - 1) * dst_pitch +
+ dst_x + width) * (1 << format));
if (dst_len) {
memory_region_set_dirty(&s->local_mem_region, dst_base, dst_len);
}
--
2.21.3
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 4/6] sm501: Clean up local variables in sm501_2d_operation
2020-05-20 13:39 [PATCH 0/6] Misc display/sm501 clean ups and fixes BALATON Zoltan
` (3 preceding siblings ...)
2020-05-20 13:39 ` [PATCH 2/6] sm501: Shorten long variable names in sm501_2d_operation BALATON Zoltan
@ 2020-05-20 13:39 ` BALATON Zoltan
2020-05-21 14:09 ` Philippe Mathieu-Daudé
2020-05-20 13:39 ` [PATCH 5/6] sm501: Replace hand written implementation with pixman where possible BALATON Zoltan
2020-05-20 17:55 ` [PATCH 0/6] Misc display/sm501 clean ups and fixes BALATON Zoltan
6 siblings, 1 reply; 12+ messages in thread
From: BALATON Zoltan @ 2020-05-20 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sebastian Bauer, Magnus Damm, Gerd Hoffmann,
Aurelien Jarno
Make variables local to the block they are used in to make it clearer
which operation they are needed for.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/display/sm501.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 97660090bb..5ed57703d8 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -699,28 +699,19 @@ static inline void hwc_invalidate(SM501State *s, int crt)
static void sm501_2d_operation(SM501State *s)
{
- /* obtain operation parameters */
int cmd = (s->twoD_control >> 16) & 0x1F;
int rtl = s->twoD_control & BIT(27);
- int src_x = (s->twoD_source >> 16) & 0x01FFF;
- int src_y = s->twoD_source & 0xFFFF;
- int dst_x = (s->twoD_destination >> 16) & 0x01FFF;
- int dst_y = s->twoD_destination & 0xFFFF;
- int width = (s->twoD_dimension >> 16) & 0x1FFF;
- int height = s->twoD_dimension & 0xFFFF;
- uint32_t color = s->twoD_foreground;
int format = (s->twoD_stretch >> 20) & 0x3;
int rop_mode = (s->twoD_control >> 15) & 0x1; /* 1 for rop2, else rop3 */
/* 1 if rop2 source is the pattern, otherwise the source is the bitmap */
int rop2_source_is_pattern = (s->twoD_control >> 14) & 0x1;
int rop = s->twoD_control & 0xFF;
- uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
+ int dst_x = (s->twoD_destination >> 16) & 0x01FFF;
+ int dst_y = s->twoD_destination & 0xFFFF;
+ int width = (s->twoD_dimension >> 16) & 0x1FFF;
+ int height = s->twoD_dimension & 0xFFFF;
uint32_t dst_base = s->twoD_destination_base & 0x03FFFFFF;
-
- /* get frame buffer info */
- uint8_t *src = s->local_mem + src_base;
uint8_t *dst = s->local_mem + dst_base;
- int src_pitch = s->twoD_pitch & 0x1FFF;
int dst_pitch = (s->twoD_pitch >> 16) & 0x1FFF;
int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
@@ -758,6 +749,13 @@ static void sm501_2d_operation(SM501State *s)
switch (cmd) {
case 0x00: /* copy area */
+ {
+ int src_x = (s->twoD_source >> 16) & 0x01FFF;
+ int src_y = s->twoD_source & 0xFFFF;
+ uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
+ uint8_t *src = s->local_mem + src_base;
+ int src_pitch = s->twoD_pitch & 0x1FFF;
+
#define COPY_AREA(_bpp, _pixel_type, rtl) { \
int y, x, index_d, index_s; \
for (y = 0; y < height; y++) { \
@@ -793,8 +791,11 @@ static void sm501_2d_operation(SM501State *s)
break;
}
break;
-
+ }
case 0x01: /* fill rectangle */
+ {
+ uint32_t color = s->twoD_foreground;
+
#define FILL_RECT(_bpp, _pixel_type) { \
int y, x; \
for (y = 0; y < height; y++) { \
@@ -819,7 +820,7 @@ static void sm501_2d_operation(SM501State *s)
break;
}
break;
-
+ }
default:
qemu_log_mask(LOG_UNIMP, "sm501: not implemented 2D operation: %d\n",
cmd);
--
2.21.3
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 4/6] sm501: Clean up local variables in sm501_2d_operation
2020-05-20 13:39 ` [PATCH 4/6] sm501: Clean up local variables " BALATON Zoltan
@ 2020-05-21 14:09 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-21 14:09 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel
Cc: Peter Maydell, Sebastian Bauer, Magnus Damm, Gerd Hoffmann,
Aurelien Jarno
On 5/20/20 3:39 PM, BALATON Zoltan wrote:
> Make variables local to the block they are used in to make it clearer
> which operation they are needed for.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/display/sm501.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 97660090bb..5ed57703d8 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -699,28 +699,19 @@ static inline void hwc_invalidate(SM501State *s, int crt)
>
> static void sm501_2d_operation(SM501State *s)
> {
> - /* obtain operation parameters */
> int cmd = (s->twoD_control >> 16) & 0x1F;
> int rtl = s->twoD_control & BIT(27);
> - int src_x = (s->twoD_source >> 16) & 0x01FFF;
> - int src_y = s->twoD_source & 0xFFFF;
> - int dst_x = (s->twoD_destination >> 16) & 0x01FFF;
> - int dst_y = s->twoD_destination & 0xFFFF;
> - int width = (s->twoD_dimension >> 16) & 0x1FFF;
> - int height = s->twoD_dimension & 0xFFFF;
> - uint32_t color = s->twoD_foreground;
> int format = (s->twoD_stretch >> 20) & 0x3;
> int rop_mode = (s->twoD_control >> 15) & 0x1; /* 1 for rop2, else rop3 */
> /* 1 if rop2 source is the pattern, otherwise the source is the bitmap */
> int rop2_source_is_pattern = (s->twoD_control >> 14) & 0x1;
> int rop = s->twoD_control & 0xFF;
> - uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
> + int dst_x = (s->twoD_destination >> 16) & 0x01FFF;
> + int dst_y = s->twoD_destination & 0xFFFF;
> + int width = (s->twoD_dimension >> 16) & 0x1FFF;
> + int height = s->twoD_dimension & 0xFFFF;
> uint32_t dst_base = s->twoD_destination_base & 0x03FFFFFF;
> -
> - /* get frame buffer info */
> - uint8_t *src = s->local_mem + src_base;
> uint8_t *dst = s->local_mem + dst_base;
> - int src_pitch = s->twoD_pitch & 0x1FFF;
> int dst_pitch = (s->twoD_pitch >> 16) & 0x1FFF;
> int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
> int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
> @@ -758,6 +749,13 @@ static void sm501_2d_operation(SM501State *s)
>
> switch (cmd) {
> case 0x00: /* copy area */
> + {
> + int src_x = (s->twoD_source >> 16) & 0x01FFF;
> + int src_y = s->twoD_source & 0xFFFF;
> + uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
> + uint8_t *src = s->local_mem + src_base;
> + int src_pitch = s->twoD_pitch & 0x1FFF;
> +
> #define COPY_AREA(_bpp, _pixel_type, rtl) { \
> int y, x, index_d, index_s; \
> for (y = 0; y < height; y++) { \
> @@ -793,8 +791,11 @@ static void sm501_2d_operation(SM501State *s)
> break;
> }
> break;
> -
> + }
> case 0x01: /* fill rectangle */
> + {
> + uint32_t color = s->twoD_foreground;
> +
> #define FILL_RECT(_bpp, _pixel_type) { \
> int y, x; \
> for (y = 0; y < height; y++) { \
> @@ -819,7 +820,7 @@ static void sm501_2d_operation(SM501State *s)
> break;
> }
> break;
> -
> + }
> default:
> qemu_log_mask(LOG_UNIMP, "sm501: not implemented 2D operation: %d\n",
> cmd);
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 5/6] sm501: Replace hand written implementation with pixman where possible
2020-05-20 13:39 [PATCH 0/6] Misc display/sm501 clean ups and fixes BALATON Zoltan
` (4 preceding siblings ...)
2020-05-20 13:39 ` [PATCH 4/6] sm501: Clean up local variables " BALATON Zoltan
@ 2020-05-20 13:39 ` BALATON Zoltan
2020-05-20 17:55 ` [PATCH 0/6] Misc display/sm501 clean ups and fixes BALATON Zoltan
6 siblings, 0 replies; 12+ messages in thread
From: BALATON Zoltan @ 2020-05-20 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sebastian Bauer, Magnus Damm, Gerd Hoffmann,
Aurelien Jarno
Besides being faster this should also prevent malicious guests to
abuse 2D engine to overwrite data or cause a crash.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/display/sm501.c | 181 +++++++++++++++++++++++----------------------
1 file changed, 93 insertions(+), 88 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 5ed57703d8..13269cc9f4 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -706,13 +706,12 @@ static void sm501_2d_operation(SM501State *s)
/* 1 if rop2 source is the pattern, otherwise the source is the bitmap */
int rop2_source_is_pattern = (s->twoD_control >> 14) & 0x1;
int rop = s->twoD_control & 0xFF;
- int dst_x = (s->twoD_destination >> 16) & 0x01FFF;
- int dst_y = s->twoD_destination & 0xFFFF;
- int width = (s->twoD_dimension >> 16) & 0x1FFF;
- int height = s->twoD_dimension & 0xFFFF;
+ unsigned int dst_x = (s->twoD_destination >> 16) & 0x01FFF;
+ unsigned int dst_y = s->twoD_destination & 0xFFFF;
+ unsigned int width = (s->twoD_dimension >> 16) & 0x1FFF;
+ unsigned int height = s->twoD_dimension & 0xFFFF;
uint32_t dst_base = s->twoD_destination_base & 0x03FFFFFF;
- uint8_t *dst = s->local_mem + dst_base;
- int dst_pitch = (s->twoD_pitch >> 16) & 0x1FFF;
+ unsigned int dst_pitch = (s->twoD_pitch >> 16) & 0x1FFF;
int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
@@ -721,104 +720,110 @@ static void sm501_2d_operation(SM501State *s)
return;
}
- if (rop_mode == 0) {
- if (rop != 0xcc) {
- /* Anything other than plain copies are not supported */
- qemu_log_mask(LOG_UNIMP, "sm501: rop3 mode with rop %x is not "
- "supported.\n", rop);
- }
- } else {
- if (rop2_source_is_pattern && rop != 0x5) {
- /* For pattern source, we support only inverse dest */
- qemu_log_mask(LOG_UNIMP, "sm501: rop2 source being the pattern and "
- "rop %x is not supported.\n", rop);
- } else {
- if (rop != 0x5 && rop != 0xc) {
- /* Anything other than plain copies or inverse dest is not
- * supported */
- qemu_log_mask(LOG_UNIMP, "sm501: rop mode %x is not "
- "supported.\n", rop);
- }
- }
- }
-
if (s->twoD_source_base & BIT(27) || s->twoD_destination_base & BIT(27)) {
qemu_log_mask(LOG_UNIMP, "sm501: only local memory is supported.\n");
return;
}
+ if (!dst_pitch) {
+ qemu_log_mask(LOG_GUEST_ERROR, "sm501: Zero dest pitch.\n");
+ return;
+ }
+
+ if (!width || !height) {
+ qemu_log_mask(LOG_GUEST_ERROR, "sm501: Zero size 2D op.\n");
+ return;
+ }
+
+ /* Do we need to handle overlapping regions? */
+ if (rtl) {
+ dst_x -= width - 1;
+ dst_y -= height - 1;
+ }
+
+ if (dst_base >= get_local_mem_size(s) ||
+ dst_base + (dst_x + width + (dst_y + height) * dst_pitch) *
+ (1 << format) >= get_local_mem_size(s)) {
+ qemu_log_mask(LOG_GUEST_ERROR, "sm501: 2D op dest is outside vram.\n");
+ return;
+ }
+
switch (cmd) {
- case 0x00: /* copy area */
+ case 0: /* BitBlt */
{
- int src_x = (s->twoD_source >> 16) & 0x01FFF;
- int src_y = s->twoD_source & 0xFFFF;
+ unsigned int src_x = (s->twoD_source >> 16) & 0x01FFF;
+ unsigned int src_y = s->twoD_source & 0xFFFF;
uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
- uint8_t *src = s->local_mem + src_base;
- int src_pitch = s->twoD_pitch & 0x1FFF;
-
-#define COPY_AREA(_bpp, _pixel_type, rtl) { \
- int y, x, index_d, index_s; \
- for (y = 0; y < height; y++) { \
- for (x = 0; x < width; x++) { \
- _pixel_type val; \
- \
- if (rtl) { \
- index_s = ((src_y - y) * src_pitch + src_x - x) * _bpp; \
- index_d = ((dst_y - y) * dst_pitch + dst_x - x) * _bpp; \
- } else { \
- index_s = ((src_y + y) * src_pitch + src_x + x) * _bpp; \
- index_d = ((dst_y + y) * dst_pitch + dst_x + x) * _bpp; \
- } \
- if (rop_mode == 1 && rop == 5) { \
- /* Invert dest */ \
- val = ~*(_pixel_type *)&dst[index_d]; \
- } else { \
- val = *(_pixel_type *)&src[index_s]; \
- } \
- *(_pixel_type *)&dst[index_d] = val; \
- } \
- } \
- }
- switch (format) {
- case 0:
- COPY_AREA(1, uint8_t, rtl);
- break;
- case 1:
- COPY_AREA(2, uint16_t, rtl);
- break;
- case 2:
- COPY_AREA(4, uint32_t, rtl);
- break;
+ unsigned int src_pitch = s->twoD_pitch & 0x1FFF;
+
+ if (!src_pitch) {
+ qemu_log_mask(LOG_GUEST_ERROR, "sm501: Zero src pitch.\n");
+ return;
+ }
+
+ if (rtl) {
+ src_x -= width - 1;
+ src_y -= height - 1;
+ }
+
+ if (src_base >= get_local_mem_size(s) ||
+ src_base + (src_x + width + (src_y + height) * src_pitch) *
+ (1 << format) >= get_local_mem_size(s)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "sm501: 2D op src is outside vram.\n");
+ return;
+ }
+
+ if ((rop_mode && rop == 0x5) || (!rop_mode && rop == 0x55)) {
+ /* Invert dest, is there a way to do this with pixman? */
+ unsigned int x, y, i;
+ uint8_t *d = s->local_mem + dst_base;
+
+ for (y = 0; y < height; y++) {
+ i = (dst_x + (dst_y + y) * dst_pitch) * (1 << format);
+ for (x = 0; x < width; x++, i += (1 << format)) {
+ switch (format) {
+ case 0:
+ d[i] = ~d[i];
+ break;
+ case 1:
+ *(uint16_t *)&d[i] = ~*(uint16_t *)&d[i];
+ break;
+ case 2:
+ *(uint32_t *)&d[i] = ~*(uint32_t *)&d[i];
+ break;
+ }
+ }
+ }
+ } else if ((rop_mode && rop == 0xc && !rop2_source_is_pattern) ||
+ (!rop_mode && rop == 0xcc)) {
+ /* Do copy src directly */
+ pixman_blt((uint32_t *)&s->local_mem[src_base],
+ (uint32_t *)&s->local_mem[dst_base],
+ src_pitch * (1 << format) / sizeof(uint32_t),
+ dst_pitch * (1 << format) / sizeof(uint32_t),
+ 8 * (1 << format), 8 * (1 << format),
+ src_x, src_y, dst_x, dst_y, width, height);
+ } else {
+ qemu_log_mask(LOG_UNIMP, "sm501: rop%d op %x%s not implemented\n",
+ (rop_mode ? 2 : 3), rop, (rop2_source_is_pattern ?
+ " with pattern source" : ""));
}
break;
}
- case 0x01: /* fill rectangle */
+ case 1: /* Rectangle Fill */
{
uint32_t color = s->twoD_foreground;
-#define FILL_RECT(_bpp, _pixel_type) { \
- int y, x; \
- for (y = 0; y < height; y++) { \
- for (x = 0; x < width; x++) { \
- int index = ((dst_y + y) * dst_pitch + dst_x + x) * _bpp; \
- *(_pixel_type *)&dst[index] = (_pixel_type)color; \
- } \
- } \
- }
-
- switch (format) {
- case 0:
- FILL_RECT(1, uint8_t);
- break;
- case 1:
- color = cpu_to_le16(color);
- FILL_RECT(2, uint16_t);
- break;
- case 2:
+ if (format == 2) {
color = cpu_to_le32(color);
- FILL_RECT(4, uint32_t);
- break;
+ } else if (format == 1) {
+ color = cpu_to_le16(color);
}
+
+ pixman_fill((uint32_t *)&s->local_mem[dst_base],
+ dst_pitch * (1 << format) / sizeof(uint32_t),
+ 8 * (1 << format), dst_x, dst_y, width, height, color);
break;
}
default:
--
2.21.3
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 0/6] Misc display/sm501 clean ups and fixes
2020-05-20 13:39 [PATCH 0/6] Misc display/sm501 clean ups and fixes BALATON Zoltan
` (5 preceding siblings ...)
2020-05-20 13:39 ` [PATCH 5/6] sm501: Replace hand written implementation with pixman where possible BALATON Zoltan
@ 2020-05-20 17:55 ` BALATON Zoltan
6 siblings, 0 replies; 12+ messages in thread
From: BALATON Zoltan @ 2020-05-20 17:55 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sebastian Bauer, Magnus Damm, Gerd Hoffmann,
dino.papararo, Aurelien Jarno
On Wed, 20 May 2020, BALATON Zoltan wrote:
> Hello,
>
> These are some small clean ups and changes to hopefully improve 2D
> engine performance and fix a security bug in it. I've only tested it
> lightly, haven't verified if breaking it is still possible. It's also
> known to change handling of right to left blits which may not be able
> to handle overlaping regions but the only guest known to use it
> (AmigaOS on sam460ex) seems to be OK with this so unless this is
> proven to be needed I won't try to fix that now, this could be
> addressed in later patches.
I've found a case where not handling right to left causes a problem (can
be reproduced by scrolling window content which will do overlapping blits)
so this will need some more work but the basic idea can be tested with
this version.
Does anyone know how the X server or other users of pixman handle this as
pixman does not seem to have support for other than left to right top to
bottom blits?
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 12+ messages in thread