From: Stewart Hildebrand <stewart.hildebrand@amd.com>
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.
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
xen/arch/arm/include/asm/pci.h | 7 +++
xen/arch/arm/pci/pci-host-common.c | 78 +++++++++++++++++++++++++++++
xen/arch/x86/include/asm/pci.h | 14 ++++++
xen/common/rangeset.c | 35 +++++++++++++
xen/drivers/passthrough/pci.c | 79 ++++++++++++++++++++++++++++++
xen/include/xen/rangeset.h | 6 ++-
6 files changed, 218 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 4fc46da315..ba4caa56ae 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -74,6 +74,8 @@ struct pci_host_bridge {
struct pci_config_window *child_cfg;
const struct pci_ops *child_ops;
void *priv; /* Private data of the bridge. */
+ struct rangeset *bar_ranges;
+ struct rangeset *bar_ranges_prefetch;
};
struct pci_ops {
@@ -154,6 +156,11 @@ void pci_generic_init_bus_range_child(struct dt_device_node *dev,
bool arch_pci_device_physdevop(void);
+uint64_t pci_get_new_bar_addr(const struct pci_dev *pdev, uint64_t size,
+ bool is_64bit, bool prefetch);
+int pci_reserve_bar_range(const struct pci_dev *pdev, uint64_t addr,
+ uint64_t size, bool prefetch);
+
#else /*!CONFIG_HAS_PCI*/
#define pci_scan_enabled false
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 67447d8696..bfe610a403 100644
--- 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)
+{
+ 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);
+}
+
struct pci_host_bridge *
pci_host_common_probe(struct dt_device_node *dev,
const struct pci_ecam_ops *ops,
@@ -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;
err_child:
@@ -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)
+{
+ 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;
+
+ 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;
+ }
+ if ( !rangeset_find_aligned_range(range, size, 0, &addr) )
+ {
+ if ( !is_64bit && addr >= GB(4) )
+ return 0;
+ if ( rangeset_remove_range(range, addr, addr + size - 1) )
+ {
+ printk("%s:%d:%s error\n", __FILE__, __LINE__, __func__);
+ }
+ return addr;
+ }
+
+ return 0;
+}
+
+int pci_reserve_bar_range(const struct pci_dev *pdev, uint64_t addr,
+ uint64_t size, bool prefetch)
+{
+ struct pci_host_bridge *bridge;
+ struct rangeset *range;
+
+ bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
+ if ( !bridge )
+ return 0;
+
+ range = prefetch ? bridge->bar_ranges_prefetch : bridge->bar_ranges;
+
+ return rangeset_remove_range(range, addr, addr + size - 1);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index 08e8959d0d..2cabbbb5d0 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -82,4 +82,18 @@ static inline bool hwdom_uses_vpci(void)
return false;
}
+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;
+}
+
#endif /* __X86_PCI_H__ */
diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index 0e3b9acd35..c1c6f8610c 100644
--- 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);
+ *s = start_aligned;
+ return 0;
+ }
+ }
+
+ read_unlock(&r->lock);
+ *s = 0;
+
+ return -ENOSPC;
+}
+
int rangeset_consume_ranges(struct rangeset *r,
int (*cb)(unsigned long s, unsigned long e,
void *ctxt, unsigned long *c),
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 3edcfa8a04..4f5de9a542 100644
--- 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;
+
+ 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 )
+ {
+ ret = 1;
+ continue;
+ }
+
+ ret = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
+ (i == num_bars - 1) ? PCI_BAR_LAST : 0);
+
+ if ( !size )
+ continue;
+ prefetch = !!(pci_conf_read32(pdev->sbdf, reg) &
+ PCI_BASE_ADDRESS_MEM_PREFETCH);
+
+ cb(pdev, reg, addr, size, ret == 2, prefetch);
+ }
+ }
+ }
+
+ return 0;
+}
+
struct setup_hwdom {
struct domain *d;
int (*handler)(uint8_t devfn, struct pci_dev *pdev);
@@ -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();
}
diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 817505badf..e71e810f82 100644
--- 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);
int __must_check rangeset_remove_range(
struct rangeset *r, unsigned long s, unsigned long e);
bool __must_check rangeset_contains_range(
--
2.34.1On 24.09.2025 09:59, Mykyta Poturai wrote:
> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
>
> 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
© 2016 - 2026 Red Hat, Inc.