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

[Philippe Mathieu-Daudé]

On 9/16/20 12:47 PM, Philippe Mathieu-Daudé wrote:
> 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...

Which makes me also wonder why are you splitting out 256MB of the RAM?

This was a physical restriction of the GT64120 on 32-bit targets.
Your hardware is virtual and 64-bit...