[Xen-devel] [PATCH v6 00/11] More Hyper-V infrastructures

[Xen-devel] [PATCH v6 00/11] More Hyper-V infrastructures

[Wei Liu]

This patch sereis implements several important functionalities to run
Xen on top of Hyper-V. See individual patches for more details.

I've checked the assembly code as well as putting in a test patch to
make sure the hypercall interface is implemented correctly.

Wei.

Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Michael Kelley <mikelley@microsoft.com>
Cc: Paul Durrant <pdurrant@amazon.com>

Wei Liu (11):
  x86/hypervisor: make hypervisor_ap_setup return an error code
  x86/smp: don't online cpu if hypervisor_ap_setup fails
  x86: provide executable fixmap facility
  x86/hypervisor: provide hypervisor_fixup_e820
  x86/hyperv: setup hypercall page
  x86/hyperv: provide Hyper-V hypercall functions
  DO NOT APPLY: x86/hyperv: issue an hypercall
  x86/hyperv: provide percpu hypercall input page
  x86/hyperv: retrieve vp_index from Hyper-V
  x86: move viridian_page_msr to hyperv-tlfs.h
  x86/hyperv: setup VP assist page

 MAINTAINERS                              |   1 +
 xen/arch/x86/boot/x86_64.S               |  15 ++-
 xen/arch/x86/e820.c                      |   4 +-
 xen/arch/x86/guest/hyperv/hyperv.c       | 152 ++++++++++++++++++++++-
 xen/arch/x86/guest/hyperv/private.h      |  31 +++++
 xen/arch/x86/guest/hypervisor.c          |  12 +-
 xen/arch/x86/guest/xen/xen.c             |  31 +++--
 xen/arch/x86/hvm/viridian/viridian.c     |   2 +-
 xen/arch/x86/livepatch.c                 |   3 +-
 xen/arch/x86/mm.c                        |  17 ++-
 xen/arch/x86/smpboot.c                   |  12 +-
 xen/arch/x86/xen.lds.S                   |   7 ++
 xen/include/asm-x86/config.h             |   2 +-
 xen/include/asm-x86/fixmap.h             |  24 ++++
 xen/include/asm-x86/guest/hyperv-hcall.h |  97 +++++++++++++++
 xen/include/asm-x86/guest/hyperv-tlfs.h  |  16 ++-
 xen/include/asm-x86/guest/hyperv.h       |   3 +
 xen/include/asm-x86/guest/hypervisor.h   |  12 +-
 xen/include/asm-x86/hvm/viridian.h       |  15 +--
 19 files changed, 411 insertions(+), 45 deletions(-)
 create mode 100644 xen/arch/x86/guest/hyperv/private.h
 create mode 100644 xen/include/asm-x86/guest/hyperv-hcall.h

-- 
2.20.1


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

[Xen-devel] [PATCH v6 01/11] x86/hypervisor: make hypervisor_ap_setup return an error code

[Wei Liu]

We want to be able to handle AP setup error in the upper layer.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
v6:
1. Change map_vcpuinfo as well
2. Make code shorter
---
 xen/arch/x86/guest/hypervisor.c        |  6 ++++--
 xen/arch/x86/guest/xen/xen.c           | 24 +++++++++++++++---------
 xen/include/asm-x86/guest/hypervisor.h |  6 +++---
 3 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/guest/hypervisor.c b/xen/arch/x86/guest/hypervisor.c
index 4f27b98740..e72c92ffdf 100644
--- a/xen/arch/x86/guest/hypervisor.c
+++ b/xen/arch/x86/guest/hypervisor.c
@@ -52,10 +52,12 @@ void __init hypervisor_setup(void)
         ops->setup();
 }
 
-void hypervisor_ap_setup(void)
+int hypervisor_ap_setup(void)
 {
     if ( ops && ops->ap_setup )
-        ops->ap_setup();
+        return ops->ap_setup();
+
+    return 0;
 }
 
 void hypervisor_resume(void)
diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c
index 6dbc5f953f..d50f86bae7 100644
--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -113,16 +113,16 @@ static int map_vcpuinfo(void)
     info.mfn = virt_to_mfn(&vcpu_info[vcpu]);
     info.offset = (unsigned long)&vcpu_info[vcpu] & ~PAGE_MASK;
     rc = xen_hypercall_vcpu_op(VCPUOP_register_vcpu_info, vcpu, &info);
-    if ( rc )
-    {
-        BUG_ON(vcpu >= XEN_LEGACY_MAX_VCPUS);
-        this_cpu(vcpu_info) = &XEN_shared_info->vcpu_info[vcpu];
-    }
-    else
+    if ( !rc )
     {
         this_cpu(vcpu_info) = &vcpu_info[vcpu];
         set_bit(vcpu, vcpu_info_mapped);
     }
+    else if ( vcpu < XEN_LEGACY_MAX_VCPUS )
+    {
+        rc = 0;
+        this_cpu(vcpu_info) = &XEN_shared_info->vcpu_info[vcpu];
+    }
 
     return rc;
 }
@@ -257,11 +257,17 @@ static void __init setup(void)
     init_evtchn();
 }
 
-static void ap_setup(void)
+static int ap_setup(void)
 {
+    int rc;
+
     set_vcpu_id();
-    map_vcpuinfo();
-    init_evtchn();
+    rc = map_vcpuinfo();
+
+    if ( !rc )
+        init_evtchn();
+
+    return rc;
 }
 
 int xg_alloc_unused_page(mfn_t *mfn)
diff --git a/xen/include/asm-x86/guest/hypervisor.h b/xen/include/asm-x86/guest/hypervisor.h
index 392f4b90ae..b503854c5b 100644
--- a/xen/include/asm-x86/guest/hypervisor.h
+++ b/xen/include/asm-x86/guest/hypervisor.h
@@ -25,7 +25,7 @@ struct hypervisor_ops {
     /* Main setup routine */
     void (*setup)(void);
     /* AP setup */
-    void (*ap_setup)(void);
+    int (*ap_setup)(void);
     /* Resume from suspension */
     void (*resume)(void);
 };
@@ -34,7 +34,7 @@ struct hypervisor_ops {
 
 const char *hypervisor_probe(void);
 void hypervisor_setup(void);
-void hypervisor_ap_setup(void);
+int hypervisor_ap_setup(void);
 void hypervisor_resume(void);
 
 #else
@@ -44,7 +44,7 @@ void hypervisor_resume(void);
 
 static inline const char *hypervisor_probe(void) { return NULL; }
 static inline void hypervisor_setup(void) { ASSERT_UNREACHABLE(); }
-static inline void hypervisor_ap_setup(void) { ASSERT_UNREACHABLE(); }
+static inline int hypervisor_ap_setup(void) { ASSERT_UNREACHABLE(); return 0; }
 static inline void hypervisor_resume(void) { ASSERT_UNREACHABLE(); }
 
 #endif  /* CONFIG_GUEST */
-- 
2.20.1


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

[Xen-devel] [PATCH v6 02/11] x86/smp: don't online cpu if hypervisor_ap_setup fails

[Wei Liu]

Push hypervisor_ap_setup down to smp_callin.

Take the chance to replace xen_guest with cpu_has_hypervisor.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/smpboot.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index c9d1ab4423..93b86a09e9 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -199,6 +199,13 @@ static void smp_callin(void)
         goto halt;
     }
 
+    if ( cpu_has_hypervisor && (rc = hypervisor_ap_setup()) != 0 )
+    {
+        printk("CPU%d: Failed to initialise hypervisor functions. Not coming online.\n", cpu);
+        cpu_error = rc;
+        goto halt;
+    }
+
     if ( (rc = hvm_cpu_up()) != 0 )
     {
         printk("CPU%d: Failed to initialise HVM. Not coming online.\n", cpu);
@@ -371,9 +378,6 @@ void start_secondary(void *unused)
 
     tsx_init(); /* Needs microcode.  May change HLE/RTM feature bits. */
 
-    if ( xen_guest )
-        hypervisor_ap_setup();
-
     smp_callin();
 
     set_cpu_sibling_map(cpu);
-- 
2.20.1


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

[Xen-devel] [PATCH v6 03/11] x86: provide executable fixmap facility

[Wei Liu]

This allows us to set aside some address space for executable mapping.
This fixed map range starts from XEN_VIRT_END so that it is within reach
of the .text section.

Shift the percpu stub range and shrink livepatch range accordingly.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
v6:
1. Move symbol generation snippet to arch_init_memory and use %P0

v5:
1. drop __virt_to_fix_x
2. also check FIX*_RESERVED in __set_fixmap*
3. generate global symbol to be used in linker script
4. address other misc comments
---
 xen/arch/x86/boot/x86_64.S   | 15 ++++++++++++---
 xen/arch/x86/livepatch.c     |  3 ++-
 xen/arch/x86/mm.c            | 15 ++++++++++++++-
 xen/arch/x86/smpboot.c       |  2 +-
 xen/arch/x86/xen.lds.S       |  3 +++
 xen/include/asm-x86/config.h |  2 +-
 xen/include/asm-x86/fixmap.h | 24 ++++++++++++++++++++++++
 7 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 1cbf5acdfb..314a32a19f 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -81,11 +81,20 @@ GLOBAL(l2_directmap)
         .size l2_directmap, . - l2_directmap
 
 /*
- * L2 mapping the Xen text/data/bss region, constructed dynamically.  Uses 1x
- * 4k page.
+ * L2 mapping the Xen text/data/bss region, constructed dynamically.
+ * Executable fixmap is hooked up statically.
+ * Uses 1x 4k page.
  */
 GLOBAL(l2_xenmap)
-        .fill L2_PAGETABLE_ENTRIES, 8, 0
+        idx = 0
+        .rept L2_PAGETABLE_ENTRIES
+        .if idx == l2_table_offset(FIXADDR_X_TOP - 1)
+        .quad sym_offs(l1_fixmap_x) + __PAGE_HYPERVISOR
+        .else
+        .quad 0
+        .endif
+        idx = idx + 1
+        .endr
         .size l2_xenmap, . - l2_xenmap
 
 /* L2 mapping the fixmap.  Uses 1x 4k page. */
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 2749cbc5cf..513b0f3841 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -12,6 +12,7 @@
 #include <xen/livepatch.h>
 #include <xen/sched.h>
 
+#include <asm/fixmap.h>
 #include <asm/nmi.h>
 #include <asm/livepatch.h>
 
@@ -311,7 +312,7 @@ void __init arch_livepatch_init(void)
     void *start, *end;
 
     start = (void *)xen_virt_end;
-    end = (void *)(XEN_VIRT_END - NR_CPUS * PAGE_SIZE);
+    end = (void *)(XEN_VIRT_END - FIXADDR_X_SIZE - NR_CPUS * PAGE_SIZE);
 
     BUG_ON(end <= start);
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f50c065af3..6b1361845c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -157,6 +157,8 @@
 /* Mapping of the fixmap space needed early. */
 l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
     l1_fixmap[L1_PAGETABLE_ENTRIES];
+l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
+    l1_fixmap_x[L1_PAGETABLE_ENTRIES];
 
 paddr_t __read_mostly mem_hotplug;
 
@@ -372,6 +374,10 @@ void __init arch_init_memory(void)
         }
     }
 #endif
+
+    /* Generate a symbol to be used in linker script */
+    asm ( ".equ FIXADDR_X_SIZE, %P0; .global FIXADDR_X_SIZE"
+          :: "i" (FIXADDR_X_SIZE) );
 }
 
 int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
@@ -5718,10 +5724,17 @@ int destroy_xen_mappings(unsigned long s, unsigned long e)
 void __set_fixmap(
     enum fixed_addresses idx, unsigned long mfn, unsigned long flags)
 {
-    BUG_ON(idx >= __end_of_fixed_addresses);
+    BUG_ON(idx >= __end_of_fixed_addresses || idx <= FIX_RESERVED);
     map_pages_to_xen(__fix_to_virt(idx), _mfn(mfn), 1, flags);
 }
 
+void __set_fixmap_x(
+    enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags)
+{
+    BUG_ON(idx >= __end_of_fixed_addresses_x || idx <= FIX_X_RESERVED);
+    map_pages_to_xen(__fix_x_to_virt(idx), _mfn(mfn), 1, flags);
+}
+
 void *__init arch_vmap_virt_end(void)
 {
     return fix_to_virt(__end_of_fixed_addresses);
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 93b86a09e9..e83e4564a4 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -644,7 +644,7 @@ unsigned long alloc_stub_page(unsigned int cpu, unsigned long *mfn)
         unmap_domain_page(memset(__map_domain_page(pg), 0xcc, PAGE_SIZE));
     }
 
