[PATCH V6 0/2] Add handling of extended regions (safe ranges) on Arm (Was "xen/memory: Introduce a hypercall to provide unallocated space")

[PATCH V6 0/2] Add handling of extended regions (safe ranges) on Arm (Was "xen/memory: Introduce a hypercall to provide unallocated space")

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

You can find an initial discussion at [1]-[6].

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
(the indexes for extended regions are 1...N).
No device tree bindings update is needed, guest infers the presense of extended
regions from the number of regions in "reg" property.
New compatible/property will be needed (but only after this patch [7] or alternative
goes in) to indicate that "region 0 is safe to use". Until this patch is merged it is
not safe to use extended regions for the grant table space.

The extended regions are calculated differently for direct mapped Dom0 (with and without
IOMMU) and non-direct mapped DomUs.

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

Please note that support for Dom0 was already committed, so these patches are remaining DomU bits.

Xen patch series is also available at [8]. The corresponding Linux patch series is at [9]
for now (last 4 patches).

Tested on Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with updated virtio-disk backend [10]
running in Dom0 (256MB RAM) and DomD (2GB RAM). In both cases the backend pre-maps DomU memory
which is 3GB. All foreign memory gets mapped into extended regions (so the amount of RAM in
the backend domain is not reduced). No issues were observed.

[1] https://lore.kernel.org/xen-devel/1627489110-25633-1-git-send-email-olekstysh@gmail.com/
[2] https://lore.kernel.org/xen-devel/1631034578-12598-1-git-send-email-olekstysh@gmail.com/
[3] https://lore.kernel.org/xen-devel/1631297924-8658-1-git-send-email-olekstysh@gmail.com/
[4] https://lore.kernel.org/xen-devel/1632437334-12015-1-git-send-email-olekstysh@gmail.com/
[5] https://lore.kernel.org/xen-devel/1632955927-27911-1-git-send-email-olekstysh@gmail.com/
[6] https://lore.kernel.org/xen-devel/1633519346-3686-1-git-send-email-olekstysh@gmail.com/
[7] https://lore.kernel.org/xen-devel/1632425551-18910-1-git-send-email-olekstysh@gmail.com/
[8] https://github.com/otyshchenko1/xen/commits/map_opt_ml7
[9] https://github.com/otyshchenko1/linux/commits/map_opt_ml4
[10] https://github.com/otyshchenko1/virtio-disk/commits/map_opt_next

Oleksandr Tyshchenko (2):
  xen/arm: Introduce gpaddr_bits field to struct
    xen_domctl_getdomaininfo
  libxl/arm: Add handling of extended regions for DomU

 tools/include/libxl.h            |  8 +++++
 tools/include/xenctrl.h          |  1 +
 tools/libs/ctrl/xc_domain.c      |  1 +
 tools/libs/light/libxl_arm.c     | 76 +++++++++++++++++++++++++++++++++++++---
 tools/libs/light/libxl_domain.c  |  1 +
 tools/libs/light/libxl_types.idl |  1 +
 xen/arch/arm/domctl.c            |  2 ++
 xen/arch/x86/domctl.c            |  1 +
 xen/include/public/arch-arm.h    |  2 ++
 xen/include/public/domctl.h      |  3 +-
 10 files changed, 90 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

We need to pass info about maximum supported guest physical
address space size to the toolstack on Arm in order to properly
calculate the base and size of the extended region (safe range)
for the guest. The extended region is unused address space which
could be safely used by domain for foreign/grant mappings on Arm.
The extended region itself will be handled by the subsequent
patch.

Currently the same guest physical address space size is used
for all guests (p2m_ipa_bits variable on Arm, the x86 equivalent
is hap_paddr_bits).

As we add new field to the structure bump the interface version.

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
Changes RFC -> V2:
   - update patch subject/description
   - replace arch-specific sub-struct with common gpaddr_bits
     field and update code to reflect that

Changes V2 -> V3:
   - make the field uint8_t and add uint8_t pad[7] after
   - remove leading blanks in libxl.h

Changes V3 -> V4:
   - also print gpaddr_bits from output_physinfo()
   - add Michal's R-b

Changes V4 -> V5:
   - update patch subject and description
   - drop Michal's R-b
   - pass gpaddr_bits via createdomain domctl
     (struct xen_arch_domainconfig)

Changes V5 -> V6:
   - update patch subject and description
   - pass gpaddr_bits via getdomaininfo domctl
     (struct xen_domctl_getdomaininfo)
---
 tools/include/libxl.h            | 8 ++++++++
 tools/include/xenctrl.h          | 1 +
 tools/libs/ctrl/xc_domain.c      | 1 +
 tools/libs/light/libxl_domain.c  | 1 +
 tools/libs/light/libxl_types.idl | 1 +
 xen/arch/arm/domctl.c            | 2 ++
 xen/arch/x86/domctl.c            | 1 +
 xen/include/public/domctl.h      | 3 ++-
 8 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d..deb5022 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -874,6 +874,14 @@ typedef struct libxl__ctx libxl_ctx;
 #define LIBXL_HAVE_DOMINFO_NEVER_STOP 1
 
 /*
+ * LIBXL_HAVE_DOMINFO_GPADDR_BITS
+ *
+ * If this is defined, libxl_dominfo will contain an uint8 field called
+ * gpaddr_bits, containing the guest physical address space size.
+ */
+#define LIBXL_HAVE_DOMINFO_GPADDR_BITS 1
+
+/*
  * LIBXL_HAVE_QXL
  *
  * If defined, then the libxl_vga_interface_type will contain another value:
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index a306399..07b96e6 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -462,6 +462,7 @@ typedef struct xc_dominfo {
     unsigned int  max_vcpu_id;
     xen_domain_handle_t handle;
     unsigned int  cpupool;
+    uint8_t       gpaddr_bits;
     struct xen_arch_domainconfig arch_config;
 } xc_dominfo_t;
 
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index 23322b7..b155d6a 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -396,6 +396,7 @@ int xc_domain_getinfo(xc_interface *xch,
         info->nr_online_vcpus = domctl.u.getdomaininfo.nr_online_vcpus;
         info->max_vcpu_id = domctl.u.getdomaininfo.max_vcpu_id;
         info->cpupool = domctl.u.getdomaininfo.cpupool;
+        info->gpaddr_bits = domctl.u.getdomaininfo.gpaddr_bits;
         info->arch_config = domctl.u.getdomaininfo.arch_config;
 
         memcpy(info->handle, domctl.u.getdomaininfo.handle,
diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c
index 51a6127..544a9bf 100644
--- a/tools/libs/light/libxl_domain.c
+++ b/tools/libs/light/libxl_domain.c
@@ -306,6 +306,7 @@ void libxl__xcinfo2xlinfo(libxl_ctx *ctx,
     xlinfo->vcpu_max_id = xcinfo->max_vcpu_id;
     xlinfo->vcpu_online = xcinfo->nr_online_vcpus;
     xlinfo->cpupool = xcinfo->cpupool;
+    xlinfo->gpaddr_bits = xcinfo->gpaddr_bits;
     xlinfo->domain_type = (xcinfo->flags & XEN_DOMINF_hvm_guest) ?
         LIBXL_DOMAIN_TYPE_HVM : LIBXL_DOMAIN_TYPE_PV;
 }
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 3f9fff6..2df7258 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -357,6 +357,7 @@ libxl_dominfo = Struct("dominfo",[
     ("vcpu_max_id", uint32),
     ("vcpu_online", uint32),
     ("cpupool",     uint32),
+    ("gpaddr_bits", uint8),
     ("domain_type", libxl_domain_type),
     ], dir=DIR_OUT)
 
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index b7d27f3..6245af6 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -20,6 +20,8 @@ void arch_get_domain_info(const struct domain *d,
 {
     /* All ARM domains use hardware assisted paging. */
     info->flags |= XEN_DOMINF_hap;
+
+    info->gpaddr_bits = p2m_ipa_bits;
 }
 
 static int handle_vuart_init(struct domain *d, 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 26a76d2..7d102e0 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -151,6 +151,7 @@ void arch_get_domain_info(const struct domain *d,
         info->flags |= XEN_DOMINF_hap;
 
     info->arch_config.emulation_flags = d->arch.emulation_flags;
+    info->gpaddr_bits = hap_paddr_bits;
 }
 
 static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op,
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 4cb3f66..b93f776 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -38,7 +38,7 @@
 #include "hvm/save.h"
 #include "memory.h"
 
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
 
 /*
  * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -150,6 +150,7 @@ struct xen_domctl_getdomaininfo {
     uint32_t ssidref;
     xen_domain_handle_t handle;
     uint32_t cpupool;
+    uint8_t gpaddr_bits; /* Guest physical address space size. */
     struct xen_arch_domainconfig arch_config;
 };
 typedef struct xen_domctl_getdomaininfo xen_domctl_getdomaininfo_t;
-- 
2.7.4

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

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

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
  currently.
- The ACPI case is not covered.

***

The algorithm to choose extended regions for non-direct mapped
DomU is simpler in comparison with the algorithm for direct mapped
Dom0. We usually have a lot of unused space above 4GB, and might
have some unused space below 4GB (depends on guest memory size).
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. The minimum size of extended region
the same as for Dom0 (64MB).

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
Changes RFC -> V2:
   - update patch description
   - drop uneeded "extended-region" DT property
   - clear reg array in finalise_ext_region() and add a TODO

Changes V2 -> V3:
   - update patch description, comments in code
   - only pick up regions with size >= 64MB
   - move the region calculation to make_hypervisor_node() and drop
     finalise_ext_region()
   - extend the list of arguments for make_hypervisor_node()
   - do not show warning for 32-bit domain
   - change the region alignment from 1GB to 2MB
   - move EXT_REGION_SIZE to public/arch-arm.h

Changes V3 -> V4:
   - add R-b, A-b and T-b

Changes V4 -> V5:
   - update patch description and comments in code
   - reflect changes done in previous patch to pass gpaddr_bits
     via createdomain domctl (struct xen_arch_domainconfig)
   - drop R-b, A-b and T-b
   - drop limit for maximum extended region size (128GB)
   - try to also allocate region below 4GB, optimize code
     for calculating extended regions

Change V5 -> V6:
   - reflect changes done in previous patch to pass gpaddr_bits
     via getdomaininfo domctl (struct xen_domctl_getdomaininfo)
   - reduce the number of local variables, rework calculations
---
 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;
+    }
+
+    res = libxl_domain_info(CTX, &info, dom->guest_domid);
+    if (res) return res;
+
+    assert(info.gpaddr_bits >= 32 && info.gpaddr_bits <= 48);
+
+    /*
+     * 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.
+     */
+    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];
+
+out:
+    /*
+     * The region 0 for grant table space must be always present. If we managed
+     * to allocate the extended regions then insert them as regions 1...N.
+     */
+    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
+
+    for (i = 0; i < GUEST_RAM_BANKS; i++) {
+        if (region_size[i] < GUEST_EXT_REGION_MIN_SIZE)
+            continue;
+
+        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"\n",
+            nr_regions, region_base[i], region_base[i] + region_size[i]);
+
+        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+                  region_base[i], region_size[i]);
+        nr_regions++;
+    }
+
+    if (!nr_regions)
+        LOG(WARN, "The extended regions cannot be allocated, not enough space");
+
+    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
+        (nr_regions + 1);
+    res = fdt_property(fdt, "reg", regs, len);
     if (res) return res;
 
     /*
@@ -963,7 +1029,7 @@ next_resize:
         }
 
         FDT( make_timer_node(gc, fdt, ainfo, state->clock_frequency) );
-        FDT( make_hypervisor_node(gc, fdt, vers) );
+        FDT( make_hypervisor_node(gc, fdt, vers, info, dom) );
 
         if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART)
             FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index d46c61f..19ca2b0 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -451,6 +451,8 @@ typedef uint64_t xen_callback_t;
 #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
 #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
 
+#define GUEST_EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
+
 /* Current supported guest VCPUs */
 #define GUEST_MAX_VCPUS 128
 
-- 
2.7.4

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

[Stefano Stabellini]

On Mon, 11 Oct 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> 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
>   currently.
> - The ACPI case is not covered.
> 
> ***
> 
> The algorithm to choose extended regions for non-direct mapped
> DomU is simpler in comparison with the algorithm for direct mapped
> Dom0. We usually have a lot of unused space above 4GB, and might
> have some unused space below 4GB (depends on guest memory size).
> 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. The minimum size of extended region
> the same as for Dom0 (64MB).
> 
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Changes RFC -> V2:
>    - update patch description
>    - drop uneeded "extended-region" DT property
>    - clear reg array in finalise_ext_region() and add a TODO
> 
> Changes V2 -> V3:
>    - update patch description, comments in code
>    - only pick up regions with size >= 64MB
>    - move the region calculation to make_hypervisor_node() and drop
>      finalise_ext_region()
>    - extend the list of arguments for make_hypervisor_node()
>    - do not show warning for 32-bit domain
>    - change the region alignment from 1GB to 2MB
>    - move EXT_REGION_SIZE to public/arch-arm.h
> 
> Changes V3 -> V4:
>    - add R-b, A-b and T-b
> 
> Changes V4 -> V5:
>    - update patch description and comments in code
>    - reflect changes done in previous patch to pass gpaddr_bits
>      via createdomain domctl (struct xen_arch_domainconfig)
>    - drop R-b, A-b and T-b
>    - drop limit for maximum extended region size (128GB)
>    - try to also allocate region below 4GB, optimize code
>      for calculating extended regions
> 
> Change V5 -> V6:
>    - reflect changes done in previous patch to pass gpaddr_bits
>      via getdomaininfo domctl (struct xen_domctl_getdomaininfo)
>    - reduce the number of local variables, rework calculations
> ---
>  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;
> +    }
> +
> +    res = libxl_domain_info(CTX, &info, dom->guest_domid);
> +    if (res) return res;
> +
> +    assert(info.gpaddr_bits >= 32 && info.gpaddr_bits <= 48);
> +
> +    /*
> +     * 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.
> +     */
> +    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];
> +
> +out:
> +    /*
> +     * The region 0 for grant table space must be always present. If we managed
> +     * to allocate the extended regions then insert them as regions 1...N.
> +     */
> +    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> +
> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> +        if (region_size[i] < GUEST_EXT_REGION_MIN_SIZE)
> +            continue;
> +
> +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"\n",
> +            nr_regions, region_base[i], region_base[i] + region_size[i]);
> +
> +        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +                  region_base[i], region_size[i]);
> +        nr_regions++;
> +    }
> +
> +    if (!nr_regions)
> +        LOG(WARN, "The extended regions cannot be allocated, not enough space");
> +
> +    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> +        (nr_regions + 1);
> +    res = fdt_property(fdt, "reg", regs, len);
>      if (res) return res;
>  
>      /*
> @@ -963,7 +1029,7 @@ next_resize:
>          }
>  
>          FDT( make_timer_node(gc, fdt, ainfo, state->clock_frequency) );
> -        FDT( make_hypervisor_node(gc, fdt, vers) );
> +        FDT( make_hypervisor_node(gc, fdt, vers, info, dom) );
>  
>          if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART)
>              FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index d46c61f..19ca2b0 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -451,6 +451,8 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
>  #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
>  
> +#define GUEST_EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
> +
>  /* Current supported guest VCPUs */
>  #define GUEST_MAX_VCPUS 128
>  
> -- 
> 2.7.4
> 

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Stefano Stabellini]

On Mon, 11 Oct 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> We need to pass info about maximum supported guest physical
> address space size to the toolstack on Arm in order to properly
> calculate the base and size of the extended region (safe range)
> for the guest. The extended region is unused address space which
> could be safely used by domain for foreign/grant mappings on Arm.
> The extended region itself will be handled by the subsequent
> patch.
> 
> Currently the same guest physical address space size is used
> for all guests (p2m_ipa_bits variable on Arm, the x86 equivalent
> is hap_paddr_bits).
> 
> As we add new field to the structure bump the interface version.
> 
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

I couldn't spot any errors in this patch


