[PATCH 00/10] mini-os: add missing PVH features

[PATCH 00/10] mini-os: add missing PVH features

[Juergen Gross]

Mini-OS in PVH mode is missing some features, especially in the areas
of ballooning and grant tables.

With this series I am able to run Xenstore stubdom in PVH mode.

Juergen Gross (10):
  mini-os: split e820 map handling into new source file
  mini-os: sort and sanitize e820 memory map
  mini-os: don't assume contiguous RAM when initializing in PVH mode
  mini-os: respect memory map when ballooning up
  mini-os: don't repeat definition available via header file
  mini-os: add memory map service functions
  mini-os: move x86 specific gnttab coding into arch/x86/gnttab.c
  mini-os: add proper pvh grant table handling
  mini-os: prepare grantmap entry interface for use by PVH mode
  mini-os: modify grant mappings to work in PVH mode

 Makefile           |   1 +
 arch/arm/mm.c      |  11 +-
 arch/x86/balloon.c |   4 +-
 arch/x86/gnttab.c  | 109 ++++++++++++++++++
 arch/x86/mm.c      | 121 +-------------------
 arch/x86/setup.c   |   8 +-
 balloon.c          |  33 ++++--
 e820.c             | 269 +++++++++++++++++++++++++++++++++++++++++++++
 gntmap.c           | 125 +++++++++++++--------
 include/balloon.h  |   5 +-
 include/e820.h     |  11 ++
 include/gntmap.h   |   1 +
 mm.c               |   7 +-
 13 files changed, 508 insertions(+), 197 deletions(-)
 create mode 100644 arch/x86/gnttab.c
 create mode 100644 e820.c

-- 
2.26.2

[PATCH 01/10] mini-os: split e820 map handling into new source file

[Juergen Gross]

Introduce e820.c containing all the E820 memory map handling.

No functional change.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 Makefile       |   1 +
 arch/arm/mm.c  |   8 ----
 arch/x86/mm.c  |  70 +----------------------------
 e820.c         | 119 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/e820.h |   6 +++
 5 files changed, 128 insertions(+), 76 deletions(-)
 create mode 100644 e820.c

diff --git a/Makefile b/Makefile
index 4b76b55..06b60fc 100644
--- a/Makefile
+++ b/Makefile
@@ -41,6 +41,7 @@ src-$(CONFIG_TPMFRONT) += tpmfront.c
 src-$(CONFIG_TPM_TIS) += tpm_tis.c
 src-$(CONFIG_TPMBACK) += tpmback.c
 src-y += daytime.c
+src-y += e820.c
 src-y += events.c
 src-$(CONFIG_FBFRONT) += fbfront.c
 src-y += gntmap.c
diff --git a/arch/arm/mm.c b/arch/arm/mm.c
index f806c9f..9068166 100644
--- a/arch/arm/mm.c
+++ b/arch/arm/mm.c
@@ -7,14 +7,6 @@
 #include <lib.h>
 
 uint32_t physical_address_offset;
-struct e820entry e820_map[1] = {
-    {
-        .addr = 0,
-        .size = ULONG_MAX - 1,
-        .type = E820_RAM
-    }
-};
-unsigned e820_entries = 1;
 
 unsigned long allocate_ondemand(unsigned long n, unsigned long alignment)
 {
diff --git a/arch/x86/mm.c b/arch/x86/mm.c
index 8ba14a5..8df93da 100644
--- a/arch/x86/mm.c
+++ b/arch/x86/mm.c
@@ -64,15 +64,6 @@ extern char stack[];
 extern void page_walk(unsigned long va);
 
 #ifdef CONFIG_PARAVIRT
-struct e820entry e820_map[1] = {
-    {
-        .addr = 0,
-        .size = ULONG_MAX - 1,
-        .type = E820_RAM
-    }
-};
-unsigned e820_entries = 1;
-
 void arch_mm_preinit(void *p)
 {
     start_info_t *si = p;
@@ -112,25 +103,11 @@ desc_ptr idt_ptr =
     .base = (unsigned long)&idt,
 };
 
-struct e820entry e820_map[E820_MAX];
-unsigned e820_entries;
-
-static char *e820_types[E820_TYPES] = {
-    [E820_RAM]      = "RAM",
-    [E820_RESERVED] = "Reserved",
-    [E820_ACPI]     = "ACPI",
-    [E820_NVS]      = "NVS",
-    [E820_UNUSABLE] = "Unusable",
-    [E820_PMEM]     = "PMEM"
-};
-
 void arch_mm_preinit(void *p)
 {
     long ret;
     domid_t domid = DOMID_SELF;
-    struct xen_memory_map memmap;
-    int i;
-    unsigned long pfn, max = 0;
+    unsigned long max;
 
     pt_base = page_table_base;
     first_free_pfn = PFN_UP(to_phys(&_end));
@@ -142,53 +119,10 @@ void arch_mm_preinit(void *p)
     }
     last_free_pfn = ret;
 
-    memmap.nr_entries = E820_MAX;
-    set_xen_guest_handle(memmap.buffer, e820_map);
-    ret = HYPERVISOR_memory_op(XENMEM_memory_map, &memmap);
-    if ( ret < 0 )
-    {
-        xprintk("could not get memory map\n");
-        do_exit();
-    }
-    e820_entries = memmap.nr_entries;
-
-    for ( i = 0; i < e820_entries; i++ )
-    {
-        if ( e820_map[i].type != E820_RAM )
-            continue;
-        pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
-        if ( pfn > max )
-            max = pfn;
-    }
-
+    max = e820_get_maxpfn();
     if ( max < last_free_pfn )
         last_free_pfn = max;
 }
-
-void arch_print_memmap(void)
-{
-    int i;
-    unsigned long from, to;
-    char *type;
-    char buf[12];
-
-    printk("Memory map:\n");
-    for ( i = 0; i < e820_entries; i++ )
-    {
-        if ( e820_map[i].type >= E820_TYPES || !e820_types[e820_map[i].type] )
-        {
-            snprintf(buf, sizeof(buf), "%8x", e820_map[i].type);
-            type = buf;
-        }
-        else
-        {
-            type = e820_types[e820_map[i].type];
-        }
-        from = e820_map[i].addr;
-        to = from + e820_map[i].size - 1;
-        printk("%012lx-%012lx: %s\n", from, to, type);
-    }
-}
 #endif
 
 /*
diff --git a/e820.c b/e820.c
new file mode 100644
index 0000000..2165280
--- /dev/null
+++ b/e820.c
@@ -0,0 +1,119 @@
+/* -*-  Mode:C; c-basic-offset:4; tab-width:4 -*-
+ *
+ * (C) 2021 - Juergen Gross, SUSE Software Solutions Germany GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include <mini-os/types.h>
+#include <mini-os/lib.h>
+#include <mini-os/console.h>
+#include <mini-os/os.h>
+#include <mini-os/posix/limits.h>
+#include <mini-os/e820.h>
+#include <xen/memory.h>
+
+#ifdef CONFIG_E820_TRIVIAL
+struct e820entry e820_map[1] = {
+    {
+        .addr = 0,
+        .size = ULONG_MAX - 1,
+        .type = E820_RAM
+    }
+};
+
+unsigned e820_entries = 1;
+
+static void e820_get_memmap(void)
+{
+}
+
+#else
+struct e820entry e820_map[E820_MAX];
+unsigned e820_entries;
+
+static char *e820_types[E820_TYPES] = {
+    [E820_RAM]      = "RAM",
+    [E820_RESERVED] = "Reserved",
+    [E820_ACPI]     = "ACPI",
+    [E820_NVS]      = "NVS",
+    [E820_UNUSABLE] = "Unusable",
+    [E820_PMEM]     = "PMEM"
+};
+
+static void e820_get_memmap(void)
+{
+    long ret;
+    struct xen_memory_map memmap;
+
+    memmap.nr_entries = E820_MAX;
+    set_xen_guest_handle(memmap.buffer, e820_map);
+    ret = HYPERVISOR_memory_op(XENMEM_memory_map, &memmap);
+    if ( ret < 0 )
+    {
+        xprintk("could not get memory map\n");
+        do_exit();
+    }
+    e820_entries = memmap.nr_entries;
+}
+
+void arch_print_memmap(void)
+{
+    int i;
+    unsigned long from, to;
+    char *type;
+    char buf[12];
+
+    printk("Memory map:\n");
+    for ( i = 0; i < e820_entries; i++ )
+    {
+        if ( e820_map[i].type >= E820_TYPES || !e820_types[e820_map[i].type] )
+        {
+            snprintf(buf, sizeof(buf), "%8x", e820_map[i].type);
+            type = buf;
+        }
+        else
+        {
+            type = e820_types[e820_map[i].type];
+        }
+        from = e820_map[i].addr;
+        to = from + e820_map[i].size - 1;
+        printk("%012lx-%012lx: %s\n", from, to, type);
+    }
+}
+#endif
+
+unsigned long e820_get_maxpfn(void)
+{
+    int i;
+    unsigned long pfn, max = 0;
+
+    e820_get_memmap();
+
+    for ( i = 0; i < e820_entries; i++ )
+    {
+        if ( e820_map[i].type != E820_RAM )
+            continue;
+        pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
+        if ( pfn > max )
+            max = pfn;
+    }
+
+    return max;
+}
diff --git a/include/e820.h b/include/e820.h
index 920551c..af2129f 100644
--- a/include/e820.h
+++ b/include/e820.h
@@ -24,6 +24,10 @@
 #ifndef __E820_HEADER
 #define __E820_HEADER
 
+#if defined(__arm__) || defined(__aarch64__) || defined(CONFIG_PARAVIRT)
+#define CONFIG_E820_TRIVIAL
+#endif
+
 /* PC BIOS standard E820 types and structure. */
 #define E820_RAM          1
 #define E820_RESERVED     2
@@ -45,4 +49,6 @@ struct __packed e820entry {
 extern struct e820entry e820_map[];
 extern unsigned e820_entries;
 
+unsigned long e820_get_maxpfn(void);
+
 #endif /*__E820_HEADER*/
-- 
2.26.2

[PATCH 02/10] mini-os: sort and sanitize e820 memory map

[Juergen Gross]

Do some processing of the E820 memory map obtained from the hypervisor:

- align the entries to page boundaries
- sort the entries by their start address
- merge adjacent entries of same type

This is relevant for PVH mode only.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 e820.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/e820.c b/e820.c
index 2165280..336a8b8 100644
--- a/e820.c
+++ b/e820.c
@@ -57,6 +57,60 @@ static char *e820_types[E820_TYPES] = {
     [E820_PMEM]     = "PMEM"
 };
 
+static void e820_remove_entry(int idx)
+{
+    int i;
+
+    e820_entries--;
+    for ( i = idx; i < e820_entries; i++ )
+        e820_map[i] = e820_map[i + 1];
+}
+
+static void e820_swap_entries(int idx1, int idx2)
+{
+    struct e820entry entry;
+
+    entry = e820_map[idx1];
+    e820_map[idx1] = e820_map[idx2];
+    e820_map[idx2] = entry;
+}
+
+static void e820_sanitize(void)
+{
+    int i;
+    unsigned long end;
+
+    /* Adjust map entries to page boundaries. */
+    for ( i = 0; i < e820_entries; i++ )
+    {
+        end = (e820_map[i].addr + e820_map[i].size + PAGE_SIZE - 1) & PAGE_MASK;
+        e820_map[i].addr &= PAGE_MASK;
+        e820_map[i].size = end - e820_map[i].addr;
+    }
+
+    /* Sort entries by start address. */
+    for ( i = 0; i < e820_entries - 1; i++ )
+    {
+        if ( e820_map[i].addr > e820_map[i + 1].addr )
+        {
+            e820_swap_entries(i, i + 1);
+            i = -1;
+        }
+    }
+
+    /* Merge adjacent entries of same type. */
+    for ( i = 0; i < e820_entries - 1; i++ )
+    {
+        if ( e820_map[i].type == e820_map[i + 1].type &&
+             e820_map[i].addr + e820_map[i].size == e820_map[i + 1].addr )
+        {
+            e820_map[i].size += e820_map[i + 1].size;
+            e820_remove_entry(i + 1);
+            i--;
+        }
+    }
+}
+
 static void e820_get_memmap(void)
 {
     long ret;
@@ -71,6 +125,8 @@ static void e820_get_memmap(void)
         do_exit();
     }
     e820_entries = memmap.nr_entries;
+
+    e820_sanitize();
 }
 
 void arch_print_memmap(void)
-- 
2.26.2

[PATCH 03/10] mini-os: don't assume contiguous RAM when initializing in PVH mode

[Juergen Gross]

Sizing the available memory should respect memory holes, so look at
the memory map when setting the boundary for the memory allocator.

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

