Re: [PATCH V9 6/6] hw/mips: Add Loongson-3 machine support

[Philippe Mathieu-Daudé]

On 9/16/20 11:49 AM, Huacai Chen wrote:
> On Wed, Sep 16, 2020 at 3:56 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
>> On 9/16/20 4:12 AM, Huacai Chen wrote:
[...]
>>> +static void mips_loongson3_virt_init(MachineState *machine)
>>> +{
>>> +    int i;
>>> +    long bios_size;
>>> +    MIPSCPU *cpu;
>>> +    CPUMIPSState *env;
>>> +    DeviceState *liointc;
>>> +    char *filename;
>>> +    const char *kernel_cmdline = machine->kernel_cmdline;
>>> +    const char *kernel_filename = machine->kernel_filename;
>>> +    const char *initrd_filename = machine->initrd_filename;
>>> +    ram_addr_t ram_size = machine->ram_size;
>>> +    MemoryRegion *address_space_mem = get_system_memory();
>>> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
>>> +    MemoryRegion *bios = g_new(MemoryRegion, 1);
>>> +    MemoryRegion *iomem = g_new(MemoryRegion, 1);
>>> +
>>> +    /* TODO: TCG will support all CPU types */
>>> +    if (!kvm_enabled()) {
>>> +        if (!machine->cpu_type) {
>>> +            machine->cpu_type = MIPS_CPU_TYPE_NAME("Loongson-3A1000");
>>> +        }
>>> +        if (!strstr(machine->cpu_type, "Loongson-3A1000")) {
>>> +            error_report("Loongson-3/TCG need cpu type Loongson-3A1000");
>>> +            exit(1);
>>> +        }
>>> +    } else {
>>> +        if (!machine->cpu_type) {
>>> +            machine->cpu_type = MIPS_CPU_TYPE_NAME("Loongson-3A4000");
>>> +        }
>>> +        if (!strstr(machine->cpu_type, "Loongson-3A4000")) {
>>> +            error_report("Loongson-3/KVM need cpu type Loongson-3A4000");
>>> +            exit(1);
>>> +        }
>>> +    }
>>> +
>>> +    if (ram_size < 512 * MiB) {
>>> +        error_report("Loongson-3 need at least 512MB memory");
>>
>> Typo "needs", but why?
> Though you told me "QEMU shouldn't assume anything about the guest",
> but Loongson-3 machine really need at least 512M memory. And as you
> said, this can simplify the memsize/highmemsize process (always larger
> than 256).

OK, that's fine.

> 
>>
>>> +        exit(1);
>>> +    }
>>> +
>>> +    /*
>>> +     * The whole MMIO range among configure registers doesn't generate
>>> +     * exception when accessing invalid memory. Create an empty slot to
>>> +     * emulate this feature.
>>> +     */
>>> +    empty_slot_init("fallback", 0, 0x80000000);
>>
>> Again, this doesn't look correct (no comment in my previous review).
> This is written by Jiaxun because this is only needed by TCG, and he
> said that malta also uses empty_slot_init() here.

IIRC for Malta this is a GT64120 specific hole.

In this case I'd like to know the justification first.
Maybe you want to add this hole in the LOONGSON_LIOINTC device...

> 
>>
>>> +
>>> +    liointc = qdev_new("loongson.liointc");
>>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(liointc), &error_fatal);
>>> +
>>> +    sysbus_mmio_map(SYS_BUS_DEVICE(liointc), 0, 
>>> virt_memmap[VIRT_LIOINTC].base);
>>> +
>>> +    for (i = 0; i < machine->smp.cpus; i++) {
>>> +        int ip;
>>> +
>>> +        /* init CPUs */
>>> +        cpu = MIPS_CPU(cpu_create(machine->cpu_type));
>>> +
>>> +        /* Init internal devices */
>>> +        cpu_mips_irq_init_cpu(cpu);
>>> +        cpu_mips_clock_init(cpu);
>>> +        qemu_register_reset(main_cpu_reset, cpu);
>>> +
>>> +        if (i >= 4) {
>>> +            continue; /* Only node-0 can be connected to LIOINTC */
>>> +        }
>>> +
>>> +        for (ip = 0; ip < 4 ; ip++) {
>>> +            int pin = i * 4 + ip;
>>> +            sysbus_connect_irq(SYS_BUS_DEVICE(liointc),
>>> +                               pin, cpu->env.irq[ip + 2]);
>>> +        }
>>> +    }
>>> +    env = &MIPS_CPU(first_cpu)->env;
>>> +
>>> +    /* Allocate RAM/BIOS, 0x00000000~0x10000000 is alias of 
>>> 0x80000000~0x90000000 */
>>> +    memory_region_init_rom(bios, NULL, "loongson3.bios",
>>> +                           virt_memmap[VIRT_BIOS_ROM].size, &error_fatal);
>>> +    memory_region_init_alias(ram, NULL, "loongson3.lowmem",
>>> +                           machine->ram, 0, virt_memmap[VIRT_LOWMEM].size);
>>> +    memory_region_init_io(iomem, NULL, &loongson3_pm_ops,
>>> +                           NULL, "loongson3_pm", 
>>> virt_memmap[VIRT_PM].size);
>>> +
>>> +    memory_region_add_subregion(address_space_mem,
>>> +                      virt_memmap[VIRT_LOWMEM].base, ram);
>>> +    memory_region_add_subregion(address_space_mem,
>>> +                      virt_memmap[VIRT_BIOS_ROM].base, bios);
>>> +    memory_region_add_subregion(address_space_mem,
>>> +                      virt_memmap[VIRT_HIGHMEM].base, machine->ram);
>>> +    memory_region_add_subregion(address_space_mem,
>>> +                      virt_memmap[VIRT_PM].base, iomem);
>>> +
[...]