Re: [PATCH v4 1/5] xen/domctl, tools: Introduce a new domctl to get guest memory map

[Henry Wang]

Hi Jan,

On 4/18/2024 8:37 PM, Jan Beulich wrote:
On 09.04.2024 06:53, Henry Wang wrote:
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -697,6 +697,43 @@ int xc_domain_setmaxmem(xc_interface *xch,
      return do_domctl(xch, &domctl);
  }
  
+int xc_get_domain_mem_map(xc_interface *xch, uint32_t domid,
+                          struct xen_mem_region mem_regions[],
+                          uint32_t *nr_regions)
+{
+    int rc;
+    uint32_t nr = *nr_regions;
+    struct xen_domctl domctl = {
+        .cmd         = XEN_DOMCTL_get_mem_map,
+        .domain      = domid,
+        .u.mem_map = {
+            .nr_mem_regions = nr,
+        },
+    };
+
+    DECLARE_HYPERCALL_BOUNCE(mem_regions, sizeof(xen_mem_region_t) * nr,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+    if ( !mem_regions || xc_hypercall_bounce_pre(xch, mem_regions) || nr < 1 )
Why the nr < 1 part? For a caller to size the necessary buffer, it may want
to pass in 0 (and a NULL buffer pointer) first.

I will drop this nr < 1 part.

@@ -176,6 +175,33 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
domain *d,
  
          return rc;
      }
+    case XEN_DOMCTL_get_mem_map:
+    {
+        int rc = 0;
+        uint32_t nr_regions;
unsigned int (see ./CODING_STYLE)?

Ok, I will use unsigned int.

--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -223,6 +223,13 @@ typedef uint64_t xen_pfn_t;
   */
  #define XEN_LEGACY_MAX_VCPUS 1
  
+/*
+ * Maximum number of memory map regions for guest memory layout.
+ * Used by XEN_DOMCTL_get_mem_map, currently there is only one region
+ * for the guest magic pages.
+ */
+#define XEN_MAX_MEM_REGIONS 1
Why is this in the public header? I can only find Xen-internal uses.

It will also be used in the init-dom0less app which is the toolstack side.

--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -946,6 +946,33 @@ struct xen_domctl_paging_mempool {
      uint64_aligned_t size; /* Size in bytes. */
  };
  
+#ifndef XEN_MAX_MEM_REGIONS
+#define XEN_MAX_MEM_REGIONS 1
+#endif
+
+struct xen_mem_region {
+    uint64_aligned_t start;
+    uint64_aligned_t size;
+#define XEN_MEM_REGION_DEFAULT    0
I can't spot any use of this. What's its purpose?

I can drop it. My original intention is to define a default type since 
the struct arch_domain should be zalloc-ed.

+#define XEN_MEM_REGION_MAGIC      1
+    uint32_t         type;
+    /* Must be zero */
+    uint32_t         pad;
This being OUT only, I don't think the comment makes sense. I'd omit it
completely; if you absolutely want one, please say "will" instead of "must".

Sure, I will follow your suggestion. Thanks.

Kind regards,
Henry



Jan