[PATCH v2 0/4] x86/shim: minor fixes to the pv-shim mode

[PATCH v2 0/4] x86/shim: minor fixes to the pv-shim mode

[Roger Pau Monne]

Hello,

This series includes some miscellaneous fixes for the pv-shim mode,
specially regarding the handling of the memory map.

It can be found on my git branch:

git://xenbits.xen.org/people/royger/xen.git guest-fixes-v2

Thanks, Roger.

Roger Pau Monne (4):
  x86/e820: introduce a function to remove ranges from e820
  x86/e820: do not fixup memmap in copy_e820_map
  x86/e820: assume memmap provided when booted virtualized is correct
  x86/shim: only mark special pages as RAM in pvshim mode

 xen/arch/x86/e820.c             | 88 +++++++++++++++++++++++++++------
 xen/arch/x86/guest/xen.c        | 42 ----------------
 xen/arch/x86/pv/shim.c          | 43 ++++++++++++++++
 xen/common/page_alloc.c         |  4 +-
 xen/include/asm-x86/e820.h      |  2 +
 xen/include/asm-x86/guest/xen.h | 13 -----
 xen/include/asm-x86/pv/shim.h   | 12 +++++
 7 files changed, 131 insertions(+), 73 deletions(-)

-- 
2.17.2 (Apple Git-113)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 1/4] x86/e820: introduce a function to remove ranges from e820

[Roger Pau Monne]

This function is based on the Linux e820__range_remove function,
adjusted to fit Xen.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v1:
 - Fix one coding style issue.
---
 xen/arch/x86/e820.c        | 57 ++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/e820.h |  2 ++
 2 files changed, 59 insertions(+)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 590ea985ef..ba81b2f2dd 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -599,6 +599,63 @@ int __init e820_add_range(
     return 1;
 }
 
+uint64_t __init e820_remove_range(struct e820map *e820, uint64_t start,
+                                  uint64_t end, uint32_t type, bool check_type)
+{
+    unsigned int i;
+    uint64_t real_removed_size = 0;
+
+    ASSERT(end > start);
+
+    for ( i = 0; i < e820->nr_map; i++ )
+    {
+        struct e820entry *entry = &e820->map[i];
+        uint64_t final_start, final_end, entry_end;
+
+        if ( check_type && entry->type != type )
+            continue;
+
+        entry_end = entry->addr + entry->size;
+
+        /* Completely covered? */
+        if ( entry->addr >= start && entry_end <= end )
+        {
+            real_removed_size += entry->size;
+            memset(entry, 0, sizeof(*entry));
+            continue;
+        }
+
+        /* Is the new range completely covered? */
+        if ( entry->addr < start && entry_end > end )
+        {
+            e820_add_range(e820, end, entry_end, entry->type);
+            entry->size = start - entry->addr;
+            real_removed_size += end - start;
+            continue;
+        }
+
+        /* Partially covered: */
+        final_start = max(start, entry->addr);
+        final_end = min(end, entry_end);
+        if ( final_start >= final_end )
+            continue;
+
+        real_removed_size += final_end - final_start;
+
+        /*
+         * Left range could be head or tail, so need to update
+         * the size first:
+         */
+        entry->size -= final_end - final_start;
+        if ( entry->addr < final_start )
+            continue;
+
+        entry->addr = final_end;
+    }
+
+    return real_removed_size;
+}
+
 int __init e820_change_range_type(
     struct e820map *e820, uint64_t s, uint64_t e,
     uint32_t orig_type, uint32_t new_type)
diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
index ee317b17aa..f77b821ae9 100644
--- a/xen/include/asm-x86/e820.h
+++ b/xen/include/asm-x86/e820.h
@@ -31,6 +31,8 @@ extern int e820_change_range_type(
     uint32_t orig_type, uint32_t new_type);
 extern int e820_add_range(
     struct e820map *, uint64_t s, uint64_t e, uint32_t type);
+extern uint64_t e820_remove_range(struct e820map *e820, uint64_t start,
+                                  uint64_t end, uint32_t type, bool check_type);
 extern unsigned long init_e820(const char *, struct e820map *);
 extern struct e820map e820;
 extern struct e820map e820_raw;
-- 
2.17.2 (Apple Git-113)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/4] x86/e820: do not fixup memmap in copy_e820_map

