[Xen-devel] [PATCH v5 00/12] More Hyper-V infrastructures

[Xen-devel] [PATCH v5 00/12] 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. First few patches are generic
x86 changes. The rest is Hyper-V specific.

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 (12):
  MAINTAINERS: put Hyper-V code under Viridian maintainership
  x86/hypervisor: make hypervisor_ap_setup return an error code
  x86/smp: don't online cpu if hypervisor_ap_setup fails
  x86: make paddr_bits available earlier
  x86: provide executable fixmap facility
  x86/hypervisor: provide hypervisor_reserve_top_pages
  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/hyperv: setup VP assist page

 MAINTAINERS                              |   6 +-
 xen/arch/x86/boot/x86_64.S               |  15 ++-
 xen/arch/x86/e820.c                      |  19 ++-
 xen/arch/x86/guest/hyperv/hyperv.c       | 155 ++++++++++++++++++++++-
 xen/arch/x86/guest/hyperv/private.h      |  31 +++++
 xen/arch/x86/guest/hypervisor.c          |  14 +-
 xen/arch/x86/guest/xen/xen.c             |  11 +-
 xen/arch/x86/livepatch.c                 |   3 +-
 xen/arch/x86/mm.c                        |  15 ++-
 xen/arch/x86/setup.c                     |   5 +-
 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 |  96 ++++++++++++++
 xen/include/asm-x86/guest/hyperv-tlfs.h  |   5 +-
 xen/include/asm-x86/guest/hyperv.h       |   3 +
 xen/include/asm-x86/guest/hypervisor.h   |  10 +-
 18 files changed, 398 insertions(+), 35 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 v5 01/12] MAINTAINERS: put Hyper-V code under Viridian maintainership

[Wei Liu]

And add myself as a maintainer.

Sort the list alphabetically while at it.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
Signed-off-by: Wei Liu <wl@xen.org>
---
 MAINTAINERS | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1915e09f8b..04d91482cd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -514,10 +514,13 @@ F:	xen/arch/x86/mm/shadow/
 
 X86 VIRIDIAN ENLIGHTENMENTS
 M:	Paul Durrant <pdurrant@amazon.com>
+M:	Wei Liu <wl@xen.org>
 S:	Supported
+F:	xen/arch/x86/guest/hyperv/
 F:	xen/arch/x86/hvm/viridian/
-F:	xen/include/asm-x86/hvm/viridian.h
+F:	xen/include/asm-x86/guest/hyperv.h
 F:	xen/include/asm-x86/guest/hyperv-tlfs.h
+F:	xen/include/asm-x86/hvm/viridian.h
 
 XENTRACE
 M:	George Dunlap <george.dunlap@eu.citrix.com>
-- 
2.20.1


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

[Xen-devel] [PATCH v5 02/12] 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>
---
 xen/arch/x86/guest/hypervisor.c        |  6 ++++--
 xen/arch/x86/guest/xen/xen.c           | 11 +++++++++--
 xen/include/asm-x86/guest/hypervisor.h |  6 +++---
 3 files changed, 16 insertions(+), 7 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..eed8a6edae 100644
--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -257,11 +257,18 @@ static void __init setup(void)
     init_evtchn();
 }
 
-static void ap_setup(void)
+static int ap_setup(void)
 {
+    int rc;
+
     set_vcpu_id();
-    map_vcpuinfo();
+    rc = map_vcpuinfo();
+    if ( rc )
+        return rc;
+
     init_evtchn();
+
+    return 0;
 }
 
 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 v5 03/12] 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>
---
 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 v5 04/12] x86: make paddr_bits available earlier

[Wei Liu]

Move early_cpu_init before init_e820, such that paddr_bits can be used
by e820 code.

This will reduce code repetition and prepare for further adjustment when
L0 hypervisor comes into play.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
 xen/arch/x86/e820.c  | 14 ++++----------
 xen/arch/x86/setup.c |  5 +++--
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 082f9928a1..3892c9cfb7 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -420,7 +420,7 @@ static uint64_t __init mtrr_top_of_ram(void)
 {
     uint32_t eax, ebx, ecx, edx;
     uint64_t mtrr_cap, mtrr_def, addr_mask, base, mask, top;
-    unsigned int i, phys_bits = 36;
+    unsigned int i;
 
     /* By default we check only Intel systems. */
     if ( e820_mtrr_clip == -1 )
@@ -445,15 +445,9 @@ static uint64_t __init mtrr_top_of_ram(void)
     if ( !test_bit(X86_FEATURE_MTRR & 31, &edx) )
          return 0;
 
-    /* Find the physical address size for this CPU. */
-    eax = cpuid_eax(0x80000000);
-    if ( (eax >> 16) == 0x8000 && eax >= 0x80000008 )
-    {
-        phys_bits = (uint8_t)cpuid_eax(0x80000008);
-        if ( phys_bits > PADDR_BITS )
-            phys_bits = PADDR_BITS;
-    }
-    addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
+    /* paddr_bits must have been set at this point */
+    ASSERT(paddr_bits);
+    addr_mask = ((1ull << paddr_bits) - 1) & PAGE_MASK;
 
     rdmsrl(MSR_MTRRcap, mtrr_cap);
     rdmsrl(MSR_MTRRdefType, mtrr_def);
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d858883404..89fe49149f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -954,6 +954,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     else
         panic("Bootloader provided no memory information\n");
 
+    /* This must come before e820 code becuause it sets paddr_bits. */
+    early_cpu_init();
+
     /* Sanitise the raw E820 map to produce a final clean version. */
     max_page = raw_max_page = init_e820(memmap_type, &e820_raw);
 
@@ -1532,8 +1535,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     softirq_init();
     tasklet_subsys_init();
 
-    early_cpu_init();
-
     paging_init();
 
     tboot_probe();
-- 
2.20.1


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

[Xen-devel] [PATCH v5 05/12] 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>
---
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 | 25 +++++++++++++++++++++++++
 7 files changed, 58 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..44abde24b2 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;
 
@@ -5718,10 +5720,21 @@ 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);
+
+    /* Generate a symbol to be used in linker script */
+    asm ( ".equ FIXADDR_X_SIZE, %c0; .global FIXADDR_X_SIZE"
+          :: "i" (__end_of_fixed_addresses_x << PAGE_SHIFT) );
+}
+
 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 07c6448dbb..97f9c07891 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -3,6 +3,8 @@
 
 #include <xen/cache.h>
 #include <xen/lib.h>
+
+#include <asm/fixmap.h>
 #include <asm/page.h>
 #undef ENTRY
 #undef ALIGN
@@ -353,6 +355,7 @@ SECTIONS
 }
 
 ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START -
+                          FIXADDR_X_SIZE -
                           DIV_ROUND_UP(NR_CPUS, STUBS_PER_PAGE) * 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..8094546b75 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,30 @@ 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 v5 06/12] x86/hypervisor: provide hypervisor_reserve_top_pages

[Wei Liu]

This function will return the number of pages that need to be reserved
in the machine address space.

E820 code will use that number to adjust the maximum PFN available to
Xen.

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

diff --git a/xen/arch/x86/guest/hypervisor.c b/xen/arch/x86/guest/hypervisor.c
index e72c92ffdf..8b9cf1ce4c 100644
--- a/xen/arch/x86/guest/hypervisor.c
+++ b/xen/arch/x86/guest/hypervisor.c
@@ -66,6 +66,14 @@ void hypervisor_resume(void)
         ops->resume();
 }
 
+unsigned int hypervisor_reserve_top_pages(void)
+{
+    if ( ops && ops->reserve_top_pages )
+        return ops->reserve_top_pages();
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/guest/hypervisor.h b/xen/include/asm-x86/guest/hypervisor.h
index b503854c5b..37eb9d531e 100644
--- a/xen/include/asm-x86/guest/hypervisor.h
+++ b/xen/include/asm-x86/guest/hypervisor.h
@@ -28,6 +28,8 @@ struct hypervisor_ops {
     int (*ap_setup)(void);
     /* Resume from suspension */
     void (*resume)(void);
+    /* How many top pages to be reserved in machine address space? */
+    unsigned int (*reserve_top_pages)(void);
 };
 
 #ifdef CONFIG_GUEST
@@ -36,6 +38,7 @@ const char *hypervisor_probe(void);
 void hypervisor_setup(void);
 int hypervisor_ap_setup(void);
 void hypervisor_resume(void);
+unsigned int hypervisor_reserve_top_pages(void);
 
 #else
 
@@ -46,6 +49,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 unsigned int hypervisor_reserve_top_pages(void) { return 0; }
 
 #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 v5 07/12] 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 code
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>
---
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/e820.c                     |  5 +++
 xen/arch/x86/guest/hyperv/hyperv.c      | 57 +++++++++++++++++++++++--
 xen/include/asm-x86/guest/hyperv-tlfs.h |  5 ++-
 xen/include/asm-x86/guest/hyperv.h      |  3 ++
 4 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 3892c9cfb7..99643f3ea0 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -343,6 +343,7 @@ static unsigned long __init find_max_pfn(void)
 {
     unsigned int i;
     unsigned long max_pfn = 0;
+    unsigned long top_pfn = ((1ull << paddr_bits) - 1) >> PAGE_SHIFT;
 
     for (i = 0; i < e820.nr_map; i++) {
         unsigned long start, end;
@@ -357,6 +358,10 @@ static unsigned long __init find_max_pfn(void)
             max_pfn = end;
     }
 
+    top_pfn -= hypervisor_reserve_top_pages();
+    if ( max_pfn >= top_pfn )
+        max_pfn = top_pfn;
+
     return max_pfn;
 }
 
diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 8d38313d7a..2bedcc438c 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -19,15 +19,26 @@
  * 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)