> ---
> Changes RFC -> V2:
>    - update patch subject/description
>    - replace arch-specific sub-struct with common gpaddr_bits
>      field and update code to reflect that
> 
> Changes V2 -> V3:
>    - make the field uint8_t and add uint8_t pad[7] after
>    - remove leading blanks in libxl.h
> 
> Changes V3 -> V4:
>    - also print gpaddr_bits from output_physinfo()
>    - add Michal's R-b
> 
> Changes V4 -> V5:
>    - update patch subject and description
>    - drop Michal's R-b
>    - pass gpaddr_bits via createdomain domctl
>      (struct xen_arch_domainconfig)
> 
> Changes V5 -> V6:
>    - update patch subject and description
>    - pass gpaddr_bits via getdomaininfo domctl
>      (struct xen_domctl_getdomaininfo)
> ---
>  tools/include/libxl.h            | 8 ++++++++
>  tools/include/xenctrl.h          | 1 +
>  tools/libs/ctrl/xc_domain.c      | 1 +
>  tools/libs/light/libxl_domain.c  | 1 +
>  tools/libs/light/libxl_types.idl | 1 +
>  xen/arch/arm/domctl.c            | 2 ++
>  xen/arch/x86/domctl.c            | 1 +
>  xen/include/public/domctl.h      | 3 ++-
>  8 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index b9ba16d..deb5022 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -874,6 +874,14 @@ typedef struct libxl__ctx libxl_ctx;
>  #define LIBXL_HAVE_DOMINFO_NEVER_STOP 1
>  
>  /*
> + * LIBXL_HAVE_DOMINFO_GPADDR_BITS
> + *
> + * If this is defined, libxl_dominfo will contain an uint8 field called
> + * gpaddr_bits, containing the guest physical address space size.
> + */
> +#define LIBXL_HAVE_DOMINFO_GPADDR_BITS 1
> +
> +/*
>   * LIBXL_HAVE_QXL
>   *
>   * If defined, then the libxl_vga_interface_type will contain another value:
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index a306399..07b96e6 100644
> --- a/tools/include/xenctrl.h
> +++ b/ tools/include/xenctrl.h
> @@ -462,6 +462,7 @@ typedef struct xc_dominfo {
>      unsigned int  max_vcpu_id;
>      xen_domain_handle_t handle;
>      unsigned int  cpupool;
> +    uint8_t       gpaddr_bits;
>      struct xen_arch_domainconfig arch_config;
>  } xc_dominfo_t;
>  
> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> index 23322b7..b155d6a 100644
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -396,6 +396,7 @@ int xc_domain_getinfo(xc_interface *xch,
>          info->nr_online_vcpus = domctl.u.getdomaininfo.nr_online_vcpus;
>          info->max_vcpu_id = domctl.u.getdomaininfo.max_vcpu_id;
>          info->cpupool = domctl.u.getdomaininfo.cpupool;
> +        info->gpaddr_bits = domctl.u.getdomaininfo.gpaddr_bits;
>          info->arch_config = domctl.u.getdomaininfo.arch_config;
>  
>          memcpy(info->handle, domctl.u.getdomaininfo.handle,
> diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c
> index 51a6127..544a9bf 100644
> --- a/tools/libs/light/libxl_domain.c
> +++ b/tools/libs/light/libxl_domain.c
> @@ -306,6 +306,7 @@ void libxl__xcinfo2xlinfo(libxl_ctx *ctx,
>      xlinfo->vcpu_max_id = xcinfo->max_vcpu_id;
>      xlinfo->vcpu_online = xcinfo->nr_online_vcpus;
>      xlinfo->cpupool = xcinfo->cpupool;
> +    xlinfo->gpaddr_bits = xcinfo->gpaddr_bits;
>      xlinfo->domain_type = (xcinfo->flags & XEN_DOMINF_hvm_guest) ?
>          LIBXL_DOMAIN_TYPE_HVM : LIBXL_DOMAIN_TYPE_PV;
>  }
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 3f9fff6..2df7258 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -357,6 +357,7 @@ libxl_dominfo = Struct("dominfo",[
>      ("vcpu_max_id", uint32),
>      ("vcpu_online", uint32),
>      ("cpupool",     uint32),
> +    ("gpaddr_bits", uint8),
>      ("domain_type", libxl_domain_type),
>      ], dir=DIR_OUT)
>  
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index b7d27f3..6245af6 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -20,6 +20,8 @@ void arch_get_domain_info(const struct domain *d,
>  {
>      /* All ARM domains use hardware assisted paging. */
>      info->flags |= XEN_DOMINF_hap;
> +
> +    info->gpaddr_bits = p2m_ipa_bits;
>  }
>  
>  static int handle_vuart_init(struct domain *d, 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 26a76d2..7d102e0 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -151,6 +151,7 @@ void arch_get_domain_info(const struct domain *d,
>          info->flags |= XEN_DOMINF_hap;
>  
>      info->arch_config.emulation_flags = d->arch.emulation_flags;
> +    info->gpaddr_bits = hap_paddr_bits;
>  }
>  
>  static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op,
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 4cb3f66..b93f776 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
>  #include "hvm/save.h"
>  #include "memory.h"
>  
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
>  
>  /*
>   * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> @@ -150,6 +150,7 @@ struct xen_domctl_getdomaininfo {
>      uint32_t ssidref;
>      xen_domain_handle_t handle;
>      uint32_t cpupool;
> +    uint8_t gpaddr_bits; /* Guest physical address space size. */
>      struct xen_arch_domainconfig arch_config;
>  };
>  typedef struct xen_domctl_getdomaininfo xen_domctl_getdomaininfo_t;
> -- 
> 2.7.4
> 

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Jan Beulich]

On 11.10.2021 19:48, Oleksandr Tyshchenko wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
>  #include "hvm/save.h"
>  #include "memory.h"
>  
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015

So this bump would not have been needed if the rule of making padding
fields explicit in the public interface had been followed by earlier
changes, as you could have fit the 8-bit field in the 16-bit gap
after domid.

Furthermore this bump is only going to be necessary if your patch
doesn't make 4.16. 4.15 uses 0x13 here, i.e. a bump has already
occurred in this release cycle.

Otoh, because of the re-use of the struct in a sysctl, you do need
to bump XEN_SYSCTL_INTERFACE_VERSION here (unless you fit the new
field in the existing padding slot, which for the sysctl has been
guaranteed to be zero; see also below).

> @@ -150,6 +150,7 @@ struct xen_domctl_getdomaininfo {
>      uint32_t ssidref;
>      xen_domain_handle_t handle;
>      uint32_t cpupool;
> +    uint8_t gpaddr_bits; /* Guest physical address space size. */
>      struct xen_arch_domainconfig arch_config;

On the basis of the above, please add uint8_t pad[3] (or perhaps
better pad[7] to be independent of the present
alignof(struct xen_arch_domainconfig) == 4) and make sure the
array will always be zero-filled, such that the padding space can
subsequently be assigned a purpose without needing to bump the
interface version(s) again.

Right now the sysctl caller of getdomaininfo() clears the full
structure (albeit only once, so stale / inapplicable data might get
reported for higher numbered domains if any field was filled only
in certain cases), while the domctl one doesn't. Maybe it would be
best looking forward if the domctl path also cleared the full buffer
up front, or if the clearing was moved into the function. (In the
latter case some writes of zeros into the struct could be eliminated
in return.)

Perhaps, once properly first zero-filling the struct in all cases,
the padding field near the start should also be made explicit,
clarifying visually that it is an option to use the space there for
something that makes sense to live early in the struct (i.e. I
wouldn't necessarily recommend putting there the new field you add,
albeit - as mentioned near the top - that's certainly an option).

Jan

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Oleksandr]

On 12.10.21 12:24, Jan Beulich wrote:

Hi Jan

Thank you for thorough review.

> On 11.10.2021 19:48, Oleksandr Tyshchenko wrote:
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -38,7 +38,7 @@
>>   #include "hvm/save.h"
>>   #include "memory.h"
>>   
>> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
>> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
> So this bump would not have been needed if the rule of making padding
> fields explicit in the public interface had been followed by earlier
> changes, as you could have fit the 8-bit field in the 16-bit gap
> after domid.
>
> Furthermore this bump is only going to be necessary if your patch
> doesn't make 4.16. 4.15 uses 0x13 here, i.e. a bump has already
> occurred in this release cycle.

I got it, will remove the bumping.


>
> Otoh, because of the re-use of the struct in a sysctl, you do need
> to bump XEN_SYSCTL_INTERFACE_VERSION here (unless you fit the new
> field in the existing padding slot, which for the sysctl has been
> guaranteed to be zero; see also below).

Oops, indeed, will bump.


>
>> @@ -150,6 +150,7 @@ struct xen_domctl_getdomaininfo {
>>       uint32_t ssidref;
>>       xen_domain_handle_t handle;
>>       uint32_t cpupool;
>> +    uint8_t gpaddr_bits; /* Guest physical address space size. */
>>       struct xen_arch_domainconfig arch_config;
> On the basis of the above, please add uint8_t pad[3] (or perhaps
> better pad[7] to be independent of the present
> alignof(struct xen_arch_domainconfig) == 4) and make sure the
> array will always be zero-filled, such that the padding space can
> subsequently be assigned a purpose without needing to bump the
> interface version(s) again.

ok, will do.


>
> Right now the sysctl caller of getdomaininfo() clears the full
> structure (albeit only once, so stale / inapplicable data might get
> reported for higher numbered domains if any field was filled only
> in certain cases), while the domctl one doesn't. Maybe it would be
> best looking forward if the domctl path also cleared the full buffer
> up front, or if the clearing was moved into the function. (In the
> latter case some writes of zeros into the struct could be eliminated
> in return.)

Well, I would be OK either way, with a little preference for the latter.

Is it close to what you meant?


diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 46a0c8a..9bca133 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -69,10 +69,10 @@ void getdomaininfo(struct domain *d, struct 
xen_domctl_getdomaininfo *info)
      int flags = XEN_DOMINF_blocked;
      struct vcpu_runstate_info runstate;

+    memset(info, 0, sizeof(*info));
+
      info->domain = d->domain_id;
      info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
-    info->nr_online_vcpus = 0;
-    info->ssidref = 0;

      /*
       * - domain is marked as blocked only if all its vcpus are blocked
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 3558641..a7ab95d 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -76,7 +76,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
u_sysctl)
      case XEN_SYSCTL_getdomaininfolist:
      {
          struct domain *d;
-        struct xen_domctl_getdomaininfo info = { 0 };
+        struct xen_domctl_getdomaininfo info;
          u32 num_domains = 0;

          rcu_read_lock(&domlist_read_lock);


>
> Perhaps, once properly first zero-filling the struct in all cases,
> the padding field near the start should also be made explicit,
> clarifying visually that it is an option to use the space there for
> something that makes sense to live early in the struct (i.e. I
> wouldn't necessarily recommend putting there the new field you add,
> albeit - as mentioned near the top - that's certainly an option).

I read this as a request to also add uint16_t pad after "domid_t domain" 
at least. Correct?


>
> Jan
>
-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Jan Beulich]

On 12.10.2021 13:28, Oleksandr wrote:
> On 12.10.21 12:24, Jan Beulich wrote:
>> On 11.10.2021 19:48, Oleksandr Tyshchenko wrote:
>>> @@ -150,6 +150,7 @@ struct xen_domctl_getdomaininfo {
>>>       uint32_t ssidref;
>>>       xen_domain_handle_t handle;
>>>       uint32_t cpupool;
>>> +    uint8_t gpaddr_bits; /* Guest physical address space size. */
>>>       struct xen_arch_domainconfig arch_config;
>> On the basis of the above, please add uint8_t pad[3] (or perhaps
>> better pad[7] to be independent of the present
>> alignof(struct xen_arch_domainconfig) == 4) and make sure the
>> array will always be zero-filled, such that the padding space can
>> subsequently be assigned a purpose without needing to bump the
>> interface version(s) again.
> 
> ok, will do.
> 
> 
>>
>> Right now the sysctl caller of getdomaininfo() clears the full
>> structure (albeit only once, so stale / inapplicable data might get
>> reported for higher numbered domains if any field was filled only
>> in certain cases), while the domctl one doesn't. Maybe it would be
>> best looking forward if the domctl path also cleared the full buffer
>> up front, or if the clearing was moved into the function. (In the
>> latter case some writes of zeros into the struct could be eliminated
>> in return.)
> 
> Well, I would be OK either way, with a little preference for the latter.
> 
> Is it close to what you meant?

Yes, just that ...

> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -69,10 +69,10 @@ void getdomaininfo(struct domain *d, struct 
> xen_domctl_getdomaininfo *info)
>       int flags = XEN_DOMINF_blocked;
>       struct vcpu_runstate_info runstate;
> 
> +    memset(info, 0, sizeof(*info));
> +
>       info->domain = d->domain_id;
>       info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
> -    info->nr_online_vcpus = 0;
> -    info->ssidref = 0;

... there are a few more "... = 0" further down iirc.

>> Perhaps, once properly first zero-filling the struct in all cases,
>> the padding field near the start should also be made explicit,
>> clarifying visually that it is an option to use the space there for
>> something that makes sense to live early in the struct (i.e. I
>> wouldn't necessarily recommend putting there the new field you add,
>> albeit - as mentioned near the top - that's certainly an option).
> 
> I read this as a request to also add uint16_t pad after "domid_t domain" 
> at least. Correct?

I guess I should really leave this up to you - that's largely a cosmetic
thing after all once clearing the whole struct up front.

Jan

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Oleksandr]

On 12.10.21 14:49, Jan Beulich wrote:

Hi Jan

> On 12.10.2021 13:28, Oleksandr wrote:
>> On 12.10.21 12:24, Jan Beulich wrote:
>>> On 11.10.2021 19:48, Oleksandr Tyshchenko wrote:
>>>> @@ -150,6 +150,7 @@ struct xen_domctl_getdomaininfo {
>>>>        uint32_t ssidref;
>>>>        xen_domain_handle_t handle;
>>>>        uint32_t cpupool;
>>>> +    uint8_t gpaddr_bits; /* Guest physical address space size. */
>>>>        struct xen_arch_domainconfig arch_config;
>>> On the basis of the above, please add uint8_t pad[3] (or perhaps
>>> better pad[7] to be independent of the present
>>> alignof(struct xen_arch_domainconfig) == 4) and make sure the
>>> array will always be zero-filled, such that the padding space can
>>> subsequently be assigned a purpose without needing to bump the
>>> interface version(s) again.
>> ok, will do.
>>
>>
>>> Right now the sysctl caller of getdomaininfo() clears the full
>>> structure (albeit only once, so stale / inapplicable data might get
>>> reported for higher numbered domains if any field was filled only
>>> in certain cases), while the domctl one doesn't. Maybe it would be
>>> best looking forward if the domctl path also cleared the full buffer
>>> up front, or if the clearing was moved into the function. (In the
>>> latter case some writes of zeros into the struct could be eliminated
>>> in return.)
>> Well, I would be OK either way, with a little preference for the latter.
>>
>> Is it close to what you meant?
> Yes, just that ...
>
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -69,10 +69,10 @@ void getdomaininfo(struct domain *d, struct
>> xen_domctl_getdomaininfo *info)
>>        int flags = XEN_DOMINF_blocked;
>>        struct vcpu_runstate_info runstate;
>>
>> +    memset(info, 0, sizeof(*info));
>> +
>>        info->domain = d->domain_id;
>>        info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
>> -    info->nr_online_vcpus = 0;
>> -    info->ssidref = 0;
> ... there are a few more "... = 0" further down iirc.

I didn't spot anything except "info->flags = ..." which probably can now 
be converted into "info->flags |= ..."


>
>>> Perhaps, once properly first zero-filling the struct in all cases,
>>> the padding field near the start should also be made explicit,
>>> clarifying visually that it is an option to use the space there for
>>> something that makes sense to live early in the struct (i.e. I
>>> wouldn't necessarily recommend putting there the new field you add,
>>> albeit - as mentioned near the top - that's certainly an option).
>> I read this as a request to also add uint16_t pad after "domid_t domain"
>> at least. Correct?
> I guess I should really leave this up to you - that's largely a cosmetic
> thing after all once clearing the whole struct up front.

ok, thank you for the clarification.


>
> Jan
>
-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Jan Beulich]

On 12.10.2021 15:18, Oleksandr wrote:
> On 12.10.21 14:49, Jan Beulich wrote:
>> On 12.10.2021 13:28, Oleksandr wrote:
>>> On 12.10.21 12:24, Jan Beulich wrote:
>>>> On 11.10.2021 19:48, Oleksandr Tyshchenko wrote:
>>>>> @@ -150,6 +150,7 @@ struct xen_domctl_getdomaininfo {
>>>>>        uint32_t ssidref;
>>>>>        xen_domain_handle_t handle;
>>>>>        uint32_t cpupool;
>>>>> +    uint8_t gpaddr_bits; /* Guest physical address space size. */
>>>>>        struct xen_arch_domainconfig arch_config;
>>>> On the basis of the above, please add uint8_t pad[3] (or perhaps
>>>> better pad[7] to be independent of the present
>>>> alignof(struct xen_arch_domainconfig) == 4) and make sure the
>>>> array will always be zero-filled, such that the padding space can
>>>> subsequently be assigned a purpose without needing to bump the
>>>> interface version(s) again.
>>> ok, will do.
>>>
>>>
>>>> Right now the sysctl caller of getdomaininfo() clears the full
>>>> structure (albeit only once, so stale / inapplicable data might get
>>>> reported for higher numbered domains if any field was filled only
>>>> in certain cases), while the domctl one doesn't. Maybe it would be
>>>> best looking forward if the domctl path also cleared the full buffer
>>>> up front, or if the clearing was moved into the function. (In the
>>>> latter case some writes of zeros into the struct could be eliminated
>>>> in return.)
>>> Well, I would be OK either way, with a little preference for the latter.
>>>
>>> Is it close to what you meant?
>> Yes, just that ...
>>
>>> --- a/xen/common/domctl.c
>>> +++ b/xen/common/domctl.c
>>> @@ -69,10 +69,10 @@ void getdomaininfo(struct domain *d, struct
>>> xen_domctl_getdomaininfo *info)
>>>        int flags = XEN_DOMINF_blocked;
>>>        struct vcpu_runstate_info runstate;
>>>
>>> +    memset(info, 0, sizeof(*info));
>>> +
>>>        info->domain = d->domain_id;
>>>        info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
>>> -    info->nr_online_vcpus = 0;
>>> -    info->ssidref = 0;
>> ... there are a few more "... = 0" further down iirc.
> 
> I didn't spot anything except "info->flags = ..." which probably can now 
> be converted into "info->flags |= ..."

Oh, I guess you're right: I've been looking at my own tree, with
"paged_pages field is MEM_PAGING-only" and "shr_pages field is
MEM_SHARING-only" already applied. These sadly are still waiting
to go in, as they depend on earlier patches in their series.

Jan

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

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH V6 2/2] libxl/arm: Add handling of extended regions for DomU"):
> On Mon, 11 Oct 2021, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > 
> > 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
> >   currently.
> > - The ACPI case is not covered.
> > 
> > ***
> > 
> > The algorithm to choose extended regions for non-direct mapped
> > DomU is simpler in comparison with the algorithm for direct mapped
> > Dom0. We usually have a lot of unused space above 4GB, and might
> > have some unused space below 4GB (depends on guest memory size).
> > 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. The minimum size of extended region
> > the same as for Dom0 (64MB).
> > 
> > Suggested-by: Julien Grall <jgrall@amazon.com>
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

On the basis of that and the diffstat:

Acked-by: Ian Jackson <iwj@xenproject.org>

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Ian Jackson]

Oleksandr Tyshchenko writes ("[PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> We need to pass info about maximum supported guest physical
> address space size to the toolstack on Arm in order to properly
> calculate the base and size of the extended region (safe range)
> for the guest. The extended region is unused address space which
> could be safely used by domain for foreign/grant mappings on Arm.
> The extended region itself will be handled by the subsequent
> patch.

You say "to the toolstack", but you are exposing this information up
to callers of libxl.  Do you mean some higher-layer toolstack that
uses libxl ?  What does it use this information for ?

FTAOD I am not opposed to exposing this in this way; indeed it seems
likely to be useful.  I just want to fully understand before I give
this my tools ack.

> +        info->gpaddr_bits = domctl.u.getdomaininfo.gpaddr_bits;

I'm pleased to find that this is not arch-specific.

Thanks,
Ian.

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

[Oleksandr]

On 11.10.21 20:48, Oleksandr Tyshchenko wrote:

Hi Ian, Stefano

> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> 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
>    currently.
> - The ACPI case is not covered.
>
> ***
>
> The algorithm to choose extended regions for non-direct mapped
> DomU is simpler in comparison with the algorithm for direct mapped
> Dom0. We usually have a lot of unused space above 4GB, and might
> have some unused space below 4GB (depends on guest memory size).
> 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. The minimum size of extended region
> the same as for Dom0 (64MB).
>
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> Changes RFC -> V2:
>     - update patch description
>     - drop uneeded "extended-region" DT property
>     - clear reg array in finalise_ext_region() and add a TODO
>
> Changes V2 -> V3:
>     - update patch description, comments in code
>     - only pick up regions with size >= 64MB
>     - move the region calculation to make_hypervisor_node() and drop
>       finalise_ext_region()
>     - extend the list of arguments for make_hypervisor_node()
>     - do not show warning for 32-bit domain
>     - change the region alignment from 1GB to 2MB
>     - move EXT_REGION_SIZE to public/arch-arm.h
>
> Changes V3 -> V4:
>     - add R-b, A-b and T-b
>
> Changes V4 -> V5:
>     - update patch description and comments in code
>     - reflect changes done in previous patch to pass gpaddr_bits
>       via createdomain domctl (struct xen_arch_domainconfig)
>     - drop R-b, A-b and T-b
>     - drop limit for maximum extended region size (128GB)
>     - try to also allocate region below 4GB, optimize code
>       for calculating extended regions
>
> Change V5 -> V6:
>     - reflect changes done in previous patch to pass gpaddr_bits
>       via getdomaininfo domctl (struct xen_domctl_getdomaininfo)
>     - reduce the number of local variables, rework calculations
> ---
>   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;
> +    }
> +
> +    res = libxl_domain_info(CTX, &info, dom->guest_domid);
> +    if (res) return res;
> +
> +    assert(info.gpaddr_bits >= 32 && info.gpaddr_bits <= 48);
> +
> +    /*
> +     * 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.
> +     */
> +    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];
> +
> +out:
> +    /*
> +     * The region 0 for grant table space must be always present. If we managed
> +     * to allocate the extended regions then insert them as regions 1...N.
> +     */
> +    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> +
> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> +        if (region_size[i] < GUEST_EXT_REGION_MIN_SIZE)
> +            continue;
> +
> +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"\n",

If this appears to be the last version, may I please ask to remove 
unneeded "\n" in the message above while committing? Thank you.


> +            nr_regions, region_base[i], region_base[i] + region_size[i]);
> +
> +        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +                  region_base[i], region_size[i]);
> +        nr_regions++;
> +    }
> +
> +    if (!nr_regions)
> +        LOG(WARN, "The extended regions cannot be allocated, not enough space");
> +
> +    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> +        (nr_regions + 1);
> +    res = fdt_property(fdt, "reg", regs, len);
>       if (res) return res;
>   
>       /*
> @@ -963,7 +1029,7 @@ next_resize:
>           }
>   
>           FDT( make_timer_node(gc, fdt, ainfo, state->clock_frequency) );
> -        FDT( make_hypervisor_node(gc, fdt, vers) );
> +        FDT( make_hypervisor_node(gc, fdt, vers, info, dom) );
>   
>           if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART)
>               FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index d46c61f..19ca2b0 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -451,6 +451,8 @@ typedef uint64_t xen_callback_t;
>   #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
>   #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
>   
> +#define GUEST_EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
> +
>   /* Current supported guest VCPUs */
>   #define GUEST_MAX_VCPUS 128
>   

