Re: [Qemu-devel] [PATCH 04/11] heathrow: convert to trace-events

2018-02-19 Thread Mark Cave-Ayland 

On 20/02/18 04:20, David Gibson wrote:


On Mon, Feb 19, 2018 at 06:19:15PM +, Mark Cave-Ayland wrote:

Signed-off-by: Mark Cave-Ayland 
---
  hw/intc/heathrow_pic.c | 31 ---
  hw/intc/trace-events   |  5 +
  include/hw/intc/heathrow_pic.h |  1 +
  3 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/hw/intc/heathrow_pic.c b/hw/intc/heathrow_pic.c
index 7bf44e0d86..afe0b08cbb 100644
--- a/hw/intc/heathrow_pic.c
+++ b/hw/intc/heathrow_pic.c
@@ -26,16 +26,7 @@
  #include "hw/hw.h"
  #include "hw/ppc/mac.h"
  #include "hw/intc/heathrow_pic.h"
-
-/* debug PIC */
-//#define DEBUG_PIC
-
-#ifdef DEBUG_PIC
-#define PIC_DPRINTF(fmt, ...)   \
-do { printf("PIC: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define PIC_DPRINTF(fmt, ...)
-#endif
+#include "trace.h"
  
  static inline int heathrow_check_irq(HeathrowPICState *pic)

  {
@@ -61,7 +52,7 @@ static void heathrow_write(void *opaque, hwaddr addr,
  unsigned int n;
  
  n = ((addr & 0xfff) - 0x10) >> 4;

-PIC_DPRINTF("writel: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
+trace_heathrow_write(addr, n, value);
  if (n >= 2)
  return;
  pic = >pics[n];
@@ -109,7 +100,7 @@ static uint64_t heathrow_read(void *opaque, hwaddr addr,
  break;
  }
  }
-PIC_DPRINTF("readl: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
+trace_heathrow_read(addr, n, value);
  return value;
  }
  
@@ -124,16 +115,18 @@ static void heathrow_set_irq(void *opaque, int num, int level)

  HeathrowState *s = opaque;
  HeathrowPICState *pic;
  unsigned int irq_bit;
+int last_level = (s->last_levels & (num << 1UL)) ? 1 : 0;
  
-#if defined(DEBUG)

-{
-static int last_level[64];
-if (last_level[num] != level) {
-PIC_DPRINTF("set_irq: num=0x%02x level=%d\n", num, level);
-last_level[num] = level;
+if (last_level != level) {
+trace_heathrow_set_irq(num, level);
+
+if (level) {
+s->last_levels |= (num << 1UL);
+} else {
+s->last_levels &= ~(num << 1UL);


Cleaning up this last_level stuff is a good idea, but is a bit more
involved than merely converting to trace-events.

It's also not really the "last" level - it's effectively maintaining
the current level of the input irqs.  .. and isn't that information
already within the HeathrowPICState structure?


Yeah actually you're right here - I was so focused on converting the 
code as-is that I didn't really look at what was happening at the end of 
heathrow_set_irq(). The motivation was that without this hack the trace 
output is always outputting the IRQ status, presumably because we have a 
level-triggered interrupt being asserted.


Let me rework this patch to use the last state information from 
HeathrowPICState instead.



ATB,

Mark.

Re: [Qemu-devel] [PATCH 04/11] heathrow: convert to trace-events

2018-02-19 Thread David Gibson 
On Mon, Feb 19, 2018 at 06:19:15PM +, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/intc/heathrow_pic.c | 31 ---
>  hw/intc/trace-events   |  5 +
>  include/hw/intc/heathrow_pic.h |  1 +
>  3 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/intc/heathrow_pic.c b/hw/intc/heathrow_pic.c
> index 7bf44e0d86..afe0b08cbb 100644
> --- a/hw/intc/heathrow_pic.c
> +++ b/hw/intc/heathrow_pic.c
> @@ -26,16 +26,7 @@
>  #include "hw/hw.h"
>  #include "hw/ppc/mac.h"
>  #include "hw/intc/heathrow_pic.h"
> -
> -/* debug PIC */
> -//#define DEBUG_PIC
> -
> -#ifdef DEBUG_PIC
> -#define PIC_DPRINTF(fmt, ...)   \
> -do { printf("PIC: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define PIC_DPRINTF(fmt, ...)
> -#endif
> +#include "trace.h"
>  
>  static inline int heathrow_check_irq(HeathrowPICState *pic)
>  {
> @@ -61,7 +52,7 @@ static void heathrow_write(void *opaque, hwaddr addr,
>  unsigned int n;
>  
>  n = ((addr & 0xfff) - 0x10) >> 4;
> -PIC_DPRINTF("writel: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
> +trace_heathrow_write(addr, n, value);
>  if (n >= 2)
>  return;
>  pic = >pics[n];
> @@ -109,7 +100,7 @@ static uint64_t heathrow_read(void *opaque, hwaddr addr,
>  break;
>  }
>  }
> -PIC_DPRINTF("readl: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
> +trace_heathrow_read(addr, n, value);
>  return value;
>  }
>  
> @@ -124,16 +115,18 @@ static void heathrow_set_irq(void *opaque, int num, int 
> level)
>  HeathrowState *s = opaque;
>  HeathrowPICState *pic;
>  unsigned int irq_bit;
> +int last_level = (s->last_levels & (num << 1UL)) ? 1 : 0;
>  
> -#if defined(DEBUG)
> -{
> -static int last_level[64];
> -if (last_level[num] != level) {
> -PIC_DPRINTF("set_irq: num=0x%02x level=%d\n", num, level);
> -last_level[num] = level;
> +if (last_level != level) {
> +trace_heathrow_set_irq(num, level);
> +
> +if (level) {
> +s->last_levels |= (num << 1UL);
> +} else {
> +s->last_levels &= ~(num << 1UL);

Cleaning up this last_level stuff is a good idea, but is a bit more
involved than merely converting to trace-events.

It's also not really the "last" level - it's effectively maintaining
the current level of the input irqs.  .. and isn't that information
already within the HeathrowPICState structure?

>  }
>  }
> -#endif
> +
>  pic = >pics[1 - (num >> 5)];
>  irq_bit = 1 << (num & 0x1f);
>  if (level) {
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 4092d2825e..55e8c2570c 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -186,3 +186,8 @@ nvic_complete_irq(int irq, bool secure) "NVIC complete 
> IRQ %d (secure %d)"
>  nvic_set_irq_level(int irq, int level) "NVIC external irq %d level set to %d"
>  nvic_sysreg_read(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg 
> read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
>  nvic_sysreg_write(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg 
> write addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
> +
> +# hw/intc/heathrow_pic.c
> +heathrow_write(uint64_t addr, unsigned int n, uint64_t value) "0x%"PRIx64" 
> %u: 0x%"PRIx64
> +heathrow_read(uint64_t addr, unsigned int n, uint64_t value) "0x%"PRIx64" 
> %u: 0x%"PRIx64
> +heathrow_set_irq(int num, int level) "set_irq: num=0x%02x level=%d"
> diff --git a/include/hw/intc/heathrow_pic.h b/include/hw/intc/heathrow_pic.h
> index bc3ffaab87..878a0f7cdb 100644
> --- a/include/hw/intc/heathrow_pic.h
> +++ b/include/hw/intc/heathrow_pic.h
> @@ -42,6 +42,7 @@ typedef struct HeathrowState {
>  MemoryRegion mem;
>  HeathrowPICState pics[2];
>  qemu_irq *irqs;
> +uint64_t last_levels;
>  } HeathrowState;
>  
>  #define HEATHROW_NUM_IRQS 64

-- 
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


signature.asc
Description: PGP signature