[Qemu-devel] [PATCH v2 0/8] Add LOG_GUEST_ERROR for reporting guest bugs

[Qemu-devel] [PATCH v2 0/8] Add LOG_GUEST_ERROR for reporting guest bugs

[Peter Maydell]

This patch series introduces a new logging category to
qemu_log_mask(), for reporting situations which the device
model can cope with but which indicate bugs in the guest.
The most common cause of these is "guest tried to access
a register which doesn't exist". At the moment device models
use an inconsistent mix of "silently ignore", "hw_error(),
killing qemu" and "direct print to stderr" for reporting
this kind of event.

Patches 3-8 are a random selection of ARM devices which I've
switched to using the new LOG_GUEST_ERROR (or the existing
LOG_UNIMP where that is more appropriate). There are obviously
more conversions that could be made; this is just a sample
to demonstrate the utility of the log category.

Changes v1->v2:
 * add patch 2 including qemu-log.h in hw/hw.h
 * update 3..8 to not include qemu-log.h directly

Peter Maydell (8):
  qemu-log: Add new log category for guest bugs
  hw/hw.h: Add include of qemu-log.h
  hw/pl181: Use LOG_UNIMP and LOG_GUEST_ERROR
  hw/pl041: Use LOG_UNIMP
  hw/pl190: Use LOG_GUEST_ERROR
  hw/pl011: Use LOG_UNIMP and LOG_GUEST_ERROR
  hw/pl022: Use LOG_UNIMP and LOG_GUEST_ERROR
  hw/pl031: Use LOG_GUEST_ERROR

 hw/hw.h    |    1 +
 hw/pl011.c |   11 +++++++----
 hw/pl022.c |    8 +++++---
 hw/pl031.c |   16 ++++++++++------
 hw/pl041.c |    5 +++--
 hw/pl181.c |   18 ++++++++++--------
 hw/pl190.c |    6 ++++--
 qemu-log.c |    3 +++
 qemu-log.h |    1 +
 9 files changed, 44 insertions(+), 25 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH v2 1/8] qemu-log: Add new log category for guest bugs

[Peter Maydell]

Add a new category for device models to log guest behaviour
which is likely to be a guest bug of some kind (accessing
nonexistent registers, reading 32 bit wide registers with
a byte access, etc). Making this its own log category allows
those who care (mostly guest OS authors) to see the complaints
without bothering most users.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 qemu-log.c |    3 +++
 qemu-log.h |    1 +
 2 files changed, 4 insertions(+)

diff --git a/qemu-log.c b/qemu-log.c
index 396aafd..a4c3d1f 100644
--- a/qemu-log.c
+++ b/qemu-log.c
@@ -116,6 +116,9 @@ const CPULogItem cpu_log_items[] = {
       "show all i/o ports accesses" },
     { LOG_UNIMP, "unimp",
       "log unimplemented functionality" },
+    { LOG_GUEST_ERROR, "guest_errors",
+      "log when the guest OS does something invalid (eg accessing a\n"
+      "non-existent register)" },
     { 0, NULL, NULL },
 };
 
diff --git a/qemu-log.h b/qemu-log.h
index 5ccecf3..ce6bb09 100644
--- a/qemu-log.h
+++ b/qemu-log.h
@@ -35,6 +35,7 @@ static inline bool qemu_log_enabled(void)
 #define CPU_LOG_TB_CPU     (1 << 8)
 #define CPU_LOG_RESET      (1 << 9)
 #define LOG_UNIMP          (1 << 10)
+#define LOG_GUEST_ERROR    (1 << 11)
 
 /* Returns true if a bit is set in the current loglevel mask
  */
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 2/8] hw/hw.h: Add include of qemu-log.h

[Peter Maydell]

Add an include of qemu-log.h to hw.h, so that device model
code has access to these logging functions without the need
to directly include qemu-log.h.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/hw.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/hw.h b/hw/hw.h
index 16101de..b337ee3 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -12,6 +12,7 @@
 #include "irq.h"
 #include "qemu-file.h"
 #include "vmstate.h"
