[PATCH V2 0/9] Domain on Static Allocation

[PATCH V2 0/9] 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.

This Patch Serie only talks about Domain on Static Allocation.

ABOUT IMPLEMENTATION ON DEVICE TREE FORMAT OF STAIIC ALLOCATION IS
STILL UNDER DISCUSSION, AND RELATED CHANGE WILL BE INCLUDED IN PATCH V3.

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 v2:
 - consider reserved-memory binding and use phandle to refer(still in
discussion)
 - remove unused reserved field in struct page_info, unused helper
page_get_reserved_owner and page_set_reserved_owner
 - introduce CONFIG_STATIC_ALLOCATION to avoid bringing dead codes in
other archs.
 - extract common code from free_heap_pages and free_staticmem_pages
 - introduce new interface assign_pages_nr
 - change pfn-named to mfn-named
 - remove meaningless MEMF_no_owner case
 - leave zone concept out of DMA limitation check
 - take care of concurrency issue on static memory allocation
 - ensure the consistency when `xen,static-mem` and `memory` are both defined
 - fix the scalability issue in allocate_static_memory
 - allocate static memory when parse

Penny Zheng (9):
  xen/arm: introduce domain on Static Allocation
  xen/arm: introduce PGC_reserved
  xen/arm: introduce CONFIG_STATIC_ALLOCATION
  xen/arm: static memory initialization
  xen: introduce assign_pages_nr
  xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages
  xen/arm: take care of concurrency on static memory allocation
  xen/arm: check `xen,static-mem` property during domain construction
  xen/arm: introduce allocate_static_memory

 docs/misc/arm/device-tree/booting.txt |  49 ++++++
 xen/arch/arm/Kconfig                  |   3 +
 xen/arch/arm/bootfdt.c                |  12 +-
 xen/arch/arm/domain_build.c           | 188 +++++++++++++++++++-
 xen/arch/arm/setup.c                  |  27 +++
 xen/common/page_alloc.c               | 238 +++++++++++++++++++++++---
 xen/include/asm-arm/mm.h              |   3 +
 xen/include/asm-arm/setup.h           |   2 +
 xen/include/xen/mm.h                  |  18 +-
 9 files changed, 507 insertions(+), 33 deletions(-)

-- 
2.25.1

[PATCH 1/9] 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 static memory nodes,
defined in reserved-memory node with `xen,static-memory-domain` compatible,
which are specifying physical RAM as this domain's guest RAM.
{address, size}-cells will be consistent with the ones defined in the
parent node, reserved-memory.

