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;
+
+ PCIBus *bus = s->bus;
+ FWCfgState *fw_cfg = vms->fw_cfg;
/*
* 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;
/* 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;
+ 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
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"
--
2.19.1
+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
+Phil, +Gerd On 11/04/20 20:54, Laszlo Ersek wrote: > +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(-) I realize I never reacted to this patch before (and we're at v9), so you might not welcome my feedback at this point. The explanation why I've only reacted now is the following: there's an agreement between Phil and myself that Phil handles fw_cfg reviews primarily, and I review only (or almost only) Phil's patches for fw_cfg. Furthermore, all versions of this particular patch, as far as I can see, CC'd me but not Phil. So Phil never knew to look, and I never checked (this being an fw_cfg patch, per subject line) beyond the author being Phil or not. The solution is of course to use the get_maintainer.pl script for determining reviewers, and adding them with "Cc:" tags to the commit message. (You also missed Marcel at the least; see my previous response.) git-send-email is supposed to adhere to those "Cc:" tags. The reason I've looked now is... I guess I was too tired to remember our agreement with Phil. Thanks Laszlo
On 2020/11/5 4:05, Laszlo Ersek wrote: > +Phil, +Gerd > > On 11/04/20 20:54, Laszlo Ersek wrote: >> +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(-) > > I realize I never reacted to this patch before (and we're at v9), so you > might not welcome my feedback at this point. Hi Laszlo, Thanks for your comments. Any feedback is welcomed at any time :) > The explanation why I've only reacted now is the following: there's an > agreement between Phil and myself that Phil handles fw_cfg reviews > primarily, and I review only (or almost only) Phil's patches for fw_cfg. > Furthermore, all versions of this particular patch, as far as I can see, > CC'd me but not Phil. So Phil never knew to look, and I never checked > (this being an fw_cfg patch, per subject line) beyond the author being > Phil or not. I did not know the agreement between you and Phil before, and it's my mistake forgetting to CC Phil. I'll check the CC list in the future patches. Thanks Jiahui > The solution is of course to use the get_maintainer.pl script for > determining reviewers, and adding them with "Cc:" tags to the commit > message. (You also missed Marcel at the least; see my previous > response.) git-send-email is supposed to adhere to those "Cc:" tags. > > The reason I've looked now is... I guess I was too tired to remember our > agreement with Phil. >> Thanks > Laszlo > > . >
Hi Laszlo, On 11/4/20 9:05 PM, Laszlo Ersek wrote: > +Phil, +Gerd > > On 11/04/20 20:54, Laszlo Ersek wrote: >> +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(-) > > I realize I never reacted to this patch before (and we're at v9), so you > might not welcome my feedback at this point. > > The explanation why I've only reacted now is the following: there's an > agreement between Phil and myself that Phil handles fw_cfg reviews > primarily, and I review only (or almost only) Phil's patches for fw_cfg. > Furthermore, all versions of this particular patch, as far as I can see, > CC'd me but not Phil. So Phil never knew to look, and I never checked > (this being an fw_cfg patch, per subject line) beyond the author being > Phil or not. I noticed this patch yesterday too (by luck looking at the cover). Since we are in freeze, I tagged it for review but postponed it for in 2 or 3 weeks. Sorry for not mentioning that I would review this later on the list. > The solution is of course to use the get_maintainer.pl script for > determining reviewers, and adding them with "Cc:" tags to the commit > message. (You also missed Marcel at the least; see my previous > response.) git-send-email is supposed to adhere to those "Cc:" tags. > > The reason I've looked now is... I guess I was too tired to remember our > agreement with Phil. > > Thanks > Laszlo >
Hi Phil, On 2020/11/5 5:11, Philippe Mathieu-Daudé wrote: > Hi Laszlo, > > On 11/4/20 9:05 PM, Laszlo Ersek wrote: >> +Phil, +Gerd >> >> On 11/04/20 20:54, Laszlo Ersek wrote: >>> +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(-) >> >> I realize I never reacted to this patch before (and we're at v9), so you >> might not welcome my feedback at this point. >> >> The explanation why I've only reacted now is the following: there's an >> agreement between Phil and myself that Phil handles fw_cfg reviews >> primarily, and I review only (or almost only) Phil's patches for fw_cfg. >> Furthermore, all versions of this particular patch, as far as I can see, >> CC'd me but not Phil. So Phil never knew to look, and I never checked >> (this being an fw_cfg patch, per subject line) beyond the author being >> Phil or not. > > I noticed this patch yesterday too (by luck looking at the cover). > Since we are in freeze, I tagged it for review but postponed it for > in 2 or 3 weeks. Sorry for not mentioning that I would review this > later on the list. Sorry for forgetting to CC you. I'm glad to wait for your review :) Thanks Jiahui > >> The solution is of course to use the get_maintainer.pl script for >> determining reviewers, and adding them with "Cc:" tags to the commit >> message. (You also missed Marcel at the least; see my previous >> response.) git-send-email is supposed to adhere to those "Cc:" tags. >> >> The reason I've looked now is... I guess I was too tired to remember our >> agreement with Phil. >> >> Thanks >> Laszlo >> > > . >
© 2016 - 2026 Red Hat, Inc.