[PATCH V4 00/10] Domain on Static Allocation

[PATCH V4 00/10] Domain on Static Allocation

[Penny Zheng]

Static Allocation refers to system or sub-system(domains) for which memory
areas are pre-defined by configuration using physical address ranges.
Those pre-defined memory, -- Static Memory, as parts of RAM reserved in the
beginning, shall never go to heap allocator or boot allocator for any use.

Domain on Static Allocation is supported through device tree property
`xen,static-mem` specifying reserved RAM banks as this domain's guest RAM.
The size of address-cells/size-cells must be defined in
"#xen,static-mem-address-cells" and "#xen,static-mem-size-cells".

This Patch Serie only talks about Domain on Static Allocation.

Looking into related [design link](
https://lists.xenproject.org/archives/html/xen-devel/2021-05/msg00882.html)
for more details.

The whole design is about Static Allocation and 1:1 direct-map, and this
Patch Serie only covers parts of it, which are Domain on Static Allocation.
Other features will be delievered through different patch series.

---
changes in v4:
- move the option CONFIG_STATIC_MEMORY to common code, and with Arm
"select"ing it
- replace round_pg{down,up}() with PFN_DOWN()/PFN_UP()
- in all cases where order-0 pages get passed, prefer using new assign_pages
to pass literal 1
- reconstruct the order of assign_pages parameters
- moving tlb/cache flush outside of the locked region, considering XSA-364
and reducing the amount of work happening with the heap_lock held
- remove MEMF_no_refcount case
- make acquire_staticmem_pages/acquire_domstatic_pages being __init

TODO:
- reboot domain on static allocation
- Implement all memory-ops(hypercalls) regarding domain on static allocation
to balloon in/out memory
- asynchronously scrubbing PGC_reserved pages
- consider domain on static allocation on NUMA-support scenario

Penny Zheng (10):
  xen/arm: introduce domain on Static Allocation
  xen/arm: introduce new helper device_tree_get_meminfo
  xen/arm: handle static memory in dt_unreserved_regions
  xen: introduce mark_page_free
  xen/arm: static memory initialization
  xen/arm: introduce PGC_reserved
  xen: re-define assign_pages and introduce assign_page
  xen/arm: introduce acquire_staticmem_pages and acquire_domstatic_pages
  xen/arm: check "xen,static-mem" property during domain construction
  xen/arm: introduce allocate_static_memory

 docs/misc/arm/device-tree/booting.txt |  40 +++++
 xen/arch/arm/Kconfig                  |   1 +
 xen/arch/arm/bootfdt.c                |  89 +++++++---
 xen/arch/arm/domain_build.c           | 170 +++++++++++++++++-
 xen/arch/arm/setup.c                  |  71 ++++++--
 xen/arch/x86/pv/dom0_build.c          |   2 +-
 xen/common/Kconfig                    |   3 +
 xen/common/grant_table.c              |   2 +-
 xen/common/memory.c                   |   4 +-
 xen/common/page_alloc.c               | 243 ++++++++++++++++++++------
 xen/include/asm-arm/mm.h              |   3 +
 xen/include/asm-arm/setup.h           |   2 +
 xen/include/xen/mm.h                  |  15 ++
 13 files changed, 542 insertions(+), 103 deletions(-)

-- 
2.25.1

[PATCH V4 01/10] xen/arm: introduce domain on Static Allocation

[Penny Zheng]

Static Allocation refers to system or sub-system(domains) for which memory
areas are pre-defined by configuration using physical address ranges.
Those pre-defined memory, -- Static Memory, as parts of RAM reserved in the
beginning, shall never go to heap allocator or boot allocator for any use.

Domains on Static Allocation is supported through device tree property
`xen,static-mem` specifying reserved RAM banks as this domain's guest RAM.
By default, they shall be mapped to the fixed guest RAM address
`GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`.

This patch introduces this new `xen,static-mem` feature, and also documents
and parses this new attribute at boot time and stores related info in
static_mem for later initialization.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 docs/misc/arm/device-tree/booting.txt | 40 +++++++++++++++++++++
 xen/arch/arm/bootfdt.c                | 51 +++++++++++++++++++++++++++
 xen/include/asm-arm/setup.h           |  2 ++
 3 files changed, 93 insertions(+)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 5243bc7fd3..2a1ddca29b 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -268,3 +268,43 @@ The DTB fragment is loaded at 0xc000000 in the example above. It should
 follow the convention explained in docs/misc/arm/passthrough.txt. The
 DTB fragment will be added to the guest device tree, so that the guest
 kernel will be able to discover the device.
+
+
+Static Allocation
+=============
+
+Static Allocation refers to system or sub-system(domains) for which memory
+areas are pre-defined by configuration using physical address ranges.
+Those pre-defined memory, -- Static Memory, as parts of RAM reserved in the
+beginning, shall never go to heap allocator or boot allocator for any use.
+
+Domains on Static Allocation is supported through static memory property,
+defined under according /domUx in the name of "xen,static-mem", which are
+specifying physical RAM as this domain's guest RAM.
+The size of address-cells/size-cells must be defined in
+"#xen,static-mem-address-cells" and "#xen,static-mem-size-cells".
+
+On memory allocation, these pre-defined static memory ranges shall be
+firstly mapped to the fixed guest bank "GUEST_RAM0". Until it exhausts the
+`GUEST_RAM0_SIZE`, then it will seek to `GUEST_RAM1_BASE`, and so on.
+`GUEST_RAM0` may take up several pre-defined physical RAM regions.
+
+The dtb property should look like as follows:
+
+    / {
+        chosen {
+            domU1 {
+                compatible = "xen,domain";
+                #address-cells = <0x2>;
+                #size-cells = <0x2>;
+                cpus = <2>;
+                #xen,static-mem-address-cells = <0x1>;
+                #xen,static-mem-size-cells = <0x1>;
+                xen,static-mem = <0x30000000 0x20000000>;
+                ...
+            };
+        };
+    };
+
+DomU1 will have a static memory of 512MB reserved from the physical address
+0x30000000 to 0x50000000.
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 476e32e0f5..d2714446e1 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -193,6 +193,55 @@ static int __init process_reserved_memory_node(const void *fdt, int node,
     return 0;
 }
 
+static int __init process_static_memory(const void *fdt, int node, void *data)
+{
+    int i = 0, banks;
+    const __be32 *cell;
+    paddr_t start, size;
+    u32 address_cells, size_cells, reg_cells;
+    struct meminfo *mem = data;
+    const struct fdt_property *prop;
+
+
+    address_cells = device_tree_get_u32(fdt, node,
+                                        "#xen,static-mem-address-cells", 0);
+    size_cells = device_tree_get_u32(fdt, node,
+                                     "#xen,static-mem-size-cells", 0);
+    if ( (address_cells == 0) || (size_cells == 0) )
+    {
+         printk("Missing \"#xen,static-mem-address-cell\" or "
+                 "\"#xen,static-mem-address-cell\".\n");
+         return -EINVAL;
+    }
+    reg_cells = address_cells + size_cells;
+
+    prop = fdt_get_property(fdt, node, "xen,static-mem", NULL);
+    /*
+     * Static memory shall belong to a specific domain, that is,
+     * its node `domUx` has compatible string "xen,domain".
+     */
+    if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
+    {
+        printk("xen,static-mem property can only be located under /domUx node.\n");
+        return -EINVAL;
+    }
+
+    cell = (const __be32 *)prop->data;
+    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
+
+    for ( ; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
+    {
+        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
+        mem->bank[mem->nr_banks].start = start;
+        mem->bank[mem->nr_banks].size = size;
+        mem->nr_banks++;
+    }
+
+    if ( i < banks )
+        return -ENOSPC;
+    return 0;
+}
+
 static int __init process_reserved_memory(const void *fdt, int node,
                                           const char *name, int depth,
                                           u32 address_cells, u32 size_cells)
@@ -346,6 +395,8 @@ static int __init early_scan_node(const void *fdt,
         process_multiboot_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
         process_chosen_node(fdt, node, name, address_cells, size_cells);
+    else if ( depth == 2 && fdt_get_property(fdt, node, "xen,static-mem", NULL) )
+        process_static_memory(fdt, node, &bootinfo.static_mem);
 
     if ( rc < 0 )
         printk("fdt: node `%s': parsing failed\n", name);
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index c4b6af6029..e076329fc4 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -74,6 +74,8 @@ struct bootinfo {
 #ifdef CONFIG_ACPI
     struct meminfo acpi;
 #endif
+    /* Static Memory */
+    struct meminfo static_mem;
 };
 
 extern struct bootinfo bootinfo;
-- 
2.25.1

[PATCH V4 02/10] xen/arm: introduce new helper device_tree_get_meminfo

[Penny Zheng]

A few functions iterate over the device tree property to get memory info,
like "reg" or "xen,static-mem", so this commit creates a new helper
device_tree_get_meminfo to extract the
common codes.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/arch/arm/bootfdt.c | 104 +++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 62 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index d2714446e1..04210684c9 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -63,6 +63,44 @@ void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
     *size = dt_next_cell(size_cells, cell);
 }
 
+static int __init device_tree_get_meminfo(const void *fdt, int node,
+                                          const char *prop_name,
+                                          u32 address_cells, u32 size_cells,
+                                          void *data)
+{
+    const struct fdt_property *prop;
+    unsigned int i, banks;
+    const __be32 *cell;
+    u32 reg_cells = address_cells + size_cells;
+    paddr_t start, size;
+    struct meminfo *mem = data;
+
+    prop = fdt_get_property(fdt, node, prop_name, NULL);
+    if ( !prop )
+        return -ENOENT;
+
+    cell = (const __be32 *)prop->data;
+    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
+
+    for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
+    {
+        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
+        /* Some DT may describe empty bank, ignore them */
+        if ( !size )
+            continue;
+        mem->bank[mem->nr_banks].start = start;
+        mem->bank[mem->nr_banks].size = size;
+        mem->nr_banks++;
+    }
+
+    if ( i < banks )
+    {
+        printk("Warning: Max number of supported memory regions reached.\n");
+        return -ENOSPC;
+    }
+    return 0;
+}
+
 u32 __init device_tree_get_u32(const void *fdt, int node,
                                const char *prop_name, u32 dflt)
 {
@@ -139,14 +177,6 @@ static int __init process_memory_node(const void *fdt, int node,
                                       u32 address_cells, u32 size_cells,
                                       void *data)
 {
-    const struct fdt_property *prop;
-    int i;
-    int banks;
-    const __be32 *cell;
-    paddr_t start, size;
-    u32 reg_cells = address_cells + size_cells;
-    struct meminfo *mem = data;
-
     if ( address_cells < 1 || size_cells < 1 )
     {
         printk("fdt: node `%s': invalid #address-cells or #size-cells",
@@ -154,27 +184,7 @@ static int __init process_memory_node(const void *fdt, int node,
         return -EINVAL;
     }
 
-    prop = fdt_get_property(fdt, node, "reg", NULL);
-    if ( !prop )
-        return -ENOENT;
-
-    cell = (const __be32 *)prop->data;
-    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
-
-    for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
-    {
-        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
-        /* Some DT may describe empty bank, ignore them */
-        if ( !size )
-            continue;
-        mem->bank[mem->nr_banks].start = start;
-        mem->bank[mem->nr_banks].size = size;
-        mem->nr_banks++;
-    }
-
-    if ( i < banks )
-        return -ENOSPC;
-    return 0;
+    return device_tree_get_meminfo(fdt, node, "reg", address_cells, size_cells, data);
 }
 
 static int __init process_reserved_memory_node(const void *fdt, int node,
@@ -195,13 +205,7 @@ static int __init process_reserved_memory_node(const void *fdt, int node,
 
 static int __init process_static_memory(const void *fdt, int node, void *data)
 {
-    int i = 0, banks;
-    const __be32 *cell;
-    paddr_t start, size;
-    u32 address_cells, size_cells, reg_cells;
-    struct meminfo *mem = data;
-    const struct fdt_property *prop;
-
+    u32 address_cells, size_cells;
 
     address_cells = device_tree_get_u32(fdt, node,
                                         "#xen,static-mem-address-cells", 0);
@@ -213,33 +217,9 @@ static int __init process_static_memory(const void *fdt, int node, void *data)
                  "\"#xen,static-mem-address-cell\".\n");
          return -EINVAL;
     }
-    reg_cells = address_cells + size_cells;
-
-    prop = fdt_get_property(fdt, node, "xen,static-mem", NULL);
-    /*
-     * Static memory shall belong to a specific domain, that is,
-     * its node `domUx` has compatible string "xen,domain".
-     */
-    if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
-    {
-        printk("xen,static-mem property can only be located under /domUx node.\n");
-        return -EINVAL;
-    }
-
-    cell = (const __be32 *)prop->data;
-    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
-
-    for ( ; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
-    {
-        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
-        mem->bank[mem->nr_banks].start = start;
-        mem->bank[mem->nr_banks].size = size;
-        mem->nr_banks++;
-    }
 
-    if ( i < banks )
-        return -ENOSPC;
-    return 0;
+    return device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
+                                   size_cells, data);
 }
 
 static int __init process_reserved_memory(const void *fdt, int node,
-- 
2.25.1

[PATCH V4 03/10] xen/arm: handle static memory in dt_unreserved_regions

[Penny Zheng]

static memory regions overlap with memory nodes. The
overlapping memory is reserved-memory and should be
handled accordingly:
dt_unreserved_regions should skip these regions the
same way they are already skipping mem-reserved regions.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/arch/arm/setup.c | 47 ++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 63a908e325..f569134317 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -200,6 +200,13 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
                                          int first)
 {
     int i, nr = fdt_num_mem_rsv(device_tree_flattened);
+    /*
+     * There are two types of reserved memory stored in bootinfo, one defines
+     * in /reserved-memory node, the other refers to domain on static allocation
+     * through "xen,static-mem" property.
+     */
+    int nr_rsv_type = 2, t = 0, prev_nr;
+    struct meminfo *rsv_type[2] = {&bootinfo.reserved_mem, &bootinfo.static_mem};
 
     for ( i = first; i < nr ; i++ )
     {
@@ -219,26 +226,32 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
         }
     }
 
-    /*
-     * i is the current bootmodule we are evaluating across all possible
-     * kinds.
-     *
-     * When retrieving the corresponding reserved-memory addresses
-     * below, we need to index the bootinfo.reserved_mem bank starting
-     * from 0, and only counting the reserved-memory modules. Hence,
-     * we need to use i - nr.
-     */
-    for ( ; i - nr < bootinfo.reserved_mem.nr_banks; i++ )
+    prev_nr = nr;
+    while ( t < nr_rsv_type )
     {
-        paddr_t r_s = bootinfo.reserved_mem.bank[i - nr].start;
-        paddr_t r_e = r_s + bootinfo.reserved_mem.bank[i - nr].size;
-
-        if ( s < r_e && r_s < e )
+        /*
+         * i is the current bootmodule we are evaluating across all possible
+         * kinds.
+         *
+         * When retrieving the corresponding reserved-memory addresses
+         * below, we need to index the reserved mem bank starting
+         * from 0, and only counting the reserved-memory modules. Hence,
+         * we need to use i - prev_nr.
+         */
+        i = i - prev_nr;
+        for ( ; i < rsv_type[t]->nr_banks; i++ )
         {
-            dt_unreserved_regions(r_e, e, cb, i + 1);
-            dt_unreserved_regions(s, r_s, cb, i + 1);
-            return;
+            paddr_t r_s = rsv_type[t]->bank[i].start;
+            paddr_t r_e = r_s + rsv_type[t]->bank[i].size;
+
+            if ( s < r_e && r_s < e )
+            {
+                dt_unreserved_regions(r_e, e, cb, i + 1);
+                dt_unreserved_regions(s, r_s, cb, i + 1);
+                return;
+            }
         }
+        prev_nr = rsv_type[t++]->nr_banks;
     }
 
     cb(s, e);
-- 
2.25.1

[PATCH V4 04/10] xen: introduce mark_page_free

[Penny Zheng]

This commit defines a new helper mark_page_free to extract common code,
like following the same cache/TLB coherency policy, between free_heap_pages
and the new function free_staticmem_pages, which will be introduced later.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 xen/common/page_alloc.c | 89 ++++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 958ba0cd92..a3ee5eca9e 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1376,6 +1376,53 @@ bool scrub_free_pages(void)
     return node_to_scrub(false) != NUMA_NO_NODE;
 }
 
+static void mark_page_free(struct page_info *pg, mfn_t mfn)
+{
+    ASSERT(mfn_x(mfn) == mfn_x(page_to_mfn(pg)));
+
+    /*
+     * Cannot assume that count_info == 0, as there are some corner cases
+     * where it isn't the case and yet it isn't a bug:
+     *  1. page_get_owner() is NULL
+     *  2. page_get_owner() is a domain that was never accessible by
+     *     its domid (e.g., failed to fully construct the domain).
+     *  3. page was never addressable by the guest (e.g., it's an
+     *     auto-translate-physmap guest and the page was never included
+     *     in its pseudophysical address space).
+     * In all the above cases there can be no guest mappings of this page.
+     */
+    switch ( pg->count_info & PGC_state )
+    {
+    case PGC_state_inuse:
+        BUG_ON(pg->count_info & PGC_broken);
+        pg->count_info = PGC_state_free;
+        break;
+
+    case PGC_state_offlining:
+        pg->count_info = (pg->count_info & PGC_broken) |
+                         PGC_state_offlined;
+        tainted = 1;
+        break;
+
+    default:
+        printk(XENLOG_ERR
+               "pg MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n",
+               mfn_x(mfn),
+               pg->count_info, pg->v.free.order,
+               pg->u.free.val, pg->tlbflush_timestamp);
+        BUG();
+    }
+
+    /* If a page has no owner it will need no safety TLB flush. */
+    pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL);
+    if ( pg->u.free.need_tlbflush )
+        page_set_tlbflush_timestamp(pg);
+
+    /* This page is not a guest frame any more. */
+    page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */
+    set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY);
+}
+
 /* Free 2^@order set of pages. */
 static void free_heap_pages(
     struct page_info *pg, unsigned int order, bool need_scrub)
@@ -1392,47 +1439,7 @@ static void free_heap_pages(
 
     for ( i = 0; i < (1 << order); i++ )
     {
-        /*
-         * Cannot assume that count_info == 0, as there are some corner cases
-         * where it isn't the case and yet it isn't a bug:
-         *  1. page_get_owner() is NULL
-         *  2. page_get_owner() is a domain that was never accessible by
-         *     its domid (e.g., failed to fully construct the domain).
-         *  3. page was never addressable by the guest (e.g., it's an
-         *     auto-translate-physmap guest and the page was never included
-         *     in its pseudophysical address space).
-         * In all the above cases there can be no guest mappings of this page.
-         */
-        switch ( pg[i].count_info & PGC_state )
-        {
-        case PGC_state_inuse:
-            BUG_ON(pg[i].count_info & PGC_broken);
-            pg[i].count_info = PGC_state_free;
-            break;
-
-        case PGC_state_offlining:
-            pg[i].count_info = (pg[i].count_info & PGC_broken) |
-                               PGC_state_offlined;
-            tainted = 1;
-            break;
-
-        default:
-            printk(XENLOG_ERR
-                   "pg[%u] MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n",
-                   i, mfn_x(mfn) + i,
-                   pg[i].count_info, pg[i].v.free.order,
-                   pg[i].u.free.val, pg[i].tlbflush_timestamp);
-            BUG();
-        }
-
-        /* If a page has no owner it will need no safety TLB flush. */
-        pg[i].u.free.need_tlbflush = (page_get_owner(&pg[i]) != NULL);
-        if ( pg[i].u.free.need_tlbflush )
-            page_set_tlbflush_timestamp(&pg[i]);
-
-        /* This page is not a guest frame any more. */
-        page_set_owner(&pg[i], NULL); /* set_gpfn_from_mfn snoops pg owner */
-        set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY);
+        mark_page_free(&pg[i], mfn_add(mfn, i));
 
         if ( need_scrub )
         {
-- 
2.25.1

[PATCH V4 05/10] xen/arm: static memory initialization

[Penny Zheng]

This patch introduces static memory initialization, during system boot up.

The new function init_staticmem_pages is responsible for static memory
initialization.

Helper free_staticmem_pages is the equivalent of free_heap_pages, to free
nr_mfns pages of static memory.

This commit also introduces new CONFIG_STATIC_MEMORY to avoid bringing dead
codes in other archs.

Put asynchronous scrubbing for pages of static memory in TODO list.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v4 change:
- move the option CONFIG_STATIC_MEMORY to common code, and with Arm
"select"ing it
- replace round_pg{down,up}() with PFN_DOWN()/PFN_UP()
---
 xen/arch/arm/Kconfig    |  1 +
 xen/arch/arm/setup.c    | 24 ++++++++++++++++++++++++
 xen/common/Kconfig      |  3 +++
 xen/common/page_alloc.c | 20 ++++++++++++++++++++
 xen/include/xen/mm.h    |  6 ++++++
 5 files changed, 54 insertions(+)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index ecfa6822e4..cc7a943d27 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -15,6 +15,7 @@ config ARM
 	select HAS_PASSTHROUGH
 	select HAS_PDX
 	select IOMMU_FORCE_PT_SHARE
+	select STATIC_MEMORY
 
 config ARCH_DEFCONFIG
 	string
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index f569134317..369f6631ee 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -622,6 +622,26 @@ static void __init init_pdx(void)
     }
 }
 
+/* Static memory initialization */
+static void __init init_staticmem_pages(void)
+{
+    unsigned int bank;
+
+    /* TODO: Considering NUMA-support scenario. */
+    for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ )
+    {
+        unsigned long bank_start = PFN_UP(bootinfo.static_mem.bank[bank].start);
+        unsigned long bank_size = PFN_DOWN(bootinfo.static_mem.bank[bank].size);
+        unsigned long bank_end = bank_start + bank_size;
+
+        if ( bank_end <= bank_start )
+            return;
+
+        free_staticmem_pages(mfn_to_page(_mfn(bank_start)),
+                             bank_size, false);
+    }
+}
+
 #ifdef CONFIG_ARM_32
 static void __init setup_mm(void)
 {
@@ -749,6 +769,8 @@ static void __init setup_mm(void)
     /* Add xenheap memory that was not already added to the boot allocator. */
     init_xenheap_pages(mfn_to_maddr(xenheap_mfn_start),
                        mfn_to_maddr(xenheap_mfn_end));
+
+    init_staticmem_pages();
 }
 #else /* CONFIG_ARM_64 */
 static void __init setup_mm(void)
@@ -802,6 +824,8 @@ static void __init setup_mm(void)
 
     setup_frametable_mappings(ram_start, ram_end);
     max_page = PFN_DOWN(ram_end);
+
+    init_staticmem_pages();
 }
 #endif
 
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 0ddd18e11a..8f736eea82 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -67,6 +67,9 @@ config MEM_ACCESS
 config NEEDS_LIBELF
 	bool
 