+#include "qemu-log.h"
 
 #ifdef NEED_CPU_H
 #if TARGET_LONG_BITS == 64
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 3/8] hw/pl181: Use LOG_UNIMP and LOG_GUEST_ERROR

[Peter Maydell]

Rather than a mix of direct printing to stderr and aborting
via hw_error(), use LOG_UNIMP and LOG_GUEST_ERROR.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pl181.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/pl181.c b/hw/pl181.c
index 7d91fbb..5a73473 100644
--- a/hw/pl181.c
+++ b/hw/pl181.c
@@ -352,7 +352,7 @@ static uint64_t pl181_read(void *opaque, target_phys_addr_t offset,
     case 0xa0: case 0xa4: case 0xa8: case 0xac:
     case 0xb0: case 0xb4: case 0xb8: case 0xbc:
         if (s->fifo_len == 0) {
-            fprintf(stderr, "pl181: Unexpected FIFO read\n");
+            qemu_log_mask(LOG_GUEST_ERROR, "pl181: Unexpected FIFO read\n");
             return 0;
         } else {
             uint32_t value;
@@ -363,7 +363,8 @@ static uint64_t pl181_read(void *opaque, target_phys_addr_t offset,
             return value;
         }
     default:
-        hw_error("pl181_read: Bad offset %x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl181_read: Bad offset %x\n", (int)offset);
         return 0;
     }
 }
@@ -387,11 +388,11 @@ static void pl181_write(void *opaque, target_phys_addr_t offset,
         s->cmd = value;
         if (s->cmd & PL181_CMD_ENABLE) {
             if (s->cmd & PL181_CMD_INTERRUPT) {
-                fprintf(stderr, "pl181: Interrupt mode not implemented\n");
-                abort();
+                qemu_log_mask(LOG_UNIMP,
+                              "pl181: Interrupt mode not implemented\n");
             } if (s->cmd & PL181_CMD_PENDING) {
-                fprintf(stderr, "pl181: Pending commands not implemented\n");
-                abort();
+                qemu_log_mask(LOG_UNIMP,
+                              "pl181: Pending commands not implemented\n");
             } else {
                 pl181_send_command(s);
                 pl181_fifo_run(s);
@@ -427,14 +428,15 @@ static void pl181_write(void *opaque, target_phys_addr_t offset,
     case 0xa0: case 0xa4: case 0xa8: case 0xac:
     case 0xb0: case 0xb4: case 0xb8: case 0xbc:
         if (s->datacnt == 0) {
-            fprintf(stderr, "pl181: Unexpected FIFO write\n");
+            qemu_log_mask(LOG_GUEST_ERROR, "pl181: Unexpected FIFO write\n");
         } else {
             pl181_fifo_push(s, value);
             pl181_fifo_run(s);
         }
         break;
     default:
-        hw_error("pl181_write: Bad offset %x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl181_write: Bad offset %x\n", (int)offset);
     }
     pl181_update(s);
 }
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 4/8] hw/pl041: Use LOG_UNIMP

[Peter Maydell]

Use the new LOG_UNIMP tracing to report unimplemented
features.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pl041.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/pl041.c b/hw/pl041.c
index b6723be..9a6db1b 100644
--- a/hw/pl041.c
+++ b/hw/pl041.c
@@ -536,8 +536,9 @@ static int pl041_init(SysBusDevice *dev)
     default:
         /* NC FIFO depth of 16 is not allowed because its id bits in
            AACIPERIPHID3 overlap with the id for the default NC FIFO depth */
-        fprintf(stderr, "pl041: unsupported non-compact fifo depth [%i]\n",
-                s->fifo_depth);
+        qemu_log_mask(LOG_UNIMP,
+                      "pl041: unsupported non-compact fifo depth [%i]\n",
+                      s->fifo_depth);
         return -1;
     }
 
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 5/8] hw/pl190: Use LOG_GUEST_ERROR

[Peter Maydell]

