[Xen-devel] [PATCH v4 0/7] More Hyper-V infrastructure

[Xen-devel] [PATCH v4 0/7] More Hyper-V infrastructure

[Wei Liu]

This patch sereis implements several important functionalities to run
Xen on top of Hyper-V.

See individual patches for more details. The first patch adds an
executable fixmap facility, which is x86 generic. The rest of the series
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 <paul@xen.org>

Wei Liu (7):
  x86: provide executable fixmap facility
  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

 xen/arch/x86/boot/x86_64.S               |  10 +-
 xen/arch/x86/e820.c                      |  41 ++++++--
 xen/arch/x86/guest/hyperv/hyperv.c       | 119 ++++++++++++++++++++++-
 xen/arch/x86/guest/hyperv/private.h      |  31 ++++++
 xen/arch/x86/livepatch.c                 |   3 +-
 xen/arch/x86/mm.c                        |   9 ++
 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             |  28 ++++++
 xen/include/asm-x86/guest/hyperv-hcall.h |  98 +++++++++++++++++++
 xen/include/asm-x86/guest/hyperv-tlfs.h  |   5 +-
 12 files changed, 334 insertions(+), 17 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 v4 1/7] 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 livepatch range accordingly.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
 xen/arch/x86/boot/x86_64.S   | 10 +++++++++-
 xen/arch/x86/livepatch.c     |  3 ++-
 xen/arch/x86/mm.c            |  9 +++++++++
 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 | 28 ++++++++++++++++++++++++++++
 7 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 1cbf5acdfb..605d01f1dd 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -85,7 +85,15 @@ GLOBAL(l2_directmap)
  * 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 654190e9e9..aabe1a4c64 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;
 
@@ -5763,6 +5765,13 @@ void __set_fixmap(
     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);
+    map_pages_to_xen(__fix_x_to_virt(idx), _mfn(mfn), 1, flags);
+}
+
 void *__init arch_vmap_virt_end(void)
 {
     return fix_to_virt(__end_of_fixed_addresses);
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index c9d1ab4423..2da42fb691 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -640,7 +640,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..cbc5701214 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 -
+                          MAX_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..4fa56ea0a9 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 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..c2a9d2b50a 100644
--- a/xen/include/asm-x86/fixmap.h
+++ b/xen/include/asm-x86/fixmap.h
@@ -15,6 +15,9 @@
 #include <asm/page.h>
 
 #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
+#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
+/* This constant is derived from enum fixed_addresses_x below */
+#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT)
 
 #ifndef __ASSEMBLY__
 
@@ -89,6 +92,31 @@ 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 __virt_to_fix_x(x) ((FIXADDR_X_TOP - ((x)&PAGE_MASK)) >> 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

Re: [Xen-devel] [PATCH v4 1/7] x86: provide executable fixmap facility

[Andrew Cooper]

On 22/01/2020 20:23, Wei Liu wrote:
> diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
> index 1cbf5acdfb..605d01f1dd 100644
> --- a/xen/arch/x86/boot/x86_64.S
> +++ b/xen/arch/x86/boot/x86_64.S
> @@ -85,7 +85,15 @@ GLOBAL(l2_directmap)
>   * 4k page.
>   */

Adjust this comment as well?

> diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
> index d0cfbb70a8..4fa56ea0a9 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 and executable fixmap (1GB). */

And per-cpu stubs.  Might as well fix the comment while editing.

>  #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..c2a9d2b50a 100644
> --- a/xen/include/asm-x86/fixmap.h
> +++ b/xen/include/asm-x86/fixmap.h
> @@ -15,6 +15,9 @@
>  #include <asm/page.h>
>  
>  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
> +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
> +/* This constant is derived from enum fixed_addresses_x below */
> +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT)

Answering slightly out of order, for clarity:

FIXADDR_X_SIZE should be 0 or 1 by the end of this patch.

As for MAX_FIXADDR_X_SIZE, how about simply
IS_ENABLED(CONFIG_HYPERV_GUEST) ?  That should work, even in a linker
script.

Somewhere, there should be a BUILD_BUG_ON() cross-checking
MAX_FIXADDR_X_SIZE and __end_of_fixed_addresses_x.  We don't yet have a
build_assertions() in x86/mm.c, so I guess now is the time to gain one.

>  
>  #ifndef __ASSEMBLY__
>  
> @@ -89,6 +92,31 @@ 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)

-1, seeing as 0 is reserved.

> +#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 __virt_to_fix_x(x) ((FIXADDR_X_TOP - ((x)&PAGE_MASK)) >> PAGE_SHIFT)

The &PAGE_MASK is redundant, given the following shift, but can't be
optimised out because of its effect on the high 12 bits of the address
as well.  These helpers aren't safe to wild inputs, even with the
&PAGE_MASK, so I'd just drop it.

Otherwise, LGTM.  There is some cleanup we ought to do to the fixmap
infrastructure, but that isn't appropriate for this series.

~Andrew

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

Re: [Xen-devel] [PATCH v4 1/7] x86: provide executable fixmap facility

[Wei Liu]

On Wed, Jan 22, 2020 at 08:56:55PM +0000, Andrew Cooper wrote:
> On 22/01/2020 20:23, Wei Liu wrote:
> > diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
> > index 1cbf5acdfb..605d01f1dd 100644
> > --- a/xen/arch/x86/boot/x86_64.S
> > +++ b/xen/arch/x86/boot/x86_64.S
> > @@ -85,7 +85,15 @@ GLOBAL(l2_directmap)
> >   * 4k page.
> >   */
> 
> Adjust this comment as well?

I thought it was still accurate, so I didn't touch it.

Now it reads:

/*
 * L2 mapping the Xen text/data/bss region, constructed dynamically.
 * Executable fixmap region is hooked up statically.
 * Uses 1x * 4k page.
 */

Does this sound good to you?

> 
> > diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
> > index d0cfbb70a8..4fa56ea0a9 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 and executable fixmap (1GB). */
> 
> And per-cpu stubs.  Might as well fix the comment while editing.

Ack.

> 
> >  #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..c2a9d2b50a 100644
> > --- a/xen/include/asm-x86/fixmap.h
> > +++ b/xen/include/asm-x86/fixmap.h
> > @@ -15,6 +15,9 @@
> >  #include <asm/page.h>
> >  
> >  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
> > +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
> > +/* This constant is derived from enum fixed_addresses_x below */
> > +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT)
> 
> Answering slightly out of order, for clarity:
> 
> FIXADDR_X_SIZE should be 0 or 1 by the end of this patch.
> 
> As for MAX_FIXADDR_X_SIZE, how about simply
> IS_ENABLED(CONFIG_HYPERV_GUEST) ?  That should work, even in a linker
> script.
> 

It should be at least 1<<PAGE_SHIFT because __end_of_fixed_addresses_x
is at least 1.

So for now it can be

#define MAX_FIXADDR_X_SIZE ((IS_ENABLED(CONFIG_HYPERV_GUEST) + 1) << PAGE_SHIFT) 

> Somewhere, there should be a BUILD_BUG_ON() cross-checking
> MAX_FIXADDR_X_SIZE and __end_of_fixed_addresses_x.  We don't yet have a
> build_assertions() in x86/mm.c, so I guess now is the time to gain one.

No, the build_assertions shouldn't be necessary. 

MAX_FIXADDR_X_SIZE is intentionally set to the maximum possible value,
while __end_of_fixed_addresses_x is subject to change according to
configuration. They can differ.

Unless you're talking about adding CONFIG_HYPERV_GUEST to
MAX_FIXADDR_X_SIZE like I mentioned above?

> 
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > @@ -89,6 +92,31 @@ 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)
> 
> -1, seeing as 0 is reserved.
> 

What does -1 mean here?

> > +#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 __virt_to_fix_x(x) ((FIXADDR_X_TOP - ((x)&PAGE_MASK)) >> PAGE_SHIFT)
> 
> The &PAGE_MASK is redundant, given the following shift, but can't be
> optimised out because of its effect on the high 12 bits of the address
> as well.  These helpers aren't safe to wild inputs, even with the
> &PAGE_MASK, so I'd just drop it.
> 

OK. I will drop it.

Wei.

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

Re: [Xen-devel] [PATCH v4 1/7] x86: provide executable fixmap facility

[Jan Beulich]

On 22.01.2020 21:23, Wei Liu wrote:
> 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 livepatch range accordingly.

Hmm, the livepatch range gets shrunk, not shifted, but yes. Is there
a particular reason why you move the stubs area down? It looks as if
the patch would be smaller overall if you didn't. (Possibly down
the road the stubs area could be made part of the FIXADDR_X range
anyway.)

> @@ -5763,6 +5765,13 @@ void __set_fixmap(
>      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);

Also check for FIX_X_RESERVED?

> --- a/xen/include/asm-x86/fixmap.h
> +++ b/xen/include/asm-x86/fixmap.h
> @@ -15,6 +15,9 @@
>  #include <asm/page.h>
>  
>  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
> +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
> +/* This constant is derived from enum fixed_addresses_x below */
> +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT)

If this can't be properly derived, then a BUILD_BUG_ON() is needed.
But didn't we discuss on irc already possible approaches of how to
derive it from the enum? Did none of this work?

> @@ -89,6 +92,31 @@ 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)

Can't __set_fixmap() be used here, making its implementation derive
which one is mean from whether _PAGE_NX is set in the passed in flags?

Jan

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

Re: [Xen-devel] [PATCH v4 1/7] x86: provide executable fixmap facility

[Wei Liu]

On Thu, Jan 23, 2020 at 12:04:00PM +0100, Jan Beulich wrote:
> On 22.01.2020 21:23, Wei Liu wrote:
> > 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 livepatch range accordingly.
> 
> Hmm, the livepatch range gets shrunk, not shifted, but yes. Is there
> a particular reason why you move the stubs area down? It looks as if
> the patch would be smaller overall if you didn't. (Possibly down
> the road the stubs area could be made part of the FIXADDR_X range
> anyway.)