+config STATIC_MEMORY
+	bool
+
 menu "Speculative hardening"
 
 config SPECULATIVE_HARDEN_ARRAY
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index a3ee5eca9e..2acb73e323 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1519,6 +1519,26 @@ static void free_heap_pages(
     spin_unlock(&heap_lock);
 }
 
+#ifdef CONFIG_STATIC_MEMORY
+/* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */
+void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
+                                 bool need_scrub)
+{
+    mfn_t mfn = page_to_mfn(pg);
+    unsigned long i;
+
+    for ( i = 0; i < nr_mfns; i++ )
+    {
+        mark_page_free(&pg[i], mfn_add(mfn, i));
+
+        if ( need_scrub )
+        {
+            /* TODO: asynchronous scrubbing for pages of static memory. */
+            scrub_one_page(pg);
+        }
+    }
+}
+#endif
 
 /*
  * Following rules applied for page offline:
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 667f9dac83..8e8fb5a615 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -85,6 +85,12 @@ bool scrub_free_pages(void);
 } while ( false )
 #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
 
+#ifdef CONFIG_STATIC_MEMORY
+/* These functions are for static memory */
+void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
+                          bool need_scrub);
+#endif
+
 /* Map machine page range in Xen virtual address space. */
 int map_pages_to_xen(
     unsigned long virt,
-- 
2.25.1

[PATCH V4 06/10] xen/arm: introduce PGC_reserved

[Penny Zheng]

This patch introduces a new page flag PGC_reserved in order to differentiate
pages of static memory from those allocated from heap.

Mark pages of static memory PGC_reserved when initializing them.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/common/page_alloc.c  | 3 +++
 xen/include/asm-arm/mm.h | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 2acb73e323..f51e406401 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1536,6 +1536,9 @@ void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
             /* TODO: asynchronous scrubbing for pages of static memory. */
             scrub_one_page(pg);
         }
+
+        /* In case initializing page of static memory, mark it PGC_reserved. */
+        pg[i].count_info |= PGC_reserved;
     }
 }
 #endif
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index ded74d29da..7b5e7b7f69 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -108,6 +108,9 @@ struct page_info
   /* Page is Xen heap? */
 #define _PGC_xen_heap     PG_shift(2)
 #define PGC_xen_heap      PG_mask(1, 2)
+  /* Page is reserved */
+#define _PGC_reserved     PG_shift(3)
+#define PGC_reserved      PG_mask(1, 3)
 /* ... */
 /* Page is broken? */
 #define _PGC_broken       PG_shift(7)
-- 
2.25.1

[PATCH V4 07/10] xen: re-define assign_pages and introduce assign_page

[Penny Zheng]

In order to deal with the trouble of count-to-order conversion when page number
is not in a power-of-two, this commit re-define assign_pages for nr pages and
assign_page for original page with a single order.