This patch introduces this new static memory device tree node, 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>
---
v2 changes:
- fix typos
- consider reserved-memory binding and use phandle to refer
---
 docs/misc/arm/device-tree/booting.txt | 49 +++++++++++++++++++++++++++
 xen/arch/arm/bootfdt.c                | 12 +++++--
 xen/include/asm-arm/setup.h           |  2 ++
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 5243bc7fd3..ba7854b2d3 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -268,3 +268,52 @@ 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 nodes,
+defined in reserved-memory node with `xen,static-memory-domain` compatible,
+which are specifying physical RAM as this domain's guest RAM.
+#{address, size}-cells will be consistent with the ones defined in the
+parent node, reserved-memory.
+
+On memory allocation, these pre-defiend static memory ranges 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`.
+`GUEST_RAM0` may take up several pre-defined physical RAM regions.
+
+The dtb property should look like as follows:
+
+    / {
+        reserved-memory {
+            #address-cells = <2>;
+            #size-cells = <2>;
+
+            staticmemdomU1: static-memory@0x30000000 {
+                compatible = "xen,static-memory-domain";
+                reg = <0x0 0x30000000 0x0 0x20000000>;
+            };
+        };
+
+        chosen {
+            domU1 {
+                compatible = "xen,domain";
+                #address-cells = <0x2>;
+                #size-cells = <0x2>;
+                cpus = <2>;
+                xen,static-mem = <&staticmemdomU1>;
+
+                ...
+            };
+        };
+    };
+
+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 dcff512648..5b3bb75b5f 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -187,9 +187,17 @@ static int __init process_reserved_memory_node(const void *fdt, int node,
 
     if ( rc == -ENOSPC )
         panic("Max number of supported reserved-memory regions reached.");
-    else if ( rc != -ENOENT )
+    else if ( rc == -ENOENT )
+        return 0;
+    else if ( rc )
         return rc;
-    return 0;
+
+    /* Static memory node with compatible string "xen,static-memory-domain". */
+    if ( fdt_node_check_compatible(fdt, node, "xen,static-memory-domain") == 0 )
+        rc = process_memory_node(fdt, node, name, depth, address_cells,
+                                 size_cells, &bootinfo.static_mem);
+
+    return rc;
 }
 
 static int __init process_reserved_memory(const void *fdt, int node,
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 5283244015..5e9f296760 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 2/9] xen/arm: introduce PGC_reserved

[Penny Zheng]

In order to differentiate pages of static memory, from those allocated from
heap, this patch introduces a new page flag PGC_reserved to tell.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
changes v2:
- remove unused reserved field in struct page_info
- remove unused helper page_get_reserved_owner and page_set_reserved_owner
---
 xen/include/asm-arm/mm.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 0b7de3102e..7034fae1b6 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

Re: [PATCH 2/9] xen/arm: introduce PGC_reserved

[Julien Grall]

Hi Penny,

On 07/06/2021 03:43, Penny Zheng wrote:
> In order to differentiate pages of static memory, from those allocated from
> heap, this patch introduces a new page flag PGC_reserved to tell.

I would prefer if this patch is folded in the patch first using it. This 
will be easier to understand how this flag will be used.

Cheers,

> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> changes v2:
> - remove unused reserved field in struct page_info
> - remove unused helper page_get_reserved_owner and page_set_reserved_owner
> ---
>   xen/include/asm-arm/mm.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 0b7de3102e..7034fae1b6 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)
> 

-- 
Julien Grall

RE: [PATCH 2/9] xen/arm: introduce PGC_reserved

[Penny Zheng]

Hi Julien

Sorry for so long to respond, just back from a long holiday. 😉

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, July 1, 2021 1:44 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 2/9] xen/arm: introduce PGC_reserved
> 
> Hi Penny,
> 
> On 07/06/2021 03:43, Penny Zheng wrote:
> > In order to differentiate pages of static memory, from those allocated
> > from heap, this patch introduces a new page flag PGC_reserved to tell.
> 
> I would prefer if this patch is folded in the patch first using it. This will be
> easier to understand how this flag will be used.
> 
> Cheers,

Sure, I'll re-organize this commit.

> 
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > changes v2:
> > - remove unused reserved field in struct page_info
> > - remove unused helper page_get_reserved_owner and
> > page_set_reserved_owner
> > ---
> >   xen/include/asm-arm/mm.h | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index
> > 0b7de3102e..7034fae1b6 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)
> >
> 
> --
> Julien Grall


Cheers

--
Penny Zheng

[PATCH 3/9] xen/arm: introduce CONFIG_STATIC_ALLOCATION

[Penny Zheng]

For now, since the feature of Domain on Static Allocation is only supported
on ARM Architecture, this commit introduces new CONFIG_STATIC_ALLOCATION to
avoid bringing dead codes in other archs.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
changes v2:
- new commit
---
 xen/arch/arm/Kconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index ecfa6822e4..f165db8ecd 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -278,6 +278,9 @@ config ARM64_ERRATUM_1286807
 
 	  If unsure, say Y.
 
+config STATIC_ALLOCATION
+    def_bool y
+
 endmenu
 
 config ARM64_HARDEN_BRANCH_PREDICTOR
-- 
2.25.1

Re: [PATCH 3/9] xen/arm: introduce CONFIG_STATIC_ALLOCATION

[Jan Beulich]

On 07.06.2021 04:43, Penny Zheng wrote:
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -278,6 +278,9 @@ config ARM64_ERRATUM_1286807
>  
>  	  If unsure, say Y.
>  
> +config STATIC_ALLOCATION
> +    def_bool y
> +
>  endmenu
>  
>  config ARM64_HARDEN_BRANCH_PREDICTOR

Nit: While this and the following option exhibit bad indentation,
everything up from here looks to be fine, and that's what you want
to take as reference when adding a new option.

Jan

Re: [PATCH 3/9] xen/arm: introduce CONFIG_STATIC_ALLOCATION

[Julien Grall]

Hi Penny,

On 07/06/2021 03:43, Penny Zheng wrote:
> For now, since the feature of Domain on Static Allocation is only supported
> on ARM Architecture, this commit introduces new CONFIG_STATIC_ALLOCATION to
> avoid bringing dead codes in other archs.

Similarly to patch #2, I think it would be better to introduce this 
Kconfig when it is used or after the common code is introduced. This 
would prevent dead Kconfig.

Cheers,

> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> changes v2:
> - new commit
> ---
>   xen/arch/arm/Kconfig | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ecfa6822e4..f165db8ecd 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -278,6 +278,9 @@ config ARM64_ERRATUM_1286807
>   
>   	  If unsure, say Y.
>   
> +config STATIC_ALLOCATION
> +    def_bool y
> +
>   endmenu
>   
>   config ARM64_HARDEN_BRANCH_PREDICTOR
> 

-- 
Julien Grall

RE: [PATCH 3/9] xen/arm: introduce CONFIG_STATIC_ALLOCATION

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, July 1, 2021 1:45 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 3/9] xen/arm: introduce CONFIG_STATIC_ALLOCATION
> 
> Hi Penny,
> 
> On 07/06/2021 03:43, Penny Zheng wrote:
> > For now, since the feature of Domain on Static Allocation is only
> > supported on ARM Architecture, this commit introduces new
> > CONFIG_STATIC_ALLOCATION to avoid bringing dead codes in other archs.
> 
> Similarly to patch #2, I think it would be better to introduce this Kconfig when
> it is used or after the common code is introduced. This would prevent dead
> Kconfig.
> 

sure, I'll remove this commit, and combine it with the codes where this config is firstly used.

Cheers

> Cheers,
> 
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > changes v2:
> > - new commit
> > ---
> >   xen/arch/arm/Kconfig | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index
> > ecfa6822e4..f165db8ecd 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -278,6 +278,9 @@ config ARM64_ERRATUM_1286807
> >
> >   	  If unsure, say Y.
> >
> > +config STATIC_ALLOCATION
> > +    def_bool y
> > +
> >   endmenu
> >
> >   config ARM64_HARDEN_BRANCH_PREDICTOR
> >
> 
> --
> Julien Grall

--
Penny Zheng

[PATCH 4/9] xen/arm: static memory initialization

[Penny Zheng]

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

New func 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 defines a new helper free_page to extract common code between
free_heap_pages and free_staticmem_pages, like following the same cache/TLB
coherency policy.

For each page, free_staticmem_pages includes the following extra steps to
initialize:
1. change page state from inuse to free state and grant PGC_reserved.
2. scrub the page in need synchronously.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
changes v2:
- rename to nr_mfns
- extract common code from free_heap_pages and free_staticmem_pages
- remove dead codes in other archs, including move some to arm-specific file,
and put some under CONFIG_ARM
- mark free_staticmem_pages __init
---
 xen/arch/arm/setup.c    | 27 ++++++++++++++
 xen/common/page_alloc.c | 78 +++++++++++++++++++++++++++++++++--------
 xen/include/xen/mm.h    |  6 ++++
 3 files changed, 97 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 00aad1c194..daafea0abb 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -611,6 +611,30 @@ static void __init init_pdx(void)
     }
 }
 
+/* Static memory initialization */
+static void __init init_staticmem_pages(void)
+{
+    int bank;
+
+    /*
+     * TODO: Considering NUMA-support scenario.
+     */
+    for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ )
+    {
+        paddr_t bank_start = bootinfo.static_mem.bank[bank].start;
+        paddr_t bank_size = bootinfo.static_mem.bank[bank].size;
+        paddr_t bank_end = bank_start + bank_size;
+
+        bank_start = round_pgup(bank_start);
+        bank_end = round_pgdown(bank_end);
+        if ( bank_end <= bank_start )
+            return;
+
+        free_staticmem_pages(maddr_to_page(bank_start),
+                            (bank_end - bank_start) >> PAGE_SHIFT, false);
+    }
+}
+
 #ifdef CONFIG_ARM_32
 static void __init setup_mm(void)
 {
@@ -872,6 +896,9 @@ void __init start_xen(unsigned long boot_phys_offset,
     cmdline_parse(cmdline);
 
     setup_mm();
+    /* If exists, Static Memory Initialization. */
+    if ( bootinfo.static_mem.nr_banks > 0 )
+        init_staticmem_pages();
 
     /* Parse the ACPI tables for possible boot-time configuration */
     acpi_boot_table_init();
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 958ba0cd92..8c00262c04 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1376,6 +1376,37 @@ bool scrub_free_pages(void)
     return node_to_scrub(false) != NUMA_NO_NODE;
 }
 
+static void free_page(struct page_info *pg, bool need_scrub)
+{
+    mfn_t mfn = page_to_mfn(pg);
+
+    /* 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);
+
+#ifdef CONFIG_ARM
+    if ( pg->count_info & PGC_reserved )
+    {
+        /* TODO: asynchronous scrubbing. */
+        if ( need_scrub )
+            scrub_one_page(pg);
+        return;
+    }
+#endif
+    if ( need_scrub )
+    {
+        pg->count_info |= PGC_need_scrub;
+        poison_one_page(pg);
+    }
+
+    return;
+}
+
 /* Free 2^@order set of pages. */
 static void free_heap_pages(
     struct page_info *pg, unsigned int order, bool need_scrub)
@@ -1425,20 +1456,7 @@ static void free_heap_pages(
             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);
-
-        if ( need_scrub )
-        {
-            pg[i].count_info |= PGC_need_scrub;
-            poison_one_page(&pg[i]);
-        }
+        free_page(&pg[i], need_scrub);
     }
 
     avail[node][zone] += 1 << order;
@@ -1512,6 +1530,38 @@ static void free_heap_pages(
     spin_unlock(&heap_lock);
 }
 
+#ifdef CONFIG_STATIC_ALLOCATION
+/* 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++ )
+    {
+        switch ( pg[i].count_info & PGC_state )
+        {
+        case PGC_state_inuse:
+            BUG_ON(pg[i].count_info & PGC_broken);
+            /* Mark it free and reserved. */
+            pg[i].count_info = PGC_state_free | PGC_reserved;
+            break;
+
+        default:
+            printk(XENLOG_ERR
+                   "Page state shall be only in PGC_state_inuse. "
+                   "pg[%lu] MFN %"PRI_mfn" count_info=%#lx tlbflush_timestamp=%#x.\n",
+                   i, mfn_x(mfn) + i,
+                   pg[i].count_info,
+                   pg[i].tlbflush_timestamp);
+            BUG();
+        }
+
+        free_page(&pg[i], need_scrub);
+    }
+}
+#endif
 
 /*
  * Following rules applied for page offline:
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 667f9dac83..df25e55966 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_ARM
+/* Static Allocation */
+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

Re: [PATCH 4/9] xen/arm: static memory initialization

[Jan Beulich]

On 07.06.2021 04:43, Penny Zheng wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -611,6 +611,30 @@ static void __init init_pdx(void)
>      }
>  }
>  
> +/* Static memory initialization */
> +static void __init init_staticmem_pages(void)
> +{
> +    int bank;

While I'm not a maintainer of this code, I'd still like to point out
that wherever possible we prefer "unsigned int" when dealing with
only non-negative values, and even more so when using them as array
indexes.

> +    /*
> +     * TODO: Considering NUMA-support scenario.
> +     */

Nit: Comment style.

> @@ -872,6 +896,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>      cmdline_parse(cmdline);
>  
>      setup_mm();
> +    /* If exists, Static Memory Initialization. */
> +    if ( bootinfo.static_mem.nr_banks > 0 )
> +        init_staticmem_pages();

I don't think the conditional is really needed here?

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void)
>      return node_to_scrub(false) != NUMA_NO_NODE;
>  }
>  
> +static void free_page(struct page_info *pg, bool need_scrub)
> +{
> +    mfn_t mfn = page_to_mfn(pg);

With pdx compression this is a non-trivial conversion. The function
being an internal helper and the caller already holding the MFN, I
think it would be preferable if the MFN was passed in here. If done
this way, you may want to consider adding an ASSERT() to double
check both passed in arguments match up.

> +    /* 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);
> +
> +#ifdef CONFIG_ARM

If avoidable there should be no arch-specific code added to this
file. Assuming another arch gained PGC_reserved, what's wrong with
enabling this code right away for them as well? I.e. use
PGC_reserved here instead of CONFIG_ARM? Alternatively this may
want to be CONFIG_STATIC_ALLOCATION, assuming we consider
PGC_reserved tied to it.

> +    if ( pg->count_info & PGC_reserved )
> +    {
> +        /* TODO: asynchronous scrubbing. */
> +        if ( need_scrub )
> +            scrub_one_page(pg);
> +        return;
> +    }
> +#endif
> +    if ( need_scrub )

Nit: Please have a blank line between these last two.

> +    {
> +        pg->count_info |= PGC_need_scrub;
> +        poison_one_page(pg);
> +    }
> +
> +    return;

Please omit return statements at the end of functions returning void.

> +}

On the whole, bike shedding or not, I'm afraid the function's name
doesn't match what it does: There's no freeing of a page here. What
gets done is marking of a page as free. Hence maybe mark_page_free()
or mark_free_page() or some such?

> @@ -1512,6 +1530,38 @@ static void free_heap_pages(
>      spin_unlock(&heap_lock);
>  }
>  
> +#ifdef CONFIG_STATIC_ALLOCATION
> +/* 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++ )
> +    {
> +        switch ( pg[i].count_info & PGC_state )
> +        {
> +        case PGC_state_inuse:
> +            BUG_ON(pg[i].count_info & PGC_broken);
> +            /* Mark it free and reserved. */
> +            pg[i].count_info = PGC_state_free | PGC_reserved;
> +            break;
> +
> +        default:
> +            printk(XENLOG_ERR
> +                   "Page state shall be only in PGC_state_inuse. "

Why? A page (static or not) can become broken while in use. IOW I
don't think you can avoid handling PGC_state_offlining here. At which
point this code will match free_heap_pages()'es, and hence likely
will want folding as well.

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

ITYM CONFIG_STATIC_ALLOCATION here?

Jan

Re: [PATCH 4/9] xen/arm: static memory initialization

[Julien Grall]

Hi,

On 10/06/2021 10:35, Jan Beulich wrote:
> On 07.06.2021 04:43, Penny Zheng wrote:
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -611,6 +611,30 @@ static void __init init_pdx(void)
>>       }
>>   }
>>   
>> +/* Static memory initialization */
>> +static void __init init_staticmem_pages(void)
>> +{
>> +    int bank;
> 
> While I'm not a maintainer of this code, I'd still like to point out
> that wherever possible we prefer "unsigned int" when dealing with
> only non-negative values, and even more so when using them as array
> indexes.

+1.

> 
>> +    /*
>> +     * TODO: Considering NUMA-support scenario.
>> +     */
> 
> Nit: Comment style.
> 
>> @@ -872,6 +896,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>>       cmdline_parse(cmdline);
>>   
>>       setup_mm();
>> +    /* If exists, Static Memory Initialization. */
>> +    if ( bootinfo.static_mem.nr_banks > 0 )
>> +        init_staticmem_pages();
> 
> I don't think the conditional is really needed here?
> 
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void)
>>       return node_to_scrub(false) != NUMA_NO_NODE;
>>   }
>>   
>> +static void free_page(struct page_info *pg, bool need_scrub)
>> +{
>> +    mfn_t mfn = page_to_mfn(pg);
> 
> With pdx compression this is a non-trivial conversion. The function
> being an internal helper and the caller already holding the MFN, I
> think it would be preferable if the MFN was passed in here. If done
> this way, you may want to consider adding an ASSERT() to double
> check both passed in arguments match up.
> 
>> +    /* 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);
>> +
>> +#ifdef CONFIG_ARM
> 
> If avoidable there should be no arch-specific code added to this
> file. Assuming another arch gained PGC_reserved, what's wrong with
> enabling this code right away for them as well? I.e. use
> PGC_reserved here instead of CONFIG_ARM? Alternatively this may
> want to be CONFIG_STATIC_ALLOCATION, assuming we consider
> PGC_reserved tied to it.
> 
>> +    if ( pg->count_info & PGC_reserved )
>> +    {
>> +        /* TODO: asynchronous scrubbing. */
>> +        if ( need_scrub )
>> +            scrub_one_page(pg);
>> +        return;
>> +    }
>> +#endif
>> +    if ( need_scrub )
> 
> Nit: Please have a blank line between these last two.
> 
>> +    {
>> +        pg->count_info |= PGC_need_scrub;
>> +        poison_one_page(pg);
>> +    }
>> +
>> +    return;
> 
> Please omit return statements at the end of functions returning void.
> 
>> +}
> 
> On the whole, bike shedding or not, I'm afraid the function's name
> doesn't match what it does: There's no freeing of a page here. What
> gets done is marking of a page as free. Hence maybe mark_page_free()
> or mark_free_page() or some such?
> 
>> @@ -1512,6 +1530,38 @@ static void free_heap_pages(
>>       spin_unlock(&heap_lock);
>>   }
>>   
>> +#ifdef CONFIG_STATIC_ALLOCATION
>> +/* 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++ )
>> +    {
>> +        switch ( pg[i].count_info & PGC_state )
>> +        {
>> +        case PGC_state_inuse:
>> +            BUG_ON(pg[i].count_info & PGC_broken);
>> +            /* Mark it free and reserved. */
>> +            pg[i].count_info = PGC_state_free | PGC_reserved;
>> +            break;
>> +
>> +        default:
>> +            printk(XENLOG_ERR
>> +                   "Page state shall be only in PGC_state_inuse. "
> 
> Why? A page (static or not) can become broken while in use. IOW I
> don't think you can avoid handling PGC_state_offlining here. At which
> point this code will match free_heap_pages()'es, and hence likely
> will want folding as well.
> 
>> --- 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_ARM
> 
> ITYM CONFIG_STATIC_ALLOCATION here?
> 
> Jan
> 

-- 
Julien Grall

RE: [PATCH 4/9] xen/arm: static memory initialization

[Penny Zheng]

Hi Julien and Jan

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, July 1, 2021 1:46 AM
> To: Jan Beulich <jbeulich@suse.com>; Penny Zheng <Penny.Zheng@arm.com>
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
> 
> Hi,
> 
> On 10/06/2021 10:35, Jan Beulich wrote:
> > On 07.06.2021 04:43, Penny Zheng wrote:
> >> --- a/xen/arch/arm/setup.c
> >> +++ b/xen/arch/arm/setup.c
> >> @@ -611,6 +611,30 @@ static void __init init_pdx(void)
> >>       }
> >>   }
> >>
> >> +/* Static memory initialization */
> >> +static void __init init_staticmem_pages(void) {
> >> +    int bank;
> >
> > While I'm not a maintainer of this code, I'd still like to point out
> > that wherever possible we prefer "unsigned int" when dealing with only
> > non-negative values, and even more so when using them as array
> > indexes.
> 
> +1.
>

Understood. Thx.

> >
> >> +    /*
> >> +     * TODO: Considering NUMA-support scenario.
> >> +     */
> >
> > Nit: Comment style.
> >

Sure, thx.

> >> @@ -872,6 +896,9 @@ void __init start_xen(unsigned long
> boot_phys_offset,
> >>       cmdline_parse(cmdline);
> >>
> >>       setup_mm();
> >> +    /* If exists, Static Memory Initialization. */
> >> +    if ( bootinfo.static_mem.nr_banks > 0 )
> >> +        init_staticmem_pages();
> >
> > I don't think the conditional is really needed here?
> >

Sure, right. 
Like what Julien suggests,  init_staticmem_pages() is already able to cope with nr_banks == 0.

> >> --- a/xen/common/page_alloc.c
> >> +++ b/xen/common/page_alloc.c
> >> @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void)
> >>       return node_to_scrub(false) != NUMA_NO_NODE;
> >>   }
> >>
> >> +static void free_page(struct page_info *pg, bool need_scrub) {
> >> +    mfn_t mfn = page_to_mfn(pg);
> >
> > With pdx compression this is a non-trivial conversion. The function
> > being an internal helper and the caller already holding the MFN, I
> > think it would be preferable if the MFN was passed in here. If done
> > this way, you may want to consider adding an ASSERT() to double check
> > both passed in arguments match up.
> >

Thank for the suggestion~

> >> +    /* 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);
> >> +
> >> +#ifdef CONFIG_ARM
> >
> > If avoidable there should be no arch-specific code added to this file.
> > Assuming another arch gained PGC_reserved, what's wrong with enabling
> > this code right away for them as well? I.e. use PGC_reserved here
> > instead of CONFIG_ARM? Alternatively this may want to be
> > CONFIG_STATIC_ALLOCATION, assuming we consider PGC_reserved tied to
> > it.
> >

To not bring dead codes in other archs, I'll use more generic option CONFIG_STATIC_ALLOCATION.

> >> +    if ( pg->count_info & PGC_reserved )
> >> +    {
> >> +        /* TODO: asynchronous scrubbing. */
> >> +        if ( need_scrub )
> >> +            scrub_one_page(pg);
> >> +        return;
> >> +    }
> >> +#endif
> >> +    if ( need_scrub )
> >
> > Nit: Please have a blank line between these last two.
> >

Sure. Will do.

> >> +    {
> >> +        pg->count_info |= PGC_need_scrub;
> >> +        poison_one_page(pg);
> >> +    }
> >> +
> >> +    return;
> >
> > Please omit return statements at the end of functions returning void.
> >

Sure, thx

> >> +}
> >
> > On the whole, bike shedding or not, I'm afraid the function's name
> > doesn't match what it does: There's no freeing of a page here. What
> > gets done is marking of a page as free. Hence maybe mark_page_free()
> > or mark_free_page() or some such?
> >

Ok. Thx. Always not good at giving names. I'll take mark_page_free()

> >> @@ -1512,6 +1530,38 @@ static void free_heap_pages(
> >>       spin_unlock(&heap_lock);
> >>   }
> >>
> >> +#ifdef CONFIG_STATIC_ALLOCATION
> >> +/* 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++ )
> >> +    {
> >> +        switch ( pg[i].count_info & PGC_state )
> >> +        {
> >> +        case PGC_state_inuse:
> >> +            BUG_ON(pg[i].count_info & PGC_broken);
> >> +            /* Mark it free and reserved. */
> >> +            pg[i].count_info = PGC_state_free | PGC_reserved;
> >> +            break;
> >> +
> >> +        default:
> >> +            printk(XENLOG_ERR
> >> +                   "Page state shall be only in PGC_state_inuse. "
> >
> > Why? A page (static or not) can become broken while in use. IOW I
> > don't think you can avoid handling PGC_state_offlining here. At which
> > point this code will match free_heap_pages()'es, and hence likely will
> > want folding as well.
> >

Yeah, I was following the logic in free_heap_pages.
Hmmm, I could not think of any scenario that will lead to PGC_state_offlining, that's why
I was not including it at the first place.
For broken issue, tbh, I just copy the bug_on from free_heap_pages, after quite a time thinking,
I also could not find any scenario when a page(static or not) can become broken while in use. ;/

> >> --- 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_ARM
> >
> > ITYM CONFIG_STATIC_ALLOCATION here?
> >
> > Jan
> >
> 
> --
> Julien Grall

Cheers

--
Penny Zheng

RE: [PATCH 4/9] xen/arm: static memory initialization

[Penny Zheng]

Hi Jan 

> -----Original Message-----
> From: Penny Zheng
> Sent: Monday, July 5, 2021 1:22 PM
> To: Julien Grall <julien@xen.org>; Jan Beulich <jbeulich@suse.com>
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Subject: RE: [PATCH 4/9] xen/arm: static memory initialization
> 
> Hi Julien and Jan
> 
> > -----Original Message-----
> > From: Julien Grall <julien@xen.org>
> > Sent: Thursday, July 1, 2021 1:46 AM
> > To: Jan Beulich <jbeulich@suse.com>; Penny Zheng
> <Penny.Zheng@arm.com>
> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> > <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> > sstabellini@kernel.org
> > Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
> >
> > Hi,
> >
> > On 10/06/2021 10:35, Jan Beulich wrote:
> > > On 07.06.2021 04:43, Penny Zheng wrote:
> > >> --- a/xen/arch/arm/setup.c
> > >> +++ b/xen/arch/arm/setup.c
> > >> @@ -611,6 +611,30 @@ static void __init init_pdx(void)
> > >>       }
> > >>   }
> > >>
> > >> +/* Static memory initialization */ static void __init
> > >> +init_staticmem_pages(void) {
> > >> +    int bank;
> > >
> > > While I'm not a maintainer of this code, I'd still like to point out
> > > that wherever possible we prefer "unsigned int" when dealing with
> > > only non-negative values, and even more so when using them as array
> > > indexes.
> >
> > +1.
> >
> 
> Understood. Thx.
> 
> > >
> > >> +    /*
> > >> +     * TODO: Considering NUMA-support scenario.
> > >> +     */
> > >
> > > Nit: Comment style.
> > >
> 
> Sure, thx.
> 
> > >> @@ -872,6 +896,9 @@ void __init start_xen(unsigned long
> > boot_phys_offset,
> > >>       cmdline_parse(cmdline);
> > >>
> > >>       setup_mm();
> > >> +    /* If exists, Static Memory Initialization. */
> > >> +    if ( bootinfo.static_mem.nr_banks > 0 )
> > >> +        init_staticmem_pages();
> > >
> > > I don't think the conditional is really needed here?
> > >
> 
> Sure, right.
> Like what Julien suggests,  init_staticmem_pages() is already able to cope
> with nr_banks == 0.
> 
> > >> --- a/xen/common/page_alloc.c
> > >> +++ b/xen/common/page_alloc.c
> > >> @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void)
> > >>       return node_to_scrub(false) != NUMA_NO_NODE;
> > >>   }
> > >>
> > >> +static void free_page(struct page_info *pg, bool need_scrub) {
> > >> +    mfn_t mfn = page_to_mfn(pg);
> > >
> > > With pdx compression this is a non-trivial conversion. The function
> > > being an internal helper and the caller already holding the MFN, I
> > > think it would be preferable if the MFN was passed in here. If done
> > > this way, you may want to consider adding an ASSERT() to double
> > > check both passed in arguments match up.
> > >
> 
> Thank for the suggestion~
> 

While applying your suggestion here, if adding an ASSERT() to double check both passed-in
arguments match up, either use like page_to_mfn to establish connection, which is absolutely
unacceptable, or pass more parameters like base page/mfn to compare the offset. Hmmm,
I am not in favor of this,  since both extra parameters are only used in assertion only. 
 
> > >> +    /* 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);
> > >> +
> > >> +#ifdef CONFIG_ARM
> > >
> > > If avoidable there should be no arch-specific code added to this file.
> > > Assuming another arch gained PGC_reserved, what's wrong with
> > > enabling this code right away for them as well? I.e. use
> > > PGC_reserved here instead of CONFIG_ARM? Alternatively this may want
> > > to be CONFIG_STATIC_ALLOCATION, assuming we consider PGC_reserved
> > > tied to it.
> > >
> 
> To not bring dead codes in other archs, I'll use more generic option
> CONFIG_STATIC_ALLOCATION.
> 
> > >> +    if ( pg->count_info & PGC_reserved )
> > >> +    {
> > >> +        /* TODO: asynchronous scrubbing. */
> > >> +        if ( need_scrub )
> > >> +            scrub_one_page(pg);
> > >> +        return;
> > >> +    }
> > >> +#endif
> > >> +    if ( need_scrub )
> > >
> > > Nit: Please have a blank line between these last two.
> > >
> 
> Sure. Will do.
> 
> > >> +    {
> > >> +        pg->count_info |= PGC_need_scrub;
> > >> +        poison_one_page(pg);
> > >> +    }
> > >> +
> > >> +    return;
> > >
> > > Please omit return statements at the end of functions returning void.
> > >
> 
> Sure, thx
> 
> > >> +}
> > >
> > > On the whole, bike shedding or not, I'm afraid the function's name
> > > doesn't match what it does: There's no freeing of a page here. What
> > > gets done is marking of a page as free. Hence maybe mark_page_free()
> > > or mark_free_page() or some such?
> > >
> 
> Ok. Thx. Always not good at giving names. I'll take mark_page_free()
> 
> > >> @@ -1512,6 +1530,38 @@ static void free_heap_pages(
> > >>       spin_unlock(&heap_lock);
> > >>   }
> > >>
> > >> +#ifdef CONFIG_STATIC_ALLOCATION
> > >> +/* 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++ )
> > >> +    {
> > >> +        switch ( pg[i].count_info & PGC_state )
> > >> +        {
> > >> +        case PGC_state_inuse:
> > >> +            BUG_ON(pg[i].count_info & PGC_broken);
> > >> +            /* Mark it free and reserved. */
> > >> +            pg[i].count_info = PGC_state_free | PGC_reserved;
> > >> +            break;
> > >> +
> > >> +        default:
> > >> +            printk(XENLOG_ERR
> > >> +                   "Page state shall be only in PGC_state_inuse. "
> > >
> > > Why? A page (static or not) can become broken while in use. IOW I
> > > don't think you can avoid handling PGC_state_offlining here. At
> > > which point this code will match free_heap_pages()'es, and hence
> > > likely will want folding as well.
> > >
> 
> Yeah, I was following the logic in free_heap_pages.
> Hmmm, I could not think of any scenario that will lead to PGC_state_offlining,
> that's why I was not including it at the first place.
> For broken issue, tbh, I just copy the bug_on from free_heap_pages, after
> quite a time thinking, I also could not find any scenario when a page(static or
> not) can become broken while in use. ;/
> 
> > >> --- 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_ARM
> > >
> > > ITYM CONFIG_STATIC_ALLOCATION here?
> > >
> > > Jan
> > >
> >
> > --
> > Julien Grall
> 
> Cheers
> 
> --
> Penny Zheng

Re: [PATCH 4/9] xen/arm: static memory initialization

[Jan Beulich]

On 05.07.2021 09:14, Penny Zheng wrote:
>> From: Penny Zheng
>> Sent: Monday, July 5, 2021 1:22 PM
>>
>>> From: Julien Grall <julien@xen.org>
>>> Sent: Thursday, July 1, 2021 1:46 AM
>>>
>>> On 10/06/2021 10:35, Jan Beulich wrote:
>>>> On 07.06.2021 04:43, Penny Zheng wrote:
>>>>> @@ -1512,6 +1530,38 @@ static void free_heap_pages(
>>>>>       spin_unlock(&heap_lock);
>>>>>   }
>>>>>
>>>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>>>> +/* 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++ )
>>>>> +    {
>>>>> +        switch ( pg[i].count_info & PGC_state )
>>>>> +        {
>>>>> +        case PGC_state_inuse:
>>>>> +            BUG_ON(pg[i].count_info & PGC_broken);
>>>>> +            /* Mark it free and reserved. */
>>>>> +            pg[i].count_info = PGC_state_free | PGC_reserved;
>>>>> +            break;
>>>>> +
>>>>> +        default:
>>>>> +            printk(XENLOG_ERR
>>>>> +                   "Page state shall be only in PGC_state_inuse. "
>>>>
>>>> Why? A page (static or not) can become broken while in use. IOW I
>>>> don't think you can avoid handling PGC_state_offlining here. At
>>>> which point this code will match free_heap_pages()'es, and hence
>>>> likely will want folding as well.
>>>>
>>
>> Yeah, I was following the logic in free_heap_pages.
>> Hmmm, I could not think of any scenario that will lead to PGC_state_offlining,
>> that's why I was not including it at the first place.
>> For broken issue, tbh, I just copy the bug_on from free_heap_pages, after
>> quite a time thinking, I also could not find any scenario when a page(static or
>> not) can become broken while in use. ;/

I'm, afraid I don't understand. Using page_to_mfn(), expensive or not,
in ASSERT() is quite fine. The (expensive) expression won't be evaluated
in release builds. This is specifically different from BUG_ON().

Jan

RE: [PATCH 4/9] xen/arm: static memory initialization

[Penny Zheng]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, July 5, 2021 3:51 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; Julien Grall <julien@xen.org>
> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
> 
> On 05.07.2021 09:14, Penny Zheng wrote:
> >> From: Penny Zheng
> >> Sent: Monday, July 5, 2021 1:22 PM
> >>
> >>> From: Julien Grall <julien@xen.org>
> >>> Sent: Thursday, July 1, 2021 1:46 AM
> >>>
> >>> On 10/06/2021 10:35, Jan Beulich wrote:
> >>>> On 07.06.2021 04:43, Penny Zheng wrote:
> >>>>> @@ -1512,6 +1530,38 @@ static void free_heap_pages(
> >>>>>       spin_unlock(&heap_lock);
> >>>>>   }
> >>>>>
> >>>>> +#ifdef CONFIG_STATIC_ALLOCATION
> >>>>> +/* 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++ )
> >>>>> +    {
> >>>>> +        switch ( pg[i].count_info & PGC_state )
> >>>>> +        {
> >>>>> +        case PGC_state_inuse:
> >>>>> +            BUG_ON(pg[i].count_info & PGC_broken);
> >>>>> +            /* Mark it free and reserved. */
> >>>>> +            pg[i].count_info = PGC_state_free | PGC_reserved;
> >>>>> +            break;
> >>>>> +
> >>>>> +        default:
> >>>>> +            printk(XENLOG_ERR
> >>>>> +                   "Page state shall be only in PGC_state_inuse. "
> >>>>
> >>>> Why? A page (static or not) can become broken while in use. IOW I
> >>>> don't think you can avoid handling PGC_state_offlining here. At
> >>>> which point this code will match free_heap_pages()'es, and hence
> >>>> likely will want folding as well.
> >>>>
> >>
> >> Yeah, I was following the logic in free_heap_pages.
> >> Hmmm, I could not think of any scenario that will lead to
> >> PGC_state_offlining, that's why I was not including it at the first place.
> >> For broken issue, tbh, I just copy the bug_on from free_heap_pages,
> >> after quite a time thinking, I also could not find any scenario when
> >> a page(static or
> >> not) can become broken while in use. ;/
> 
> I'm, afraid I don't understand. Using page_to_mfn(), expensive or not, in
> ASSERT() is quite fine. The (expensive) expression won't be evaluated in
> release builds. This is specifically different from BUG_ON().
> 

Thanks for the explanation. 😉
I'll use the ASSERT() to do the check.

> Jan

Cheers

Penny Zheng

Re: [PATCH 4/9] xen/arm: static memory initialization

[Jan Beulich]

On 05.07.2021 07:22, Penny Zheng wrote:
>> From: Julien Grall <julien@xen.org>
>> Sent: Thursday, July 1, 2021 1:46 AM
>>
>> On 10/06/2021 10:35, Jan Beulich wrote:
>>> On 07.06.2021 04:43, Penny Zheng wrote:
>>>> @@ -1512,6 +1530,38 @@ static void free_heap_pages(
>>>>       spin_unlock(&heap_lock);
>>>>   }
>>>>
>>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>>> +/* 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++ )
>>>> +    {
>>>> +        switch ( pg[i].count_info & PGC_state )
>>>> +        {
>>>> +        case PGC_state_inuse:
>>>> +            BUG_ON(pg[i].count_info & PGC_broken);
>>>> +            /* Mark it free and reserved. */
>>>> +            pg[i].count_info = PGC_state_free | PGC_reserved;
>>>> +            break;
>>>> +
>>>> +        default:
>>>> +            printk(XENLOG_ERR
>>>> +                   "Page state shall be only in PGC_state_inuse. "
>>>
>>> Why? A page (static or not) can become broken while in use. IOW I
>>> don't think you can avoid handling PGC_state_offlining here. At which
>>> point this code will match free_heap_pages()'es, and hence likely will
>>> want folding as well.
>>>
> 
> Yeah, I was following the logic in free_heap_pages.
> Hmmm, I could not think of any scenario that will lead to PGC_state_offlining, that's why
> I was not including it at the first place.
> For broken issue, tbh, I just copy the bug_on from free_heap_pages, after quite a time thinking,
> I also could not find any scenario when a page(static or not) can become broken while in use. ;/

I can see that what you say may be true for Arm, but we're in generic
code here with an arch-independent CONFIG_STATIC_ALLOCATION conditional
around. Hence you want to avoid deliberately not handling a case that
can occur on e.g. x86 (see mark_page_offline() and further related
handling elsewhere). I'd perhaps view this differently if you were
introducing completely new code, but you've specifically said you're
cloning existing code (where the case is being handled). Plus, as said,
you'll likely be able to actually share code by not excluding the case
here.

Jan

Re: [PATCH 4/9] xen/arm: static memory initialization

[Julien Grall]

Hi Penny,

On 07/06/2021 03:43, Penny Zheng wrote:
> This patch introduces static memory initialization, during system RAM boot up.

The word "RAM" looks spurious.

> New func init_staticmem_pages is responsible for static memory initialization.

s/New func/The new function/

> Helper free_staticmem_pages is the equivalent of free_heap_pages, to free
> nr_mfns pages of static memory.
> 
> This commit defines a new helper free_page to extract common code between
> free_heap_pages and free_staticmem_pages, like following the same cache/TLB
> coherency policy.
> 
> For each page, free_staticmem_pages includes the following extra steps to
> initialize:
> 1. change page state from inuse to free state and grant PGC_reserved.

I think you mean "set" rather than "grant".

> 2. scrub the page in need synchronously.

Can you explain why this is necessary?

> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> changes v2:
> - rename to nr_mfns
> - extract common code from free_heap_pages and free_staticmem_pages
> - remove dead codes in other archs, including move some to arm-specific file,
> and put some under CONFIG_ARM
> - mark free_staticmem_pages __init
> ---
>   xen/arch/arm/setup.c    | 27 ++++++++++++++

I think it would be best to split the arm use in a separate patch.

>   xen/common/page_alloc.c | 78 +++++++++++++++++++++++++++++++++--------
>   xen/include/xen/mm.h    |  6 ++++
>   3 files changed, 97 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 00aad1c194..daafea0abb 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -611,6 +611,30 @@ static void __init init_pdx(void)
>       }
>   }
>   
> +/* Static memory initialization */
> +static void __init init_staticmem_pages(void)
> +{
> +    int bank;
> +
> +    /*
> +     * TODO: Considering NUMA-support scenario.
> +     */
> +    for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ )
> +    {
> +        paddr_t bank_start = bootinfo.static_mem.bank[bank].start;
> +        paddr_t bank_size = bootinfo.static_mem.bank[bank].size;
> +        paddr_t bank_end = bank_start + bank_size;
> +
> +        bank_start = round_pgup(bank_start);
> +        bank_end = round_pgdown(bank_end);
> +        if ( bank_end <= bank_start )
> +            return;
> +
> +        free_staticmem_pages(maddr_to_page(bank_start),
> +                            (bank_end - bank_start) >> PAGE_SHIFT, false);
> +    }
> +}
> +
>   #ifdef CONFIG_ARM_32
>   static void __init setup_mm(void)
>   {
> @@ -872,6 +896,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>       cmdline_parse(cmdline);
>   
>       setup_mm();
> +    /* If exists, Static Memory Initialization. */
> +    if ( bootinfo.static_mem.nr_banks > 0 )

This check seems a pointless because init_staticmem_pages() is already 
able to cope with nr_banks == 0.

> +        init_staticmem_pages();
I would prefer if this is folded in setup_mm().

>   
>       /* Parse the ACPI tables for possible boot-time configuration */
>       acpi_boot_table_init();
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 958ba0cd92..8c00262c04 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void)
>       return node_to_scrub(false) != NUMA_NO_NODE;
>   }
>   
> +static void free_page(struct page_info *pg, bool need_scrub)
> +{
> +    mfn_t mfn = page_to_mfn(pg);
> +
> +    /* 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);
> +
> +#ifdef CONFIG_ARM

To echo what Jan already wrote, I am not in favor of adding new #ifdef 
CONFIG_<arch> in common code. I would expect the logic for static memory 
to be the same for each arch, so this should be protected with a generic 
Kconfig.

> +    if ( pg->count_info & PGC_reserved )
> +    {
> +        /* TODO: asynchronous scrubbing. */
> +        if ( need_scrub )
> +            scrub_one_page(pg);
> +        return;
> +    }
> +#endif
> +    if ( need_scrub )
> +    {
> +        pg->count_info |= PGC_need_scrub;
> +        poison_one_page(pg);
> +    }
> +
> +    return;
> +}
> +
>   /* Free 2^@order set of pages. */
>   static void free_heap_pages(
>       struct page_info *pg, unsigned int order, bool need_scrub)
> @@ -1425,20 +1456,7 @@ static void free_heap_pages(
>               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);
> -
> -        if ( need_scrub )
> -        {
> -            pg[i].count_info |= PGC_need_scrub;
> -            poison_one_page(&pg[i]);
> -        }
> +        free_page(&pg[i], need_scrub);
>       }
>   
>       avail[node][zone] += 1 << order;
> @@ -1512,6 +1530,38 @@ static void free_heap_pages(
>       spin_unlock(&heap_lock);
>   }
>   
> +#ifdef CONFIG_STATIC_ALLOCATION
> +/* 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++ )
> +    {
> +        switch ( pg[i].count_info & PGC_state )
> +        {
> +        case PGC_state_inuse:
> +            BUG_ON(pg[i].count_info & PGC_broken);
> +            /* Mark it free and reserved. */
> +            pg[i].count_info = PGC_state_free | PGC_reserved;
> +            break;
> +
> +        default:
> +            printk(XENLOG_ERR
> +                   "Page state shall be only in PGC_state_inuse. "
> +                   "pg[%lu] MFN %"PRI_mfn" count_info=%#lx tlbflush_timestamp=%#x.\n",
> +                   i, mfn_x(mfn) + i,
> +                   pg[i].count_info,
> +                   pg[i].tlbflush_timestamp);
> +            BUG();
> +        }
> +
> +        free_page(&pg[i], need_scrub);
> +    }
> +}
> +#endif
>   
>   /*
>    * Following rules applied for page offline:
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 667f9dac83..df25e55966 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_ARM
> +/* Static Allocation */
> +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,
> 

-- 
Julien Grall

RE: [PATCH 4/9] xen/arm: static memory initialization

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, July 1, 2021 2:10 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
> 
> Hi Penny,
> 
> On 07/06/2021 03:43, Penny Zheng wrote:
> > This patch introduces static memory initialization, during system RAM boot
> up.
> 
> The word "RAM" looks spurious.
> 

Thx. I check the "spurious" in dictionary, it means fake? So I will leave "during system boot up"
here.

> > New func init_staticmem_pages is responsible for static memory
> initialization.
> 
> s/New func/The new function/
>
 
Sure. thx

> > Helper free_staticmem_pages is the equivalent of free_heap_pages, to
> > free nr_mfns pages of static memory.
> >
> > This commit defines a new helper free_page to extract common code
> > between free_heap_pages and free_staticmem_pages, like following the
> > same cache/TLB coherency policy.
> >
> > For each page, free_staticmem_pages includes the following extra steps
> > to
> > initialize:
> > 1. change page state from inuse to free state and grant PGC_reserved.
> 
> I think you mean "set" rather than "grant".
> 

Yeah.  I'll change to set here~

> > 2. scrub the page in need synchronously.
> 
> Can you explain why this is necessary?
>

Since I'm borrowing the logic in free_heap_pages, I'm also trying to cover all the scenarios here like it does.
So I assume that free_staticmem_page will not only be used on initialization, but also when destroying/rebooting the domain.
On these cases, it is necessary to scrub the page, ig.
 
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > changes v2:
> > - rename to nr_mfns
> > - extract common code from free_heap_pages and free_staticmem_pages
> > - remove dead codes in other archs, including move some to
> > arm-specific file, and put some under CONFIG_ARM
> > - mark free_staticmem_pages __init
> > ---
> >   xen/arch/arm/setup.c    | 27 ++++++++++++++
> 
> I think it would be best to split the arm use in a separate patch.

Sure, I'll move them to another commit.

> 
> >   xen/common/page_alloc.c | 78 +++++++++++++++++++++++++++++++++---
> -----
> >   xen/include/xen/mm.h    |  6 ++++
> >   3 files changed, 97 insertions(+), 14 deletions(-)
> >
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index
> > 00aad1c194..daafea0abb 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -611,6 +611,30 @@ static void __init init_pdx(void)
> >       }
> >   }
> >
> > +/* Static memory initialization */
> > +static void __init init_staticmem_pages(void) {
> > +    int bank;
> > +
> > +    /*
> > +     * TODO: Considering NUMA-support scenario.
> > +     */
> > +    for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ )
> > +    {
> > +        paddr_t bank_start = bootinfo.static_mem.bank[bank].start;
> > +        paddr_t bank_size = bootinfo.static_mem.bank[bank].size;
> > +        paddr_t bank_end = bank_start + bank_size;
> > +
> > +        bank_start = round_pgup(bank_start);
> > +        bank_end = round_pgdown(bank_end);
> > +        if ( bank_end <= bank_start )
> > +            return;
> > +
> > +        free_staticmem_pages(maddr_to_page(bank_start),
> > +                            (bank_end - bank_start) >> PAGE_SHIFT, false);
> > +    }
> > +}
> > +
> >   #ifdef CONFIG_ARM_32
> >   static void __init setup_mm(void)
> >   {
> > @@ -872,6 +896,9 @@ void __init start_xen(unsigned long
> boot_phys_offset,
> >       cmdline_parse(cmdline);
> >
> >       setup_mm();
> > +    /* If exists, Static Memory Initialization. */
> > +    if ( bootinfo.static_mem.nr_banks > 0 )
> 
> This check seems a pointless because init_staticmem_pages() is already able
> to cope with nr_banks == 0.
> 
> > +        init_staticmem_pages();
> I would prefer if this is folded in setup_mm().
>

Sure.
 
> >
> >       /* Parse the ACPI tables for possible boot-time configuration */
> >       acpi_boot_table_init();
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > 958ba0cd92..8c00262c04 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void)
> >       return node_to_scrub(false) != NUMA_NO_NODE;
> >   }
> >
> > +static void free_page(struct page_info *pg, bool need_scrub) {
> > +    mfn_t mfn = page_to_mfn(pg);
> > +
> > +    /* 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);
> > +
> > +#ifdef CONFIG_ARM
> 
> To echo what Jan already wrote, I am not in favor of adding new #ifdef
> CONFIG_<arch> in common code. I would expect the logic for static memory
> to be the same for each arch, so this should be protected with a generic
> Kconfig.
> 
> > +    if ( pg->count_info & PGC_reserved )
> > +    {
> > +        /* TODO: asynchronous scrubbing. */
> > +        if ( need_scrub )
> > +            scrub_one_page(pg);
> > +        return;
> > +    }
> > +#endif
> > +    if ( need_scrub )
> > +    {
> > +        pg->count_info |= PGC_need_scrub;
> > +        poison_one_page(pg);
> > +    }
> > +
> > +    return;
> > +}
> > +
> >   /* Free 2^@order set of pages. */
> >   static void free_heap_pages(
> >       struct page_info *pg, unsigned int order, bool need_scrub) @@
> > -1425,20 +1456,7 @@ static void free_heap_pages(
> >               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);
> > -
> > -        if ( need_scrub )
> > -        {
> > -            pg[i].count_info |= PGC_need_scrub;
> > -            poison_one_page(&pg[i]);
> > -        }
> > +        free_page(&pg[i], need_scrub);
> >       }
> >
> >       avail[node][zone] += 1 << order; @@ -1512,6 +1530,38 @@ static
> > void free_heap_pages(
> >       spin_unlock(&heap_lock);
> >   }
> >
> > +#ifdef CONFIG_STATIC_ALLOCATION
> > +/* 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++ )
> > +    {
> > +        switch ( pg[i].count_info & PGC_state )
> > +        {
> > +        case PGC_state_inuse:
> > +            BUG_ON(pg[i].count_info & PGC_broken);
> > +            /* Mark it free and reserved. */
> > +            pg[i].count_info = PGC_state_free | PGC_reserved;
> > +            break;
> > +
> > +        default:
> > +            printk(XENLOG_ERR
> > +                   "Page state shall be only in PGC_state_inuse. "
> > +                   "pg[%lu] MFN %"PRI_mfn" count_info=%#lx
> tlbflush_timestamp=%#x.\n",
> > +                   i, mfn_x(mfn) + i,
> > +                   pg[i].count_info,
> > +                   pg[i].tlbflush_timestamp);
> > +            BUG();
> > +        }
> > +
> > +        free_page(&pg[i], need_scrub);
> > +    }
> > +}
> > +#endif
> >
> >   /*
> >    * Following rules applied for page offline:
> > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index
> > 667f9dac83..df25e55966 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_ARM
> > +/* Static Allocation */
> > +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,
> >
> 
> --
> Julien Grall


Cheers
Penny Zheng

Re: [PATCH 4/9] xen/arm: static memory initialization

[Julien Grall]

On 05/07/2021 08:28, Penny Zheng wrote:
> Hi Julien

Hi Penny,

>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Thursday, July 1, 2021 2:10 AM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org; jbeulich@suse.com
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>
>> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
>>
>> Hi Penny,
>>
>> On 07/06/2021 03:43, Penny Zheng wrote:
>>> This patch introduces static memory initialization, during system RAM boot
>> up.
>>
>> The word "RAM" looks spurious.
>>
> 
> Thx. I check the "spurious" in dictionary, it means fake? So I will leave "during system boot up"
> here.

Yes, this reads better.

>>> 2. scrub the page in need synchronously.
>>
>> Can you explain why this is necessary?
>>
> 
> Since I'm borrowing the logic in free_heap_pages, I'm also trying to cover all the scenarios here like it does.
> So I assume that free_staticmem_page will not only be used on initialization, but also when destroying/rebooting the domain.
> On these cases, it is necessary to scrub the page, ig.

I wasn't asking about scrubbing specifically but instead why it is 
synchronous. Sorry for the confusion.

Cheers,

-- 
Julien Grall

RE: [PATCH 4/9] xen/arm: static memory initialization

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Tuesday, July 6, 2021 5:10 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
> 
> 
> 
> On 05/07/2021 08:28, Penny Zheng wrote:
> > Hi Julien
> 
> Hi Penny,
> 
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Thursday, July 1, 2021 2:10 AM
> >> To: Penny Zheng <Penny.Zheng@arm.com>;
> >> xen-devel@lists.xenproject.org; sstabellini@kernel.org;
> >> jbeulich@suse.com
> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> >> <Wei.Chen@arm.com>
> >> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
> >>
> >> Hi Penny,
> >>
> >> On 07/06/2021 03:43, Penny Zheng wrote:
> >>> This patch introduces static memory initialization, during system
> >>> RAM boot
> >> up.
> >>
> >> The word "RAM" looks spurious.
> >>
> >
> > Thx. I check the "spurious" in dictionary, it means fake? So I will leave
> "during system boot up"
> > here.
> 
> Yes, this reads better.
> 
> >>> 2. scrub the page in need synchronously.
> >>
> >> Can you explain why this is necessary?
> >>
> >
> > Since I'm borrowing the logic in free_heap_pages, I'm also trying to cover all
> the scenarios here like it does.
> > So I assume that free_staticmem_page will not only be used on initialization,
> but also when destroying/rebooting the domain.
> > On these cases, it is necessary to scrub the page, ig.
> 
> I wasn't asking about scrubbing specifically but instead why it is synchronous.
> Sorry for the confusion.
> 

I've read asynchronous scrubbing in buddy allocator, pages that need a scrub are added to tail, 
and specific working thread is working on it, hmm, imo, I don't think it could be simply applied into static
memory. :/

So I put asynchronous scrubbing in #TODO.

> Cheers,
> 
> --
> Julien Grall

Cheers

--
Penny Zheng

Re: [PATCH 4/9] xen/arm: static memory initialization

[Julien Grall]

On 06/07/2021 10:20, Penny Zheng wrote:
> Hi Julien

Hi Penny,

>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Tuesday, July 6, 2021 5:10 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org; jbeulich@suse.com
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>
>> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
>>
>>
>>
>> On 05/07/2021 08:28, Penny Zheng wrote:
>>> Hi Julien
>>
>> Hi Penny,
>>
>>>> -----Original Message-----
>>>> From: Julien Grall <julien@xen.org>
>>>> Sent: Thursday, July 1, 2021 2:10 AM
>>>> To: Penny Zheng <Penny.Zheng@arm.com>;
>>>> xen-devel@lists.xenproject.org; sstabellini@kernel.org;
>>>> jbeulich@suse.com
>>>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>>>> <Wei.Chen@arm.com>
>>>> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
>>>>
>>>> Hi Penny,
>>>>
>>>> On 07/06/2021 03:43, Penny Zheng wrote:
>>>>> This patch introduces static memory initialization, during system
>>>>> RAM boot
>>>> up.
>>>>
>>>> The word "RAM" looks spurious.
>>>>
>>>
>>> Thx. I check the "spurious" in dictionary, it means fake? So I will leave
>> "during system boot up"
>>> here.
>>
>> Yes, this reads better.
>>
>>>>> 2. scrub the page in need synchronously.
>>>>
>>>> Can you explain why this is necessary?
>>>>
>>>
>>> Since I'm borrowing the logic in free_heap_pages, I'm also trying to cover all
>> the scenarios here like it does.
>>> So I assume that free_staticmem_page will not only be used on initialization,
>> but also when destroying/rebooting the domain.
>>> On these cases, it is necessary to scrub the page, ig.
>>
>> I wasn't asking about scrubbing specifically but instead why it is synchronous.
>> Sorry for the confusion.
>>
> 
> I've read asynchronous scrubbing in buddy allocator, pages that need a scrub are added to tail,
> and specific working thread is working on it, hmm, imo, I don't think it could be simply applied into static
> memory. :/

I am not sure to understand why the same concept can't be applied. 
Anyway, what I am asking is to clarify in the commit message why this is 
synchronous (e.g. this just temporary).

>  > So I put asynchronous scrubbing in #TODO.

We seem to have quite a few TODOs (some of them are implicit). Can we 
start to gather a list of what's missing? A first good place would be 
the cover lette.r

Cheers,

-- 
Julien Grall

[PATCH 5/9] xen: introduce assign_pages_nr

[Penny Zheng]

Introduce new interface assign_pages_nr to deal with when page number is
not in a power-of-two, which will save the trouble each time user needs
to split the size in a power of 2 to use assign_pages.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
changes v2:
- introduce new interface assign_pages_nr
---
 xen/common/page_alloc.c | 26 +++++++++++++++++---------
 xen/include/xen/mm.h    | 10 ++++++++--
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 8c00262c04..e244d2e52e 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2301,14 +2301,14 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
 }
 
 
-int assign_pages(
+int assign_pages_nr(
     struct domain *d,
     struct page_info *pg,
-    unsigned int order,
+    unsigned int nr_pfns,
     unsigned int memflags)
 {
     int rc = 0;
-    unsigned long i;
+    unsigned int i;
 
     spin_lock(&d->page_alloc_lock);
 
@@ -2324,7 +2324,7 @@ int assign_pages(
     {
         unsigned int extra_pages = 0;
 
-        for ( i = 0; i < (1ul << order); i++ )
+        for ( i = 0; i < nr_pfns; i++ )
         {
             ASSERT(!(pg[i].count_info & ~PGC_extra));
             if ( pg[i].count_info & PGC_extra )
@@ -2333,18 +2333,18 @@ int assign_pages(
 
         ASSERT(!extra_pages ||
                ((memflags & MEMF_no_refcount) &&
-                extra_pages == 1u << order));
+                extra_pages == nr_pfns));
     }
 #endif
 
     if ( pg[0].count_info & PGC_extra )
     {
-        d->extra_pages += 1u << order;
+        d->extra_pages += nr_pfns;
         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_pfns;
 
         if ( unlikely(tot_pages > d->max_pages) )
         {
@@ -2356,10 +2356,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_pfns) == nr_pfns) )
         get_knownalive_domain(d);
 
-    for ( i = 0; i < (1 << order); i++ )
+    for ( i = 0; i < nr_pfns; i++ )
     {
         ASSERT(page_get_owner(&pg[i]) == NULL);
         page_set_owner(&pg[i], d);
@@ -2374,6 +2374,14 @@ int assign_pages(
     return rc;
 }
 
+int assign_pages(
+    struct domain *d,
+    struct page_info *pg,
+    unsigned int order,
+    unsigned int memflags)
+{
+    return assign_pages_nr(d, pg, (1U << order), memflags);
+}
 
 struct page_info *alloc_domheap_pages(
     struct domain *d, unsigned int order, unsigned int memflags)
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index df25e55966..25d970e857 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -131,12 +131,18 @@ int query_page_offline(mfn_t mfn, uint32_t *status);
 
 void heap_init_late(void);
 
-int assign_pages(
+int assign_pages_nr(
     struct domain *d,
     struct page_info *pg,
-    unsigned int order,
+    unsigned int nr_pfns,
     unsigned int memflags);
 
+ int assign_pages(
+     struct domain *d,
+     struct page_info *pg,
+     unsigned int order,
+     unsigned int memflags);
+
 /* Dump info to serial console */
 void arch_dump_shared_mem_info(void);
 
-- 
2.25.1

Re: [PATCH 5/9] xen: introduce assign_pages_nr

[Jan Beulich]

On 07.06.2021 04:43, Penny Zheng wrote:
> Introduce new interface assign_pages_nr to deal with when page number is
> not in a power-of-two, which will save the trouble each time user needs
> to split the size in a power of 2 to use assign_pages.

First of all I still don't see why in this one special case it is a
meaningful burden to do the count-to-order conversion in the caller you
mean to add, and hence why we really need this new function (to keep it
simple, you could even have the caller not break down to arbitrary
power-of-2 chunks, but simply iterate over all individual [order-0]
pages). The more that I'm not happy with the chosen name, despite it
having been suggested during v1 review. _If_ we needed two functions,
imo they ought to be named assign_page() (dealing with a single page of
the given order) and assign_pages(). Backporting confusion could be
helped by altering the order of parameters, such that the compiler
would point out that adjustments at call sites are needed.

Irrespective of this a few remarks on the code change itself:

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2301,14 +2301,14 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
>  }
>  
>  
> -int assign_pages(
> +int assign_pages_nr(
>      struct domain *d,
>      struct page_info *pg,
> -    unsigned int order,
> +    unsigned int nr_pfns,

Even leaving the naming aspect of "pfns" aside, I can't see why this
can't be simply "nr" (of appropriate type, see next remark).

>      unsigned int memflags)
>  {
>      int rc = 0;
> -    unsigned long i;
> +    unsigned int i;

This is not an acceptable type change, at least not as long as it's not
justified at all in the description. While both Arm and x86 will be
fine this way, the code here is supposed to be generic, and hence would
better remain generally correct.

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -131,12 +131,18 @@ int query_page_offline(mfn_t mfn, uint32_t *status);
>  
>  void heap_init_late(void);
>  
> -int assign_pages(
> +int assign_pages_nr(
>      struct domain *d,
>      struct page_info *pg,
> -    unsigned int order,
> +    unsigned int nr_pfns,
>      unsigned int memflags);
>  
> + int assign_pages(
> +     struct domain *d,
> +     struct page_info *pg,
> +     unsigned int order,
> +     unsigned int memflags);

Bogus extra leading space on all lines of this addition.

Jan

Re: [PATCH 5/9] xen: introduce assign_pages_nr

[Julien Grall]

Hi,

On 10/06/2021 10:49, Jan Beulich wrote:
> On 07.06.2021 04:43, Penny Zheng wrote:
>> Introduce new interface assign_pages_nr to deal with when page number is
>> not in a power-of-two, which will save the trouble each time user needs
>> to split the size in a power of 2 to use assign_pages.
> 
> First of all I still don't see why in this one special case it is a
> meaningful burden to do the count-to-order conversion in the caller you
> mean to add,

This sort of works for one caller. However, I would expect some more 
user in the future (we use it for Live-Update).

> and hence why we really need this new function (to keep it
> simple, you could even have the caller not break down to arbitrary
> power-of-2 chunks, but simply iterate over all individual [order-0]
> pages).

The function assign_pages() will always use 1U << order (and sadly 1 << 
order). So we would end up to convert the count in multiple order for 
then directly converting back to a number. To me, this sounds rather 
pointless...

There are also a slight benefits to call assign_pages() a single time 
during boot because it will reduce the number of time we need to 
lock/unlock d->page_alloc_lock.

> The more that I'm not happy with the chosen name, despite it
> having been suggested during v1 review. _If_ we needed two functions,
> imo they ought to be named assign_page() (dealing with a single page of
> the given order) and assign_pages(). Backporting confusion could be
> helped by altering the order of parameters, such that the compiler
> would point out that adjustments at call sites are needed.
> 
> Irrespective of this a few remarks on the code change itself:
> 
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -2301,14 +2301,14 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
>>   }
>>   
>>   
>> -int assign_pages(
>> +int assign_pages_nr(
>>       struct domain *d,
>>       struct page_info *pg,
>> -    unsigned int order,
>> +    unsigned int nr_pfns,
> 
> Even leaving the naming aspect of "pfns" aside, I can't see why this
> can't be simply "nr" (of appropriate type, see next remark).
> 
>>       unsigned int memflags)
>>   {
>>       int rc = 0;
>> -    unsigned long i;
>> +    unsigned int i;
> 
> This is not an acceptable type change, at least not as long as it's not
> justified at all in the description. While both Arm and x86 will be
> fine this way, the code here is supposed to be generic, and hence would
> better remain generally correct.

I would like to point out the code is already not correct as we are 
using 1U << order or worse 1 << order :).

Cheers,

-- 
Julien Grall

Re: [PATCH 5/9] xen: introduce assign_pages_nr

[Jan Beulich]

On 30.06.2021 20:29, Julien Grall wrote:
> On 10/06/2021 10:49, Jan Beulich wrote:
>> On 07.06.2021 04:43, Penny Zheng wrote:
>>> Introduce new interface assign_pages_nr to deal with when page number is
>>> not in a power-of-two, which will save the trouble each time user needs
>>> to split the size in a power of 2 to use assign_pages.
>>
>> First of all I still don't see why in this one special case it is a
>> meaningful burden to do the count-to-order conversion in the caller you
>> mean to add,
> 
> This sort of works for one caller. However, I would expect some more 
> user in the future (we use it for Live-Update).
> 
>> and hence why we really need this new function (to keep it
>> simple, you could even have the caller not break down to arbitrary
>> power-of-2 chunks, but simply iterate over all individual [order-0]
>> pages).
> 
> The function assign_pages() will always use 1U << order (and sadly 1 << 
> order). So we would end up to convert the count in multiple order for 
> then directly converting back to a number. To me, this sounds rather 
> pointless...
> 
> There are also a slight benefits to call assign_pages() a single time 
> during boot because it will reduce the number of time we need to 
> lock/unlock d->page_alloc_lock.

Well, all of this is why I did add ...

>> The more that I'm not happy with the chosen name, despite it
>> having been suggested during v1 review. _If_ we needed two functions,
>> imo they ought to be named assign_page() (dealing with a single page of
>> the given order) and assign_pages(). Backporting confusion could be
>> helped by altering the order of parameters, such that the compiler
>> would point out that adjustments at call sites are needed.

... this. Not sure whether you not commenting on it means you agree
with the proposal.

>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2301,14 +2301,14 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
>>>   }
>>>   
>>>   
>>> -int assign_pages(
>>> +int assign_pages_nr(
>>>       struct domain *d,
>>>       struct page_info *pg,
>>> -    unsigned int order,
>>> +    unsigned int nr_pfns,
>>
>> Even leaving the naming aspect of "pfns" aside, I can't see why this
>> can't be simply "nr" (of appropriate type, see next remark).
>>
>>>       unsigned int memflags)
>>>   {
>>>       int rc = 0;
>>> -    unsigned long i;
>>> +    unsigned int i;
>>
>> This is not an acceptable type change, at least not as long as it's not
>> justified at all in the description. While both Arm and x86 will be
>> fine this way, the code here is supposed to be generic, and hence would
>> better remain generally correct.
> 
> I would like to point out the code is already not correct as we are 
> using 1U << order or worse 1 << order :).

Indeed there are improvements (towards being consistent) to be made. But
this is not an excuse to make things worse here. At least one of the two
loops already properly uses 1ul; sadly that's only debugging code. And
of course something like domain_tot_pages() (and the underlying field)
dealing with "unsigned int" doesn't help consistency either. As it stands
we're limiting ourselves to 8Tb VMs, as it seems, and for no good reason.

Jan

Re: [PATCH 5/9] xen: introduce assign_pages_nr

[Julien Grall]

Hi Jan,

On 01/07/2021 09:26, Jan Beulich wrote:
> On 30.06.2021 20:29, Julien Grall wrote:
>> On 10/06/2021 10:49, Jan Beulich wrote:
>>> On 07.06.2021 04:43, Penny Zheng wrote:
>>>> Introduce new interface assign_pages_nr to deal with when page number is
>>>> not in a power-of-two, which will save the trouble each time user needs
>>>> to split the size in a power of 2 to use assign_pages.
>>>
>>> First of all I still don't see why in this one special case it is a
>>> meaningful burden to do the count-to-order conversion in the caller you
>>> mean to add,
>>
>> This sort of works for one caller. However, I would expect some more
>> user in the future (we use it for Live-Update).
>>
>>> and hence why we really need this new function (to keep it
>>> simple, you could even have the caller not break down to arbitrary
>>> power-of-2 chunks, but simply iterate over all individual [order-0]
>>> pages).
>>
>> The function assign_pages() will always use 1U << order (and sadly 1 <<
>> order). So we would end up to convert the count in multiple order for
>> then directly converting back to a number. To me, this sounds rather
>> pointless...
>>
>> There are also a slight benefits to call assign_pages() a single time
>> during boot because it will reduce the number of time we need to
>> lock/unlock d->page_alloc_lock.
> 
> Well, all of this is why I did add ...
> 
>>> The more that I'm not happy with the chosen name, despite it
>>> having been suggested during v1 review. _If_ we needed two functions,
>>> imo they ought to be named assign_page() (dealing with a single page of
>>> the given order) and assign_pages(). Backporting confusion could be
>>> helped by altering the order of parameters, such that the compiler
>>> would point out that adjustments at call sites are needed.
> 
> ... this. 

Oh, it wasn't entirely clear whether you were objecting of offering the 
possibility to pass a number of pages rather than an order.

> Not sure whether you not commenting on it means you agree
> with the proposal.

Yes I am happy with your proposal.

> 
>>>> --- a/xen/common/page_alloc.c
>>>> +++ b/xen/common/page_alloc.c
>>>> @@ -2301,14 +2301,14 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
>>>>    }
>>>>    
>>>>    
>>>> -int assign_pages(
>>>> +int assign_pages_nr(
>>>>        struct domain *d,
>>>>        struct page_info *pg,
>>>> -    unsigned int order,
>>>> +    unsigned int nr_pfns,
>>>
>>> Even leaving the naming aspect of "pfns" aside, I can't see why this
>>> can't be simply "nr" (of appropriate type, see next remark).
>>>
>>>>        unsigned int memflags)
>>>>    {
>>>>        int rc = 0;
>>>> -    unsigned long i;
>>>> +    unsigned int i;
>>>
>>> This is not an acceptable type change, at least not as long as it's not
>>> justified at all in the description. While both Arm and x86 will be
>>> fine this way, the code here is supposed to be generic, and hence would
>>> better remain generally correct.
>>
>> I would like to point out the code is already not correct as we are
>> using 1U << order or worse 1 << order :).
> 
> Indeed there are improvements (towards being consistent) to be made. But
> this is not an excuse to make things worse here. At least one of the two
> loops already properly uses 1ul; sadly that's only debugging code. And
> of course something like domain_tot_pages() (and the underlying field)
> dealing with "unsigned int" doesn't help consistency either. As it stands
> we're limiting ourselves to 8Tb VMs, as it seems, and for no good reason.

I actually have a patch in my queue that hardens assign_pages(). I will 
aim to send it later today.

Cheers,

-- 
Julien Grall

Re: [PATCH 5/9] xen: introduce assign_pages_nr

[Jan Beulich]

On 01.07.2021 11:24, Julien Grall wrote:
> On 01/07/2021 09:26, Jan Beulich wrote:
>> On 30.06.2021 20:29, Julien Grall wrote:
>>> On 10/06/2021 10:49, Jan Beulich wrote:
>>>> On 07.06.2021 04:43, Penny Zheng wrote:
>>>>> Introduce new interface assign_pages_nr to deal with when page number is
>>>>> not in a power-of-two, which will save the trouble each time user needs
>>>>> to split the size in a power of 2 to use assign_pages.
>>>>
>>>> First of all I still don't see why in this one special case it is a
>>>> meaningful burden to do the count-to-order conversion in the caller you
>>>> mean to add,
>>>
>>> This sort of works for one caller. However, I would expect some more
>>> user in the future (we use it for Live-Update).
>>>
>>>> and hence why we really need this new function (to keep it
>>>> simple, you could even have the caller not break down to arbitrary
>>>> power-of-2 chunks, but simply iterate over all individual [order-0]
>>>> pages).
>>>
>>> The function assign_pages() will always use 1U << order (and sadly 1 <<
>>> order). So we would end up to convert the count in multiple order for
>>> then directly converting back to a number. To me, this sounds rather
>>> pointless...
>>>
>>> There are also a slight benefits to call assign_pages() a single time
>>> during boot because it will reduce the number of time we need to
>>> lock/unlock d->page_alloc_lock.
>>
>> Well, all of this is why I did add ...
>>
>>>> The more that I'm not happy with the chosen name, despite it
>>>> having been suggested during v1 review. _If_ we needed two functions,
>>>> imo they ought to be named assign_page() (dealing with a single page of
>>>> the given order) and assign_pages(). Backporting confusion could be
>>>> helped by altering the order of parameters, such that the compiler
>>>> would point out that adjustments at call sites are needed.
>>
>> ... this. 
> 
> Oh, it wasn't entirely clear whether you were objecting of offering the 
> possibility to pass a number of pages rather than an order.

Easily understood: I indeed we trying to express my preference to stick
to what we have, but trying to suggest a variant that I think I could
live with.

Jan

[PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Penny Zheng]

alloc_staticmem_pages aims to allocate nr_mfns contiguous pages of
static memory. And it is the equivalent of alloc_heap_pages for static
memory. Here only covers allocating at specified starting address.

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.

alloc_domstatic_pages is the equivalent of alloc_domheap_pages for
static mmeory, and it is to allocate nr_mfns pages of static memory
and assign them to one specific domain.

It uses alloc_staticmen_pages to get nr_mfns pages of static memory,
then on success, it will use assign_pages_nr to assign those pages to
one specific domain.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
changes v2:
- use mfn_valid() to do validation
- change pfn-named to mfn-named
- put CONFIG_STATIC_ALLOCATION around to remove dead codes
- correct off-by-one indentation
- remove meaningless MEMF_no_owner case
- leave zone concept out of DMA limitation check
---
 xen/common/page_alloc.c | 129 ++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/mm.h    |   2 +
 2 files changed, 131 insertions(+)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index e244d2e52e..a0eea5f1a4 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
     return pg;
 }
 
+#ifdef CONFIG_STATIC_ALLOCATION
+/*
+ * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory.
+ * It is the equivalent of alloc_heap_pages for static memory
+ */
+static struct page_info *alloc_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 allocating at specified address. */
+    if ( !mfn_valid(smfn) || !nr_mfns )
+    {
+        printk(XENLOG_ERR
+               "Invalid %lu static memory starting at %"PRI_mfn"\n",
+               nr_mfns, mfn_x(smfn));
+        return NULL;
+    }
+    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).
+         */
+        ASSERT(pg[i].count_info & PGC_reserved);
+        if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free )
+        {
+            printk(XENLOG_ERR
+                   "Reference count must continuously be zero for free pages"
+                   "pg[%lu] 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 = (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);
+
+        /*
+         * Ensure cache and RAM are consistent for platforms where the
+         * guest can control its own visibility of/through the cache.
+         */
+        flush_page_to_ram(mfn_x(page_to_mfn(&pg[i])),
+                            !(memflags & MEMF_no_icache_flush));
+    }
+
+    if ( need_tlbflush )
+        filtered_flush_tlb_mask(tlbflush_timestamp);
+
+    return pg;
+}
+#endif
+
 /* Remove any offlined page in the buddy pointed to by head. */
 static int reserve_offlined_page(struct page_info *head)
 {
@@ -2326,7 +2395,11 @@ int assign_pages_nr(
 
         for ( i = 0; i < nr_pfns; i++ )
         {
+#ifdef CONFIG_STATIC_ALLOCATION
+            ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_reserved)));
+#else
             ASSERT(!(pg[i].count_info & ~PGC_extra));
+#endif
             if ( pg[i].count_info & PGC_extra )
                 extra_pages++;
         }
@@ -2365,7 +2438,12 @@ int assign_pages_nr(
         page_set_owner(&pg[i], d);
         smp_wmb(); /* Domain pointer must be visible before updating refcnt. */
         pg[i].count_info =
+#ifdef CONFIG_STATIC_ALLOCATION
+            (pg[i].count_info & (PGC_extra | PGC_reserved)) | PGC_allocated | 1;
+#else
             (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
+#endif
+
         page_list_add_tail(&pg[i], page_to_list(d, &pg[i]));
     }
 
@@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages(
     return pg;
 }
 
+#ifdef CONFIG_STATIC_ALLOCATION
+/*
+ * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory,
+ * then assign them to one specific domain #d.
+ * It is the equivalent of alloc_domheap_pages for static memory.
+ */
+struct page_info *alloc_domstatic_pages(
+        struct domain *d, unsigned long nr_mfns, mfn_t smfn,
+        unsigned int memflags)
+{
+    struct page_info *pg = NULL;
+    unsigned long dma_size;
+
+    ASSERT(!in_irq());
+
+    if ( !dma_bitsize )
+        memflags &= ~MEMF_no_dma;
+    else
+    {
+        if ( (dma_bitsize - PAGE_SHIFT) > 0 )
+        {
+            dma_size = 1ul << (dma_bitsize - PAGE_SHIFT);
+            /* Starting address shall meet the DMA limitation. */
+            if ( mfn_x(smfn) < dma_size )
+                return NULL;
+        }
+    }
+
+    pg = alloc_staticmem_pages(nr_mfns, smfn, memflags);
+    if ( !pg )
+        return NULL;
+
+    /* Right now, MEMF_no_owner case is meaningless here. */
+    ASSERT(d);
+    if ( memflags & MEMF_no_refcount )
+    {
+        unsigned long i;
+
+        for ( i = 0; i < nr_mfns; i++ )
+            pg[i].count_info |= PGC_extra;
+    }
+    if ( assign_pages_nr(d, pg, nr_mfns, 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 25d970e857..a07bd02923 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -89,6 +89,8 @@ bool scrub_free_pages(void);
 /* Static Allocation */
 void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                           bool need_scrub);
+struct page_info *alloc_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

Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Jan Beulich]

On 07.06.2021 04:43, Penny Zheng wrote:
> alloc_staticmem_pages aims to allocate nr_mfns contiguous pages of
> static memory. And it is the equivalent of alloc_heap_pages for static
> memory. Here only covers allocating at specified starting address.
> 
> 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.
> 
> alloc_domstatic_pages is the equivalent of alloc_domheap_pages for
> static mmeory, and it is to allocate nr_mfns pages of static memory
> and assign them to one specific domain.
> 
> It uses alloc_staticmen_pages to get nr_mfns pages of static memory,
> then on success, it will use assign_pages_nr to assign those pages to
> one specific domain.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> changes v2:
> - use mfn_valid() to do validation
> - change pfn-named to mfn-named
> - put CONFIG_STATIC_ALLOCATION around to remove dead codes
> - correct off-by-one indentation
> - remove meaningless MEMF_no_owner case
> - leave zone concept out of DMA limitation check
> ---
>  xen/common/page_alloc.c | 129 ++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/mm.h    |   2 +
>  2 files changed, 131 insertions(+)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index e244d2e52e..a0eea5f1a4 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
>      return pg;
>  }
>  
> +#ifdef CONFIG_STATIC_ALLOCATION
> +/*
> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory.
> + * It is the equivalent of alloc_heap_pages for static memory
> + */
> +static struct page_info *alloc_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 allocating at specified address. */
> +    if ( !mfn_valid(smfn) || !nr_mfns )
> +    {
> +        printk(XENLOG_ERR
> +               "Invalid %lu static memory starting at %"PRI_mfn"\n",

Reading a log containing e.g. "Invalid 0 static memory starting at
..." I don't think I would recognize that the "0" is the count of
pages.

> +               nr_mfns, mfn_x(smfn));
> +        return NULL;
> +    }
> +    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).
> +         */
> +        ASSERT(pg[i].count_info & PGC_reserved);

What logic elsewhere guarantees that this will hold? ASSERT()s are
to verify that assumptions are met. But I don't think you can
sensibly assume the caller knows the range is reserved (and free),
or else you could get away without any allocation function.

> +        if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free )
> +        {
> +            printk(XENLOG_ERR
> +                   "Reference count must continuously be zero for free pages"
> +                   "pg[%lu] 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();
> +        }

The same applies here at least until proper locking gets added,
which I guess is happening in the next patch when really it would
need to happen right here.

Furthermore I don't see why you don't fold ASSERT() and if into

        if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )

After all PGC_reserved is not similar to PGC_need_scrub, which
alloc_heap_pages() masks out the way you also have it here.

As to the printk() - the extra verbosity compared to the original
isn't helpful or necessary imo. The message needs to be
distinguishable from the other one, yes, so it would better
mention "static" in some way. But the prefix you have is too long
for my taste (and lacks a separating blank anyway).

As a separate matter - have you given up on the concept of
reserving particular memory ranges for _particular_ guests? The
cover letter, saying "Static Allocation refers to system or
sub-system(domains) for which memory areas are pre-defined by
configuration using physical address ranges" as the very first
thing, doesn't seem to suggest so.

> +        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 = (pg[i].count_info & PGC_reserved) | PGC_state_inuse;

Why not

        pg[i].count_info = PGC_state_inuse | PGC_reserved;

? Again, PGC_reserved is sufficiently different from PGC_need_scrub.

> +        /* Initialise fields which have other uses for free pages. */
> +        pg[i].u.inuse.type_info = 0;
> +        page_set_owner(&pg[i], NULL);
> +
> +        /*
> +         * Ensure cache and RAM are consistent for platforms where the
> +         * guest can control its own visibility of/through the cache.
> +         */
> +        flush_page_to_ram(mfn_x(page_to_mfn(&pg[i])),
> +                            !(memflags & MEMF_no_icache_flush));
> +    }
> +
> +    if ( need_tlbflush )
> +        filtered_flush_tlb_mask(tlbflush_timestamp);

Depending on whether static pages have a designated owner, this may
(as suggested before) not be necessary.

> @@ -2326,7 +2395,11 @@ int assign_pages_nr(
>  
>          for ( i = 0; i < nr_pfns; i++ )
>          {
> +#ifdef CONFIG_STATIC_ALLOCATION
> +            ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_reserved)));
> +#else
>              ASSERT(!(pg[i].count_info & ~PGC_extra));
> +#endif
>              if ( pg[i].count_info & PGC_extra )
>                  extra_pages++;
>          }
> @@ -2365,7 +2438,12 @@ int assign_pages_nr(
>          page_set_owner(&pg[i], d);
>          smp_wmb(); /* Domain pointer must be visible before updating refcnt. */
>          pg[i].count_info =
> +#ifdef CONFIG_STATIC_ALLOCATION
> +            (pg[i].count_info & (PGC_extra | PGC_reserved)) | PGC_allocated | 1;
> +#else
>              (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
> +#endif

Both hunks' #ifdef-ary needs to be avoided, e.g. by

#ifndef CONFIG_STATIC_ALLOCATION
# define PGC_reserved 0
#endif

near the top of the file.

> @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages(
>      return pg;
>  }
>  
> +#ifdef CONFIG_STATIC_ALLOCATION
> +/*
> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory,
> + * then assign them to one specific domain #d.
> + * It is the equivalent of alloc_domheap_pages for static memory.
> + */
> +struct page_info *alloc_domstatic_pages(
> +        struct domain *d, unsigned long nr_mfns, mfn_t smfn,
> +        unsigned int memflags)
> +{
> +    struct page_info *pg = NULL;
> +    unsigned long dma_size;
> +
> +    ASSERT(!in_irq());
> +
> +    if ( !dma_bitsize )
> +        memflags &= ~MEMF_no_dma;
> +    else
> +    {
> +        if ( (dma_bitsize - PAGE_SHIFT) > 0 )
> +        {
> +            dma_size = 1ul << (dma_bitsize - PAGE_SHIFT);
> +            /* Starting address shall meet the DMA limitation. */
> +            if ( mfn_x(smfn) < dma_size )
> +                return NULL;

I think I did ask this on v1 already: Why the first page? Static
memory regions, unlike buddy allocator zones, can cross power-of-2
address boundaries. Hence it ought to be the last page that gets
checked for fitting address width restriction requirements.

And then - is this necessary at all? Shouldn't "pre-defined by
configuration using physical address ranges" imply the memory
designated for a guest fits its DMA needs?

Jan

RE: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Penny Zheng]

Hi

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, June 10, 2021 6:23 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; julien@xen.org
> Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and
> alloc_domstatic_pages
> 
> On 07.06.2021 04:43, Penny Zheng wrote:
> > alloc_staticmem_pages aims to allocate nr_mfns contiguous pages of
> > static memory. And it is the equivalent of alloc_heap_pages for static
> > memory. Here only covers allocating at specified starting address.
> >
> > 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.
> >
> > alloc_domstatic_pages is the equivalent of alloc_domheap_pages for
> > static mmeory, and it is to allocate nr_mfns pages of static memory
> > and assign them to one specific domain.
> >
> > It uses alloc_staticmen_pages to get nr_mfns pages of static memory,
> > then on success, it will use assign_pages_nr to assign those pages to
> > one specific domain.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > changes v2:
> > - use mfn_valid() to do validation
> > - change pfn-named to mfn-named
> > - put CONFIG_STATIC_ALLOCATION around to remove dead codes
> > - correct off-by-one indentation
> > - remove meaningless MEMF_no_owner case
> > - leave zone concept out of DMA limitation check
> > ---
> >  xen/common/page_alloc.c | 129
> ++++++++++++++++++++++++++++++++++++++++
> >  xen/include/xen/mm.h    |   2 +
> >  2 files changed, 131 insertions(+)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > e244d2e52e..a0eea5f1a4 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
> >      return pg;
> >  }
> >
> > +#ifdef CONFIG_STATIC_ALLOCATION
> > +/*
> > + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory.
> > + * It is the equivalent of alloc_heap_pages for static memory  */
> > +static struct page_info *alloc_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 allocating at specified address. */
> > +    if ( !mfn_valid(smfn) || !nr_mfns )
> > +    {
> > +        printk(XENLOG_ERR
> > +               "Invalid %lu static memory starting at %"PRI_mfn"\n",
> 
> Reading a log containing e.g. "Invalid 0 static memory starting at ..." I don't
> think I would recognize that the "0" is the count of pages.
> 

Sure. How about "try to allocate out of range page %"PRI_mfn"\n"?

> > +               nr_mfns, mfn_x(smfn));
> > +        return NULL;
> > +    }
> > +    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).
> > +         */
> > +        ASSERT(pg[i].count_info & PGC_reserved);
> 
> What logic elsewhere guarantees that this will hold? ASSERT()s are to verify
> that assumptions are met. But I don't think you can sensibly assume the caller
> knows the range is reserved (and free), or else you could get away without any
> allocation function.
> 

The caller shall only call alloc_staticmem_pages when it knows range is reserved,
like, alloc_staticmem_pages is only called in the context of alloc_domstatic_pages
for now.

Normal domain uses alloc_heap_pages/alloc_domheap_pages to do the allocation.

Proper initialization must happen before allocation. Init_staticmem_pages shall be
called before alloc_staticmem_pages. And we set PGC_reserved in init_staticmem_pages.

So here I use ASSERT()s to check whether above proper initialization is met.

> > +        if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "Reference count must continuously be zero for free pages"
> > +                   "pg[%lu] 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();
> > +        }
> 
> The same applies here at least until proper locking gets added, which I guess is
> happening in the next patch when really it would need to happen right here.
>

Ok, I will combine two commits together, and add locking here. 
I thought the content of this commit is a little bit too much, so maybe
adding the proper lock shall be created as a new patch. 😉
 
> Furthermore I don't see why you don't fold ASSERT() and if into
> 
>         if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
> 
> After all PGC_reserved is not similar to PGC_need_scrub, which
> alloc_heap_pages() masks out the way you also have it here.
> 

I understand that you prefer if condition is phrased as follows:
 	if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
Agree that PGC_reserved shall has the same position as PGC_state_free.
Hmmm, for why I don't fold ASSERT(), do you mean that 
ASSERT(pg[i].count_info == (PGC_state_free | PGC_reserved))?

> As to the printk() - the extra verbosity compared to the original isn't helpful or
> necessary imo. The message needs to be distinguishable from the other one,
> yes, so it would better mention "static" in some way. But the prefix you have is
> too long for my taste (and lacks a separating blank anyway).
> 

If you don't like the extra verbosity, maybe just
" Static pg[%lu] MFN %"PRI_mfn" c=%#lx t=%#x.\n"?

> As a separate matter - have you given up on the concept of reserving particular
> memory ranges for _particular_ guests? The cover letter, saying "Static
> Allocation refers to system or
> sub-system(domains) for which memory areas are pre-defined by
> configuration using physical address ranges" as the very first thing, doesn't
> seem to suggest so.
> 

Yeah, I may take suggestion from Julien to retrieve reserved mem info from device tree
on rebooting. So it may not need store domain info in struct page_info.

And also for rebooting domain on static allocation, it will not be introduced in this patch serie.
This patch serie may only focus on initialization and allocation part.

> > +        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 = (pg[i].count_info & PGC_reserved) |
> > + PGC_state_inuse;
> 
> Why not
> 
>         pg[i].count_info = PGC_state_inuse | PGC_reserved;
> 
> ? Again, PGC_reserved is sufficiently different from PGC_need_scrub.
> 

Sure. Thanks. You're right.

> > +        /* Initialise fields which have other uses for free pages. */
> > +        pg[i].u.inuse.type_info = 0;
> > +        page_set_owner(&pg[i], NULL);
> > +
> > +        /*
> > +         * Ensure cache and RAM are consistent for platforms where the
> > +         * guest can control its own visibility of/through the cache.
> > +         */
> > +        flush_page_to_ram(mfn_x(page_to_mfn(&pg[i])),
> > +                            !(memflags & MEMF_no_icache_flush));
> > +    }
> > +
> > +    if ( need_tlbflush )
> > +        filtered_flush_tlb_mask(tlbflush_timestamp);
> 
> Depending on whether static pages have a designated owner, this may (as
> suggested before) not be necessary.
>

This has also been discussed in patch v1~ Julien has replied on it, here may just refer
what he said:

"
I would rather not make the assumption. I can see future where we just 
want to allocate memory from a static pool that may be shared with 
multiple domains.
"
 
> > @@ -2326,7 +2395,11 @@ int assign_pages_nr(
> >
> >          for ( i = 0; i < nr_pfns; i++ )
> >          {
> > +#ifdef CONFIG_STATIC_ALLOCATION
> > +            ASSERT(!(pg[i].count_info & ~(PGC_extra |
> > +PGC_reserved))); #else
> >              ASSERT(!(pg[i].count_info & ~PGC_extra));
> > +#endif
> >              if ( pg[i].count_info & PGC_extra )
> >                  extra_pages++;
> >          }
> > @@ -2365,7 +2438,12 @@ int assign_pages_nr(
> >          page_set_owner(&pg[i], d);
> >          smp_wmb(); /* Domain pointer must be visible before updating refcnt.
> */
> >          pg[i].count_info =
> > +#ifdef CONFIG_STATIC_ALLOCATION
> > +            (pg[i].count_info & (PGC_extra | PGC_reserved)) |
> > +PGC_allocated | 1; #else
> >              (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
> > +#endif
> 
> Both hunks' #ifdef-ary needs to be avoided, e.g. by
> 
> #ifndef CONFIG_STATIC_ALLOCATION
> # define PGC_reserved 0
> #endif
> 
> near the top of the file.
> 

Ok, it may also help remove the #ifdefs in make_page_free, thx!

> > @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages(
> >      return pg;
> >  }
> >
> > +#ifdef CONFIG_STATIC_ALLOCATION
> > +/*
> > + * Allocate nr_mfns contiguous pages, starting at #smfn, of static
> > +memory,
> > + * then assign them to one specific domain #d.
> > + * It is the equivalent of alloc_domheap_pages for static memory.
> > + */
> > +struct page_info *alloc_domstatic_pages(
> > +        struct domain *d, unsigned long nr_mfns, mfn_t smfn,
> > +        unsigned int memflags)
> > +{
> > +    struct page_info *pg = NULL;
> > +    unsigned long dma_size;
> > +
> > +    ASSERT(!in_irq());
> > +
> > +    if ( !dma_bitsize )
> > +        memflags &= ~MEMF_no_dma;
> > +    else
> > +    {
> > +        if ( (dma_bitsize - PAGE_SHIFT) > 0 )
> > +        {
> > +            dma_size = 1ul << (dma_bitsize - PAGE_SHIFT);
> > +            /* Starting address shall meet the DMA limitation. */
> > +            if ( mfn_x(smfn) < dma_size )
> > +                return NULL;
> 
> I think I did ask this on v1 already: Why the first page? Static memory regions,
> unlike buddy allocator zones, can cross power-of-2 address boundaries. Hence
> it ought to be the last page that gets checked for fitting address width
> restriction requirements.
> 
> And then - is this necessary at all? Shouldn't "pre-defined by configuration
> using physical address ranges" imply the memory designated for a guest fits its
> DMA needs?
> 

Hmmm, In my understanding, here is the DMA restriction when using buddy allocator:
    else if ( (dma_zone = bits_to_zone(dma_bitsize)) < zone_hi )
        pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);
dma_zone is restricting the starting buddy allocator zone, so I am thinking that here, it shall
restrict the first page.

imo, if let user define, it also could be missing DMA restriction? If you all think its not
necessary, I'll remove it in v3~~~

> Jan

Cheers
Penny Zheng

Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Jan Beulich]

On 06.07.2021 07:58, Penny Zheng wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, June 10, 2021 6:23 PM
>>
>> On 07.06.2021 04:43, Penny Zheng wrote:
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
>>>      return pg;
>>>  }
>>>
>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>> +/*
>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory.
>>> + * It is the equivalent of alloc_heap_pages for static memory  */
>>> +static struct page_info *alloc_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 allocating at specified address. */
>>> +    if ( !mfn_valid(smfn) || !nr_mfns )
>>> +    {
>>> +        printk(XENLOG_ERR
>>> +               "Invalid %lu static memory starting at %"PRI_mfn"\n",
>>
>> Reading a log containing e.g. "Invalid 0 static memory starting at ..." I don't
>> think I would recognize that the "0" is the count of pages.
> 
> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"?

This still doesn't convey _both_ parts of the if() that would cause
the log message to be issued.

>>> +               nr_mfns, mfn_x(smfn));
>>> +        return NULL;
>>> +    }
>>> +    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).
>>> +         */
>>> +        ASSERT(pg[i].count_info & PGC_reserved);
>>
>> What logic elsewhere guarantees that this will hold? ASSERT()s are to verify
>> that assumptions are met. But I don't think you can sensibly assume the caller
>> knows the range is reserved (and free), or else you could get away without any
>> allocation function.
> 
> The caller shall only call alloc_staticmem_pages when it knows range is reserved,
> like, alloc_staticmem_pages is only called in the context of alloc_domstatic_pages
> for now.

If the caller knows the static ranges, this isn't really "allocation".
I.e. I then question the need for "allocating" in the first place.

>>> +        if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free )
>>> +        {
>>> +            printk(XENLOG_ERR
>>> +                   "Reference count must continuously be zero for free pages"
>>> +                   "pg[%lu] 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();
>>> +        }
>>
>> The same applies here at least until proper locking gets added, which I guess is
>> happening in the next patch when really it would need to happen right here.
>>
> 
> Ok, I will combine two commits together, and add locking here. 
> I thought the content of this commit is a little bit too much, so maybe
> adding the proper lock shall be created as a new patch. 😉
>  
>> Furthermore I don't see why you don't fold ASSERT() and if into
>>
>>         if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
>>
>> After all PGC_reserved is not similar to PGC_need_scrub, which
>> alloc_heap_pages() masks out the way you also have it here.
>>
> 
> I understand that you prefer if condition is phrased as follows:
>  	if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
> Agree that PGC_reserved shall has the same position as PGC_state_free.
> Hmmm, for why I don't fold ASSERT(), do you mean that 
> ASSERT(pg[i].count_info == (PGC_state_free | PGC_reserved))?

No. By converting to the suggested construct the ASSERT() disappears
by way of folding _into_ the if().

>> As to the printk() - the extra verbosity compared to the original isn't helpful or
>> necessary imo. The message needs to be distinguishable from the other one,
>> yes, so it would better mention "static" in some way. But the prefix you have is
>> too long for my taste (and lacks a separating blank anyway).
>>
> 
> If you don't like the extra verbosity, maybe just
> " Static pg[%lu] MFN %"PRI_mfn" c=%#lx t=%#x.\n"?

Something along these lines, yes, but I wonder how difficult it is
to take the original message and insert "static" at a suitable place.
Any part you omit would again want justifying. Personally I'd go with
"pg[%u] static MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n" unless any
of the parts are provably pointless to log for static pages.

>>> @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages(
>>>      return pg;
>>>  }
>>>
>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>> +/*
>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static
>>> +memory,
>>> + * then assign them to one specific domain #d.
>>> + * It is the equivalent of alloc_domheap_pages for static memory.
>>> + */
>>> +struct page_info *alloc_domstatic_pages(
>>> +        struct domain *d, unsigned long nr_mfns, mfn_t smfn,
>>> +        unsigned int memflags)
>>> +{
>>> +    struct page_info *pg = NULL;
>>> +    unsigned long dma_size;
>>> +
>>> +    ASSERT(!in_irq());
>>> +
>>> +    if ( !dma_bitsize )
>>> +        memflags &= ~MEMF_no_dma;
>>> +    else
>>> +    {
>>> +        if ( (dma_bitsize - PAGE_SHIFT) > 0 )
>>> +        {
>>> +            dma_size = 1ul << (dma_bitsize - PAGE_SHIFT);
>>> +            /* Starting address shall meet the DMA limitation. */
>>> +            if ( mfn_x(smfn) < dma_size )
>>> +                return NULL;
>>
>> I think I did ask this on v1 already: Why the first page? Static memory regions,
>> unlike buddy allocator zones, can cross power-of-2 address boundaries. Hence
>> it ought to be the last page that gets checked for fitting address width
>> restriction requirements.
>>
>> And then - is this necessary at all? Shouldn't "pre-defined by configuration
>> using physical address ranges" imply the memory designated for a guest fits its
>> DMA needs?
>>
> 
> Hmmm, In my understanding, here is the DMA restriction when using buddy allocator:
>     else if ( (dma_zone = bits_to_zone(dma_bitsize)) < zone_hi )
>         pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);
> dma_zone is restricting the starting buddy allocator zone, so I am thinking that here, it shall
> restrict the first page.
> 
> imo, if let user define, it also could be missing DMA restriction?

Did you read my earlier reply? Again: The difference is that ordinary
allocations (buddies) can't cross zone boundaries. Hence it is
irrelevant if you check DMA properties on the first or last page - both
will have the same number of significant bits. The same is - afaict -
not true for static allocation ranges.

Of course, as expressed before, a question is whether DMA suitability
needs checking in the first place for static allocations: I'd view it
as mis-configuration if a domain was provided memory it can't really
use properly.

Jan

Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Julien Grall]

Hi Jan & Penny,

On 06/07/2021 07:53, Jan Beulich wrote:
> On 06.07.2021 07:58, Penny Zheng wrote:
>>> From: Jan Beulich <jbeulich@suse.com>
>>> Sent: Thursday, June 10, 2021 6:23 PM
>>>
>>> On 07.06.2021 04:43, Penny Zheng wrote:
>>>> --- a/xen/common/page_alloc.c
>>>> +++ b/xen/common/page_alloc.c
>>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
>>>>       return pg;
>>>>   }
>>>>
>>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>>> +/*
>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory.
>>>> + * It is the equivalent of alloc_heap_pages for static memory  */
>>>> +static struct page_info *alloc_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 allocating at specified address. */
>>>> +    if ( !mfn_valid(smfn) || !nr_mfns )
>>>> +    {
>>>> +        printk(XENLOG_ERR
>>>> +               "Invalid %lu static memory starting at %"PRI_mfn"\n",
>>>
>>> Reading a log containing e.g. "Invalid 0 static memory starting at ..." I don't
>>> think I would recognize that the "0" is the count of pages.
>>
>> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"?
> 
> This still doesn't convey _both_ parts of the if() that would cause
> the log message to be issued.
> 
>>>> +               nr_mfns, mfn_x(smfn));
>>>> +        return NULL;
>>>> +    }
>>>> +    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).
>>>> +         */
>>>> +        ASSERT(pg[i].count_info & PGC_reserved);
>>>
>>> What logic elsewhere guarantees that this will hold? ASSERT()s are to verify
>>> that assumptions are met. But I don't think you can sensibly assume the caller
>>> knows the range is reserved (and free), or else you could get away without any
>>> allocation function.
>>
>> The caller shall only call alloc_staticmem_pages when it knows range is reserved,
>> like, alloc_staticmem_pages is only called in the context of alloc_domstatic_pages
>> for now.
> 
> If the caller knows the static ranges, this isn't really "allocation".
> I.e. I then question the need for "allocating" in the first place.

We still need to setup the page so the reference counting works 
properly. So can you clarify whether you are objecting on the name? If 
yes, do you have a better suggestion?

Cheers,

-- 
Julien Grall

Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Jan Beulich]

On 06.07.2021 11:39, Julien Grall wrote:
> Hi Jan & Penny,
> 
> On 06/07/2021 07:53, Jan Beulich wrote:
>> On 06.07.2021 07:58, Penny Zheng wrote:
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Thursday, June 10, 2021 6:23 PM
>>>>
>>>> On 07.06.2021 04:43, Penny Zheng wrote:
>>>>> --- a/xen/common/page_alloc.c
>>>>> +++ b/xen/common/page_alloc.c
>>>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
>>>>>       return pg;
>>>>>   }
>>>>>
>>>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>>>> +/*
>>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory.
>>>>> + * It is the equivalent of alloc_heap_pages for static memory  */
>>>>> +static struct page_info *alloc_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 allocating at specified address. */
>>>>> +    if ( !mfn_valid(smfn) || !nr_mfns )
>>>>> +    {
>>>>> +        printk(XENLOG_ERR
>>>>> +               "Invalid %lu static memory starting at %"PRI_mfn"\n",
>>>>
>>>> Reading a log containing e.g. "Invalid 0 static memory starting at ..." I don't
>>>> think I would recognize that the "0" is the count of pages.
>>>
>>> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"?
>>
>> This still doesn't convey _both_ parts of the if() that would cause
>> the log message to be issued.
>>
>>>>> +               nr_mfns, mfn_x(smfn));
>>>>> +        return NULL;
>>>>> +    }
>>>>> +    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).
>>>>> +         */
>>>>> +        ASSERT(pg[i].count_info & PGC_reserved);
>>>>
>>>> What logic elsewhere guarantees that this will hold? ASSERT()s are to verify
>>>> that assumptions are met. But I don't think you can sensibly assume the caller
>>>> knows the range is reserved (and free), or else you could get away without any
>>>> allocation function.
>>>
>>> The caller shall only call alloc_staticmem_pages when it knows range is reserved,
>>> like, alloc_staticmem_pages is only called in the context of alloc_domstatic_pages
>>> for now.
>>
>> If the caller knows the static ranges, this isn't really "allocation".
>> I.e. I then question the need for "allocating" in the first place.
> 
> We still need to setup the page so the reference counting works 
> properly. So can you clarify whether you are objecting on the name? If 
> yes, do you have a better suggestion?

It's not the name alone but the disconnect between name and actual
behavior. Note that here we've started from an ASSERT(), which is
reasonable to have if the caller knows where static pages are, but
which should be converted to an if() when talking about allocation
(i.e. the caller may not have enough knowledge). If it's not really
allocation, how about naming it "acquire" or "obtain" (and the
config option perhaps STATIC_MEMORY)?

Jan

Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Julien Grall]

Hi,

On 06/07/2021 10:59, Jan Beulich wrote:
> On 06.07.2021 11:39, Julien Grall wrote:
>> Hi Jan & Penny,
>>
>> On 06/07/2021 07:53, Jan Beulich wrote:
>>> On 06.07.2021 07:58, Penny Zheng wrote:
>>>>> From: Jan Beulich <jbeulich@suse.com>
>>>>> Sent: Thursday, June 10, 2021 6:23 PM
>>>>>
>>>>> On 07.06.2021 04:43, Penny Zheng wrote:
>>>>>> --- a/xen/common/page_alloc.c
>>>>>> +++ b/xen/common/page_alloc.c
>>>>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
>>>>>>        return pg;
>>>>>>    }
>>>>>>
>>>>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>>>>> +/*
>>>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory.
>>>>>> + * It is the equivalent of alloc_heap_pages for static memory  */
>>>>>> +static struct page_info *alloc_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 allocating at specified address. */
>>>>>> +    if ( !mfn_valid(smfn) || !nr_mfns )
>>>>>> +    {
>>>>>> +        printk(XENLOG_ERR
>>>>>> +               "Invalid %lu static memory starting at %"PRI_mfn"\n",
>>>>>
>>>>> Reading a log containing e.g. "Invalid 0 static memory starting at ..." I don't
>>>>> think I would recognize that the "0" is the count of pages.
>>>>
>>>> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"?
>>>
>>> This still doesn't convey _both_ parts of the if() that would cause
>>> the log message to be issued.
>>>
>>>>>> +               nr_mfns, mfn_x(smfn));
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>> +    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).
>>>>>> +         */
>>>>>> +        ASSERT(pg[i].count_info & PGC_reserved);
>>>>>
>>>>> What logic elsewhere guarantees that this will hold? ASSERT()s are to verify
>>>>> that assumptions are met. But I don't think you can sensibly assume the caller
>>>>> knows the range is reserved (and free), or else you could get away without any
>>>>> allocation function.
>>>>
>>>> The caller shall only call alloc_staticmem_pages when it knows range is reserved,
>>>> like, alloc_staticmem_pages is only called in the context of alloc_domstatic_pages
>>>> for now.
>>>
>>> If the caller knows the static ranges, this isn't really "allocation".
>>> I.e. I then question the need for "allocating" in the first place.
>>
>> We still need to setup the page so the reference counting works
>> properly. So can you clarify whether you are objecting on the name? If
>> yes, do you have a better suggestion?
> 
> It's not the name alone but the disconnect between name and actual
> behavior. Note that here we've started from an ASSERT(), which is
> reasonable to have if the caller knows where static pages are, but
> which should be converted to an if() when talking about allocation
> (i.e. the caller may not have enough knowledge). If it's not really
> allocation, how about naming it "acquire" or "obtain" (and the
> config option perhaps STATIC_MEMORY)?

The caller will fetch the information from the Device Tree but it 
doesn't sanity check it. I think it can be easy to make mistake as the 
information will be scattered in various node of the DT.

So I think it would be better if we have a check in both prod and debug 
build to ease the integration.

Regarding the name itself, anyone of the one you suggested are fine with 
me because to me the difference between them is too subtle to be 
understood by most of the users.

Cheers,

-- 
Julien Grall

RE: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Penny Zheng]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, July 6, 2021 2:54 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; Julien Grall <julien@xen.org>
> Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and
> alloc_domstatic_pages
> 
> On 06.07.2021 07:58, Penny Zheng wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Thursday, June 10, 2021 6:23 PM
> >>
> >> On 07.06.2021 04:43, Penny Zheng wrote:
> >>> --- a/xen/common/page_alloc.c
> >>> +++ b/xen/common/page_alloc.c
> >>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
> >>>      return pg;
> >>>  }
> >>>
> >>> +#ifdef CONFIG_STATIC_ALLOCATION
> >>> +/*
> >>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory.
> >>> + * It is the equivalent of alloc_heap_pages for static memory  */
> >>> +static struct page_info *alloc_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 allocating at specified address. */
> >>> +    if ( !mfn_valid(smfn) || !nr_mfns )
> >>> +    {
> >>> +        printk(XENLOG_ERR
> >>> +               "Invalid %lu static memory starting at
> >>> + %"PRI_mfn"\n",
> >>
> >> Reading a log containing e.g. "Invalid 0 static memory starting at
> >> ..." I don't think I would recognize that the "0" is the count of pages.
> >
> > Sure. How about "try to allocate out of range page %"PRI_mfn"\n"?
> 
> This still doesn't convey _both_ parts of the if() that would cause the log
> message to be issued.
> 

Sorry. How about
"
        printk(XENLOG_ERR
               "Either out-of-range static memory starting at %"PRI_mfn""
               "or invalid number of pages: %ul.\n",
               mfn_x(smfn), nr_mfns);
"

> >>> +               nr_mfns, mfn_x(smfn));
> >>> +        return NULL;
> >>> +    }
> >>> +    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).
> >>> +         */
> >>> +        ASSERT(pg[i].count_info & PGC_reserved);
> >>
> >> What logic elsewhere guarantees that this will hold? ASSERT()s are to
> >> verify that assumptions are met. But I don't think you can sensibly
> >> assume the caller knows the range is reserved (and free), or else you
> >> could get away without any allocation function.
> >
> > The caller shall only call alloc_staticmem_pages when it knows range
> > is reserved, like, alloc_staticmem_pages is only called in the context
> > of alloc_domstatic_pages for now.
> 
> If the caller knows the static ranges, this isn't really "allocation".
> I.e. I then question the need for "allocating" in the first place.
>
> >>> +        if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free )
> >>> +        {
> >>> +            printk(XENLOG_ERR
> >>> +                   "Reference count must continuously be zero for free pages"
> >>> +                   "pg[%lu] 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();
> >>> +        }
> >>
> >> The same applies here at least until proper locking gets added, which
> >> I guess is happening in the next patch when really it would need to happen
> right here.
> >>
> >
> > Ok, I will combine two commits together, and add locking here.
> > I thought the content of this commit is a little bit too much, so
> > maybe adding the proper lock shall be created as a new patch. 😉
> >
> >> Furthermore I don't see why you don't fold ASSERT() and if into
> >>
> >>         if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
> >>
> >> After all PGC_reserved is not similar to PGC_need_scrub, which
> >> alloc_heap_pages() masks out the way you also have it here.
> >>
> >
> > I understand that you prefer if condition is phrased as follows:
> >  	if ( pg[i].count_info != (PGC_state_free | PGC_reserved) ) Agree
> > that PGC_reserved shall has the same position as PGC_state_free.
> > Hmmm, for why I don't fold ASSERT(), do you mean that
> > ASSERT(pg[i].count_info == (PGC_state_free | PGC_reserved))?
> 
> No. By converting to the suggested construct the ASSERT() disappears by way
> of folding _into_ the if().
> 
> >> As to the printk() - the extra verbosity compared to the original
> >> isn't helpful or necessary imo. The message needs to be
> >> distinguishable from the other one, yes, so it would better mention
> >> "static" in some way. But the prefix you have is too long for my taste (and
> lacks a separating blank anyway).
> >>
> >
> > If you don't like the extra verbosity, maybe just " Static pg[%lu] MFN
> > %"PRI_mfn" c=%#lx t=%#x.\n"?
> 
> Something along these lines, yes, but I wonder how difficult it is to take the
> original message and insert "static" at a suitable place.
> Any part you omit would again want justifying. Personally I'd go with "pg[%u]
> static MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n" unless any of the parts
> are provably pointless to log for static pages.
> 
> >>> @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages(
> >>>      return pg;
> >>>  }
> >>>
> >>> +#ifdef CONFIG_STATIC_ALLOCATION
> >>> +/*
> >>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static
> >>> +memory,
> >>> + * then assign them to one specific domain #d.
> >>> + * It is the equivalent of alloc_domheap_pages for static memory.
> >>> + */
> >>> +struct page_info *alloc_domstatic_pages(
> >>> +        struct domain *d, unsigned long nr_mfns, mfn_t smfn,
> >>> +        unsigned int memflags)
> >>> +{
> >>> +    struct page_info *pg = NULL;
> >>> +    unsigned long dma_size;
> >>> +
> >>> +    ASSERT(!in_irq());
> >>> +
> >>> +    if ( !dma_bitsize )
> >>> +        memflags &= ~MEMF_no_dma;
> >>> +    else
> >>> +    {
> >>> +        if ( (dma_bitsize - PAGE_SHIFT) > 0 )
> >>> +        {
> >>> +            dma_size = 1ul << (dma_bitsize - PAGE_SHIFT);
> >>> +            /* Starting address shall meet the DMA limitation. */
> >>> +            if ( mfn_x(smfn) < dma_size )
> >>> +                return NULL;
> >>
> >> I think I did ask this on v1 already: Why the first page? Static
> >> memory regions, unlike buddy allocator zones, can cross power-of-2
> >> address boundaries. Hence it ought to be the last page that gets
> >> checked for fitting address width restriction requirements.
> >>
> >> And then - is this necessary at all? Shouldn't "pre-defined by
> >> configuration using physical address ranges" imply the memory
> >> designated for a guest fits its DMA needs?
> >>
> >
> > Hmmm, In my understanding, here is the DMA restriction when using buddy
> allocator:
> >     else if ( (dma_zone = bits_to_zone(dma_bitsize)) < zone_hi )
> >         pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags,
> > d); dma_zone is restricting the starting buddy allocator zone, so I am
> > thinking that here, it shall restrict the first page.
> >
> > imo, if let user define, it also could be missing DMA restriction?
> 
> Did you read my earlier reply? Again: The difference is that ordinary
> allocations (buddies) can't cross zone boundaries. Hence it is irrelevant if you
> check DMA properties on the first or last page - both will have the same
> number of significant bits. The same is - afaict - not true for static allocation
> ranges.
> 

True.

Ordinary allocations (buddies) can't cross zone boundaries, So I understand that
following the logic in "alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);"
pages of the smallest address shall be allocated from "dma_zone + 1", like you
said, it is irrelevant if you check DMA properties on the first or last pages, but imo, no matter
first or last page, both shall be larger than (2^(dma_zone + 1)).

Taking 32 as dma_bitsize, then the memory with this DMA restriction allocated by
"alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);" shall be at least
more than 4G.

That’s why I keep comparing the first page of static allocation, that I am following the
"more than" logic here.

But you're right, I got a little investigation on ARM DMA limitation, still taking dma_bitsize=32
as an example, we want that the actually allocated memory is smaller than 4G, not more than.
So I think the logic behind this code line
" alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);" is not right for ARM, and it shall
be changed to "alloc_heap_pages(zone_lo, dma_zone + 1, order, memflags, d);" as correction.

And Later wei will send a new issue on DMA limitation on ARM to community for discussion.

For here, I'll take your suggestion for removing DMA limitation on Static Allocation.

> Of course, as expressed before, a question is whether DMA suitability needs
> checking in the first place for static allocations: I'd view it as mis-configuration
> if a domain was provided memory it can't really use properly.
> 
> Jan

Cheers

Penny Zheng

Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Jan Beulich]

On 08.07.2021 11:09, Penny Zheng wrote:
> Hi Jan
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, July 6, 2021 2:54 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org; Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and
>> alloc_domstatic_pages
>>
>> On 06.07.2021 07:58, Penny Zheng wrote:
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Thursday, June 10, 2021 6:23 PM
>>>>
>>>> On 07.06.2021 04:43, Penny Zheng wrote:
>>>>> --- a/xen/common/page_alloc.c
>>>>> +++ b/xen/common/page_alloc.c
>>>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
>>>>>      return pg;
>>>>>  }
>>>>>
>>>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>>>> +/*
>>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory.
>>>>> + * It is the equivalent of alloc_heap_pages for static memory  */
>>>>> +static struct page_info *alloc_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 allocating at specified address. */
>>>>> +    if ( !mfn_valid(smfn) || !nr_mfns )
>>>>> +    {
>>>>> +        printk(XENLOG_ERR
>>>>> +               "Invalid %lu static memory starting at
>>>>> + %"PRI_mfn"\n",
>>>>
>>>> Reading a log containing e.g. "Invalid 0 static memory starting at
>>>> ..." I don't think I would recognize that the "0" is the count of pages.
>>>
>>> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"?
>>
>> This still doesn't convey _both_ parts of the if() that would cause the log
>> message to be issued.
>>
> 
> Sorry. How about
> "
>         printk(XENLOG_ERR
>                "Either out-of-range static memory starting at %"PRI_mfn""
>                "or invalid number of pages: %ul.\n",
>                mfn_x(smfn), nr_mfns);
> "

I'm sorry - while now you convey both aspects, the message has become
too verbose. What's wrong with "Invalid static memory request: ... pages
at ...\"? But I wonder anyway if a log message is appropriate here in
the first place.

>>>>> @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages(
>>>>>      return pg;
>>>>>  }
>>>>>
>>>>> +#ifdef CONFIG_STATIC_ALLOCATION
>>>>> +/*
>>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static
>>>>> +memory,
>>>>> + * then assign them to one specific domain #d.
>>>>> + * It is the equivalent of alloc_domheap_pages for static memory.
>>>>> + */
>>>>> +struct page_info *alloc_domstatic_pages(
>>>>> +        struct domain *d, unsigned long nr_mfns, mfn_t smfn,
>>>>> +        unsigned int memflags)
>>>>> +{
>>>>> +    struct page_info *pg = NULL;
>>>>> +    unsigned long dma_size;
>>>>> +
>>>>> +    ASSERT(!in_irq());
>>>>> +
>>>>> +    if ( !dma_bitsize )
>>>>> +        memflags &= ~MEMF_no_dma;
>>>>> +    else
>>>>> +    {
>>>>> +        if ( (dma_bitsize - PAGE_SHIFT) > 0 )
>>>>> +        {
>>>>> +            dma_size = 1ul << (dma_bitsize - PAGE_SHIFT);
>>>>> +            /* Starting address shall meet the DMA limitation. */
>>>>> +            if ( mfn_x(smfn) < dma_size )
>>>>> +                return NULL;
>>>>
>>>> I think I did ask this on v1 already: Why the first page? Static
>>>> memory regions, unlike buddy allocator zones, can cross power-of-2
>>>> address boundaries. Hence it ought to be the last page that gets
>>>> checked for fitting address width restriction requirements.
>>>>
>>>> And then - is this necessary at all? Shouldn't "pre-defined by
>>>> configuration using physical address ranges" imply the memory
>>>> designated for a guest fits its DMA needs?
>>>>
>>>
>>> Hmmm, In my understanding, here is the DMA restriction when using buddy
>> allocator:
>>>     else if ( (dma_zone = bits_to_zone(dma_bitsize)) < zone_hi )
>>>         pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags,
>>> d); dma_zone is restricting the starting buddy allocator zone, so I am
>>> thinking that here, it shall restrict the first page.
>>>
>>> imo, if let user define, it also could be missing DMA restriction?
>>
>> Did you read my earlier reply? Again: The difference is that ordinary
>> allocations (buddies) can't cross zone boundaries. Hence it is irrelevant if you
>> check DMA properties on the first or last page - both will have the same
>> number of significant bits. The same is - afaict - not true for static allocation
>> ranges.
>>
> 
> True.
> 
> Ordinary allocations (buddies) can't cross zone boundaries, So I understand that
> following the logic in "alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);"
> pages of the smallest address shall be allocated from "dma_zone + 1", like you
> said, it is irrelevant if you check DMA properties on the first or last pages, but imo, no matter
> first or last page, both shall be larger than (2^(dma_zone + 1)).
> 
> Taking 32 as dma_bitsize, then the memory with this DMA restriction allocated by
> "alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);" shall be at least
> more than 4G.

DMA restrictions are always "needs to be no more than N bits".

> That’s why I keep comparing the first page of static allocation, that I am following the
> "more than" logic here.
> 
> But you're right, I got a little investigation on ARM DMA limitation, still taking dma_bitsize=32
> as an example, we want that the actually allocated memory is smaller than 4G, not more than.
> So I think the logic behind this code line
> " alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);" is not right for ARM, and it shall
> be changed to "alloc_heap_pages(zone_lo, dma_zone + 1, order, memflags, d);" as correction.

But this step is to _avoid_ the DMA-reserved part of the heap.
The caller requests address restricted memory by passing suitable
memflags. If the request doesn't require access to the DMA-
reserved part of the heap (dma_zone < zone_hi) we first try to
get memory from there. Only if that fails will we fall back and
try taking memory from the lower region. IOW the problem with
your code is more fundamental: You use dma_bitsize when really
you ought to extract the caller's requested restriction (if any)
from memflags. I would assume that dma_bitsize is orthogonal to
static memory, i.e. you don't need to try to preserve low memory.

Jan

RE: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages

[Penny Zheng]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, July 8, 2021 6:06 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; xen-
> devel@lists.xenproject.org; sstabellini@kernel.org; Julien Grall
> <julien@xen.org>; Wei Chen <Wei.Chen@arm.com>
> Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and
> alloc_domstatic_pages
> 
> On 08.07.2021 11:09, Penny Zheng wrote:
> > Hi Jan
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Tuesday, July 6, 2021 2:54 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>
> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> >> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> >> sstabellini@kernel.org; Julien Grall <julien@xen.org>
> >> Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and
> >> alloc_domstatic_pages
> >>
> >> On 06.07.2021 07:58, Penny Zheng wrote:
> >>>> From: Jan Beulich <jbeulich@suse.com>
> >>>> Sent: Thursday, June 10, 2021 6:23 PM
> >>>>
> >>>> On 07.06.2021 04:43, Penny Zheng wrote:
> >>>>> --- a/xen/common/page_alloc.c
> >>>>> +++ b/xen/common/page_alloc.c
> >>>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages(
> >>>>>      return pg;
> >>>>>  }
> >>>>>
> >>>>> +#ifdef CONFIG_STATIC_ALLOCATION
> >>>>> +/*
> >>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static
> memory.
> >>>>> + * It is the equivalent of alloc_heap_pages for static memory  */
> >>>>> +static struct page_info *alloc_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 allocating at specified address. */
> >>>>> +    if ( !mfn_valid(smfn) || !nr_mfns )
> >>>>> +    {
> >>>>> +        printk(XENLOG_ERR
> >>>>> +               "Invalid %lu static memory starting at
> >>>>> + %"PRI_mfn"\n",
> >>>>
> >>>> Reading a log containing e.g. "Invalid 0 static memory starting at
> >>>> ..." I don't think I would recognize that the "0" is the count of pages.
> >>>
> >>> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"?
> >>
> >> This still doesn't convey _both_ parts of the if() that would cause
> >> the log message to be issued.
> >>
> >
> > Sorry. How about
> > "
> >         printk(XENLOG_ERR
> >                "Either out-of-range static memory starting at %"PRI_mfn""
> >                "or invalid number of pages: %ul.\n",
> >                mfn_x(smfn), nr_mfns);
> > "
> 
> I'm sorry - while now you convey both aspects, the message has become too
> verbose. What's wrong with "Invalid static memory request: ... pages at ...\"?
> But I wonder anyway if a log message is appropriate here in the first place.
> 

Sorry for my poor English writing. :/
Just having a habit(maybe not a good habit) of printing error messages, if you think the
log itself is verbose here, I'll remove it. ;)

> >>>>> @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages(
> >>>>>      return pg;
> >>>>>  }
> >>>>>
> >>>>> +#ifdef CONFIG_STATIC_ALLOCATION
> >>>>> +/*
> >>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of
> >>>>> +static memory,
> >>>>> + * then assign them to one specific domain #d.
> >>>>> + * It is the equivalent of alloc_domheap_pages for static memory.
> >>>>> + */
> >>>>> +struct page_info *alloc_domstatic_pages(
> >>>>> +        struct domain *d, unsigned long nr_mfns, mfn_t smfn,
> >>>>> +        unsigned int memflags)
> >>>>> +{
> >>>>> +    struct page_info *pg = NULL;
> >>>>> +    unsigned long dma_size;
> >>>>> +
> >>>>> +    ASSERT(!in_irq());
> >>>>> +
> >>>>> +    if ( !dma_bitsize )
> >>>>> +        memflags &= ~MEMF_no_dma;
> >>>>> +    else
> >>>>> +    {
> >>>>> +        if ( (dma_bitsize - PAGE_SHIFT) > 0 )
> >>>>> +        {
> >>>>> +            dma_size = 1ul << (dma_bitsize - PAGE_SHIFT);
> >>>>> +            /* Starting address shall meet the DMA limitation. */
> >>>>> +            if ( mfn_x(smfn) < dma_size )
> >>>>> +                return NULL;
> >>>>
> >>>> I think I did ask this on v1 already: Why the first page? Static
> >>>> memory regions, unlike buddy allocator zones, can cross power-of-2
> >>>> address boundaries. Hence it ought to be the last page that gets
> >>>> checked for fitting address width restriction requirements.
> >>>>
> >>>> And then - is this necessary at all? Shouldn't "pre-defined by
> >>>> configuration using physical address ranges" imply the memory
> >>>> designated for a guest fits its DMA needs?
> >>>>
> >>>
> >>> Hmmm, In my understanding, here is the DMA restriction when using
> >>> buddy
> >> allocator:
> >>>     else if ( (dma_zone = bits_to_zone(dma_bitsize)) < zone_hi )
> >>>         pg = alloc_heap_pages(dma_zone + 1, zone_hi, order,
> >>> memflags, d); dma_zone is restricting the starting buddy allocator
> >>> zone, so I am thinking that here, it shall restrict the first page.
> >>>
> >>> imo, if let user define, it also could be missing DMA restriction?
> >>
> >> Did you read my earlier reply? Again: The difference is that ordinary
> >> allocations (buddies) can't cross zone boundaries. Hence it is
> >> irrelevant if you check DMA properties on the first or last page -
> >> both will have the same number of significant bits. The same is -
> >> afaict - not true for static allocation ranges.
> >>
> >
> > True.
> >
> > Ordinary allocations (buddies) can't cross zone boundaries, So I
> > understand that following the logic in "alloc_heap_pages(dma_zone + 1,
> zone_hi, order, memflags, d);"
> > pages of the smallest address shall be allocated from "dma_zone + 1",
> > like you said, it is irrelevant if you check DMA properties on the
> > first or last pages, but imo, no matter first or last page, both shall be larger
> than (2^(dma_zone + 1)).
> >
> > Taking 32 as dma_bitsize, then the memory with this DMA restriction
> > allocated by "alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags,
> > d);" shall be at least more than 4G.
> 
> DMA restrictions are always "needs to be no more than N bits".
> 
> > That’s why I keep comparing the first page of static allocation, that
> > I am following the "more than" logic here.
> >
> > But you're right, I got a little investigation on ARM DMA limitation,
> > still taking dma_bitsize=32 as an example, we want that the actually
> allocated memory is smaller than 4G, not more than.
> > So I think the logic behind this code line " alloc_heap_pages(dma_zone
> > + 1, zone_hi, order, memflags, d);" is not right for ARM, and it shall
> > be changed to "alloc_heap_pages(zone_lo, dma_zone + 1, order, memflags,
> d);" as correction.
> 
> But this step is to _avoid_ the DMA-reserved part of the heap.

Thanks for the explanation!!!

I totally mis-understood the usage of dma_bitsize. It turns out to be the DMA-reserved...
Putting DMA limitation on zone_lo is absolutely right on ARM also.

> The caller requests address restricted memory by passing suitable memflags. If
> the request doesn't require access to the DMA- reserved part of the heap
> (dma_zone < zone_hi) we first try to get memory from there. Only if that fails
> will we fall back and try taking memory from the lower region. IOW the
> problem with your code is more fundamental: You use dma_bitsize when
> really you ought to extract the caller's requested restriction (if any) from
> memflags. I would assume that dma_bitsize is orthogonal to static memory, i.e.
> you don't need to try to preserve low memory.
> 

True, No need to preserve low memory for static memory.

> Jan

Penny Zheng

[PATCH 7/9] xen/arm: take care of concurrency on static memory allocation

[Penny Zheng]

In the future, user may want to allocate static memory at runtime,
and it is quite important to get the code protected from concurrent
access.

Re-use heap_lock to protect concurrent access in alloc_staticmem_pages.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
changes v2:
- new commit
---
 xen/common/page_alloc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index a0eea5f1a4..c6ccfc3216 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1087,6 +1087,9 @@ static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns,
                nr_mfns, mfn_x(smfn));
         return NULL;
     }
+
+    spin_lock(&heap_lock);
+
     pg = mfn_to_page(smfn);
 
     for ( i = 0; i < nr_mfns; i++ )
@@ -1127,6 +1130,8 @@ static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns,
                             !(memflags & MEMF_no_icache_flush));
     }
 
+    spin_unlock(&heap_lock);
+
     if ( need_tlbflush )
         filtered_flush_tlb_mask(tlbflush_timestamp);
 
-- 
2.25.1

Re: [PATCH 7/9] xen/arm: take care of concurrency on static memory allocation

[Jan Beulich]

On 07.06.2021 04:43, Penny Zheng wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1087,6 +1087,9 @@ static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns,
>                 nr_mfns, mfn_x(smfn));
>          return NULL;
>      }
> +
> +    spin_lock(&heap_lock);
> +
>      pg = mfn_to_page(smfn);
>  
>      for ( i = 0; i < nr_mfns; i++ )
> @@ -1127,6 +1130,8 @@ static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns,
>                              !(memflags & MEMF_no_icache_flush));
>      }
>  
> +    spin_unlock(&heap_lock);
> +
>      if ( need_tlbflush )
>          filtered_flush_tlb_mask(tlbflush_timestamp);

Besides, as indicated there, the need to fold this into the previous
patch, you will also want to pay attention to alloc_heap_pages()
carefully avoiding to scrub or flush pages with the heap lock held.
You will want to follow this for your additions.

Jan

[PATCH 8/9] 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.

And if the `memory` property and `xen,static-mem` are both set, it shall
be verified that if the memory size defined in both is consistent.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
changes v2:
- remove parsing procedure here
- check the consistency when `xen,static-mem` and `memory` are both defined
---
 xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 282416e74d..4166d7993c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2424,23 +2424,47 @@ static int __init construct_domU(struct domain *d,
 {
     struct kernel_info kinfo = {};
     int rc;
-    u64 mem;
+    u64 mem, static_mem_size = 0;
+    const struct dt_property *prop;
+    bool static_mem = false;
+
+    d->max_pages = ~0U;
+    /*
+     * Guest RAM could be of static memory from static allocation,
+     * which will be specified through "xen,static-mem" phandle.
+     */
+    prop = dt_find_property(node, "xen,static-mem", NULL);
+    if ( prop )
+    {
+        static_mem = true;
+        /* static_mem_size = allocate_static_memory(...); */
+        BUG();
+    }
 
     rc = dt_property_read_u64(node, "memory", &mem);
-    if ( !rc )
+    if ( !static_mem && !rc )
     {
         printk("Error building DomU: cannot read \"memory\" property\n");
         return -EINVAL;
+    } else if ( rc && static_mem )
+    {
+        if ( static_mem_size != mem * SZ_1K )
+        {
+            printk("Memory size in \"memory\" property isn't consistent with"
+                   "the ones defined in \"xen,static-mem\".\n");
+            return -EINVAL;
+        }
     }
-    kinfo.unassigned_mem = (paddr_t)mem * SZ_1K;
+    kinfo.unassigned_mem = static_mem ? 0 : (paddr_t)mem * SZ_1K;
 
-    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n", d->max_vcpus, mem);
+    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n",
+            d->max_vcpus,
+            static_mem ? static_mem_size : (kinfo.unassigned_mem) >> 10);
 
     kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
 
     if ( vcpu_create(d, 0) == NULL )
         return -ENOMEM;
-    d->max_pages = ~0U;
 
     kinfo.d = d;
 
@@ -2452,7 +2476,8 @@ 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);
 
     rc = prepare_dtb_domU(d, &kinfo);
     if ( rc < 0 )
-- 
2.25.1

Re: [PATCH 8/9] xen/arm: check `xen,static-mem` property during domain construction

[Julien Grall]

Hi Penny,

On 07/06/2021 03:43, 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.
> 
> And if the `memory` property and `xen,static-mem` are both set, it shall
> be verified that if the memory size defined in both is consistent.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> changes v2:
> - remove parsing procedure here
> - check the consistency when `xen,static-mem` and `memory` are both defined
> ---
>   xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++------
>   1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 282416e74d..4166d7993c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2424,23 +2424,47 @@ static int __init construct_domU(struct domain *d,
>   {
>       struct kernel_info kinfo = {};
>       int rc;
> -    u64 mem;
> +    u64 mem, static_mem_size = 0;
> +    const struct dt_property *prop;
> +    bool static_mem = false;
> +
> +    d->max_pages = ~0U;
> +    /*
> +     * Guest RAM could be of static memory from static allocation,
> +     * which will be specified through "xen,static-mem" phandle.
> +     */
> +    prop = dt_find_property(node, "xen,static-mem", NULL);
> +    if ( prop )
> +    {
> +        static_mem = true;
> +        /* static_mem_size = allocate_static_memory(...); */
> +        BUG();
> +    }

I would prefer if the static memory is allocated close to 
allocate_memory() below. AFAICT, the reason you allocate here is because 
you want to have the property "memory" optional.

However, I am not entirely convinced this is a good idea to make 
optional. It would be easier for a reader to figure out from the 
device-tree how much memory we give to the guest.

>   
>       rc = dt_property_read_u64(node, "memory", &mem);
> -    if ( !rc )
> +    if ( !static_mem && !rc )
>       {
>           printk("Error building DomU: cannot read \"memory\" property\n");
>           return -EINVAL;
> +    } else if ( rc && static_mem )
> +    {
> +        if ( static_mem_size != mem * SZ_1K )
> +        {
> +            printk("Memory size in \"memory\" property isn't consistent with"
> +                   "the ones defined in \"xen,static-mem\".\n");
> +            return -EINVAL;
> +        }
>       } > -    kinfo.unassigned_mem = (paddr_t)mem * SZ_1K;
> +    kinfo.unassigned_mem = static_mem ? 0 : (paddr_t)mem * SZ_1K; >
> -    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n", d->max_vcpus, mem);
> +    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n",
> +            d->max_vcpus,
> +            static_mem ? static_mem_size : (kinfo.unassigned_mem) >> 10);


If we mandate the property "memory", then kinfo.unassigned_mem doesn't 
need to be touched. Instead, you could simply check the 
kinfo.unassigned_mem is equivalent to static_mem_size.

>   
>       kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
>   
>       if ( vcpu_create(d, 0) == NULL )
>           return -ENOMEM;
> -    d->max_pages = ~0U;
>   
>       kinfo.d = d;
>   
> @@ -2452,7 +2476,8 @@ 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);
>   
>       rc = prepare_dtb_domU(d, &kinfo);
>       if ( rc < 0 )
> 

Cheers,

-- 
Julien Grall

RE: [PATCH 8/9] xen/arm: check `xen,static-mem` property during domain construction

[Penny Zheng]

Hi 

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, July 3, 2021 9:26 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 8/9] xen/arm: check `xen,static-mem` property during
> domain construction
> 
> Hi Penny,
> 
> On 07/06/2021 03:43, 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.
> >
> > And if the `memory` property and `xen,static-mem` are both set, it
> > shall be verified that if the memory size defined in both is consistent.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > changes v2:
> > - remove parsing procedure here
> > - check the consistency when `xen,static-mem` and `memory` are both
> > defined
> > ---
> >   xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++---
> ---
> >   1 file changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 282416e74d..4166d7993c 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2424,23 +2424,47 @@ static int __init construct_domU(struct domain
> *d,
> >   {
> >       struct kernel_info kinfo = {};
> >       int rc;
> > -    u64 mem;
> > +    u64 mem, static_mem_size = 0;
> > +    const struct dt_property *prop;
> > +    bool static_mem = false;
> > +
> > +    d->max_pages = ~0U;
> > +    /*
> > +     * Guest RAM could be of static memory from static allocation,
> > +     * which will be specified through "xen,static-mem" phandle.
> > +     */
> > +    prop = dt_find_property(node, "xen,static-mem", NULL);
> > +    if ( prop )
> > +    {
> > +        static_mem = true;
> > +        /* static_mem_size = allocate_static_memory(...); */
> > +        BUG();
> > +    }
> 
> I would prefer if the static memory is allocated close to
> allocate_memory() below. AFAICT, the reason you allocate here is because you
> want to have the property "memory" optional.
> 
> However, I am not entirely convinced this is a good idea to make optional. It
> would be easier for a reader to figure out from the device-tree how much
> memory we give to the guest.
> 

Hmmm, now I think maybe I understand wrongly what you suggested in v1.
"
Could we allocate the memory as we parse?
"
I just simply think it means the code sequence, putting allocation immediately
after parsing. ;/

Re-investigating the docs on "memory":

"
- memory

    A 64-bit integer specifying the amount of kilobytes of RAM to
    allocate to the guest.
"
If you prefer "memory" mandate, then tbh, it will make the code
here more easily-read, no ifs.
But maybe I shall put more info on docs to clarify that even though user using
"xen, static-mem" to refer to static memory allocation, they shall still use
"memory" property to tell how much memory we give to the guest.

> >
> >       rc = dt_property_read_u64(node, "memory", &mem);
> > -    if ( !rc )
> > +    if ( !static_mem && !rc )
> >       {
> >           printk("Error building DomU: cannot read \"memory\" property\n");
> >           return -EINVAL;
> > +    } else if ( rc && static_mem )
> > +    {
> > +        if ( static_mem_size != mem * SZ_1K )
> > +        {
> > +            printk("Memory size in \"memory\" property isn't consistent with"
> > +                   "the ones defined in \"xen,static-mem\".\n");
> > +            return -EINVAL;
> > +        }
> >       } > -    kinfo.unassigned_mem = (paddr_t)mem * SZ_1K;
> > +    kinfo.unassigned_mem = static_mem ? 0 : (paddr_t)mem * SZ_1K; >
> > -    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n", d-
> >max_vcpus, mem);
> > +    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n",
> > +            d->max_vcpus,
> > +            static_mem ? static_mem_size : (kinfo.unassigned_mem) >>
> > + 10);
> 
> 
> If we mandate the property "memory", then kinfo.unassigned_mem doesn't
> need to be touched. Instead, you could simply check the
> kinfo.unassigned_mem is equivalent to static_mem_size.
> 

True, true. 

> >
> >       kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
> >
> >       if ( vcpu_create(d, 0) == NULL )
> >           return -ENOMEM;
> > -    d->max_pages = ~0U;
> >
> >       kinfo.d = d;
> >
> > @@ -2452,7 +2476,8 @@ 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);
> >
> >       rc = prepare_dtb_domU(d, &kinfo);
> >       if ( rc < 0 )
> >
> 
> Cheers,
> 
> --
> Julien Grall

Cheers,

--
Penny Zheng

Re: [PATCH 8/9] xen/arm: check `xen,static-mem` property during domain construction

[Jan Beulich]

On 06.07.2021 08:31, Penny Zheng wrote:
> Hi 

I'm sorry, but since this has been ongoing: Can the two of you please
properly separate between To: and Cc:. For quite some parts of this
overall thread I've been on the To: list for no reason at all, afaict.

Thanks, Jan

RE: [PATCH 8/9] xen/arm: check `xen,static-mem` property during domain construction

[Penny Zheng]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, July 6, 2021 2:58 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; Julien Grall <julien@xen.org>
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>; sstabellini@kernel.org; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH 8/9] xen/arm: check `xen,static-mem` property during
> domain construction
> 
> On 06.07.2021 08:31, Penny Zheng wrote:
> > Hi
> 
> I'm sorry, but since this has been ongoing: Can the two of you please properly
> separate between To: and Cc:. For quite some parts of this overall thread I've
> been on the To: list for no reason at all, afaict.
> 

So sorry, I'll check the To: and Cc: more carefully.

> Thanks, Jan

Thanks,penny

Re: [PATCH 8/9] xen/arm: check `xen,static-mem` property during domain construction

[Julien Grall]

On 06/07/2021 07:31, Penny Zheng wrote:
> Hi

Hi,

> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Saturday, July 3, 2021 9:26 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org; jbeulich@suse.com
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>
>> Subject: Re: [PATCH 8/9] xen/arm: check `xen,static-mem` property during
>> domain construction
>>
>> Hi Penny,
>>
>> On 07/06/2021 03:43, 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.
>>>
>>> And if the `memory` property and `xen,static-mem` are both set, it
>>> shall be verified that if the memory size defined in both is consistent.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>> changes v2:
>>> - remove parsing procedure here
>>> - check the consistency when `xen,static-mem` and `memory` are both
>>> defined
>>> ---
>>>    xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++---
>> ---
>>>    1 file changed, 31 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 282416e74d..4166d7993c 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -2424,23 +2424,47 @@ static int __init construct_domU(struct domain
>> *d,
>>>    {
>>>        struct kernel_info kinfo = {};
>>>        int rc;
>>> -    u64 mem;
>>> +    u64 mem, static_mem_size = 0;
>>> +    const struct dt_property *prop;
>>> +    bool static_mem = false;
>>> +
>>> +    d->max_pages = ~0U;
>>> +    /*
>>> +     * Guest RAM could be of static memory from static allocation,
>>> +     * which will be specified through "xen,static-mem" phandle.
>>> +     */
>>> +    prop = dt_find_property(node, "xen,static-mem", NULL);
>>> +    if ( prop )
>>> +    {
>>> +        static_mem = true;
>>> +        /* static_mem_size = allocate_static_memory(...); */
>>> +        BUG();
>>> +    }
>>
>> I would prefer if the static memory is allocated close to
>> allocate_memory() below. AFAICT, the reason you allocate here is because you
>> want to have the property "memory" optional.
>>
>> However, I am not entirely convinced this is a good idea to make optional. It
>> would be easier for a reader to figure out from the device-tree how much
>> memory we give to the guest.
>>
> 
> Hmmm, now I think maybe I understand wrongly what you suggested in v1.
> "
> Could we allocate the memory as we parse?
> "
> I just simply think it means the code sequence, putting allocation immediately
> after parsing. ;/

I really meant "parse and allocate" in a iteration. My comment this time 
is the parsing/allocation for static memory should happen close to when 
the allocation for dynamic memory is done.

After all you are allocating memory for domain, so it makes more sense 
to have the two different way to allocate cloe to each other.

> 
> Re-investigating the docs on "memory":
> 
> "
> - memory
> 
>      A 64-bit integer specifying the amount of kilobytes of RAM to
>      allocate to the guest.
> "
> If you prefer "memory" mandate, then tbh, it will make the code
> here more easily-read, no ifs.
> But maybe I shall put more info on docs to clarify that even though user using
> "xen, static-mem" to refer to static memory allocation, they shall still use
> "memory" property to tell how much memory we give to the guest.

Hmm... I am not sure this is necessary, the property "memory" is not 
marked as optional even after your patch.

However, I would clarify that all the memory should either be allocated 
statically or dynamically...

Cheers,

-- 
Julien Grall

[PATCH 9/9] 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 alloc_domstatic_pages to allocate pre-configured static memory banks
for this domain, and uses guest_physmap_add_page to set up P2M table.
These pre-defiend static memory ranges 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`.
`GUEST_RAM0` may take up several pre-defined physical RAM regions.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
changes v2:
- rename the values, like prefix it g/p
- fix the scalability issue
- allocate when parse
---
 xen/arch/arm/domain_build.c | 155 +++++++++++++++++++++++++++++++++++-
 1 file changed, 153 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4166d7993c..63b6a97b2c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -437,6 +437,48 @@ static bool __init allocate_bank_memory(struct domain *d,
     return true;
 }
 
