[Qemu-devel] [PATCH 0/6] dbdma: improve logging and various fixes

[Qemu-devel] [PATCH 0/6] dbdma: improve logging and various fixes

[Mark Cave-Ayland]

This patchset is based upon some work Ben H did to fix various DBDMA issues
found whilst trying to boot MacOS 9 (effectively a minimal rework of a
WIP diff). With this patch, along with the patch for odd MSR combinations,
it becomes possible to boot MacOS 9 relibably in QEMU.

It has been part of my local tests for a few weeks now, however since the
PowerNV work has caused regressions in my MacOS 9 tests, Ben suggested 
that I submit the patchset anyway with his Ack to allow others to test their
latest patches before submission.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Mark Cave-Ayland (6):
  dbdma: always define DBDMA_DPRINTF and enable debug with DEBUG_DBDMA
  dbdma: add per-channel debugging enabled via DEBUG_DBDMA_CHANMASK
  dbdma: fix endian of DBDMA_CMDPTR_LO during branch
  dbdma: fix load_word/store_word value endianness
  dbdma: set FLUSH bit upon reception of flush command for unassigned
    DBDMA channels
  dbdma: reset io->processing flag for unassigned DBDMA channel rw
    accesses

 hw/misc/macio/mac_dbdma.c |  125 +++++++++++++++++++++++----------------------
 1 file changed, 65 insertions(+), 60 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/6] dbdma: always define DBDMA_DPRINTF and enable debug with DEBUG_DBDMA

[Mark Cave-Ayland]

Enabling DBDMA_DPRINTF unconditionally ensures that any errors in debug
statements are picked up immediately.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/misc/macio/mac_dbdma.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index f116f9c..b6639f4 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -45,14 +45,13 @@
 #include "sysemu/dma.h"
 
 /* debug DBDMA */
-//#define DEBUG_DBDMA
+#define DEBUG_DBDMA 0
 
-#ifdef DEBUG_DBDMA
-#define DBDMA_DPRINTF(fmt, ...)                                 \
-    do { printf("DBDMA: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DBDMA_DPRINTF(fmt, ...)
-#endif
+#define DBDMA_DPRINTF(fmt, ...) do { \
+    if (DEBUG_DBDMA) { \
+        printf("DBDMA: " fmt , ## __VA_ARGS__); \
+    } \
+} while (0);
 
 /*
  */
@@ -62,7 +61,7 @@ static DBDMAState *dbdma_from_ch(DBDMA_channel *ch)
     return container_of(ch, DBDMAState, channels[ch->channel]);
 }
 
-#ifdef DEBUG_DBDMA
+#if DEBUG_DBDMA
 static void dump_dbdma_cmd(dbdma_cmd *cmd)
 {
     printf("dbdma_cmd %p\n", cmd);
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/6] dbdma: add per-channel debugging enabled via DEBUG_DBDMA_CHANMASK

[Mark Cave-Ayland]

By default large amounts of DBDMA debugging are produced when often it is just
1 or 2 channels that are of interest. Introduce DEBUG_DBDMA_CHANMASK to allow
the developer to select the channels of interest at compile time, and then
further add the extra channel information to each debug statement where
possible.

Also clearly mark the start/end of DBDMA_run_bh to allow tracking the bottom
half execution.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/misc/macio/mac_dbdma.c |   75 +++++++++++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 33 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index b6639f4..8e4b208 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -46,6 +46,7 @@
 
 /* debug DBDMA */
 #define DEBUG_DBDMA 0
+#define DEBUG_DBDMA_CHANMASK ((1ul << DBDMA_CHANNELS) - 1)
 
 #define DBDMA_DPRINTF(fmt, ...) do { \
     if (DEBUG_DBDMA) { \
@@ -53,6 +54,14 @@
     } \
 } while (0);
 
+#define DBDMA_DPRINTFCH(ch, fmt, ...) do { \
+    if (DEBUG_DBDMA) { \
+        if ((1ul << (ch)->channel) & DEBUG_DBDMA_CHANMASK) { \
+            printf("DBDMA[%02x]: " fmt , (ch)->channel, ## __VA_ARGS__); \
+        } \
+    } \
+} while (0);
+
 /*
  */
 
