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

[Aleksandar Markovic]

> >>> +    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