I think having a well-known fixed address is more useful for debugging.

Going the other way around would mean the hypercall page location
becomes dependent on the number of CPUs configured.

> 
> > @@ -5763,6 +5765,13 @@ void __set_fixmap(
> >      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);
> 
> Also check for FIX_X_RESERVED?

Ack. In that case the same should be done for __set_fixmap.

> 
> > --- a/xen/include/asm-x86/fixmap.h
> > +++ b/xen/include/asm-x86/fixmap.h
> > @@ -15,6 +15,9 @@
> >  #include <asm/page.h>
> >  
> >  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
> > +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
> > +/* This constant is derived from enum fixed_addresses_x below */
> > +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT)
> 
> If this can't be properly derived, then a BUILD_BUG_ON() is needed.
> But didn't we discuss on irc already possible approaches of how to
> derive it from the enum? Did none of this work?
> 

The only option I remember discussing was to define macros instead of
using enum. I said at the time at would make us lose the ability to
dynamically size this area.

If there are other ways that I missed, let me know.

> > @@ -89,6 +92,31 @@ 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)
> 
> Can't __set_fixmap() be used here, making its implementation derive
> which one is mean from whether _PAGE_NX is set in the passed in flags?

__set_fixmap and __set_fixmap_x take different enum types for their
first argument. I would prefer type safety and explicitness here.

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v4 1/7] x86: provide executable fixmap facility

[Jan Beulich]

On 28.01.2020 16:15, Wei Liu wrote:
> On Thu, Jan 23, 2020 at 12:04:00PM +0100, Jan Beulich wrote:
>> On 22.01.2020 21:23, Wei Liu wrote:
>>> 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 livepatch range accordingly.
>>
>> Hmm, the livepatch range gets shrunk, not shifted, but yes. Is there
>> a particular reason why you move the stubs area down? It looks as if
>> the patch would be smaller overall if you didn't. (Possibly down
>> the road the stubs area could be made part of the FIXADDR_X range
>> anyway.)
> 
> I think having a well-known fixed address is more useful for debugging.
> 
> Going the other way around would mean the hypercall page location
> becomes dependent on the number of CPUs configured.

Depending on how future insertions are done into
enum fixed_addresses_x, the address also won't be "well-known fixed".

>>> --- a/xen/include/asm-x86/fixmap.h
>>> +++ b/xen/include/asm-x86/fixmap.h
>>> @@ -15,6 +15,9 @@
>>>  #include <asm/page.h>
>>>  
>>>  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
>>> +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
>>> +/* This constant is derived from enum fixed_addresses_x below */
>>> +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT)
>>
>> If this can't be properly derived, then a BUILD_BUG_ON() is needed.
>> But didn't we discuss on irc already possible approaches of how to
>> derive it from the enum? Did none of this work?
> 
> The only option I remember discussing was to define macros instead of
> using enum. I said at the time at would make us lose the ability to
> dynamically size this area.
> 
> If there are other ways that I missed, let me know.

I seem to recall recommending to export absolute symbols from
assembly code. The question is how easily usable they would
be from C, or how clumsy the resulting code would look.

>>> @@ -89,6 +92,31 @@ 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)
>>
>> Can't __set_fixmap() be used here, making its implementation derive
>> which one is mean from whether _PAGE_NX is set in the passed in flags?
> 
> __set_fixmap and __set_fixmap_x take different enum types for their
> first argument. I would prefer type safety and explicitness here.

Well, okay then. Duplication like this simply makes me a little
nervous, and even more so when it extends our set of name space
violations.

Jan

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

Re: [Xen-devel] [PATCH v4 1/7] x86: provide executable fixmap facility

[Wei Liu]

On Tue, Jan 28, 2020 at 04:38:42PM +0100, Jan Beulich wrote:
> On 28.01.2020 16:15, Wei Liu wrote:
> > On Thu, Jan 23, 2020 at 12:04:00PM +0100, Jan Beulich wrote:
> >> On 22.01.2020 21:23, Wei Liu wrote:
> >>> 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 livepatch range accordingly.
> >>
> >> Hmm, the livepatch range gets shrunk, not shifted, but yes. Is there
> >> a particular reason why you move the stubs area down? It looks as if
> >> the patch would be smaller overall if you didn't. (Possibly down
> >> the road the stubs area could be made part of the FIXADDR_X range
> >> anyway.)
> > 
> > I think having a well-known fixed address is more useful for debugging.
> > 
> > Going the other way around would mean the hypercall page location
> > becomes dependent on the number of CPUs configured.
> 
> Depending on how future insertions are done into
> enum fixed_addresses_x, the address also won't be "well-known fixed".

Going back to this, not moving stubs will make the change to
alloc_stub_page become unnecessary (one line); on the other hand it
makes FIX_X_ADDR_START become XEN_VIRT_END - NR_CPUS * PAGE_SIZE -
PAGE_SIZE.

Are you really concerned about this? I can make the change if you really
want that, but it is just work with no apparent benefit.

> 
> >>> --- a/xen/include/asm-x86/fixmap.h
> >>> +++ b/xen/include/asm-x86/fixmap.h
> >>> @@ -15,6 +15,9 @@
> >>>  #include <asm/page.h>
> >>>  
> >>>  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
> >>> +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
> >>> +/* This constant is derived from enum fixed_addresses_x below */
> >>> +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT)
> >>
> >> If this can't be properly derived, then a BUILD_BUG_ON() is needed.
> >> But didn't we discuss on irc already possible approaches of how to
> >> derive it from the enum? Did none of this work?
> > 
> > The only option I remember discussing was to define macros instead of
> > using enum. I said at the time at would make us lose the ability to
> > dynamically size this area.
> > 
> > If there are other ways that I missed, let me know.
> 
> I seem to recall recommending to export absolute symbols from
> assembly code. The question is how easily usable they would
> be from C, or how clumsy the resulting code would look.

Even if I use absolute symbol I would still need to define a macro for
it. There is no way around it, because enum can't be used in asm or
linker script.

I want to keep using enum because that would allow us to size the area
according to Kconfig.

Wei.

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

Re: [Xen-devel] [PATCH v4 1/7] x86: provide executable fixmap facility

[Jan Beulich]

On 29.01.2020 15:42, Wei Liu wrote:
> On Tue, Jan 28, 2020 at 04:38:42PM +0100, Jan Beulich wrote:
>> On 28.01.2020 16:15, Wei Liu wrote:
>>> On Thu, Jan 23, 2020 at 12:04:00PM +0100, Jan Beulich wrote:
>>>> On 22.01.2020 21:23, Wei Liu wrote:
>>>>> 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 livepatch range accordingly.
>>>>
>>>> Hmm, the livepatch range gets shrunk, not shifted, but yes. Is there
>>>> a particular reason why you move the stubs area down? It looks as if
>>>> the patch would be smaller overall if you didn't. (Possibly down
>>>> the road the stubs area could be made part of the FIXADDR_X range
>>>> anyway.)
>>>
>>> I think having a well-known fixed address is more useful for debugging.
>>>
>>> Going the other way around would mean the hypercall page location
>>> becomes dependent on the number of CPUs configured.
>>
>> Depending on how future insertions are done into
>> enum fixed_addresses_x, the address also won't be "well-known fixed".
> 
> Going back to this, not moving stubs will make the change to
> alloc_stub_page become unnecessary (one line); on the other hand it
> makes FIX_X_ADDR_START become XEN_VIRT_END - NR_CPUS * PAGE_SIZE -
> PAGE_SIZE.
> 
> Are you really concerned about this? I can make the change if you really
> want that, but it is just work with no apparent benefit.

Hmm, indeed, it's just one line. Not sure why I thought there
would be more of an effect. Leave it as is, and sorry for the
noise.

>>>>> --- a/xen/include/asm-x86/fixmap.h
>>>>> +++ b/xen/include/asm-x86/fixmap.h
>>>>> @@ -15,6 +15,9 @@
>>>>>  #include <asm/page.h>
>>>>>  
>>>>>  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
>>>>> +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
>>>>> +/* This constant is derived from enum fixed_addresses_x below */
>>>>> +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT)
>>>>
>>>> If this can't be properly derived, then a BUILD_BUG_ON() is needed.
>>>> But didn't we discuss on irc already possible approaches of how to
>>>> derive it from the enum? Did none of this work?
>>>
>>> The only option I remember discussing was to define macros instead of
>>> using enum. I said at the time at would make us lose the ability to
>>> dynamically size this area.
>>>
>>> If there are other ways that I missed, let me know.
>>
>> I seem to recall recommending to export absolute symbols from
>> assembly code. The question is how easily usable they would
>> be from C, or how clumsy the resulting code would look.
> 
> Even if I use absolute symbol I would still need to define a macro for
> it. There is no way around it, because enum can't be used in asm or
> linker script.

I'm afraid I don't understand. Why a macro? The absolute symbol would
be there to communicate the relevant (enum-derived) value to the
linker script. I.e. with

enum { e0, e1, e2 };

in some C file

asm ( ".equ GBL_e2, %c0; .global GBL_e2" :: "i" (e2) );

which I then hope would allow you to use GBL_e2 in the linker
script ASSERT().

> I want to keep using enum because that would allow us to size the area
> according to Kconfig.

Of course, I fully agree with this goal.

Jan

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

Re: [Xen-devel] [PATCH v4 1/7] x86: provide executable fixmap facility

[Wei Liu]

On Wed, Jan 29, 2020 at 03:59:32PM +0100, Jan Beulich wrote:
[...]
> >> I seem to recall recommending to export absolute symbols from
> >> assembly code. The question is how easily usable they would
> >> be from C, or how clumsy the resulting code would look.
> > 
> > Even if I use absolute symbol I would still need to define a macro for
> > it. There is no way around it, because enum can't be used in asm or
> > linker script.
> 
> I'm afraid I don't understand. Why a macro? The absolute symbol would
> be there to communicate the relevant (enum-derived) value to the
> linker script. I.e. with
> 
> enum { e0, e1, e2 };
> 
> in some C file
> 
> asm ( ".equ GBL_e2, %c0; .global GBL_e2" :: "i" (e2) );
> 
> which I then hope would allow you to use GBL_e2 in the linker
> script ASSERT().
> 