-    stub_va = XEN_VIRT_END - (cpu + 1) * PAGE_SIZE;
+    stub_va = XEN_VIRT_END - FIXADDR_X_SIZE - (cpu + 1) * PAGE_SIZE;
     if ( map_pages_to_xen(stub_va, page_to_mfn(pg), 1,
                           PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES) )
     {
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index f5730ffe93..de0856b88e 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -2,6 +2,8 @@
 /* Modified for i386/x86-64 Xen by Keir Fraser */
 
 #include <xen/cache.h>
+
+#include <asm/fixmap.h>
 #include <asm/page.h>
 #undef ENTRY
 #undef ALIGN
@@ -352,6 +354,7 @@ SECTIONS
 }
 
 ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START -
+                          FIXADDR_X_SIZE -
                           NR_CPUS * PAGE_SIZE,
        "Xen image overlaps stubs area")
 
diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
index d0cfbb70a8..a34053c4c0 100644
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -218,7 +218,7 @@ extern unsigned char boot_edid_info[128];
 /* Slot 261: high read-only compat machine-to-phys conversion table (1GB). */
 #define HIRO_COMPAT_MPT_VIRT_START RDWR_COMPAT_MPT_VIRT_END
 #define HIRO_COMPAT_MPT_VIRT_END (HIRO_COMPAT_MPT_VIRT_START + GB(1))
-/* Slot 261: xen text, static data and bss (1GB). */
+/* Slot 261: xen text, static data, bss, per-cpu stubs and executable fixmap (1GB). */
 #define XEN_VIRT_START          (HIRO_COMPAT_MPT_VIRT_END)
 #define XEN_VIRT_END            (XEN_VIRT_START + GB(1))
 
diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h
index 9fb2f47946..8330097a74 100644
--- a/xen/include/asm-x86/fixmap.h
+++ b/xen/include/asm-x86/fixmap.h
@@ -15,6 +15,7 @@
 #include <asm/page.h>
 
 #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
+#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
 
 #ifndef __ASSEMBLY__
 
@@ -89,6 +90,29 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr)
     return __virt_to_fix(vaddr);
 }
 
+enum fixed_addresses_x {
+    /* Index 0 is reserved since fix_x_to_virt(0) == FIXADDR_X_TOP. */
+    FIX_X_RESERVED,
+#ifdef CONFIG_HYPERV_GUEST
+    FIX_X_HYPERV_HCALL,
+#endif
+    __end_of_fixed_addresses_x
+};
+
+#define FIXADDR_X_SIZE  (__end_of_fixed_addresses_x << PAGE_SHIFT)
+#define FIXADDR_X_START (FIXADDR_X_TOP - FIXADDR_X_SIZE)
+
+extern void __set_fixmap_x(
+    enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags);
+
+#define set_fixmap_x(idx, phys) \
+    __set_fixmap_x(idx, (phys)>>PAGE_SHIFT, PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES)
+
+#define clear_fixmap_x(idx) __set_fixmap_x(idx, 0, 0)
+
+#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT))
+#define fix_x_to_virt(x)   ((void *)__fix_x_to_virt(x))
+
 #endif /* __ASSEMBLY__ */
 
 #endif
-- 
2.20.1


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

[Xen-devel] [PATCH v6 04/11] x86/hypervisor: provide hypervisor_fixup_e820

[Wei Liu]

And implement the hook for Xen guest.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
 xen/arch/x86/e820.c                    | 4 ++--
 xen/arch/x86/guest/hypervisor.c        | 6 ++++++
 xen/arch/x86/guest/xen/xen.c           | 7 +++++++
 xen/include/asm-x86/guest/hypervisor.h | 6 ++++++
 4 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 3892c9cfb7..2219c63861 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -690,8 +690,8 @@ unsigned long __init init_e820(const char *str, struct e820map *raw)
 
     machine_specific_memory_setup(raw);
 
-    if ( pv_shim )
-        pv_shim_fixup_e820(&e820);
+    if ( cpu_has_hypervisor )
+        hypervisor_e820_fixup(&e820);
 
     printk("%s RAM map:\n", str);
     print_e820_memory_map(e820.map, e820.nr_map);
diff --git a/xen/arch/x86/guest/hypervisor.c b/xen/arch/x86/guest/hypervisor.c
index e72c92ffdf..5fd433c8d4 100644
--- a/xen/arch/x86/guest/hypervisor.c
+++ b/xen/arch/x86/guest/hypervisor.c
@@ -66,6 +66,12 @@ void hypervisor_resume(void)
         ops->resume();
 }
 
+void __init hypervisor_e820_fixup(struct e820map *e820)
+{
+    if ( ops && ops->e820_fixup )
+        ops->e820_fixup(e820);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c
index d50f86bae7..45e54dfbba 100644
--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -316,11 +316,18 @@ static void resume(void)
         pv_console_init();
 }
 
+static void __init e820_fixup(struct e820map *e820)
+{
+    if ( pv_shim )
+        pv_shim_fixup_e820(e820);
+}
+
 static const struct hypervisor_ops ops = {
     .name = "Xen",
     .setup = setup,
     .ap_setup = ap_setup,
     .resume = resume,
+    .e820_fixup = e820_fixup,
 };
 
 const struct hypervisor_ops *__init xg_probe(void)
diff --git a/xen/include/asm-x86/guest/hypervisor.h b/xen/include/asm-x86/guest/hypervisor.h
index b503854c5b..b66cb28333 100644
--- a/xen/include/asm-x86/guest/hypervisor.h
+++ b/xen/include/asm-x86/guest/hypervisor.h
@@ -19,6 +19,8 @@
 #ifndef __X86_HYPERVISOR_H__
 #define __X86_HYPERVISOR_H__
 
