Re: [PATCH v1 08/14] xen:arm: Implement pci access functions

[Stefano Stabellini <sstabellini@kernel.org>]

On Thu, 19 Aug 2021, Rahul Singh wrote:
> Implement generic pci access functions to read/write the configuration
> space.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>  xen/arch/arm/pci/pci-access.c      | 31 +++++++++++++++++++++++++++++-
>  xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++++++++++
>  xen/include/asm-arm/pci.h          |  2 ++
>  3 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
> index f39f6a3a38..b94de3c3ac 100644
> --- a/xen/arch/arm/pci/pci-access.c
> +++ b/xen/arch/arm/pci/pci-access.c
> @@ -72,12 +72,41 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
>  static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
>                                  unsigned int len)
>  {
> -    return ~0U;
> +    uint32_t val = GENMASK(0, len * 8);

This seems to be another default error value that it would be better to
define with its own macro


> +    struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
> +
> +    if ( unlikely(!bridge) )
> +    {
> +        printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
> +                sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);

You are not actually printing sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn ?


> +        return val;
> +    }
> +
> +    if ( unlikely(!bridge->ops->read) )
> +        return val;
> +
> +    bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);

Would it make sense to make the interface take a pci_sbdf_t directly
instead of casting to uint32_t and back?


> +    return val;
>  }
>  
>  static void pci_config_write(pci_sbdf_t sbdf, unsigned int reg,
>                               unsigned int len, uint32_t val)
>  {
> +    struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
> +
> +    if ( unlikely(!bridge) )
> +    {
> +        printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
> +                sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);

same here


> +        return;
> +    }
> +
> +    if ( unlikely(!bridge->ops->write) )
> +        return;
> +
> +    bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val);

same here


>  }
>  
>  /*
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index c582527e92..62715b4676 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -261,6 +261,25 @@ err_exit:
>      return err;
>  }
>  
> +/*
> + * This function will lookup an hostbridge based on the segment and bus
> + * number.
> + */
> +struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus)
> +{
> +    struct pci_host_bridge *bridge;
> +
> +    list_for_each_entry( bridge, &pci_host_bridges, node )
> +    {
> +        if ( bridge->segment != segment )
> +            continue;
> +        if ( (bus < bridge->bus_start) || (bus > bridge->bus_end) )
> +            continue;
> +        return bridge;
> +    }
> +
> +    return NULL;
> +}
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 22866244d2..756f8637ab 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -20,6 +20,7 @@
>  #ifdef CONFIG_HAS_PCI
>  
>  #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
> +#define PRI_pci "%04x:%02x:%02x.%u"
>  
>  /* Arch pci dev struct */
>  struct arch_pci_dev {
> @@ -86,6 +87,7 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
>  void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
>                                 uint32_t sbdf, uint32_t where);
>  
> +struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus);
>  #else   /*!CONFIG_HAS_PCI*/
>  
>  struct arch_pci_dev { };
> -- 
> 2.17.1
>