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
