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

Re: [PATCH V6 2/2] libxl/arm: Add handling of extended regions for DomU




On 12.10.21 19:05, Julien Grall wrote:
Hi Oleksandr,

Hi Julien




On 11/10/2021 18:48, Oleksandr Tyshchenko wrote:
---
  tools/libs/light/libxl_arm.c  | 76 ++++++++++++++++++++++++++++++++++++++++---
  xen/include/public/arch-arm.h |  2 ++
  2 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6..c0e8415 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -598,9 +598,20 @@ static int make_timer_node(libxl__gc *gc, void *fdt,
      return 0;
  }
  +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
+
  static int make_hypervisor_node(libxl__gc *gc, void *fdt,
-                                const libxl_version_info *vers)
+                                const libxl_version_info *vers,
+                                const libxl_domain_build_info *b_info,
+                                const struct xc_dom_image *dom)
  {
+    uint64_t region_size[GUEST_RAM_BANKS] = {0}, region_base[GUEST_RAM_BANKS],
+        bank1end, ramsize;
+    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
+                  (GUEST_RAM_BANKS + 1)];
+    be32 *cells = &regs[0];
+    unsigned int i, len, nr_regions = 0;
+    libxl_dominfo info;
      int res;
      gic_interrupt intr;
  @@ -615,9 +626,64 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
                                "xen,xen");
      if (res) return res;
  -    /* reg 0 is grant table space */
-    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
-                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
+    if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
+        LOG(WARN, "The extended regions are only supported for 64-bit guest currently");
+        goto out;
+    }

I understand why we want to limit to 64-bit domain for dom0. But I am not sure this is warrant for 32-bit domain. At worse, the guest will ignore the bank because it is not usable. So could we drop the check?

Yes.




+
+    res = libxl_domain_info(CTX, &info, dom->guest_domid);
+    if (res) return res;
+
+    assert(info.gpaddr_bits >= 32 && info.gpaddr_bits <= 48);
What could go wrong below if gpaddr_bits is not within this range?

if info.gpaddr_bits is less than 64, then nothing bad, otherwise, I assume we will get shift count overflow.




+
+    /*
+     * Try to allocate separate 2MB-aligned extended regions from the first +     * (below 4GB) and second (above 4GB) RAM banks taking into the account +     * the maximum supported guest physical address space size and the amount
+     * of memory assigned to the guest.
+     * As the guest memory layout is not populated yet we cannot rely on +     * dom->rambank_size[], so calculate the actual size of both banks using
+     * "max_memkb" value.
+     */

At the moment, libxl doesn't know how libxc will allocate the memory. We may decide in the future to have only a small amount of memory below 4GB and then the rest above 4GB. With this approach it would be more difficult to modify the memory layout. Instead, I think we should create a placeholder that is updated once we know the banks in libxl__arch_domain_finalise_hw_description.

If I got your point correctly, this is close to how it was done from the beginning. Yes, we can create placeholder(s) here and then update them once the memory layout is populated. The problem is that we won't be able to remove the placeholder(s) if we fail to allocate region(s) for some reasons. So, we should know for sure in advance how many region(s) we will be able to allocate later on in order to create the required number of placeholders right now...  Please, look at the TODO I wrote in finalise_ext_region() [1]. Or I misread your point?




We also probably want to mention in the memory layout in public/arch-arm.h this decision as the suggested way to find extended regions will definitely impact our decision to re-order the memory layout or shrink some regions in the future (I have in mind the PCI Passthrough work).

Sorry, I couldn't parse.




+    ramsize = b_info->max_memkb * 1024;
+    if (ramsize <= GUEST_RAM0_SIZE) {
+        region_base[0] = GUEST_RAM0_BASE + ALIGN_UP_TO_2MB(ramsize);
+        region_size[0] = GUEST_RAM0_SIZE - ALIGN_UP_TO_2MB(ramsize);
+        region_base[1] = GUEST_RAM1_BASE;
+    } else
+        region_base[1] = GUEST_RAM1_BASE +
+            ALIGN_UP_TO_2MB(ramsize - GUEST_RAM0_SIZE);
+
+    bank1end = min(1ULL << info.gpaddr_bits, GUEST_RAM1_BASE + GUEST_RAM1_SIZE);
+    if (bank1end > region_base[1])
+        region_size[1] = bank1end - region_base[1];

It would be best to not rely on the fact that Bank on is always below 4GB. If the code is too complex then we should look to add a BUILD_BUG_ON() to avoid any surprise.

Yes, I can add:

BUILD_BUG_ON((GUEST_RAM0_BASE + GUEST_RAM0_SIZE) > GB(4));


[1] https://lore.kernel.org/xen-devel/1631297924-8658-4-git-send-email-olekstysh@xxxxxxxxx/



Cheers,

--
Regards,

Oleksandr Tyshchenko




 


Rackspace

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