-- 
Regards,

Oleksandr Tyshchenko

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

[Ian Jackson]

Oleksandr writes ("Re: [PATCH V6 2/2] libxl/arm: Add handling of extended regions for DomU"):
> On 11.10.21 20:48, Oleksandr Tyshchenko wrote:
> Hi Ian, Stefano
> > +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"\n",
> 
> If this appears to be the last version, may I please ask to remove 
> unneeded "\n" in the message above while committing? Thank you.

I think Stefano will be committing this but: personally I don't like
the "edit on commit" workflow.  Committers are already a bottleneck
and it is easy to make uncontrolled mistakes.  I find the most
convenient workflow to be to acquire a git branch from somewhere and
commit that, so if you provide a git branch with the acks folded in
and this kind of minor fix included, that would be ideal.  Ie,
git-request-merge format or something like it.

Thanks,
Ian.

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

[Oleksandr]

On 12.10.21 18:32, Ian Jackson wrote:

Hi Ian

> Oleksandr writes ("Re: [PATCH V6 2/2] libxl/arm: Add handling of extended regions for DomU"):
>> On 11.10.21 20:48, Oleksandr Tyshchenko wrote:
>> Hi Ian, Stefano
>>> +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"\n",
>> If this appears to be the last version, may I please ask to remove
>> unneeded "\n" in the message above while committing? Thank you.
> I think Stefano will be committing this but: personally I don't like
> the "edit on commit" workflow.  Committers are already a bottleneck
> and it is easy to make uncontrolled mistakes.  I find the most
> convenient workflow to be to acquire a git branch from somewhere and
> commit that, so if you provide a git branch with the acks folded in
> and this kind of minor fix included, that would be ideal.  Ie,
> git-request-merge format or something like it.

Yes, I will provide a git branch later on today.


>
> Thanks,
> Ian.

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Oleksandr]

On 12.10.21 18:15, Ian Jackson wrote:

Hi Ian

