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

[Jia Liu]

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.