[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH V2 2/3] xen/arm: Add handling of extended regions for Dom0



Hi,

On 18/09/2021 00:51, Oleksandr wrote:
On 17.09.21 18:48, Julien Grall wrote:
On 10/09/2021 23:18, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

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 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 extended regions could not be allocated for some reason,
Xen doesn't fail and behaves as usual, so only inserts region 0.

Please note the following limitations:
- The extended region feature is only supported for 64-bit domain.
- The ACPI case is not covered.

I understand the ACPI is not covered because we would need to create a new binding. But I am not sure to understand why 32-bit domain is not supported. Can you explain it?

The 32-bit domain is not supported for simplifying things from the beginning. It is a little bit difficult to get everything working at start. As I understand from discussion at [1] we can afford that simplification. However, I should have mentioned that 32-bit domain is not supported "for now".

Right, I forgot that. This is where it is useful to write down the decision in the commit message.




***

As Dom0 is direct mapped domain on Arm (e.g. MFN == GFN)
the algorithm to choose extended regions for it is different
in comparison with the algorithm for non-direct mapped DomU.
What is more, that extended regions should be chosen differently
whether IOMMU is enabled or not.

Provide RAM not assigned to Dom0 if IOMMU is disabled or memory
holes found in host device-tree if otherwise.

For the case when the IOMMU is disabled, this will only work if dom0 cannot allocate memory outside of the original range. This is currently the case... but I think this should be spelled out in at least the commit message.

Agree, will update commit description.




Make sure that
extended regions are 2MB-aligned and located within maximum possible
addressable physical memory range. The maximum number of extended
regions is 128.

Please explain how this limit was chosen.
Well, I decided to not introduce new data struct and etc to represent extended regions but reuse existing struct meminfo used for memory/reserved-memory and, as I though, perfectly fitted. So, that limit come from NR_MEM_BANKS which is 128.

Ok. So this is an artificial limit. Please make it clear in the commit message.




Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
---
Changes since RFC:
    - update patch description
    - drop uneeded "extended-region" DT property
---

  xen/arch/arm/domain_build.c | 226 +++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 224 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 206038d..070ec27 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -724,6 +724,196 @@ static int __init make_memory_node(const struct domain *d,
      return res;
  }
  +static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
+{
+    struct meminfo *ext_regions = data;
+    paddr_t start, size;
+
+    if ( ext_regions->nr_banks >= ARRAY_SIZE(ext_regions->bank) )
+        return 0;
+
+    /* Both start and size of the extended region should be 2MB aligned */
+    start = (s + SZ_2M - 1) & ~(SZ_2M - 1);
+    if ( start > e )
+        return 0;
+
+    size = (e - start + 1) & ~(SZ_2M - 1);
+    if ( !size )
+        return 0;
+
+    ext_regions->bank[ext_regions->nr_banks].start = start;
+    ext_regions->bank[ext_regions->nr_banks].size = size;
+    ext_regions->nr_banks ++;
+
+    return 0;
+}
+
+/*
+ * The extended regions will be prevalidated by the memory hotplug path
+ * in Linux which requires for any added address range to be within maximum + * possible addressable physical memory range for which the linear mapping
+ * could be created.
+ * For 48-bit VA space size the maximum addressable range are:

When I read "maximum", I understand an upper limit. But below, you are providing a range. So should you drop "maximum"?

yes, it is a little bit confusing.




Also, this is tailored to Linux using 48-bit VA. How about other limits?
These limits are calculated at [2]. Sorry, I didn't investigate yet what values would be for other CONFIG_ARM64_VA_BITS_XXX. Also looks like some configs depend on 16K/64K pages...
I will try to investigate and provide limits later on.

I have thought a bit more about it. At the moment, you are relying on Xen to find a range that is addressable by the OS. This can be quite complex as different OS may have different requirement. So how about letting the OS to filter the ranges based on its limitations?





+ * 0x40000000 - 0x80003fffffff
+ */
+#define EXT_REGION_START   0x40000000ULL

I am probably missing something here.... There are platform out there with memory starting at 0 (IIRC ZynqMP is one example). So wouldn't this potentially rule out the extended region on such platform?

From my understanding the extended region cannot be in 0...0x40000000 range. If these platforms have memory above first GB, I believe the extended region(s) can be allocated for them.

Do you mean "cannot"? Technically this is a limitation of the current version of Linux. Tomorrow, someone may be able to remove that limitations. So, as mentionned above, maybe Xen should not do the filtering.

+static int __init find_memory_holes(const struct kernel_info *kinfo,
+                                    struct meminfo *ext_regions)
+{
+    struct dt_device_node *np;
+    struct rangeset *mem_holes;
+    paddr_t start, end;
+    unsigned int i;
+    int res;
+
+    dt_dprintk("Find memory holes for extended regions\n");
+
+    mem_holes = rangeset_new(NULL, NULL, 0);
+    if ( !mem_holes )
+        return -ENOMEM;
+
+    /* Start with maximum possible addressable physical memory range */
+    start = EXT_REGION_START;
+    end = min((1ULL << p2m_ipa_bits) - 1, EXT_REGION_END);
+    res = rangeset_add_range(mem_holes, start, end);
+    if ( res )
+    {
+        printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n",
+               start, end);
+        goto out;
+    }
+
+    /* Remove all regions described by "reg" property (MMIO, RAM, etc) */

Well... The loop below is not going to handle all the regions described in the property "reg". Instead, it will cover a subset of "reg" where the memory is addressable.

As I understand, we are only interested in subset of "reg" where the memory is addressable.

Right... That's not what your comment is saying.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.