> Oleksandr Tyshchenko writes ("[PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> We need to pass info about maximum supported guest physical
>> address space size to the toolstack on Arm in order to properly
>> calculate the base and size of the extended region (safe range)
>> for the guest. The extended region is unused address space which
>> could be safely used by domain for foreign/grant mappings on Arm.
>> The extended region itself will be handled by the subsequent
>> patch.
> You say "to the toolstack", but you are exposing this information up
> to callers of libxl.  Do you mean some higher-layer toolstack that
> uses libxl ?  What does it use this information for ?
>
> FTAOD I am not opposed to exposing this in this way; indeed it seems
> likely to be useful.  I just want to fully understand before I give
> this my tools ack.
I didn't mean any higher-layer toolstack, sorry if I was unclear. In the 
first place this information is
needed by the entity which generates the device-tree for the guest on 
Arm (tools/libs/light/libxl_arm.c) to properly calculate the extended 
regions to be inserted into the hypervisor node.


>
>> +        info->gpaddr_bits = domctl.u.getdomaininfo.gpaddr_bits;
> I'm pleased to find that this is not arch-specific.
>
> Thanks,
> Ian.

-- 
Regards,

Oleksandr Tyshchenko

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

[Julien Grall]

Hi Oleksandr,

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?

> +
> +    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?

> +
> +    /*
> +     * 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.

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).

> +    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.

Cheers,

-- 
Julien Grall

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Ian Jackson]

Oleksandr writes ("Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
> > Oleksandr Tyshchenko writes ("[PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
> > You say "to the toolstack", but you are exposing this information up
> > to callers of libxl.  Do you mean some higher-layer toolstack that
> > uses libxl ?  What does it use this information for ?
> >
> > FTAOD I am not opposed to exposing this in this way; indeed it seems
> > likely to be useful.  I just want to fully understand before I give
> > this my tools ack.
> 
> I didn't mean any higher-layer toolstack, sorry if I was unclear. In the 
> first place this information is
> needed by the entity which generates the device-tree for the guest on 
> Arm (tools/libs/light/libxl_arm.c) to properly calculate the extended 
> regions to be inserted into the hypervisor node.

Right, OK.  So I think this is being exposed at the libxl
gratuitously, because someone might want it in the future.
I approve :-).

Reviewed-by: Ian Jackson <iwj@xenproject.org>

Thanks,
Ian.

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

[Oleksandr]

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@gmail.com/


>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Oleksandr]

On 12.10.21 19:18, Ian Jackson wrote:

Hi Ian

> Oleksandr writes ("Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
>>> Oleksandr Tyshchenko writes ("[PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
>>> You say "to the toolstack", but you are exposing this information up
>>> to callers of libxl.  Do you mean some higher-layer toolstack that
>>> uses libxl ?  What does it use this information for ?
>>>
>>> FTAOD I am not opposed to exposing this in this way; indeed it seems
>>> likely to be useful.  I just want to fully understand before I give
>>> this my tools ack.
>> I didn't mean any higher-layer toolstack, sorry if I was unclear. In the
>> first place this information is
>> needed by the entity which generates the device-tree for the guest on
>> Arm (tools/libs/light/libxl_arm.c) to properly calculate the extended
>> regions to be inserted into the hypervisor node.
> Right, OK.  So I think this is being exposed at the libxl
> gratuitously, because someone might want it in the future.
> I approve :-).
>
> Reviewed-by: Ian Jackson <iwj@xenproject.org>

Thanks!


Please note, it is going to be a new version of this patch based on 
today's discussion with Jan.


>
> Thanks,
> Ian.

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Ian Jackson]

Oleksandr writes ("Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
> Please note, it is going to be a new version of this patch based on 
> today's discussion with Jan.

OK.  Please retain my R-b if you don't change any of the tools parts.

Ian.

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Oleksandr]

On 12.10.21 21:07, Ian Jackson wrote:

Hi Ian

> Oleksandr writes ("Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
>> Please note, it is going to be a new version of this patch based on
>> today's discussion with Jan.
> OK.  Please retain my R-b if you don't change any of the tools parts.

Sure, thanks for confirming. There won't be no new changes for the tools.


>
> Ian.

-- 
Regards,

Oleksandr Tyshchenko

[PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

We need to pass info about maximum supported guest physical
address space size to the toolstack on Arm in order to properly
calculate the base and size of the extended region (safe range)
for the guest. The extended region is unused address space which
could be safely used by domain for foreign/grant mappings on Arm.
The extended region itself will be handled by the subsequent
patch.

Currently the same guest physical address space size is used
for all guests (p2m_ipa_bits variable on Arm, the x86 equivalent
is hap_paddr_bits).

Add an explicit padding after "gpaddr_bits" field and also
(while at it) after "domain" field.

Also make sure that full structure is cleared in all cases by
moving the clearing into getdomaininfo(). Currently it is only
cleared by the sysctl caller (and only once).

Please note, we do not need to bump XEN_DOMCTL_INTERFACE_VERSION
as a bump has already occurred in this release cycle. But we do
need to bump XEN_SYSCTL_INTERFACE_VERSION as the structure is
re-used in a sysctl.

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Reviewed-by: Ian Jackson <iwj@xenproject.org>
---
Changes RFC -> V2:
   - update patch subject/description
   - replace arch-specific sub-struct with common gpaddr_bits
     field and update code to reflect that

Changes V2 -> V3:
   - make the field uint8_t and add uint8_t pad[7] after
   - remove leading blanks in libxl.h

Changes V3 -> V4:
   - also print gpaddr_bits from output_physinfo()
   - add Michal's R-b

Changes V4 -> V5:
   - update patch subject and description
   - drop Michal's R-b
   - pass gpaddr_bits via createdomain domctl
     (struct xen_arch_domainconfig)

Changes V5 -> V6:
   - update patch subject and description
   - pass gpaddr_bits via getdomaininfo domctl
     (struct xen_domctl_getdomaininfo)

Changes V6 -> V7:
   - update patch description
   - do not bump XEN_DOMCTL_INTERFACE_VERSION
   - bump XEN_SYSCTL_INTERFACE_VERSION
   - add explicit paddings
   - clear the full structure in getdomaininfo()
   - add Ian's R-b
---
 tools/include/libxl.h            | 8 ++++++++
 tools/include/xenctrl.h          | 1 +
 tools/libs/ctrl/xc_domain.c      | 1 +
 tools/libs/light/libxl_domain.c  | 1 +
 tools/libs/light/libxl_types.idl | 1 +
 xen/arch/arm/domctl.c            | 2 ++
 xen/arch/x86/domctl.c            | 1 +
 xen/common/domctl.c              | 6 +++---
 xen/common/sysctl.c              | 2 +-
 xen/include/public/domctl.h      | 3 +++
 xen/include/public/sysctl.h      | 2 +-
 11 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d..deb5022 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -874,6 +874,14 @@ typedef struct libxl__ctx libxl_ctx;
 #define LIBXL_HAVE_DOMINFO_NEVER_STOP 1
 
 /*
+ * LIBXL_HAVE_DOMINFO_GPADDR_BITS
+ *
+ * If this is defined, libxl_dominfo will contain an uint8 field called
+ * gpaddr_bits, containing the guest physical address space size.
+ */
+#define LIBXL_HAVE_DOMINFO_GPADDR_BITS 1
+
+/*
  * LIBXL_HAVE_QXL
  *
  * If defined, then the libxl_vga_interface_type will contain another value:
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index a306399..07b96e6 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -462,6 +462,7 @@ typedef struct xc_dominfo {
     unsigned int  max_vcpu_id;
     xen_domain_handle_t handle;
     unsigned int  cpupool;
+    uint8_t       gpaddr_bits;
     struct xen_arch_domainconfig arch_config;
 } xc_dominfo_t;
 
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index 23322b7..b155d6a 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -396,6 +396,7 @@ int xc_domain_getinfo(xc_interface *xch,
         info->nr_online_vcpus = domctl.u.getdomaininfo.nr_online_vcpus;
         info->max_vcpu_id = domctl.u.getdomaininfo.max_vcpu_id;
         info->cpupool = domctl.u.getdomaininfo.cpupool;
+        info->gpaddr_bits = domctl.u.getdomaininfo.gpaddr_bits;
         info->arch_config = domctl.u.getdomaininfo.arch_config;
 
         memcpy(info->handle, domctl.u.getdomaininfo.handle,
diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c
index 51a6127..544a9bf 100644
--- a/tools/libs/light/libxl_domain.c
+++ b/tools/libs/light/libxl_domain.c
@@ -306,6 +306,7 @@ void libxl__xcinfo2xlinfo(libxl_ctx *ctx,
     xlinfo->vcpu_max_id = xcinfo->max_vcpu_id;
     xlinfo->vcpu_online = xcinfo->nr_online_vcpus;
     xlinfo->cpupool = xcinfo->cpupool;
+    xlinfo->gpaddr_bits = xcinfo->gpaddr_bits;
     xlinfo->domain_type = (xcinfo->flags & XEN_DOMINF_hvm_guest) ?
         LIBXL_DOMAIN_TYPE_HVM : LIBXL_DOMAIN_TYPE_PV;
 }
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 3f9fff6..2df7258 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -357,6 +357,7 @@ libxl_dominfo = Struct("dominfo",[
     ("vcpu_max_id", uint32),
     ("vcpu_online", uint32),
     ("cpupool",     uint32),
+    ("gpaddr_bits", uint8),
     ("domain_type", libxl_domain_type),
     ], dir=DIR_OUT)
 
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index b7d27f3..6245af6 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -20,6 +20,8 @@ void arch_get_domain_info(const struct domain *d,
 {
     /* All ARM domains use hardware assisted paging. */
     info->flags |= XEN_DOMINF_hap;
+
+    info->gpaddr_bits = p2m_ipa_bits;
 }
 
 static int handle_vuart_init(struct domain *d, 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 26a76d2..7d102e0 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -151,6 +151,7 @@ void arch_get_domain_info(const struct domain *d,
         info->flags |= XEN_DOMINF_hap;
 
     info->arch_config.emulation_flags = d->arch.emulation_flags;
+    info->gpaddr_bits = hap_paddr_bits;
 }
 
 static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op,
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 12d6144..2d07a12 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -69,10 +69,10 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
     int flags = XEN_DOMINF_blocked;
     struct vcpu_runstate_info runstate;
 
+    memset(info, 0, sizeof(*info));
+
     info->domain = d->domain_id;
     info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
-    info->nr_online_vcpus = 0;
-    info->ssidref = 0;
 
     /*
      * - domain is marked as blocked only if all its vcpus are blocked
@@ -95,7 +95,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
 
     info->cpu_time = cpu_time;
 
-    info->flags = (info->nr_online_vcpus ? flags : 0) |
+    info->flags |= (info->nr_online_vcpus ? flags : 0) |
         ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying     : 0) |
         (d->is_shut_down                ? XEN_DOMINF_shutdown  : 0) |
         (d->controller_pause_count > 0  ? XEN_DOMINF_paused    : 0) |
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 3558641..a7ab95d 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -76,7 +76,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
     case XEN_SYSCTL_getdomaininfolist:
     { 
         struct domain *d;
-        struct xen_domctl_getdomaininfo info = { 0 };
+        struct xen_domctl_getdomaininfo info;
         u32 num_domains = 0;
 
         rcu_read_lock(&domlist_read_lock);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 4cb3f66..46acc8f 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -106,6 +106,7 @@ struct xen_domctl_createdomain {
 struct xen_domctl_getdomaininfo {
     /* OUT variables. */
     domid_t  domain;              /* Also echoed in domctl.domain */
+    uint16_t pad1;
  /* Domain is scheduled to die. */
 #define _XEN_DOMINF_dying     0
 #define XEN_DOMINF_dying      (1U<<_XEN_DOMINF_dying)
@@ -150,6 +151,8 @@ struct xen_domctl_getdomaininfo {
     uint32_t ssidref;
     xen_domain_handle_t handle;
     uint32_t cpupool;
+    uint8_t gpaddr_bits; /* Guest physical address space size. */
+    uint8_t pad2[7];
     struct xen_arch_domainconfig arch_config;
 };
 typedef struct xen_domctl_getdomaininfo xen_domctl_getdomaininfo_t;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 039ccf8..41ef7a2 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -35,7 +35,7 @@
 #include "domctl.h"
 #include "physdev.h"
 
-#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013
+#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014
 
 /*
  * Read console content from Xen buffer ring.
-- 
2.7.4

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

[Julien Grall]

Hi Oleksandr,

On 12/10/2021 18:42, Oleksandr wrote:
> On 12.10.21 19:05, Julien Grall wrote:
>> 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.

So I think the assert() is not suitable here because even if the 
gpaddr_bits is provided by the hypervisor (and therefore should be 
trusted), this is a different component so hardening the code is a good 
practice.

In this case, I would check that info.gpaddr_bits <= 64 and return an 
error. The reason I am suggesting <= 64 and not 48 is because Arm 
already supports 52 bits address space. Yet, I still like to avoid this 
assumption in the code. Something like below should work:

bank1end = GUEST_RAM1_BASE + GUEST_RAM1_SIZE - 1;
bank1end = min(bank1end, ~(0ULL) >> (64 - info.gpaddr_bits);

>>> +
>>> +    /*
>>> +     * 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?

You read correctly my point. However, I disagree that it is a problem to 
remove the placeholder if we fail to allocate the amount of regions 
expected.

Looking at libfdt, I can see two ways to deal with it:
   1) Use fdt_setprop()
   2) Delete the property using fdt_delprop() and then recreate it with 
fdt_appendprop()

The first solution is ideal and I think can work here to downsize the 
property. At worse, the second solution should work as the FDT blob will 
not increase.

>> 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.

So this patch is relying on the fact that the regions reserved for the 
RAM are big enough to also accommodate the extended regions.

I am happy with this approach. However, I would like the approach to be 
documented in arch-arm.h because this is the first place one would look 
to understand the memory layout. This will be helpful if/when we need to 
modify the guest memory layout.

> 
> 
>>
>>
>>> +    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));

I am OK with that. But I wonder if we could simply use min(..., ) to 
avoid the BUILD_BUG_ON().

Cheers,

-- 
Julien Grall

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

[Oleksandr]

On 13.10.21 00:22, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien, Ian.


Julien, thank you for the detailed answer, I will analyze it tomorrow.

Ian, I think, there is no reason in providing git branch with the acks 
folded in + my minor fix for the debug message as it was discussed 
before, it sounds like there is more work to do, so it is going to be a 
new version anyway.




>
> On 12/10/2021 18:42, Oleksandr wrote:
>> On 12.10.21 19:05, Julien Grall wrote:
>>> 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.
>
> So I think the assert() is not suitable here because even if the 
> gpaddr_bits is provided by the hypervisor (and therefore should be 
> trusted), this is a different component so hardening the code is a 
> good practice.
>
> In this case, I would check that info.gpaddr_bits <= 64 and return an 
> error. The reason I am suggesting <= 64 and not 48 is because Arm 
> already supports 52 bits address space. Yet, I still like to avoid 
> this assumption in the code. Something like below should work:
>
> bank1end = GUEST_RAM1_BASE + GUEST_RAM1_SIZE - 1;
> bank1end = min(bank1end, ~(0ULL) >> (64 - info.gpaddr_bits);
>
>>>> +
>>>> +    /*
>>>> +     * 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?
>
> You read correctly my point. However, I disagree that it is a problem 
> to remove the placeholder if we fail to allocate the amount of regions 
> expected.
>
> Looking at libfdt, I can see two ways to deal with it:
>   1) Use fdt_setprop()
>   2) Delete the property using fdt_delprop() and then recreate it with 
> fdt_appendprop()
>
> The first solution is ideal and I think can work here to downsize the 
> property. At worse, the second solution should work as the FDT blob 
> will not increase.
>
>>> 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.
>
> So this patch is relying on the fact that the regions reserved for the 
> RAM are big enough to also accommodate the extended regions.
>
> I am happy with this approach. However, I would like the approach to 
> be documented in arch-arm.h because this is the first place one would 
> look to understand the memory layout. This will be helpful if/when we 
> need to modify the guest memory layout.
>
>>
>>
>>>
>>>
>>>> +    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));
>
> I am OK with that. But I wonder if we could simply use min(..., ) to 
> avoid the BUILD_BUG_ON().
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko

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

[Oleksandr]

Hi Julien


On 13.10.21 00:43, Oleksandr wrote:
>
> On 13.10.21 00:22, Julien Grall wrote:
>> Hi Oleksandr,
>
> Hi Julien, Ian.
>
>
> Julien, thank you for the detailed answer, I will analyze it tomorrow.

Already analyzed, please see comments below.


>
> Ian, I think, there is no reason in providing git branch with the acks 
> folded in + my minor fix for the debug message as it was discussed 
> before, it sounds like there is more work to do, so it is going to be 
> a new version anyway.
>
>
>
>
>>
>> On 12/10/2021 18:42, Oleksandr wrote:
>>> On 12.10.21 19:05, Julien Grall wrote:
>>>> 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.
>>
>> So I think the assert() is not suitable here because even if the 
>> gpaddr_bits is provided by the hypervisor (and therefore should be 
>> trusted), this is a different component so hardening the code is a 
>> good practice.
>>
>> In this case, I would check that info.gpaddr_bits <= 64 and return an 
>> error. The reason I am suggesting <= 64 and not 48 is because Arm 
>> already supports 52 bits address space. Yet, I still like to avoid 
>> this assumption in the code. Something like below should work:
>>
>> bank1end = GUEST_RAM1_BASE + GUEST_RAM1_SIZE - 1;
>> bank1end = min(bank1end, ~(0ULL) >> (64 - info.gpaddr_bits);

ok, makes sense.


>>
>>
>>>>> +
>>>>> +    /*
>>>>> +     * 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?
>>
>> You read correctly my point. However, I disagree that it is a problem 
>> to remove the placeholder if we fail to allocate the amount of 
>> regions expected.
>>
>> Looking at libfdt, I can see two ways to deal with it:
>>   1) Use fdt_setprop()
>>   2) Delete the property using fdt_delprop() and then recreate it 
>> with fdt_appendprop()
>>
>> The first solution is ideal and I think can work here to downsize the 
>> property. At worse, the second solution should work as the FDT blob 
>> will not increase.

Indeed, fdt_setprop() will work in our case, thank you. When I looked at 
it, I only experimented with two high-level functions: 
fdt_setprop_inplace() and fdt_nop_node() used in libxl_arm.c and no one 
wasn't suitable.


>>
>>
>>>> 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.
>>
>> So this patch is relying on the fact that the regions reserved for 
>> the RAM are big enough to also accommodate the extended regions.
>>
>> I am happy with this approach. However, I would like the approach to 
>> be documented in arch-arm.h because this is the first place one would 
>> look to understand the memory layout. This will be helpful if/when we 
>> need to modify the guest memory layout.

Thank you for the clarification, it is clear now.


>>
>>
>>>
>>>
>>>>
>>>>
>>>>> +    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));
>>
>> I am OK with that. But I wonder if we could simply use min(..., ) to 
>> avoid the BUILD_BUG_ON().

I think, we could. And probably we could avoid relying on the fact that 
Bank 0 is always below 4GB.

Below, the changes based on my understanding of the whole request. Would 
you be OK with them?


diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6..53ae0f3 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc *gc, void 
*fdt,
                                "xen,xen");
      if (res) return res;

-    /* reg 0 is grant table space */
+    /*
+     * reg 0 is a placeholder for grant table space, reg 1...N are
+     * the placeholders for extended regions.
+     */
      res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, 
GUEST_ROOT_SIZE_CELLS,
-                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
+                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0, 0);
      if (res) return res;

      /*
@@ -1069,6 +1072,74 @@ static void finalise_one_node(libxl__gc *gc, void 
*fdt, const char *uname,
      }
  }

+#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
+
+static void finalise_ext_region(libxl__gc *gc, struct xc_dom_image *dom)
+{
+    void *fdt = dom->devicetree_blob;
+    uint64_t region_size[GUEST_RAM_BANKS] = {0}, 
region_base[GUEST_RAM_BANKS],
+        bankend[GUEST_RAM_BANKS];
+    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
+                  (GUEST_RAM_BANKS + 1)];
+    be32 *cells = &regs[0];
+    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
+    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
+    unsigned int i, len, nr_regions = 0;
+    libxl_dominfo info;
+    int offset, rc;
+
+    offset = fdt_path_offset(fdt, "/hypervisor");
+    assert(offset > 0);
+
+    rc = libxl_domain_info(CTX, &info, dom->guest_domid);
+    assert(!rc);
+
+    assert(info.gpaddr_bits <= 64);
+
+    /*
+     * Try to allocate separate 2MB-aligned extended regions from the first
+     * and second RAM banks taking into the account the maximum supported
+     * guest physical address space size and the amount of memory assigned
+     * to the guest.
+     */
+    for (i = 0; i < GUEST_RAM_BANKS; i++) {
+        region_base[i] = bankbase[i] +
+            ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << 
XC_PAGE_SHIFT);
+
+        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
+        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
+        if (bankend[i] > region_base[i])
+            region_size[i] = bankend[i] - region_base[i] + 1;
+    }
+
+    /*
+     * The region 0 for grant table space must be always present. If we 
managed
+     * to allocate the extended regions then insert them as regions 1...N.
+     */
+    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
+
+    for (i = 0; i < GUEST_RAM_BANKS; i++) {
+        if (region_size[i] < GUEST_EXT_REGION_MIN_SIZE)
+            continue;
+
+        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
+            nr_regions, region_base[i], region_base[i] + region_size[i]);
+
+        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+                  region_base[i], region_size[i]);
+        nr_regions++;
+    }
+
+    if (!nr_regions)
+        LOG(WARN, "The extended regions cannot be allocated, not enough 
space");
+
+    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + 
GUEST_ROOT_SIZE_CELLS) *
+        (nr_regions + 1);
+    rc = fdt_setprop(fdt, offset, "reg", regs, len);
+    assert(!rc);
+}
+
  int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
                                                 uint32_t domid,
libxl_domain_config *d_config,
@@ -1109,6 +1180,8 @@ int 
libxl__arch_domain_finalise_hw_description(libxl__gc *gc,

      }

+    finalise_ext_region(gc, dom);
+
      for (i = 0; i < GUEST_RAM_BANKS; i++) {
          const uint64_t size = (uint64_t)dom->rambank_size[i] << 
XC_PAGE_SHIFT;

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index d46c61f..7425a78 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -438,6 +438,11 @@ typedef uint64_t xen_callback_t;

  #define GUEST_RAM_BANKS   2

+/*
+ * The way to find the extended regions (to be exposed to the guest as 
unused
+ * address space) relies on the fact that the regions reserved for the RAM
+ * below are big enough to also accommodate such regions.
+ */
  #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM 
@ 1GB */
  #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)

@@ -451,6 +456,8 @@ typedef uint64_t xen_callback_t;
  #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
  #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }

+#define GUEST_EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
+
  /* Current supported guest VCPUs */
  #define GUEST_MAX_VCPUS 128

(END)





-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Oleksandr]

Hi Jan.


May I please ask, are you OK with the proposed changes?