Backporting confusion could be helped by altering the order of assign_page
parameters, such that the compiler would point out that adjustments at call
sites are needed.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v4 change:
- in all cases where order-0 pages get passed, prefer using assign_pages
to pass literal 1
- reconstruct the order of assign_pages parameters
- remove the unnecessary parentheses
---
 xen/arch/x86/pv/dom0_build.c |  2 +-
 xen/common/grant_table.c     |  2 +-
 xen/common/memory.c          |  4 ++--
 xen/common/page_alloc.c      | 23 ++++++++++++++---------
 xen/include/xen/mm.h         |  6 ++++++
 5 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index af47615b22..9142f359da 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -556,7 +556,7 @@ int __init dom0_construct_pv(struct domain *d,
         else
         {
             while ( count-- )
-                if ( assign_pages(d, mfn_to_page(_mfn(mfn++)), 0, 0) )
+                if ( assign_pages(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
                     BUG();
         }
         initrd->mod_end = 0;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fab77ab9cc..1f6b89bff4 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2342,7 +2342,7 @@ gnttab_transfer(
          * is respected and speculative execution is blocked accordingly
          */
         if ( unlikely(!evaluate_nospec(okay)) ||
-            unlikely(assign_pages(e, page, 0, MEMF_no_refcount)) )
+            unlikely(assign_pages(page, 1, e, MEMF_no_refcount)) )
         {
             bool drop_dom_ref;
 
diff --git a/xen/common/memory.c b/xen/common/memory.c
index e07bd9a5ea..083e14b84f 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -728,7 +728,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
         /* Assign each output page to the domain. */
         for ( j = 0; (page = page_list_remove_head(&out_chunk_list)); ++j )
         {
-            if ( assign_pages(d, page, exch.out.extent_order,
+            if ( assign_page(d, page, exch.out.extent_order,
                               MEMF_no_refcount) )
             {
                 unsigned long dec_count;
@@ -797,7 +797,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
      * cleared PGC_allocated.
      */
     while ( (page = page_list_remove_head(&in_chunk_list)) )
-        if ( assign_pages(d, page, 0, MEMF_no_refcount) )
+        if ( assign_pages(page, 1, d, MEMF_no_refcount) )
         {
             BUG_ON(!d->is_dying);
             free_domheap_page(page);
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index f51e406401..e279c6f713 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2282,9 +2282,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
 
 
 int assign_pages(
-    struct domain *d,
     struct page_info *pg,
-    unsigned int order,
+    unsigned long nr,
+    struct domain *d,
     unsigned int memflags)
 {
     int rc = 0;
@@ -2304,7 +2304,7 @@ int assign_pages(
     {
         unsigned int extra_pages = 0;
 
-        for ( i = 0; i < (1ul << order); i++ )
+        for ( i = 0; i < nr; i++ )
         {
             ASSERT(!(pg[i].count_info & ~PGC_extra));
             if ( pg[i].count_info & PGC_extra )
@@ -2313,18 +2313,18 @@ int assign_pages(
 
         ASSERT(!extra_pages ||
                ((memflags & MEMF_no_refcount) &&
-                extra_pages == 1u << order));
+                extra_pages == nr));
     }
 #endif
 
     if ( pg[0].count_info & PGC_extra )
     {
-        d->extra_pages += 1u << order;
+        d->extra_pages += nr;
         memflags &= ~MEMF_no_refcount;
     }
     else if ( !(memflags & MEMF_no_refcount) )
     {
-        unsigned int tot_pages = domain_tot_pages(d) + (1 << order);
+        unsigned int tot_pages = domain_tot_pages(d) + nr;
 
         if ( unlikely(tot_pages > d->max_pages) )
         {
@@ -2336,10 +2336,10 @@ int assign_pages(
     }
 
     if ( !(memflags & MEMF_no_refcount) &&
-         unlikely(domain_adjust_tot_pages(d, 1 << order) == (1 << order)) )
+         unlikely(domain_adjust_tot_pages(d, nr) == nr) )
         get_knownalive_domain(d);
 
-    for ( i = 0; i < (1 << order); i++ )
+    for ( i = 0; i < nr; i++ )
     {
         ASSERT(page_get_owner(&pg[i]) == NULL);
         page_set_owner(&pg[i], d);
@@ -2354,6 +2354,11 @@ int assign_pages(
     return rc;
 }
 
+int assign_page(struct domain *d, struct page_info *pg, unsigned int order,
+                unsigned int memflags)
+{
+    return assign_pages(pg, 1UL << order, d, memflags);
+}
 
 struct page_info *alloc_domheap_pages(
     struct domain *d, unsigned int order, unsigned int memflags)
@@ -2396,7 +2401,7 @@ struct page_info *alloc_domheap_pages(
                 pg[i].count_info = PGC_extra;
             }
         }
-        if ( assign_pages(d, pg, order, memflags) )
+        if ( assign_page(d, pg, order, memflags) )
         {
             free_heap_pages(pg, order, memflags & MEMF_no_scrub);
             return NULL;
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 8e8fb5a615..2e75cdcbb7 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -132,6 +132,12 @@ int query_page_offline(mfn_t mfn, uint32_t *status);
 void heap_init_late(void);
 
 int assign_pages(
+    struct page_info *pg,
+    unsigned long nr,
+    struct domain *d,
+    unsigned int memflags);
+
+int assign_page(
     struct domain *d,
     struct page_info *pg,
     unsigned int order,
-- 
2.25.1

[PATCH V4 08/10] xen/arm: introduce acquire_staticmem_pages and acquire_domstatic_pages

[Penny Zheng]

alloc_staticmem_pages aims to acquire nr_mfns contiguous pages of
static memory. And it is the equivalent of alloc_heap_pages for static
memory. Here only covers acquiring pre-configured static memory.

For each page, it shall check if the page is reserved(PGC_reserved)
and free. It shall also do a set of necessary initialization, which are
mostly the same ones in alloc_heap_pages, like, following the same
cache-coherency policy and turning page status into PGC_state_inuse, etc.

acquire_domstatic_pages is the equivalent of alloc_domheap_pages for
static memory, and it is to acquire nr_mfns contiguous pages of static memory
and assign them to one specific domain.

It uses acquire_staticmem_pages to acquire nr_mfns pre-configured pages of
static memory, then on success, it will use assign_pages to assign those pages
to one specific domain.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v4 change:
- moving tlb/cache flush outside of the locked region, considering XSA-364
and reducing the amount of work happening with the heap_lock held
- remove MEMF_no_refcount case
- make acquire_staticmem_pages/acquire_domstatic_pages being __init
---
 xen/common/page_alloc.c | 108 +++++++++++++++++++++++++++++++++++++++-
 xen/include/xen/mm.h    |   3 ++
 2 files changed, 109 insertions(+), 2 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index e279c6f713..b0edaf12b3 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -151,6 +151,10 @@
 #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
 #endif
 
+#ifndef CONFIG_STATIC_MEMORY
+#define PGC_reserved 0
+#endif
+
 /*
  * Comma-separated list of hexadecimal page numbers containing bad bytes.
  * e.g. 'badpage=0x3f45,0x8a321'.
@@ -1065,6 +1069,73 @@ static struct page_info *alloc_heap_pages(
     return pg;
 }
 
+#ifdef CONFIG_STATIC_MEMORY
+/*
+ * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of
+ * static memory.
+ */
+static struct page_info * __init acquire_staticmem_pages(unsigned long nr_mfns,
+                                                         mfn_t smfn,
+                                                         unsigned int memflags)
+{
+    bool need_tlbflush = false;
+    uint32_t tlbflush_timestamp = 0;
+    unsigned long i;
+    struct page_info *pg;
+
+    /* For now, it only supports pre-configured static memory. */
+    if ( !mfn_valid(smfn) || !nr_mfns )
+        return NULL;
+
+    spin_lock(&heap_lock);
+
+    pg = mfn_to_page(smfn);
+
+    for ( i = 0; i < nr_mfns; i++ )
+    {
+        /*
+         * Reference count must continuously be zero for free pages
+         * of static memory(PGC_reserved).
+         */
+        if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
+        {
+            printk(XENLOG_ERR
+                   "pg[%lu] Static MFN %"PRI_mfn" c=%#lx t=%#x\n",
+                   i, mfn_x(page_to_mfn(pg + i)),
+                   pg[i].count_info, pg[i].tlbflush_timestamp);
+            BUG();
+        }
+
+        if ( !(memflags & MEMF_no_tlbflush) )
+            accumulate_tlbflush(&need_tlbflush, &pg[i],
+                                &tlbflush_timestamp);
+
+        /*
+         * Preserve flag PGC_reserved and change page state
+         * to PGC_state_inuse.
+         */
+        pg[i].count_info = (PGC_reserved | PGC_state_inuse);
+        /* Initialise fields which have other uses for free pages. */
+        pg[i].u.inuse.type_info = 0;
+        page_set_owner(&pg[i], NULL);
+    }
+
+    spin_unlock(&heap_lock);
+
+    if ( need_tlbflush )
+        filtered_flush_tlb_mask(tlbflush_timestamp);
+
+    /*
+     * Ensure cache and RAM are consistent for platforms where the guest
+     * can control its own visibility of/through the cache.
+     */
+    for ( i = 0; i < nr_mfns; i++ )
+        flush_page_to_ram(mfn_x(smfn) + i, !(memflags & MEMF_no_icache_flush));
+
+    return pg;
+}
+#endif
+
 /* Remove any offlined page in the buddy pointed to by head. */
 static int reserve_offlined_page(struct page_info *head)
 {
@@ -2306,7 +2377,7 @@ int assign_pages(
 
         for ( i = 0; i < nr; i++ )
         {
-            ASSERT(!(pg[i].count_info & ~PGC_extra));
+            ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_reserved)));
             if ( pg[i].count_info & PGC_extra )
                 extra_pages++;
         }
@@ -2345,7 +2416,8 @@ int assign_pages(
         page_set_owner(&pg[i], d);
         smp_wmb(); /* Domain pointer must be visible before updating refcnt. */
         pg[i].count_info =
-            (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
+            (pg[i].count_info & (PGC_extra | PGC_reserved)) | PGC_allocated | 1;
+
         page_list_add_tail(&pg[i], page_to_list(d, &pg[i]));
     }
 
@@ -2411,6 +2483,38 @@ struct page_info *alloc_domheap_pages(
     return pg;
 }
 
+#ifdef CONFIG_STATIC_MEMORY
+/*
+ * Acquire nr_mfns contiguous pages, starting at #smfn, of static memory,
+ * then assign them to one specific domain #d.
+ */
+struct page_info * __init acquire_domstatic_pages(struct domain *d,
+                                                  unsigned long nr_mfns,
+                                                  mfn_t smfn, unsigned int memflags)
+{
+    struct page_info *pg = NULL;
+
+    ASSERT(!in_irq());
+
+    pg = acquire_staticmem_pages(nr_mfns, smfn, memflags);
+    if ( !pg )
+        return NULL;
+
+    /*
+     * MEMF_no_owner/MEMF_no_refcount cases are missing here because
+     * right now, acquired static memory is only for guest RAM.
+     */
+    ASSERT(d);
+    if ( assign_pages(pg, nr_mfns, d, memflags) )
+    {
+        free_staticmem_pages(pg, nr_mfns, memflags & MEMF_no_scrub);
+        return NULL;
+    }
+
+    return pg;
+}
+#endif
+
 void free_domheap_pages(struct page_info *pg, unsigned int order)
 {
     struct domain *d = page_get_owner(pg);
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 2e75cdcbb7..62e8e2ad61 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -89,6 +89,9 @@ bool scrub_free_pages(void);
 /* These functions are for static memory */
 void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                           bool need_scrub);
+struct page_info *acquire_domstatic_pages(struct domain *d,
+                                          unsigned long nr_mfns, mfn_t smfn,
+                                          unsigned int memflags);
 #endif
 
 /* Map machine page range in Xen virtual address space. */
-- 
2.25.1

[PATCH V4 09/10] xen/arm: check "xen,static-mem" property during domain construction

[Penny Zheng]

This commit checks "xen,static-mem" device tree property in /domUx node,
to determine whether domain is on Static Allocation, when constructing
domain during boot-up.

Right now, the implementation of allocate_static_memory is missing, and
will be introduced later. It just BUG() out at the moment.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/arch/arm/domain_build.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6c86d52781..cdb16f2086 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2425,6 +2425,37 @@ static int __init construct_domU(struct domain *d,
     struct kernel_info kinfo = {};
     int rc;
     u64 mem;
+    const struct dt_property *static_mem_prop;
+    u32 static_mem_addr_cells, static_mem_size_cells;
+    bool static_mem = false;
+
+    /*
+     * Guest RAM could be static memory which will be specified through
+     * "xen,static-mem" property.
+     */
+    static_mem_prop = dt_find_property(node, "xen,static-mem", NULL);
+    if ( static_mem_prop )
+    {
+        static_mem = true;
+
+        if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
+                                   &static_mem_addr_cells) )
+        {
+            printk("Error building DomU: cannot read "
+                   "\"#xen,static-mem-address-cells\" property\n");
+            return -EINVAL;
+        }
+
+        if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
+                                   &static_mem_size_cells) )
+        {
+            printk("Error building DomU: cannot read "
+                   "\"#xen,static-mem-size-cells\" property\n");
+            return -EINVAL;
+        }
+
+        BUG_ON(static_mem_size_cells > 2 || static_mem_addr_cells > 2);
+    }
 
     rc = dt_property_read_u64(node, "memory", &mem);
     if ( !rc )
@@ -2452,7 +2483,11 @@ static int __init construct_domU(struct domain *d,
     /* type must be set before allocate memory */
     d->arch.type = kinfo.type;
 #endif
-    allocate_memory(d, &kinfo);
+    if ( !static_mem )
+        allocate_memory(d, &kinfo);
+    else
+        /* TODO: allocate_static_memory(...). */
+        BUG();
 
     rc = prepare_dtb_domU(d, &kinfo);
     if ( rc < 0 )
-- 
2.25.1

[PATCH V4 10/10] xen/arm: introduce allocate_static_memory

[Penny Zheng]

This commit introduces allocate_static_memory to allocate static memory as
guest RAM for Domain on Static Allocation.

It uses acquire_domstatic_pages to acquire pre-configured static memory
for this domain, and uses guest_physmap_add_page to set up P2M table.
These pre-defined static memory banks shall be firstly mapped to the
fixed guest RAM address `GUEST_RAM0_BASE`. And until it exhausts the
`GUEST_RAM0_SIZE`, it will seek to `GUEST_RAM1_BASE`, and so on.
`GUEST_RAM0` may take up several pre-defined physical RAM regions.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/arch/arm/domain_build.c | 137 +++++++++++++++++++++++++++++++++++-
 1 file changed, 135 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index cdb16f2086..ed290ee31b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -480,6 +480,139 @@ fail:
           (unsigned long)kinfo->unassigned_mem >> 10);
 }
 
+static bool __init append_static_memory_to_bank(struct domain *d,
+                                                struct membank *bank,
+                                                mfn_t smfn,
+                                                paddr_t size)
+{
+    int res;
+    paddr_t tot_size = size;
+    /* Infer next GFN. */
+    gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
+
+    while ( tot_size > 0 )
+    {
+        unsigned int order = get_allocation_size(tot_size);
+
+        res = guest_physmap_add_page(d, sgfn, smfn, order);
+        if ( res )
+        {
+            dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
+            return false;
+        }
+
+        smfn = mfn_add(smfn, 1UL << order);
+        tot_size -= (1UL << (PAGE_SHIFT + order));
+    }
+
+    bank->size = bank->size + size;
+    return true;
+}
+
+/* Allocate memory from static memory as RAM for one specific domain d. */
+static void __init allocate_static_memory(struct domain *d,
+                                          struct kernel_info *kinfo,
+                                          const struct dt_property *prop,
+                                          u32 addr_cells, u32 size_cells)
+{
+    unsigned int nr_banks, gbank, bank = 0;
+    const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
+    const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
+    const __be32 *cell;
+    u32 reg_cells = addr_cells + size_cells;
+    u64 tot_size = 0;
+    paddr_t pbase, psize, gsize;
+    mfn_t smfn;
+
+    /* Start with GUEST_RAM0. */
+    kinfo->mem.nr_banks = 0;
+    gbank = 0;
+    gsize = ramsize[gbank];
+    kinfo->mem.bank[gbank].start = rambase[gbank];
+
+    cell = (const __be32 *)prop->value;
+    nr_banks = (prop->length) / (reg_cells * sizeof (u32));
+    BUG_ON(nr_banks > NR_MEM_BANKS);
+
+    while ( bank < nr_banks )
+    {
+        device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize);
+        tot_size += psize;
+        smfn = maddr_to_mfn(pbase);
+
+        if ( !acquire_domstatic_pages(d, psize >> PAGE_SHIFT, smfn, 0) )
+        {
+            printk(XENLOG_ERR
+                    "%pd: cannot acquire static memory "
+                    "(0x%"PRIpaddr" - 0x%"PRIpaddr").\n",
+                    d, pbase, pbase + psize);
+            goto fail;
+        }
+
+        printk(XENLOG_INFO "%pd: STATIC BANK[%d] %#"PRIpaddr"-%#"PRIpaddr"\n",
+               d, bank, pbase, pbase + psize);
+
+        /*
+         * It shall be mapped to the fixed guest RAM address rambase[i],
+         * And until it exhausts the ramsize[i], it will seek to the next
+         * rambase[i+1].
+         */
+        while ( 1 )
+        {
+            /*
+             * The current physical bank is fully mapped.
+             * Handle the next physical bank.
+             */
+            if ( gsize >= psize )
+            {
+                if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
+                                                   smfn, psize) )
+                    goto fail;
+
+                gsize = gsize - psize;
+                bank++;
+                break;
+            }
+            /*
+             * Current guest bank memory is not enough to map.
+             * Check if we have another guest bank available.
+             * gbank refers guest memory bank index.
+             */
+            else if ( (gbank + 2) > GUEST_RAM_BANKS ) {
+                printk("Exhausted the number of guest bank\n");
+                goto fail;
+            }
+            else
+            {
+                if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
+                                                   smfn, gsize) )
+                    goto fail;
+
+                psize = psize - gsize;
+                smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
+                /* Update to the next guest bank. */
+                gbank++;
+                gsize = ramsize[gbank];
+                kinfo->mem.bank[gbank].start = rambase[gbank];
+            }
+        }
+    }
+
+    kinfo->mem.nr_banks = ++gbank;
+    kinfo->unassigned_mem -= tot_size;
+    if ( kinfo->unassigned_mem )
+        printk(XENLOG_ERR
+               "Size of \"memory\" property doesn't match up with the ones "
+               "defined in \"xen,static-mem\".\n");
+
+    return;
+
+fail:
+    panic("Failed to allocate requested static memory for domain %pd."
+          "Fix the VMs configurations.\n",
+          d);
+}
+
 static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
                                    const struct dt_device_node *node)
 {
@@ -2486,8 +2619,8 @@ static int __init construct_domU(struct domain *d,
     if ( !static_mem )
         allocate_memory(d, &kinfo);
     else
-        /* TODO: allocate_static_memory(...). */
-        BUG();
+        allocate_static_memory(d, &kinfo, static_mem_prop,
+                               static_mem_addr_cells, static_mem_size_cells);
 
     rc = prepare_dtb_domU(d, &kinfo);
     if ( rc < 0 )
-- 
2.25.1

[PATCH V4 04/10] xen: introduce mark_page_free

[Penny Zheng]

This commit defines a new helper mark_page_free to extract common code,
like following the same cache/TLB coherency policy, between free_heap_pages
and the new function free_staticmem_pages, which will be introduced later.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 xen/common/page_alloc.c | 89 ++++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 958ba0cd92..a3ee5eca9e 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1376,6 +1376,53 @@ bool scrub_free_pages(void)
     return node_to_scrub(false) != NUMA_NO_NODE;
 }
 
+static void mark_page_free(struct page_info *pg, mfn_t mfn)
+{
+    ASSERT(mfn_x(mfn) == mfn_x(page_to_mfn(pg)));
+
+    /*
+     * Cannot assume that count_info == 0, as there are some corner cases
+     * where it isn't the case and yet it isn't a bug:
+     *  1. page_get_owner() is NULL
+     *  2. page_get_owner() is a domain that was never accessible by
+     *     its domid (e.g., failed to fully construct the domain).
+     *  3. page was never addressable by the guest (e.g., it's an
+     *     auto-translate-physmap guest and the page was never included
+     *     in its pseudophysical address space).
+     * In all the above cases there can be no guest mappings of this page.
+     */
+    switch ( pg->count_info & PGC_state )
+    {
+    case PGC_state_inuse:
+        BUG_ON(pg->count_info & PGC_broken);
+        pg->count_info = PGC_state_free;
+        break;
+
+    case PGC_state_offlining:
+        pg->count_info = (pg->count_info & PGC_broken) |
+                         PGC_state_offlined;
+        tainted = 1;
+        break;
+
+    default:
+        printk(XENLOG_ERR
+               "pg MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n",
+               mfn_x(mfn),
+               pg->count_info, pg->v.free.order,
+               pg->u.free.val, pg->tlbflush_timestamp);
+        BUG();
+    }
+
+    /* If a page has no owner it will need no safety TLB flush. */
+    pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL);
+    if ( pg->u.free.need_tlbflush )
+        page_set_tlbflush_timestamp(pg);
+
+    /* This page is not a guest frame any more. */
+    page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */
+    set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY);
+}
+
 /* Free 2^@order set of pages. */
 static void free_heap_pages(
     struct page_info *pg, unsigned int order, bool need_scrub)
@@ -1392,47 +1439,7 @@ static void free_heap_pages(
 
     for ( i = 0; i < (1 << order); i++ )
     {
-        /*
-         * Cannot assume that count_info == 0, as there are some corner cases
-         * where it isn't the case and yet it isn't a bug:
-         *  1. page_get_owner() is NULL
-         *  2. page_get_owner() is a domain that was never accessible by
-         *     its domid (e.g., failed to fully construct the domain).
-         *  3. page was never addressable by the guest (e.g., it's an
-         *     auto-translate-physmap guest and the page was never included
-         *     in its pseudophysical address space).
-         * In all the above cases there can be no guest mappings of this page.
-         */
-        switch ( pg[i].count_info & PGC_state )
-        {
-        case PGC_state_inuse:
-            BUG_ON(pg[i].count_info & PGC_broken);
-            pg[i].count_info = PGC_state_free;
-            break;
-
-        case PGC_state_offlining:
-            pg[i].count_info = (pg[i].count_info & PGC_broken) |
-                               PGC_state_offlined;
-            tainted = 1;
-            break;
-
-        default:
-            printk(XENLOG_ERR
-                   "pg[%u] MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n",
-                   i, mfn_x(mfn) + i,
-                   pg[i].count_info, pg[i].v.free.order,
-                   pg[i].u.free.val, pg[i].tlbflush_timestamp);
-            BUG();
-        }
-
-        /* If a page has no owner it will need no safety TLB flush. */
-        pg[i].u.free.need_tlbflush = (page_get_owner(&pg[i]) != NULL);
-        if ( pg[i].u.free.need_tlbflush )
-            page_set_tlbflush_timestamp(&pg[i]);
-
-        /* This page is not a guest frame any more. */
-        page_set_owner(&pg[i], NULL); /* set_gpfn_from_mfn snoops pg owner */
-        set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY);
+        mark_page_free(&pg[i], mfn_add(mfn, i));
 
         if ( need_scrub )
         {
-- 
2.25.1

Re: [PATCH V4 05/10] xen/arm: static memory initialization

[Jan Beulich]

On 28.07.2021 12:27, Penny Zheng wrote:
> This patch introduces static memory initialization, during system boot up.
> 
> The new function init_staticmem_pages is responsible for static memory
> initialization.
> 
> Helper free_staticmem_pages is the equivalent of free_heap_pages, to free
> nr_mfns pages of static memory.
> 
> This commit also introduces new CONFIG_STATIC_MEMORY to avoid bringing dead
> codes in other archs.
> 
> Put asynchronous scrubbing for pages of static memory in TODO list.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Common code parts:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH V4 07/10] xen: re-define assign_pages and introduce assign_page

[Jan Beulich]

On 28.07.2021 12:27, Penny Zheng wrote:
> In order to deal with the trouble of count-to-order conversion when page number
> is not in a power-of-two, this commit re-define assign_pages for nr pages and
> assign_page for original page with a single order.
> 
> Backporting confusion could be helped by altering the order of assign_page
> parameters, such that the compiler would point out that adjustments at call
> sites are needed.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Largely to indicate my main concerns were addressed, yet not so much to
indicate I'm actually happy with the introduction of the 2nd function:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH V4 08/10] xen/arm: introduce acquire_staticmem_pages and acquire_domstatic_pages

[Jan Beulich]

On 28.07.2021 12:27, Penny Zheng wrote:
> @@ -1065,6 +1069,73 @@ static struct page_info *alloc_heap_pages(
>      return pg;
>  }
>  
> +#ifdef CONFIG_STATIC_MEMORY
> +/*
> + * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of
> + * static memory.
> + */
> +static struct page_info * __init acquire_staticmem_pages(unsigned long nr_mfns,
> +                                                         mfn_t smfn,
> +                                                         unsigned int memflags)
> +{
> +    bool need_tlbflush = false;
> +    uint32_t tlbflush_timestamp = 0;
> +    unsigned long i;
> +    struct page_info *pg;
> +
> +    /* For now, it only supports pre-configured static memory. */
> +    if ( !mfn_valid(smfn) || !nr_mfns )
> +        return NULL;
> +
> +    spin_lock(&heap_lock);
> +
> +    pg = mfn_to_page(smfn);

Nit: This can easily be done prior to acquiring the lock. And since PDX
compression causes this to be a non-trivial calculation, it is probably
better that way despite the compiler then being forced to use a non-
call-clobbered register to hold the variable (it might do so anyway,
but as it looks there currently is nothing actually requiring this).

> +    for ( i = 0; i < nr_mfns; i++ )
> +    {
> +        /*
> +         * Reference count must continuously be zero for free pages
> +         * of static memory(PGC_reserved).
> +         */
> +        if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
> +        {
> +            printk(XENLOG_ERR
> +                   "pg[%lu] Static MFN %"PRI_mfn" c=%#lx t=%#x\n",
> +                   i, mfn_x(page_to_mfn(pg + i)),

"mfn_x(smfn) + i" would cause less code to be generated, again due to
PDX compression making the transformation a non-trivial operation.

> +                   pg[i].count_info, pg[i].tlbflush_timestamp);
> +            BUG();
> +        }
> +
> +        if ( !(memflags & MEMF_no_tlbflush) )
> +            accumulate_tlbflush(&need_tlbflush, &pg[i],
> +                                &tlbflush_timestamp);
> +
> +        /*
> +         * Preserve flag PGC_reserved and change page state
> +         * to PGC_state_inuse.
> +         */
> +        pg[i].count_info = (PGC_reserved | PGC_state_inuse);

Nit: Parentheses aren't really needed here.

> @@ -2411,6 +2483,38 @@ struct page_info *alloc_domheap_pages(
>      return pg;
>  }
>  
> +#ifdef CONFIG_STATIC_MEMORY
> +/*
> + * Acquire nr_mfns contiguous pages, starting at #smfn, of static memory,
> + * then assign them to one specific domain #d.
> + */
> +struct page_info * __init acquire_domstatic_pages(struct domain *d,
> +                                                  unsigned long nr_mfns,
> +                                                  mfn_t smfn, unsigned int memflags)
> +{
> +    struct page_info *pg = NULL;

Unnecessary initializer.

> +    ASSERT(!in_irq());
> +
> +    pg = acquire_staticmem_pages(nr_mfns, smfn, memflags);
> +    if ( !pg )
> +        return NULL;
> +
> +    /*
> +     * MEMF_no_owner/MEMF_no_refcount cases are missing here because
> +     * right now, acquired static memory is only for guest RAM.
> +     */

I think you want to ASSERT() or otherwise check that the two flags are
clear. Whether you then still deem the comment appropriate in this
form is up to you, I guess. Otoh ...

> +    ASSERT(d);

... I'm less convinced that this ASSERT() is very useful. At the very
least if you use other than or more than ASSERT() for the checking
above, this one should follow suit. Perhaps altogether

    if ( !d || (memflags & (MEMF_no_owner | MEMF_no_refcount)) )
    {
        /*
         * Respective handling omitted here because right now
         * acquired static memory is only for guest RAM.
         */
        ASSERT_UNREACHABLE();
        return NULL;
    }

?

Jan

Re: [PATCH V4 01/10] xen/arm: introduce domain on Static Allocation

[Julien Grall]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> Static Allocation refers to system or sub-system(domains) for which memory
> areas are pre-defined by configuration using physical address ranges.
> Those pre-defined memory, -- Static Memory, as parts of RAM reserved in the
> beginning, shall never go to heap allocator or boot allocator for any use.
> 
> Domains on Static Allocation is supported through device tree property
> `xen,static-mem` specifying reserved RAM banks as this domain's guest RAM.
> By default, they shall be mapped to the fixed guest RAM address
> `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`.
> 
> This patch introduces this new `xen,static-mem` feature, and also documents
> and parses this new attribute at boot time and stores related info in
> static_mem for later initialization.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   docs/misc/arm/device-tree/booting.txt | 40 +++++++++++++++++++++
>   xen/arch/arm/bootfdt.c                | 51 +++++++++++++++++++++++++++
>   xen/include/asm-arm/setup.h           |  2 ++
>   3 files changed, 93 insertions(+)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 5243bc7fd3..2a1ddca29b 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -268,3 +268,43 @@ The DTB fragment is loaded at 0xc000000 in the example above. It should
>   follow the convention explained in docs/misc/arm/passthrough.txt. The
>   DTB fragment will be added to the guest device tree, so that the guest
>   kernel will be able to discover the device.
> +
> +
> +Static Allocation
> +=============
> +
> +Static Allocation refers to system or sub-system(domains) for which memory
> +areas are pre-defined by configuration using physical address ranges.
> +Those pre-defined memory, -- Static Memory, as parts of RAM reserved in the
> +beginning, shall never go to heap allocator or boot allocator for any use.

I don't understand "as parts of RAM reserved in the beginning". Could 
you clarify it?

> +
> +Domains on Static Allocation is supported through static memory property,
> +defined under according /domUx in the name of "xen,static-mem", which are

We don't require the domU node to be called /domUx.

> +specifying physical RAM as this domain's guest RAM.
>

How about:

Memory can be statically allocated to a domain using the property 
"xen,static-mem" defined in the domain configuration.

> +The size of address-cells/size-cells must be defined in

I would say "The number of cells for the address and the size must be 
defined using respectively the properties..."

> +"#xen,static-mem-address-cells" and "#xen,static-mem-size-cells".
> +
> +On memory allocation, these pre-defined static memory ranges shall be
> +firstly mapped to the fixed guest bank "GUEST_RAM0". Until it exhausts the
> +`GUEST_RAM0_SIZE`, then it will seek to `GUEST_RAM1_BASE`, and so on.
> +`GUEST_RAM0` may take up several pre-defined physical RAM regions.

GUEST_RAM0 & co are not part of the stable ABI. So I don't think the 
documentation should mention them.

But I am not convinced we should provide a guarantee how the allocation 
will happen. Why does it matter?

> +
> +The dtb property should look like as follows:

Do you mean "node" rather than "property"?

> +
> +    / {
> +        chosen {
> +            domU1 {
> +                compatible = "xen,domain";
> +                #address-cells = <0x2>;
> +                #size-cells = <0x2>;
> +                cpus = <2>;
> +                #xen,static-mem-address-cells = <0x1>;
> +                #xen,static-mem-size-cells = <0x1>;
> +                xen,static-mem = <0x30000000 0x20000000>;
> +                ...
> +            };
> +        };
> +    };
> +
> +DomU1 will have a static memory of 512MB reserved from the physical address
> +0x30000000 to 0x50000000.

I would write "This will reserve a 512MB region starting at the host 
physical address 0x30000000 to be exclusively used by DomU1".

> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 476e32e0f5..d2714446e1 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -193,6 +193,55 @@ static int __init process_reserved_memory_node(const void *fdt, int node,
>       return 0;
>   }
>   
> +static int __init process_static_memory(const void *fdt, int node, void *data)
> +{

This is pretty much a copy of process_memory_node(). So can we avoid the 
duplication?

I think I mentionned it in the past but I can't find the outcome.

> +    int i = 0, banks;
> +    const __be32 *cell;
> +    paddr_t start, size;
> +    u32 address_cells, size_cells, reg_cells;
> +    struct meminfo *mem = data;
> +    const struct fdt_property *prop;
> +
> +
> +    address_cells = device_tree_get_u32(fdt, node,
> +                                        "#xen,static-mem-address-cells", 0);
> +    size_cells = device_tree_get_u32(fdt, node,
> +                                     "#xen,static-mem-size-cells", 0);
> +    if ( (address_cells == 0) || (size_cells == 0) )
> +    {
> +         printk("Missing \"#xen,static-mem-address-cell\" or "
> +                 "\"#xen,static-mem-address-cell\".\n");
> +         return -EINVAL;
> +    }
> +    reg_cells = address_cells + size_cells;
> +
> +    prop = fdt_get_property(fdt, node, "xen,static-mem", NULL);
> +    /*
> +     * Static memory shall belong to a specific domain, that is,
> +     * its node `domUx` has compatible string "xen,domain".
> +     */

This code is just checking the node compatible is "xen,domain". So I 
would drop the "domUx". This is also...

> +    if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
> +    {
> +        printk("xen,static-mem property can only be located under /domUx node.\n");

... not correct.

> +        return -EINVAL;
> +    }
> +
> +    cell = (const __be32 *)prop->data;
> +    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> +
> +    for ( ; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
> +    {
> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +        mem->bank[mem->nr_banks].start = start;
> +        mem->bank[mem->nr_banks].size = size;
> +        mem->nr_banks++;
> +    }
> +
> +    if ( i < banks )
> +        return -ENOSPC;
> +    return 0;
> +}
> +
>   static int __init process_reserved_memory(const void *fdt, int node,
>                                             const char *name, int depth,
>                                             u32 address_cells, u32 size_cells)
> @@ -346,6 +395,8 @@ static int __init early_scan_node(const void *fdt,
>           process_multiboot_node(fdt, node, name, address_cells, size_cells);
>       else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
>           process_chosen_node(fdt, node, name, address_cells, size_cells);
> +    else if ( depth == 2 && fdt_get_property(fdt, node, "xen,static-mem", NULL) )

How about checking the compatible instead?

> +        process_static_memory(fdt, node, &bootinfo.static_mem);

You want "rc = ..." so the error is propaged if there is an issue (e.g. 
we don't have space for more static region).

>   
>       if ( rc < 0 )
>           printk("fdt: node `%s': parsing failed\n", name);
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index c4b6af6029..e076329fc4 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -74,6 +74,8 @@ struct bootinfo {
>   #ifdef CONFIG_ACPI
>       struct meminfo acpi;
>   #endif
> +    /* Static Memory */
> +    struct meminfo static_mem;
>   };
>   
>   extern struct bootinfo bootinfo;
> 

Cheers,

-- 
Julien Grall

Re: [PATCH V4 02/10] xen/arm: introduce new helper device_tree_get_meminfo

[Julien Grall]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> A few functions iterate over the device tree property to get memory info,
> like "reg" or "xen,static-mem", so this commit creates a new helper
> device_tree_get_meminfo to extract the
> common codes.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Ah, this is where you did the consolidation. Sorry, I didn't notice this 
patch.

In general, we are avoiding to introduce code and then rework it in the 
same series. Instead, the rework is done first and then the function is 
used.

So can you move this patch first?

Cheers,

-- 
Julien Grall

Re: [PATCH V4 03/10] xen/arm: handle static memory in dt_unreserved_regions

[Julien Grall]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> static memory regions overlap with memory nodes. The
> overlapping memory is reserved-memory and should be
> handled accordingly:
> dt_unreserved_regions should skip these regions the
> same way they are already skipping mem-reserved regions.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   xen/arch/arm/setup.c | 47 ++++++++++++++++++++++++++++----------------
>   1 file changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 63a908e325..f569134317 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -200,6 +200,13 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>                                            int first)
>   {
>       int i, nr = fdt_num_mem_rsv(device_tree_flattened);
> +    /*
> +     * There are two types of reserved memory stored in bootinfo, one defines
> +     * in /reserved-memory node, the other refers to domain on static allocation
> +     * through "xen,static-mem" property.
> +     */
> +    int nr_rsv_type = 2, t = 0, prev_nr;
> +    struct meminfo *rsv_type[2] = {&bootinfo.reserved_mem, &bootinfo.static_mem};

Looking at the rest of the series, it doesn't look like there is a real 
benefits to have the static memory and reserved memory in separate 
arrays as they are walked only a few times and they are both meant to be 
small. In fact, I think this code is lot more difficult to read.

So it would be best to merge the two arrays in one. We can add a flag in 
the structure to differentiate between "static" and "reserved" memory.

Cheers,

-- 
Julien Grall

Re: [PATCH V4 04/10] xen: introduce mark_page_free

[Julien Grall]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> This commit defines a new helper mark_page_free to extract common code,
> like following the same cache/TLB coherency policy, between free_heap_pages
> and the new function free_staticmem_pages, which will be introduced later.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
>   xen/common/page_alloc.c | 89 ++++++++++++++++++++++-------------------
>   1 file changed, 48 insertions(+), 41 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 958ba0cd92..a3ee5eca9e 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1376,6 +1376,53 @@ bool scrub_free_pages(void)
>       return node_to_scrub(false) != NUMA_NO_NODE;
>   }
>   
> +static void mark_page_free(struct page_info *pg, mfn_t mfn)
> +{
> +    ASSERT(mfn_x(mfn) == mfn_x(page_to_mfn(pg)));

NIT: If I got it correctly, the assumption is page_to_mfn() is 
expensive, so we want to avoid the conversation. I am not sure I agree 
with that but I would at least suggest to write it down in the commit 
message.

So it is easier for the next person to figure out the rationale. 
Something like:

"The PDX compression makes makes conversion between the MFN and the page 
can be potentially non-trivial. As the function is internal, pass the 
MFN and the page. They are both expected to match."

For the rest of the patch:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH V4 05/10] xen/arm: static memory initialization

[Julien Grall]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> This patch introduces static memory initialization, during system boot up.
> 
> The new function init_staticmem_pages is responsible for static memory
> initialization.
> 
> Helper free_staticmem_pages is the equivalent of free_heap_pages, to free
> nr_mfns pages of static memory.
> 
> This commit also introduces new CONFIG_STATIC_MEMORY to avoid bringing dead
> codes in other archs.
> 
> Put asynchronous scrubbing for pages of static memory in TODO list.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> v4 change:
> - move the option CONFIG_STATIC_MEMORY to common code, and with Arm
> "select"ing it
> - replace round_pg{down,up}() with PFN_DOWN()/PFN_UP()
> ---
>   xen/arch/arm/Kconfig    |  1 +
>   xen/arch/arm/setup.c    | 24 ++++++++++++++++++++++++
>   xen/common/Kconfig      |  3 +++
>   xen/common/page_alloc.c | 20 ++++++++++++++++++++
>   xen/include/xen/mm.h    |  6 ++++++
>   5 files changed, 54 insertions(+)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ecfa6822e4..cc7a943d27 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -15,6 +15,7 @@ config ARM
>   	select HAS_PASSTHROUGH
>   	select HAS_PDX
>   	select IOMMU_FORCE_PT_SHARE
> +	select STATIC_MEMORY

Given the list of TODOs, I think it would be better if STATIC_MEMORY is 
user selectable and gated by UNSUPPORTED.

We can remove the dependency on UNSUPPORTED once every have been addressed.

>   
>   config ARCH_DEFCONFIG
>   	string
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index f569134317..369f6631ee 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -622,6 +622,26 @@ static void __init init_pdx(void)
>       }
>   }
>   
> +/* Static memory initialization */
> +static void __init init_staticmem_pages(void)
> +{
> +    unsigned int bank;
> +
> +    /* TODO: Considering NUMA-support scenario. */
> +    for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ )
> +    {
> +        unsigned long bank_start = PFN_UP(bootinfo.static_mem.bank[bank].start);

I would prefer if bank_start is a mfn_t.

> +        unsigned long bank_size = PFN_DOWN(bootinfo.static_mem.bank[bank].size);

NIT: I would suggest to name it bank_pages or bank_nr_pages. This would 
make clear in the user that this contains pages.

> +        unsigned long bank_end = bank_start + bank_size;

mfn_t please.

> +
> +        if ( bank_end <= bank_start )

This will mean you will need to use mfn_x() for both. This code would be 
less nice but at least it avoids mixing address and MFN.

> +            return;
> +
> +        free_staticmem_pages(mfn_to_page(_mfn(bank_start)),
> +                             bank_size, false);
> +    }
> +}
> +
>   #ifdef CONFIG_ARM_32
>   static void __init setup_mm(void)
>   {
> @@ -749,6 +769,8 @@ static void __init setup_mm(void)
>       /* Add xenheap memory that was not already added to the boot allocator. */
>       init_xenheap_pages(mfn_to_maddr(xenheap_mfn_start),
>                          mfn_to_maddr(xenheap_mfn_end));
> +
> +    init_staticmem_pages();
>   }
>   #else /* CONFIG_ARM_64 */
>   static void __init setup_mm(void)
> @@ -802,6 +824,8 @@ static void __init setup_mm(void)
>   
>       setup_frametable_mappings(ram_start, ram_end);
>       max_page = PFN_DOWN(ram_end);
> +
> +    init_staticmem_pages();
>   }
>   #endif
>   
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 0ddd18e11a..8f736eea82 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -67,6 +67,9 @@ config MEM_ACCESS
>   config NEEDS_LIBELF
>   	bool
>   
> +config STATIC_MEMORY
> +	bool
> +
>   menu "Speculative hardening"
>   
>   config SPECULATIVE_HARDEN_ARRAY
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index a3ee5eca9e..2acb73e323 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1519,6 +1519,26 @@ static void free_heap_pages(
>       spin_unlock(&heap_lock);
>   }
>   
> +#ifdef CONFIG_STATIC_MEMORY
> +/* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */
> +void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> +                                 bool need_scrub)
> +{
> +    mfn_t mfn = page_to_mfn(pg);
> +    unsigned long i;
> +
> +    for ( i = 0; i < nr_mfns; i++ )
> +    {
> +        mark_page_free(&pg[i], mfn_add(mfn, i));
> +
> +        if ( need_scrub )
> +        {
> +            /* TODO: asynchronous scrubbing for pages of static memory. */
> +            scrub_one_page(pg);
> +        }
> +    }
> +}
> +#endif
>   
>   /*
>    * Following rules applied for page offline:
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 667f9dac83..8e8fb5a615 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -85,6 +85,12 @@ bool scrub_free_pages(void);
>   } while ( false )
>   #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
>   
> +#ifdef CONFIG_STATIC_MEMORY
> +/* These functions are for static memory */
> +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> +                          bool need_scrub);
> +#endif
> +
>   /* Map machine page range in Xen virtual address space. */
>   int map_pages_to_xen(
>       unsigned long virt,
> 

Cheers,

-- 
Julien Grall

Re: [PATCH V4 06/10] xen/arm: introduce PGC_reserved

[Julien Grall]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> This patch introduces a new page flag PGC_reserved in order to differentiate
> pages of static memory from those allocated from heap.
> 
> Mark pages of static memory PGC_reserved when initializing them.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

I think this want to be folded in patch #7.

Cheers,

-- 
Julien Grall

Re: [PATCH V4 07/10] xen: re-define assign_pages and introduce assign_page

[Julien Grall]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> In order to deal with the trouble of count-to-order conversion when page number
> is not in a power-of-two, this commit re-define assign_pages for nr pages and
> assign_page for original page with a single order.
> 
> Backporting confusion could be helped by altering the order of assign_page
> parameters, such that the compiler would point out that adjustments at call
> sites are needed.

Looking at the code, you don't alter the order of assign_page() 
parameters. So did you mean to refer to "assign_pages()"?

> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> v4 change:
> - in all cases where order-0 pages get passed, prefer using assign_pages
> to pass literal 1
> - reconstruct the order of assign_pages parameters
> - remove the unnecessary parentheses
> ---
>   xen/arch/x86/pv/dom0_build.c |  2 +-
>   xen/common/grant_table.c     |  2 +-
>   xen/common/memory.c          |  4 ++--
>   xen/common/page_alloc.c      | 23 ++++++++++++++---------
>   xen/include/xen/mm.h         |  6 ++++++
>   5 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index af47615b22..9142f359da 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -556,7 +556,7 @@ int __init dom0_construct_pv(struct domain *d,
>           else
>           {
>               while ( count-- )
> -                if ( assign_pages(d, mfn_to_page(_mfn(mfn++)), 0, 0) )
> +                if ( assign_pages(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
>                       BUG();
>           }
>           initrd->mod_end = 0;
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index fab77ab9cc..1f6b89bff4 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2342,7 +2342,7 @@ gnttab_transfer(
>            * is respected and speculative execution is blocked accordingly
>            */
>           if ( unlikely(!evaluate_nospec(okay)) ||
> -            unlikely(assign_pages(e, page, 0, MEMF_no_refcount)) )
> +            unlikely(assign_pages(page, 1, e, MEMF_no_refcount)) )
>           {
>               bool drop_dom_ref;
>   
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index e07bd9a5ea..083e14b84f 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -728,7 +728,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>           /* Assign each output page to the domain. */
>           for ( j = 0; (page = page_list_remove_head(&out_chunk_list)); ++j )
>           {
> -            if ( assign_pages(d, page, exch.out.extent_order,
> +            if ( assign_page(d, page, exch.out.extent_order,
>                                 MEMF_no_refcount) )
>               {
>                   unsigned long dec_count;
> @@ -797,7 +797,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>        * cleared PGC_allocated.
>        */
>       while ( (page = page_list_remove_head(&in_chunk_list)) )
> -        if ( assign_pages(d, page, 0, MEMF_no_refcount) )
> +        if ( assign_pages(page, 1, d, MEMF_no_refcount) )
>           {
>               BUG_ON(!d->is_dying);
>               free_domheap_page(page);
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index f51e406401..e279c6f713 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2282,9 +2282,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
>   
>   
>   int assign_pages(
> -    struct domain *d,
>       struct page_info *pg,
> -    unsigned int order,
> +    unsigned long nr,
> +    struct domain *d,
>       unsigned int memflags)
>   {
>       int rc = 0;
> @@ -2304,7 +2304,7 @@ int assign_pages(
>       {
>           unsigned int extra_pages = 0;
>   
> -        for ( i = 0; i < (1ul << order); i++ )
> +        for ( i = 0; i < nr; i++ )
>           {
>               ASSERT(!(pg[i].count_info & ~PGC_extra));
>               if ( pg[i].count_info & PGC_extra )
> @@ -2313,18 +2313,18 @@ int assign_pages(
>   
>           ASSERT(!extra_pages ||
>                  ((memflags & MEMF_no_refcount) &&
> -                extra_pages == 1u << order));
> +                extra_pages == nr));
>       }
>   #endif
>   
>       if ( pg[0].count_info & PGC_extra )
>       {
> -        d->extra_pages += 1u << order;
> +        d->extra_pages += nr;
>           memflags &= ~MEMF_no_refcount;
>       }
>       else if ( !(memflags & MEMF_no_refcount) )
>       {
> -        unsigned int tot_pages = domain_tot_pages(d) + (1 << order);
> +        unsigned int tot_pages = domain_tot_pages(d) + nr;
>   
>           if ( unlikely(tot_pages > d->max_pages) )
>           {
> @@ -2336,10 +2336,10 @@ int assign_pages(
>       }
>   
>       if ( !(memflags & MEMF_no_refcount) &&
> -         unlikely(domain_adjust_tot_pages(d, 1 << order) == (1 << order)) )
> +         unlikely(domain_adjust_tot_pages(d, nr) == nr) )
>           get_knownalive_domain(d);
>   
> -    for ( i = 0; i < (1 << order); i++ )
> +    for ( i = 0; i < nr; i++ )
>       {
>           ASSERT(page_get_owner(&pg[i]) == NULL);
>           page_set_owner(&pg[i], d);
> @@ -2354,6 +2354,11 @@ int assign_pages(
>       return rc;
>   }
>   
> +int assign_page(struct domain *d, struct page_info *pg, unsigned int order,
> +                unsigned int memflags)
> +{
> +    return assign_pages(pg, 1UL << order, d, memflags);
> +}
>   
>   struct page_info *alloc_domheap_pages(
>       struct domain *d, unsigned int order, unsigned int memflags)
> @@ -2396,7 +2401,7 @@ struct page_info *alloc_domheap_pages(
>                   pg[i].count_info = PGC_extra;
>               }
>           }
> -        if ( assign_pages(d, pg, order, memflags) )
> +        if ( assign_page(d, pg, order, memflags) )
>           {
>               free_heap_pages(pg, order, memflags & MEMF_no_scrub);
>               return NULL;
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 8e8fb5a615..2e75cdcbb7 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -132,6 +132,12 @@ int query_page_offline(mfn_t mfn, uint32_t *status);
>   void heap_init_late(void);
>   
>   int assign_pages(
> +    struct page_info *pg,
> +    unsigned long nr,
> +    struct domain *d,
> +    unsigned int memflags);
> +
> +int assign_page(
>       struct domain *d,
>       struct page_info *pg,
>       unsigned int order,

I find a bit odd that the parameters are ordered differently between 
assign_pages() and assign_page(). They are similar interface after all.

I don't think it would be a problem for backporting purpose if 
assign_page() has a different order for the arguments.

Jan, what do you think?

Cheers,

-- 
Julien Grall

Re: [PATCH V4 07/10] xen: re-define assign_pages and introduce assign_page

[Jan Beulich]

On 13.08.2021 14:27, Julien Grall wrote:
> On 28/07/2021 11:27, Penny Zheng wrote:
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -132,6 +132,12 @@ int query_page_offline(mfn_t mfn, uint32_t *status);
>>   void heap_init_late(void);
>>   
>>   int assign_pages(
>> +    struct page_info *pg,
>> +    unsigned long nr,
>> +    struct domain *d,
>> +    unsigned int memflags);
>> +
>> +int assign_page(
>>       struct domain *d,
>>       struct page_info *pg,
>>       unsigned int order,
> 
> I find a bit odd that the parameters are ordered differently between 
> assign_pages() and assign_page(). They are similar interface after all.
> 
> I don't think it would be a problem for backporting purpose if 
> assign_page() has a different order for the arguments.
> 
> Jan, what do you think?

Having both functions with similar parameter arrangement would
certainly seem better to me, too.

Jan

Re: [PATCH V4 05/10] xen/arm: static memory initialization

[Julien Grall]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> +/* Static memory initialization */
> +static void __init init_staticmem_pages(void)
> +{
> +    unsigned int bank;
> +
> +    /* TODO: Considering NUMA-support scenario. */

I forgot to ask about this. What do you expect to be different with NUMA?

> +    for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ )
> +    {
> +        unsigned long bank_start = PFN_UP(bootinfo.static_mem.bank[bank].start);
> +        unsigned long bank_size = PFN_DOWN(bootinfo.static_mem.bank[bank].size);
> +        unsigned long bank_end = bank_start + bank_size;
> +
> +        if ( bank_end <= bank_start )
> +            return;
> +
> +        free_staticmem_pages(mfn_to_page(_mfn(bank_start)),
> +                             bank_size, false);
> +    }
> +}
> +

Cheers,

-- 
Julien Grall

Re: [PATCH V4 08/10] xen/arm: introduce acquire_staticmem_pages and acquire_domstatic_pages

[Julien Grall]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> alloc_staticmem_pages aims to acquire nr_mfns contiguous pages of
> static memory. And it is the equivalent of alloc_heap_pages for static
> memory. Here only covers acquiring pre-configured static memory.
> 
> For each page, it shall check if the page is reserved(PGC_reserved)
> and free. It shall also do a set of necessary initialization, which are
> mostly the same ones in alloc_heap_pages, like, following the same
> cache-coherency policy and turning page status into PGC_state_inuse, etc.
> 
> acquire_domstatic_pages is the equivalent of alloc_domheap_pages for
> static memory, and it is to acquire nr_mfns contiguous pages of static memory
> and assign them to one specific domain.
> 
> It uses acquire_staticmem_pages to acquire nr_mfns pre-configured pages of
> static memory, then on success, it will use assign_pages to assign those pages
> to one specific domain.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> v4 change:
> - moving tlb/cache flush outside of the locked region, considering XSA-364
> and reducing the amount of work happening with the heap_lock held
> - remove MEMF_no_refcount case
> - make acquire_staticmem_pages/acquire_domstatic_pages being __init
> ---
>   xen/common/page_alloc.c | 108 +++++++++++++++++++++++++++++++++++++++-
>   xen/include/xen/mm.h    |   3 ++
>   2 files changed, 109 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index e279c6f713..b0edaf12b3 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -151,6 +151,10 @@
>   #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
>   #endif
>   
> +#ifndef CONFIG_STATIC_MEMORY
> +#define PGC_reserved 0
> +#endif
> +
>   /*
>    * Comma-separated list of hexadecimal page numbers containing bad bytes.
>    * e.g. 'badpage=0x3f45,0x8a321'.
> @@ -1065,6 +1069,73 @@ static struct page_info *alloc_heap_pages(
>       return pg;
>   }
>   
> +#ifdef CONFIG_STATIC_MEMORY

Rather than having multiple #ifdef in the code. Could we bundle all the 
functions for static allocation in a single place?

> +/*
> + * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of
> + * static memory.
> + */
> +static struct page_info * __init acquire_staticmem_pages(unsigned long nr_mfns,
> +                                                         mfn_t smfn,
> +                                                         unsigned int memflags)

NIT: I find more intuitive if we pass the start MFN first and then the 
number of pages. So this can be seen as a range.

If you agree with that, then the caller would also have to be changed.

> +{
> +    bool need_tlbflush = false;
> +    uint32_t tlbflush_timestamp = 0;
> +    unsigned long i;
> +    struct page_info *pg;
> +
> +    /* For now, it only supports pre-configured static memory. */

This comment doesn't seem to match the check below.

> +    if ( !mfn_valid(smfn) || !nr_mfns )

This check only guarantees that there will be a page for the first MFN. 
Shouldn't we also check for the other MFNs?

> +        return NULL;
> +
> +    spin_lock(&heap_lock);
> +
> +    pg = mfn_to_page(smfn);
> +
> +    for ( i = 0; i < nr_mfns; i++ )
> +    {
> +        /*
> +         * Reference count must continuously be zero for free pages
> +         * of static memory(PGC_reserved).
> +         */

How about: "The page should be reserved and not yet allocated"?

> +        if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
> +        {
> +            printk(XENLOG_ERR
> +                   "pg[%lu] Static MFN %"PRI_mfn" c=%#lx t=%#x\n",
> +                   i, mfn_x(page_to_mfn(pg + i)),
> +                   pg[i].count_info, pg[i].tlbflush_timestamp);
> +            BUG();

This BUG() can be easily hit by misconfiguring the Device-Tree. I think 
it would be best if we return an error and revert the changes.

> +        }
> +
> +        if ( !(memflags & MEMF_no_tlbflush) )
> +            accumulate_tlbflush(&need_tlbflush, &pg[i],
> +                                &tlbflush_timestamp);
> +
> +        /*
> +         * Preserve flag PGC_reserved and change page state
> +         * to PGC_state_inuse.
> +         */
> +        pg[i].count_info = (PGC_reserved | PGC_state_inuse);
> +        /* Initialise fields which have other uses for free pages. */
> +        pg[i].u.inuse.type_info = 0;
> +        page_set_owner(&pg[i], NULL);
> +    }
> +
> +    spin_unlock(&heap_lock);
> +
> +    if ( need_tlbflush )
> +        filtered_flush_tlb_mask(tlbflush_timestamp);
> +
> +    /*
> +     * Ensure cache and RAM are consistent for platforms where the guest
> +     * can control its own visibility of/through the cache.
> +     */
> +    for ( i = 0; i < nr_mfns; i++ )
> +        flush_page_to_ram(mfn_x(smfn) + i, !(memflags & MEMF_no_icache_flush));
> +
> +    return pg;
> +}
> +#endif
> +
>   /* Remove any offlined page in the buddy pointed to by head. */
>   static int reserve_offlined_page(struct page_info *head)
>   {
> @@ -2306,7 +2377,7 @@ int assign_pages(
>   
>           for ( i = 0; i < nr; i++ )
>           {
> -            ASSERT(!(pg[i].count_info & ~PGC_extra));
> +            ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_reserved)));
>               if ( pg[i].count_info & PGC_extra )
>                   extra_pages++;
>           }
> @@ -2345,7 +2416,8 @@ int assign_pages(
>           page_set_owner(&pg[i], d);
>           smp_wmb(); /* Domain pointer must be visible before updating refcnt. */
>           pg[i].count_info =
> -            (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
> +            (pg[i].count_info & (PGC_extra | PGC_reserved)) | PGC_allocated | 1;
> +
>           page_list_add_tail(&pg[i], page_to_list(d, &pg[i]));
>       }
>   
> @@ -2411,6 +2483,38 @@ struct page_info *alloc_domheap_pages(
>       return pg;
>   }
>   
> +#ifdef CONFIG_STATIC_MEMORY
> +/*
> + * Acquire nr_mfns contiguous pages, starting at #smfn, of static memory,
> + * then assign them to one specific domain #d.
> + */
> +struct page_info * __init acquire_domstatic_pages(struct domain *d,
> +                                                  unsigned long nr_mfns,
> +                                                  mfn_t smfn, unsigned int memflags)
> +{
> +    struct page_info *pg = NULL;
> +
> +    ASSERT(!in_irq());
> +
> +    pg = acquire_staticmem_pages(nr_mfns, smfn, memflags);
> +    if ( !pg )
> +        return NULL;
> +
> +    /*
> +     * MEMF_no_owner/MEMF_no_refcount cases are missing here because
> +     * right now, acquired static memory is only for guest RAM.
> +     */
> +    ASSERT(d);
> +    if ( assign_pages(pg, nr_mfns, d, memflags) )
> +    {
> +        free_staticmem_pages(pg, nr_mfns, memflags & MEMF_no_scrub);
> +        return NULL;
> +    }
> +
> +    return pg;
> +}
> +#endif
> +
>   void free_domheap_pages(struct page_info *pg, unsigned int order)
>   {
>       struct domain *d = page_get_owner(pg);
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 2e75cdcbb7..62e8e2ad61 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -89,6 +89,9 @@ bool scrub_free_pages(void);
>   /* These functions are for static memory */
>   void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>                             bool need_scrub);
> +struct page_info *acquire_domstatic_pages(struct domain *d,
> +                                          unsigned long nr_mfns, mfn_t smfn,
> +                                          unsigned int memflags);
>   #endif
>   
>   /* Map machine page range in Xen virtual address space. */
> 

Cheers,

-- 
Julien Grall

Re: [PATCH V4 09/10] xen/arm: check "xen,static-mem" property during domain construction

[Julien Grall]

Hi,

On 28/07/2021 11:27, Penny Zheng wrote:
> This commit checks "xen,static-mem" device tree property in /domUx node,
> to determine whether domain is on Static Allocation, when constructing
> domain during boot-up.
> 
> Right now, the implementation of allocate_static_memory is missing, and
> will be introduced later. It just BUG() out at the moment.

I think the code is small enough to fold it in patch #10. In fact...

> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   xen/arch/arm/domain_build.c | 37 ++++++++++++++++++++++++++++++++++++-
>   1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 6c86d52781..cdb16f2086 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2425,6 +2425,37 @@ static int __init construct_domU(struct domain *d,
>       struct kernel_info kinfo = {};
>       int rc;
>       u64 mem;
> +    const struct dt_property *static_mem_prop;
> +    u32 static_mem_addr_cells, static_mem_size_cells;
> +    bool static_mem = false;

You don't need those information outside of allocate_static_memory(). So 
I think it would be best to move the code in that function.

> +
> +    /*
> +     * Guest RAM could be static memory which will be specified through
> +     * "xen,static-mem" property.
> +     */
> +    static_mem_prop = dt_find_property(node, "xen,static-mem", NULL);
> +    if ( static_mem_prop )
> +    {
> +        static_mem = true;
> +
> +        if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
> +                                   &static_mem_addr_cells) )
> +        {
> +            printk("Error building DomU: cannot read "
> +                   "\"#xen,static-mem-address-cells\" property\n");
We don't split comment over multi-line (even they are more than 80 
characters). This is to help grep message in the code.

Although for this one I would replaced "Error building Domu:" with 
simply with the domain ID (you can use %pd and 'd'). The caller will 
then print there was an error during building.


> +            return -EINVAL;
> +        }
> +
> +        if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
> +                                   &static_mem_size_cells) )
> +        {
> +            printk("Error building DomU: cannot read "
> +                   "\"#xen,static-mem-size-cells\" property\n");

My remark applies here as well.

> +            return -EINVAL;
> +        }
> +
> +        BUG_ON(static_mem_size_cells > 2 || static_mem_addr_cells > 2);

Did we validate the DT before hand? If not, then I think

> +    }


>   
>       rc = dt_property_read_u64(node, "memory", &mem);
>       if ( !rc )
> @@ -2452,7 +2483,11 @@ static int __init construct_domU(struct domain *d,
>       /* type must be set before allocate memory */
>       d->arch.type = kinfo.type;
>   #endif
> -    allocate_memory(d, &kinfo);
> +    if ( !static_mem )

With my suggestion above, the check can be replaced with:

if ( !dt_find_property(node, "xen,static-mem", NULL) )

> +        allocate_memory(d, &kinfo);
> +    else
> +        /* TODO: allocate_static_memory(...). */
> +        BUG();
>   
>       rc = prepare_dtb_domU(d, &kinfo);
>       if ( rc < 0 )
> 

Cheers,

-- 
Julien Grall

Re: [PATCH V4 10/10] xen/arm: introduce allocate_static_memory

[Julien Grall]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> This commit introduces allocate_static_memory to allocate static memory as
> guest RAM for Domain on Static Allocation.
> 
> It uses acquire_domstatic_pages to acquire pre-configured static memory
> for this domain, and uses guest_physmap_add_page to set up P2M table.
> These pre-defined static memory banks shall be firstly mapped to the
> fixed guest RAM address `GUEST_RAM0_BASE`. And until it exhausts the
> `GUEST_RAM0_SIZE`, it will seek to `GUEST_RAM1_BASE`, and so on.
> `GUEST_RAM0` may take up several pre-defined physical RAM regions.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   xen/arch/arm/domain_build.c | 137 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 135 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index cdb16f2086..ed290ee31b 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -480,6 +480,139 @@ fail:
>             (unsigned long)kinfo->unassigned_mem >> 10);
>   }
>   
> +static bool __init append_static_memory_to_bank(struct domain *d,
> +                                                struct membank *bank,
> +                                                mfn_t smfn,
> +                                                paddr_t size)
> +{
> +    int res;
> +    paddr_t tot_size = size;
> +    /* Infer next GFN. */
> +    gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
> +
> +    while ( tot_size > 0 )
> +    {
> +        unsigned int order = get_allocation_size(tot_size);
> +
> +        res = guest_physmap_add_page(d, sgfn, smfn, order);
> +        if ( res )
> +        {
> +            dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> +            return false;
> +        }
> +
> +        smfn = mfn_add(smfn, 1UL << order);
> +        tot_size -= (1UL << (PAGE_SHIFT + order));
> +    }

AFAICT, the loop is only here to suit guest_physmap_add_page(). Further 
down the line, the order will be converted back to a number of pages 
before calling p2m_insert_mapping().

So how about exporting p2m_insert_mapping() and use it?

> + > +    bank->size = bank->size + size;

We usually add a newline before the last return of the function.

> +    return true;
> +}
> +
> +/* Allocate memory from static memory as RAM for one specific domain d. */
> +static void __init allocate_static_memory(struct domain *d,
> +                                          struct kernel_info *kinfo,
> +                                          const struct dt_property *prop,
> +                                          u32 addr_cells, u32 size_cells)
> +{
> +    unsigned int nr_banks, gbank, bank = 0;
> +    const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
> +    const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
> +    const __be32 *cell;
> +    u32 reg_cells = addr_cells + size_cells;
> +    u64 tot_size = 0;
> +    paddr_t pbase, psize, gsize;
> +    mfn_t smfn;
> +
> +    /* Start with GUEST_RAM0. */
> +    kinfo->mem.nr_banks = 0;
> +    gbank = 0;
> +    gsize = ramsize[gbank];
> +    kinfo->mem.bank[gbank].start = rambase[gbank];
> +
> +    cell = (const __be32 *)prop->value;
> +    nr_banks = (prop->length) / (reg_cells * sizeof (u32));
> +    BUG_ON(nr_banks > NR_MEM_BANKS);
> +
> +    while ( bank < nr_banks )
> +    {
> +        device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize);
> +        tot_size += psize;
> +        smfn = maddr_to_mfn(pbase);
> +
> +        if ( !acquire_domstatic_pages(d, psize >> PAGE_SHIFT, smfn, 0) )

I think we want to check that both pbase and psize are page aligned 
first. This can be done here or earlier when reserving the pages (this 
would be a previous patch).

Also, given that you can easily figure out the page from the mfn. I 
think it would be better for acquire_domstatic_pages() to return an 
error. This could be helpful to figure out an error.

> +        {
> +            printk(XENLOG_ERR
> +                    "%pd: cannot acquire static memory "
> +                    "(0x%"PRIpaddr" - 0x%"PRIpaddr").\n",
> +                    d, pbase, pbase + psize);
> +            goto fail;
> +        }
> +
> +        printk(XENLOG_INFO "%pd: STATIC BANK[%d] %#"PRIpaddr"-%#"PRIpaddr"\n",

bank is unsigned so s/%d/%u/

> +               d, bank, pbase, pbase + psize);
> +
> +        /*
> +         * It shall be mapped to the fixed guest RAM address rambase[i],
> +         * And until it exhausts the ramsize[i], it will seek to the next
> +         * rambase[i+1].
> +         */
> +        while ( 1 )
> +        {
> +            /*
> +             * The current physical bank is fully mapped.
> +             * Handle the next physical bank.
> +             */
> +            if ( gsize >= psize )
> +            {
> +                if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> +                                                   smfn, psize) )
> +                    goto fail;
> +
> +                gsize = gsize - psize;
> +                bank++;
> +                break;
> +            }
> +            /*
> +             * Current guest bank memory is not enough to map.
> +             * Check if we have another guest bank available.
> +             * gbank refers guest memory bank index.
> +             */
> +            else if ( (gbank + 2) > GUEST_RAM_BANKS ) {

I don't understand the +2. Can you clarify it?

Also, the coding style for Xen requires the { to be on a separate line.

> +                printk("Exhausted the number of guest bank\n");
> +                goto fail;
> +            }
> +            else
> +            {
> +                if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> +                                                   smfn, gsize) )
> +                    goto fail;

As I may have mentionned earlier, I find the double loop quite difficult 
to read. I don't think we can drop the double loop, but we can at least 
try to simplify the code in the loops.

The one I can think right now is moving allocate_static_memory_to_bank() 
outside of the if/else. Something like:

/* Map as much as possible the static range to the guest bank */
if ( !allocate_static_bank(.., min(psize, gize)) )

> +
> +                psize = psize - gsize;
> +                smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> +                /* Update to the next guest bank. */
> +                gbank++;
> +                gsize = ramsize[gbank];
> +                kinfo->mem.bank[gbank].start = rambase[gbank];
> +            }
> +        }
> +    }
> +
> +    kinfo->mem.nr_banks = ++gbank;
> +    kinfo->unassigned_mem -= tot_size;
> +    if ( kinfo->unassigned_mem )
> +        printk(XENLOG_ERR
> +               "Size of \"memory\" property doesn't match up with the ones "
> +               "defined in \"xen,static-mem\".\n");

We don't split the single line message accross multi-line even if the 
result code is more than 80 characters long.

> +
> +    return;
> +
> +fail:
> +    panic("Failed to allocate requested static memory for domain %pd."
> +          "Fix the VMs configurations.\n",

Same here.

> +          d);
> +}
> +
>   static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>                                      const struct dt_device_node *node)
>   {
> @@ -2486,8 +2619,8 @@ static int __init construct_domU(struct domain *d,
>       if ( !static_mem )
>           allocate_memory(d, &kinfo);
>       else
> -        /* TODO: allocate_static_memory(...). */
> -        BUG();
> +        allocate_static_memory(d, &kinfo, static_mem_prop,
> +                               static_mem_addr_cells, static_mem_size_cells);
>   
>       rc = prepare_dtb_domU(d, &kinfo);
>       if ( rc < 0 )
> 

Cheers,

-- 
Julien Grall

RE: [PATCH V4 01/10] xen/arm: introduce domain on Static Allocation

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Wednesday, August 11, 2021 9:32 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 01/10] xen/arm: introduce domain on Static Allocation
> 
> Hi Penny,
> 
> On 28/07/2021 11:27, Penny Zheng wrote:
> > Static Allocation refers to system or sub-system(domains) for which
> > memory areas are pre-defined by configuration using physical address
> ranges.
> > Those pre-defined memory, -- Static Memory, as parts of RAM reserved
> > in the beginning, shall never go to heap allocator or boot allocator for any
> use.
> >
> > Domains on Static Allocation is supported through device tree property
> > `xen,static-mem` specifying reserved RAM banks as this domain's guest RAM.
> > By default, they shall be mapped to the fixed guest RAM address
> > `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`.
> >
> > This patch introduces this new `xen,static-mem` feature, and also
> > documents and parses this new attribute at boot time and stores
> > related info in static_mem for later initialization.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> >   docs/misc/arm/device-tree/booting.txt | 40 +++++++++++++++++++++
> >   xen/arch/arm/bootfdt.c                | 51 +++++++++++++++++++++++++++
> >   xen/include/asm-arm/setup.h           |  2 ++
> >   3 files changed, 93 insertions(+)
> >
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 5243bc7fd3..2a1ddca29b 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -268,3 +268,43 @@ The DTB fragment is loaded at 0xc000000 in the
> example above. It should
> >   follow the convention explained in docs/misc/arm/passthrough.txt. The
> >   DTB fragment will be added to the guest device tree, so that the guest
> >   kernel will be able to discover the device.
> > +
> > +
> > +Static Allocation
> > +=============
> > +
> > +Static Allocation refers to system or sub-system(domains) for which
> > +memory areas are pre-defined by configuration using physical address
> ranges.
> > +Those pre-defined memory, -- Static Memory, as parts of RAM reserved
> > +in the beginning, shall never go to heap allocator or boot allocator for any
> use.
> 
> I don't understand "as parts of RAM reserved in the beginning". Could you
> clarify it?
> 

I mean, static memory is very alike reserved memory, reserved during system boot time,
not dynamically allocated at runtime.

> > +
> > +Domains on Static Allocation is supported through static memory
> > +property, defined under according /domUx in the name of
> > +"xen,static-mem", which are
> 
> We don't require the domU node to be called /domUx.
> 

Oh, thx for explanation. I will take domU node instead.

> > +specifying physical RAM as this domain's guest RAM.
> >
> 
> How about:
> 
> Memory can be statically allocated to a domain using the property "xen,static-
> mem" defined in the domain configuration.
> 
> > +The size of address-cells/size-cells must be defined in
> 
> I would say "The number of cells for the address and the size must be defined
> using respectively the properties..."
> 

Sure. Thx for the rephrasing.

> > +"#xen,static-mem-address-cells" and "#xen,static-mem-size-cells".
> > +
> > +On memory allocation, these pre-defined static memory ranges shall be
> > +firstly mapped to the fixed guest bank "GUEST_RAM0". Until it
> > +exhausts the `GUEST_RAM0_SIZE`, then it will seek to `GUEST_RAM1_BASE`,
> and so on.
> > +`GUEST_RAM0` may take up several pre-defined physical RAM regions.
> 
> GUEST_RAM0 & co are not part of the stable ABI. So I don't think the
> documentation should mention them.
> 
> But I am not convinced we should provide a guarantee how the allocation will
> happen. Why does it matter?
> 

Yeah, I put it here to be in comparison with the later 1:1 direct-map, however, it is truly not
part of the stable ABI, so I will delete it in documentation here,

> > +
> > +The dtb property should look like as follows:
> 
> Do you mean "node" rather than "property"?
> 

Oh, sure. Maybe "as an example" shall be more clarified.

> > +                compatible = "xen,domain";
> > +                #address-cells = <0x2>;
> > +                #size-cells = <0x2>;
> > +                cpus = <2>;
> > +                #xen,static-mem-address-cells = <0x1>;
> > +                #xen,static-mem-size-cells = <0x1>;
> > +                xen,static-mem = <0x30000000 0x20000000>;
> > +                ...
> > +            };
> > +        };
> > +    };
> > +
> > +DomU1 will have a static memory of 512MB reserved from the physical
> > +address
> > +0x30000000 to 0x50000000.
> 
> I would write "This will reserve a 512MB region starting at the host physical
> address 0x30000000 to be exclusively used by DomU1".
>

Sure, thx.
 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index
> > 476e32e0f5..d2714446e1 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -193,6 +193,55 @@ static int __init
> process_reserved_memory_node(const void *fdt, int node,
> >       return 0;
> >   }
> >
> > +static int __init process_static_memory(const void *fdt, int node,
> > +void *data) {
> 
> This is pretty much a copy of process_memory_node(). So can we avoid the
> duplication?
> 
> I think I mentionned it in the past but I can't find the outcome.
> 
> > +    int i = 0, banks;
> > +    const __be32 *cell;
> > +    paddr_t start, size;
> > +    u32 address_cells, size_cells, reg_cells;
> > +    struct meminfo *mem = data;
> > +    const struct fdt_property *prop;
> > +
> > +
> > +    address_cells = device_tree_get_u32(fdt, node,
> > +                                        "#xen,static-mem-address-cells", 0);
> > +    size_cells = device_tree_get_u32(fdt, node,
> > +                                     "#xen,static-mem-size-cells", 0);
> > +    if ( (address_cells == 0) || (size_cells == 0) )
> > +    {
> > +         printk("Missing \"#xen,static-mem-address-cell\" or "
> > +                 "\"#xen,static-mem-address-cell\".\n");
> > +         return -EINVAL;
> > +    }
> > +    reg_cells = address_cells + size_cells;
> > +
> > +    prop = fdt_get_property(fdt, node, "xen,static-mem", NULL);
> > +    /*
> > +     * Static memory shall belong to a specific domain, that is,
> > +     * its node `domUx` has compatible string "xen,domain".
> > +     */
> 
> This code is just checking the node compatible is "xen,domain". So I would
> drop the "domUx". This is also...
> 
> > +    if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
> > +    {
> > +        printk("xen,static-mem property can only be located under
> > + /domUx node.\n");
> 
> ... not correct.
> 

I checked it here, to make sure the "xen,static-mem" property must be used in a domain node, since
for now, static memory could be only configured as guest RAM. 

Which part do you think it is not appropriate here?

> > +        return -EINVAL;
> > +    }
> > +
> > +    cell = (const __be32 *)prop->data;
> > +    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> > +
> > +    for ( ; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
> > +    {
> > +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> > +        mem->bank[mem->nr_banks].start = start;
> > +        mem->bank[mem->nr_banks].size = size;
> > +        mem->nr_banks++;
> > +    }
> > +
> > +    if ( i < banks )
> > +        return -ENOSPC;
> > +    return 0;
> > +}
> > +
> >   static int __init process_reserved_memory(const void *fdt, int node,
> >                                             const char *name, int depth,
> >                                             u32 address_cells, u32
> > size_cells) @@ -346,6 +395,8 @@ static int __init early_scan_node(const
> void *fdt,
> >           process_multiboot_node(fdt, node, name, address_cells, size_cells);
> >       else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
> >           process_chosen_node(fdt, node, name, address_cells,
> > size_cells);
> > +    else if ( depth == 2 && fdt_get_property(fdt, node,
> > + "xen,static-mem", NULL) )
> 
> How about checking the compatible instead?
>

hmm, since it is a property, not a node. so...
 
> > +        process_static_memory(fdt, node, &bootinfo.static_mem);
> 
> You want "rc = ..." so the error is propaged if there is an issue (e.g.
> we don't have space for more static region).
> 

Yes, my neglect. I'll make sure the error get propagated here.

> >
> >       if ( rc < 0 )
> >           printk("fdt: node `%s': parsing failed\n", name); diff --git
> > a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h index
> > c4b6af6029..e076329fc4 100644
> > --- a/xen/include/asm-arm/setup.h
> > +++ b/xen/include/asm-arm/setup.h
> > @@ -74,6 +74,8 @@ struct bootinfo {
> >   #ifdef CONFIG_ACPI
> >       struct meminfo acpi;
> >   #endif
> > +    /* Static Memory */
> > +    struct meminfo static_mem;
> >   };
> >
> >   extern struct bootinfo bootinfo;
> >
> 
> Cheers,
> 
> --

Cheers

Penny
> Julien Grall

RE: [PATCH V4 02/10] xen/arm: introduce new helper device_tree_get_meminfo

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Wednesday, August 11, 2021 9:35 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 02/10] xen/arm: introduce new helper
> device_tree_get_meminfo
> 
> Hi Penny,
> 
> On 28/07/2021 11:27, Penny Zheng wrote:
> > A few functions iterate over the device tree property to get memory
> > info, like "reg" or "xen,static-mem", so this commit creates a new
> > helper device_tree_get_meminfo to extract the common codes.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> 
> Ah, this is where you did the consolidation. Sorry, I didn't notice this patch.
> 
> In general, we are avoiding to introduce code and then rework it in the same
> series. Instead, the rework is done first and then the function is used.
> 
> So can you move this patch first?
> 

Thx for the explanation. I'll reorganize. ;)

> Cheers,
> 
> --

Cheers,

Penny


> Julien Grall

RE: [PATCH V4 03/10] xen/arm: handle static memory in dt_unreserved_regions

[Penny Zheng]

Hi Julien 

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Wednesday, August 11, 2021 9:48 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 03/10] xen/arm: handle static memory in
> dt_unreserved_regions
> 
> Hi Penny,
> 
> On 28/07/2021 11:27, Penny Zheng wrote:
> > static memory regions overlap with memory nodes. The overlapping
> > memory is reserved-memory and should be handled accordingly:
> > dt_unreserved_regions should skip these regions the same way they are
> > already skipping mem-reserved regions.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> >   xen/arch/arm/setup.c | 47 ++++++++++++++++++++++++++++----------------
> >   1 file changed, 30 insertions(+), 17 deletions(-)
> >
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index
> > 63a908e325..f569134317 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -200,6 +200,13 @@ static void __init dt_unreserved_regions(paddr_t s,
> paddr_t e,
> >                                            int first)
> >   {
> >       int i, nr = fdt_num_mem_rsv(device_tree_flattened);
> > +    /*
> > +     * There are two types of reserved memory stored in bootinfo, one
> defines
> > +     * in /reserved-memory node, the other refers to domain on static
> allocation
> > +     * through "xen,static-mem" property.
> > +     */
> > +    int nr_rsv_type = 2, t = 0, prev_nr;
> > +    struct meminfo *rsv_type[2] = {&bootinfo.reserved_mem,
> > + &bootinfo.static_mem};
> 
> Looking at the rest of the series, it doesn't look like there is a real benefits to
> have the static memory and reserved memory in separate arrays as they are
> walked only a few times and they are both meant to be small. In fact, I think
> this code is lot more difficult to read.
> 
> So it would be best to merge the two arrays in one. We can add a flag in the
> structure to differentiate between "static" and "reserved" memory.
> 

How about adding a "static" flag in "struct meminfo" to tell. See the below example:
"
struct meminfo {
    int nr_banks;
    struct membank bank[NR_MEM_BANKS];
    bool static;  /* whether memory is reserved as static memory. */
};
"

And I will delete "struct meminfo static_mem" array, all "static" and "reserved" memory
will be stored in one "struct meminfo reserved_mem" array.

> Cheers,
> 
> --

Cheers,

--
> Julien Grall

RE: [PATCH V4 05/10] xen/arm: static memory initialization

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Friday, August 13, 2021 8:21 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 05/10] xen/arm: static memory initialization
> 
> Hi Penny,
> 
> On 28/07/2021 11:27, Penny Zheng wrote:
> > This patch introduces static memory initialization, during system boot up.
> >
> > The new function init_staticmem_pages is responsible for static memory
> > initialization.
> >
> > Helper free_staticmem_pages is the equivalent of free_heap_pages, to
> > free nr_mfns pages of static memory.
> >
> > This commit also introduces new CONFIG_STATIC_MEMORY to avoid
> bringing
> > dead codes in other archs.
> >
> > Put asynchronous scrubbing for pages of static memory in TODO list.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > v4 change:
> > - move the option CONFIG_STATIC_MEMORY to common code, and with
> Arm
> > "select"ing it
> > - replace round_pg{down,up}() with PFN_DOWN()/PFN_UP()
> > ---
> >   xen/arch/arm/Kconfig    |  1 +
> >   xen/arch/arm/setup.c    | 24 ++++++++++++++++++++++++
> >   xen/common/Kconfig      |  3 +++
> >   xen/common/page_alloc.c | 20 ++++++++++++++++++++
> >   xen/include/xen/mm.h    |  6 ++++++
> >   5 files changed, 54 insertions(+)
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index
> > ecfa6822e4..cc7a943d27 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -15,6 +15,7 @@ config ARM
> >   	select HAS_PASSTHROUGH
> >   	select HAS_PDX
> >   	select IOMMU_FORCE_PT_SHARE
> > +	select STATIC_MEMORY
> 
> Given the list of TODOs, I think it would be better if STATIC_MEMORY is user
> selectable and gated by UNSUPPORTED.
> 
> We can remove the dependency on UNSUPPORTED once every have been
> addressed.
> 

Sure. I'll change it UNSUPPORTED.
 
config STATIC_ALLOCATION
        bool "Static Allocation Support (UNSUPPORTED)" if UNSUPPORTED

> >
> >   config ARCH_DEFCONFIG
> >   	string
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index
> > f569134317..369f6631ee 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -622,6 +622,26 @@ static void __init init_pdx(void)
> >       }
> >   }
> >
> > +/* Static memory initialization */
> > +static void __init init_staticmem_pages(void) {
> > +    unsigned int bank;
> > +
> > +    /* TODO: Considering NUMA-support scenario. */
> > +    for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ )
> > +    {
> > +        unsigned long bank_start =
> > + PFN_UP(bootinfo.static_mem.bank[bank].start);
> 
> I would prefer if bank_start is a mfn_t.
> 

Sure, it's more appropriate.

> > +        unsigned long bank_size =
> > + PFN_DOWN(bootinfo.static_mem.bank[bank].size);
> 
> NIT: I would suggest to name it bank_pages or bank_nr_pages. This would
> make clear in the user that this contains pages.
> 

Sure.

> > +        unsigned long bank_end = bank_start + bank_size;
> 
> mfn_t please.
> 

Sure.

> > +
> > +        if ( bank_end <= bank_start )
> 
> This will mean you will need to use mfn_x() for both. This code would be less
> nice but at least it avoids mixing address and MFN.
>

Sure.

> > +            return;
> > +
> > +        free_staticmem_pages(mfn_to_page(_mfn(bank_start)),
> > +                             bank_size, false);
> > +    }
> > +}
> > +
> >   #ifdef CONFIG_ARM_32
> >   static void __init setup_mm(void)
> >   {
> > @@ -749,6 +769,8 @@ static void __init setup_mm(void)
> >       /* Add xenheap memory that was not already added to the boot allocator.
> */
> >       init_xenheap_pages(mfn_to_maddr(xenheap_mfn_start),
> >                          mfn_to_maddr(xenheap_mfn_end));
> > +
> > +    init_staticmem_pages();
> >   }
> >   #else /* CONFIG_ARM_64 */
> >   static void __init setup_mm(void)
> > @@ -802,6 +824,8 @@ static void __init setup_mm(void)
> >
> >       setup_frametable_mappings(ram_start, ram_end);
> >       max_page = PFN_DOWN(ram_end);
> > +
> > +    init_staticmem_pages();
> >   }
> >   #endif
> >
> > diff --git a/xen/common/Kconfig b/xen/common/Kconfig index
> > 0ddd18e11a..8f736eea82 100644
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -67,6 +67,9 @@ config MEM_ACCESS
> >   config NEEDS_LIBELF
> >   	bool
> >
> > +config STATIC_MEMORY
> > +	bool
> > +
> >   menu "Speculative hardening"
> >
> >   config SPECULATIVE_HARDEN_ARRAY
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > a3ee5eca9e..2acb73e323 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -1519,6 +1519,26 @@ static void free_heap_pages(
> >       spin_unlock(&heap_lock);
> >   }
> >
> > +#ifdef CONFIG_STATIC_MEMORY
> > +/* Equivalent of free_heap_pages to free nr_mfns pages of static
> > +memory. */ void __init free_staticmem_pages(struct page_info *pg,
> unsigned long nr_mfns,
> > +                                 bool need_scrub) {
> > +    mfn_t mfn = page_to_mfn(pg);
> > +    unsigned long i;
> > +
> > +    for ( i = 0; i < nr_mfns; i++ )
> > +    {
> > +        mark_page_free(&pg[i], mfn_add(mfn, i));
> > +
> > +        if ( need_scrub )
> > +        {
> > +            /* TODO: asynchronous scrubbing for pages of static memory. */
> > +            scrub_one_page(pg);
> > +        }
> > +    }
> > +}
> > +#endif
> >
> >   /*
> >    * Following rules applied for page offline:
> > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index
> > 667f9dac83..8e8fb5a615 100644
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -85,6 +85,12 @@ bool scrub_free_pages(void);
> >   } while ( false )
> >   #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
> >
> > +#ifdef CONFIG_STATIC_MEMORY
> > +/* These functions are for static memory */ void
> > +free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> > +                          bool need_scrub); #endif
> > +
> >   /* Map machine page range in Xen virtual address space. */
> >   int map_pages_to_xen(
> >       unsigned long virt,
> >
> 
> Cheers,
> 
> --

Cheers

--
> Julien Grall

RE: [PATCH V4 06/10] xen/arm: introduce PGC_reserved

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Friday, August 13, 2021 8:22 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 06/10] xen/arm: introduce PGC_reserved
> 
> Hi Penny,
> 
> On 28/07/2021 11:27, Penny Zheng wrote:
> > This patch introduces a new page flag PGC_reserved in order to
> > differentiate pages of static memory from those allocated from heap.
> >
> > Mark pages of static memory PGC_reserved when initializing them.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> 
> I think this want to be folded in patch #7.
> 

Ok. Will do.

> Cheers,
> 
> --

Cheers,

--
Penny Zheng

> Julien Grall

RE: [PATCH V4 08/10] xen/arm: introduce acquire_staticmem_pages and acquire_domstatic_pages

[Penny Zheng]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Friday, August 13, 2021 9:00 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 08/10] xen/arm: introduce acquire_staticmem_pages
> and acquire_domstatic_pages
> 
> Hi Penny,
> 
> On 28/07/2021 11:27, Penny Zheng wrote:
> > alloc_staticmem_pages aims to acquire nr_mfns contiguous pages of
> > static memory. And it is the equivalent of alloc_heap_pages for static
> > memory. Here only covers acquiring pre-configured static memory.
> >
> > For each page, it shall check if the page is reserved(PGC_reserved)
> > and free. It shall also do a set of necessary initialization, which
> > are mostly the same ones in alloc_heap_pages, like, following the same
> > cache-coherency policy and turning page status into PGC_state_inuse, etc.
> >
> > acquire_domstatic_pages is the equivalent of alloc_domheap_pages for
> > static memory, and it is to acquire nr_mfns contiguous pages of static
> > memory and assign them to one specific domain.
> >
> > It uses acquire_staticmem_pages to acquire nr_mfns pre-configured
> > pages of static memory, then on success, it will use assign_pages to
> > assign those pages to one specific domain.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > v4 change:
> > - moving tlb/cache flush outside of the locked region, considering
> > XSA-364 and reducing the amount of work happening with the heap_lock
> > held
> > - remove MEMF_no_refcount case
> > - make acquire_staticmem_pages/acquire_domstatic_pages being __init
> > ---
> >   xen/common/page_alloc.c | 108
> +++++++++++++++++++++++++++++++++++++++-
> >   xen/include/xen/mm.h    |   3 ++
> >   2 files changed, 109 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > e279c6f713..b0edaf12b3 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -151,6 +151,10 @@
> >   #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
> >   #endif
> >
> > +#ifndef CONFIG_STATIC_MEMORY
> > +#define PGC_reserved 0
> > +#endif
> > +
> >   /*
> >    * Comma-separated list of hexadecimal page numbers containing bad
> bytes.
> >    * e.g. 'badpage=0x3f45,0x8a321'.
> > @@ -1065,6 +1069,73 @@ static struct page_info *alloc_heap_pages(
> >       return pg;
> >   }
> >
> > +#ifdef CONFIG_STATIC_MEMORY
> 
> Rather than having multiple #ifdef in the code. Could we bundle all the
> functions for static allocation in a single place?
> 

Sure. I'll reorganize them. 

> > +/*
> > + * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of
> > + * static memory.
> > + */
> > +static struct page_info * __init acquire_staticmem_pages(unsigned long
> nr_mfns,
> > +                                                         mfn_t smfn,
> > +                                                         unsigned int
> > +memflags)
> 
> NIT: I find more intuitive if we pass the start MFN first and then the number of
> pages. So this can be seen as a range.
> 
> If you agree with that, then the caller would also have to be changed.
> 

Sure, it's more clear in your way.

> > +{
> > +    bool need_tlbflush = false;
> > +    uint32_t tlbflush_timestamp = 0;
> > +    unsigned long i;
> > +    struct page_info *pg;
> > +
> > +    /* For now, it only supports pre-configured static memory. */
> 
> This comment doesn't seem to match the check below.
> 
> > +    if ( !mfn_valid(smfn) || !nr_mfns )
> 
> This check only guarantees that there will be a page for the first MFN.
> Shouldn't we also check for the other MFNs?
> 

Hmm, Do you think that it should be all checked, the whole range, [smfn, smfn + nr_mfns).
Since it is in linear growth, maybe adding another check of "!mfn_valid(smfn + nr_mfns - 1)"
is enough?

> > +        return NULL;
> > +
> > +    spin_lock(&heap_lock);
> > +
> > +    pg = mfn_to_page(smfn);
> > +
> > +    for ( i = 0; i < nr_mfns; i++ )
> > +    {
> > +        /*
> > +         * Reference count must continuously be zero for free pages
> > +         * of static memory(PGC_reserved).
> > +         */
> 
> How about: "The page should be reserved and not yet allocated"?
>

Sure.

> > +        if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "pg[%lu] Static MFN %"PRI_mfn" c=%#lx t=%#x\n",
> > +                   i, mfn_x(page_to_mfn(pg + i)),
> > +                   pg[i].count_info, pg[i].tlbflush_timestamp);
> > +            BUG();
> 
> This BUG() can be easily hit by misconfiguring the Device-Tree. I think it would
> be best if we return an error and revert the changes.
> 

Ok. I'll return NULL.

And about the reverting part, do you mean changing the state of pages back to "PGC_state_free | PGC_reserved",
like something as follows:
"
out_error:
for ( unsigned long j = 0; j < i; j++ )
	pg[j].count_info = PGC_state_free | PGC_reserved;
"
 
> > +        }
> > +
> > +        if ( !(memflags & MEMF_no_tlbflush) )
> > +            accumulate_tlbflush(&need_tlbflush, &pg[i],
> > +                                &tlbflush_timestamp);
> > +
> > +        /*
> > +         * Preserve flag PGC_reserved and change page state
> > +         * to PGC_state_inuse.
> > +         */
> > +        pg[i].count_info = (PGC_reserved | PGC_state_inuse);
> > +        /* Initialise fields which have other uses for free pages. */
> > +        pg[i].u.inuse.type_info = 0;
> > +        page_set_owner(&pg[i], NULL);
> > +    }
> > +
> > +    spin_unlock(&heap_lock);
> > +
> > +    if ( need_tlbflush )
> > +        filtered_flush_tlb_mask(tlbflush_timestamp);
> > +
> > +    /*
> > +     * Ensure cache and RAM are consistent for platforms where the guest
> > +     * can control its own visibility of/through the cache.
> > +     */
> > +    for ( i = 0; i < nr_mfns; i++ )
> > +        flush_page_to_ram(mfn_x(smfn) + i, !(memflags &
> > + MEMF_no_icache_flush));
> > +
> > +    return pg;
> > +}
> > +#endif
> > +
> >   /* Remove any offlined page in the buddy pointed to by head. */
> >   static int reserve_offlined_page(struct page_info *head)
> >   {
> > @@ -2306,7 +2377,7 @@ int assign_pages(
> >
> >           for ( i = 0; i < nr; i++ )
> >           {
> > -            ASSERT(!(pg[i].count_info & ~PGC_extra));
> > +            ASSERT(!(pg[i].count_info & ~(PGC_extra |
> > + PGC_reserved)));
> >               if ( pg[i].count_info & PGC_extra )
> >                   extra_pages++;
> >           }
> > @@ -2345,7 +2416,8 @@ int assign_pages(
> >           page_set_owner(&pg[i], d);
> >           smp_wmb(); /* Domain pointer must be visible before updating refcnt.
> */
> >           pg[i].count_info =
> > -            (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
> > +            (pg[i].count_info & (PGC_extra | PGC_reserved)) |
> > + PGC_allocated | 1;
> > +
> >           page_list_add_tail(&pg[i], page_to_list(d, &pg[i]));
> >       }
> >
> > @@ -2411,6 +2483,38 @@ struct page_info *alloc_domheap_pages(
> >       return pg;
> >   }
> >
> > +#ifdef CONFIG_STATIC_MEMORY
> > +/*
> > + * Acquire nr_mfns contiguous pages, starting at #smfn, of static
> > +memory,
> > + * then assign them to one specific domain #d.
> > + */
> > +struct page_info * __init acquire_domstatic_pages(struct domain *d,
> > +                                                  unsigned long nr_mfns,
> > +                                                  mfn_t smfn,
> > +unsigned int memflags) {
> > +    struct page_info *pg = NULL;
> > +
> > +    ASSERT(!in_irq());
> > +
> > +    pg = acquire_staticmem_pages(nr_mfns, smfn, memflags);
> > +    if ( !pg )
> > +        return NULL;
> > +
> > +    /*
> > +     * MEMF_no_owner/MEMF_no_refcount cases are missing here because
> > +     * right now, acquired static memory is only for guest RAM.
> > +     */
> > +    ASSERT(d);
> > +    if ( assign_pages(pg, nr_mfns, d, memflags) )
> > +    {
> > +        free_staticmem_pages(pg, nr_mfns, memflags & MEMF_no_scrub);
> > +        return NULL;
> > +    }
> > +
> > +    return pg;
> > +}
> > +#endif
> > +
> >   void free_domheap_pages(struct page_info *pg, unsigned int order)
> >   {
> >       struct domain *d = page_get_owner(pg); diff --git
> > a/xen/include/xen/mm.h b/xen/include/xen/mm.h index
> > 2e75cdcbb7..62e8e2ad61 100644
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -89,6 +89,9 @@ bool scrub_free_pages(void);
> >   /* These functions are for static memory */
> >   void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> >                             bool need_scrub);
> > +struct page_info *acquire_domstatic_pages(struct domain *d,
> > +                                          unsigned long nr_mfns, mfn_t smfn,
> > +                                          unsigned int memflags);
> >   #endif
> >
> >   /* Map machine page range in Xen virtual address space. */
> >
> 
> Cheers,
> 
> --

Cheers

--
Penny Zheng
> Julien Grall

RE: [PATCH V4 09/10] xen/arm: check "xen,static-mem" property during domain construction

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Friday, August 13, 2021 9:12 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 09/10] xen/arm: check "xen,static-mem" property
> during domain construction
> 
> Hi,
> 
> On 28/07/2021 11:27, Penny Zheng wrote:
> > This commit checks "xen,static-mem" device tree property in /domUx
> > node, to determine whether domain is on Static Allocation, when
> > constructing domain during boot-up.
> >
> > Right now, the implementation of allocate_static_memory is missing,
> > and will be introduced later. It just BUG() out at the moment.
> 
> I think the code is small enough to fold it in patch #10. In fact...
> 

Ok. I'll combine.

> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> >   xen/arch/arm/domain_build.c | 37
> ++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 6c86d52781..cdb16f2086 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2425,6 +2425,37 @@ static int __init construct_domU(struct domain *d,
> >       struct kernel_info kinfo = {};
> >       int rc;
> >       u64 mem;
> > +    const struct dt_property *static_mem_prop;
> > +    u32 static_mem_addr_cells, static_mem_size_cells;
> > +    bool static_mem = false;
> 
> You don't need those information outside of allocate_static_memory(). So I
> think it would be best to move the code in that function.
> 

Sure.

> > +
> > +    /*
> > +     * Guest RAM could be static memory which will be specified through
> > +     * "xen,static-mem" property.
> > +     */
> > +    static_mem_prop = dt_find_property(node, "xen,static-mem", NULL);
> > +    if ( static_mem_prop )
> > +    {
> > +        static_mem = true;
> > +
> > +        if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
> > +                                   &static_mem_addr_cells) )
> > +        {
> > +            printk("Error building DomU: cannot read "
> > +                   "\"#xen,static-mem-address-cells\" property\n");
> We don't split comment over multi-line (even they are more than 80
> characters). This is to help grep message in the code.
> 
> Although for this one I would replaced "Error building Domu:" with simply
> with the domain ID (you can use %pd and 'd'). The caller will then print there
> was an error during building.
> 

Thanks for the explanation, learned.

> 
> > +            return -EINVAL;
> > +        }
> > +
> > +        if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
> > +                                   &static_mem_size_cells) )
> > +        {
> > +            printk("Error building DomU: cannot read "
> > +                   "\"#xen,static-mem-size-cells\" property\n");
> 
> My remark applies here as well.
> 
> > +            return -EINVAL;
> > +        }
> > +
> > +        BUG_ON(static_mem_size_cells > 2 || static_mem_addr_cells >
> > + 2);
> 
> Did we validate the DT before hand? If not, then I think
> 