+#include <asm/e820.h>
+
 struct hypervisor_ops {
     /* Name of the hypervisor */
     const char *name;
@@ -28,6 +30,8 @@ struct hypervisor_ops {
     int (*ap_setup)(void);
     /* Resume from suspension */
     void (*resume)(void);
+    /* Fix up e820 map */
+    void (*e820_fixup)(struct e820map *e820);
 };
 
 #ifdef CONFIG_GUEST
@@ -36,6 +40,7 @@ const char *hypervisor_probe(void);
 void hypervisor_setup(void);
 int hypervisor_ap_setup(void);
 void hypervisor_resume(void);
+void hypervisor_e820_fixup(struct e820map *e820);
 
 #else
 
@@ -46,6 +51,7 @@ static inline const char *hypervisor_probe(void) { return NULL; }
 static inline void hypervisor_setup(void) { ASSERT_UNREACHABLE(); }
 static inline int hypervisor_ap_setup(void) { ASSERT_UNREACHABLE(); return 0; }
 static inline void hypervisor_resume(void) { ASSERT_UNREACHABLE(); }
+static inline void hypervisor_e820_fixup(struct e820map *e820) { ASSERT_UNREACHABLE(); }
 
 #endif  /* CONFIG_GUEST */
 
-- 
2.20.1


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

[Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Wei Liu]

Hyper-V uses a technique called overlay page for its hypercall page. It
will insert a backing page to the guest when the hypercall functionality
is enabled. That means we can use a page that is not backed by real
memory for hypercall page.

Use the top-most addressable page for that purpose. Adjust e820 map
accordingly.

We also need to register Xen's guest OS ID to Hyper-V. Use 0x3 as the
vendor ID. Fix the comment in hyperv-tlfs.h while at it.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
v6:
1. Use hv_guest_os_id
2. Use new e820_fixup hook
3. Add a BUILD_BUG_ON

v5:
1. use hypervisor_reserve_top_pages
2. add a macro for hypercall page mfn
3. address other misc comments

v4:
1. Use fixmap
2. Follow routines listed in TLFS
---
 xen/arch/x86/guest/hyperv/hyperv.c      | 69 +++++++++++++++++++++++--
 xen/include/asm-x86/guest/hyperv-tlfs.h |  5 +-
 xen/include/asm-x86/guest/hyperv.h      |  3 ++
 3 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 8d38313d7a..7c2a96d70e 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -19,15 +19,27 @@
  * Copyright (c) 2019 Microsoft.
  */
 #include <xen/init.h>
+#include <xen/version.h>
 
+#include <asm/fixmap.h>
 #include <asm/guest.h>
 #include <asm/guest/hyperv-tlfs.h>
+#include <asm/processor.h>
 
 struct ms_hyperv_info __read_mostly ms_hyperv;
 
-static const struct hypervisor_ops ops = {
-    .name = "Hyper-V",
-};
+static uint64_t generate_guest_id(void)
+{
+    union hv_guest_os_id id;
+
+    id.vendor = HV_XEN_VENDOR_ID;
+    id.major = xen_major_version();
+    id.minor = xen_minor_version();
+
+    return id.raw;
+}
+
+static const struct hypervisor_ops ops;
 
 const struct hypervisor_ops *__init hyperv_probe(void)
 {
@@ -72,6 +84,57 @@ const struct hypervisor_ops *__init hyperv_probe(void)
     return &ops;
 }
 
+static void __init setup_hypercall_page(void)
+{
+    union hv_x64_msr_hypercall_contents hypercall_msr;
+    union hv_guest_os_id guest_id;
+    unsigned long mfn;
+
+    BUILD_BUG_ON(HV_HYP_PAGE_SHIFT != PAGE_SHIFT);
+
+    rdmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
+    if ( !guest_id.raw )
+    {
+        guest_id.raw = generate_guest_id();
+        wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
+    }
+
+    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+    if ( !hypercall_msr.enable )
+    {
+        mfn = HV_HCALL_MFN;
+        hypercall_msr.enable = 1;
+        hypercall_msr.guest_physical_address = mfn;
+        wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+    } else {
+        mfn = hypercall_msr.guest_physical_address;
+    }
+
+    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+    BUG_ON(!hypercall_msr.enable);
+
+    set_fixmap_x(FIX_X_HYPERV_HCALL, mfn << PAGE_SHIFT);
+}
+
+static void __init setup(void)
+{
+    setup_hypercall_page();
+}
+
+static void __init e820_fixup(struct e820map *e820)
+{
+    uint64_t s = HV_HCALL_MFN << PAGE_SHIFT;
+
+    if ( !e820_add_range(e820, s, s + PAGE_SIZE, E820_RESERVED) )
+        panic("Unable to reserve Hyper-V hypercall range\n");
+}
+
+static const struct hypervisor_ops ops = {
+    .name = "Hyper-V",
+    .setup = setup,
+    .e820_fixup = e820_fixup,
+};
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/guest/hyperv-tlfs.h b/xen/include/asm-x86/guest/hyperv-tlfs.h
index 05c4044976..07db57b55f 100644
--- a/xen/include/asm-x86/guest/hyperv-tlfs.h
+++ b/xen/include/asm-x86/guest/hyperv-tlfs.h
@@ -318,15 +318,16 @@ struct ms_hyperv_tsc_page {
  *
  * Bit(s)
  * 63 - Indicates if the OS is Open Source or not; 1 is Open Source
- * 62:56 - Os Type; Linux is 0x100
+ * 62:56 - Os Type; Linux 0x1, FreeBSD 0x2, Xen 0x3
  * 55:48 - Distro specific identification
- * 47:16 - Linux kernel version number
+ * 47:16 - Guest OS version number
  * 15:0  - Distro specific identification
  *
  *
  */
 
 #define HV_LINUX_VENDOR_ID              0x8100
+#define HV_XEN_VENDOR_ID                0x8300
 union hv_guest_os_id
 {
     uint64_t raw;
diff --git a/xen/include/asm-x86/guest/hyperv.h b/xen/include/asm-x86/guest/hyperv.h
index c7a7f32bd5..1a1b47831c 100644
--- a/xen/include/asm-x86/guest/hyperv.h
+++ b/xen/include/asm-x86/guest/hyperv.h
@@ -21,6 +21,9 @@
 
 #include <xen/types.h>
 
+/* Use top-most MFN for hypercall page */
+#define HV_HCALL_MFN   (((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT)
+
 /*
  * The specification says: "The partition reference time is computed
  * by the following formula:
-- 
2.20.1


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

[Xen-devel] [PATCH v6 06/11] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

These functions will be used later to make hypercalls to Hyper-V.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
Reviewed-by: Paul Durrant <pdurrant@amazon.com>
---
v6:
1. Use asm(...) to generate symbol
2. Add a comment regarding volatile registers

v5:
1. Switch back to direct call
2. Fix some issues pointed out by Jan
---
 MAINTAINERS                              |  1 +
 xen/arch/x86/mm.c                        |  4 +-
 xen/arch/x86/xen.lds.S                   |  4 +
 xen/include/asm-x86/guest/hyperv-hcall.h | 97 ++++++++++++++++++++++++
 4 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/asm-x86/guest/hyperv-hcall.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 04d91482cd..d0a5ed635b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -519,6 +519,7 @@ S:	Supported
 F:	xen/arch/x86/guest/hyperv/
 F:	xen/arch/x86/hvm/viridian/
 F:	xen/include/asm-x86/guest/hyperv.h
+F:	xen/include/asm-x86/guest/hyperv-hcall.h
 F:	xen/include/asm-x86/guest/hyperv-tlfs.h
 F:	xen/include/asm-x86/hvm/viridian.h
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 6b1361845c..e7787c8fde 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -375,9 +375,11 @@ void __init arch_init_memory(void)
     }
 #endif
 
-    /* Generate a symbol to be used in linker script */
+    /* Generate symbols to be used in linker script */
     asm ( ".equ FIXADDR_X_SIZE, %P0; .global FIXADDR_X_SIZE"
           :: "i" (FIXADDR_X_SIZE) );
+    asm ( ".equ HV_HCALL_PAGE, %P0; .global HV_HCALL_PAGE"
+          :: "i" (__fix_x_to_virt(FIX_X_HYPERV_HCALL)) );
 }
 
 int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index de0856b88e..ec8ab2bec6 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -328,6 +328,10 @@ SECTIONS
   efi = .;
 #endif
 
+#ifdef CONFIG_HYPERV_GUEST
+  hv_hcall_page = ABSOLUTE(HV_HCALL_PAGE);
+#endif
+
   /* Sections to be discarded */
   /DISCARD/ : {
        *(.exit.text)
diff --git a/xen/include/asm-x86/guest/hyperv-hcall.h b/xen/include/asm-x86/guest/hyperv-hcall.h
new file mode 100644
index 0000000000..4d3b131b3a
--- /dev/null
+++ b/xen/include/asm-x86/guest/hyperv-hcall.h
@@ -0,0 +1,97 @@
+/******************************************************************************
+ * asm-x86/guest/hyperv-hcall.h
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2019 Microsoft.
+ */
+
+#ifndef __X86_HYPERV_HCALL_H__
+#define __X86_HYPERV_HCALL_H__
+
+#include <xen/lib.h>
+#include <xen/types.h>
+
+#include <asm/asm_defns.h>
+#include <asm/fixmap.h>
+#include <asm/guest/hyperv-tlfs.h>
+#include <asm/page.h>
+
+static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input_addr,
+                                       paddr_t output_addr)
+{
+    uint64_t status;
+    register unsigned long r8 asm ( "r8" ) = output_addr;
+
+    /* See TLFS for volatile registers */
+    asm volatile ( "call hv_hcall_page"
+                   : "=a" (status), "+c" (control),
+                     "+d" (input_addr) ASM_CALL_CONSTRAINT
+                   : "r" (r8)
+                   : "memory" );
+
+    return status;
+}
+
+static inline uint64_t hv_do_fast_hypercall(uint16_t code,
+                                            uint64_t input1, uint64_t input2)
+{
+    uint64_t status;
+    uint64_t control = code | HV_HYPERCALL_FAST_BIT;
+    register unsigned long r8 asm ( "r8" ) = input2;
+
+    /* See TLFS for volatile registers */
+    asm volatile ( "call hv_hcall_page"
+                   : "=a" (status), "+c" (control),
+                     "+d" (input1) ASM_CALL_CONSTRAINT
+                   : "r" (r8) );
+
+    return status;
+}
+
+static inline uint64_t hv_do_rep_hypercall(uint16_t code, uint16_t rep_count,
+                                           uint16_t varhead_size,
+                                           paddr_t input, paddr_t output)
+{
+    uint64_t control = code;
+    uint64_t status;
+    uint16_t rep_comp;
+
+    control |= (uint64_t)varhead_size << HV_HYPERCALL_VARHEAD_OFFSET;
+    control |= (uint64_t)rep_count << HV_HYPERCALL_REP_COMP_OFFSET;
+
+    do {
+        status = hv_do_hypercall(control, input, output);
+        if ( (status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS )
+            break;
+
+        rep_comp = MASK_EXTR(status, HV_HYPERCALL_REP_COMP_MASK);
+
+        control &= ~HV_HYPERCALL_REP_START_MASK;
+        control |= MASK_INSR(rep_comp, HV_HYPERCALL_REP_START_MASK);
+    } while ( rep_comp < rep_count );
+
+    return status;
+}
+
+#endif /* __X86_HYPERV_HCALL_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.20.1


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

[Xen-devel] [PATCH v6 07/11] DO NOT APPLY: x86/hyperv: issue an hypercall

[Wei Liu]

Test if the infrastructure works.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
 xen/arch/x86/guest/hyperv/hyperv.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 7c2a96d70e..052b053160 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -23,6 +23,7 @@
 
 #include <asm/fixmap.h>
 #include <asm/guest.h>
+#include <asm/guest/hyperv-hcall.h>
 #include <asm/guest/hyperv-tlfs.h>
 #include <asm/processor.h>
 
@@ -114,6 +115,19 @@ static void __init setup_hypercall_page(void)
     BUG_ON(!hypercall_msr.enable);
 
     set_fixmap_x(FIX_X_HYPERV_HCALL, mfn << PAGE_SHIFT);
+
+    /* XXX Wei: Issue an hypercall here to make sure things are set up
+     * correctly.  When there is actual use of the hypercall facility,
+     * this can be removed.
+     */
+    {
+        uint16_t r = hv_do_hypercall(0xffff, 0, 0);
+        BUG_ON(r != HV_STATUS_INVALID_HYPERCALL_CODE);
+        r = hv_do_fast_hypercall(0xffff, 0, 0);
+        BUG_ON(r != HV_STATUS_INVALID_HYPERCALL_CODE);
+
+        printk("Successfully issued Hyper-V hypercalls\n");
+    }
 }
 
 static void __init setup(void)
-- 
2.20.1


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

[Xen-devel] [PATCH v6 08/11] x86/hyperv: provide percpu hypercall input page

[Wei Liu]

Hyper-V's input / output argument must be 8 bytes aligned an not cross
page boundary. One way to satisfy those requirements is to use percpu
page.

For the foreseeable future we only need to provide input for TLB
and APIC hypercalls, so skip setting up an output page.

We will also need to provide an ap_setup hook for secondary cpus to
setup its own input page.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
Reviewed-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
v6:
1. Make code shorter
2. Change variable name

v5:
1. Adjust to new ap_setup
2. Change variable name to hv_pcpu_input_page

v4:
1. Change wording in commit message
2. Prevent leak
3. Introduce a private header

v3:
1. Use xenheap page instead
2. Drop page tracking structure
3. Drop Paul's review tag

address comments
---
 xen/arch/x86/guest/hyperv/hyperv.c  | 28 ++++++++++++++++++++++++++++
 xen/arch/x86/guest/hyperv/private.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)
 create mode 100644 xen/arch/x86/guest/hyperv/private.h

diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 052b053160..8d44b35aa6 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -27,7 +27,10 @@
 #include <asm/guest/hyperv-tlfs.h>
 #include <asm/processor.h>
 
+#include "private.h"
+
 struct ms_hyperv_info __read_mostly ms_hyperv;
+DEFINE_PER_CPU_READ_MOSTLY(void *, hv_input_page);
 
 static uint64_t generate_guest_id(void)
 {
@@ -130,9 +133,33 @@ static void __init setup_hypercall_page(void)
     }
 }
 
+static int setup_hypercall_pcpu_arg(void)
+{
+    if ( this_cpu(hv_input_page) )
+        return 0;
+
+    this_cpu(hv_input_page) = alloc_xenheap_page();
+    if ( !this_cpu(hv_input_page) )
+    {
+        printk("CPU%u: Failed to allocate hypercall input page\n",
+               smp_processor_id());
+        return -ENOMEM;
+    }
+
+    return 0;
+}
+
 static void __init setup(void)
 {
     setup_hypercall_page();
+
+    if ( setup_hypercall_pcpu_arg() )
+        panic("Hyper-V hypercall percpu arg setup failed\n");
+}
+
+static int ap_setup(void)
+{
+    return setup_hypercall_pcpu_arg();
 }
 
 static void __init e820_fixup(struct e820map *e820)
@@ -146,6 +173,7 @@ static void __init e820_fixup(struct e820map *e820)
 static const struct hypervisor_ops ops = {
     .name = "Hyper-V",
     .setup = setup,
+    .ap_setup = ap_setup,
     .e820_fixup = e820_fixup,
 };
 
diff --git a/xen/arch/x86/guest/hyperv/private.h b/xen/arch/x86/guest/hyperv/private.h
new file mode 100644
index 0000000000..093985a94b
--- /dev/null
+++ b/xen/arch/x86/guest/hyperv/private.h
@@ -0,0 +1,29 @@
+/******************************************************************************
+ * arch/x86/guest/hyperv/private.h
+ *
+ * Definitions / declarations only useful to Hyper-V code.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2020 Microsoft.
+ */
+
+#ifndef __XEN_HYPERV_PRIVIATE_H__
+#define __XEN_HYPERV_PRIVIATE_H__
+
+#include <xen/percpu.h>
+
+DECLARE_PER_CPU(void *, hv_input_page);
+
+#endif /* __XEN_HYPERV_PRIVIATE_H__  */
-- 
2.20.1


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

[Xen-devel] [PATCH v6 09/11] x86/hyperv: retrieve vp_index from Hyper-V

[Wei Liu]

This will be useful when invoking hypercall that targets specific
vcpu(s).

Signed-off-by: Wei Liu <liuwe@microsoft.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v5:
1. Add Jan's Ack.

v4:
1. Use private.h
2. Add Paul's review tag

v2:
1. Fold into setup_pcpu_arg function
---
 xen/arch/x86/guest/hyperv/hyperv.c  | 6 ++++++
 xen/arch/x86/guest/hyperv/private.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 8d44b35aa6..6dac3bfceb 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -31,6 +31,7 @@
 
 struct ms_hyperv_info __read_mostly ms_hyperv;
 DEFINE_PER_CPU_READ_MOSTLY(void *, hv_input_page);
+DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index);
 
 static uint64_t generate_guest_id(void)
 {
@@ -135,6 +136,8 @@ static void __init setup_hypercall_page(void)
 
 static int setup_hypercall_pcpu_arg(void)
 {
+    uint64_t vp_index_msr;
+
     if ( this_cpu(hv_input_page) )
         return 0;
 
@@ -146,6 +149,9 @@ static int setup_hypercall_pcpu_arg(void)
         return -ENOMEM;
     }
 
+    rdmsrl(HV_X64_MSR_VP_INDEX, vp_index_msr);
+    this_cpu(hv_vp_index) = vp_index_msr;
+
     return 0;
 }
 
diff --git a/xen/arch/x86/guest/hyperv/private.h b/xen/arch/x86/guest/hyperv/private.h
index 093985a94b..d1765d4f23 100644
--- a/xen/arch/x86/guest/hyperv/private.h
+++ b/xen/arch/x86/guest/hyperv/private.h
@@ -25,5 +25,6 @@
 #include <xen/percpu.h>
 
 DECLARE_PER_CPU(void *, hv_input_page);
+DECLARE_PER_CPU(unsigned int, hv_vp_index);
 
 #endif /* __XEN_HYPERV_PRIVIATE_H__  */
-- 
2.20.1


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

[Xen-devel] [PATCH v6 10/11] x86: move viridian_page_msr to hyperv-tlfs.h

[Wei Liu]

And rename it to hv_vp_assist_page_msr.

