[PATCH 0/6] hw/nvram/fw_cfg: Move PCI bus methods out

[PATCH 0/6] hw/nvram/fw_cfg: Move PCI bus methods out

[Philippe Mathieu-Daudé]

In order to keep fw_cfg device model clean, remove the PCI
bus specific code. Instead, the equivalent functionality is
implemented within the PCI_BUS object in hw/pci/,
implementing TYPE_FW_CFG_DATA_GENERATOR_INTERFACE.

Philippe Mathieu-Daudé (6):
  hw/nvram/fw_cfg: Rename fw_cfg_add_[file]_from_generator()
  hw/nvram/fw_cfg: Pass QOM parent to fw_cfg_add_file_from_generator()
  hw/pci: Have PCI_BUS implement TYPE_FW_CFG_DATA_GENERATOR_INTERFACE
  hw/pci: Add pci_bus_add_fw_cfg_extra_pci_roots() helper
  hw: Use pci_bus_add_fw_cfg_extra_pci_roots()
  hw/nvram/fw_cfg: Remove fw_cfg_add_extra_pci_roots()

 include/hw/nvram/fw_cfg.h | 21 ++++++----------
 include/hw/pci/pci.h      |  3 +++
 hw/arm/virt.c             |  3 ++-
 hw/hppa/machine.c         |  2 +-
 hw/i386/pc.c              |  3 ++-
 hw/nvram/fw_cfg.c         | 34 +++++--------------------
 hw/pci/pci.c              | 53 +++++++++++++++++++++++++++++++++++++++
 system/vl.c               |  3 ++-
 8 files changed, 76 insertions(+), 46 deletions(-)

-- 
2.45.2

[PATCH 1/6] hw/nvram/fw_cfg: Rename fw_cfg_add_[file]_from_generator()

[Philippe Mathieu-Daudé]

fw_cfg_add_from_generator() is adding a 'file' entry,
so rename as fw_cfg_add_file_from_generator() for
clarity. Besides, we might introduce generators for
other entry types.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/nvram/fw_cfg.h | 6 +++---
 hw/nvram/fw_cfg.c         | 4 ++--
 system/vl.c               | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index fa426776192..14e68966c59 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -291,7 +291,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
                          size_t len);
 
 /**
- * fw_cfg_add_from_generator:
+ * fw_cfg_add_file_from_generator:
  * @s: fw_cfg device being modified
  * @filename: name of new fw_cfg file item
  * @gen_id: name of object implementing FW_CFG_DATA_GENERATOR interface
@@ -307,8 +307,8 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
  *
  * Returns: %true on success, %false on error.
  */
-bool fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
-                               const char *gen_id, Error **errp);
+bool fw_cfg_add_file_from_generator(FWCfgState *s, const char *filename,
+                                    const char *gen_id, Error **errp);
 
 /**
  * fw_cfg_add_extra_pci_roots:
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b644577734c..fe3b86135a7 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1027,8 +1027,8 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
     return NULL;
 }
 
-bool fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
-                               const char *gen_id, Error **errp)
+bool fw_cfg_add_file_from_generator(FWCfgState *s, const char *filename,
+                                    const char *gen_id, Error **errp)
 {
     FWCfgDataGeneratorClass *klass;
     GByteArray *array;
diff --git a/system/vl.c b/system/vl.c
index 2f855d83fbb..f103532a9a1 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -1184,7 +1184,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
         size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
         buf = g_memdup(str, size);
     } else if (nonempty_str(gen_id)) {
-        if (!fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp)) {
+        if (!fw_cfg_add_file_from_generator(fw_cfg, name, gen_id, errp)) {
             return -1;
         }
         return 0;
-- 
2.45.2

[PATCH 2/6] hw/nvram/fw_cfg: Pass QOM parent to fw_cfg_add_file_from_generator()

[Philippe Mathieu-Daudé]

Currently fw_cfg_add_file_from_generator() is restricted
to command line created objects which reside in the
'/objects' QOM container. In order to extend to other
types of containers, pass the QOM parent by argument.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/nvram/fw_cfg.h | 10 ++++++----
 hw/nvram/fw_cfg.c         | 11 ++++++-----
 system/vl.c               |  3 ++-
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 14e68966c59..fcb06f18cc3 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -294,11 +294,12 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
  * fw_cfg_add_file_from_generator:
  * @s: fw_cfg device being modified
  * @filename: name of new fw_cfg file item
- * @gen_id: name of object implementing FW_CFG_DATA_GENERATOR interface
+ * @part: name of object implementing FW_CFG_DATA_GENERATOR interface
+ * @parent: the object in which to resolve the @part
  * @errp: pointer to a NULL initialized error object
  *
  * Add a new NAMED fw_cfg item with the content generated from the
- * @gen_id object. The data generated by the @gen_id object is copied
+ * @part object. The data generated by the @part object is copied
  * into the data structure of the fw_cfg device.
  * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
  * will be used; also, a new entry will be added to the file directory
@@ -307,8 +308,9 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
  *
  * Returns: %true on success, %false on error.
  */
