[PATCH 00/17] target/avr: Increase page size

[PATCH 00/17] target/avr: Increase page size

[Richard Henderson]

For single-binary, we would really like to have a common
TARGET_PAGE_BITS_MIN.  A value of 10 will suffice for armv4
and will just fit required page flags and alignment.

AVR's use of TARGET_PAGE_BITS == 8 is unfortunate, and is due to
having memory mapped i/o in the first 256 or 512 bytes and sram
starting immediately afterward -- the softmmu page table mapping
really doesn't like mixed i/o and sram on the same page.

My solution is to bias the entire AVR address space up in the
QEMU address space.  This places sram at the start of the second
QEMU page, and the i/o immediately beforehand, at the end of
the first QEMU page.

Once the bias exists, we can choose any value we like.
Use this to select a larger page size, based on the size of sram.
This minimizes the number of pages required to span flash and sram.

There are also two bugs fixed in the avr disassembler.


r~


Richard Henderson (17):
  hw/core/cpu: Use size_t for memory_rw_debug len argument
  target/avr: Fix buffer read in avr_print_insn
  target/avr: Improve decode of LDS, STS
  target/avr: Remove OFFSET_CPU_REGISTERS
  target/avr: Move cpu register accesses into system memory
  target/avr: Use cpu_stb_mmuidx_ra in helper_fullwr
  target/avr: Use do_stb in avr_cpu_do_interrupt
  target/avr: Add offset-io cpu property
  target/avr: Introduce gen_data_{load,store}_raw
  target/avr: Update cpu_sp after push and pop
  target/avr: Implement CPUState.memory_rw_debug
  target/avr: Handle offset_io in helper.c
  target/avr: Handle offset_io in avr_cpu_realizefn
  hw/avr: Set offset_io and increase page size to 1k
  hw/avr: Pass mcu_type to class_base_init via .class_data
  hw/avr: Move AtmegaMcuClass to atmega.h
  target/avr: Enable TARGET_PAGE_BITS_VARY

 hw/avr/atmega.h           |  20 +++
 include/hw/core/cpu.h     |   2 +-
 target/avr/cpu-param.h    |  13 +-
 target/avr/cpu.h          |  11 +-
 target/avr/helper.h       |   3 -
 target/sparc/cpu.h        |   2 +-
 hw/avr/arduino.c          |  31 ++++-
 hw/avr/atmega.c           |  76 ++++++-----
 target/avr/cpu.c          |  49 +++++++
 target/avr/disas.c        |  21 ++-
 target/avr/helper.c       | 263 +++++++++++++++++---------------------
 target/avr/translate.c    | 106 +++++++++------
 target/sparc/mmu_helper.c |   2 +-
 target/avr/insn.decode    |   7 +-
 14 files changed, 346 insertions(+), 260 deletions(-)

-- 
2.43.0

[PATCH 01/17] hw/core/cpu: Use size_t for memory_rw_debug len argument

[Richard Henderson]

Match the prototype of cpu_memory_rw_debug().

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/hw/core/cpu.h     | 2 +-
 target/sparc/cpu.h        | 2 +-
 target/sparc/mmu_helper.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5d11d26556..abd8764e83 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -154,7 +154,7 @@ struct CPUClass {
 
     int (*mmu_index)(CPUState *cpu, bool ifetch);
     int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
-                           uint8_t *buf, int len, bool is_write);
+                           uint8_t *buf, size_t len, bool is_write);
     void (*dump_state)(CPUState *cpu, FILE *, int flags);
     void (*query_cpu_fast)(CPUState *cpu, CpuInfoFast *value);
     int64_t (*get_arch_id)(CPUState *cpu);
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 462bcb6c0e..68f8c21e7c 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -604,7 +604,7 @@ void dump_mmu(CPUSPARCState *env);
 
 #if !defined(TARGET_SPARC64) && !defined(CONFIG_USER_ONLY)
 int sparc_cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
-                              uint8_t *buf, int len, bool is_write);
+                              uint8_t *buf, size_t len, bool is_write);
 #endif
 
 /* translate.c */
diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index 7548d01777..3821cd91ec 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -389,7 +389,7 @@ void dump_mmu(CPUSPARCState *env)
  * that the sparc ABI is followed.
  */
 int sparc_cpu_memory_rw_debug(CPUState *cs, vaddr address,
-                              uint8_t *buf, int len, bool is_write)
+                              uint8_t *buf, size_t len, bool is_write)
 {
     CPUSPARCState *env = cpu_env(cs);
     target_ulong addr = address;
-- 
2.43.0

[PATCH 02/17] target/avr: Fix buffer read in avr_print_insn

[Richard Henderson]

Do not unconditionally attempt to read 4 bytes, as there
may only be 2 bytes remaining in the translator cache.

Cc: qemu-stable@nongnu.org
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/disas.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/target/avr/disas.c b/target/avr/disas.c
index b7689e8d7c..d341030174 100644
--- a/target/avr/disas.c
+++ b/target/avr/disas.c
@@ -68,28 +68,35 @@ static bool decode_insn(DisasContext *ctx, uint16_t insn);
 
 int avr_print_insn(bfd_vma addr, disassemble_info *info)
 {
-    DisasContext ctx;
+    DisasContext ctx = { info };
     DisasContext *pctx = &ctx;
     bfd_byte buffer[4];
     uint16_t insn;
     int status;
 
-    ctx.info = info;
-
-    status = info->read_memory_func(addr, buffer, 4, info);
+    status = info->read_memory_func(addr, buffer, 2, info);
     if (status != 0) {
         info->memory_error_func(status, addr, info);
         return -1;
     }
     insn = bfd_getl16(buffer);
-    ctx.next_word = bfd_getl16(buffer + 2);
-    ctx.next_word_used = false;
+
+    status = info->read_memory_func(addr + 2, buffer + 2, 2, info);
+    if (status == 0) {
+        ctx.next_word = bfd_getl16(buffer + 2);
+    }
 
     if (!decode_insn(&ctx, insn)) {
         output(".db", "0x%02x, 0x%02x", buffer[0], buffer[1]);
     }
 
-    return ctx.next_word_used ? 4 : 2;
+    if (!ctx.next_word_used) {
+        return 2;
+    } else if (status == 0) {
+        return 4;
+    }
+    info->memory_error_func(status, addr + 2, info);
+    return -1;
 }
 
 
-- 
2.43.0

[PATCH 03/17] target/avr: Improve decode of LDS, STS

[Richard Henderson]

The comment about not being able to define a field with
zero bits is out of date since 94597b6146f3
("decodetree: Allow !function with no input bits").

This fixes the missing load of imm in the disassembler.

Cc: qemu-stable@nongnu.org
Fixes: 9d8caa67a24 ("target/avr: Add support for disassembling via option '-d in_asm'")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/translate.c | 2 --
 target/avr/insn.decode | 7 ++-----
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index 4ab71d8138..e7f8ced9b3 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -1578,7 +1578,6 @@ static bool trans_LDS(DisasContext *ctx, arg_LDS *a)
     TCGv Rd = cpu_r[a->rd];
     TCGv addr = tcg_temp_new_i32();
     TCGv H = cpu_rampD;
-    a->imm = next_word(ctx);
 
     tcg_gen_mov_tl(addr, H); /* addr = H:M:L */
     tcg_gen_shli_tl(addr, addr, 16);
@@ -1783,7 +1782,6 @@ static bool trans_STS(DisasContext *ctx, arg_STS *a)
     TCGv Rd = cpu_r[a->rd];
     TCGv addr = tcg_temp_new_i32();
     TCGv H = cpu_rampD;
-    a->imm = next_word(ctx);
 
     tcg_gen_mov_tl(addr, H); /* addr = H:M:L */
     tcg_gen_shli_tl(addr, addr, 16);
diff --git a/target/avr/insn.decode b/target/avr/insn.decode
index 482c23ad0c..cc302249db 100644
--- a/target/avr/insn.decode
+++ b/target/avr/insn.decode
@@ -118,11 +118,8 @@ BRBC            1111 01 ....... ...         @op_bit_imm
 @io_rd_imm      .... . .. ..... ....        &rd_imm     rd=%rd imm=%io_imm
 @ldst_d         .. . . .. . rd:5  . ...     &rd_imm     imm=%ldst_d_imm
 
-# The 16-bit immediate is completely in the next word.
-# Fields cannot be defined with no bits, so we cannot play
-# the same trick and append to a zero-bit value.
-# Defer reading the immediate until trans_{LDS,STS}.
-@ldst_s         .... ... rd:5 ....          imm=0
+%ldst_imm       !function=next_word
+@ldst_s         .... ... rd:5 ....          imm=%ldst_imm
 
 MOV             0010 11 . ..... ....        @op_rd_rr
 MOVW            0000 0001 .... ....         &rd_rr      rd=%rd_d rr=%rr_d
-- 
2.43.0

[PATCH 04/17] target/avr: Remove OFFSET_CPU_REGISTERS

[Richard Henderson]

This define isn't really used.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/cpu.h    | 2 --
 target/avr/helper.c | 3 +--
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/target/avr/cpu.h b/target/avr/cpu.h
index 06f5ae4d1b..84a8f5cc8c 100644
--- a/target/avr/cpu.h
+++ b/target/avr/cpu.h
@@ -60,8 +60,6 @@
 #define OFFSET_CODE 0x00000000
 /* CPU registers, IO registers, and SRAM */
 #define OFFSET_DATA 0x00800000
-/* CPU registers specifically, these are mapped at the start of data */
-#define OFFSET_CPU_REGISTERS OFFSET_DATA
 /*
  * IO registers, including status register, stack pointer, and memory
  * mapped peripherals, mapped just after CPU registers
diff --git a/target/avr/helper.c b/target/avr/helper.c
index 3412312ad5..e5bf16c6b7 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -340,8 +340,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr)
     env->fullacc = false;
 
     /* Following logic assumes this: */
-    assert(OFFSET_CPU_REGISTERS == OFFSET_DATA);
-    assert(OFFSET_IO_REGISTERS == OFFSET_CPU_REGISTERS +
+    assert(OFFSET_IO_REGISTERS == OFFSET_DATA +
                                   NUMBER_OF_CPU_REGISTERS);
 
     if (addr < NUMBER_OF_CPU_REGISTERS) {
-- 
2.43.0

[PATCH 05/17] target/avr: Move cpu register accesses into system memory

[Richard Henderson]

Integrate the i/o 0x00-0x1f and 0x38-0x3f loopbacks into
the cpu registers with normal address space accesses.
We no longer need to trap accesses to the first page within
avr_cpu_tlb_fill but can wait until a write occurs.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/cpu.h       |   7 ++
 target/avr/helper.h    |   3 -
 target/avr/cpu.c       |  16 +++
 target/avr/helper.c    | 239 +++++++++++++++++------------------------
 target/avr/translate.c |  42 ++++----
 5 files changed, 146 insertions(+), 161 deletions(-)

diff --git a/target/avr/cpu.h b/target/avr/cpu.h
index 84a8f5cc8c..be27b0152b 100644
--- a/target/avr/cpu.h
+++ b/target/avr/cpu.h
@@ -23,6 +23,7 @@
 
 #include "cpu-qom.h"
 #include "exec/cpu-defs.h"
+#include "exec/memory.h"
 
 #ifdef CONFIG_USER_ONLY
 #error "AVR 8-bit does not support user mode"
@@ -142,6 +143,9 @@ struct ArchCPU {
 
     CPUAVRState env;
 
+    MemoryRegion cpu_reg1;
+    MemoryRegion cpu_reg2;
+
     /* Initial value of stack pointer */
     uint32_t init_sp;
 };
@@ -242,6 +246,9 @@ bool avr_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                       MMUAccessType access_type, int mmu_idx,
                       bool probe, uintptr_t retaddr);
 
+extern const MemoryRegionOps avr_cpu_reg1;
+extern const MemoryRegionOps avr_cpu_reg2;
+
 #include "exec/cpu-all.h"
 
 #endif /* QEMU_AVR_CPU_H */
diff --git a/target/avr/helper.h b/target/avr/helper.h
index 4d02e648fa..e8d13e925f 100644
--- a/target/avr/helper.h
+++ b/target/avr/helper.h
@@ -23,7 +23,4 @@ DEF_HELPER_1(debug, noreturn, env)
 DEF_HELPER_1(break, noreturn, env)
 DEF_HELPER_1(sleep, noreturn, env)
 DEF_HELPER_1(unsupported, noreturn, env)
-DEF_HELPER_3(outb, void, env, i32, i32)
-DEF_HELPER_2(inb, tl, env, i32)
 DEF_HELPER_3(fullwr, void, env, i32, i32)
-DEF_HELPER_2(fullrd, tl, env, i32)
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 834c7082aa..0b14b36c17 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -23,6 +23,7 @@
 #include "qemu/qemu-print.h"
 #include "exec/exec-all.h"
 #include "exec/translation-block.h"
+#include "exec/address-spaces.h"
 #include "cpu.h"
 #include "disas/dis-asm.h"
 #include "tcg/debug-assert.h"
@@ -110,6 +111,8 @@ static void avr_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
 static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    CPUAVRState *env = cpu_env(cs);
+    AVRCPU *cpu = env_archcpu(env);
     AVRCPUClass *mcc = AVR_CPU_GET_CLASS(dev);
     Error *local_err = NULL;
 
@@ -122,6 +125,19 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_reset(cs);
 
     mcc->parent_realize(dev, errp);
+
+    /*
+     * Two blocks in the low data space loop back into cpu registers.
+     */
+    memory_region_init_io(&cpu->cpu_reg1, OBJECT(cpu), &avr_cpu_reg1, env,
+                          "avr-cpu-reg1", 32);
+    memory_region_add_subregion(get_system_memory(),
+                                OFFSET_DATA, &cpu->cpu_reg1);
+
+    memory_region_init_io(&cpu->cpu_reg2, OBJECT(cpu), &avr_cpu_reg2, env,
+                          "avr-cpu-reg2", 8);
+    memory_region_add_subregion(get_system_memory(),
+                                OFFSET_DATA + 0x58, &cpu->cpu_reg2);
 }
 
 static void avr_cpu_set_int(void *opaque, int irq, int level)
diff --git a/target/avr/helper.c b/target/avr/helper.c
index e5bf16c6b7..df7e2109d4 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -108,7 +108,7 @@ bool avr_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                       MMUAccessType access_type, int mmu_idx,
                       bool probe, uintptr_t retaddr)
 {
-    int prot, page_size = TARGET_PAGE_SIZE;
+    int prot;
     uint32_t paddr;
 
     address &= TARGET_PAGE_MASK;
@@ -133,23 +133,9 @@ bool avr_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
         /* Access to memory. */
         paddr = OFFSET_DATA + address;
         prot = PAGE_READ | PAGE_WRITE;
-        if (address < NUMBER_OF_CPU_REGISTERS + NUMBER_OF_IO_REGISTERS) {
-            /*
-             * Access to CPU registers, exit and rebuilt this TB to use
-             * full access in case it touches specially handled registers
-             * like SREG or SP.  For probing, set page_size = 1, in order
-             * to force tlb_fill to be called for the next access.
-             */
-            if (probe) {
-                page_size = 1;
-            } else {
-                cpu_env(cs)->fullacc = 1;
-                cpu_loop_exit_restore(cs, retaddr);
-            }
-        }
     }
 
-    tlb_set_page(cs, address, paddr, prot, mmu_idx, page_size);
+    tlb_set_page(cs, address, paddr, prot, mmu_idx, TARGET_PAGE_SIZE);
     return true;
 }
 
@@ -203,134 +189,78 @@ void helper_wdr(CPUAVRState *env)
 }
 
 /*
- * 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
- *
+ * The first 32 bytes of the data space are mapped to the cpu regs.
+ * We cannot write these from normal store operations because TCG
+ * does not expect global temps to be modified -- a global may be
+ * live in a host cpu register across the store.  We can however
+ * read these, as TCG does make sure the global temps are saved
+ * in case the load operation traps.
  */