+{
+    uint64_t id;
+
+    id = (uint64_t)HV_XEN_VENDOR_ID << 48;
+    id |= (xen_major_version() << 16) | xen_minor_version();
+
+    return id;
+}
+
+static const struct hypervisor_ops ops;
 
 const struct hypervisor_ops *__init hyperv_probe(void)
 {
@@ -72,6 +83,46 @@ 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;
+
+    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 const struct hypervisor_ops ops = {
+    .name = "Hyper-V",
+    .setup = setup,
+};
+
 /*
  * 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..0dcd8082ad 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 v5 08/12] 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>
---
v5:
1. Switch back to direct call
2. Fix some issues pointed out by Jan

I tried using the asm(".equ ..") trick but hit a problem with %c again.

mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c'
               asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
---
 MAINTAINERS                              |  1 +
 xen/arch/x86/guest/hyperv/hyperv.c       |  6 ++
 xen/arch/x86/xen.lds.S                   |  4 +
 xen/include/asm-x86/fixmap.h             |  3 +-
 xen/include/asm-x86/guest/hyperv-hcall.h | 96 ++++++++++++++++++++++++
 5 files changed, 108 insertions(+), 2 deletions(-)
 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/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 2bedcc438c..932a648ff7 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -123,6 +123,12 @@ static const struct hypervisor_ops ops = {
     .setup = setup,
 };
 
+static void __maybe_unused build_assertions(void)
+{
+    /* We use 1 in linker script */
+    BUILD_BUG_ON(FIX_X_HYPERV_HCALL != 1);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 97f9c07891..8e02b4c648 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -329,6 +329,10 @@ SECTIONS
   efi = .;
 #endif
 
+#ifdef CONFIG_HYPERV_GUEST
+  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
+#endif
+
   /* Sections to be discarded */
   /DISCARD/ : {
        *(.exit.text)
diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h
index 8094546b75..a9bcb068cb 100644
--- a/xen/include/asm-x86/fixmap.h
+++ b/xen/include/asm-x86/fixmap.h
@@ -16,6 +16,7 @@
 
 #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
 #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
+#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT))
 
 #ifndef __ASSEMBLY__
 
@@ -110,8 +111,6 @@ extern void __set_fixmap_x(
 
 #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__ */
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..5b7509b3b5
--- /dev/null
+++ b/xen/include/asm-x86/guest/hyperv-hcall.h
@@ -0,0 +1,96 @@
+/******************************************************************************
+ * 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;
+
+    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;
+
+    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 v5 09/12] 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 932a648ff7..4387b6541e 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>
 
@@ -111,6 +112,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 v5 10/12] 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>
---
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
---
 xen/arch/x86/guest/hyperv/hyperv.c  | 31 +++++++++++++++++++++++++++++
 xen/arch/x86/guest/hyperv/private.h | 29 +++++++++++++++++++++++++++
 2 files changed, 60 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 4387b6541e..f0facccbad 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_pcpu_input_page);
 
 static uint64_t generate_guest_id(void)
 {
@@ -127,14 +130,42 @@ static void __init setup_hypercall_page(void)
     }
 }
 
+static int setup_hypercall_pcpu_arg(void)
+{
+    void *mapping;
+
+    if ( this_cpu(hv_pcpu_input_page) )
+        return 0;
+
+    mapping = alloc_xenheap_page();
+    if ( !mapping )
+    {
+        printk("Failed to allocate hypercall input page for CPU%u\n",
+               smp_processor_id());
+        return -ENOMEM;
+    }
+
+    this_cpu(hv_pcpu_input_page) = mapping;
+
+    return 0;
+}
+
 static void __init setup(void)
 {
     setup_hypercall_page();
+    if ( setup_hypercall_pcpu_arg() )
+        panic("Hypercall percpu arg setup failed\n");
+}
+
+static int ap_setup(void)
+{
+    return setup_hypercall_pcpu_arg();
 }
 
 static const struct hypervisor_ops ops = {
     .name = "Hyper-V",
     .setup = setup,
+    .ap_setup = ap_setup,
 };
 
 static void __maybe_unused build_assertions(void)
diff --git a/xen/arch/x86/guest/hyperv/private.h b/xen/arch/x86/guest/hyperv/private.h
new file mode 100644
index 0000000000..a339274985
--- /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_pcpu_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 v5 11/12] 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  | 5 +++++
 xen/arch/x86/guest/hyperv/private.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index f0facccbad..af0d6ed692 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_pcpu_input_page);
+DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index);
 
 static uint64_t generate_guest_id(void)
 {
@@ -133,6 +134,7 @@ static void __init setup_hypercall_page(void)
 static int setup_hypercall_pcpu_arg(void)
 {
     void *mapping;
+    uint64_t vp_index_msr;
 
     if ( this_cpu(hv_pcpu_input_page) )
         return 0;
@@ -147,6 +149,9 @@ static int setup_hypercall_pcpu_arg(void)
 
     this_cpu(hv_pcpu_input_page) = mapping;
 
+    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 a339274985..c1c2431eff 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_pcpu_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 v5 12/12] 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>
---
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  | 44 ++++++++++++++++++++++++++++-
 xen/arch/x86/guest/hyperv/private.h |  1 +
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index af0d6ed692..bc40a3d338 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_pcpu_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,16 +156,57 @@ static int setup_hypercall_pcpu_arg(void)
     return 0;
 }
 
+static int setup_vp_assist(void)
+{
+    void *mapping;
+    uint64_t val;
+
+    mapping = this_cpu(hv_vp_assist);
+
+    if ( !mapping )
+    {
+        mapping = alloc_xenheap_page();
+        if ( !mapping )
+        {
+            printk("Failed to allocate vp_assist page for CPU%u\n",
+                   smp_processor_id());
+            return -ENOMEM;
+        }
+
+        clear_page(mapping);
+        this_cpu(hv_vp_assist) = mapping;
+    }
+
+    val = (virt_to_mfn(mapping) << HV_HYP_PAGE_SHIFT)
+        | HV_X64_MSR_VP_ASSIST_PAGE_ENABLE;
+    wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, val);
+
+    return 0;
+}
+
 static void __init setup(void)
 {
     setup_hypercall_page();
+
     if ( setup_hypercall_pcpu_arg() )
         panic("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 )
+        goto out;
+
+    rc = setup_vp_assist();
+
+ out:
+    return rc;
 }
 
 static const struct hypervisor_ops ops = {
diff --git a/xen/arch/x86/guest/hyperv/private.h b/xen/arch/x86/guest/hyperv/private.h
index c1c2431eff..fcddc47544 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_pcpu_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 v5 05/12] x86: provide executable fixmap facility

[Wei Liu]

On Wed, Jan 29, 2020 at 08:20:27PM +0000, Wei Liu wrote:
>  
> +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);
> +
> +    /* Generate a symbol to be used in linker script */
> +    asm ( ".equ FIXADDR_X_SIZE, %c0; .global FIXADDR_X_SIZE"
> +          :: "i" (__end_of_fixed_addresses_x << PAGE_SHIFT) );

The (__end << SHIFT) part can be replaced with FIXADDR_X_SIZE (the macro
defined in fixmap.h) directly.

Wei.

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

Re: [Xen-devel] [PATCH v5 01/12] MAINTAINERS: put Hyper-V code under Viridian maintainership

[Durrant, Paul]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei
> Liu
> Sent: 29 January 2020 20:20
> To: Xen Development List <xen-devel@lists.xenproject.org>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <liuwe@microsoft.com>; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@eu.citrix.com>;
> Andrew Cooper <andrew.cooper3@citrix.com>; Durrant, Paul
> <pdurrant@amazon.co.uk>; Ian Jackson <ian.jackson@eu.citrix.com>; Michael
> Kelley <mikelley@microsoft.com>; Julien Grall <julien@xen.org>
> Subject: [Xen-devel] [PATCH v5 01/12] MAINTAINERS: put Hyper-V code under
> Viridian maintainership
> 
> And add myself as a maintainer.
> 
> Sort the list alphabetically while at it.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>
> Signed-off-by: Wei Liu <wl@xen.org>

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

> ---
>  MAINTAINERS | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1915e09f8b..04d91482cd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -514,10 +514,13 @@ F:	xen/arch/x86/mm/shadow/
> 
>  X86 VIRIDIAN ENLIGHTENMENTS
>  M:	Paul Durrant <pdurrant@amazon.com>
> +M:	Wei Liu <wl@xen.org>
>  S:	Supported
> +F:	xen/arch/x86/guest/hyperv/
>  F:	xen/arch/x86/hvm/viridian/
> -F:	xen/include/asm-x86/hvm/viridian.h
> +F:	xen/include/asm-x86/guest/hyperv.h
>  F:	xen/include/asm-x86/guest/hyperv-tlfs.h
> +F:	xen/include/asm-x86/hvm/viridian.h
> 
>  XENTRACE
>  M:	George Dunlap <george.dunlap@eu.citrix.com>
> --
> 2.20.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 07/12] x86/hyperv: setup hypercall page

[Durrant, Paul]

> -----Original Message-----
> From: Wei Liu <wei.liu.xen@gmail.com> On Behalf Of Wei Liu
> Sent: 29 January 2020 20:20
> 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>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu
> <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: [PATCH v5 07/12] x86/hyperv: setup hypercall page
> 
> 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 code
> 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>
> ---
> 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/e820.c                     |  5 +++
>  xen/arch/x86/guest/hyperv/hyperv.c      | 57 +++++++++++++++++++++++--
>  xen/include/asm-x86/guest/hyperv-tlfs.h |  5 ++-
>  xen/include/asm-x86/guest/hyperv.h      |  3 ++
>  4 files changed, 65 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> index 3892c9cfb7..99643f3ea0 100644
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -343,6 +343,7 @@ static unsigned long __init find_max_pfn(void)
>  {
>      unsigned int i;
>      unsigned long max_pfn = 0;
> +    unsigned long top_pfn = ((1ull << paddr_bits) - 1) >> PAGE_SHIFT;
> 
>      for (i = 0; i < e820.nr_map; i++) {
>          unsigned long start, end;
> @@ -357,6 +358,10 @@ static unsigned long __init find_max_pfn(void)
>              max_pfn = end;
>      }
> 
> +    top_pfn -= hypervisor_reserve_top_pages();
> +    if ( max_pfn >= top_pfn )
> +        max_pfn = top_pfn;
> +
>      return max_pfn;
>  }
> 
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c
> b/xen/arch/x86/guest/hyperv/hyperv.c
> index 8d38313d7a..2bedcc438c 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -19,15 +19,26 @@
>   * 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)
> +{
> +    uint64_t id;
> +
> +    id = (uint64_t)HV_XEN_VENDOR_ID << 48;
> +    id |= (xen_major_version() << 16) | xen_minor_version();
> +
> +    return id;

I think this should use the hv_guest_os_id union. You can then set the values using the bit-fields and return the raw.

  Paul

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

Re: [Xen-devel] [PATCH v5 02/12] x86/hypervisor: make hypervisor_ap_setup return an error code

[Roger Pau Monné]

On Wed, Jan 29, 2020 at 08:20:24PM +0000, Wei Liu wrote:
> We want to be able to handle AP setup error in the upper layer.

Thanks, some comments below.

> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>
> ---
>  xen/arch/x86/guest/hypervisor.c        |  6 ++++--
>  xen/arch/x86/guest/xen/xen.c           | 11 +++++++++--
>  xen/include/asm-x86/guest/hypervisor.h |  6 +++---
>  3 files changed, 16 insertions(+), 7 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..eed8a6edae 100644
> --- a/xen/arch/x86/guest/xen/xen.c
> +++ b/xen/arch/x86/guest/xen/xen.c
> @@ -257,11 +257,18 @@ static void __init setup(void)
>      init_evtchn();
>  }
>  
> -static void ap_setup(void)
> +static int ap_setup(void)
>  {
> +    int rc;
> +
>      set_vcpu_id();
> -    map_vcpuinfo();
> +    rc = map_vcpuinfo();

map_vcpuinfo should be changed so that the BUG_ON is removed, and an
error is only returned if VCPUOP_register_vcpu_info fails and vcpu >=
XEN_LEGACY_MAX_VCPUS, else no error should be returned.

> +    if ( rc )
> +        return rc;
> +
>      init_evtchn();
> +
> +    return 0;

In order to keep this shorter, you could do:

if ( !rc )
    init_evtchn();

return rc;

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Durrant, Paul]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei
> Liu
> Sent: 29 January 2020 20:21
> To: Xen Development List <xen-devel@lists.xenproject.org>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <liuwe@microsoft.com>; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@eu.citrix.com>;
> Andrew Cooper <andrew.cooper3@citrix.com>; Durrant, Paul
> <pdurrant@amazon.co.uk>; Ian Jackson <ian.jackson@eu.citrix.com>; Michael
> Kelley <mikelley@microsoft.com>; Julien Grall <julien@xen.org>; Roger Pau
> Monné <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V
> hypercall functions
> 
> 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>

> ---
> v5:
> 1. Switch back to direct call
> 2. Fix some issues pointed out by Jan
> 
> I tried using the asm(".equ ..") trick but hit a problem with %c again.
> 
> mm.c:5736:5: error: invalid 'asm': operand is not a condition code,
> invalid operand code 'c'
>                asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
> ---
>  MAINTAINERS                              |  1 +
>  xen/arch/x86/guest/hyperv/hyperv.c       |  6 ++
>  xen/arch/x86/xen.lds.S                   |  4 +
>  xen/include/asm-x86/fixmap.h             |  3 +-
>  xen/include/asm-x86/guest/hyperv-hcall.h | 96 ++++++++++++++++++++++++
>  5 files changed, 108 insertions(+), 2 deletions(-)
>  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/guest/hyperv/hyperv.c
> b/xen/arch/x86/guest/hyperv/hyperv.c
> index 2bedcc438c..932a648ff7 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -123,6 +123,12 @@ static const struct hypervisor_ops ops = {
>      .setup = setup,
>  };
> 
> +static void __maybe_unused build_assertions(void)
> +{
> +    /* We use 1 in linker script */
> +    BUILD_BUG_ON(FIX_X_HYPERV_HCALL != 1);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 97f9c07891..8e02b4c648 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -329,6 +329,10 @@ SECTIONS
>    efi = .;
>  #endif
> 
> +#ifdef CONFIG_HYPERV_GUEST
> +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> +#endif
> +
>    /* Sections to be discarded */
>    /DISCARD/ : {
>         *(.exit.text)
> diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h
> index 8094546b75..a9bcb068cb 100644
> --- a/xen/include/asm-x86/fixmap.h
> +++ b/xen/include/asm-x86/fixmap.h
> @@ -16,6 +16,7 @@
> 
>  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
>  #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
> +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT))
> 
>  #ifndef __ASSEMBLY__
> 
> @@ -110,8 +111,6 @@ extern void __set_fixmap_x(
> 
>  #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__ */
> 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..5b7509b3b5
> --- /dev/null
> +++ b/xen/include/asm-x86/guest/hyperv-hcall.h
> @@ -0,0 +1,96 @@
> +/************************************************************************
> ******
> + * 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;
> +
> +    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;
> +
> +    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 mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 03/12] x86/smp: don't online cpu if hypervisor_ap_setup fails

[Roger Pau Monné]

On Wed, Jan 29, 2020 at 08:20:25PM +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>

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 10/12] x86/hyperv: provide percpu hypercall input page

[Durrant, Paul]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei
> Liu
> Sent: 29 January 2020 20:21
> To: Xen Development List <xen-devel@lists.xenproject.org>
> Cc: Wei Liu <liuwe@microsoft.com>; Wei Liu <wl@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Durrant, Paul <pdurrant@amazon.co.uk>;
> Michael Kelley <mikelley@microsoft.com>; Roger Pau Monné
> <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH v5 10/12] x86/hyperv: provide percpu hypercall
> input page
> 
> 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>

> ---
> 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
> ---
>  xen/arch/x86/guest/hyperv/hyperv.c  | 31 +++++++++++++++++++++++++++++
>  xen/arch/x86/guest/hyperv/private.h | 29 +++++++++++++++++++++++++++
>  2 files changed, 60 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 4387b6541e..f0facccbad 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_pcpu_input_page);
> 
>  static uint64_t generate_guest_id(void)
>  {
> @@ -127,14 +130,42 @@ static void __init setup_hypercall_page(void)
>      }
>  }
> 
> +static int setup_hypercall_pcpu_arg(void)
> +{
> +    void *mapping;
> +
> +    if ( this_cpu(hv_pcpu_input_page) )
> +        return 0;
> +
> +    mapping = alloc_xenheap_page();
> +    if ( !mapping )
> +    {
> +        printk("Failed to allocate hypercall input page for CPU%u\n",
> +               smp_processor_id());
> +        return -ENOMEM;
> +    }
> +
> +    this_cpu(hv_pcpu_input_page) = mapping;
> +
> +    return 0;
> +}
> +
>  static void __init setup(void)
>  {
>      setup_hypercall_page();
> +    if ( setup_hypercall_pcpu_arg() )
> +        panic("Hypercall percpu arg setup failed\n");
> +}
> +
> +static int ap_setup(void)
> +{
> +    return setup_hypercall_pcpu_arg();
>  }
> 
>  static const struct hypervisor_ops ops = {
>      .name = "Hyper-V",
>      .setup = setup,
> +    .ap_setup = ap_setup,
>  };
> 
>  static void __maybe_unused build_assertions(void)
> diff --git a/xen/arch/x86/guest/hyperv/private.h
> b/xen/arch/x86/guest/hyperv/private.h
> new file mode 100644
> index 0000000000..a339274985
> --- /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_pcpu_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 mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 04/12] x86: make paddr_bits available earlier

[Roger Pau Monné]

On Wed, Jan 29, 2020 at 08:20:26PM +0000, Wei Liu wrote:
> Move early_cpu_init before init_e820, such that paddr_bits can be used
> by e820 code.
> 
> This will reduce code repetition and prepare for further adjustment when
> L0 hypervisor comes into play.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>

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

One typo below.

> ---
>  xen/arch/x86/e820.c  | 14 ++++----------
>  xen/arch/x86/setup.c |  5 +++--
>  2 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> index 082f9928a1..3892c9cfb7 100644
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -420,7 +420,7 @@ static uint64_t __init mtrr_top_of_ram(void)
>  {
>      uint32_t eax, ebx, ecx, edx;
>      uint64_t mtrr_cap, mtrr_def, addr_mask, base, mask, top;
> -    unsigned int i, phys_bits = 36;
> +    unsigned int i;
>  
>      /* By default we check only Intel systems. */
>      if ( e820_mtrr_clip == -1 )
> @@ -445,15 +445,9 @@ static uint64_t __init mtrr_top_of_ram(void)
>      if ( !test_bit(X86_FEATURE_MTRR & 31, &edx) )
>           return 0;
>  
> -    /* Find the physical address size for this CPU. */
> -    eax = cpuid_eax(0x80000000);
> -    if ( (eax >> 16) == 0x8000 && eax >= 0x80000008 )
> -    {
> -        phys_bits = (uint8_t)cpuid_eax(0x80000008);
> -        if ( phys_bits > PADDR_BITS )
> -            phys_bits = PADDR_BITS;
> -    }
> -    addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
> +    /* paddr_bits must have been set at this point */
> +    ASSERT(paddr_bits);
> +    addr_mask = ((1ull << paddr_bits) - 1) & PAGE_MASK;
>  
>      rdmsrl(MSR_MTRRcap, mtrr_cap);
>      rdmsrl(MSR_MTRRdefType, mtrr_def);
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index d858883404..89fe49149f 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -954,6 +954,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      else
>          panic("Bootloader provided no memory information\n");
>  
> +    /* This must come before e820 code becuause it sets paddr_bits. */
                                          ^ because

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 06/12] x86/hypervisor: provide hypervisor_reserve_top_pages

[Roger Pau Monné]

On Wed, Jan 29, 2020 at 08:20:28PM +0000, Wei Liu wrote:
> This function will return the number of pages that need to be reserved
> in the machine address space.
> 
> E820 code will use that number to adjust the maximum PFN available to
> Xen.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>

It's hard to figure out whether the proposed code is suitable since
it's not yet called by any other function, and there's no hypervisor
implementation.

I wouldn't mind if this was merged into the next patch so that there's
a implementation and a user of the introduced code.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 12/12] x86/hyperv: setup VP assist page