Yeah... I think I did the check in first parse. It's redundant.

> > +    }
> 
> 
> >
> >       rc = dt_property_read_u64(node, "memory", &mem);
> >       if ( !rc )
> > @@ -2452,7 +2483,11 @@ static int __init construct_domU(struct domain *d,
> >       /* type must be set before allocate memory */
> >       d->arch.type = kinfo.type;
> >   #endif
> > -    allocate_memory(d, &kinfo);
> > +    if ( !static_mem )
> 
> With my suggestion above, the check can be replaced with:
> 
> if ( !dt_find_property(node, "xen,static-mem", NULL) )
> 

Sure, That’s more simple.

> > +        allocate_memory(d, &kinfo);
> > +    else
> > +        /* TODO: allocate_static_memory(...). */
> > +        BUG();
> >
> >       rc = prepare_dtb_domU(d, &kinfo);
> >       if ( rc < 0 )
> >
> 
> Cheers,
> 
> --

Cheers

--
Penny
> Julien Grall

RE: [PATCH V4 05/10] xen/arm: static memory initialization

[Wei Chen]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2021年8月13日 20:38
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 05/10] xen/arm: static memory initialization
> 
> Hi Penny,
> 
> On 28/07/2021 11:27, Penny Zheng wrote:
> > +/* Static memory initialization */
> > +static void __init init_staticmem_pages(void)
> > +{
> > +    unsigned int bank;
> > +
> > +    /* TODO: Considering NUMA-support scenario. */
> 
> I forgot to ask about this. What do you expect to be different with NUMA?
> 

From our current NUMA implementation, I think there is no difference
between NUMA and Non-NUMA system for static allocation. Maybe in the
future, we will add some checks to warning user about cross-nodes
configuration. But now, I think it's better for Penny to remove this
comment.

> > +    for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ )
> > +    {
> > +        unsigned long bank_start =
> PFN_UP(bootinfo.static_mem.bank[bank].start);
> > +        unsigned long bank_size =
> PFN_DOWN(bootinfo.static_mem.bank[bank].size);
> > +        unsigned long bank_end = bank_start + bank_size;
> > +
> > +        if ( bank_end <= bank_start )
> > +            return;
> > +
> > +        free_staticmem_pages(mfn_to_page(_mfn(bank_start)),
> > +                             bank_size, false);
> > +    }
> > +}
> > +
> 
> Cheers,
> 
> --
> Julien Grall

