[Qemu-devel] [PATCH 0/6] openpic: first batch of cleanups and minor fixes

[Qemu-devel] [PATCH 0/6] openpic: first batch of cleanups and minor fixes

[Scott Wood]

There'll be more to come, but here's an initial set of cleanups
and fixes for openpic.

This is based on the ppc-mpic-cleanup branch in
git://repo.or.cz/qemu/agraf.git

Scott Wood (6):
  openpic: symbolicize some magic numbers
  openpic: remove pcsr (CPU sensitivity register)
  openpic: support large vectors on FSL mpic
  openpic: don't crash on a register access without a CPU context
  openpic: BRR1 is not a CPU-specific register.
  openpic: s/opp->nb_irqs -1/opp->nb_cpus - 1/

 hw/openpic.c |  102 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 59 insertions(+), 43 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/6] openpic: symbolicize some magic numbers

[Scott Wood]

Deefine symbolic names for some register bits, and use some that
have already been defined.

Also convert some register values from hex to decimal when it improves
readability.

IPVP_PRIORITY_MASK is corrected from (0x1F << 16) to (0xF << 16), in
conjunction with making wider use of the symbolic name.  I looked at
Freescale and IBM MPIC docs and at the base OpenPIC spec, and all three
had priority as 4 bits rather than 5.  Plus, the magic nubmer that is
being replaced with symbolic values treated the field as 4 bits wide.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 hw/openpic.c |   54 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/hw/openpic.c b/hw/openpic.c
index 25d5cd7..7e72c29 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -124,6 +124,11 @@
 
 #define VENI_GENERIC      0x00000000 /* Generic Vendor ID */
 
+#define GLBC_RESET        0x80000000
+
+#define TIBC_CI           0x80000000 /* count inhibit */
+#define TICC_TOG          0x80000000 /* toggles when decrement to zero */
+
 #define IDR_EP_SHIFT      31
 #define IDR_EP_MASK       (1 << IDR_EP_SHIFT)
 #define IDR_CI0_SHIFT     30