diff --git a/arch/x86/mm.c b/arch/x86/mm.c
index 8df93da..3bf6170 100644
--- a/arch/x86/mm.c
+++ b/arch/x86/mm.c
@@ -107,7 +107,6 @@ void arch_mm_preinit(void *p)
 {
     long ret;
     domid_t domid = DOMID_SELF;
-    unsigned long max;
 
     pt_base = page_table_base;
     first_free_pfn = PFN_UP(to_phys(&_end));
@@ -117,11 +116,8 @@ void arch_mm_preinit(void *p)
         xprintk("could not get memory size\n");
         do_exit();
     }
-    last_free_pfn = ret;
 
-    max = e820_get_maxpfn();
-    if ( max < last_free_pfn )
-        last_free_pfn = max;
+    last_free_pfn = e820_get_maxpfn(ret);
 }
 #endif
 
diff --git a/e820.c b/e820.c
index 336a8b8..14fd3cd 100644
--- a/e820.c
+++ b/e820.c
@@ -155,10 +155,10 @@ void arch_print_memmap(void)
 }
 #endif
 
-unsigned long e820_get_maxpfn(void)
+unsigned long e820_get_maxpfn(unsigned long pages)
 {
     int i;
-    unsigned long pfn, max = 0;
+    unsigned long pfns, max = 0;
 
     e820_get_memmap();
 
@@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void)
     {
         if ( e820_map[i].type != E820_RAM )
             continue;
-        pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
-        if ( pfn > max )
-            max = pfn;
+        pfns = e820_map[i].size >> PAGE_SHIFT;
+        max = e820_map[i].addr >> PAGE_SHIFT;
+        if ( pages <= pfns )
+            return max + pages;
+        pages -= pfns;
+        max += pfns;
     }
 
     return max;
diff --git a/include/e820.h b/include/e820.h
index af2129f..6a57f05 100644
--- a/include/e820.h
+++ b/include/e820.h
@@ -49,6 +49,6 @@ struct __packed e820entry {
 extern struct e820entry e820_map[];
 extern unsigned e820_entries;
 
-unsigned long e820_get_maxpfn(void);
+unsigned long e820_get_maxpfn(unsigned long pages);
 
 #endif /*__E820_HEADER*/
-- 
2.26.2

[PATCH 04/10] mini-os: respect memory map when ballooning up

[Juergen Gross]

Today Mini-OS won't look at the memory map when ballooning up. This can
result in problems for PVH domains with more than 4 GB of RAM, as
ballooning will happily run into the ACPI area.

Fix that by adding only pages being marked as RAM in the memory map and
by distinguishing between the current number of RAM pages and the first
unallocated page.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/arm/mm.c      |  3 +++
 arch/x86/balloon.c |  4 ++--
 arch/x86/mm.c      |  2 ++
 balloon.c          | 33 ++++++++++++++++++++++++---------
 e820.c             | 21 ++++++++++++++++++++-
 include/balloon.h  |  5 +++--
 include/e820.h     |  1 +
 mm.c               |  7 ++-----
 8 files changed, 57 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mm.c b/arch/arm/mm.c
index 9068166..11962f8 100644
--- a/arch/arm/mm.c
+++ b/arch/arm/mm.c
@@ -3,6 +3,7 @@
 #include <arch_mm.h>
 #include <mini-os/errno.h>
 #include <mini-os/hypervisor.h>
+#include <mini-os/balloon.h>
 #include <libfdt.h>
 #include <lib.h>
 
@@ -70,6 +71,8 @@ void arch_init_mm(unsigned long *start_pfn_p, unsigned long *max_pfn_p)
     }
     device_tree = new_device_tree;
     *max_pfn_p = to_phys(new_device_tree) >> PAGE_SHIFT;
+
+    balloon_set_nr_pages(*max_pfn_p, *max_pfn_p);
 }
 
 void arch_init_demand_mapping_area(void)
diff --git a/arch/x86/balloon.c b/arch/x86/balloon.c
index 10b440c..fe79644 100644
--- a/arch/x86/balloon.c
+++ b/arch/x86/balloon.c
@@ -61,10 +61,10 @@ void arch_remap_p2m(unsigned long max_pfn)
     p2m_invalidate(l2_list, L2_P2M_IDX(max_pfn - 1) + 1);
     p2m_invalidate(l1_list, L1_P2M_IDX(max_pfn - 1) + 1);
 
-    if ( p2m_pages(nr_max_pages) <= p2m_pages(max_pfn) )
+    if ( p2m_pages(nr_max_pfn) <= p2m_pages(max_pfn) )
         return;
 
-    new_p2m = alloc_virt_kernel(p2m_pages(nr_max_pages));
+    new_p2m = alloc_virt_kernel(p2m_pages(nr_max_pfn));
     for ( pfn = 0; pfn < max_pfn; pfn += P2M_ENTRIES )
     {
         map_frame_rw(new_p2m + PAGE_SIZE * (pfn / P2M_ENTRIES),
diff --git a/arch/x86/mm.c b/arch/x86/mm.c
index 3bf6170..c30d8bc 100644
--- a/arch/x86/mm.c
+++ b/arch/x86/mm.c
@@ -72,6 +72,7 @@ void arch_mm_preinit(void *p)
     pt_base = (pgentry_t *)si->pt_base;
     first_free_pfn = PFN_UP(to_phys(pt_base)) + si->nr_pt_frames;
     last_free_pfn = si->nr_pages;
+    balloon_set_nr_pages(last_free_pfn, last_free_pfn);
 }
 #else
 #include <mini-os/desc.h>
@@ -118,6 +119,7 @@ void arch_mm_preinit(void *p)
     }
 
     last_free_pfn = e820_get_maxpfn(ret);
+    balloon_set_nr_pages(ret, last_free_pfn);
 }
 #endif
 
diff --git a/balloon.c b/balloon.c
index 5676d3b..80d89c7 100644
--- a/balloon.c
+++ b/balloon.c
@@ -23,14 +23,24 @@
 
 #include <mini-os/os.h>
 #include <mini-os/balloon.h>
+#include <mini-os/e820.h>
 #include <mini-os/errno.h>
 #include <mini-os/lib.h>
 #include <mini-os/paravirt.h>
 #include <xen/xen.h>
 #include <xen/memory.h>
 
-unsigned long nr_max_pages;
-unsigned long nr_mem_pages;
+unsigned long nr_max_pfn;
+
+static unsigned long nr_max_pages;
+static unsigned long nr_mem_pfn;
+static unsigned long nr_mem_pages;
+
+void balloon_set_nr_pages(unsigned long pages, unsigned long pfn)
+{
+    nr_mem_pages = pages;
+    nr_mem_pfn = pfn;
+}
 
 void get_max_pages(void)
 {
@@ -46,16 +56,18 @@ void get_max_pages(void)
 
     nr_max_pages = ret;
     printk("Maximum memory size: %ld pages\n", nr_max_pages);
+
+    nr_max_pfn = e820_get_maxpfn(nr_max_pages);
 }
 
 void mm_alloc_bitmap_remap(void)
 {
     unsigned long i, new_bitmap;
 
-    if ( mm_alloc_bitmap_size >= ((nr_max_pages + 1) >> 3) )
+    if ( mm_alloc_bitmap_size >= ((nr_max_pfn + 1) >> 3) )
         return;
 
-    new_bitmap = alloc_virt_kernel(PFN_UP((nr_max_pages + 1) >> 3));
+    new_bitmap = alloc_virt_kernel(PFN_UP((nr_max_pfn + 1) >> 3));
     for ( i = 0; i < mm_alloc_bitmap_size; i += PAGE_SIZE )
     {
         map_frame_rw(new_bitmap + i,
@@ -70,7 +82,7 @@ static unsigned long balloon_frames[N_BALLOON_FRAMES];
 
 int balloon_up(unsigned long n_pages)
 {
-    unsigned long page, pfn;
+    unsigned long page, pfn, start_pfn;
     int rc;
     struct xen_memory_reservation reservation = {
         .domid        = DOMID_SELF
@@ -81,8 +93,11 @@ int balloon_up(unsigned long n_pages)
     if ( n_pages > N_BALLOON_FRAMES )
         n_pages = N_BALLOON_FRAMES;
 
+    start_pfn = e820_get_maxpfn(nr_mem_pages + 1) - 1;
+    n_pages = e820_get_max_pages(start_pfn, n_pages);
+
     /* Resize alloc_bitmap if necessary. */
-    while ( mm_alloc_bitmap_size * 8 < nr_mem_pages + n_pages )
+    while ( mm_alloc_bitmap_size * 8 < start_pfn + n_pages )
     {
         page = alloc_page();
         if ( !page )
@@ -99,14 +114,14 @@ int balloon_up(unsigned long n_pages)
         mm_alloc_bitmap_size += PAGE_SIZE;
     }
 
-    rc = arch_expand_p2m(nr_mem_pages + n_pages);
+    rc = arch_expand_p2m(start_pfn + n_pages);
     if ( rc )
         return rc;
 
     /* Get new memory from hypervisor. */
     for ( pfn = 0; pfn < n_pages; pfn++ )
     {
-        balloon_frames[pfn] = nr_mem_pages + pfn;
+        balloon_frames[pfn] = start_pfn + pfn;
     }
     set_xen_guest_handle(reservation.extent_start, balloon_frames);
     reservation.nr_extents = n_pages;
@@ -116,7 +131,7 @@ int balloon_up(unsigned long n_pages)
 
     for ( pfn = 0; pfn < rc; pfn++ )
     {
-        arch_pfn_add(nr_mem_pages + pfn, balloon_frames[pfn]);
+        arch_pfn_add(start_pfn + pfn, balloon_frames[pfn]);
         free_page(pfn_to_virt(nr_mem_pages + pfn));
     }
 
diff --git a/e820.c b/e820.c
index 14fd3cd..50029bb 100644
--- a/e820.c
+++ b/e820.c
@@ -160,7 +160,8 @@ unsigned long e820_get_maxpfn(unsigned long pages)
     int i;
     unsigned long pfns, max = 0;
 
-    e820_get_memmap();
+    if ( !e820_entries )
+        e820_get_memmap();
 
     for ( i = 0; i < e820_entries; i++ )
     {
@@ -176,3 +177,21 @@ unsigned long e820_get_maxpfn(unsigned long pages)
 
     return max;
 }
+
+unsigned long e820_get_max_pages(unsigned long pfn, unsigned long pages)
+{
+    int i;
+    unsigned long end;
+
+    for ( i = 0; i < e820_entries; i++ )
+    {
+        if ( e820_map[i].type != E820_RAM ||
+             (e820_map[i].addr >> PAGE_SHIFT) > pfn )
+            continue;
+
+        end = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
+        return ((end - pfn) > pages) ? pages : end - pfn;
+    }
+
+    return 0;
+}
diff --git a/include/balloon.h b/include/balloon.h
index 6cfec4f..8f7c8bd 100644
--- a/include/balloon.h
+++ b/include/balloon.h
@@ -32,11 +32,11 @@
  */
 #define BALLOON_EMERGENCY_PAGES   64
 
-extern unsigned long nr_max_pages;
-extern unsigned long nr_mem_pages;
+extern unsigned long nr_max_pfn;
 
 void get_max_pages(void);
 int balloon_up(unsigned long n_pages);
+void balloon_set_nr_pages(unsigned long pages, unsigned long pfn);
 
 void mm_alloc_bitmap_remap(void);
 void arch_pfn_add(unsigned long pfn, unsigned long mfn);
@@ -50,6 +50,7 @@ static inline int chk_free_pages(unsigned long needed)
 {
     return needed <= nr_free_pages;
 }
+static inline balloon_set_nr_pages(unsigned long pages, unsigned long pfn) { }
 
 #endif /* CONFIG_BALLOON */
 #endif /* _BALLOON_H_ */
diff --git a/include/e820.h b/include/e820.h
index 6a57f05..49daefa 100644
--- a/include/e820.h
+++ b/include/e820.h
@@ -50,5 +50,6 @@ extern struct e820entry e820_map[];
 extern unsigned e820_entries;
 
 unsigned long e820_get_maxpfn(unsigned long pages);
+unsigned long e820_get_max_pages(unsigned long pfn, unsigned long pages);
 
 #endif /*__E820_HEADER*/
diff --git a/mm.c b/mm.c
index 932ceeb..6493bdd 100644
--- a/mm.c
+++ b/mm.c
@@ -396,8 +396,9 @@ void init_mm(void)
 
     printk("MM: Init\n");
 
-    get_max_pages();
     arch_init_mm(&start_pfn, &max_pfn);
+    get_max_pages();
+
     /*
      * now we can initialise the page allocator
      */
@@ -407,10 +408,6 @@ void init_mm(void)
     arch_init_p2m(max_pfn);
     
     arch_init_demand_mapping_area();
-
-#ifdef CONFIG_BALLOON
-    nr_mem_pages = max_pfn;
-#endif
 }
 
 void fini_mm(void)
-- 
2.26.2

[PATCH 05/10] mini-os: don't repeat definition available via header file

[Juergen Gross]

arch/x86/setup.c is repeating the definition of __pte() instead using
the appropriate header. Fix that.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/setup.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/x86/setup.c b/arch/x86/setup.c
index 1ec68d3..b27bbed 100644
--- a/arch/x86/setup.c
+++ b/arch/x86/setup.c
@@ -29,6 +29,7 @@
 #include <mini-os/os.h>
 #include <mini-os/lib.h> /* for printk, memcpy */
 #include <mini-os/kernel.h>
+#include <mini-os/hypervisor.h>
 #include <xen/xen.h>
 #include <xen/arch-x86/cpuid.h>
 #include <xen/arch-x86/hvm/start_info.h>
@@ -61,13 +62,6 @@ char stack[2*STACK_SIZE];
 
 extern char shared_info[PAGE_SIZE];
 
-#if defined(__x86_64__)
-#define __pte(x) ((pte_t) { (x) } )
-#else
-#define __pte(x) ({ unsigned long long _x = (x);        \
-    ((pte_t) {(unsigned long)(_x), (unsigned long)(_x>>32)}); })
-#endif
-
 static inline void fpu_init(void) {
 	asm volatile("fninit");
 }
-- 
2.26.2

[PATCH 06/10] mini-os: add memory map service functions

[Juergen Gross]

Add two functions for adding reserved areas to the memory map and
for removing them again.

Those will be needed for proper grant table/mapping support in PVH
mode.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 e820.c         | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/e820.h |  4 +++
 2 files changed, 76 insertions(+)

diff --git a/e820.c b/e820.c
index 50029bb..2888932 100644
--- a/e820.c
+++ b/e820.c
@@ -66,6 +66,21 @@ static void e820_remove_entry(int idx)
         e820_map[i] = e820_map[i + 1];
 }
 
+static void e820_insert_entry(int idx)
+{
+    int i;
+
+    if ( e820_entries == E820_MAX )
+    {
+        xprintk("E820 memory map overflow\n");
+        do_exit();
+    }
+
+    e820_entries++;
+    for ( i = e820_entries - 1; i > idx; i-- )
+        e820_map[i] = e820_map[i - 1];
+}
+
 static void e820_swap_entries(int idx1, int idx2)
 {
     struct e820entry entry;
@@ -153,6 +168,63 @@ void arch_print_memmap(void)
         printk("%012lx-%012lx: %s\n", from, to, type);
     }
 }