[Roger Pau Monne]

And instead use the newly introduced e820_remove_range helper to
remove any RAM region from the low 1MB VGA/ROM region afterwards.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/e820.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index ba81b2f2dd..5b63667087 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -320,20 +320,6 @@ static int __init copy_e820_map(struct e820entry * biosmap, int nr_map)
         if (start > end)
             return -1;
 
-        /*
-         * Some BIOSes claim RAM in the 640k - 1M region.
-         * Not right. Fix it up.
-         */
-        if (type == E820_RAM) {
-            if (start < 0x100000ULL && end > 0xA0000ULL) {
-                if (start < 0xA0000ULL)
-                    add_memory_region(start, 0xA0000ULL-start, type);
-                if (end <= 0x100000ULL)
-                    continue;
-                start = 0x100000ULL;
-                size = end - start;
-            }
-        }
         add_memory_region(start, size, type);
     } while (biosmap++,--nr_map);
     return 0;
@@ -510,6 +496,12 @@ static void __init reserve_dmi_region(void)
     }
 }
 
+static void __init reserve_vga_region(void)
+{
+    /* Remove any RAM regions from the VGA hole. */
+    e820_remove_range(&e820, KB(640), MB(1) - 1, E820_RAM, true);
+}
+
 static void __init machine_specific_memory_setup(struct e820map *raw)
 {
     unsigned long mpt_limit, ro_mpt_limit;
@@ -545,6 +537,12 @@ static void __init machine_specific_memory_setup(struct e820map *raw)
 
     reserve_dmi_region();
 
+    /*
+     * Some BIOSes claim RAM in the 640k - 1M region.
+     * Not right. Fix it up.
+     */
+    reserve_vga_region();
+
     top_of_ram = mtrr_top_of_ram();
     if ( top_of_ram )
         clip_to_limit(top_of_ram, "MTRRs do not cover all of memory.");
-- 
2.17.2 (Apple Git-113)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 3/4] x86/e820: assume memmap provided when booted virtualized is correct

[Roger Pau Monne]

This implies there's no need to forcefully reserve the VGA MMIO
region, since the memory map provided will be correct.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@cirix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v1:
 - Assume the memory map is always correct when booted as a guest
   under any hypervisor, not only Xen.
---
 xen/arch/x86/e820.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 5b63667087..2407367c57 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -537,11 +537,12 @@ static void __init machine_specific_memory_setup(struct e820map *raw)
 
     reserve_dmi_region();
 
-    /*
-     * Some BIOSes claim RAM in the 640k - 1M region.
-     * Not right. Fix it up.
-     */
-    reserve_vga_region();
+    if ( !cpu_has_hypervisor )
+        /*
+         * Some BIOSes claim RAM in the 640k - 1M region.
+         * Not right. Fix it up.
+         */
+        reserve_vga_region();
 
     top_of_ram = mtrr_top_of_ram();
     if ( top_of_ram )
-- 
2.17.2 (Apple Git-113)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 4/4] x86/shim: only mark special pages as RAM in pvshim mode

[Roger Pau Monne]

When running Xen as a guest it's not necessary to mark such pages as
RAM because they won't be assigned to the initial domain memory map.

While there move the functions to the PV shim specific file and rename
them accordingly.

No functional change expected.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/e820.c             |  4 +--
 xen/arch/x86/guest/xen.c        | 42 --------------------------------
 xen/arch/x86/pv/shim.c          | 43 +++++++++++++++++++++++++++++++++
 xen/common/page_alloc.c         |  4 +--
 xen/include/asm-x86/guest/xen.h | 13 ----------
 xen/include/asm-x86/pv/shim.h   | 12 +++++++++
 6 files changed, 59 insertions(+), 59 deletions(-)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 2407367c57..32a4e9a57e 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -756,8 +756,8 @@ unsigned long __init init_e820(const char *str, struct e820map *raw)
 
     machine_specific_memory_setup(raw);
 
-    if ( xen_guest )
-        hypervisor_fixup_e820(&e820);
+    if ( pv_shim )
+        pv_shim_fixup_e820(&e820);
 
     printk("%s RAM map:\n", str);
     print_e820_memory_map(e820.map, e820.nr_map);
