Re: [PATCH v2 1/7] xen/arm: allow PCI host bridge to have private data

[Julien Grall <julien@xxxxxxx>]

Hi,

On 11/03/2025 15:05, Grygorii Strashko wrote:
> On 11.03.25 12:24, Mykyta Poturai wrote:
> > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> >
> > Some of the PCI host bridges require private data. Create a generic
> > approach for that, so such bridges may request the private data to
> > be allocated during initialization.
> >
> > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> > ---
> > v1->v2:
> > * no change
> > ---
> >   xen/arch/arm/include/asm/pci.h      |  4 +++-
> >   xen/arch/arm/pci/pci-host-common.c  | 18 +++++++++++++++++-
> >   xen/arch/arm/pci/pci-host-generic.c |  2 +-
> >   xen/arch/arm/pci/pci-host-zynqmp.c  |  2 +-
> >   4 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/ 
> > asm/pci.h
> > index 7f77226c9b..44344ac8c1 100644
> > --- a/xen/arch/arm/include/asm/pci.h
> > +++ b/xen/arch/arm/include/asm/pci.h
> > @@ -66,6 +66,7 @@ struct pci_host_bridge {
> >       uint16_t segment;                /* Segment number */
> >       struct pci_config_window* cfg;   /* Pointer to the bridge config 
> > window */
> >       const struct pci_ops *ops;
> > +    void *priv;                      /* Private data of the bridge. */
> >   };
> >   struct pci_ops {
> > @@ -95,7 +96,8 @@ struct pci_ecam_ops {
> >   extern const struct pci_ecam_ops pci_generic_ecam_ops;
> >   int pci_host_common_probe(struct dt_device_node *dev,
> > -                          const struct pci_ecam_ops *ops);
> > +                          const struct pci_ecam_ops *ops,
> > +                          size_t priv_sz);
> >   int pci_generic_config_read(struct pci_host_bridge *bridge, 
> > pci_sbdf_t sbdf,
> >                               uint32_t reg, uint32_t len, uint32_t 
> > *value);
> >   int pci_generic_config_write(struct pci_host_bridge *bridge, 
> > pci_sbdf_t sbdf,
> > diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/ 
> > pci-host-common.c
> > index c0faf0f436..be7e6c3510 100644
> > --- a/xen/arch/arm/pci/pci-host-common.c
> > +++ b/xen/arch/arm/pci/pci-host-common.c
> > @@ -209,7 +209,8 @@ static int pci_bus_find_domain_nr(struct 
> > dt_device_node *dev)
> >   }
> >   int pci_host_common_probe(struct dt_device_node *dev,
> > -                          const struct pci_ecam_ops *ops)
> > +                          const struct pci_ecam_ops *ops,
> > +                          size_t priv_sz)
> >   {
> >       struct pci_host_bridge *bridge;
> >       struct pci_config_window *cfg;
> > @@ -241,11 +242,26 @@ int pci_host_common_probe(struct dt_device_node 
> > *dev,
> >           printk(XENLOG_ERR "Inconsistent \"linux,pci-domain\" 
> > property in DT\n");
> >           BUG();
> >       }
> > +
> >       bridge->segment = domain;
> > +
> > +    if ( priv_sz )
> > +    {
> > +        bridge->priv = xzalloc_bytes(priv_sz);
> > +        if ( !bridge->priv )
> > +        {
> > +            err = -ENOMEM;
> > +            goto err_priv;
> > +        }
> > +    }
> > +
> I'd like to propose to move allocation into pci_alloc_host_bridge()
> to keep mallocs in one place and do it earlier, before proceeding
> with other initialization steps.
In both cases, we need to pass the size of the structure in parameter 
and the structure is not initialized until later. I would rather prefer
if the allocation is done by each PCI controller that needs it close to 
the initialization of "priv".
Cheers,

--
Julien Grall