+/*
+ * Static memory bank at #smfn of #gsize shall be mapped to #sgfn of #gsize,
+ * and #sgfn will be next guest address to map when returning.
+ */
+static bool __init allocate_static_bank_memory(struct domain *d,
+                                               struct kernel_info *kinfo,
+                                               int gbank,
+                                               gfn_t* sgfn,
+                                               mfn_t smfn,
+                                               paddr_t gsize)
+{
+    int res;
+    paddr_t tot_size = gsize;
+    const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
+
+    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;
+        }
+
+        *sgfn = gfn_add(*sgfn, 1UL << order);
+        smfn = mfn_add(smfn, 1UL << order);
+        tot_size -= (1ULL << (PAGE_SHIFT + order));
+    }
+
+    /* Guest RAM bank in kinfo hasn't been initialized. */
+    if ( gbank == kinfo->mem.nr_banks )
+    {
+        kinfo->mem.bank[gbank].start = rambase[gbank];
+        kinfo->mem.nr_banks++;
+    }
+    kinfo->mem.bank[gbank].size += gsize;
+
+    return true;
+}
+
 static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
 {
     unsigned int i;
@@ -480,6 +522,116 @@ fail:
           (unsigned long)kinfo->unassigned_mem >> 10);
 }
 
+/* Allocate memory from static memory as RAM for one specific domain d. */
+static u64 __init allocate_static_memory(struct domain *d,
+                                          struct kernel_info *kinfo,
+                                          const struct dt_device_node *node)
+{
+    int nr_banks, bank = 0, gbank = 0;
+    const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
+    const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
+    const __be32 *cell;
+    const struct dt_property *prop;
+    struct dt_device_node *static_mem_node;
+    const struct dt_device_node *parent = dt_find_node_by_path("/reserved-memory");
+    u32 addr_cells = 2, size_cells = 2, reg_cells;
+    u64 tot_size;
+
+    paddr_t pbase, psize, gsize;
+    gfn_t sgfn;
+    mfn_t smfn;
+
+    kinfo->mem.nr_banks = 0;
+    /* Start with GUEST_RAM0. */
+    gsize = ramsize[gbank];
+    sgfn = gaddr_to_gfn(rambase[gbank]);
+
+    /* Parse phandle in `xen,static-mem`. */
+    static_mem_node = dt_parse_phandle(node, "xen,static-mem", 0);
+    if ( !static_mem_node )
+        goto fail;
+
+    /*
+     * #address-cells and #size-cells must be consistent with the parent node,
+     * "reserved-memory".
+     */
+    dt_property_read_u32(parent, "#address-cells", &addr_cells);
+    dt_property_read_u32(parent, "#size-cells", &size_cells);
+    BUG_ON(size_cells > 2 || addr_cells > 2);
+    reg_cells = addr_cells + size_cells;
+
+    prop = dt_find_property(static_mem_node, "reg", NULL);
+    if ( !prop )
+        goto fail;
+    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 += (u64)psize;
+        smfn = maddr_to_mfn(pbase);
+
+        if ( !alloc_domstatic_pages(d, psize >> PAGE_SHIFT, smfn, 0) )
+        {
+            printk(XENLOG_ERR
+                    "%pd: cannot allocate static memory"
+                    "(0x%"PRIpaddr" - 0x%"PRIpaddr")",
+                    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 )
+        {
+            if ( gsize >= psize )
+            {
+                if ( !allocate_static_bank_memory(d, kinfo, gbank,
+                                                  &sgfn, smfn, psize) )
+                    goto fail;
+
+                gsize = gsize - psize;
+                bank++;
+                break;
+            }
+            else
+            {
+                if ( !allocate_static_bank_memory(d, kinfo, gbank,
+                                                  &sgfn, smfn, gsize) )
+                    goto fail;
+
+                /*
+                 * Physical bank hasn't been totally mapped,
+                 * seeking to the next guest RAM i+1, if exist.
+                 */
+                if ( ++gbank < GUEST_RAM_BANKS )
+                {
+                    psize = psize - gsize;
+                    smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
+                    gsize = ramsize[gbank];
+                    sgfn = gaddr_to_gfn(rambase[gbank]);
+                }
+                else
+                    goto fail;
+            }
+        }
+    }
+    return tot_size;
+
+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)
 {
@@ -2437,8 +2589,7 @@ static int __init construct_domU(struct domain *d,
     if ( prop )
     {
         static_mem = true;
-        /* static_mem_size = allocate_static_memory(...); */
-        BUG();
+        static_mem_size = allocate_static_memory(d, &kinfo, node);
     }
 
     rc = dt_property_read_u64(node, "memory", &mem);
-- 
2.25.1

Re: [PATCH 9/9] xen/arm: introduce allocate_static_memory

[Julien Grall]

Hi Penny,

On 07/06/2021 03:43, Penny Zheng wrote:
> This commit introduces allocate_static_memory to allocate static memory as
> guest RAM for Domain on Static Allocation.
> 
> It uses alloc_domstatic_pages to allocate pre-configured static memory banks
> for this domain, and uses guest_physmap_add_page to set up P2M table.
> These pre-defiend static memory ranges 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`.
> `GUEST_RAM0` may take up several pre-defined physical RAM regions.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> changes v2:
> - rename the values, like prefix it g/p
> - fix the scalability issue
> - allocate when parse
> ---
>   xen/arch/arm/domain_build.c | 155 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 153 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4166d7993c..63b6a97b2c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -437,6 +437,48 @@ static bool __init allocate_bank_memory(struct domain *d,
>       return true;
>   }
>   
> +/*
> + * Static memory bank at #smfn of #gsize shall be mapped to #sgfn of #gsize,
> + * and #sgfn will be next guest address to map when returning.
> + */
> +static bool __init allocate_static_bank_memory(struct domain *d,
> +                                               struct kernel_info *kinfo,
> +                                               int gbank,

Please use unsigned int for index.

> +                                               gfn_t* sgfn,
> +                                               mfn_t smfn,
> +                                               paddr_t gsize)

This function doesn't allocate memory and neither a "bank" per-se. So 
would suggest to rename to (or something similar):

append_static_memory_to_bank()

Also, I don't think you need all those parameters. You can infer the 
next GFN to use from the bank information. So how about something like:

static bool __init append_static_memory_to_bank(struct domain *d,
                                                 struct membank *bank,
                                                 mfn_t smfn,
                                                 paddr_t size)
{
    gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);

    [....]
}

> +{
> +    int res;
> +    paddr_t tot_size = gsize;
> +    const uint64_t rambase[] = GUEST_RAM_BANK_BASES;

I don't like the idea of introducing rambase here. Can't the bank be 
initialized in the caller?

> +
> +    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;
> +        }
> +
> +        *sgfn = gfn_add(*sgfn, 1UL << order);
> +        smfn = mfn_add(smfn, 1UL << order);
> +        tot_size -= (1ULL << (PAGE_SHIFT + order));
> +    }
> +
> +    /* Guest RAM bank in kinfo hasn't been initialized. */
> +    if ( gbank == kinfo->mem.nr_banks )
> +    {
> +        kinfo->mem.bank[gbank].start = rambase[gbank];
> +        kinfo->mem.nr_banks++;
> +    }
> +    kinfo->mem.bank[gbank].size += gsize;
> +
> +    return true;
> +}
> +
>   static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)

It feels a bit odd that the two functions you introduced are not 
together. Can they be moved together?

>   {
>       unsigned int i;
> @@ -480,6 +522,116 @@ fail:
>             (unsigned long)kinfo->unassigned_mem >> 10);
>   }
>   
> +/* Allocate memory from static memory as RAM for one specific domain d. */
> +static u64 __init allocate_static_memory(struct domain *d,
> +                                          struct kernel_info *kinfo,
> +                                          const struct dt_device_node *node)
> +{
> +    int nr_banks, bank = 0, gbank = 0;
> +    const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
> +    const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
> +    const __be32 *cell;
> +    const struct dt_property *prop;
> +    struct dt_device_node *static_mem_node;
> +    const struct dt_device_node *parent = dt_find_node_by_path("/reserved-memory");
> +    u32 addr_cells = 2, size_cells = 2, reg_cells;
> +    u64 tot_size;
> +
> +    paddr_t pbase, psize, gsize;
> +    gfn_t sgfn;
> +    mfn_t smfn;
> +
> +    kinfo->mem.nr_banks = 0;
> +    /* Start with GUEST_RAM0. */
> +    gsize = ramsize[gbank];
> +    sgfn = gaddr_to_gfn(rambase[gbank]);
> +
> +    /* Parse phandle in `xen,static-mem`. */
> +    static_mem_node = dt_parse_phandle(node, "xen,static-mem", 0);
> +    if ( !static_mem_node )
> +        goto fail;
> +
> +    /*
> +     * #address-cells and #size-cells must be consistent with the parent node,
> +     * "reserved-memory".
> +     */
> +    dt_property_read_u32(parent, "#address-cells", &addr_cells);
> +    dt_property_read_u32(parent, "#size-cells", &size_cells);

The return for dt_property_read_u32() should be checked.

> +    BUG_ON(size_cells > 2 || addr_cells > 2);
> +    reg_cells = addr_cells + size_cells;
> +
> +    prop = dt_find_property(static_mem_node, "reg", NULL);
> +    if ( !prop )
> +        goto fail;
> +    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);

We seem to have quite a few functions now that will iterate over "regs". 
It would be worth considering to introduce a helper to iterate it.

> +        tot_size += (u64)psize;

This cast seems unnecessary.

> +        smfn = maddr_to_mfn(pbase);
> +
> +        if ( !alloc_domstatic_pages(d, psize >> PAGE_SHIFT, smfn, 0) )
> +        {
> +            printk(XENLOG_ERR
> +                    "%pd: cannot allocate static memory"
> +                    "(0x%"PRIpaddr" - 0x%"PRIpaddr")",
> +                    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 )
> +        {
> +            if ( gsize >= psize )
> +            {
> +                if ( !allocate_static_bank_memory(d, kinfo, gbank,
> +                                                  &sgfn, smfn, psize) )
> +                    goto fail;
> +
> +                gsize = gsize - psize;
> +                bank++;
> +                break;
> +            }
> +            else
> +            {
> +                if ( !allocate_static_bank_memory(d, kinfo, gbank,
> +                                                  &sgfn, smfn, gsize) )
> +                    goto fail;
> +
> +                /*
> +                 * Physical bank hasn't been totally mapped,
> +                 * seeking to the next guest RAM i+1, if exist.
> +                 */
> +                if ( ++gbank < GUEST_RAM_BANKS )
> +                {
> +                    psize = psize - gsize;
> +                    smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> +                    gsize = ramsize[gbank];
> +                    sgfn = gaddr_to_gfn(rambase[gbank]);
> +                }
> +                else
> +                    goto fail;
> +            }

The double loop is not nice but I can't think of a better way. However, 
I think the code in the loop can be simplified. You could write 
something like:

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

/* The physical bank is fully mapped. Handle the next bank. */
if ( gsize >= psize )
{
     gsize = gsize - psize;
     bank++;
     break;
}
/* We still have memory to map. Check if we have another guest bank 
available */
else if ( ++gbank > GUEST_RAM_BANKS ) {
    printk("Exhausted the number of guest bank\n");
    goto fail;
}

[Update to the next guest bank]

> +        }
> +    }
> +    return tot_size;
> +
> +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)
>   {
> @@ -2437,8 +2589,7 @@ static int __init construct_domU(struct domain *d,
>       if ( prop )
>       {
>           static_mem = true;
> -        /* static_mem_size = allocate_static_memory(...); */
> -        BUG();
> +        static_mem_size = allocate_static_memory(d, &kinfo, node);
>       }
>   
>       rc = dt_property_read_u64(node, "memory", &mem);
> 

Cheers,

-- 
Julien Grall

RE: [PATCH 9/9] xen/arm: introduce allocate_static_memory

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, July 3, 2021 10:18 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 9/9] xen/arm: introduce allocate_static_memory
> 
> Hi Penny,
> 
> On 07/06/2021 03:43, Penny Zheng wrote:
> > This commit introduces allocate_static_memory to allocate static
> > memory as guest RAM for Domain on Static Allocation.
> >
> > It uses alloc_domstatic_pages to allocate pre-configured static memory
> > banks for this domain, and uses guest_physmap_add_page to set up P2M
> table.
> > These pre-defiend static memory ranges 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`.
> > `GUEST_RAM0` may take up several pre-defined physical RAM regions.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > changes v2:
> > - rename the values, like prefix it g/p
> > - fix the scalability issue
> > - allocate when parse
> > ---
> >   xen/arch/arm/domain_build.c | 155
> +++++++++++++++++++++++++++++++++++-
> >   1 file changed, 153 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 4166d7993c..63b6a97b2c 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -437,6 +437,48 @@ static bool __init allocate_bank_memory(struct
> domain *d,
> >       return true;
> >   }
> >
> > +/*
> > + * Static memory bank at #smfn of #gsize shall be mapped to #sgfn of
> > +#gsize,
> > + * and #sgfn will be next guest address to map when returning.
> > + */
> > +static bool __init allocate_static_bank_memory(struct domain *d,
> > +                                               struct kernel_info *kinfo,
> > +                                               int gbank,
> 
> Please use unsigned int for index.
> 

Sure.

> > +                                               gfn_t* sgfn,
> > +                                               mfn_t smfn,
> > +                                               paddr_t gsize)
> 
> This function doesn't allocate memory and neither a "bank" per-se. So would
> suggest to rename to (or something similar):
> 
> append_static_memory_to_bank()
> 

Ok~~~

> Also, I don't think you need all those parameters. You can infer the next GFN
> to use from the bank information. So how about something like:
> 
> static bool __init append_static_memory_to_bank(struct domain *d,
>                                                  struct membank *bank,
>                                                  mfn_t smfn,
>                                                  paddr_t size) {
>     gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
> 
>     [....]
> }
> 

Oh. Right, since we are setting info in kinfo.mem.bank[i], we could infer itself to get the
Next GFN, clever! Thks a lot!

> > +{
> > +    int res;
> > +    paddr_t tot_size = gsize;
> > +    const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
> 
> I don't like the idea of introducing rambase here. Can't the bank be initialized
> in the caller?
> 

Hmm, I'm kinds of confused the suggestion here. If we using rambase[] here,
then in later double loop, when updating to the next guest bank,  we could
use rambase[gbank] to refer.

> > +
> > +    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;
> > +        }
> > +
> > +        *sgfn = gfn_add(*sgfn, 1UL << order);
> > +        smfn = mfn_add(smfn, 1UL << order);
> > +        tot_size -= (1ULL << (PAGE_SHIFT + order));
> > +    }
> > +
> > +    /* Guest RAM bank in kinfo hasn't been initialized. */
> > +    if ( gbank == kinfo->mem.nr_banks )
> > +    {
> > +        kinfo->mem.bank[gbank].start = rambase[gbank];
> > +        kinfo->mem.nr_banks++;
> > +    }
> > +    kinfo->mem.bank[gbank].size += gsize;
> > +
> > +    return true;
> > +}
> > +
> >   static void __init allocate_memory(struct domain *d, struct
> > kernel_info *kinfo)
> 
> It feels a bit odd that the two functions you introduced are not together. Can
> they be moved together?
> 