OK. Let me see if this is possible.

Wei.

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

[Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Wei Liu]

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 0x300 as the
OS type.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
XXX the decision on Xen's vendor ID is pending.

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

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 082f9928a1..5a4ef27a0b 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -36,6 +36,22 @@ boolean_param("e820-verbose", e820_verbose);
 struct e820map e820;
 struct e820map __initdata e820_raw;
 
+static unsigned int find_phys_addr_bits(void)
+{
+    uint32_t eax;
+    unsigned int phys_bits = 36;
+
+    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;
+    }
+
+    return phys_bits;
+}
+
 /*
  * This function checks if the entire range <start,end> is mapped with type.
  *
@@ -357,6 +373,21 @@ static unsigned long __init find_max_pfn(void)
             max_pfn = end;
     }
 
+#ifdef CONFIG_HYPERV_GUEST
+    {
+	/*
+	 * We reserve the top-most page for hypercall page. Adjust
+	 * max_pfn if necessary.
+	 */
+        unsigned int phys_bits = find_phys_addr_bits();
+        unsigned long hcall_pfn =
+          ((1ull << phys_bits) - 1) >> PAGE_SHIFT;
+
+        if ( max_pfn >= hcall_pfn )
+          max_pfn = hcall_pfn - 1;
+    }
+#endif
+
     return max_pfn;
 }
 
@@ -420,7 +451,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, phys_bits;
 
     /* By default we check only Intel systems. */
     if ( e820_mtrr_clip == -1 )
@@ -446,13 +477,7 @@ static uint64_t __init mtrr_top_of_ram(void)
          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;
-    }
+    phys_bits = find_phys_addr_bits();
     addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
 
     rdmsrl(MSR_MTRRcap, mtrr_cap);
diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 8d38313d7a..f986c1a805 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -18,17 +18,27 @@
  *
  * Copyright (c) 2019 Microsoft.
  */
+#include <xen/version.h>
 #include <xen/init.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 = 0;
+
+    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)
 {
     uint32_t eax, ebx, ecx, edx;
@@ -72,6 +82,43 @@ 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 = ((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT;
+        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;
+    }
+
+    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..5d37efd2d2 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 0x100, FreeBSD 0x200, Xen 0x300
  * 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;
-- 
2.20.1


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

Re: [Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Andrew Cooper]

On 22/01/2020 20:23, Wei Liu wrote:
> 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 0x300 as the
> OS type.
>
> Signed-off-by: Wei Liu <liuwe@microsoft.com>
> ---
> XXX the decision on Xen's vendor ID is pending.

Presumably this is pending a published update to the TLFS?  (And I
presume using 0x8088 is out of the question?  That is an X in the bottom
byte, not a reference to an 8 bit microprocessor.)

> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> index 082f9928a1..5a4ef27a0b 100644
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -36,6 +36,22 @@ boolean_param("e820-verbose", e820_verbose);
> @@ -357,6 +373,21 @@ static unsigned long __init find_max_pfn(void)
>              max_pfn = end;
>      }
>  
> +#ifdef CONFIG_HYPERV_GUEST
> +    {
> +	/*
> +	 * We reserve the top-most page for hypercall page. Adjust
> +	 * max_pfn if necessary.

It might be worth leaving a "TODO: Better algorithm/guess?" here.

> +	 */
> +        unsigned int phys_bits = find_phys_addr_bits();
> +        unsigned long hcall_pfn =
> +          ((1ull << phys_bits) - 1) >> PAGE_SHIFT;

(1ull << (phys_bits - PAGE_SHIFT)) - 1 is equivalent, and doesn't
require a right shift.  I don't know if the compiler is smart enough to
make this optimisation automatically.

> +
> +        if ( max_pfn >= hcall_pfn )
> +          max_pfn = hcall_pfn - 1;

Indentation looks weird.

> @@ -446,13 +477,7 @@ static uint64_t __init mtrr_top_of_ram(void)
>           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;
> -    }
> +    phys_bits = find_phys_addr_bits();
>      addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);

Note for whomever is next doing cleanup in this area.  This wants to be
& PAGE_MASK.

> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> index 8d38313d7a..f986c1a805 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -72,6 +82,43 @@ 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 = ((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT;
> +        hypercall_msr.enable = 1;
> +        hypercall_msr.guest_physical_address = mfn;
> +        wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);

Is it worth reading back, and BUG() if it is different?  It will be a
more obvious failure than hypercalls disappearing mysteriously.

> +    } else {
> +        mfn = hypercall_msr.guest_physical_address;
> +    }

Style.

Otherwise, LGTM.

~Andrew

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

Re: [Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Jan Beulich]

On 22.01.2020 22:31, Andrew Cooper wrote:
> On 22/01/2020 20:23, Wei Liu wrote:
>> --- a/xen/arch/x86/e820.c
>> +++ b/xen/arch/x86/e820.c
>> +	 */
>> +        unsigned int phys_bits = find_phys_addr_bits();
>> +        unsigned long hcall_pfn =
>> +          ((1ull << phys_bits) - 1) >> PAGE_SHIFT;
> 
> (1ull << (phys_bits - PAGE_SHIFT)) - 1 is equivalent, and doesn't
> require a right shift.  I don't know if the compiler is smart enough to
> make this optimisation automatically.

It's not allowed to, without having a way to know that phys_bits is
no less than PAGE_SHIFT.

Jan

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

Re: [Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Wei Liu]

On Wed, Jan 22, 2020 at 09:31:52PM +0000, Andrew Cooper wrote:
> On 22/01/2020 20:23, Wei Liu wrote:
> > 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 0x300 as the
> > OS type.
> >
> > Signed-off-by: Wei Liu <liuwe@microsoft.com>
> > ---
> > XXX the decision on Xen's vendor ID is pending.
> 
> Presumably this is pending a published update to the TLFS?  (And I
> presume using 0x8088 is out of the question?  That is an X in the bottom
> byte, not a reference to an 8 bit microprocessor.)

We're discussing internally what Xen (and other OSes) should use.

There will be an update to TLFS for this.

At this point I can say Xen can use the ID I wrote in this patch.

0x8088 is out of the question. :-)

> 
> > diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> > index 082f9928a1..5a4ef27a0b 100644
> > --- a/xen/arch/x86/e820.c
> > +++ b/xen/arch/x86/e820.c
> > @@ -36,6 +36,22 @@ boolean_param("e820-verbose", e820_verbose);
> > @@ -357,6 +373,21 @@ static unsigned long __init find_max_pfn(void)
> >              max_pfn = end;
> >      }
> >  
> > +#ifdef CONFIG_HYPERV_GUEST
> > +    {
> > +	/*
> > +	 * We reserve the top-most page for hypercall page. Adjust
> > +	 * max_pfn if necessary.
> 
> It might be worth leaving a "TODO: Better algorithm/guess?" here.
> 

Ack.

> > +	 */
> > +        unsigned int phys_bits = find_phys_addr_bits();
> > +        unsigned long hcall_pfn =
> > +          ((1ull << phys_bits) - 1) >> PAGE_SHIFT;
> 
> (1ull << (phys_bits - PAGE_SHIFT)) - 1 is equivalent, and doesn't
> require a right shift.  I don't know if the compiler is smart enough to
> make this optimisation automatically.

OK. I can change it.

> 
> > +
> > +        if ( max_pfn >= hcall_pfn )
> > +          max_pfn = hcall_pfn - 1;
> 
> Indentation looks weird.
> 

I blame Emacs.

> > @@ -446,13 +477,7 @@ static uint64_t __init mtrr_top_of_ram(void)
> >           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;
> > -    }
> > +    phys_bits = find_phys_addr_bits();
> >      addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
> 
> Note for whomever is next doing cleanup in this area.  This wants to be
> & PAGE_MASK.
> 

Ack.

> > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> > index 8d38313d7a..f986c1a805 100644
> > --- a/xen/arch/x86/guest/hyperv/hyperv.c
> > +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> > @@ -72,6 +82,43 @@ 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 = ((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT;
> > +        hypercall_msr.enable = 1;
> > +        hypercall_msr.guest_physical_address = mfn;
> > +        wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> 
> Is it worth reading back, and BUG() if it is different?  It will be a
> more obvious failure than hypercalls disappearing mysteriously.
> 

Sure. I don't expect that BUG_ON to trigger in practice though.

> > +    } else {
> > +        mfn = hypercall_msr.guest_physical_address;
> > +    }
> 
> Style.
> 

Will fix.

Wei.

> Otherwise, LGTM.
> 
> ~Andrew

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

Re: [Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Michael Kelley]

From: Wei Liu <wei.liu.xen@gmail.com> On Behalf Of Wei Liu  Sent: Wednesday, January 22, 2020 12:24 PM
> 
> 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 0x300 as the
> OS type.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>
> ---
> XXX the decision on Xen's vendor ID is pending.
> 
> v4:
> 1. Use fixmap
> 2. Follow routines listed in TLFS
> ---
>  xen/arch/x86/e820.c                     | 41 +++++++++++++++----
>  xen/arch/x86/guest/hyperv/hyperv.c      | 53 +++++++++++++++++++++++--
>  xen/include/asm-x86/guest/hyperv-tlfs.h |  5 ++-
>  3 files changed, 86 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> index 082f9928a1..5a4ef27a0b 100644
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -36,6 +36,22 @@ boolean_param("e820-verbose", e820_verbose);
>  struct e820map e820;
>  struct e820map __initdata e820_raw;
> 
> +static unsigned int find_phys_addr_bits(void)
> +{
> +    uint32_t eax;
> +    unsigned int phys_bits = 36;
> +
> +    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;
> +    }
> +
> +    return phys_bits;
> +}
> +
>  /*
>   * This function checks if the entire range <start,end> is mapped with type.
>   *
> @@ -357,6 +373,21 @@ static unsigned long __init find_max_pfn(void)
>              max_pfn = end;
>      }
> 
> +#ifdef CONFIG_HYPERV_GUEST
> +    {
> +	/*
> +	 * We reserve the top-most page for hypercall page. Adjust
> +	 * max_pfn if necessary.
> +	 */
> +        unsigned int phys_bits = find_phys_addr_bits();
> +        unsigned long hcall_pfn =
> +          ((1ull << phys_bits) - 1) >> PAGE_SHIFT;
> +
> +        if ( max_pfn >= hcall_pfn )
> +          max_pfn = hcall_pfn - 1;
> +    }
> +#endif
> +
>      return max_pfn;
>  }
> 
> @@ -420,7 +451,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, phys_bits;
> 
>      /* By default we check only Intel systems. */
>      if ( e820_mtrr_clip == -1 )
> @@ -446,13 +477,7 @@ static uint64_t __init mtrr_top_of_ram(void)
>           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;
> -    }
> +    phys_bits = find_phys_addr_bits();
>      addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
> 
>      rdmsrl(MSR_MTRRcap, mtrr_cap);
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> index 8d38313d7a..f986c1a805 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -18,17 +18,27 @@
>   *
>   * Copyright (c) 2019 Microsoft.
>   */
> +#include <xen/version.h>
>  #include <xen/init.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 = 0;
> +
> +    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)
>  {
>      uint32_t eax, ebx, ecx, edx;
> @@ -72,6 +82,43 @@ 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 = ((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT;
> +        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;
> +    }
> +
> +    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..5d37efd2d2 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 0x100, FreeBSD 0x200, Xen 0x300

This comment isn't quite right -- it reflects the mistake in the
TLFS that is being corrected.  The field 62:56 is only 7 bits wide,
so 0x100, 0x200, etc. won't fit.  The actual values are: Linux 0x1,
FreeBSD 0x2, and Xen 0x3.  Then bits 55:48 are 0x00.

>   * 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;
> --
> 2.20.1


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

Re: [Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Wei Liu]

On Thu, Jan 23, 2020 at 01:35:22AM +0000, Michael Kelley wrote:
[...]
> > diff --git a/xen/include/asm-x86/guest/hyperv-tlfs.h b/xen/include/asm-
> > x86/guest/hyperv-tlfs.h
> > index 05c4044976..5d37efd2d2 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 0x100, FreeBSD 0x200, Xen 0x300
> 
> This comment isn't quite right -- it reflects the mistake in the
> TLFS that is being corrected.  The field 62:56 is only 7 bits wide,
> so 0x100, 0x200, etc. won't fit.  The actual values are: Linux 0x1,
> FreeBSD 0x2, and Xen 0x3.  Then bits 55:48 are 0x00.

Thanks Michael.

I will fix this section (and perhaps submit a patch for Linux as well).

Wei.

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

Re: [Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Jan Beulich]

On 22.01.2020 21:23, Wei Liu wrote:
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -36,6 +36,22 @@ boolean_param("e820-verbose", e820_verbose);
>  struct e820map e820;
>  struct e820map __initdata e820_raw;
>  
> +static unsigned int find_phys_addr_bits(void)
> +{
> +    uint32_t eax;
> +    unsigned int phys_bits = 36;
> +
> +    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;
> +    }
> +
> +    return phys_bits;
> +}

Instead of this, how about pulling further ahead the call to
early_cpu_init() in setup.c? (Otherwise the function wants to
be __init at least.)

> @@ -357,6 +373,21 @@ static unsigned long __init find_max_pfn(void)
>              max_pfn = end;
>      }
>  
> +#ifdef CONFIG_HYPERV_GUEST
> +    {
> +	/*
> +	 * We reserve the top-most page for hypercall page. Adjust
> +	 * max_pfn if necessary.
> +	 */
> +        unsigned int phys_bits = find_phys_addr_bits();
> +        unsigned long hcall_pfn =
> +          ((1ull << phys_bits) - 1) >> PAGE_SHIFT;
> +
> +        if ( max_pfn >= hcall_pfn )
> +          max_pfn = hcall_pfn - 1;
> +    }
> +#endif

This wants abstracting away: There shouldn't be Hyper-V specific
code in here if at all possible, and the adjustment also shouldn't
be made unless actually running on Hyper-V.

> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -18,17 +18,27 @@
>   *
>   * Copyright (c) 2019 Microsoft.
>   */
> +#include <xen/version.h>
>  #include <xen/init.h>

Please sort alphabetically.

> +#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 = 0;

Pointless initializer.

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

Blank line between these two please (if you can't get away without
this declaration in the first place).

> @@ -72,6 +82,43 @@ 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 = ((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT;

Along the lines of the abstracting-away request above: How is
anyone to notice what else needs changing if it is decided
that this page gets moved elsewhere?

> +        hypercall_msr.enable = 1;
> +        hypercall_msr.guest_physical_address = mfn;
> +        wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);

So on Hyper-V the hypervisor magically populates this page as
a side effect of the MSR write?

Jan

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

Re: [Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Michael Kelley]

From: Jan Beulich <jbeulich@suse.com> Sent: Thursday, January 23, 2020 3:19 AM
> 
> On 22.01.2020 21:23, Wei Liu wrote:
> > --- a/xen/arch/x86/e820.c
> > +++ b/xen/arch/x86/e820.c
> > @@ -36,6 +36,22 @@ boolean_param("e820-verbose", e820_verbose);
> >  struct e820map e820;
> >  struct e820map __initdata e820_raw;
> >
> > +static unsigned int find_phys_addr_bits(void)
> > +{
> > +    uint32_t eax;
> > +    unsigned int phys_bits = 36;
> > +
> > +    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;
> > +    }
> > +
> > +    return phys_bits;
> > +}
> 
> Instead of this, how about pulling further ahead the call to
> early_cpu_init() in setup.c? (Otherwise the function wants to
> be __init at least.)
> 
> > @@ -357,6 +373,21 @@ static unsigned long __init find_max_pfn(void)
> >              max_pfn = end;
> >      }
> >
> > +#ifdef CONFIG_HYPERV_GUEST
> > +    {
> > +	/*
> > +	 * We reserve the top-most page for hypercall page. Adjust
> > +	 * max_pfn if necessary.
> > +	 */
> > +        unsigned int phys_bits = find_phys_addr_bits();
> > +        unsigned long hcall_pfn =
> > +          ((1ull << phys_bits) - 1) >> PAGE_SHIFT;
> > +
> > +        if ( max_pfn >= hcall_pfn )
> > +          max_pfn = hcall_pfn - 1;
> > +    }
> > +#endif
> 
> This wants abstracting away: There shouldn't be Hyper-V specific
> code in here if at all possible, and the adjustment also shouldn't
> be made unless actually running on Hyper-V.
> 
> > --- a/xen/arch/x86/guest/hyperv/hyperv.c
> > +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> > @@ -18,17 +18,27 @@
> >   *
> >   * Copyright (c) 2019 Microsoft.
> >   */
> > +#include <xen/version.h>
> >  #include <xen/init.h>
> 
> Please sort alphabetically.
> 
> > +#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 = 0;
> 
> Pointless initializer.
> 
> > +    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)
> 
> Blank line between these two please (if you can't get away without
> this declaration in the first place).
> 
> > @@ -72,6 +82,43 @@ 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 = ((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT;
> 
> Along the lines of the abstracting-away request above: How is
> anyone to notice what else needs changing if it is decided
> that this page gets moved elsewhere?
> 
> > +        hypercall_msr.enable = 1;
> > +        hypercall_msr.guest_physical_address = mfn;
> > +        wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> 
> So on Hyper-V the hypervisor magically populates this page as
> a side effect of the MSR write?
> 

Yes, that's exactly what happens. :-)  Hyper-V takes the guest
physical address and remaps it to a different physical page that
Hyper-V has set up with whatever is needed to execute the hypercall
sequence. The original physical page corresponding to the guest physical
address is not modified, but it remains unused and inaccessible to the
guest.  When/if the MSR is written again with the enable flag set to 0,
the mapping is flipped back, and the original physical page, with its
original contents, reappears. There are a few other pages with this
same behavior.  The Hyper-V TLFS refers to these "GPA Overlay
Pages".  See Section 5.2.1 of the TLFS.

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

Re: [Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Wei Liu]

On Thu, Jan 23, 2020 at 12:18:41PM +0100, Jan Beulich wrote:
> On 22.01.2020 21:23, Wei Liu wrote:
> > --- a/xen/arch/x86/e820.c
> > +++ b/xen/arch/x86/e820.c
> > @@ -36,6 +36,22 @@ boolean_param("e820-verbose", e820_verbose);
> >  struct e820map e820;
> >  struct e820map __initdata e820_raw;
> >  
> > +static unsigned int find_phys_addr_bits(void)
> > +{
> > +    uint32_t eax;
> > +    unsigned int phys_bits = 36;
> > +
> > +    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;
> > +    }
> > +
> > +    return phys_bits;
> > +}
> 
> Instead of this, how about pulling further ahead the call to
> early_cpu_init() in setup.c? (Otherwise the function wants to
> be __init at least.)

I can certainly try that, but that would require modifying e820.c
nonetheless because we can drop the cpuid invocation here if the move is
successful.

> 
> > @@ -357,6 +373,21 @@ static unsigned long __init find_max_pfn(void)
> >              max_pfn = end;
> >      }
> >  
> > +#ifdef CONFIG_HYPERV_GUEST
> > +    {
> > +	/*
> > +	 * We reserve the top-most page for hypercall page. Adjust
> > +	 * max_pfn if necessary.
> > +	 */
> > +        unsigned int phys_bits = find_phys_addr_bits();
> > +        unsigned long hcall_pfn =
> > +          ((1ull << phys_bits) - 1) >> PAGE_SHIFT;
> > +
> > +        if ( max_pfn >= hcall_pfn )
> > +          max_pfn = hcall_pfn - 1;
> > +    }
> > +#endif
> 
> This wants abstracting away: There shouldn't be Hyper-V specific
> code in here if at all possible, and the adjustment also shouldn't
> be made unless actually running on Hyper-V.
> 