On 13.10.21 00:22, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> We need to pass info about maximum supported guest physical
> address space size to the toolstack on Arm in order to properly
> calculate the base and size of the extended region (safe range)
> for the guest. The extended region is unused address space which
> could be safely used by domain for foreign/grant mappings on Arm.
> The extended region itself will be handled by the subsequent
> patch.
>
> Currently the same guest physical address space size is used
> for all guests (p2m_ipa_bits variable on Arm, the x86 equivalent
> is hap_paddr_bits).
>
> Add an explicit padding after "gpaddr_bits" field and also
> (while at it) after "domain" field.
>
> Also make sure that full structure is cleared in all cases by
> moving the clearing into getdomaininfo(). Currently it is only
> cleared by the sysctl caller (and only once).
>
> Please note, we do not need to bump XEN_DOMCTL_INTERFACE_VERSION
> as a bump has already occurred in this release cycle. But we do
> need to bump XEN_SYSCTL_INTERFACE_VERSION as the structure is
> re-used in a sysctl.
>
> Suggested-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Reviewed-by: Ian Jackson <iwj@xenproject.org>
> ---
> Changes RFC -> V2:
>     - update patch subject/description
>     - replace arch-specific sub-struct with common gpaddr_bits
>       field and update code to reflect that
>
> Changes V2 -> V3:
>     - make the field uint8_t and add uint8_t pad[7] after
>     - remove leading blanks in libxl.h
>
> Changes V3 -> V4:
>     - also print gpaddr_bits from output_physinfo()
>     - add Michal's R-b
>
> Changes V4 -> V5:
>     - update patch subject and description
>     - drop Michal's R-b
>     - pass gpaddr_bits via createdomain domctl
>       (struct xen_arch_domainconfig)
>
> Changes V5 -> V6:
>     - update patch subject and description
>     - pass gpaddr_bits via getdomaininfo domctl
>       (struct xen_domctl_getdomaininfo)
>
> Changes V6 -> V7:
>     - update patch description
>     - do not bump XEN_DOMCTL_INTERFACE_VERSION
>     - bump XEN_SYSCTL_INTERFACE_VERSION
>     - add explicit paddings
>     - clear the full structure in getdomaininfo()
>     - add Ian's R-b
> ---
>   tools/include/libxl.h            | 8 ++++++++
>   tools/include/xenctrl.h          | 1 +
>   tools/libs/ctrl/xc_domain.c      | 1 +
>   tools/libs/light/libxl_domain.c  | 1 +
>   tools/libs/light/libxl_types.idl | 1 +
>   xen/arch/arm/domctl.c            | 2 ++
>   xen/arch/x86/domctl.c            | 1 +
>   xen/common/domctl.c              | 6 +++---
>   xen/common/sysctl.c              | 2 +-
>   xen/include/public/domctl.h      | 3 +++
>   xen/include/public/sysctl.h      | 2 +-
>   11 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index b9ba16d..deb5022 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -874,6 +874,14 @@ typedef struct libxl__ctx libxl_ctx;
>   #define LIBXL_HAVE_DOMINFO_NEVER_STOP 1
>   
>   /*
> + * LIBXL_HAVE_DOMINFO_GPADDR_BITS
> + *
> + * If this is defined, libxl_dominfo will contain an uint8 field called
> + * gpaddr_bits, containing the guest physical address space size.
> + */
> +#define LIBXL_HAVE_DOMINFO_GPADDR_BITS 1
> +
> +/*
>    * LIBXL_HAVE_QXL
>    *
>    * If defined, then the libxl_vga_interface_type will contain another value:
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index a306399..07b96e6 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -462,6 +462,7 @@ typedef struct xc_dominfo {
>       unsigned int  max_vcpu_id;
>       xen_domain_handle_t handle;
>       unsigned int  cpupool;
> +    uint8_t       gpaddr_bits;
>       struct xen_arch_domainconfig arch_config;
>   } xc_dominfo_t;
>   
> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> index 23322b7..b155d6a 100644
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -396,6 +396,7 @@ int xc_domain_getinfo(xc_interface *xch,
>           info->nr_online_vcpus = domctl.u.getdomaininfo.nr_online_vcpus;
>           info->max_vcpu_id = domctl.u.getdomaininfo.max_vcpu_id;
>           info->cpupool = domctl.u.getdomaininfo.cpupool;
> +        info->gpaddr_bits = domctl.u.getdomaininfo.gpaddr_bits;
>           info->arch_config = domctl.u.getdomaininfo.arch_config;
>   
>           memcpy(info->handle, domctl.u.getdomaininfo.handle,
> diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c
> index 51a6127..544a9bf 100644
> --- a/tools/libs/light/libxl_domain.c
> +++ b/tools/libs/light/libxl_domain.c
> @@ -306,6 +306,7 @@ void libxl__xcinfo2xlinfo(libxl_ctx *ctx,
>       xlinfo->vcpu_max_id = xcinfo->max_vcpu_id;
>       xlinfo->vcpu_online = xcinfo->nr_online_vcpus;
>       xlinfo->cpupool = xcinfo->cpupool;
> +    xlinfo->gpaddr_bits = xcinfo->gpaddr_bits;
>       xlinfo->domain_type = (xcinfo->flags & XEN_DOMINF_hvm_guest) ?
>           LIBXL_DOMAIN_TYPE_HVM : LIBXL_DOMAIN_TYPE_PV;
>   }
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 3f9fff6..2df7258 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -357,6 +357,7 @@ libxl_dominfo = Struct("dominfo",[
>       ("vcpu_max_id", uint32),
>       ("vcpu_online", uint32),
>       ("cpupool",     uint32),
> +    ("gpaddr_bits", uint8),
>       ("domain_type", libxl_domain_type),
>       ], dir=DIR_OUT)
>   
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index b7d27f3..6245af6 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -20,6 +20,8 @@ void arch_get_domain_info(const struct domain *d,
>   {
>       /* All ARM domains use hardware assisted paging. */
>       info->flags |= XEN_DOMINF_hap;
> +
> +    info->gpaddr_bits = p2m_ipa_bits;
>   }
>   
>   static int handle_vuart_init(struct domain *d,
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 26a76d2..7d102e0 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -151,6 +151,7 @@ void arch_get_domain_info(const struct domain *d,
>           info->flags |= XEN_DOMINF_hap;
>   
>       info->arch_config.emulation_flags = d->arch.emulation_flags;
> +    info->gpaddr_bits = hap_paddr_bits;
>   }
>   
>   static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op,
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 12d6144..2d07a12 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -69,10 +69,10 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>       int flags = XEN_DOMINF_blocked;
>       struct vcpu_runstate_info runstate;
>   
> +    memset(info, 0, sizeof(*info));
> +
>       info->domain = d->domain_id;
>       info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
> -    info->nr_online_vcpus = 0;
> -    info->ssidref = 0;
>   
>       /*
>        * - domain is marked as blocked only if all its vcpus are blocked
> @@ -95,7 +95,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>   
>       info->cpu_time = cpu_time;
>   
> -    info->flags = (info->nr_online_vcpus ? flags : 0) |
> +    info->flags |= (info->nr_online_vcpus ? flags : 0) |
>           ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying     : 0) |
>           (d->is_shut_down                ? XEN_DOMINF_shutdown  : 0) |
>           (d->controller_pause_count > 0  ? XEN_DOMINF_paused    : 0) |
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 3558641..a7ab95d 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -76,7 +76,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>       case XEN_SYSCTL_getdomaininfolist:
>       {
>           struct domain *d;
> -        struct xen_domctl_getdomaininfo info = { 0 };
> +        struct xen_domctl_getdomaininfo info;
>           u32 num_domains = 0;
>   
>           rcu_read_lock(&domlist_read_lock);
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 4cb3f66..46acc8f 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -106,6 +106,7 @@ struct xen_domctl_createdomain {
>   struct xen_domctl_getdomaininfo {
>       /* OUT variables. */
>       domid_t  domain;              /* Also echoed in domctl.domain */
> +    uint16_t pad1;
>    /* Domain is scheduled to die. */
>   #define _XEN_DOMINF_dying     0
>   #define XEN_DOMINF_dying      (1U<<_XEN_DOMINF_dying)
> @@ -150,6 +151,8 @@ struct xen_domctl_getdomaininfo {
>       uint32_t ssidref;
>       xen_domain_handle_t handle;
>       uint32_t cpupool;
> +    uint8_t gpaddr_bits; /* Guest physical address space size. */
> +    uint8_t pad2[7];
>       struct xen_arch_domainconfig arch_config;
>   };
>   typedef struct xen_domctl_getdomaininfo xen_domctl_getdomaininfo_t;
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 039ccf8..41ef7a2 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -35,7 +35,7 @@
>   #include "domctl.h"
>   #include "physdev.h"
>   
> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013
> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014
>   
>   /*
>    * Read console content from Xen buffer ring.

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Jan Beulich]

On 12.10.2021 23:22, Oleksandr Tyshchenko wrote:
> @@ -95,7 +95,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>  
>      info->cpu_time = cpu_time;
>  
> -    info->flags = (info->nr_online_vcpus ? flags : 0) |
> +    info->flags |= (info->nr_online_vcpus ? flags : 0) |
>          ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying     : 0) |
>          (d->is_shut_down                ? XEN_DOMINF_shutdown  : 0) |
>          (d->controller_pause_count > 0  ? XEN_DOMINF_paused    : 0) |

I don't think this is a useful change - you move from a simple write
to a read-modify-write operation. With this dropped again, hypervisor
parts:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

While this has meanwhile moved quite far from the original proposal,
I still wonder in how far Andrew may have remaining concerns. Did
you check with him, perhaps on irc? But of course catching his
attention may be difficult, so no (further) feedback by him should
probably not keep this from getting committed (if no other open
issues remain).

Jan

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Oleksandr]

On 13.10.21 16:56, Jan Beulich wrote:

Hi Jan

> On 12.10.2021 23:22, Oleksandr Tyshchenko wrote:
>> @@ -95,7 +95,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>>   
>>       info->cpu_time = cpu_time;
>>   
>> -    info->flags = (info->nr_online_vcpus ? flags : 0) |
>> +    info->flags |= (info->nr_online_vcpus ? flags : 0) |
>>           ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying     : 0) |
>>           (d->is_shut_down                ? XEN_DOMINF_shutdown  : 0) |
>>           (d->controller_pause_count > 0  ? XEN_DOMINF_paused    : 0) |
> I don't think this is a useful change - you move from a simple write
> to a read-modify-write operation. With this dropped again, hypervisor
> parts:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks!

Will drop.


>
> While this has meanwhile moved quite far from the original proposal,
> I still wonder in how far Andrew may have remaining concerns. Did
> you check with him, perhaps on irc?
> But of course catching his
> attention may be difficult, so no (further) feedback by him should
> probably not keep this from getting committed (if no other open
> issues remain).

If I remember correctly, I made several attempts to clarify in the 
initial thread. But, I have to admit, not via IRC (I am expecting some 
troubles with my IRC client last time and I haven't figured out yet 
why). Of course, it would be correct to get a feedback to make sure that 
there would be no remaining concerns (as the main concern to use domctl 
was addressed) or otherwise.


>
> Jan
>
-- 
Regards,

Oleksandr Tyshchenko

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

[Julien Grall]

On 13/10/2021 14:46, Oleksandr wrote:
> 
> Hi Julien

Hi Oleksandr,

> On 13.10.21 00:43, Oleksandr wrote:
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index e3140a6..53ae0f3 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc *gc, void 
> *fdt,
>                                 "xen,xen");
>       if (res) return res;
> 
> -    /* reg 0 is grant table space */
> +    /*
> +     * reg 0 is a placeholder for grant table space, reg 1...N are
> +     * the placeholders for extended regions.
> +     */
>       res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, 
> GUEST_ROOT_SIZE_CELLS,
> -                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> +                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0, 0);

Here you are relying on GUEST_RAM_BANKS == 2. I think this is pretty 
fragile as this may change in the future.

fdt_property_regs() is not really suitable for here. I would suggest to 
create a new helper fdt_property_placeholder() which takes the address 
cells, size cells and the number of ranges. The function will then 
create N range that are zeroed.

>       if (res) return res;
> 
>       /*
> @@ -1069,6 +1072,74 @@ static void finalise_one_node(libxl__gc *gc, void 
> *fdt, const char *uname,
>       }
>   }
> 
> +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
> +
> +static void finalise_ext_region(libxl__gc *gc, struct xc_dom_image *dom)

The function is doing more than finalizing extend regions, it also 
create the grant table regs. So how about naming it: 
finalize_hypervisor_node()?

> +{
> +    void *fdt = dom->devicetree_blob;
> +    uint64_t region_size[GUEST_RAM_BANKS] = {0}, 
> region_base[GUEST_RAM_BANKS],
> +        bankend[GUEST_RAM_BANKS];
> +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> +                  (GUEST_RAM_BANKS + 1)];
> +    be32 *cells = &regs[0];
> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> +    unsigned int i, len, nr_regions = 0;
> +    libxl_dominfo info;
> +    int offset, rc;
> +
> +    offset = fdt_path_offset(fdt, "/hypervisor");
> +    assert(offset > 0);
> +
> +    rc = libxl_domain_info(CTX, &info, dom->guest_domid);
> +    assert(!rc);
> +
> +    assert(info.gpaddr_bits <= 64);

Neither of the two should be assert(). They should be proper check so we 
don't end up with a disaster (in particularly for the former) if there 
is a problem.

> +
> +    /*
> +     * Try to allocate separate 2MB-aligned extended regions from the 
> first
> +     * and second RAM banks taking into the account the maximum supported
> +     * guest physical address space size and the amount of memory assigned
> +     * to the guest.
> +     */
> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> +        region_base[i] = bankbase[i] +
> +            ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << 
> XC_PAGE_SHIFT);
> +
> +        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
> +        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
> +        if (bankend[i] > region_base[i])
> +            region_size[i] = bankend[i] - region_base[i] + 1;
> +    } > +
> +    /*
> +     * The region 0 for grant table space must be always present. If we 
> managed
> +     * to allocate the extended regions then insert them as regions 1...N.
> +     */
> +    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> +
> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> +        if (region_size[i] < GUEST_EXT_REGION_MIN_SIZE)
> +            continue;
> +
> +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
> +            nr_regions, region_base[i], region_base[i] + region_size[i]);
> +
> +        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +                  region_base[i], region_size[i]);
> +        nr_regions++;
> +    }
> +
> +    if (!nr_regions)
> +        LOG(WARN, "The extended regions cannot be allocated, not enough 
> space");
> +
> +    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + 
> GUEST_ROOT_SIZE_CELLS) *
> +        (nr_regions + 1);
> +    rc = fdt_setprop(fdt, offset, "reg", regs, len);
> +    assert(!rc);

We should propagate the error.

> +}
> +
>   int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>                                                  uint32_t domid,
> libxl_domain_config *d_config,
> @@ -1109,6 +1180,8 @@ int 
> libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
> 
>       }
> 
> +    finalise_ext_region(gc, dom);
> +
>       for (i = 0; i < GUEST_RAM_BANKS; i++) {
>           const uint64_t size = (uint64_t)dom->rambank_size[i] << 
> XC_PAGE_SHIFT;
> 
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index d46c61f..7425a78 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -438,6 +438,11 @@ typedef uint64_t xen_callback_t;
> 
>   #define GUEST_RAM_BANKS   2
> 
> +/*
> + * The way to find the extended regions (to be exposed to the guest as 
> unused
> + * address space) relies on the fact that the regions reserved for the RAM
> + * below are big enough to also accommodate such regions.
> + */
>   #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM 
> @ 1GB */
>   #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
> 
> @@ -451,6 +456,8 @@ typedef uint64_t xen_callback_t;
>   #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
>   #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
> 
> +#define GUEST_EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */

I would prefer if this value is not part of the public header because 
this is not a value that the hypervisor needs to know. So it is better 
to restrict it to the libxl_arm.c

Cheers,

-- 
Julien Grall

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Julien Grall]

Hi Oleksandr,

On 13/10/2021 16:05, Oleksandr wrote:
> On 13.10.21 16:56, Jan Beulich wrote:
>> While this has meanwhile moved quite far from the original proposal,
>> I still wonder in how far Andrew may have remaining concerns. Did
>> you check with him, perhaps on irc?
>> But of course catching his
>> attention may be difficult, so no (further) feedback by him should
>> probably not keep this from getting committed (if no other open
>> issues remain).
> 
> If I remember correctly, I made several attempts to clarify in the 
> initial thread. But, I have to admit, not via IRC (I am expecting some 
> troubles with my IRC client last time and I haven't figured out yet 
> why). Of course, it would be correct to get a feedback to make sure that 
> there would be no remaining concerns (as the main concern to use domctl 
> was addressed) or otherwise.

I have pinged Andrew on IRC for you.

Cheers,

-- 
Julien Grall

Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Oleksandr]

On 13.10.21 18:17, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


>
> On 13/10/2021 16:05, Oleksandr wrote:
>> On 13.10.21 16:56, Jan Beulich wrote:
>>> While this has meanwhile moved quite far from the original proposal,
>>> I still wonder in how far Andrew may have remaining concerns. Did
>>> you check with him, perhaps on irc?
>>> But of course catching his
>>> attention may be difficult, so no (further) feedback by him should
>>> probably not keep this from getting committed (if no other open
>>> issues remain).
>>
>> If I remember correctly, I made several attempts to clarify in the 
>> initial thread. But, I have to admit, not via IRC (I am expecting 
>> some troubles with my IRC client last time and I haven't figured out 
>> yet why). Of course, it would be correct to get a feedback to make 
>> sure that there would be no remaining concerns (as the main concern 
>> to use domctl was addressed) or otherwise.
>
> I have pinged Andrew on IRC for you.

Thank you for the help.


>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko

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

[Oleksandr]

On 13.10.21 18:15, Julien Grall wrote:
> On 13/10/2021 14:46, Oleksandr wrote:
>>
>> Hi Julien
>
> Hi Oleksandr,

Hi Julien


Thank you for the prompt response.


>
>> On 13.10.21 00:43, Oleksandr wrote:
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index e3140a6..53ae0f3 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc *gc, 
>> void *fdt,
>>                                 "xen,xen");
>>       if (res) return res;
>>
>> -    /* reg 0 is grant table space */
>> +    /*
>> +     * reg 0 is a placeholder for grant table space, reg 1...N are
>> +     * the placeholders for extended regions.
>> +     */
>>       res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, 
>> GUEST_ROOT_SIZE_CELLS,
>> -                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>> +                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0, 0);
>
> Here you are relying on GUEST_RAM_BANKS == 2. I think this is pretty 
> fragile as this may change in the future.
>
> fdt_property_regs() is not really suitable for here. I would suggest 
> to create a new helper fdt_property_placeholder() which takes the 
> address cells, size cells and the number of ranges. The function will 
> then create N range that are zeroed.

You are right. Probably, we could add an assert here for now to be 
triggered if "GUEST_RAM_BANKS != 2"?
But, if you still think we need to make it with the helper right now, I 
will. However, I don't know how long this will take.


>
>
>>       if (res) return res;
>>
>>       /*
>> @@ -1069,6 +1072,74 @@ static void finalise_one_node(libxl__gc *gc, 
>> void *fdt, const char *uname,
>>       }
>>   }
>>
>> +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
>> +
>> +static void finalise_ext_region(libxl__gc *gc, struct xc_dom_image 
>> *dom)
>
> The function is doing more than finalizing extend regions, it also 
> create the grant table regs. So how about naming it: 
> finalize_hypervisor_node()?

ok, I don't mind.


>
>
>> +{
>> +    void *fdt = dom->devicetree_blob;
>> +    uint64_t region_size[GUEST_RAM_BANKS] = {0}, 
>> region_base[GUEST_RAM_BANKS],
>> +        bankend[GUEST_RAM_BANKS];
>> +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
>> +                  (GUEST_RAM_BANKS + 1)];
>> +    be32 *cells = &regs[0];
>> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>> +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
>> +    unsigned int i, len, nr_regions = 0;
>> +    libxl_dominfo info;
>> +    int offset, rc;
>> +
>> +    offset = fdt_path_offset(fdt, "/hypervisor");
>> +    assert(offset > 0);
>> +
>> +    rc = libxl_domain_info(CTX, &info, dom->guest_domid);
>> +    assert(!rc);
>> +
>> +    assert(info.gpaddr_bits <= 64);
>
> Neither of the two should be assert(). They should be proper check so 
> we don't end up with a disaster (in particularly for the former) if 
> there is a problem.

I looked at the similar finalise_*(), and it looks like no one bothers 
with returning an error. Of course, this is not an excuse, will add a 
proper check.


>
>
>> +
>> +    /*
>> +     * Try to allocate separate 2MB-aligned extended regions from 
>> the first
>> +     * and second RAM banks taking into the account the maximum 
>> supported
>> +     * guest physical address space size and the amount of memory 
>> assigned
>> +     * to the guest.
>> +     */
>> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
>> +        region_base[i] = bankbase[i] +
>> +            ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << 
>> XC_PAGE_SHIFT);
>> +
>> +        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
>> +        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
>> +        if (bankend[i] > region_base[i])
>> +            region_size[i] = bankend[i] - region_base[i] + 1;
>> +    } > +
>> +    /*
>> +     * The region 0 for grant table space must be always present. If 
>> we managed
>> +     * to allocate the extended regions then insert them as regions 
>> 1...N.
>> +     */
>> +    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>> +              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>> +
>> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
>> +        if (region_size[i] < GUEST_EXT_REGION_MIN_SIZE)
>> +            continue;
>> +
>> +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
>> +            nr_regions, region_base[i], region_base[i] + 
>> region_size[i]);
>> +
>> +        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, 
>> GUEST_ROOT_SIZE_CELLS,
>> +                  region_base[i], region_size[i]);
>> +        nr_regions++;
>> +    }
>> +
>> +    if (!nr_regions)
>> +        LOG(WARN, "The extended regions cannot be allocated, not 
>> enough space");
>> +
>> +    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + 
>> GUEST_ROOT_SIZE_CELLS) *
>> +        (nr_regions + 1);
>> +    rc = fdt_setprop(fdt, offset, "reg", regs, len);
>> +    assert(!rc);
>
> We should propagate the error.

