Re: [PATCH v1 05/14] xen/arm: PCI host bridge discovery within XEN on ARM

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien

> On 7 Sep 2021, at 10:05 am, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> On 19/08/2021 13:02, Rahul Singh wrote:
>> XEN during boot will read the PCI device tree node “reg” property
>> and will map the PCI config space to the XEN memory.
>> As of now "pci-host-ecam-generic" compatible board is supported.
> 
> I think the word "only" is missing.
Ok. 
> 
>> "linux,pci-domain" device tree property assigns a fixed PCI domain
>> number to a host bridge, otherwise an unstable (across boots) unique
>> number will be assigned by Linux.This property has to be in sync with
> 
> Typo: missing space after the ‘.’
Ack.
>> XEN to access the PCI devices.
> 
> I would expand a little bit the last sentence to explain why the need to be 
> sync-ed.

I will explain why segment and domain need to be in sync in next version.
> 
>> > XEN will read the “linux,pci-domain” property from the device tree node
>> and configure the host bridge segment number accordingly. If this
>> property is not available XEN will allocate the unique segment number
>> to the host bridge.
>> dt_get_pci_domain_nr(..) and dt_pci_bus_find_domain_nr(..) are directly
>> imported from the Linux source tree.
> 
> What was the Linux commit used? I also read "directly imported" as a 
> verbartim copy but AFAICT the implementation has been slightly reworked.

I will add the Linux commit used in commit msg and also will add that code is 
slightly modified.
> 
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> ---
>>  xen/arch/arm/pci/Makefile           |   2 +
>>  xen/arch/arm/pci/pci-host-common.c  | 261 ++++++++++++++++++++++++++++
>>  xen/arch/arm/pci/pci-host-generic.c |  55 ++++++
>>  xen/include/asm-arm/pci.h           |  28 +++
>>  4 files changed, 346 insertions(+)
>>  create mode 100644 xen/arch/arm/pci/pci-host-common.c
>>  create mode 100644 xen/arch/arm/pci/pci-host-generic.c
>> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
>> index a9ee0b9b44..f3d97f859e 100644
>> --- a/xen/arch/arm/pci/Makefile
>> +++ b/xen/arch/arm/pci/Makefile
>> @@ -1,2 +1,4 @@
>>  obj-y += pci.o
>>  obj-y += pci-access.o
>> +obj-y += pci-host-generic.o
>> +obj-y += pci-host-common.o
>> diff --git a/xen/arch/arm/pci/pci-host-common.c 
>> b/xen/arch/arm/pci/pci-host-common.c
>> new file mode 100644
>> index 0000000000..9dd9b02271
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -0,0 +1,261 @@
>> +/*
>> + * Copyright (C) 2021 Arm Ltd.
>> + *
>> + * Based on Linux drivers/pci/ecam.c
>> + * Copyright 2016 Broadcom.
>> + *
>> + * Based on Linux drivers/pci/controller/pci-host-common.c
>> + * Based on Linux drivers/pci/controller/pci-host-generic.c
>> + * Copyright (C) 2014 ARM Limited Will Deacon <will.deacon@xxxxxxx>
>> + *
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/init.h>
>> +#include <xen/pci.h>
>> +#include <asm/pci.h>
> 
> AFAICT, <xen/pci.h> already includes <asm/pci.h>. So this looks unneccessary.
Ack.
> 
>> +#include <xen/rwlock.h>
>> +#include <xen/sched.h>
>> +#include <xen/vmap.h>
>> +
>> +/*
>> + * List for all the pci host bridges.
>> + */
>> +
>> +static LIST_HEAD(pci_host_bridges);
>> +
>> +static atomic_t domain_nr = ATOMIC_INIT(-1);
>> +
>> +bool dt_pci_parse_bus_range(struct dt_device_node *dev,
>> +                            struct pci_config_window *cfg)
> 
> Aside, "pci_config_window", the function is not Arm specific. Would it be 
> possible to consider to introduce "struct resource" in Xen so this function 
> can be moved in common/device_tree.c?

