Re: [Qemu-devel] [PATCH 3/3] target-xtensa: xtfpga: support noMMU cores

[Peter Crosthwaite]

On Sun, Sep 27, 2015 at 10:16 AM, Max Filippov <address@hidden> wrote:
> Cores with and without MMU have system RAM and ROM at different locations.
> Also with noMMU cores system IO region is accessible through two physical
> address ranges.
>
> Signed-off-by: Max Filippov <address@hidden>
> ---
>  hw/xtensa/xtfpga.c | 49 +++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
> index d4b9afb..b53f40d 100644
> --- a/hw/xtensa/xtfpga.c
> +++ b/hw/xtensa/xtfpga.c
> @@ -199,7 +199,29 @@ static void lx_init(const LxBoardDesc *board, 
> MachineState *machine)
>      const char *kernel_cmdline = qemu_opt_get(machine_opts, "append");
>      const char *dtb_filename = qemu_opt_get(machine_opts, "dtb");
>      const char *initrd_filename = qemu_opt_get(machine_opts, "initrd");
> +    const unsigned system_io_size = 224 * 1024 * 1024;
> +    bool mmu;

You are indexing into an array of configs so it's really an int (or
better, an enum).

>      int n;

Blank line.

> +    static const struct {
> +        hwaddr ram;
> +        hwaddr rom;
> +        hwaddr io[2];
> +    } base[2] = {
> +        {
> +            .ram = 0x60000000,
> +            .rom = 0x50000000,
> +            .io = {
> +                0x70000000,
> +                0x90000000,
> +            },
> +        }, {
> +            .ram = 0,
> +            .rom = 0xfe000000,
> +            .io = {
> +                0xf0000000,
> +            },
> +        }
> +    };
>
>      if (!cpu_model) {
>          cpu_model = XTENSA_DEFAULT_CPU_MODEL;
> @@ -222,16 +244,24 @@ static void lx_init(const LxBoardDesc *board, 
> MachineState *machine)
>          cpu_reset(CPU(cpu));
>      }
>
> +    mmu = xtensa_option_enabled(env->config, XTENSA_OPTION_MMU);

This looks backwards, the board should be in charge of itself and the
CPU config, rather than spying on the CPU setup to rewire the board.

>      ram = g_malloc(sizeof(*ram));
>      memory_region_init_ram(ram, NULL, "lx60.dram", machine->ram_size,
>                             &error_fatal);
>      vmstate_register_ram_global(ram);
> -    memory_region_add_subregion(system_memory, 0, ram);
> +    memory_region_add_subregion(system_memory, base[mmu].ram, ram);
>
>      system_io = g_malloc(sizeof(*system_io));
>      memory_region_init_io(system_io, NULL, &lx60_io_ops, NULL, "lx60.io",
> -                          224 * 1024 * 1024);
> -    memory_region_add_subregion(system_memory, 0xf0000000, system_io);
> +                          system_io_size);
> +    memory_region_add_subregion(system_memory, base[mmu].io[0], system_io);
> +    if (!mmu) {

The boolean switch for whether the alias exists could go in the
struct. That makes it more robust to add yet more configs in the
future rather than iffery on the config index.

Regards,
Peter

> +        MemoryRegion *io = g_malloc(sizeof(*io));
> +
> +        memory_region_init_alias(io, NULL, "lx60.io.cached",
> +                                 system_io, 0, system_io_size);
> +        memory_region_add_subregion(system_memory, base[mmu].io[1], io);
> +    }
>      lx60_fpga_init(system_io, 0x0d020000);
>      if (nd_table[0].used) {
>          lx60_net_init(system_io, 0x0d030000, 0x0d030400, 0x0d800000,
> @@ -267,22 +297,25 @@ static void lx_init(const LxBoardDesc *board, 
> MachineState *machine)
>      if (kernel_filename) {
>          uint32_t entry_point = env->pc;
>          size_t bp_size = 3 * get_tag_size(0); /* first/last and memory tags 
> */
> -        uint32_t tagptr = 0xfe000000 + board->sram_size;
> +        uint32_t tagptr = base[mmu].rom + board->sram_size;
>          uint32_t cur_tagptr;
>          BpMemInfo memory_location = {
>              .type = tswap32(MEMORY_TYPE_CONVENTIONAL),
> -            .start = tswap32(0),
> -            .end = tswap32(machine->ram_size),
> +            .start = tswap32(base[mmu].ram),
> +            .end = tswap32(base[mmu].ram + machine->ram_size),
>          };
>          uint32_t lowmem_end = machine->ram_size < 0x08000000 ?
>              machine->ram_size : 0x08000000;
>          uint32_t cur_lowmem = QEMU_ALIGN_UP(lowmem_end / 2, 4096);
>
> +        lowmem_end += base[mmu].ram;
> +        cur_lowmem += base[mmu].ram;
> +
>          rom = g_malloc(sizeof(*rom));
>          memory_region_init_ram(rom, NULL, "lx60.sram", board->sram_size,
>                                 &error_fatal);
>          vmstate_register_ram_global(rom);
> -        memory_region_add_subregion(system_memory, 0xfe000000, rom);
> +        memory_region_add_subregion(system_memory, base[mmu].rom, rom);
>
>          if (kernel_cmdline) {
>              bp_size += get_tag_size(strlen(kernel_cmdline) + 1);
> @@ -381,7 +414,7 @@ static void lx_init(const LxBoardDesc *board, 
> MachineState *machine)
>                      flash_mr, board->flash_boot_base,
>                      board->flash_size - board->flash_boot_base < 0x02000000 ?
>                      board->flash_size - board->flash_boot_base : 0x02000000);
> -            memory_region_add_subregion(system_memory, 0xfe000000,
> +            memory_region_add_subregion(system_memory, base[mmu].rom,
>                      flash_io);
>          }
>      }
> --
> 1.8.1.4
>
>