diff --git a/xen/arch/x86/guest/xen.c b/xen/arch/x86/guest/xen.c
index 8cee880adc..7b7a5badab 100644
--- a/xen/arch/x86/guest/xen.c
+++ b/xen/arch/x86/guest/xen.c
@@ -40,7 +40,6 @@ bool __read_mostly xen_guest;
 static __read_mostly uint32_t xen_cpuid_base;
 extern char hypercall_page[];
 static struct rangeset *mem;
-static struct platform_bad_page __initdata reserved_pages[2];
 
 DEFINE_PER_CPU(unsigned int, vcpu_id);
 
@@ -302,47 +301,6 @@ int hypervisor_free_unused_page(mfn_t mfn)
     return rangeset_remove_range(mem, mfn_x(mfn), mfn_x(mfn));
 }
 
-static void __init mark_pfn_as_ram(struct e820map *e820, uint64_t pfn)
-{
-    if ( !e820_add_range(e820, pfn << PAGE_SHIFT,
-                         (pfn << PAGE_SHIFT) + PAGE_SIZE, E820_RAM) )
-        if ( !e820_change_range_type(e820, pfn << PAGE_SHIFT,
-                                     (pfn << PAGE_SHIFT) + PAGE_SIZE,
-                                     E820_RESERVED, E820_RAM) )
-            panic("Unable to add/change memory type of pfn %#lx to RAM\n", pfn);
-}
-
-void __init hypervisor_fixup_e820(struct e820map *e820)
-{
-    uint64_t pfn = 0;
-    unsigned int i = 0;
-    long rc;
-
-    ASSERT(xen_guest);
-
-#define MARK_PARAM_RAM(p) ({                    \
-    rc = xen_hypercall_hvm_get_param(p, &pfn);  \
-    if ( rc )                                   \
-        panic("Unable to get " #p "\n");        \
-    mark_pfn_as_ram(e820, pfn);                 \
-    ASSERT(i < ARRAY_SIZE(reserved_pages));     \
-    reserved_pages[i++].mfn = pfn;              \
-})
-    MARK_PARAM_RAM(HVM_PARAM_STORE_PFN);
-    if ( !pv_console )
-        MARK_PARAM_RAM(HVM_PARAM_CONSOLE_PFN);
-#undef MARK_PARAM_RAM
-}
-
-const struct platform_bad_page *__init hypervisor_reserved_pages(unsigned int *size)
-{
-    ASSERT(xen_guest);
-
-    *size = ARRAY_SIZE(reserved_pages);
-
-    return reserved_pages;
-}
-
 uint32_t hypervisor_cpuid_base(void)
 {
     return xen_cpuid_base;
diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index cdc72f787d..551bc5c8d8 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -54,6 +54,8 @@ static DEFINE_SPINLOCK(grant_lock);
 static PAGE_LIST_HEAD(balloon);
 static DEFINE_SPINLOCK(balloon_lock);
 
+static struct platform_bad_page __initdata reserved_pages[2];
+
 static long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
 static long pv_shim_grant_table_op(unsigned int cmd,
                                    XEN_GUEST_HANDLE_PARAM(void) uop,
@@ -113,6 +115,47 @@ uint64_t pv_shim_mem(uint64_t avail)
     return shim_nrpages;
 }
 
+static void __init mark_pfn_as_ram(struct e820map *e820, uint64_t pfn)
+{
+    if ( !e820_add_range(e820, pfn << PAGE_SHIFT,
+                         (pfn << PAGE_SHIFT) + PAGE_SIZE, E820_RAM) )
+        if ( !e820_change_range_type(e820, pfn << PAGE_SHIFT,
+                                     (pfn << PAGE_SHIFT) + PAGE_SIZE,
+                                     E820_RESERVED, E820_RAM) )
+            panic("Unable to add/change memory type of pfn %#lx to RAM\n", pfn);
+}
+
+void pv_shim_fixup_e820(struct e820map *e820)
+{
+    uint64_t pfn = 0;
+    unsigned int i = 0;
+    long rc;
+
+    ASSERT(xen_guest);
+
+#define MARK_PARAM_RAM(p) ({                    \
+    rc = xen_hypercall_hvm_get_param(p, &pfn);  \
+    if ( rc )                                   \
+        panic("Unable to get " #p "\n");        \
+    mark_pfn_as_ram(e820, pfn);                 \
+    ASSERT(i < ARRAY_SIZE(reserved_pages));     \
+    reserved_pages[i++].mfn = pfn;              \
+})
+    MARK_PARAM_RAM(HVM_PARAM_STORE_PFN);
+    if ( !pv_console )
+        MARK_PARAM_RAM(HVM_PARAM_CONSOLE_PFN);
+#undef MARK_PARAM_RAM
+}
+
+const struct platform_bad_page *__init pv_shim_reserved_pages(unsigned int *size)
+{
+    ASSERT(xen_guest);
+
+    *size = ARRAY_SIZE(reserved_pages);
+
+    return reserved_pages;
+}
+
 #define L1_PROT (_PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_USER| \
                  _PAGE_GUEST_KERNEL)
 #define COMPAT_L1_PROT (_PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED)
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index e591601f9c..37a52aaa0d 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -348,9 +348,9 @@ void __init init_boot_pages(paddr_t ps, paddr_t pe)
         }
     }
 
