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
