Re: [PATCH v1 6/8] xen/pci: initialize BARs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2025 09:59, Mykyta Poturai wrote:
> From: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> 
> A PCI device must have valid BARs in order to assign it to a domain.  On
> ARM, firmware is unlikely to have initialized the BARs, so we must do
> this in Xen. During setup_hwdom_pci_devices(), check if each BAR is
> valid. If the BAR happens to already be valid, remove the BAR range from
> a rangeset of valid PCI ranges so as to avoid overlap when reserving a
> new BAR. If not valid, reserve a new BAR address from the rangeset and
> write it to the device.
> 
> Avaliable ranges are read from DT during init and stored in distinct
> rangesets.

The "distinct" lacks context here, imo. Maybe add "non-prefetchable and
prefetchable" earlier in the sentence?

> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -21,6 +21,7 @@
>  #include <xen/rwlock.h>
>  #include <xen/sched.h>
>  #include <xen/vmap.h>
> +#include <xen/resource.h>
>  
>  #include <asm/setup.h>
>  
> @@ -232,6 +233,21 @@ static int pci_bus_find_domain_nr(struct dt_device_node 
> *dev)
>      return domain;
>  }
>  
> +static int add_bar_range(const struct dt_device_node *dev, uint32_t flags,
> +                         uint64_t addr, uint64_t len, void *data)

Simply by its purpose, this function looks like it ought to be __init. And 
indeed,
while its only caller - pci_host_common_probe() - isn't, all callers of that
function are. Hence another prereq patch is wanted to add the missing __init 
there.

> @@ -286,6 +302,14 @@ pci_host_common_probe(struct dt_device_node *dev,
>      pci_add_host_bridge(bridge);
>      pci_add_segment(bridge->segment);
>  
> +    bridge->bar_ranges = rangeset_new(NULL, "BAR ranges",
> +                                      RANGESETF_prettyprint_hex);
> +    bridge->bar_ranges_prefetch = rangeset_new(NULL,
> +                                               "BAR ranges (prefetchable)",
> +                                               RANGESETF_prettyprint_hex);
> +    if ( bridge->bar_ranges && bridge->bar_ranges_prefetch )
> +        dt_for_each_range(bridge->dt_node, add_bar_range, bridge);
> +
>      return bridge;

This is odd: Why would you silently ignore the -ENOMEM condition here? You can't
blindly use the NULL pointer(s) ...

> @@ -476,6 +500,60 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t 
> start, mfn_t end)
>  
>      return bar_data.is_valid;
>  }
> +
> +uint64_t pci_get_new_bar_addr(const struct pci_dev *pdev, uint64_t size,
> +                              bool is_64bit, bool prefetch)

(__init as well here)