+
+unsigned long e820_get_reserved_pfns(int pages)
+{
+    int i;
+    unsigned long last = 0, needed = (long)pages << PAGE_SHIFT;
+
+    for ( i = 0; i < e820_entries && e820_map[i].addr < last + needed; i++ )
+        last = e820_map[i].addr + e820_map[i].size;
+
+    if ( i == 0 || e820_map[i - 1].type != E820_RESERVED )
+    {
+        e820_insert_entry(i);
+        e820_map[i].addr = last;
+        e820_map[i].size = needed;
+        e820_map[i].type = E820_RESERVED;
+    }
+    else
+    {
+        e820_map[i - 1].size += needed;
+    }
+
+    return last >> PAGE_SHIFT;
+}
+
+void e820_put_reserved_pfns(unsigned long start_pfn, int pages)
+{
+    int i;
+    unsigned long addr = start_pfn << PAGE_SHIFT;
+    unsigned long size = (long)pages << PAGE_SHIFT;
+
+    for ( i = 0; i < e820_entries && addr < e820_map[i].addr; i++ );
+
+    BUG_ON(i == e820_entries || e820_map[i].type != E820_RESERVED);
+
+    if ( addr == e820_map[i].addr )
+    {
+        e820_map[i].addr += size;
+        e820_map[i].size -= size;
+        if ( e820_map[i].size == 0 )
+            e820_remove_entry(i);
+        return;
+    }
+
+    if ( addr + size == e820_map[i].addr + e820_map[i].size )
+    {
+        e820_map[i].addr = addr;
+        e820_map[i].size = size;
+        return;
+    }
+
+    e820_insert_entry(i + 1);
+    e820_map[i + 1].addr = addr + size;
+    e820_map[i + 1].size = e820_map[i].addr + e820_map[i].size -
+                           e820_map[i + 1].addr;
+    e820_map[i + 1].type = E820_RESERVED;
+    e820_map[i].size = addr - e820_map[i].addr;
+}
 #endif
 
 unsigned long e820_get_maxpfn(unsigned long pages)
diff --git a/include/e820.h b/include/e820.h
index 49daefa..694ce3b 100644
--- a/include/e820.h
+++ b/include/e820.h
@@ -51,5 +51,9 @@ extern unsigned e820_entries;
 
 unsigned long e820_get_maxpfn(unsigned long pages);
 unsigned long e820_get_max_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);
+#endif
 
 #endif /*__E820_HEADER*/
-- 
2.26.2

[PATCH 07/10] mini-os: move x86 specific gnttab coding into arch/x86/gnttab.c

[Juergen Gross]

Having grant table code in arch/x86/mm.c seems wrong. Move it to the
new file arch/x86/gnttab.c, especially as the amount of code is
expected to grow further.

No functional change.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/gnttab.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/mm.c     | 47 ----------------------------
 2 files changed, 78 insertions(+), 47 deletions(-)
 create mode 100644 arch/x86/gnttab.c

diff --git a/arch/x86/gnttab.c b/arch/x86/gnttab.c
new file mode 100644
index 0000000..56e59d7
--- /dev/null
+++ b/arch/x86/gnttab.c
@@ -0,0 +1,78 @@
+/* -*-  Mode:C; c-basic-offset:4; tab-width:4 -*-
+ *
+ * (C) 2021 - Juergen Gross, SUSE Software Solutions Germany GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include <mini-os/os.h>
+#include <mini-os/hypervisor.h>
+#include <mini-os/gnttab.h>
+#include <mini-os/mm.h>
+#include <mini-os/types.h>
+
+grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
+{
+    struct gnttab_setup_table setup;
+    unsigned long frames[nr_grant_frames];
+
+    setup.dom = DOMID_SELF;
+    setup.nr_frames = nr_grant_frames;
+    set_xen_guest_handle(setup.frame_list, frames);
+
+    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
+    return map_frames(frames, nr_grant_frames);
+}
+
+void arch_suspend_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
+{
+#ifdef CONFIG_PARAVIRT
+    int i;
+
+    for ( i = 0; i < nr_grant_frames; i++ )
+    {
+        HYPERVISOR_update_va_mapping((unsigned long)gnttab_table + PAGE_SIZE * i,
+                __pte(0x0 << PAGE_SHIFT), UVMF_INVLPG);
+    }
+#endif
+    return;
+}
+
+void arch_resume_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
+{
+    struct gnttab_setup_table setup;
+    unsigned long frames[nr_grant_frames];
+#ifdef CONFIG_PARAVIRT
+    int i;
+#endif
+
+    setup.dom = DOMID_SELF;
+    setup.nr_frames = nr_grant_frames;
+    set_xen_guest_handle(setup.frame_list, frames);
+
+    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
+
+#ifdef CONFIG_PARAVIRT
+    for ( i = 0; i < nr_grant_frames; i++ )
+    {
+        HYPERVISOR_update_va_mapping((unsigned long)gnttab_table + PAGE_SIZE * i,
+                __pte((frames[i] << PAGE_SHIFT) | L1_PROT), UVMF_INVLPG);
+    }
+#endif
+}
diff --git a/arch/x86/mm.c b/arch/x86/mm.c
index c30d8bc..220c0b4 100644
--- a/arch/x86/mm.c
+++ b/arch/x86/mm.c
@@ -837,53 +837,6 @@ void arch_init_mm(unsigned long* start_pfn_p, unsigned long* max_pfn_p)
 #endif
 }
 
-grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
-{
-    struct gnttab_setup_table setup;
-    unsigned long frames[nr_grant_frames];
-
-    setup.dom = DOMID_SELF;
-    setup.nr_frames = nr_grant_frames;
-    set_xen_guest_handle(setup.frame_list, frames);
-
-    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
-    return map_frames(frames, nr_grant_frames);
-}
-
-void arch_suspend_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
-{
-#ifdef CONFIG_PARAVIRT
-    int i;
-
-    for (i = 0; i < nr_grant_frames; i++) {
-        HYPERVISOR_update_va_mapping((unsigned long)(((char *)gnttab_table) + PAGE_SIZE * i),
-                (pte_t){0x0<<PAGE_SHIFT}, UVMF_INVLPG);
-    }
-#endif
-    return;
-}
-
-void arch_resume_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
-{
-    struct gnttab_setup_table setup;
-    unsigned long frames[nr_grant_frames];
-#ifdef CONFIG_PARAVIRT
-    int i;
-#endif
-    setup.dom = DOMID_SELF;
-    setup.nr_frames = nr_grant_frames;
-    set_xen_guest_handle(setup.frame_list, frames);
-
-    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
-
-#ifdef CONFIG_PARAVIRT
-    for (i = 0; i < nr_grant_frames; i++) {
-        HYPERVISOR_update_va_mapping((unsigned long)(((char *)gnttab_table) + PAGE_SIZE * i),
-                (pte_t){(frames[i] << PAGE_SHIFT) | L1_PROT}, UVMF_INVLPG);
-    }
-#endif
-}
-
 unsigned long alloc_virt_kernel(unsigned n_pages)
 {
     unsigned long addr;
-- 
2.26.2

[PATCH 08/10] mini-os: add proper pvh grant table handling

[Juergen Gross]

Grant table initialization for PVH requires some additional actions
compared to PV mode. Add those.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/gnttab.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/x86/gnttab.c b/arch/x86/gnttab.c
index 56e59d7..281c207 100644
--- a/arch/x86/gnttab.c
+++ b/arch/x86/gnttab.c
@@ -22,11 +22,15 @@
  */
 
 #include <mini-os/os.h>
+#include <mini-os/console.h>
+#include <mini-os/e820.h>
 #include <mini-os/hypervisor.h>
 #include <mini-os/gnttab.h>
 #include <mini-os/mm.h>
 #include <mini-os/types.h>
+#include <xen/memory.h>
 
+#ifdef CONFIG_PARAVIRT
 grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
 {
     struct gnttab_setup_table setup;
@@ -39,6 +43,33 @@ grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
     HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
     return map_frames(frames, nr_grant_frames);
 }
+#else
+grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
+{
+    int i, rc;
+    struct xen_add_to_physmap xatp;
+    unsigned long pfn;
+    unsigned long frames[nr_grant_frames];
+
+    pfn = e820_get_reserved_pfns(nr_grant_frames);
+    for ( i = 0; i < nr_grant_frames; i++ )
+    {
+        xatp.domid = DOMID_SELF;
+        xatp.idx = i;
+        xatp.space = XENMAPSPACE_grant_table;
+        xatp.gpfn = pfn + i;
+        rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
+        if ( rc )
+        {
+            xprintk("could not init grant table\n");
+            do_exit();
+        }
+        frames[i] = pfn + i;
+    }
+
+    return map_frames(frames, nr_grant_frames);
+}
+#endif
 
 void arch_suspend_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
 {
-- 
2.26.2

[PATCH 09/10] mini-os: prepare grantmap entry interface for use by PVH mode

[Juergen Gross]

Instead of passing the pointer of a grantmap entry to the
_gntmap_[un]map_grant_ref() sub-functions use the map pointer and the
entry index instead. This will be needed for PVH mode usage.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 gntmap.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/gntmap.c b/gntmap.c
index f6ab3ad..7ae8fe6 100644
--- a/gntmap.c
+++ b/gntmap.c
@@ -55,36 +55,34 @@ struct gntmap_entry {
 };
 
 static inline int
-gntmap_entry_used(struct gntmap_entry *entry)
+gntmap_entry_used(struct gntmap *map, int idx)
 {
-    return entry->host_addr != 0;
+    return map->entries[idx].host_addr != 0;
 }
 
-static struct gntmap_entry*
-gntmap_find_free_entry(struct gntmap *map)
+static int gntmap_find_free_entry(struct gntmap *map)
 {
     int i;
 
     for (i = 0; i < map->nentries; i++) {
-        if (!gntmap_entry_used(&map->entries[i]))
-            return &map->entries[i];
+        if (!gntmap_entry_used(map, i))
+            return i;
     }
 
     DEBUG("(map=%p): all %d entries full",
            map, map->nentries);
-    return NULL;
+    return -1;
 }
 
-static struct gntmap_entry*
-gntmap_find_entry(struct gntmap *map, unsigned long addr)
+static int gntmap_find_entry(struct gntmap *map, unsigned long addr)
 {
     int i;
 
     for (i = 0; i < map->nentries; i++) {
         if (map->entries[i].host_addr == addr)
-            return &map->entries[i];
+            return i;
     }
-    return NULL;
+    return -1;
 }
 
 int
@@ -105,12 +103,13 @@ gntmap_set_max_grants(struct gntmap *map, int count)
 }
 
 static int