-target_ulong helper_inb(CPUAVRState *env, uint32_t port)
+
+static uint64_t avr_cpu_reg1_read(void *opaque, hwaddr addr, unsigned size)
 {
-    target_ulong data = 0;
+    CPUAVRState *env = opaque;
 
-    switch (port) {
-    case 0x38: /* RAMPD */
-        data = 0xff & (env->rampD >> 16);
-        break;
-    case 0x39: /* RAMPX */
-        data = 0xff & (env->rampX >> 16);
-        break;
-    case 0x3a: /* RAMPY */
-        data = 0xff & (env->rampY >> 16);
-        break;
-    case 0x3b: /* RAMPZ */
-        data = 0xff & (env->rampZ >> 16);
-        break;
-    case 0x3c: /* EIND */
-        data = 0xff & (env->eind >> 16);
-        break;
-    case 0x3d: /* SPL */
-        data = env->sp & 0x00ff;
-        break;
-    case 0x3e: /* SPH */
-        data = env->sp >> 8;
-        break;
-    case 0x3f: /* SREG */
-        data = cpu_get_sreg(env);
-        break;
-    default:
-        /* not a special register, pass to normal memory access */
-        data = address_space_ldub(&address_space_memory,
-                                  OFFSET_IO_REGISTERS + port,
-                                  MEMTXATTRS_UNSPECIFIED, NULL);
-    }
-
-    return data;
+    assert(addr < 32);
+    return env->r[addr];
 }
 
 /*
- *  This function implements OUT instruction
- *
- *  It does the following
- *  a.  if an IO register belongs to CPU, its value is written into the register
- *  b.  otherwise io address is translated to mem address and physical memory
- *      is written.
- *  c.  it caches the value for sake of SBI, SBIC, SBIS & CBI implementation
- *
+ * The range 0x58-0x5f of the data space are mapped to cpu regs.
+ * As above, we cannot write these from normal store operations.
  */
-void helper_outb(CPUAVRState *env, uint32_t port, uint32_t data)
-{
-    data &= 0x000000ff;
 
-    switch (port) {
-    case 0x38: /* RAMPD */
-        if (avr_feature(env, AVR_FEATURE_RAMPD)) {
-            env->rampD = (data & 0xff) << 16;
-        }
-        break;
-    case 0x39: /* RAMPX */
-        if (avr_feature(env, AVR_FEATURE_RAMPX)) {
-            env->rampX = (data & 0xff) << 16;
-        }
-        break;
-    case 0x3a: /* RAMPY */
-        if (avr_feature(env, AVR_FEATURE_RAMPY)) {
-            env->rampY = (data & 0xff) << 16;
-        }
-        break;
-    case 0x3b: /* RAMPZ */
-        if (avr_feature(env, AVR_FEATURE_RAMPZ)) {
-            env->rampZ = (data & 0xff) << 16;
-        }
-        break;
-    case 0x3c: /* EIDN */
-        env->eind = (data & 0xff) << 16;
-        break;
-    case 0x3d: /* SPL */
-        env->sp = (env->sp & 0xff00) | (data);
-        break;
-    case 0x3e: /* SPH */
-        if (avr_feature(env, AVR_FEATURE_2_BYTE_SP)) {
-            env->sp = (env->sp & 0x00ff) | (data << 8);
-        }
-        break;
-    case 0x3f: /* SREG */
-        cpu_set_sreg(env, data);
-        break;
-    default:
-        /* not a special register, pass to normal memory access */
-        address_space_stb(&address_space_memory, OFFSET_IO_REGISTERS + port,
-                          data, MEMTXATTRS_UNSPECIFIED, NULL);
+static uint64_t avr_cpu_reg2_read(void *opaque, hwaddr addr, unsigned size)
+{
+    CPUAVRState *env = opaque;
+
+    switch (addr) {
+    case 0: /* RAMPD */
+        return 0xff & (env->rampD >> 16);
+    case 1: /* RAMPX */
+        return 0xff & (env->rampX >> 16);
+    case 2: /* RAMPY */
+        return 0xff & (env->rampY >> 16);
+    case 3: /* RAMPZ */
+        return 0xff & (env->rampZ >> 16);
+    case 4: /* EIND */
+        return 0xff & (env->eind >> 16);
+    case 5: /* SPL */
+        return env->sp & 0x00ff;
+    case 6: /* SPH */
+        return 0xff & (env->sp >> 8);
+    case 7: /* SREG */
+        return cpu_get_sreg(env);
     }
+    g_assert_not_reached();
 }
 
-/*
- *  this function implements LD instruction when there is a possibility to read
- *  from a CPU register
- */
-target_ulong helper_fullrd(CPUAVRState *env, uint32_t addr)
+static void avr_cpu_trap_write(void *opaque, hwaddr addr,
+                               uint64_t data64, unsigned size)
 {
-    uint8_t data;
+    CPUAVRState *env = opaque;
+    CPUState *cs = env_cpu(env);
 
-    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 */
-        data = address_space_ldub(&address_space_memory, OFFSET_DATA + addr,
-                                  MEMTXATTRS_UNSPECIFIED, NULL);
-    }
-    return data;
+    env->fullacc = true;
+    cpu_loop_exit_restore(cs, cs->mem_io_pc);
 }
 
+const MemoryRegionOps avr_cpu_reg1 = {
+    .read = avr_cpu_reg1_read,
+    .write = avr_cpu_trap_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 1,
+};
+
+const MemoryRegionOps avr_cpu_reg2 = {
+    .read = avr_cpu_reg2_read,
+    .write = avr_cpu_trap_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 1,
+};
+
 /*
  *  this function implements ST instruction when there is a possibility to write
  *  into a CPU register
@@ -339,19 +269,50 @@ void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr)
 {
     env->fullacc = false;
 
-    /* Following logic assumes this: */
-    assert(OFFSET_IO_REGISTERS == OFFSET_DATA +
-                                  NUMBER_OF_CPU_REGISTERS);
-
-    if (addr < NUMBER_OF_CPU_REGISTERS) {
+    switch (addr) {
+    case 0 ... 31:
         /* CPU registers */
         env->r[addr] = data;
-    } else if (addr < NUMBER_OF_CPU_REGISTERS + NUMBER_OF_IO_REGISTERS) {
-        /* IO registers */
-        helper_outb(env, addr - NUMBER_OF_CPU_REGISTERS, data);
-    } else {
-        /* memory */
+        break;
+
+    case 0x58: /* RAMPD */
+        if (avr_feature(env, AVR_FEATURE_RAMPD)) {
+            env->rampD = data << 16;
+        }
+        break;
+    case 0x59: /* RAMPX */
+        if (avr_feature(env, AVR_FEATURE_RAMPX)) {
+            env->rampX = data << 16;
+        }
+        break;
+    case 0x5a: /* RAMPY */
+        if (avr_feature(env, AVR_FEATURE_RAMPY)) {
+            env->rampY = data << 16;
+        }
+        break;
+    case 0x5b: /* RAMPZ */
+        if (avr_feature(env, AVR_FEATURE_RAMPZ)) {
+            env->rampZ = data << 16;
+        }
+        break;
+    case 0x5c: /* EIDN */
+        env->eind = data << 16;
+        break;
+    case 0x5d: /* SPL */
+        env->sp = (env->sp & 0xff00) | data;
+        break;
+    case 0x5e: /* SPH */
+        if (avr_feature(env, AVR_FEATURE_2_BYTE_SP)) {
+            env->sp = (env->sp & 0x00ff) | (data << 8);
+        }
+        break;
+    case 0x5f: /* SREG */
+        cpu_set_sreg(env, data);
+        break;
+
+    default:
         address_space_stb(&address_space_memory, OFFSET_DATA + addr, data,
                           MEMTXATTRS_UNSPECIFIED, NULL);
+        break;
     }
 }
diff --git a/target/avr/translate.c b/target/avr/translate.c
index e7f8ced9b3..0490936cd5 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -194,6 +194,9 @@ static bool avr_have_feature(DisasContext *ctx, int feature)
 static bool decode_insn(DisasContext *ctx, uint16_t insn);
 #include "decode-insn.c.inc"
 
+static void gen_inb(DisasContext *ctx, TCGv data, int port);
+static void gen_outb(DisasContext *ctx, TCGv data, int port);
+
 /*
  * Arithmetic Instructions
  */
@@ -1293,9 +1296,8 @@ static bool trans_SBRS(DisasContext *ctx, arg_SBRS *a)
 static bool trans_SBIC(DisasContext *ctx, arg_SBIC *a)
 {
     TCGv data = tcg_temp_new_i32();
-    TCGv port = tcg_constant_i32(a->reg);
 
-    gen_helper_inb(data, tcg_env, port);
+    gen_inb(ctx, data, a->reg);
     tcg_gen_andi_tl(data, data, 1 << a->bit);
     ctx->skip_cond = TCG_COND_EQ;
     ctx->skip_var0 = data;
@@ -1311,9 +1313,8 @@ static bool trans_SBIC(DisasContext *ctx, arg_SBIC *a)
 static bool trans_SBIS(DisasContext *ctx, arg_SBIS *a)
 {
     TCGv data = tcg_temp_new_i32();
-    TCGv port = tcg_constant_i32(a->reg);
 
-    gen_helper_inb(data, tcg_env, port);
+    gen_inb(ctx, data, a->reg);
     tcg_gen_andi_tl(data, data, 1 << a->bit);
     ctx->skip_cond = TCG_COND_NE;
     ctx->skip_var0 = data;
@@ -1502,11 +1503,18 @@ static void gen_data_store(DisasContext *ctx, TCGv data, TCGv addr)
 
 static void gen_data_load(DisasContext *ctx, TCGv data, TCGv addr)
 {
-    if (ctx->base.tb->flags & TB_FLAGS_FULL_ACCESS) {
-        gen_helper_fullrd(data, tcg_env, addr);
-    } else {
-        tcg_gen_qemu_ld_tl(data, addr, MMU_DATA_IDX, MO_UB);
-    }
+    tcg_gen_qemu_ld_tl(data, addr, MMU_DATA_IDX, MO_UB);
+}
+
+static void gen_inb(DisasContext *ctx, TCGv data, int port)
+{
+    gen_data_load(ctx, data, tcg_constant_i32(port + NUMBER_OF_CPU_REGISTERS));
+}
+
+static void gen_outb(DisasContext *ctx, TCGv data, int port)
+{
+    gen_helper_fullwr(tcg_env, data,
+                      tcg_constant_i32(port + NUMBER_OF_CPU_REGISTERS));
 }
 
 /*
@@ -2126,9 +2134,8 @@ static bool trans_SPMX(DisasContext *ctx, arg_SPMX *a)
 static bool trans_IN(DisasContext *ctx, arg_IN *a)
 {
     TCGv Rd = cpu_r[a->rd];
-    TCGv port = tcg_constant_i32(a->imm);
 
-    gen_helper_inb(Rd, tcg_env, port);
+    gen_inb(ctx, Rd, a->imm);
     return true;
 }
 
@@ -2139,9 +2146,8 @@ static bool trans_IN(DisasContext *ctx, arg_IN *a)
 static bool trans_OUT(DisasContext *ctx, arg_OUT *a)
 {
     TCGv Rd = cpu_r[a->rd];
-    TCGv port = tcg_constant_i32(a->imm);
 
-    gen_helper_outb(tcg_env, port, Rd);
+    gen_outb(ctx, Rd, a->imm);
     return true;
 }
 
@@ -2407,11 +2413,10 @@ static bool trans_SWAP(DisasContext *ctx, arg_SWAP *a)
 static bool trans_SBI(DisasContext *ctx, arg_SBI *a)
 {
     TCGv data = tcg_temp_new_i32();
-    TCGv port = tcg_constant_i32(a->reg);
 
-    gen_helper_inb(data, tcg_env, port);
+    gen_inb(ctx, data, a->reg);
     tcg_gen_ori_tl(data, data, 1 << a->bit);
-    gen_helper_outb(tcg_env, port, data);
+    gen_outb(ctx, data, a->reg);
     return true;
 }
 
@@ -2422,11 +2427,10 @@ static bool trans_SBI(DisasContext *ctx, arg_SBI *a)
 static bool trans_CBI(DisasContext *ctx, arg_CBI *a)
 {
     TCGv data = tcg_temp_new_i32();
-    TCGv port = tcg_constant_i32(a->reg);
 
-    gen_helper_inb(data, tcg_env, port);
+    gen_inb(ctx, data, a->reg);
     tcg_gen_andi_tl(data, data, ~(1 << a->bit));
-    gen_helper_outb(tcg_env, port, data);
+    gen_outb(ctx, data, a->reg);
     return true;
 }
 
-- 
2.43.0

[PATCH 06/17] target/avr: Use cpu_stb_mmuidx_ra in helper_fullwr

[Richard Henderson]

Avoid direct use of address_space_memory.
Make use of the softmmu cache of the i/o page.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/helper.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/target/avr/helper.c b/target/avr/helper.c
index df7e2109d4..7cfd3d1093 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -23,10 +23,10 @@
 #include "qemu/error-report.h"
 #include "cpu.h"
 #include "accel/tcg/cpu-ops.h"
+#include "accel/tcg/getpc.h"
 #include "exec/cputlb.h"
 #include "exec/page-protection.h"
 #include "exec/cpu_ldst.h"
-#include "exec/address-spaces.h"
 #include "exec/helper-proto.h"
 
 bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
@@ -67,6 +67,11 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     return false;
 }
 
+static void do_stb(CPUAVRState *env, uint32_t addr, uint8_t data, uintptr_t ra)
+{
+    cpu_stb_mmuidx_ra(env, addr, data, MMU_DATA_IDX, ra);
+}
+
 void avr_cpu_do_interrupt(CPUState *cs)
 {
     CPUAVRState *env = cpu_env(cs);
@@ -311,8 +316,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr)
         break;
 
     default:
-        address_space_stb(&address_space_memory, OFFSET_DATA + addr, data,
-                          MEMTXATTRS_UNSPECIFIED, NULL);
+        do_stb(env, addr, data, GETPC());
         break;
     }
 }
-- 
2.43.0

[PATCH 07/17] target/avr: Use do_stb in avr_cpu_do_interrupt

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/avr/helper.c b/target/avr/helper.c
index 7cfd3d1093..9608e59584 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -88,14 +88,14 @@ void avr_cpu_do_interrupt(CPUState *cs)
     }
 
     if (avr_feature(env, AVR_FEATURE_3_BYTE_PC)) {
-        cpu_stb_data(env, env->sp--, (ret & 0x0000ff));
-        cpu_stb_data(env, env->sp--, (ret & 0x00ff00) >> 8);
-        cpu_stb_data(env, env->sp--, (ret & 0xff0000) >> 16);
+        do_stb(env, env->sp--, ret, 0);
+        do_stb(env, env->sp--, ret >> 8, 0);
+        do_stb(env, env->sp--, ret >> 16, 0);
     } else if (avr_feature(env, AVR_FEATURE_2_BYTE_PC)) {
-        cpu_stb_data(env, env->sp--, (ret & 0x0000ff));
-        cpu_stb_data(env, env->sp--, (ret & 0x00ff00) >> 8);
+        do_stb(env, env->sp--, ret, 0);
+        do_stb(env, env->sp--, ret >> 8, 0);
     } else {
-        cpu_stb_data(env, env->sp--, (ret & 0x0000ff));
+        do_stb(env, env->sp--, ret, 0);
     }
 
     env->pc_w = base + vector * size;
-- 
2.43.0

[PATCH 08/17] target/avr: Add offset-io cpu property

[Richard Henderson]

Communicate the offset of io within the first page between the
board, the cpu, and the translator.  So far this is always 0.
This will be used to optimize memory layout.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/cpu.h       | 2 ++
 hw/avr/atmega.c        | 2 ++
 target/avr/cpu.c       | 1 +
 target/avr/translate.c | 2 ++
 4 files changed, 7 insertions(+)

diff --git a/target/avr/cpu.h b/target/avr/cpu.h
index be27b0152b..0f5e1a53bc 100644
--- a/target/avr/cpu.h
+++ b/target/avr/cpu.h
@@ -148,6 +148,8 @@ struct ArchCPU {
 
     /* Initial value of stack pointer */
     uint32_t init_sp;
+    /* Offset of the beginning of I/O within the first page. */
+    uint32_t offset_io;
 };
 
 /**
diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
index f6844bf118..273582b8af 100644
--- a/hw/avr/atmega.c
+++ b/hw/avr/atmega.c
@@ -236,6 +236,8 @@ static void atmega_realize(DeviceState *dev, Error **errp)
 
     object_property_set_uint(OBJECT(&s->cpu), "init-sp",
                              mc->io_size + mc->sram_size - 1, &error_abort);
+    object_property_set_uint(OBJECT(&s->cpu), "offset-io",
+                             0, &error_abort);
 
     qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
     cpudev = DEVICE(&s->cpu);
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 0b14b36c17..080f6f30d3 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -169,6 +169,7 @@ static void avr_cpu_initfn(Object *obj)
 
 static const Property avr_cpu_properties[] = {
     DEFINE_PROP_UINT32("init-sp", AVRCPU, init_sp, 0),
+    DEFINE_PROP_UINT32("offset-io", AVRCPU, offset_io, 0),
 };
 
 static ObjectClass *avr_cpu_class_by_name(const char *cpu_model)
diff --git a/target/avr/translate.c b/target/avr/translate.c
index 0490936cd5..e9fef1aaad 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -92,6 +92,7 @@ struct DisasContext {
 
     /* Routine used to access memory */
     int memidx;
+    uint32_t offset_io;
 
     /*
      * some AVR instructions can make the following instruction to be skipped
@@ -2664,6 +2665,7 @@ static void avr_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->cs = cs;
     ctx->env = cpu_env(cs);
     ctx->npc = ctx->base.pc_first / 2;
+    ctx->offset_io = env_archcpu(ctx->env)->offset_io;
 
     ctx->skip_cond = TCG_COND_NEVER;
     if (tb_flags & TB_FLAGS_SKIP) {
-- 
2.43.0

[PATCH 09/17] target/avr: Introduce gen_data_{load,store}_raw

[Richard Henderson]

Prepare for offset_io being non-zero; also allow folding
stack pointer offsets into the arithmetic.
So far, all offsets are 0.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/translate.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index e9fef1aaad..6bb4154dff 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -198,6 +198,28 @@ static bool decode_insn(DisasContext *ctx, uint16_t insn);
 static void gen_inb(DisasContext *ctx, TCGv data, int port);
 static void gen_outb(DisasContext *ctx, TCGv data, int port);
 
+static void gen_data_store_raw(DisasContext *ctx, TCGv data,
+                               TCGv addr, int offset, MemOp mop)
+{
+    if (ctx->offset_io + offset) {
+        TCGv t = tcg_temp_new();
+        tcg_gen_addi_tl(t, addr, ctx->offset_io + offset);
+        addr = t;
+    }
+    tcg_gen_qemu_st_tl(data, addr, MMU_DATA_IDX, mop);
+}
+
+static void gen_data_load_raw(DisasContext *ctx, TCGv data,
+                              TCGv addr, int offset, MemOp mop)
+{
+    if (ctx->offset_io + offset) {
+        TCGv t = tcg_temp_new();
+        tcg_gen_addi_tl(t, addr, ctx->offset_io + offset);
+        addr = t;
+    }
+    tcg_gen_qemu_ld_tl(data, addr, MMU_DATA_IDX, mop);
+}
+
 /*
  * Arithmetic Instructions
  */
@@ -940,21 +962,21 @@ static void gen_push_ret(DisasContext *ctx, int ret)
     if (avr_feature(ctx->env, AVR_FEATURE_1_BYTE_PC)) {
         TCGv t0 = tcg_constant_i32(ret & 0x0000ff);
 
-        tcg_gen_qemu_st_tl(t0, cpu_sp, MMU_DATA_IDX, MO_UB);
+        gen_data_store_raw(ctx, t0, cpu_sp, 0, MO_UB);
         tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
     } else if (avr_feature(ctx->env, AVR_FEATURE_2_BYTE_PC)) {
         TCGv t0 = tcg_constant_i32(ret & 0x00ffff);
 
         tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
-        tcg_gen_qemu_st_tl(t0, cpu_sp, MMU_DATA_IDX, MO_BEUW);
+        gen_data_store_raw(ctx, t0, cpu_sp, 0, MO_BEUW);
         tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
     } else if (avr_feature(ctx->env, AVR_FEATURE_3_BYTE_PC)) {
         TCGv lo = tcg_constant_i32(ret & 0x0000ff);
         TCGv hi = tcg_constant_i32((ret & 0xffff00) >> 8);
 
-        tcg_gen_qemu_st_tl(lo, cpu_sp, MMU_DATA_IDX, MO_UB);
+        gen_data_store_raw(ctx, lo, cpu_sp, 0, MO_UB);
         tcg_gen_subi_tl(cpu_sp, cpu_sp, 2);
-        tcg_gen_qemu_st_tl(hi, cpu_sp, MMU_DATA_IDX, MO_BEUW);
+        gen_data_store_raw(ctx, hi, cpu_sp, 0, MO_BEUW);
         tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
     }
 }
@@ -963,20 +985,20 @@ static void gen_pop_ret(DisasContext *ctx, TCGv ret)
 {
     if (avr_feature(ctx->env, AVR_FEATURE_1_BYTE_PC)) {
         tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
-        tcg_gen_qemu_ld_tl(ret, cpu_sp, MMU_DATA_IDX, MO_UB);
+        gen_data_load_raw(ctx, ret, cpu_sp, 0, MO_UB);
     } else if (avr_feature(ctx->env, AVR_FEATURE_2_BYTE_PC)) {
         tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
-        tcg_gen_qemu_ld_tl(ret, cpu_sp, MMU_DATA_IDX, MO_BEUW);
+        gen_data_load_raw(ctx, ret, cpu_sp, 0, MO_BEUW);
         tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
     } else if (avr_feature(ctx->env, AVR_FEATURE_3_BYTE_PC)) {
         TCGv lo = tcg_temp_new_i32();
         TCGv hi = tcg_temp_new_i32();
 
         tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
-        tcg_gen_qemu_ld_tl(hi, cpu_sp, MMU_DATA_IDX, MO_BEUW);
+        gen_data_load_raw(ctx, hi, cpu_sp, 0, MO_BEUW);
 
         tcg_gen_addi_tl(cpu_sp, cpu_sp, 2);
-        tcg_gen_qemu_ld_tl(lo, cpu_sp, MMU_DATA_IDX, MO_UB);
+        gen_data_load_raw(ctx, lo, cpu_sp, 0, MO_UB);
 
         tcg_gen_deposit_tl(ret, lo, hi, 8, 16);
     }
@@ -1498,13 +1520,13 @@ static void gen_data_store(DisasContext *ctx, TCGv data, TCGv addr)
     if (ctx->base.tb->flags & TB_FLAGS_FULL_ACCESS) {
         gen_helper_fullwr(tcg_env, data, addr);
     } else {
-        tcg_gen_qemu_st_tl(data, addr, MMU_DATA_IDX, MO_UB);
+        gen_data_store_raw(ctx, data, addr, 0, MO_UB);
     }
 }
 
 static void gen_data_load(DisasContext *ctx, TCGv data, TCGv addr)
 {
-    tcg_gen_qemu_ld_tl(data, addr, MMU_DATA_IDX, MO_UB);
+    gen_data_load_raw(ctx, data, addr, 0, MO_UB);
 }
 
 static void gen_inb(DisasContext *ctx, TCGv data, int port)
-- 
2.43.0

[PATCH 10/17] target/avr: Update cpu_sp after push and pop

[Richard Henderson]

Not that AVR has memory paging traps, but it's better
form to allow the memory operation to finish before
updating the cpu register.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/translate.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index 6bb4154dff..3446007be1 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -967,40 +967,38 @@ static void gen_push_ret(DisasContext *ctx, int ret)
     } else if (avr_feature(ctx->env, AVR_FEATURE_2_BYTE_PC)) {
         TCGv t0 = tcg_constant_i32(ret & 0x00ffff);
 
-        tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
-        gen_data_store_raw(ctx, t0, cpu_sp, 0, MO_BEUW);
-        tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
+        gen_data_store_raw(ctx, t0, cpu_sp, -1, MO_BEUW);
+        tcg_gen_subi_tl(cpu_sp, cpu_sp, 2);
     } else if (avr_feature(ctx->env, AVR_FEATURE_3_BYTE_PC)) {
         TCGv lo = tcg_constant_i32(ret & 0x0000ff);
         TCGv hi = tcg_constant_i32((ret & 0xffff00) >> 8);
 
         gen_data_store_raw(ctx, lo, cpu_sp, 0, MO_UB);
-        tcg_gen_subi_tl(cpu_sp, cpu_sp, 2);
-        gen_data_store_raw(ctx, hi, cpu_sp, 0, MO_BEUW);
-        tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
+        gen_data_store_raw(ctx, hi, cpu_sp, -2, MO_BEUW);
+        tcg_gen_subi_tl(cpu_sp, cpu_sp, 3);
+    } else {
+        g_assert_not_reached();
     }
 }
 
 static void gen_pop_ret(DisasContext *ctx, TCGv ret)
 {
     if (avr_feature(ctx->env, AVR_FEATURE_1_BYTE_PC)) {
+        gen_data_load_raw(ctx, ret, cpu_sp, 1, MO_UB);
         tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
-        gen_data_load_raw(ctx, ret, cpu_sp, 0, MO_UB);
     } else if (avr_feature(ctx->env, AVR_FEATURE_2_BYTE_PC)) {
-        tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
-        gen_data_load_raw(ctx, ret, cpu_sp, 0, MO_BEUW);
-        tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
+        gen_data_load_raw(ctx, ret, cpu_sp, 1, MO_BEUW);
+        tcg_gen_addi_tl(cpu_sp, cpu_sp, 2);
     } else if (avr_feature(ctx->env, AVR_FEATURE_3_BYTE_PC)) {
-        TCGv lo = tcg_temp_new_i32();
         TCGv hi = tcg_temp_new_i32();
 
-        tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
-        gen_data_load_raw(ctx, hi, cpu_sp, 0, MO_BEUW);
+        gen_data_load_raw(ctx, hi, cpu_sp, 1, MO_BEUW);
+        gen_data_load_raw(ctx, ret, cpu_sp, 3, MO_UB);
+        tcg_gen_addi_tl(cpu_sp, cpu_sp, 3);
 
-        tcg_gen_addi_tl(cpu_sp, cpu_sp, 2);
-        gen_data_load_raw(ctx, lo, cpu_sp, 0, MO_UB);
-
-        tcg_gen_deposit_tl(ret, lo, hi, 8, 16);
+        tcg_gen_deposit_tl(ret, ret, hi, 8, 16);
+    } else {
+        g_assert_not_reached();
     }
 }
 
-- 
2.43.0

[PATCH 11/17] target/avr: Implement CPUState.memory_rw_debug

[Richard Henderson]

Prepare for offset_io being non-zero when accessing from gdb.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/cpu.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 080f6f30d3..e4011004b4 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -68,6 +68,35 @@ static void avr_restore_state_to_opc(CPUState *cs,
     cpu_env(cs)->pc_w = data[0];
 }
 
+static int avr_memory_rw_debug(CPUState *cpu, vaddr addr,
+                               uint8_t *buf, size_t len, bool is_write)
+{
+    if (addr < OFFSET_DATA) {
+        size_t len_code;
+        int ret;
+
+        if (addr + len <= OFFSET_DATA) {
+            return cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
+        }
+
+        len_code = addr + len - OFFSET_DATA;
+        ret = cpu_memory_rw_debug(cpu, addr, buf, len_code, is_write);
+        if (ret != 0) {
+            return ret;
+        }
+        addr = OFFSET_DATA;
+        len -= len_code;
+    }
+
+    /*
+     * Data is biased such that SRAM begins at TARGET_PAGE_SIZE,
+     * and I/O is immediately prior.  This leave a hole between
+     * OFFSET_DATA and the relative start of the address space.
+     */
+    addr += env_archcpu(cpu_env(cpu))->offset_io;
+    return cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
+}
+
 static void avr_cpu_reset_hold(Object *obj, ResetType type)
 {
     CPUState *cs = CPU(obj);
@@ -262,6 +291,7 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_write_register = avr_cpu_gdb_write_register;
     cc->gdb_adjust_breakpoint = avr_cpu_gdb_adjust_breakpoint;
     cc->gdb_core_xml_file = "avr-cpu.xml";
+    cc->memory_rw_debug = avr_memory_rw_debug;
     cc->tcg_ops = &avr_tcg_ops;
 }
 
-- 
2.43.0

[PATCH 12/17] target/avr: Handle offset_io in helper.c

[Richard Henderson]

Prepare for offset_io being non-zero in do_stb.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/avr/helper.c b/target/avr/helper.c
index 9608e59584..3323f32c22 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -69,7 +69,8 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 
 static void do_stb(CPUAVRState *env, uint32_t addr, uint8_t data, uintptr_t ra)
 {
-    cpu_stb_mmuidx_ra(env, addr, data, MMU_DATA_IDX, ra);
+    cpu_stb_mmuidx_ra(env, addr + env_archcpu(env)->offset_io,
+                      data, MMU_DATA_IDX, ra);
 }
 
 void avr_cpu_do_interrupt(CPUState *cs)
-- 
2.43.0

[PATCH 13/17] target/avr: Handle offset_io in avr_cpu_realizefn

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/cpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index e4011004b4..538fcbc215 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -161,12 +161,14 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
     memory_region_init_io(&cpu->cpu_reg1, OBJECT(cpu), &avr_cpu_reg1, env,
                           "avr-cpu-reg1", 32);
     memory_region_add_subregion(get_system_memory(),
-                                OFFSET_DATA, &cpu->cpu_reg1);
+                                OFFSET_DATA + cpu->offset_io,
+                                &cpu->cpu_reg1);
 
     memory_region_init_io(&cpu->cpu_reg2, OBJECT(cpu), &avr_cpu_reg2, env,
                           "avr-cpu-reg2", 8);
     memory_region_add_subregion(get_system_memory(),
-                                OFFSET_DATA + 0x58, &cpu->cpu_reg2);
+                                OFFSET_DATA + cpu->offset_io + 0x58,
+                                &cpu->cpu_reg2);
 }
 
 static void avr_cpu_set_int(void *opaque, int irq, int level)
-- 
2.43.0

[PATCH 14/17] hw/avr: Set offset_io and increase page size to 1k

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/cpu-param.h |  8 +------
 hw/avr/atmega.c        | 54 ++++++++++++++++++++++++++----------------
 2 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/target/avr/cpu-param.h b/target/avr/cpu-param.h
index 81f3f49ee1..f5248ce9e7 100644
--- a/target/avr/cpu-param.h
+++ b/target/avr/cpu-param.h
@@ -21,13 +21,7 @@
 #ifndef AVR_CPU_PARAM_H
 #define AVR_CPU_PARAM_H
 
-/*
- * TARGET_PAGE_BITS cannot be more than 8 bits because
- * 1.  all IO registers occupy [0x0000 .. 0x00ff] address range, and they
- *     should be implemented as a device and not memory
- * 2.  SRAM starts at the address 0x0100
- */
-#define TARGET_PAGE_BITS 8
+#define TARGET_PAGE_BITS 10
 #define TARGET_PHYS_ADDR_SPACE_BITS 24
 #define TARGET_VIRT_ADDR_SPACE_BITS 24
 
diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
index 273582b8af..d4fc9c4aee 100644
--- a/hw/avr/atmega.c
+++ b/hw/avr/atmega.c
@@ -14,6 +14,7 @@
 #include "qapi/error.h"
 #include "exec/memory.h"
 #include "exec/address-spaces.h"
+#include "exec/target_page.h"
 #include "system/system.h"
 #include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
@@ -222,6 +223,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
     DeviceState *cpudev;
     SysBusDevice *sbd;
     char *devname;
+    uint32_t offset_io, offset_sram;
     size_t i;
 
     assert(mc->io_size <= 0x200);
@@ -231,13 +233,25 @@ static void atmega_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    /*
+     * Bias the virtual data section so that the start of RAM is at
+     * the start of the second page of the physical data section.
+     * This puts all of the I/O at the end of the first page of the
+     * physical data section.
+     */
+    offset_io = TARGET_PAGE_SIZE - mc->io_size;
+    offset_sram = TARGET_PAGE_SIZE;
+
     /* CPU */
     object_initialize_child(OBJECT(dev), "cpu", &s->cpu, mc->cpu_type);
 
     object_property_set_uint(OBJECT(&s->cpu), "init-sp",
                              mc->io_size + mc->sram_size - 1, &error_abort);
     object_property_set_uint(OBJECT(&s->cpu), "offset-io",
-                             0, &error_abort);
+                             offset_io, &error_abort);
+
+    offset_io += OFFSET_DATA;
+    offset_sram += OFFSET_DATA;
 
     qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
     cpudev = DEVICE(&s->cpu);
@@ -245,8 +259,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
     /* SRAM */
     memory_region_init_ram(&s->sram, OBJECT(dev), "sram", mc->sram_size,
                            &error_abort);
-    memory_region_add_subregion(get_system_memory(),
-                                OFFSET_DATA + mc->io_size, &s->sram);
+    memory_region_add_subregion(get_system_memory(), offset_sram, &s->sram);
 
     /* Flash */
     memory_region_init_rom(&s->flash, OBJECT(dev),
@@ -258,13 +271,14 @@ static void atmega_realize(DeviceState *dev, Error **errp)
      *
      * 0x00 - 0x1f: Registers
      * 0x20 - 0x5f: I/O memory
-     * 0x60 - 0xff: Extended I/O
+     * 0x60 - 0x[1]ff: Extended I/O
      */
     s->io = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
     qdev_prop_set_string(s->io, "name", "I/O");
     qdev_prop_set_uint64(s->io, "size", mc->io_size);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(s->io), &error_fatal);
-    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(s->io), 0, OFFSET_DATA, -1234);
+
+    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(s->io), 0, offset_io, -1234);
 
     /* Power Reduction */
     for (i = 0; i < POWER_MAX; i++) {
@@ -277,7 +291,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
                                 TYPE_AVR_MASK);
         sysbus_realize(SYS_BUS_DEVICE(&s->pwr[i]), &error_abort);
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->pwr[i]), 0,
-                        OFFSET_DATA + mc->dev[idx].addr);
+                        offset_io + mc->dev[idx].addr);
         g_free(devname);
     }
 
@@ -289,7 +303,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
         }
         devname = g_strdup_printf("atmega-gpio-%c", 'a' + (char)i);
         create_unimplemented_device(devname,
-                                    OFFSET_DATA + mc->dev[idx].addr, 3);
+                                    offset_io + mc->dev[idx].addr, 3);
         g_free(devname);
     }
 
@@ -305,7 +319,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
         qdev_prop_set_chr(DEVICE(&s->usart[i]), "chardev", serial_hd(i));
         sbd = SYS_BUS_DEVICE(&s->usart[i]);
         sysbus_realize(sbd, &error_abort);
-        sysbus_mmio_map(sbd, 0, OFFSET_DATA + mc->dev[USART(i)].addr);
+        sysbus_mmio_map(sbd, 0, offset_io + mc->dev[USART(i)].addr);
         connect_peripheral_irq(mc, sbd, 0, cpudev, USART_RXC_IRQ(i));
         connect_peripheral_irq(mc, sbd, 1, cpudev, USART_DRE_IRQ(i));
         connect_peripheral_irq(mc, sbd, 2, cpudev, USART_TXC_IRQ(i));
@@ -321,12 +335,12 @@ static void atmega_realize(DeviceState *dev, Error **errp)
         }
         if (!mc->dev[idx].is_timer16) {
             create_unimplemented_device("avr-timer8",
-                                        OFFSET_DATA + mc->dev[idx].addr, 5);
+                                        offset_io + mc->dev[idx].addr, 5);
             create_unimplemented_device("avr-timer8-intmask",
-                                        OFFSET_DATA
+                                        offset_io
                                         + mc->dev[idx].intmask_addr, 1);
             create_unimplemented_device("avr-timer8-intflag",
-                                        OFFSET_DATA
+                                        offset_io
                                         + mc->dev[idx].intflag_addr, 1);
             continue;
         }
@@ -337,9 +351,9 @@ static void atmega_realize(DeviceState *dev, Error **errp)
                                  s->xtal_freq_hz, &error_abort);
         sbd = SYS_BUS_DEVICE(&s->timer[i]);
         sysbus_realize(sbd, &error_abort);
-        sysbus_mmio_map(sbd, 0, OFFSET_DATA + mc->dev[idx].addr);
-        sysbus_mmio_map(sbd, 1, OFFSET_DATA + mc->dev[idx].intmask_addr);
-        sysbus_mmio_map(sbd, 2, OFFSET_DATA + mc->dev[idx].intflag_addr);
+        sysbus_mmio_map(sbd, 0, offset_io + mc->dev[idx].addr);
+        sysbus_mmio_map(sbd, 1, offset_io + mc->dev[idx].intmask_addr);
+        sysbus_mmio_map(sbd, 2, offset_io + mc->dev[idx].intflag_addr);
         connect_peripheral_irq(mc, sbd, 0, cpudev, TIMER_CAPT_IRQ(i));
         connect_peripheral_irq(mc, sbd, 1, cpudev, TIMER_COMPA_IRQ(i));
         connect_peripheral_irq(mc, sbd, 2, cpudev, TIMER_COMPB_IRQ(i));
@@ -349,12 +363,12 @@ static void atmega_realize(DeviceState *dev, Error **errp)
         g_free(devname);
     }
 
-    create_unimplemented_device("avr-twi",          OFFSET_DATA + 0x0b8, 6);
-    create_unimplemented_device("avr-adc",          OFFSET_DATA + 0x078, 8);
-    create_unimplemented_device("avr-ext-mem-ctrl", OFFSET_DATA + 0x074, 2);
-    create_unimplemented_device("avr-watchdog",     OFFSET_DATA + 0x060, 1);
-    create_unimplemented_device("avr-spi",          OFFSET_DATA + 0x04c, 3);
-    create_unimplemented_device("avr-eeprom",       OFFSET_DATA + 0x03f, 3);
+    create_unimplemented_device("avr-twi",          offset_io + 0x0b8, 6);
+    create_unimplemented_device("avr-adc",          offset_io + 0x078, 8);
+    create_unimplemented_device("avr-ext-mem-ctrl", offset_io + 0x074, 2);
+    create_unimplemented_device("avr-watchdog",     offset_io + 0x060, 1);
+    create_unimplemented_device("avr-spi",          offset_io + 0x04c, 3);
+    create_unimplemented_device("avr-eeprom",       offset_io + 0x03f, 3);
 }
 
 static const Property atmega_props[] = {
-- 
2.43.0

[PATCH 15/17] hw/avr: Pass mcu_type to class_base_init via .class_data

[Richard Henderson]

We want to be able to do more common work on MachineClass.
Pass the class name as a string in .class_data.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/avr/arduino.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
index 48ef478346..29cb776848 100644
--- a/hw/avr/arduino.c
+++ b/hw/avr/arduino.c
@@ -69,6 +69,13 @@ static void arduino_machine_class_init(ObjectClass *oc, void *data)
     mc->no_parallel = 1;
 }
 
+static void arduino_machine_class_base_init(ObjectClass *oc, void *data)
+{
+    ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
+
+    amc->mcu_type = data;
+}
+
 static void arduino_duemilanove_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -80,7 +87,6 @@ static void arduino_duemilanove_class_init(ObjectClass *oc, void *data)
      */
     mc->desc        = "Arduino Duemilanove (ATmega168)",
     mc->alias       = "2009";
-    amc->mcu_type   = TYPE_ATMEGA168_MCU;
     amc->xtal_hz    = 16 * 1000 * 1000;
 };
 
@@ -95,7 +101,6 @@ static void arduino_uno_class_init(ObjectClass *oc, void *data)
      */
     mc->desc        = "Arduino UNO (ATmega328P)";
     mc->alias       = "uno";
-    amc->mcu_type   = TYPE_ATMEGA328_MCU;
     amc->xtal_hz    = 16 * 1000 * 1000;
 };
 
@@ -110,7 +115,6 @@ static void arduino_mega_class_init(ObjectClass *oc, void *data)
      */
     mc->desc        = "Arduino Mega (ATmega1280)";
     mc->alias       = "mega";
-    amc->mcu_type   = TYPE_ATMEGA1280_MCU;
     amc->xtal_hz    = 16 * 1000 * 1000;
 };
 
@@ -125,7 +129,6 @@ static void arduino_mega2560_class_init(ObjectClass *oc, void *data)
      */
     mc->desc        = "Arduino Mega 2560 (ATmega2560)";
     mc->alias       = "mega2560";
-    amc->mcu_type   = TYPE_ATMEGA2560_MCU;
     amc->xtal_hz    = 16 * 1000 * 1000; /* CSTCE16M0V53-R0 */
 };
 
@@ -134,24 +137,29 @@ static const TypeInfo arduino_machine_types[] = {
         .name          = MACHINE_TYPE_NAME("arduino-duemilanove"),
         .parent        = TYPE_ARDUINO_MACHINE,
         .class_init    = arduino_duemilanove_class_init,
+        .class_data    = (void *)TYPE_ATMEGA168_MCU,
     }, {
         .name          = MACHINE_TYPE_NAME("arduino-uno"),
         .parent        = TYPE_ARDUINO_MACHINE,
         .class_init    = arduino_uno_class_init,
+        .class_data    = (void *)TYPE_ATMEGA328_MCU,
     }, {
         .name          = MACHINE_TYPE_NAME("arduino-mega"),
         .parent        = TYPE_ARDUINO_MACHINE,
         .class_init    = arduino_mega_class_init,
+        .class_data    = (void *)TYPE_ATMEGA1280_MCU,
     }, {
         .name          = MACHINE_TYPE_NAME("arduino-mega-2560-v3"),
         .parent        = TYPE_ARDUINO_MACHINE,
         .class_init    = arduino_mega2560_class_init,
+        .class_data    = (void *)TYPE_ATMEGA2560_MCU,
     }, {
         .name           = TYPE_ARDUINO_MACHINE,
         .parent         = TYPE_MACHINE,
         .instance_size  = sizeof(ArduinoMachineState),
         .class_size     = sizeof(ArduinoMachineClass),
         .class_init     = arduino_machine_class_init,
+        .class_base_init = arduino_machine_class_base_init,
         .abstract       = true,
     }
 };
-- 
2.43.0

[PATCH 16/17] hw/avr: Move AtmegaMcuClass to atmega.h

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/avr/atmega.h | 20 ++++++++++++++++++++
 hw/avr/atmega.c | 22 +---------------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
index a99ee15c7e..f031e6c10a 100644
--- a/hw/avr/atmega.h
+++ b/hw/avr/atmega.h
@@ -23,6 +23,10 @@
 #define TYPE_ATMEGA1280_MCU "ATmega1280"
 #define TYPE_ATMEGA2560_MCU "ATmega2560"
 
+typedef struct AtmegaMcuClass AtmegaMcuClass;
+DECLARE_CLASS_CHECKERS(AtmegaMcuClass, ATMEGA_MCU,
+                       TYPE_ATMEGA_MCU)
+
 typedef struct AtmegaMcuState AtmegaMcuState;
 DECLARE_INSTANCE_CHECKER(AtmegaMcuState, ATMEGA_MCU,
                          TYPE_ATMEGA_MCU)
@@ -32,6 +36,22 @@ DECLARE_INSTANCE_CHECKER(AtmegaMcuState, ATMEGA_MCU,
 #define TIMER_MAX 6
 #define GPIO_MAX 12
 
+struct AtmegaMcuClass {
+    /*< private >*/
+    SysBusDeviceClass parent_class;
+    /*< public >*/
+    const char *uc_name;
+    const char *cpu_type;
+    size_t flash_size;
+    size_t eeprom_size;
+    size_t sram_size;
+    size_t io_size;
+    size_t gpio_count;
+    size_t adc_count;
+    const uint8_t *irq;
+    const struct peripheral_cfg *dev;
+};
+
 struct AtmegaMcuState {
     /*< private >*/
     SysBusDevice parent_obj;
diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
index d4fc9c4aee..96e36743bc 100644
--- a/hw/avr/atmega.c
+++ b/hw/avr/atmega.c
@@ -36,7 +36,7 @@ enum AtmegaPeripheral {
 #define TIMER(n)    (n + TIMER0)
 #define POWER(n)    (n + POWER0)
 
-typedef struct {
+typedef struct peripheral_cfg {
     uint16_t addr;
     enum AtmegaPeripheral power_index;
     uint8_t power_bit;
@@ -46,26 +46,6 @@ typedef struct {
     bool is_timer16;
 } peripheral_cfg;
 
-struct AtmegaMcuClass {
-    /*< private >*/
-    SysBusDeviceClass parent_class;
-    /*< public >*/
-    const char *uc_name;
-    const char *cpu_type;
-    size_t flash_size;
-    size_t eeprom_size;
-    size_t sram_size;
-    size_t io_size;
-    size_t gpio_count;
-    size_t adc_count;
-    const uint8_t *irq;
-    const peripheral_cfg *dev;
-};
-typedef struct AtmegaMcuClass AtmegaMcuClass;
-
-DECLARE_CLASS_CHECKERS(AtmegaMcuClass, ATMEGA_MCU,
-                       TYPE_ATMEGA_MCU)
-
 static const peripheral_cfg dev168_328[PERIFMAX] = {
     [USART0]        = {  0xc0, POWER0, 1 },
     [TIMER2]        = {  0xb0, POWER0, 6, 0x70, 0x37, false },
-- 
2.43.0

[PATCH 17/17] target/avr: Enable TARGET_PAGE_BITS_VARY

[Richard Henderson]

Increase TARGET_PHYS_ADDR_SPACE_BITS to allow flexibility in the page
size without triggering an assert.  Select the page size based on the
size of sram.  This leaves sram on exactly one page and minimizes the
number of pages required to span the flash.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/cpu-param.h | 11 +++++++++--
 hw/avr/arduino.c       | 15 +++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/target/avr/cpu-param.h b/target/avr/cpu-param.h
index f5248ce9e7..a18bf39bb9 100644
--- a/target/avr/cpu-param.h
+++ b/target/avr/cpu-param.h
@@ -21,8 +21,15 @@
 #ifndef AVR_CPU_PARAM_H
 #define AVR_CPU_PARAM_H
 
-#define TARGET_PAGE_BITS 10
-#define TARGET_PHYS_ADDR_SPACE_BITS 24
+#define TARGET_PAGE_BITS_VARY
+#define TARGET_PAGE_BITS_MIN 10
+
+/*
+ * The real value for TARGET_PHYS_ADDR_SPACE_BITS is 24, but selecting
+ * an overly small value will assert in tb-maint.c when selecting the
+ * shape of the page_table tree.  This allows an 8k page size.
+ */
+#define TARGET_PHYS_ADDR_SPACE_BITS 28
 #define TARGET_VIRT_ADDR_SPACE_BITS 24
 
 #define TCG_GUEST_DEFAULT_MO 0
diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
index 29cb776848..f309aa5597 100644
--- a/hw/avr/arduino.c
+++ b/hw/avr/arduino.c
@@ -71,9 +71,24 @@ static void arduino_machine_class_init(ObjectClass *oc, void *data)
 
 static void arduino_machine_class_base_init(ObjectClass *oc, void *data)
 {
+    MachineClass *mc = MACHINE_CLASS(oc);
     ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
+    AtmegaMcuClass *acc;
+    int page_bits;
 
     amc->mcu_type = data;
+
+    /* Find the mcu class that we will instantiate. */
+    acc = ATMEGA_MCU_CLASS(object_class_by_name(amc->mcu_type));
+
+    /*
+     * Select a page size based on the size of sram.
+     * This will result in a page size between 1k and 8k
+     * and minimize the number of pages to span flash.
+     */
+    page_bits = ctz32(acc->sram_size);
+    assert(page_bits >= TARGET_PAGE_BITS_MIN && page_bits <= 13);
+    mc->minimum_page_bits = page_bits;
 }
 
 static void arduino_duemilanove_class_init(ObjectClass *oc, void *data)
-- 
2.43.0

Re: [PATCH 01/17] hw/core/cpu: Use size_t for memory_rw_debug len argument

[Philippe Mathieu-Daudé]

On 23/3/25 18:37, Richard Henderson wrote:
> Match the prototype of cpu_memory_rw_debug().
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/hw/core/cpu.h     | 2 +-
>   target/sparc/cpu.h        | 2 +-
>   target/sparc/mmu_helper.c | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 04/17] target/avr: Remove OFFSET_CPU_REGISTERS

[Philippe Mathieu-Daudé]

On 23/3/25 18:37, Richard Henderson wrote:
> This define isn't really used.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/cpu.h    | 2 --
>   target/avr/helper.c | 3 +--
>   2 files changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 07/17] target/avr: Use do_stb in avr_cpu_do_interrupt

[Philippe Mathieu-Daudé]

On 23/3/25 18:37, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/helper.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 11/17] target/avr: Implement CPUState.memory_rw_debug

[Philippe Mathieu-Daudé]

On 23/3/25 18:37, Richard Henderson wrote:
> Prepare for offset_io being non-zero when accessing from gdb.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/cpu.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 12/17] target/avr: Handle offset_io in helper.c

[Philippe Mathieu-Daudé]

On 23/3/25 18:37, Richard Henderson wrote:
> Prepare for offset_io being non-zero in do_stb.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/helper.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 13/17] target/avr: Handle offset_io in avr_cpu_realizefn

[Philippe Mathieu-Daudé]

On 23/3/25 18:37, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/cpu.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index e4011004b4..538fcbc215 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -161,12 +161,14 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
>       memory_region_init_io(&cpu->cpu_reg1, OBJECT(cpu), &avr_cpu_reg1, env,
>                             "avr-cpu-reg1", 32);
>       memory_region_add_subregion(get_system_memory(),
> -                                OFFSET_DATA, &cpu->cpu_reg1);
> +                                OFFSET_DATA + cpu->offset_io,
> +                                &cpu->cpu_reg1);
>   
>       memory_region_init_io(&cpu->cpu_reg2, OBJECT(cpu), &avr_cpu_reg2, env,
>                             "avr-cpu-reg2", 8);
>       memory_region_add_subregion(get_system_memory(),
> -                                OFFSET_DATA + 0x58, &cpu->cpu_reg2);
> +                                OFFSET_DATA + cpu->offset_io + 0x58,
> +                                &cpu->cpu_reg2);

Always zero, but I agree it is clearer.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   }

Re: [PATCH 13/17] target/avr: Handle offset_io in avr_cpu_realizefn

[Philippe Mathieu-Daudé]

On 23/3/25 22:35, Philippe Mathieu-Daudé wrote:
> On 23/3/25 18:37, Richard Henderson wrote:
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/avr/cpu.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
>> index e4011004b4..538fcbc215 100644
>> --- a/target/avr/cpu.c
>> +++ b/target/avr/cpu.c
>> @@ -161,12 +161,14 @@ static void avr_cpu_realizefn(DeviceState *dev, 
>> Error **errp)
>>       memory_region_init_io(&cpu->cpu_reg1, OBJECT(cpu), 
>> &avr_cpu_reg1, env,
>>                             "avr-cpu-reg1", 32);
>>       memory_region_add_subregion(get_system_memory(),
>> -                                OFFSET_DATA, &cpu->cpu_reg1);
>> +                                OFFSET_DATA + cpu->offset_io,
>> +                                &cpu->cpu_reg1);
>>       memory_region_init_io(&cpu->cpu_reg2, OBJECT(cpu), 
>> &avr_cpu_reg2, env,
>>                             "avr-cpu-reg2", 8);
>>       memory_region_add_subregion(get_system_memory(),
>> -                                OFFSET_DATA + 0x58, &cpu->cpu_reg2);
>> +                                OFFSET_DATA + cpu->offset_io + 0x58,
>> +                                &cpu->cpu_reg2);
> 
> Always zero, but I agree it is clearer.

I figured out the use after reading the following patch ;)

> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>>   }

Re: [PATCH 15/17] hw/avr: Pass mcu_type to class_base_init via .class_data

[Philippe Mathieu-Daudé]

