diff mbox series

[v4,5/7] x86/hyperv: provide percpu hypercall input page

Message ID 20200122202343.5703-6-liuwe@microsoft.com (mailing list archive)
State New, archived
Headers show
Series More Hyper-V infrastructure | expand

Commit Message

Wei Liu Jan. 22, 2020, 8:23 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>
---
v4:
1. Change wording in commit message
2. Prevent leak
3. Introduce a private header

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

Comments

Jan Beulich Jan. 23, 2020, 3:45 p.m. UTC | #1 
On 22.01.2020 21:23, Wei Liu wrote:
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -27,7 +27,10 @@
>  #include <asm/guest/hyperv-tlfs.h>
>  #include <asm/processor.h>
>  
> +#include "private.h"
> +
>  struct ms_hyperv_info __read_mostly ms_hyperv;
> +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_arg);

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

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

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

Jan
Wei Liu Jan. 28, 2020, 3:50 p.m. UTC | #2 
On Thu, Jan 23, 2020 at 04:45:38PM +0100, Jan Beulich wrote:
> On 22.01.2020 21:23, Wei Liu wrote:
> > --- a/xen/arch/x86/guest/hyperv/hyperv.c
> > +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> > @@ -27,7 +27,10 @@
> >  #include <asm/guest/hyperv-tlfs.h>
> >  #include <asm/processor.h>
> >  
> > +#include "private.h"
> > +
> >  struct ms_hyperv_info __read_mostly ms_hyperv;
> > +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_arg);
> 
> Would it perhaps be helpful to make recognizable that this can hold
> up to a page's worth of data, either by its type or by a slightly
> different name?

I will change this to hv_pcpu_input_arg_page instead.

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

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

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

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

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

Wei.

> 
> Jan
Jan Beulich Jan. 28, 2020, 4:15 p.m. UTC | #3 
On 28.01.2020 16:50, Wei Liu wrote:
> On Thu, Jan 23, 2020 at 04:45:38PM +0100, Jan Beulich wrote:
>> On 22.01.2020 21:23, Wei Liu wrote:
>>> --- a/xen/arch/x86/guest/hyperv/hyperv.c
>>> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
>>> @@ -27,7 +27,10 @@
>>>  #include <asm/guest/hyperv-tlfs.h>
>>>  #include <asm/processor.h>
>>>  
>>> +#include "private.h"
>>> +
>>>  struct ms_hyperv_info __read_mostly ms_hyperv;
>>> +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_arg);
>>
>> Would it perhaps be helpful to make recognizable that this can hold
>> up to a page's worth of data, either by its type or by a slightly
>> different name?
> 
> I will change this to hv_pcpu_input_arg_page instead.

Or maybe hv_pcpu_input_page?

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

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

Jan
Wei Liu Jan. 28, 2020, 4:52 p.m. UTC | #4 
On Tue, Jan 28, 2020 at 05:15:39PM +0100, Jan Beulich wrote:
> On 28.01.2020 16:50, Wei Liu wrote:
> > On Thu, Jan 23, 2020 at 04:45:38PM +0100, Jan Beulich wrote:
> >> On 22.01.2020 21:23, Wei Liu wrote:
> >>> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> >>> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> >>> @@ -27,7 +27,10 @@
> >>>  #include <asm/guest/hyperv-tlfs.h>
> >>>  #include <asm/processor.h>
> >>>  
> >>> +#include "private.h"
> >>> +
> >>>  struct ms_hyperv_info __read_mostly ms_hyperv;
> >>> +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_arg);
> >>
> >> Would it perhaps be helpful to make recognizable that this can hold
> >> up to a page's worth of data, either by its type or by a slightly
> >> different name?
> > 
> > I will change this to hv_pcpu_input_arg_page instead.
> 
> Or maybe hv_pcpu_input_page?

Fine by me.

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

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

Wei.

> 
> Jan
diff mbox series

Patch

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