I will introduce a hook called hypervisor_reserve_top_pages.

> > --- a/xen/arch/x86/guest/hyperv/hyperv.c
> > +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> > @@ -18,17 +18,27 @@
[...]
> > @@ -72,6 +82,43 @@ 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 = ((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT;
> 
> Along the lines of the abstracting-away request above: How is
> anyone to notice what else needs changing if it is decided
> that this page gets moved elsewhere?

I don't have a good answer to this other than documenting. It is
probably as fragile as livepatch or pcpu stub.

> 
> > +        hypercall_msr.enable = 1;
> > +        hypercall_msr.guest_physical_address = mfn;
> > +        wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> 
> So on Hyper-V the hypervisor magically populates this page as
> a side effect of the MSR write?

Yes. See Michael's reply.

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Jan Beulich]

On 28.01.2020 16:30, Wei Liu wrote:
> On Thu, Jan 23, 2020 at 12:18:41PM +0100, Jan Beulich wrote:
>> On 22.01.2020 21:23, Wei Liu wrote:
>>> --- a/xen/arch/x86/e820.c
>>> +++ b/xen/arch/x86/e820.c
>>> @@ -36,6 +36,22 @@ boolean_param("e820-verbose", e820_verbose);
>>>  struct e820map e820;
>>>  struct e820map __initdata e820_raw;
>>>  
>>> +static unsigned int find_phys_addr_bits(void)
>>> +{
>>> +    uint32_t eax;
>>> +    unsigned int phys_bits = 36;
>>> +
>>> +    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;
>>> +    }
>>> +
>>> +    return phys_bits;
>>> +}
>>
>> Instead of this, how about pulling further ahead the call to
>> early_cpu_init() in setup.c? (Otherwise the function wants to
>> be __init at least.)
> 
> I can certainly try that, but that would require modifying e820.c
> nonetheless because we can drop the cpuid invocation here if the move is
> successful.

Right, but this could then be a separate, follow-on cleanup
patch aiui.