On 23/3/25 18:37, Richard Henderson wrote:
> We want to be able to do more common work on MachineClass.
> Pass the class name as a string in .class_data.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/avr/arduino.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 01/17] hw/core/cpu: Use size_t for memory_rw_debug len argument

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Match the prototype of cpu_memory_rw_debug().
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/hw/core/cpu.h     | 2 +-
>   target/sparc/cpu.h        | 2 +-
>   target/sparc/mmu_helper.c | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 5d11d26556..abd8764e83 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -154,7 +154,7 @@ struct CPUClass {
>   
>       int (*mmu_index)(CPUState *cpu, bool ifetch);
>       int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
> -                           uint8_t *buf, int len, bool is_write);
> +                           uint8_t *buf, size_t len, bool is_write);
>       void (*dump_state)(CPUState *cpu, FILE *, int flags);
>       void (*query_cpu_fast)(CPUState *cpu, CpuInfoFast *value);
>       int64_t (*get_arch_id)(CPUState *cpu);
> diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
> index 462bcb6c0e..68f8c21e7c 100644
> --- a/target/sparc/cpu.h
> +++ b/target/sparc/cpu.h
> @@ -604,7 +604,7 @@ void dump_mmu(CPUSPARCState *env);
>   
>   #if !defined(TARGET_SPARC64) && !defined(CONFIG_USER_ONLY)
>   int sparc_cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
> -                              uint8_t *buf, int len, bool is_write);
> +                              uint8_t *buf, size_t len, bool is_write);
>   #endif
>   
>   /* translate.c */
> diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
> index 7548d01777..3821cd91ec 100644
> --- a/target/sparc/mmu_helper.c
> +++ b/target/sparc/mmu_helper.c
> @@ -389,7 +389,7 @@ void dump_mmu(CPUSPARCState *env)
>    * that the sparc ABI is followed.
>    */
>   int sparc_cpu_memory_rw_debug(CPUState *cs, vaddr address,
> -                              uint8_t *buf, int len, bool is_write)
> +                              uint8_t *buf, size_t len, bool is_write)
>   {
>       CPUSPARCState *env = cpu_env(cs);
>       target_ulong addr = address;

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 02/17] target/avr: Fix buffer read in avr_print_insn

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Do not unconditionally attempt to read 4 bytes, as there
> may only be 2 bytes remaining in the translator cache.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/disas.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/target/avr/disas.c b/target/avr/disas.c
> index b7689e8d7c..d341030174 100644
> --- a/target/avr/disas.c
> +++ b/target/avr/disas.c
> @@ -68,28 +68,35 @@ static bool decode_insn(DisasContext *ctx, uint16_t insn);
>   
>   int avr_print_insn(bfd_vma addr, disassemble_info *info)
>   {
> -    DisasContext ctx;
> +    DisasContext ctx = { info };
>       DisasContext *pctx = &ctx;
>       bfd_byte buffer[4];
>       uint16_t insn;
>       int status;
>   
> -    ctx.info = info;
> -
> -    status = info->read_memory_func(addr, buffer, 4, info);
> +    status = info->read_memory_func(addr, buffer, 2, info);
>       if (status != 0) {
>           info->memory_error_func(status, addr, info);
>           return -1;
>       }
>       insn = bfd_getl16(buffer);
> -    ctx.next_word = bfd_getl16(buffer + 2);
> -    ctx.next_word_used = false;
> +
> +    status = info->read_memory_func(addr + 2, buffer + 2, 2, info);
> +    if (status == 0) {
> +        ctx.next_word = bfd_getl16(buffer + 2);
> +    }
>   
>       if (!decode_insn(&ctx, insn)) {
>           output(".db", "0x%02x, 0x%02x", buffer[0], buffer[1]);
>       }
>   
> -    return ctx.next_word_used ? 4 : 2;
> +    if (!ctx.next_word_used) {
> +        return 2;
> +    } else if (status == 0) {
> +        return 4;
> +    }
> +    info->memory_error_func(status, addr + 2, info);
> +    return -1;
>   }
>   
>   

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 03/17] target/avr: Improve decode of LDS, STS

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> The comment about not being able to define a field with
> zero bits is out of date since 94597b6146f3
> ("decodetree: Allow !function with no input bits").
> 
> This fixes the missing load of imm in the disassembler.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 9d8caa67a24 ("target/avr: Add support for disassembling via option '-d in_asm'")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/translate.c | 2 --
>   target/avr/insn.decode | 7 ++-----
>   2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/target/avr/translate.c b/target/avr/translate.c
> index 4ab71d8138..e7f8ced9b3 100644
> --- a/target/avr/translate.c
> +++ b/target/avr/translate.c
> @@ -1578,7 +1578,6 @@ static bool trans_LDS(DisasContext *ctx, arg_LDS *a)
>       TCGv Rd = cpu_r[a->rd];
>       TCGv addr = tcg_temp_new_i32();
>       TCGv H = cpu_rampD;
> -    a->imm = next_word(ctx);
>   
>       tcg_gen_mov_tl(addr, H); /* addr = H:M:L */
>       tcg_gen_shli_tl(addr, addr, 16);
> @@ -1783,7 +1782,6 @@ static bool trans_STS(DisasContext *ctx, arg_STS *a)
>       TCGv Rd = cpu_r[a->rd];
>       TCGv addr = tcg_temp_new_i32();
>       TCGv H = cpu_rampD;
> -    a->imm = next_word(ctx);
>   
>       tcg_gen_mov_tl(addr, H); /* addr = H:M:L */
>       tcg_gen_shli_tl(addr, addr, 16);
> diff --git a/target/avr/insn.decode b/target/avr/insn.decode
> index 482c23ad0c..cc302249db 100644
> --- a/target/avr/insn.decode
> +++ b/target/avr/insn.decode
> @@ -118,11 +118,8 @@ BRBC            1111 01 ....... ...         @op_bit_imm
>   @io_rd_imm      .... . .. ..... ....        &rd_imm     rd=%rd imm=%io_imm
>   @ldst_d         .. . . .. . rd:5  . ...     &rd_imm     imm=%ldst_d_imm
>   
> -# The 16-bit immediate is completely in the next word.
> -# Fields cannot be defined with no bits, so we cannot play
> -# the same trick and append to a zero-bit value.
> -# Defer reading the immediate until trans_{LDS,STS}.
> -@ldst_s         .... ... rd:5 ....          imm=0
> +%ldst_imm       !function=next_word
> +@ldst_s         .... ... rd:5 ....          imm=%ldst_imm
>   
>   MOV             0010 11 . ..... ....        @op_rd_rr
>   MOVW            0000 0001 .... ....         &rd_rr      rd=%rd_d rr=%rr_d

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 04/17] target/avr: Remove OFFSET_CPU_REGISTERS

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> This define isn't really used.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/cpu.h    | 2 --
>   target/avr/helper.c | 3 +--
>   2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/target/avr/cpu.h b/target/avr/cpu.h
> index 06f5ae4d1b..84a8f5cc8c 100644
> --- a/target/avr/cpu.h
> +++ b/target/avr/cpu.h
> @@ -60,8 +60,6 @@
>   #define OFFSET_CODE 0x00000000
>   /* CPU registers, IO registers, and SRAM */
>   #define OFFSET_DATA 0x00800000
> -/* CPU registers specifically, these are mapped at the start of data */
> -#define OFFSET_CPU_REGISTERS OFFSET_DATA
>   /*
>    * IO registers, including status register, stack pointer, and memory
>    * mapped peripherals, mapped just after CPU registers
> diff --git a/target/avr/helper.c b/target/avr/helper.c
> index 3412312ad5..e5bf16c6b7 100644
> --- a/target/avr/helper.c
> +++ b/target/avr/helper.c
> @@ -340,8 +340,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr)
>       env->fullacc = false;
>   
>       /* Following logic assumes this: */
> -    assert(OFFSET_CPU_REGISTERS == OFFSET_DATA);
> -    assert(OFFSET_IO_REGISTERS == OFFSET_CPU_REGISTERS +
> +    assert(OFFSET_IO_REGISTERS == OFFSET_DATA +
>                                     NUMBER_OF_CPU_REGISTERS);
>   
>       if (addr < NUMBER_OF_CPU_REGISTERS) {

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 05/17] target/avr: Move cpu register accesses into system memory

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Integrate the i/o 0x00-0x1f and 0x38-0x3f loopbacks into
> the cpu registers with normal address space accesses.
> We no longer need to trap accesses to the first page within
> avr_cpu_tlb_fill but can wait until a write occurs.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/cpu.h       |   7 ++
>   target/avr/helper.h    |   3 -
>   target/avr/cpu.c       |  16 +++
>   target/avr/helper.c    | 239 +++++++++++++++++------------------------
>   target/avr/translate.c |  42 ++++----
>   5 files changed, 146 insertions(+), 161 deletions(-)
> 
> diff --git a/target/avr/cpu.h b/target/avr/cpu.h
> index 84a8f5cc8c..be27b0152b 100644
> --- a/target/avr/cpu.h
> +++ b/target/avr/cpu.h
> @@ -23,6 +23,7 @@
>   
>   #include "cpu-qom.h"
>   #include "exec/cpu-defs.h"
> +#include "exec/memory.h"
>   
>   #ifdef CONFIG_USER_ONLY
>   #error "AVR 8-bit does not support user mode"
> @@ -142,6 +143,9 @@ struct ArchCPU {
>   
>       CPUAVRState env;
>   
> +    MemoryRegion cpu_reg1;
> +    MemoryRegion cpu_reg2;
> +
>       /* Initial value of stack pointer */
>       uint32_t init_sp;
>   };
> @@ -242,6 +246,9 @@ bool avr_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                         MMUAccessType access_type, int mmu_idx,
>                         bool probe, uintptr_t retaddr);
>   
> +extern const MemoryRegionOps avr_cpu_reg1;
> +extern const MemoryRegionOps avr_cpu_reg2;
> +
>   #include "exec/cpu-all.h"
>   
>   #endif /* QEMU_AVR_CPU_H */
> diff --git a/target/avr/helper.h b/target/avr/helper.h
> index 4d02e648fa..e8d13e925f 100644
> --- a/target/avr/helper.h
> +++ b/target/avr/helper.h
> @@ -23,7 +23,4 @@ DEF_HELPER_1(debug, noreturn, env)
>   DEF_HELPER_1(break, noreturn, env)
>   DEF_HELPER_1(sleep, noreturn, env)
>   DEF_HELPER_1(unsupported, noreturn, env)
> -DEF_HELPER_3(outb, void, env, i32, i32)
> -DEF_HELPER_2(inb, tl, env, i32)
>   DEF_HELPER_3(fullwr, void, env, i32, i32)
> -DEF_HELPER_2(fullrd, tl, env, i32)
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 834c7082aa..0b14b36c17 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -23,6 +23,7 @@
>   #include "qemu/qemu-print.h"
>   #include "exec/exec-all.h"
>   #include "exec/translation-block.h"
> +#include "exec/address-spaces.h"
>   #include "cpu.h"
>   #include "disas/dis-asm.h"
>   #include "tcg/debug-assert.h"
> @@ -110,6 +111,8 @@ static void avr_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
>   static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
>   {
>       CPUState *cs = CPU(dev);
> +    CPUAVRState *env = cpu_env(cs);
> +    AVRCPU *cpu = env_archcpu(env);
>       AVRCPUClass *mcc = AVR_CPU_GET_CLASS(dev);
>       Error *local_err = NULL;
>   
> @@ -122,6 +125,19 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
>       cpu_reset(cs);
>   
>       mcc->parent_realize(dev, errp);
> +
> +    /*
> +     * Two blocks in the low data space loop back into cpu registers.
> +     */
> +    memory_region_init_io(&cpu->cpu_reg1, OBJECT(cpu), &avr_cpu_reg1, env,
> +                          "avr-cpu-reg1", 32);
> +    memory_region_add_subregion(get_system_memory(),
> +                                OFFSET_DATA, &cpu->cpu_reg1);
> +
> +    memory_region_init_io(&cpu->cpu_reg2, OBJECT(cpu), &avr_cpu_reg2, env,
> +                          "avr-cpu-reg2", 8);
> +    memory_region_add_subregion(get_system_memory(),
> +                                OFFSET_DATA + 0x58, &cpu->cpu_reg2);
>   }
>   
>   static void avr_cpu_set_int(void *opaque, int irq, int level)
> diff --git a/target/avr/helper.c b/target/avr/helper.c
> index e5bf16c6b7..df7e2109d4 100644
> --- a/target/avr/helper.c
> +++ b/target/avr/helper.c
> @@ -108,7 +108,7 @@ bool avr_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                         MMUAccessType access_type, int mmu_idx,
>                         bool probe, uintptr_t retaddr)
>   {
> -    int prot, page_size = TARGET_PAGE_SIZE;
> +    int prot;
>       uint32_t paddr;
>   
>       address &= TARGET_PAGE_MASK;
> @@ -133,23 +133,9 @@ bool avr_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>           /* Access to memory. */
>           paddr = OFFSET_DATA + address;
>           prot = PAGE_READ | PAGE_WRITE;
> -        if (address < NUMBER_OF_CPU_REGISTERS + NUMBER_OF_IO_REGISTERS) {
> -            /*
> -             * Access to CPU registers, exit and rebuilt this TB to use
> -             * full access in case it touches specially handled registers
> -             * like SREG or SP.  For probing, set page_size = 1, in order
> -             * to force tlb_fill to be called for the next access.
> -             */
> -            if (probe) {
> -                page_size = 1;
> -            } else {
> -                cpu_env(cs)->fullacc = 1;
> -                cpu_loop_exit_restore(cs, retaddr);
> -            }
> -        }
>       }
>   
> -    tlb_set_page(cs, address, paddr, prot, mmu_idx, page_size);
> +    tlb_set_page(cs, address, paddr, prot, mmu_idx, TARGET_PAGE_SIZE);
>       return true;
>   }
>   
> @@ -203,134 +189,78 @@ void helper_wdr(CPUAVRState *env)
>   }
>   
>   /*
> - * 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
> - *
> + * The first 32 bytes of the data space are mapped to the cpu regs.
> + * We cannot write these from normal store operations because TCG
> + * does not expect global temps to be modified -- a global may be
> + * live in a host cpu register across the store.  We can however
> + * read these, as TCG does make sure the global temps are saved
> + * in case the load operation traps.
>    */
> -target_ulong helper_inb(CPUAVRState *env, uint32_t port)
> +
> +static uint64_t avr_cpu_reg1_read(void *opaque, hwaddr addr, unsigned size)
>   {
> -    target_ulong data = 0;
> +    CPUAVRState *env = opaque;
>   
> -    switch (port) {
> -    case 0x38: /* RAMPD */
> -        data = 0xff & (env->rampD >> 16);
> -        break;
> -    case 0x39: /* RAMPX */
> -        data = 0xff & (env->rampX >> 16);
> -        break;
> -    case 0x3a: /* RAMPY */
> -        data = 0xff & (env->rampY >> 16);
> -        break;
> -    case 0x3b: /* RAMPZ */
> -        data = 0xff & (env->rampZ >> 16);
> -        break;
> -    case 0x3c: /* EIND */
> -        data = 0xff & (env->eind >> 16);
> -        break;
> -    case 0x3d: /* SPL */
> -        data = env->sp & 0x00ff;
> -        break;
> -    case 0x3e: /* SPH */
> -        data = env->sp >> 8;
> -        break;
> -    case 0x3f: /* SREG */
> -        data = cpu_get_sreg(env);
> -        break;
> -    default:
> -        /* not a special register, pass to normal memory access */
> -        data = address_space_ldub(&address_space_memory,
> -                                  OFFSET_IO_REGISTERS + port,
> -                                  MEMTXATTRS_UNSPECIFIED, NULL);
> -    }
> -
> -    return data;
> +    assert(addr < 32);
> +    return env->r[addr];
>   }
>   
>   /*
> - *  This function implements OUT instruction
> - *
> - *  It does the following
> - *  a.  if an IO register belongs to CPU, its value is written into the register
> - *  b.  otherwise io address is translated to mem address and physical memory
> - *      is written.
> - *  c.  it caches the value for sake of SBI, SBIC, SBIS & CBI implementation
> - *
> + * The range 0x58-0x5f of the data space are mapped to cpu regs.
> + * As above, we cannot write these from normal store operations.
>    */
> -void helper_outb(CPUAVRState *env, uint32_t port, uint32_t data)
> -{
> -    data &= 0x000000ff;
>   
> -    switch (port) {
> -    case 0x38: /* RAMPD */
> -        if (avr_feature(env, AVR_FEATURE_RAMPD)) {
> -            env->rampD = (data & 0xff) << 16;
> -        }
> -        break;
> -    case 0x39: /* RAMPX */
> -        if (avr_feature(env, AVR_FEATURE_RAMPX)) {
> -            env->rampX = (data & 0xff) << 16;
> -        }
> -        break;
> -    case 0x3a: /* RAMPY */
> -        if (avr_feature(env, AVR_FEATURE_RAMPY)) {
> -            env->rampY = (data & 0xff) << 16;
> -        }
> -        break;
> -    case 0x3b: /* RAMPZ */
> -        if (avr_feature(env, AVR_FEATURE_RAMPZ)) {
> -            env->rampZ = (data & 0xff) << 16;
> -        }
> -        break;
> -    case 0x3c: /* EIDN */
> -        env->eind = (data & 0xff) << 16;
> -        break;
> -    case 0x3d: /* SPL */
> -        env->sp = (env->sp & 0xff00) | (data);
> -        break;
> -    case 0x3e: /* SPH */
> -        if (avr_feature(env, AVR_FEATURE_2_BYTE_SP)) {
> -            env->sp = (env->sp & 0x00ff) | (data << 8);
> -        }
> -        break;
> -    case 0x3f: /* SREG */
> -        cpu_set_sreg(env, data);
> -        break;
> -    default:
> -        /* not a special register, pass to normal memory access */
> -        address_space_stb(&address_space_memory, OFFSET_IO_REGISTERS + port,
> -                          data, MEMTXATTRS_UNSPECIFIED, NULL);
> +static uint64_t avr_cpu_reg2_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    CPUAVRState *env = opaque;
> +
> +    switch (addr) {
> +    case 0: /* RAMPD */
> +        return 0xff & (env->rampD >> 16);
> +    case 1: /* RAMPX */
> +        return 0xff & (env->rampX >> 16);
> +    case 2: /* RAMPY */
> +        return 0xff & (env->rampY >> 16);
> +    case 3: /* RAMPZ */
> +        return 0xff & (env->rampZ >> 16);
> +    case 4: /* EIND */
> +        return 0xff & (env->eind >> 16);
> +    case 5: /* SPL */
> +        return env->sp & 0x00ff;
> +    case 6: /* SPH */
> +        return 0xff & (env->sp >> 8);
> +    case 7: /* SREG */
> +        return cpu_get_sreg(env);
>       }
> +    g_assert_not_reached();
>   }
>   
> -/*
> - *  this function implements LD instruction when there is a possibility to read
> - *  from a CPU register
> - */
> -target_ulong helper_fullrd(CPUAVRState *env, uint32_t addr)
> +static void avr_cpu_trap_write(void *opaque, hwaddr addr,
> +                               uint64_t data64, unsigned size)
>   {
> -    uint8_t data;
> +    CPUAVRState *env = opaque;
> +    CPUState *cs = env_cpu(env);
>   
> -    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 */
> -        data = address_space_ldub(&address_space_memory, OFFSET_DATA + addr,
> -                                  MEMTXATTRS_UNSPECIFIED, NULL);
> -    }
> -    return data;
> +    env->fullacc = true;
> +    cpu_loop_exit_restore(cs, cs->mem_io_pc);
>   }
>   
> +const MemoryRegionOps avr_cpu_reg1 = {
> +    .read = avr_cpu_reg1_read,
> +    .write = avr_cpu_trap_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 1,
> +};
> +
> +const MemoryRegionOps avr_cpu_reg2 = {
> +    .read = avr_cpu_reg2_read,
> +    .write = avr_cpu_trap_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 1,
> +};
> +
>   /*
>    *  this function implements ST instruction when there is a possibility to write
>    *  into a CPU register
> @@ -339,19 +269,50 @@ void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr)
>   {
>       env->fullacc = false;
>   
> -    /* Following logic assumes this: */
> -    assert(OFFSET_IO_REGISTERS == OFFSET_DATA +
> -                                  NUMBER_OF_CPU_REGISTERS);
> -
> -    if (addr < NUMBER_OF_CPU_REGISTERS) {
> +    switch (addr) {
> +    case 0 ... 31:
>           /* CPU registers */
>           env->r[addr] = data;
> -    } else if (addr < NUMBER_OF_CPU_REGISTERS + NUMBER_OF_IO_REGISTERS) {
> -        /* IO registers */
> -        helper_outb(env, addr - NUMBER_OF_CPU_REGISTERS, data);
> -    } else {
> -        /* memory */
> +        break;
> +
> +    case 0x58: /* RAMPD */
> +        if (avr_feature(env, AVR_FEATURE_RAMPD)) {
> +            env->rampD = data << 16;
> +        }
> +        break;
> +    case 0x59: /* RAMPX */
> +        if (avr_feature(env, AVR_FEATURE_RAMPX)) {
> +            env->rampX = data << 16;
> +        }
> +        break;
> +    case 0x5a: /* RAMPY */
> +        if (avr_feature(env, AVR_FEATURE_RAMPY)) {
> +            env->rampY = data << 16;
> +        }
> +        break;
> +    case 0x5b: /* RAMPZ */
> +        if (avr_feature(env, AVR_FEATURE_RAMPZ)) {
> +            env->rampZ = data << 16;
> +        }
> +        break;
> +    case 0x5c: /* EIDN */
> +        env->eind = data << 16;
> +        break;
> +    case 0x5d: /* SPL */
> +        env->sp = (env->sp & 0xff00) | data;
> +        break;
> +    case 0x5e: /* SPH */
> +        if (avr_feature(env, AVR_FEATURE_2_BYTE_SP)) {
> +            env->sp = (env->sp & 0x00ff) | (data << 8);
> +        }
> +        break;
> +    case 0x5f: /* SREG */
> +        cpu_set_sreg(env, data);
> +        break;
> +
> +    default:
>           address_space_stb(&address_space_memory, OFFSET_DATA + addr, data,
>                             MEMTXATTRS_UNSPECIFIED, NULL);
> +        break;
>       }
>   }
> diff --git a/target/avr/translate.c b/target/avr/translate.c
> index e7f8ced9b3..0490936cd5 100644
> --- a/target/avr/translate.c
> +++ b/target/avr/translate.c
> @@ -194,6 +194,9 @@ static bool avr_have_feature(DisasContext *ctx, int feature)
>   static bool decode_insn(DisasContext *ctx, uint16_t insn);
>   #include "decode-insn.c.inc"
>   
> +static void gen_inb(DisasContext *ctx, TCGv data, int port);
> +static void gen_outb(DisasContext *ctx, TCGv data, int port);
> +
>   /*
>    * Arithmetic Instructions
>    */
> @@ -1293,9 +1296,8 @@ static bool trans_SBRS(DisasContext *ctx, arg_SBRS *a)
>   static bool trans_SBIC(DisasContext *ctx, arg_SBIC *a)
>   {
>       TCGv data = tcg_temp_new_i32();
> -    TCGv port = tcg_constant_i32(a->reg);
>   
> -    gen_helper_inb(data, tcg_env, port);
> +    gen_inb(ctx, data, a->reg);
>       tcg_gen_andi_tl(data, data, 1 << a->bit);
>       ctx->skip_cond = TCG_COND_EQ;
>       ctx->skip_var0 = data;
> @@ -1311,9 +1313,8 @@ static bool trans_SBIC(DisasContext *ctx, arg_SBIC *a)
>   static bool trans_SBIS(DisasContext *ctx, arg_SBIS *a)
>   {
>       TCGv data = tcg_temp_new_i32();
> -    TCGv port = tcg_constant_i32(a->reg);
>   
> -    gen_helper_inb(data, tcg_env, port);
> +    gen_inb(ctx, data, a->reg);
>       tcg_gen_andi_tl(data, data, 1 << a->bit);
>       ctx->skip_cond = TCG_COND_NE;
>       ctx->skip_var0 = data;
> @@ -1502,11 +1503,18 @@ static void gen_data_store(DisasContext *ctx, TCGv data, TCGv addr)
>   
>   static void gen_data_load(DisasContext *ctx, TCGv data, TCGv addr)
>   {
> -    if (ctx->base.tb->flags & TB_FLAGS_FULL_ACCESS) {
> -        gen_helper_fullrd(data, tcg_env, addr);
> -    } else {
> -        tcg_gen_qemu_ld_tl(data, addr, MMU_DATA_IDX, MO_UB);
> -    }
> +    tcg_gen_qemu_ld_tl(data, addr, MMU_DATA_IDX, MO_UB);
> +}
> +
> +static void gen_inb(DisasContext *ctx, TCGv data, int port)
> +{
> +    gen_data_load(ctx, data, tcg_constant_i32(port + NUMBER_OF_CPU_REGISTERS));
> +}
> +
> +static void gen_outb(DisasContext *ctx, TCGv data, int port)
> +{
> +    gen_helper_fullwr(tcg_env, data,
> +                      tcg_constant_i32(port + NUMBER_OF_CPU_REGISTERS));
>   }
>   
>   /*
> @@ -2126,9 +2134,8 @@ static bool trans_SPMX(DisasContext *ctx, arg_SPMX *a)
>   static bool trans_IN(DisasContext *ctx, arg_IN *a)
>   {
>       TCGv Rd = cpu_r[a->rd];
> -    TCGv port = tcg_constant_i32(a->imm);
>   
> -    gen_helper_inb(Rd, tcg_env, port);
> +    gen_inb(ctx, Rd, a->imm);
>       return true;
>   }
>   
> @@ -2139,9 +2146,8 @@ static bool trans_IN(DisasContext *ctx, arg_IN *a)
>   static bool trans_OUT(DisasContext *ctx, arg_OUT *a)
>   {
>       TCGv Rd = cpu_r[a->rd];
> -    TCGv port = tcg_constant_i32(a->imm);
>   
> -    gen_helper_outb(tcg_env, port, Rd);
> +    gen_outb(ctx, Rd, a->imm);
>       return true;
>   }
>   
> @@ -2407,11 +2413,10 @@ static bool trans_SWAP(DisasContext *ctx, arg_SWAP *a)
>   static bool trans_SBI(DisasContext *ctx, arg_SBI *a)
>   {
>       TCGv data = tcg_temp_new_i32();
> -    TCGv port = tcg_constant_i32(a->reg);
>   
> -    gen_helper_inb(data, tcg_env, port);
> +    gen_inb(ctx, data, a->reg);
>       tcg_gen_ori_tl(data, data, 1 << a->bit);
> -    gen_helper_outb(tcg_env, port, data);
> +    gen_outb(ctx, data, a->reg);
>       return true;
>   }
>   
> @@ -2422,11 +2427,10 @@ static bool trans_SBI(DisasContext *ctx, arg_SBI *a)
>   static bool trans_CBI(DisasContext *ctx, arg_CBI *a)
>   {
>       TCGv data = tcg_temp_new_i32();
> -    TCGv port = tcg_constant_i32(a->reg);
>   
> -    gen_helper_inb(data, tcg_env, port);
> +    gen_inb(ctx, data, a->reg);
>       tcg_gen_andi_tl(data, data, ~(1 << a->bit));
> -    gen_helper_outb(tcg_env, port, data);
> +    gen_outb(ctx, data, a->reg);
>       return true;
>   }
>   

A simple nit, maybe we could define constant for register names. This 
way, it can be used in the two switch for read/access.

By the way, are there other architectures where cpu registers are mapped 
in memory like AVR?

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 06/17] target/avr: Use cpu_stb_mmuidx_ra in helper_fullwr

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Avoid direct use of address_space_memory.
> Make use of the softmmu cache of the i/o page.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/helper.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 07/17] target/avr: Use do_stb in avr_cpu_do_interrupt

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/helper.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target/avr/helper.c b/target/avr/helper.c
> index 7cfd3d1093..9608e59584 100644
> --- a/target/avr/helper.c
> +++ b/target/avr/helper.c
> @@ -88,14 +88,14 @@ void avr_cpu_do_interrupt(CPUState *cs)
>       }
>   
>       if (avr_feature(env, AVR_FEATURE_3_BYTE_PC)) {
> -        cpu_stb_data(env, env->sp--, (ret & 0x0000ff));
> -        cpu_stb_data(env, env->sp--, (ret & 0x00ff00) >> 8);
> -        cpu_stb_data(env, env->sp--, (ret & 0xff0000) >> 16);
> +        do_stb(env, env->sp--, ret, 0);
> +        do_stb(env, env->sp--, ret >> 8, 0);
> +        do_stb(env, env->sp--, ret >> 16, 0);
>       } else if (avr_feature(env, AVR_FEATURE_2_BYTE_PC)) {
> -        cpu_stb_data(env, env->sp--, (ret & 0x0000ff));
> -        cpu_stb_data(env, env->sp--, (ret & 0x00ff00) >> 8);
> +        do_stb(env, env->sp--, ret, 0);
> +        do_stb(env, env->sp--, ret >> 8, 0);
>       } else {
> -        cpu_stb_data(env, env->sp--, (ret & 0x0000ff));
> +        do_stb(env, env->sp--, ret, 0);
>       }
>   
>       env->pc_w = base + vector * size;

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 08/17] target/avr: Add offset-io cpu property

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Communicate the offset of io within the first page between the
> board, the cpu, and the translator.  So far this is always 0.
> This will be used to optimize memory layout.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/cpu.h       | 2 ++
>   hw/avr/atmega.c        | 2 ++
>   target/avr/cpu.c       | 1 +
>   target/avr/translate.c | 2 ++
>   4 files changed, 7 insertions(+)
> 
> diff --git a/target/avr/cpu.h b/target/avr/cpu.h
> index be27b0152b..0f5e1a53bc 100644
> --- a/target/avr/cpu.h
> +++ b/target/avr/cpu.h
> @@ -148,6 +148,8 @@ struct ArchCPU {
>   
>       /* Initial value of stack pointer */
>       uint32_t init_sp;
> +    /* Offset of the beginning of I/O within the first page. */
> +    uint32_t offset_io;
>   };
>   
>   /**
> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
> index f6844bf118..273582b8af 100644
> --- a/hw/avr/atmega.c
> +++ b/hw/avr/atmega.c
> @@ -236,6 +236,8 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>   
>       object_property_set_uint(OBJECT(&s->cpu), "init-sp",
>                                mc->io_size + mc->sram_size - 1, &error_abort);
> +    object_property_set_uint(OBJECT(&s->cpu), "offset-io",
> +                             0, &error_abort);
>   
>       qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
>       cpudev = DEVICE(&s->cpu);
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 0b14b36c17..080f6f30d3 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -169,6 +169,7 @@ static void avr_cpu_initfn(Object *obj)
>   
>   static const Property avr_cpu_properties[] = {
>       DEFINE_PROP_UINT32("init-sp", AVRCPU, init_sp, 0),
> +    DEFINE_PROP_UINT32("offset-io", AVRCPU, offset_io, 0),
>   };
>   
>   static ObjectClass *avr_cpu_class_by_name(const char *cpu_model)
> diff --git a/target/avr/translate.c b/target/avr/translate.c
> index 0490936cd5..e9fef1aaad 100644
> --- a/target/avr/translate.c
> +++ b/target/avr/translate.c
> @@ -92,6 +92,7 @@ struct DisasContext {
>   
>       /* Routine used to access memory */
>       int memidx;
> +    uint32_t offset_io;
>   
>       /*
>        * some AVR instructions can make the following instruction to be skipped
> @@ -2664,6 +2665,7 @@ static void avr_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>       ctx->cs = cs;
>       ctx->env = cpu_env(cs);
>       ctx->npc = ctx->base.pc_first / 2;
> +    ctx->offset_io = env_archcpu(ctx->env)->offset_io;
>   
>       ctx->skip_cond = TCG_COND_NEVER;
>       if (tb_flags & TB_FLAGS_SKIP) {

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 09/17] target/avr: Introduce gen_data_{load,store}_raw

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Prepare for offset_io being non-zero; also allow folding
> stack pointer offsets into the arithmetic.
> So far, all offsets are 0.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/translate.c | 42 ++++++++++++++++++++++++++++++++----------
>   1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/target/avr/translate.c b/target/avr/translate.c
> index e9fef1aaad..6bb4154dff 100644
> --- a/target/avr/translate.c
> +++ b/target/avr/translate.c
> @@ -198,6 +198,28 @@ static bool decode_insn(DisasContext *ctx, uint16_t insn);
>   static void gen_inb(DisasContext *ctx, TCGv data, int port);
>   static void gen_outb(DisasContext *ctx, TCGv data, int port);
>   
> +static void gen_data_store_raw(DisasContext *ctx, TCGv data,
> +                               TCGv addr, int offset, MemOp mop)
> +{
> +    if (ctx->offset_io + offset) {
> +        TCGv t = tcg_temp_new();
> +        tcg_gen_addi_tl(t, addr, ctx->offset_io + offset);
> +        addr = t;
> +    }
> +    tcg_gen_qemu_st_tl(data, addr, MMU_DATA_IDX, mop);
> +}
> +
> +static void gen_data_load_raw(DisasContext *ctx, TCGv data,
> +                              TCGv addr, int offset, MemOp mop)
> +{
> +    if (ctx->offset_io + offset) {
> +        TCGv t = tcg_temp_new();
> +        tcg_gen_addi_tl(t, addr, ctx->offset_io + offset);
> +        addr = t;
> +    }
> +    tcg_gen_qemu_ld_tl(data, addr, MMU_DATA_IDX, mop);
> +}
> +
>   /*
>    * Arithmetic Instructions
>    */
> @@ -940,21 +962,21 @@ static void gen_push_ret(DisasContext *ctx, int ret)
>       if (avr_feature(ctx->env, AVR_FEATURE_1_BYTE_PC)) {
>           TCGv t0 = tcg_constant_i32(ret & 0x0000ff);
>   
> -        tcg_gen_qemu_st_tl(t0, cpu_sp, MMU_DATA_IDX, MO_UB);
> +        gen_data_store_raw(ctx, t0, cpu_sp, 0, MO_UB);
>           tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
>       } else if (avr_feature(ctx->env, AVR_FEATURE_2_BYTE_PC)) {
>           TCGv t0 = tcg_constant_i32(ret & 0x00ffff);
>   
>           tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
> -        tcg_gen_qemu_st_tl(t0, cpu_sp, MMU_DATA_IDX, MO_BEUW);
> +        gen_data_store_raw(ctx, t0, cpu_sp, 0, MO_BEUW);
>           tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
>       } else if (avr_feature(ctx->env, AVR_FEATURE_3_BYTE_PC)) {
>           TCGv lo = tcg_constant_i32(ret & 0x0000ff);
>           TCGv hi = tcg_constant_i32((ret & 0xffff00) >> 8);
>   
> -        tcg_gen_qemu_st_tl(lo, cpu_sp, MMU_DATA_IDX, MO_UB);
> +        gen_data_store_raw(ctx, lo, cpu_sp, 0, MO_UB);
>           tcg_gen_subi_tl(cpu_sp, cpu_sp, 2);
> -        tcg_gen_qemu_st_tl(hi, cpu_sp, MMU_DATA_IDX, MO_BEUW);
> +        gen_data_store_raw(ctx, hi, cpu_sp, 0, MO_BEUW);
>           tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
>       }
>   }
> @@ -963,20 +985,20 @@ static void gen_pop_ret(DisasContext *ctx, TCGv ret)
>   {
>       if (avr_feature(ctx->env, AVR_FEATURE_1_BYTE_PC)) {
>           tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
> -        tcg_gen_qemu_ld_tl(ret, cpu_sp, MMU_DATA_IDX, MO_UB);
> +        gen_data_load_raw(ctx, ret, cpu_sp, 0, MO_UB);
>       } else if (avr_feature(ctx->env, AVR_FEATURE_2_BYTE_PC)) {
>           tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
> -        tcg_gen_qemu_ld_tl(ret, cpu_sp, MMU_DATA_IDX, MO_BEUW);
> +        gen_data_load_raw(ctx, ret, cpu_sp, 0, MO_BEUW);
>           tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
>       } else if (avr_feature(ctx->env, AVR_FEATURE_3_BYTE_PC)) {
>           TCGv lo = tcg_temp_new_i32();
>           TCGv hi = tcg_temp_new_i32();
>   
>           tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
> -        tcg_gen_qemu_ld_tl(hi, cpu_sp, MMU_DATA_IDX, MO_BEUW);
> +        gen_data_load_raw(ctx, hi, cpu_sp, 0, MO_BEUW);
>   
>           tcg_gen_addi_tl(cpu_sp, cpu_sp, 2);
> -        tcg_gen_qemu_ld_tl(lo, cpu_sp, MMU_DATA_IDX, MO_UB);
> +        gen_data_load_raw(ctx, lo, cpu_sp, 0, MO_UB);
>   
>           tcg_gen_deposit_tl(ret, lo, hi, 8, 16);
>       }
> @@ -1498,13 +1520,13 @@ static void gen_data_store(DisasContext *ctx, TCGv data, TCGv addr)
>       if (ctx->base.tb->flags & TB_FLAGS_FULL_ACCESS) {
>           gen_helper_fullwr(tcg_env, data, addr);
>       } else {
> -        tcg_gen_qemu_st_tl(data, addr, MMU_DATA_IDX, MO_UB);
> +        gen_data_store_raw(ctx, data, addr, 0, MO_UB);
>       }
>   }
>   
>   static void gen_data_load(DisasContext *ctx, TCGv data, TCGv addr)
>   {
> -    tcg_gen_qemu_ld_tl(data, addr, MMU_DATA_IDX, MO_UB);
> +    gen_data_load_raw(ctx, data, addr, 0, MO_UB);
>   }
>   
>   static void gen_inb(DisasContext *ctx, TCGv data, int port)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 11/17] target/avr: Implement CPUState.memory_rw_debug

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Prepare for offset_io being non-zero when accessing from gdb.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/cpu.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 080f6f30d3..e4011004b4 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -68,6 +68,35 @@ static void avr_restore_state_to_opc(CPUState *cs,
>       cpu_env(cs)->pc_w = data[0];
>   }
>   
> +static int avr_memory_rw_debug(CPUState *cpu, vaddr addr,
> +                               uint8_t *buf, size_t len, bool is_write)
> +{
> +    if (addr < OFFSET_DATA) {
> +        size_t len_code;
> +        int ret;
> +
> +        if (addr + len <= OFFSET_DATA) {
> +            return cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
> +        }
> +
> +        len_code = addr + len - OFFSET_DATA;
> +        ret = cpu_memory_rw_debug(cpu, addr, buf, len_code, is_write);
> +        if (ret != 0) {
> +            return ret;
> +        }
> +        addr = OFFSET_DATA;
> +        len -= len_code;
> +    }
> +
> +    /*
> +     * Data is biased such that SRAM begins at TARGET_PAGE_SIZE,
> +     * and I/O is immediately prior.  This leave a hole between
> +     * OFFSET_DATA and the relative start of the address space.
> +     */
> +    addr += env_archcpu(cpu_env(cpu))->offset_io;
> +    return cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
> +}
> +
>   static void avr_cpu_reset_hold(Object *obj, ResetType type)
>   {
>       CPUState *cs = CPU(obj);
> @@ -262,6 +291,7 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
>       cc->gdb_write_register = avr_cpu_gdb_write_register;
>       cc->gdb_adjust_breakpoint = avr_cpu_gdb_adjust_breakpoint;
>       cc->gdb_core_xml_file = "avr-cpu.xml";
> +    cc->memory_rw_debug = avr_memory_rw_debug;
>       cc->tcg_ops = &avr_tcg_ops;
>   }
>   

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 12/17] target/avr: Handle offset_io in helper.c

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Prepare for offset_io being non-zero in do_stb.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/helper.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/avr/helper.c b/target/avr/helper.c
> index 9608e59584..3323f32c22 100644
> --- a/target/avr/helper.c
> +++ b/target/avr/helper.c
> @@ -69,7 +69,8 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>   
>   static void do_stb(CPUAVRState *env, uint32_t addr, uint8_t data, uintptr_t ra)
>   {
> -    cpu_stb_mmuidx_ra(env, addr, data, MMU_DATA_IDX, ra);
> +    cpu_stb_mmuidx_ra(env, addr + env_archcpu(env)->offset_io,
> +                      data, MMU_DATA_IDX, ra);
>   }
>   
>   void avr_cpu_do_interrupt(CPUState *cs)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 13/17] target/avr: Handle offset_io in avr_cpu_realizefn

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/cpu.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index e4011004b4..538fcbc215 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -161,12 +161,14 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
>       memory_region_init_io(&cpu->cpu_reg1, OBJECT(cpu), &avr_cpu_reg1, env,
>                             "avr-cpu-reg1", 32);
>       memory_region_add_subregion(get_system_memory(),
> -                                OFFSET_DATA, &cpu->cpu_reg1);
> +                                OFFSET_DATA + cpu->offset_io,
> +                                &cpu->cpu_reg1);
>   
>       memory_region_init_io(&cpu->cpu_reg2, OBJECT(cpu), &avr_cpu_reg2, env,
>                             "avr-cpu-reg2", 8);
>       memory_region_add_subregion(get_system_memory(),
> -                                OFFSET_DATA + 0x58, &cpu->cpu_reg2);
> +                                OFFSET_DATA + cpu->offset_io + 0x58,
> +                                &cpu->cpu_reg2);
>   }
>   
>   static void avr_cpu_set_int(void *opaque, int irq, int level)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 14/17] hw/avr: Set offset_io and increase page size to 1k

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/cpu-param.h |  8 +------
>   hw/avr/atmega.c        | 54 ++++++++++++++++++++++++++----------------
>   2 files changed, 35 insertions(+), 27 deletions(-)
> 
> diff --git a/target/avr/cpu-param.h b/target/avr/cpu-param.h
> index 81f3f49ee1..f5248ce9e7 100644
> --- a/target/avr/cpu-param.h
> +++ b/target/avr/cpu-param.h
> @@ -21,13 +21,7 @@
>   #ifndef AVR_CPU_PARAM_H
>   #define AVR_CPU_PARAM_H
>   
> -/*
> - * TARGET_PAGE_BITS cannot be more than 8 bits because
> - * 1.  all IO registers occupy [0x0000 .. 0x00ff] address range, and they
> - *     should be implemented as a device and not memory
> - * 2.  SRAM starts at the address 0x0100
> - */
> -#define TARGET_PAGE_BITS 8
> +#define TARGET_PAGE_BITS 10
>   #define TARGET_PHYS_ADDR_SPACE_BITS 24
>   #define TARGET_VIRT_ADDR_SPACE_BITS 24
>   
> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
> index 273582b8af..d4fc9c4aee 100644
> --- a/hw/avr/atmega.c
> +++ b/hw/avr/atmega.c
> @@ -14,6 +14,7 @@
>   #include "qapi/error.h"
>   #include "exec/memory.h"
>   #include "exec/address-spaces.h"
> +#include "exec/target_page.h"
>   #include "system/system.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/sysbus.h"
> @@ -222,6 +223,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>       DeviceState *cpudev;
>       SysBusDevice *sbd;
>       char *devname;
> +    uint32_t offset_io, offset_sram;
>       size_t i;
>   
>       assert(mc->io_size <= 0x200);
> @@ -231,13 +233,25 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> +    /*
> +     * Bias the virtual data section so that the start of RAM is at
> +     * the start of the second page of the physical data section.
> +     * This puts all of the I/O at the end of the first page of the
> +     * physical data section.
> +     */
> +    offset_io = TARGET_PAGE_SIZE - mc->io_size;
> +    offset_sram = TARGET_PAGE_SIZE;
> +
>       /* CPU */
>       object_initialize_child(OBJECT(dev), "cpu", &s->cpu, mc->cpu_type);
>   
>       object_property_set_uint(OBJECT(&s->cpu), "init-sp",
>                                mc->io_size + mc->sram_size - 1, &error_abort);
>       object_property_set_uint(OBJECT(&s->cpu), "offset-io",
> -                             0, &error_abort);
> +                             offset_io, &error_abort);
> +
> +    offset_io += OFFSET_DATA;
> +    offset_sram += OFFSET_DATA;
>   
>       qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
>       cpudev = DEVICE(&s->cpu);
> @@ -245,8 +259,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>       /* SRAM */
>       memory_region_init_ram(&s->sram, OBJECT(dev), "sram", mc->sram_size,
>                              &error_abort);
> -    memory_region_add_subregion(get_system_memory(),
> -                                OFFSET_DATA + mc->io_size, &s->sram);
> +    memory_region_add_subregion(get_system_memory(), offset_sram, &s->sram);
>   
>       /* Flash */
>       memory_region_init_rom(&s->flash, OBJECT(dev),
> @@ -258,13 +271,14 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>        *
>        * 0x00 - 0x1f: Registers
>        * 0x20 - 0x5f: I/O memory
> -     * 0x60 - 0xff: Extended I/O
> +     * 0x60 - 0x[1]ff: Extended I/O
>        */
>       s->io = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
>       qdev_prop_set_string(s->io, "name", "I/O");
>       qdev_prop_set_uint64(s->io, "size", mc->io_size);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(s->io), &error_fatal);
> -    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(s->io), 0, OFFSET_DATA, -1234);
> +
> +    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(s->io), 0, offset_io, -1234);
>   
>       /* Power Reduction */
>       for (i = 0; i < POWER_MAX; i++) {
> @@ -277,7 +291,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>                                   TYPE_AVR_MASK);
>           sysbus_realize(SYS_BUS_DEVICE(&s->pwr[i]), &error_abort);
>           sysbus_mmio_map(SYS_BUS_DEVICE(&s->pwr[i]), 0,
> -                        OFFSET_DATA + mc->dev[idx].addr);
> +                        offset_io + mc->dev[idx].addr);
>           g_free(devname);
>       }
>   
> @@ -289,7 +303,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>           }
>           devname = g_strdup_printf("atmega-gpio-%c", 'a' + (char)i);
>           create_unimplemented_device(devname,
> -                                    OFFSET_DATA + mc->dev[idx].addr, 3);
> +                                    offset_io + mc->dev[idx].addr, 3);
>           g_free(devname);
>       }
>   
> @@ -305,7 +319,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>           qdev_prop_set_chr(DEVICE(&s->usart[i]), "chardev", serial_hd(i));
>           sbd = SYS_BUS_DEVICE(&s->usart[i]);
>           sysbus_realize(sbd, &error_abort);
> -        sysbus_mmio_map(sbd, 0, OFFSET_DATA + mc->dev[USART(i)].addr);
> +        sysbus_mmio_map(sbd, 0, offset_io + mc->dev[USART(i)].addr);
>           connect_peripheral_irq(mc, sbd, 0, cpudev, USART_RXC_IRQ(i));
>           connect_peripheral_irq(mc, sbd, 1, cpudev, USART_DRE_IRQ(i));
>           connect_peripheral_irq(mc, sbd, 2, cpudev, USART_TXC_IRQ(i));
> @@ -321,12 +335,12 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>           }
>           if (!mc->dev[idx].is_timer16) {
>               create_unimplemented_device("avr-timer8",
> -                                        OFFSET_DATA + mc->dev[idx].addr, 5);
> +                                        offset_io + mc->dev[idx].addr, 5);
>               create_unimplemented_device("avr-timer8-intmask",
> -                                        OFFSET_DATA
> +                                        offset_io
>                                           + mc->dev[idx].intmask_addr, 1);
>               create_unimplemented_device("avr-timer8-intflag",
> -                                        OFFSET_DATA
> +                                        offset_io
>                                           + mc->dev[idx].intflag_addr, 1);
>               continue;
>           }
> @@ -337,9 +351,9 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>                                    s->xtal_freq_hz, &error_abort);
>           sbd = SYS_BUS_DEVICE(&s->timer[i]);
>           sysbus_realize(sbd, &error_abort);
> -        sysbus_mmio_map(sbd, 0, OFFSET_DATA + mc->dev[idx].addr);
> -        sysbus_mmio_map(sbd, 1, OFFSET_DATA + mc->dev[idx].intmask_addr);
> -        sysbus_mmio_map(sbd, 2, OFFSET_DATA + mc->dev[idx].intflag_addr);
> +        sysbus_mmio_map(sbd, 0, offset_io + mc->dev[idx].addr);
> +        sysbus_mmio_map(sbd, 1, offset_io + mc->dev[idx].intmask_addr);
> +        sysbus_mmio_map(sbd, 2, offset_io + mc->dev[idx].intflag_addr);
>           connect_peripheral_irq(mc, sbd, 0, cpudev, TIMER_CAPT_IRQ(i));
>           connect_peripheral_irq(mc, sbd, 1, cpudev, TIMER_COMPA_IRQ(i));
>           connect_peripheral_irq(mc, sbd, 2, cpudev, TIMER_COMPB_IRQ(i));
> @@ -349,12 +363,12 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>           g_free(devname);
>       }
>   
> -    create_unimplemented_device("avr-twi",          OFFSET_DATA + 0x0b8, 6);
> -    create_unimplemented_device("avr-adc",          OFFSET_DATA + 0x078, 8);
> -    create_unimplemented_device("avr-ext-mem-ctrl", OFFSET_DATA + 0x074, 2);
> -    create_unimplemented_device("avr-watchdog",     OFFSET_DATA + 0x060, 1);
> -    create_unimplemented_device("avr-spi",          OFFSET_DATA + 0x04c, 3);
> -    create_unimplemented_device("avr-eeprom",       OFFSET_DATA + 0x03f, 3);
> +    create_unimplemented_device("avr-twi",          offset_io + 0x0b8, 6);
> +    create_unimplemented_device("avr-adc",          offset_io + 0x078, 8);
> +    create_unimplemented_device("avr-ext-mem-ctrl", offset_io + 0x074, 2);
> +    create_unimplemented_device("avr-watchdog",     offset_io + 0x060, 1);
> +    create_unimplemented_device("avr-spi",          offset_io + 0x04c, 3);
> +    create_unimplemented_device("avr-eeprom",       offset_io + 0x03f, 3);
>   }
>   
>   static const Property atmega_props[] = {

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 16/17] hw/avr: Move AtmegaMcuClass to atmega.h

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/avr/atmega.h | 20 ++++++++++++++++++++
>   hw/avr/atmega.c | 22 +---------------------
>   2 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
> index a99ee15c7e..f031e6c10a 100644
> --- a/hw/avr/atmega.h
> +++ b/hw/avr/atmega.h
> @@ -23,6 +23,10 @@
>   #define TYPE_ATMEGA1280_MCU "ATmega1280"
>   #define TYPE_ATMEGA2560_MCU "ATmega2560"
>   
> +typedef struct AtmegaMcuClass AtmegaMcuClass;
> +DECLARE_CLASS_CHECKERS(AtmegaMcuClass, ATMEGA_MCU,
> +                       TYPE_ATMEGA_MCU)
> +
>   typedef struct AtmegaMcuState AtmegaMcuState;
>   DECLARE_INSTANCE_CHECKER(AtmegaMcuState, ATMEGA_MCU,
>                            TYPE_ATMEGA_MCU)
> @@ -32,6 +36,22 @@ DECLARE_INSTANCE_CHECKER(AtmegaMcuState, ATMEGA_MCU,
>   #define TIMER_MAX 6
>   #define GPIO_MAX 12
>   
> +struct AtmegaMcuClass {
> +    /*< private >*/
> +    SysBusDeviceClass parent_class;
> +    /*< public >*/
> +    const char *uc_name;
> +    const char *cpu_type;
> +    size_t flash_size;
> +    size_t eeprom_size;
> +    size_t sram_size;
> +    size_t io_size;
> +    size_t gpio_count;
> +    size_t adc_count;
> +    const uint8_t *irq;
> +    const struct peripheral_cfg *dev;
> +};
> +
>   struct AtmegaMcuState {
>       /*< private >*/
>       SysBusDevice parent_obj;
> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
> index d4fc9c4aee..96e36743bc 100644
> --- a/hw/avr/atmega.c
> +++ b/hw/avr/atmega.c
> @@ -36,7 +36,7 @@ enum AtmegaPeripheral {
>   #define TIMER(n)    (n + TIMER0)
>   #define POWER(n)    (n + POWER0)
>   
> -typedef struct {
> +typedef struct peripheral_cfg {
>       uint16_t addr;
>       enum AtmegaPeripheral power_index;
>       uint8_t power_bit;
> @@ -46,26 +46,6 @@ typedef struct {
>       bool is_timer16;
>   } peripheral_cfg;
>   
> -struct AtmegaMcuClass {
> -    /*< private >*/
> -    SysBusDeviceClass parent_class;
> -    /*< public >*/
> -    const char *uc_name;
> -    const char *cpu_type;
> -    size_t flash_size;
> -    size_t eeprom_size;
> -    size_t sram_size;
> -    size_t io_size;
> -    size_t gpio_count;
> -    size_t adc_count;
> -    const uint8_t *irq;
> -    const peripheral_cfg *dev;
> -};
> -typedef struct AtmegaMcuClass AtmegaMcuClass;
> -
> -DECLARE_CLASS_CHECKERS(AtmegaMcuClass, ATMEGA_MCU,
> -                       TYPE_ATMEGA_MCU)
> -
>   static const peripheral_cfg dev168_328[PERIFMAX] = {
>       [USART0]        = {  0xc0, POWER0, 1 },
>       [TIMER2]        = {  0xb0, POWER0, 6, 0x70, 0x37, false },

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 17/17] target/avr: Enable TARGET_PAGE_BITS_VARY

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Increase TARGET_PHYS_ADDR_SPACE_BITS to allow flexibility in the page
> size without triggering an assert.  Select the page size based on the
> size of sram.  This leaves sram on exactly one page and minimizes the
> number of pages required to span the flash.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/cpu-param.h | 11 +++++++++--
>   hw/avr/arduino.c       | 15 +++++++++++++++
>   2 files changed, 24 insertions(+), 2 deletions(-)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 15/17] hw/avr: Pass mcu_type to class_base_init via .class_data

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> We want to be able to do more common work on MachineClass.
> Pass the class name as a string in .class_data.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/avr/arduino.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 10/17] target/avr: Update cpu_sp after push and pop

[Pierrick Bouvier]

On 3/23/25 10:37, Richard Henderson wrote:
> Not that AVR has memory paging traps, but it's better
> form to allow the memory operation to finish before
> updating the cpu register.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/avr/translate.c | 32 +++++++++++++++-----------------
>   1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/target/avr/translate.c b/target/avr/translate.c
> index 6bb4154dff..3446007be1 100644
> --- a/target/avr/translate.c
> +++ b/target/avr/translate.c
> @@ -967,40 +967,38 @@ static void gen_push_ret(DisasContext *ctx, int ret)
>       } else if (avr_feature(ctx->env, AVR_FEATURE_2_BYTE_PC)) {
>           TCGv t0 = tcg_constant_i32(ret & 0x00ffff);
>   
> -        tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
> -        gen_data_store_raw(ctx, t0, cpu_sp, 0, MO_BEUW);
> -        tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
> +        gen_data_store_raw(ctx, t0, cpu_sp, -1, MO_BEUW);
> +        tcg_gen_subi_tl(cpu_sp, cpu_sp, 2);
>       } else if (avr_feature(ctx->env, AVR_FEATURE_3_BYTE_PC)) {
>           TCGv lo = tcg_constant_i32(ret & 0x0000ff);
>           TCGv hi = tcg_constant_i32((ret & 0xffff00) >> 8);
>   
>           gen_data_store_raw(ctx, lo, cpu_sp, 0, MO_UB);
> -        tcg_gen_subi_tl(cpu_sp, cpu_sp, 2);
> -        gen_data_store_raw(ctx, hi, cpu_sp, 0, MO_BEUW);
> -        tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
> +        gen_data_store_raw(ctx, hi, cpu_sp, -2, MO_BEUW);
> +        tcg_gen_subi_tl(cpu_sp, cpu_sp, 3);
> +    } else {
> +        g_assert_not_reached();
>       }
>   }
>   
>   static void gen_pop_ret(DisasContext *ctx, TCGv ret)
>   {
>       if (avr_feature(ctx->env, AVR_FEATURE_1_BYTE_PC)) {
> +        gen_data_load_raw(ctx, ret, cpu_sp, 1, MO_UB);
>           tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
> -        gen_data_load_raw(ctx, ret, cpu_sp, 0, MO_UB);
>       } else if (avr_feature(ctx->env, AVR_FEATURE_2_BYTE_PC)) {
> -        tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
> -        gen_data_load_raw(ctx, ret, cpu_sp, 0, MO_BEUW);
> -        tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
> +        gen_data_load_raw(ctx, ret, cpu_sp, 1, MO_BEUW);
> +        tcg_gen_addi_tl(cpu_sp, cpu_sp, 2);
>       } else if (avr_feature(ctx->env, AVR_FEATURE_3_BYTE_PC)) {
> -        TCGv lo = tcg_temp_new_i32();
>           TCGv hi = tcg_temp_new_i32();
>   
> -        tcg_gen_addi_tl(cpu_sp, cpu_sp, 1);
> -        gen_data_load_raw(ctx, hi, cpu_sp, 0, MO_BEUW);
> +        gen_data_load_raw(ctx, hi, cpu_sp, 1, MO_BEUW);
> +        gen_data_load_raw(ctx, ret, cpu_sp, 3, MO_UB);
> +        tcg_gen_addi_tl(cpu_sp, cpu_sp, 3);
>   
> -        tcg_gen_addi_tl(cpu_sp, cpu_sp, 2);
> -        gen_data_load_raw(ctx, lo, cpu_sp, 0, MO_UB);
> -
> -        tcg_gen_deposit_tl(ret, lo, hi, 8, 16);
> +        tcg_gen_deposit_tl(ret, ret, hi, 8, 16);
> +    } else {
> +        g_assert_not_reached();
>       }
>   }
>   

A bit hard to review with the side effect of gen_data_*_raw, but number 
seems to match.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 05/17] target/avr: Move cpu register accesses into system memory

