[PATCH 0/3] mini-os: some memory map updates for PVH

[PATCH 0/3] mini-os: some memory map updates for PVH

[Juergen Gross]

Do some memory map related changes/fixes for PVH mode:

- Prefer the memory map delivered via start-info over the one obtained
  from the hypervisor. This is a prerequisite for Xenstore-stubdom
  live-update with rising the memory limit.

- Fix a bug related to ballooning in PVH mode: PVH Xenstore-stubdom
  can't read its target memory size from Xenstore, as this introduces
  a chicken-and-egg problem. The memory size read from the hypervisor
  OTOH includes additional "special" pages marked as reserved in the
  memory map. Those pages need to be subtracted from the read size.

Juergen Gross (3):
  mini-os: take newest version of arch-x86/hvm/start_info.h
  mini-os: prefer memory map via start_info for PVH
  mini-os: fix number of pages for PVH

 arch/x86/mm.c                         | 10 ++++-
 balloon.c                             |  2 +-
 e820.c                                | 53 +++++++++++++++++++---
 include/e820.h                        |  5 +++
 include/x86/arch_mm.h                 |  2 +
 include/xen/arch-x86/hvm/start_info.h | 63 ++++++++++++++++++++++++++-
 6 files changed, 125 insertions(+), 10 deletions(-)

-- 
2.35.3

[PATCH 1/3] mini-os: take newest version of arch-x86/hvm/start_info.h

[Juergen Gross]

Update include/xen/arch-x86/hvm/start_info.h to the newest version
from the Xen tree.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 include/xen/arch-x86/hvm/start_info.h | 63 ++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/include/xen/arch-x86/hvm/start_info.h b/include/xen/arch-x86/hvm/start_info.h
index 64841597..50af9ea2 100644
--- a/include/xen/arch-x86/hvm/start_info.h
+++ b/include/xen/arch-x86/hvm/start_info.h
@@ -33,7 +33,7 @@
  *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
  *    |                | ("xEn3" with the 0x80 bit of the "E" set).
  *  4 +----------------+
- *    | version        | Version of this structure. Current version is 0. New
+ *    | version        | Version of this structure. Current version is 1. New
  *    |                | versions are guaranteed to be backwards-compatible.
  *  8 +----------------+
  *    | flags          | SIF_xxx flags.
@@ -48,6 +48,15 @@
  * 32 +----------------+
  *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
  * 40 +----------------+
+ *    | memmap_paddr   | Physical address of the (optional) memory map. Only
+ *    |                | present in version 1 and newer of the structure.
+ * 48 +----------------+
+ *    | memmap_entries | Number of entries in the memory map table. Zero
+ *    |                | if there is no memory map being provided. Only
+ *    |                | present in version 1 and newer of the structure.
+ * 52 +----------------+
+ *    | reserved       | Version 1 and newer only.
+ * 56 +----------------+
  *
  * The layout of each entry in the module structure is the following:
  *
@@ -62,13 +71,51 @@
  *    | reserved       |
  * 32 +----------------+
  *
+ * The layout of each entry in the memory map table is as follows:
+ *
+ *  0 +----------------+
+ *    | addr           | Base address
+ *  8 +----------------+
+ *    | size           | Size of mapping in bytes
+ * 16 +----------------+
+ *    | type           | Type of mapping as defined between the hypervisor
+ *    |                | and guest. See XEN_HVM_MEMMAP_TYPE_* values below.
+ * 20 +----------------|
+ *    | reserved       |
+ * 24 +----------------+
+ *
  * The address and sizes are always a 64bit little endian unsigned integer.
  *
  * NB: Xen on x86 will always try to place all the data below the 4GiB
  * boundary.
+ *
+ * Version numbers of the hvm_start_info structure have evolved like this:
+ *
+ * Version 0:  Initial implementation.
+ *
+ * Version 1:  Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
+ *             padding) to the end of the hvm_start_info struct. These new
+ *             fields can be used to pass a memory map to the guest. The
+ *             memory map is optional and so guests that understand version 1
+ *             of the structure must check that memmap_entries is non-zero
+ *             before trying to read the memory map.
  */
 #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
 
