Re: [PATCH for-6.1 v6 11/17] hw/core: Introduce CPUClass.gdb_adjust_breakpoint

[Philippe Mathieu-Daudé]

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);
 }