-    if ( xen_guest )
+    if ( pv_shim )
     {
-        badpage = hypervisor_reserved_pages(&array_size);
+        badpage = pv_shim_reserved_pages(&array_size);
         if ( badpage )
         {
             for ( i = 0; i < array_size; i++ )
diff --git a/xen/include/asm-x86/guest/xen.h b/xen/include/asm-x86/guest/xen.h
index 6f15e24b6b..7e04e4a7ab 100644
--- a/xen/include/asm-x86/guest/xen.h
+++ b/xen/include/asm-x86/guest/xen.h
@@ -36,8 +36,6 @@ void hypervisor_setup(void);
 void hypervisor_ap_setup(void);
 int hypervisor_alloc_unused_page(mfn_t *mfn);
 int hypervisor_free_unused_page(mfn_t mfn);
-void hypervisor_fixup_e820(struct e820map *e820);
-const struct platform_bad_page *hypervisor_reserved_pages(unsigned int *size);
 uint32_t hypervisor_cpuid_base(void);
 void hypervisor_resume(void);
 
@@ -60,17 +58,6 @@ static inline void hypervisor_ap_setup(void)
     ASSERT_UNREACHABLE();
 }
 
-static inline void hypervisor_fixup_e820(struct e820map *e820)
-{
-    ASSERT_UNREACHABLE();
-}
-
-static inline const struct platform_bad_page *hypervisor_reserved_pages(unsigned int *size)
-{
-    ASSERT_UNREACHABLE();
-    return NULL;
-}
-
 #endif /* CONFIG_XEN_GUEST */
 #endif /* __X86_GUEST_XEN_H__ */
 
diff --git a/xen/include/asm-x86/pv/shim.h b/xen/include/asm-x86/pv/shim.h
index fb739772df..b8818dfde7 100644
--- a/xen/include/asm-x86/pv/shim.h
+++ b/xen/include/asm-x86/pv/shim.h
@@ -43,6 +43,8 @@ void pv_shim_online_memory(unsigned int nr, unsigned int order);
 void pv_shim_offline_memory(unsigned int nr, unsigned int order);
 domid_t get_initial_domain_id(void);
 uint64_t pv_shim_mem(uint64_t avail);
+void pv_shim_fixup_e820(struct e820map *e820);
+const struct platform_bad_page *pv_shim_reserved_pages(unsigned int *size);
 
 #else
 
@@ -91,6 +93,16 @@ static inline uint64_t pv_shim_mem(uint64_t avail)
     ASSERT_UNREACHABLE();
     return 0;
 }
+static inline void pv_shim_fixup_e820(struct e820map *e820)
+{
+    ASSERT_UNREACHABLE();
+}
+static inline const struct platform_bad_page
+*pv_shim_reserved_pages(unsigned int *s)
+{
+    ASSERT_UNREACHABLE();
+    return NULL;
+}
 
 #endif
 
-- 
2.17.2 (Apple Git-113)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 0/4] x86/shim: minor fixes to the pv-shim mode

[Andrew Cooper]

On 28/12/2018 12:04, Roger Pau Monne wrote:
> Hello,
>
> This series includes some miscellaneous fixes for the pv-shim mode,
> specially regarding the handling of the memory map.

Unfortunately, something overall is now wonky.  With this series
applied, I'm getting:

(d9) (XEN) Initial PVH-e820 RAM map:
(d9) (XEN)  0000000000000000 - 00000000fc000000 (usable)
(d9) (XEN)  00000000fc000000 - 00000000fc009040 (ACPI data)
(d9) (XEN)  00000000feff8000 - 00000000ff000000 (reserved)
(d9) (XEN)  0000000100000000 - 0000000104000400 (usable)
(d9) (XEN) Checking MTRR ranges...
(d9) (XEN)  MTRR cap: 508 type: 806
(d9) (XEN) PVH-e820 RAM map:
(d9) (XEN)  0000000000000000 - 00000000000a0000 (usable)
(d9) (XEN)  00000000fc000000 - 00000000fc009040 (ACPI data)
(d9) (XEN)  00000000feff8000 - 00000000ff000000 (reserved)
(d9) (XEN)  0000000100000000 - 0000000104000400 (usable)

which a) is trying to account for the legacy range despite starting as a
PVH guest.  (I think that perhaps cpu_has_hypervisor doesn't work that
early, and I'll try making a fix for it.)

b) The entire remainder of the RAM block which covered the legacy hole
got deleted, so Xen failed to relocate itself and panic()'d

c) Xen really oughtn't to relocate itself in cases like this, but that
is way out of scope here.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 0/4] x86/shim: minor fixes to the pv-shim mode

[Roger Pau Monné]

On Fri, Dec 28, 2018 at 02:58:49PM +0000, Andrew Cooper wrote:
> On 28/12/2018 12:04, Roger Pau Monne wrote:
> > Hello,
> >
> > This series includes some miscellaneous fixes for the pv-shim mode,
> > specially regarding the handling of the memory map.
> 
> Unfortunately, something overall is now wonky.  With this series
> applied, I'm getting:
> 
> (d9) (XEN) Initial PVH-e820 RAM map:
> (d9) (XEN)  0000000000000000 - 00000000fc000000 (usable)
> (d9) (XEN)  00000000fc000000 - 00000000fc009040 (ACPI data)
> (d9) (XEN)  00000000feff8000 - 00000000ff000000 (reserved)
> (d9) (XEN)  0000000100000000 - 0000000104000400 (usable)
> (d9) (XEN) Checking MTRR ranges...
> (d9) (XEN)  MTRR cap: 508 type: 806
> (d9) (XEN) PVH-e820 RAM map:
> (d9) (XEN)  0000000000000000 - 00000000000a0000 (usable)
> (d9) (XEN)  00000000fc000000 - 00000000fc009040 (ACPI data)
> (d9) (XEN)  00000000feff8000 - 00000000ff000000 (reserved)
> (d9) (XEN)  0000000100000000 - 0000000104000400 (usable)
> 
> which a) is trying to account for the legacy range despite starting as a
> PVH guest.  (I think that perhaps cpu_has_hypervisor doesn't work that
> early, and I'll try making a fix for it.)

I've likely tested an stall version of the shim binary, sorry.

> b) The entire remainder of the RAM block which covered the legacy hole
> got deleted, so Xen failed to relocate itself and panic()'d

Hm, so the Linux function doesn't seem to work as expected.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/4] x86/e820: do not fixup memmap in copy_e820_map

[Wei Liu]

On Fri, Dec 28, 2018 at 01:04:02PM +0100, Roger Pau Monne wrote:
> And instead use the newly introduced e820_remove_range helper to
> remove any RAM region from the low 1MB VGA/ROM region afterwards.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/4] x86/e820: assume memmap provided when booted virtualized is correct

[Wei Liu]

On Fri, Dec 28, 2018 at 01:04:03PM +0100, Roger Pau Monne wrote:
> This implies there's no need to forcefully reserve the VGA MMIO
> region, since the memory map provided will be correct.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@cirix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/4] x86/e820: introduce a function to remove ranges from e820

[Jan Beulich]

