Re: [Qemu-devel] [PATCH v6 02/15] target-tricore: Add board for systemmode

[Peter Maydell]

On 22 August 2014 17:52, Bastian Koppelmann
<address@hidden> wrote:
> Add basic board to allow systemmode emulation
>
> Signed-off-by: Bastian Koppelmann <address@hidden>
> ---
> v5 -> v6:
>     - tricore_testboard: Fix machine name containing blanks.
>
>  hw/tricore/Makefile.objs       |   1 +
>  hw/tricore/tricore_testboard.c | 129 
> +++++++++++++++++++++++++++++++++++++++++
>  include/hw/tricore/tricore.h   |  54 +++++++++++++++++
>  3 files changed, 184 insertions(+)
>  create mode 100644 hw/tricore/Makefile.objs
>  create mode 100644 hw/tricore/tricore_testboard.c
>  create mode 100644 include/hw/tricore/tricore.h
>
> diff --git a/hw/tricore/Makefile.objs b/hw/tricore/Makefile.objs
> new file mode 100644
> index 0000000..435e095
> --- /dev/null
> +++ b/hw/tricore/Makefile.objs
> @@ -0,0 +1 @@
> +obj-y += tricore_testboard.o
> diff --git a/hw/tricore/tricore_testboard.c b/hw/tricore/tricore_testboard.c
> new file mode 100644
> index 0000000..04c0f89
> --- /dev/null
> +++ b/hw/tricore/tricore_testboard.c
> @@ -0,0 +1,129 @@
> +/*
> + * TriCore Baseboard System emulation.
> + *
> + * Copyright (c) 2014 Bastian Koppelmann
> + *
> + * This code is licensed under the GPL.

Please at least say "v2 or later" (or whatever you mean here);
just "GPL" is ambiguous.

> + */
> +
> +#include "hw/hw.h"
> +#include "hw/devices.h"
> +#include "net/net.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/boards.h"
> +#include "hw/loader.h"
> +#include "sysemu/blockdev.h"
> +#include "exec/address-spaces.h"
> +#include "hw/block/flash.h"
> +#include "elf.h"
> +#include "hw/tricore/tricore.h"
> +
> +#define TRICORE_FLASH_ADDR 0xa0000000
> +#define TRICORE_FLASH_SIZE (2 * 1024 * 1024)
> +#define TRICORE_FLASH_SECT_SIZE (256 * 1024)
> +
> +
> +/* Board init.  */
> +
> +static struct tricore_boot_info tricoretb_binfo;
> +
> +static void tricore_load_kernel(CPUTRICOREState *env)
> +{
> +    int64_t entry;

Should this really be signed? It looks rather odd, especially
given you immediately cast it below.

> +    long kernel_size;
> +
> +    kernel_size = load_elf(tricoretb_binfo.kernel_filename, NULL,
> +                           NULL, (uint64_t *)&entry, NULL,
> +                           NULL, 0,
> +                           ELF_MACHINE, 1);
> +    if (kernel_size <= 0) {
> +        fprintf(stderr, "qemu: no kernel file '%s'\n",
> +                tricoretb_binfo.kernel_filename);

    error_report() is preferred over directly printing to stderr
(Note that it provides the trailing \n so you don't need to.)

> +        exit(1);
> +    }
> +    env->PC = entry;
> +
> +}
> +
> +static void tricore_testboard_init(MachineState *machine, int board_id)
> +{
> +    TRICORECPU *cpu;
> +    CPUTRICOREState *env;
> +
> +    MemoryRegion *sysmem = get_system_memory();
> +    MemoryRegion *ext_cram = g_new(MemoryRegion, 1);
> +    MemoryRegion *ext_dram = g_new(MemoryRegion, 1);
> +    MemoryRegion *int_cram = g_new(MemoryRegion, 1);
> +    MemoryRegion *int_dram = g_new(MemoryRegion, 1);
> +    MemoryRegion *pcp_data = g_new(MemoryRegion, 1);
> +    MemoryRegion *pcp_text = g_new(MemoryRegion, 1);
> +    DriveInfo *dinfo;
> +
> +    if (!machine->cpu_model) {
> +        machine->cpu_model = "tc1796";
> +    }
> +    cpu = cpu_tricore_init(machine->cpu_model);
> +    env = &cpu->env;
> +    if (!cpu) {
> +        fprintf(stderr, "Unable to find CPU definition\n");

    error_report("Unable to find CPU definition");

> +        exit(1);
> +    }
> +    memory_region_init_ram(ext_cram, NULL, "powerlink_ext_c.ram", 
> 2*1024*1024);
> +    vmstate_register_ram_global(ext_cram);
> +    memory_region_init_ram(ext_dram, NULL, "powerlink_ext_d.ram", 
> 4*1024*1024);
> +    vmstate_register_ram_global(ext_dram);
> +    memory_region_init_ram(int_cram, NULL, "powerlink_int_c.ram", 48*1024);
> +    vmstate_register_ram_global(int_cram);
> +    memory_region_init_ram(int_dram, NULL, "powerlink_int_d.ram", 48*1024);
> +    vmstate_register_ram_global(int_dram);
> +    memory_region_init_ram(pcp_data, NULL, "powerlink_pcp_data.ram", 
> 16*1024);
> +    vmstate_register_ram_global(pcp_data);
> +    memory_region_init_ram(pcp_text, NULL, "powerlink_pcp_text.ram", 
> 32*1024);
> +    vmstate_register_ram_global(pcp_text);
> +
> +    memory_region_add_subregion(sysmem, 0x80000000, ext_cram);
> +    memory_region_add_subregion(sysmem, 0xa1000000, ext_dram);
> +    memory_region_add_subregion(sysmem, 0xd4000000, int_cram);
> +    memory_region_add_subregion(sysmem, 0xd0000000, int_dram);
> +    memory_region_add_subregion(sysmem, 0xf0050000, pcp_data);
> +    memory_region_add_subregion(sysmem, 0xf0060000, pcp_text);

You could make this all data driven if you like, but I don't insist.

> +
> +    dinfo = drive_get(IF_PFLASH, 0, 0);
> +    if (!pflash_cfi01_register(TRICORE_FLASH_ADDR, NULL,
> +                          "tricore_testboard.flash",
> +                          TRICORE_FLASH_SIZE, dinfo ? dinfo->bdrv : NULL,
> +                          TRICORE_FLASH_SECT_SIZE,
> +                          TRICORE_FLASH_SIZE / TRICORE_FLASH_SECT_SIZE,
> +                          2, 0x00, 0x00, 0x0000, 0x0, 0)) {

Don't use pflash_cfi01_register() in new code, it gives you a
weird not-like-hardware flash device that we only have for
backwards compatibility with existing board models. Instantiate
and configure the flash device directly (compare vexpress.c's
ve_pflash_cfi01_register(), but use the correct config for the
hardware you're modelling, which might not be two parallel
16 bit wide flash chips).

> +
> +        fprintf(stderr, "qemu: Error registering flash memory.\n");

error_report().

> +    } else {
> +        env->PC = TRICORE_FLASH_ADDR;

This looks bogus. You should let the emulated CPU reset to its
own reset address as per whatever the architecture says CPUs
reset to. If the CPU allows a board to configure the reset address
(eg by setting input lines the CPU samples on reset) then you
should model that by making the reset address a QOM property
of the CPU.

> +    }
> +
> +    tricoretb_binfo.ram_size = machine->ram_size;
> +    tricoretb_binfo.kernel_filename = machine->kernel_filename;
> +
> +    if (machine->kernel_filename) {
> +        tricore_load_kernel(env);
> +    }
> +}
> +
> +static void tricoreboard_init(MachineState *machine)
> +{
> +    tricore_testboard_init(machine, 0x183);
> +}
> +
> +static QEMUMachine ttb_machine = {
> +    .name = "tricore_testboard",
> +    .desc = "Just for testing",
> +    .init = tricoreboard_init,
> +    .is_default = 1,
> +};

The .desc text here appears in the user-visible help output, so
can we have something slightly more useful to them, please?

$ ./build/all/tricore-softmmu/qemu-system-tricore -M help
Supported machines are:
tricore_testboard    Just for testing (default)
none                 empty machine

Also, think about whether you really want to set the
is_default flag. Will all users of TriCore based boards
always want the test board?

> +++ b/include/hw/tricore/tricore.h
> @@ -0,0 +1,54 @@
> +#ifndef TRICORE_MISC_H
> +#define TRICORE_MISC_H 1
> +
> +#include "exec/memory.h"
> +#include "hw/irq.h"
> +
> +struct tricore_boot_info {
> +    uint64_t ram_size;
> +    const char *kernel_filename;
> +    const char *kernel_cmdline;
> +    const char *initrd_filename;
> +    const char *dtb_filename;
> +    hwaddr loader_start;
> +    /* multicore boards that use the default secondary core boot functions
> +     * need to put the address of the secondary boot code, the boot reg,
> +     * and the GIC address in the next 3 values, respectively. boards that
> +     * have their own boot functions can use these values as they want.
> +     */
> +    hwaddr smp_loader_start;
> +    hwaddr smp_bootreg_addr;
> +    hwaddr gic_cpu_if_addr;
> +    int nb_cpus;
> +    int board_id;
> +    int (*atag_board)(const struct tricore_boot_info *info, void *p);
> +    /* multicore boards that use the default secondary core boot functions
> +     * can ignore these two function calls. If the default functions won't
> +     * work, then write_secondary_boot() should write a suitable blob of
> +     * code mimicking the secondary CPU startup process used by the board's
> +     * boot loader/boot ROM code, and secondary_cpu_reset_hook() should
> +     * perform any necessary CPU reset handling and set the PC for the
> +     * secondary CPUs to point at this boot blob.
> +     */
> +    void (*write_secondary_boot)(TRICORECPU *cpu,
> +                                 const struct tricore_boot_info *info);
> +    void (*secondary_cpu_reset_hook)(TRICORECPU *cpu,
> +                                     const struct tricore_boot_info *info);
> +    /* if a board is able to create a dtb without a dtb file then it
> +     * sets get_dtb. This will only be used if no dtb file is provided
> +     * by the user. On success, sets *size to the length of the created
> +     * dtb, and returns a pointer to it. (The caller must free this memory
> +     * with g_free() when it has finished with it.) On failure, returns NULL.
> +     */
> +    void *(*get_dtb)(const struct tricore_boot_info *info, int *size);
> +    /* if a board needs to be able to modify a device tree provided by
> +     * the user it should implement this hook.
> +     */
> +    void (*modify_dtb)(const struct tricore_boot_info *info, void *fdt);
> +    /* Used internally by arm_boot.c */
> +    int is_linux;
> +    hwaddr initrd_start;
> +    hwaddr initrd_size;
> +    hwaddr entry;
> +};

This looks pretty obviously like you just copied it wholesale from
the ARM board support code. Please only include struct fields
you actually use.

thanks
-- PMM