No functional change.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
 xen/arch/x86/hvm/viridian/viridian.c    |  2 +-
 xen/include/asm-x86/guest/hyperv-tlfs.h | 11 +++++++++++
 xen/include/asm-x86/hvm/viridian.h      | 15 ++-------------
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 44c8e6cac6..9a6cafcb62 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -230,7 +230,7 @@ static void dump_guest_os_id(const struct domain *d)
 
 static void dump_hypercall(const struct domain *d)
 {
-    const union viridian_page_msr *hg;
+    const union hv_vp_assist_page_msr *hg;
 
     hg = &d->arch.hvm.viridian->hypercall_gpa;
 
diff --git a/xen/include/asm-x86/guest/hyperv-tlfs.h b/xen/include/asm-x86/guest/hyperv-tlfs.h
index 07db57b55f..0a0f3398c1 100644
--- a/xen/include/asm-x86/guest/hyperv-tlfs.h
+++ b/xen/include/asm-x86/guest/hyperv-tlfs.h
@@ -558,6 +558,17 @@ struct hv_nested_enlightenments_control {
 	} hypercallControls;
 };
 
+union hv_vp_assist_page_msr
+{
+    uint64_t raw;
+    struct
+    {
+        uint64_t enabled:1;
+        uint64_t reserved_preserved:11;
+        uint64_t pfn:48;
+    };
+};
+
 /* Define virtual processor assist page structure. */
 struct hv_vp_assist_page {
 	__u32 apic_assist;
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index d9138562e6..844e56b38f 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -11,20 +11,9 @@
 
 #include <asm/guest/hyperv-tlfs.h>
 
-union viridian_page_msr
-{
-    uint64_t raw;
-    struct
-    {
-        uint64_t enabled:1;
-        uint64_t reserved_preserved:11;
-        uint64_t pfn:48;
-    };
-};
-
 struct viridian_page
 {
-    union viridian_page_msr msr;
+    union hv_vp_assist_page_msr msr;
     void *ptr;
 };
 
@@ -70,7 +59,7 @@ struct viridian_time_ref_count
 struct viridian_domain
 {
     union hv_guest_os_id guest_os_id;
-    union viridian_page_msr hypercall_gpa;
+    union hv_vp_assist_page_msr hypercall_gpa;
     struct viridian_time_ref_count time_ref_count;
     struct viridian_page reference_tsc;
 };
-- 
2.20.1


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

[Xen-devel] [PATCH v6 11/11] x86/hyperv: setup VP assist page

[Wei Liu]

VP assist page is rather important as we need to toggle some bits in it
for efficient nested virtualisation.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
v6:
1. Use hv_vp_assist_page_msr
2. Make code shorter
3. Preserve rsvdP fields

v5:
1. Deal with error properly instead of always panicking
2. Swap percpu variables declarations' location

v4:
1. Use private.h
2. Prevent leak

v3:
1. Use xenheap page
2. Drop set_vp_assist

v2:
1. Use HV_HYP_PAGE_SHIFT instead
---
 xen/arch/x86/guest/hyperv/hyperv.c  | 37 ++++++++++++++++++++++++++++-
 xen/arch/x86/guest/hyperv/private.h |  1 +
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 6dac3bfceb..75fb329d4d 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -31,6 +31,7 @@
 
 struct ms_hyperv_info __read_mostly ms_hyperv;
 DEFINE_PER_CPU_READ_MOSTLY(void *, hv_input_page);
+DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist);
 DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index);
 
 static uint64_t generate_guest_id(void)
@@ -155,17 +156,51 @@ static int setup_hypercall_pcpu_arg(void)
     return 0;
 }
 
+static int setup_vp_assist(void)
+{
+    union hv_vp_assist_page_msr msr;
+
+    if ( !this_cpu(hv_vp_assist) )
+    {
+        this_cpu(hv_vp_assist) = alloc_xenheap_page();
+        if ( !this_cpu(hv_vp_assist) )
+        {
+            printk("CPU%u: Failed to allocate vp_assist page\n",
+                   smp_processor_id());
+            return -ENOMEM;
+        }
+
+        clear_page(this_cpu(hv_vp_assist));
+    }
+
+    rdmsrl(HV_X64_MSR_VP_ASSIST_PAGE, msr.raw);
+    msr.pfn = virt_to_mfn(this_cpu(hv_vp_assist));
+    msr.enabled = 1;
+    wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, msr.raw);
+
+    return 0;
+}
+
 static void __init setup(void)
 {
     setup_hypercall_page();
 
     if ( setup_hypercall_pcpu_arg() )
         panic("Hyper-V hypercall percpu arg setup failed\n");
+
+    if ( setup_vp_assist() )
+        panic("VP assist page setup failed\n");
 }
 
 static int ap_setup(void)
 {
-    return setup_hypercall_pcpu_arg();
+    int rc;
+
+    rc = setup_hypercall_pcpu_arg();
+    if ( rc )
+        return rc;
+
+    return setup_vp_assist();
 }
 
 static void __init e820_fixup(struct e820map *e820)
diff --git a/xen/arch/x86/guest/hyperv/private.h b/xen/arch/x86/guest/hyperv/private.h
index d1765d4f23..956eff831f 100644
--- a/xen/arch/x86/guest/hyperv/private.h
+++ b/xen/arch/x86/guest/hyperv/private.h
@@ -25,6 +25,7 @@
 #include <xen/percpu.h>
 
 DECLARE_PER_CPU(void *, hv_input_page);
+DECLARE_PER_CPU(void *, hv_vp_assist);
 DECLARE_PER_CPU(unsigned int, hv_vp_index);
 
 #endif /* __XEN_HYPERV_PRIVIATE_H__  */
-- 
2.20.1


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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Wei Liu]

(Note to self)

On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
[...]
> +static uint64_t generate_guest_id(void)
> +{
> +    union hv_guest_os_id id;
> +

       id.raw = 0;

> +    id.vendor = HV_XEN_VENDOR_ID;
> +    id.major = xen_major_version();
> +    id.minor = xen_minor_version();
> +
> +    return id.raw;
> +}

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Durrant, Paul]

> -----Original Message-----
> From: Wei Liu <wl@xen.org>
> Sent: 31 January 2020 17:57
> To: Xen Development List <xen-devel@lists.xenproject.org>
> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley
> <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu
> <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> 
> (Note to self)
> 
> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
> [...]
> > +static uint64_t generate_guest_id(void)
> > +{
> > +    union hv_guest_os_id id;
> > +
> 
>        id.raw = 0;

Or just use a C99 initializer to set things up. A bit neater IMO.

  Paul

> 
> > +    id.vendor = HV_XEN_VENDOR_ID;
> > +    id.major = xen_major_version();
> > +    id.minor = xen_minor_version();
> > +
> > +    return id.raw;
> > +}

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Jan Beulich]

On 03.02.2020 11:39, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Wei Liu <wl@xen.org>
>> Sent: 31 January 2020 17:57
>> To: Xen Development List <xen-devel@lists.xenproject.org>
>> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley
>> <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu
>> <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
>> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
>>
>> (Note to self)
>>
>> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
>> [...]
>>> +static uint64_t generate_guest_id(void)
>>> +{
>>> +    union hv_guest_os_id id;
>>> +
>>
>>        id.raw = 0;
> 
> Or just use a C99 initializer to set things up. A bit neater IMO.

If you mean this for ...

>>> +    id.vendor = HV_XEN_VENDOR_ID;
>>> +    id.major = xen_major_version();
>>> +    id.minor = xen_minor_version();

... these three fields, then this won't work with rather old gcc
we still permit to be used. Using { .raw = 0 } would work afaict.

Jan

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 03 February 2020 11:06
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
> devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>; Wei
> Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> 
> On 03.02.2020 11:39, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Wei Liu <wl@xen.org>
> >> Sent: 31 January 2020 17:57
> >> To: Xen Development List <xen-devel@lists.xenproject.org>
> >> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley
> >> <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu
> >> <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
> >> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> >>
> >> (Note to self)
> >>
> >> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
> >> [...]
> >>> +static uint64_t generate_guest_id(void)
> >>> +{
> >>> +    union hv_guest_os_id id;
> >>> +
> >>
> >>        id.raw = 0;
> >
> > Or just use a C99 initializer to set things up. A bit neater IMO.
> 
> If you mean this for ...
> 
> >>> +    id.vendor = HV_XEN_VENDOR_ID;
> >>> +    id.major = xen_major_version();
> >>> +    id.minor = xen_minor_version();
> 
> ... these three fields, then this won't work with rather old gcc
> we still permit to be used. Using { .raw = 0 } would work afaict.
> 

Not even { .vendor = HV_XEN_VENDOR_ID } ?

  Paul

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

Re: [Xen-devel] [PATCH v6 10/11] x86: move viridian_page_msr to hyperv-tlfs.h

[Durrant, Paul]

> -----Original Message-----
> From: Wei Liu <wei.liu.xen@gmail.com> On Behalf Of Wei Liu
> Sent: 31 January 2020 17:49
> To: Xen Development List <xen-devel@lists.xenproject.org>
> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley
> <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu
> <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: [PATCH v6 10/11] x86: move viridian_page_msr to hyperv-tlfs.h
> 
> And rename it to hv_vp_assist_page_msr.
> 
> No functional change.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>

Reviewed-by: Paul Durrant <pdurrant@amazon.com>

> ---
>  xen/arch/x86/hvm/viridian/viridian.c    |  2 +-
>  xen/include/asm-x86/guest/hyperv-tlfs.h | 11 +++++++++++
>  xen/include/asm-x86/hvm/viridian.h      | 15 ++-------------
>  3 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c
> b/xen/arch/x86/hvm/viridian/viridian.c
> index 44c8e6cac6..9a6cafcb62 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -230,7 +230,7 @@ static void dump_guest_os_id(const struct domain *d)
> 
>  static void dump_hypercall(const struct domain *d)
>  {
> -    const union viridian_page_msr *hg;
> +    const union hv_vp_assist_page_msr *hg;
> 
>      hg = &d->arch.hvm.viridian->hypercall_gpa;
> 
> diff --git a/xen/include/asm-x86/guest/hyperv-tlfs.h b/xen/include/asm-
> x86/guest/hyperv-tlfs.h
> index 07db57b55f..0a0f3398c1 100644
> --- a/xen/include/asm-x86/guest/hyperv-tlfs.h
> +++ b/xen/include/asm-x86/guest/hyperv-tlfs.h
> @@ -558,6 +558,17 @@ struct hv_nested_enlightenments_control {
>  	} hypercallControls;
>  };
> 
> +union hv_vp_assist_page_msr
> +{
> +    uint64_t raw;
> +    struct
> +    {
> +        uint64_t enabled:1;
> +        uint64_t reserved_preserved:11;
> +        uint64_t pfn:48;
> +    };
> +};
> +
>  /* Define virtual processor assist page structure. */
>  struct hv_vp_assist_page {
>  	__u32 apic_assist;
> diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-
> x86/hvm/viridian.h
> index d9138562e6..844e56b38f 100644
> --- a/xen/include/asm-x86/hvm/viridian.h
> +++ b/xen/include/asm-x86/hvm/viridian.h
> @@ -11,20 +11,9 @@
> 
>  #include <asm/guest/hyperv-tlfs.h>
> 
> -union viridian_page_msr
> -{
> -    uint64_t raw;
> -    struct
> -    {
> -        uint64_t enabled:1;
> -        uint64_t reserved_preserved:11;
> -        uint64_t pfn:48;
> -    };
> -};
> -
>  struct viridian_page
>  {
> -    union viridian_page_msr msr;
> +    union hv_vp_assist_page_msr msr;
>      void *ptr;
>  };
> 
> @@ -70,7 +59,7 @@ struct viridian_time_ref_count
>  struct viridian_domain
>  {
>      union hv_guest_os_id guest_os_id;
> -    union viridian_page_msr hypercall_gpa;
> +    union hv_vp_assist_page_msr hypercall_gpa;
>      struct viridian_time_ref_count time_ref_count;
>      struct viridian_page reference_tsc;
>  };
> --
> 2.20.1


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

Re: [Xen-devel] [PATCH v6 11/11] x86/hyperv: setup VP assist page

[Durrant, Paul]

> -----Original Message-----
> From: Wei Liu <wei.liu.xen@gmail.com> On Behalf Of Wei Liu
> Sent: 31 January 2020 17:50
> To: Xen Development List <xen-devel@lists.xenproject.org>
> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley
> <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu
> <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: [PATCH v6 11/11] x86/hyperv: setup VP assist page
> 
> VP assist page is rather important as we need to toggle some bits in it
> for efficient nested virtualisation.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>

Reviewed-by: Paul Durrant <pdurrant@amazon.com>

> ---
> v6:
> 1. Use hv_vp_assist_page_msr
> 2. Make code shorter
> 3. Preserve rsvdP fields
> 
> v5:
> 1. Deal with error properly instead of always panicking
> 2. Swap percpu variables declarations' location
> 
> v4:
> 1. Use private.h
> 2. Prevent leak
> 
> v3:
> 1. Use xenheap page
> 2. Drop set_vp_assist
> 
> v2:
> 1. Use HV_HYP_PAGE_SHIFT instead
> ---
>  xen/arch/x86/guest/hyperv/hyperv.c  | 37 ++++++++++++++++++++++++++++-
>  xen/arch/x86/guest/hyperv/private.h |  1 +
>  2 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c
> b/xen/arch/x86/guest/hyperv/hyperv.c
> index 6dac3bfceb..75fb329d4d 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -31,6 +31,7 @@
> 
>  struct ms_hyperv_info __read_mostly ms_hyperv;
>  DEFINE_PER_CPU_READ_MOSTLY(void *, hv_input_page);
> +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist);
>  DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index);
> 
>  static uint64_t generate_guest_id(void)
> @@ -155,17 +156,51 @@ static int setup_hypercall_pcpu_arg(void)
>      return 0;
>  }
> 
> +static int setup_vp_assist(void)
> +{
> +    union hv_vp_assist_page_msr msr;
> +
> +    if ( !this_cpu(hv_vp_assist) )
> +    {
> +        this_cpu(hv_vp_assist) = alloc_xenheap_page();
> +        if ( !this_cpu(hv_vp_assist) )
> +        {
> +            printk("CPU%u: Failed to allocate vp_assist page\n",
> +                   smp_processor_id());
> +            return -ENOMEM;
> +        }
> +
> +        clear_page(this_cpu(hv_vp_assist));
> +    }
> +
> +    rdmsrl(HV_X64_MSR_VP_ASSIST_PAGE, msr.raw);
> +    msr.pfn = virt_to_mfn(this_cpu(hv_vp_assist));
> +    msr.enabled = 1;
> +    wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, msr.raw);
> +
> +    return 0;
> +}
> +
>  static void __init setup(void)
>  {
>      setup_hypercall_page();
> 
>      if ( setup_hypercall_pcpu_arg() )
>          panic("Hyper-V hypercall percpu arg setup failed\n");
> +
> +    if ( setup_vp_assist() )
> +        panic("VP assist page setup failed\n");
>  }
> 
>  static int ap_setup(void)
>  {
> -    return setup_hypercall_pcpu_arg();
> +    int rc;
> +
> +    rc = setup_hypercall_pcpu_arg();
> +    if ( rc )
> +        return rc;
> +
> +    return setup_vp_assist();
>  }
> 
>  static void __init e820_fixup(struct e820map *e820)
> diff --git a/xen/arch/x86/guest/hyperv/private.h
> b/xen/arch/x86/guest/hyperv/private.h
> index d1765d4f23..956eff831f 100644
> --- a/xen/arch/x86/guest/hyperv/private.h
> +++ b/xen/arch/x86/guest/hyperv/private.h
> @@ -25,6 +25,7 @@
>  #include <xen/percpu.h>
> 
>  DECLARE_PER_CPU(void *, hv_input_page);
> +DECLARE_PER_CPU(void *, hv_vp_assist);
>  DECLARE_PER_CPU(unsigned int, hv_vp_index);
> 
>  #endif /* __XEN_HYPERV_PRIVIATE_H__  */
> --
> 2.20.1


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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Jan Beulich]