ok, will propagate, it looks like an upper layer 
libxl__arch_domain_finalise_hw_description() also needs to propagate it.


>
>
>> +}
>> +
>>   int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>>                                                  uint32_t domid,
>> libxl_domain_config *d_config,
>> @@ -1109,6 +1180,8 @@ int 
>> libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>>
>>       }
>>
>> +    finalise_ext_region(gc, dom);
>> +
>>       for (i = 0; i < GUEST_RAM_BANKS; i++) {
>>           const uint64_t size = (uint64_t)dom->rambank_size[i] << 
>> XC_PAGE_SHIFT;
>>
>> diff --git a/xen/include/public/arch-arm.h 
>> b/xen/include/public/arch-arm.h
>> index d46c61f..7425a78 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -438,6 +438,11 @@ typedef uint64_t xen_callback_t;
>>
>>   #define GUEST_RAM_BANKS   2
>>
>> +/*
>> + * The way to find the extended regions (to be exposed to the guest 
>> as unused
>> + * address space) relies on the fact that the regions reserved for 
>> the RAM
>> + * below are big enough to also accommodate such regions.
>> + */
>>   #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low 
>> RAM @ 1GB */
>>   #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
>>
>> @@ -451,6 +456,8 @@ typedef uint64_t xen_callback_t;
>>   #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
>>   #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
>>
>> +#define GUEST_EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 
>> 64MB */
>
> I would prefer if this value is not part of the public header because 
> this is not a value that the hypervisor needs to know. So it is better 
> to restrict it to the libxl_arm.c

ok, will do.


-- 
Regards,

Oleksandr Tyshchenko

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

[Julien Grall]

Hi Oleksandr,

On 13/10/2021 16:49, Oleksandr wrote:
> 
> On 13.10.21 18:15, Julien Grall wrote:
>> On 13/10/2021 14:46, Oleksandr wrote:
>>>
>>> Hi Julien
>>
>> Hi Oleksandr,
> 
> Hi Julien
> 
> 
> Thank you for the prompt response.
> 
> 
>>
>>> On 13.10.21 00:43, Oleksandr wrote:
>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>> index e3140a6..53ae0f3 100644
>>> --- a/tools/libs/light/libxl_arm.c
>>> +++ b/tools/libs/light/libxl_arm.c
>>> @@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc *gc, 
>>> void *fdt,
>>>                                 "xen,xen");
>>>       if (res) return res;
>>>
>>> -    /* reg 0 is grant table space */
>>> +    /*
>>> +     * reg 0 is a placeholder for grant table space, reg 1...N are
>>> +     * the placeholders for extended regions.
>>> +     */
>>>       res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, 
>>> GUEST_ROOT_SIZE_CELLS,
>>> -                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>>> +                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0, 0);
>>
>> Here you are relying on GUEST_RAM_BANKS == 2. I think this is pretty 
>> fragile as this may change in the future.
>>
>> fdt_property_regs() is not really suitable for here. I would suggest 
>> to create a new helper fdt_property_placeholder() which takes the 
>> address cells, size cells and the number of ranges. The function will 
>> then create N range that are zeroed.
> 
> You are right. Probably, we could add an assert here for now to be 
> triggered if "GUEST_RAM_BANKS != 2"?
> But, if you still think we need to make it with the helper right now, I 
> will. However, I don't know how long this will take.

I would prefer if we introduce the helper now. Below a potential version 
(not compiled):

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6e0039..df59a0521412 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -269,6 +269,20 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
      return fdt_property(fdt, "reg", regs, sizeof(regs));
  }

+static int fdt_property_reg_placeholder(libxl__gc *gc, void *fdt,
+                                        unsigned int addr_cells,
+                                        unsigned int size_cells,
+                                        unsigned int num_regs)
+{
+    uint32_t regs[num_regs * (addr_cells + size_cells)];
+
+    for (i = 0 ; i < num_regs; i++) {
+        set_range(&cells, addr_cells, size_cells, 0, 0);
+    }
+
+    return fdt_property(fdt, "reg", regs, sizeof(regs));
+}
+
  static int make_root_properties(libxl__gc *gc,
                                  const libxl_version_info *vers,

>>> +{
>>> +    void *fdt = dom->devicetree_blob;
>>> +    uint64_t region_size[GUEST_RAM_BANKS] = {0}, 
>>> region_base[GUEST_RAM_BANKS],
>>> +        bankend[GUEST_RAM_BANKS];
>>> +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
>>> +                  (GUEST_RAM_BANKS + 1)];
>>> +    be32 *cells = &regs[0];
>>> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>>> +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
>>> +    unsigned int i, len, nr_regions = 0;
>>> +    libxl_dominfo info;
>>> +    int offset, rc;
>>> +
>>> +    offset = fdt_path_offset(fdt, "/hypervisor");
>>> +    assert(offset > 0);
>>> +
>>> +    rc = libxl_domain_info(CTX, &info, dom->guest_domid);
>>> +    assert(!rc);
>>> +
>>> +    assert(info.gpaddr_bits <= 64);
>>
>> Neither of the two should be assert(). They should be proper check so 
>> we don't end up with a disaster (in particularly for the former) if 
>> there is a problem.
> 
> I looked at the similar finalise_*(), and it looks like no one bothers 
> with returning an error. Of course, this is not an excuse, will add a 
> proper check.

This is a bit unfortunate. I would prefer if this can be avoided for new 
code (the more libxl__arch_domain_finalise_hw_description() can already 
propagate the error).

Cheers,

-- 
Julien Grall

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

[Oleksandr]

On 13.10.21 19:24, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien



>
> On 13/10/2021 16:49, Oleksandr wrote:
>>
>> On 13.10.21 18:15, Julien Grall wrote:
>>> On 13/10/2021 14:46, Oleksandr wrote:
>>>>
>>>> Hi Julien
>>>
>>> Hi Oleksandr,
>>
>> Hi Julien
>>
>>
>> Thank you for the prompt response.
>>
>>
>>>
>>>> On 13.10.21 00:43, Oleksandr wrote:
>>>> diff --git a/tools/libs/light/libxl_arm.c 
>>>> b/tools/libs/light/libxl_arm.c
>>>> index e3140a6..53ae0f3 100644
>>>> --- a/tools/libs/light/libxl_arm.c
>>>> +++ b/tools/libs/light/libxl_arm.c
>>>> @@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc *gc, 
>>>> void *fdt,
>>>>                                 "xen,xen");
>>>>       if (res) return res;
>>>>
>>>> -    /* reg 0 is grant table space */
>>>> +    /*
>>>> +     * reg 0 is a placeholder for grant table space, reg 1...N are
>>>> +     * the placeholders for extended regions.
>>>> +     */
>>>>       res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, 
>>>> GUEST_ROOT_SIZE_CELLS,
>>>> -                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>>>> +                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0, 0);
>>>
>>> Here you are relying on GUEST_RAM_BANKS == 2. I think this is pretty 
>>> fragile as this may change in the future.
>>>
>>> fdt_property_regs() is not really suitable for here. I would suggest 
>>> to create a new helper fdt_property_placeholder() which takes the 
>>> address cells, size cells and the number of ranges. The function 
>>> will then create N range that are zeroed.
>>
>> You are right. Probably, we could add an assert here for now to be 
>> triggered if "GUEST_RAM_BANKS != 2"?
>> But, if you still think we need to make it with the helper right now, 
>> I will. However, I don't know how long this will take.
>
> I would prefer if we introduce the helper now. Below a potential 
> version (not compiled):


You wouldn't believe)))
I decided to not wait for the answer and re-check. So, I ended up with 
the similar to what you suggested below. Thank you.
Yes, it will work if add missing locals. However, I initially named it 
exactly as was suggested (fdt_property_placeholder) and got a 
compilation error, since fdt_property_placeholder is already present.
So, I was thinking to choose another name or to even open-code it, but I 
see you already proposed new name fdt_property_reg_placeholder.

...

libxl_arm.c:366:12: error: conflicting types for 'fdt_property_placeholder'
   366 | static int fdt_property_placeholder(libxl__gc *gc, void *fdt,
       |            ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from libxl_libfdt_compat.h:64,
                  from libxl_arm.c:3:

...


>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index e3140a6e0039..df59a0521412 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -269,6 +269,20 @@ static int fdt_property_regs(libxl__gc *gc, void 
> *fdt,
>      return fdt_property(fdt, "reg", regs, sizeof(regs));
>  }
>
> +static int fdt_property_reg_placeholder(libxl__gc *gc, void *fdt,
> +                                        unsigned int addr_cells,
> +                                        unsigned int size_cells,
> +                                        unsigned int num_regs)
> +{
> +    uint32_t regs[num_regs * (addr_cells + size_cells)];
> +
> +    for (i = 0 ; i < num_regs; i++) {
> +        set_range(&cells, addr_cells, size_cells, 0, 0);
> +    }
> +
> +    return fdt_property(fdt, "reg", regs, sizeof(regs));
> +}
> +
>  static int make_root_properties(libxl__gc *gc,
>                                  const libxl_version_info *vers,
>
>>>> +{
>>>> +    void *fdt = dom->devicetree_blob;
>>>> +    uint64_t region_size[GUEST_RAM_BANKS] = {0}, 
>>>> region_base[GUEST_RAM_BANKS],
>>>> +        bankend[GUEST_RAM_BANKS];
>>>> +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + 
>>>> GUEST_ROOT_SIZE_CELLS) *
>>>> +                  (GUEST_RAM_BANKS + 1)];
>>>> +    be32 *cells = &regs[0];
>>>> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>>>> +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
>>>> +    unsigned int i, len, nr_regions = 0;
>>>> +    libxl_dominfo info;
>>>> +    int offset, rc;
>>>> +
>>>> +    offset = fdt_path_offset(fdt, "/hypervisor");
>>>> +    assert(offset > 0);
>>>> +
>>>> +    rc = libxl_domain_info(CTX, &info, dom->guest_domid);
>>>> +    assert(!rc);
>>>> +
>>>> +    assert(info.gpaddr_bits <= 64);
>>>
>>> Neither of the two should be assert(). They should be proper check 
>>> so we don't end up with a disaster (in particularly for the former) 
>>> if there is a problem.
>>
>> I looked at the similar finalise_*(), and it looks like no one 
>> bothers with returning an error. Of course, this is not an excuse, 
>> will add a proper check.
>
> This is a bit unfortunate. I would prefer if this can be avoided for 
> new code (the more libxl__arch_domain_finalise_hw_description() can 
> already propagate the error).
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko

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

[Julien Grall]

Hi,

On 13/10/2021 17:44, Oleksandr wrote:
> On 13.10.21 19:24, Julien Grall wrote:
>> On 13/10/2021 16:49, Oleksandr wrote:
>>>
>>> On 13.10.21 18:15, Julien Grall wrote:
>>>> On 13/10/2021 14:46, Oleksandr wrote:
>>>>>
>>>>> Hi Julien
>>>>
>>>> Hi Oleksandr,
>>>
>>> Hi Julien
>>>
>>>
>>> Thank you for the prompt response.
>>>
>>>
>>>>
>>>>> On 13.10.21 00:43, Oleksandr wrote:
>>>>> diff --git a/tools/libs/light/libxl_arm.c 
>>>>> b/tools/libs/light/libxl_arm.c
>>>>> index e3140a6..53ae0f3 100644
>>>>> --- a/tools/libs/light/libxl_arm.c
>>>>> +++ b/tools/libs/light/libxl_arm.c
>>>>> @@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc *gc, 
>>>>> void *fdt,
>>>>>                                 "xen,xen");
>>>>>       if (res) return res;
>>>>>
>>>>> -    /* reg 0 is grant table space */
>>>>> +    /*
>>>>> +     * reg 0 is a placeholder for grant table space, reg 1...N are
>>>>> +     * the placeholders for extended regions.
>>>>> +     */
>>>>>       res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, 
>>>>> GUEST_ROOT_SIZE_CELLS,
>>>>> -                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>>>>> +                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0, 0);
>>>>
>>>> Here you are relying on GUEST_RAM_BANKS == 2. I think this is pretty 
>>>> fragile as this may change in the future.
>>>>
>>>> fdt_property_regs() is not really suitable for here. I would suggest 
>>>> to create a new helper fdt_property_placeholder() which takes the 
>>>> address cells, size cells and the number of ranges. The function 
>>>> will then create N range that are zeroed.
>>>
>>> You are right. Probably, we could add an assert here for now to be 
>>> triggered if "GUEST_RAM_BANKS != 2"?
>>> But, if you still think we need to make it with the helper right now, 
>>> I will. However, I don't know how long this will take.
>>
>> I would prefer if we introduce the helper now. Below a potential 
>> version (not compiled):
> 
> 
> You wouldn't believe)))
> I decided to not wait for the answer and re-check. So, I ended up with 
> the similar to what you suggested below. Thank you.
> Yes, it will work if add missing locals. However, I initially named it 
> exactly as was suggested (fdt_property_placeholder) and got a 
> compilation error, since fdt_property_placeholder is already present.
> So, I was thinking to choose another name or to even open-code it, but I 
> see you already proposed new name fdt_property_reg_placeholder.

Ah I didn't realize that libfdt already implemented 
fdt_property_placeholder(). The function sounds perfect for us (and the 
code is nicer :)), but unfortunately this was introdcued only in 2017. 
So it is possible that some distros may not ship the last libfdt.

So for now, I think we need to implement our own. In a follow-up we 
could try to provide a compat layer as we did for other missing fdt_* 
helpers.

Cheers,

-- 
Julien Grall

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

[Oleksandr]

On 13.10.21 20:07, Julien Grall wrote:

Hi Julien

> Hi,
>
> On 13/10/2021 17:44, Oleksandr wrote:
>> On 13.10.21 19:24, Julien Grall wrote:
>>> On 13/10/2021 16:49, Oleksandr wrote:
>>>>
>>>> On 13.10.21 18:15, Julien Grall wrote:
>>>>> On 13/10/2021 14:46, Oleksandr wrote:
>>>>>>
>>>>>> Hi Julien
>>>>>
>>>>> Hi Oleksandr,
>>>>
>>>> Hi Julien
>>>>
>>>>
>>>> Thank you for the prompt response.
>>>>
>>>>
>>>>>
>>>>>> On 13.10.21 00:43, Oleksandr wrote:
>>>>>> diff --git a/tools/libs/light/libxl_arm.c 
>>>>>> b/tools/libs/light/libxl_arm.c
>>>>>> index e3140a6..53ae0f3 100644
>>>>>> --- a/tools/libs/light/libxl_arm.c
>>>>>> +++ b/tools/libs/light/libxl_arm.c
>>>>>> @@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc 
>>>>>> *gc, void *fdt,
>>>>>>                                 "xen,xen");
>>>>>>       if (res) return res;
>>>>>>
>>>>>> -    /* reg 0 is grant table space */
>>>>>> +    /*
>>>>>> +     * reg 0 is a placeholder for grant table space, reg 1...N are
>>>>>> +     * the placeholders for extended regions.
>>>>>> +     */
>>>>>>       res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, 
>>>>>> GUEST_ROOT_SIZE_CELLS,
>>>>>> -                            1,GUEST_GNTTAB_BASE, 
>>>>>> GUEST_GNTTAB_SIZE);
>>>>>> +                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0, 0);
>>>>>
>>>>> Here you are relying on GUEST_RAM_BANKS == 2. I think this is 
>>>>> pretty fragile as this may change in the future.
>>>>>
>>>>> fdt_property_regs() is not really suitable for here. I would 
>>>>> suggest to create a new helper fdt_property_placeholder() which 
>>>>> takes the address cells, size cells and the number of ranges. The 
>>>>> function will then create N range that are zeroed.
>>>>
>>>> You are right. Probably, we could add an assert here for now to be 
>>>> triggered if "GUEST_RAM_BANKS != 2"?
>>>> But, if you still think we need to make it with the helper right 
>>>> now, I will. However, I don't know how long this will take.
>>>
>>> I would prefer if we introduce the helper now. Below a potential 
>>> version (not compiled):
>>
>>
>> You wouldn't believe)))
>> I decided to not wait for the answer and re-check. So, I ended up 
>> with the similar to what you suggested below. Thank you.
>> Yes, it will work if add missing locals. However, I initially named 
>> it exactly as was suggested (fdt_property_placeholder) and got a 
>> compilation error, since fdt_property_placeholder is already present.
>> So, I was thinking to choose another name or to even open-code it, 
>> but I see you already proposed new name fdt_property_reg_placeholder.
>
> Ah I didn't realize that libfdt already implemented 
> fdt_property_placeholder(). The function sounds perfect for us (and 
> the code is nicer :)), but unfortunately this was introdcued only in 
> 2017. So it is possible that some distros may not ship the last libfdt.
>
> So for now, I think we need to implement our own. In a follow-up we 
> could try to provide a compat layer as we did for other missing fdt_* 
> helpers.
>
> Cheers,


Well, I hope that I addressed all your comments. If so, I will prepare V7.


diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6..a780155 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -269,6 +269,21 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
      return fdt_property(fdt, "reg", regs, sizeof(regs));
  }

