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

[Qemu-devel] [PATCH 0/7] 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 2-7 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.

One question that comes to mind -- should we include qemu-log.h
in qemu-common.h (or some other common header?) rather than
having to include qemu-log.h directly in lots of device model
source files?

Peter Maydell (7):
  qemu-log: Add new log category for guest bugs
  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/pl011.c | 12 ++++++++----
 hw/pl022.c |  9 ++++++---
 hw/pl031.c | 17 +++++++++++------
 hw/pl041.c |  6 ++++--
 hw/pl181.c | 19 +++++++++++--------
 hw/pl190.c |  7 +++++--
 qemu-log.c |  3 +++
 qemu-log.h |  1 +
 8 files changed, 49 insertions(+), 25 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH 1/7] 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.11.4

[Qemu-devel] [PATCH 2/7] 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 | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/pl181.c b/hw/pl181.c
index 7d91fbb..23fc063 100644
--- a/hw/pl181.c
+++ b/hw/pl181.c
@@ -10,6 +10,7 @@
 #include "blockdev.h"
 #include "sysbus.h"
 #include "sd.h"
+#include "qemu-log.h"
 
 //#define DEBUG_PL181 1
 
@@ -352,7 +353,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 +364,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 +389,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 +429,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.11.4

[Qemu-devel] [PATCH 3/7] 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/pl041.c b/hw/pl041.c
index b6723be..260c560 100644
--- a/hw/pl041.c
+++ b/hw/pl041.c
@@ -21,6 +21,7 @@
  */
 
 #include "sysbus.h"
+#include "qemu-log.h"
 
 #include "pl041.h"
 #include "lm4549.h"
@@ -536,8 +537,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.11.4

[Qemu-devel] [PATCH 4/7] 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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/pl190.c b/hw/pl190.c
index 7332f4d..e8b2fc4 100644
--- a/hw/pl190.c
+++ b/hw/pl190.c
@@ -8,6 +8,7 @@
  */
 
 #include "sysbus.h"
+#include "qemu-log.h"
 
 /* The number of virtual priority levels.  16 user vectors plus the
    unvectored IRQ.  Chained interrupts would require an additional level
@@ -143,7 +144,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 +204,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.11.4

[Qemu-devel] [PATCH 5/7] 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 | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/pl011.c b/hw/pl011.c
index 3245702..743eea6 100644
--- a/hw/pl011.c
+++ b/hw/pl011.c
@@ -9,6 +9,7 @@
 
 #include "sysbus.h"
 #include "qemu-char.h"
+#include "qemu-log.h"
 
 typedef struct {
     SysBusDevice busdev;
@@ -107,7 +108,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 +180,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.11.4

[Qemu-devel] [PATCH 6/7] 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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/pl022.c b/hw/pl022.c
index 60e35da..91e5df0 100644
--- a/hw/pl022.c
+++ b/hw/pl022.c
@@ -9,6 +9,7 @@
 
 #include "sysbus.h"
 #include "ssi.h"
+#include "qemu-log.h"
 
 //#define DEBUG_PL022 1
 
@@ -168,7 +169,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 +213,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.11.4

[Qemu-devel] [PATCH 7/7] 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 | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/pl031.c b/hw/pl031.c
index 9602664..a718d2e 100644
--- a/hw/pl031.c
+++ b/hw/pl031.c
@@ -14,6 +14,7 @@
 #include "sysbus.h"
 #include "qemu-timer.h"
 #include "sysemu.h"
+#include "qemu-log.h"
 
 //#define DEBUG_PL031
 
@@ -120,11 +121,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 +170,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.11.4

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

[Blue Swirl]

On Sun, Oct 14, 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 2-7 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.

The conversion is not entirely trivial, since hw_error() does not
return but qemu_log_mask() does. Your changes look OK.

>
> One question that comes to mind -- should we include qemu-log.h
> in qemu-common.h (or some other common header?) rather than
> having to include qemu-log.h directly in lots of device model
> source files?

Yes.

>
> Peter Maydell (7):
>   qemu-log: Add new log category for guest bugs
>   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/pl011.c | 12 ++++++++----
>  hw/pl022.c |  9 ++++++---
>  hw/pl031.c | 17 +++++++++++------
>  hw/pl041.c |  6 ++++--
>  hw/pl181.c | 19 +++++++++++--------
>  hw/pl190.c |  7 +++++--
>  qemu-log.c |  3 +++
>  qemu-log.h |  1 +
>  8 files changed, 49 insertions(+), 25 deletions(-)
>
> --
> 1.7.11.4
>

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

[Peter Maydell]

On 14 October 2012 15:02, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Oct 14, 2012 at 1:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> One question that comes to mind -- should we include qemu-log.h
>> in qemu-common.h (or some other common header?) rather than
>> having to include qemu-log.h directly in lots of device model
>> source files?
>
> Yes.

This turns out not to be quite as trivial as I'd thought,
since qemu-log.h requires (if NEED_CPU_H is defined) definitions
of types like target_ulong. So we can't include it from qemu-common.h
unless we change all the target-*/cpu.h to include cpu-defs.h
before qemu-common.h. Alternatively we could split the NEED_CPU_H
bits of qemu-log.h out into a qemu-cpu-log.h which would be
manually included by the bits of qemu that needed it.

The other option would be to include qemu-log.h from hw/hw.h.
This just works without requiring any complicated workarounds,
and all the device code includes hw.h already...

-- PMM

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

[Blue Swirl]

On Sun, Oct 14, 2012 at 3:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 14 October 2012 15:02, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Oct 14, 2012 at 1:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> One question that comes to mind -- should we include qemu-log.h
>>> in qemu-common.h (or some other common header?) rather than
>>> having to include qemu-log.h directly in lots of device model
>>> source files?
>>
>> Yes.
>
> This turns out not to be quite as trivial as I'd thought,
> since qemu-log.h requires (if NEED_CPU_H is defined) definitions
> of types like target_ulong. So we can't include it from qemu-common.h
> unless we change all the target-*/cpu.h to include cpu-defs.h
> before qemu-common.h. Alternatively we could split the NEED_CPU_H
> bits of qemu-log.h out into a qemu-cpu-log.h which would be
> manually included by the bits of qemu that needed it.
>
> The other option would be to include qemu-log.h from hw/hw.h.
> This just works without requiring any complicated workarounds,
> and all the device code includes hw.h already...

I was also thinking about that but then I thought that it would be
nice to use qemu-log.h elsewhere too. But hw/hw.h sounds much easier
for now.

>
> -- PMM