>>> On 28.12.18 at 13:04, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -599,6 +599,63 @@ int __init e820_add_range(
>      return 1;
>  }
>  
> +uint64_t __init e820_remove_range(struct e820map *e820, uint64_t start,
> +                                  uint64_t end, uint32_t type, bool check_type)
> +{
> +    unsigned int i;
> +    uint64_t real_removed_size = 0;
> +
> +    ASSERT(end > start);
> +
> +    for ( i = 0; i < e820->nr_map; i++ )
> +    {
> +        struct e820entry *entry = &e820->map[i];
> +        uint64_t final_start, final_end, entry_end;
> +
> +        if ( check_type && entry->type != type )
> +            continue;
> +
> +        entry_end = entry->addr + entry->size;
> +
> +        /* Completely covered? */
> +        if ( entry->addr >= start && entry_end <= end )
> +        {
> +            real_removed_size += entry->size;
> +            memset(entry, 0, sizeof(*entry));

This will break assumptions by other functions, e.g. the
neighboring e820_add_range().

> +            continue;
> +        }
> +
> +        /* Is the new range completely covered? */
> +        if ( entry->addr < start && entry_end > end )

Why < and > instead of <= and >= ? At the very least this is
not in line with the comment.

> +        {
> +            e820_add_range(e820, end, entry_end, entry->type);
> +            entry->size = start - entry->addr;
> +            real_removed_size += end - start;
> +            continue;
> +        }
> +
> +        /* Partially covered: */
> +        final_start = max(start, entry->addr);
> +        final_end = min(end, entry_end);
> +        if ( final_start >= final_end )
> +            continue;

Isn't this supposed to be unreachable?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/4] x86/e820: do not fixup memmap in copy_e820_map

[Jan Beulich]

>>> On 28.12.18 at 13:04, <roger.pau@citrix.com> wrote:
> @@ -510,6 +496,12 @@ static void __init reserve_dmi_region(void)
>      }
>  }
>  
> +static void __init reserve_vga_region(void)
> +{
> +    /* Remove any RAM regions from the VGA hole. */
> +    e820_remove_range(&e820, KB(640), MB(1) - 1, E820_RAM, true);

From looking at patch 1 I got the impression that the "end" parameter
is exclusive. If that's not intended to be that way, then there may
be further issues with the use of < / <= / > / >= .

I'm also unconvinced of this getting put in a separate function.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 4/4] x86/shim: only mark special pages as RAM in pvshim mode

[Jan Beulich]

>>> On 28.12.18 at 13:04, <roger.pau@citrix.com> wrote:
> @@ -113,6 +115,47 @@ uint64_t pv_shim_mem(uint64_t avail)
>      return shim_nrpages;
>  }
>  
> +static void __init mark_pfn_as_ram(struct e820map *e820, uint64_t pfn)
> +{
> +    if ( !e820_add_range(e820, pfn << PAGE_SHIFT,
> +                         (pfn << PAGE_SHIFT) + PAGE_SIZE, E820_RAM) )
> +        if ( !e820_change_range_type(e820, pfn << PAGE_SHIFT,
> +                                     (pfn << PAGE_SHIFT) + PAGE_SIZE,
> +                                     E820_RESERVED, E820_RAM) )
> +            panic("Unable to add/change memory type of pfn %#lx to RAM\n", pfn);

While doing the move it would have been nice if you folded the two
if()-s. (Arguably pfn_to_paddr() could also be used here.)

> +void pv_shim_fixup_e820(struct e820map *e820)

You've lost the __init here.

> @@ -91,6 +93,16 @@ static inline uint64_t pv_shim_mem(uint64_t avail)
>      ASSERT_UNREACHABLE();
>      return 0;
>  }
> +static inline void pv_shim_fixup_e820(struct e820map *e820)
> +{
> +    ASSERT_UNREACHABLE();
> +}
> +static inline const struct platform_bad_page
> +*pv_shim_reserved_pages(unsigned int *s)

I can see why you split the previously too long line, but surely it
shouldn't be split in the middle of the return type.

I've taken the liberty and made all three modifications in preparation
for committing.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/4] x86/e820: assume memmap provided when booted virtualized is correct

[Jan Beulich]

>>> On 02.01.19 at 13:44, <wei.liu2@citrix.com> wrote:
> On Fri, Dec 28, 2018 at 01:04:03PM +0100, Roger Pau Monne wrote:
>> This implies there's no need to forcefully reserve the VGA MMIO
>> region, since the memory map provided will be correct.
>> 
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Roger Pau Monné <roger.pau@cirix.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
once suitably re-based over whatever changes to the earlier
patches will be done.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel