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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

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.

Also the pci_alloc_host_bridge() could be made static, seems.

>       pci_add_host_bridge(bridge);
>   
>       return 0;
>   
> +err_priv:
> +    xfree(bridge->priv);
> +
>   err_exit:
>       xfree(bridge);
>   
[...]

-grygorii