-bool fw_cfg_add_file_from_generator(FWCfgState *s, const char *filename,
-                                    const char *gen_id, Error **errp);
+bool fw_cfg_add_file_from_generator(FWCfgState *s,
+                                    Object *parent, const char *part,
+                                    const char *filename, Error **errp);
 
 /**
  * fw_cfg_add_extra_pci_roots:
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index fe3b86135a7..b94cd27bd85 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1027,22 +1027,23 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
     return NULL;
 }
 
-bool fw_cfg_add_file_from_generator(FWCfgState *s, const char *filename,
-                                    const char *gen_id, Error **errp)
+bool fw_cfg_add_file_from_generator(FWCfgState *s,
+                                    Object *parent, const char *part,
+                                    const char *filename, Error **errp)
 {
     FWCfgDataGeneratorClass *klass;
     GByteArray *array;
     Object *obj;
     gsize size;
 
-    obj = object_resolve_path_component(object_get_objects_root(), gen_id);
+    obj = object_resolve_path_component(parent, part);
     if (!obj) {
-        error_setg(errp, "Cannot find object ID '%s'", gen_id);
+        error_setg(errp, "Cannot find object ID '%s'", part);
         return false;
     }
     if (!object_dynamic_cast(obj, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) {
         error_setg(errp, "Object ID '%s' is not a '%s' subclass",
-                   gen_id, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE);
+                   part, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE);
         return false;
     }
     klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
diff --git a/system/vl.c b/system/vl.c
index f103532a9a1..4a370da624a 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -1184,7 +1184,8 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
         size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
         buf = g_memdup(str, size);
     } else if (nonempty_str(gen_id)) {
-        if (!fw_cfg_add_file_from_generator(fw_cfg, name, gen_id, errp)) {
+        if (!fw_cfg_add_file_from_generator(fw_cfg, object_get_objects_root(),
+                                            gen_id, name, errp)) {
             return -1;
         }
         return 0;
-- 
2.45.2

[PATCH 3/6] hw/pci: Have PCI_BUS implement TYPE_FW_CFG_DATA_GENERATOR_INTERFACE

[Philippe Mathieu-Daudé]

The FW_CFG_DATA_GENERATOR allows any object to produce
blob of data consumable by the fw_cfg device. Implement
that for PCI bus objects.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci/pci.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 1416ae202c3..8844251eceb 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -46,6 +46,7 @@
 #include "hw/pci/msix.h"
 #include "hw/hotplug.h"
 #include "hw/boards.h"
+#include "hw/nvram/fw_cfg.h"
 #include "qapi/error.h"
 #include "qemu/cutils.h"
 #include "pci-internal.h"
@@ -216,11 +217,41 @@ static uint16_t pcibus_numa_node(PCIBus *bus)
     return NUMA_NODE_UNASSIGNED;
 }
 
+static GByteArray *pci_bus_fw_cfg_gen_data(Object *obj, Error **errp)
+{
+    PCIBus *bus = PCI_BUS(obj);
+    GByteArray *byte_array;
+    uint64_t extra_hosts = 0;
+
+    if (!bus) {
+        return NULL;
+    }
+
+    QLIST_FOREACH(bus, &bus->child, sibling) {
+        /* look for expander root buses */
+        if (pci_bus_is_root(bus)) {
+            extra_hosts++;
+        }
+    }
+
+    if (!extra_hosts) {
+        return NULL;
+    }
+    extra_hosts = cpu_to_le64(extra_hosts);
+
+    byte_array = g_byte_array_new();
+    g_byte_array_append(byte_array,
+                        (const void *)&extra_hosts, sizeof(extra_hosts));
+
+    return byte_array;
+}
+
 static void pci_bus_class_init(ObjectClass *klass, void *data)
 {
     BusClass *k = BUS_CLASS(klass);
     PCIBusClass *pbc = PCI_BUS_CLASS(klass);
     ResettableClass *rc = RESETTABLE_CLASS(klass);
+    FWCfgDataGeneratorClass *fwgc = FW_CFG_DATA_GENERATOR_CLASS(klass);
 
     k->print_dev = pcibus_dev_print;
     k->get_dev_path = pcibus_get_dev_path;
@@ -232,6 +263,8 @@ static void pci_bus_class_init(ObjectClass *klass, void *data)
 
     pbc->bus_num = pcibus_num;
     pbc->numa_node = pcibus_numa_node;
+
+    fwgc->get_data = pci_bus_fw_cfg_gen_data;
 }
 
 static const TypeInfo pci_bus_info = {
@@ -240,6 +273,10 @@ static const TypeInfo pci_bus_info = {
     .instance_size = sizeof(PCIBus),
     .class_size = sizeof(PCIBusClass),
     .class_init = pci_bus_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_FW_CFG_DATA_GENERATOR_INTERFACE },
+        { }
+    }
 };
 
 static const TypeInfo cxl_interface_info = {
-- 
2.45.2

[PATCH 4/6] hw/pci: Add pci_bus_add_fw_cfg_extra_pci_roots() helper

[Philippe Mathieu-Daudé]

pci_bus_add_fw_cfg_extra_pci_roots() calls the fw_cfg
API with PCI bus specific arguments.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci/pci.h |  3 +++
 hw/pci/pci.c         | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index c0717e31219..603c456c3a8 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -297,6 +297,9 @@ int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
 uint32_t pci_bus_get_slot_reserved_mask(PCIBus *bus);
 void pci_bus_set_slot_reserved_mask(PCIBus *bus, uint32_t mask);
 void pci_bus_clear_slot_reserved_mask(PCIBus *bus, uint32_t mask);
+bool pci_bus_add_fw_cfg_extra_pci_roots(FWCfgState *fw_cfg,
+                                        PCIBus *bus,
+                                        Error **errp);
 /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
 static inline int pci_swizzle(int slot, int pin)
 {
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8844251eceb..bf0a1840dbe 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -217,6 +217,22 @@ static uint16_t pcibus_numa_node(PCIBus *bus)
     return NUMA_NODE_UNASSIGNED;
 }
 
+bool pci_bus_add_fw_cfg_extra_pci_roots(FWCfgState *fw_cfg,
+                                        PCIBus *bus,
+                                        Error **errp)
+{
+    Object *obj;
+
+    if (!bus) {
+        return true;
+    }
+    obj = OBJECT(bus);
+
+    return fw_cfg_add_file_from_generator(fw_cfg, obj->parent,
+                                          object_get_canonical_path_component(obj),
+                                          "etc/extra-pci-roots", errp);
+}
+
 static GByteArray *pci_bus_fw_cfg_gen_data(Object *obj, Error **errp)
 {
     PCIBus *bus = PCI_BUS(obj);
-- 
2.45.2

[PATCH 5/6] hw: Use pci_bus_add_fw_cfg_extra_pci_roots()

[Philippe Mathieu-Daudé]

We want to remove fw_cfg_add_extra_pci_roots() which introduced
PCI bus knowledge within the generic hw/nvram/fw_cfg.c file.
Replace the calls by the pci_bus_add_fw_cfg_extra_pci_roots()
which is a 1:1 equivalent, but using correct API.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/virt.c     | 3 ++-
 hw/hppa/machine.c | 2 +-
 hw/i386/pc.c      | 3 ++-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1a381e9a2bd..191e1d65a8f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1750,7 +1750,8 @@ void virt_machine_done(Notifier *notifier, void *data)
         exit(1);
     }
 
-    fw_cfg_add_extra_pci_roots(vms->bus, vms->fw_cfg);
+    pci_bus_add_fw_cfg_extra_pci_roots(vms->fw_cfg, vms->bus,
+                                       &error_abort);
 
     virt_acpi_setup(vms);
     virt_build_smbios(vms);
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index a31dc32a9f7..4e673353225 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -240,7 +240,7 @@ static FWCfgState *create_fw_cfg(MachineState *ms, PCIBus *pci_bus,
                     g_memdup2(qemu_version, sizeof(qemu_version)),
                     sizeof(qemu_version));
 
-    fw_cfg_add_extra_pci_roots(pci_bus, fw_cfg);
+    pci_bus_add_fw_cfg_extra_pci_roots(fw_cfg, pci_bus, &error_abort);
 
     return fw_cfg;
 }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 317aaca25a0..3a656f398fd 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -628,7 +628,8 @@ void pc_machine_done(Notifier *notifier, void *data)
     /* set the number of CPUs */
     x86_rtc_set_cpus_count(x86ms->rtc, x86ms->boot_cpus);
 
