[Xen-devel] [PATCH v3 0/5] More Hyper-V infrastructure

[Xen-devel] [PATCH v3 0/5] 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.

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 (5):
  x86/hyperv: setup hypercall page
  x86/hyperv: provide Hyper-V hypercall functions
  x86/hyperv: provide percpu hypercall input page
  x86/hyperv: retrieve vp_index from Hyper-V
  x86/hyperv: setup VP assist page

 xen/arch/x86/guest/hyperv/Makefile         |  1 +
 xen/arch/x86/guest/hyperv/hypercall_page.S | 21 +++++
 xen/arch/x86/guest/hyperv/hyperv.c         | 73 ++++++++++++++++-
 xen/include/asm-x86/guest/hyperv-hcall.h   | 95 ++++++++++++++++++++++
 xen/include/asm-x86/guest/hyperv.h         |  6 ++
 5 files changed, 193 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/x86/guest/hyperv/hypercall_page.S
 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 v3 1/5] x86/hyperv: setup hypercall page

[Wei Liu]

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
v2:
1. Fix issue discovered by Michael
2. Use a statically allocated page as hypercall page
---
 xen/arch/x86/guest/hyperv/Makefile         |  1 +
 xen/arch/x86/guest/hyperv/hypercall_page.S | 21 +++++++++++++++++
 xen/arch/x86/guest/hyperv/hyperv.c         | 27 +++++++++++++++++++---
 3 files changed, 46 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/x86/guest/hyperv/hypercall_page.S

diff --git a/xen/arch/x86/guest/hyperv/Makefile b/xen/arch/x86/guest/hyperv/Makefile
index 68170109a9..1a8887d2f4 100644
--- a/xen/arch/x86/guest/hyperv/Makefile
+++ b/xen/arch/x86/guest/hyperv/Makefile
@@ -1 +1,2 @@
+obj-y += hypercall_page.o
 obj-y += hyperv.o
diff --git a/xen/arch/x86/guest/hyperv/hypercall_page.S b/xen/arch/x86/guest/hyperv/hypercall_page.S
new file mode 100644
index 0000000000..6d6ab913be
--- /dev/null
+++ b/xen/arch/x86/guest/hyperv/hypercall_page.S
@@ -0,0 +1,21 @@
+#include <asm/asm_defns.h>
+#include <asm/page.h>
+
+        .section ".text.page_aligned", "ax", @progbits
+        .p2align PAGE_SHIFT
+GLOBAL(hv_hypercall_page)
+        /* Return -1 for "not yet ready" state */
+        mov -1, %rax
+        ret
+1:
+        /* Fill the rest with `ret` */
+        .fill PAGE_SIZE - (1b - hv_hypercall_page), 1, 0xc3
+        .type hv_hypercall_page, STT_OBJECT
+        .size hv_hypercall_page, PAGE_SIZE
+
+/*
+ * Local variables:
+ * tab-width: 8
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 8d38313d7a..381be2a68c 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -19,16 +19,16 @@
  * Copyright (c) 2019 Microsoft.
  */
 #include <xen/init.h>
+#include <xen/domain_page.h>
 
 #include <asm/guest.h>
 #include <asm/guest/hyperv-tlfs.h>
 
 struct ms_hyperv_info __read_mostly ms_hyperv;
 
-static const struct hypervisor_ops ops = {
-    .name = "Hyper-V",
-};
+extern char hv_hypercall_page[];
 
+static const struct hypervisor_ops ops;
 const struct hypervisor_ops *__init hyperv_probe(void)
 {
     uint32_t eax, ebx, ecx, edx;
@@ -72,6 +72,27 @@ 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;
+
+    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+    hypercall_msr.enable = 1;
+    hypercall_msr.guest_physical_address =
+        __pa(hv_hypercall_page) >> HV_HYP_PAGE_SHIFT;
+    wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+}
+
+static void __init setup(void)
+{
+    setup_hypercall_page();
+}
+
+static const struct hypervisor_ops ops = {
+    .name = "Hyper-V",
+    .setup = setup,
+};
+
 /*
  * Local variables:
  * mode: C
-- 
2.20.1


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

Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page

[Andrew Cooper]

On 05/01/2020 16:47, Wei Liu wrote:
> diff --git a/xen/arch/x86/guest/hyperv/Makefile b/xen/arch/x86/guest/hyperv/Makefile
> index 68170109a9..1a8887d2f4 100644
> --- a/xen/arch/x86/guest/hyperv/Makefile
> +++ b/xen/arch/x86/guest/hyperv/Makefile
> @@ -1 +1,2 @@
> +obj-y += hypercall_page.o
>  obj-y += hyperv.o
> diff --git a/xen/arch/x86/guest/hyperv/hypercall_page.S b/xen/arch/x86/guest/hyperv/hypercall_page.S
> new file mode 100644
> index 0000000000..6d6ab913be
> --- /dev/null
> +++ b/xen/arch/x86/guest/hyperv/hypercall_page.S
> @@ -0,0 +1,21 @@
> +#include <asm/asm_defns.h>
> +#include <asm/page.h>
> +
> +        .section ".text.page_aligned", "ax", @progbits
> +        .p2align PAGE_SHIFT
> +GLOBAL(hv_hypercall_page)
> +        /* Return -1 for "not yet ready" state */
> +        mov -1, %rax
> +        ret
> +1:
> +        /* Fill the rest with `ret` */
> +        .fill PAGE_SIZE - (1b - hv_hypercall_page), 1, 0xc3

If you want to fill with rets, you can do this more simply with:

    .p2lign PAGE_SHIFT, 0xc3

which will do the size calculation for you.

That said, I retract my statement about wanting this in the middle of
.text.  (Sorry.  See below.)

> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> index 8d38313d7a..381be2a68c 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -72,6 +72,27 @@ 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;
> +
> +    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +    hypercall_msr.enable = 1;
> +    hypercall_msr.guest_physical_address =
> +        __pa(hv_hypercall_page) >> HV_HYP_PAGE_SHIFT;
> +    wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +}
> +
> +static void __init setup(void)
> +{
> +    setup_hypercall_page();
> +}

The TLFS says that writing enable will fail until the OS identity is
set, which AFACIT, isn't done anywhere in the series.  The whole
sequence is described in "3.13 Establishing the Hypercall Interface"

The locked bit is probably a good idea, but one aspect missing here is
the check to see whether the hypercall page is already enabled, which I
expect is for a kexec crash scenario.

However, the most important point is the one which describes the #GP
properties of the guest trying to modify the page.  This can only be
achieved with an EPT/NPT mapping lacking the W permission, which will
shatter host superpages.   Therefore, putting it in .text is going to be
rather poor, perf wise.

I also note that Xen's implementation of the Viridian hypercall page
doesn't conform to these properties, and wants fixing.  It is going to
need a new kind identification of the page (probably a new p2m type)
which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.

As for suggestions here, I'm struggling to find any memory map details
exposed in the Viridian interface, and therefore which gfn is best to
choose.  I have a sinking feeling that the answer is ACPI...

~Andrew

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

Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page

[Wei Liu]

On Sun, Jan 05, 2020 at 05:37:44PM +0000, Andrew Cooper wrote:
> On 05/01/2020 16:47, Wei Liu wrote:
> > diff --git a/xen/arch/x86/guest/hyperv/Makefile b/xen/arch/x86/guest/hyperv/Makefile
> > index 68170109a9..1a8887d2f4 100644
> > --- a/xen/arch/x86/guest/hyperv/Makefile
> > +++ b/xen/arch/x86/guest/hyperv/Makefile
> > @@ -1 +1,2 @@
> > +obj-y += hypercall_page.o
> >  obj-y += hyperv.o
> > diff --git a/xen/arch/x86/guest/hyperv/hypercall_page.S b/xen/arch/x86/guest/hyperv/hypercall_page.S
> > new file mode 100644
> > index 0000000000..6d6ab913be
> > --- /dev/null
> > +++ b/xen/arch/x86/guest/hyperv/hypercall_page.S
> > @@ -0,0 +1,21 @@
> > +#include <asm/asm_defns.h>
> > +#include <asm/page.h>
> > +
> > +        .section ".text.page_aligned", "ax", @progbits
> > +        .p2align PAGE_SHIFT
> > +GLOBAL(hv_hypercall_page)
> > +        /* Return -1 for "not yet ready" state */
> > +        mov -1, %rax
> > +        ret
> > +1:
> > +        /* Fill the rest with `ret` */
> > +        .fill PAGE_SIZE - (1b - hv_hypercall_page), 1, 0xc3
> 
> If you want to fill with rets, you can do this more simply with:
> 
>     .p2lign PAGE_SHIFT, 0xc3
> 
> which will do the size calculation for you.
> 
> That said, I retract my statement about wanting this in the middle of
> .text.  (Sorry.  See below.)
> 
> > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> > index 8d38313d7a..381be2a68c 100644
> > --- a/xen/arch/x86/guest/hyperv/hyperv.c
> > +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> > @@ -72,6 +72,27 @@ 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;
> > +
> > +    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > +    hypercall_msr.enable = 1;
> > +    hypercall_msr.guest_physical_address =
> > +        __pa(hv_hypercall_page) >> HV_HYP_PAGE_SHIFT;
> > +    wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > +}
> > +
> > +static void __init setup(void)
> > +{
> > +    setup_hypercall_page();
> > +}
> 
> The TLFS says that writing enable will fail until the OS identity is
> set, which AFACIT, isn't done anywhere in the series.  The whole
> sequence is described in "3.13 Establishing the Hypercall Interface"