[Durrant, Paul]

> -----Original Message-----
> From: Wei Liu <wei.liu.xen@gmail.com> On Behalf Of Wei Liu
> Sent: 29 January 2020 20:21
> 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 v5 12/12] 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>
> ---
> 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  | 44 ++++++++++++++++++++++++++++-
>  xen/arch/x86/guest/hyperv/private.h |  1 +
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c
> b/xen/arch/x86/guest/hyperv/hyperv.c
> index af0d6ed692..bc40a3d338 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_pcpu_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,16 +156,57 @@ static int setup_hypercall_pcpu_arg(void)
>      return 0;
>  }
> 
> +static int setup_vp_assist(void)
> +{
> +    void *mapping;
> +    uint64_t val;
> +
> +    mapping = this_cpu(hv_vp_assist);
> +
> +    if ( !mapping )
> +    {
> +        mapping = alloc_xenheap_page();
> +        if ( !mapping )
> +        {
> +            printk("Failed to allocate vp_assist page for CPU%u\n",
> +                   smp_processor_id());
> +            return -ENOMEM;
> +        }
> +
> +        clear_page(mapping);
> +        this_cpu(hv_vp_assist) = mapping;
> +    }
> +
> +    val = (virt_to_mfn(mapping) << HV_HYP_PAGE_SHIFT)
> +        | HV_X64_MSR_VP_ASSIST_PAGE_ENABLE;

Perhaps it would be neater to put the viridian_page_msr union into hyperv-tlfs.h and then use that.

  Paul

> +    wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, val);
> +
> +    return 0;
> +}
> +
>  static void __init setup(void)
>  {
>      setup_hypercall_page();
> +
>      if ( setup_hypercall_pcpu_arg() )
>          panic("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 )
> +        goto out;
> +
> +    rc = setup_vp_assist();
> +
> + out:
> +    return rc;
>  }
> 
>  static const struct hypervisor_ops ops = {
> diff --git a/xen/arch/x86/guest/hyperv/private.h
> b/xen/arch/x86/guest/hyperv/private.h
> index c1c2431eff..fcddc47544 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_pcpu_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 v5 07/12] x86/hyperv: setup hypercall page

[Roger Pau Monné]

On Wed, Jan 29, 2020 at 08:20:29PM +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 code
> 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>
> ---
> 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/e820.c                     |  5 +++
>  xen/arch/x86/guest/hyperv/hyperv.c      | 57 +++++++++++++++++++++++--
>  xen/include/asm-x86/guest/hyperv-tlfs.h |  5 ++-
>  xen/include/asm-x86/guest/hyperv.h      |  3 ++
>  4 files changed, 65 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> index 3892c9cfb7..99643f3ea0 100644
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -343,6 +343,7 @@ static unsigned long __init find_max_pfn(void)
>  {
>      unsigned int i;
>      unsigned long max_pfn = 0;
> +    unsigned long top_pfn = ((1ull << paddr_bits) - 1) >> PAGE_SHIFT;
>  
>      for (i = 0; i < e820.nr_map; i++) {
>          unsigned long start, end;
> @@ -357,6 +358,10 @@ static unsigned long __init find_max_pfn(void)
>              max_pfn = end;
>      }
>  
> +    top_pfn -= hypervisor_reserve_top_pages();
> +    if ( max_pfn >= top_pfn )
> +        max_pfn = top_pfn;

Hm, I'm not sure I see the point of this. The value returned by
find_max_pfn is the maximum RAM address found in the memory map, but
the physical address you are using to map the hypercall page is almost
certainly much higher than the maximum address found in the physmap
(and certainly not RAM), and hence I'm not sure what's the point of
this.

Also you haven't introduced a HyperV implementation of
hypervisor_reserve_top_pages so far, so it's hard to tell the intend
of this.

> +
>      return max_pfn;
>  }
>  
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> index 8d38313d7a..2bedcc438c 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -19,15 +19,26 @@
>   * 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)
> +{
> +    uint64_t id;
> +
> +    id = (uint64_t)HV_XEN_VENDOR_ID << 48;
> +    id |= (xen_major_version() << 16) | xen_minor_version();
> +
> +    return id;
> +}
> +
> +static const struct hypervisor_ops ops;
>  
>  const struct hypervisor_ops *__init hyperv_probe(void)
>  {
> @@ -72,6 +83,46 @@ 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;
> +
> +    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 const struct hypervisor_ops ops = {
> +    .name = "Hyper-V",
> +    .setup = setup,
> +};
> +
>  /*
>   * 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..0dcd8082ad 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)

I think you should use PAGE_SHIFT here, or else you also need to make
sure the fixmap reserved entry is of size ((1 << HV_HYP_PAGE_SHIFT) -
1), and the call to set_fixmap_x in setup_hypercall_page needs to take
this into account AFAICT and not use PAGE_SHIFT.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 07/12] x86/hyperv: setup hypercall page

[Wei Liu]

On Thu, Jan 30, 2020 at 11:41:43AM +0100, Roger Pau Monné wrote:
> On Wed, Jan 29, 2020 at 08:20:29PM +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 code
> > 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>
> > ---
> > 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/e820.c                     |  5 +++
> >  xen/arch/x86/guest/hyperv/hyperv.c      | 57 +++++++++++++++++++++++--
> >  xen/include/asm-x86/guest/hyperv-tlfs.h |  5 ++-
> >  xen/include/asm-x86/guest/hyperv.h      |  3 ++
> >  4 files changed, 65 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> > index 3892c9cfb7..99643f3ea0 100644
> > --- a/xen/arch/x86/e820.c
> > +++ b/xen/arch/x86/e820.c
> > @@ -343,6 +343,7 @@ static unsigned long __init find_max_pfn(void)
> >  {
> >      unsigned int i;
> >      unsigned long max_pfn = 0;
> > +    unsigned long top_pfn = ((1ull << paddr_bits) - 1) >> PAGE_SHIFT;
> >  
> >      for (i = 0; i < e820.nr_map; i++) {
> >          unsigned long start, end;
> > @@ -357,6 +358,10 @@ static unsigned long __init find_max_pfn(void)
> >              max_pfn = end;
> >      }
> >  
> > +    top_pfn -= hypervisor_reserve_top_pages();
> > +    if ( max_pfn >= top_pfn )
> > +        max_pfn = top_pfn;
> 
> Hm, I'm not sure I see the point of this. The value returned by
> find_max_pfn is the maximum RAM address found in the memory map, but
> the physical address you are using to map the hypercall page is almost
> certainly much higher than the maximum address found in the physmap
> (and certainly not RAM), and hence I'm not sure what's the point of
> this.

Yes, the keyword is "almost certainly". :-)

This is done for correctness's sake. I don't expect in practice there
would be a configuration that has that much memory, but correctness is
still important.

It also guards against weird configuration in which memory is put into
that part of the physical address space for whatever reason. I don't
know why anyone would do that, but again, we should be prepared for
that.


> 
> Also you haven't introduced a HyperV implementation of
> hypervisor_reserve_top_pages so far, so it's hard to tell the intend
> of this.

D'oh. That was supposed to be in this patch. I guess I forgot to commit
that hunk!

That function for Hyper-V is going to return 1 (page).

> 
> > +
> >      return max_pfn;
> >  }
> >  
[...]
> > diff --git a/xen/include/asm-x86/guest/hyperv.h b/xen/include/asm-x86/guest/hyperv.h
> > index c7a7f32bd5..0dcd8082ad 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)
> 
> I think you should use PAGE_SHIFT here, or else you also need to make
> sure the fixmap reserved entry is of size ((1 << HV_HYP_PAGE_SHIFT) -
> 1), and the call to set_fixmap_x in setup_hypercall_page needs to take
> this into account AFAICT and not use PAGE_SHIFT.

PAGE_SHIFT and HV_HYP_PAGE_SHIFT are in fact the same.

I can add a BUILD_BUG_ON somewhere.

Wei.

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 07/12] x86/hyperv: setup hypercall page

[Wei Liu]

On Thu, Jan 30, 2020 at 09:57:17AM +0000, Durrant, Paul wrote:
> > -static const struct hypervisor_ops ops = {
> > -    .name = "Hyper-V",
> > -};
> > +static uint64_t generate_guest_id(void)
> > +{
> > +    uint64_t id;
> > +
> > +    id = (uint64_t)HV_XEN_VENDOR_ID << 48;
> > +    id |= (xen_major_version() << 16) | xen_minor_version();
> > +
> > +    return id;
> 
> I think this should use the hv_guest_os_id union. You can then set the
> values using the bit-fields and return the raw.

NP.

Wei.

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

Re: [Xen-devel] [PATCH v5 07/12] x86/hyperv: setup hypercall page

[Roger Pau Monné]

On Thu, Jan 30, 2020 at 11:18:21AM +0000, Wei Liu wrote:
> On Thu, Jan 30, 2020 at 11:41:43AM +0100, Roger Pau Monné wrote:
> > On Wed, Jan 29, 2020 at 08:20:29PM +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 code
> > > 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>
> > > ---
> > > 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/e820.c                     |  5 +++
> > >  xen/arch/x86/guest/hyperv/hyperv.c      | 57 +++++++++++++++++++++++--
> > >  xen/include/asm-x86/guest/hyperv-tlfs.h |  5 ++-
> > >  xen/include/asm-x86/guest/hyperv.h      |  3 ++
> > >  4 files changed, 65 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> > > index 3892c9cfb7..99643f3ea0 100644
> > > --- a/xen/arch/x86/e820.c
> > > +++ b/xen/arch/x86/e820.c
> > > @@ -343,6 +343,7 @@ static unsigned long __init find_max_pfn(void)
> > >  {
> > >      unsigned int i;
> > >      unsigned long max_pfn = 0;
> > > +    unsigned long top_pfn = ((1ull << paddr_bits) - 1) >> PAGE_SHIFT;
> > >  
> > >      for (i = 0; i < e820.nr_map; i++) {
> > >          unsigned long start, end;
> > > @@ -357,6 +358,10 @@ static unsigned long __init find_max_pfn(void)
> > >              max_pfn = end;
> > >      }
> > >  
> > > +    top_pfn -= hypervisor_reserve_top_pages();
> > > +    if ( max_pfn >= top_pfn )
> > > +        max_pfn = top_pfn;
> > 
> > Hm, I'm not sure I see the point of this. The value returned by
> > find_max_pfn is the maximum RAM address found in the memory map, but
> > the physical address you are using to map the hypercall page is almost
> > certainly much higher than the maximum address found in the physmap
> > (and certainly not RAM), and hence I'm not sure what's the point of
> > this.
> 
> Yes, the keyword is "almost certainly". :-)
> 
> This is done for correctness's sake. I don't expect in practice there
> would be a configuration that has that much memory, but correctness is
> still important.
> 
> It also guards against weird configuration in which memory is put into
> that part of the physical address space for whatever reason. I don't
> know why anyone would do that, but again, we should be prepared for
> that.
> 
> 
> > 
> > Also you haven't introduced a HyperV implementation of
> > hypervisor_reserve_top_pages so far, so it's hard to tell the intend
> > of this.
> 
> D'oh. That was supposed to be in this patch. I guess I forgot to commit
> that hunk!
> 
> That function for Hyper-V is going to return 1 (page).

But that would likely be wrong, unless the memory map has a RAM
region that expands up to (1 << paddr_bits)?

Or else you are just removing a page from the last RAM region in
the memory map for no reason. max_pfn is almost certainly way below (1
<< paddr_bits).

I think what you need is a hook that modifies the memory map and adds
a reserved region at ((1 << paddr_bits) - PAGE_SIZE) of size
PAGE_SIZE. See where pv_shim_fixup_e820 is used, and I think you want
to make this a hypervisor hook and add the HyperV code to reserve the
hypercall page in the e820 there.

> > 
> > > +
> > >      return max_pfn;
> > >  }
> > >  
> [...]
> > > diff --git a/xen/include/asm-x86/guest/hyperv.h b/xen/include/asm-x86/guest/hyperv.h
> > > index c7a7f32bd5..0dcd8082ad 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)
> > 
> > I think you should use PAGE_SHIFT here, or else you also need to make
> > sure the fixmap reserved entry is of size ((1 << HV_HYP_PAGE_SHIFT) -
> > 1), and the call to set_fixmap_x in setup_hypercall_page needs to take
> > this into account AFAICT and not use PAGE_SHIFT.
> 
> PAGE_SHIFT and HV_HYP_PAGE_SHIFT are in fact the same.
> 
> I can add a BUILD_BUG_ON somewhere.

Yes please.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 07/12] x86/hyperv: setup hypercall page

[Wei Liu]

On Thu, Jan 30, 2020 at 12:39:47PM +0100, Roger Pau Monné wrote:
> On Thu, Jan 30, 2020 at 11:18:21AM +0000, Wei Liu wrote:
> > On Thu, Jan 30, 2020 at 11:41:43AM +0100, Roger Pau Monné wrote:
> > > On Wed, Jan 29, 2020 at 08:20:29PM +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 code
> > > > 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>
> > > > ---
> > > > 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/e820.c                     |  5 +++
> > > >  xen/arch/x86/guest/hyperv/hyperv.c      | 57 +++++++++++++++++++++++--
> > > >  xen/include/asm-x86/guest/hyperv-tlfs.h |  5 ++-
> > > >  xen/include/asm-x86/guest/hyperv.h      |  3 ++
> > > >  4 files changed, 65 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> > > > index 3892c9cfb7..99643f3ea0 100644
> > > > --- a/xen/arch/x86/e820.c
> > > > +++ b/xen/arch/x86/e820.c
> > > > @@ -343,6 +343,7 @@ static unsigned long __init find_max_pfn(void)
> > > >  {
> > > >      unsigned int i;
> > > >      unsigned long max_pfn = 0;
> > > > +    unsigned long top_pfn = ((1ull << paddr_bits) - 1) >> PAGE_SHIFT;
> > > >  
> > > >      for (i = 0; i < e820.nr_map; i++) {
> > > >          unsigned long start, end;
> > > > @@ -357,6 +358,10 @@ static unsigned long __init find_max_pfn(void)
> > > >              max_pfn = end;
> > > >      }
> > > >  
> > > > +    top_pfn -= hypervisor_reserve_top_pages();
> > > > +    if ( max_pfn >= top_pfn )
> > > > +        max_pfn = top_pfn;
> > > 
> > > Hm, I'm not sure I see the point of this. The value returned by
> > > find_max_pfn is the maximum RAM address found in the memory map, but
> > > the physical address you are using to map the hypercall page is almost
> > > certainly much higher than the maximum address found in the physmap
> > > (and certainly not RAM), and hence I'm not sure what's the point of
> > > this.
> > 
> > Yes, the keyword is "almost certainly". :-)
> > 
> > This is done for correctness's sake. I don't expect in practice there
> > would be a configuration that has that much memory, but correctness is
> > still important.
> > 
> > It also guards against weird configuration in which memory is put into
> > that part of the physical address space for whatever reason. I don't
> > know why anyone would do that, but again, we should be prepared for
> > that.
> > 
> > 
> > > 
> > > Also you haven't introduced a HyperV implementation of
> > > hypervisor_reserve_top_pages so far, so it's hard to tell the intend
> > > of this.
> > 
> > D'oh. That was supposed to be in this patch. I guess I forgot to commit
> > that hunk!
> > 
> > That function for Hyper-V is going to return 1 (page).
> 
> But that would likely be wrong, unless the memory map has a RAM
> region that expands up to (1 << paddr_bits)?
> 
> Or else you are just removing a page from the last RAM region in
> the memory map for no reason. max_pfn is almost certainly way below (1
> << paddr_bits).
> 

Why? The adjustment will not be applied unless RAM overlaps with that
reserved region.

> I think what you need is a hook that modifies the memory map and adds
> a reserved region at ((1 << paddr_bits) - PAGE_SIZE) of size
> PAGE_SIZE. See where pv_shim_fixup_e820 is used, and I think you want
> to make this a hypervisor hook and add the HyperV code to reserve the
> hypercall page in the e820 there.

That works for me too. Let's see what other people think.

Wei.

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

Re: [Xen-devel] [PATCH v5 04/12] x86: make paddr_bits available earlier

[Wei Liu]

On Thu, Jan 30, 2020 at 11:17:33AM +0100, Roger Pau Monné wrote:
> >  
> > +    /* This must come before e820 code becuause it sets paddr_bits. */
>                                           ^ because

Fixed. 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 v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Roger Pau Monné]