RE: [PATCH V4 10/10] xen/arm: introduce allocate_static_memory

[Penny Zheng]

Hi julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Friday, August 13, 2021 9:37 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 10/10] xen/arm: introduce allocate_static_memory
> 
> Hi Penny,
> 
> On 28/07/2021 11:27, Penny Zheng wrote:
> > This commit introduces allocate_static_memory to allocate static
> > memory as guest RAM for Domain on Static Allocation.
> >
> > It uses acquire_domstatic_pages to acquire pre-configured static
> > memory for this domain, and uses guest_physmap_add_page to set up P2M
> table.
> > These pre-defined static memory banks shall be firstly mapped to the
> > fixed guest RAM address `GUEST_RAM0_BASE`. And until it exhausts the
> > `GUEST_RAM0_SIZE`, it will seek to `GUEST_RAM1_BASE`, and so on.
> > `GUEST_RAM0` may take up several pre-defined physical RAM regions.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> >   xen/arch/arm/domain_build.c | 137
> +++++++++++++++++++++++++++++++++++-
> >   1 file changed, 135 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index cdb16f2086..ed290ee31b 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -480,6 +480,139 @@ fail:
> >             (unsigned long)kinfo->unassigned_mem >> 10);
> >   }
> >
> > +static bool __init append_static_memory_to_bank(struct domain *d,
> > +                                                struct membank *bank,
> > +                                                mfn_t smfn,
> > +                                                paddr_t size) {
> > +    int res;
> > +    paddr_t tot_size = size;
> > +    /* Infer next GFN. */
> > +    gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
> > +
> > +    while ( tot_size > 0 )
> > +    {
> > +        unsigned int order = get_allocation_size(tot_size);
> > +
> > +        res = guest_physmap_add_page(d, sgfn, smfn, order);
> > +        if ( res )
> > +        {
> > +            dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> > +            return false;
> > +        }
> > +
> > +        smfn = mfn_add(smfn, 1UL << order);
> > +        tot_size -= (1UL << (PAGE_SHIFT + order));
> > +    }
> 
> AFAICT, the loop is only here to suit guest_physmap_add_page(). Further
> down the line, the order will be converted back to a number of pages before
> calling p2m_insert_mapping().
> 
> So how about exporting p2m_insert_mapping() and use it?
> 

Sure. Looks perfect to me. 

> > + > +    bank->size = bank->size + size;
> 
> We usually add a newline before the last return of the function.
> 
> > +    return true;
> > +}
> > +
> > +/* Allocate memory from static memory as RAM for one specific domain
> > +d. */ static void __init allocate_static_memory(struct domain *d,
> > +                                          struct kernel_info *kinfo,
> > +                                          const struct dt_property *prop,
> > +                                          u32 addr_cells, u32
> > +size_cells) {
> > +    unsigned int nr_banks, gbank, bank = 0;
> > +    const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
> > +    const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
> > +    const __be32 *cell;
> > +    u32 reg_cells = addr_cells + size_cells;
> > +    u64 tot_size = 0;
> > +    paddr_t pbase, psize, gsize;
> > +    mfn_t smfn;
> > +
> > +    /* Start with GUEST_RAM0. */
> > +    kinfo->mem.nr_banks = 0;
> > +    gbank = 0;
> > +    gsize = ramsize[gbank];
> > +    kinfo->mem.bank[gbank].start = rambase[gbank];
> > +
> > +    cell = (const __be32 *)prop->value;
> > +    nr_banks = (prop->length) / (reg_cells * sizeof (u32));
> > +    BUG_ON(nr_banks > NR_MEM_BANKS);
> > +
> > +    while ( bank < nr_banks )
> > +    {
> > +        device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize);
> > +        tot_size += psize;
> > +        smfn = maddr_to_mfn(pbase);
> > +
> > +        if ( !acquire_domstatic_pages(d, psize >> PAGE_SHIFT, smfn,
> > + 0) )
> 
> I think we want to check that both pbase and psize are page aligned first. This
> can be done here or earlier when reserving the pages (this would be a previous
> patch).
> 

I will do the check in static memory initialization, in function init_staticmem_pages.

> Also, given that you can easily figure out the page from the mfn. I think it
> would be better for acquire_domstatic_pages() to return an error. This could
> be helpful to figure out an error.
> 

Sure. I'll return the errno.

> > +        {
> > +            printk(XENLOG_ERR
> > +                    "%pd: cannot acquire static memory "
> > +                    "(0x%"PRIpaddr" - 0x%"PRIpaddr").\n",
> > +                    d, pbase, pbase + psize);
> > +            goto fail;
> > +        }
> > +
> > +        printk(XENLOG_INFO "%pd: STATIC BANK[%d]
> > + %#"PRIpaddr"-%#"PRIpaddr"\n",
> 
> bank is unsigned so s/%d/%u/
>

Oh, thx.

> > +               d, bank, pbase, pbase + psize);
> > +
> > +        /*
> > +         * It shall be mapped to the fixed guest RAM address rambase[i],
> > +         * And until it exhausts the ramsize[i], it will seek to the next
> > +         * rambase[i+1].
> > +         */
> > +        while ( 1 )
> > +        {
> > +            /*
> > +             * The current physical bank is fully mapped.
> > +             * Handle the next physical bank.
> > +             */
> > +            if ( gsize >= psize )
> > +            {
> > +                if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> > +                                                   smfn, psize) )
> > +                    goto fail;
> > +
> > +                gsize = gsize - psize;
> > +                bank++;
> > +                break;
> > +            }
> > +            /*
> > +             * Current guest bank memory is not enough to map.
> > +             * Check if we have another guest bank available.
> > +             * gbank refers guest memory bank index.
> > +             */
> > +            else if ( (gbank + 2) > GUEST_RAM_BANKS ) {
> 
> I don't understand the +2. Can you clarify it?
> 

gbank refers to the index of the guest bank, and here since current guest bank(gbank)
 memory is not enough to map, users seeks to the next one(gbank + 1),

gbank + 2 is the number of requested guest memory banks right now, and shall not be
larger than GUEST_RAM_BANKS.
 
> Also, the coding style for Xen requires the { to be on a separate line.
> 

Sure.

> > +                printk("Exhausted the number of guest bank\n");
> > +                goto fail;
> > +            }
> > +            else
> > +            {
> > +                if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> > +                                                   smfn, gsize) )
> > +                    goto fail;
> 
> As I may have mentionned earlier, I find the double loop quite difficult to read.
> I don't think we can drop the double loop, but we can at least try to simplify
> the code in the loops.
> 
> The one I can think right now is moving allocate_static_memory_to_bank()
> outside of the if/else. Something like:
> 
> /* Map as much as possible the static range to the guest bank */ if
> ( !allocate_static_bank(.., min(psize, gize)) )
> 