Good catch. I will make up an identity number for Xen. I will also
follow the sequence strictly.

> 
> The locked bit is probably a good idea, but one aspect missing here is
> the check to see whether the hypercall page is already enabled, which I
> expect is for a kexec crash scenario.
> 
> However, the most important point is the one which describes the #GP
> properties of the guest trying to modify the page.  This can only be
> achieved with an EPT/NPT mapping lacking the W permission, which will
> shatter host superpages.   Therefore, putting it in .text is going to be
> rather poor, perf wise.
> 
> I also note that Xen's implementation of the Viridian hypercall page
> doesn't conform to these properties, and wants fixing.  It is going to
> need a new kind identification of the page (probably a new p2m type)
> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
> 
> As for suggestions here, I'm struggling to find any memory map details
> exposed in the Viridian interface, and therefore which gfn is best to
> choose.  I have a sinking feeling that the answer is ACPI...

TLFS only says "go find one suitable page yourself" without further
hints.

Since we're still quite far away from a functioning system, finding a
most suitable page isn't my top priority at this point. If there is a
simple way to extrapolate suitable information from ACPI, that would be
great. If it requires writing a set of functionalities, than that will
need to wait till later.

Wei.

> 
> ~Andrew

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

Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page

[Andrew Cooper]

On 05/01/2020 21:45, Wei Liu wrote:
> On Sun, Jan 05, 2020 at 05:37:44PM +0000, Andrew Cooper wrote:
>> On 05/01/2020 16:47, Wei Liu wrote:
>>> diff --git a/xen/arch/x86/guest/hyperv/Makefile b/xen/arch/x86/guest/hyperv/Makefile
>>> index 68170109a9..1a8887d2f4 100644
>>> --- a/xen/arch/x86/guest/hyperv/Makefile
>>> +++ b/xen/arch/x86/guest/hyperv/Makefile
>>> @@ -1 +1,2 @@
>>> +obj-y += hypercall_page.o
>>>  obj-y += hyperv.o
>>> diff --git a/xen/arch/x86/guest/hyperv/hypercall_page.S b/xen/arch/x86/guest/hyperv/hypercall_page.S
>>> new file mode 100644
>>> index 0000000000..6d6ab913be
>>> --- /dev/null
>>> +++ b/xen/arch/x86/guest/hyperv/hypercall_page.S
>>> @@ -0,0 +1,21 @@
>>> +#include <asm/asm_defns.h>
>>> +#include <asm/page.h>
>>> +
>>> +        .section ".text.page_aligned", "ax", @progbits
>>> +        .p2align PAGE_SHIFT
>>> +GLOBAL(hv_hypercall_page)
>>> +        /* Return -1 for "not yet ready" state */
>>> +        mov -1, %rax
>>> +        ret
>>> +1:
>>> +        /* Fill the rest with `ret` */
>>> +        .fill PAGE_SIZE - (1b - hv_hypercall_page), 1, 0xc3
>> If you want to fill with rets, you can do this more simply with:
>>
>>     .p2lign PAGE_SHIFT, 0xc3
>>
>> which will do the size calculation for you.
>>
>> That said, I retract my statement about wanting this in the middle of
>> .text.  (Sorry.  See below.)
>>
>>> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
>>> index 8d38313d7a..381be2a68c 100644
>>> --- a/xen/arch/x86/guest/hyperv/hyperv.c
>>> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
>>> @@ -72,6 +72,27 @@ 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;
>>> +
>>> +    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>>> +    hypercall_msr.enable = 1;
>>> +    hypercall_msr.guest_physical_address =
>>> +        __pa(hv_hypercall_page) >> HV_HYP_PAGE_SHIFT;
>>> +    wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>>> +}
>>> +
>>> +static void __init setup(void)
>>> +{
>>> +    setup_hypercall_page();
>>> +}
>> The TLFS says that writing enable will fail until the OS identity is
>> set, which AFACIT, isn't done anywhere in the series.  The whole
>> sequence is described in "3.13 Establishing the Hypercall Interface"
> Good catch. I will make up an identity number for Xen. I will also
> follow the sequence strictly.
>
>> The locked bit is probably a good idea, but one aspect missing here is
>> the check to see whether the hypercall page is already enabled, which I
>> expect is for a kexec crash scenario.
>>
>> However, the most important point is the one which describes the #GP
>> properties of the guest trying to modify the page.  This can only be
>> achieved with an EPT/NPT mapping lacking the W permission, which will
>> shatter host superpages.   Therefore, putting it in .text is going to be
>> rather poor, perf wise.
>>
>> I also note that Xen's implementation of the Viridian hypercall page
>> doesn't conform to these properties, and wants fixing.  It is going to
>> need a new kind identification of the page (probably a new p2m type)
>> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
>>
>> As for suggestions here, I'm struggling to find any memory map details
>> exposed in the Viridian interface, and therefore which gfn is best to
>> choose.  I have a sinking feeling that the answer is ACPI...
> TLFS only says "go find one suitable page yourself" without further
> hints.
>
> Since we're still quite far away from a functioning system, finding a
> most suitable page isn't my top priority at this point. If there is a
> simple way to extrapolate suitable information from ACPI, that would be
> great. If it requires writing a set of functionalities, than that will
> need to wait till later.

To cope with the "one is already established and it is already locked"
case, the only option is to have a fixmap entry which can be set
dynamically.  The problem is that the fixmap region is marked NX and 64G
away from .text.

Possibly the least bad option is to have some build-time space (so 0 or
4k depending on CONFIG_HYPERV) between the per-cpu stubs and
XEN_VIRT_END, which operates like the fixmap, but ends up as X/RO mappings.

That way, the virtual address ends up in a useful position (wrt using
direct call instructions) irrespective of where the gfn is/ends up.  As
for guessing, a good start is probably MAXPHYSADDR.

~Andrew

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

Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page

[Wei Liu]

On Sun, Jan 05, 2020 at 09:57:56PM +0000, Andrew Cooper wrote:
[...]
> >
> >> The locked bit is probably a good idea, but one aspect missing here is
> >> the check to see whether the hypercall page is already enabled, which I
> >> expect is for a kexec crash scenario.
> >>
> >> However, the most important point is the one which describes the #GP
> >> properties of the guest trying to modify the page.  This can only be
> >> achieved with an EPT/NPT mapping lacking the W permission, which will
> >> shatter host superpages.   Therefore, putting it in .text is going to be
> >> rather poor, perf wise.
> >>
> >> I also note that Xen's implementation of the Viridian hypercall page
> >> doesn't conform to these properties, and wants fixing.  It is going to
> >> need a new kind identification of the page (probably a new p2m type)
> >> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
> >>
> >> As for suggestions here, I'm struggling to find any memory map details
> >> exposed in the Viridian interface, and therefore which gfn is best to
> >> choose.  I have a sinking feeling that the answer is ACPI...
> > TLFS only says "go find one suitable page yourself" without further
> > hints.
> >
> > Since we're still quite far away from a functioning system, finding a
> > most suitable page isn't my top priority at this point. If there is a
> > simple way to extrapolate suitable information from ACPI, that would be
> > great. If it requires writing a set of functionalities, than that will
> > need to wait till later.
> 
> To cope with the "one is already established and it is already locked"
> case, the only option is to have a fixmap entry which can be set
> dynamically.  The problem is that the fixmap region is marked NX and 64G
> away from .text.
> 
> Possibly the least bad option is to have some build-time space (so 0 or
> 4k depending on CONFIG_HYPERV) between the per-cpu stubs and
> XEN_VIRT_END, which operates like the fixmap, but ends up as X/RO mappings.
> 

OK. This is probably not too difficult. 

> That way, the virtual address ends up in a useful position (wrt using
> direct call instructions) irrespective of where the gfn is/ends up.  As
> for guessing, a good start is probably MAXPHYSADDR.

To make sure I understand your correctly: you're talking about using the
page just below MAXPHYSADDR (derived from paddr_bits from xen source),
right?

Wei.

> 
> ~Andrew

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

Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page

[Wei Liu]

On Tue, Jan 07, 2020 at 03:42:14PM +0000, Wei Liu wrote:
> On Sun, Jan 05, 2020 at 09:57:56PM +0000, Andrew Cooper wrote:
> [...]
> > >
> > >> The locked bit is probably a good idea, but one aspect missing here is
> > >> the check to see whether the hypercall page is already enabled, which I
> > >> expect is for a kexec crash scenario.
> > >>
> > >> However, the most important point is the one which describes the #GP
> > >> properties of the guest trying to modify the page.  This can only be
> > >> achieved with an EPT/NPT mapping lacking the W permission, which will
> > >> shatter host superpages.   Therefore, putting it in .text is going to be
> > >> rather poor, perf wise.
> > >>
> > >> I also note that Xen's implementation of the Viridian hypercall page
> > >> doesn't conform to these properties, and wants fixing.  It is going to
> > >> need a new kind identification of the page (probably a new p2m type)
> > >> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
> > >>
> > >> As for suggestions here, I'm struggling to find any memory map details
> > >> exposed in the Viridian interface, and therefore which gfn is best to
> > >> choose.  I have a sinking feeling that the answer is ACPI...
> > > TLFS only says "go find one suitable page yourself" without further
> > > hints.
> > >
> > > Since we're still quite far away from a functioning system, finding a
> > > most suitable page isn't my top priority at this point. If there is a
> > > simple way to extrapolate suitable information from ACPI, that would be
> > > great. If it requires writing a set of functionalities, than that will
> > > need to wait till later.
> > 
> > To cope with the "one is already established and it is already locked"
> > case, the only option is to have a fixmap entry which can be set
> > dynamically.  The problem is that the fixmap region is marked NX and 64G
> > away from .text.
> > 
> > Possibly the least bad option is to have some build-time space (so 0 or
> > 4k depending on CONFIG_HYPERV) between the per-cpu stubs and
> > XEN_VIRT_END, which operates like the fixmap, but ends up as X/RO mappings.
> > 
> 
> OK. This is probably not too difficult. 
> 

