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

["Daniel P. Berrangé" <berrange@redhat.com>]

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 :|