If the guest attempts an offset to a nonexistent register, just
log this via LOG_GUEST_ERROR rather than killing QEMU with a hw_error.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pl190.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/pl190.c b/hw/pl190.c
index 7332f4d..961da5b 100644
--- a/hw/pl190.c
+++ b/hw/pl190.c
@@ -143,7 +143,8 @@ static uint64_t pl190_read(void *opaque, target_phys_addr_t offset,
     case 13: /* DEFVECTADDR */
         return s->vect_addr[16];
     default:
-        hw_error("pl190_read: Bad offset %x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl190_read: Bad offset %x\n", (int)offset);
         return 0;
     }
 }
@@ -202,7 +203,8 @@ static void pl190_write(void *opaque, target_phys_addr_t offset,
         }
         break;
     default:
-        hw_error("pl190_write: Bad offset %x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                     "pl190_write: Bad offset %x\n", (int)offset);
         return;
     }
     pl190_update(s);
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 6/8] hw/pl011: Use LOG_UNIMP and LOG_GUEST_ERROR

[Peter Maydell]

Use the new LOG_UNIMP and LOG_GUEST_ERROR logging types rather
than hw_error().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pl011.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/pl011.c b/hw/pl011.c
index 3245702..fb22736 100644
--- a/hw/pl011.c
+++ b/hw/pl011.c
@@ -107,7 +107,8 @@ static uint64_t pl011_read(void *opaque, target_phys_addr_t offset,
     case 18: /* UARTDMACR */
         return s->dmacr;
     default:
-        hw_error("pl011_read: Bad offset %x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl011_read: Bad offset %x\n", (int)offset);
         return 0;
     }
 }
@@ -178,11 +179,13 @@ static void pl011_write(void *opaque, target_phys_addr_t offset,
         break;
     case 18: /* UARTDMACR */
         s->dmacr = value;
-        if (value & 3)
-            hw_error("PL011: DMA not implemented\n");
+        if (value & 3) {
+            qemu_log_mask(LOG_UNIMP, "pl011: DMA not implemented\n");
+        }
         break;
     default:
-        hw_error("pl011_write: Bad offset %x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl011_write: Bad offset %x\n", (int)offset);
     }
 }
 
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 7/8] hw/pl022: Use LOG_UNIMP and LOG_GUEST_ERROR

[Peter Maydell]

Use LOG_UNIMP and LOG_GUEST_ERROR where appropriate rather
than hw_error().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pl022.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/pl022.c b/hw/pl022.c
index 60e35da..e2ae315 100644
--- a/hw/pl022.c
+++ b/hw/pl022.c
@@ -168,7 +168,8 @@ static uint64_t pl022_read(void *opaque, target_phys_addr_t offset,
         /* Not implemented.  */
         return 0;
     default:
-        hw_error("pl022_read: Bad offset %x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl022_read: Bad offset %x\n", (int)offset);
         return 0;
     }
 }
@@ -211,11 +212,12 @@ static void pl022_write(void *opaque, target_phys_addr_t offset,
         break;
     case 0x20: /* DMACR */
         if (value) {
-            hw_error("pl022: DMA not implemented\n");
+            qemu_log_mask(LOG_UNIMP, "pl022: DMA not implemented\n");
         }
         break;
     default:
-        hw_error("pl022_write: Bad offset %x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl022_write: Bad offset %x\n", (int)offset);
     }
 }
 
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 8/8] hw/pl031: Use LOG_GUEST_ERROR

[Peter Maydell]

Use LOG_GUEST_ERROR rather than hw_error or direct fprintf.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pl031.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/pl031.c b/hw/pl031.c
index 9602664..6cbaf23 100644
--- a/hw/pl031.c
+++ b/hw/pl031.c
@@ -120,11 +120,13 @@ static uint64_t pl031_read(void *opaque, target_phys_addr_t offset,
     case RTC_MIS:
         return s->is & s->im;
     case RTC_ICR:
-        fprintf(stderr, "qemu: pl031_read: Unexpected offset 0x%x\n",
-                (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl031: read of write-only register at offset 0x%x\n",
+                      (int)offset);
         break;
     default:
-        hw_error("pl031_read: Bad offset 0x%x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl031_read: Bad offset 0x%x\n", (int)offset);
         break;
     }
 
@@ -167,12 +169,14 @@ static void pl031_write(void * opaque, target_phys_addr_t offset,
     case RTC_DR:
     case RTC_MIS:
     case RTC_RIS:
-        fprintf(stderr, "qemu: pl031_write: Unexpected offset 0x%x\n",
-                (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl031: write to read-only register at offset 0x%x\n",
+                      (int)offset);
         break;
 
     default:
-        hw_error("pl031_write: Bad offset 0x%x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl031_write: Bad offset 0x%x\n", (int)offset);
         break;
     }
 }
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v2 2/8] hw/hw.h: Add include of qemu-log.h