-    fw_cfg_add_extra_pci_roots(pcms->pcibus, x86ms->fw_cfg);
+    pci_bus_add_fw_cfg_extra_pci_roots(x86ms->fw_cfg, pcms->pcibus,
+                                       &error_abort);
 
     acpi_setup();
     if (x86ms->fw_cfg) {
-- 
2.45.2

[PATCH 6/6] hw/nvram/fw_cfg: Remove fw_cfg_add_extra_pci_roots()

[Philippe Mathieu-Daudé]

Now that all uses of fw_cfg_add_extra_pci_roots() have been
converted to the newer pci_bus_add_fw_cfg_extra_pci_roots(),
we can remove that bogus method. hw/nvram/fw_cfg must
stay generic. Device specific entries have to be implemented
using TYPE_FW_CFG_DATA_GENERATOR_INTERFACE.

This mostly reverts commit 0abd38885ac0fcdb08653922f339849cad387961
("fw_cfg: Refactor extra pci roots addition").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/nvram/fw_cfg.h |  9 ---------
 hw/nvram/fw_cfg.c         | 23 -----------------------
 2 files changed, 32 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index fcb06f18cc3..80a1549ad05 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -312,15 +312,6 @@ bool fw_cfg_add_file_from_generator(FWCfgState *s,
                                     Object *parent, const char *part,
                                     const char *filename, Error **errp);
 
-/**
- * fw_cfg_add_extra_pci_roots:
- * @bus: main pci root bus to be scanned from
- * @s: fw_cfg device being modified
- *
- * Add a new fw_cfg item...
- */
-void fw_cfg_add_extra_pci_roots(PCIBus *bus, FWCfgState *s);
-
 FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
                                 AddressSpace *dma_as);
 FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr);
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b94cd27bd85..053db7e08cc 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -41,7 +41,6 @@
 #include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "hw/acpi/aml-build.h"