Sure, will do.

> > +
> > +                psize = psize - gsize;
> > +                smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> > +                /* Update to the next guest bank. */
> > +                gbank++;
> > +                gsize = ramsize[gbank];
> > +                kinfo->mem.bank[gbank].start = rambase[gbank];
> > +            }
> > +        }
> > +    }
> > +
> > +    kinfo->mem.nr_banks = ++gbank;
> > +    kinfo->unassigned_mem -= tot_size;
> > +    if ( kinfo->unassigned_mem )
> > +        printk(XENLOG_ERR
> > +               "Size of \"memory\" property doesn't match up with the ones "
> > +               "defined in \"xen,static-mem\".\n");
> 
> We don't split the single line message accross multi-line even if the result code
> is more than 80 characters long.
> 

Sure.

> > +
> > +    return;
> > +
> > +fail:
> > +    panic("Failed to allocate requested static memory for domain %pd."
> > +          "Fix the VMs configurations.\n",
> 
> Same here.
> 
> > +          d);
> > +}
> > +
> >   static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
> >                                      const struct dt_device_node *node)
> >   {
> > @@ -2486,8 +2619,8 @@ static int __init construct_domU(struct domain *d,
> >       if ( !static_mem )
> >           allocate_memory(d, &kinfo);
> >       else
> > -        /* TODO: allocate_static_memory(...). */
> > -        BUG();
> > +        allocate_static_memory(d, &kinfo, static_mem_prop,
> > +                               static_mem_addr_cells,
> > + static_mem_size_cells);
> >
> >       rc = prepare_dtb_domU(d, &kinfo);
> >       if ( rc < 0 )
> >
> 
> Cheers,
> 
> --

Cheers

Penny
> Julien Grall

Re: [PATCH V4 01/10] xen/arm: introduce domain on Static Allocation

[Julien Grall]

On 16/08/2021 06:21, Penny Zheng wrote:
> Hi Julien

Hi Penny,

>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Wednesday, August 11, 2021 9:32 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>; nd <nd@arm.com>
>> Subject: Re: [PATCH V4 01/10] xen/arm: introduce domain on Static Allocation
>>
>> Hi Penny,
>>
>> On 28/07/2021 11:27, Penny Zheng wrote:
>>> Static Allocation refers to system or sub-system(domains) for which
>>> memory areas are pre-defined by configuration using physical address
>> ranges.
>>> Those pre-defined memory, -- Static Memory, as parts of RAM reserved
>>> in the beginning, shall never go to heap allocator or boot allocator for any
>> use.
>>>
>>> Domains on Static Allocation is supported through device tree property
>>> `xen,static-mem` specifying reserved RAM banks as this domain's guest RAM.
>>> By default, they shall be mapped to the fixed guest RAM address
>>> `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`.
>>>
>>> This patch introduces this new `xen,static-mem` feature, and also
>>> documents and parses this new attribute at boot time and stores
>>> related info in static_mem for later initialization.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>>    docs/misc/arm/device-tree/booting.txt | 40 +++++++++++++++++++++
>>>    xen/arch/arm/bootfdt.c                | 51 +++++++++++++++++++++++++++
>>>    xen/include/asm-arm/setup.h           |  2 ++
>>>    3 files changed, 93 insertions(+)
>>>
>>> diff --git a/docs/misc/arm/device-tree/booting.txt
>>> b/docs/misc/arm/device-tree/booting.txt
>>> index 5243bc7fd3..2a1ddca29b 100644
>>> --- a/docs/misc/arm/device-tree/booting.txt
>>> +++ b/docs/misc/arm/device-tree/booting.txt
>>> @@ -268,3 +268,43 @@ The DTB fragment is loaded at 0xc000000 in the
>> example above. It should
>>>    follow the convention explained in docs/misc/arm/passthrough.txt. The
>>>    DTB fragment will be added to the guest device tree, so that the guest
>>>    kernel will be able to discover the device.
>>> +
>>> +
>>> +Static Allocation
>>> +=============
>>> +
>>> +Static Allocation refers to system or sub-system(domains) for which
>>> +memory areas are pre-defined by configuration using physical address
>> ranges.
>>> +Those pre-defined memory, -- Static Memory, as parts of RAM reserved
>>> +in the beginning, shall never go to heap allocator or boot allocator for any
>> use.
>>
>> I don't understand "as parts of RAM reserved in the beginning". Could you
>> clarify it?
>>
> 
> I mean, static memory is very alike reserved memory, reserved during system boot time,
> not dynamically allocated at runtime.

Thanks for the clarification. The documentation is meant to be for the 
users, so I would suggest to drop the "-- Static memory, as parse of RAM 
reserved" because it doesn't add any value to know we treat the static 
memory and reserved memory the same way.

>>> +
>>> +The dtb property should look like as follows:
>>
>> Do you mean "node" rather than "property"?
>>
> 
> Oh, sure. Maybe "as an example" shall be more clarified.

I would write "Below an example on how to specific the static memory 
region in the device-tree".

> 
>>> +                compatible = "xen,domain";
>>> +                #address-cells = <0x2>;
>>> +                #size-cells = <0x2>;
>>> +                cpus = <2>;
>>> +                #xen,static-mem-address-cells = <0x1>;
>>> +                #xen,static-mem-size-cells = <0x1>;
>>> +                xen,static-mem = <0x30000000 0x20000000>;
>>> +                ...
>>> +            };
>>> +        };
>>> +    };
>>> +
>>> +DomU1 will have a static memory of 512MB reserved from the physical
>>> +address
>>> +0x30000000 to 0x50000000.
>>
>> I would write "This will reserve a 512MB region starting at the host physical
>> address 0x30000000 to be exclusively used by DomU1".
>>
> 
> Sure, thx.
>   
>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index
>>> 476e32e0f5..d2714446e1 100644
>>> --- a/xen/arch/arm/bootfdt.c
>>> +++ b/xen/arch/arm/bootfdt.c
>>> @@ -193,6 +193,55 @@ static int __init
>> process_reserved_memory_node(const void *fdt, int node,
>>>        return 0;
>>>    }
>>>
>>> +static int __init process_static_memory(const void *fdt, int node,
>>> +void *data) {
>>
>> This is pretty much a copy of process_memory_node(). So can we avoid the
>> duplication?
>>
>> I think I mentionned it in the past but I can't find the outcome.
>>
>>> +    int i = 0, banks;
>>> +    const __be32 *cell;
>>> +    paddr_t start, size;
>>> +    u32 address_cells, size_cells, reg_cells;
>>> +    struct meminfo *mem = data;
>>> +    const struct fdt_property *prop;
>>> +
>>> +
>>> +    address_cells = device_tree_get_u32(fdt, node,
>>> +                                        "#xen,static-mem-address-cells", 0);
>>> +    size_cells = device_tree_get_u32(fdt, node,
>>> +                                     "#xen,static-mem-size-cells", 0);
>>> +    if ( (address_cells == 0) || (size_cells == 0) )
>>> +    {
>>> +         printk("Missing \"#xen,static-mem-address-cell\" or "
>>> +                 "\"#xen,static-mem-address-cell\".\n");
>>> +         return -EINVAL;
>>> +    }
>>> +    reg_cells = address_cells + size_cells;
>>> +
>>> +    prop = fdt_get_property(fdt, node, "xen,static-mem", NULL);
>>> +    /*
>>> +     * Static memory shall belong to a specific domain, that is,
>>> +     * its node `domUx` has compatible string "xen,domain".
>>> +     */
>>
>> This code is just checking the node compatible is "xen,domain". So I would
>> drop the "domUx". This is also...
>>
>>> +    if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
>>> +    {
>>> +        printk("xen,static-mem property can only be located under
>>> + /domUx node.\n");
>>
>> ... not correct.
>>
> 
> I checked it here, to make sure the "xen,static-mem" property must be used in a domain node, since
> for now, static memory could be only configured as guest RAM.
> 
> Which part do you think it is not appropriate here?

You wrote "... can only be located under /domUx". That's not correct 
because we don't force (or even mention to) the user to name the node 
that way.


> 
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    cell = (const __be32 *)prop->data;
>>> +    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>>> +
>>> +    for ( ; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
>>> +    {
>>> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>>> +        mem->bank[mem->nr_banks].start = start;
>>> +        mem->bank[mem->nr_banks].size = size;
>>> +        mem->nr_banks++;
>>> +    }
>>> +
>>> +    if ( i < banks )
>>> +        return -ENOSPC;
>>> +    return 0;
>>> +}
>>> +
>>>    static int __init process_reserved_memory(const void *fdt, int node,
>>>                                              const char *name, int depth,
>>>                                              u32 address_cells, u32
>>> size_cells) @@ -346,6 +395,8 @@ static int __init early_scan_node(const
>> void *fdt,
>>>            process_multiboot_node(fdt, node, name, address_cells, size_cells);
>>>        else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
>>>            process_chosen_node(fdt, node, name, address_cells,
>>> size_cells);
>>> +    else if ( depth == 2 && fdt_get_property(fdt, node,
>>> + "xen,static-mem", NULL) )
>>
>> How about checking the compatible instead?
>>
> 
> hmm, since it is a property, not a node. so...
Right, but you could write:

device_tree_node_compatible(fdt, node, "xen,domain")

This would be more correct because we are interested in node using the 
Xen domain binding that contains the property "xen,static-mem".

All the other nodes with the property "xen,static-mem" should be left 
alone because it may have a different meaning.

Cheers,

-- 
Julien Grall

Re: [PATCH V4 03/10] xen/arm: handle static memory in dt_unreserved_regions

[Julien Grall]

On 16/08/2021 07:00, Penny Zheng wrote:
> Hi Julien

Hi Penny,

>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Wednesday, August 11, 2021 9:48 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>; nd <nd@arm.com>
>> Subject: Re: [PATCH V4 03/10] xen/arm: handle static memory in
>> dt_unreserved_regions
>>
>> Hi Penny,
>>
>> On 28/07/2021 11:27, Penny Zheng wrote:
>>> static memory regions overlap with memory nodes. The overlapping
>>> memory is reserved-memory and should be handled accordingly:
>>> dt_unreserved_regions should skip these regions the same way they are
>>> already skipping mem-reserved regions.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>>    xen/arch/arm/setup.c | 47 ++++++++++++++++++++++++++++----------------
>>>    1 file changed, 30 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index
>>> 63a908e325..f569134317 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -200,6 +200,13 @@ static void __init dt_unreserved_regions(paddr_t s,
>> paddr_t e,
>>>                                             int first)
>>>    {
>>>        int i, nr = fdt_num_mem_rsv(device_tree_flattened);
>>> +    /*
>>> +     * There are two types of reserved memory stored in bootinfo, one
>> defines
>>> +     * in /reserved-memory node, the other refers to domain on static
>> allocation
>>> +     * through "xen,static-mem" property.
>>> +     */
>>> +    int nr_rsv_type = 2, t = 0, prev_nr;
>>> +    struct meminfo *rsv_type[2] = {&bootinfo.reserved_mem,
>>> + &bootinfo.static_mem};
>>
>> Looking at the rest of the series, it doesn't look like there is a real benefits to
>> have the static memory and reserved memory in separate arrays as they are
>> walked only a few times and they are both meant to be small. In fact, I think
>> this code is lot more difficult to read.
>>
>> So it would be best to merge the two arrays in one. We can add a flag in the
>> structure to differentiate between "static" and "reserved" memory.
>>
> 
> How about adding a "static" flag in "struct meminfo" to tell. See the below example:
> "
> struct meminfo {
>      int nr_banks;
>      struct membank bank[NR_MEM_BANKS];
>      bool static;  /* whether memory is reserved as static memory. */
> };
> "
> 
> And I will delete "struct meminfo static_mem" array, all "static" and "reserved" memory
> will be stored in one "struct meminfo reserved_mem" array.

Did you intend to suggest to add the new member in struct membank rather 
than struct meminfo?

If not, then I don't understand how this would help have the static and 
reserved region in a single array and avoid the extra loop you added.

Cheers,

-- 
Julien Grall

Re: [PATCH V4 08/10] xen/arm: introduce acquire_staticmem_pages and acquire_domstatic_pages

[Julien Grall]

On 16/08/2021 07:43, Penny Zheng wrote:
> Hi Julien,

Hi,

>>> +{
>>> +    bool need_tlbflush = false;
>>> +    uint32_t tlbflush_timestamp = 0;
>>> +    unsigned long i;
>>> +    struct page_info *pg;
>>> +
>>> +    /* For now, it only supports pre-configured static memory. */
>>
>> This comment doesn't seem to match the check below.
>>
>>> +    if ( !mfn_valid(smfn) || !nr_mfns )
>>
>> This check only guarantees that there will be a page for the first MFN.
>> Shouldn't we also check for the other MFNs?
>>
> 
> Hmm, Do you think that it should be all checked, the whole range, [smfn, smfn + nr_mfns).
> Since it is in linear growth, maybe adding another check of "!mfn_valid(smfn + nr_mfns - 1)"
> is enough?

The only requirement for Xen is to provide a valid struct page for each 
RAM page. So checking the first and end page may not be sufficient if 
there is a hole in the middle.

My point here is either:
   - we trust the callers so we remove the mfn_valid() check
   - we don't trust the callers so we check the MFN is valid for every page

My preference is the latter, the more if we plan to us the helper after 
boot in the future. An possible compromise is to add a comment that this 
function needs to be reworked if used outside of boot.

Cheers,


-- 
Julien Grall

Re: [PATCH V4 10/10] xen/arm: introduce allocate_static_memory

[Julien Grall]

On 16/08/2021 08:51, Penny Zheng wrote:
>>> +               d, bank, pbase, pbase + psize);
>>> +
>>> +        /*
>>> +         * It shall be mapped to the fixed guest RAM address rambase[i],
>>> +         * And until it exhausts the ramsize[i], it will seek to the next
>>> +         * rambase[i+1].
>>> +         */
>>> +        while ( 1 )
>>> +        {
>>> +            /*
>>> +             * The current physical bank is fully mapped.
>>> +             * Handle the next physical bank.
>>> +             */
>>> +            if ( gsize >= psize )
>>> +            {
>>> +                if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
>>> +                                                   smfn, psize) )
>>> +                    goto fail;
>>> +
>>> +                gsize = gsize - psize;
>>> +                bank++;
>>> +                break;
>>> +            }
>>> +            /*
>>> +             * Current guest bank memory is not enough to map.
>>> +             * Check if we have another guest bank available.
>>> +             * gbank refers guest memory bank index.
>>> +             */
>>> +            else if ( (gbank + 2) > GUEST_RAM_BANKS ) {
>>
>> I don't understand the +2. Can you clarify it?
>>
> 
> gbank refers to the index of the guest bank, and here since current guest bank(gbank)
>   memory is not enough to map, users seeks to the next one(gbank + 1),
> 
> gbank + 2 is the number of requested guest memory banks right now, and shall not be
> larger than GUEST_RAM_BANKS.

Thanks for the clarification. When I read "2" I tend to think we are 
checking the bank after the next. How about writing:

(gbank + 1) >= GUEST_RAM_BANKS

or

gbank >= (GUEST_RAM_BANKS - 1)

This as the same end results, but we check the index rather than the 
number of banks.

Anyway, I can settle with 2 if you really prefer it.

Cheers,

-- 
Julien Grall

RE: [PATCH V4 01/10] xen/arm: introduce domain on Static Allocation

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Tuesday, August 17, 2021 1:28 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 01/10] xen/arm: introduce domain on Static Allocation
> 
> 
> 
> On 16/08/2021 06:21, Penny Zheng wrote:
> > Hi Julien
> 
> Hi Penny,
> 
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Wednesday, August 11, 2021 9:32 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>;
> >> xen-devel@lists.xenproject.org; sstabellini@kernel.org
> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> >> <Wei.Chen@arm.com>; nd <nd@arm.com>
> >> Subject: Re: [PATCH V4 01/10] xen/arm: introduce domain on Static
> >> Allocation
> >>
> >> Hi Penny,
> >>
> >> On 28/07/2021 11:27, Penny Zheng wrote:
> >>> Static Allocation refers to system or sub-system(domains) for which
> >>> memory areas are pre-defined by configuration using physical address
> >> ranges.
> >>> Those pre-defined memory, -- Static Memory, as parts of RAM reserved
> >>> in the beginning, shall never go to heap allocator or boot allocator
> >>> for any
> >> use.
> >>>
> >>> Domains on Static Allocation is supported through device tree
> >>> property `xen,static-mem` specifying reserved RAM banks as this domain's
> guest RAM.
> >>> By default, they shall be mapped to the fixed guest RAM address
> >>> `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`.
> >>>
> >>> This patch introduces this new `xen,static-mem` feature, and also
> >>> documents and parses this new attribute at boot time and stores
> >>> related info in static_mem for later initialization.
> >>>
> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> >>> ---
> >>>    docs/misc/arm/device-tree/booting.txt | 40 +++++++++++++++++++++
> >>>    xen/arch/arm/bootfdt.c                | 51 +++++++++++++++++++++++++++
> >>>    xen/include/asm-arm/setup.h           |  2 ++
> >>>    3 files changed, 93 insertions(+)
> >>>
> >>> diff --git a/docs/misc/arm/device-tree/booting.txt
> >>> b/docs/misc/arm/device-tree/booting.txt
> >>> index 5243bc7fd3..2a1ddca29b 100644
> >>> --- a/docs/misc/arm/device-tree/booting.txt
> >>> +++ b/docs/misc/arm/device-tree/booting.txt
> >>> @@ -268,3 +268,43 @@ The DTB fragment is loaded at 0xc000000 in the
> >> example above. It should
> >>>    follow the convention explained in docs/misc/arm/passthrough.txt. The
> >>>    DTB fragment will be added to the guest device tree, so that the guest
> >>>    kernel will be able to discover the device.
> >>> +
> >>> +
> >>> +Static Allocation
> >>> +=============
> >>> +
> >>> +Static Allocation refers to system or sub-system(domains) for which
> >>> +memory areas are pre-defined by configuration using physical
> >>> +address
> >> ranges.
> >>> +Those pre-defined memory, -- Static Memory, as parts of RAM
> >>> +reserved in the beginning, shall never go to heap allocator or boot
> >>> +allocator for any
> >> use.
> >>
> >> I don't understand "as parts of RAM reserved in the beginning". Could
> >> you clarify it?
> >>
> >
> > I mean, static memory is very alike reserved memory, reserved during
> > system boot time, not dynamically allocated at runtime.
> 
> Thanks for the clarification. The documentation is meant to be for the users, so
> I would suggest to drop the "-- Static memory, as parse of RAM reserved"
> because it doesn't add any value to know we treat the static memory and
> reserved memory the same way.
> 

Got it. Thx

> >>> +
> >>> +The dtb property should look like as follows:
> >>
> >> Do you mean "node" rather than "property"?
> >>
> >
> > Oh, sure. Maybe "as an example" shall be more clarified.
> 
> I would write "Below an example on how to specific the static memory region
> in the device-tree".
> 

Thanks for the example. Will do 

> >
> >>> +                compatible = "xen,domain";
> >>> +                #address-cells = <0x2>;
> >>> +                #size-cells = <0x2>;
> >>> +                cpus = <2>;
> >>> +                #xen,static-mem-address-cells = <0x1>;
> >>> +                #xen,static-mem-size-cells = <0x1>;
> >>> +                xen,static-mem = <0x30000000 0x20000000>;
> >>> +                ...
> >>> +            };
> >>> +        };
> >>> +    };
> >>> +
> >>> +DomU1 will have a static memory of 512MB reserved from the physical
> >>> +address
> >>> +0x30000000 to 0x50000000.
> >>
> >> I would write "This will reserve a 512MB region starting at the host
> >> physical address 0x30000000 to be exclusively used by DomU1".
> >>
> >
> > Sure, thx.
> >
> >>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index
> >>> 476e32e0f5..d2714446e1 100644
> >>> --- a/xen/arch/arm/bootfdt.c
> >>> +++ b/xen/arch/arm/bootfdt.c
> >>> @@ -193,6 +193,55 @@ static int __init
> >> process_reserved_memory_node(const void *fdt, int node,
> >>>        return 0;
> >>>    }
> >>>
> >>> +static int __init process_static_memory(const void *fdt, int node,
> >>> +void *data) {
> >>
> >> This is pretty much a copy of process_memory_node(). So can we avoid
> >> the duplication?
> >>
> >> I think I mentionned it in the past but I can't find the outcome.
> >>
> >>> +    int i = 0, banks;
> >>> +    const __be32 *cell;
> >>> +    paddr_t start, size;
> >>> +    u32 address_cells, size_cells, reg_cells;
> >>> +    struct meminfo *mem = data;
> >>> +    const struct fdt_property *prop;
> >>> +
> >>> +
> >>> +    address_cells = device_tree_get_u32(fdt, node,
> >>> +                                        "#xen,static-mem-address-cells", 0);
> >>> +    size_cells = device_tree_get_u32(fdt, node,
> >>> +                                     "#xen,static-mem-size-cells", 0);
> >>> +    if ( (address_cells == 0) || (size_cells == 0) )
> >>> +    {
> >>> +         printk("Missing \"#xen,static-mem-address-cell\" or "
> >>> +                 "\"#xen,static-mem-address-cell\".\n");
> >>> +         return -EINVAL;
> >>> +    }
> >>> +    reg_cells = address_cells + size_cells;
> >>> +
> >>> +    prop = fdt_get_property(fdt, node, "xen,static-mem", NULL);
> >>> +    /*
> >>> +     * Static memory shall belong to a specific domain, that is,
> >>> +     * its node `domUx` has compatible string "xen,domain".
> >>> +     */
> >>
> >> This code is just checking the node compatible is "xen,domain". So I
> >> would drop the "domUx". This is also...
> >>
> >>> +    if ( fdt_node_check_compatible(fdt, node, "xen,domain") != 0 )
> >>> +    {
> >>> +        printk("xen,static-mem property can only be located under
> >>> + /domUx node.\n");
> >>
> >> ... not correct.
> >>
> >
> > I checked it here, to make sure the "xen,static-mem" property must be
> > used in a domain node, since for now, static memory could be only
> configured as guest RAM.
> >
> > Which part do you think it is not appropriate here?
> 
> You wrote "... can only be located under /domUx". That's not correct because
> we don't force (or even mention to) the user to name the node that way.
> 
> 
> >
> >>> +        return -EINVAL;
> >>> +    }
> >>> +
> >>> +    cell = (const __be32 *)prop->data;
> >>> +    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> >>> +
> >>> +    for ( ; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
> >>> +    {
> >>> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> >>> +        mem->bank[mem->nr_banks].start = start;
> >>> +        mem->bank[mem->nr_banks].size = size;
> >>> +        mem->nr_banks++;
> >>> +    }
> >>> +
> >>> +    if ( i < banks )
> >>> +        return -ENOSPC;
> >>> +    return 0;
> >>> +}
> >>> +
> >>>    static int __init process_reserved_memory(const void *fdt, int node,
> >>>                                              const char *name, int depth,
> >>>                                              u32 address_cells, u32
> >>> size_cells) @@ -346,6 +395,8 @@ static int __init
> >>> early_scan_node(const
> >> void *fdt,
> >>>            process_multiboot_node(fdt, node, name, address_cells, size_cells);
> >>>        else if ( depth == 1 && device_tree_node_matches(fdt, node,
> "chosen") )
> >>>            process_chosen_node(fdt, node, name, address_cells,
> >>> size_cells);
> >>> +    else if ( depth == 2 && fdt_get_property(fdt, node,
> >>> + "xen,static-mem", NULL) )
> >>
> >> How about checking the compatible instead?
> >>
> >
> > hmm, since it is a property, not a node. so...
> Right, but you could write:
> 
> device_tree_node_compatible(fdt, node, "xen,domain")
> 
> This would be more correct because we are interested in node using the Xen
> domain binding that contains the property "xen,static-mem".
> 
> All the other nodes with the property "xen,static-mem" should be left alone
> because it may have a different meaning.
> 

True, true. Will do

> Cheers,
> 
> --
> Julien Grall

RE: [PATCH V4 08/10] xen/arm: introduce acquire_staticmem_pages and acquire_domstatic_pages

[Penny Zheng]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Tuesday, August 17, 2021 1:44 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 08/10] xen/arm: introduce acquire_staticmem_pages
> and acquire_domstatic_pages
> 
> 
> 
> On 16/08/2021 07:43, Penny Zheng wrote:
> > Hi Julien,
> 
> Hi,
> 
> >>> +{
> >>> +    bool need_tlbflush = false;
> >>> +    uint32_t tlbflush_timestamp = 0;
> >>> +    unsigned long i;
> >>> +    struct page_info *pg;
> >>> +
> >>> +    /* For now, it only supports pre-configured static memory. */
> >>
> >> This comment doesn't seem to match the check below.
> >>
> >>> +    if ( !mfn_valid(smfn) || !nr_mfns )
> >>
> >> This check only guarantees that there will be a page for the first MFN.
> >> Shouldn't we also check for the other MFNs?
> >>
> >
> > Hmm, Do you think that it should be all checked, the whole range, [smfn,
> smfn + nr_mfns).
> > Since it is in linear growth, maybe adding another check of "!mfn_valid(smfn
> + nr_mfns - 1)"
> > is enough?
> 
> The only requirement for Xen is to provide a valid struct page for each RAM
> page. So checking the first and end page may not be sufficient if there is a hole
> in the middle.
> 
> My point here is either:
>    - we trust the callers so we remove the mfn_valid() check
>    - we don't trust the callers so we check the MFN is valid for every page
> 
> My preference is the latter, the more if we plan to us the helper after boot in
> the future. An possible compromise is to add a comment that this function
> needs to be reworked if used outside of boot.
> 

Ok. I'll do the whole range check and add the comments.

> Cheers,
> 
> 
> --
> Julien Grall

RE: [PATCH V4 10/10] xen/arm: introduce allocate_static_memory

[Penny Zheng]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Tuesday, August 17, 2021 1:55 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 10/10] xen/arm: introduce allocate_static_memory
> 
> 
> 
> On 16/08/2021 08:51, Penny Zheng wrote:
> >>> +               d, bank, pbase, pbase + psize);
> >>> +
> >>> +        /*
> >>> +         * It shall be mapped to the fixed guest RAM address rambase[i],
> >>> +         * And until it exhausts the ramsize[i], it will seek to the next
> >>> +         * rambase[i+1].
> >>> +         */
> >>> +        while ( 1 )
> >>> +        {
> >>> +            /*
> >>> +             * The current physical bank is fully mapped.
> >>> +             * Handle the next physical bank.
> >>> +             */
> >>> +            if ( gsize >= psize )
> >>> +            {
> >>> +                if ( !append_static_memory_to_bank(d, &kinfo-
> >mem.bank[gbank],
> >>> +                                                   smfn, psize) )
> >>> +                    goto fail;
> >>> +
> >>> +                gsize = gsize - psize;
> >>> +                bank++;
> >>> +                break;
> >>> +            }
> >>> +            /*
> >>> +             * Current guest bank memory is not enough to map.
> >>> +             * Check if we have another guest bank available.
> >>> +             * gbank refers guest memory bank index.
> >>> +             */
> >>> +            else if ( (gbank + 2) > GUEST_RAM_BANKS ) {
> >>
> >> I don't understand the +2. Can you clarify it?
> >>
> >
> > gbank refers to the index of the guest bank, and here since current guest
> bank(gbank)
> >   memory is not enough to map, users seeks to the next one(gbank + 1),
> >
> > gbank + 2 is the number of requested guest memory banks right now, and
> > shall not be larger than GUEST_RAM_BANKS.
> 
> Thanks for the clarification. When I read "2" I tend to think we are checking
> the bank after the next. How about writing:
> 
> (gbank + 1) >= GUEST_RAM_BANKS
> 
> or
> 
> gbank >= (GUEST_RAM_BANKS - 1)
> 
> This as the same end results, but we check the index rather than the number of
> banks.
> 
> Anyway, I can settle with 2 if you really prefer it.
> 

Nah, I'll take your suggestion. Yours is more generic.
 
> Cheers,
> 
> --
> Julien Grall

RE: [PATCH V4 07/10] xen: re-define assign_pages and introduce assign_page

[Penny Zheng]

Hi Julian and Jan

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Friday, August 13, 2021 8:27 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH V4 07/10] xen: re-define assign_pages and introduce
> assign_page
> 
> Hi Penny,
> 
> On 28/07/2021 11:27, Penny Zheng wrote:
> > In order to deal with the trouble of count-to-order conversion when
> > page number is not in a power-of-two, this commit re-define
> > assign_pages for nr pages and assign_page for original page with a single
> order.
> >
> > Backporting confusion could be helped by altering the order of
> > assign_page parameters, such that the compiler would point out that
> > adjustments at call sites are needed.
> 
> Looking at the code, you don't alter the order of assign_page() parameters. So
> did you mean to refer to "assign_pages()"?
> 
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > v4 change:
> > - in all cases where order-0 pages get passed, prefer using
> > assign_pages to pass literal 1
> > - reconstruct the order of assign_pages parameters
> > - remove the unnecessary parentheses
> > ---
> >   xen/arch/x86/pv/dom0_build.c |  2 +-
> >   xen/common/grant_table.c     |  2 +-
> >   xen/common/memory.c          |  4 ++--
> >   xen/common/page_alloc.c      | 23 ++++++++++++++---------
> >   xen/include/xen/mm.h         |  6 ++++++
> >   5 files changed, 24 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/x86/pv/dom0_build.c
> > b/xen/arch/x86/pv/dom0_build.c index af47615b22..9142f359da 100644
> > --- a/xen/arch/x86/pv/dom0_build.c
> > +++ b/xen/arch/x86/pv/dom0_build.c
> > @@ -556,7 +556,7 @@ int __init dom0_construct_pv(struct domain *d,
> >           else
> >           {
> >               while ( count-- )
> > -                if ( assign_pages(d, mfn_to_page(_mfn(mfn++)), 0, 0) )
> > +                if ( assign_pages(mfn_to_page(_mfn(mfn++)), 1, d, 0)
> > + )
> >                       BUG();
> >           }
> >           initrd->mod_end = 0;
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index
> > fab77ab9cc..1f6b89bff4 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -2342,7 +2342,7 @@ gnttab_transfer(
> >            * is respected and speculative execution is blocked accordingly
> >            */
> >           if ( unlikely(!evaluate_nospec(okay)) ||
> > -            unlikely(assign_pages(e, page, 0, MEMF_no_refcount)) )
> > +            unlikely(assign_pages(page, 1, e, MEMF_no_refcount)) )
> >           {
> >               bool drop_dom_ref;
> >
> > diff --git a/xen/common/memory.c b/xen/common/memory.c index
> > e07bd9a5ea..083e14b84f 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -728,7 +728,7 @@ static long
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t)
> arg)
> >           /* Assign each output page to the domain. */
> >           for ( j = 0; (page = page_list_remove_head(&out_chunk_list)); ++j )
> >           {
> > -            if ( assign_pages(d, page, exch.out.extent_order,
> > +            if ( assign_page(d, page, exch.out.extent_order,
> >                                 MEMF_no_refcount) )
> >               {
> >                   unsigned long dec_count; @@ -797,7 +797,7 @@ static
> > long
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t)
> arg)
> >        * cleared PGC_allocated.
> >        */
> >       while ( (page = page_list_remove_head(&in_chunk_list)) )
> > -        if ( assign_pages(d, page, 0, MEMF_no_refcount) )
> > +        if ( assign_pages(page, 1, d, MEMF_no_refcount) )
> >           {
> >               BUG_ON(!d->is_dying);
> >               free_domheap_page(page); diff --git
> > a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > f51e406401..e279c6f713 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2282,9 +2282,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
> >
> >
> >   int assign_pages(
> > -    struct domain *d,
> >       struct page_info *pg,
> > -    unsigned int order,
> > +    unsigned long nr,
> > +    struct domain *d,
> >       unsigned int memflags)
> >   {
> >       int rc = 0;
> > @@ -2304,7 +2304,7 @@ int assign_pages(
> >       {
> >           unsigned int extra_pages = 0;
> >
> > -        for ( i = 0; i < (1ul << order); i++ )
> > +        for ( i = 0; i < nr; i++ )
> >           {
> >               ASSERT(!(pg[i].count_info & ~PGC_extra));
> >               if ( pg[i].count_info & PGC_extra ) @@ -2313,18 +2313,18
> > @@ int assign_pages(
> >
> >           ASSERT(!extra_pages ||
> >                  ((memflags & MEMF_no_refcount) &&
> > -                extra_pages == 1u << order));
> > +                extra_pages == nr));
> >       }
> >   #endif
> >
> >       if ( pg[0].count_info & PGC_extra )
> >       {
> > -        d->extra_pages += 1u << order;
> > +        d->extra_pages += nr;
> >           memflags &= ~MEMF_no_refcount;
> >       }
> >       else if ( !(memflags & MEMF_no_refcount) )
> >       {
> > -        unsigned int tot_pages = domain_tot_pages(d) + (1 << order);
> > +        unsigned int tot_pages = domain_tot_pages(d) + nr;
> >
> >           if ( unlikely(tot_pages > d->max_pages) )
> >           {
> > @@ -2336,10 +2336,10 @@ int assign_pages(
> >       }
> >
> >       if ( !(memflags & MEMF_no_refcount) &&
> > -         unlikely(domain_adjust_tot_pages(d, 1 << order) == (1 << order)) )
> > +         unlikely(domain_adjust_tot_pages(d, nr) == nr) )
> >           get_knownalive_domain(d);
> >
> > -    for ( i = 0; i < (1 << order); i++ )
> > +    for ( i = 0; i < nr; i++ )
> >       {
> >           ASSERT(page_get_owner(&pg[i]) == NULL);
> >           page_set_owner(&pg[i], d);
> > @@ -2354,6 +2354,11 @@ int assign_pages(
> >       return rc;
> >   }
> >
> > +int assign_page(struct domain *d, struct page_info *pg, unsigned int order,
> > +                unsigned int memflags) {
> > +    return assign_pages(pg, 1UL << order, d, memflags); }
> >
> >   struct page_info *alloc_domheap_pages(
> >       struct domain *d, unsigned int order, unsigned int memflags) @@
> > -2396,7 +2401,7 @@ struct page_info *alloc_domheap_pages(
> >                   pg[i].count_info = PGC_extra;
> >               }
> >           }
> > -        if ( assign_pages(d, pg, order, memflags) )
> > +        if ( assign_page(d, pg, order, memflags) )
> >           {
> >               free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> >               return NULL;
> > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index
> > 8e8fb5a615..2e75cdcbb7 100644
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -132,6 +132,12 @@ int query_page_offline(mfn_t mfn, uint32_t *status);
> >   void heap_init_late(void);
> >
> >   int assign_pages(
> > +    struct page_info *pg,
> > +    unsigned long nr,
> > +    struct domain *d,
> > +    unsigned int memflags);
> > +
> > +int assign_page(
> >       struct domain *d,
> >       struct page_info *pg,
> >       unsigned int order,
> 
> I find a bit odd that the parameters are ordered differently between
> assign_pages() and assign_page(). They are similar interface after all.
> 

I will change the order back and make them in the similar order.

> I don't think it would be a problem for backporting purpose if
> assign_page() has a different order for the arguments.
> 
> Jan, what do you think?
> 
> Cheers,
> 
> --
> Julien Grall