+/*
+ * The values used in the type field of the memory map table entries are
+ * defined below and match the Address Range Types as defined in the "System
+ * Address Map Interfaces" section of the ACPI Specification. Please refer to
+ * section 15 in version 6.2 of the ACPI spec: http://uefi.org/specifications
+ */
+#define XEN_HVM_MEMMAP_TYPE_RAM       1
+#define XEN_HVM_MEMMAP_TYPE_RESERVED  2
+#define XEN_HVM_MEMMAP_TYPE_ACPI      3
+#define XEN_HVM_MEMMAP_TYPE_NVS       4
+#define XEN_HVM_MEMMAP_TYPE_UNUSABLE  5
+#define XEN_HVM_MEMMAP_TYPE_DISABLED  6
+#define XEN_HVM_MEMMAP_TYPE_PMEM      7
+
 /*
  * C representation of the x86/HVM start info layout.
  *
@@ -86,6 +133,13 @@ struct hvm_start_info {
     uint64_t cmdline_paddr;     /* Physical address of the command line.     */
     uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
                                 /* structure.                                */
+    /* All following fields only present in version 1 and newer */
+    uint64_t memmap_paddr;      /* Physical address of an array of           */
+                                /* hvm_memmap_table_entry.                   */
+    uint32_t memmap_entries;    /* Number of entries in the memmap table.    */
+                                /* Value will be zero if there is no memory  */
+                                /* map being provided.                       */
+    uint32_t reserved;          /* Must be zero.                             */
 };
 
 struct hvm_modlist_entry {
@@ -95,4 +149,11 @@ struct hvm_modlist_entry {
     uint64_t reserved;
 };
 
+struct hvm_memmap_table_entry {
+    uint64_t addr;              /* Base address of the memory region         */
+    uint64_t size;              /* Size of the memory region in bytes        */
+    uint32_t type;              /* Mapping type                              */
+    uint32_t reserved;          /* Must be zero for Version 1.               */
+};
+
 #endif /* __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ */
-- 
2.35.3

[PATCH 2/3] mini-os: prefer memory map via start_info for PVH

[Juergen Gross]

Since some time now a guest started in PVH mode will get the memory
map from Xen via the start_info structure.

Modify the PVH initialization to prefer this memory map over the one
obtained via hypercall, as this will allow to add information to the
memory map for a new kernel when supporting kexec.

In case the start_info structure doesn't contain memory map information
fall back to the hypercall.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/mm.c  |  6 ++++++
 e820.c         | 25 +++++++++++++++++++++++++
 include/e820.h |  4 ++++
 3 files changed, 35 insertions(+)

diff --git a/arch/x86/mm.c b/arch/x86/mm.c
index 220c0b4d..41fcee67 100644
--- a/arch/x86/mm.c
+++ b/arch/x86/mm.c
@@ -45,6 +45,7 @@
 #include <mini-os/xmalloc.h>
 #include <mini-os/e820.h>
 #include <xen/memory.h>
+#include <xen/arch-x86/hvm/start_info.h>
 
 #ifdef MM_DEBUG
 #define DEBUG(_f, _a...) \
@@ -108,6 +109,11 @@ void arch_mm_preinit(void *p)
 {
     long ret;
     domid_t domid = DOMID_SELF;
+    struct hvm_start_info *hsi = p;
+
+    if ( hsi->version >= 1 && hsi->memmap_entries > 0 )
+        e820_init_memmap((struct hvm_memmap_table_entry *)(unsigned long)
+                         hsi->memmap_paddr, hsi->memmap_entries);
 
     pt_base = page_table_base;
     first_free_pfn = PFN_UP(to_phys(&_end));
diff --git a/e820.c b/e820.c
index 991ed382..ad91e00b 100644
--- a/e820.c
+++ b/e820.c
@@ -54,6 +54,7 @@ static char *e820_types[E820_TYPES] = {
     [E820_ACPI]     = "ACPI",
     [E820_NVS]      = "NVS",
     [E820_UNUSABLE] = "Unusable",
+    [E820_DISABLED] = "Disabled",
     [E820_PMEM]     = "PMEM"
 };
 
@@ -259,6 +260,30 @@ static void e820_get_memmap(void)
     e820_sanitize();
 }
 
+void e820_init_memmap(struct hvm_memmap_table_entry *entry, unsigned int num)
+{
+    unsigned int i;
+
+    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
+    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RESERVED != E820_RESERVED);
+    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
+    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_NVS != E820_NVS);
+    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_UNUSABLE != E820_UNUSABLE);
+    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_DISABLED != E820_DISABLED);
+    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_PMEM != E820_PMEM);
+
+    for ( i = 0; i < num; i++ )
+    {
+        e820_map[i].addr = entry[i].addr;
+        e820_map[i].size = entry[i].size;
+        e820_map[i].type = entry[i].type;
+    }
+
+    e820_entries = num;
+
+    e820_sanitize();
+}
+
 void arch_print_memmap(void)
 {
     int i;
diff --git a/include/e820.h b/include/e820.h
index aaf2f2ca..5438a7c8 100644
--- a/include/e820.h
+++ b/include/e820.h
@@ -26,6 +26,8 @@
 
 #if defined(__arm__) || defined(__aarch64__) || defined(CONFIG_PARAVIRT)
 #define CONFIG_E820_TRIVIAL
+#else
+#include <xen/arch-x86/hvm/start_info.h>
 #endif
 
 /* PC BIOS standard E820 types and structure. */
@@ -34,6 +36,7 @@
 #define E820_ACPI         3
 #define E820_NVS          4
 #define E820_UNUSABLE     5
+#define E820_DISABLED     6
 #define E820_PMEM         7
 #define E820_TYPES        8
 
@@ -54,6 +57,7 @@ unsigned long e820_get_max_contig_pages(unsigned long pfn, unsigned long pages);
 #ifndef CONFIG_E820_TRIVIAL
 unsigned long e820_get_reserved_pfns(int pages);
 void e820_put_reserved_pfns(unsigned long start_pfn, int pages);
+void e820_init_memmap(struct hvm_memmap_table_entry *entry, unsigned int num);
 #endif
 
 #endif /*__E820_HEADER*/
-- 
2.35.3

[PATCH 3/3] mini-os: fix number of pages for PVH

[Juergen Gross]

When getting the current allocation from Xen, this value includes the
pages allocated in the MMIO area. Fix the highest available RAM page
by subtracting the size of that area.

This requires to read the E820 map before needing this value.

At the same time add the LAPIC page to the memory map in order to
avoid reusing that PFN for internal purposes.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/mm.c         |  4 +++-
 balloon.c             |  2 +-
 e820.c                | 28 +++++++++++++++++++++-------
 include/e820.h        |  1 +
 include/x86/arch_mm.h |  2 ++
 5 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/arch/x86/mm.c b/arch/x86/mm.c
index 41fcee67..37089978 100644
--- a/arch/x86/mm.c
+++ b/arch/x86/mm.c
@@ -114,6 +114,8 @@ void arch_mm_preinit(void *p)
     if ( hsi->version >= 1 && hsi->memmap_entries > 0 )
         e820_init_memmap((struct hvm_memmap_table_entry *)(unsigned long)
                          hsi->memmap_paddr, hsi->memmap_entries);
+    else
+        e820_init_memmap(NULL, 0);
 
     pt_base = page_table_base;
     first_free_pfn = PFN_UP(to_phys(&_end));
@@ -124,7 +126,7 @@ void arch_mm_preinit(void *p)
         do_exit();
     }
 
