Hi, Alexandar, On Thu, Jun 11, 2020 at 4:51 PM Aleksandar Markovic <aleksandar.qemu.de...@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