-#include "hw/pci/pci_bus.h"
 #include "hw/loader.h"
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
@@ -1057,28 +1056,6 @@ bool fw_cfg_add_file_from_generator(FWCfgState *s,
     return true;
 }
 
-void fw_cfg_add_extra_pci_roots(PCIBus *bus, FWCfgState *s)
-{
-    int extra_hosts = 0;
-
-    if (!bus) {
-        return;
-    }
-
-    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));
-    }
-}
-
 static void fw_cfg_machine_reset(void *opaque)
 {
     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
-- 
2.45.2

Re: [PATCH 0/6] hw/nvram/fw_cfg: Move PCI bus methods out

[Philippe Mathieu-Daudé]

On 6/12/24 19:13, Philippe Mathieu-Daudé wrote:
> In order to keep fw_cfg device model clean, remove the PCI
> bus specific code. Instead, the equivalent functionality is
> implemented within the PCI_BUS object in hw/pci/,
> implementing TYPE_FW_CFG_DATA_GENERATOR_INTERFACE.
> 
> Philippe Mathieu-Daudé (6):
>    hw/nvram/fw_cfg: Rename fw_cfg_add_[file]_from_generator()
>    hw/nvram/fw_cfg: Pass QOM parent to fw_cfg_add_file_from_generator()
>    hw/pci: Have PCI_BUS implement TYPE_FW_CFG_DATA_GENERATOR_INTERFACE
>    hw/pci: Add pci_bus_add_fw_cfg_extra_pci_roots() helper
>    hw: Use pci_bus_add_fw_cfg_extra_pci_roots()
>    hw/nvram/fw_cfg: Remove fw_cfg_add_extra_pci_roots()

ping

Re: [PATCH 1/6] hw/nvram/fw_cfg: Rename fw_cfg_add_[file]_from_generator()

[Daniel P. Berrangé]

On Fri, Dec 06, 2024 at 07:13:47PM +0100, Philippe Mathieu-Daudé wrote:
> fw_cfg_add_from_generator() is adding a 'file' entry,
> so rename as fw_cfg_add_file_from_generator() for
> clarity. Besides, we might introduce generators for
> other entry types.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/nvram/fw_cfg.h | 6 +++---
>  hw/nvram/fw_cfg.c         | 4 ++--
>  system/vl.c               | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/6] hw/nvram/fw_cfg: Pass QOM parent to fw_cfg_add_file_from_generator()

[Daniel P. Berrangé]