On 03.02.2020 12:21, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 03 February 2020 11:06
>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>> Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
>> devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>; Wei
>> Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
>> Roger Pau Monné <roger.pau@citrix.com>
>> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
>>
>> On 03.02.2020 11:39, Durrant, Paul wrote:
>>>> -----Original Message-----
>>>> From: Wei Liu <wl@xen.org>
>>>> Sent: 31 January 2020 17:57
>>>> To: Xen Development List <xen-devel@lists.xenproject.org>
>>>> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley
>>>> <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu
>>>> <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
>>>> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
>>>> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
>>>>
>>>> (Note to self)
>>>>
>>>> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
>>>> [...]
>>>>> +static uint64_t generate_guest_id(void)
>>>>> +{
>>>>> +    union hv_guest_os_id id;
>>>>> +
>>>>
>>>>        id.raw = 0;
>>>
>>> Or just use a C99 initializer to set things up. A bit neater IMO.
>>
>> If you mean this for ...
>>
>>>>> +    id.vendor = HV_XEN_VENDOR_ID;
>>>>> +    id.major = xen_major_version();
>>>>> +    id.minor = xen_minor_version();
>>
>> ... these three fields, then this won't work with rather old gcc
>> we still permit to be used. Using { .raw = 0 } would work afaict.
>>
> 
> Not even { .vendor = HV_XEN_VENDOR_ID } ?

No, because of it being part of an unnamed (struct) member of
the union.

Jan

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 03 February 2020 11:34
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
> devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>; Wei
> Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> 
> On 03.02.2020 12:21, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 03 February 2020 11:06
> >> To: Durrant, Paul <pdurrant@amazon.co.uk>
> >> Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
> >> devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>;
> Wei
> >> Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> >> Roger Pau Monné <roger.pau@citrix.com>
> >> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> >>
> >> On 03.02.2020 11:39, Durrant, Paul wrote:
> >>>> -----Original Message-----
> >>>> From: Wei Liu <wl@xen.org>
> >>>> Sent: 31 January 2020 17:57
> >>>> To: Xen Development List <xen-devel@lists.xenproject.org>
> >>>> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley
> >>>> <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu
> >>>> <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> >>>> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
> >>>> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> >>>>
> >>>> (Note to self)
> >>>>
> >>>> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
> >>>> [...]
> >>>>> +static uint64_t generate_guest_id(void)
> >>>>> +{
> >>>>> +    union hv_guest_os_id id;
> >>>>> +
> >>>>
> >>>>        id.raw = 0;
> >>>
> >>> Or just use a C99 initializer to set things up. A bit neater IMO.
> >>
> >> If you mean this for ...
> >>
> >>>>> +    id.vendor = HV_XEN_VENDOR_ID;
> >>>>> +    id.major = xen_major_version();
> >>>>> +    id.minor = xen_minor_version();
> >>
> >> ... these three fields, then this won't work with rather old gcc
> >> we still permit to be used. Using { .raw = 0 } would work afaict.
> >>
> >
> > Not even { .vendor = HV_XEN_VENDOR_ID } ?
> 
> No, because of it being part of an unnamed (struct) member of
> the union.

Ok... shame. Presumably an empty initializer - {} -  would be ok?

  Paul

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Jan Beulich]

On 03.02.2020 12:37, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 03 February 2020 11:34
>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>> Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
>> devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>; Wei
>> Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
>> Roger Pau Monné <roger.pau@citrix.com>
>> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
>>
>> On 03.02.2020 12:21, Durrant, Paul wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: 03 February 2020 11:06
>>>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>>>> Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
>>>> devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>;
>> Wei
>>>> Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
>>>> Roger Pau Monné <roger.pau@citrix.com>
>>>> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
>>>>
>>>> On 03.02.2020 11:39, Durrant, Paul wrote:
>>>>>> -----Original Message-----
>>>>>> From: Wei Liu <wl@xen.org>
>>>>>> Sent: 31 January 2020 17:57
>>>>>> To: Xen Development List <xen-devel@lists.xenproject.org>
>>>>>> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley
>>>>>> <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu
>>>>>> <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
>>>>>> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
>>>>>> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
>>>>>>
>>>>>> (Note to self)
>>>>>>
>>>>>> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
>>>>>> [...]
>>>>>>> +static uint64_t generate_guest_id(void)
>>>>>>> +{
>>>>>>> +    union hv_guest_os_id id;
>>>>>>> +
>>>>>>
>>>>>>        id.raw = 0;
>>>>>
>>>>> Or just use a C99 initializer to set things up. A bit neater IMO.
>>>>
>>>> If you mean this for ...
>>>>
>>>>>>> +    id.vendor = HV_XEN_VENDOR_ID;
>>>>>>> +    id.major = xen_major_version();
>>>>>>> +    id.minor = xen_minor_version();
>>>>
>>>> ... these three fields, then this won't work with rather old gcc
>>>> we still permit to be used. Using { .raw = 0 } would work afaict.
>>>>
>>>
>>> Not even { .vendor = HV_XEN_VENDOR_ID } ?
>>
>> No, because of it being part of an unnamed (struct) member of
>> the union.
> 
> Ok... shame. Presumably an empty initializer - {} -  would be ok?

I think so, yes. I understand you'd like this better than
{ .raw = 0 } ?

Jan

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 03 February 2020 11:39
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
> devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>; Wei
> Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> 
> On 03.02.2020 12:37, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 03 February 2020 11:34
> >> To: Durrant, Paul <pdurrant@amazon.co.uk>
> >> Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
> >> devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>;
> Wei
> >> Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> >> Roger Pau Monné <roger.pau@citrix.com>
> >> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> >>
> >> On 03.02.2020 12:21, Durrant, Paul wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich <jbeulich@suse.com>
> >>>> Sent: 03 February 2020 11:06
> >>>> To: Durrant, Paul <pdurrant@amazon.co.uk>
> >>>> Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
> >>>> devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>;
> >> Wei
> >>>> Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> >>>> Roger Pau Monné <roger.pau@citrix.com>
> >>>> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> >>>>
> >>>> On 03.02.2020 11:39, Durrant, Paul wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Wei Liu <wl@xen.org>
> >>>>>> Sent: 31 January 2020 17:57
> >>>>>> To: Xen Development List <xen-devel@lists.xenproject.org>
> >>>>>> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley
> >>>>>> <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu
> >>>>>> <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> >>>>>> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
> >>>>>> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> >>>>>>
> >>>>>> (Note to self)
> >>>>>>
> >>>>>> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
> >>>>>> [...]
> >>>>>>> +static uint64_t generate_guest_id(void)
> >>>>>>> +{
> >>>>>>> +    union hv_guest_os_id id;
> >>>>>>> +
> >>>>>>
> >>>>>>        id.raw = 0;
> >>>>>
> >>>>> Or just use a C99 initializer to set things up. A bit neater IMO.
> >>>>
> >>>> If you mean this for ...
> >>>>
> >>>>>>> +    id.vendor = HV_XEN_VENDOR_ID;
> >>>>>>> +    id.major = xen_major_version();
> >>>>>>> +    id.minor = xen_minor_version();
> >>>>
> >>>> ... these three fields, then this won't work with rather old gcc
> >>>> we still permit to be used. Using { .raw = 0 } would work afaict.
> >>>>
> >>>
> >>> Not even { .vendor = HV_XEN_VENDOR_ID } ?
> >>
> >> No, because of it being part of an unnamed (struct) member of
> >> the union.
> >
> > Ok... shame. Presumably an empty initializer - {} -  would be ok?
> 
> I think so, yes. I understand you'd like this better than
> { .raw = 0 } ?
> 

Yes. In general, using a c99 initializer to explicitly set something to 0 seems a bit redundant to me.

  Paul

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Wei Liu]

On Mon, Feb 03, 2020 at 11:41:57AM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Jan Beulich <jbeulich@suse.com>
> > Sent: 03 February 2020 11:39
> > To: Durrant, Paul <pdurrant@amazon.co.uk>
> > Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
> > devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>; Wei
> > Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> > Roger Pau Monné <roger.pau@citrix.com>
> > Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> > 
> > On 03.02.2020 12:37, Durrant, Paul wrote:
> > >> -----Original Message-----
> > >> From: Jan Beulich <jbeulich@suse.com>
> > >> Sent: 03 February 2020 11:34
> > >> To: Durrant, Paul <pdurrant@amazon.co.uk>
> > >> Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
> > >> devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>;
> > Wei
> > >> Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> > >> Roger Pau Monné <roger.pau@citrix.com>
> > >> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> > >>
> > >> On 03.02.2020 12:21, Durrant, Paul wrote:
> > >>>> -----Original Message-----
> > >>>> From: Jan Beulich <jbeulich@suse.com>
> > >>>> Sent: 03 February 2020 11:06
> > >>>> To: Durrant, Paul <pdurrant@amazon.co.uk>
> > >>>> Cc: Wei Liu <wl@xen.org>; Xen Development List <xen-
> > >>>> devel@lists.xenproject.org>; Michael Kelley <mikelley@microsoft.com>;
> > >> Wei
> > >>>> Liu <liuwe@microsoft.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> > >>>> Roger Pau Monné <roger.pau@citrix.com>
> > >>>> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> > >>>>
> > >>>> On 03.02.2020 11:39, Durrant, Paul wrote:
> > >>>>>> -----Original Message-----
> > >>>>>> From: Wei Liu <wl@xen.org>
> > >>>>>> Sent: 31 January 2020 17:57
> > >>>>>> To: Xen Development List <xen-devel@lists.xenproject.org>
> > >>>>>> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley
> > >>>>>> <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu
> > >>>>>> <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> > >>>>>> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
> > >>>>>> Subject: Re: [PATCH v6 05/11] x86/hyperv: setup hypercall page
> > >>>>>>
> > >>>>>> (Note to self)
> > >>>>>>
> > >>>>>> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
> > >>>>>> [...]
> > >>>>>>> +static uint64_t generate_guest_id(void)
> > >>>>>>> +{
> > >>>>>>> +    union hv_guest_os_id id;
> > >>>>>>> +
> > >>>>>>
> > >>>>>>        id.raw = 0;
> > >>>>>
> > >>>>> Or just use a C99 initializer to set things up. A bit neater IMO.
> > >>>>
> > >>>> If you mean this for ...
> > >>>>
> > >>>>>>> +    id.vendor = HV_XEN_VENDOR_ID;
> > >>>>>>> +    id.major = xen_major_version();
> > >>>>>>> +    id.minor = xen_minor_version();
> > >>>>
> > >>>> ... these three fields, then this won't work with rather old gcc
> > >>>> we still permit to be used. Using { .raw = 0 } would work afaict.
> > >>>>
> > >>>
> > >>> Not even { .vendor = HV_XEN_VENDOR_ID } ?
> > >>
> > >> No, because of it being part of an unnamed (struct) member of
> > >> the union.
> > >
> > > Ok... shame. Presumably an empty initializer - {} -  would be ok?
> > 
> > I think so, yes. I understand you'd like this better than
> > { .raw = 0 } ?
> > 
> 
> Yes. In general, using a c99 initializer to explicitly set something
> to 0 seems a bit redundant to me.

Alright. I have changed it to

  union hv_guest_os_id id = {};

in my local branch.

Wei.

> 
>   Paul
> 
> > Jan

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

Re: [Xen-devel] [PATCH v6 01/11] x86/hypervisor: make hypervisor_ap_setup return an error code

[Jan Beulich]

On 31.01.2020 18:49, Wei Liu wrote:
> We want to be able to handle AP setup error in the upper layer.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>
> ---
> v6:
> 1. Change map_vcpuinfo as well

And by implication then ...

> @@ -257,11 +257,17 @@ static void __init setup(void)
>      init_evtchn();
>  }
>  
> -static void ap_setup(void)
> +static int ap_setup(void)
>  {
> +    int rc;
> +
>      set_vcpu_id();
> -    map_vcpuinfo();
> -    init_evtchn();
> +    rc = map_vcpuinfo();
> +
> +    if ( !rc )
> +        init_evtchn();

... init_evtchn() as well?

Jan

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

Re: [Xen-devel] [PATCH v6 03/11] x86: provide executable fixmap facility

[Jan Beulich]

On 31.01.2020 18:49, Wei Liu wrote:
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -2,6 +2,8 @@
>  /* Modified for i386/x86-64 Xen by Keir Fraser */
>  
>  #include <xen/cache.h>
> +
> +#include <asm/fixmap.h>

I don't think you need this anymore? If so, with this dropped
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Otherwise please clarify.

Jan

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

Re: [Xen-devel] [PATCH v6 04/11] x86/hypervisor: provide hypervisor_fixup_e820

[Jan Beulich]

On 31.01.2020 18:49, Wei Liu wrote:
> And implement the hook for Xen guest.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Jan Beulich]

On 31.01.2020 18:49, Wei Liu wrote:
> +static void __init setup_hypercall_page(void)
> +{
> +    union hv_x64_msr_hypercall_contents hypercall_msr;
> +    union hv_guest_os_id guest_id;
> +    unsigned long mfn;
> +
> +    BUILD_BUG_ON(HV_HYP_PAGE_SHIFT != PAGE_SHIFT);
> +
> +    rdmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
> +    if ( !guest_id.raw )
> +    {
> +        guest_id.raw = generate_guest_id();
> +        wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
> +    }
> +
> +    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +    if ( !hypercall_msr.enable )
> +    {
> +        mfn = HV_HCALL_MFN;
> +        hypercall_msr.enable = 1;
> +        hypercall_msr.guest_physical_address = mfn;
> +        wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +    } else {
> +        mfn = hypercall_msr.guest_physical_address;
> +    }

Nit: Style. Preferably omit the braces from "else" altogether.

Jan


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

Re: [Xen-devel] [PATCH v6 03/11] x86: provide executable fixmap facility

[Wei Liu]

On Mon, Feb 03, 2020 at 02:10:07PM +0100, Jan Beulich wrote:
> On 31.01.2020 18:49, Wei Liu wrote:
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -2,6 +2,8 @@
> >  /* Modified for i386/x86-64 Xen by Keir Fraser */
> >  
> >  #include <xen/cache.h>
> > +
> > +#include <asm/fixmap.h>
> 
> I don't think you need this anymore? If so, with this dropped

I think it can be dropped.

> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

Wei.

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

Re: [Xen-devel] [PATCH v6 06/11] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich]

On 31.01.2020 18:49, Wei Liu wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -375,9 +375,11 @@ void __init arch_init_memory(void)
>      }
>  #endif
>  
> -    /* Generate a symbol to be used in linker script */
> +    /* Generate symbols to be used in linker script */
>      asm ( ".equ FIXADDR_X_SIZE, %P0; .global FIXADDR_X_SIZE"
>            :: "i" (FIXADDR_X_SIZE) );
> +    asm ( ".equ HV_HCALL_PAGE, %P0; .global HV_HCALL_PAGE"
> +          :: "i" (__fix_x_to_virt(FIX_X_HYPERV_HCALL)) );

Would this even build without CONFIG_HYPERV_GUEST? In any event
it doesn't belong here, but should go in a Hyper-V specific
file.

Seeing you now need two of these, how about macro-izing the
construct?

Jan

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

Re: [Xen-devel] [PATCH v6 06/11] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Mon, Feb 03, 2020 at 02:26:24PM +0100, Jan Beulich wrote:
> On 31.01.2020 18:49, Wei Liu wrote:
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -375,9 +375,11 @@ void __init arch_init_memory(void)
> >      }
> >  #endif
> >  
> > -    /* Generate a symbol to be used in linker script */
> > +    /* Generate symbols to be used in linker script */
> >      asm ( ".equ FIXADDR_X_SIZE, %P0; .global FIXADDR_X_SIZE"
> >            :: "i" (FIXADDR_X_SIZE) );
> > +    asm ( ".equ HV_HCALL_PAGE, %P0; .global HV_HCALL_PAGE"
> > +          :: "i" (__fix_x_to_virt(FIX_X_HYPERV_HCALL)) );
> 
> Would this even build without CONFIG_HYPERV_GUEST? In any event
> it doesn't belong here, but should go in a Hyper-V specific
> file.
> 