> +{
> +    struct pci_host_bridge *bridge;
> +    struct rangeset *range;
> +    uint64_t addr;
> +
> +    bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
> +    if ( !bridge )
> +        return 0;
> +
> +    range = prefetch ? bridge->bar_ranges_prefetch : bridge->bar_ranges;

... here, for example.

> +    if ( size < PAGE_SIZE )
> +        size = PAGE_SIZE;
> +
> +    if ( is_64bit && !rangeset_find_aligned_range(range, size, GB(4), &addr) 
> )
> +    {
> +        if ( rangeset_remove_range(range, addr, addr + size - 1) )
> +        {
> +            printk("%s:%d:%s error\n", __FILE__, __LINE__, __func__);
> +        }
> +        return addr;

No need for the inner figure braces, but more importantly: printk()-style
error handling is, well, insufficient? I'm pretty sure I indicated before that
you can't assume the return value of rangeset_find_aligned_range() isn't stale
the moment you look at (or use) it, unless you make further (written down)
assumptions. For example, rangeset_find_aligned_range() becoming __init would
already be a partial indication that it's not fit for general use.

__FILE__ / __LINE__ based diagnostics also aren't all this useful (and get in
the way elsewhere, e.g. for livepatching).

> +    }
> +    if ( !rangeset_find_aligned_range(range, size, 0, &addr) )
> +    {
> +        if ( !is_64bit && addr >= GB(4) )
> +            return 0;

What guarantees that no lower range is available? It's not said anywhere that
the function would search addresses in increasing order.

> +int pci_reserve_bar_range(const struct pci_dev *pdev, uint64_t addr,
> +                          uint64_t size, bool prefetch)

__init again (I won't further repeat this)

> --- a/xen/common/rangeset.c
> +++ b/xen/common/rangeset.c
> @@ -357,6 +357,41 @@ int rangeset_claim_range(struct rangeset *r, unsigned 
> long size,
>      return 0;
>  }
>  
> +int rangeset_find_aligned_range(struct rangeset *r, unsigned long size,
> +                                unsigned long min, unsigned long *s)

What is "min" when there ought to be some alignment specified instead?
(Reading the function body I understand that "size" also specifies the
alignment, but that's not clear at all when looking at e.g. just the
function declaration.)

How does the use of unsigned long here fit with the use of uint64_t in the
callers?

> +{
> +    struct range *x;
> +
> +    /* Power of 2 check */
> +    if ( (size & (size - 1)) != 0 )
> +    {
> +        *s = 0;
> +        return -EINVAL;
> +    }
> +
> +    read_lock(&r->lock);
> +
> +    for ( x = first_range(r); x; x = next_range(r, x) )
> +    {
> +        /* Assumes size is a power of 2 */
> +        unsigned long start_aligned = (x->s + size - 1) & ~(size - 1);

The comment says what somehow you should actually assure: It _needs_ to be
a power of 2 for this to work, yet size being 0 passes the check at the top
of the function.

Also please don't open-code ROUNDUP().

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1172,6 +1172,80 @@ int __init scan_pci_devices(void)
>      return ret;
>  }
>  
> +static void __init cf_check reserve_bar_range(struct pci_dev *pdev, uint8_t 
> reg,
> +                                              uint64_t addr, uint64_t size,
> +                                              bool is_64bit, bool prefetch)
> +{
> +    if ( pci_check_bar(pdev, maddr_to_mfn(addr),
> +                       maddr_to_mfn(addr + size - 1)) )
> +        pci_reserve_bar_range(pdev, addr, size, prefetch);
> +}
> +
> +static void __init cf_check get_new_bar_addr(struct pci_dev *pdev, uint8_t 
> reg,
> +                                             uint64_t addr, uint64_t size,
> +                                             bool is_64bit, bool prefetch)
> +{
> +    if ( !pci_check_bar(pdev, maddr_to_mfn(addr),
> +                        maddr_to_mfn(addr + size - 1)) )
> +    {

With how pci_check_bar() works on x86 right now, ...

> +        uint16_t cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> +
> +        addr = pci_get_new_bar_addr(pdev, size, is_64bit, prefetch);
> +
> +        pci_conf_write16(pdev->sbdf, PCI_COMMAND,
> +                         cmd & ~(PCI_COMMAND_MEMORY | PCI_COMMAND_IO));
> +
> +        pci_conf_write32(pdev->sbdf, reg,
> +                         (addr & GENMASK(31, 0)) |
> +                         (is_64bit ? PCI_BASE_ADDRESS_MEM_TYPE_64 : 0));
> +
> +        if ( is_64bit )
> +            pci_conf_write32(pdev->sbdf, reg + 4, addr >> 32);
> +
> +        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);

... all of this is unreachable code there. Misra doesn't like such.

Furthermore x86'es variant emits diagnostics, and aiui we'd now see them more
frequently (possibly: than wanted).

> +static int __init cf_check bars_iterate(struct pci_seg *pseg, void *arg)
> +{
> +    struct pci_dev *pdev;
> +    unsigned int i, ret, num_bars = PCI_HEADER_NORMAL_NR_BARS;
> +    uint64_t addr, size;
> +    void (*cb)(struct pci_dev *, uint8_t, uint64_t, uint64_t, bool, bool) = 
> arg;

There is still no connection at all between this function pointer type variable
and the two functions tobe called? Why is "cb" not directly the function
parameter? (For Misra's sake it may also be necessary to name the six 
parameters;
I'm not entirely sure, though. There was some back and forth at the time.)

Also, why is the first parameter not pointer-to-const?

> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> +    {
> +        if ( (pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f) ==
> +             PCI_HEADER_TYPE_NORMAL )
> +        {
> +            for ( i = 0; i < num_bars; i += ret )
> +            {
> +                uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
> +                bool prefetch;
> +
> +                if ( (pci_conf_read32(pdev->sbdf, reg) & 
> PCI_BASE_ADDRESS_SPACE)
> +                     == PCI_BASE_ADDRESS_SPACE_IO )

Nit (style): Operator placement.

> +                {
> +                    ret = 1;
> +                    continue;
> +                }
> +
> +                ret = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
> +                                      (i == num_bars - 1) ? PCI_BAR_LAST : 
> 0);

Nit (style): Indentation.

> +                if ( !size )
> +                    continue;
> +                prefetch = !!(pci_conf_read32(pdev->sbdf, reg) &
> +                              PCI_BASE_ADDRESS_MEM_PREFETCH);

No need for !!.

> @@ -1263,6 +1337,11 @@ void __hwdom_init setup_hwdom_pci_devices(
>      struct setup_hwdom ctxt = { .d = d, .handler = handler };
>  
>      pcidevs_lock();
> +    if ( hwdom_uses_vpci() )
> +    {
> +        pci_segments_iterate(bars_iterate, reserve_bar_range);
> +        pci_segments_iterate(bars_iterate, get_new_bar_addr);
> +    }
>      pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt);
>      pcidevs_unlock();

Does this really need splitting into 3 steps? Can't _setup_hwdom_pci_devices()
take care of the uninitialized BARs?

Jan