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

[Philippe Mathieu-Daudé]

On 9/19/20 3:00 AM, Huacai Chen wrote:
> Hi, Philippe,
> 
> On Thu, Sep 17, 2020 at 3:53 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
>>
>> 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...
> The physical memory address layout of Loongson-3:
> 0-0x40000000  Low RAM (256MB)
> 0x40000000-0x80000000 Hole for several MMIO registers (256MB)
> 0x80000000-TopOfMemory High RAM
> 
> Thogh this is a virtual platform, but the kernel link address is in
> CKSEG0, so "Low RAM" should exist. Though MMIO is different from real
> hardware, but put it in the same hole can make life easy.

OK...

> 
> Then it seems there is really a mistake of empty_slot_init() but has
> nothing to do with liointc, and the right one should be
> empty_slot_init("fallback", 0x40000000, 0x40000000);

The EMPTY_SLOT models physical slot for busses that don't
generate bus error when the slot is accessed and there is
nothing there.

If the 256MiB region starting at 0x40000000 is reserved for
MMIO registers, you certainly want to get a bus error if the
CPU tries to address an unmapped/illegal address.

If you know some area belong to a device that might be accessed
by firmware/kernel but it isn't important to model it, then you
can create an UNIMP_DEVICE with create_unimplemented_device(),
which behaves as RAZ/WI accesses on the bus.

Regards,

Phil.

> 
> Huacai
>