Good catch. When I did my full build tests it was done with my previous
version.

I can move this to hyperv.c.

> Seeing you now need two of these, how about macro-izing the
> construct?

What name would you suggest? I'm thinking about GEN_XEN_LDS_SYMBOL.

And presumably I should put it in xen/lib.h?

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Wei Liu]

On Mon, Feb 03, 2020 at 02:21:14PM +0100, Jan Beulich wrote:
> On 31.01.2020 18:49, Wei Liu wrote:
> > +static void __init setup_hypercall_page(void)
> > +{
> > +    union hv_x64_msr_hypercall_contents hypercall_msr;
> > +    union hv_guest_os_id guest_id;
> > +    unsigned long mfn;
> > +
> > +    BUILD_BUG_ON(HV_HYP_PAGE_SHIFT != PAGE_SHIFT);
> > +
> > +    rdmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
> > +    if ( !guest_id.raw )
> > +    {
> > +        guest_id.raw = generate_guest_id();
> > +        wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
> > +    }
> > +
> > +    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > +    if ( !hypercall_msr.enable )
> > +    {
> > +        mfn = HV_HCALL_MFN;
> > +        hypercall_msr.enable = 1;
> > +        hypercall_msr.guest_physical_address = mfn;
> > +        wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > +    } else {
> > +        mfn = hypercall_msr.guest_physical_address;
> > +    }
> 
> Nit: Style. Preferably omit the braces from "else" altogether.

Fixed.

Wei.

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

Re: [Xen-devel] [PATCH v6 06/11] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich]

On 03.02.2020 14:31, Wei Liu wrote:
> On Mon, Feb 03, 2020 at 02:26:24PM +0100, Jan Beulich wrote:
>> On 31.01.2020 18:49, Wei Liu wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -375,9 +375,11 @@ void __init arch_init_memory(void)
>>>      }
>>>  #endif
>>>  
>>> -    /* Generate a symbol to be used in linker script */
>>> +    /* Generate symbols to be used in linker script */
>>>      asm ( ".equ FIXADDR_X_SIZE, %P0; .global FIXADDR_X_SIZE"
>>>            :: "i" (FIXADDR_X_SIZE) );
>>> +    asm ( ".equ HV_HCALL_PAGE, %P0; .global HV_HCALL_PAGE"
>>> +          :: "i" (__fix_x_to_virt(FIX_X_HYPERV_HCALL)) );
>>
>> Would this even build without CONFIG_HYPERV_GUEST? In any event
>> it doesn't belong here, but should go in a Hyper-V specific
>> file.
>>
> 
> Good catch. When I did my full build tests it was done with my previous
> version.
> 
> I can move this to hyperv.c.
> 
>> Seeing you now need two of these, how about macro-izing the
>> construct?
> 
> What name would you suggest? I'm thinking about GEN_XEN_LDS_SYMBOL.

In principle this isn't limiting things to use by xen.lds, so
I'd prefer to not encode such in the name. asm_constant()? Or
all caps if so preferred by others?

> And presumably I should put it in xen/lib.h?

Would be nice to have it there, but I'm afraid this is against
what gcc documents. Hence if anything the P would need to be
abstracted away as a per-arch thing. If you don't want to go
this far, asm_defns.h might be the best fit among the x86
headers.

Jan

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

Re: [Xen-devel] [PATCH v6 06/11] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Mon, Feb 03, 2020 at 02:48:42PM +0100, Jan Beulich wrote:
> On 03.02.2020 14:31, Wei Liu wrote:
> > On Mon, Feb 03, 2020 at 02:26:24PM +0100, Jan Beulich wrote:
> >> On 31.01.2020 18:49, Wei Liu wrote:
> >>> --- a/xen/arch/x86/mm.c
> >>> +++ b/xen/arch/x86/mm.c
> >>> @@ -375,9 +375,11 @@ void __init arch_init_memory(void)
> >>>      }
> >>>  #endif
> >>>  
> >>> -    /* Generate a symbol to be used in linker script */
> >>> +    /* Generate symbols to be used in linker script */
> >>>      asm ( ".equ FIXADDR_X_SIZE, %P0; .global FIXADDR_X_SIZE"
> >>>            :: "i" (FIXADDR_X_SIZE) );
> >>> +    asm ( ".equ HV_HCALL_PAGE, %P0; .global HV_HCALL_PAGE"
> >>> +          :: "i" (__fix_x_to_virt(FIX_X_HYPERV_HCALL)) );
> >>
> >> Would this even build without CONFIG_HYPERV_GUEST? In any event
> >> it doesn't belong here, but should go in a Hyper-V specific
> >> file.
> >>
> > 
> > Good catch. When I did my full build tests it was done with my previous
> > version.
> > 
> > I can move this to hyperv.c.
> > 
> >> Seeing you now need two of these, how about macro-izing the
> >> construct?
> > 
> > What name would you suggest? I'm thinking about GEN_XEN_LDS_SYMBOL.
> 
> In principle this isn't limiting things to use by xen.lds, so
> I'd prefer to not encode such in the name. asm_constant()? Or
> all caps if so preferred by others?

I am certainly okay with ASM_CONSTANT().