>>> --- a/xen/arch/x86/guest/hyperv/hyperv.c
>>> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
>>> @@ -18,17 +18,27 @@
> [...]
>>> @@ -72,6 +82,43 @@ 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 = ((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT;
>>
>> Along the lines of the abstracting-away request above: How is
>> anyone to notice what else needs changing if it is decided
>> that this page gets moved elsewhere?
> 
> I don't have a good answer to this other than documenting. It is
> probably as fragile as livepatch or pcpu stub.

At least macro-ize it then, so that all use sites can be easily
identified.

Jan

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

[Xen-devel] [PATCH v4 3/7] 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>
---
v4:
1. Adjust code due to previous patch has changed
2. Address comments
---
 xen/include/asm-x86/guest/hyperv-hcall.h | 98 ++++++++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100644 xen/include/asm-x86/guest/hyperv-hcall.h

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..509e57f481
--- /dev/null
+++ b/xen/include/asm-x86/guest/hyperv-hcall.h
@@ -0,0 +1,98 @@
+/******************************************************************************
+ * 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 ("INDIRECT_CALL %P[hcall_page]"
+                  : "=a" (status), "+c" (control),
+                    "+d" (input_addr) ASM_CALL_CONSTRAINT
+                  : "r" (r8),
+                    [hcall_page] "p" (fix_x_to_virt(FIX_X_HYPERV_HCALL))
+                  : "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 ("INDIRECT_CALL %P[hcall_page]"
+                  : "=a" (status), "+c" (control),
+                    "+d" (input1) ASM_CALL_CONSTRAINT
+                  : "r" (r8),
+                    [hcall_page] "p" (fix_x_to_virt(FIX_X_HYPERV_HCALL))
+                  :);
+
+    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_COMP_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

Re: [Xen-devel] [PATCH v4 3/7] x86/hyperv: provide Hyper-V hypercall functions

[Andrew Cooper]

On 22/01/2020 20:23, Wei Liu wrote:
> These functions will be used later to make hypercalls to Hyper-V.
>
> Signed-off-by: Wei Liu <liuwe@microsoft.com>

After some experimentation,

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index cbc5701214..3708a60b5c 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -329,6 +329,8 @@ SECTIONS
   efi = .;
 #endif
 
+  hv_hcall_page = ABSOLUTE(0xffff82d0bfffe000);
+
   /* Sections to be discarded */
   /DISCARD/ : {
        *(.exit.text)

in the linker script lets direct calls work correctly:

ffff82d080637935:       b9 01 00 00 40          mov    $0x40000001,%ecx
ffff82d08063793a:       0f 30                   wrmsr 
ffff82d08063793c:       ba 21 03 00 00          mov    $0x321,%edx
ffff82d080637941:       bf 01 00 00 00          mov    $0x1,%edi
ffff82d080637946:       e8 ac 4f c7 ff          callq  ffff82d0802ac8f7
<__set_fixmap_x>
ffff82d08063794b:       41 b8 00 00 00 00       mov    $0x0,%r8d
ffff82d080637951:       b9 ff ff 00 00          mov    $0xffff,%ecx
ffff82d080637956:       ba 00 00 00 00          mov    $0x0,%edx
ffff82d08063795b:       e8 a0 66 9c 3f          callq  ffff82d0bfffe000
<hv_hcall_page>
ffff82d080637960:       66 83 f8 02             cmp    $0x2,%ax

but it does throw:

Difference at .init:00032edf is 0xc0000000 (expected 0x40000000)
Difference at .init:00032edf is 0xc0000000 (expected 0x40000000)

as a diagnostic presumably from the final link  (both with a standard
Debian 2.28 binutils, and upstream 2.33 build).  I'm not sure what its
trying to complain about, as both xen.gz and xen.efi have correctly
generated code.

Depending on whether they are benign or not, a linker-friendly
fix_to_virt() should be all we need to keep these strictly as direct calls.

~Andrew

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

Re: [Xen-devel] [PATCH v4 3/7] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich]

On 22.01.2020 22:57, Andrew Cooper wrote:
> On 22/01/2020 20:23, Wei Liu wrote:
>> These functions will be used later to make hypercalls to Hyper-V.
>>
>> Signed-off-by: Wei Liu <liuwe@microsoft.com>
> 
> After some experimentation,
> 
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index cbc5701214..3708a60b5c 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -329,6 +329,8 @@ SECTIONS
>    efi = .;
>  #endif
>  
> +  hv_hcall_page = ABSOLUTE(0xffff82d0bfffe000);
> +
>    /* Sections to be discarded */
>    /DISCARD/ : {
>         *(.exit.text)
> 
> in the linker script lets direct calls work correctly:
> 
> ffff82d080637935:       b9 01 00 00 40          mov    $0x40000001,%ecx
> ffff82d08063793a:       0f 30                   wrmsr 
> ffff82d08063793c:       ba 21 03 00 00          mov    $0x321,%edx
> ffff82d080637941:       bf 01 00 00 00          mov    $0x1,%edi
> ffff82d080637946:       e8 ac 4f c7 ff          callq  ffff82d0802ac8f7
> <__set_fixmap_x>
> ffff82d08063794b:       41 b8 00 00 00 00       mov    $0x0,%r8d
> ffff82d080637951:       b9 ff ff 00 00          mov    $0xffff,%ecx
> ffff82d080637956:       ba 00 00 00 00          mov    $0x0,%edx
> ffff82d08063795b:       e8 a0 66 9c 3f          callq  ffff82d0bfffe000
> <hv_hcall_page>
> ffff82d080637960:       66 83 f8 02             cmp    $0x2,%ax
> 
> but it does throw:
> 
> Difference at .init:00032edf is 0xc0000000 (expected 0x40000000)
> Difference at .init:00032edf is 0xc0000000 (expected 0x40000000)
> 
> as a diagnostic presumably from the final link  (both with a standard
> Debian 2.28 binutils, and upstream 2.33 build).  I'm not sure what its
> trying to complain about, as both xen.gz and xen.efi have correctly
> generated code.
> 
> Depending on whether they are benign or not, a linker-friendly
> fix_to_virt() should be all we need to keep these strictly as direct calls.

They're benign in the particular case of them actually resulting
from relative CALLs, which hence require no relocation to be
recorded in xen.efi's .reloc section. But they should nevertheless
be silenced. We've been discussing this on irc, iirc. The absolute
address used wants to move by 1Gb for the $(ALT_BASE) intermediate
linking step.

Jan

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

Re: [Xen-devel] [PATCH v4 3/7] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Thu, Jan 23, 2020 at 11:13:10AM +0100, Jan Beulich wrote:
> On 22.01.2020 22:57, Andrew Cooper wrote:
> > On 22/01/2020 20:23, Wei Liu wrote:
> >> These functions will be used later to make hypercalls to Hyper-V.
> >>
> >> Signed-off-by: Wei Liu <liuwe@microsoft.com>
> > 
> > After some experimentation,
> > 
> > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > index cbc5701214..3708a60b5c 100644
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -329,6 +329,8 @@ SECTIONS
> >    efi = .;
> >  #endif
> >  
> > +  hv_hcall_page = ABSOLUTE(0xffff82d0bfffe000);
> > +
> >    /* Sections to be discarded */
> >    /DISCARD/ : {
> >         *(.exit.text)
> > 
> > in the linker script lets direct calls work correctly:
> > 
> > ffff82d080637935:       b9 01 00 00 40          mov    $0x40000001,%ecx
> > ffff82d08063793a:       0f 30                   wrmsr 
> > ffff82d08063793c:       ba 21 03 00 00          mov    $0x321,%edx
> > ffff82d080637941:       bf 01 00 00 00          mov    $0x1,%edi
> > ffff82d080637946:       e8 ac 4f c7 ff          callq  ffff82d0802ac8f7
> > <__set_fixmap_x>
> > ffff82d08063794b:       41 b8 00 00 00 00       mov    $0x0,%r8d
> > ffff82d080637951:       b9 ff ff 00 00          mov    $0xffff,%ecx
> > ffff82d080637956:       ba 00 00 00 00          mov    $0x0,%edx
> > ffff82d08063795b:       e8 a0 66 9c 3f          callq  ffff82d0bfffe000
> > <hv_hcall_page>
> > ffff82d080637960:       66 83 f8 02             cmp    $0x2,%ax
> > 
> > but it does throw:
> > 
> > Difference at .init:00032edf is 0xc0000000 (expected 0x40000000)
> > Difference at .init:00032edf is 0xc0000000 (expected 0x40000000)
> > 
> > as a diagnostic presumably from the final link  (both with a standard
> > Debian 2.28 binutils, and upstream 2.33 build).  I'm not sure what its
> > trying to complain about, as both xen.gz and xen.efi have correctly
> > generated code.
> > 
> > Depending on whether they are benign or not, a linker-friendly
> > fix_to_virt() should be all we need to keep these strictly as direct calls.
> 
> They're benign in the particular case of them actually resulting
> from relative CALLs, which hence require no relocation to be
> recorded in xen.efi's .reloc section. But they should nevertheless
> be silenced. We've been discussing this on irc, iirc. The absolute
> address used wants to move by 1Gb for the $(ALT_BASE) intermediate
> linking step.

FWIW I don't see those messages with my current code.

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v4 3/7] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich]

On 22.01.2020 21:23, Wei Liu wrote:
> --- /dev/null
> +++ b/xen/include/asm-x86/guest/hyperv-hcall.h
> @@ -0,0 +1,98 @@
> +/******************************************************************************
> + * 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 ("INDIRECT_CALL %P[hcall_page]"
> +                  : "=a" (status), "+c" (control),
> +                    "+d" (input_addr) ASM_CALL_CONSTRAINT
> +                  : "r" (r8),
> +                    [hcall_page] "p" (fix_x_to_virt(FIX_X_HYPERV_HCALL))
> +                  : "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 ("INDIRECT_CALL %P[hcall_page]"
> +                  : "=a" (status), "+c" (control),
> +                    "+d" (input1) ASM_CALL_CONSTRAINT
> +                  : "r" (r8),
> +                    [hcall_page] "p" (fix_x_to_virt(FIX_X_HYPERV_HCALL))
> +                  :);

This comes through as a smiley in my mail viewer, because of the
missing blanks immediately inside the outermost parentheses.

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

What about the upper bit(s) spilling into the next field? Perhaps
better use MASK_INSR() here too?

Also, this leaves the START field zero, which makes me think you
mean ...

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

... REP_START_MASK here.

Jan

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

Re: [Xen-devel] [PATCH v4 3/7] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Thu, Jan 23, 2020 at 12:28:00PM +0100, Jan Beulich wrote:
> On 22.01.2020 21:23, Wei Liu wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-x86/guest/hyperv-hcall.h
> > @@ -0,0 +1,98 @@
> > +/******************************************************************************
> > + * 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 ("INDIRECT_CALL %P[hcall_page]"
> > +                  : "=a" (status), "+c" (control),
> > +                    "+d" (input_addr) ASM_CALL_CONSTRAINT
> > +                  : "r" (r8),
> > +                    [hcall_page] "p" (fix_x_to_virt(FIX_X_HYPERV_HCALL))
> > +                  : "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 ("INDIRECT_CALL %P[hcall_page]"
> > +                  : "=a" (status), "+c" (control),
> > +                    "+d" (input1) ASM_CALL_CONSTRAINT
> > +                  : "r" (r8),
> > +                    [hcall_page] "p" (fix_x_to_virt(FIX_X_HYPERV_HCALL))
> > +                  :);
> 
> This comes through as a smiley in my mail viewer, because of the
> missing blanks immediately inside the outermost parentheses.

Fixed.

> 
> > +
> > +    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;
> 
> What about the upper bit(s) spilling into the next field? Perhaps
> better use MASK_INSR() here too?
> 

I didn't do it because we didn't have the two MASKs defined. Plus I
trusted Hyper-V to reject what's abnormal so I wouldn't worry too much
about it.


> Also, this leaves the START field zero, which makes me think you
> mean ...
> 
> > +    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_COMP_MASK);
> 
> ... REP_START_MASK here.

Yes, indeed. Fixed.

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v4 3/7] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich]

On 29.01.2020 19:37, Wei Liu wrote:
> On Thu, Jan 23, 2020 at 12:28:00PM +0100, Jan Beulich wrote:
>> On 22.01.2020 21:23, Wei Liu wrote:
>>> --- /dev/null
>>> +++ b/xen/include/asm-x86/guest/hyperv-hcall.h
>>> @@ -0,0 +1,98 @@
>>> +/******************************************************************************
>>> + * 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 ("INDIRECT_CALL %P[hcall_page]"
>>> +                  : "=a" (status), "+c" (control),
>>> +                    "+d" (input_addr) ASM_CALL_CONSTRAINT
>>> +                  : "r" (r8),
>>> +                    [hcall_page] "p" (fix_x_to_virt(FIX_X_HYPERV_HCALL))
>>> +                  : "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 ("INDIRECT_CALL %P[hcall_page]"
>>> +                  : "=a" (status), "+c" (control),
>>> +                    "+d" (input1) ASM_CALL_CONSTRAINT
>>> +                  : "r" (r8),
>>> +                    [hcall_page] "p" (fix_x_to_virt(FIX_X_HYPERV_HCALL))
>>> +                  :);
>>
>> This comes through as a smiley in my mail viewer, because of the
>> missing blanks immediately inside the outermost parentheses.
> 
> Fixed.

By dropping the :, I assume? My suggestion of just the missing blank
may have been misleading here (albeit the blanks still need adding).

Jan

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

Re: [Xen-devel] [PATCH v4 3/7] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Thu, Jan 30, 2020 at 09:12:14AM +0100, Jan Beulich wrote:
> On 29.01.2020 19:37, Wei Liu wrote:
> > On Thu, Jan 23, 2020 at 12:28:00PM +0100, Jan Beulich wrote:
> >> On 22.01.2020 21:23, Wei Liu wrote:
> >>> --- /dev/null
> >>> +++ b/xen/include/asm-x86/guest/hyperv-hcall.h
> >>> @@ -0,0 +1,98 @@
> >>> +/******************************************************************************
> >>> + * 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 ("INDIRECT_CALL %P[hcall_page]"
> >>> +                  : "=a" (status), "+c" (control),
> >>> +                    "+d" (input_addr) ASM_CALL_CONSTRAINT
> >>> +                  : "r" (r8),
> >>> +                    [hcall_page] "p" (fix_x_to_virt(FIX_X_HYPERV_HCALL))
> >>> +                  : "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 ("INDIRECT_CALL %P[hcall_page]"
> >>> +                  : "=a" (status), "+c" (control),
> >>> +                    "+d" (input1) ASM_CALL_CONSTRAINT
> >>> +                  : "r" (r8),
> >>> +                    [hcall_page] "p" (fix_x_to_virt(FIX_X_HYPERV_HCALL))
> >>> +                  :);
> >>
> >> This comes through as a smiley in my mail viewer, because of the
> >> missing blanks immediately inside the outermost parentheses.
> > 
> > Fixed.
> 
> By dropping the :, I assume? My suggestion of just the missing blank
> may have been misleading here (albeit the blanks still need adding).

No, by adding a blank.

I will drop that ":" locally in my tree for next round of posting.

Wei.

> 
> Jan

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

[Xen-devel] [PATCH v4 4/7] 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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index f986c1a805..536ce0d0dd 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>
 
@@ -107,6 +108,15 @@ static void __init setup_hypercall_page(void)
     }
 
     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);
+    }
 }
 
 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 v4 5/7] 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>
---
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  | 25 +++++++++++++++++++++++++
 xen/arch/x86/guest/hyperv/private.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 54 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 536ce0d0dd..c5195af948 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_arg);
 
 static uint64_t generate_guest_id(void)
 {
@@ -119,14 +122,36 @@ static void __init setup_hypercall_page(void)
     }
 }
 
+static void setup_hypercall_pcpu_arg(void)
+{
+    void *mapping;
+
+    if ( this_cpu(hv_pcpu_input_arg) )
+        return;
+
+    mapping = alloc_xenheap_page();
+    if ( !mapping )
+        panic("Failed to allocate hypercall input page for CPU%u\n",
+              smp_processor_id());
+
+    this_cpu(hv_pcpu_input_arg) = mapping;
+}
+
 static void __init setup(void)
 {
     setup_hypercall_page();
+    setup_hypercall_pcpu_arg();
+}
+
+static void ap_setup(void)
+{
+    setup_hypercall_pcpu_arg();
 }
 
 static const struct hypervisor_ops ops = {
     .name = "Hyper-V",
     .setup = setup,
+    .ap_setup = ap_setup,
 };
 
 /*
diff --git a/xen/arch/x86/guest/hyperv/private.h b/xen/arch/x86/guest/hyperv/private.h
new file mode 100644
index 0000000000..b6902b5639
--- /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_arg);
+
+#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 v4 5/7] x86/hyperv: provide percpu hypercall input page

[Jan Beulich]

On 22.01.2020 21:23, Wei Liu wrote:
> --- 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_arg);

Would it perhaps be helpful to make recognizable that this can hold
up to a page's worth of data, either by its type or by a slightly
different name?

> @@ -119,14 +122,36 @@ static void __init setup_hypercall_page(void)
>      }
>  }
>  
> +static void setup_hypercall_pcpu_arg(void)
> +{
> +    void *mapping;
> +
> +    if ( this_cpu(hv_pcpu_input_arg) )
> +        return;
> +
> +    mapping = alloc_xenheap_page();
> +    if ( !mapping )
> +        panic("Failed to allocate hypercall input page for CPU%u\n",
> +              smp_processor_id());

panic() is likely fine for the BSP, but I don't think any AP should
be able to bring down the system because of memory shortage. Such
an AP should simply not come online. (Even for the BSP the question
is whether Xen would be able to run despite failure here.)

Jan

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

Re: [Xen-devel] [PATCH v4 5/7] x86/hyperv: provide percpu hypercall input page

[Wei Liu]

On Thu, Jan 23, 2020 at 04:45:38PM +0100, Jan Beulich wrote:
> On 22.01.2020 21:23, Wei Liu wrote:
> > --- 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_arg);
> 
> Would it perhaps be helpful to make recognizable that this can hold
> up to a page's worth of data, either by its type or by a slightly
> different name?

I will change this to hv_pcpu_input_arg_page instead.

> 
> > @@ -119,14 +122,36 @@ static void __init setup_hypercall_page(void)
> >      }
> >  }
> >  
> > +static void setup_hypercall_pcpu_arg(void)
> > +{
> > +    void *mapping;
> > +
> > +    if ( this_cpu(hv_pcpu_input_arg) )
> > +        return;
> > +
> > +    mapping = alloc_xenheap_page();
> > +    if ( !mapping )
> > +        panic("Failed to allocate hypercall input page for CPU%u\n",
> > +              smp_processor_id());
> 
> panic() is likely fine for the BSP, but I don't think any AP should
> be able to bring down the system because of memory shortage. Such
> an AP should simply not come online. (Even for the BSP the question
> is whether Xen would be able to run despite failure here.)

This is no different than what is already done for Xen on Xen, i.e.
failure in setting up AP for any reason is fatal.

start_secondary doesn't even handling any failure by itself or
propagate failure back to caller.

Rewinding is a bit complex, given that we would enable hypervisor
features very early.

To achieve what you want it would require rewriting of other parts that
are outside of hypervisor framework.

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v4 5/7] x86/hyperv: provide percpu hypercall input page

[Jan Beulich]

On 28.01.2020 16:50, Wei Liu wrote:
> On Thu, Jan 23, 2020 at 04:45:38PM +0100, Jan Beulich wrote:
>> On 22.01.2020 21:23, Wei Liu wrote:
>>> --- 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_arg);
>>
>> Would it perhaps be helpful to make recognizable that this can hold
>> up to a page's worth of data, either by its type or by a slightly
>> different name?
> 
> I will change this to hv_pcpu_input_arg_page instead.

Or maybe hv_pcpu_input_page?

>>> @@ -119,14 +122,36 @@ static void __init setup_hypercall_page(void)
>>>      }
>>>  }
>>>  
>>> +static void setup_hypercall_pcpu_arg(void)
>>> +{
>>> +    void *mapping;
>>> +
>>> +    if ( this_cpu(hv_pcpu_input_arg) )
>>> +        return;
>>> +
>>> +    mapping = alloc_xenheap_page();
>>> +    if ( !mapping )
>>> +        panic("Failed to allocate hypercall input page for CPU%u\n",
>>> +              smp_processor_id());
>>
>> panic() is likely fine for the BSP, but I don't think any AP should
>> be able to bring down the system because of memory shortage. Such
>> an AP should simply not come online. (Even for the BSP the question
>> is whether Xen would be able to run despite failure here.)
> 
> This is no different than what is already done for Xen on Xen, i.e.
> failure in setting up AP for any reason is fatal.
> 
> start_secondary doesn't even handling any failure by itself or
> propagate failure back to caller.
> 
> Rewinding is a bit complex, given that we would enable hypervisor
> features very early.
> 
> To achieve what you want it would require rewriting of other parts that
> are outside of hypervisor framework.

Not sure. Comparing with start_secondary() is perhaps sub-optimal.
The function calls smp_callin(), and there you'll find some error
handling. I would suppose this could be extended (there or in
start_secondary() itself, if need be) to deal with cases like this
one. As to Xen-on-Xen - iirc that code was pretty much rushed in
for the shim to become usable, so I wouldn't take its error
handling model as the canonical reference.

Jan

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

Re: [Xen-devel] [PATCH v4 5/7] x86/hyperv: provide percpu hypercall input page

[Wei Liu]

On Tue, Jan 28, 2020 at 05:15:39PM +0100, Jan Beulich wrote:
> On 28.01.2020 16:50, Wei Liu wrote:
> > On Thu, Jan 23, 2020 at 04:45:38PM +0100, Jan Beulich wrote:
> >> On 22.01.2020 21:23, Wei Liu wrote:
> >>> --- 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_arg);
> >>
> >> Would it perhaps be helpful to make recognizable that this can hold
> >> up to a page's worth of data, either by its type or by a slightly
> >> different name?
> > 
> > I will change this to hv_pcpu_input_arg_page instead.
> 
> Or maybe hv_pcpu_input_page?

Fine by me.

> 
> >>> @@ -119,14 +122,36 @@ static void __init setup_hypercall_page(void)
> >>>      }
> >>>  }
> >>>  
> >>> +static void setup_hypercall_pcpu_arg(void)
> >>> +{
> >>> +    void *mapping;
> >>> +
> >>> +    if ( this_cpu(hv_pcpu_input_arg) )
> >>> +        return;
> >>> +
> >>> +    mapping = alloc_xenheap_page();
> >>> +    if ( !mapping )
> >>> +        panic("Failed to allocate hypercall input page for CPU%u\n",
> >>> +              smp_processor_id());
> >>
> >> panic() is likely fine for the BSP, but I don't think any AP should
> >> be able to bring down the system because of memory shortage. Such
> >> an AP should simply not come online. (Even for the BSP the question
> >> is whether Xen would be able to run despite failure here.)
> > 
> > This is no different than what is already done for Xen on Xen, i.e.
> > failure in setting up AP for any reason is fatal.
> > 
> > start_secondary doesn't even handling any failure by itself or
> > propagate failure back to caller.
> > 
> > Rewinding is a bit complex, given that we would enable hypervisor
> > features very early.
> > 
> > To achieve what you want it would require rewriting of other parts that
> > are outside of hypervisor framework.
> 
> Not sure. Comparing with start_secondary() is perhaps sub-optimal.
> The function calls smp_callin(), and there you'll find some error
> handling. I would suppose this could be extended (there or in
> start_secondary() itself, if need be) to deal with cases like this
> one. As to Xen-on-Xen - iirc that code was pretty much rushed in
> for the shim to become usable, so I wouldn't take its error
> handling model as the canonical reference.

OK. What I can do here is to write some patches to 1) make the hook
return sensible error code and b) push hypervisor_ap_setup down to
smp_callin.

Wei.

> 
> Jan

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

[Xen-devel] [PATCH v4 6/7] 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>
---
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 c5195af948..085e646dc6 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_arg);
+DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index);
 
 static uint64_t generate_guest_id(void)
 {
@@ -125,6 +126,7 @@ static void __init setup_hypercall_page(void)
 static void setup_hypercall_pcpu_arg(void)
 {
     void *mapping;
+    uint64_t vp_index_msr;
 
     if ( this_cpu(hv_pcpu_input_arg) )
         return;
@@ -135,6 +137,9 @@ static void setup_hypercall_pcpu_arg(void)
               smp_processor_id());
 
     this_cpu(hv_pcpu_input_arg) = mapping;
+
+    rdmsrl(HV_X64_MSR_VP_INDEX, vp_index_msr);
+    this_cpu(hv_vp_index) = vp_index_msr;
 }
 
 static void __init setup(void)
diff --git a/xen/arch/x86/guest/hyperv/private.h b/xen/arch/x86/guest/hyperv/private.h
index b6902b5639..da70990401 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_arg);
+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 v4 6/7] x86/hyperv: retrieve vp_index from Hyper-V

[Jan Beulich]

On 22.01.2020 21:23, 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>

For formal reasons
Acked-by: Jan Beulich <jbeulich@suse.com>

However I wonder whether the Viridian entry in MAINTAINERS shouldn't
be extended by

F:	xen/arch/x86/guest/hyperv/

(and possibly have its title adjusted). Thoughts?

Jan

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

Re: [Xen-devel] [PATCH v4 6/7] x86/hyperv: retrieve vp_index from Hyper-V

[Wei Liu]

On Thu, Jan 23, 2020 at 04:48:38PM +0100, Jan Beulich wrote:
> On 22.01.2020 21:23, 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>
> 
> For formal reasons
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> However I wonder whether the Viridian entry in MAINTAINERS shouldn't
> be extended by
> 
> F:	xen/arch/x86/guest/hyperv/
> 
> (and possibly have its title adjusted). Thoughts?

This isn't about emulating Hyper-V inside Xen, so I don't think that's
the right approach here.

That said, if Paul wants to take this under his purview, it's fine by me
-- that would make me easier to upstream my patch. ;-)  I also wouldn't
mind adding myself as maintainer for this path.

Wei.

> 
> Jan

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

Re: [Xen-devel] [PATCH v4 6/7] x86/hyperv: retrieve vp_index from Hyper-V

[Jan Beulich]

On 28.01.2020 16:55, Wei Liu wrote:
> On Thu, Jan 23, 2020 at 04:48:38PM +0100, Jan Beulich wrote:
>> On 22.01.2020 21:23, 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>
>>
>> For formal reasons
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>> However I wonder whether the Viridian entry in MAINTAINERS shouldn't
>> be extended by
>>
>> F:	xen/arch/x86/guest/hyperv/
>>
>> (and possibly have its title adjusted). Thoughts?
> 
> This isn't about emulating Hyper-V inside Xen, so I don't think that's
> the right approach here.

Well, it's the code producing the interface in one case, and
consuming it here. So there is some overlap at least.

> That said, if Paul wants to take this under his purview, it's fine by me
> -- that would make me easier to upstream my patch. ;-)  I also wouldn't
> mind adding myself as maintainer for this path.

Perhaps best both of you? Paul, Andrew, what do you think?

Jan

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

Re: [Xen-devel] [PATCH v4 6/7] x86/hyperv: retrieve vp_index from Hyper-V

[Durrant, Paul]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan
> Beulich
> Sent: 28 January 2020 16:19
> To: Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>
> Cc: Xen Development List <xen-devel@lists.xenproject.org>; Roger Pau Monné
> <roger.pau@citrix.com>; Wei Liu <liuwe@microsoft.com>; Michael Kelley
> <mikelley@microsoft.com>
> Subject: Re: [Xen-devel] [PATCH v4 6/7] x86/hyperv: retrieve vp_index from
> Hyper-V
> 
> On 28.01.2020 16:55, Wei Liu wrote:
> > On Thu, Jan 23, 2020 at 04:48:38PM +0100, Jan Beulich wrote:
> >> On 22.01.2020 21:23, 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>
> >>
> >> For formal reasons
> >> Acked-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> However I wonder whether the Viridian entry in MAINTAINERS shouldn't
> >> be extended by
> >>
> >> F:	xen/arch/x86/guest/hyperv/
> >>
> >> (and possibly have its title adjusted). Thoughts?
> >
> > This isn't about emulating Hyper-V inside Xen, so I don't think that's
> > the right approach here.
> 
> Well, it's the code producing the interface in one case, and
> consuming it here. So there is some overlap at least.
> 
> > That said, if Paul wants to take this under his purview, it's fine by me
> > -- that would make me easier to upstream my patch. ;-)  I also wouldn't
> > mind adding myself as maintainer for this path.
> 
> Perhaps best both of you? Paul, Andrew, what do you think?
> 

IMO it's probably best to the put the Hyper-V stuff under 'Viridian' and add yourself as a maintainer there. There really is likely to be significant overlap and it'd make it easier (for me at least) to keep track of the bigger picture (i.e. Xen using enlightenments as well as implementing them).

  Cheers,

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

Re: [Xen-devel] [PATCH v4 6/7] x86/hyperv: retrieve vp_index from Hyper-V

[Wei Liu]

On Tue, Jan 28, 2020 at 04:33:00PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan
> > Beulich
> > Sent: 28 January 2020 16:19
> > To: Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>; Andrew Cooper
> > <andrew.cooper3@citrix.com>
> > Cc: Xen Development List <xen-devel@lists.xenproject.org>; Roger Pau Monné
> > <roger.pau@citrix.com>; Wei Liu <liuwe@microsoft.com>; Michael Kelley
> > <mikelley@microsoft.com>
> > Subject: Re: [Xen-devel] [PATCH v4 6/7] x86/hyperv: retrieve vp_index from
> > Hyper-V
> > 
> > On 28.01.2020 16:55, Wei Liu wrote:
> > > On Thu, Jan 23, 2020 at 04:48:38PM +0100, Jan Beulich wrote:
> > >> On 22.01.2020 21:23, 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>
> > >>
> > >> For formal reasons
> > >> Acked-by: Jan Beulich <jbeulich@suse.com>
> > >>
> > >> However I wonder whether the Viridian entry in MAINTAINERS shouldn't
> > >> be extended by
> > >>
> > >> F:	xen/arch/x86/guest/hyperv/
> > >>
> > >> (and possibly have its title adjusted). Thoughts?
> > >
> > > This isn't about emulating Hyper-V inside Xen, so I don't think that's
> > > the right approach here.
> > 
> > Well, it's the code producing the interface in one case, and
> > consuming it here. So there is some overlap at least.
> > 
> > > That said, if Paul wants to take this under his purview, it's fine by me
> > > -- that would make me easier to upstream my patch. ;-)  I also wouldn't
> > > mind adding myself as maintainer for this path.
> > 
> > Perhaps best both of you? Paul, Andrew, what do you think?
> > 
> 
> IMO it's probably best to the put the Hyper-V stuff under 'Viridian'
> and add yourself as a maintainer there. There really is likely to be
> significant overlap and it'd make it easier (for me at least) to keep
> track of the bigger picture (i.e. Xen using enlightenments as well as
> implementing them).

When you said "yourself", did you mean me or Jan?

Wei.

> 
>   Cheers,
> 
>     Paul

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

Re: [Xen-devel] [PATCH v4 6/7] x86/hyperv: retrieve vp_index from Hyper-V

[Durrant, Paul]

> -----Original Message-----
> From: Wei Liu <wl@xen.org>
> Sent: 28 January 2020 16:54
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>; Paul Durrant
> <paul@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>; Xen Development
> List <xen-devel@lists.xenproject.org>; Roger Pau Monné
> <roger.pau@citrix.com>; Wei Liu <liuwe@microsoft.com>; Michael Kelley
> <mikelley@microsoft.com>
> Subject: Re: [Xen-devel] [PATCH v4 6/7] x86/hyperv: retrieve vp_index from
> Hyper-V
> 
> On Tue, Jan 28, 2020 at 04:33:00PM +0000, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Jan
> > > Beulich
> > > Sent: 28 January 2020 16:19
> > > To: Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>; Andrew Cooper
> > > <andrew.cooper3@citrix.com>
> > > Cc: Xen Development List <xen-devel@lists.xenproject.org>; Roger Pau
> Monné
> > > <roger.pau@citrix.com>; Wei Liu <liuwe@microsoft.com>; Michael Kelley
> > > <mikelley@microsoft.com>
> > > Subject: Re: [Xen-devel] [PATCH v4 6/7] x86/hyperv: retrieve vp_index
> from
> > > Hyper-V
> > >
> > > On 28.01.2020 16:55, Wei Liu wrote:
> > > > On Thu, Jan 23, 2020 at 04:48:38PM +0100, Jan Beulich wrote:
> > > >> On 22.01.2020 21:23, 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>
> > > >>
> > > >> For formal reasons
> > > >> Acked-by: Jan Beulich <jbeulich@suse.com>
> > > >>
> > > >> However I wonder whether the Viridian entry in MAINTAINERS
> shouldn't
> > > >> be extended by
> > > >>
> > > >> F:	xen/arch/x86/guest/hyperv/
> > > >>
> > > >> (and possibly have its title adjusted). Thoughts?
> > > >
> > > > This isn't about emulating Hyper-V inside Xen, so I don't think
> that's
> > > > the right approach here.
> > >
> > > Well, it's the code producing the interface in one case, and
> > > consuming it here. So there is some overlap at least.
> > >
> > > > That said, if Paul wants to take this under his purview, it's fine
> by me
> > > > -- that would make me easier to upstream my patch. ;-)  I also
> wouldn't
> > > > mind adding myself as maintainer for this path.
> > >
> > > Perhaps best both of you? Paul, Andrew, what do you think?
> > >
> >
> > IMO it's probably best to the put the Hyper-V stuff under 'Viridian'
> > and add yourself as a maintainer there. There really is likely to be
> > significant overlap and it'd make it easier (for me at least) to keep
> > track of the bigger picture (i.e. Xen using enlightenments as well as
> > implementing them).
> 
> When you said "yourself", did you mean me or Jan?
>

You. I thought was replying to your question. Apologies for any confusion.

  Paul
 
> Wei.
> 
> >
> >   Cheers,
> >
> >     Paul

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

[Xen-devel] [PATCH v4 7/7] 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>
---
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  | 26 ++++++++++++++++++++++++++
 xen/arch/x86/guest/hyperv/private.h |  1 +
 2 files changed, 27 insertions(+)

diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 085e646dc6..89a8f316b2 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -32,6 +32,7 @@
 struct ms_hyperv_info __read_mostly ms_hyperv;
 DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_arg);
 DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index);
+DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist);
 
 static uint64_t generate_guest_id(void)
 {
@@ -142,15 +143,40 @@ static void setup_hypercall_pcpu_arg(void)
     this_cpu(hv_vp_index) = vp_index_msr;
 }
 
+static void setup_vp_assist(void)
+{
+    void *mapping;
+    uint64_t val;
+
+    mapping = this_cpu(hv_vp_assist);
+
+    if ( !mapping )
+    {
+        mapping = alloc_xenheap_page();
+        if ( !mapping )
+            panic("Failed to allocate vp_assist page for CPU%u\n",
+                  smp_processor_id());
+
+        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);
+}
+
 static void __init setup(void)
 {
     setup_hypercall_page();
     setup_hypercall_pcpu_arg();
+    setup_vp_assist();
 }
 
 static void ap_setup(void)
 {
     setup_hypercall_pcpu_arg();
+    setup_vp_assist();
 }
 
 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 da70990401..af419e9d4b 100644
--- a/xen/arch/x86/guest/hyperv/private.h
+++ b/xen/arch/x86/guest/hyperv/private.h
@@ -26,5 +26,6 @@
 
 DECLARE_PER_CPU(void *, hv_pcpu_input_arg);
 DECLARE_PER_CPU(unsigned int, hv_vp_index);
+DECLARE_PER_CPU(void *, hv_vp_assist);
 
 #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 v4 7/7] x86/hyperv: setup VP assist page

[Andrew Cooper]

On 22/01/2020 20:23, Wei Liu wrote:
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> index 085e646dc6..89a8f316b2 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -32,6 +32,7 @@
>  struct ms_hyperv_info __read_mostly ms_hyperv;
>  DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_arg);
>  DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index);
> +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist);

You'll get fewer holes in the percpu data area by moving this
declaration up by one.

~Andrew

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

Re: [Xen-devel] [PATCH v4 7/7] x86/hyperv: setup VP assist page

[Jan Beulich]

On 22.01.2020 21:23, Wei Liu wrote:
> @@ -142,15 +143,40 @@ static void setup_hypercall_pcpu_arg(void)
>      this_cpu(hv_vp_index) = vp_index_msr;
>  }
>  
> +static void setup_vp_assist(void)
> +{
> +    void *mapping;
> +    uint64_t val;
> +
> +    mapping = this_cpu(hv_vp_assist);
> +
> +    if ( !mapping )
> +    {
> +        mapping = alloc_xenheap_page();
> +        if ( !mapping )
> +            panic("Failed to allocate vp_assist page for CPU%u\n",
> +                  smp_processor_id());

Quite obviously the same AP/BSP related remark here as was given for
patch 5.

Jan

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