I have a closer look at this today and want to make sure I understand
what you had in mind.

Suppose we set aside some space in linker script. Using the following
will give you a WAX section. I still need to add CONFIG_GUEST around it,
but you get the idea.


diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 111edb5360..a7af321139 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -305,6 +305,15 @@ SECTIONS
        . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
   } :text
+
+  . = ALIGN(SECTION_ALIGN);
+  DECL_SECTION(.text.hypercall_page) {
+       . = ALIGN(PAGE_SIZE);
+       __hypercall_page_start = .;
+       . = . + PAGE_SIZE;
+       __hypercall_page_end = .;
+  } :text=0x9090
+
   _end = . ;
 
   . = ALIGN(SECTION_ALIGN);


And then? Use map_pages_to_xen(..., PAGE_HYPERVISOR_RX) to point that
address to (MAXPHYSADDR-PAGE_SIZE)?

Wei.

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

Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page

[Andrew Cooper]

On 08/01/2020 17:43, Wei Liu wrote:
> On Tue, Jan 07, 2020 at 03:42:14PM +0000, Wei Liu wrote:
>> On Sun, Jan 05, 2020 at 09:57:56PM +0000, Andrew Cooper wrote:
>> [...]
>>>>> The locked bit is probably a good idea, but one aspect missing here is
>>>>> the check to see whether the hypercall page is already enabled, which I
>>>>> expect is for a kexec crash scenario.
>>>>>
>>>>> However, the most important point is the one which describes the #GP
>>>>> properties of the guest trying to modify the page.  This can only be
>>>>> achieved with an EPT/NPT mapping lacking the W permission, which will
>>>>> shatter host superpages.   Therefore, putting it in .text is going to be
>>>>> rather poor, perf wise.
>>>>>
>>>>> I also note that Xen's implementation of the Viridian hypercall page
>>>>> doesn't conform to these properties, and wants fixing.  It is going to
>>>>> need a new kind identification of the page (probably a new p2m type)
>>>>> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
>>>>>
>>>>> As for suggestions here, I'm struggling to find any memory map details
>>>>> exposed in the Viridian interface, and therefore which gfn is best to
>>>>> choose.  I have a sinking feeling that the answer is ACPI...
>>>> TLFS only says "go find one suitable page yourself" without further
>>>> hints.
>>>>
>>>> Since we're still quite far away from a functioning system, finding a
>>>> most suitable page isn't my top priority at this point. If there is a
>>>> simple way to extrapolate suitable information from ACPI, that would be
>>>> great. If it requires writing a set of functionalities, than that will
>>>> need to wait till later.
>>> To cope with the "one is already established and it is already locked"
>>> case, the only option is to have a fixmap entry which can be set
>>> dynamically.  The problem is that the fixmap region is marked NX and 64G
>>> away from .text.
>>>
>>> Possibly the least bad option is to have some build-time space (so 0 or
>>> 4k depending on CONFIG_HYPERV) between the per-cpu stubs and
>>> XEN_VIRT_END, which operates like the fixmap, but ends up as X/RO mappings.
>>>
>> OK. This is probably not too difficult. 
>>
> I have a closer look at this today and want to make sure I understand
> what you had in mind.
>
> Suppose we set aside some space in linker script. Using the following
> will give you a WAX section. I still need to add CONFIG_GUEST around it,
> but you get the idea.
>
>
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 111edb5360..a7af321139 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -305,6 +305,15 @@ SECTIONS
>         . = ALIGN(POINTER_ALIGN);
>         __bss_end = .;
>    } :text
> +
> +  . = ALIGN(SECTION_ALIGN);
> +  DECL_SECTION(.text.hypercall_page) {
> +       . = ALIGN(PAGE_SIZE);
> +       __hypercall_page_start = .;
> +       . = . + PAGE_SIZE;
> +       __hypercall_page_end = .;
> +  } :text=0x9090
> +
>    _end = . ;
>  
>    . = ALIGN(SECTION_ALIGN);
>
>
> And then? Use map_pages_to_xen(..., PAGE_HYPERVISOR_RX) to point that
> address to (MAXPHYSADDR-PAGE_SIZE)?

Ah no.  This still puts the hypercall page (or at least, space for it)
in the Xen image itself, which is something we are trying to avoid.

What I meant was to basically copy(/extend) the existing fixmap
implementation, calling it fixmap_x() (or something better), and put
FIXMAP_X_SIZE as an additional gap in the calculation
alloc_stub_page().  Even the current fixmap code has an ifdef example
for CONFIG_XEN_GUEST.

You'd then end up with something like
__set_fixmap_x(FIXMAP_X_HYPERV_HYPERCALL_PAGE, mfn) which uses _RX in
the underlying call to map_pages_to_xen()

~Andrew

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

Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page

[Wei Liu]

On Wed, Jan 08, 2020 at 05:53:36PM +0000, Andrew Cooper wrote:
> On 08/01/2020 17:43, Wei Liu wrote:
> > On Tue, Jan 07, 2020 at 03:42:14PM +0000, Wei Liu wrote:
> >> On Sun, Jan 05, 2020 at 09:57:56PM +0000, Andrew Cooper wrote:
> >> [...]
> >>>>> The locked bit is probably a good idea, but one aspect missing here is
> >>>>> the check to see whether the hypercall page is already enabled, which I
> >>>>> expect is for a kexec crash scenario.
> >>>>>
> >>>>> However, the most important point is the one which describes the #GP
> >>>>> properties of the guest trying to modify the page.  This can only be
> >>>>> achieved with an EPT/NPT mapping lacking the W permission, which will
> >>>>> shatter host superpages.   Therefore, putting it in .text is going to be
> >>>>> rather poor, perf wise.
> >>>>>
> >>>>> I also note that Xen's implementation of the Viridian hypercall page
> >>>>> doesn't conform to these properties, and wants fixing.  It is going to
> >>>>> need a new kind identification of the page (probably a new p2m type)
> >>>>> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
> >>>>>
> >>>>> As for suggestions here, I'm struggling to find any memory map details
> >>>>> exposed in the Viridian interface, and therefore which gfn is best to
> >>>>> choose.  I have a sinking feeling that the answer is ACPI...
> >>>> TLFS only says "go find one suitable page yourself" without further
> >>>> hints.
> >>>>
> >>>> Since we're still quite far away from a functioning system, finding a
> >>>> most suitable page isn't my top priority at this point. If there is a
> >>>> simple way to extrapolate suitable information from ACPI, that would be
> >>>> great. If it requires writing a set of functionalities, than that will
> >>>> need to wait till later.
> >>> To cope with the "one is already established and it is already locked"
> >>> case, the only option is to have a fixmap entry which can be set
> >>> dynamically.  The problem is that the fixmap region is marked NX and 64G
> >>> away from .text.
> >>>
> >>> Possibly the least bad option is to have some build-time space (so 0 or
> >>> 4k depending on CONFIG_HYPERV) between the per-cpu stubs and
> >>> XEN_VIRT_END, which operates like the fixmap, but ends up as X/RO mappings.
> >>>
> >> OK. This is probably not too difficult. 
> >>
> > I have a closer look at this today and want to make sure I understand
> > what you had in mind.
> >
> > Suppose we set aside some space in linker script. Using the following
> > will give you a WAX section. I still need to add CONFIG_GUEST around it,
> > but you get the idea.
> >
> >
> > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > index 111edb5360..a7af321139 100644
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -305,6 +305,15 @@ SECTIONS
> >         . = ALIGN(POINTER_ALIGN);
> >         __bss_end = .;
> >    } :text
> > +
> > +  . = ALIGN(SECTION_ALIGN);
> > +  DECL_SECTION(.text.hypercall_page) {
> > +       . = ALIGN(PAGE_SIZE);
> > +       __hypercall_page_start = .;
> > +       . = . + PAGE_SIZE;
> > +       __hypercall_page_end = .;
> > +  } :text=0x9090
> > +
> >    _end = . ;
> >  
> >    . = ALIGN(SECTION_ALIGN);
> >
> >
> > And then? Use map_pages_to_xen(..., PAGE_HYPERVISOR_RX) to point that
> > address to (MAXPHYSADDR-PAGE_SIZE)?
> 
> Ah no.  This still puts the hypercall page (or at least, space for it)
> in the Xen image itself, which is something we are trying to avoid.
> 
> What I meant was to basically copy(/extend) the existing fixmap
> implementation, calling it fixmap_x() (or something better), and put
> FIXMAP_X_SIZE as an additional gap in the calculation
> alloc_stub_page().  Even the current fixmap code has an ifdef example
> for CONFIG_XEN_GUEST.

Not just alloc_stub_page, but also livepatch infrastructure's address
space arrangement -- see arch/x86/liveptch.c:arch_livepatch_init.