On Fri, Dec 06, 2024 at 07:13:48PM +0100, Philippe Mathieu-Daudé wrote:
> Currently fw_cfg_add_file_from_generator() is restricted
> to command line created objects which reside in the
> '/objects' QOM container. In order to extend to other
> types of containers, pass the QOM parent by argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/nvram/fw_cfg.h | 10 ++++++----
>  hw/nvram/fw_cfg.c         | 11 ++++++-----
>  system/vl.c               |  3 ++-
>  3 files changed, 14 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/6] hw/pci: Have PCI_BUS implement TYPE_FW_CFG_DATA_GENERATOR_INTERFACE

[Daniel P. Berrangé]

On Fri, Dec 06, 2024 at 07:13:49PM +0100, Philippe Mathieu-Daudé wrote:
> The FW_CFG_DATA_GENERATOR allows any object to produce
                           ^
                          ^^^
                         ^^^^^  insert word 'interface'

> blob of data consumable by the fw_cfg device. Implement

 ^ 'a' as in 'a blob of...'

> that for PCI bus objects.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/pci/pci.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 1416ae202c3..8844251eceb 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -46,6 +46,7 @@
>  #include "hw/pci/msix.h"
>  #include "hw/hotplug.h"
>  #include "hw/boards.h"
> +#include "hw/nvram/fw_cfg.h"
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "pci-internal.h"
> @@ -216,11 +217,41 @@ static uint16_t pcibus_numa_node(PCIBus *bus)
>      return NUMA_NODE_UNASSIGNED;
>  }
>  
> +static GByteArray *pci_bus_fw_cfg_gen_data(Object *obj, Error **errp)
> +{
> +    PCIBus *bus = PCI_BUS(obj);
> +    GByteArray *byte_array;
> +    uint64_t extra_hosts = 0;
> +
> +    if (!bus) {
> +        return NULL;
> +    }

Returning NULL without setting 'errp'.

On the one hand I could see returning NULL, without errp, as being
useful semantics for "I have no data to add currently". THe callers
don't appear to distinguish NULL without errp, from NULL with errp.
They just propagate a failure return code up the stack.

The API docs for 'get_data' method don't say you can return NULL
without setting errp either.

> +
> +    QLIST_FOREACH(bus, &bus->child, sibling) {
> +        /* look for expander root buses */
> +        if (pci_bus_is_root(bus)) {
> +            extra_hosts++;
> +        }
> +    }
> +
> +    if (!extra_hosts) {
> +        return NULL;

Same.

> +    }
> +    extra_hosts = cpu_to_le64(extra_hosts);
> +
> +    byte_array = g_byte_array_new();
> +    g_byte_array_append(byte_array,
> +                        (const void *)&extra_hosts, sizeof(extra_hosts));
> +
> +    return byte_array;
> +}
> +
>  static void pci_bus_class_init(ObjectClass *klass, void *data)
>  {
>      BusClass *k = BUS_CLASS(klass);
>      PCIBusClass *pbc = PCI_BUS_CLASS(klass);
>      ResettableClass *rc = RESETTABLE_CLASS(klass);
> +    FWCfgDataGeneratorClass *fwgc = FW_CFG_DATA_GENERATOR_CLASS(klass);
>  
>      k->print_dev = pcibus_dev_print;
>      k->get_dev_path = pcibus_get_dev_path;
> @@ -232,6 +263,8 @@ static void pci_bus_class_init(ObjectClass *klass, void *data)
>  
>      pbc->bus_num = pcibus_num;
>      pbc->numa_node = pcibus_numa_node;
> +
> +    fwgc->get_data = pci_bus_fw_cfg_gen_data;
>  }
>  
>  static const TypeInfo pci_bus_info = {
> @@ -240,6 +273,10 @@ static const TypeInfo pci_bus_info = {
>      .instance_size = sizeof(PCIBus),
>      .class_size = sizeof(PCIBusClass),
>      .class_init = pci_bus_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_FW_CFG_DATA_GENERATOR_INTERFACE },
> +        { }
> +    }
>  };
>  
>  static const TypeInfo cxl_interface_info = {
> -- 
> 2.45.2
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 4/6] hw/pci: Add pci_bus_add_fw_cfg_extra_pci_roots() helper

[Daniel P. Berrangé]

On Fri, Dec 06, 2024 at 07:13:50PM +0100, Philippe Mathieu-Daudé wrote:
> pci_bus_add_fw_cfg_extra_pci_roots() calls the fw_cfg
> API with PCI bus specific arguments.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/pci/pci.h |  3 +++
>  hw/pci/pci.c         | 16 ++++++++++++++++
>  2 files changed, 19 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 5/6] hw: Use pci_bus_add_fw_cfg_extra_pci_roots()

