Re: [PATCH v2 5/5] xen/arm: Support ARM standard PV time for domains created via toolstack

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 7/5/25 10:27, Koichiro Den wrote:
> Implement ARM DEN 0057A PV time support for domains created via the
> toolstack, utilizing the newly introduced XENMAPSPACE_pv_time.
> 
> Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> ---
>  tools/libs/light/libxl_arm.c | 185 ++++++++++++++++++++++++++++-------
>  xen/arch/arm/mm.c            |  14 +++
>  xen/include/public/memory.h  |   1 +
>  3 files changed, 167 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 4a19a8d22bdf..33251520c07a 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -684,6 +684,40 @@ static int make_memory_nodes(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>  
> +static int make_resv_memory_node(libxl__gc *gc, void *fdt,
> +                                 const struct xc_dom_image *dom)
> +{
> +    int res;
> +
> +    if (strcmp(dom->guest_type, "xen-3.0-aarch64"))
> +        /*
> +         * The stolen time shared memory region for ARM DEN 0057A is 
> currently
> +         * the only user of /reserved-memory node when a domain is created 
> via
> +         * the toolstack, and it requires both the hypervisor and the domain 
> to
> +         * be AArch64.
> +         */
> +        return 0;
> +
> +    res = fdt_begin_node(fdt, "reserved-memory");
> +    if (res) return res;
> +
> +    res = fdt_property_cell(fdt, "#address-cells", GUEST_ROOT_ADDRESS_CELLS);
> +    if (res) return res;
> +
> +    res = fdt_property_cell(fdt, "#size-cells", GUEST_ROOT_SIZE_CELLS);
> +    if (res) return res;
> +
> +    /* reg 0 is a placeholder for PV time region */
> +    res = fdt_property_reg_placeholder(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
> +                                       GUEST_ROOT_SIZE_CELLS, 1);
> +    if (res) return res;
> +
> +    res = fdt_end_node(fdt);
> +    if (res) return res;
> +
> +    return 0;
> +}
> +
>  static int make_gicv2_node(libxl__gc *gc, void *fdt,
>                             uint64_t gicd_base, uint64_t gicd_size,
>                             uint64_t gicc_base, uint64_t gicc_size)
> @@ -1352,6 +1386,7 @@ next_resize:
>          FDT( make_psci_node(gc, fdt) );
>  
>          FDT( make_memory_nodes(gc, fdt, dom) );
> +        FDT( make_resv_memory_node(gc, fdt, dom) );
>  
>          switch (info->arch_arm.gic_version) {
>          case LIBXL_GIC_VERSION_V2:
> @@ -1519,6 +1554,9 @@ static void finalise_one_node(libxl__gc *gc, void *fdt, 
> const char *uname,
>  
>  #define EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
>  
> +/* As per ARM DEN 0057A, stolen time memory regions are 64-byte aligned */
> +#define PV_REGIONS_PER_PAGE  (XC_PAGE_SIZE / 64)
> +
>  static int compare_iomem(const void *a, const void *b)
>  {
>      const libxl_iomem_range *x = a, *y = b;
> @@ -1530,24 +1568,92 @@ static int compare_iomem(const void *a, const void *b)
>      return 0;
>  }
>  
> -static int finalize_hypervisor_node(libxl__gc *gc,
> -                                    libxl_domain_build_info *b_info,
> -                                    struct xc_dom_image *dom)
> +static int get_pv_region(libxl_domain_build_info *b_info,
> +                         struct xc_dom_image *dom,
> +                         uint64_t *start, uint64_t end,
> +                         uint64_t *region_base, uint64_t *region_size)
> +{
> +    unsigned int npages = DIV_ROUNDUP(b_info->max_vcpus, 
> PV_REGIONS_PER_PAGE);
> +    unsigned int len = npages * XC_PAGE_SIZE;
> +    uint32_t domid = dom->guest_domid;
> +    xc_interface *xch = dom->xch;
> +    unsigned long idx = 0;
> +    uint64_t size;
> +    int rc;
> +
> +    if (*start >= end)
> +        return -1;
> +    size = end - *start;
> +    if (size < len)
> +        return -1;
> +
> +    for (; npages; npages--, idx++) {
> +        rc = xc_domain_add_to_physmap(xch, domid, XENMAPSPACE_pv_time, idx,
> +                                      (*start >> XC_PAGE_SHIFT) + idx);
> +        if (rc)
> +            return rc;
> +    }
> +
> +    region_base[0] = *start;
> +    region_size[0] = len;
> +    *start += len;
> +    return 0;
> +}
> +
> +static void get_ext_region(uint64_t start, uint64_t end, uint64_t 
> *region_base,
> +                           uint64_t *region_size, unsigned int *nr_regions)
> +{
> +    uint64_t size;
> +
> +    start = ALIGN_UP_TO_2MB(start);
> +    if (start >= end)
> +        return;
> +
> +    size = end - start;
> +    if (size < EXT_REGION_MIN_SIZE)
> +        return;
> +
> +    region_base[*nr_regions] = start;
> +    region_size[*nr_regions] = size;
> +    (*nr_regions)++;
> +}
> +
> +static int finalize_extra_regions(libxl__gc *gc,
> +                                  libxl_domain_build_info *b_info,
> +                                  struct xc_dom_image *dom)
>  {
>      void *fdt = dom->devicetree_blob;
> -    uint64_t region_base[MAX_NR_EXT_REGIONS], 
> region_size[MAX_NR_EXT_REGIONS];
> -    uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> +
> +    /* For extended regions */
> +    uint64_t ext_region_base[MAX_NR_EXT_REGIONS], 
> ext_region_size[MAX_NR_EXT_REGIONS];
> +    uint32_t ext_regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
>                    (MAX_NR_EXT_REGIONS + 1)];
> -    be32 *cells = &regs[0];
> +    be32 *ext_cells = &ext_regs[0];
> +    int hyp_offset;
> +
> +    /* For pv regions */
> +    uint64_t pv_region_base[1], pv_region_size[1];
> +    uint32_t pv_regs[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
> +    be32 *pv_cells = &pv_regs[0];
> +    int resv_offset;
> +
>      const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>      const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
>      unsigned int i, j, len, nr_regions = 0;
> +    bool pv_region_pending = true;
>      libxl_dominfo info;
> -    int offset, rc;
> +    int rc;
>  
> -    offset = fdt_path_offset(fdt, "/hypervisor");
> -    if (offset < 0)
> -        return offset;
> +    resv_offset = fdt_path_offset(fdt, "/reserved-memory");
> +    if (!strcmp(dom->guest_type, "xen-3.0-aarch64")) {
> +        if (resv_offset < 0)
> +            return resv_offset;
> +    } else
> +        pv_region_pending = false;
> +
> +    hyp_offset = fdt_path_offset(fdt, "/hypervisor");
> +    if (hyp_offset < 0)
> +        return hyp_offset;
>  
>      rc = libxl_domain_info(CTX, &info, dom->guest_domid);
>      if (rc)
> @@ -1572,8 +1678,7 @@ static int finalize_hypervisor_node(libxl__gc *gc,
>          } unallocated;
>          uint64_t unallocated_size = 0;
>  
> -        unallocated.start = bankbase[i] +
> -            ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << XC_PAGE_SHIFT);
> +        unallocated.start = bankbase[i] + ((uint64_t)dom->rambank_size[i] << 
> XC_PAGE_SHIFT);
>  
>          unallocated.end = ~0ULL >> (64 - info.gpaddr_bits);
>          unallocated.end = min(unallocated.end, bankbase[i] + banksize[i] - 
> 1);
> @@ -1581,7 +1686,7 @@ static int finalize_hypervisor_node(libxl__gc *gc,
>          if (unallocated.end >= unallocated.start)
>              unallocated_size = unallocated.end - unallocated.start + 1;
>  
> -        if (unallocated_size < EXT_REGION_MIN_SIZE)
> +        if (unallocated_size <= 0)
>              continue;
>  
>          /* Exclude iomem */
> @@ -1605,14 +1710,14 @@ static int finalize_hypervisor_node(libxl__gc *gc,
>                      if (unallocated.start > unallocated.end)
>                          break;
>                  } else {
> -                    uint64_t size = iomem.start - unallocated.start;
> -
> -                    if (size >= EXT_REGION_MIN_SIZE) {
> -                        region_base[nr_regions] = unallocated.start;
> -                        region_size[nr_regions] = size;
> -                        nr_regions++;
> +                    if (pv_region_pending) {
> +                        rc = get_pv_region(b_info, dom, &unallocated.start, 
> iomem.start,
> +                                           pv_region_base, pv_region_size);
> +                        if (!rc)
> +                            pv_region_pending = false;
>                      }
> -
> +                    get_ext_region(unallocated.start, iomem.start, 
> ext_region_base,
> +                                   ext_region_size, &nr_regions);
>                      unallocated.start = iomem.end + 1;
>  
>                      if (unallocated.start > unallocated.end)
> @@ -1624,38 +1729,52 @@ static int finalize_hypervisor_node(libxl__gc *gc,
>          if (unallocated.end >= unallocated.start
>              && nr_regions < MAX_NR_EXT_REGIONS)
>          {
> -            uint64_t size = unallocated.end - unallocated.start + 1;
> -
> -            if (size >= EXT_REGION_MIN_SIZE) {
> -                region_base[nr_regions] = unallocated.start;
> -                region_size[nr_regions] = size;
> -                nr_regions++;
> +            if (pv_region_pending) {
> +                rc = get_pv_region(b_info, dom, &unallocated.start, 
> unallocated.end,
> +                                   pv_region_base, pv_region_size);
> +                if (!rc)
> +                    pv_region_pending = false;
>              }
> +            get_ext_region(unallocated.start, unallocated.end, 
> ext_region_base,
> +                           ext_region_size, &nr_regions);
>          }
>      }
>  
> +    if (!strcmp(dom->guest_type, "xen-3.0-aarch64")) {
> +        if (pv_region_pending) {
> +            LOG(ERROR, "The PV time region cannot be allocated, not enough 
> space");
> +            return ERROR_FAIL;
> +        }
> +        set_range(&pv_cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +                  pv_region_base[0], pv_region_size[0]);
> +        len = sizeof(pv_regs[0]) * (GUEST_ROOT_ADDRESS_CELLS + 
> GUEST_ROOT_SIZE_CELLS);
> +        rc = fdt_setprop(fdt, resv_offset, "reg", pv_regs, len);

I can appreciate the effort that it took to get this working. However,
given the relatively small size of PV time region, is there a reason
that you chose to allocate it from unallocated guest RAM rather than
reserving a new region/space in xen/include/public/arch-arm.h? Doing so
may also simplify the dom0less case.

E.g.:

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index e2412a17474e..681e3d4778ba 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -466,6 +466,13 @@ typedef uint64_t xen_callback_t;
 #define GUEST_VPCI_MEM_ADDR                 xen_mk_ullong(0x23000000)
 #define GUEST_VPCI_MEM_SIZE                 xen_mk_ullong(0x10000000)
 
+/* Current supported guest VCPUs */
+#define GUEST_MAX_VCPUS 128
+
+/* PV time region */
+#define GUEST_PVTIME_BASE xen_mk_ullong(0x37000000)
+#define GUEST_PVTIME_SIZE xen_mk_ullong(GUEST_MAX_VCPUS * 64)
+
 /*
  * 16MB == 4096 pages reserved for guest to use as a region to map its
  * grant table in.
@@ -501,9 +508,6 @@ 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 }
 
-/* Current supported guest VCPUs */
-#define GUEST_MAX_VCPUS 128
-
 /* Interrupts */
 
 #define GUEST_TIMER_VIRT_PPI    27