On Wed, Jan 29, 2020 at 08:20:30PM +0000, Wei Liu wrote:
> These functions will be used later to make hypercalls to Hyper-V.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>
> ---
> v5:
> 1. Switch back to direct call
> 2. Fix some issues pointed out by Jan
> 
> I tried using the asm(".equ ..") trick but hit a problem with %c again.
> 
> mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c'
>                asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
> ---
>  MAINTAINERS                              |  1 +
>  xen/arch/x86/guest/hyperv/hyperv.c       |  6 ++
>  xen/arch/x86/xen.lds.S                   |  4 +
>  xen/include/asm-x86/fixmap.h             |  3 +-
>  xen/include/asm-x86/guest/hyperv-hcall.h | 96 ++++++++++++++++++++++++
>  5 files changed, 108 insertions(+), 2 deletions(-)
>  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/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> index 2bedcc438c..932a648ff7 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -123,6 +123,12 @@ static const struct hypervisor_ops ops = {
>      .setup = setup,
>  };
>  
> +static void __maybe_unused build_assertions(void)
> +{
> +    /* We use 1 in linker script */
> +    BUILD_BUG_ON(FIX_X_HYPERV_HCALL != 1);

I wouldn't mind if this was placed together with the hypercall page
setup instead of creating a dummy function for it.

> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 97f9c07891..8e02b4c648 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -329,6 +329,10 @@ SECTIONS
>    efi = .;
>  #endif
>  
> +#ifdef CONFIG_HYPERV_GUEST
> +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));

I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
enum?

> +#endif
> +
>    /* Sections to be discarded */
>    /DISCARD/ : {
>         *(.exit.text)
> diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h
> index 8094546b75..a9bcb068cb 100644
> --- a/xen/include/asm-x86/fixmap.h
> +++ b/xen/include/asm-x86/fixmap.h
> @@ -16,6 +16,7 @@
>  
>  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
>  #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
> +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT))
>  
>  #ifndef __ASSEMBLY__
>  
> @@ -110,8 +111,6 @@ extern void __set_fixmap_x(
>  
>  #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))

This seems like some unrelated code movement?

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 07/12] x86/hyperv: setup hypercall page

[Roger Pau Monné]

On Thu, Jan 30, 2020 at 11:47:52AM +0000, Wei Liu wrote:
> On Thu, Jan 30, 2020 at 12:39:47PM +0100, Roger Pau Monné wrote:
> > On Thu, Jan 30, 2020 at 11:18:21AM +0000, Wei Liu wrote:
> > > On Thu, Jan 30, 2020 at 11:41:43AM +0100, Roger Pau Monné wrote:
> > > > On Wed, Jan 29, 2020 at 08:20:29PM +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 code
> > > > > 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>
> > > > > ---
> > > > > 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/e820.c                     |  5 +++
> > > > >  xen/arch/x86/guest/hyperv/hyperv.c      | 57 +++++++++++++++++++++++--
> > > > >  xen/include/asm-x86/guest/hyperv-tlfs.h |  5 ++-
> > > > >  xen/include/asm-x86/guest/hyperv.h      |  3 ++
> > > > >  4 files changed, 65 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> > > > > index 3892c9cfb7..99643f3ea0 100644
> > > > > --- a/xen/arch/x86/e820.c
> > > > > +++ b/xen/arch/x86/e820.c
> > > > > @@ -343,6 +343,7 @@ static unsigned long __init find_max_pfn(void)
> > > > >  {
> > > > >      unsigned int i;
> > > > >      unsigned long max_pfn = 0;
> > > > > +    unsigned long top_pfn = ((1ull << paddr_bits) - 1) >> PAGE_SHIFT;
> > > > >  
> > > > >      for (i = 0; i < e820.nr_map; i++) {
> > > > >          unsigned long start, end;
> > > > > @@ -357,6 +358,10 @@ static unsigned long __init find_max_pfn(void)
> > > > >              max_pfn = end;
> > > > >      }
> > > > >  
> > > > > +    top_pfn -= hypervisor_reserve_top_pages();
> > > > > +    if ( max_pfn >= top_pfn )
> > > > > +        max_pfn = top_pfn;
> > > > 
> > > > Hm, I'm not sure I see the point of this. The value returned by
> > > > find_max_pfn is the maximum RAM address found in the memory map, but
> > > > the physical address you are using to map the hypercall page is almost
> > > > certainly much higher than the maximum address found in the physmap
> > > > (and certainly not RAM), and hence I'm not sure what's the point of
> > > > this.
> > > 
> > > Yes, the keyword is "almost certainly". :-)
> > > 
> > > This is done for correctness's sake. I don't expect in practice there
> > > would be a configuration that has that much memory, but correctness is
> > > still important.
> > > 
> > > It also guards against weird configuration in which memory is put into
> > > that part of the physical address space for whatever reason. I don't
> > > know why anyone would do that, but again, we should be prepared for
> > > that.
> > > 
> > > 
> > > > 
> > > > Also you haven't introduced a HyperV implementation of
> > > > hypervisor_reserve_top_pages so far, so it's hard to tell the intend
> > > > of this.
> > > 
> > > D'oh. That was supposed to be in this patch. I guess I forgot to commit
> > > that hunk!
> > > 
> > > That function for Hyper-V is going to return 1 (page).
> > 
> > But that would likely be wrong, unless the memory map has a RAM
> > region that expands up to (1 << paddr_bits)?
> > 
> > Or else you are just removing a page from the last RAM region in
> > the memory map for no reason. max_pfn is almost certainly way below (1
> > << paddr_bits).
> > 
> 
> Why? The adjustment will not be applied unless RAM overlaps with that
> reserved region.