-    last_free_pfn = e820_get_maxpfn(ret);
+    last_free_pfn = e820_get_maxpfn(ret - e820_initial_reserved_pfns);
     balloon_set_nr_pages(ret, last_free_pfn);
 }
 #endif
diff --git a/balloon.c b/balloon.c
index 9dc77c54..779223de 100644
--- a/balloon.c
+++ b/balloon.c
@@ -54,7 +54,7 @@ void get_max_pages(void)
         return;
     }
 
-    nr_max_pages = ret;
+    nr_max_pages = ret - e820_initial_reserved_pfns;
     printk("Maximum memory size: %ld pages\n", nr_max_pages);
 
     nr_max_pfn = e820_get_maxpfn(nr_max_pages);
diff --git a/e820.c b/e820.c
index ad91e00b..c3047336 100644
--- a/e820.c
+++ b/e820.c
@@ -29,6 +29,8 @@
 #include <mini-os/e820.h>
 #include <xen/memory.h>
 
+unsigned int e820_initial_reserved_pfns;
+
 #ifdef CONFIG_E820_TRIVIAL
 struct e820entry e820_map[1] = {
     {
@@ -40,10 +42,6 @@ struct e820entry e820_map[1] = {
 
 unsigned e820_entries = 1;
 
-static void e820_get_memmap(void)
-{
-}
-
 #else
 struct e820entry e820_map[E820_MAX];
 unsigned e820_entries;
@@ -199,6 +197,7 @@ static void e820_sanitize(void)
 {
     int i;
     unsigned long end, start;
+    bool found_lapic = false;
 
     /* Sanitize memory map in current form. */
     e820_process_entries();
@@ -238,8 +237,20 @@ static void e820_sanitize(void)
 
     /* Make remaining temporarily reserved entries permanently reserved. */
     for ( i = 0; i < e820_entries; i++ )
+    {
         if ( e820_map[i].type == E820_TMP_RESERVED )
             e820_map[i].type = E820_RESERVED;
+        if ( e820_map[i].type == E820_RESERVED )
+        {
+            e820_initial_reserved_pfns += e820_map[i].size / PAGE_SIZE;
+            if ( e820_map[i].addr <= LAPIC_ADDRESS &&
+                 e820_map[i].addr + e820_map[i].size > LAPIC_ADDRESS )
+                found_lapic = true;
+        }
+    }
+
+    if ( !found_lapic )
+        e820_insert_entry(LAPIC_ADDRESS, PAGE_SIZE, E820_RESERVED);
 }
 
 static void e820_get_memmap(void)
@@ -264,6 +275,12 @@ void e820_init_memmap(struct hvm_memmap_table_entry *entry, unsigned int num)
 {
     unsigned int i;
 
+    if ( !entry )
+    {
+        e820_get_memmap();
+        return;
+    }
+
     BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
     BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RESERVED != E820_RESERVED);
     BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
@@ -365,9 +382,6 @@ unsigned long e820_get_maxpfn(unsigned long pages)
     int i;
     unsigned long pfns = 0, start = 0;
 
-    if ( !e820_entries )
-        e820_get_memmap();
-
     for ( i = 0; i < e820_entries; i++ )
     {
         if ( e820_map[i].type != E820_RAM )
diff --git a/include/e820.h b/include/e820.h
index 5438a7c8..5533894e 100644
--- a/include/e820.h
+++ b/include/e820.h
@@ -51,6 +51,7 @@ struct __packed e820entry {
 
 extern struct e820entry e820_map[];
 extern unsigned e820_entries;
+extern unsigned int e820_initial_reserved_pfns;
 
 unsigned long e820_get_maxpfn(unsigned long pages);
 unsigned long e820_get_max_contig_pages(unsigned long pfn, unsigned long pages);
diff --git a/include/x86/arch_mm.h b/include/x86/arch_mm.h
index ffbec5a8..a1b975dc 100644
--- a/include/x86/arch_mm.h
+++ b/include/x86/arch_mm.h
@@ -207,6 +207,8 @@ typedef unsigned long pgentry_t;
 /* to align the pointer to the (next) page boundary */
 #define PAGE_ALIGN(addr)        (((addr)+PAGE_SIZE-1)&PAGE_MASK)
 
+#define LAPIC_ADDRESS	CONST(0xfee00000)
+
 #ifndef __ASSEMBLY__
 /* Definitions for machine and pseudophysical addresses. */
 #ifdef __i386__
-- 
2.35.3

Re: [PATCH 3/3] mini-os: fix number of pages for PVH

[Samuel Thibault]

Hello,

Juergen Gross, le sam. 18 juin 2022 12:48:16 +0200, a ecrit:
> @@ -124,7 +126,7 @@ void arch_mm_preinit(void *p)
>          do_exit();
>      }
>  
> -    last_free_pfn = e820_get_maxpfn(ret);
> +    last_free_pfn = e820_get_maxpfn(ret - e820_initial_reserved_pfns);

Mmm, but the reserved pfn could be in the middle of the e820 address
space.

Samuel

Re: [PATCH 1/3] mini-os: take newest version of arch-x86/hvm/start_info.h

[Samuel Thibault]

Juergen Gross, le sam. 18 juin 2022 12:48:14 +0200, a ecrit:
> Update include/xen/arch-x86/hvm/start_info.h to the newest version
> from the Xen tree.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  include/xen/arch-x86/hvm/start_info.h | 63 ++++++++++++++++++++++++++-
>  1 file changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/include/xen/arch-x86/hvm/start_info.h b/include/xen/arch-x86/hvm/start_info.h
> index 64841597..50af9ea2 100644
> --- a/include/xen/arch-x86/hvm/start_info.h
> +++ b/include/xen/arch-x86/hvm/start_info.h
> @@ -33,7 +33,7 @@
>   *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
>   *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>   *  4 +----------------+
> - *    | version        | Version of this structure. Current version is 0. New
> + *    | version        | Version of this structure. Current version is 1. New
>   *    |                | versions are guaranteed to be backwards-compatible.
>   *  8 +----------------+
>   *    | flags          | SIF_xxx flags.
> @@ -48,6 +48,15 @@
>   * 32 +----------------+
>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>   * 40 +----------------+
> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
> + *    |                | present in version 1 and newer of the structure.
> + * 48 +----------------+
> + *    | memmap_entries | Number of entries in the memory map table. Zero
> + *    |                | if there is no memory map being provided. Only
> + *    |                | present in version 1 and newer of the structure.
> + * 52 +----------------+
> + *    | reserved       | Version 1 and newer only.
> + * 56 +----------------+
>   *
>   * The layout of each entry in the module structure is the following:
>   *
> @@ -62,13 +71,51 @@
>   *    | reserved       |
>   * 32 +----------------+
>   *
> + * The layout of each entry in the memory map table is as follows:
> + *
> + *  0 +----------------+
> + *    | addr           | Base address
> + *  8 +----------------+
> + *    | size           | Size of mapping in bytes
> + * 16 +----------------+
> + *    | type           | Type of mapping as defined between the hypervisor
> + *    |                | and guest. See XEN_HVM_MEMMAP_TYPE_* values below.
> + * 20 +----------------|
> + *    | reserved       |
> + * 24 +----------------+
> + *
>   * The address and sizes are always a 64bit little endian unsigned integer.
>   *
>   * NB: Xen on x86 will always try to place all the data below the 4GiB
>   * boundary.
> + *
> + * Version numbers of the hvm_start_info structure have evolved like this:
> + *
> + * Version 0:  Initial implementation.
> + *
> + * Version 1:  Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
> + *             padding) to the end of the hvm_start_info struct. These new
> + *             fields can be used to pass a memory map to the guest. The
> + *             memory map is optional and so guests that understand version 1
> + *             of the structure must check that memmap_entries is non-zero
> + *             before trying to read the memory map.
>   */
>  #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
>  
> +/*
> + * The values used in the type field of the memory map table entries are
> + * defined below and match the Address Range Types as defined in the "System
> + * Address Map Interfaces" section of the ACPI Specification. Please refer to
> + * section 15 in version 6.2 of the ACPI spec: http://uefi.org/specifications
> + */
> +#define XEN_HVM_MEMMAP_TYPE_RAM       1
> +#define XEN_HVM_MEMMAP_TYPE_RESERVED  2
> +#define XEN_HVM_MEMMAP_TYPE_ACPI      3
> +#define XEN_HVM_MEMMAP_TYPE_NVS       4
> +#define XEN_HVM_MEMMAP_TYPE_UNUSABLE  5
> +#define XEN_HVM_MEMMAP_TYPE_DISABLED  6
> +#define XEN_HVM_MEMMAP_TYPE_PMEM      7
> +
>  /*
>   * C representation of the x86/HVM start info layout.
>   *
> @@ -86,6 +133,13 @@ struct hvm_start_info {
>      uint64_t cmdline_paddr;     /* Physical address of the command line.     */
>      uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
>                                  /* structure.                                */
> +    /* All following fields only present in version 1 and newer */
> +    uint64_t memmap_paddr;      /* Physical address of an array of           */
> +                                /* hvm_memmap_table_entry.                   */
> +    uint32_t memmap_entries;    /* Number of entries in the memmap table.    */
> +                                /* Value will be zero if there is no memory  */
> +                                /* map being provided.                       */
> +    uint32_t reserved;          /* Must be zero.                             */
>  };
>  
>  struct hvm_modlist_entry {
> @@ -95,4 +149,11 @@ struct hvm_modlist_entry {
>      uint64_t reserved;
>  };
>  
> +struct hvm_memmap_table_entry {
> +    uint64_t addr;              /* Base address of the memory region         */
> +    uint64_t size;              /* Size of the memory region in bytes        */
> +    uint32_t type;              /* Mapping type                              */
> +    uint32_t reserved;          /* Must be zero for Version 1.               */
> +};
> +
>  #endif /* __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ */
> -- 
> 2.35.3
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH 2/3] mini-os: prefer memory map via start_info for PVH

[Samuel Thibault]

Juergen Gross, le sam. 18 juin 2022 12:48:15 +0200, a ecrit:
> Since some time now a guest started in PVH mode will get the memory
> map from Xen via the start_info structure.
> 
> Modify the PVH initialization to prefer this memory map over the one
> obtained via hypercall, as this will allow to add information to the
> memory map for a new kernel when supporting kexec.
> 
> In case the start_info structure doesn't contain memory map information
> fall back to the hypercall.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  arch/x86/mm.c  |  6 ++++++
>  e820.c         | 25 +++++++++++++++++++++++++
>  include/e820.h |  4 ++++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/arch/x86/mm.c b/arch/x86/mm.c
> index 220c0b4d..41fcee67 100644
> --- a/arch/x86/mm.c
> +++ b/arch/x86/mm.c
> @@ -45,6 +45,7 @@
>  #include <mini-os/xmalloc.h>
>  #include <mini-os/e820.h>
>  #include <xen/memory.h>
> +#include <xen/arch-x86/hvm/start_info.h>
>  
>  #ifdef MM_DEBUG
>  #define DEBUG(_f, _a...) \
> @@ -108,6 +109,11 @@ void arch_mm_preinit(void *p)
>  {
>      long ret;
>      domid_t domid = DOMID_SELF;
> +    struct hvm_start_info *hsi = p;
> +
> +    if ( hsi->version >= 1 && hsi->memmap_entries > 0 )
> +        e820_init_memmap((struct hvm_memmap_table_entry *)(unsigned long)
> +                         hsi->memmap_paddr, hsi->memmap_entries);
>  
>      pt_base = page_table_base;
>      first_free_pfn = PFN_UP(to_phys(&_end));
> diff --git a/e820.c b/e820.c
> index 991ed382..ad91e00b 100644
> --- a/e820.c
> +++ b/e820.c
> @@ -54,6 +54,7 @@ static char *e820_types[E820_TYPES] = {
>      [E820_ACPI]     = "ACPI",
>      [E820_NVS]      = "NVS",
>      [E820_UNUSABLE] = "Unusable",
> +    [E820_DISABLED] = "Disabled",
>      [E820_PMEM]     = "PMEM"
>  };
>  
> @@ -259,6 +260,30 @@ static void e820_get_memmap(void)
>      e820_sanitize();
>  }
>  
> +void e820_init_memmap(struct hvm_memmap_table_entry *entry, unsigned int num)
> +{
> +    unsigned int i;
> +
> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RESERVED != E820_RESERVED);
> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_NVS != E820_NVS);
> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_UNUSABLE != E820_UNUSABLE);
> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_DISABLED != E820_DISABLED);
> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_PMEM != E820_PMEM);
> +
> +    for ( i = 0; i < num; i++ )
> +    {
> +        e820_map[i].addr = entry[i].addr;
> +        e820_map[i].size = entry[i].size;
> +        e820_map[i].type = entry[i].type;
> +    }
> +
> +    e820_entries = num;
> +
> +    e820_sanitize();
> +}
> +
>  void arch_print_memmap(void)
>  {
>      int i;
> diff --git a/include/e820.h b/include/e820.h
> index aaf2f2ca..5438a7c8 100644
> --- a/include/e820.h
> +++ b/include/e820.h
> @@ -26,6 +26,8 @@
>  
>  #if defined(__arm__) || defined(__aarch64__) || defined(CONFIG_PARAVIRT)
>  #define CONFIG_E820_TRIVIAL
> +#else
> +#include <xen/arch-x86/hvm/start_info.h>
>  #endif
>  
>  /* PC BIOS standard E820 types and structure. */
> @@ -34,6 +36,7 @@
>  #define E820_ACPI         3
>  #define E820_NVS          4
>  #define E820_UNUSABLE     5
> +#define E820_DISABLED     6
>  #define E820_PMEM         7
>  #define E820_TYPES        8
>  
> @@ -54,6 +57,7 @@ unsigned long e820_get_max_contig_pages(unsigned long pfn, unsigned long pages);
>  #ifndef CONFIG_E820_TRIVIAL
>  unsigned long e820_get_reserved_pfns(int pages);
>  void e820_put_reserved_pfns(unsigned long start_pfn, int pages);
> +void e820_init_memmap(struct hvm_memmap_table_entry *entry, unsigned int num);
>  #endif
>  
>  #endif /*__E820_HEADER*/
> -- 
> 2.35.3
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH 3/3] mini-os: fix number of pages for PVH

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 816 bytes --]

