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 two distinct
rangesets, one for prefetchable and one for non-prefetchable BARs.
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
v2->v3:
* drop hwdom_uses_vpci
* check that rangeset can handle u64
* rework rangeset manipulaiton
* mark more functions as __init
* move bar init to arm files
* style fixes
v1->v2:
* move hwdom_uses_vpci to this patch
* fixup error reporting
---
xen/arch/arm/include/asm/pci.h | 7 ++
xen/arch/arm/pci/pci-host-common.c | 92 ++++++++++++++++++++++++
xen/arch/arm/pci/pci.c | 110 +++++++++++++++++++++++++++++
xen/common/rangeset.c | 62 ++++++++++++++--
xen/include/xen/rangeset.h | 11 +++
5 files changed, 277 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 7289f7688b..ac4e87f9c1 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 de30fb0aec..28c26af9eb 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,25 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
return domain;
}
+static int __init 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;
+
+ /* Ensure we are not using bits in a rangeset */
+ BUILD_BUG_ON(sizeof(unsigned long) != sizeof(uint64_t));
+
+ 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 * __init
pci_host_common_probe(struct dt_device_node *dev,
const struct pci_ecam_ops *ops,
@@ -283,6 +303,18 @@ pci_host_common_probe(struct dt_device_node *dev,
bridge->child_ops = &child_ops->pci_ops;
}
+ 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 )
+ {
+ err = dt_for_each_range(bridge->dt_node, add_bar_range, bridge);
+ if ( err )
+ goto err_child;
+ }
+
pci_add_host_bridge(bridge);
pci_add_segment(bridge->segment);
@@ -476,6 +508,66 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
return bar_data.is_valid;
}
+
+/*
+ * Find suitable place for an uninitialized bar of specified size in the
+ * host bridge ranges
+ */
+uint64_t __init 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 = 0, end = GB(4);
+
+ /* Make sure we can store addr in a rangeset */
+ BUILD_BUG_ON(sizeof(addr) != sizeof(unsigned long));
+
+ 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 )
+ {
+ addr = GB(4);
+ end = ~0;
+ }
+
+ if ( !rangeset_claim_aligned_range(range, size, &addr, end) )
+ return addr;
+
+ printk(XENLOG_ERR "Failed to claim BAR range %lx-%lx from rangeset\n",
+ addr, addr + size - 1);
+
+ return 0;
+}
+
+/*
+ * Remove already used memory from the host bridge bar ranges
+ */
+int __init 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;
+
+ /* Make sure we can store addr in a rangeset */
+ BUILD_BUG_ON(sizeof(addr) != sizeof(unsigned long));
+
+ 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/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
index 951639eb3f..0330220e93 100644
--- a/xen/arch/arm/pci/pci.c
+++ b/xen/arch/arm/pci/pci.c
@@ -95,6 +95,108 @@ boolean_param("pci-passthrough", pci_passthrough_enabled);
__ro_after_init bool pci_scan_enabled;
boolean_param("pci-scan", pci_scan_enabled);
+typedef int (*bar_callback_t)(struct pci_dev *, uint8_t, uint64_t, uint64_t,
+ bool, bool);
+
+static int __init 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)) )
+ return pci_reserve_bar_range(pdev, addr, size, prefetch);
+ return 0;
+}
+
+static int __init setup_bar(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);
+ if ( !addr )
+ return -ENOMEM;
+
+ 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);
+ }
+
+ return 0;
+}
+
+static int __init bars_iterate(struct pci_dev *pdev, void *arg)
+{
+ unsigned int i, barsize, ret = 0, num_bars = PCI_HEADER_NORMAL_NR_BARS;
+ uint64_t addr, size;
+ bar_callback_t cb = arg;
+
+ if ( (pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f) ==
+ PCI_HEADER_TYPE_NORMAL )
+ {
+ for ( i = 0; i < num_bars; i += barsize )
+ {
+ 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 )
+ {
+ barsize = 1;
+ continue;
+ }
+
+ barsize = 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;
+
+ ret = cb(pdev, reg, addr, size, barsize == 2, prefetch);
+ if ( ret )
+ return ret;
+ }
+ }
+
+ return ret;
+}
+
+static int __init pci_setup_bars(void)
+{
+ int ret;
+ /* We can't change the signature of bars_iterate to only accept
+ * bar_callback_t, so use intermediate variables to ensure callback
+ * signatures are always correct
+ */
+ bar_callback_t cb_reserve = reserve_bar_range;
+ bar_callback_t cb_setup = setup_bar;
+
+ pcidevs_lock();
+ ret = pci_iterate_devices(bars_iterate, cb_reserve);
+ if ( ret )
+ goto out;
+
+ ret = pci_iterate_devices(bars_iterate, cb_setup);
+
+out:
+ pcidevs_unlock();
+ return ret;
+}
+
static int __init pci_init(void)
{
int ret;
@@ -129,6 +231,14 @@ static int __init pci_init(void)
printk(XENLOG_ERR "PCI: Failed to scan PCI devices (rc=%d)\n", ret);
return 0;
}
+
+ ret = pci_setup_bars();
+
+ if ( ret < 0 )
+ {
+ printk(XENLOG_ERR "PCI: Failed to configure BARs (rc=%d)\n", ret);
+ return 0;
+ }
}
return 0;
diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index 0e3b9acd35..6a0c20ab41 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -180,16 +180,13 @@ int rangeset_add_range(
return rc;
}
-int rangeset_remove_range(
- struct rangeset *r, unsigned long s, unsigned long e)
+static int remove_range(struct rangeset *r, unsigned long s, unsigned long e)
{
struct range *x, *y, *t;
int rc = 0;
ASSERT(s <= e);
- write_lock(&r->lock);
-
x = find_range(r, s);
y = find_range(r, e);
@@ -244,8 +241,18 @@ int rangeset_remove_range(
destroy_range(r, x);
}
- out:
+out:
+ return rc;
+}
+
+int rangeset_remove_range(struct rangeset *r, unsigned long s, unsigned long e)
+{
+ int rc = 0;
+
+ write_lock(&r->lock);
+ rc = remove_range(r, s, e);
write_unlock(&r->lock);
+
return rc;
}
@@ -357,6 +364,51 @@ int rangeset_claim_range(struct rangeset *r, unsigned long size,
return 0;
}
+int rangeset_claim_aligned_range(struct rangeset *r, unsigned long size,
+ unsigned long *s, unsigned long e)
+{
+ struct range *x;
+ int rc = 0;
+
+ /* Power of 2 check */
+ if ( (size & (size - 1)) != 0 && size != 0 )
+ {
+ *s = 0;
+ return -EINVAL;
+ }
+
+ if ( e < *s )
+ return -EINVAL;
+
+ write_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 = ROUNDUP(x->s, size);
+
+ if ( x->e > start_aligned &&
+ (x->e - start_aligned) >= size &&
+ start_aligned >= *s &&
+ start_aligned + size <= e)
+ {
+ rc = remove_range(r, start_aligned, start_aligned + size - 1);
+ if ( !rc )
+ *s = start_aligned;
+ else
+ *s = 0;
+
+ write_unlock(&r->lock);
+ return rc;
+ }
+ }
+
+ *s = 0;
+
+ write_unlock(&r->lock);
+ 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/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 817505badf..dcef96cb2c 100644
--- a/xen/include/xen/rangeset.h
+++ b/xen/include/xen/rangeset.h
@@ -61,6 +61,17 @@ 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);
+
+/*
+ * Find a range subset that starts at or after s, ends before e,
+ * and is aligned to the size.
+ * If such subset is present it is removed from the rangeset and
+ * it's start is written to s, otherwise s is set to 0.
+ */
+int __must_check rangeset_claim_aligned_range(struct rangeset *r,
+ unsigned long size,
+ unsigned long *s,
+ unsigned long e);
int __must_check rangeset_remove_range(
struct rangeset *r, unsigned long s, unsigned long e);
bool __must_check rangeset_contains_range(
--
2.51.2On 18.11.2025 14:36, Mykyta Poturai wrote:
> @@ -232,6 +233,25 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
> return domain;
> }
>
> +static int __init 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;
> +
> + /* Ensure we are not using bits in a rangeset */
> + BUILD_BUG_ON(sizeof(unsigned long) != sizeof(uint64_t));
Can you please help me interpret the comment?
Also, rather than != isn't < sufficient to check for?
> @@ -283,6 +303,18 @@ pci_host_common_probe(struct dt_device_node *dev,
> bridge->child_ops = &child_ops->pci_ops;
> }
>
> + 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 )
> + {
> + err = dt_for_each_range(bridge->dt_node, add_bar_range, bridge);
> + if ( err )
> + goto err_child;
> + }
I'm pretty sure I commented on this already: Without an "else" use sites
of the two rangesets need to have NULL checks added, plus imo there would
want to be a comment here explaining to readers why omitting the "else"
(and hence proper error handling) is okay.
> @@ -476,6 +508,66 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>
> return bar_data.is_valid;
> }
> +
> +/*
> + * Find suitable place for an uninitialized bar of specified size in the
> + * host bridge ranges
> + */
> +uint64_t __init pci_get_new_bar_addr(const struct pci_dev *pdev, uint64_t size,
> + bool is_64bit, bool prefetch)
Seeing the comment - why only "host bridge"? Especially for Dom0, if other
bridges are present in the system, I think you won't get away without having
a virtual counterpart for evey one of them (or alternatively without hiding
all of them plus the devices behind them).
> +{
> + struct pci_host_bridge *bridge;
> + struct rangeset *range;
> + uint64_t addr = 0, end = GB(4);
> +
> + /* Make sure we can store addr in a rangeset */
> + BUILD_BUG_ON(sizeof(addr) != sizeof(unsigned long));
While "store" looks right here, ...
> + 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 )
> + {
> + addr = GB(4);
> + end = ~0;
> + }
> +
> + if ( !rangeset_claim_aligned_range(range, size, &addr, end) )
> + return addr;
> +
> + printk(XENLOG_ERR "Failed to claim BAR range %lx-%lx from rangeset\n",
> + addr, addr + size - 1);
> +
> + return 0;
> +}
> +
> +/*
> + * Remove already used memory from the host bridge bar ranges
> + */
> +int __init 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;
> +
> + /* Make sure we can store addr in a rangeset */
> + BUILD_BUG_ON(sizeof(addr) != sizeof(unsigned long));
... it doen't here, as ...
> + 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);
... there's nothing being stored.
But I'm apparently confused in a broader way: Here you remove a range from the
selected rangeset. rangeset_claim_aligned_range() also does so. Why are there
two removals?
> --- a/xen/arch/arm/pci/pci.c
> +++ b/xen/arch/arm/pci/pci.c
> @@ -95,6 +95,108 @@ boolean_param("pci-passthrough", pci_passthrough_enabled);
> __ro_after_init bool pci_scan_enabled;
> boolean_param("pci-scan", pci_scan_enabled);
>
> +typedef int (*bar_callback_t)(struct pci_dev *, uint8_t, uint64_t, uint64_t,
> + bool, bool);
Hmm, okay, you have a typedef now. But ...
> +static int __init reserve_bar_range(struct pci_dev *pdev, uint8_t reg,
> + uint64_t addr, uint64_t size, bool is_64bit,
> + bool prefetch)
... if I altered e.g. this function's signature, ...
> +{
> + if ( pci_check_bar(pdev, maddr_to_mfn(addr),
> + maddr_to_mfn(addr + size - 1)) )
> + return pci_reserve_bar_range(pdev, addr, size, prefetch);
> + return 0;
> +}
> +
> +static int __init setup_bar(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);
> + if ( !addr )
> + return -ENOMEM;
> +
> + 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);
> + }
> +
> + return 0;
> +}
> +
> +static int __init bars_iterate(struct pci_dev *pdev, void *arg)
... the use of void * here still renders things type-unsafe. Plus you
needlessly introduce function pointer <=> data pointer conversions,
which Misra wants us to avoid. IOW ...
> +{
> + unsigned int i, barsize, ret = 0, num_bars = PCI_HEADER_NORMAL_NR_BARS;
> + uint64_t addr, size;
> + bar_callback_t cb = arg;
... this (bar_callback_t cb) is what the function parameter wants to be.
Ideally though as "bar_callback_t *cb" to not hide the pointer-ness, then
requiring the pointer part to be dropped from the typedef itself.
> + if ( (pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f) ==
> + PCI_HEADER_TYPE_NORMAL )
> + {
> + for ( i = 0; i < num_bars; i += barsize )
> + {
> + 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 )
> + {
> + barsize = 1;
> + continue;
> + }
> +
> + barsize = 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;
> +
> + ret = cb(pdev, reg, addr, size, barsize == 2, prefetch);
> + if ( ret )
> + return ret;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int __init pci_setup_bars(void)
> +{
> + int ret;
> + /* We can't change the signature of bars_iterate to only accept
> + * bar_callback_t, so use intermediate variables to ensure callback
> + * signatures are always correct
> + */
> + bar_callback_t cb_reserve = reserve_bar_range;
> + bar_callback_t cb_setup = setup_bar;
Oh, here you actually fake partial type-safety. This should be dropped.
> @@ -244,8 +241,18 @@ int rangeset_remove_range(
> destroy_range(r, x);
> }
>
> - out:
> +out:
Why are you violating style here? See ./CODING_STYLE, also for other labels
you introduce.
> + return rc;
> +}
> +
> +int rangeset_remove_range(struct rangeset *r, unsigned long s, unsigned long e)
> +{
> + int rc = 0;
Pointless initializer.
> + write_lock(&r->lock);
> + rc = remove_range(r, s, e);
> write_unlock(&r->lock);
> +
> return rc;
> }
>
> @@ -357,6 +364,51 @@ int rangeset_claim_range(struct rangeset *r, unsigned long size,
> return 0;
> }
>
> +int rangeset_claim_aligned_range(struct rangeset *r, unsigned long size,
> + unsigned long *s, unsigned long e)
> +{
> + struct range *x;
> + int rc = 0;
Again, plus this variable looks to be used only ...
> + /* Power of 2 check */
> + if ( (size & (size - 1)) != 0 && size != 0 )
> + {
> + *s = 0;
> + return -EINVAL;
> + }
> +
> + if ( e < *s )
> + return -EINVAL;
> +
> + write_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 = ROUNDUP(x->s, size);
I don't think the comment is very useful - you do the necessary check above,
and what is said is an inherent property of ROUNDUP().
> + if ( x->e > start_aligned &&
> + (x->e - start_aligned) >= size &&
Remember that x->e is an inclusive upper bound.
> + start_aligned >= *s &&
> + start_aligned + size <= e)
> + {
> + rc = remove_range(r, start_aligned, start_aligned + size - 1);
... in the narrow scope (so should move here).
> + if ( !rc )
> + *s = start_aligned;
> + else
> + *s = 0;
Is it reasonably possible to take this path? If not, please add
ASSERT_UNREACHABLE().
> + write_unlock(&r->lock);
This can move up some, can't it? We want to keep locked regions as narrow as
possible.
> + return rc;
> + }
> + }
> +
> + *s = 0;
> +
> + write_unlock(&r->lock);
> + return -ENOSPC;
Blank line please ahead of the main / final "return" of a function.
> --- a/xen/include/xen/rangeset.h
> +++ b/xen/include/xen/rangeset.h
> @@ -61,6 +61,17 @@ 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);
> +
> +/*
> + * Find a range subset that starts at or after s, ends before e,
> + * and is aligned to the size.
Is "before" correct? Isn't it "at or before", just like for s it's "at or after"?
As to "aligned", nothing contrary being said here I think I ought to be able to
pass e.g. 7. (Tying together size and alignment is suitable for the BAR handling
purpose you have, but is making this new interface pretty much not general-
purpose.)
Jan
© 2016 - 2025 Red Hat, Inc.