[Igor Mammedov]

On Thu, 18 Oct 2012 14:11:36 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> Add an include of qemu-log.h to hw.h, so that device model
> code has access to these logging functions without the need
> to directly include qemu-log.h.
It would be better to include qemu-log.h to files that have
immediate dependency on it, then creating another include all
"qemu-common.h." and compile/preprocess it for every hw file.

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/hw.h |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/hw.h b/hw/hw.h
> index 16101de..b337ee3 100644
> --- a/hw/hw.h
> +++ b/hw/hw.h
> @@ -12,6 +12,7 @@
>  #include "irq.h"
>  #include "qemu-file.h"
>  #include "vmstate.h"
> +#include "qemu-log.h"
>  
>  #ifdef NEED_CPU_H
>  #if TARGET_LONG_BITS == 64

Re: [Qemu-devel] [PATCH v2 2/8] hw/hw.h: Add include of qemu-log.h

[Peter Maydell]

On 18 October 2012 15:26, Igor Mammedov <imammedo@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> Add an include of qemu-log.h to hw.h, so that device model
>> code has access to these logging functions without the need
>> to directly include qemu-log.h.
> It would be better to include qemu-log.h to files that have
> immediate dependency on it, then creating another include all
> "qemu-common.h." and compile/preprocess it for every hw file.

That is what v1 of this patch did; Blue suggested it would
be better to put it in a common header, and I agree:
basically every device model will have cases where it will
want to diagnose bad guest behaviour, and so we would end
up including qemu-log.h in every file in hw/.

-- PMM

Re: [Qemu-devel] [PATCH v2 0/8] Add LOG_GUEST_ERROR for reporting guest bugs

[Blue Swirl]

On Thu, Oct 18, 2012 at 1:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patch series introduces a new logging category to
> qemu_log_mask(), for reporting situations which the device
> model can cope with but which indicate bugs in the guest.
> The most common cause of these is "guest tried to access
> a register which doesn't exist". At the moment device models
> use an inconsistent mix of "silently ignore", "hw_error(),
> killing qemu" and "direct print to stderr" for reporting
> this kind of event.
>
> Patches 3-8 are a random selection of ARM devices which I've
> switched to using the new LOG_GUEST_ERROR (or the existing
> LOG_UNIMP where that is more appropriate). There are obviously
> more conversions that could be made; this is just a sample
> to demonstrate the utility of the log category.
>
> Changes v1->v2:
>  * add patch 2 including qemu-log.h in hw/hw.h
>  * update 3..8 to not include qemu-log.h directly

Thanks, applied all.

>
> Peter Maydell (8):
>   qemu-log: Add new log category for guest bugs
>   hw/hw.h: Add include of qemu-log.h
>   hw/pl181: Use LOG_UNIMP and LOG_GUEST_ERROR
>   hw/pl041: Use LOG_UNIMP
>   hw/pl190: Use LOG_GUEST_ERROR
>   hw/pl011: Use LOG_UNIMP and LOG_GUEST_ERROR
>   hw/pl022: Use LOG_UNIMP and LOG_GUEST_ERROR
>   hw/pl031: Use LOG_GUEST_ERROR
>
>  hw/hw.h    |    1 +
>  hw/pl011.c |   11 +++++++----
>  hw/pl022.c |    8 +++++---
>  hw/pl031.c |   16 ++++++++++------
>  hw/pl041.c |    5 +++--
>  hw/pl181.c |   18 ++++++++++--------
>  hw/pl190.c |    6 ++++--
>  qemu-log.c |    3 +++
>  qemu-log.h |    1 +
>  9 files changed, 44 insertions(+), 25 deletions(-)
>
> --
> 1.7.9.5
>
>