On 18.06.22 14:13, Samuel Thibault wrote:
> Hello,
> 
> Juergen Gross, le sam. 18 juin 2022 12:48:16 +0200, a ecrit:
>> @@ -124,7 +126,7 @@ void arch_mm_preinit(void *p)
>>           do_exit();
>>       }
>>   
>> -    last_free_pfn = e820_get_maxpfn(ret);
>> +    last_free_pfn = e820_get_maxpfn(ret - e820_initial_reserved_pfns);
> 
> Mmm, but the reserved pfn could be in the middle of the e820 address
> space.

That doesn't matter.

e820_get_maxpfn(n) will just return the pfn of the n-th RAM pfn it is
finding in the E820 map. This should be the last pfn with allocated
memory. Without subtracting the number of reserved pfns (which contain
normally memory which is allocated for the guest, but not usable as
RAM), Mini-OS tries to use RAM beyond its allocation, which fails.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 3/3] mini-os: fix number of pages for PVH

[Samuel Thibault]

Juergen Gross, le sam. 18 juin 2022 16:07:07 +0200, a ecrit:
> On 18.06.22 14:13, Samuel Thibault wrote:
> > Hello,
> > 
> > Juergen Gross, le sam. 18 juin 2022 12:48:16 +0200, a ecrit:
> > > @@ -124,7 +126,7 @@ void arch_mm_preinit(void *p)
> > >           do_exit();
> > >       }
> > > -    last_free_pfn = e820_get_maxpfn(ret);
> > > +    last_free_pfn = e820_get_maxpfn(ret - e820_initial_reserved_pfns);
> > 
> > Mmm, but the reserved pfn could be in the middle of the e820 address
> > space.
> 
> That doesn't matter.
> 
> e820_get_maxpfn(n) will just return the pfn of the n-th RAM pfn it is
> finding in the E820 map.