Oh, OK, from your previous reply I understood that
hypervisor_reserve_top_pages would unconditionally return 1 for
HyperV, so that would end up always subtracting 1 page from the last
RAM region, even when not overlapping with (1 << paddr_bits).

> 
> > I think what you need is a hook that modifies the memory map and adds
> > a reserved region at ((1 << paddr_bits) - PAGE_SIZE) of size
> > PAGE_SIZE. See where pv_shim_fixup_e820 is used, and I think you want
> > to make this a hypervisor hook and add the HyperV code to reserve the
> > hypercall page in the e820 there.
> 
> That works for me too. Let's see what other people think.

I think that's the safest way, as you can assure there's nothing in
the region to be used by the hypercall page, and you can actually mark
it as reserved in the e820.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 10/12] x86/hyperv: provide percpu hypercall input page

[Roger Pau Monné]

On Wed, Jan 29, 2020 at 08:20:32PM +0000, Wei Liu wrote:
> 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: Roger Pau Monné <roger.pau@citrix.com>

Just some nits below.

> ---
> 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
> ---
>  xen/arch/x86/guest/hyperv/hyperv.c  | 31 +++++++++++++++++++++++++++++
>  xen/arch/x86/guest/hyperv/private.h | 29 +++++++++++++++++++++++++++
>  2 files changed, 60 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 4387b6541e..f0facccbad 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_pcpu_input_page);

I would drop the 'pcpu_' from the name, as you already know it's
per-cpu because of the accessors you have to use.

>  
>  static uint64_t generate_guest_id(void)
>  {
> @@ -127,14 +130,42 @@ static void __init setup_hypercall_page(void)
>      }
>  }
>  
> +static int setup_hypercall_pcpu_arg(void)
> +{
> +    void *mapping;

There's no need for the local variable, you can just assign to
this_cpu(hv_pcpu_input_page) directly, as a failure will just set it
to NULL (as it already was).

> +
> +    if ( this_cpu(hv_pcpu_input_page) )
> +        return 0;
> +
> +    mapping = alloc_xenheap_page();
> +    if ( !mapping )
> +    {
> +        printk("Failed to allocate hypercall input page for CPU%u\n",
> +               smp_processor_id());

I find it clearer to have the CPU%u prefix at the begging of the line,
but it's up to you.

> +        return -ENOMEM;
> +    }
> +
> +    this_cpu(hv_pcpu_input_page) = mapping;
> +
> +    return 0;
> +}
> +
>  static void __init setup(void)
>  {
>      setup_hypercall_page();
> +    if ( setup_hypercall_pcpu_arg() )
> +        panic("Hypercall percpu arg setup failed\n");

Could you add "HyperV hypercall...", just hypercall page is too
generic IMO.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
> 
> > +}
> > +
> >  /*
> >   * Local variables:
> >   * mode: C
> > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > index 97f9c07891..8e02b4c648 100644
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -329,6 +329,10 @@ SECTIONS
> >    efi = .;
> >  #endif
> >  
> > +#ifdef CONFIG_HYPERV_GUEST
> > +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> 
> I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
> enum?
> 

Yes.

And the trick to generate a symbol didn't work either.

> > +#endif
> > +
> >    /* Sections to be discarded */
> >    /DISCARD/ : {
> >         *(.exit.text)
> > diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h
> > index 8094546b75..a9bcb068cb 100644
> > --- a/xen/include/asm-x86/fixmap.h
> > +++ b/xen/include/asm-x86/fixmap.h
> > @@ -16,6 +16,7 @@
> >  
> >  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
> >  #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
> > +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT))
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > @@ -110,8 +111,6 @@ extern void __set_fixmap_x(
> >  
> >  #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))
> 
> This seems like some unrelated code movement?
> 

It is required. This section is not supposed to be used in linker
script. I have to move that macro ahead.

> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Roger Pau Monné]

On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote:
> On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
> > 
> > > +}
> > > +
> > >  /*
> > >   * Local variables:
> > >   * mode: C
> > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > > index 97f9c07891..8e02b4c648 100644
> > > --- a/xen/arch/x86/xen.lds.S
> > > +++ b/xen/arch/x86/xen.lds.S
> > > @@ -329,6 +329,10 @@ SECTIONS
> > >    efi = .;
> > >  #endif
> > >  
> > > +#ifdef CONFIG_HYPERV_GUEST
> > > +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> > 
> > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
> > enum?
> > 
> 
> Yes.
> 
> And the trick to generate a symbol didn't work either.

And you must define that symbol in the linker script? It doesn't seem
to be used at link time.

> > > +#endif
> > > +
> > >    /* Sections to be discarded */
> > >    /DISCARD/ : {
> > >         *(.exit.text)
> > > diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h
> > > index 8094546b75..a9bcb068cb 100644
> > > --- a/xen/include/asm-x86/fixmap.h
> > > +++ b/xen/include/asm-x86/fixmap.h
> > > @@ -16,6 +16,7 @@
> > >  
> > >  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
> > >  #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
> > > +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT))
> > >  
> > >  #ifndef __ASSEMBLY__
> > >  
> > > @@ -110,8 +111,6 @@ extern void __set_fixmap_x(
> > >  
> > >  #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))
> > 
> > This seems like some unrelated code movement?
> > 
> 
> It is required. This section is not supposed to be used in linker
> script. I have to move that macro ahead.

Oh, but you introduce that macro in patch #5, can you place it at the
right position when introduced?

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote:
> On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote:
> > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
> > > 
> > > > +}
> > > > +
> > > >  /*
> > > >   * Local variables:
> > > >   * mode: C
> > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > > > index 97f9c07891..8e02b4c648 100644
> > > > --- a/xen/arch/x86/xen.lds.S
> > > > +++ b/xen/arch/x86/xen.lds.S
> > > > @@ -329,6 +329,10 @@ SECTIONS
> > > >    efi = .;
> > > >  #endif
> > > >  
> > > > +#ifdef CONFIG_HYPERV_GUEST
> > > > +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> > > 
> > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
> > > enum?
> > > 
> > 
> > Yes.
> > 
> > And the trick to generate a symbol didn't work either.
> 
> And you must define that symbol in the linker script? It doesn't seem
> to be used at link time.
> 

I don't follow. I wish I could define and use a symbol in the linker
script but couldn't.

As for defining a symbol, see the patch that introduces the executable
fixmap facility, in function __set_fixmap_x.

> > > > +#endif
> > > > +
> > > >    /* Sections to be discarded */
> > > >    /DISCARD/ : {
> > > >         *(.exit.text)
> > > > diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h
> > > > index 8094546b75..a9bcb068cb 100644
> > > > --- a/xen/include/asm-x86/fixmap.h
> > > > +++ b/xen/include/asm-x86/fixmap.h
> > > > @@ -16,6 +16,7 @@
> > > >  
> > > >  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
> > > >  #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
> > > > +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT))
> > > >  
> > > >  #ifndef __ASSEMBLY__
> > > >  
> > > > @@ -110,8 +111,6 @@ extern void __set_fixmap_x(
> > > >  
> > > >  #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))
> > > 
> > > This seems like some unrelated code movement?
> > > 
> > 
> > It is required. This section is not supposed to be used in linker
> > script. I have to move that macro ahead.
> 
> Oh, but you introduce that macro in patch #5, can you place it at the
> right position when introduced?

It wasn't needed in the linker script until now. I don't mind doing it
that way, but sometimes I'm told to now introduce something until it is
used. I wish we could be more consistent on this sort of things.

And frankly this sort of change adds no particular value in this series
whatsoever.

Wei.

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 12/12] x86/hyperv: setup VP assist page

[Roger Pau Monné]

On Wed, Jan 29, 2020 at 08:20:34PM +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>
> ---
> 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  | 44 ++++++++++++++++++++++++++++-
>  xen/arch/x86/guest/hyperv/private.h |  1 +
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> index af0d6ed692..bc40a3d338 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_pcpu_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,16 +156,57 @@ static int setup_hypercall_pcpu_arg(void)
>      return 0;
>  }
>  
> +static int setup_vp_assist(void)
> +{
> +    void *mapping;
> +    uint64_t val;
> +
> +    mapping = this_cpu(hv_vp_assist);

You could also avoid the usage of the local mapping variable here.

> +
> +    if ( !mapping )
> +    {
> +        mapping = alloc_xenheap_page();
> +        if ( !mapping )
> +        {
> +            printk("Failed to allocate vp_assist page for CPU%u\n",
> +                   smp_processor_id());
> +            return -ENOMEM;
> +        }
> +
> +        clear_page(mapping);
> +        this_cpu(hv_vp_assist) = mapping;
> +    }
> +
> +    val = (virt_to_mfn(mapping) << HV_HYP_PAGE_SHIFT)

There's virt_to_maddr which would avoid the shift.

> +        | HV_X64_MSR_VP_ASSIST_PAGE_ENABLE;
> +    wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, val);
> +
> +    return 0;
> +}
> +
>  static void __init setup(void)
>  {
>      setup_hypercall_page();
> +
>      if ( setup_hypercall_pcpu_arg() )
>          panic("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 )
> +        goto out;

No need for a label, as just returning here would make the function
shorter:

rc = setup_hypercall_pcpu_arg();
if ( rc )
    return rc;

return setup_vp_assist();

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 02/12] x86/hypervisor: make hypervisor_ap_setup return an error code

[Wei Liu]

On Thu, Jan 30, 2020 at 11:01:29AM +0100, Roger Pau Monné wrote:
> On Wed, Jan 29, 2020 at 08:20:24PM +0000, Wei Liu wrote:
> > We want to be able to handle AP setup error in the upper layer.
> 
> Thanks, some comments below.
> 
> > 
> > Signed-off-by: Wei Liu <liuwe@microsoft.com>
> > ---
> >  xen/arch/x86/guest/hypervisor.c        |  6 ++++--
> >  xen/arch/x86/guest/xen/xen.c           | 11 +++++++++--
> >  xen/include/asm-x86/guest/hypervisor.h |  6 +++---
> >  3 files changed, 16 insertions(+), 7 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..eed8a6edae 100644
> > --- a/xen/arch/x86/guest/xen/xen.c
> > +++ b/xen/arch/x86/guest/xen/xen.c
> > @@ -257,11 +257,18 @@ static void __init setup(void)
> >      init_evtchn();
> >  }
> >  
> > -static void ap_setup(void)
> > +static int ap_setup(void)
> >  {
> > +    int rc;
> > +
> >      set_vcpu_id();
> > -    map_vcpuinfo();
> > +    rc = map_vcpuinfo();
> 
> map_vcpuinfo should be changed so that the BUG_ON is removed, and an
> error is only returned if VCPUOP_register_vcpu_info fails and vcpu >=
> XEN_LEGACY_MAX_VCPUS, else no error should be returned.

Done.

> 
> > +    if ( rc )
> > +        return rc;
> > +
> >      init_evtchn();
> > +
> > +    return 0;
> 
> In order to keep this shorter, you could do:
> 
> if ( !rc )
>     init_evtchn();
> 
> return rc;

Done.

Wei.

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 12/12] x86/hyperv: setup VP assist page

[Wei Liu]

On Thu, Jan 30, 2020 at 10:34:29AM +0000, Durrant, Paul wrote:
> > +
> > +    val = (virt_to_mfn(mapping) << HV_HYP_PAGE_SHIFT)
> > +        | HV_X64_MSR_VP_ASSIST_PAGE_ENABLE;
> 

> Perhaps it would be neater to put the viridian_page_msr union into
> hyperv-tlfs.h and then use that.

Done. Now this series is one patch longer...

Wei.

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

Re: [Xen-devel] [PATCH v5 12/12] x86/hyperv: setup VP assist page

[Wei Liu]

On Thu, Jan 30, 2020 at 01:42:29PM +0100, Roger Pau Monné wrote:
> On Wed, Jan 29, 2020 at 08:20:34PM +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>
> > ---
> > 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  | 44 ++++++++++++++++++++++++++++-
> >  xen/arch/x86/guest/hyperv/private.h |  1 +
> >  2 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> > index af0d6ed692..bc40a3d338 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_pcpu_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,16 +156,57 @@ static int setup_hypercall_pcpu_arg(void)
> >      return 0;
> >  }
> >  
> > +static int setup_vp_assist(void)
> > +{
> > +    void *mapping;
> > +    uint64_t val;
> > +
> > +    mapping = this_cpu(hv_vp_assist);
> 
> You could also avoid the usage of the local mapping variable here.

this_cpu(...) is longer than mapping. But since you ask for the change,
I have made the change.

> 
> > +
> > +    if ( !mapping )
> > +    {
> > +        mapping = alloc_xenheap_page();
> > +        if ( !mapping )
> > +        {
> > +            printk("Failed to allocate vp_assist page for CPU%u\n",
> > +                   smp_processor_id());
> > +            return -ENOMEM;
> > +        }
> > +
> > +        clear_page(mapping);
> > +        this_cpu(hv_vp_assist) = mapping;
> > +    }
> > +
> > +    val = (virt_to_mfn(mapping) << HV_HYP_PAGE_SHIFT)
> 
> There's virt_to_maddr which would avoid the shift.

This line is gone.

> 
> > +        | HV_X64_MSR_VP_ASSIST_PAGE_ENABLE;
> > +    wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, val);
> > +
> > +    return 0;
> > +}
> > +
> >  static void __init setup(void)
> >  {
> >      setup_hypercall_page();
> > +
> >      if ( setup_hypercall_pcpu_arg() )
> >          panic("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 )
> > +        goto out;
> 
> No need for a label, as just returning here would make the function
> shorter:
> 
> rc = setup_hypercall_pcpu_arg();
> if ( rc )
>     return rc;
> 
> return setup_vp_assist();

Done.

Wei.

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 10/12] x86/hyperv: provide percpu hypercall input page

[Wei Liu]

On Thu, Jan 30, 2020 at 01:26:55PM +0100, Roger Pau Monné wrote:
> On Wed, Jan 29, 2020 at 08:20:32PM +0000, Wei Liu wrote:
> > 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: Roger Pau Monné <roger.pau@citrix.com>
> 
> Just some nits below.
> 

All comments addressed.

Wei.

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Roger Pau Monné]