> 
> > And presumably I should put it in xen/lib.h?
> 
> Would be nice to have it there, but I'm afraid this is against
> what gcc documents. Hence if anything the P would need to be
> abstracted away as a per-arch thing. If you don't want to go
> this far, asm_defns.h might be the best fit among the x86
> headers.

OK. asm_defns.h it is. Arm doesn't need this for now anyway.

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v6 01/11] x86/hypervisor: make hypervisor_ap_setup return an error code

[Wei Liu]

On Mon, Feb 03, 2020 at 01:56:48PM +0100, Jan Beulich wrote:
> On 31.01.2020 18:49, Wei Liu wrote:
> > We want to be able to handle AP setup error in the upper layer.
> > 
> > Signed-off-by: Wei Liu <liuwe@microsoft.com>
> > ---
> > v6:
> > 1. Change map_vcpuinfo as well
> 
> And by implication then ...
> 
> > @@ -257,11 +257,17 @@ static void __init setup(void)
> >      init_evtchn();
> >  }
> >  
> > -static void ap_setup(void)
> > +static int ap_setup(void)
> >  {
> > +    int rc;
> > +
> >      set_vcpu_id();
> > -    map_vcpuinfo();
> > -    init_evtchn();
> > +    rc = map_vcpuinfo();
> > +
> > +    if ( !rc )
> > +        init_evtchn();
> 
> ... init_evtchn() as well?

OK. I can change that too.

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v6 11/11] x86/hyperv: setup VP assist page

[Roger Pau Monné]

On Fri, Jan 31, 2020 at 05:49:30PM +0000, Wei Liu wrote:
> VP assist page is rather important as we need to toggle some bits in it
> for efficient nested virtualisation.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v6 04/11] x86/hypervisor: provide hypervisor_fixup_e820

[Roger Pau Monné]

On Fri, Jan 31, 2020 at 05:49:23PM +0000, Wei Liu wrote:
> And implement the hook for Xen guest.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>
> ---
>  xen/arch/x86/e820.c                    | 4 ++--
>  xen/arch/x86/guest/hypervisor.c        | 6 ++++++
>  xen/arch/x86/guest/xen/xen.c           | 7 +++++++
>  xen/include/asm-x86/guest/hypervisor.h | 6 ++++++
>  4 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> index 3892c9cfb7..2219c63861 100644
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -690,8 +690,8 @@ unsigned long __init init_e820(const char *str, struct e820map *raw)
>  
>      machine_specific_memory_setup(raw);
>  
> -    if ( pv_shim )
> -        pv_shim_fixup_e820(&e820);
> +    if ( cpu_has_hypervisor )
> +        hypervisor_e820_fixup(&e820);
>  
>      printk("%s RAM map:\n", str);
>      print_e820_memory_map(e820.map, e820.nr_map);
> diff --git a/xen/arch/x86/guest/hypervisor.c b/xen/arch/x86/guest/hypervisor.c
> index e72c92ffdf..5fd433c8d4 100644
> --- a/xen/arch/x86/guest/hypervisor.c
> +++ b/xen/arch/x86/guest/hypervisor.c
> @@ -66,6 +66,12 @@ void hypervisor_resume(void)
>          ops->resume();
>  }
>  
> +void __init hypervisor_e820_fixup(struct e820map *e820)
> +{
> +    if ( ops && ops->e820_fixup )
> +        ops->e820_fixup(e820);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c
> index d50f86bae7..45e54dfbba 100644
> --- a/xen/arch/x86/guest/xen/xen.c
> +++ b/xen/arch/x86/guest/xen/xen.c
> @@ -316,11 +316,18 @@ static void resume(void)
>          pv_console_init();
>  }
>  
> +static void __init e820_fixup(struct e820map *e820)
> +{
> +    if ( pv_shim )
> +        pv_shim_fixup_e820(e820);
> +}
> +
>  static const struct hypervisor_ops ops = {
>      .name = "Xen",
>      .setup = setup,
>      .ap_setup = ap_setup,
>      .resume = resume,
> +    .e820_fixup = e820_fixup,
>  };
>  
>  const struct hypervisor_ops *__init xg_probe(void)
> diff --git a/xen/include/asm-x86/guest/hypervisor.h b/xen/include/asm-x86/guest/hypervisor.h
> index b503854c5b..b66cb28333 100644
> --- a/xen/include/asm-x86/guest/hypervisor.h
> +++ b/xen/include/asm-x86/guest/hypervisor.h
> @@ -19,6 +19,8 @@
>  #ifndef __X86_HYPERVISOR_H__
>  #define __X86_HYPERVISOR_H__
>  
> +#include <asm/e820.h>
> +
>  struct hypervisor_ops {
>      /* Name of the hypervisor */
>      const char *name;
> @@ -28,6 +30,8 @@ struct hypervisor_ops {
>      int (*ap_setup)(void);
>      /* Resume from suspension */
>      void (*resume)(void);
> +    /* Fix up e820 map */
> +    void (*e820_fixup)(struct e820map *e820);
>  };
>  
>  #ifdef CONFIG_GUEST
> @@ -36,6 +40,7 @@ const char *hypervisor_probe(void);
>  void hypervisor_setup(void);
>  int hypervisor_ap_setup(void);
>  void hypervisor_resume(void);
> +void hypervisor_e820_fixup(struct e820map *e820);
>  
>  #else
>  
> @@ -46,6 +51,7 @@ static inline const char *hypervisor_probe(void) { return NULL; }
>  static inline void hypervisor_setup(void) { ASSERT_UNREACHABLE(); }
>  static inline int hypervisor_ap_setup(void) { ASSERT_UNREACHABLE(); return 0; }
>  static inline void hypervisor_resume(void) { ASSERT_UNREACHABLE(); }
> +static inline void hypervisor_e820_fixup(struct e820map *e820) { ASSERT_UNREACHABLE(); }

Are you sure the assert here is fine?

Consider Xen running nested on another hypervisor, and built without
CONFIG_GUEST enabled, I think the above assert would trigger.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v6 04/11] x86/hypervisor: provide hypervisor_fixup_e820

[Wei Liu]

On Mon, Feb 03, 2020 at 03:32:01PM +0100, Roger Pau Monné wrote:
[...]
> >  
> >  #else
> >  
> > @@ -46,6 +51,7 @@ static inline const char *hypervisor_probe(void) { return NULL; }
> >  static inline void hypervisor_setup(void) { ASSERT_UNREACHABLE(); }
> >  static inline int hypervisor_ap_setup(void) { ASSERT_UNREACHABLE(); return 0; }
> >  static inline void hypervisor_resume(void) { ASSERT_UNREACHABLE(); }
> > +static inline void hypervisor_e820_fixup(struct e820map *e820) { ASSERT_UNREACHABLE(); }
> 
> Are you sure the assert here is fine?
> 
> Consider Xen running nested on another hypervisor, and built without
> CONFIG_GUEST enabled, I think the above assert would trigger.

Hmm... yes, this assertion should be dropped. Thanks.

Wei.

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Roger Pau Monné]

On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
> Hyper-V uses a technique called overlay page for its hypercall page. It
> will insert a backing page to the guest when the hypercall functionality
> is enabled. That means we can use a page that is not backed by real
> memory for hypercall page.
> 
> Use the top-most addressable page for that purpose. Adjust e820 map
> accordingly.

Can you add this is done to avoid page shattering and to make sure
Xen isn't overwriting any MMIO area which might be present at lower
addresses?

> 
> We also need to register Xen's guest OS ID to Hyper-V. Use 0x3 as the
> vendor ID. Fix the comment in hyperv-tlfs.h while at it.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>
> ---
> v6:
> 1. Use hv_guest_os_id
> 2. Use new e820_fixup hook
> 3. Add a BUILD_BUG_ON
> 
> v5:
> 1. use hypervisor_reserve_top_pages
> 2. add a macro for hypercall page mfn
> 3. address other misc comments
> 
> v4:
> 1. Use fixmap
> 2. Follow routines listed in TLFS
> ---
>  xen/arch/x86/guest/hyperv/hyperv.c      | 69 +++++++++++++++++++++++--
>  xen/include/asm-x86/guest/hyperv-tlfs.h |  5 +-
>  xen/include/asm-x86/guest/hyperv.h      |  3 ++
>  3 files changed, 72 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> index 8d38313d7a..7c2a96d70e 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -19,15 +19,27 @@
>   * Copyright (c) 2019 Microsoft.
>   */
>  #include <xen/init.h>
> +#include <xen/version.h>
>  
> +#include <asm/fixmap.h>
>  #include <asm/guest.h>
>  #include <asm/guest/hyperv-tlfs.h>
> +#include <asm/processor.h>
>  
>  struct ms_hyperv_info __read_mostly ms_hyperv;
>  
> -static const struct hypervisor_ops ops = {
> -    .name = "Hyper-V",
> -};
> +static uint64_t generate_guest_id(void)
> +{
> +    union hv_guest_os_id id;
> +
> +    id.vendor = HV_XEN_VENDOR_ID;
> +    id.major = xen_major_version();
> +    id.minor = xen_minor_version();
> +
> +    return id.raw;
> +}
> +
> +static const struct hypervisor_ops ops;
>  
>  const struct hypervisor_ops *__init hyperv_probe(void)
>  {
> @@ -72,6 +84,57 @@ const struct hypervisor_ops *__init hyperv_probe(void)
>      return &ops;
>  }
>  
> +static void __init setup_hypercall_page(void)
> +{
> +    union hv_x64_msr_hypercall_contents hypercall_msr;
> +    union hv_guest_os_id guest_id;
> +    unsigned long mfn;
> +
> +    BUILD_BUG_ON(HV_HYP_PAGE_SHIFT != PAGE_SHIFT);
> +
> +    rdmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
> +    if ( !guest_id.raw )
> +    {
> +        guest_id.raw = generate_guest_id();
> +        wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
> +    }
> +
> +    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +    if ( !hypercall_msr.enable )
> +    {
> +        mfn = HV_HCALL_MFN;
> +        hypercall_msr.enable = 1;
> +        hypercall_msr.guest_physical_address = mfn;
> +        wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +    } else {
> +        mfn = hypercall_msr.guest_physical_address;
> +    }
> +
> +    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +    BUG_ON(!hypercall_msr.enable);
> +
> +    set_fixmap_x(FIX_X_HYPERV_HCALL, mfn << PAGE_SHIFT);
> +}
> +
> +static void __init setup(void)
> +{
> +    setup_hypercall_page();
> +}
> +
> +static void __init e820_fixup(struct e820map *e820)
> +{
> +    uint64_t s = HV_HCALL_MFN << PAGE_SHIFT;
> +
> +    if ( !e820_add_range(e820, s, s + PAGE_SIZE, E820_RESERVED) )

I think end should be s + PAGE_SIZE - 1, or else it expands across two
pages?

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Wei Liu]

On Mon, Feb 03, 2020 at 04:01:54PM +0100, Roger Pau Monné wrote:
> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
> > Hyper-V uses a technique called overlay page for its hypercall page. It
> > will insert a backing page to the guest when the hypercall functionality
> > is enabled. That means we can use a page that is not backed by real
> > memory for hypercall page.
> > 
> > Use the top-most addressable page for that purpose. Adjust e820 map
> > accordingly.
> 
> Can you add this is done to avoid page shattering and to make sure
> Xen isn't overwriting any MMIO area which might be present at lower
> addresses?

NP.

> 
> > +
> > +static void __init e820_fixup(struct e820map *e820)
> > +{
> > +    uint64_t s = HV_HCALL_MFN << PAGE_SHIFT;
> > +
> > +    if ( !e820_add_range(e820, s, s + PAGE_SIZE, E820_RESERVED) )
> 
> I think end should be s + PAGE_SIZE - 1, or else it expands across two
> pages?

No, it shouldn't.

E820 entry records the size of the region, which is calculated as
end-start. The one usage in pv/shim.c follows the same pattern here.

Wei.

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Roger Pau Monné]