-_gntmap_map_grant_ref(struct gntmap_entry *entry, 
+_gntmap_map_grant_ref(struct gntmap *map, int idx,
                       unsigned long host_addr,
                       uint32_t domid,
                       uint32_t ref,
                       int writable)
 {
+    struct gntmap_entry *entry = map->entries + idx;
     struct gnttab_map_grant_ref op;
     int rc;
 
@@ -135,8 +134,9 @@ _gntmap_map_grant_ref(struct gntmap_entry *entry,
 }
 
 static int
-_gntmap_unmap_grant_ref(struct gntmap_entry *entry)
+_gntmap_unmap_grant_ref(struct gntmap *map, int idx)
 {
+    struct gntmap_entry *entry = map->entries + idx;
     struct gnttab_unmap_grant_ref op;
     int rc;
 
@@ -160,19 +160,19 @@ int
 gntmap_munmap(struct gntmap *map, unsigned long start_address, int count)
 {
     int i, rc;
-    struct gntmap_entry *ent;
+    int idx;
 
     DEBUG("(map=%p, start_address=%lx, count=%d)",
            map, start_address, count);
 
     for (i = 0; i < count; i++) {
-        ent = gntmap_find_entry(map, start_address + PAGE_SIZE * i);
-        if (ent == NULL) {
+        idx = gntmap_find_entry(map, start_address + PAGE_SIZE * i);
+        if (idx < 0) {
             printk("gntmap: tried to munmap unknown page\n");
             return -EINVAL;
         }
 
-        rc = _gntmap_unmap_grant_ref(ent);
+        rc = _gntmap_unmap_grant_ref(map, idx);
         if (rc != 0)
             return rc;
     }
@@ -189,7 +189,7 @@ gntmap_map_grant_refs(struct gntmap *map,
                       int writable)
 {
     unsigned long addr;
-    struct gntmap_entry *ent;
+    int idx;
     int i;
 
     DEBUG("(map=%p, count=%" PRIu32 ", "
@@ -206,9 +206,9 @@ gntmap_map_grant_refs(struct gntmap *map,
         return NULL;
 
     for (i = 0; i < count; i++) {
-        ent = gntmap_find_free_entry(map);
-        if (ent == NULL ||
-            _gntmap_map_grant_ref(ent,
+        idx = gntmap_find_free_entry(map);
+        if (idx < 0 ||
+            _gntmap_map_grant_ref(map, idx,
                                   addr + PAGE_SIZE * i,
                                   domids[i * domids_stride],
                                   refs[i],
@@ -233,15 +233,13 @@ gntmap_init(struct gntmap *map)
 void
 gntmap_fini(struct gntmap *map)
 {
-    struct gntmap_entry *ent;
     int i;
 
     DEBUG("(map=%p)", map);
 
     for (i = 0; i < map->nentries; i++) {
-        ent = &map->entries[i];
-        if (gntmap_entry_used(ent))
-            (void) _gntmap_unmap_grant_ref(ent);
+        if (gntmap_entry_used(map, i))
+            (void) _gntmap_unmap_grant_ref(map, i);
     }
 
     xfree(map->entries);
-- 
2.26.2

[PATCH 10/10] mini-os: modify grant mappings to work in PVH mode

[Juergen Gross]

For being able to use the grant mapping interface in PVH mode some
changes are required, as the guest needs to specify a physical address
in the hypercall interface.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 gntmap.c         | 81 ++++++++++++++++++++++++++++++++++--------------
 include/gntmap.h |  1 +
 2 files changed, 59 insertions(+), 23 deletions(-)

diff --git a/gntmap.c b/gntmap.c
index 7ae8fe6..126b04f 100644
--- a/gntmap.c
+++ b/gntmap.c
@@ -32,6 +32,7 @@
 
 #include <mini-os/os.h>
 #include <mini-os/lib.h>
+#include <mini-os/e820.h>
 #include <mini-os/xmalloc.h>
 #include <errno.h>
 #include <xen/grant_table.h>
@@ -97,11 +98,42 @@ gntmap_set_max_grants(struct gntmap *map, int count)
     if (map->entries == NULL)
         return -ENOMEM;
 
+#ifndef CONFIG_PARAVIRT
+    map->start_pfn = e820_get_reserved_pfns(count);
+#endif
+
     memset(map->entries, 0, sizeof(struct gntmap_entry) * count);
     map->nentries = count;
     return 0;
 }
 
+static int
+_gntmap_unmap_grant_ref(struct gntmap *map, int idx)
+{
+    struct gntmap_entry *entry = map->entries + idx;
+    struct gnttab_unmap_grant_ref op;
+    int rc;
+
+#ifdef CONFIG_PARAVIRT
+    op.host_addr    = (uint64_t) entry->host_addr;
+#else
+    op.host_addr    = (uint64_t)(map->start_pfn + idx) << PAGE_SHIFT;
+#endif
+    op.dev_bus_addr = 0;
+    op.handle       = entry->handle;
+
+    rc = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1);
+    if (rc != 0 || op.status != GNTST_okay) {
+        printk("GNTTABOP_unmap_grant_ref failed: "
+               "returned %d, status %" PRId16 "\n",
+               rc, op.status);
+        return rc != 0 ? rc : op.status;
+    }
+
+    entry->host_addr = 0;
+    return 0;
+}
+
 static int
 _gntmap_map_grant_ref(struct gntmap *map, int idx,
                       unsigned long host_addr,
@@ -112,10 +144,17 @@ _gntmap_map_grant_ref(struct gntmap *map, int idx,
     struct gntmap_entry *entry = map->entries + idx;
     struct gnttab_map_grant_ref op;
     int rc;
+#ifndef CONFIG_PARAVIRT
+    unsigned long pfn = map->start_pfn + idx;
+#endif
 
     op.ref = (grant_ref_t) ref;
     op.dom = (domid_t) domid;
+#ifdef CONFIG_PARAVIRT
     op.host_addr = (uint64_t) host_addr;
+#else
+    op.host_addr = (uint64_t)pfn << PAGE_SHIFT; 
+#endif
     op.flags = GNTMAP_host_map;
     if (!writable)
         op.flags |= GNTMAP_readonly;
@@ -128,31 +167,18 @@ _gntmap_map_grant_ref(struct gntmap *map, int idx,
         return rc != 0 ? rc : op.status;
     }
 
-    entry->host_addr = host_addr;
-    entry->handle = op.handle;
-    return 0;
-}
-
-static int
-_gntmap_unmap_grant_ref(struct gntmap *map, int idx)
-{
-    struct gntmap_entry *entry = map->entries + idx;
-    struct gnttab_unmap_grant_ref op;
-    int rc;
-
-    op.host_addr    = (uint64_t) entry->host_addr;
-    op.dev_bus_addr = 0;
-    op.handle       = entry->handle;
-
-    rc = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1);
-    if (rc != 0 || op.status != GNTST_okay) {
-        printk("GNTTABOP_unmap_grant_ref failed: "
-               "returned %d, status %" PRId16 "\n",
-               rc, op.status);
-        return rc != 0 ? rc : op.status;
+#ifndef CONFIG_PARAVIRT
+    rc = do_map_frames(host_addr, &pfn, 1, 0, 0, DOMID_SELF, NULL,
+                       writable ? L1_PROT : L1_PROT_RO);
+    if ( rc )
+    {
+        _gntmap_unmap_grant_ref(map, idx);
+        return rc;
     }
+#endif
 
-    entry->host_addr = 0;
+    entry->host_addr = host_addr;
+    entry->handle = op.handle;
     return 0;
 }
 
@@ -165,6 +191,10 @@ gntmap_munmap(struct gntmap *map, unsigned long start_address, int count)
     DEBUG("(map=%p, start_address=%lx, count=%d)",
            map, start_address, count);
 
+#ifndef CONFIG_PARAVIRT
+    unmap_frames(start_address, count);
+#endif
+
     for (i = 0; i < count; i++) {
         idx = gntmap_find_entry(map, start_address + PAGE_SIZE * i);
         if (idx < 0) {
@@ -242,6 +272,11 @@ gntmap_fini(struct gntmap *map)
             (void) _gntmap_unmap_grant_ref(map, i);
     }
 
+#ifndef CONFIG_PARAVIRT
+    e820_put_reserved_pfns(map->start_pfn, map->nentries);
+    map->start_pfn = 0;
+#endif
+
     xfree(map->entries);
     map->entries = NULL;
     map->nentries = 0;
diff --git a/include/gntmap.h b/include/gntmap.h
index fde53f3..d3d7e88 100644
--- a/include/gntmap.h
+++ b/include/gntmap.h
@@ -10,6 +10,7 @@
 struct gntmap {
     int nentries;
     struct gntmap_entry *entries;
+    unsigned long start_pfn;
 };
 
 int
-- 
2.26.2

[PATCH] mini-os: support event channel 0 for console

[Juergen Gross]

The console event channel might be 0 for the console, so use the value
of ~0 as invalid instead.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 console/xencons_ring.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/console/xencons_ring.c b/console/xencons_ring.c
index b6db74e..5c2573e 100644
--- a/console/xencons_ring.c
+++ b/console/xencons_ring.c
@@ -17,7 +17,7 @@
 DECLARE_WAIT_QUEUE_HEAD(console_queue);
 
 static struct xencons_interface *console_ring;
-uint32_t console_evtchn;
+uint32_t console_evtchn = ~0;
 
 static struct consfront_dev* resume_xen_console(struct consfront_dev* dev);
 
@@ -55,7 +55,7 @@ static inline void notify_daemon(struct consfront_dev *dev)
 
 static inline struct xencons_interface *xencons_interface(void)
 {
-    return console_evtchn ? console_ring : NULL;
+    return (console_evtchn != ~0) ? console_ring : NULL;
 } 
  
 int xencons_ring_send_no_notify(struct consfront_dev *dev, const char *data, unsigned len)
@@ -181,7 +181,7 @@ struct consfront_dev *xencons_ring_init(void)
 {
     struct consfront_dev *dev;
 
-    if (!console_evtchn)
+    if (console_evtchn != ~0)
         return 0;
 
     dev = malloc(sizeof(struct consfront_dev));
-- 
2.26.2

Re: [PATCH] mini-os: support event channel 0 for console

[Jan Beulich]

On 06.12.2021 13:46, Juergen Gross wrote:
> The console event channel might be 0 for the console, so use the value
> of ~0 as invalid instead.

I may be missing something mini-os specific here, but in Xen channel 0
is always invalid. It's not just here that this value would be used as
a sentinel.

Jan

Re: [PATCH] mini-os: support event channel 0 for console

[Juergen Gross]

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

On 06.12.21 14:24, Jan Beulich wrote:
> On 06.12.2021 13:46, Juergen Gross wrote:
>> The console event channel might be 0 for the console, so use the value
>> of ~0 as invalid instead.
> 
> I may be missing something mini-os specific here, but in Xen channel 0
> is always invalid. It's not just here that this value would be used as
> a sentinel.

Maybe this is a special case for Xenstore stubdom. Without this change
I can't connect to the console when the system is up, with this patch
it is possible.


Juergen

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

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

Re: [PATCH] mini-os: support event channel 0 for console

[Juergen Gross]

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

On 06.12.21 14:24, Jan Beulich wrote:
> On 06.12.2021 13:46, Juergen Gross wrote:
>> The console event channel might be 0 for the console, so use the value
>> of ~0 as invalid instead.
> 
> I may be missing something mini-os specific here, but in Xen channel 0
> is always invalid. It's not just here that this value would be used as
> a sentinel.

This made me look at th domain creation paths again, and it seems as if
the parameter settings for HVM guests is split in an awful way: the
console ring page pfn is set from libxenguest, while the console event
channel is set from libxl only. :-(

This means that this patch can be dropped, while init-xenstore-domain
needs to gain another one.

Thanks for your feedback,


Juergen

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

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

Re: [PATCH 01/10] mini-os: split e820 map handling into new source file

[Samuel Thibault]

Juergen Gross, le lun. 06 déc. 2021 08:23:28 +0100, a ecrit:
> Introduce e820.c containing all the E820 memory map handling.
> 
> No functional change.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> ---
>  Makefile       |   1 +
>  arch/arm/mm.c  |   8 ----
>  arch/x86/mm.c  |  70 +----------------------------
>  e820.c         | 119 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/e820.h |   6 +++
>  5 files changed, 128 insertions(+), 76 deletions(-)
>  create mode 100644 e820.c
> 
> diff --git a/Makefile b/Makefile
> index 4b76b55..06b60fc 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -41,6 +41,7 @@ src-$(CONFIG_TPMFRONT) += tpmfront.c
>  src-$(CONFIG_TPM_TIS) += tpm_tis.c
>  src-$(CONFIG_TPMBACK) += tpmback.c
>  src-y += daytime.c
> +src-y += e820.c
>  src-y += events.c
>  src-$(CONFIG_FBFRONT) += fbfront.c
>  src-y += gntmap.c
> diff --git a/arch/arm/mm.c b/arch/arm/mm.c
> index f806c9f..9068166 100644
> --- a/arch/arm/mm.c
> +++ b/arch/arm/mm.c
> @@ -7,14 +7,6 @@
>  #include <lib.h>
>  
>  uint32_t physical_address_offset;
> -struct e820entry e820_map[1] = {
> -    {
> -        .addr = 0,
> -        .size = ULONG_MAX - 1,
> -        .type = E820_RAM
> -    }
> -};
> -unsigned e820_entries = 1;
>  
>  unsigned long allocate_ondemand(unsigned long n, unsigned long alignment)
>  {
> diff --git a/arch/x86/mm.c b/arch/x86/mm.c
> index 8ba14a5..8df93da 100644
> --- a/arch/x86/mm.c
> +++ b/arch/x86/mm.c
> @@ -64,15 +64,6 @@ extern char stack[];
>  extern void page_walk(unsigned long va);
>  
>  #ifdef CONFIG_PARAVIRT
> -struct e820entry e820_map[1] = {
> -    {
> -        .addr = 0,
> -        .size = ULONG_MAX - 1,
> -        .type = E820_RAM
> -    }
> -};
> -unsigned e820_entries = 1;
> -
>  void arch_mm_preinit(void *p)
>  {
>      start_info_t *si = p;
> @@ -112,25 +103,11 @@ desc_ptr idt_ptr =
>      .base = (unsigned long)&idt,
>  };
>  
> -struct e820entry e820_map[E820_MAX];
> -unsigned e820_entries;
> -
> -static char *e820_types[E820_TYPES] = {
> -    [E820_RAM]      = "RAM",
> -    [E820_RESERVED] = "Reserved",
> -    [E820_ACPI]     = "ACPI",
> -    [E820_NVS]      = "NVS",
> -    [E820_UNUSABLE] = "Unusable",
> -    [E820_PMEM]     = "PMEM"
> -};
> -
>  void arch_mm_preinit(void *p)
>  {
>      long ret;
>      domid_t domid = DOMID_SELF;
> -    struct xen_memory_map memmap;
> -    int i;
> -    unsigned long pfn, max = 0;
> +    unsigned long max;
>  
>      pt_base = page_table_base;
>      first_free_pfn = PFN_UP(to_phys(&_end));
> @@ -142,53 +119,10 @@ void arch_mm_preinit(void *p)
>      }
>      last_free_pfn = ret;
>  
> -    memmap.nr_entries = E820_MAX;
> -    set_xen_guest_handle(memmap.buffer, e820_map);
> -    ret = HYPERVISOR_memory_op(XENMEM_memory_map, &memmap);
> -    if ( ret < 0 )
> -    {
> -        xprintk("could not get memory map\n");
> -        do_exit();
> -    }
> -    e820_entries = memmap.nr_entries;
> -
> -    for ( i = 0; i < e820_entries; i++ )
> -    {
> -        if ( e820_map[i].type != E820_RAM )
> -            continue;
> -        pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
> -        if ( pfn > max )
> -            max = pfn;
> -    }
> -
> +    max = e820_get_maxpfn();
>      if ( max < last_free_pfn )
>          last_free_pfn = max;
>  }
> -
> -void arch_print_memmap(void)
> -{
> -    int i;
> -    unsigned long from, to;
> -    char *type;
> -    char buf[12];
> -
> -    printk("Memory map:\n");
> -    for ( i = 0; i < e820_entries; i++ )
> -    {
> -        if ( e820_map[i].type >= E820_TYPES || !e820_types[e820_map[i].type] )
> -        {
> -            snprintf(buf, sizeof(buf), "%8x", e820_map[i].type);
> -            type = buf;
> -        }
> -        else
> -        {
> -            type = e820_types[e820_map[i].type];
> -        }
> -        from = e820_map[i].addr;
> -        to = from + e820_map[i].size - 1;
> -        printk("%012lx-%012lx: %s\n", from, to, type);
> -    }
> -}
>  #endif
>  
>  /*
> diff --git a/e820.c b/e820.c
> new file mode 100644
> index 0000000..2165280
> --- /dev/null
> +++ b/e820.c
> @@ -0,0 +1,119 @@
> +/* -*-  Mode:C; c-basic-offset:4; tab-width:4 -*-
> + *
> + * (C) 2021 - Juergen Gross, SUSE Software Solutions Germany GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <mini-os/types.h>
> +#include <mini-os/lib.h>
> +#include <mini-os/console.h>
> +#include <mini-os/os.h>
> +#include <mini-os/posix/limits.h>
> +#include <mini-os/e820.h>
> +#include <xen/memory.h>
> +
> +#ifdef CONFIG_E820_TRIVIAL
> +struct e820entry e820_map[1] = {
> +    {
> +        .addr = 0,
> +        .size = ULONG_MAX - 1,
> +        .type = E820_RAM
> +    }
> +};
> +
> +unsigned e820_entries = 1;
> +
> +static void e820_get_memmap(void)
> +{
> +}
> +
> +#else
> +struct e820entry e820_map[E820_MAX];
> +unsigned e820_entries;
> +
> +static char *e820_types[E820_TYPES] = {
> +    [E820_RAM]      = "RAM",
> +    [E820_RESERVED] = "Reserved",
> +    [E820_ACPI]     = "ACPI",
> +    [E820_NVS]      = "NVS",
> +    [E820_UNUSABLE] = "Unusable",
> +    [E820_PMEM]     = "PMEM"
> +};
> +
> +static void e820_get_memmap(void)
> +{
> +    long ret;
> +    struct xen_memory_map memmap;
> +
> +    memmap.nr_entries = E820_MAX;
> +    set_xen_guest_handle(memmap.buffer, e820_map);
> +    ret = HYPERVISOR_memory_op(XENMEM_memory_map, &memmap);
> +    if ( ret < 0 )
> +    {
> +        xprintk("could not get memory map\n");
> +        do_exit();
> +    }
> +    e820_entries = memmap.nr_entries;
> +}
> +
> +void arch_print_memmap(void)
> +{
> +    int i;
> +    unsigned long from, to;
> +    char *type;
> +    char buf[12];
> +
> +    printk("Memory map:\n");
> +    for ( i = 0; i < e820_entries; i++ )
> +    {
> +        if ( e820_map[i].type >= E820_TYPES || !e820_types[e820_map[i].type] )
> +        {
> +            snprintf(buf, sizeof(buf), "%8x", e820_map[i].type);
> +            type = buf;
> +        }
> +        else
> +        {
> +            type = e820_types[e820_map[i].type];
> +        }
> +        from = e820_map[i].addr;
> +        to = from + e820_map[i].size - 1;
> +        printk("%012lx-%012lx: %s\n", from, to, type);
> +    }
> +}
> +#endif
> +
> +unsigned long e820_get_maxpfn(void)
> +{
> +    int i;
> +    unsigned long pfn, max = 0;
> +
> +    e820_get_memmap();
> +
> +    for ( i = 0; i < e820_entries; i++ )
> +    {
> +        if ( e820_map[i].type != E820_RAM )
> +            continue;
> +        pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
> +        if ( pfn > max )
> +            max = pfn;
> +    }
> +
> +    return max;
> +}
> diff --git a/include/e820.h b/include/e820.h
> index 920551c..af2129f 100644
> --- a/include/e820.h
> +++ b/include/e820.h
> @@ -24,6 +24,10 @@
>  #ifndef __E820_HEADER
>  #define __E820_HEADER
>  
> +#if defined(__arm__) || defined(__aarch64__) || defined(CONFIG_PARAVIRT)
> +#define CONFIG_E820_TRIVIAL
> +#endif
> +
>  /* PC BIOS standard E820 types and structure. */
>  #define E820_RAM          1
>  #define E820_RESERVED     2
> @@ -45,4 +49,6 @@ struct __packed e820entry {
>  extern struct e820entry e820_map[];
>  extern unsigned e820_entries;
>  
> +unsigned long e820_get_maxpfn(void);
> +
>  #endif /*__E820_HEADER*/
> -- 
> 2.26.2
> 

-- 
Samuel
* c is away : cuisine; bouffe
<y> oh, moi je fais plutôt cuisine & bouffe en fait :)
<c> oui c'est vrai, certains font cuisine && bouffe (juste au cas où... ;-))
<y> ( cuisine && bouffe ) || restau
<N> voire ((cuisine && bouffe) || restau) & apéritif
 -+- #ens-mim -+-

Re: [PATCH 02/10] mini-os: sort and sanitize e820 memory map

[Samuel Thibault]

Hello,

Juergen Gross, le lun. 06 déc. 2021 08:23:29 +0100, a ecrit:
> - align the entries to page boundaries

> +    /* Adjust map entries to page boundaries. */
> +    for ( i = 0; i < e820_entries; i++ )
> +    {
> +        end = (e820_map[i].addr + e820_map[i].size + PAGE_SIZE - 1) & PAGE_MASK;
> +        e820_map[i].addr &= PAGE_MASK;
> +        e820_map[i].size = end - e820_map[i].addr;
> +    }

Mmm, what if the previous entry ends after the aligned start?

On real machines that does happen, and you'd rather round up the start
address of usable areas, rather than rounding it down (and conversely
for the end).

> +    /* Sort entries by start address. */
> +    for ( i = 0; i < e820_entries - 1; i++ )
> +    {
> +        if ( e820_map[i].addr > e820_map[i + 1].addr )
> +        {
> +            e820_swap_entries(i, i + 1);
> +            i = -1;
> +        }
> +    }

This looks O(n^3) to me? A bubble sort like this should be fine:

    /* Sort entries by start address. */
    for ( last = e820_entries; last > 1; last-- )
    {
        for ( i = 0; i < last - 1; i++ )
        {
            if ( e820_map[i].addr > e820_map[i + 1].addr )
            {
                e820_swap_entries(i, i + 1);
            }
        }
    }

Samuel

Re: [PATCH 03/10] mini-os: don't assume contiguous RAM when initializing in PVH mode

[Samuel Thibault]

Juergen Gross, le lun. 06 déc. 2021 08:23:30 +0100, a ecrit:
> -    unsigned long pfn, max = 0;
> +    unsigned long pfns, max = 0;

I'd say rather rename max to start.

>      e820_get_memmap();
>  
> @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void)
>      {
>          if ( e820_map[i].type != E820_RAM )
>              continue;
> -        pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
> -        if ( pfn > max )
> -            max = pfn;
> +        pfns = e820_map[i].size >> PAGE_SHIFT;
> +        max = e820_map[i].addr >> PAGE_SHIFT;

since it's it's always the start of the e820 entry.

> +        if ( pages <= pfns )
> +            return max + pages;
> +        pages -= pfns;
> +        max += pfns;

Here we don't need do change max, only pages.

Samuel

Re: [PATCH 04/10] mini-os: respect memory map when ballooning up

[Samuel Thibault]

Juergen Gross, le lun. 06 déc. 2021 08:23:31 +0100, a ecrit:
> @@ -81,8 +93,11 @@ int balloon_up(unsigned long n_pages)
>      if ( n_pages > N_BALLOON_FRAMES )
>          n_pages = N_BALLOON_FRAMES;
>  
> +    start_pfn = e820_get_maxpfn(nr_mem_pages + 1) - 1;
> +    n_pages = e820_get_max_pages(start_pfn, n_pages);

I'd say call it e820_get_max_contig_pages?

> +unsigned long e820_get_max_pages(unsigned long pfn, unsigned long pages)
> +{
> +    int i;
> +    unsigned long end;
> +
> +    for ( i = 0; i < e820_entries; i++ )
> +    {
> +        if ( e820_map[i].type != E820_RAM ||
> +             (e820_map[i].addr >> PAGE_SHIFT) > pfn )
> +            continue;

"> pfn" looks odd to me? If the start of the e820 entry is already
beyond pfn, we'll never find any other entry. We however do want to skip
entries that have addr+size that is below pfn.

> +        end = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
> +        return ((end - pfn) > pages) ? pages : end - pfn;
> +    }
> +
> +    return 0;
> +}

Re: [PATCH 05/10] mini-os: don't repeat definition available via header file

[Samuel Thibault]

Juergen Gross, le lun. 06 déc. 2021 08:23:32 +0100, a ecrit:
> arch/x86/setup.c is repeating the definition of __pte() instead using
> the appropriate header. Fix that.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> ---
>  arch/x86/setup.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/arch/x86/setup.c b/arch/x86/setup.c
> index 1ec68d3..b27bbed 100644
> --- a/arch/x86/setup.c
> +++ b/arch/x86/setup.c
> @@ -29,6 +29,7 @@
>  #include <mini-os/os.h>
>  #include <mini-os/lib.h> /* for printk, memcpy */
>  #include <mini-os/kernel.h>
> +#include <mini-os/hypervisor.h>
>  #include <xen/xen.h>
>  #include <xen/arch-x86/cpuid.h>
>  #include <xen/arch-x86/hvm/start_info.h>
> @@ -61,13 +62,6 @@ char stack[2*STACK_SIZE];
>  
>  extern char shared_info[PAGE_SIZE];
>  
> -#if defined(__x86_64__)
> -#define __pte(x) ((pte_t) { (x) } )
> -#else
> -#define __pte(x) ({ unsigned long long _x = (x);        \
> -    ((pte_t) {(unsigned long)(_x), (unsigned long)(_x>>32)}); })
> -#endif
> -
>  static inline void fpu_init(void) {
>  	asm volatile("fninit");
>  }
> -- 
> 2.26.2
> 

-- 
Samuel
 jr> J'ai fait.
 Ne bougez pas, l'aide soignante va venir nettoyer.
 -+- FF in GNU - Le vieil homme et la merde -+-

Re: [PATCH 06/10] mini-os: add memory map service functions

[Samuel Thibault]

Juergen Gross, le lun. 06 déc. 2021 08:23:33 +0100, a ecrit:
> +void e820_put_reserved_pfns(unsigned long start_pfn, int pages)
> +{
> +    int i;
> +    unsigned long addr = start_pfn << PAGE_SHIFT;
> +    unsigned long size = (long)pages << PAGE_SHIFT;
> +
> +    for ( i = 0; i < e820_entries && addr < e820_map[i].addr; i++ );

Shouldn't that be addr > e820_map[i].addr + e820_map[i].size?

> +    BUG_ON(i == e820_entries || e820_map[i].type != E820_RESERVED);

We should also BUG_ON e820_map[i].addr > addr (i.e. we didn't find an
entry that contained our address).

> +    if ( addr == e820_map[i].addr )
> +    {
> +        e820_map[i].addr += size;

I'd say BUG_ON here if e820_map[i].size < size.

> +        e820_map[i].size -= size;
> +        if ( e820_map[i].size == 0 )
> +            e820_remove_entry(i);
> +        return;
> +    }
> +
> +    if ( addr + size == e820_map[i].addr + e820_map[i].size )
> +    {
> +        e820_map[i].addr = addr;
> +        e820_map[i].size = size;

? Shouldn't that rather be just

> +        e820_map[i].size -= size;

? (since what we remove is at the end of the area, the start of the area
doesn't change)

> +        return;
> +    }

Re: [PATCH 07/10] mini-os: move x86 specific gnttab coding into arch/x86/gnttab.c

[Samuel Thibault]

Juergen Gross, le lun. 06 déc. 2021 08:23:34 +0100, a ecrit:
> Having grant table code in arch/x86/mm.c seems wrong. Move it to the
> new file arch/x86/gnttab.c, especially as the amount of code is
> expected to grow further.
> 
> No functional change.

There is the __pte fix that you'd probably want to mention.

> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> ---
>  arch/x86/gnttab.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/mm.c     | 47 ----------------------------
>  2 files changed, 78 insertions(+), 47 deletions(-)
>  create mode 100644 arch/x86/gnttab.c
> 
> diff --git a/arch/x86/gnttab.c b/arch/x86/gnttab.c
> new file mode 100644
> index 0000000..56e59d7
> --- /dev/null
> +++ b/arch/x86/gnttab.c
> @@ -0,0 +1,78 @@
> +/* -*-  Mode:C; c-basic-offset:4; tab-width:4 -*-
> + *
> + * (C) 2021 - Juergen Gross, SUSE Software Solutions Germany GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <mini-os/os.h>
> +#include <mini-os/hypervisor.h>
> +#include <mini-os/gnttab.h>
> +#include <mini-os/mm.h>
> +#include <mini-os/types.h>
> +
> +grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
> +{
> +    struct gnttab_setup_table setup;
> +    unsigned long frames[nr_grant_frames];
> +
> +    setup.dom = DOMID_SELF;
> +    setup.nr_frames = nr_grant_frames;
> +    set_xen_guest_handle(setup.frame_list, frames);
> +
> +    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
> +    return map_frames(frames, nr_grant_frames);
> +}
> +
> +void arch_suspend_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
> +{
> +#ifdef CONFIG_PARAVIRT
> +    int i;
> +
> +    for ( i = 0; i < nr_grant_frames; i++ )
> +    {
> +        HYPERVISOR_update_va_mapping((unsigned long)gnttab_table + PAGE_SIZE * i,
> +                __pte(0x0 << PAGE_SHIFT), UVMF_INVLPG);
> +    }
> +#endif
> +    return;
> +}
> +
> +void arch_resume_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
> +{
> +    struct gnttab_setup_table setup;
> +    unsigned long frames[nr_grant_frames];
> +#ifdef CONFIG_PARAVIRT
> +    int i;
> +#endif
> +
> +    setup.dom = DOMID_SELF;
> +    setup.nr_frames = nr_grant_frames;
> +    set_xen_guest_handle(setup.frame_list, frames);
> +
> +    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
> +
> +#ifdef CONFIG_PARAVIRT
> +    for ( i = 0; i < nr_grant_frames; i++ )
> +    {
> +        HYPERVISOR_update_va_mapping((unsigned long)gnttab_table + PAGE_SIZE * i,
> +                __pte((frames[i] << PAGE_SHIFT) | L1_PROT), UVMF_INVLPG);
> +    }
> +#endif
> +}
> diff --git a/arch/x86/mm.c b/arch/x86/mm.c
> index c30d8bc..220c0b4 100644
> --- a/arch/x86/mm.c
> +++ b/arch/x86/mm.c
> @@ -837,53 +837,6 @@ void arch_init_mm(unsigned long* start_pfn_p, unsigned long* max_pfn_p)
>  #endif
>  }
>  
> -grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
> -{
> -    struct gnttab_setup_table setup;
> -    unsigned long frames[nr_grant_frames];
> -
> -    setup.dom = DOMID_SELF;
> -    setup.nr_frames = nr_grant_frames;
> -    set_xen_guest_handle(setup.frame_list, frames);
> -
> -    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
> -    return map_frames(frames, nr_grant_frames);
> -}
> -
> -void arch_suspend_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
> -{
> -#ifdef CONFIG_PARAVIRT
> -    int i;
> -
> -    for (i = 0; i < nr_grant_frames; i++) {
> -        HYPERVISOR_update_va_mapping((unsigned long)(((char *)gnttab_table) + PAGE_SIZE * i),
> -                (pte_t){0x0<<PAGE_SHIFT}, UVMF_INVLPG);
> -    }
> -#endif
> -    return;
> -}
> -
> -void arch_resume_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
> -{
> -    struct gnttab_setup_table setup;
> -    unsigned long frames[nr_grant_frames];
> -#ifdef CONFIG_PARAVIRT
> -    int i;
> -#endif
> -    setup.dom = DOMID_SELF;
> -    setup.nr_frames = nr_grant_frames;
> -    set_xen_guest_handle(setup.frame_list, frames);
> -
> -    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
> -
> -#ifdef CONFIG_PARAVIRT
> -    for (i = 0; i < nr_grant_frames; i++) {
> -        HYPERVISOR_update_va_mapping((unsigned long)(((char *)gnttab_table) + PAGE_SIZE * i),
> -                (pte_t){(frames[i] << PAGE_SHIFT) | L1_PROT}, UVMF_INVLPG);
> -    }
> -#endif
> -}
> -
>  unsigned long alloc_virt_kernel(unsigned n_pages)
>  {
>      unsigned long addr;
> -- 
> 2.26.2
> 

-- 
Samuel
War doesn't prove who's right, just who's left.

Re: [PATCH 08/10] mini-os: add proper pvh grant table handling

[Samuel Thibault]

Juergen Gross, le lun. 06 déc. 2021 08:23:35 +0100, a ecrit:
> Grant table initialization for PVH requires some additional actions
> compared to PV mode. Add those.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> ---
>  arch/x86/gnttab.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/arch/x86/gnttab.c b/arch/x86/gnttab.c
> index 56e59d7..281c207 100644
> --- a/arch/x86/gnttab.c
> +++ b/arch/x86/gnttab.c
> @@ -22,11 +22,15 @@
>   */
>  
>  #include <mini-os/os.h>
> +#include <mini-os/console.h>
> +#include <mini-os/e820.h>
>  #include <mini-os/hypervisor.h>
>  #include <mini-os/gnttab.h>
>  #include <mini-os/mm.h>
>  #include <mini-os/types.h>
> +#include <xen/memory.h>
>  
> +#ifdef CONFIG_PARAVIRT
>  grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
>  {
>      struct gnttab_setup_table setup;
> @@ -39,6 +43,33 @@ grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
>      HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
>      return map_frames(frames, nr_grant_frames);
>  }
> +#else
> +grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
> +{
> +    int i, rc;
> +    struct xen_add_to_physmap xatp;
> +    unsigned long pfn;
> +    unsigned long frames[nr_grant_frames];
> +
> +    pfn = e820_get_reserved_pfns(nr_grant_frames);
> +    for ( i = 0; i < nr_grant_frames; i++ )
> +    {
> +        xatp.domid = DOMID_SELF;
> +        xatp.idx = i;
> +        xatp.space = XENMAPSPACE_grant_table;
> +        xatp.gpfn = pfn + i;
> +        rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> +        if ( rc )
> +        {
> +            xprintk("could not init grant table\n");
> +            do_exit();
> +        }
> +        frames[i] = pfn + i;
> +    }
> +
> +    return map_frames(frames, nr_grant_frames);
> +}
> +#endif
>  
>  void arch_suspend_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
>  {
> -- 
> 2.26.2
> 

-- 
Samuel
 > Il [e2fsck] a bien démarré, mais il m'a rendu la main aussitot en me
 > disant "houlala, c'est pas beau à voir votre truc, je préfèrerai que
 > vous teniez vous même la tronçonneuse" (traduction libre)
 NC in Guide du linuxien pervers : "Bien configurer sa tronçonneuse."

Re: [PATCH 09/10] mini-os: prepare grantmap entry interface for use by PVH mode

[Samuel Thibault]

Juergen Gross, le lun. 06 déc. 2021 08:23:36 +0100, a ecrit:
> Instead of passing the pointer of a grantmap entry to the
> _gntmap_[un]map_grant_ref() sub-functions use the map pointer and the
> entry index instead. This will be needed for PVH mode usage.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> ---
>  gntmap.c | 48 +++++++++++++++++++++++-------------------------
>  1 file changed, 23 insertions(+), 25 deletions(-)
> 
> diff --git a/gntmap.c b/gntmap.c
> index f6ab3ad..7ae8fe6 100644
> --- a/gntmap.c
> +++ b/gntmap.c
> @@ -55,36 +55,34 @@ struct gntmap_entry {
>  };
>  
>  static inline int
> -gntmap_entry_used(struct gntmap_entry *entry)
> +gntmap_entry_used(struct gntmap *map, int idx)
>  {
> -    return entry->host_addr != 0;
> +    return map->entries[idx].host_addr != 0;
>  }
>  
> -static struct gntmap_entry*
> -gntmap_find_free_entry(struct gntmap *map)
> +static int gntmap_find_free_entry(struct gntmap *map)
>  {
>      int i;
>  
>      for (i = 0; i < map->nentries; i++) {
> -        if (!gntmap_entry_used(&map->entries[i]))
> -            return &map->entries[i];
> +        if (!gntmap_entry_used(map, i))
> +            return i;
>      }
>  
>      DEBUG("(map=%p): all %d entries full",
>             map, map->nentries);
> -    return NULL;
> +    return -1;
>  }
>  
> -static struct gntmap_entry*
> -gntmap_find_entry(struct gntmap *map, unsigned long addr)
> +static int gntmap_find_entry(struct gntmap *map, unsigned long addr)
>  {
>      int i;
>  
>      for (i = 0; i < map->nentries; i++) {
>          if (map->entries[i].host_addr == addr)
> -            return &map->entries[i];
> +            return i;
>      }
> -    return NULL;
> +    return -1;
>  }
>  
>  int
> @@ -105,12 +103,13 @@ gntmap_set_max_grants(struct gntmap *map, int count)
>  }
>  
>  static int
> -_gntmap_map_grant_ref(struct gntmap_entry *entry, 
> +_gntmap_map_grant_ref(struct gntmap *map, int idx,
>                        unsigned long host_addr,
>                        uint32_t domid,
>                        uint32_t ref,
>                        int writable)
>  {
> +    struct gntmap_entry *entry = map->entries + idx;
>      struct gnttab_map_grant_ref op;
>      int rc;
>  
> @@ -135,8 +134,9 @@ _gntmap_map_grant_ref(struct gntmap_entry *entry,
>  }
>  
>  static int
> -_gntmap_unmap_grant_ref(struct gntmap_entry *entry)
> +_gntmap_unmap_grant_ref(struct gntmap *map, int idx)
>  {
> +    struct gntmap_entry *entry = map->entries + idx;
>      struct gnttab_unmap_grant_ref op;
>      int rc;
>  
> @@ -160,19 +160,19 @@ int
>  gntmap_munmap(struct gntmap *map, unsigned long start_address, int count)
>  {
>      int i, rc;
> -    struct gntmap_entry *ent;
> +    int idx;
>  
>      DEBUG("(map=%p, start_address=%lx, count=%d)",
>             map, start_address, count);
>  
>      for (i = 0; i < count; i++) {
> -        ent = gntmap_find_entry(map, start_address + PAGE_SIZE * i);
> -        if (ent == NULL) {
> +        idx = gntmap_find_entry(map, start_address + PAGE_SIZE * i);
> +        if (idx < 0) {
>              printk("gntmap: tried to munmap unknown page\n");
>              return -EINVAL;
>          }
>  
> -        rc = _gntmap_unmap_grant_ref(ent);
> +        rc = _gntmap_unmap_grant_ref(map, idx);
>          if (rc != 0)
>              return rc;
>      }
> @@ -189,7 +189,7 @@ gntmap_map_grant_refs(struct gntmap *map,
>                        int writable)
>  {
>      unsigned long addr;
> -    struct gntmap_entry *ent;
> +    int idx;
>      int i;
>  
>      DEBUG("(map=%p, count=%" PRIu32 ", "
> @@ -206,9 +206,9 @@ gntmap_map_grant_refs(struct gntmap *map,
>          return NULL;
>  
>      for (i = 0; i < count; i++) {
> -        ent = gntmap_find_free_entry(map);
> -        if (ent == NULL ||
> -            _gntmap_map_grant_ref(ent,
> +        idx = gntmap_find_free_entry(map);
> +        if (idx < 0 ||
> +            _gntmap_map_grant_ref(map, idx,
>                                    addr + PAGE_SIZE * i,
>                                    domids[i * domids_stride],
>                                    refs[i],
> @@ -233,15 +233,13 @@ gntmap_init(struct gntmap *map)
>  void
>  gntmap_fini(struct gntmap *map)
>  {
> -    struct gntmap_entry *ent;
>      int i;
>  
>      DEBUG("(map=%p)", map);
>  
>      for (i = 0; i < map->nentries; i++) {
> -        ent = &map->entries[i];
> -        if (gntmap_entry_used(ent))
> -            (void) _gntmap_unmap_grant_ref(ent);
> +        if (gntmap_entry_used(map, i))
> +            (void) _gntmap_unmap_grant_ref(map, i);
>      }
>  
>      xfree(map->entries);
> -- 
> 2.26.2
> 

-- 
Samuel
<L> pour moi le seul qui est autorisé à fasciser, c moi :-)

Re: [PATCH 10/10] mini-os: modify grant mappings to work in PVH mode

[Samuel Thibault]

Juergen Gross, le lun. 06 déc. 2021 08:23:37 +0100, a ecrit:
> For being able to use the grant mapping interface in PVH mode some
> changes are required, as the guest needs to specify a physical address
> in the hypercall interface.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

> ---
>  gntmap.c         | 81 ++++++++++++++++++++++++++++++++++--------------
>  include/gntmap.h |  1 +
>  2 files changed, 59 insertions(+), 23 deletions(-)
> 
> diff --git a/gntmap.c b/gntmap.c
> index 7ae8fe6..126b04f 100644
> --- a/gntmap.c
> +++ b/gntmap.c
> @@ -32,6 +32,7 @@
>  
>  #include <mini-os/os.h>
>  #include <mini-os/lib.h>
> +#include <mini-os/e820.h>
>  #include <mini-os/xmalloc.h>
>  #include <errno.h>
>  #include <xen/grant_table.h>
> @@ -97,11 +98,42 @@ gntmap_set_max_grants(struct gntmap *map, int count)
>      if (map->entries == NULL)
>          return -ENOMEM;
>  
> +#ifndef CONFIG_PARAVIRT
> +    map->start_pfn = e820_get_reserved_pfns(count);
> +#endif
> +
>      memset(map->entries, 0, sizeof(struct gntmap_entry) * count);
>      map->nentries = count;
>      return 0;
>  }
>  
> +static int
> +_gntmap_unmap_grant_ref(struct gntmap *map, int idx)
> +{
> +    struct gntmap_entry *entry = map->entries + idx;
> +    struct gnttab_unmap_grant_ref op;
> +    int rc;
> +
> +#ifdef CONFIG_PARAVIRT
> +    op.host_addr    = (uint64_t) entry->host_addr;
> +#else
> +    op.host_addr    = (uint64_t)(map->start_pfn + idx) << PAGE_SHIFT;
> +#endif
> +    op.dev_bus_addr = 0;
> +    op.handle       = entry->handle;
> +
> +    rc = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1);
> +    if (rc != 0 || op.status != GNTST_okay) {
> +        printk("GNTTABOP_unmap_grant_ref failed: "
> +               "returned %d, status %" PRId16 "\n",
> +               rc, op.status);
> +        return rc != 0 ? rc : op.status;
> +    }
> +
> +    entry->host_addr = 0;
> +    return 0;
> +}
> +
>  static int
>  _gntmap_map_grant_ref(struct gntmap *map, int idx,
>                        unsigned long host_addr,
> @@ -112,10 +144,17 @@ _gntmap_map_grant_ref(struct gntmap *map, int idx,
>      struct gntmap_entry *entry = map->entries + idx;
>      struct gnttab_map_grant_ref op;
>      int rc;
> +#ifndef CONFIG_PARAVIRT
> +    unsigned long pfn = map->start_pfn + idx;
> +#endif
>  
>      op.ref = (grant_ref_t) ref;
>      op.dom = (domid_t) domid;
> +#ifdef CONFIG_PARAVIRT
>      op.host_addr = (uint64_t) host_addr;
> +#else
> +    op.host_addr = (uint64_t)pfn << PAGE_SHIFT; 
> +#endif
>      op.flags = GNTMAP_host_map;
>      if (!writable)
>          op.flags |= GNTMAP_readonly;
> @@ -128,31 +167,18 @@ _gntmap_map_grant_ref(struct gntmap *map, int idx,
>          return rc != 0 ? rc : op.status;
>      }
>  
> -    entry->host_addr = host_addr;
> -    entry->handle = op.handle;
> -    return 0;
> -}
> -
> -static int
> -_gntmap_unmap_grant_ref(struct gntmap *map, int idx)
> -{
> -    struct gntmap_entry *entry = map->entries + idx;
> -    struct gnttab_unmap_grant_ref op;
> -    int rc;
> -
> -    op.host_addr    = (uint64_t) entry->host_addr;
> -    op.dev_bus_addr = 0;
> -    op.handle       = entry->handle;
> -
> -    rc = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1);
> -    if (rc != 0 || op.status != GNTST_okay) {
> -        printk("GNTTABOP_unmap_grant_ref failed: "
> -               "returned %d, status %" PRId16 "\n",
> -               rc, op.status);
> -        return rc != 0 ? rc : op.status;
> +#ifndef CONFIG_PARAVIRT
> +    rc = do_map_frames(host_addr, &pfn, 1, 0, 0, DOMID_SELF, NULL,
> +                       writable ? L1_PROT : L1_PROT_RO);
> +    if ( rc )
> +    {
> +        _gntmap_unmap_grant_ref(map, idx);
> +        return rc;
>      }
> +#endif
>  
> -    entry->host_addr = 0;
> +    entry->host_addr = host_addr;
> +    entry->handle = op.handle;
>      return 0;
>  }
>  
> @@ -165,6 +191,10 @@ gntmap_munmap(struct gntmap *map, unsigned long start_address, int count)
>      DEBUG("(map=%p, start_address=%lx, count=%d)",
>             map, start_address, count);
>  
> +#ifndef CONFIG_PARAVIRT
> +    unmap_frames(start_address, count);
> +#endif
> +
>      for (i = 0; i < count; i++) {
>          idx = gntmap_find_entry(map, start_address + PAGE_SIZE * i);
>          if (idx < 0) {
> @@ -242,6 +272,11 @@ gntmap_fini(struct gntmap *map)
>              (void) _gntmap_unmap_grant_ref(map, i);
>      }
>  
> +#ifndef CONFIG_PARAVIRT
> +    e820_put_reserved_pfns(map->start_pfn, map->nentries);
> +    map->start_pfn = 0;
> +#endif
> +
>      xfree(map->entries);
>      map->entries = NULL;
>      map->nentries = 0;
> diff --git a/include/gntmap.h b/include/gntmap.h
> index fde53f3..d3d7e88 100644
> --- a/include/gntmap.h
> +++ b/include/gntmap.h
> @@ -10,6 +10,7 @@
>  struct gntmap {
>      int nentries;
>      struct gntmap_entry *entries;
> +    unsigned long start_pfn;
>  };
>  
>  int
> -- 
> 2.26.2
> 

-- 
Samuel
 Yep. Moi j'ai un clavier à une touche. 
 Par contre, ma souris a 102 boutons, c'est pas toujours pratique.
 -+- OG in: Guide du Cabaliste Usenet - Le mulot contre attaque -+-

Re: [PATCH 02/10] mini-os: sort and sanitize e820 memory map

[Juergen Gross]

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

On 12.12.21 01:05, Samuel Thibault wrote:
> Hello,
> 
> Juergen Gross, le lun. 06 déc. 2021 08:23:29 +0100, a ecrit:
>> - align the entries to page boundaries
> 
>> +    /* Adjust map entries to page boundaries. */
>> +    for ( i = 0; i < e820_entries; i++ )
>> +    {
>> +        end = (e820_map[i].addr + e820_map[i].size + PAGE_SIZE - 1) & PAGE_MASK;
>> +        e820_map[i].addr &= PAGE_MASK;
>> +        e820_map[i].size = end - e820_map[i].addr;
>> +    }
> 
> Mmm, what if the previous entry ends after the aligned start?
> 
> On real machines that does happen, and you'd rather round up the start
> address of usable areas, rather than rounding it down (and conversely
> for the end).

I think you are partially right. :-)

Entries for resources managed by Mini-OS (RAM, maybe NVME?) should be
rounded to cover only complete pages (start rounded up, end rounded
down), but all other entries should be rounded to cover the complete
area (start rounded down, end rounded up) in order not to use any
partial used page for e.g. mapping foreign pages.

> 
>> +    /* Sort entries by start address. */
>> +    for ( i = 0; i < e820_entries - 1; i++ )
>> +    {
>> +        if ( e820_map[i].addr > e820_map[i + 1].addr )
>> +        {
>> +            e820_swap_entries(i, i + 1);
>> +            i = -1;
>> +        }
>> +    }
> 
> This looks O(n^3) to me? A bubble sort like this should be fine:
> 
>      /* Sort entries by start address. */
>      for ( last = e820_entries; last > 1; last-- )
>      {
>          for ( i = 0; i < last - 1; i++ )
>          {
>              if ( e820_map[i].addr > e820_map[i + 1].addr )
>              {
>                  e820_swap_entries(i, i + 1);
>              }
>          }
>      }

Hmm, depends.

Assuming a rather well sorted map my version is O(n), while yours
is still O(n^2).

In the end it won't matter that much, because a normal map will have
only very few entries (usually 5 before merging consecutive entries).

I'm fine both ways, whatever you prefer.


Juergen

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

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

Re: [PATCH 03/10] mini-os: don't assume contiguous RAM when initializing in PVH mode

[Juergen Gross]

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

On 12.12.21 01:15, Samuel Thibault wrote:
> Juergen Gross, le lun. 06 déc. 2021 08:23:30 +0100, a ecrit:
>> -    unsigned long pfn, max = 0;
>> +    unsigned long pfns, max = 0;
> 
> I'd say rather rename max to start.
> 
>>       e820_get_memmap();
>>   
>> @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void)
>>       {
>>           if ( e820_map[i].type != E820_RAM )
>>               continue;
>> -        pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
>> -        if ( pfn > max )
>> -            max = pfn;
>> +        pfns = e820_map[i].size >> PAGE_SHIFT;
>> +        max = e820_map[i].addr >> PAGE_SHIFT;
> 
> since it's it's always the start of the e820 entry.
> 
>> +        if ( pages <= pfns )
>> +            return max + pages;
>> +        pages -= pfns;
>> +        max += pfns;
> 
> Here we don't need do change max, only pages.

It is needed in case the loop is finished.

And this was the reason for naming it max.


Juergen

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

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

Re: [PATCH 04/10] mini-os: respect memory map when ballooning up

[Juergen Gross]

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

On 12.12.21 01:26, Samuel Thibault wrote:
> Juergen Gross, le lun. 06 déc. 2021 08:23:31 +0100, a ecrit:
>> @@ -81,8 +93,11 @@ int balloon_up(unsigned long n_pages)
>>       if ( n_pages > N_BALLOON_FRAMES )
>>           n_pages = N_BALLOON_FRAMES;
>>   
>> +    start_pfn = e820_get_maxpfn(nr_mem_pages + 1) - 1;
>> +    n_pages = e820_get_max_pages(start_pfn, n_pages);
> 
> I'd say call it e820_get_max_contig_pages?

Fine with me.

> 
>> +unsigned long e820_get_max_pages(unsigned long pfn, unsigned long pages)
>> +{
>> +    int i;
>> +    unsigned long end;
>> +
>> +    for ( i = 0; i < e820_entries; i++ )
>> +    {
>> +        if ( e820_map[i].type != E820_RAM ||
>> +             (e820_map[i].addr >> PAGE_SHIFT) > pfn )
>> +            continue;
> 
> "> pfn" looks odd to me? If the start of the e820 entry is already
> beyond pfn, we'll never find any other entry. We however do want to skip
> entries that have addr+size that is below pfn.

Oh, you are right.


Juergen

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

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

Re: [PATCH 02/10] mini-os: sort and sanitize e820 memory map

[Samuel Thibault]

Juergen Gross, le lun. 13 déc. 2021 15:56:21 +0100, a ecrit:
> On 12.12.21 01:05, Samuel Thibault wrote:
> > Hello,
> > 
> > Juergen Gross, le lun. 06 déc. 2021 08:23:29 +0100, a ecrit:
> > > - align the entries to page boundaries
> > 
> > > +    /* Adjust map entries to page boundaries. */
> > > +    for ( i = 0; i < e820_entries; i++ )
> > > +    {
> > > +        end = (e820_map[i].addr + e820_map[i].size + PAGE_SIZE - 1) & PAGE_MASK;
> > > +        e820_map[i].addr &= PAGE_MASK;
> > > +        e820_map[i].size = end - e820_map[i].addr;
> > > +    }
> > 
> > Mmm, what if the previous entry ends after the aligned start?
> > 
> > On real machines that does happen, and you'd rather round up the start
> > address of usable areas, rather than rounding it down (and conversely
> > for the end).
> 
> I think you are partially right. :-)
> 
> Entries for resources managed by Mini-OS (RAM, maybe NVME?) should be
> rounded to cover only complete pages (start rounded up, end rounded
> down), but all other entries should be rounded to cover the complete
> area (start rounded down, end rounded up) in order not to use any
> partial used page for e.g. mapping foreign pages.

Right!

> > > +    /* Sort entries by start address. */
> > > +    for ( i = 0; i < e820_entries - 1; i++ )
> > > +    {
> > > +        if ( e820_map[i].addr > e820_map[i + 1].addr )
> > > +        {
> > > +            e820_swap_entries(i, i + 1);
> > > +            i = -1;
> > > +        }
> > > +    }
> > 
> > This looks O(n^3) to me? A bubble sort like this should be fine:
> > 
> >      /* Sort entries by start address. */
> >      for ( last = e820_entries; last > 1; last-- )
> >      {
> >          for ( i = 0; i < last - 1; i++ )
> >          {
> >              if ( e820_map[i].addr > e820_map[i + 1].addr )
> >              {
> >                  e820_swap_entries(i, i + 1);
> >              }
> >          }
> >      }
> 
> Hmm, depends.
> 
> Assuming a rather well sorted map my version is O(n), while yours
> is still O(n^2).

Right, I was a bit lazy :)

This should be fine:

/* Sort entries by start address. */
for ( i = 1; i < e820_entries; i++ )
    for ( j = i; j > 0 && e820_map[j-1].addr > e820_map[j].addr ) ; j-- )
        e820_swap_entries(j - 1, j);

> I'm fine both ways, whatever you prefer.

I really prefer for loops which don't unexpectedly modify their loop
index, that's much less scary :)

Samuel

Re: [PATCH 03/10] mini-os: don't assume contiguous RAM when initializing in PVH mode

[Samuel Thibault]

Juergen Gross, le lun. 13 déc. 2021 15:58:58 +0100, a ecrit:
> On 12.12.21 01:15, Samuel Thibault wrote:
> > Juergen Gross, le lun. 06 déc. 2021 08:23:30 +0100, a ecrit:
> > > -    unsigned long pfn, max = 0;
> > > +    unsigned long pfns, max = 0;
> > 
> > I'd say rather rename max to start.
> > 
> > >       e820_get_memmap();
> > > @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void)
> > >       {
> > >           if ( e820_map[i].type != E820_RAM )
> > >               continue;
> > > -        pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
> > > -        if ( pfn > max )
> > > -            max = pfn;
> > > +        pfns = e820_map[i].size >> PAGE_SHIFT;
> > > +        max = e820_map[i].addr >> PAGE_SHIFT;
> > 
> > since it's it's always the start of the e820 entry.
> > 
> > > +        if ( pages <= pfns )
> > > +            return max + pages;
> > > +        pages -= pfns;
> > > +        max += pfns;
> > 
> > Here we don't need do change max, only pages.
> 
> It is needed in case the loop is finished.
> 
> And this was the reason for naming it max.

Ah, ok.

At first read the name was confusing me. Perhaps better use two
variables then: start and max, so that we have

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

Samuel

Re: [PATCH 02/10] mini-os: sort and sanitize e820 memory map

[Juergen Gross]

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

On 13.12.21 22:19, Samuel Thibault wrote:
> Juergen Gross, le lun. 13 déc. 2021 15:56:21 +0100, a ecrit:
>> On 12.12.21 01:05, Samuel Thibault wrote:
>>> Hello,
>>>
>>> Juergen Gross, le lun. 06 déc. 2021 08:23:29 +0100, a ecrit:
>>>> - align the entries to page boundaries
>>>
>>>> +    /* Adjust map entries to page boundaries. */
>>>> +    for ( i = 0; i < e820_entries; i++ )
>>>> +    {
>>>> +        end = (e820_map[i].addr + e820_map[i].size + PAGE_SIZE - 1) & PAGE_MASK;
>>>> +        e820_map[i].addr &= PAGE_MASK;
>>>> +        e820_map[i].size = end - e820_map[i].addr;
>>>> +    }
>>>
>>> Mmm, what if the previous entry ends after the aligned start?
>>>
>>> On real machines that does happen, and you'd rather round up the start
>>> address of usable areas, rather than rounding it down (and conversely
>>> for the end).
>>
>> I think you are partially right. :-)
>>
>> Entries for resources managed by Mini-OS (RAM, maybe NVME?) should be
>> rounded to cover only complete pages (start rounded up, end rounded
>> down), but all other entries should be rounded to cover the complete
>> area (start rounded down, end rounded up) in order not to use any
>> partial used page for e.g. mapping foreign pages.
> 
> Right!
> 
>>>> +    /* Sort entries by start address. */
>>>> +    for ( i = 0; i < e820_entries - 1; i++ )
>>>> +    {
>>>> +        if ( e820_map[i].addr > e820_map[i + 1].addr )
>>>> +        {
>>>> +            e820_swap_entries(i, i + 1);
>>>> +            i = -1;
>>>> +        }
>>>> +    }
>>>
>>> This looks O(n^3) to me? A bubble sort like this should be fine:
>>>
>>>       /* Sort entries by start address. */
>>>       for ( last = e820_entries; last > 1; last-- )
>>>       {
>>>           for ( i = 0; i < last - 1; i++ )
>>>           {
>>>               if ( e820_map[i].addr > e820_map[i + 1].addr )
>>>               {
>>>                   e820_swap_entries(i, i + 1);
>>>               }
>>>           }
>>>       }
>>
>> Hmm, depends.
>>
>> Assuming a rather well sorted map my version is O(n), while yours
>> is still O(n^2).
> 
> Right, I was a bit lazy :)
> 
> This should be fine:
> 
> /* Sort entries by start address. */
> for ( i = 1; i < e820_entries; i++ )
>      for ( j = i; j > 0 && e820_map[j-1].addr > e820_map[j].addr ) ; j-- )
>          e820_swap_entries(j - 1, j);
> 
>> I'm fine both ways, whatever you prefer.
> 
> I really prefer for loops which don't unexpectedly modify their loop
> index, that's much less scary :)

Agreed, I'll take your version.


Juergen

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

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

Re: [PATCH 03/10] mini-os: don't assume contiguous RAM when initializing in PVH mode

[Juergen Gross]

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

On 13.12.21 22:22, Samuel Thibault wrote:
> Juergen Gross, le lun. 13 déc. 2021 15:58:58 +0100, a ecrit:
>> On 12.12.21 01:15, Samuel Thibault wrote:
>>> Juergen Gross, le lun. 06 déc. 2021 08:23:30 +0100, a ecrit:
>>>> -    unsigned long pfn, max = 0;
>>>> +    unsigned long pfns, max = 0;
>>>
>>> I'd say rather rename max to start.
>>>
>>>>        e820_get_memmap();
>>>> @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void)
>>>>        {
>>>>            if ( e820_map[i].type != E820_RAM )
>>>>                continue;
>>>> -        pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
>>>> -        if ( pfn > max )
>>>> -            max = pfn;
>>>> +        pfns = e820_map[i].size >> PAGE_SHIFT;
>>>> +        max = e820_map[i].addr >> PAGE_SHIFT;
>>>
>>> since it's it's always the start of the e820 entry.
>>>
>>>> +        if ( pages <= pfns )
>>>> +            return max + pages;
>>>> +        pages -= pfns;
>>>> +        max += pfns;
>>>
>>> Here we don't need do change max, only pages.
>>
>> It is needed in case the loop is finished.
>>
>> And this was the reason for naming it max.
> 
> Ah, ok.
> 
> At first read the name was confusing me. Perhaps better use two
> variables then: start and max, so that we have
> 
> start = e820_map[i].addr >> PAGE_SHIFT;
> if ( pages <= pfns )
>      return start + pages;
> pages -= pfns;
> max = start + pfns;

Hmm, or I can rename max to start, drop the "max += pfns;" and do a
"return start + pfns;" at the end of the function.


Juergen

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

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

Re: [PATCH 03/10] mini-os: don't assume contiguous RAM when initializing in PVH mode

[Samuel Thibault]

Juergen Gross, le mar. 14 déc. 2021 07:35:54 +0100, a ecrit:
> On 13.12.21 22:22, Samuel Thibault wrote:
> > Juergen Gross, le lun. 13 déc. 2021 15:58:58 +0100, a ecrit:
> > > On 12.12.21 01:15, Samuel Thibault wrote:
> > > > Juergen Gross, le lun. 06 déc. 2021 08:23:30 +0100, a ecrit:
> > > > > -    unsigned long pfn, max = 0;
> > > > > +    unsigned long pfns, max = 0;
> > > > 
> > > > I'd say rather rename max to start.
> > > > 
> > > > >        e820_get_memmap();
> > > > > @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void)
> > > > >        {
> > > > >            if ( e820_map[i].type != E820_RAM )
> > > > >                continue;
> > > > > -        pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
> > > > > -        if ( pfn > max )
> > > > > -            max = pfn;
> > > > > +        pfns = e820_map[i].size >> PAGE_SHIFT;
> > > > > +        max = e820_map[i].addr >> PAGE_SHIFT;
> > > > 
> > > > since it's it's always the start of the e820 entry.
> > > > 
> > > > > +        if ( pages <= pfns )
> > > > > +            return max + pages;
> > > > > +        pages -= pfns;
> > > > > +        max += pfns;
> > > > 
> > > > Here we don't need do change max, only pages.
> > > 
> > > It is needed in case the loop is finished.
> > > 
> > > And this was the reason for naming it max.
> > 
> > Ah, ok.
> > 
> > At first read the name was confusing me. Perhaps better use two
> > variables then: start and max, so that we have
> > 
> > start = e820_map[i].addr >> PAGE_SHIFT;
> > if ( pages <= pfns )
> >      return start + pages;
> > pages -= pfns;
> > max = start + pfns;
> 
> Hmm, or I can rename max to start, drop the "max += pfns;" and do a
> "return start + pfns;" at the end of the function.

That could do as well, yes.

Samuel