On Fri, 19 Nov 2021, Oleksandr wrote:
On 19.11.21 03:19, Stefano Stabellini wrote:You are right it is better to distinguish the two cases. I suggest the
On Wed, 10 Nov 2021, Oleksandr wrote:Sorry, I don't entirely understand the question.
On 28.10.21 04:40, Stefano Stabellini wrote:OK. But the check:
Hi Stefano
I am sorry for the late response.
On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote:Yes, we can do this here (since the checks are equivalent) but ...
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>Should this be:
This patch implements arch_xen_unpopulated_init() on Arm where
the extended regions (if any) are gathered from DT and inserted
into passed Xen resource to be used as unused address space
for Xen scratch pages by unpopulated-alloc code.
The extended region (safe range) is a region of guest physical
address space which is unused and could be safely used to create
grant/foreign mappings instead of wasting real RAM pages from
the domain memory for establishing these mappings.
The extended regions are chosen by the hypervisor at the domain
creation time and advertised to it via "reg" property under
hypervisor node in the guest device-tree. As region 0 is reserved
for grant table space (always present), the indexes for extended
regions are 1...N.
If arch_xen_unpopulated_init() fails for some reason the default
behaviour will be restored (allocate xenballooned pages).
This patch also removes XEN_UNPOPULATED_ALLOC dependency on x86.
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
---
Changes RFC -> V2:
- new patch, instead of
"[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to
provide
unallocated space"
---
arch/arm/xen/enlighten.c | 112
+++++++++++++++++++++++++++++++++++++++++++++++
drivers/xen/Kconfig | 2 +-
2 files changed, 113 insertions(+), 1 deletion(-)
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index dea46ec..1a1e0d3 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -62,6 +62,7 @@ static __read_mostly unsigned int xen_events_irq;
static phys_addr_t xen_grant_frames;
#define GRANT_TABLE_INDEX 0
+#define EXT_REGION_INDEX 1
uint32_t xen_start_flags;
EXPORT_SYMBOL(xen_start_flags);
@@ -303,6 +304,117 @@ static void __init xen_acpi_guest_init(void)
#endif
}
+#ifdef CONFIG_XEN_UNPOPULATED_ALLOC
+int arch_xen_unpopulated_init(struct resource *res)
+{
+ struct device_node *np;
+ struct resource *regs, *tmp_res;
+ uint64_t min_gpaddr = -1, max_gpaddr = 0;
+ unsigned int i, nr_reg = 0;
+ struct range mhp_range;
+ int rc;
+
+ if (!xen_domain())
+ return -ENODEV;
+
+ np = of_find_compatible_node(NULL, NULL, "xen,xen");
+ if (WARN_ON(!np))
+ return -ENODEV;
+
+ /* Skip region 0 which is reserved for grant table space */
+ while (of_get_address(np, nr_reg + EXT_REGION_INDEX, NULL,
NULL))
+ nr_reg++;
+ if (!nr_reg) {
+ pr_err("No extended regions are found\n");
+ return -EINVAL;
+ }
+
+ regs = kcalloc(nr_reg, sizeof(*regs), GFP_KERNEL);
+ if (!regs)
+ return -ENOMEM;
+
+ /*
+ * Create resource from extended regions provided by the
hypervisor to
be
+ * used as unused address space for Xen scratch pages.
+ */
+ for (i = 0; i < nr_reg; i++) {
+ rc = of_address_to_resource(np, i + EXT_REGION_INDEX,
®s[i]);
+ if (rc)
+ goto err;
+
+ if (max_gpaddr < regs[i].end)
+ max_gpaddr = regs[i].end;
+ if (min_gpaddr > regs[i].start)
+ min_gpaddr = regs[i].start;
+ }
+
+ /* Check whether the resource range is within the hotpluggable
range
*/
+ mhp_range = mhp_get_pluggable_range(true);
+ if (min_gpaddr < mhp_range.start)
+ min_gpaddr = mhp_range.start;
+ if (max_gpaddr > mhp_range.end)
+ max_gpaddr = mhp_range.end;
+
+ res->start = min_gpaddr;
+ res->end = max_gpaddr;
+
+ /*
+ * Mark holes between extended regions as unavailable. The
rest of
that
+ * address space will be available for the allocation.
+ */
+ for (i = 1; i < nr_reg; i++) {
+ resource_size_t start, end;
+
+ start = regs[i - 1].end + 1;
+ end = regs[i].start - 1;
+
+ if (start > (end + 1)) {
if (start >= end)
?
... not here.+ rc = -EINVAL;Also here, shouldn't it be:
+ goto err;
+ }
+
+ /* There is no hole between regions */
+ if (start == (end + 1))
if (start == end)
?
As
"(start == (end + 1))" is equal to "(regs[i - 1].end + 1 ==
regs[i].start)"
but
"(start == end)" is equal to "(regs[i - 1].end + 1 == regs[i].start - 1)"
if (start >= end)
Actually covers both cases so that's the only check we need?
Is the question to use only a single check in that loop?
Paste the updated code which I have locally for the convenience.
[snip]
/*
* Mark holes between extended regions as unavailable. The rest of that
* address space will be available for the allocation.
*/
for (i = 1; i < nr_reg; i++) {
resource_size_t start, end;
start = regs[i - 1].end + 1;
end = regs[i].start - 1;
if (start > (end + 1)) {
rc = -EINVAL;
goto err;
}
/* There is no hole between regions */
if (start == (end + 1))
continue;
tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
if (!tmp_res) {
rc = -ENOMEM;
goto err;
}
tmp_res->name = "Unavailable space";
tmp_res->start = start;
tmp_res->end = end;
rc = insert_resource(&xen_resource, tmp_res);
if (rc) {
pr_err("Cannot insert resource %pR (%d)\n", tmp_res, rc);
kfree(tmp_res);
goto err;
}
}
[snip]
1. The first check is to detect an overlap (which is a wrong configuration,
correct?) and bail out if true (for example, regX: 0x81000000...0x82FFFFFF and
regY: 0x82000000...0x83FFFFFF).
2. The second check is just to skip current iteration as there is no
space/hole between regions (for example, regX: 0x81000000...0x82FFFFFF and
regY: 0x83000000...0x83FFFFFF).
Therefore I think they should be distinguished.
Yes, both check could be transformed to a single one, but this way the
overlaps will be ignored:
if (start >= (end + 1))
continue;
Or I really missed something?
code below because I think it is a clearer, even if it might be slightly
less efficient. I don't feel too strongly about it though.
resource_size_t start, end;
/* There is no hole between regions */
if ( regs[i - 1].end + 1 == regs[i].start )
continue;
if ( regs[i - 1].end + 1 > regs[i].start) {
rc = -EINVAL;
goto err;
}
start = regs[i - 1].end + 1;
end = regs[i].start - 1;