@@ -79,26 +88,26 @@ static void dump_dbdma_cmd(dbdma_cmd *cmd)
 #endif
 static void dbdma_cmdptr_load(DBDMA_channel *ch)
 {
-    DBDMA_DPRINTF("dbdma_cmdptr_load 0x%08x\n",
-                  ch->regs[DBDMA_CMDPTR_LO]);
+    DBDMA_DPRINTFCH(ch, "dbdma_cmdptr_load 0x%08x\n",
+                    ch->regs[DBDMA_CMDPTR_LO]);
     dma_memory_read(&address_space_memory, ch->regs[DBDMA_CMDPTR_LO],
                     &ch->current, sizeof(dbdma_cmd));
 }
 
 static void dbdma_cmdptr_save(DBDMA_channel *ch)
 {
-    DBDMA_DPRINTF("dbdma_cmdptr_save 0x%08x\n",
-                  ch->regs[DBDMA_CMDPTR_LO]);
-    DBDMA_DPRINTF("xfer_status 0x%08x res_count 0x%04x\n",
-                  le16_to_cpu(ch->current.xfer_status),
-                  le16_to_cpu(ch->current.res_count));
+    DBDMA_DPRINTFCH(ch, "dbdma_cmdptr_save 0x%08x\n",
+                    ch->regs[DBDMA_CMDPTR_LO]);
+    DBDMA_DPRINTFCH(ch, "xfer_status 0x%08x res_count 0x%04x\n",
+                    le16_to_cpu(ch->current.xfer_status),
+                    le16_to_cpu(ch->current.res_count));
     dma_memory_write(&address_space_memory, ch->regs[DBDMA_CMDPTR_LO],
                      &ch->current, sizeof(dbdma_cmd));
 }
 
 static void kill_channel(DBDMA_channel *ch)
 {
-    DBDMA_DPRINTF("kill_channel\n");
+    DBDMA_DPRINTFCH(ch, "kill_channel\n");
 
     ch->regs[DBDMA_STATUS] |= DEAD;
     ch->regs[DBDMA_STATUS] &= ~ACTIVE;
@@ -114,7 +123,7 @@ static void conditional_interrupt(DBDMA_channel *ch)
     uint32_t status;
     int cond;
 
-    DBDMA_DPRINTF("%s\n", __func__);
+    DBDMA_DPRINTFCH(ch, "%s\n", __func__);
 
     intr = le16_to_cpu(current->command) & INTR_MASK;
 
@@ -123,7 +132,7 @@ static void conditional_interrupt(DBDMA_channel *ch)
         return;
     case INTR_ALWAYS: /* always interrupt */
         qemu_irq_raise(ch->irq);
-        DBDMA_DPRINTF("%s: raise\n", __func__);
+        DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
         return;
     }
 
@@ -138,13 +147,13 @@ static void conditional_interrupt(DBDMA_channel *ch)
     case INTR_IFSET:  /* intr if condition bit is 1 */
         if (cond) {
             qemu_irq_raise(ch->irq);
-            DBDMA_DPRINTF("%s: raise\n", __func__);
+            DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
         }
         return;
     case INTR_IFCLR:  /* intr if condition bit is 0 */
         if (!cond) {
             qemu_irq_raise(ch->irq);
-            DBDMA_DPRINTF("%s: raise\n", __func__);
+            DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
         }
         return;
     }
@@ -158,7 +167,7 @@ static int conditional_wait(DBDMA_channel *ch)
     uint32_t status;
     int cond;
 
-    DBDMA_DPRINTF("conditional_wait\n");
+    DBDMA_DPRINTFCH(ch, "conditional_wait\n");
 
     wait = le16_to_cpu(current->command) & WAIT_MASK;
 
@@ -217,7 +226,7 @@ static void conditional_branch(DBDMA_channel *ch)
     uint32_t status;
     int cond;
 
-    DBDMA_DPRINTF("conditional_branch\n");
+    DBDMA_DPRINTFCH(ch, "conditional_branch\n");
 
     /* check if we must branch */
 
@@ -262,7 +271,7 @@ static void dbdma_end(DBDMA_io *io)
     DBDMA_channel *ch = io->channel;
     dbdma_cmd *current = &ch->current;
 
-    DBDMA_DPRINTF("%s\n", __func__);
+    DBDMA_DPRINTFCH(ch, "%s\n", __func__);
 
     if (conditional_wait(ch))
         goto wait;