Ok. Will do.

> >   {
> >       unsigned int i;
> > @@ -480,6 +522,116 @@ fail:
> >             (unsigned long)kinfo->unassigned_mem >> 10);
> >   }
> >
> > +/* Allocate memory from static memory as RAM for one specific domain
> > +d. */ static u64 __init allocate_static_memory(struct domain *d,
> > +                                          struct kernel_info *kinfo,
> > +                                          const struct dt_device_node
> > +*node) {
> > +    int nr_banks, bank = 0, gbank = 0;
> > +    const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
> > +    const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
> > +    const __be32 *cell;
> > +    const struct dt_property *prop;
> > +    struct dt_device_node *static_mem_node;
> > +    const struct dt_device_node *parent =
> dt_find_node_by_path("/reserved-memory");
> > +    u32 addr_cells = 2, size_cells = 2, reg_cells;
> > +    u64 tot_size;
> > +
> > +    paddr_t pbase, psize, gsize;
> > +    gfn_t sgfn;
> > +    mfn_t smfn;
> > +
> > +    kinfo->mem.nr_banks = 0;
> > +    /* Start with GUEST_RAM0. */
> > +    gsize = ramsize[gbank];
> > +    sgfn = gaddr_to_gfn(rambase[gbank]);
> > +
> > +    /* Parse phandle in `xen,static-mem`. */
> > +    static_mem_node = dt_parse_phandle(node, "xen,static-mem", 0);
> > +    if ( !static_mem_node )
> > +        goto fail;
> > +
> > +    /*
> > +     * #address-cells and #size-cells must be consistent with the parent node,
> > +     * "reserved-memory".
> > +     */
> > +    dt_property_read_u32(parent, "#address-cells", &addr_cells);
> > +    dt_property_read_u32(parent, "#size-cells", &size_cells);
> 
> The return for dt_property_read_u32() should be checked.

Sure.

> 
> > +    BUG_ON(size_cells > 2 || addr_cells > 2);
> > +    reg_cells = addr_cells + size_cells;
> > +
> > +    prop = dt_find_property(static_mem_node, "reg", NULL);
> > +    if ( !prop )
> > +        goto fail;
> > +    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);
> 
> We seem to have quite a few functions now that will iterate over "regs".
> It would be worth considering to introduce a helper to iterate it.
>