On Mon, Feb 03, 2020 at 03:07:24PM +0000, Wei Liu wrote:
> On Mon, Feb 03, 2020 at 04:01:54PM +0100, Roger Pau Monné wrote:
> > On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
> > > Hyper-V uses a technique called overlay page for its hypercall page. It
> > > will insert a backing page to the guest when the hypercall functionality
> > > is enabled. That means we can use a page that is not backed by real
> > > memory for hypercall page.
> > > 
> > > Use the top-most addressable page for that purpose. Adjust e820 map
> > > accordingly.
> > 
> > Can you add this is done to avoid page shattering and to make sure
> > Xen isn't overwriting any MMIO area which might be present at lower
> > addresses?
> 
> NP.
> 
> > 
> > > +
> > > +static void __init e820_fixup(struct e820map *e820)
> > > +{
> > > +    uint64_t s = HV_HCALL_MFN << PAGE_SHIFT;
> > > +
> > > +    if ( !e820_add_range(e820, s, s + PAGE_SIZE, E820_RESERVED) )
> > 
> > I think end should be s + PAGE_SIZE - 1, or else it expands across two
> > pages?
> 
> No, it shouldn't.
> 
> E820 entry records the size of the region, which is calculated as
> end-start. The one usage in pv/shim.c follows the same pattern here.

Hm, I see. I'm not sure this is correct, I think the e820 entry
should look like:

addr = s;
size = PAGE_SIZE - 1;

As ranges on the e820 are inclusive, so if size ends up being
PAGE_SIZE then the entry would expand across two pages.

Anyway, this needs fixing elsewhere, and is out of the scope of this
patch.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Jan Beulich]

On 03.02.2020 16:21, Roger Pau Monné wrote:
> On Mon, Feb 03, 2020 at 03:07:24PM +0000, Wei Liu wrote:
>> On Mon, Feb 03, 2020 at 04:01:54PM +0100, Roger Pau Monné wrote:
>>> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
>>>> Hyper-V uses a technique called overlay page for its hypercall page. It
>>>> will insert a backing page to the guest when the hypercall functionality
>>>> is enabled. That means we can use a page that is not backed by real
>>>> memory for hypercall page.
>>>>
>>>> Use the top-most addressable page for that purpose. Adjust e820 map
>>>> accordingly.
>>>
>>> Can you add this is done to avoid page shattering and to make sure
>>> Xen isn't overwriting any MMIO area which might be present at lower
>>> addresses?
>>
>> NP.
>>
>>>
>>>> +
>>>> +static void __init e820_fixup(struct e820map *e820)
>>>> +{
>>>> +    uint64_t s = HV_HCALL_MFN << PAGE_SHIFT;
>>>> +
>>>> +    if ( !e820_add_range(e820, s, s + PAGE_SIZE, E820_RESERVED) )
>>>
>>> I think end should be s + PAGE_SIZE - 1, or else it expands across two
>>> pages?
>>
>> No, it shouldn't.
>>
>> E820 entry records the size of the region, which is calculated as
>> end-start. The one usage in pv/shim.c follows the same pattern here.
> 
> Hm, I see. I'm not sure this is correct, I think the e820 entry
> should look like:
> 
> addr = s;
> size = PAGE_SIZE - 1;
> 
> As ranges on the e820 are inclusive, so if size ends up being
> PAGE_SIZE then the entry would expand across two pages.

Ranges can sensibly be inclusive only when specified as [start,end]
tuples. (start,size) pairs make no sense for representing
[start,start+size], they only make sense for [start,start+size).
Otherwise, as in your example above, size taken on its own is off
by one (i.e. is rather "last byte" than "size").

Modern Linux, when logging the memory map, indeed subtracts 1 from
the sum of addr and size, to show an inclusive range.

Jan

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Wei Liu]

On Mon, Feb 03, 2020 at 04:32:52PM +0100, Jan Beulich wrote:
> On 03.02.2020 16:21, Roger Pau Monné wrote:
> > On Mon, Feb 03, 2020 at 03:07:24PM +0000, Wei Liu wrote:
> >> On Mon, Feb 03, 2020 at 04:01:54PM +0100, Roger Pau Monné wrote:
> >>> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
> >>>> Hyper-V uses a technique called overlay page for its hypercall page. It
> >>>> will insert a backing page to the guest when the hypercall functionality
> >>>> is enabled. That means we can use a page that is not backed by real
> >>>> memory for hypercall page.
> >>>>
> >>>> Use the top-most addressable page for that purpose. Adjust e820 map
> >>>> accordingly.
> >>>
> >>> Can you add this is done to avoid page shattering and to make sure
> >>> Xen isn't overwriting any MMIO area which might be present at lower
> >>> addresses?
> >>
> >> NP.
> >>
> >>>
> >>>> +
> >>>> +static void __init e820_fixup(struct e820map *e820)
> >>>> +{
> >>>> +    uint64_t s = HV_HCALL_MFN << PAGE_SHIFT;
> >>>> +
> >>>> +    if ( !e820_add_range(e820, s, s + PAGE_SIZE, E820_RESERVED) )
> >>>
> >>> I think end should be s + PAGE_SIZE - 1, or else it expands across two
> >>> pages?
> >>
> >> No, it shouldn't.
> >>
> >> E820 entry records the size of the region, which is calculated as
> >> end-start. The one usage in pv/shim.c follows the same pattern here.
> > 
> > Hm, I see. I'm not sure this is correct, I think the e820 entry
> > should look like:
> > 
> > addr = s;
> > size = PAGE_SIZE - 1;
> > 
> > As ranges on the e820 are inclusive, so if size ends up being
> > PAGE_SIZE then the entry would expand across two pages.
> 
> Ranges can sensibly be inclusive only when specified as [start,end]
> tuples. (start,size) pairs make no sense for representing
> [start,start+size], they only make sense for [start,start+size).
> Otherwise, as in your example above, size taken on its own is off
> by one (i.e. is rather "last byte" than "size").
> 
> Modern Linux, when logging the memory map, indeed subtracts 1 from
> the sum of addr and size, to show an inclusive range.

We should perhaps do the same then.

If people agree this is the way to go, I can write a patch.

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v6 05/11] x86/hyperv: setup hypercall page

[Roger Pau Monné]

On Mon, Feb 03, 2020 at 03:42:17PM +0000, Wei Liu wrote:
> On Mon, Feb 03, 2020 at 04:32:52PM +0100, Jan Beulich wrote:
> > On 03.02.2020 16:21, Roger Pau Monné wrote:
> > > On Mon, Feb 03, 2020 at 03:07:24PM +0000, Wei Liu wrote:
> > >> On Mon, Feb 03, 2020 at 04:01:54PM +0100, Roger Pau Monné wrote:
> > >>> On Fri, Jan 31, 2020 at 05:49:24PM +0000, Wei Liu wrote:
> > >>>> Hyper-V uses a technique called overlay page for its hypercall page. It
> > >>>> will insert a backing page to the guest when the hypercall functionality
> > >>>> is enabled. That means we can use a page that is not backed by real
> > >>>> memory for hypercall page.
> > >>>>
> > >>>> Use the top-most addressable page for that purpose. Adjust e820 map
> > >>>> accordingly.
> > >>>
> > >>> Can you add this is done to avoid page shattering and to make sure
> > >>> Xen isn't overwriting any MMIO area which might be present at lower
> > >>> addresses?
> > >>
> > >> NP.
> > >>
> > >>>
> > >>>> +
> > >>>> +static void __init e820_fixup(struct e820map *e820)
> > >>>> +{
> > >>>> +    uint64_t s = HV_HCALL_MFN << PAGE_SHIFT;
> > >>>> +
> > >>>> +    if ( !e820_add_range(e820, s, s + PAGE_SIZE, E820_RESERVED) )
> > >>>
> > >>> I think end should be s + PAGE_SIZE - 1, or else it expands across two
> > >>> pages?
> > >>
> > >> No, it shouldn't.
> > >>
> > >> E820 entry records the size of the region, which is calculated as
> > >> end-start. The one usage in pv/shim.c follows the same pattern here.
> > > 
> > > Hm, I see. I'm not sure this is correct, I think the e820 entry
> > > should look like:
> > > 
> > > addr = s;
> > > size = PAGE_SIZE - 1;
> > > 
> > > As ranges on the e820 are inclusive, so if size ends up being
> > > PAGE_SIZE then the entry would expand across two pages.
> > 
> > Ranges can sensibly be inclusive only when specified as [start,end]
> > tuples. (start,size) pairs make no sense for representing
> > [start,start+size], they only make sense for [start,start+size).
> > Otherwise, as in your example above, size taken on its own is off
> > by one (i.e. is rather "last byte" than "size").
> > 
> > Modern Linux, when logging the memory map, indeed subtracts 1 from
> > the sum of addr and size, to show an inclusive range.
> 
> We should perhaps do the same then.
> 
> If people agree this is the way to go, I can write a patch.

Oh, sorry. I got messed up by the way we print the ranges.

I think it would be helpful to -1 when printing the ranges, but
there's no need to do it in this series.

Thanks, Roger

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

Re: [Xen-devel] [PATCH v6 02/11] x86/smp: don't online cpu if hypervisor_ap_setup fails

[Wei Liu]

On Fri, Jan 31, 2020 at 05:49:21PM +0000, Wei Liu wrote:
> Push hypervisor_ap_setup down to smp_callin.
> 
> Take the chance to replace xen_guest with cpu_has_hypervisor.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/arch/x86/smpboot.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index c9d1ab4423..93b86a09e9 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -199,6 +199,13 @@ static void smp_callin(void)
>          goto halt;
>      }
>  
> +    if ( cpu_has_hypervisor && (rc = hypervisor_ap_setup()) != 0 )

In light of a comment made by Roger yesterday, with this change the
ASSERT_UNREACHABLE in hypervisor_ap_setup() stub should be dropped, to
deal with Xen running on a hypervisor with !CONFIG_GUEST.

I have folded in the following diff:

diff --git a/xen/include/asm-x86/guest/hypervisor.h b/xen/include/asm-x86/guest/hypervisor.h
index b503854c5b..64383f0c3d 100644
--- a/xen/include/asm-x86/guest/hypervisor.h
+++ b/xen/include/asm-x86/guest/hypervisor.h
@@ -44,7 +44,7 @@ void hypervisor_resume(void);

 static inline const char *hypervisor_probe(void) { return NULL; }
 static inline void hypervisor_setup(void) { ASSERT_UNREACHABLE(); }
-static inline int hypervisor_ap_setup(void) { ASSERT_UNREACHABLE(); return 0; }
+static inline int hypervisor_ap_setup(void) { return 0; }
 static inline void hypervisor_resume(void) { ASSERT_UNREACHABLE(); }

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

Re: [Xen-devel] [PATCH v6 02/11] x86/smp: don't online cpu if hypervisor_ap_setup fails

[Roger Pau Monné]

On Tue, Feb 04, 2020 at 11:20:38AM +0000, Wei Liu wrote:
> On Fri, Jan 31, 2020 at 05:49:21PM +0000, Wei Liu wrote:
> > Push hypervisor_ap_setup down to smp_callin.
> > 
> > Take the chance to replace xen_guest with cpu_has_hypervisor.
> > 
> > Signed-off-by: Wei Liu <liuwe@microsoft.com>
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > ---
> >  xen/arch/x86/smpboot.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> > index c9d1ab4423..93b86a09e9 100644
> > --- a/xen/arch/x86/smpboot.c
> > +++ b/xen/arch/x86/smpboot.c
> > @@ -199,6 +199,13 @@ static void smp_callin(void)
> >          goto halt;
> >      }
> >  
> > +    if ( cpu_has_hypervisor && (rc = hypervisor_ap_setup()) != 0 )
> 
> In light of a comment made by Roger yesterday, with this change the
> ASSERT_UNREACHABLE in hypervisor_ap_setup() stub should be dropped, to
> deal with Xen running on a hypervisor with !CONFIG_GUEST.
> 
> I have folded in the following diff:
> 
> diff --git a/xen/include/asm-x86/guest/hypervisor.h b/xen/include/asm-x86/guest/hypervisor.h
> index b503854c5b..64383f0c3d 100644
> --- a/xen/include/asm-x86/guest/hypervisor.h
> +++ b/xen/include/asm-x86/guest/hypervisor.h
> @@ -44,7 +44,7 @@ void hypervisor_resume(void);
> 
>  static inline const char *hypervisor_probe(void) { return NULL; }
>  static inline void hypervisor_setup(void) { ASSERT_UNREACHABLE(); }
> -static inline int hypervisor_ap_setup(void) { ASSERT_UNREACHABLE(); return 0; }
> +static inline int hypervisor_ap_setup(void) { return 0; }
>  static inline void hypervisor_resume(void) { ASSERT_UNREACHABLE(); }

Oh, I didn't notice this one indeed. Please keep my R-b.

Thanks, Roger.

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