Re: [Qemu-devel] [PATCH v7 11/16] target-or32: Add a IIS dummy board

[Peter Crosthwaite]

Hi Jia,

On Wed, Jun 27, 2012 at 7:54 PM, Jia Liu <address@hidden> wrote:
> Add a IIS dummy board.
>
> Signed-off-by: Jia Liu <address@hidden>
> ---
>  hw/openrisc/Makefile.objs |    2 +-
>  hw/openrisc_sim.c         |  149 
> +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 150 insertions(+), 1 deletion(-)
>  create mode 100644 hw/openrisc_sim.c
>
> diff --git a/hw/openrisc/Makefile.objs b/hw/openrisc/Makefile.objs
> index 1c541a5..38ff8f5 100644
> --- a/hw/openrisc/Makefile.objs
> +++ b/hw/openrisc/Makefile.objs
> @@ -1,3 +1,3 @@
> -obj-y = openrisc_pic.o openrisc_timer.o
> +obj-y = openrisc_pic.o openrisc_sim.o openrisc_timer.o
>
>  obj-y := $(addprefix ../,$(obj-y))
> diff --git a/hw/openrisc_sim.c b/hw/openrisc_sim.c
> new file mode 100644
> index 0000000..b0c1e69
> --- /dev/null
> +++ b/hw/openrisc_sim.c
> @@ -0,0 +1,149 @@
> +/*
> + *  OpenRISC simulator for use as an IIS.
> + *
> + *  Copyright (c) 2011-2012 Jia Liu <address@hidden>
> + *                          Feng Gao <address@hidden>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw.h"
> +#include "openrisc_cpudev.h"
> +#include "boards.h"
> +#include "elf.h"
> +#include "pc.h"
> +#include "loader.h"
> +#include "exec-memory.h"
> +#include "sysemu.h"
> +#include "sysbus.h"
> +#include "qtest.h"
> +
> +#define KERNEL_LOAD_ADDR 0x100
> +

Unused definition.

> +static struct _loaderparams {
> +    uint64_t ram_size;
> +    const char *kernel_filename;
> +    const char *kernel_cmdline;
> +    const char *initrd_filename;

ram_size, kernel_cmdline and initrd_filename are all unused AFAICT?
Can you simplify your bootloader by passing just the filename and none
of this Linux specific stuff?

> +} loaderparams;
> +
> +static void main_cpu_reset(void *opaque)
> +{
> +    CPUOpenRISCState *env = opaque;
> +    cpu_reset(ENV_GET_CPU(env));
> +}
> +
> +static void openrisc_sim_net_init(MemoryRegion *address_space,
> +                                  target_phys_addr_t base,
> +                                  target_phys_addr_t descriptors,
> +                                  qemu_irq irq, NICInfo *nd)
> +{
> +    DeviceState *dev;
> +    SysBusDevice *s;
> +
> +    dev = qdev_create(NULL, "open_eth");
> +    qdev_set_nic_properties(dev, nd);
> +    qdev_init_nofail(dev);
> +
> +    s = sysbus_from_qdev(dev);
> +    sysbus_connect_irq(s, 0, irq);
> +    memory_region_add_subregion(address_space, base,
> +                                sysbus_mmio_get_region(s, 0));
> +    memory_region_add_subregion(address_space, descriptors,
> +                                sysbus_mmio_get_region(s, 1));
> +}
> +
> +static uint64_t openrisc_load_kernel(void)
> +{

cc Peter Maydell Re the bootloader discussion. - This is yet another
bootloader. This one is just load elf else load uimage else load
raw-image. No append, initrd, dtb etc. Not a blocker at this stage but
looking ahead this will fall under the umbrella of the bootloader
unifications we need to discuss.

> +    long kernel_size;
> +    uint64_t elf_entry;
> +    target_phys_addr_t entry;
> +
> +    if (loaderparams.kernel_filename && !qtest_enabled()) {
> +        kernel_size = load_uimage(loaderparams.kernel_filename,
> +                                  &entry, NULL, NULL);
> +        if (kernel_size < 0) {
> +            kernel_size = load_elf(loaderparams.kernel_filename, NULL, NULL,
> +                                   &elf_entry, NULL, NULL, 1, ELF_MACHINE, 
> 1);
> +            entry = elf_entry;
> +        }
> +
> +        if (kernel_size < 0) {
> +            fprintf(stderr, "QEMU: couldn't load the kernel '%s'\n",
> +                    loaderparams.kernel_filename);
> +            exit(1);
> +        }
> +    }
> +
> +    return entry;
> +}
> +
> +static void openrisc_sim_init(ram_addr_t ram_size,
> +                              const char *boot_device,
> +                              const char *kernel_filename,
> +                              const char *kernel_cmdline,
> +                              const char *initrd_filename,
> +                              const char *cpu_model)
> +{
> +    CPUOpenRISCState *env;
> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
> +
> +    if (!cpu_model) {
> +        cpu_model = "or1200";
> +    }
> +    env = cpu_init(cpu_model);
> +    if (!env) {
> +        fprintf(stderr, "Unable to find CPU definition!\n");
> +        exit(1);
> +    }
> +
> +    qemu_register_reset(main_cpu_reset, env);
> +    main_cpu_reset(env);
> +

I think this needs rebasing. Andreas a while back abstracted back to
the CPU level instead for resets. Andreas can you confirm? should this
be changed to pass the CPU QOM object to the reset instead? cc
andreas.

> +    memory_region_init_ram(ram, "openrisc.ram", ram_size);
> +    memory_region_add_subregion(get_system_memory(), 0, ram);
> +
> +    if (kernel_filename) {
> +        loaderparams.ram_size = ram_size;
> +        loaderparams.kernel_filename = kernel_filename;
> +        loaderparams.kernel_cmdline = kernel_cmdline;
> +        env->pc = openrisc_load_kernel();
> +    }

Its probably more usual and safer to bootload last. You are still
creating your machine after this. Can you move this hunk to be the
last thing in the function?

Regards,
Peter
> +
> +    cpu_openrisc_pic_init(env);
> +    cpu_openrisc_clock_init(env);
> +
> +    serial_mm_init(get_system_memory(), 0x90000000, 0,
> +                   env->irq[2], 115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
> +
> +    if (nd_table[0].vlan) {
> +        openrisc_sim_net_init(get_system_memory(), 0x92000000,
> +                              0x92000400, env->irq[4], nd_table);
> +    }
> +}
> +
> +static QEMUMachine openrisc_sim_machine = {
> +    .name = "or32-sim",
> +    .desc = "or32 simulation",
> +    .init = openrisc_sim_init,
> +    .max_cpus = 1,
> +    .is_default = 1,
> +};
> +
> +static void openrisc_sim_machine_init(void)
> +{
> +    qemu_register_machine(&openrisc_sim_machine);
> +}
> +
> +machine_init(openrisc_sim_machine_init);
> --
> 1.7.9.5
>
>