Re: [PATCH v2 09/10] vt82c686: Convert debug printf to trace points

[Philippe Mathieu-Daudé]

On 12/28/20 3:08 AM, BALATON Zoltan via wrote:
> Drop DPRINTF and use trace functions instead. Two debug messages about
> unimplemented registers could be converted to qemu_log_mask() but in
> reality all registers are currently unimplemented (we just store and
> return values of writable regs but do nothing with them). As we
> already trace register access there's no need for additional debug
> messages so these are just removed and a comment is added as a reminder.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

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

> ---
> v2: Extended commit message
> 
>  hw/isa/trace-events |  6 ++++++
>  hw/isa/vt82c686.c   | 51 +++++++++++++--------------------------------
>  2 files changed, 21 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/isa/trace-events b/hw/isa/trace-events
> index 3544c6213c..d267d3e652 100644
> --- a/hw/isa/trace-events
> +++ b/hw/isa/trace-events
> @@ -13,3 +13,9 @@ pc87312_io_write(uint32_t addr, uint32_t val) "write 
> addr=0x%x val=0x%x"
>  # apm.c
>  apm_io_read(uint8_t addr, uint8_t val) "read addr=0x%x val=0x%02x"
>  apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x"
> +
> +# vt82c686.c
> +via_isa_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 
> 0x%x"
> +via_pm_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 
> 0x%x"
> +via_superio_read(uint8_t addr, uint8_t val) "addr 0x%x val 0x%x"
> +via_superio_write(uint8_t addr, uint32_t val) "addr 0x%x val 0x%x"
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index cd87ec0103..d7ce15bf9f 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -27,14 +27,7 @@
>  #include "qemu/timer.h"
>  #include "exec/address-spaces.h"
>  #include "qom/object.h"
> -
> -/* #define DEBUG_VT82C686B */
> -
> -#ifdef DEBUG_VT82C686B
> -#define DPRINTF(fmt, ...) fprintf(stderr, "%s: " fmt, __func__, 
> ##__VA_ARGS__)
> -#else
> -#define DPRINTF(fmt, ...)
> -#endif
> +#include "trace.h"
>  
>  typedef struct SuperIOConfig {
>      uint8_t config[0x100];
> @@ -55,12 +48,12 @@ static void superio_ioport_writeb(void *opaque, hwaddr 
> addr, uint64_t data,
>  {
>      SuperIOConfig *superio_conf = opaque;
>  
> -    DPRINTF("superio_ioport_writeb  address 0x%x  val 0x%x\n", addr, data);
> -    if (addr == 0x3f0) {
> +    if (addr == 0x3f0) { /* config index register */
>          superio_conf->index = data & 0xff;
>      } else {
>          bool can_write = true;
> -        /* 0x3f1 */
> +        /* 0x3f1, config data register */
> +        trace_via_superio_write(superio_conf->index, data & 0xff);
>          switch (superio_conf->index) {
>          case 0x00 ... 0xdf:
>          case 0xe4:
> @@ -73,18 +66,7 @@ static void superio_ioport_writeb(void *opaque, hwaddr 
> addr, uint64_t data,
>          case 0xfd ... 0xff:
>              can_write = false;
>              break;
> -        case 0xe7:
> -            if ((data & 0xff) != 0xfe) {
> -                DPRINTF("change uart 1 base. unsupported yet\n");
> -                can_write = false;
> -            }
> -            break;
> -        case 0xe8:
> -            if ((data & 0xff) != 0xbe) {
> -                DPRINTF("change uart 2 base. unsupported yet\n");
> -                can_write = false;
> -            }
> -            break;
> +        /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */

If you don't mind I'll prepend this patch:
-- >8 --
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index cd87ec01039..23b4deaac93 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -25,6 +25,7 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "qemu/timer.h"
+#include "qemu/log.h"
 #include "exec/address-spaces.h"
 #include "qom/object.h"

@@ -73,17 +74,9 @@ static void superio_ioport_writeb(void *opaque,
hwaddr addr, uint64_t data,
         case 0xfd ... 0xff:
             can_write = false;
             break;
-        case 0xe7:
-            if ((data & 0xff) != 0xfe) {
-                DPRINTF("change uart 1 base. unsupported yet\n");
-                can_write = false;
-            }
-            break;
-        case 0xe8:
-            if ((data & 0xff) != 0xbe) {
-                DPRINTF("change uart 2 base. unsupported yet\n");
-                can_write = false;
-            }
+        case 0xe6 ... 0xe8: /* set base port of parallel and serial */
+            qemu_log_mask(LOG_UNIMP, "change base port not implemented\n");
+            can_write = false;
             break;
         default:
             break;
---