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