I can introduce the "struct resource” but I am not sure whether "struct 
resource” will be
useful later point in time. What I prefer as of now, we can have this function 
and we can
move this to common/device_tree.c once we have the requirement for "struct 
resource”.
> 
>> +{
>> +    const __be32 *cells;
>> +    uint32_t len;
>> +
>> +    cells = dt_get_property(dev, "bus-range", &len);
>> +    /* bus-range should at least be 2 cells */
>> +    if ( !cells || (len < (sizeof(*cells) * 2)) )
>> +        return false;
> 
> How about introducing dt_property_read_u32_array()?

Ok. I will introduce dt_property_read_u32_array().

> 
>> +
>> +    cfg->busn_start = dt_next_cell(1, &cells);
>> +    cfg->busn_end = dt_next_cell(1, &cells);
>> +
>> +    return true;
>> +}
>> +
>> +static inline void __iomem *pci_remap_cfgspace(paddr_t start, size_t len)
>> +{
>> +    return ioremap_nocache(start, len);
>> +}
>> +
>> +static void pci_ecam_free(struct pci_config_window *cfg)
>> +{
>> +    if ( cfg->win )
>> +        iounmap(cfg->win);
>> +
>> +    xfree(cfg);
>> +}
>> +
>> +static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
>> +                                              int ecam_reg_idx)
>> +{
>> +    int err;
>> +    struct pci_config_window *cfg;
>> +    paddr_t addr, size;
>> +
>> +    cfg = xzalloc(struct pci_config_window);
>> +    if ( !cfg )
>> +        return NULL;
>> +
>> +    err = dt_pci_parse_bus_range(dev, cfg);
>> +    if ( !err ) {
>> +        cfg->busn_start = 0;
>> +        cfg->busn_end = 0xff;
>> +        printk(XENLOG_ERR "%s:No bus range found for pci controller\n",
> 
> Typo: Missing space after ':’.
Ack.
> 
>> +               dt_node_full_name(dev));
>> +    } else {
>> +        if ( cfg->busn_end > cfg->busn_start + 0xff )
>> +            cfg->busn_end = cfg->busn_start + 0xff;
>> +    }
>> +
>> +    /* Parse our PCI ecam register address*/
>> +    err = dt_device_get_address(dev, ecam_reg_idx, &addr, &size);
>> +    if ( err )
>> +        goto err_exit;
>> +
>> +    cfg->phys_addr = addr;
>> +    cfg->size = size;
>> +
>> +    /*
>> +     * On 64-bit systems, we do a single ioremap for the whole config space
>> +     * since we have enough virtual address range available.  On 32-bit, we
> 
> In Xen on Arm64, the VMAP is actually only 1GB. So it is not that big and 
> this will compete with other mapping like ITS, global domain mapping...
> 
> So I think the vMAP area will need to increase to cater to increase usage.

Let me check on this and come back to you.

> 
>> +     * ioremap the config space for each bus individually.
>> +     *
>> +     * As of now only 64-bit is supported 32-bit is not supported.
>> +     */
>> +    cfg->win = pci_remap_cfgspace(cfg->phys_addr, cfg->size);
>> +    if ( !cfg->win )
>> +        goto err_exit_remap;
>> +
>> +    printk("ECAM at [mem %lx-%lx] for [bus %x-%x] \n",cfg->phys_addr,
> 
> The physical address is a paddr_t. So this needs to use PRIpaddr. Also, 
> please use preprent hexadecimal with 0x. This makes a lot easier to 
> differentiate hexa vs decimal in the log.
Ack.
> 
>> +            cfg->phys_addr + cfg->size - 1, cfg->busn_start, cfg->busn_end);
>> +
>> +    return cfg;
>> +
>> +err_exit_remap:
>> +    printk(XENLOG_ERR "ECAM ioremap failed\n");
>> +err_exit:
>> +    pci_ecam_free(cfg);
> 
> Coding style: Please add a new line before return.
Ack.
> 
>> +    return NULL;
>> +}
>> +
>> +struct pci_host_bridge *pci_alloc_host_bridge(void)
>> +{
>> +    struct pci_host_bridge *bridge = xzalloc(struct pci_host_bridge);
>> +
>> +    if ( !bridge )
>> +        return NULL;
>> +
>> +    INIT_LIST_HEAD(&bridge->node);
>> +    bridge->bus_start = ~0;
>> +    bridge->bus_end = ~0;
> 
> Coding style: Please add a new line before return.
Ack.
> 
> 
>> +    return bridge;
>> +}
>> +
>> +void pci_add_host_bridge(struct pci_host_bridge *bridge)
>> +{
>> +    list_add_tail(&bridge->node, &pci_host_bridges);
>> +}
>> +
>> +static int pci_get_new_domain_nr(void)
>> +{
>> +    return atomic_inc_return(&domain_nr);
>> +}
>> +
>> +/*
>> + * This function will try to obtain the host bridge domain number by
>> + * finding a property called "linux,pci-domain" of the given device node.
>> + *
>> + * @node: device tree node with the domain information
>> + *
>> + * Returns the associated domain number from DT in the range [0-0xffff], or
>> + * a negative value if the required property is not found.
>> + */
>> +static int dt_get_pci_domain_nr(struct dt_device_node *node)
> 
> Nothing looks Arm specific for this function. Can you move it in 
> device_tree.c?
Ack.
> 
>> +{
>> +    u32 domain;
>> +    int error;
>> +
>> +    error = dt_property_read_u32(node, "linux,pci-domain", &domain);
>> +    if ( !error )
>> +        return -EINVAL;
>> +
>> +    return (u16)domain;
>> +}
>> +
>> +static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>> +{
>> +    static int use_dt_domains = -1;
>> +    int domain;
>> +
>> +    domain = dt_get_pci_domain_nr(dev);
>> +
>> +    /*
>> +     * Check DT domain and use_dt_domains values.
>> +     *
>> +     * If DT domain property is valid (domain >= 0) and
>> +     * use_dt_domains != 0, the DT assignment is valid since this means
>> +     * we have not previously allocated a domain number by using
>> +     * pci_get_new_domain_nr(); we should also update use_dt_domains to
>> +     * 1, to indicate that we have just assigned a domain number from
>> +     * DT.
>> +     *
>> +     * If DT domain property value is not valid (ie domain < 0), and we
>> +     * have not previously assigned a domain number from DT
>> +     * (use_dt_domains != 1) we should assign a domain number by
>> +     * using the:
>> +     *
>> +     * pci_get_new_domain_nr()
>> +     *
>> +     * API and update the use_dt_domains value to keep track of method we
>> +     * are using to assign domain numbers (use_dt_domains = 0).
>> +     *
>> +     * All other combinations imply we have a platform that is trying
>> +     * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
>> +     * which is a recipe for domain mishandling and it is prevented by
>> +     * invalidating the domain value (domain = -1) and printing a
>> +     * corresponding error.
>> +     */
>> +    if ( domain >= 0 && use_dt_domains )
>> +    {
>> +        use_dt_domains = 1;
>> +    }
>> +    else if ( domain < 0 && use_dt_domains != 1 )
>> +    {
>> +        use_dt_domains = 0;
>> +        domain = pci_get_new_domain_nr();
>> +    }
>> +    else
>> +    {
>> +        printk(XENLOG_ERR "Inconsistent \"linux,pci-domain\" property in 
>> DT\n");
>> +        BUG();
> 
> I don't think pci_bus_find_domain_nr() should be the function that crashes 
> Xen. Instead, this should be propagated to the highest possible callers and 
> let it decide what to do.

Ack. 
> 
>> +    }
>> +
>> +    return domain;
>> +}
>> +
>> +int pci_host_common_probe(struct dt_device_node *dev,
>> +                          int ecam_reg_idx)
>> +{
>> +    struct pci_host_bridge *bridge;
>> +    struct pci_config_window *cfg;
>> +    int err;
>> +
>> +    bridge = pci_alloc_host_bridge();
>> +    if ( !bridge )
>> +        return -ENOMEM;
>> +
>> +    /* Parse and map our Configuration Space windows */
>> +    cfg = gen_pci_init(dev, ecam_reg_idx);
>> +    if ( !cfg )
>> +    {
>> +        err = -ENOMEM;
>> +        goto err_exit;
>> +    }
>> +
>> +    bridge->dt_node = dev;
>> +    bridge->sysdata = cfg;
>> +    bridge->bus_start = cfg->busn_start;
>> +    bridge->bus_end = cfg->busn_end;
>> +
>> +    bridge->segment = pci_bus_find_domain_nr(dev);
>> +
>> +    pci_add_host_bridge(bridge);
>> +
>> +    return 0;
>> +
>> +err_exit:
>> +    xfree(bridge);
>> +    return err;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/pci/pci-host-generic.c 
>> b/xen/arch/arm/pci/pci-host-generic.c
>> new file mode 100644
>> index 0000000000..13d0f7f999
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci-host-generic.c
>> @@ -0,0 +1,55 @@
>> +/*
>> + * Copyright (C) 2021 Arm Ltd.
>> + *
>> + * Based on Linux drivers/pci/controller/pci-host-common.c
>> + * Based on Linux drivers/pci/controller/pci-host-generic.c
>> + * Copyright (C) 2014 ARM Limited Will Deacon <will.deacon@xxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <asm/device.h>
>> +#include <xen/pci.h>
>> +#include <asm/pci.h>
>> +
>> +static const struct dt_device_match gen_pci_dt_match[] = {
>> +    { .compatible = "pci-host-ecam-generic" },
>> +    { },
>> +};
>> +
>> +static int gen_pci_dt_init(struct dt_device_node *dev, const void *data)
>> +{
>> +    const struct dt_device_match *of_id;
>> +
>> +    of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node);
> 
> This seems to be a bit pointless to me as you already know the compatible 
> (there is only one possible...).
 As of now we are only implementing the "pci-host-ecam-generic” compatible PCI, 
but in future we might 
need to implement the other compatible like Linux see as below.

https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-host-generic.c#L59

> 
>> +
>> +    printk(XENLOG_INFO "Found PCI host bridge %s compatible:%s \n",
>> +           dt_node_full_name(dev), of_id->compatible);
>> +
>> +    return pci_host_common_probe(dev, 0);
>> +}
>> +
>> +DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
>> +.dt_match = gen_pci_dt_match,
>> +.init = gen_pci_dt_init,
>> +DT_DEVICE_END
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>> index 61e43da088..58a51e724e 100644
>> --- a/xen/include/asm-arm/pci.h
>> +++ b/xen/include/asm-arm/pci.h
>> @@ -26,6 +26,34 @@ struct arch_pci_dev {
>>      struct device dev;
>>  };
>>  +/*
>> + * struct to hold the mappings of a config space window. This
>> + * is expected to be used as sysdata for PCI controllers that
>> + * use ECAM.
>> + */
>> +struct pci_config_window {
>> +    paddr_t         phys_addr;
>> +    paddr_t         size;
>> +    uint8_t         busn_start;
>> +    uint8_t         busn_end;
>> +    void __iomem    *win;
>> +};
>> +
>> +/*
>> + * struct to hold pci host bridge information
>> + * for a PCI controller.
>> + */
>> +struct pci_host_bridge {
>> +    struct dt_device_node *dt_node;  /* Pointer to the associated DT node */
>> +    struct list_head node;           /* Node in list of host bridges */
>> +    uint16_t segment;                /* Segment number */
>> +    u8 bus_start;                    /* Bus start of this bridge. */
>> +    u8 bus_end;                      /* Bus end of this bridge. */
> 
> Please use uint8_t rather than u8.
Ack. 

Regards,
Rahul