On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote:
> On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote:
> > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote:
> > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
> > > > 
> > > > > +}
> > > > > +
> > > > >  /*
> > > > >   * Local variables:
> > > > >   * mode: C
> > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > > > > index 97f9c07891..8e02b4c648 100644
> > > > > --- a/xen/arch/x86/xen.lds.S
> > > > > +++ b/xen/arch/x86/xen.lds.S
> > > > > @@ -329,6 +329,10 @@ SECTIONS
> > > > >    efi = .;
> > > > >  #endif
> > > > >  
> > > > > +#ifdef CONFIG_HYPERV_GUEST
> > > > > +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> > > > 
> > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
> > > > enum?
> > > > 
> > > 
> > > Yes.
> > > 
> > > And the trick to generate a symbol didn't work either.
> > 
> > And you must define that symbol in the linker script? It doesn't seem
> > to be used at link time.
> > 
> 
> I don't follow. I wish I could define and use a symbol in the linker
> script but couldn't.

It's likely my fault, as I haven't been following the patch series in
that much detail. I assume this is done in order to generate better
code, rather than doing something like:

void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL);

In a C file somewhere when the hypercall page is setup?

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Thu, Jan 30, 2020 at 03:22:01PM +0100, Roger Pau Monné wrote:
> On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote:
> > On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote:
> > > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote:
> > > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
> > > > > 
> > > > > > +}
> > > > > > +
> > > > > >  /*
> > > > > >   * Local variables:
> > > > > >   * mode: C
> > > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > > > > > index 97f9c07891..8e02b4c648 100644
> > > > > > --- a/xen/arch/x86/xen.lds.S
> > > > > > +++ b/xen/arch/x86/xen.lds.S
> > > > > > @@ -329,6 +329,10 @@ SECTIONS
> > > > > >    efi = .;
> > > > > >  #endif
> > > > > >  
> > > > > > +#ifdef CONFIG_HYPERV_GUEST
> > > > > > +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> > > > > 
> > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
> > > > > enum?
> > > > > 
> > > > 
> > > > Yes.
> > > > 
> > > > And the trick to generate a symbol didn't work either.
> > > 
> > > And you must define that symbol in the linker script? It doesn't seem
> > > to be used at link time.
> > > 
> > 
> > I don't follow. I wish I could define and use a symbol in the linker
> > script but couldn't.
> 
> It's likely my fault, as I haven't been following the patch series in
> that much detail. I assume this is done in order to generate better
> code, rather than doing something like:
> 
> void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL);
> 
> In a C file somewhere when the hypercall page is setup?

Andrew wanted badly to be able to use direct call in the hypercall
functions. This is what we managed to come up with so far.

I think what you wrote will still result in an indirect call.

(The majority of my time spent on this series has been extending Xen to
do more than it could before.)

Wei.

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Roger Pau Monné]

On Thu, Jan 30, 2020 at 02:25:26PM +0000, Wei Liu wrote:
> On Thu, Jan 30, 2020 at 03:22:01PM +0100, Roger Pau Monné wrote:
> > On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote:
> > > On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote:
> > > > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote:
> > > > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
> > > > > > 
> > > > > > > +}
> > > > > > > +
> > > > > > >  /*
> > > > > > >   * Local variables:
> > > > > > >   * mode: C
> > > > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > > > > > > index 97f9c07891..8e02b4c648 100644
> > > > > > > --- a/xen/arch/x86/xen.lds.S
> > > > > > > +++ b/xen/arch/x86/xen.lds.S
> > > > > > > @@ -329,6 +329,10 @@ SECTIONS
> > > > > > >    efi = .;
> > > > > > >  #endif
> > > > > > >  
> > > > > > > +#ifdef CONFIG_HYPERV_GUEST
> > > > > > > +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> > > > > > 
> > > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
> > > > > > enum?
> > > > > > 
> > > > > 
> > > > > Yes.
> > > > > 
> > > > > And the trick to generate a symbol didn't work either.
> > > > 
> > > > And you must define that symbol in the linker script? It doesn't seem
> > > > to be used at link time.
> > > > 
> > > 
> > > I don't follow. I wish I could define and use a symbol in the linker
> > > script but couldn't.
> > 
> > It's likely my fault, as I haven't been following the patch series in
> > that much detail. I assume this is done in order to generate better
> > code, rather than doing something like:
> > 
> > void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL);
> > 
> > In a C file somewhere when the hypercall page is setup?
> 
> Andrew wanted badly to be able to use direct call in the hypercall
> functions. This is what we managed to come up with so far.
> 
> I think what you wrote will still result in an indirect call.
> 
> (The majority of my time spent on this series has been extending Xen to
> do more than it could before.)

Ack, sorry to bother you with questions you have already answered. Not
sure whether defining hv_hcall_page as a global const would make much
difference. Could you maybe use something like alternative_vcall
patching to get rid of the indirection?

I have to admit I find this all quite hard to follow and reason about,
likely because of the mix of C, assembly, and linker script to build
this machinery, but that doesn't mean this isn't the best way.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Thu, Jan 30, 2020 at 03:47:04PM +0100, Roger Pau Monné wrote:
> On Thu, Jan 30, 2020 at 02:25:26PM +0000, Wei Liu wrote:
> > On Thu, Jan 30, 2020 at 03:22:01PM +0100, Roger Pau Monné wrote:
> > > On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote:
> > > > On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote:
> > > > > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote:
> > > > > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
> > > > > > > 
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  /*
> > > > > > > >   * Local variables:
> > > > > > > >   * mode: C
> > > > > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > > > > > > > index 97f9c07891..8e02b4c648 100644
> > > > > > > > --- a/xen/arch/x86/xen.lds.S
> > > > > > > > +++ b/xen/arch/x86/xen.lds.S
> > > > > > > > @@ -329,6 +329,10 @@ SECTIONS
> > > > > > > >    efi = .;
> > > > > > > >  #endif
> > > > > > > >  
> > > > > > > > +#ifdef CONFIG_HYPERV_GUEST
> > > > > > > > +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> > > > > > > 
> > > > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
> > > > > > > enum?
> > > > > > > 
> > > > > > 
> > > > > > Yes.
> > > > > > 
> > > > > > And the trick to generate a symbol didn't work either.
> > > > > 
> > > > > And you must define that symbol in the linker script? It doesn't seem
> > > > > to be used at link time.
> > > > > 
> > > > 
> > > > I don't follow. I wish I could define and use a symbol in the linker
> > > > script but couldn't.
> > > 
> > > It's likely my fault, as I haven't been following the patch series in
> > > that much detail. I assume this is done in order to generate better
> > > code, rather than doing something like:
> > > 
> > > void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL);
> > > 
> > > In a C file somewhere when the hypercall page is setup?
> > 
> > Andrew wanted badly to be able to use direct call in the hypercall
> > functions. This is what we managed to come up with so far.
> > 
> > I think what you wrote will still result in an indirect call.
> > 
> > (The majority of my time spent on this series has been extending Xen to
> > do more than it could before.)
> 
> Ack, sorry to bother you with questions you have already answered. Not

No worries. I value your feedback. And having more people understand
what is going on is important to the project.

> sure whether defining hv_hcall_page as a global const would make much
> difference. Could you maybe use something like alternative_vcall
> patching to get rid of the indirection?

Tried that and didn't work either. :-(

> 
> I have to admit I find this all quite hard to follow and reason about,
> likely because of the mix of C, assembly, and linker script to build
> this machinery, but that doesn't mean this isn't the best way.
> 

Yes, a lot of trickeries are used to make this work. Not the most
elegant combination I would say, but it does achieve what is desired.

Wei.

> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Roger Pau Monné]

On Thu, Jan 30, 2020 at 03:03:03PM +0000, Wei Liu wrote:
> On Thu, Jan 30, 2020 at 03:47:04PM +0100, Roger Pau Monné wrote:
> > On Thu, Jan 30, 2020 at 02:25:26PM +0000, Wei Liu wrote:
> > > On Thu, Jan 30, 2020 at 03:22:01PM +0100, Roger Pau Monné wrote:
> > > > On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote:
> > > > > On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote:
> > > > > > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote:
> > > > > > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
> > > > > > > > 
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  /*
> > > > > > > > >   * Local variables:
> > > > > > > > >   * mode: C
> > > > > > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > > > > > > > > index 97f9c07891..8e02b4c648 100644
> > > > > > > > > --- a/xen/arch/x86/xen.lds.S
> > > > > > > > > +++ b/xen/arch/x86/xen.lds.S
> > > > > > > > > @@ -329,6 +329,10 @@ SECTIONS
> > > > > > > > >    efi = .;
> > > > > > > > >  #endif
> > > > > > > > >  
> > > > > > > > > +#ifdef CONFIG_HYPERV_GUEST
> > > > > > > > > +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> > > > > > > > 
> > > > > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
> > > > > > > > enum?
> > > > > > > > 
> > > > > > > 
> > > > > > > Yes.
> > > > > > > 
> > > > > > > And the trick to generate a symbol didn't work either.
> > > > > > 
> > > > > > And you must define that symbol in the linker script? It doesn't seem
> > > > > > to be used at link time.
> > > > > > 
> > > > > 
> > > > > I don't follow. I wish I could define and use a symbol in the linker
> > > > > script but couldn't.
> > > > 
> > > > It's likely my fault, as I haven't been following the patch series in
> > > > that much detail. I assume this is done in order to generate better
> > > > code, rather than doing something like:
> > > > 
> > > > void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL);
> > > > 
> > > > In a C file somewhere when the hypercall page is setup?
> > > 
> > > Andrew wanted badly to be able to use direct call in the hypercall
> > > functions. This is what we managed to come up with so far.
> > > 
> > > I think what you wrote will still result in an indirect call.
> > > 
> > > (The majority of my time spent on this series has been extending Xen to
> > > do more than it could before.)
> > 
> > Ack, sorry to bother you with questions you have already answered. Not
> 
> No worries. I value your feedback. And having more people understand
> what is going on is important to the project.
> 
> > sure whether defining hv_hcall_page as a global const would make much
> > difference. Could you maybe use something like alternative_vcall
> > patching to get rid of the indirection?
> 
> Tried that and didn't work either. :-(

How do you check whether there's an indirect call or not when using
alternative_vcall?

It's my understanding that in that case the patching will happen at
runtime, and hence the generated assembly code would still use an
indirect call, but once patched at runtime it should become a direct
call.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Thu, Jan 30, 2020 at 04:25:44PM +0100, Roger Pau Monné wrote:
> On Thu, Jan 30, 2020 at 03:03:03PM +0000, Wei Liu wrote:
> > On Thu, Jan 30, 2020 at 03:47:04PM +0100, Roger Pau Monné wrote:
> > > On Thu, Jan 30, 2020 at 02:25:26PM +0000, Wei Liu wrote:
> > > > On Thu, Jan 30, 2020 at 03:22:01PM +0100, Roger Pau Monné wrote:
> > > > > On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote:
> > > > > > On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote:
> > > > > > > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote:
> > > > > > > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
> > > > > > > > > 
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >  /*
> > > > > > > > > >   * Local variables:
> > > > > > > > > >   * mode: C
> > > > > > > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > > > > > > > > > index 97f9c07891..8e02b4c648 100644
> > > > > > > > > > --- a/xen/arch/x86/xen.lds.S
> > > > > > > > > > +++ b/xen/arch/x86/xen.lds.S
> > > > > > > > > > @@ -329,6 +329,10 @@ SECTIONS
> > > > > > > > > >    efi = .;
> > > > > > > > > >  #endif
> > > > > > > > > >  
> > > > > > > > > > +#ifdef CONFIG_HYPERV_GUEST
> > > > > > > > > > +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> > > > > > > > > 
> > > > > > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
> > > > > > > > > enum?
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Yes.
> > > > > > > > 
> > > > > > > > And the trick to generate a symbol didn't work either.
> > > > > > > 
> > > > > > > And you must define that symbol in the linker script? It doesn't seem
> > > > > > > to be used at link time.
> > > > > > > 
> > > > > > 
> > > > > > I don't follow. I wish I could define and use a symbol in the linker
> > > > > > script but couldn't.
> > > > > 
> > > > > It's likely my fault, as I haven't been following the patch series in
> > > > > that much detail. I assume this is done in order to generate better
> > > > > code, rather than doing something like:
> > > > > 
> > > > > void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL);
> > > > > 
> > > > > In a C file somewhere when the hypercall page is setup?
> > > > 
> > > > Andrew wanted badly to be able to use direct call in the hypercall
> > > > functions. This is what we managed to come up with so far.
> > > > 
> > > > I think what you wrote will still result in an indirect call.
> > > > 
> > > > (The majority of my time spent on this series has been extending Xen to
> > > > do more than it could before.)
> > > 
> > > Ack, sorry to bother you with questions you have already answered. Not
> > 
> > No worries. I value your feedback. And having more people understand
> > what is going on is important to the project.
> > 
> > > sure whether defining hv_hcall_page as a global const would make much
> > > difference. Could you maybe use something like alternative_vcall
> > > patching to get rid of the indirection?
> > 
> > Tried that and didn't work either. :-(
> 
> How do you check whether there's an indirect call or not when using
> alternative_vcall?
> 

I didn't check, because alternative_vcall didn't compile in that case.

> It's my understanding that in that case the patching will happen at
> runtime, and hence the generated assembly code would still use an
> indirect call, but once patched at runtime it should become a direct
> call.

It didn't even compile. :-(

Wei.

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 07/12] x86/hyperv: setup hypercall page

[Wei Liu]

On Thu, Jan 30, 2020 at 01:11:59PM +0100, Roger Pau Monné wrote:
> On Thu, Jan 30, 2020 at 11:47:52AM +0000, Wei Liu wrote:
> > On Thu, Jan 30, 2020 at 12:39:47PM +0100, Roger Pau Monné wrote:
> > > On Thu, Jan 30, 2020 at 11:18:21AM +0000, Wei Liu wrote:
> > > > On Thu, Jan 30, 2020 at 11:41:43AM +0100, Roger Pau Monné wrote:
> > > > > On Wed, Jan 29, 2020 at 08:20:29PM +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 code
> > > > > > 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>
> > > > > > ---
> > > > > > 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/e820.c                     |  5 +++
> > > > > >  xen/arch/x86/guest/hyperv/hyperv.c      | 57 +++++++++++++++++++++++--
> > > > > >  xen/include/asm-x86/guest/hyperv-tlfs.h |  5 ++-
> > > > > >  xen/include/asm-x86/guest/hyperv.h      |  3 ++
> > > > > >  4 files changed, 65 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> > > > > > index 3892c9cfb7..99643f3ea0 100644
> > > > > > --- a/xen/arch/x86/e820.c
> > > > > > +++ b/xen/arch/x86/e820.c
> > > > > > @@ -343,6 +343,7 @@ static unsigned long __init find_max_pfn(void)
> > > > > >  {
> > > > > >      unsigned int i;
> > > > > >      unsigned long max_pfn = 0;
> > > > > > +    unsigned long top_pfn = ((1ull << paddr_bits) - 1) >> PAGE_SHIFT;
> > > > > >  
> > > > > >      for (i = 0; i < e820.nr_map; i++) {
> > > > > >          unsigned long start, end;
> > > > > > @@ -357,6 +358,10 @@ static unsigned long __init find_max_pfn(void)
> > > > > >              max_pfn = end;
> > > > > >      }
> > > > > >  
> > > > > > +    top_pfn -= hypervisor_reserve_top_pages();
> > > > > > +    if ( max_pfn >= top_pfn )
> > > > > > +        max_pfn = top_pfn;
> > > > > 
> > > > > Hm, I'm not sure I see the point of this. The value returned by
> > > > > find_max_pfn is the maximum RAM address found in the memory map, but
> > > > > the physical address you are using to map the hypercall page is almost
> > > > > certainly much higher than the maximum address found in the physmap
> > > > > (and certainly not RAM), and hence I'm not sure what's the point of
> > > > > this.
> > > > 
> > > > Yes, the keyword is "almost certainly". :-)
> > > > 
> > > > This is done for correctness's sake. I don't expect in practice there
> > > > would be a configuration that has that much memory, but correctness is
> > > > still important.
> > > > 
> > > > It also guards against weird configuration in which memory is put into
> > > > that part of the physical address space for whatever reason. I don't
> > > > know why anyone would do that, but again, we should be prepared for
> > > > that.
> > > > 
> > > > 
> > > > > 
> > > > > Also you haven't introduced a HyperV implementation of
> > > > > hypervisor_reserve_top_pages so far, so it's hard to tell the intend
> > > > > of this.
> > > > 
> > > > D'oh. That was supposed to be in this patch. I guess I forgot to commit
> > > > that hunk!
> > > > 
> > > > That function for Hyper-V is going to return 1 (page).
> > > 
> > > But that would likely be wrong, unless the memory map has a RAM
> > > region that expands up to (1 << paddr_bits)?
> > > 
> > > Or else you are just removing a page from the last RAM region in
> > > the memory map for no reason. max_pfn is almost certainly way below (1
> > > << paddr_bits).
> > > 
> > 
> > Why? The adjustment will not be applied unless RAM overlaps with that
> > reserved region.
> 
> Oh, OK, from your previous reply I understood that
> hypervisor_reserve_top_pages would unconditionally return 1 for
> HyperV, so that would end up always subtracting 1 page from the last
> RAM region, even when not overlapping with (1 << paddr_bits).
> 
> > 
> > > I think what you need is a hook that modifies the memory map and adds
> > > a reserved region at ((1 << paddr_bits) - PAGE_SIZE) of size
> > > PAGE_SIZE. See where pv_shim_fixup_e820 is used, and I think you want
> > > to make this a hypervisor hook and add the HyperV code to reserve the
> > > hypercall page in the e820 there.
> > 
> > That works for me too. Let's see what other people think.
> 
> I think that's the safest way, as you can assure there's nothing in
> the region to be used by the hypercall page, and you can actually mark
> it as reserved in the e820.

I like this idea. I will post a new version shortly.

Wei.

> 
> Thanks, Roger.

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

Re: [Xen-devel] [PATCH v5 05/12] x86: provide executable fixmap facility

[Wei Liu]

On Thu, Jan 30, 2020 at 12:30:47AM +0000, Wei Liu wrote:
> On Wed, Jan 29, 2020 at 08:20:27PM +0000, Wei Liu wrote:
> >  
> > +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);
> > +
> > +    /* Generate a symbol to be used in linker script */
> > +    asm ( ".equ FIXADDR_X_SIZE, %c0; .global FIXADDR_X_SIZE"
> > +          :: "i" (__end_of_fixed_addresses_x << PAGE_SHIFT) );
> 
> The (__end << SHIFT) part can be replaced with FIXADDR_X_SIZE (the macro
> defined in fixmap.h) directly.