+static int fdt_property_reg_placeholder(libxl__gc *gc, void *fdt,
+                                        unsigned int addr_cells,
+                                        unsigned int size_cells,
+                                        unsigned int num_regs)
+{
+    uint32_t regs[num_regs * (addr_cells + size_cells)];
+    be32 *cells = &regs[0];
+    unsigned int i;
+
+    for (i = 0; i < num_regs; i++)
+        set_range(&cells, addr_cells, size_cells, 0, 0);
+
+    return fdt_property(fdt, "reg", regs, sizeof(regs));
+}
+
  static int make_root_properties(libxl__gc *gc,
                                  const libxl_version_info *vers,
                                  void *fdt)
@@ -615,9 +630,13 @@ 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);
+    /*
+     * reg 0 is a placeholder for grant table space, reg 1...N are
+     * the placeholders for extended regions.
+     */
+    res = fdt_property_reg_placeholder(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
+                                       GUEST_ROOT_SIZE_CELLS,
+                                       GUEST_RAM_BANKS + 1);
      if (res) return res;

      /*
@@ -1069,20 +1088,93 @@ static void finalise_one_node(libxl__gc *gc, 
void *fdt, const char *uname,
      }
  }

+#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
+
+#define EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
+
+static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image 
*dom)
+{
+    void *fdt = dom->devicetree_blob;
+    uint64_t region_size[GUEST_RAM_BANKS] = {0}, 
region_base[GUEST_RAM_BANKS],
+        bankend[GUEST_RAM_BANKS];
+    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
+                  (GUEST_RAM_BANKS + 1)];
+    be32 *cells = &regs[0];
+    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
+    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
+    unsigned int i, len, nr_regions = 0;
+    libxl_dominfo info;
+    int offset, rc;
+
+    offset = fdt_path_offset(fdt, "/hypervisor");
+    if (offset < 0)
+        return offset;
+
+    rc = libxl_domain_info(CTX, &info, dom->guest_domid);
+    if (rc)
+        return rc;
+
+    if (info.gpaddr_bits > 64)
+        return ERROR_INVAL;
+
+    /*
+     * Try to allocate separate 2MB-aligned extended regions from the first
+     * and second RAM banks taking into the account the maximum supported
+     * guest physical address space size and the amount of memory assigned
+     * to the guest.
+     */
+    for (i = 0; i < GUEST_RAM_BANKS; i++) {
+        region_base[i] = bankbase[i] +
+            ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << 
XC_PAGE_SHIFT);
+
+        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
+        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
+        if (bankend[i] > region_base[i])
+            region_size[i] = bankend[i] - region_base[i] + 1;
+    }
+
+    /*
+     * The region 0 for grant table space must be always present. If we 
managed
+     * to allocate the extended regions then insert them as regions 1...N.
+     */
+    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
+
+    for (i = 0; i < GUEST_RAM_BANKS; i++) {
+        if (region_size[i] < EXT_REGION_MIN_SIZE)
+            continue;
+
+        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
+            nr_regions, region_base[i], region_base[i] + region_size[i]);
+
+        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+                  region_base[i], region_size[i]);
+        nr_regions++;
+    }
+
+    if (!nr_regions)
+        LOG(WARN, "The extended regions cannot be allocated, not enough 
space");
+
+    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + 
GUEST_ROOT_SIZE_CELLS) *
+        (nr_regions + 1);
+
+    return fdt_setprop(fdt, offset, "reg", regs, len);
+}
+
  int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
                                                 uint32_t domid,
libxl_domain_config *d_config,
                                                 struct xc_dom_image *dom)
  {
      void *fdt = dom->devicetree_blob;
-    int i;
+    int i, res;
      const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;

      const struct xc_dom_seg *ramdisk = dom->modules[0].blob ?
          &dom->modules[0].seg : NULL;

      if (ramdisk) {
-        int chosen, res;
+        int chosen;
          uint64_t val;

          /* Neither the fdt_path_offset() nor either of the
@@ -1109,6 +1201,10 @@ int 
libxl__arch_domain_finalise_hw_description(libxl__gc *gc,

      }

+    res = finalize_hypervisor_node(gc, dom);
+    if (res)
+        return res;
+
      for (i = 0; i < GUEST_RAM_BANKS; i++) {
          const uint64_t size = (uint64_t)dom->rambank_size[i] << 
XC_PAGE_SHIFT;

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index d46c61f..96ead3d 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -438,6 +438,11 @@ typedef uint64_t xen_callback_t;

  #define GUEST_RAM_BANKS   2

+/*
+ * The way to find the extended regions (to be exposed to the guest as 
unused
+ * address space) relies on the fact that the regions reserved for the RAM
+ * below are big enough to also accommodate such regions.
+ */
  #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM 
@ 1GB */
  #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)

(END)


-- 
Regards,

Oleksandr Tyshchenko

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

[Oleksandr]

Hi Stefano,


On 13.10.21 21:26, Oleksandr wrote:
>
> On 13.10.21 20:07, Julien Grall wrote:
>
> Hi Julien
>
>> Hi,
>>
>> On 13/10/2021 17:44, Oleksandr wrote:
>>> On 13.10.21 19:24, Julien Grall wrote:
>>>> On 13/10/2021 16:49, Oleksandr wrote:
>>>>>
>>>>> On 13.10.21 18:15, Julien Grall wrote:
>>>>>> On 13/10/2021 14:46, Oleksandr wrote:
>>>>>>>
>>>>>>> Hi Julien
>>>>>>
>>>>>> Hi Oleksandr,
>>>>>
>>>>> Hi Julien
>>>>>
>>>>>
>>>>> Thank you for the prompt response.
>>>>>
>>>>>
>>>>>>
>>>>>>> On 13.10.21 00:43, Oleksandr wrote:
>>>>>>> diff --git a/tools/libs/light/libxl_arm.c 
>>>>>>> b/tools/libs/light/libxl_arm.c
>>>>>>> index e3140a6..53ae0f3 100644
>>>>>>> --- a/tools/libs/light/libxl_arm.c
>>>>>>> +++ b/tools/libs/light/libxl_arm.c
>>>>>>> @@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc 
>>>>>>> *gc, void *fdt,
>>>>>>>                                 "xen,xen");
>>>>>>>       if (res) return res;
>>>>>>>
>>>>>>> -    /* reg 0 is grant table space */
>>>>>>> +    /*
>>>>>>> +     * reg 0 is a placeholder for grant table space, reg 1...N are
>>>>>>> +     * the placeholders for extended regions.
>>>>>>> +     */
>>>>>>>       res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, 
>>>>>>> GUEST_ROOT_SIZE_CELLS,
>>>>>>> -                            1,GUEST_GNTTAB_BASE, 
>>>>>>> GUEST_GNTTAB_SIZE);
>>>>>>> +                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0, 
>>>>>>> 0);
>>>>>>
>>>>>> Here you are relying on GUEST_RAM_BANKS == 2. I think this is 
>>>>>> pretty fragile as this may change in the future.
>>>>>>
>>>>>> fdt_property_regs() is not really suitable for here. I would 
>>>>>> suggest to create a new helper fdt_property_placeholder() which 
>>>>>> takes the address cells, size cells and the number of ranges. The 
>>>>>> function will then create N range that are zeroed.
>>>>>
>>>>> You are right. Probably, we could add an assert here for now to be 
>>>>> triggered if "GUEST_RAM_BANKS != 2"?
>>>>> But, if you still think we need to make it with the helper right 
>>>>> now, I will. However, I don't know how long this will take.
>>>>
>>>> I would prefer if we introduce the helper now. Below a potential 
>>>> version (not compiled):
>>>
>>>
>>> You wouldn't believe)))
>>> I decided to not wait for the answer and re-check. So, I ended up 
>>> with the similar to what you suggested below. Thank you.
>>> Yes, it will work if add missing locals. However, I initially named 
>>> it exactly as was suggested (fdt_property_placeholder) and got a 
>>> compilation error, since fdt_property_placeholder is already present.
>>> So, I was thinking to choose another name or to even open-code it, 
>>> but I see you already proposed new name fdt_property_reg_placeholder.
>>
>> Ah I didn't realize that libfdt already implemented 
>> fdt_property_placeholder(). The function sounds perfect for us (and 
>> the code is nicer :)), but unfortunately this was introdcued only in 
>> 2017. So it is possible that some distros may not ship the last libfdt.
>>
>> So for now, I think we need to implement our own. In a follow-up we 
>> could try to provide a compat layer as we did for other missing fdt_* 
>> helpers.
>>
>> Cheers,
>
>
> Well, I hope that I addressed all your comments. If so, I will prepare V7.


May I please ask, are you *preliminary* OK with new changes requested by 
Julien? The main changes are to bring finalize_*() back (hopefully there 
is a way how to downsize a placeholder), avoid relying on the fact that 
Bank 0 is always below 4GB, adding a convenient helper to create a 
placeholder for N ranges plus some code hardening, etc.


>
>
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index e3140a6..a780155 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -269,6 +269,21 @@ static int fdt_property_regs(libxl__gc *gc, void 
> *fdt,
>      return fdt_property(fdt, "reg", regs, sizeof(regs));
>  }
>
> +static int fdt_property_reg_placeholder(libxl__gc *gc, void *fdt,
> +                                        unsigned int addr_cells,
> +                                        unsigned int size_cells,
> +                                        unsigned int num_regs)
> +{
> +    uint32_t regs[num_regs * (addr_cells + size_cells)];
> +    be32 *cells = &regs[0];
> +    unsigned int i;
> +
> +    for (i = 0; i < num_regs; i++)
> +        set_range(&cells, addr_cells, size_cells, 0, 0);
> +
> +    return fdt_property(fdt, "reg", regs, sizeof(regs));
> +}
> +
>  static int make_root_properties(libxl__gc *gc,
>                                  const libxl_version_info *vers,
>                                  void *fdt)
> @@ -615,9 +630,13 @@ 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);
> +    /*
> +     * reg 0 is a placeholder for grant table space, reg 1...N are
> +     * the placeholders for extended regions.
> +     */
> +    res = fdt_property_reg_placeholder(gc, fdt, 
> GUEST_ROOT_ADDRESS_CELLS,
> +                                       GUEST_ROOT_SIZE_CELLS,
> +                                       GUEST_RAM_BANKS + 1);
>      if (res) return res;
>
>      /*
> @@ -1069,20 +1088,93 @@ static void finalise_one_node(libxl__gc *gc, 
> void *fdt, const char *uname,
>      }
>  }
>
> +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
> +
> +#define EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
> +
> +static int finalize_hypervisor_node(libxl__gc *gc, struct 
> xc_dom_image *dom)
> +{
> +    void *fdt = dom->devicetree_blob;
> +    uint64_t region_size[GUEST_RAM_BANKS] = {0}, 
> region_base[GUEST_RAM_BANKS],
> +        bankend[GUEST_RAM_BANKS];
> +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> +                  (GUEST_RAM_BANKS + 1)];
> +    be32 *cells = &regs[0];
> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> +    unsigned int i, len, nr_regions = 0;
> +    libxl_dominfo info;
> +    int offset, rc;
> +
> +    offset = fdt_path_offset(fdt, "/hypervisor");
> +    if (offset < 0)
> +        return offset;
> +
> +    rc = libxl_domain_info(CTX, &info, dom->guest_domid);
> +    if (rc)
> +        return rc;
> +
> +    if (info.gpaddr_bits > 64)
> +        return ERROR_INVAL;
> +
> +    /*
> +     * Try to allocate separate 2MB-aligned extended regions from the 
> first
> +     * and second RAM banks taking into the account the maximum 
> supported
> +     * guest physical address space size and the amount of memory 
> assigned
> +     * to the guest.
> +     */
> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> +        region_base[i] = bankbase[i] +
> +            ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << 
> XC_PAGE_SHIFT);
> +
> +        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
> +        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
> +        if (bankend[i] > region_base[i])
> +            region_size[i] = bankend[i] - region_base[i] + 1;
> +    }
> +
> +    /*
> +     * The region 0 for grant table space must be always present. If 
> we managed
> +     * to allocate the extended regions then insert them as regions 
> 1...N.
> +     */
> +    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> +
> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> +        if (region_size[i] < EXT_REGION_MIN_SIZE)
> +            continue;
> +
> +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
> +            nr_regions, region_base[i], region_base[i] + 
> region_size[i]);
> +
> +        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, 
> GUEST_ROOT_SIZE_CELLS,
> +                  region_base[i], region_size[i]);
> +        nr_regions++;
> +    }
> +
> +    if (!nr_regions)
> +        LOG(WARN, "The extended regions cannot be allocated, not 
> enough space");
> +
> +    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + 
> GUEST_ROOT_SIZE_CELLS) *
> +        (nr_regions + 1);
> +
> +    return fdt_setprop(fdt, offset, "reg", regs, len);
> +}
> +
>  int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>                                                 uint32_t domid,
> libxl_domain_config *d_config,
>                                                 struct xc_dom_image *dom)
>  {
>      void *fdt = dom->devicetree_blob;
> -    int i;
> +    int i, res;
>      const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>
>      const struct xc_dom_seg *ramdisk = dom->modules[0].blob ?
>          &dom->modules[0].seg : NULL;
>
>      if (ramdisk) {
> -        int chosen, res;
> +        int chosen;
>          uint64_t val;
>
>          /* Neither the fdt_path_offset() nor either of the
> @@ -1109,6 +1201,10 @@ int 
> libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>
>      }
>
> +    res = finalize_hypervisor_node(gc, dom);
> +    if (res)
> +        return res;
> +
>      for (i = 0; i < GUEST_RAM_BANKS; i++) {
>          const uint64_t size = (uint64_t)dom->rambank_size[i] << 
> XC_PAGE_SHIFT;
>
> diff --git a/xen/include/public/arch-arm.h 
> b/xen/include/public/arch-arm.h
> index d46c61f..96ead3d 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -438,6 +438,11 @@ typedef uint64_t xen_callback_t;
>
>  #define GUEST_RAM_BANKS   2
>
> +/*
> + * The way to find the extended regions (to be exposed to the guest 
> as unused
> + * address space) relies on the fact that the regions reserved for 
> the RAM
> + * below are big enough to also accommodate such regions.
> + */
>  #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM 
> @ 1GB */
>  #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
>
> (END)
>
>
-- 
Regards,

Oleksandr Tyshchenko

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 11953 bytes --]

On Wed, 13 Oct 2021, Oleksandr wrote:
> On 13.10.21 21:26, Oleksandr wrote:
> > 
> > On 13.10.21 20:07, Julien Grall wrote:
> > 
> > Hi Julien
> > 
> > > Hi,
> > > 
> > > On 13/10/2021 17:44, Oleksandr wrote:
> > > > On 13.10.21 19:24, Julien Grall wrote:
> > > > > On 13/10/2021 16:49, Oleksandr wrote:
> > > > > > 
> > > > > > On 13.10.21 18:15, Julien Grall wrote:
> > > > > > > On 13/10/2021 14:46, Oleksandr wrote:
> > > > > > > > 
> > > > > > > > Hi Julien
> > > > > > > 
> > > > > > > Hi Oleksandr,
> > > > > > 
> > > > > > Hi Julien
> > > > > > 
> > > > > > 
> > > > > > Thank you for the prompt response.
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > > On 13.10.21 00:43, Oleksandr wrote:
> > > > > > > > diff --git a/tools/libs/light/libxl_arm.c
> > > > > > > > b/tools/libs/light/libxl_arm.c
> > > > > > > > index e3140a6..53ae0f3 100644
> > > > > > > > --- a/tools/libs/light/libxl_arm.c
> > > > > > > > +++ b/tools/libs/light/libxl_arm.c
> > > > > > > > @@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc
> > > > > > > > *gc, void *fdt,
> > > > > > > >                                 "xen,xen");
> > > > > > > >       if (res) return res;
> > > > > > > > 
> > > > > > > > -    /* reg 0 is grant table space */
> > > > > > > > +    /*
> > > > > > > > +     * reg 0 is a placeholder for grant table space, reg 1...N
> > > > > > > > are
> > > > > > > > +     * the placeholders for extended regions.
> > > > > > > > +     */
> > > > > > > >       res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
> > > > > > > > GUEST_ROOT_SIZE_CELLS,
> > > > > > > > -                            1,GUEST_GNTTAB_BASE,
> > > > > > > > GUEST_GNTTAB_SIZE);
> > > > > > > > +                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0,
> > > > > > > > 0);
> > > > > > > 
> > > > > > > Here you are relying on GUEST_RAM_BANKS == 2. I think this is
> > > > > > > pretty fragile as this may change in the future.
> > > > > > > 
> > > > > > > fdt_property_regs() is not really suitable for here. I would
> > > > > > > suggest to create a new helper fdt_property_placeholder() which
> > > > > > > takes the address cells, size cells and the number of ranges. The
> > > > > > > function will then create N range that are zeroed.
> > > > > > 
> > > > > > You are right. Probably, we could add an assert here for now to be
> > > > > > triggered if "GUEST_RAM_BANKS != 2"?
> > > > > > But, if you still think we need to make it with the helper right
> > > > > > now, I will. However, I don't know how long this will take.
> > > > > 
> > > > > I would prefer if we introduce the helper now. Below a potential
> > > > > version (not compiled):
> > > > 
> > > > 
> > > > You wouldn't believe)))
> > > > I decided to not wait for the answer and re-check. So, I ended up with
> > > > the similar to what you suggested below. Thank you.
> > > > Yes, it will work if add missing locals. However, I initially named it
> > > > exactly as was suggested (fdt_property_placeholder) and got a
> > > > compilation error, since fdt_property_placeholder is already present.
> > > > So, I was thinking to choose another name or to even open-code it, but I
> > > > see you already proposed new name fdt_property_reg_placeholder.
> > > 
> > > Ah I didn't realize that libfdt already implemented
> > > fdt_property_placeholder(). The function sounds perfect for us (and the
> > > code is nicer :)), but unfortunately this was introdcued only in 2017. So
> > > it is possible that some distros may not ship the last libfdt.
> > > 
> > > So for now, I think we need to implement our own. In a follow-up we could
> > > try to provide a compat layer as we did for other missing fdt_* helpers.
> > > 
> > > Cheers,
> > 
> > 
> > Well, I hope that I addressed all your comments. If so, I will prepare V7.
> 
> 
> May I please ask, are you *preliminary* OK with new changes requested by
> Julien? The main changes are to bring finalize_*() back (hopefully there is a
> way how to downsize a placeholder), avoid relying on the fact that Bank 0 is
> always below 4GB, adding a convenient helper to create a placeholder for N
> ranges plus some code hardening, etc.

