Patchwork [4/6] openpic: don't crash on a register access without a CPU context

login
register
mail settings
Submitter Scott Wood
Date Dec. 14, 2012, 2:12 a.m.
Message ID <1355451124-2559-5-git-send-email-scottwood@freescale.com>
Download mbox | patch
Permalink /patch/206272/
State New
Headers show

Comments

Scott Wood - Dec. 14, 2012, 2:12 a.m.
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(-)
Alexander Graf - Dec. 14, 2012, 12:35 p.m.
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
> 
>
Scott Wood - Dec. 14, 2012, 6:18 p.m.
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
Alexander Graf - Dec. 14, 2012, 6:20 p.m.
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
Scott Wood - Dec. 14, 2012, 9:42 p.m.
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
Alexander Graf - Dec. 14, 2012, 9:58 p.m.
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
Blue Swirl - Dec. 15, 2012, 8:48 a.m.
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
>
>

Patch

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