[Daniel P. Berrangé]

On Fri, Dec 06, 2024 at 07:13:51PM +0100, Philippe Mathieu-Daudé wrote:
> We want to remove fw_cfg_add_extra_pci_roots() which introduced
> PCI bus knowledge within the generic hw/nvram/fw_cfg.c file.
> Replace the calls by the pci_bus_add_fw_cfg_extra_pci_roots()
> which is a 1:1 equivalent, but using correct API.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/virt.c     | 3 ++-
>  hw/hppa/machine.c | 2 +-
>  hw/i386/pc.c      | 3 ++-
>  3 files changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 6/6] hw/nvram/fw_cfg: Remove fw_cfg_add_extra_pci_roots()

[Daniel P. Berrangé]

On Fri, Dec 06, 2024 at 07:13:52PM +0100, Philippe Mathieu-Daudé wrote:
> Now that all uses of fw_cfg_add_extra_pci_roots() have been
> converted to the newer pci_bus_add_fw_cfg_extra_pci_roots(),
> we can remove that bogus method. hw/nvram/fw_cfg must
> stay generic. Device specific entries have to be implemented
> using TYPE_FW_CFG_DATA_GENERATOR_INTERFACE.
> 
> This mostly reverts commit 0abd38885ac0fcdb08653922f339849cad387961
> ("fw_cfg: Refactor extra pci roots addition").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/nvram/fw_cfg.h |  9 ---------
>  hw/nvram/fw_cfg.c         | 23 -----------------------
>  2 files changed, 32 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/6] hw/pci: Have PCI_BUS implement TYPE_FW_CFG_DATA_GENERATOR_INTERFACE

[Philippe Mathieu-Daudé]

On 13/12/24 12:32, Daniel P. Berrangé wrote:
> On Fri, Dec 06, 2024 at 07:13:49PM +0100, Philippe Mathieu-Daudé wrote:
>> The FW_CFG_DATA_GENERATOR allows any object to produce
>                             ^
>                            ^^^
>                           ^^^^^  insert word 'interface'
> 
>> blob of data consumable by the fw_cfg device. Implement
> 
>   ^ 'a' as in 'a blob of...'
> 
>> that for PCI bus objects.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/pci/pci.c | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 1416ae202c3..8844251eceb 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -46,6 +46,7 @@
>>   #include "hw/pci/msix.h"
>>   #include "hw/hotplug.h"
>>   #include "hw/boards.h"
>> +#include "hw/nvram/fw_cfg.h"
>>   #include "qapi/error.h"
>>   #include "qemu/cutils.h"
>>   #include "pci-internal.h"
>> @@ -216,11 +217,41 @@ static uint16_t pcibus_numa_node(PCIBus *bus)
>>       return NUMA_NODE_UNASSIGNED;
>>   }
>>   
>> +static GByteArray *pci_bus_fw_cfg_gen_data(Object *obj, Error **errp)
>> +{
>> +    PCIBus *bus = PCI_BUS(obj);
>> +    GByteArray *byte_array;
>> +    uint64_t extra_hosts = 0;
>> +
>> +    if (!bus) {
>> +        return NULL;
>> +    }
> 
> Returning NULL without setting 'errp'.
> 
> On the one hand I could see returning NULL, without errp, as being
> useful semantics for "I have no data to add currently". THe callers
> don't appear to distinguish NULL without errp, from NULL with errp.
> They just propagate a failure return code up the stack.
> 
> The API docs for 'get_data' method don't say you can return NULL
> without setting errp either.

Good catch. API updated here:
https://lore.kernel.org/qemu-devel/20241213133352.10915-4-philmd@linaro.org/

Thanks,

Phil.

> 
>> +
>> +    QLIST_FOREACH(bus, &bus->child, sibling) {
>> +        /* look for expander root buses */
>> +        if (pci_bus_is_root(bus)) {
>> +            extra_hosts++;
>> +        }
>> +    }
>> +
>> +    if (!extra_hosts) {
>> +        return NULL;
> 
> Same.
> 
>> +    }
>> +    extra_hosts = cpu_to_le64(extra_hosts);
>> +
>> +    byte_array = g_byte_array_new();
>> +    g_byte_array_append(byte_array,
>> +                        (const void *)&extra_hosts, sizeof(extra_hosts));
>> +
>> +    return byte_array;
>> +}