I also discover that this snippet to generate symbol should be moved
else where because if Hyper-V support is compiled out, this function has
no user. That causes it to be DCE'd. FIXADDR_X_SIZE would be gone and
linking would fail.

I have moved this to arch_init_memory. Its storage will be reclaimed
during runtime, but this symbol is not used anywhere else in code, so we
should be fine.

Wei.

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

Re: [Xen-devel] [PATCH v5 05/12] x86: provide executable fixmap facility

[Jan Beulich]

On 31.01.2020 14:12, Wei Liu wrote:
> On Thu, Jan 30, 2020 at 12:30:47AM +0000, Wei Liu wrote:
>> On Wed, Jan 29, 2020 at 08:20:27PM +0000, Wei Liu wrote:
>>>  
>>> +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);
>>> +
>>> +    /* Generate a symbol to be used in linker script */
>>> +    asm ( ".equ FIXADDR_X_SIZE, %c0; .global FIXADDR_X_SIZE"
>>> +          :: "i" (__end_of_fixed_addresses_x << PAGE_SHIFT) );
>>
>> The (__end << SHIFT) part can be replaced with FIXADDR_X_SIZE (the macro
>> defined in fixmap.h) directly.
> 
> I also discover that this snippet to generate symbol should be moved
> else where because if Hyper-V support is compiled out, this function has
> no user. That causes it to be DCE'd. FIXADDR_X_SIZE would be gone and
> linking would fail.
> 
> I have moved this to arch_init_memory. Its storage will be reclaimed
> during runtime, but this symbol is not used anywhere else in code, so we
> should be fine.

Storage? This is a constant, i.e. just a symbol without any
storage. Therefore an __init function is a very good place to
put it. It could also live outside of any function, if only
file scope asm()-s finally permitted [certain] inputs.

Jan

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

Re: [Xen-devel] [PATCH v5 05/12] x86: provide executable fixmap facility

[Wei Liu]

On Fri, Jan 31, 2020 at 02:19:16PM +0100, Jan Beulich wrote:
> On 31.01.2020 14:12, Wei Liu wrote:
> > On Thu, Jan 30, 2020 at 12:30:47AM +0000, Wei Liu wrote:
> >> On Wed, Jan 29, 2020 at 08:20:27PM +0000, Wei Liu wrote:
> >>>  
> >>> +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);
> >>> +
> >>> +    /* Generate a symbol to be used in linker script */
> >>> +    asm ( ".equ FIXADDR_X_SIZE, %c0; .global FIXADDR_X_SIZE"
> >>> +          :: "i" (__end_of_fixed_addresses_x << PAGE_SHIFT) );
> >>
> >> The (__end << SHIFT) part can be replaced with FIXADDR_X_SIZE (the macro
> >> defined in fixmap.h) directly.
> > 
> > I also discover that this snippet to generate symbol should be moved
> > else where because if Hyper-V support is compiled out, this function has
> > no user. That causes it to be DCE'd. FIXADDR_X_SIZE would be gone and
> > linking would fail.
> > 
> > I have moved this to arch_init_memory. Its storage will be reclaimed
> > during runtime, but this symbol is not used anywhere else in code, so we
> > should be fine.
> 
> Storage? This is a constant, i.e. just a symbol without any

"Its" meaning arch_init_memory, but I wasn't clear. Sorry.

> storage. Therefore an __init function is a very good place to
> put it. It could also live outside of any function, if only
> file scope asm()-s finally permitted [certain] inputs.

Tried putting in in file scope, didn't work.

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v5 03/12] x86/smp: don't online cpu if hypervisor_ap_setup fails

[Jan Beulich]

On 29.01.2020 21:20, 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>
> ---
>  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;
> +    }

There are a few things done up from here which may possibly
better come after hypervisor interface setup (the two APIC
related calls in particular). Are you sure you can safely
move it this far down in the function?

Jan

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

Re: [Xen-devel] [PATCH v5 04/12] x86: make paddr_bits available earlier

[Jan Beulich]

On 30.01.2020 13:00, Wei Liu wrote:
> On Thu, Jan 30, 2020 at 11:17:33AM +0100, Roger Pau Monné wrote:
>>>  
>>> +    /* This must come before e820 code becuause it sets paddr_bits. */
>>                                           ^ because
> 
> Fixed. Thanks.

And then
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

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

Re: [Xen-devel] [PATCH v5 07/12] x86/hyperv: setup hypercall page

[Jan Beulich]

On 30.01.2020 12:47, Wei Liu wrote:
> On Thu, Jan 30, 2020 at 12:39:47PM +0100, Roger Pau Monné wrote:
>> I think what you need is a hook that modifies the memory map and adds
>> a reserved region at ((1 << paddr_bits) - PAGE_SIZE) of size
>> PAGE_SIZE. See where pv_shim_fixup_e820 is used, and I think you want
>> to make this a hypervisor hook and add the HyperV code to reserve the
>> hypercall page in the e820 there.
> 
> That works for me too. Let's see what other people think.

The idea lgtm, pending seeing it actually carried out.

Jan

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

Re: [Xen-devel] [PATCH v5 03/12] x86/smp: don't online cpu if hypervisor_ap_setup fails

[Wei Liu]

On Fri, Jan 31, 2020 at 02:53:45PM +0100, Jan Beulich wrote:
> On 29.01.2020 21:20, 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>
> > ---
> >  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;
> > +    }
> 
> There are a few things done up from here which may possibly
> better come after hypervisor interface setup (the two APIC
> related calls in particular). Are you sure you can safely
> move it this far down in the function?

Xen guest's usage of APIC is no different than, say, hvm's usage. If hvm
can be this far down, Xen / Hyper-V can, too.

Furthermore, APIC code has no dependency on guest code.

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich]

