Re: [PATCH v9 2/8] fw_cfg: Write the extra roots into the fw_cfg

[Laszlo Ersek]

+Marcel

On 11/03/20 13:01, Jiahui Cen wrote:
> From: Yubo Miao <miaoyubo@huawei.com>
> 
> Write the extra roots into the fw_cfg, therefore the uefi could
> get the extra roots. Only if the uefi knows there are extra roots,
> the config space of devices behind the root could be obtained.
> 
> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> ---
>  hw/arm/virt.c              |  8 ++++++++
>  hw/i386/pc.c               | 18 ++----------------
>  hw/nvram/fw_cfg.c          | 20 ++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h  |  2 ++
>  include/hw/pci/pcie_host.h |  4 ++++
>  5 files changed, 36 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 27dbeb549e..58c3695290 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -78,6 +78,8 @@
>  #include "hw/virtio/virtio-iommu.h"
>  #include "hw/char/pl011.h"
>  #include "qemu/guest-random.h"
> +#include "hw/pci/pci_bus.h"
> +#include "hw/pci/pcie_host.h"
>  
>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -1463,6 +1465,10 @@ void virt_machine_done(Notifier *notifier, void *data)
>      ARMCPU *cpu = ARM_CPU(first_cpu);
>      struct arm_boot_info *info = &vms->bootinfo;
>      AddressSpace *as = arm_boot_address_space(cpu, info);
> +    PCIHostState *s = PCI_GET_PCIE_HOST_STATE;

(1) Not too happy about this new macro.

For pc/q35, see commit 81ed6482a347 ("hw/i386: extend pxb query for all
PC machines", 2015-12-22).

Any reason the same approach cannot work for "virt"? (I.e., saving root
bus 0 in "vms", like we do now in pc_init1().)

> +
> +    PCIBus *bus = s->bus;
> +    FWCfgState *fw_cfg = vms->fw_cfg;

(2) the helper variables "bus" and "fw_cfg" are almost entirely useless
(they don't save any work whatsoever); please just pass "vms->bus" and
"vms->fw_cfg" to the new helper function below.

>  
>      /*
>       * If the user provided a dtb, we assume the dynamic sysbus nodes
> @@ -1481,6 +1487,8 @@ void virt_machine_done(Notifier *notifier, void *data)
>          exit(1);
>      }
>  
> +    fw_cfg_write_extra_pci_roots(bus, fw_cfg);
> +
>      virt_acpi_setup(vms);
>      virt_build_smbios(vms);
>  }
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 5e6c0023e0..bdd2301d19 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -778,26 +778,12 @@ void pc_machine_done(Notifier *notifier, void *data)
>                                          PCMachineState, machine_done);
>      X86MachineState *x86ms = X86_MACHINE(pcms);
>      PCIBus *bus = pcms->bus;
> +    FWCfgState *fw_cfg = x86ms->fw_cfg;

(3) Same as (2): "fw_cfg" is useless (especially because you leave other
prior uses of "x86ms->fw_cfg" in this function untouched); furthermore,
the existent "bus" shorthand *becomes* useless with the extraction of
fw_cfg_write_extra_pci_roots(), so "bus" should be deleted when the bus
walking is factored out.

>  
>      /* set the number of CPUs */
>      x86_rtc_set_cpus_count(x86ms->rtc, x86ms->boot_cpus);
>  
> -    if (bus) {
> -        int extra_hosts = 0;
> -
> -        QLIST_FOREACH(bus, &bus->child, sibling) {
> -            /* look for expander root buses */
> -            if (pci_bus_is_root(bus)) {
> -                extra_hosts++;
> -            }
> -        }
> -        if (extra_hosts && x86ms->fw_cfg) {
> -            uint64_t *val = g_malloc(sizeof(*val));
> -            *val = cpu_to_le64(extra_hosts);
> -            fw_cfg_add_file(x86ms->fw_cfg,
> -                    "etc/extra-pci-roots", val, sizeof(*val));
> -        }
> -    }
> +    fw_cfg_write_extra_pci_roots(bus, fw_cfg);
>  
>      acpi_setup();
>      if (x86ms->fw_cfg) {
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 08539a1aab..33dcbdd31d 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -40,6 +40,7 @@
>  #include "qemu/cutils.h"
>  #include "qapi/error.h"
>  #include "hw/acpi/aml-build.h"
> +#include "hw/pci/pci_bus.h"
>  
>  #define FW_CFG_FILE_SLOTS_DFLT 0x20
>  
> @@ -742,6 +743,25 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, 
> uint16_t key,
>      return ptr;
>  }
>  
> +void fw_cfg_write_extra_pci_roots(PCIBus *bus, FWCfgState *s)
> +{
> +    if (bus) {
> +        int extra_hosts = 0;

(4) I don't understand why we need to remove the empty line after this
declaration. It helps understanding if we have an empty line between
declarations and code.

(5) Simpler to return at once if bus is NULL; the extra nesting is
unjustified in this helper function, which has its own top-level scope.

> +        QLIST_FOREACH(bus, &bus->child, sibling) {
> +            /* look for expander root buses */
> +            if (pci_bus_is_root(bus)) {
> +                extra_hosts++;
> +            }
> +        }
> +        if (extra_hosts && s) {
> +            uint64_t *val = g_malloc(sizeof(*val));
> +            *val = cpu_to_le64(extra_hosts);
> +            fw_cfg_add_file(s,
> +                   "etc/extra-pci-roots", val, sizeof(*val));
> +        }
> +    }
> +}
> +
>  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
>  {
>      trace_fw_cfg_add_bytes(key, trace_key_name(key), len);
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 8a9f5738bf..0dc75ba6ca 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -104,6 +104,8 @@ struct FWCfgMemState {
>      MemoryRegionOps wide_data_ops;
>  };
>  
> +void fw_cfg_write_extra_pci_roots(PCIBus *bus, FWCfgState *s);
> +
>  /**
>   * fw_cfg_add_bytes:
>   * @s: fw_cfg device being modified

(6) Missing documentation (interface contract).

(7) I suggest calling the function fw_cfg_add_extra_pci_roots(), to
conform to the fw_cfg_add_* pattern.

(8) Also, likely not the best place to introduce this function in the
header file. The function contracts tend to grow in complexity towards
the end of the file. (Note that this applies to the "hw/nvram/fw_cfg.c"
file as well.)

> diff --git a/include/hw/pci/pcie_host.h b/include/hw/pci/pcie_host.h
> index 076457b270..12f48ddd59 100644
> --- a/include/hw/pci/pcie_host.h
> +++ b/include/hw/pci/pcie_host.h
> @@ -27,6 +27,10 @@
>  
>  #define TYPE_PCIE_HOST_BRIDGE "pcie-host-bridge"
>  OBJECT_DECLARE_SIMPLE_TYPE(PCIExpressHost, PCIE_HOST_BRIDGE)
> +#define PCI_GET_PCIE_HOST_STATE \
> +    OBJECT_CHECK(PCIHostState, \
> +                 object_resolve_path_type("", "pcie-host-bridge", NULL), \
> +                 TYPE_PCIE_HOST_BRIDGE)
>  
>  #define PCIE_HOST_MCFG_BASE "MCFG"
>  #define PCIE_HOST_MCFG_SIZE "mcfg_size"
> 

(9) According to (1), I suggest dropping this hunk.

(10) Please split this patch into two patches; the first patch should
factor fw_cfg_add_extra_pci_roots() out of pc_machine_done(), while the
second patch should hook it into the "virt" machine.

Thanks
Laszlo