Re: [PATCH for-5.1 V4 3/4] hw/mips: Add Loongson-3 machine support (with KVM)

[Huacai Chen]

Hi, Alexandar,

On Thu, Jun 11, 2020 at 4:51 PM Aleksandar Markovic
<aleksandar.qemu.devel@gmail.com> wrote:
>
> > >>> +    int fd = 0, freq = 0;
> > >>> +    char buf[1024], *buf_p;
>
> 1024 should have been defined via preprocessor constant
>
> > >>> +
> > >>> +    fd = open("/proc/cpuinfo", O_RDONLY);
> > >>> +    if (fd == -1) {
> > >>> +        fprintf(stderr, "Failed to open /proc/cpuinfo!\n");
> > >>> +        return 0;
> > >>> +    }
> > >>> +
> > >>> +    if (read(fd, buf, 1024) < 0) {
>
> The same constant should be used here.
>
> ...
>
> > >>> +    loaderparams.a1 = 0xffffffff80000000ULL + BOOTPARAM_PHYADDR;
> > >>> +    loaderparams.a2 = 0xffffffff80000000ULL + BOOTPARAM_PHYADDR + ret;
>
> What is 0xffffffff80000000ULL? Preprocessor constant possible?
>
> ...
>
> > >>> +    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);
> > >>> +        }
> > >>> +    }
>
> Some explanation needs to be written in comments about the code segment above.
>
> I find the whole segment a little bit questionable. For non-KVM one
> CPU, for KVM another? Why non-KVM can't use both, and allow to be
> specified via command line?
>
> > >>> +    memory_region_add_subregion(address_space_mem, 0x00000000LL, ram);
> > >>> +    memory_region_add_subregion(address_space_mem, 0x1fc00000LL, bios);
> > >>> +    memory_region_add_subregion(address_space_mem, 0x80000000LL, 
> > >>> machine->ram);
> > >>> +    memory_region_add_subregion(address_space_mem, PM_MMIO_ADDR, 
> > >>> iomem);
>
> I would avoid hard coded numbers.
>
> > >>> +
> > >>> +    /*
> > >>> +     * We do not support flash operation, just loading pmon.bin as raw 
> > >>> BIOS.
> > >>> +     * Please use -L to set the BIOS path and -bios to set bios name.
> > >>> +     */
> > >>> +
> > >>> +    if (kernel_filename) {
> > >>> +        loaderparams.ram_size = ram_size;
> > >>> +        loaderparams.kernel_filename = kernel_filename;
> > >>> +        loaderparams.kernel_cmdline = kernel_cmdline;
> > >>> +        loaderparams.initrd_filename = initrd_filename;
> > >>> +        loaderparams.kernel_entry = load_kernel(env);
> > >>> +        rom_add_blob_fixed("bios",
> > >>> +                         bios_boot_code, sizeof(bios_boot_code), 
> > >>> 0x1fc00000LL);
>
> Again, here, 0x1fc00000LL. This should be defined and properly named
> via preprocessor.
>
> > >>> +    } else {
> > >>> +        if (bios_name == NULL) {
> > >>> +                bios_name = LOONGSON3_BIOSNAME;
> > >>> +        }
> > >>> +        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> > >>> +        if (filename) {
> > >>> +            bios_size = load_image_targphys(filename, 0x1fc00000LL,
>
> Again.
>
> > >>> +                                            BIOS_SIZE);
> > >>> +            g_free(filename);
> > >>> +        } else {
> > >>> +            bios_size = -1;
> > >>> +        }
> > >>> +
> > >>> +    if (serial_hd(0)) {
> > >>> +        serial_mm_init(address_space_mem, 0x1fe001e0, 0, env->irq[2],
> > >>> +                           115200, serial_hd(0), DEVICE_NATIVE_ENDIAN);
>
> 115200 should be something like XXX_DEFAULT_BAUDRATE
>
> > >>> +    }
> > >>> +}
> > >>> +
> > >>> +static void mips_loongson3_machine_init(MachineClass *mc)
> > >>> +{
> > >>> +    mc->desc = "Generic Loongson-3 Platform";
> > >>> +    mc->init = mips_loongson3_init;
> > >>> +    mc->block_default_type = IF_IDE;
> > >>> +    mc->max_cpus = LOONGSON_MAX_VCPUS;
> > >>> +    mc->default_ram_id = "loongson3.highram";
> > >>> +    mc->default_ram_size = 1200 * MiB;
> > >>
> > >> 1200MiB looks wired... Why not 1024?
> > > Oh, it is just because our Fedora28 needs more than 1024MB to work
> > > fine, maybe 1280 is better?
> >
> > Ahh if that's the reason then it looks fine for me.
> >
>
> These choices should be documented in brief comments.
>
> If left this way, we leave future developers solve puzzles and
> desperately guessing.
>
> Thanks,
> Aleksandar
Thank you for your advice, I will improve that.

Huacai