@@ -288,13 +297,13 @@ wait:
 static void start_output(DBDMA_channel *ch, int key, uint32_t addr,
                         uint16_t req_count, int is_last)
 {
-    DBDMA_DPRINTF("start_output\n");
+    DBDMA_DPRINTFCH(ch, "start_output\n");
 
     /* KEY_REGS, KEY_DEVICE and KEY_STREAM
      * are not implemented in the mac-io chip
      */
 
-    DBDMA_DPRINTF("addr 0x%x key 0x%x\n", addr, key);
+    DBDMA_DPRINTFCH(ch, "addr 0x%x key 0x%x\n", addr, key);
     if (!addr || key > KEY_STREAM3) {
         kill_channel(ch);
         return;
@@ -314,13 +323,13 @@ static void start_output(DBDMA_channel *ch, int key, uint32_t addr,
 static void start_input(DBDMA_channel *ch, int key, uint32_t addr,
                        uint16_t req_count, int is_last)
 {
-    DBDMA_DPRINTF("start_input\n");
+    DBDMA_DPRINTFCH(ch, "start_input\n");
 
     /* KEY_REGS, KEY_DEVICE and KEY_STREAM
      * are not implemented in the mac-io chip
      */
 
-    DBDMA_DPRINTF("addr 0x%x key 0x%x\n", addr, key);
+    DBDMA_DPRINTFCH(ch, "addr 0x%x key 0x%x\n", addr, key);
     if (!addr || key > KEY_STREAM3) {
         kill_channel(ch);
         return;
@@ -343,7 +352,7 @@ static void load_word(DBDMA_channel *ch, int key, uint32_t addr,
     dbdma_cmd *current = &ch->current;
     uint32_t val;
 
-    DBDMA_DPRINTF("load_word\n");
+    DBDMA_DPRINTFCH(ch, "load_word\n");
 
     /* only implements KEY_SYSTEM */
 
@@ -382,7 +391,7 @@ static void store_word(DBDMA_channel *ch, int key, uint32_t addr,
     dbdma_cmd *current = &ch->current;
     uint32_t val;
 
-    DBDMA_DPRINTF("store_word\n");
+    DBDMA_DPRINTFCH(ch, "store_word\n");
 
     /* only implements KEY_SYSTEM */
 
@@ -445,7 +454,7 @@ static void channel_run(DBDMA_channel *ch)
     uint16_t req_count;
     uint32_t phy_addr;
 
-    DBDMA_DPRINTF("channel_run\n");
+    DBDMA_DPRINTFCH(ch, "channel_run\n");
     dump_dbdma_cmd(current);
 
     /* clear WAKE flag at command fetch */
@@ -539,9 +548,9 @@ static void DBDMA_run_bh(void *opaque)
 {
     DBDMAState *s = opaque;
 
-    DBDMA_DPRINTF("DBDMA_run_bh\n");
-
+    DBDMA_DPRINTF("-> DBDMA_run_bh\n");
     DBDMA_run(s);
+    DBDMA_DPRINTF("<- DBDMA_run_bh\n");
 }
 
 void DBDMA_kick(DBDMAState *dbdma)
@@ -556,7 +565,7 @@ void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
     DBDMAState *s = dbdma;
     DBDMA_channel *ch = &s->channels[nchan];
 
-    DBDMA_DPRINTF("DBDMA_register_channel 0x%x\n", nchan);
+    DBDMA_DPRINTFCH(ch, "DBDMA_register_channel 0x%x\n", nchan);
 
     assert(rw);
     assert(flush);
@@ -600,7 +609,7 @@ dbdma_control_write(DBDMA_channel *ch)
         status &= ~FLUSH;
     }
 
-    DBDMA_DPRINTF("    status 0x%08x\n", status);
+    DBDMA_DPRINTFCH(ch, "    status 0x%08x\n", status);
 
     ch->regs[DBDMA_STATUS] = status;
 
@@ -617,10 +626,10 @@ static void dbdma_write(void *opaque, hwaddr addr,
     DBDMA_channel *ch = &s->channels[channel];
     int reg = (addr - (channel << DBDMA_CHANNEL_SHIFT)) >> 2;
 
-    DBDMA_DPRINTF("writel 0x" TARGET_FMT_plx " <= 0x%08"PRIx64"\n",
-                  addr, value);
-    DBDMA_DPRINTF("channel 0x%x reg 0x%x\n",
-                  (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
+    DBDMA_DPRINTFCH(ch, "writel 0x" TARGET_FMT_plx " <= 0x%08"PRIx64"\n",
+                    addr, value);
+    DBDMA_DPRINTFCH(ch, "channel 0x%x reg 0x%x\n",
+                    (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
 
     /* cmdptr cannot be modified if channel is ACTIVE */
 
@@ -671,9 +680,9 @@ static uint64_t dbdma_read(void *opaque, hwaddr addr,
 
     value = ch->regs[reg];
 
-    DBDMA_DPRINTF("readl 0x" TARGET_FMT_plx " => 0x%08x\n", addr, value);
-    DBDMA_DPRINTF("channel 0x%x reg 0x%x\n",
-                  (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
+    DBDMA_DPRINTFCH(ch, "readl 0x" TARGET_FMT_plx " => 0x%08x\n", addr, value);
+    DBDMA_DPRINTFCH(ch, "channel 0x%x reg 0x%x\n",
+                    (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
 
     switch(reg) {
     case DBDMA_CONTROL:
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/6] dbdma: fix endian of DBDMA_CMDPTR_LO during branch

[Mark Cave-Ayland]

The current DBDMA command is stored in little-endian format, so make sure
we convert it to match our CPU when updating the DBDMA_CMDPTR_LO register.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/misc/macio/mac_dbdma.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 8e4b208..bb52cb9 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -213,7 +213,7 @@ static void branch(DBDMA_channel *ch)
 {
     dbdma_cmd *current = &ch->current;
 
-    ch->regs[DBDMA_CMDPTR_LO] = current->cmd_dep;
+    ch->regs[DBDMA_CMDPTR_LO] = le32_to_cpu(current->cmd_dep);
     ch->regs[DBDMA_STATUS] |= BT;
     dbdma_cmdptr_load(ch);
 }
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/6] dbdma: fix load_word/store_word value endianness

[Mark Cave-Ayland]

The values to read/write to/from physical memory are copied directly to the
physical address with no endian swapping required.

Also add some extra information to debugging output while we are here.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/misc/macio/mac_dbdma.c |   24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index bb52cb9..f437a55 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -350,9 +350,8 @@ static void load_word(DBDMA_channel *ch, int key, uint32_t addr,
                      uint16_t len)
 {
     dbdma_cmd *current = &ch->current;
-    uint32_t val;
 
-    DBDMA_DPRINTFCH(ch, "load_word\n");
+    DBDMA_DPRINTFCH(ch, "load_word %d bytes, addr=%08x\n", len, addr);
 
     /* only implements KEY_SYSTEM */
 
@@ -362,14 +361,7 @@ static void load_word(DBDMA_channel *ch, int key, uint32_t addr,
         return;
     }
 
-    dma_memory_read(&address_space_memory, addr, &val, len);
-
-    if (len == 2)
-        val = (val << 16) | (current->cmd_dep & 0x0000ffff);
-    else if (len == 1)
-        val = (val << 24) | (current->cmd_dep & 0x00ffffff);
-
-    current->cmd_dep = val;
+    dma_memory_read(&address_space_memory, addr, &current->cmd_dep, len);
 
     if (conditional_wait(ch))
         goto wait;
@@ -389,9 +381,9 @@ static void store_word(DBDMA_channel *ch, int key, uint32_t addr,
                       uint16_t len)
 {
     dbdma_cmd *current = &ch->current;
-    uint32_t val;
 
-    DBDMA_DPRINTFCH(ch, "store_word\n");
+    DBDMA_DPRINTFCH(ch, "store_word %d bytes, addr=%08x pa=%x\n",
+                    len, addr, le32_to_cpu(current->cmd_dep));
 
     /* only implements KEY_SYSTEM */
 
@@ -401,13 +393,7 @@ static void store_word(DBDMA_channel *ch, int key, uint32_t addr,
         return;
     }
 
-    val = current->cmd_dep;
-    if (len == 2)
-        val >>= 16;
-    else if (len == 1)
-        val >>= 24;
-
-    dma_memory_write(&address_space_memory, addr, &val, len);
+    dma_memory_write(&address_space_memory, addr, &current->cmd_dep, len);
 
     if (conditional_wait(ch))
         goto wait;
-- 
1.7.10.4

[Qemu-devel] [PATCH 5/6] dbdma: set FLUSH bit upon reception of flush command for unassigned DBDMA channels

[Mark Cave-Ayland]

This fixes MacOS 9 whereby it continually flushes and polls the status bits
until they are set to indicate a successful flush.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/misc/macio/mac_dbdma.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index f437a55..cb740c1 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -783,8 +783,18 @@ static void dbdma_unassigned_rw(DBDMA_io *io)
 static void dbdma_unassigned_flush(DBDMA_io *io)
 {
     DBDMA_channel *ch = io->channel;
+    dbdma_cmd *current = &ch->current;
+    uint16_t cmd;
     qemu_log_mask(LOG_GUEST_ERROR, "%s: use of unassigned channel %d\n",
                   __func__, ch->channel);
+
+    cmd = le16_to_cpu(current->command) & COMMAND_MASK;
+    if (cmd == OUTPUT_MORE || cmd == OUTPUT_LAST ||
+        cmd == INPUT_MORE || cmd == INPUT_LAST) {
+        current->xfer_status = cpu_to_le16(ch->regs[DBDMA_STATUS] | FLUSH);
+        current->res_count = cpu_to_le16(io->len);
+        dbdma_cmdptr_save(ch);
+    }
 }
 
 void* DBDMA_init (MemoryRegion **dbdma_mem)
-- 
1.7.10.4

[Qemu-devel] [PATCH 6/6] dbdma: reset io->processing flag for unassigned DBDMA channel rw accesses

[Mark Cave-Ayland]

Otherwise MacOS 9 hangs upon shutdown.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/misc/macio/mac_dbdma.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index cb740c1..335a5e4 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -778,6 +778,7 @@ static void dbdma_unassigned_rw(DBDMA_io *io)
     DBDMA_channel *ch = io->channel;
     qemu_log_mask(LOG_GUEST_ERROR, "%s: use of unassigned channel %d\n",
                   __func__, ch->channel);
+    ch->io.processing = false;
 }
 
 static void dbdma_unassigned_flush(DBDMA_io *io)
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 0/6] dbdma: improve logging and various fixes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]

On Sun, Jul 10, 2016 at 07:08:52PM +0100, Mark Cave-Ayland wrote:
> This patchset is based upon some work Ben H did to fix various DBDMA issues
> found whilst trying to boot MacOS 9 (effectively a minimal rework of a
> WIP diff). With this patch, along with the patch for odd MSR combinations,
> it becomes possible to boot MacOS 9 relibably in QEMU.
> 
> It has been part of my local tests for a few weeks now, however since the
> PowerNV work has caused regressions in my MacOS 9 tests, Ben suggested 
> that I submit the patchset anyway with his Ack to allow others to test their
> latest patches before submission.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Series applied to ppc-for-2.7, thanks.
> 
> Mark Cave-Ayland (6):
>   dbdma: always define DBDMA_DPRINTF and enable debug with DEBUG_DBDMA
>   dbdma: add per-channel debugging enabled via DEBUG_DBDMA_CHANMASK
>   dbdma: fix endian of DBDMA_CMDPTR_LO during branch
>   dbdma: fix load_word/store_word value endianness
>   dbdma: set FLUSH bit upon reception of flush command for unassigned
>     DBDMA channels
>   dbdma: reset io->processing flag for unassigned DBDMA channel rw
>     accesses
> 
>  hw/misc/macio/mac_dbdma.c |  125 +++++++++++++++++++++++----------------------
>  1 file changed, 65 insertions(+), 60 deletions(-)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/6] dbdma: add per-channel debugging enabled via DEBUG_DBDMA_CHANMASK

[Mark Cave-Ayland]

On 10/07/16 19:08, Mark Cave-Ayland wrote:

> By default large amounts of DBDMA debugging are produced when often it is just
> 1 or 2 channels that are of interest. Introduce DEBUG_DBDMA_CHANMASK to allow
> the developer to select the channels of interest at compile time, and then
> further add the extra channel information to each debug statement where
> possible.
> 
> Also clearly mark the start/end of DBDMA_run_bh to allow tracking the bottom
> half execution.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  hw/misc/macio/mac_dbdma.c |   75 +++++++++++++++++++++++++--------------------
>  1 file changed, 42 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> index b6639f4..8e4b208 100644
> --- a/hw/misc/macio/mac_dbdma.c
> +++ b/hw/misc/macio/mac_dbdma.c
> @@ -46,6 +46,7 @@
>  
>  /* debug DBDMA */
>  #define DEBUG_DBDMA 0
> +#define DEBUG_DBDMA_CHANMASK ((1ul << DBDMA_CHANNELS) - 1)

Someone flagged up that this doesn't work on 32-bit hosts because while
the channel is 0 to 31, the compiler shifts first and then subtracts
which gives warnings like this:

hw/misc/macio/mac_dbdma.c: In function ‘dbdma_cmdptr_load’:
hw/misc/macio/mac_dbdma.c:49:36: error: left shift count >= width of
type [-Werror=shift-count-overflow]
 #define DEBUG_DBDMA_CHANMASK ((1ul << DBDMA_CHANNELS) - 1)

The fix here is to change the above to:

#define DEBUG_DBDMA_CHANMASK ((1ull << DBDMA_CHANNELS) - 1)

Here the unsigned long long will ensure that the shift is valid before
the subtraction to get the final 32-bit wide mask.

David, are you able to fix this up in your branch?

>  #define DBDMA_DPRINTF(fmt, ...) do { \
>      if (DEBUG_DBDMA) { \
> @@ -53,6 +54,14 @@
>      } \
>  } while (0);
>  
> +#define DBDMA_DPRINTFCH(ch, fmt, ...) do { \
> +    if (DEBUG_DBDMA) { \
> +        if ((1ul << (ch)->channel) & DEBUG_DBDMA_CHANMASK) { \
> +            printf("DBDMA[%02x]: " fmt , (ch)->channel, ## __VA_ARGS__); \
> +        } \
> +    } \
> +} while (0);
> +

This part is still okay for a channel range 0 to 31.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 2/6] dbdma: add per-channel debugging enabled via DEBUG_DBDMA_CHANMASK

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2658 bytes --]

On Fri, Jul 15, 2016 at 07:42:51AM +0100, Mark Cave-Ayland wrote:
> On 10/07/16 19:08, Mark Cave-Ayland wrote:
> 
> > By default large amounts of DBDMA debugging are produced when often it is just
> > 1 or 2 channels that are of interest. Introduce DEBUG_DBDMA_CHANMASK to allow
> > the developer to select the channels of interest at compile time, and then
> > further add the extra channel information to each debug statement where
> > possible.
> > 
> > Also clearly mark the start/end of DBDMA_run_bh to allow tracking the bottom
> > half execution.
> > 
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >  hw/misc/macio/mac_dbdma.c |   75 +++++++++++++++++++++++++--------------------
> >  1 file changed, 42 insertions(+), 33 deletions(-)
> > 
> > diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> > index b6639f4..8e4b208 100644
> > --- a/hw/misc/macio/mac_dbdma.c
> > +++ b/hw/misc/macio/mac_dbdma.c
> > @@ -46,6 +46,7 @@
> >  
> >  /* debug DBDMA */
> >  #define DEBUG_DBDMA 0
> > +#define DEBUG_DBDMA_CHANMASK ((1ul << DBDMA_CHANNELS) - 1)
> 
> Someone flagged up that this doesn't work on 32-bit hosts because while
> the channel is 0 to 31, the compiler shifts first and then subtracts
> which gives warnings like this:
> 
> hw/misc/macio/mac_dbdma.c: In function ‘dbdma_cmdptr_load’:
> hw/misc/macio/mac_dbdma.c:49:36: error: left shift count >= width of
> type [-Werror=shift-count-overflow]
>  #define DEBUG_DBDMA_CHANMASK ((1ul << DBDMA_CHANNELS) - 1)
> 
> The fix here is to change the above to:
> 
> #define DEBUG_DBDMA_CHANMASK ((1ull << DBDMA_CHANNELS) - 1)
> 
> Here the unsigned long long will ensure that the shift is valid before
> the subtraction to get the final 32-bit wide mask.
> 
> David, are you able to fix this up in your branch?

Done.

> 
> >  #define DBDMA_DPRINTF(fmt, ...) do { \
> >      if (DEBUG_DBDMA) { \
> > @@ -53,6 +54,14 @@
> >      } \
> >  } while (0);
> >  
> > +#define DBDMA_DPRINTFCH(ch, fmt, ...) do { \
> > +    if (DEBUG_DBDMA) { \
> > +        if ((1ul << (ch)->channel) & DEBUG_DBDMA_CHANMASK) { \
> > +            printf("DBDMA[%02x]: " fmt , (ch)->channel, ## __VA_ARGS__); \
> > +        } \
> > +    } \
> > +} while (0);
> > +
> 
> This part is still okay for a channel range 0 to 31.
> 
> 
> ATB,
> 
> Mark.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]