Do you want to expand the "device_tree_get_reg" to only cope with "regs" property, 
right now, it passes into cells, "prop->value", the cells of the reg property.
Changing it to that the input parameter will be the device node holding the reg property, like
"device_tree_get_reg(const struct dt_device_node *node, u32 addr_cells, u32 size_cells, struct meminfo *info)"
Or we still retain the original device_tree_get_reg()(maybe, the name shall be changed....), and
Introduce a new function to do what I said above.
 
> > +        tot_size += (u64)psize;
> 
> This cast seems unnecessary.
> 
> > +        smfn = maddr_to_mfn(pbase);
> > +
> > +        if ( !alloc_domstatic_pages(d, psize >> PAGE_SHIFT, smfn, 0) )
> > +        {
> > +            printk(XENLOG_ERR
> > +                    "%pd: cannot allocate static memory"
> > +                    "(0x%"PRIpaddr" - 0x%"PRIpaddr")",
> > +                    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 )
> > +        {
> > +            if ( gsize >= psize )
> > +            {
> > +                if ( !allocate_static_bank_memory(d, kinfo, gbank,
> > +                                                  &sgfn, smfn, psize) )
> > +                    goto fail;
> > +
> > +                gsize = gsize - psize;
> > +                bank++;
> > +                break;
> > +            }
> > +            else
> > +            {
> > +                if ( !allocate_static_bank_memory(d, kinfo, gbank,
> > +                                                  &sgfn, smfn, gsize) )
> > +                    goto fail;
> > +
> > +                /*
> > +                 * Physical bank hasn't been totally mapped,
> > +                 * seeking to the next guest RAM i+1, if exist.
> > +                 */
> > +                if ( ++gbank < GUEST_RAM_BANKS )
> > +                {
> > +                    psize = psize - gsize;
> > +                    smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> > +                    gsize = ramsize[gbank];
> > +                    sgfn = gaddr_to_gfn(rambase[gbank]);
> > +                }
> > +                else
> > +                    goto fail;
> > +            }
> 
> The double loop is not nice but I can't think of a better way. However, I think
> the code in the loop can be simplified. You could write something like:
> 
> /* Map as much as possible the static range to the guest bank */ if
> ( !allocate_static_bank(..., min(psize, gsize) )
>    goto fail;
> 
> /* The physical bank is fully mapped. Handle the next bank. */ if ( gsize >=
> psize ) {
>      gsize = gsize - psize;
>      bank++;
>      break;
> }
> /* We still have memory to map. Check if we have another guest bank
> available */ else if ( ++gbank > GUEST_RAM_BANKS ) {
>     printk("Exhausted the number of guest bank\n");
>     goto fail;
> }
> 
> [Update to the next guest bank]
> 

Thanks for the detailed simplification!!!

> > +        }
> > +    }
> > +    return tot_size;
> > +
> > +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)
> >   {
> > @@ -2437,8 +2589,7 @@ static int __init construct_domU(struct domain *d,
> >       if ( prop )
> >       {
> >           static_mem = true;
> > -        /* static_mem_size = allocate_static_memory(...); */
> > -        BUG();
> > +        static_mem_size = allocate_static_memory(d, &kinfo, node);
> >       }
> >
> >       rc = dt_property_read_u64(node, "memory", &mem);
> >
> 
> Cheers,
> 
> --
> Julien Grall


Cheers

--
Penny Zheng