On 30.01.2020 13:28, Wei Liu wrote:
> On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
>>
>>> +}
>>> +
>>>  /*
>>>   * Local variables:
>>>   * mode: C
>>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
>>> index 97f9c07891..8e02b4c648 100644
>>> --- a/xen/arch/x86/xen.lds.S
>>> +++ b/xen/arch/x86/xen.lds.S
>>> @@ -329,6 +329,10 @@ SECTIONS
>>>    efi = .;
>>>  #endif
>>>  
>>> +#ifdef CONFIG_HYPERV_GUEST
>>> +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
>>
>> I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
>> enum?
>>
> 
> Yes.
> 
> And the trick to generate a symbol didn't work either.

I guess I need an explanation here. Aiui you don't really need
the definition to be in the linker script, and it could as well
be in e.g. assembly code. How does the same .equ approach not
work in this case?

Also I think the above will trigger the warnings Andrew had
mentioned (on irc?) from the code generating xen.efi's runtime
relocation table. Just like in

ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START -
                          NR_CPUS * PAGE_SIZE,
       "Xen image overlaps stubs area")

I think you need to adjust by __XEN_VIRT_START - XEN_VIRT_START.

Jan

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Fri, Jan 31, 2020 at 03:12:50PM +0100, Jan Beulich wrote:
> On 30.01.2020 13:28, Wei Liu wrote:
> > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
> >>
> >>> +}
> >>> +
> >>>  /*
> >>>   * Local variables:
> >>>   * mode: C
> >>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> >>> index 97f9c07891..8e02b4c648 100644
> >>> --- a/xen/arch/x86/xen.lds.S
> >>> +++ b/xen/arch/x86/xen.lds.S
> >>> @@ -329,6 +329,10 @@ SECTIONS
> >>>    efi = .;
> >>>  #endif
> >>>  
> >>> +#ifdef CONFIG_HYPERV_GUEST
> >>> +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> >>
> >> I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
> >> enum?
> >>
> > 
> > Yes.
> > 
> > And the trick to generate a symbol didn't work either.
> 
> I guess I need an explanation here. Aiui you don't really need
> the definition to be in the linker script, and it could as well
> be in e.g. assembly code. How does the same .equ approach not
> work in this case?
> 

In commit message:

mm.c:5736:5: error: invalid 'asm': operand is not a condition code,
invalid operand code 'c'
               asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"

ISTR you once mentioned in IRC that there is a way around this (with a
new modifier / qualifier), but I don't have the log anymore.

> Also I think the above will trigger the warnings Andrew had
> mentioned (on irc?) from the code generating xen.efi's runtime
> relocation table. Just like in
> 

It was a reply to v4.  <cb0e82dc-a154-f918-e725-f77913f835f9@citrix.com>

I don't see the warning with this patch.

Wei.

> ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START -
>                           NR_CPUS * PAGE_SIZE,
>        "Xen image overlaps stubs area")
> 
> I think you need to adjust by __XEN_VIRT_START - XEN_VIRT_START.
> 
> Jan

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich]

On 29.01.2020 21:20, Wei Liu wrote:
> I tried using the asm(".equ ..") trick but hit a problem with %c again.
> 
> mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c'
>                asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"

Would you mind also indicating what the input operand actually
was? According to my looking at gcc sources when you first
mentioned this (on irc iirc), much depends on it actually be
recognizable as a constant by the compiler.

> +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;

I guess strictly speaking this wants to be asm ( "r8" ),
albeit I now realize that I've similarly not played by style
in alternative_callN(). In the end I guess - either way.

> +    asm volatile ( "call hv_hcall_page"
> +                   : "=a" (status), "+c" (control),
> +                     "+d" (input_addr) ASM_CALL_CONSTRAINT
> +                   : "r" (r8)

Why "+c" and "+d" but just "r"? If r8 gets treated differently
from rcx and rdx, please attach a brief comment.

Jan

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

Re: [Xen-devel] [PATCH v5 11/12] x86/hyperv: retrieve vp_index from Hyper-V

[Jan Beulich]

On 29.01.2020 21:20, Wei Liu wrote:
> 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.

And I now realize that it's actually irrelevant for this

>  xen/arch/x86/guest/hyperv/hyperv.c  | 5 +++++
>  xen/arch/x86/guest/hyperv/private.h | 1 +
>  2 files changed, 6 insertions(+)

set of changed files. (Feel free to keep, of course.)

Jan

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

Re: [Xen-devel] [PATCH v5 03/12] x86/smp: don't online cpu if hypervisor_ap_setup fails

[Jan Beulich]

On 31.01.2020 15:10, Wei Liu wrote:
> On Fri, Jan 31, 2020 at 02:53:45PM +0100, Jan Beulich wrote:
>> On 29.01.2020 21:20, 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>
>>> ---
>>>  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;
>>> +    }
>>
>> There are a few things done up from here which may possibly
>> better come after hypervisor interface setup (the two APIC
>> related calls in particular). Are you sure you can safely
>> move it this far down in the function?
> 
> Xen guest's usage of APIC is no different than, say, hvm's usage. If hvm
> can be this far down, Xen / Hyper-V can, too.
> 
> Furthermore, APIC code has no dependency on guest code.

Hmm, okay, there's no way for a HVM guest to run without LAPIC
emulation right now. This should be fine then:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich]

On 31.01.2020 15:20, Wei Liu wrote:
> On Fri, Jan 31, 2020 at 03:12:50PM +0100, Jan Beulich wrote:
>> On 30.01.2020 13:28, Wei Liu wrote:
>>> On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
>>>>
>>>>> +}
>>>>> +
>>>>>  /*
>>>>>   * Local variables:
>>>>>   * mode: C
>>>>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
>>>>> index 97f9c07891..8e02b4c648 100644
>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>> @@ -329,6 +329,10 @@ SECTIONS
>>>>>    efi = .;
>>>>>  #endif
>>>>>  
>>>>> +#ifdef CONFIG_HYPERV_GUEST
>>>>> +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
>>>>
>>>> I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
>>>> enum?
>>>>
>>>
>>> Yes.
>>>
>>> And the trick to generate a symbol didn't work either.
>>
>> I guess I need an explanation here. Aiui you don't really need
>> the definition to be in the linker script, and it could as well
>> be in e.g. assembly code. How does the same .equ approach not
>> work in this case?
>>
> 
> In commit message:
> 
> mm.c:5736:5: error: invalid 'asm': operand is not a condition code,
> invalid operand code 'c'
>                asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"

But this lacks the other half of the asm().

> ISTR you once mentioned in IRC that there is a way around this (with a
> new modifier / qualifier), but I don't have the log anymore.

And I think we did say that we didn't want to go too far with
using gcc internals. IOW I think trying different modifiers is
out of question now (it was -%n0 iirc), but getting past gcc
not recognizing the constant as being a constant may still be
a viable route.

Jan

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Fri, Jan 31, 2020 at 03:24:07PM +0100, Jan Beulich wrote:
> On 29.01.2020 21:20, Wei Liu wrote:
> > I tried using the asm(".equ ..") trick but hit a problem with %c again.
> > 
> > mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c'
> >                asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
> 
> Would you mind also indicating what the input operand actually
> was? According to my looking at gcc sources when you first
> mentioned this (on irc iirc), much depends on it actually be
> recognizable as a constant by the compiler.

Something along the line:

  asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
       :: "i" (__fix_x_to_virt(FIX_X_HV...))


> 
> > +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;
> 
> I guess strictly speaking this wants to be asm ( "r8" ),
> albeit I now realize that I've similarly not played by style
> in alternative_callN(). In the end I guess - either way.

OK. I can fix this.

> 
> > +    asm volatile ( "call hv_hcall_page"
> > +                   : "=a" (status), "+c" (control),
> > +                     "+d" (input_addr) ASM_CALL_CONSTRAINT
> > +                   : "r" (r8)
> 
> Why "+c" and "+d" but just "r"? If r8 gets treated differently
> from rcx and rdx, please attach a brief comment.
> 

Off the top of my head: r8 will not be modified by Hyper-V, while others
may.

Wei.


> Jan

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich]

On 31.01.2020 15:37, Wei Liu wrote:
> On Fri, Jan 31, 2020 at 03:24:07PM +0100, Jan Beulich wrote:
>> On 29.01.2020 21:20, Wei Liu wrote:
>>> I tried using the asm(".equ ..") trick but hit a problem with %c again.
>>>
>>> mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c'
>>>                asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
>>
>> Would you mind also indicating what the input operand actually
>> was? According to my looking at gcc sources when you first
>> mentioned this (on irc iirc), much depends on it actually be
>> recognizable as a constant by the compiler.
> 
> Something along the line:
> 
>   asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
>        :: "i" (__fix_x_to_virt(FIX_X_HV...))

Quite a bit of playing later, %P0 is documented, supported
already in gcc 4.1.x, and also used in a few cases by Linux.
%p0 would be another documented alternative, but support for
this looks to have been introduced later. Not being able to use
%c0 here still smells like a bug (and I guess I'll enter one.)

Jan

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Fri, Jan 31, 2020 at 04:35:23PM +0100, Jan Beulich wrote:
> On 31.01.2020 15:37, Wei Liu wrote:
> > On Fri, Jan 31, 2020 at 03:24:07PM +0100, Jan Beulich wrote:
> >> On 29.01.2020 21:20, Wei Liu wrote:
> >>> I tried using the asm(".equ ..") trick but hit a problem with %c again.
> >>>
> >>> mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c'
> >>>                asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
> >>
> >> Would you mind also indicating what the input operand actually
> >> was? According to my looking at gcc sources when you first
> >> mentioned this (on irc iirc), much depends on it actually be
> >> recognizable as a constant by the compiler.
> > 
> > Something along the line:
> > 
> >   asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
> >        :: "i" (__fix_x_to_virt(FIX_X_HV...))
> 
> Quite a bit of playing later, %P0 is documented, supported
> already in gcc 4.1.x, and also used in a few cases by Linux.
> %p0 would be another documented alternative, but support for
> this looks to have been introduced later. Not being able to use
> %c0 here still smells like a bug (and I guess I'll enter one.)

OK. Let me try that.

If that turns out successful, do you want me to change the other
instance to %P0 too?

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich]

On 31.01.2020 17:15, Wei Liu wrote:
> On Fri, Jan 31, 2020 at 04:35:23PM +0100, Jan Beulich wrote:
>> On 31.01.2020 15:37, Wei Liu wrote:
>>> On Fri, Jan 31, 2020 at 03:24:07PM +0100, Jan Beulich wrote:
>>>> On 29.01.2020 21:20, Wei Liu wrote:
>>>>> I tried using the asm(".equ ..") trick but hit a problem with %c again.
>>>>>
>>>>> mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c'
>>>>>                asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
>>>>
>>>> Would you mind also indicating what the input operand actually
>>>> was? According to my looking at gcc sources when you first
>>>> mentioned this (on irc iirc), much depends on it actually be
>>>> recognizable as a constant by the compiler.
>>>
>>> Something along the line:
>>>
>>>   asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
>>>        :: "i" (__fix_x_to_virt(FIX_X_HV...))
>>
>> Quite a bit of playing later, %P0 is documented, supported
>> already in gcc 4.1.x, and also used in a few cases by Linux.
>> %p0 would be another documented alternative, but support for
>> this looks to have been introduced later. Not being able to use
>> %c0 here still smells like a bug (and I guess I'll enter one.)
> 
> OK. Let me try that.
> 
> If that turns out successful, do you want me to change the other
> instance to %P0 too?

That was a pretty small value, wasn't it? I guess it might be safer
to switch to %P (and then perhaps also elsewhere in the code base).
But during my playing with it I also noticed there's a signedness
bug (affecting all possible modifiers), so we need to watch out for
results being right in any event.

Jan

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

Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Fri, Jan 31, 2020 at 05:18:14PM +0100, Jan Beulich wrote:
> On 31.01.2020 17:15, Wei Liu wrote:
> > On Fri, Jan 31, 2020 at 04:35:23PM +0100, Jan Beulich wrote:
> >> On 31.01.2020 15:37, Wei Liu wrote:
> >>> On Fri, Jan 31, 2020 at 03:24:07PM +0100, Jan Beulich wrote:
> >>>> On 29.01.2020 21:20, Wei Liu wrote:
> >>>>> I tried using the asm(".equ ..") trick but hit a problem with %c again.
> >>>>>
> >>>>> mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c'
> >>>>>                asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
> >>>>
> >>>> Would you mind also indicating what the input operand actually
> >>>> was? According to my looking at gcc sources when you first
> >>>> mentioned this (on irc iirc), much depends on it actually be
> >>>> recognizable as a constant by the compiler.
> >>>
> >>> Something along the line:
> >>>
> >>>   asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE"
> >>>        :: "i" (__fix_x_to_virt(FIX_X_HV...))
> >>
> >> Quite a bit of playing later, %P0 is documented, supported
> >> already in gcc 4.1.x, and also used in a few cases by Linux.
> >> %p0 would be another documented alternative, but support for
> >> this looks to have been introduced later. Not being able to use
> >> %c0 here still smells like a bug (and I guess I'll enter one.)
> > 
> > OK. Let me try that.
> > 
> > If that turns out successful, do you want me to change the other
> > instance to %P0 too?
> 
> That was a pretty small value, wasn't it? I guess it might be safer
> to switch to %P (and then perhaps also elsewhere in the code base).

Yes. That value is 0x2000 at the moment.

> But during my playing with it I also noticed there's a signedness
> bug (affecting all possible modifiers), so we need to watch out for
> results being right in any event.

Using %P0 works just fine for that instance, the generated value looks
correct, so I will use it.

Wei.

> 
> Jan

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