| Message ID | 1355451124-2559-5-git-send-email-scottwood@freescale.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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
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
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
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 > >
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];
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(-)