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:
20241213133352.10915-4-philmd@linaro.org/">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;
> > +}