Hi, Philippe, On Sat, Sep 19, 2020 at 9:59 PM Philippe Mathieu-Daudé <f4...@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é <f4...@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é <f4...@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