Yes, I am OK with it


> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index e3140a6..a780155 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -269,6 +269,21 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
> >      return fdt_property(fdt, "reg", regs, sizeof(regs));
> >  }
> > 
> > +static int fdt_property_reg_placeholder(libxl__gc *gc, void *fdt,
> > +                                        unsigned int addr_cells,
> > +                                        unsigned int size_cells,
> > +                                        unsigned int num_regs)
> > +{
> > +    uint32_t regs[num_regs * (addr_cells + size_cells)];
> > +    be32 *cells = &regs[0];
> > +    unsigned int i;
> > +
> > +    for (i = 0; i < num_regs; i++)
> > +        set_range(&cells, addr_cells, size_cells, 0, 0);
> > +
> > +    return fdt_property(fdt, "reg", regs, sizeof(regs));
> > +}
> > +
> >  static int make_root_properties(libxl__gc *gc,
> >                                  const libxl_version_info *vers,
> >                                  void *fdt)
> > @@ -615,9 +630,13 @@ 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);
> > +    /*
> > +     * reg 0 is a placeholder for grant table space, reg 1...N are
> > +     * the placeholders for extended regions.
> > +     */
> > +    res = fdt_property_reg_placeholder(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
> > +                                       GUEST_ROOT_SIZE_CELLS,
> > +                                       GUEST_RAM_BANKS + 1);
> >      if (res) return res;
> > 
> >      /*
> > @@ -1069,20 +1088,93 @@ static void finalise_one_node(libxl__gc *gc, void
> > *fdt, const char *uname,
> >      }
> >  }
> > 
> > +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
> > +
> > +#define EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
> > +
> > +static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image
> > *dom)
> > +{
> > +    void *fdt = dom->devicetree_blob;
> > +    uint64_t region_size[GUEST_RAM_BANKS] = {0},
> > region_base[GUEST_RAM_BANKS],
> > +        bankend[GUEST_RAM_BANKS];
> > +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> > +                  (GUEST_RAM_BANKS + 1)];
> > +    be32 *cells = &regs[0];
> > +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> > +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> > +    unsigned int i, len, nr_regions = 0;
> > +    libxl_dominfo info;
> > +    int offset, rc;
> > +
> > +    offset = fdt_path_offset(fdt, "/hypervisor");
> > +    if (offset < 0)
> > +        return offset;
> > +
> > +    rc = libxl_domain_info(CTX, &info, dom->guest_domid);
> > +    if (rc)
> > +        return rc;
> > +
> > +    if (info.gpaddr_bits > 64)
> > +        return ERROR_INVAL;
> > +
> > +    /*
> > +     * Try to allocate separate 2MB-aligned extended regions from the first
> > +     * and second RAM banks taking into the account the maximum supported
> > +     * guest physical address space size and the amount of memory assigned
> > +     * to the guest.
> > +     */
> > +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> > +        region_base[i] = bankbase[i] +
> > +            ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] <<
> > XC_PAGE_SHIFT);
> > +
> > +        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
> > +        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
> > +        if (bankend[i] > region_base[i])
> > +            region_size[i] = bankend[i] - region_base[i] + 1;
> > +    }
> > +
> > +    /*
> > +     * The region 0 for grant table space must be always present. If we
> > managed
> > +     * to allocate the extended regions then insert them as regions 1...N.
> > +     */
> > +    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> > +              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> > +
> > +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> > +        if (region_size[i] < EXT_REGION_MIN_SIZE)
> > +            continue;
> > +
> > +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
> > +            nr_regions, region_base[i], region_base[i] + region_size[i]);
> > +
> > +        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> > +                  region_base[i], region_size[i]);
> > +        nr_regions++;
> > +    }
> > +
> > +    if (!nr_regions)
> > +        LOG(WARN, "The extended regions cannot be allocated, not enough
> > space");
> > +
> > +    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS +
> > GUEST_ROOT_SIZE_CELLS) *
> > +        (nr_regions + 1);
> > +
> > +    return fdt_setprop(fdt, offset, "reg", regs, len);
> > +}
> > +
> >  int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
> >                                                 uint32_t domid,
> > libxl_domain_config *d_config,
> >                                                 struct xc_dom_image *dom)
> >  {
> >      void *fdt = dom->devicetree_blob;
> > -    int i;
> > +    int i, res;
> >      const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> > 
> >      const struct xc_dom_seg *ramdisk = dom->modules[0].blob ?
> >          &dom->modules[0].seg : NULL;
> > 
> >      if (ramdisk) {
> > -        int chosen, res;
> > +        int chosen;
> >          uint64_t val;
> > 
> >          /* Neither the fdt_path_offset() nor either of the
> > @@ -1109,6 +1201,10 @@ int
> > libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
> > 
> >      }
> > 
> > +    res = finalize_hypervisor_node(gc, dom);
> > +    if (res)
> > +        return res;
> > +
> >      for (i = 0; i < GUEST_RAM_BANKS; i++) {
> >          const uint64_t size = (uint64_t)dom->rambank_size[i] <<
> > XC_PAGE_SHIFT;
> > 
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index d46c61f..96ead3d 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -438,6 +438,11 @@ typedef uint64_t xen_callback_t;
> > 
> >  #define GUEST_RAM_BANKS   2
> > 
> > +/*
> > + * The way to find the extended regions (to be exposed to the guest as
> > unused
> > + * address space) relies on the fact that the regions reserved for the RAM
> > + * below are big enough to also accommodate such regions.
> > + */
> >  #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB
> > */
> >  #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
> > 
> > (END)
> > 
> > 
> -- 
> Regards,
> 
> Oleksandr Tyshchenko
> 

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

[Oleksandr]

Hi Stefano


On 13.10.21 23:24, Stefano Stabellini wrote:
> On Wed, 13 Oct 2021, Oleksandr wrote:
>> On 13.10.21 21:26, Oleksandr wrote:
>>> On 13.10.21 20:07, Julien Grall wrote:
>>>
>>> Hi Julien
>>>
>>>> Hi,
>>>>
>>>> On 13/10/2021 17:44, Oleksandr wrote:
>>>>> On 13.10.21 19:24, Julien Grall wrote:
>>>>>> On 13/10/2021 16:49, Oleksandr wrote:
>>>>>>> On 13.10.21 18:15, Julien Grall wrote:
>>>>>>>> On 13/10/2021 14:46, Oleksandr wrote:
>>>>>>>>> Hi Julien
>>>>>>>> Hi Oleksandr,
>>>>>>> Hi Julien
>>>>>>>
>>>>>>>
>>>>>>> Thank you for the prompt response.
>>>>>>>
>>>>>>>
>>>>>>>>> On 13.10.21 00:43, Oleksandr wrote:
>>>>>>>>> diff --git a/tools/libs/light/libxl_arm.c
>>>>>>>>> b/tools/libs/light/libxl_arm.c
>>>>>>>>> index e3140a6..53ae0f3 100644
>>>>>>>>> --- a/tools/libs/light/libxl_arm.c
>>>>>>>>> +++ b/tools/libs/light/libxl_arm.c
>>>>>>>>> @@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc
>>>>>>>>> *gc, void *fdt,
>>>>>>>>>                                  "xen,xen");
>>>>>>>>>        if (res) return res;
>>>>>>>>>
>>>>>>>>> -    /* reg 0 is grant table space */
>>>>>>>>> +    /*
>>>>>>>>> +     * reg 0 is a placeholder for grant table space, reg 1...N
>>>>>>>>> are
>>>>>>>>> +     * the placeholders for extended regions.
>>>>>>>>> +     */
>>>>>>>>>        res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>>>>>>>>> GUEST_ROOT_SIZE_CELLS,
>>>>>>>>> -                            1,GUEST_GNTTAB_BASE,
>>>>>>>>> GUEST_GNTTAB_SIZE);
>>>>>>>>> +                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0,
>>>>>>>>> 0);
>>>>>>>> Here you are relying on GUEST_RAM_BANKS == 2. I think this is
>>>>>>>> pretty fragile as this may change in the future.
>>>>>>>>
>>>>>>>> fdt_property_regs() is not really suitable for here. I would
>>>>>>>> suggest to create a new helper fdt_property_placeholder() which
>>>>>>>> takes the address cells, size cells and the number of ranges. The
>>>>>>>> function will then create N range that are zeroed.
>>>>>>> You are right. Probably, we could add an assert here for now to be
>>>>>>> triggered if "GUEST_RAM_BANKS != 2"?
>>>>>>> But, if you still think we need to make it with the helper right
>>>>>>> now, I will. However, I don't know how long this will take.
>>>>>> I would prefer if we introduce the helper now. Below a potential
>>>>>> version (not compiled):
>>>>>
>>>>> You wouldn't believe)))
>>>>> I decided to not wait for the answer and re-check. So, I ended up with
>>>>> the similar to what you suggested below. Thank you.
>>>>> Yes, it will work if add missing locals. However, I initially named it
>>>>> exactly as was suggested (fdt_property_placeholder) and got a
>>>>> compilation error, since fdt_property_placeholder is already present.
>>>>> So, I was thinking to choose another name or to even open-code it, but I
>>>>> see you already proposed new name fdt_property_reg_placeholder.
>>>> Ah I didn't realize that libfdt already implemented
>>>> fdt_property_placeholder(). The function sounds perfect for us (and the
>>>> code is nicer :)), but unfortunately this was introdcued only in 2017. So
>>>> it is possible that some distros may not ship the last libfdt.
>>>>
>>>> So for now, I think we need to implement our own. In a follow-up we could
>>>> try to provide a compat layer as we did for other missing fdt_* helpers.
>>>>
>>>> Cheers,
>>>
>>> Well, I hope that I addressed all your comments. If so, I will prepare V7.
>>
>> May I please ask, are you *preliminary* OK with new changes requested by
>> Julien? The main changes are to bring finalize_*() back (hopefully there is a
>> way how to downsize a placeholder), avoid relying on the fact that Bank 0 is
>> always below 4GB, adding a convenient helper to create a placeholder for N
>> ranges plus some code hardening, etc.
> Yes, I am OK with it

Thank you for confirming. I have just pushed V7:

https://lore.kernel.org/xen-devel/1634211645-26912-1-git-send-email-olekstysh@gmail.com/


>
>
>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>> index e3140a6..a780155 100644
>>> --- a/tools/libs/light/libxl_arm.c
>>> +++ b/tools/libs/light/libxl_arm.c
>>> @@ -269,6 +269,21 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
>>>       return fdt_property(fdt, "reg", regs, sizeof(regs));
>>>   }
>>>
>>> +static int fdt_property_reg_placeholder(libxl__gc *gc, void *fdt,
>>> +                                        unsigned int addr_cells,
>>> +                                        unsigned int size_cells,
>>> +                                        unsigned int num_regs)
>>> +{
>>> +    uint32_t regs[num_regs * (addr_cells + size_cells)];
>>> +    be32 *cells = &regs[0];
>>> +    unsigned int i;
>>> +
>>> +    for (i = 0; i < num_regs; i++)
>>> +        set_range(&cells, addr_cells, size_cells, 0, 0);
>>> +
>>> +    return fdt_property(fdt, "reg", regs, sizeof(regs));
>>> +}
>>> +
>>>   static int make_root_properties(libxl__gc *gc,
>>>                                   const libxl_version_info *vers,
>>>                                   void *fdt)
>>> @@ -615,9 +630,13 @@ 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);
>>> +    /*
>>> +     * reg 0 is a placeholder for grant table space, reg 1...N are
>>> +     * the placeholders for extended regions.
>>> +     */
>>> +    res = fdt_property_reg_placeholder(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>>> +                                       GUEST_ROOT_SIZE_CELLS,
>>> +                                       GUEST_RAM_BANKS + 1);
>>>       if (res) return res;
>>>
>>>       /*
>>> @@ -1069,20 +1088,93 @@ static void finalise_one_node(libxl__gc *gc, void
>>> *fdt, const char *uname,
>>>       }
>>>   }
>>>
>>> +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
>>> +
>>> +#define EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
>>> +
>>> +static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image
>>> *dom)
>>> +{
>>> +    void *fdt = dom->devicetree_blob;
>>> +    uint64_t region_size[GUEST_RAM_BANKS] = {0},
>>> region_base[GUEST_RAM_BANKS],
>>> +        bankend[GUEST_RAM_BANKS];
>>> +    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
>>> +                  (GUEST_RAM_BANKS + 1)];
>>> +    be32 *cells = &regs[0];
>>> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>>> +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
>>> +    unsigned int i, len, nr_regions = 0;
>>> +    libxl_dominfo info;
>>> +    int offset, rc;
>>> +
>>> +    offset = fdt_path_offset(fdt, "/hypervisor");
>>> +    if (offset < 0)
>>> +        return offset;
>>> +
>>> +    rc = libxl_domain_info(CTX, &info, dom->guest_domid);
>>> +    if (rc)
>>> +        return rc;
>>> +
>>> +    if (info.gpaddr_bits > 64)
>>> +        return ERROR_INVAL;
>>> +
>>> +    /*
>>> +     * Try to allocate separate 2MB-aligned extended regions from the first
>>> +     * and second RAM banks taking into the account the maximum supported
>>> +     * guest physical address space size and the amount of memory assigned
>>> +     * to the guest.
>>> +     */
>>> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
>>> +        region_base[i] = bankbase[i] +
>>> +            ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] <<
>>> XC_PAGE_SHIFT);
>>> +
>>> +        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
>>> +        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
>>> +        if (bankend[i] > region_base[i])
>>> +            region_size[i] = bankend[i] - region_base[i] + 1;
>>> +    }
>>> +
>>> +    /*
>>> +     * The region 0 for grant table space must be always present. If we
>>> managed
>>> +     * to allocate the extended regions then insert them as regions 1...N.
>>> +     */
>>> +    set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>>> +              GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>>> +
>>> +    for (i = 0; i < GUEST_RAM_BANKS; i++) {
>>> +        if (region_size[i] < EXT_REGION_MIN_SIZE)
>>> +            continue;
>>> +
>>> +        LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
>>> +            nr_regions, region_base[i], region_base[i] + region_size[i]);
>>> +
>>> +        set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>>> +                  region_base[i], region_size[i]);
>>> +        nr_regions++;
>>> +    }
>>> +
>>> +    if (!nr_regions)
>>> +        LOG(WARN, "The extended regions cannot be allocated, not enough
>>> space");
>>> +
>>> +    len = sizeof(regs[0]) * (GUEST_ROOT_ADDRESS_CELLS +
>>> GUEST_ROOT_SIZE_CELLS) *
>>> +        (nr_regions + 1);
>>> +
>>> +    return fdt_setprop(fdt, offset, "reg", regs, len);
>>> +}
>>> +
>>>   int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>>>                                                  uint32_t domid,
>>> libxl_domain_config *d_config,
>>>                                                  struct xc_dom_image *dom)
>>>   {
>>>       void *fdt = dom->devicetree_blob;
>>> -    int i;
>>> +    int i, res;
>>>       const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>>>
>>>       const struct xc_dom_seg *ramdisk = dom->modules[0].blob ?
>>>           &dom->modules[0].seg : NULL;
>>>
>>>       if (ramdisk) {
>>> -        int chosen, res;
>>> +        int chosen;
>>>           uint64_t val;
>>>
>>>           /* Neither the fdt_path_offset() nor either of the
>>> @@ -1109,6 +1201,10 @@ int
>>> libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>>>
>>>       }
>>>
>>> +    res = finalize_hypervisor_node(gc, dom);
>>> +    if (res)
>>> +        return res;
>>> +
>>>       for (i = 0; i < GUEST_RAM_BANKS; i++) {
>>>           const uint64_t size = (uint64_t)dom->rambank_size[i] <<
>>> XC_PAGE_SHIFT;
>>>
>>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>>> index d46c61f..96ead3d 100644
>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -438,6 +438,11 @@ typedef uint64_t xen_callback_t;
>>>
>>>   #define GUEST_RAM_BANKS   2
>>>
>>> +/*
>>> + * The way to find the extended regions (to be exposed to the guest as
>>> unused
>>> + * address space) relies on the fact that the regions reserved for the RAM
>>> + * below are big enough to also accommodate such regions.
>>> + */
>>>   #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB
>>> */
>>>   #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
>>>
>>> (END)
>>>
>>>
>> -- 
>> Regards,
>>
>> Oleksandr Tyshchenko

-- 
Regards,

Oleksandr Tyshchenko