This will allow a breakpoint hack to move out of AVR's translator.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/hw/core/cpu.h | 4 ++++
cpu.c | 10 ++++++++++
2 files changed, 14 insertions(+)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 4e0ea68efc..bc864564ce 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -103,6 +103,9 @@ struct SysemuCPUOps;
* also implement the synchronize_from_tb hook.
* @gdb_read_register: Callback for letting GDB read a register.
* @gdb_write_register: Callback for letting GDB write a register.
+ * @gdb_adjust_breakpoint: Callback for adjusting the address of a
+ * breakpoint. Used by AVR to handle a gdb mis-feature with
+ * its Harvard architecture split code and data.
* @gdb_num_core_regs: Number of core registers accessible to GDB.
* @gdb_core_xml_file: File name for core registers GDB XML description.
* @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop
@@ -137,6 +140,7 @@ struct CPUClass {
void (*set_pc)(CPUState *cpu, vaddr value);
int (*gdb_read_register)(CPUState *cpu, GByteArray *buf, int reg);
int (*gdb_write_register)(CPUState *cpu, uint8_t *buf, int reg);
+ vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr);
const char *gdb_core_xml_file;
gchar * (*gdb_arch_name)(CPUState *cpu);
diff --git a/cpu.c b/cpu.c
index 83059537d7..91d9e38acb 100644
--- a/cpu.c
+++ b/cpu.c
@@ -267,8 +267,13 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
CPUBreakpoint **breakpoint)
{
+ CPUClass *cc = CPU_GET_CLASS(cpu);
CPUBreakpoint *bp;
+ if (cc->gdb_adjust_breakpoint) {
+ pc = cc->gdb_adjust_breakpoint(cpu, pc);
+ }
+
bp = g_malloc(sizeof(*bp));
bp->pc = pc;
@@ -294,8 +299,13 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
/* Remove a specific breakpoint. */
int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
{
+ CPUClass *cc = CPU_GET_CLASS(cpu);
CPUBreakpoint *bp;
+ if (cc->gdb_adjust_breakpoint) {
+ pc = cc->gdb_adjust_breakpoint(cpu, pc);
+ }
+
QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
if (bp->pc == pc && bp->flags == flags) {
cpu_breakpoint_remove_by_ref(cpu, bp);
--
2.25.1
On Tue, 20 Jul 2021 at 20:54, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This will allow a breakpoint hack to move out of AVR's translator.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> diff --git a/cpu.c b/cpu.c
> index 83059537d7..91d9e38acb 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -267,8 +267,13 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
> int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
> CPUBreakpoint **breakpoint)
> {
> + CPUClass *cc = CPU_GET_CLASS(cpu);
> CPUBreakpoint *bp;
>
> + if (cc->gdb_adjust_breakpoint) {
> + pc = cc->gdb_adjust_breakpoint(cpu, pc);
> + }
> +
> bp = g_malloc(sizeof(*bp));
>
> bp->pc = pc;
> @@ -294,8 +299,13 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
> /* Remove a specific breakpoint. */
> int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
> {
> + CPUClass *cc = CPU_GET_CLASS(cpu);
> CPUBreakpoint *bp;
>
> + if (cc->gdb_adjust_breakpoint) {
> + pc = cc->gdb_adjust_breakpoint(cpu, pc);
> + }
> +
> QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
> if (bp->pc == pc && bp->flags == flags) {
> cpu_breakpoint_remove_by_ref(cpu, bp);
> --
So previously for AVR we would have considered the bp at 0x100
and the one at 0x800100 as distinct (in the sense that the only way
the gdb remote protocol distinguishes breakpoints is by "what address",
and these have different addresses). After this change, they won't
be distinct, because if you set a bp at 0x100 and 0x800100 and then
try to remove the one at 0x100 we might remove the 0x800100 one,
because we're storing only the adjusted-address, not the one gdb used.
This might not matter in practice...
-- PMM
On 7/20/21 10:56 AM, Peter Maydell wrote:
> On Tue, 20 Jul 2021 at 20:54, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> This will allow a breakpoint hack to move out of AVR's translator.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
>> diff --git a/cpu.c b/cpu.c
>> index 83059537d7..91d9e38acb 100644
>> --- a/cpu.c
>> +++ b/cpu.c
>> @@ -267,8 +267,13 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
>> int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>> CPUBreakpoint **breakpoint)
>> {
>> + CPUClass *cc = CPU_GET_CLASS(cpu);
>> CPUBreakpoint *bp;
>>
>> + if (cc->gdb_adjust_breakpoint) {
>> + pc = cc->gdb_adjust_breakpoint(cpu, pc);
>> + }
>> +
>> bp = g_malloc(sizeof(*bp));
>>
>> bp->pc = pc;
>> @@ -294,8 +299,13 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>> /* Remove a specific breakpoint. */
>> int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
>> {
>> + CPUClass *cc = CPU_GET_CLASS(cpu);
>> CPUBreakpoint *bp;
>>
>> + if (cc->gdb_adjust_breakpoint) {
>> + pc = cc->gdb_adjust_breakpoint(cpu, pc);
>> + }
>> +
>> QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>> if (bp->pc == pc && bp->flags == flags) {
>> cpu_breakpoint_remove_by_ref(cpu, bp);
>> --
>
> So previously for AVR we would have considered the bp at 0x100
> and the one at 0x800100 as distinct (in the sense that the only way
> the gdb remote protocol distinguishes breakpoints is by "what address",
> and these have different addresses). After this change, they won't
> be distinct, because if you set a bp at 0x100 and 0x800100 and then
> try to remove the one at 0x100 we might remove the 0x800100 one,
> because we're storing only the adjusted-address, not the one gdb used.
>
> This might not matter in practice...
I don't think it will matter.
Currently, if it sets both 0x100 and 0x800100, then we'll record two breakpoints, and with
either we'll raise EXCP_DEBUG when pc == 0x100.
Afterward, we'll have two CPUBreakpoint structures that both contain 0x100, and when pc ==
0x100 we'll raise EXCP_DEBUG. If gdb removes the breakpoint at 0x800100, we'll remove one
of the two CPUBreakpoint. But we'll still stop at 0x100, as expected. When it removes
the breakpoint at 0x100, both CPUBreakpoint structures will be gone.
In principal, gdb could now add a breakpoint at 0x800100 and remove it with 0x100, where
it could not before. But I don't expect that to happen. If we reported any kind of
status to gdb re the breakpoint insertion or removal (e.g. bp not found), then it might
matter, but we don't.
Practically, this is working around what I'd call a gdb bug wrt avr. Which may even have
been fixed -- I haven't looked.
r~
On 7/20/21 11:08 PM, Richard Henderson wrote:
> On 7/20/21 10:56 AM, Peter Maydell wrote:
>> On Tue, 20 Jul 2021 at 20:54, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> This will allow a breakpoint hack to move out of AVR's translator.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>
>>> diff --git a/cpu.c b/cpu.c
>>> index 83059537d7..91d9e38acb 100644
>>> --- a/cpu.c
>>> +++ b/cpu.c
>>> @@ -267,8 +267,13 @@ static void breakpoint_invalidate(CPUState *cpu,
>>> target_ulong pc)
>>> int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>>> CPUBreakpoint **breakpoint)
>>> {
>>> + CPUClass *cc = CPU_GET_CLASS(cpu);
>>> CPUBreakpoint *bp;
>>>
>>> + if (cc->gdb_adjust_breakpoint) {
>>> + pc = cc->gdb_adjust_breakpoint(cpu, pc);
>>> + }
>>> +
>>> bp = g_malloc(sizeof(*bp));
>>>
>>> bp->pc = pc;
>>> @@ -294,8 +299,13 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr
>>> pc, int flags,
>>> /* Remove a specific breakpoint. */
>>> int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
>>> {
>>> + CPUClass *cc = CPU_GET_CLASS(cpu);
>>> CPUBreakpoint *bp;
>>>
>>> + if (cc->gdb_adjust_breakpoint) {
>>> + pc = cc->gdb_adjust_breakpoint(cpu, pc);
>>> + }
>>> +
>>> QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>>> if (bp->pc == pc && bp->flags == flags) {
>>> cpu_breakpoint_remove_by_ref(cpu, bp);
>>> --
>>
>> So previously for AVR we would have considered the bp at 0x100
>> and the one at 0x800100 as distinct (in the sense that the only way
>> the gdb remote protocol distinguishes breakpoints is by "what address",
>> and these have different addresses). After this change, they won't
>> be distinct, because if you set a bp at 0x100 and 0x800100 and then
>> try to remove the one at 0x100 we might remove the 0x800100 one,
>> because we're storing only the adjusted-address, not the one gdb used.
>>
>> This might not matter in practice...
>
> I don't think it will matter.
>
> Currently, if it sets both 0x100 and 0x800100, then we'll record two
> breakpoints, and with either we'll raise EXCP_DEBUG when pc == 0x100.
>
> Afterward, we'll have two CPUBreakpoint structures that both contain
> 0x100, and when pc == 0x100 we'll raise EXCP_DEBUG. If gdb removes the
> breakpoint at 0x800100, we'll remove one of the two CPUBreakpoint. But
> we'll still stop at 0x100, as expected. When it removes the breakpoint
> at 0x100, both CPUBreakpoint structures will be gone.
>
> In principal, gdb could now add a breakpoint at 0x800100 and remove it
> with 0x100, where it could not before. But I don't expect that to
> happen. If we reported any kind of status to gdb re the breakpoint
> insertion or removal (e.g. bp not found), then it might matter, but we
> don't.
>
> Practically, this is working around what I'd call a gdb bug wrt avr.
> Which may even have been fixed -- I haven't looked.
This is not a bug but a feature to deal with the Harvard architecture.
QEMU AVR model is based on GCC sources so uses the same "feature".
The AVR core has 2 address spaces: "CODE" and "DATA". An address space
is always zero-based (so both are). To avoid having to deal with
relocation of symbols from different AS but having same address, the
DATA space is mapped at 0x800000 (bit 23 is "virtual" as inexistant
- masked - from the CODE AS).
The core can not execute from DATA, so CPUBreakpoint can only be
triggered from CODE.
I once implemented different AS but switched to smth else :/
It was working but for some reason I couldn't remove the
OFFSET_DATA / OFFSET_CODE definitions, I don't remember &
should respin... See
https://gitlab.com/philmd/qemu/-/compare/avr_gsoc_v1a...avr_gsoc_v1b
Extract of the patches to show the idea:
diff --git a/target/avr/cpu.h b/target/avr/cpu.h
+/* Indexes used when registering address spaces with
cpu_address_space_init */
+typedef enum AVRASIdx {
+ AVRASIdx_CODE = 0,
+ AVRASIdx_DATA = 1,
+} AVRASIdx;
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
@@ -96,6 +98,13 @@ static void avr_cpu_realizefn(DeviceState *dev, Error
**errp)
error_propagate(errp, local_err);
return;
}
+
+ cs->num_ases = 2;
+ cpu_address_space_init(cs, AVRASIdx_CODE, "cpu-program-bus",
+ get_program_memory());
+ cpu_address_space_init(cs, AVRASIdx_DATA, "cpu-data-bus",
+ get_data_memory());
+
qemu_init_vcpu(cs);
cpu_reset(cs);
diff --git a/target/avr/helper.c b/target/avr/helper.c
-/*
- * This function implements IN instruction
- *
- * It does the following
- * a. if an IO register belongs to CPU, its value is read and returned
- * b. otherwise io address is translated to mem address and physical
memory
- * is read.
- * c. it caches the value for sake of SBI, SBIC, SBIS & CBI implementation
- *
- */
-target_ulong helper_inb(CPUAVRState *env, uint32_t port)
+static uint8_t data_read(CPUAVRState *env, uint32_t addr)
{
- target_ulong data = 0;
+ CPUState *cs;
+ AddressSpace *as;
+ uint8_t data = 0;
- switch (port) {
+ switch (addr) {
+ case 0x00 ... 0x1f:
+ /* CPU registers */
+ data = env->r[addr];
+ break;
case 0x38: /* RAMPD */
- data = 0xff & (env->rampD >> 16);
+ /* FIXME check available feature? */
+ data = env->rampD >> 16;
break;
case 0x39: /* RAMPX */
- data = 0xff & (env->rampX >> 16);
+ data = env->rampX >> 16;
break;
case 0x3a: /* RAMPY */
- data = 0xff & (env->rampY >> 16);
+ data = env->rampY >> 16;
break;
case 0x3b: /* RAMPZ */
- data = 0xff & (env->rampZ >> 16);
+ data = env->rampZ >> 16;
break;
case 0x3c: /* EIND */
- data = 0xff & (env->eind >> 16);
+ data = env->eind >> 16;
break;
case 0x3d: /* SPL */
data = env->sp & 0x00ff;
@@ -232,12 +230,30 @@ target_ulong helper_inb(CPUAVRState *env, uint32_t
port)
break;
default:
/* not a special register, pass to normal memory access */
- cpu_physical_memory_read(OFFSET_IO_REGISTERS + port, &data, 1);
+ cs = env_cpu(env);
+ as = cpu_get_address_space(cs, AVRASIdx_DATA);
+ data = address_space_ldub(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
}
+ trace_avr_data_read(addr, data);
return data;
}
+/*
+ * This function implements IN instruction
+ *
+ * It does the following
+ * a. if an IO register belongs to CPU, its value is read and returned
+ * b. otherwise io address is translated to mem address and physical
memory
+ * is read.
+ * c. it caches the value for sake of SBI, SBIC, SBIS & CBI implementation
+ *
+ */
+target_ulong helper_inb(CPUAVRState *env, uint32_t port)
+{
+ return data_read(env, NUMBER_OF_CPU_REGISTERS + port);
+}
@@ -299,21 +315,9 @@ void helper_outb(CPUAVRState *env, uint32_t port,
uint32_t data)
*/
target_ulong helper_fullrd(CPUAVRState *env, uint32_t addr)
{
- uint8_t data;
-
env->fullacc = false;
- if (addr < NUMBER_OF_CPU_REGISTERS) {
- /* CPU registers */
- data = env->r[addr];
- } else if (addr < NUMBER_OF_CPU_REGISTERS + NUMBER_OF_IO_REGISTERS) {
- /* IO registers */
- data = helper_inb(env, addr - NUMBER_OF_CPU_REGISTERS);
- } else {
- /* memory */
- cpu_physical_memory_read(OFFSET_DATA + addr, &data, 1);
- }
- return data;
+ return data_read(env, addr);
}
On 7/20/21 11:53 PM, Philippe Mathieu-Daudé wrote:
> On 7/20/21 11:08 PM, Richard Henderson wrote:
>> On 7/20/21 10:56 AM, Peter Maydell wrote:
>>> On Tue, 20 Jul 2021 at 20:54, Richard Henderson
>>> <richard.henderson@linaro.org> wrote:
>>>>
>>>> This will allow a breakpoint hack to move out of AVR's translator.
>>>>
>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>
>>>> diff --git a/cpu.c b/cpu.c
>>>> index 83059537d7..91d9e38acb 100644
>>>> --- a/cpu.c
>>>> +++ b/cpu.c
>>>> @@ -267,8 +267,13 @@ static void breakpoint_invalidate(CPUState *cpu,
>>>> target_ulong pc)
>>>> int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>>>> CPUBreakpoint **breakpoint)
>>>> {
>>>> + CPUClass *cc = CPU_GET_CLASS(cpu);
>>>> CPUBreakpoint *bp;
>>>>
>>>> + if (cc->gdb_adjust_breakpoint) {
>>>> + pc = cc->gdb_adjust_breakpoint(cpu, pc);
>>>> + }
>>>> +
>>>> bp = g_malloc(sizeof(*bp));
>>>>
>>>> bp->pc = pc;
>>>> @@ -294,8 +299,13 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr
>>>> pc, int flags,
>>>> /* Remove a specific breakpoint. */
>>>> int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
>>>> {
>>>> + CPUClass *cc = CPU_GET_CLASS(cpu);
>>>> CPUBreakpoint *bp;
>>>>
>>>> + if (cc->gdb_adjust_breakpoint) {
>>>> + pc = cc->gdb_adjust_breakpoint(cpu, pc);
>>>> + }
>>>> +
>>>> QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>>>> if (bp->pc == pc && bp->flags == flags) {
>>>> cpu_breakpoint_remove_by_ref(cpu, bp);
>>>> --
>>>
>>> So previously for AVR we would have considered the bp at 0x100
>>> and the one at 0x800100 as distinct (in the sense that the only way
>>> the gdb remote protocol distinguishes breakpoints is by "what address",
>>> and these have different addresses). After this change, they won't
>>> be distinct, because if you set a bp at 0x100 and 0x800100 and then
>>> try to remove the one at 0x100 we might remove the 0x800100 one,
>>> because we're storing only the adjusted-address, not the one gdb used.
Yes. Looks good.
>>>
>>> This might not matter in practice...
>>
>> I don't think it will matter.
>>
>> Currently, if it sets both 0x100 and 0x800100, then we'll record two
>> breakpoints, and with either we'll raise EXCP_DEBUG when pc == 0x100.
>>
>> Afterward, we'll have two CPUBreakpoint structures that both contain
>> 0x100, and when pc == 0x100 we'll raise EXCP_DEBUG. If gdb removes the
>> breakpoint at 0x800100, we'll remove one of the two CPUBreakpoint. But
>> we'll still stop at 0x100, as expected. When it removes the breakpoint
>> at 0x100, both CPUBreakpoint structures will be gone.
>>
>> In principal, gdb could now add a breakpoint at 0x800100 and remove it
>> with 0x100, where it could not before. But I don't expect that to
>> happen. If we reported any kind of status to gdb re the breakpoint
>> insertion or removal (e.g. bp not found), then it might matter, but we
>> don't.
IIUC QEMU model is "hardware breakpoint". I don't know how gdb deals
if user add both soft/hard bp. Neither do I know how many soft/hard
bp are "annouced" via gdbstub monitor to gdb (Alex?). Amusingly
gdb-xml/avr-cpu.xml announces itself as a riscv cpu:
<feature name="org.gnu.gdb.riscv.cpu">
Maybe because there is no official XML declaration for AVR?
https://sourceware.org/gdb/current/onlinedocs/gdb/Standard-Target-Features.html#Standard-Target-Features
>> Practically, this is working around what I'd call a gdb bug wrt avr.
>> Which may even have been fixed -- I haven't looked.
I agree this won't matter much (and this patch makes it cleaner) so:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On 7/20/21 11:53 PM, Philippe Mathieu-Daudé wrote: >> On 7/20/21 11:08 PM, Richard Henderson wrote: >>> On 7/20/21 10:56 AM, Peter Maydell wrote: >>>> On Tue, 20 Jul 2021 at 20:54, Richard Henderson >>>> <richard.henderson@linaro.org> wrote: >>>>> >>>>> This will allow a breakpoint hack to move out of AVR's translator. >>>>> >>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>>> <snip> >>>> So previously for AVR we would have considered the bp at 0x100 >>>> and the one at 0x800100 as distinct (in the sense that the only way >>>> the gdb remote protocol distinguishes breakpoints is by "what address", >>>> and these have different addresses). After this change, they won't >>>> be distinct, because if you set a bp at 0x100 and 0x800100 and then >>>> try to remove the one at 0x100 we might remove the 0x800100 one, >>>> because we're storing only the adjusted-address, not the one gdb used. > > Yes. Looks good. > >>>> >>>> This might not matter in practice... >>> >>> I don't think it will matter. >>> >>> Currently, if it sets both 0x100 and 0x800100, then we'll record two >>> breakpoints, and with either we'll raise EXCP_DEBUG when pc == 0x100. >>> >>> Afterward, we'll have two CPUBreakpoint structures that both contain >>> 0x100, and when pc == 0x100 we'll raise EXCP_DEBUG. If gdb removes the >>> breakpoint at 0x800100, we'll remove one of the two CPUBreakpoint. But >>> we'll still stop at 0x100, as expected. When it removes the breakpoint >>> at 0x100, both CPUBreakpoint structures will be gone. >>> >>> In principal, gdb could now add a breakpoint at 0x800100 and remove it >>> with 0x100, where it could not before. But I don't expect that to >>> happen. If we reported any kind of status to gdb re the breakpoint >>> insertion or removal (e.g. bp not found), then it might matter, but we >>> don't. > > IIUC QEMU model is "hardware breakpoint". I don't know how gdb deals > if user add both soft/hard bp. Neither do I know how many soft/hard > bp are "annouced" via gdbstub monitor to gdb (Alex?). The gdbstub treats both the same and I don't think gdb cares about the limit until we tell it we can't insert a breakpoint (which we won't because breakpoints are infinite). -- Alex Bennée
On 7/20/21 11:53 AM, Philippe Mathieu-Daudé wrote: >> Practically, this is working around what I'd call a gdb bug wrt avr. >> Which may even have been fixed -- I haven't looked. > > This is not a bug but a feature to deal with the Harvard architecture. > QEMU AVR model is based on GCC sources so uses the same "feature". > > The AVR core has 2 address spaces: "CODE" and "DATA". An address space > is always zero-based (so both are). To avoid having to deal with > relocation of symbols from different AS but having same address, the > DATA space is mapped at 0x800000 (bit 23 is "virtual" as inexistant > - masked - from the CODE AS). > > The core can not execute from DATA, so CPUBreakpoint can only be > triggered from CODE. I know all this. It begs the question why gdb would ever *ask* for a CODE breakpoint in DATA space. r~
© 2016 - 2026 Red Hat, Inc.