Suppose I copy the existing fixmap mechanism, to get the uniform
experience for these two variants for fixmap, I will also need to
statically allocate its l2 and l1 page tables. This ends up putting two
more pages into Xen's image.  I want to make sure this is what you want
because you seemed to be concern about enlarging image size.

Wei.


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

[Xen-devel] [PATCH v3 2/5] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

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

I couldn't find reference in TLFS that Hyper-V clobbers flags and
r9-r11, but Linux's commit message says it does. Err on the safe side.

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
v3:
1. Name the file hyperv-hcall.h

v2:
1. Use direct call
---
 xen/include/asm-x86/guest/hyperv-hcall.h | 95 ++++++++++++++++++++++++
 1 file changed, 95 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..4b87dcfe64
--- /dev/null
+++ b/xen/include/asm-x86/guest/hyperv-hcall.h
@@ -0,0 +1,95 @@
+/******************************************************************************
+ * 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/types.h>
+
+#include <asm/asm_defns.h>
+#include <asm/guest/hyperv-tlfs.h>
+#include <asm/page.h>
+
+static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input, paddr_t output)
+{
+    uint64_t status;
+
+    asm volatile ("mov %[output], %%r8\n"
+                  "call hv_hypercall_page"
+                  : "=a" (status), "+c" (control),
+                    "+d" (input) ASM_CALL_CONSTRAINT
+                  : [output] "rm" (output)
+                  : "cc", "memory", "r8", "r9", "r10", "r11");
+
+    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 = (uint64_t)code | HV_HYPERCALL_FAST_BIT;
+
+    asm volatile ("mov %[input2], %%r8\n"
+                  "call hv_hypercall_page"
+                  : "=a" (status), "+c" (control),
+                    "+d" (input1) ASM_CALL_CONSTRAINT
+                  : [input2] "rm" (input2)
+                  : "cc", "r8", "r9", "r10", "r11");
+
+    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 = (status & HV_HYPERCALL_REP_COMP_MASK) >>
+            HV_HYPERCALL_REP_COMP_OFFSET;
+
+        control &= ~HV_HYPERCALL_REP_START_MASK;
+        control |= (uint64_t)rep_comp << HV_HYPERCALL_REP_COMP_OFFSET;
+
+    } 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 v3 2/5] x86/hyperv: provide Hyper-V hypercall functions

[Andrew Cooper]

> +static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input, paddr_t output)
> +{
> +    uint64_t status;
> +
> +    asm volatile ("mov %[output], %%r8\n"
> +                  "call hv_hypercall_page"
> +                  : "=a" (status), "+c" (control),
> +                    "+d" (input) ASM_CALL_CONSTRAINT
> +                  : [output] "rm" (output)
> +                  : "cc", "memory", "r8", "r9", "r10", "r11");

I think you want:

register unsigned long r8 asm("r8") = output;

and "+r" (r8) as an output constraint.

In particular, that doesn't force the compiler to put output into a
register other than r8 (or worse, spill it to the stack) to have the
opaque blob of asm move it back into r8.  What it will do in practice is
cause the compiler to construct output directly in r8.

As for the other clobbers, I can't find anything at all in the spec
which even mentions those registers.  There will be a decent improvement
to code generation if we don't force them to be spilled around a hypercall.

However, HyperV will(may?) modify %xmm{0..5} in some cases, and this
will corrupt the current vcpu's FPU context.  Care will need to be taken
to spill these if necessary.

~Andrew

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

Re: [Xen-devel] [PATCH v3 2/5] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Sun, Jan 05, 2020 at 07:08:28PM +0000, Andrew Cooper wrote:
> 
> > +static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input, paddr_t output)
> > +{
> > +    uint64_t status;
> > +
> > +    asm volatile ("mov %[output], %%r8\n"
> > +                  "call hv_hypercall_page"
> > +                  : "=a" (status), "+c" (control),
> > +                    "+d" (input) ASM_CALL_CONSTRAINT
> > +                  : [output] "rm" (output)
> > +                  : "cc", "memory", "r8", "r9", "r10", "r11");
> 
> I think you want:
> 
> register unsigned long r8 asm("r8") = output;
> 
> and "+r" (r8) as an output constraint.

Although it is named `output`, it is really just an input parameter from
Hyper-V's PoV. It contains the address of the page Hyper-V should write
its output to.

> 
> In particular, that doesn't force the compiler to put output into a
> register other than r8 (or worse, spill it to the stack) to have the
> opaque blob of asm move it back into r8.  What it will do in practice is
> cause the compiler to construct output directly in r8.
> 
> As for the other clobbers, I can't find anything at all in the spec
> which even mentions those registers.  There will be a decent improvement
> to code generation if we don't force them to be spilled around a hypercall.
> 

Neither can I. But Linux's commit says that's needed, so I chose to err
on the safe side.

I'm fine with dropping r9-r11.

> However, HyperV will(may?) modify %xmm{0..5} in some cases, and this
> will corrupt the current vcpu's FPU context.  Care will need to be taken
> to spill these if necessary.
> 

The hypercalls we care about (TLB, APIC etc) don't use that ABI as far
as I can tell. At the very least Linux, which uses the same set of
hypercalls, doesn't need to save / restore XMM registers around the
calls.

Wei.

> ~Andrew

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

Re: [Xen-devel] [PATCH v3 2/5] x86/hyperv: provide Hyper-V hypercall functions

[Andrew Cooper]

On 05/01/2020 21:22, Wei Liu wrote:
> On Sun, Jan 05, 2020 at 07:08:28PM +0000, Andrew Cooper wrote:
>>> +static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input, paddr_t output)
>>> +{
>>> +    uint64_t status;
>>> +
>>> +    asm volatile ("mov %[output], %%r8\n"
>>> +                  "call hv_hypercall_page"
>>> +                  : "=a" (status), "+c" (control),
>>> +                    "+d" (input) ASM_CALL_CONSTRAINT
>>> +                  : [output] "rm" (output)
>>> +                  : "cc", "memory", "r8", "r9", "r10", "r11");
>> I think you want:
>>
>> register unsigned long r8 asm("r8") = output;
>>
>> and "+r" (r8) as an output constraint.
> Although it is named `output`, it is really just an input parameter from
> Hyper-V's PoV.

Yes, but it is also clobbered.

This is an awkward corner case of gnu inline assembly.

It is not permitted to have a clobber list overlap with any input/output
operations, and because r8 doesn't have a unique letter, you can't do
the usual trick of "=r8" (discard) : "r8" (input).

The only available option is to mark it as read and written (which is
"+r" in the output list), and not use the C variable r8 at any point later.


Having looked through the spec a bit more, is this a wise API to have in
the first place?  input and output (perhaps better named input_addr and
output_addr) are fixed per CPU, and control is semantically linked to
the hypercall and its particular ABI.

I suppose the answer ultimately depends on what the callers look like.

>
>> In particular, that doesn't force the compiler to put output into a
>> register other than r8 (or worse, spill it to the stack) to have the
>> opaque blob of asm move it back into r8.  What it will do in practice is
>> cause the compiler to construct output directly in r8.
>>
>> As for the other clobbers, I can't find anything at all in the spec
>> which even mentions those registers.  There will be a decent improvement
>> to code generation if we don't force them to be spilled around a hypercall.
>>
> Neither can I. But Linux's commit says that's needed, so I chose to err
> on the safe side.

That's dull.  Is there any qualifying information?

>> However, HyperV will(may?) modify %xmm{0..5} in some cases, and this
>> will corrupt the current vcpu's FPU context.  Care will need to be taken
>> to spill these if necessary.
>>
> The hypercalls we care about (TLB, APIC etc) don't use that ABI as far
> as I can tell. At the very least Linux, which uses the same set of
> hypercalls, doesn't need to save / restore XMM registers around the
> calls.

Ok - it looks to be fine until we need to use a hypercall which uses the
fast extended ABI, which is the first to introduce the use of the %xmm
registers.

~Andrew

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

Re: [Xen-devel] [PATCH v3 2/5] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Sun, Jan 05, 2020 at 10:06:08PM +0000, Andrew Cooper wrote:
> On 05/01/2020 21:22, Wei Liu wrote:
> > On Sun, Jan 05, 2020 at 07:08:28PM +0000, Andrew Cooper wrote:
> >>> +static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input, paddr_t output)
> >>> +{
> >>> +    uint64_t status;
> >>> +
> >>> +    asm volatile ("mov %[output], %%r8\n"
> >>> +                  "call hv_hypercall_page"
> >>> +                  : "=a" (status), "+c" (control),
> >>> +                    "+d" (input) ASM_CALL_CONSTRAINT
> >>> +                  : [output] "rm" (output)
> >>> +                  : "cc", "memory", "r8", "r9", "r10", "r11");
> >> I think you want:
> >>
> >> register unsigned long r8 asm("r8") = output;
> >>
> >> and "+r" (r8) as an output constraint.
> > Although it is named `output`, it is really just an input parameter from
> > Hyper-V's PoV.
> 
> Yes, but it is also clobbered.
> 
> This is an awkward corner case of gnu inline assembly.
> 
> It is not permitted to have a clobber list overlap with any input/output
> operations, and because r8 doesn't have a unique letter, you can't do
> the usual trick of "=r8" (discard) : "r8" (input).
> 
> The only available option is to mark it as read and written (which is
> "+r" in the output list), and not use the C variable r8 at any point later.

But r8 is only listed in clobber list, so it certainly doesn't overlap
with any input register. I fail to see what the bug (if there is any) is
here.

I think what you're asking for here is an optimisation. Is that correct?
I don't mind changing the code. What I need is clarification here.

> 
> 
> Having looked through the spec a bit more, is this a wise API to have in
> the first place?  input and output (perhaps better named input_addr and
> output_addr) are fixed per CPU, and control is semantically linked to
> the hypercall and its particular ABI.
> 
> I suppose the answer ultimately depends on what the callers look like.

The call sites will be like

	struct hv_input_arg *input_arg;
	input_arg = per_cpu_input_page;
	input_arg.foo = xxx;
	input_arg.bar = xxx;

	hv_do_hypercall(control, virt_to_maddr(input_arg), NULL);

.

(Alternatively, we can put virt_to_maddr in hv_do_hypercall now that
we're sure the input page is from xenheap)

> 
> >
> >> In particular, that doesn't force the compiler to put output into a
> >> register other than r8 (or worse, spill it to the stack) to have the
> >> opaque blob of asm move it back into r8.  What it will do in practice is
> >> cause the compiler to construct output directly in r8.
> >>
> >> As for the other clobbers, I can't find anything at all in the spec
> >> which even mentions those registers.  There will be a decent improvement
> >> to code generation if we don't force them to be spilled around a hypercall.
> >>
> > Neither can I. But Linux's commit says that's needed, so I chose to err
> > on the safe side.
> 
> That's dull.  Is there any qualifying information?

See Linux commit fc53662f13b.

I will also ask my contact in Hyper-V team for clarification.

Wei.

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

Re: [Xen-devel] [PATCH v3 2/5] x86/hyperv: provide Hyper-V hypercall functions

[Andrew Cooper]

On 07/01/2020 16:17, Wei Liu wrote:
> On Sun, Jan 05, 2020 at 10:06:08PM +0000, Andrew Cooper wrote:
>> On 05/01/2020 21:22, Wei Liu wrote:
>>> On Sun, Jan 05, 2020 at 07:08:28PM +0000, Andrew Cooper wrote:
>>>>> +static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input, paddr_t output)
>>>>> +{
>>>>> +    uint64_t status;
>>>>> +
>>>>> +    asm volatile ("mov %[output], %%r8\n"
>>>>> +                  "call hv_hypercall_page"
>>>>> +                  : "=a" (status), "+c" (control),
>>>>> +                    "+d" (input) ASM_CALL_CONSTRAINT
>>>>> +                  : [output] "rm" (output)
>>>>> +                  : "cc", "memory", "r8", "r9", "r10", "r11");
>>>> I think you want:
>>>>
>>>> register unsigned long r8 asm("r8") = output;
>>>>
>>>> and "+r" (r8) as an output constraint.
>>> Although it is named `output`, it is really just an input parameter from
>>> Hyper-V's PoV.
>> Yes, but it is also clobbered.
>>
>> This is an awkward corner case of gnu inline assembly.
>>
>> It is not permitted to have a clobber list overlap with any input/output
>> operations, and because r8 doesn't have a unique letter, you can't do
>> the usual trick of "=r8" (discard) : "r8" (input).
>>
>> The only available option is to mark it as read and written (which is
>> "+r" in the output list), and not use the C variable r8 at any point later.
> But r8 is only listed in clobber list, so it certainly doesn't overlap
> with any input register. I fail to see what the bug (if there is any) is
> here.

%r8 is an input parameter.  You have "mov %[output], %%r8" in the asm.

The way this is written causes the compiler to construct %[output] in a
register, pass it in via the sole input parameter, and behind the scenes
move it into %r8.

There is a small chance of the emitted asm being "mov %r8, %r8", and a
much larger chance of the compiler being forced to spill a different
register when it could have used %r8 in the first place.

> I think what you're asking for here is an optimisation. Is that correct?
> I don't mind changing the code. What I need is clarification here.

I'm on the fence as to whether to put in the category of optimisation,
or buggy asm.

I think the generated code will DTRT, but it is a suspicious looking
piece of assembly, so "optimisation" I guess?

~Andrew

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

Re: [Xen-devel] [PATCH v3 2/5] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Sun, Jan 05, 2020 at 07:08:28PM +0000, Andrew Cooper wrote:
> 
> > +static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input, paddr_t output)
> > +{
> > +    uint64_t status;
> > +
> > +    asm volatile ("mov %[output], %%r8\n"
> > +                  "call hv_hypercall_page"
> > +                  : "=a" (status), "+c" (control),
> > +                    "+d" (input) ASM_CALL_CONSTRAINT
> > +                  : [output] "rm" (output)
> > +                  : "cc", "memory", "r8", "r9", "r10", "r11");
> 
> I think you want:
> 
> register unsigned long r8 asm("r8") = output;
> 
> and "+r" (r8) as an output constraint.
> 
> In particular, that doesn't force the compiler to put output into a
> register other than r8 (or worse, spill it to the stack) to have the
> opaque blob of asm move it back into r8.  What it will do in practice is
> cause the compiler to construct output directly in r8.
> 
> As for the other clobbers, I can't find anything at all in the spec
> which even mentions those registers.  There will be a decent improvement
> to code generation if we don't force them to be spilled around a hypercall.

This is actually from Windows 2012 R2's TLFS.

Current version of Hyper-V doesn't clobber those registers anymore. I
think just putting "memory" there should be fine.

Wei.

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

Re: [Xen-devel] [PATCH v3 2/5] x86/hyperv: provide Hyper-V hypercall functions

[Jan Beulich]

On 05.01.2020 17:47, Wei Liu wrote:
> +static inline uint64_t hv_do_fast_hypercall(uint16_t code,
> +                                            uint64_t input1, uint64_t input2)
> +{
> +    uint64_t status;
> +    uint64_t control = (uint64_t)code | HV_HYPERCALL_FAST_BIT;

Unnecessary (afaict) cast.

> +    asm volatile ("mov %[input2], %%r8\n"
> +                  "call hv_hypercall_page"
> +                  : "=a" (status), "+c" (control),
> +                    "+d" (input1) ASM_CALL_CONSTRAINT
> +                  : [input2] "rm" (input2)
> +                  : "cc", "r8", "r9", "r10", "r11");
> +
> +    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 = (status & HV_HYPERCALL_REP_COMP_MASK) >>
> +            HV_HYPERCALL_REP_COMP_OFFSET;

MASK_EXTR()? (I then also wonder whether MASK_INSR() would better be
used with some of the other constructs here.)

What's worse though - looking at the definition of
HV_HYPERCALL_REP_COMP_MASK I notice that it and a few others use
GENMASK_ULL(), when it was clearly said during review (perhaps of
another but related patch) that this macro should not be used
outside of Arm-specific code until it gets put into better shape:
https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg00705.html

> +        control &= ~HV_HYPERCALL_REP_START_MASK;
> +        control |= (uint64_t)rep_comp << HV_HYPERCALL_REP_COMP_OFFSET;
> +
> +    } while ( rep_comp < rep_count );

We don't normally have a blank line ahead of the closing brace of a
block.

Jan

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

Re: [Xen-devel] [PATCH v3 2/5] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu]

On Mon, Jan 06, 2020 at 10:38:23AM +0100, Jan Beulich wrote:
[...]
> > +
> > +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 = (status & HV_HYPERCALL_REP_COMP_MASK) >>
> > +            HV_HYPERCALL_REP_COMP_OFFSET;
> 
> MASK_EXTR()? (I then also wonder whether MASK_INSR() would better be
> used with some of the other constructs here.)

Sure, I can see if that can be used.

> 
> What's worse though - looking at the definition of
> HV_HYPERCALL_REP_COMP_MASK I notice that it and a few others use
> GENMASK_ULL(), when it was clearly said during review (perhaps of
> another but related patch) that this macro should not be used
> outside of Arm-specific code until it gets put into better shape:
> https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg00705.html

That's a straight import from Linux. I only made the header build
without further inspection.

That can be fixed, of course.

Wei.

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

[Xen-devel] [PATCH v3 3/5] 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. The easiest 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>
---
v3:
1. Use xenheap page instead
2. Drop page tracking structure
3. Drop Paul's review tag
---
 xen/arch/x86/guest/hyperv/hyperv.c | 20 ++++++++++++++++++++
 xen/include/asm-x86/guest/hyperv.h |  4 ++++
 2 files changed, 24 insertions(+)

diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index 381be2a68c..7e046dfc04 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -27,6 +27,7 @@
 struct ms_hyperv_info __read_mostly ms_hyperv;
 
 extern char hv_hypercall_page[];
+DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_arg);
 
 static const struct hypervisor_ops ops;
 const struct hypervisor_ops *__init hyperv_probe(void)
@@ -83,14 +84,33 @@ static void __init setup_hypercall_page(void)
     wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
 }
 
+static void setup_hypercall_pcpu_arg(void)
+{
+    void *mapping;
+
+    mapping = alloc_xenheap_page();
+    if ( !mapping )
+        panic("Failed to allocate hypercall input page for %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/include/asm-x86/guest/hyperv.h b/xen/include/asm-x86/guest/hyperv.h
index c7a7f32bd5..6cf2eab62f 100644
--- a/xen/include/asm-x86/guest/hyperv.h
+++ b/xen/include/asm-x86/guest/hyperv.h
@@ -51,6 +51,8 @@ static inline uint64_t hv_scale_tsc(uint64_t tsc, uint64_t scale,
 
 #ifdef CONFIG_HYPERV_GUEST
 
+#include <xen/percpu.h>
+
 #include <asm/guest/hypervisor.h>
 
 struct ms_hyperv_info {
@@ -63,6 +65,8 @@ struct ms_hyperv_info {
 };
 extern struct ms_hyperv_info ms_hyperv;
 
+DECLARE_PER_CPU(void *, hv_pcpu_input_arg);
+
 const struct hypervisor_ops *hyperv_probe(void);
 
 #else
-- 
2.20.1


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

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

[Jan Beulich]

On 05.01.2020 17:47, Wei Liu wrote:
> Hyper-V's input / output argument must be 8 bytes aligned an not cross
> page boundary. The easiest way to satisfy those requirements is to use
> percpu page.

I'm not sure "easiest" is really true here. Others could consider adding
__aligned() attributes as easy or even easier (by being even more
transparent to use sites). Could we settle on "One way ..."?

> @@ -83,14 +84,33 @@ static void __init setup_hypercall_page(void)
>      wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>  }
>  
> +static void setup_hypercall_pcpu_arg(void)
> +{
> +    void *mapping;
> +
> +    mapping = alloc_xenheap_page();
> +    if ( !mapping )
> +        panic("Failed to allocate hypercall input page for %u\n",

"... for CPU%u\n" please.

> +              smp_processor_id());
> +
> +    this_cpu(hv_pcpu_input_arg) = mapping;

When offlining and then re-onlining a CPU, the prior page will be
leaked.

> --- a/xen/include/asm-x86/guest/hyperv.h
> +++ b/xen/include/asm-x86/guest/hyperv.h
> @@ -51,6 +51,8 @@ static inline uint64_t hv_scale_tsc(uint64_t tsc, uint64_t scale,
>  
>  #ifdef CONFIG_HYPERV_GUEST
>  
> +#include <xen/percpu.h>
> +
>  #include <asm/guest/hypervisor.h>
>  
>  struct ms_hyperv_info {
> @@ -63,6 +65,8 @@ struct ms_hyperv_info {
>  };
>  extern struct ms_hyperv_info ms_hyperv;
>  
> +DECLARE_PER_CPU(void *, hv_pcpu_input_arg);

Will this really be needed outside of the file that defines it?

Also, while looking at this I notice that - despite my earlier
comment when giving the respective, sort-of-conditional ack -
there are (still) many apparently pointless __packed attributes
in hyperv-tlfs.h. Care to comment on this?

Jan

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

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

[Wei Liu]

On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote:
> On 05.01.2020 17:47, Wei Liu wrote:
> > Hyper-V's input / output argument must be 8 bytes aligned an not cross
> > page boundary. The easiest way to satisfy those requirements is to use
> > percpu page.
> 
> I'm not sure "easiest" is really true here. Others could consider adding
> __aligned() attributes as easy or even easier (by being even more
> transparent to use sites). Could we settle on "One way ..."?

Do you mean something like

   struct foo __aligned(8);

   hv_do_hypercall(OP, virt_to_maddr(&foo), ...);

?

I don't think this is transparent to user sites. Plus, foo is on stack
which is 1) difficult to get its maddr, 2) may cross page boundary.

If I misunderstood what you meant, please give me an example here.

> 
> > @@ -83,14 +84,33 @@ static void __init setup_hypercall_page(void)
> >      wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> >  }
> >  
> > +static void setup_hypercall_pcpu_arg(void)
> > +{
> > +    void *mapping;
> > +
> > +    mapping = alloc_xenheap_page();
> > +    if ( !mapping )
> > +        panic("Failed to allocate hypercall input page for %u\n",
> 
> "... for CPU%u\n" please.
> 
> > +              smp_processor_id());
> > +
> > +    this_cpu(hv_pcpu_input_arg) = mapping;
> 
> When offlining and then re-onlining a CPU, the prior page will be
> leaked.

Right. Thanks for catching this one.

> 
> > --- a/xen/include/asm-x86/guest/hyperv.h
> > +++ b/xen/include/asm-x86/guest/hyperv.h
> > @@ -51,6 +51,8 @@ static inline uint64_t hv_scale_tsc(uint64_t tsc, uint64_t scale,
> >  
> >  #ifdef CONFIG_HYPERV_GUEST
> >  
> > +#include <xen/percpu.h>
> > +
> >  #include <asm/guest/hypervisor.h>
> >  
> >  struct ms_hyperv_info {
> > @@ -63,6 +65,8 @@ struct ms_hyperv_info {
> >  };
> >  extern struct ms_hyperv_info ms_hyperv;
> >  
> > +DECLARE_PER_CPU(void *, hv_pcpu_input_arg);
> 
> Will this really be needed outside of the file that defines it?
> 

This can live in a private header for the time being.

> Also, while looking at this I notice that - despite my earlier
> comment when giving the respective, sort-of-conditional ack -
> there are (still) many apparently pointless __packed attributes
> in hyperv-tlfs.h. Care to comment on this?

Again, that's a straight import from Linux. I tried not to deviate too
much. A commit in Linux (ec084491727b0) claims "compiler can add
alignment padding to structures or reorder struct members for
randomization and optimization".

I just checked all the packed structures. They seem to have all the
required manual paddings already. I can only assume they tried to erred
on the safe side.

Wei.

> 
> Jan

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

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

[Michael Kelley]

From: Wei Liu <wl@xen.org> Sent: Tuesday, January 7, 2020 8:34 AM
> 
> On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote:
> > On 05.01.2020 17:47, Wei Liu wrote:
> > > Hyper-V's input / output argument must be 8 bytes aligned an not cross
> > > page boundary. The easiest way to satisfy those requirements is to use
> > > percpu page.
> >
> > I'm not sure "easiest" is really true here. Others could consider adding
> > __aligned() attributes as easy or even easier (by being even more
> > transparent to use sites). Could we settle on "One way ..."?
> 
> Do you mean something like
> 
>    struct foo __aligned(8);
> 
>    hv_do_hypercall(OP, virt_to_maddr(&foo), ...);
> 
> ?
> 
> I don't think this is transparent to user sites. Plus, foo is on stack
> which is 1) difficult to get its maddr, 2) may cross page boundary.
> 
> If I misunderstood what you meant, please give me an example here.
> 
> >
> > > @@ -83,14 +84,33 @@ static void __init setup_hypercall_page(void)
> > >      wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > >  }
> > >
> > > +static void setup_hypercall_pcpu_arg(void)
> > > +{
> > > +    void *mapping;
> > > +
> > > +    mapping = alloc_xenheap_page();
> > > +    if ( !mapping )
> > > +        panic("Failed to allocate hypercall input page for %u\n",
> >
> > "... for CPU%u\n" please.
> >
> > > +              smp_processor_id());
> > > +
> > > +    this_cpu(hv_pcpu_input_arg) = mapping;
> >
> > When offlining and then re-onlining a CPU, the prior page will be
> > leaked.
> 
> Right. Thanks for catching this one.
> 
> >
> > > --- a/xen/include/asm-x86/guest/hyperv.h
> > > +++ b/xen/include/asm-x86/guest/hyperv.h
> > > @@ -51,6 +51,8 @@ static inline uint64_t hv_scale_tsc(uint64_t tsc, uint64_t scale,
> > >
> > >  #ifdef CONFIG_HYPERV_GUEST
> > >
> > > +#include <xen/percpu.h>
> > > +
> > >  #include <asm/guest/hypervisor.h>
> > >
> > >  struct ms_hyperv_info {
> > > @@ -63,6 +65,8 @@ struct ms_hyperv_info {
> > >  };
> > >  extern struct ms_hyperv_info ms_hyperv;
> > >
> > > +DECLARE_PER_CPU(void *, hv_pcpu_input_arg);
> >
> > Will this really be needed outside of the file that defines it?
> >
> 
> This can live in a private header for the time being.
> 
> > Also, while looking at this I notice that - despite my earlier
> > comment when giving the respective, sort-of-conditional ack -
> > there are (still) many apparently pointless __packed attributes
> > in hyperv-tlfs.h. Care to comment on this?
> 
> Again, that's a straight import from Linux. I tried not to deviate too
> much. A commit in Linux (ec084491727b0) claims "compiler can add
> alignment padding to structures or reorder struct members for
> randomization and optimization".
> 
> I just checked all the packed structures. They seem to have all the
> required manual paddings already. I can only assume they tried to erred
> on the safe side.

Correct.  The __packed attribute was added only about a year ago
after somebody on LKML noticed that the structures were not packed.
Some discussion ensued, but the consensus was to add __packed due
to general  paranoia about what the compiler might do even though
individual fields are aligned to their natural boundary.

Michael

> 
> Wei.
> 
> >
> > Jan

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

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

[Jan Beulich]

On 07.01.2020 17:45, Michael Kelley wrote:
> From: Wei Liu <wl@xen.org> Sent: Tuesday, January 7, 2020 8:34 AM
>>
>> On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote:
>>> On 05.01.2020 17:47, Wei Liu wrote:
>>>> Hyper-V's input / output argument must be 8 bytes aligned an not cross
>>>> page boundary. The easiest way to satisfy those requirements is to use
>>>> percpu page.
>>>
>>> I'm not sure "easiest" is really true here. Others could consider adding
>>> __aligned() attributes as easy or even easier (by being even more
>>> transparent to use sites). Could we settle on "One way ..."?
>>
>> Do you mean something like
>>
>>    struct foo __aligned(8);
>>
>>    hv_do_hypercall(OP, virt_to_maddr(&foo), ...);
>>
>> ?
>>
>> I don't think this is transparent to user sites. Plus, foo is on stack
>> which is 1) difficult to get its maddr, 2) may cross page boundary.
>>
>> If I misunderstood what you meant, please give me an example here.
>>
>>>
>>>> @@ -83,14 +84,33 @@ static void __init setup_hypercall_page(void)
>>>>      wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>>>>  }
>>>>
>>>> +static void setup_hypercall_pcpu_arg(void)
>>>> +{
>>>> +    void *mapping;
>>>> +
>>>> +    mapping = alloc_xenheap_page();
>>>> +    if ( !mapping )
>>>> +        panic("Failed to allocate hypercall input page for %u\n",
>>>
>>> "... for CPU%u\n" please.
>>>
>>>> +              smp_processor_id());
>>>> +
>>>> +    this_cpu(hv_pcpu_input_arg) = mapping;
>>>
>>> When offlining and then re-onlining a CPU, the prior page will be
>>> leaked.
>>
>> Right. Thanks for catching this one.
>>
>>>
>>>> --- a/xen/include/asm-x86/guest/hyperv.h
>>>> +++ b/xen/include/asm-x86/guest/hyperv.h
>>>> @@ -51,6 +51,8 @@ static inline uint64_t hv_scale_tsc(uint64_t tsc, uint64_t scale,
>>>>
>>>>  #ifdef CONFIG_HYPERV_GUEST
>>>>
>>>> +#include <xen/percpu.h>
>>>> +
>>>>  #include <asm/guest/hypervisor.h>
>>>>
>>>>  struct ms_hyperv_info {
>>>> @@ -63,6 +65,8 @@ struct ms_hyperv_info {
>>>>  };
>>>>  extern struct ms_hyperv_info ms_hyperv;
>>>>
>>>> +DECLARE_PER_CPU(void *, hv_pcpu_input_arg);
>>>
>>> Will this really be needed outside of the file that defines it?
>>>
>>
>> This can live in a private header for the time being.
>>
>>> Also, while looking at this I notice that - despite my earlier
>>> comment when giving the respective, sort-of-conditional ack -
>>> there are (still) many apparently pointless __packed attributes
>>> in hyperv-tlfs.h. Care to comment on this?
>>
>> Again, that's a straight import from Linux. I tried not to deviate too
>> much. A commit in Linux (ec084491727b0) claims "compiler can add
>> alignment padding to structures or reorder struct members for
>> randomization and optimization".
>>
>> I just checked all the packed structures. They seem to have all the
>> required manual paddings already. I can only assume they tried to erred
>> on the safe side.
> 
> Correct.  The __packed attribute was added only about a year ago
> after somebody on LKML noticed that the structures were not packed.
> Some discussion ensued, but the consensus was to add __packed due
> to general  paranoia about what the compiler might do even though
> individual fields are aligned to their natural boundary.

Which, as wwe've found the hard way, then leads to problems (with at
least gcc 9) when wanting to take the address of a member of such a
structure, as the compiler then (mostly validly) assumes the pointer
won't be naturally aligned. Hence, as also said in reply to Wei
elsewhere, we found it necessary to drop various __packed in order
to be able to build Xen with gcc 9.

Jan

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

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

[Jan Beulich]

On 07.01.2020 17:33, Wei Liu wrote:
> On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote:
>> On 05.01.2020 17:47, Wei Liu wrote:
>>> Hyper-V's input / output argument must be 8 bytes aligned an not cross
>>> page boundary. The easiest way to satisfy those requirements is to use
>>> percpu page.
>>
>> I'm not sure "easiest" is really true here. Others could consider adding
>> __aligned() attributes as easy or even easier (by being even more
>> transparent to use sites). Could we settle on "One way ..."?
> 
> Do you mean something like
> 
>    struct foo __aligned(8);

If this is in a header and ...

>    hv_do_hypercall(OP, virt_to_maddr(&foo), ...);

... this in actual code, then yes.

> ?
> 
> I don't think this is transparent to user sites. Plus, foo is on stack
> which is 1) difficult to get its maddr,

It being on the stack may indeed complicate getting its machine address
(if not now, then down the road) - valid point.

> 2) may cross page boundary.

The __aligned() of course needs to be large enough to avoid this
happening.

>> Also, while looking at this I notice that - despite my earlier
>> comment when giving the respective, sort-of-conditional ack -
>> there are (still) many apparently pointless __packed attributes
>> in hyperv-tlfs.h. Care to comment on this?
> 
> Again, that's a straight import from Linux. I tried not to deviate too
> much. A commit in Linux (ec084491727b0) claims "compiler can add
> alignment padding to structures or reorder struct members for
> randomization and optimization".

Would a compiler doing so (without explicitly being told to) even
be in line with the C spec? I'd buy such a claim only if I see an
example proving it.

> I just checked all the packed structures. They seem to have all the
> required manual paddings already. I can only assume they tried to erred
> on the safe side.

And you surely recall we had to remove quite a few instances of
__packed for gcc 9 compatibility?

Jan

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

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

[Wei Liu]

On Tue, Jan 07, 2020 at 06:08:19PM +0100, Jan Beulich wrote:
> On 07.01.2020 17:33, Wei Liu wrote:
> > On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote:
> >> On 05.01.2020 17:47, Wei Liu wrote:
> >>> Hyper-V's input / output argument must be 8 bytes aligned an not cross
> >>> page boundary. The easiest way to satisfy those requirements is to use
> >>> percpu page.
> >>
> >> I'm not sure "easiest" is really true here. Others could consider adding
> >> __aligned() attributes as easy or even easier (by being even more
> >> transparent to use sites). Could we settle on "One way ..."?
> > 
> > Do you mean something like
> > 
> >    struct foo __aligned(8);
> 
> If this is in a header and ...
> 
> >    hv_do_hypercall(OP, virt_to_maddr(&foo), ...);
> 
> ... this in actual code, then yes.
> 
> > ?
> > 
> > I don't think this is transparent to user sites. Plus, foo is on stack
> > which is 1) difficult to get its maddr,
> 
> It being on the stack may indeed complicate getting its machine address
> (if not now, then down the road) - valid point.
> 
> > 2) may cross page boundary.
> 
> The __aligned() of course needs to be large enough to avoid this
> happening.

For this alignment to be large enough, it will need to be of PAGE_SIZE,
right? Wouldn't that blow up Xen's stack easily?  Given we only have two
pages for that.

In light of these restrictions, the approach I take in the original
patch should be okay.

I'm fine with changing the wording to "One way ..." -- if that's the
only objection you have after this mail.

> 
> >> Also, while looking at this I notice that - despite my earlier
> >> comment when giving the respective, sort-of-conditional ack -
> >> there are (still) many apparently pointless __packed attributes
> >> in hyperv-tlfs.h. Care to comment on this?
> > 
> > Again, that's a straight import from Linux. I tried not to deviate too
> > much. A commit in Linux (ec084491727b0) claims "compiler can add
> > alignment padding to structures or reorder struct members for
> > randomization and optimization".
> 
> Would a compiler doing so (without explicitly being told to) even
> be in line with the C spec? I'd buy such a claim only if I see an
> example proving it.
> 
> > I just checked all the packed structures. They seem to have all the
> > required manual paddings already. I can only assume they tried to erred
> > on the safe side.
> 
> And you surely recall we had to remove quite a few instances of
> __packed for gcc 9 compatibility?

Fair enough. I will write a patch to drop those __packed attributes.

Wei.

> 
> Jan

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

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

[Jan Beulich]

On 07.01.2020 18:27, Wei Liu wrote:
> On Tue, Jan 07, 2020 at 06:08:19PM +0100, Jan Beulich wrote:
>> On 07.01.2020 17:33, Wei Liu wrote:
>>> On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote:
>>>> On 05.01.2020 17:47, Wei Liu wrote:
>>>>> Hyper-V's input / output argument must be 8 bytes aligned an not cross
>>>>> page boundary. The easiest way to satisfy those requirements is to use
>>>>> percpu page.
>>>>
>>>> I'm not sure "easiest" is really true here. Others could consider adding
>>>> __aligned() attributes as easy or even easier (by being even more
>>>> transparent to use sites). Could we settle on "One way ..."?
>>>
>>> Do you mean something like
>>>
>>>    struct foo __aligned(8);
>>
>> If this is in a header and ...
>>
>>>    hv_do_hypercall(OP, virt_to_maddr(&foo), ...);
>>
>> ... this in actual code, then yes.
>>
>>> ?
>>>
>>> I don't think this is transparent to user sites. Plus, foo is on stack
>>> which is 1) difficult to get its maddr,
>>
>> It being on the stack may indeed complicate getting its machine address
>> (if not now, then down the road) - valid point.
>>
>>> 2) may cross page boundary.
>>
>> The __aligned() of course needs to be large enough to avoid this
>> happening.
> 
> For this alignment to be large enough, it will need to be of PAGE_SIZE,
> right? Wouldn't that blow up Xen's stack easily?  Given we only have two
> pages for that.

Why PAGE_SIZE? For example, a 24-byte structure won't cross a page
boundary if aligned to 32 bytes.

> In light of these restrictions, the approach I take in the original
> patch should be okay.
> 
> I'm fine with changing the wording to "One way ..." -- if that's the
> only objection you have after this mail.

Well, the goal was to (a) check whether alternatives have been considered
(and if not, to consider them) and then (b) if we stick to your approach,
slightly change the wording as suggested.

Jan

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

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

[Wei Liu]

On Wed, Jan 08, 2020 at 11:55:03AM +0100, Jan Beulich wrote:
> On 07.01.2020 18:27, Wei Liu wrote:
> > On Tue, Jan 07, 2020 at 06:08:19PM +0100, Jan Beulich wrote:
> >> On 07.01.2020 17:33, Wei Liu wrote:
> >>> On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote:
> >>>> On 05.01.2020 17:47, Wei Liu wrote:
> >>>>> Hyper-V's input / output argument must be 8 bytes aligned an not cross
> >>>>> page boundary. The easiest way to satisfy those requirements is to use
> >>>>> percpu page.
> >>>>
> >>>> I'm not sure "easiest" is really true here. Others could consider adding
> >>>> __aligned() attributes as easy or even easier (by being even more
> >>>> transparent to use sites). Could we settle on "One way ..."?
> >>>
> >>> Do you mean something like
> >>>
> >>>    struct foo __aligned(8);
> >>
> >> If this is in a header and ...
> >>
> >>>    hv_do_hypercall(OP, virt_to_maddr(&foo), ...);
> >>
> >> ... this in actual code, then yes.
> >>
> >>> ?
> >>>
> >>> I don't think this is transparent to user sites. Plus, foo is on stack
> >>> which is 1) difficult to get its maddr,
> >>
> >> It being on the stack may indeed complicate getting its machine address
> >> (if not now, then down the road) - valid point.
> >>
> >>> 2) may cross page boundary.
> >>
> >> The __aligned() of course needs to be large enough to avoid this
> >> happening.
> > 
> > For this alignment to be large enough, it will need to be of PAGE_SIZE,
> > right? Wouldn't that blow up Xen's stack easily?  Given we only have two
> > pages for that.
> 
> Why PAGE_SIZE? For example, a 24-byte structure won't cross a page
> boundary if aligned to 32 bytes.
> 

You're right.

I said PAGE_SIZE because I was too lazy to calculate the size of every
structures. That's tedious and error prone.

> > In light of these restrictions, the approach I take in the original
> > patch should be okay.
> > 
> > I'm fine with changing the wording to "One way ..." -- if that's the
> > only objection you have after this mail.
> 
> Well, the goal was to (a) check whether alternatives have been considered
> (and if not, to consider them) and then (b) if we stick to your approach,
> slightly change the wording as suggested.

I think the determining factor here is to the difficulty of getting
maddr of a stack variable. I will stick with this approach and change
the wording.

Wei.

> 
> Jan

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

[Xen-devel] [PATCH v3 4/5] 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>
---
v2:
1. Fold into setup_pcpu_arg function
---
 xen/arch/x86/guest/hyperv/hyperv.c | 5 +++++
 xen/include/asm-x86/guest/hyperv.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 7e046dfc04..e5f258c946 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -28,6 +28,7 @@ struct ms_hyperv_info __read_mostly ms_hyperv;
 
 extern char hv_hypercall_page[];
 DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_arg);
+DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index);
 
 static const struct hypervisor_ops ops;
 const struct hypervisor_ops *__init hyperv_probe(void)
@@ -87,6 +88,7 @@ static void __init setup_hypercall_page(void)
 static void setup_hypercall_pcpu_arg(void)
 {
     void *mapping;
+    uint64_t vp_index_msr;
 
     mapping = alloc_xenheap_page();
     if ( !mapping )
@@ -94,6 +96,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/include/asm-x86/guest/hyperv.h b/xen/include/asm-x86/guest/hyperv.h
index 6cf2eab62f..bae06c8a3a 100644
--- a/xen/include/asm-x86/guest/hyperv.h
+++ b/xen/include/asm-x86/guest/hyperv.h
@@ -66,6 +66,7 @@ struct ms_hyperv_info {
 extern struct ms_hyperv_info ms_hyperv;
 
 DECLARE_PER_CPU(void *, hv_pcpu_input_arg);
+DECLARE_PER_CPU(unsigned int, hv_vp_index);
 
 const struct hypervisor_ops *hyperv_probe(void);
 
-- 
2.20.1


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

Re: [Xen-devel] [PATCH v3 4/5] x86/hyperv: retrieve vp_index from Hyper-V

[Paul Durrant]

On Sun, 5 Jan 2020 at 16:49, Wei Liu <wl@xen.org> 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>

> ---
> v2:
> 1. Fold into setup_pcpu_arg function
> ---
>  xen/arch/x86/guest/hyperv/hyperv.c | 5 +++++
>  xen/include/asm-x86/guest/hyperv.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 7e046dfc04..e5f258c946 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -28,6 +28,7 @@ struct ms_hyperv_info __read_mostly ms_hyperv;
>
>  extern char hv_hypercall_page[];
>  DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_arg);
> +DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index);
>
>  static const struct hypervisor_ops ops;
>  const struct hypervisor_ops *__init hyperv_probe(void)
> @@ -87,6 +88,7 @@ static void __init setup_hypercall_page(void)
>  static void setup_hypercall_pcpu_arg(void)
>  {
>      void *mapping;
> +    uint64_t vp_index_msr;
>
>      mapping = alloc_xenheap_page();
>      if ( !mapping )
> @@ -94,6 +96,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/include/asm-x86/guest/hyperv.h b/xen/include/asm-x86/guest/hyperv.h
> index 6cf2eab62f..bae06c8a3a 100644
> --- a/xen/include/asm-x86/guest/hyperv.h
> +++ b/xen/include/asm-x86/guest/hyperv.h
> @@ -66,6 +66,7 @@ struct ms_hyperv_info {
>  extern struct ms_hyperv_info ms_hyperv;
>
>  DECLARE_PER_CPU(void *, hv_pcpu_input_arg);
> +DECLARE_PER_CPU(unsigned int, hv_vp_index);
>
>  const struct hypervisor_ops *hyperv_probe(void);
>
> --
> 2.20.1
>

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

Re: [Xen-devel] [PATCH v3 4/5] x86/hyperv: retrieve vp_index from Hyper-V

[Jan Beulich]

On 05.01.2020 17:48, Wei Liu wrote:
> --- a/xen/include/asm-x86/guest/hyperv.h
> +++ b/xen/include/asm-x86/guest/hyperv.h
> @@ -66,6 +66,7 @@ struct ms_hyperv_info {
>  extern struct ms_hyperv_info ms_hyperv;
>  
>  DECLARE_PER_CPU(void *, hv_pcpu_input_arg);
> +DECLARE_PER_CPU(unsigned int, hv_vp_index);

Same question here - will this need to be visible outside of the
file defining the variable? In the other patch as well as here,
if the answer is yes, the next question would be whether it needs
to be visible outside of xen/arch/x86/guest/hyperv/ (i.e. whether
it shouldn't live in a private header).

Jan

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

Re: [Xen-devel] [PATCH v3 4/5] x86/hyperv: retrieve vp_index from Hyper-V

[Wei Liu]

On Mon, Jan 06, 2020 at 11:31:50AM +0100, Jan Beulich wrote:
> On 05.01.2020 17:48, Wei Liu wrote:
> > --- a/xen/include/asm-x86/guest/hyperv.h
> > +++ b/xen/include/asm-x86/guest/hyperv.h
> > @@ -66,6 +66,7 @@ struct ms_hyperv_info {
> >  extern struct ms_hyperv_info ms_hyperv;
> >  
> >  DECLARE_PER_CPU(void *, hv_pcpu_input_arg);
> > +DECLARE_PER_CPU(unsigned int, hv_vp_index);
> 
> Same question here - will this need to be visible outside of the
> file defining the variable? In the other patch as well as here,
> if the answer is yes, the next question would be whether it needs
> to be visible outside of xen/arch/x86/guest/hyperv/ (i.e. whether
> it shouldn't live in a private header).

Private header should be fine for now.

Wei.

> 
> Jan

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

[Xen-devel] [PATCH v3 5/5] 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>
---
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 | 21 +++++++++++++++++++++
 xen/include/asm-x86/guest/hyperv.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index e5f258c946..17488f8c40 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -29,6 +29,7 @@ struct ms_hyperv_info __read_mostly ms_hyperv;
 extern char hv_hypercall_page[];
 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 const struct hypervisor_ops ops;
 const struct hypervisor_ops *__init hyperv_probe(void)
@@ -101,15 +102,35 @@ 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 = alloc_xenheap_page();
+    if ( !mapping )
+        panic("Failed to allocate vp_assist page for %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/include/asm-x86/guest/hyperv.h b/xen/include/asm-x86/guest/hyperv.h
index bae06c8a3a..2ddfd53f8c 100644
--- a/xen/include/asm-x86/guest/hyperv.h
+++ b/xen/include/asm-x86/guest/hyperv.h
@@ -67,6 +67,7 @@ extern struct ms_hyperv_info ms_hyperv;
 
 DECLARE_PER_CPU(void *, hv_pcpu_input_arg);
 DECLARE_PER_CPU(unsigned int, hv_vp_index);
+DECLARE_PER_CPU(void *, hv_vp_assist);
 
 const struct hypervisor_ops *hyperv_probe(void);
 
-- 
2.20.1


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