Re: [PULL 03/10] microvm: add device tree support.

[Peter Maydell]

On Tue, 2 Nov 2021 at 16:48, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Allows edk2 detect virtio-mmio devices and pcie ecam.
> See comment in hw/i386/microvm-dt.c for more details.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Sergio Lopez <slp@redhat.com>
> Message-Id: <20211014193617.2475578-1-kraxel@redhat.com>


Hi; Coverity points out a couple of issues in this code.

> +static void dt_add_pcie(MicrovmMachineState *mms)
> +{
> +    hwaddr base = PCIE_MMIO_BASE;
> +    int nr_pcie_buses;
> +    char *nodename;
> +
> +    nodename = g_strdup_printf("/pcie@%" PRIx64, base);

Here we allocate 'nodename'...

> +    qemu_fdt_add_subnode(mms->fdt, nodename);
> +    qemu_fdt_setprop_string(mms->fdt, nodename,
> +                            "compatible", "pci-host-ecam-generic");
> +    qemu_fdt_setprop_string(mms->fdt, nodename, "device_type", "pci");
> +    qemu_fdt_setprop_cell(mms->fdt, nodename, "#address-cells", 3);
> +    qemu_fdt_setprop_cell(mms->fdt, nodename, "#size-cells", 2);
> +    qemu_fdt_setprop_cell(mms->fdt, nodename, "linux,pci-domain", 0);
> +    qemu_fdt_setprop(mms->fdt, nodename, "dma-coherent", NULL, 0);
> +
> +    qemu_fdt_setprop_sized_cells(mms->fdt, nodename, "reg",
> +                                 2, PCIE_ECAM_BASE, 2, PCIE_ECAM_SIZE);
> +    if (mms->gpex.mmio64.size) {
> +        qemu_fdt_setprop_sized_cells(mms->fdt, nodename, "ranges",
> +
> +                                     1, FDT_PCI_RANGE_MMIO,
> +                                     2, mms->gpex.mmio32.base,
> +                                     2, mms->gpex.mmio32.base,
> +                                     2, mms->gpex.mmio32.size,
> +
> +                                     1, FDT_PCI_RANGE_MMIO_64BIT,
> +                                     2, mms->gpex.mmio64.base,
> +                                     2, mms->gpex.mmio64.base,
> +                                     2, mms->gpex.mmio64.size);
> +    } else {
> +        qemu_fdt_setprop_sized_cells(mms->fdt, nodename, "ranges",
> +
> +                                     1, FDT_PCI_RANGE_MMIO,
> +                                     2, mms->gpex.mmio32.base,
> +                                     2, mms->gpex.mmio32.base,
> +                                     2, mms->gpex.mmio32.size);
> +    }
> +
> +    nr_pcie_buses = PCIE_ECAM_SIZE / PCIE_MMCFG_SIZE_MIN;
> +    qemu_fdt_setprop_cells(mms->fdt, nodename, "bus-range", 0,
> +                           nr_pcie_buses - 1);

...but unlike other functions in this file we forget to free it.
(CID 1465240)

Might be worth considering use of g_autofree.

> +}

> +void dt_setup_microvm(MicrovmMachineState *mms)
> +{
> +    X86MachineState *x86ms = X86_MACHINE(mms);
> +    int size = 0;
> +
> +    mms->fdt = create_device_tree(&size);
> +
> +    /* root node */
> +    qemu_fdt_setprop_string(mms->fdt, "/", "compatible", "linux,microvm");
> +    qemu_fdt_setprop_cell(mms->fdt, "/", "#address-cells", 0x2);
> +    qemu_fdt_setprop_cell(mms->fdt, "/", "#size-cells", 0x2);
> +
> +    qemu_fdt_add_subnode(mms->fdt, "/chosen");
> +    dt_setup_sys_bus(mms);
> +
> +    /* add to fw_cfg */
> +    fprintf(stderr, "%s: add etc/fdt to fw_cfg\n", __func__);
> +    fw_cfg_add_file(x86ms->fw_cfg, "etc/fdt", mms->fdt, size);
> +
> +    if (debug) {
> +        fprintf(stderr, "%s: writing microvm.fdt\n", __func__);
> +        g_file_set_contents("microvm.fdt", mms->fdt, size, NULL);

g_file_set_contents() returns a value to indicate success or
failure; we should check it.
(CID 1465239)

> +        int ret = system("dtc -I dtb -O dts microvm.fdt");
> +        if (ret != 0) {
> +            fprintf(stderr, "%s: oops, dtc not installed?\n", __func__);
> +        }
> +    }
> +}

thanks
-- PMM