Re: [PATCH v2 5/7] xen/pci: initialize BARs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.10.2025 15:56, Mykyta Poturai wrote:
> @@ -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)
> +{
> +    struct pci_host_bridge *bridge = data;
> +
> +    if ( !(flags & IORESOURCE_MEM) )
> +        return 0;
> +
> +    if ( flags & IORESOURCE_PREFETCH )
> +        return rangeset_add_range(bridge->bar_ranges_prefetch, addr,
> +                                  addr + len - 1);
> +    else
> +        return rangeset_add_range(bridge->bar_ranges, addr, addr + len - 1);

Here and everywhere else you use rangesets: This loses significant bits on
Arm32. Iirc the plan was to select HAS_PCI only for Arm64, but that's entirely
invisible here. I think there want to be perhaps multiple BUILD_BUG_ON()s, at
the very least.

> --- a/xen/arch/x86/include/asm/pci.h
> +++ b/xen/arch/x86/include/asm/pci.h
> @@ -76,4 +76,24 @@ int pci_sanitize_bar_memory(struct rangeset *r);
>  
>  void pci_setup(void);
>  
> +/* Unlike ARM, HW domain alyways uses vpci for x86 */
> +static inline bool hwdom_uses_vpci(void)
> +{
> +    return true;
> +}

What the comment says is not true. It is true for PVH Dom0. The sole use
of the predicate therefore is questionable, too.

> +static inline uint64_t pci_get_new_bar_addr(const struct pci_dev *pdev,
> +                                            uint64_t size, bool is_64bit,
> +                                            bool prefetch)
> +{
> +    return 0;
> +}
> +
> +static inline int pci_reserve_bar_range(const struct pci_dev *pdev,
> +                                        uint64_t addr, uint64_t size,
> +                                        bool prefetch)
> +{
> +    return 0;
> +}

Neither here nor elsewhere is any word said on what these do, what a
"new BAR range" is, or what "reserving" would mean.

> --- 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)
> +{
> +    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);
> +
> +        if ( x->e > start_aligned &&
> +             (x->e - start_aligned) >= size &&
> +             start_aligned >= min )
> +        {
> +            read_unlock(&r->lock);

With this and ...

> +            *s = start_aligned;
> +            return 0;
> +        }
> +    }
> +
> +    read_unlock(&r->lock);

... this, how can the caller be sure the result they receive is not stale by
the time they get to look at and use it?

> --- 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)) )
> +    {
> +        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);
> +    }
> +}
> +
> +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 needs to be some build-time checking that this and the two callbacks
above actually match in type. Likely by way of using a function typedef.

> --- a/xen/include/xen/rangeset.h
> +++ b/xen/include/xen/rangeset.h
> @@ -56,11 +56,15 @@ void rangeset_limit(
>  bool __must_check rangeset_is_empty(
>      const struct rangeset *r);
>  
> -/* Add/claim/remove/query/purge a numeric range. */
> +/* Add/claim/find/remove/query/purge a numeric range. */
>  int __must_check rangeset_add_range(
>      struct rangeset *r, unsigned long s, unsigned long e);
>  int __must_check rangeset_claim_range(struct rangeset *r, unsigned long size,
>                                        unsigned long *s);
> +int __must_check rangeset_find_aligned_range(struct rangeset *r,
> +                                             unsigned long size,
> +                                             unsigned long min,
> +                                             unsigned long *s);

>From these parameter names I'm unable to tell what the "aligned" part of
the name is referring to. IOW it may be necessary to have an accompanying
comment.

Jan