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

[Peter Crosthwaite]

On Thu, Jun 28, 2012 at 11:56 PM, Jia Liu <address@hidden> wrote:
> Hi, Peter, Andreas, and Peter,
>
> I've replace env with cpu, and rewrite the load_kernel.
> Please review these new files:
>
> openrisc_pic.c
>
> #include "hw.h"
> #include "cpu.h"
>
> /* OpenRISC pic handler */
> static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
> {
>    OpenRISCCPU *cpu = (OpenRISCCPU *)opaque;
>    int i;
>    uint32_t irq_bit = 1 << irq;
>
>    if (irq > 31 || irq < 0) {
>        return;
>    }
>
>    if (level) {
>        cpu->env.picsr |= irq_bit;
>    } else {
>        cpu->env.picsr &= ~irq_bit;
>    }
>
>    for (i = 0; i < 32; i++) {
>        if ((cpu->env.picsr && (1 << i)) && (cpu->env.picmr && (1 << i))) {
>            cpu_interrupt(&cpu->env, CPU_INTERRUPT_HARD);
>        } else {
>            cpu_reset_interrupt(&cpu->env, CPU_INTERRUPT_HARD);
>            cpu->env.picsr &= ~(1 << i);
>        }
>    }
> }
>
> void cpu_openrisc_pic_init(OpenRISCCPU *cpu)
> {
>    int i;
>    qemu_irq *qi;
>    qi = qemu_allocate_irqs(openrisc_pic_cpu_handler, cpu, NR_IRQS);
>
>    for (i = 0; i < NR_IRQS; i++) {
>        cpu->env.irq[i] = qi[i];
>    }
> }
> --------------------------------------------------------------------
>
> openrisc_timer.c
>
> #include "cpu.h"
> #include "hw.h"
> #include "qemu-timer.h"
>
> #define TIMER_FREQ    (20 * 1000 * 1000)    /* 20MHz */
>
> /* The time when TTCR changes */
> static uint64_t last_clk;
> static int is_counting;
>
> void cpu_openrisc_count_update(OpenRISCCPU *cpu)
> {
>    uint64_t now, next;
>    uint32_t wait;
>
>    now = qemu_get_clock_ns(vm_clock);
>    if (!is_counting) {
>        qemu_del_timer(cpu->env.timer);
>        last_clk = now;
>        return;
>    }
>
>    cpu->env.ttcr += (uint32_t)muldiv64(now - last_clk, TIMER_FREQ,
>                                        get_ticks_per_sec());
>    last_clk = now;
>
>    if ((cpu->env.ttmr & TTMR_TP) <= (cpu->env.ttcr & TTMR_TP)) {
>        wait = TTMR_TP - (cpu->env.ttcr & TTMR_TP) + 1;
>        wait += cpu->env.ttmr & TTMR_TP;
>    } else {
>        wait = (cpu->env.ttmr & TTMR_TP) - (cpu->env.ttcr & TTMR_TP);
>    }
>
>    next = now + muldiv64(wait, get_ticks_per_sec(), TIMER_FREQ);
>    qemu_mod_timer(cpu->env.timer, next);
> }
>
> void cpu_openrisc_count_start(OpenRISCCPU *cpu)
> {
>    is_counting = 1;
>    cpu_openrisc_count_update(cpu);
> }
>
> void cpu_openrisc_count_stop(OpenRISCCPU *cpu)
> {
>    is_counting = 0;
>    cpu_openrisc_count_update(cpu);
> }
>
> static void openrisc_timer_cb(void *opaque)
> {
>    OpenRISCCPU *cpu = opaque;
>
>    if ((cpu->env.ttmr & TTMR_IE) &&
>         qemu_timer_expired(cpu->env.timer, qemu_get_clock_ns(vm_clock))) {
>        cpu->env.ttmr |= TTMR_IP;
>        cpu->env.interrupt_request |= CPU_INTERRUPT_TIMER;
>    }
>
>    switch (cpu->env.ttmr & TTMR_M) {
>    case TIMER_NONE:
>        break;
>    case TIMER_INTR:
>        cpu->env.ttcr = 0;
>        cpu_openrisc_count_start(cpu);
>        break;
>    case TIMER_SHOT:
>        cpu_openrisc_count_stop(cpu);
>        break;
>    case TIMER_CONT:
>        cpu_openrisc_count_start(cpu);
>        break;
>    }
> }
>
> void cpu_openrisc_clock_init(OpenRISCCPU *cpu)
> {
>    cpu->env.timer = qemu_new_timer_ns(vm_clock, &openrisc_timer_cb, cpu);
>    cpu->env.ttmr = 0x00000000;
>    cpu->env.ttcr = 0x00000000;
> }
> --------------------------------------------------------------------
>
> openrisc_sim.c
>
> #include "hw.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
>
> static void main_cpu_reset(void *opaque)
> {
>    OpenRISCCPU *cpu = opaque;
>
>    cpu_reset(CPU(cpu));
> }
>
> 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 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)
> {
>    long kernel_size;
>    uint64_t elf_entry;
>    target_phys_addr_t entry;
>    OpenRISCCPU *cpu = NULL;
>    MemoryRegion *ram;
>    int n;
>
>    if (!cpu_model) {
>        cpu_model = "or1200";
>    }
>
>    for (n = 0; n < smp_cpus; n++) {
>        cpu = cpu_openrisc_init(cpu_model);
>        if (cpu == NULL) {
>            qemu_log("Unable to find CPU defineition!\n");
>            exit(1);
>        }
>        qemu_register_reset(main_cpu_reset, cpu);
>        main_cpu_reset(cpu);
>    }
>
>    ram = g_malloc(sizeof(*ram));
>    memory_region_init_ram(ram, "openrisc.ram", ram_size);
>    vmstate_register_ram_global(ram);
>    memory_region_add_subregion(get_system_memory(), 0, ram);
>
>    /* load kernel */
>    if (kernel_filename && !qtest_enabled()) {
>        kernel_size = load_elf(kernel_filename, NULL, NULL,
>                               &elf_entry, NULL, NULL, 1, ELF_MACHINE, 1);
>        entry = elf_entry;
>        if (kernel_size < 0) {
>            kernel_size = load_uimage(kernel_filename,
>                                      &entry, NULL, NULL);
>        }
>        if (kernel_size < 0) {
>            kernel_size = load_image_targphys(kernel_filename,
>                                              KERNEL_LOAD_ADDR,
>                                              ram_size - KERNEL_LOAD_ADDR);
>            entry = KERNEL_LOAD_ADDR;
>        }
>
>        if (kernel_size < 0) {
>            qemu_log("QEMU: couldn't load the kernel '%s'\n",
>                    kernel_filename);
>            exit(1);
>        }
>    }

I think it was cleaner to have the load_kernel() as its own function,
just without all the dead struct args. Seperates bootloader from
machine init and add a little bit of future proofing when we come to
extract out bootloaders from machine models. But I wont block on it.
Can you move this hunk (or the load_kernel() call) to the end of the
function so bootloading happens after machine creation?

Regards,
Peter

>
>    cpu_openrisc_pic_init(cpu);
>    cpu_openrisc_clock_init(cpu);
>
>    serial_mm_init(get_system_memory(), 0x90000000, 0, cpu->env.irq[2],
>                   115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
>
>    if (nd_table[0].vlan) {
>        openrisc_sim_net_init(get_system_memory(), 0x92000000,
>                              0x92000400, cpu->env.irq[4], nd_table);
>    }

So after this stuff. Bootload here.

>
>    cpu->env.pc = entry;

I would roll this into the load_kernel call too if you went for that
approach. Part of loading a kernel is setting the entry point so it
makes sense to have one BL function do everything. Again, I wont
block, just a suggestion.

> }
>
> 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);
>
>
> Please review and let me the new is OK or not, please.
> Thank you, very much, all of you.
>
>
> On Wed, Jun 27, 2012 at 9:10 PM, Peter Crosthwaite
> <address@hidden> wrote:
>> On Wed, Jun 27, 2012 at 11:04 PM, Andreas Färber <address@hidden> wrote:
>>> Am 27.06.2012 14:25, schrieb Peter Crosthwaite:
>>>> Hi Jia,
>>>>
>>>> On Wed, Jun 27, 2012 at 7:54 PM, Jia Liu <address@hidden> wrote:
>>>>> +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.
>>>
>>> Thought I had commented that already... maybe I'm starting to confuse
>>> uc32 and or32? :) Yes please, cpu_or32_init() should be called and
>>> return an OpenRISCCPU *cpu. main_cpu_reset() should be passed the cpu,
>>> too. All new APIs (static helpers etc.) should use OpenRISCCPU, not
>>> CPUOpenRISCState.
>>
>> That rule has significant impact on patches 9-10. Andreas, you may
>> wish to check these out - they are psuedo device-models tightly
>> coupled to the cpu env.
>>
>> Regards,
>> Peter
>>
>> That will greatly simplify moving forward.
>>>
>>> Thanks for catching this, Peter.
>>>
>>> Andreas
>>>
>>> --
>>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
>
> Regards,
> Jia.