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

[Peter Crosthwaite]

Looks good to me,

Peter

On Fri, Jun 29, 2012 at 3:56 PM, Jia Liu <address@hidden> wrote:
> Hi Peter,
>
> On Fri, Jun 29, 2012 at 11:15 AM, Peter Crosthwaite
> <address@hidden> wrote:
>> 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.
>>
>
> Thanks for review. And, thank you very much for your suggestion.
>
> Is this new code OK?
>
> static void cpu_openrisc_load_kernel(ram_addr_t ram_size,
>                                     const char* kernel_filename,
>                                     OpenRISCCPU *cpu)
> {
>    long kernel_size;
>    uint64_t elf_entry;
>    target_phys_addr_t entry;
>
>    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);
>        }
>    }
>
>    cpu->env.pc = 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)
> {
>   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);
>
>    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);
>    }
>
>    cpu_openrisc_load_kernel(ram_size, kernel_filename, cpu);
> }
>
>>> }
>>>
>>> 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.
>
> Regards,
> Jia.