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

[chen huacai]

Hi, Philippe,

On Sat, Sep 19, 2020 at 9:59 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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.
Yes, there are some MMIO access from firmware/kernel that doesn't
belong to any emulated devices, then I found that "empty slot" and
"unimplemented device" is nearly the same thing, what are their
differences?

Huacai
>
> Regards,
>
> Phil.
>
> >
> > Huacai
> >



-- 
Huacai Chen