diff mbox series

[v5,10/12] x86/hyperv: provide percpu hypercall input page

Message ID 20200129202034.15052-11-liuwe@microsoft.com (mailing list archive)
State Superseded
Headers show
Series More Hyper-V infrastructures | expand

Commit Message

Wei Liu Jan. 29, 2020, 8:20 p.m. UTC 
Hyper-V's input / output argument must be 8 bytes aligned an not cross
page boundary. One way to satisfy those requirements is to use percpu
page.

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

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

Signed-off-by: Wei Liu <liuwe@microsoft.com>
---
v5:
1. Adjust to new ap_setup
2. Change variable name to hv_pcpu_input_page

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

v3:
1. Use xenheap page instead
2. Drop page tracking structure
3. Drop Paul's review tag
---
 xen/arch/x86/guest/hyperv/hyperv.c  | 31 +++++++++++++++++++++++++++++
 xen/arch/x86/guest/hyperv/private.h | 29 +++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 xen/arch/x86/guest/hyperv/private.h

Comments

Durrant, Paul Jan. 30, 2020, 10:14 a.m. UTC | #1 
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei
> Liu
> Sent: 29 January 2020 20:21
> To: Xen Development List <xen-devel@lists.xenproject.org>
> Cc: Wei Liu <liuwe@microsoft.com>; Wei Liu <wl@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Durrant, Paul <pdurrant@amazon.co.uk>;
> Michael Kelley <mikelley@microsoft.com>; Roger Pau Monné
> <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH v5 10/12] x86/hyperv: provide percpu hypercall
> input page
> 
> Hyper-V's input / output argument must be 8 bytes aligned an not cross
> page boundary. One way to satisfy those requirements is to use percpu
> page.
> 
> For the foreseeable future we only need to provide input for TLB
> and APIC hypercalls, so skip setting up an output page.
> 
> We will also need to provide an ap_setup hook for secondary cpus to
> setup its own input page.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>

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

> ---
> v5:
> 1. Adjust to new ap_setup
> 2. Change variable name to hv_pcpu_input_page
> 
> v4:
> 1. Change wording in commit message
> 2. Prevent leak
> 3. Introduce a private header
> 
> v3:
> 1. Use xenheap page instead
> 2. Drop page tracking structure
> 3. Drop Paul's review tag
> ---
>  xen/arch/x86/guest/hyperv/hyperv.c  | 31 +++++++++++++++++++++++++++++
>  xen/arch/x86/guest/hyperv/private.h | 29 +++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
>  create mode 100644 xen/arch/x86/guest/hyperv/private.h
> 
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c
> b/xen/arch/x86/guest/hyperv/hyperv.c
> index 4387b6541e..f0facccbad 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -27,7 +27,10 @@
>  #include <asm/guest/hyperv-tlfs.h>
>  #include <asm/processor.h>
> 
> +#include "private.h"
> +
>  struct ms_hyperv_info __read_mostly ms_hyperv;
> +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_page);
> 
>  static uint64_t generate_guest_id(void)
>  {
> @@ -127,14 +130,42 @@ static void __init setup_hypercall_page(void)
>      }
>  }
> 
> +static int setup_hypercall_pcpu_arg(void)
> +{
> +    void *mapping;
> +
> +    if ( this_cpu(hv_pcpu_input_page) )
> +        return 0;
> +
> +    mapping = alloc_xenheap_page();
> +    if ( !mapping )
> +    {
> +        printk("Failed to allocate hypercall input page for CPU%u\n",
> +               smp_processor_id());
> +        return -ENOMEM;
> +    }
> +
> +    this_cpu(hv_pcpu_input_page) = mapping;
> +
> +    return 0;
> +}
> +
>  static void __init setup(void)
>  {
>      setup_hypercall_page();
> +    if ( setup_hypercall_pcpu_arg() )
> +        panic("Hypercall percpu arg setup failed\n");
> +}
> +
> +static int ap_setup(void)
> +{
> +    return setup_hypercall_pcpu_arg();
>  }
> 
>  static const struct hypervisor_ops ops = {
>      .name = "Hyper-V",
>      .setup = setup,
> +    .ap_setup = ap_setup,
>  };
> 
>  static void __maybe_unused build_assertions(void)
> diff --git a/xen/arch/x86/guest/hyperv/private.h
> b/xen/arch/x86/guest/hyperv/private.h
> new file mode 100644
> index 0000000000..a339274985
> --- /dev/null
> +++ b/xen/arch/x86/guest/hyperv/private.h
> @@ -0,0 +1,29 @@
> +/************************************************************************
> ******
> + * arch/x86/guest/hyperv/private.h
> + *
> + * Definitions / declarations only useful to Hyper-V code.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (c) 2020 Microsoft.
> + */
> +
> +#ifndef __XEN_HYPERV_PRIVIATE_H__
> +#define __XEN_HYPERV_PRIVIATE_H__
> +
> +#include <xen/percpu.h>
> +
> +DECLARE_PER_CPU(void *, hv_pcpu_input_page);
> +
> +#endif /* __XEN_HYPERV_PRIVIATE_H__  */
> --
> 2.20.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
Roger Pau Monné Jan. 30, 2020, 12:26 p.m. UTC | #2 
On Wed, Jan 29, 2020 at 08:20:32PM +0000, Wei Liu wrote:
> Hyper-V's input / output argument must be 8 bytes aligned an not cross
> page boundary. One way to satisfy those requirements is to use percpu
> page.
> 
> For the foreseeable future we only need to provide input for TLB
> and APIC hypercalls, so skip setting up an output page.
> 
> We will also need to provide an ap_setup hook for secondary cpus to
> setup its own input page.
> 
> Signed-off-by: Wei Liu <liuwe@microsoft.com>

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

Just some nits below.

> ---
> v5:
> 1. Adjust to new ap_setup
> 2. Change variable name to hv_pcpu_input_page
> 
> v4:
> 1. Change wording in commit message
> 2. Prevent leak
> 3. Introduce a private header
> 
> v3:
> 1. Use xenheap page instead
> 2. Drop page tracking structure
> 3. Drop Paul's review tag
> ---
>  xen/arch/x86/guest/hyperv/hyperv.c  | 31 +++++++++++++++++++++++++++++
>  xen/arch/x86/guest/hyperv/private.h | 29 +++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
>  create mode 100644 xen/arch/x86/guest/hyperv/private.h
> 
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
> index 4387b6541e..f0facccbad 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -27,7 +27,10 @@
>  #include <asm/guest/hyperv-tlfs.h>
>  #include <asm/processor.h>
>  
> +#include "private.h"
> +
>  struct ms_hyperv_info __read_mostly ms_hyperv;
> +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_page);

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

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

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

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

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

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

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

Thanks, Roger.
Wei Liu Jan. 30, 2020, 2:09 p.m. UTC | #3 
On Thu, Jan 30, 2020 at 01:26:55PM +0100, Roger Pau Monné wrote:
> On Wed, Jan 29, 2020 at 08:20:32PM +0000, Wei Liu wrote:
> > Hyper-V's input / output argument must be 8 bytes aligned an not cross
> > page boundary. One way to satisfy those requirements is to use percpu
> > page.
> > 
> > For the foreseeable future we only need to provide input for TLB
> > and APIC hypercalls, so skip setting up an output page.
> > 
> > We will also need to provide an ap_setup hook for secondary cpus to
> > setup its own input page.
> > 
> > Signed-off-by: Wei Liu <liuwe@microsoft.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Just some nits below.
> 

All comments addressed.

Wei.

> 
> Thanks, Roger.
diff mbox series

Patch

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