@@ -189,11 +194,15 @@ typedef struct IRQ_src_t {
 #define IPVP_SENSE_SHIFT      22
 #define IPVP_SENSE_MASK       (1 << IPVP_SENSE_SHIFT)
 
-#define IPVP_PRIORITY_MASK     (0x1F << 16)
+#define IPVP_PRIORITY_MASK     (0xF << 16)
 #define IPVP_PRIORITY(_ipvpr_) ((int)(((_ipvpr_) & IPVP_PRIORITY_MASK) >> 16))
 #define IPVP_VECTOR_MASK       ((1 << VECTOR_BITS) - 1)
 #define IPVP_VECTOR(_ipvpr_)   ((_ipvpr_) & IPVP_VECTOR_MASK)
 
+/* IDE[EP/CI] are only for FSL MPIC prior to v4.0 */
+#define IDE_EP      0x80000000  /* external pin */
+#define IDE_CI      0x40000000  /* critical interrupt */
+
 typedef struct IRQ_dst_t {
     uint32_t pctp; /* CPU current task priority */
     uint32_t pcsr; /* CPU sensitivity register */
@@ -364,7 +373,7 @@ static void openpic_update_irq(OpenPICState *opp, int n_IRQ)
         DPRINTF("%s: IRQ %d is already active\n", __func__, n_IRQ);
         return;
     }
-    if (src->ide == 0x00000000) {
+    if (src->ide == 0) {
         /* No target */
         DPRINTF("%s: IRQ %d has no target\n", __func__, n_IRQ);
         return;
@@ -421,13 +430,13 @@ static void openpic_reset(DeviceState *d)
     OpenPICState *opp = FROM_SYSBUS(typeof (*opp), sysbus_from_qdev(d));
     int i;
 
-    opp->glbc = 0x80000000;
+    opp->glbc = GLBC_RESET;
     /* Initialise controller registers */
     opp->frep = ((opp->nb_irqs -1) << FREP_NIRQ_SHIFT) |
                 ((opp->nb_cpus -1) << FREP_NCPU_SHIFT) |
                 (opp->vid << FREP_VID_SHIFT);
 
-    opp->pint = 0x00000000;
+    opp->pint = 0;
     opp->spve = -1 & opp->spve_mask;
     opp->tifr = opp->tifr_reset;
     /* Initialise IRQ sources */
@@ -437,7 +446,7 @@ static void openpic_reset(DeviceState *d)
     }
     /* Initialise IRQ destinations */
     for (i = 0; i < MAX_CPU; i++) {
-        opp->dst[i].pctp      = 0x0000000F;
+        opp->dst[i].pctp      = 15;
         opp->dst[i].pcsr      = 0x00000000;
         memset(&opp->dst[i].raised, 0, sizeof(IRQ_queue_t));
         opp->dst[i].raised.next = -1;
@@ -446,11 +455,11 @@ static void openpic_reset(DeviceState *d)
     }
     /* Initialise timers */
     for (i = 0; i < MAX_TMR; i++) {
-        opp->timers[i].ticc = 0x00000000;
-        opp->timers[i].tibc = 0x80000000;
+        opp->timers[i].ticc = 0;
+        opp->timers[i].tibc = TIBC_CI;
     }
     /* Go out of RESET state */
-    opp->glbc = 0x00000000;
+    opp->glbc = 0;
 }
 
 static inline uint32_t read_IRQreg_ide(OpenPICState *opp, int n_IRQ)
@@ -467,7 +476,7 @@ static inline void write_IRQreg_ide(OpenPICState *opp, int n_IRQ, uint32_t val)
 {
     uint32_t tmp;
 
-    tmp = val & 0xC0000000;
+    tmp = val & (IDE_EP | IDE_CI);
     tmp |= val & ((1ULL << MAX_CPU) - 1);
     opp->src[n_IRQ].ide = tmp;
     DPRINTF("Set IDE %d to 0x%08x\n", n_IRQ, opp->src[n_IRQ].ide);
@@ -477,8 +486,8 @@ static inline void write_IRQreg_ipvp(OpenPICState *opp, int n_IRQ, uint32_t val)
 {
     /* NOTE: not fully accurate for special IRQs, but simple and sufficient */
     /* ACTIVITY bit is read-only */
-    opp->src[n_IRQ].ipvp = (opp->src[n_IRQ].ipvp & 0x40000000)
-                         | (val & 0x800F00FF);
+    opp->src[n_IRQ].ipvp = (opp->src[n_IRQ].ipvp & IPVP_ACTIVITY_MASK) |
+        (val & (IPVP_MASK_MASK | IPVP_PRIORITY_MASK | IPVP_VECTOR_MASK));
     openpic_update_irq(opp, n_IRQ);
     DPRINTF("Set IPVP %d to 0x%08x -> 0x%08x\n", n_IRQ, val,
             opp->src[n_IRQ].ipvp);
@@ -510,7 +519,7 @@ static void openpic_gbl_write(void *opaque, hwaddr addr, uint64_t val,
     case 0x1000: /* FREP */
         break;
     case 0x1020: /* GLBC */
-        if (val & 0x80000000) {
+        if (val & GLBC_RESET) {
             openpic_reset(&opp->busdev.qdev);
         }
         break;
@@ -623,10 +632,11 @@ static void openpic_tmr_write(void *opaque, hwaddr addr, uint64_t val,
     case 0x00: /* TICC (GTCCR) */
         break;
     case 0x10: /* TIBC (GTBCR) */
-        if ((opp->timers[idx].ticc & 0x80000000) != 0 &&
-            (val & 0x80000000) == 0 &&
-            (opp->timers[idx].tibc & 0x80000000) != 0)
-            opp->timers[idx].ticc &= ~0x80000000;
+        if ((opp->timers[idx].ticc & TICC_TOG) != 0 &&
+            (val & TIBC_CI) == 0 &&
+            (opp->timers[idx].tibc & TIBC_CI) != 0) {
+            opp->timers[idx].ticc &= ~TICC_TOG;
+        }
         opp->timers[idx].tibc = val;
         break;
     case 0x20: /* TIVP (GTIVPR) */
@@ -1179,9 +1189,9 @@ static int openpic_init(SysBusDevice *dev)
         opp->vid = VID_REVISION_1_2;
         opp->veni = VENI_GENERIC;
         opp->spve_mask = 0xFFFF;
-        opp->tifr_reset = 0x00000000;
-        opp->ipvp_reset = 0x80000000;
-        opp->ide_reset = 0x00000001;
+        opp->tifr_reset = 0;
+        opp->ipvp_reset = IPVP_MASK_MASK;
+        opp->ide_reset = 1 << 0;
         opp->max_irq = FSL_MPIC_20_MAX_IRQ;
         opp->irq_ipi0 = FSL_MPIC_20_IPI_IRQ;
         opp->irq_tim0 = FSL_MPIC_20_TMR_IRQ;
@@ -1195,9 +1205,9 @@ static int openpic_init(SysBusDevice *dev)
         opp->vid = VID_REVISION_1_3;
         opp->veni = VENI_GENERIC;
         opp->spve_mask = 0xFF;
-        opp->tifr_reset = 0x003F7A00;
-        opp->ipvp_reset = 0xA0000000;
-        opp->ide_reset = 0x00000000;
+        opp->tifr_reset = 4160000;
+        opp->ipvp_reset = IPVP_MASK_MASK | IPVP_MODE_MASK;
+        opp->ide_reset = 0;
         opp->max_irq = RAVEN_MAX_IRQ;
         opp->irq_ipi0 = RAVEN_IPI_IRQ;
         opp->irq_tim0 = RAVEN_TMR_IRQ;
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/6] openpic: remove pcsr (CPU sensitivity register)

[Scott Wood]

I could not find this register in any spec (FSL, IBM, or OpenPIC)
and the code doesn't do anything with it but initialize, save,
or restore it.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 hw/openpic.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/openpic.c b/hw/openpic.c
index 7e72c29..57218f3 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -205,7 +205,6 @@ typedef struct IRQ_src_t {
 
 typedef struct IRQ_dst_t {
     uint32_t pctp; /* CPU current task priority */
-    uint32_t pcsr; /* CPU sensitivity register */
     IRQ_queue_t raised;
     IRQ_queue_t servicing;
     qemu_irq *irqs;
@@ -447,7 +446,6 @@ static void openpic_reset(DeviceState *d)
     /* Initialise IRQ destinations */
     for (i = 0; i < MAX_CPU; i++) {
         opp->dst[i].pctp      = 15;
-        opp->dst[i].pcsr      = 0x00000000;
         memset(&opp->dst[i].raised, 0, sizeof(IRQ_queue_t));
         opp->dst[i].raised.next = -1;
         memset(&opp->dst[i].servicing, 0, sizeof(IRQ_queue_t));
@@ -1072,7 +1070,6 @@ static void openpic_save(QEMUFile* f, void *opaque)
 
     for (i = 0; i < opp->nb_cpus; i++) {
         qemu_put_be32s(f, &opp->dst[i].pctp);
-        qemu_put_be32s(f, &opp->dst[i].pcsr);
         openpic_save_IRQ_queue(f, &opp->dst[i].raised);
         openpic_save_IRQ_queue(f, &opp->dst[i].servicing);
     }
@@ -1119,7 +1116,6 @@ static int openpic_load(QEMUFile* f, void *opaque, int version_id)
 
     for (i = 0; i < opp->nb_cpus; i++) {
         qemu_get_be32s(f, &opp->dst[i].pctp);
-        qemu_get_be32s(f, &opp->dst[i].pcsr);
         openpic_load_IRQ_queue(f, &opp->dst[i].raised);
         openpic_load_IRQ_queue(f, &opp->dst[i].servicing);
     }
-- 
1.7.9.5

[Qemu-devel] [PATCH 3/6] openpic: support large vectors on FSL mpic

[Scott Wood]

Previously only the spurious vector was sized appropriately
to the openpic model.

Also, instances of "IPVP_VECTOR(opp->spve)" were replace with
just "opp->spve", as opp->spve is already just a vector and not
an IVPR.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 hw/openpic.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/hw/openpic.c b/hw/openpic.c
index 57218f3..8c3f04d 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -51,7 +51,6 @@
 #define MAX_CPU     15
 #define MAX_SRC     256
 #define MAX_TMR     4
-#define VECTOR_BITS 8
 #define MAX_IPI     4
 #define MAX_MSI     8
 #define MAX_IRQ     (MAX_SRC + MAX_IPI + MAX_TMR)
@@ -196,8 +195,7 @@ typedef struct IRQ_src_t {
 
 #define IPVP_PRIORITY_MASK     (0xF << 16)
 #define IPVP_PRIORITY(_ipvpr_) ((int)(((_ipvpr_) & IPVP_PRIORITY_MASK) >> 16))
-#define IPVP_VECTOR_MASK       ((1 << VECTOR_BITS) - 1)
-#define IPVP_VECTOR(_ipvpr_)   ((_ipvpr_) & IPVP_VECTOR_MASK)
+#define IPVP_VECTOR(opp, _ipvpr_) ((_ipvpr_) & (opp)->vector_mask)
 
 /* IDE[EP/CI] are only for FSL MPIC prior to v4.0 */
 #define IDE_EP      0x80000000  /* external pin */
@@ -220,7 +218,7 @@ typedef struct OpenPICState {
     uint32_t nb_irqs;
     uint32_t vid;
     uint32_t veni; /* Vendor identification register */
-    uint32_t spve_mask;
+    uint32_t vector_mask;
     uint32_t tifr_reset;
     uint32_t ipvp_reset;
     uint32_t ide_reset;
@@ -436,7 +434,7 @@ static void openpic_reset(DeviceState *d)
                 (opp->vid << FREP_VID_SHIFT);
 
     opp->pint = 0;
-    opp->spve = -1 & opp->spve_mask;
+    opp->spve = -1 & opp->vector_mask;
     opp->tifr = opp->tifr_reset;
     /* Initialise IRQ sources */
     for (i = 0; i < opp->max_irq; i++) {
@@ -485,7 +483,7 @@ static inline void write_IRQreg_ipvp(OpenPICState *opp, int n_IRQ, uint32_t val)
     /* NOTE: not fully accurate for special IRQs, but simple and sufficient */
     /* ACTIVITY bit is read-only */
     opp->src[n_IRQ].ipvp = (opp->src[n_IRQ].ipvp & IPVP_ACTIVITY_MASK) |
-        (val & (IPVP_MASK_MASK | IPVP_PRIORITY_MASK | IPVP_VECTOR_MASK));
+        (val & (IPVP_MASK_MASK | IPVP_PRIORITY_MASK | opp->vector_mask));
     openpic_update_irq(opp, n_IRQ);
     DPRINTF("Set IPVP %d to 0x%08x -> 0x%08x\n", n_IRQ, val,
             opp->src[n_IRQ].ipvp);
@@ -548,7 +546,7 @@ static void openpic_gbl_write(void *opaque, hwaddr addr, uint64_t val,
         }
         break;
     case 0x10E0: /* SPVE */
-        opp->spve = val & opp->spve_mask;
+        opp->spve = val & opp->vector_mask;
         break;
     default:
         break;
@@ -885,7 +883,7 @@ static uint32_t openpic_cpu_read_internal(void *opaque, hwaddr addr,
         DPRINTF("PIAC: irq=%d\n", n_IRQ);
         if (n_IRQ == -1) {
             /* No more interrupt pending */
-            retval = IPVP_VECTOR(opp->spve);
+            retval = opp->spve;
         } else {
             src = &opp->src[n_IRQ];
             if (!(src->ipvp & IPVP_ACTIVITY_MASK) ||
@@ -895,11 +893,11 @@ static uint32_t openpic_cpu_read_internal(void *opaque, hwaddr addr,
                  *   and the pending IRQ isn't allowed anymore
                  */
                 src->ipvp &= ~IPVP_ACTIVITY_MASK;
-                retval = IPVP_VECTOR(opp->spve);
+                retval = opp->spve;
             } else {
                 /* IRQ enter servicing state */
                 IRQ_setbit(&dst->servicing, n_IRQ);
-                retval = IPVP_VECTOR(src->ipvp);
+                retval = IPVP_VECTOR(opp, src->ipvp);
             }
             IRQ_resetbit(&dst->raised, n_IRQ);
             dst->raised.next = -1;
@@ -1184,7 +1182,7 @@ static int openpic_init(SysBusDevice *dev)
         opp->nb_irqs = 80;
         opp->vid = VID_REVISION_1_2;
         opp->veni = VENI_GENERIC;
-        opp->spve_mask = 0xFFFF;
+        opp->vector_mask = 0xFFFF;
         opp->tifr_reset = 0;
         opp->ipvp_reset = IPVP_MASK_MASK;
         opp->ide_reset = 1 << 0;
@@ -1200,7 +1198,7 @@ static int openpic_init(SysBusDevice *dev)
         opp->nb_irqs = RAVEN_MAX_EXT;
         opp->vid = VID_REVISION_1_3;
         opp->veni = VENI_GENERIC;
-        opp->spve_mask = 0xFF;
+        opp->vector_mask = 0xFF;
         opp->tifr_reset = 4160000;
         opp->ipvp_reset = IPVP_MASK_MASK | IPVP_MODE_MASK;
         opp->ide_reset = 0;
-- 
1.7.9.5

[Qemu-devel] [PATCH 4/6] openpic: don't crash on a register access without a CPU context

[Scott Wood]

If we access a register via the QEMU memory inspection commands (e.g.
"xp") rather than from guest code, we won't have a CPU context.
Gracefully fail to access the register in that case, rather than
crashing.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 hw/openpic.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/openpic.c b/hw/openpic.c
index 8c3f04d..c57a168 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -161,7 +161,11 @@ static inline int test_bit(uint32_t *field, int bit)
 
 static int get_current_cpu(void)
 {
-  return cpu_single_env->cpu_index;
+    if (!cpu_single_env) {
+        return -1;
+    }
+
+    return cpu_single_env->cpu_index;
 }
 
 static uint32_t openpic_cpu_read_internal(void *opaque, hwaddr addr,
@@ -797,6 +801,11 @@ static void openpic_cpu_write_internal(void *opaque, hwaddr addr,
 
     DPRINTF("%s: cpu %d addr " TARGET_FMT_plx " <= %08x\n", __func__, idx,
             addr, val);
+
+    if (idx < 0) {
+        return;
+    }
+
     if (addr & 0xF)
         return;
     dst = &opp->dst[idx];
@@ -862,6 +871,11 @@ static uint32_t openpic_cpu_read_internal(void *opaque, hwaddr addr,
 
     DPRINTF("%s: cpu %d addr " TARGET_FMT_plx "\n", __func__, idx, addr);
     retval = 0xFFFFFFFF;
+
+    if (idx < 0) {
+        return retval;
+    }
+
     if (addr & 0xF)
         return retval;
     dst = &opp->dst[idx];
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 4/6] openpic: don't crash on a register access without a CPU context

[Alexander Graf]

On 14.12.2012, at 03:12, Scott Wood wrote:

> If we access a register via the QEMU memory inspection commands (e.g.
> "xp") rather than from guest code, we won't have a CPU context.
> Gracefully fail to access the register in that case, rather than
> crashing.

Can't we set cpu_single_env in the debug memory access case? I'm not sure this is the only device with that problem, and by always having cpu_single_env available we would completely get rid of the whole bug category.


Alex

> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> hw/openpic.c |   16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/openpic.c b/hw/openpic.c
> index 8c3f04d..c57a168 100644
> --- a/hw/openpic.c
> +++ b/hw/openpic.c
> @@ -161,7 +161,11 @@ static inline int test_bit(uint32_t *field, int bit)
> 
> static int get_current_cpu(void)
> {
> -  return cpu_single_env->cpu_index;
> +    if (!cpu_single_env) {
> +        return -1;
> +    }
> +
> +    return cpu_single_env->cpu_index;
> }
> 
> static uint32_t openpic_cpu_read_internal(void *opaque, hwaddr addr,
> @@ -797,6 +801,11 @@ static void openpic_cpu_write_internal(void *opaque, hwaddr addr,
> 
>     DPRINTF("%s: cpu %d addr " TARGET_FMT_plx " <= %08x\n", __func__, idx,
>             addr, val);
> +
> +    if (idx < 0) {
> +        return;
> +    }
> +
>     if (addr & 0xF)
>         return;
>     dst = &opp->dst[idx];
> @@ -862,6 +871,11 @@ static uint32_t openpic_cpu_read_internal(void *opaque, hwaddr addr,
> 
>     DPRINTF("%s: cpu %d addr " TARGET_FMT_plx "\n", __func__, idx, addr);
>     retval = 0xFFFFFFFF;
> +
> +    if (idx < 0) {
> +        return retval;
> +    }
> +
>     if (addr & 0xF)
>         return retval;
>     dst = &opp->dst[idx];
> -- 
> 1.7.9.5
> 
> 

Re: [Qemu-devel] [PATCH 4/6] openpic: don't crash on a register access without a CPU context

[Scott Wood]

On 12/14/2012 06:35:12 AM, Alexander Graf wrote:
> 
> On 14.12.2012, at 03:12, Scott Wood wrote:
> 
> > If we access a register via the QEMU memory inspection commands  
> (e.g.
> > "xp") rather than from guest code, we won't have a CPU context.
> > Gracefully fail to access the register in that case, rather than
> > crashing.
> 
> Can't we set cpu_single_env in the debug memory access case? I'm not  
> sure this is the only device with that problem, and by always having  
> cpu_single_env available we would completely get rid of the whole bug  
> category.

Which CPU would you pick?

-Scott

Re: [Qemu-devel] [PATCH 4/6] openpic: don't crash on a register access without a CPU context

[Alexander Graf]

On 14.12.2012, at 19:18, Scott Wood wrote:

> On 12/14/2012 06:35:12 AM, Alexander Graf wrote:
>> On 14.12.2012, at 03:12, Scott Wood wrote:
>> > If we access a register via the QEMU memory inspection commands (e.g.
>> > "xp") rather than from guest code, we won't have a CPU context.
>> > Gracefully fail to access the register in that case, rather than
>> > crashing.
>> Can't we set cpu_single_env in the debug memory access case? I'm not sure this is the only device with that problem, and by always having cpu_single_env available we would completely get rid of the whole bug category.
> 
> Which CPU would you pick?

The boot CPU.


Alex

Re: [Qemu-devel] [PATCH 4/6] openpic: don't crash on a register access without a CPU context

[Scott Wood]

On 12/14/2012 06:35:12 AM, Alexander Graf wrote:
> 
> On 14.12.2012, at 03:12, Scott Wood wrote:
> 
> > If we access a register via the QEMU memory inspection commands  
> (e.g.
> > "xp") rather than from guest code, we won't have a CPU context.
> > Gracefully fail to access the register in that case, rather than
> > crashing.
> 
> Can't we set cpu_single_env in the debug memory access case? I'm not  
> sure this is the only device with that problem, and by always having  
> cpu_single_env available we would completely get rid of the whole bug  
> category.

So, how would we go about doing this?  cpu_single_env is declared as  
thread-local storage.  Even if there's some way to deliberately inspect  
a different thread's local storage, I don't see how you'd get it to  
work automatically without changing all those drivers (and are there  
really that many that care about the CPU?) -- and we might break other  
places that already check whether cpu_single_env is NULL.

FWIW, hw/pc.c does pretty much the same thing as this patch in  
cpu_get_current_apic().

-Scott

Re: [Qemu-devel] [PATCH 4/6] openpic: don't crash on a register access without a CPU context

[Alexander Graf]

On 14.12.2012, at 22:42, Scott Wood wrote:

> On 12/14/2012 06:35:12 AM, Alexander Graf wrote:
>> On 14.12.2012, at 03:12, Scott Wood wrote:
>> > If we access a register via the QEMU memory inspection commands (e.g.
>> > "xp") rather than from guest code, we won't have a CPU context.
>> > Gracefully fail to access the register in that case, rather than
>> > crashing.
>> Can't we set cpu_single_env in the debug memory access case? I'm not sure this is the only device with that problem, and by always having cpu_single_env available we would completely get rid of the whole bug category.
> 
> So, how would we go about doing this?  cpu_single_env is declared as thread-local storage.  Even if there's some way to deliberately inspect a different thread's local storage, I don't see how you'd get it to work automatically without changing all those drivers (and are there really that many that care about the CPU?) -- and we might break other places that already check whether cpu_single_env is NULL.
> 
> FWIW, hw/pc.c does pretty much the same thing as this patch in cpu_get_current_apic().

Ok, convinced :). Patch applied to ppc-next. Thanks ;)


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/6] openpic: don't crash on a register access without a CPU context

[Blue Swirl]

On Fri, Dec 14, 2012 at 9:58 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 14.12.2012, at 22:42, Scott Wood wrote:
>
>> On 12/14/2012 06:35:12 AM, Alexander Graf wrote:
>>> On 14.12.2012, at 03:12, Scott Wood wrote:
>>> > If we access a register via the QEMU memory inspection commands (e.g.
>>> > "xp") rather than from guest code, we won't have a CPU context.
>>> > Gracefully fail to access the register in that case, rather than
>>> > crashing.
>>> Can't we set cpu_single_env in the debug memory access case? I'm not sure this is the only device with that problem, and by always having cpu_single_env available we would completely get rid of the whole bug category.
>>
>> So, how would we go about doing this?  cpu_single_env is declared as thread-local storage.  Even if there's some way to deliberately inspect a different thread's local storage, I don't see how you'd get it to work automatically without changing all those drivers (and are there really that many that care about the CPU?) -- and we might break other places that already check whether cpu_single_env is NULL.
>>
>> FWIW, hw/pc.c does pretty much the same thing as this patch in cpu_get_current_apic().
>
> Ok, convinced :). Patch applied to ppc-next. Thanks ;)

The long term fix for both openpic and LAPIC is to introduce a CPU
specific address space and then instantiate a device for each CPU. The
memory inspection commands probably need to specify the CPU.

>
>
> Alex
>
>

[Qemu-devel] [PATCH 5/6] openpic: BRR1 is not a CPU-specific register.

[Scott Wood]

It's in the address range that normally contains a magic redirection
to the CPU-specific region of the curretn CPU, but it isn't actually
a per-CPU register.  On real hardware BRR1 shows up only at 0x40000,
not at 0x60000 or other non-magic per-CPU areas.  Plus, this makes
it possible to read the register on the QEMU command line with "xp".

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 hw/openpic.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/openpic.c b/hw/openpic.c
index c57a168..c0c4307 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -580,6 +580,8 @@ static uint64_t openpic_gbl_read(void *opaque, hwaddr addr, unsigned len)
         retval = 0x00000000;
         break;
     case 0x00: /* Block Revision Register1 (BRR1) */
+        retval = opp->brr1;
+        break;
     case 0x40:
     case 0x50:
     case 0x60:
@@ -881,9 +883,6 @@ static uint32_t openpic_cpu_read_internal(void *opaque, hwaddr addr,
     dst = &opp->dst[idx];
     addr &= 0xFF0;
     switch (addr) {
-    case 0x00: /* Block Revision Register1 (BRR1) */
-        retval = opp->brr1;
-        break;
     case 0x80: /* PCTP */
         retval = dst->pctp;
         break;
-- 
1.7.9.5

[Qemu-devel] [PATCH 6/6] openpic: s/opp->nb_irqs -1/opp->nb_cpus - 1/

[Scott Wood]

"opp->nb_irqs-1" would have been a minor coding style error,
but putting in one space but not the other makes it look
confusingly like a numeric literal "-1".

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 hw/openpic.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/openpic.c b/hw/openpic.c
index c0c4307..debd77d 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -433,8 +433,8 @@ static void openpic_reset(DeviceState *d)
 
     opp->glbc = GLBC_RESET;
     /* Initialise controller registers */
-    opp->frep = ((opp->nb_irqs -1) << FREP_NIRQ_SHIFT) |
-                ((opp->nb_cpus -1) << FREP_NCPU_SHIFT) |
+    opp->frep = ((opp->nb_irqs - 1) << FREP_NIRQ_SHIFT) |
+                ((opp->nb_cpus - 1) << FREP_NCPU_SHIFT) |
                 (opp->vid << FREP_VID_SHIFT);
 
     opp->pint = 0;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 0/6] openpic: first batch of cleanups and minor fixes

[Alexander Graf]

On 14.12.2012, at 03:11, Scott Wood wrote:

> There'll be more to come, but here's an initial set of cleanups
> and fixes for openpic.

Thanks, applied all except for 4/6 to ppc-next.


Alex

> 
> This is based on the ppc-mpic-cleanup branch in
> git://repo.or.cz/qemu/agraf.git
> 
> Scott Wood (6):
>  openpic: symbolicize some magic numbers
>  openpic: remove pcsr (CPU sensitivity register)
>  openpic: support large vectors on FSL mpic
>  openpic: don't crash on a register access without a CPU context
>  openpic: BRR1 is not a CPU-specific register.
>  openpic: s/opp->nb_irqs -1/opp->nb_cpus - 1/
> 
> hw/openpic.c |  102 +++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 59 insertions(+), 43 deletions(-)
> 
> -- 
> 1.7.9.5
> 
>