[Richard Henderson]

On 3/24/25 18:07, Pierrick Bouvier wrote:
> A simple nit, maybe we could define constant for register names. This way, it can be used 
> in the two switch for read/access.

Which constant?

   - The absolute address (0x58-0x5f)
   - The i/o port address (0x38-0x3f)
   - or the region offset (0-7)

?  That's where I got stuck and didn't add such.

> By the way, are there other architectures where cpu registers are mapped in memory like AVR?

Not that I know of.


r~

Re: [PATCH 05/17] target/avr: Move cpu register accesses into system memory

[Pierrick Bouvier]

On 3/25/25 06:48, Richard Henderson wrote:
> On 3/24/25 18:07, Pierrick Bouvier wrote:
>> A simple nit, maybe we could define constant for register names. This way, it can be used
>> in the two switch for read/access.
> 
> Which constant?
> 
>     - The absolute address (0x58-0x5f)
>     - The i/o port address (0x38-0x3f)
>     - or the region offset (0-7)
> 
> ?  That's where I got stuck and didn't add such.
> 

Something like:

#define RAMPD 0
...
#define SREG 7
#define REG_IOPORT_BASE_ADDR 0x38
#define REG_BASE_ADDR 0x58

And in the switch, we can substract base addresses, so only cases 0..7 
are left.

I don't have a strong opinion on names, just thought seeing those 2 
address constants + register offset is useful and makes intention clearer.

>> By the way, are there other architectures where cpu registers are mapped in memory like AVR?
> 
> Not that I know of.
> 
> 
> r~