Yes, but subtracting at this point looks a bit hacky to me.

It seems to me that it'd be better to make e820_get_maxpfn count by
itself the reserved pages (but never return its pfn of course), rather
than having to make e820_sanitize look at the reserved pages, store
it somewhere, and hope that other code will remember to subtract that
before calling e820_get_maxpfn.

I mean something like:

unsigned long e820_get_maxpfn(unsigned long pages)
{
    int i;
    unsigned long pfns = 0, start = 0;

    if ( !e820_entries )
        e820_get_memmap();

    for ( i = 0; i < e820_entries; i++ )
    {
        pfns = e820_map[i].size >> PAGE_SHIFT;

	if ( e820_map[i].type == E820_RESERVED )
	{
	    /* This counts in the memory reservation, but is not usable */
            pages -= pfns;
	    continue;
	}
        if ( e820_map[i].type != E820_RAM )
            continue;

        start = e820_map[i].addr >> PAGE_SHIFT;
        if ( pages <= pfns )
            return start + pages;
        pages -= pfns;
    }

    return start + pfns;
}

Samuel

Re: [PATCH 3/3] mini-os: fix number of pages for PVH

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2144 bytes --]

On 18.06.22 17:56, Samuel Thibault wrote:
> Juergen Gross, le sam. 18 juin 2022 16:07:07 +0200, a ecrit:
>> On 18.06.22 14:13, Samuel Thibault wrote:
>>> Hello,
>>>
>>> Juergen Gross, le sam. 18 juin 2022 12:48:16 +0200, a ecrit:
>>>> @@ -124,7 +126,7 @@ void arch_mm_preinit(void *p)
>>>>            do_exit();
>>>>        }
>>>> -    last_free_pfn = e820_get_maxpfn(ret);
>>>> +    last_free_pfn = e820_get_maxpfn(ret - e820_initial_reserved_pfns);
>>>
>>> Mmm, but the reserved pfn could be in the middle of the e820 address
>>> space.
>>
>> That doesn't matter.
>>
>> e820_get_maxpfn(n) will just return the pfn of the n-th RAM pfn it is
>> finding in the E820 map.
> 
> Yes, but subtracting at this point looks a bit hacky to me.
> 
> It seems to me that it'd be better to make e820_get_maxpfn count by
> itself the reserved pages (but never return its pfn of course), rather
> than having to make e820_sanitize look at the reserved pages, store
> it somewhere, and hope that other code will remember to subtract that
> before calling e820_get_maxpfn.
> 
> I mean something like:
> 
> unsigned long e820_get_maxpfn(unsigned long pages)
> {
>      int i;
>      unsigned long pfns = 0, start = 0;
> 
>      if ( !e820_entries )
>          e820_get_memmap();
> 
>      for ( i = 0; i < e820_entries; i++ )
>      {
>          pfns = e820_map[i].size >> PAGE_SHIFT;
> 
> 	if ( e820_map[i].type == E820_RESERVED )
> 	{
> 	    /* This counts in the memory reservation, but is not usable */
>              pages -= pfns;
> 	    continue;
> 	}
>          if ( e820_map[i].type != E820_RAM )
>              continue;
> 
>          start = e820_map[i].addr >> PAGE_SHIFT;
>          if ( pages <= pfns )
>              return start + pages;
>          pages -= pfns;
>      }
> 
>      return start + pfns;
> }

This would lead to wrong values of nr_mem_pages. I think the best solution
would be to have functions returning the number of available and max RAM
pages to e820.c. This would address your valid concern, while not leading
to wrong values at the callers side.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]