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

[Huacai Chen <chenhuacai@gmail.com>]

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.

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);

Huacai