[v2,11/30] xen/x86: split Dom0 build into PV and PVHv2

Commit Message

Roger Pau Monne Oct. 3, 2016, 10:09 a.m. UTC

On Fri, Sep 30, 2016 at 09:03:34AM -0600, Jan Beulich wrote:
> >>> On 27.09.16 at 17:57, <[e-mail]> wrote:
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -663,6 +663,13 @@ Pin dom0 vcpus to their respective pcpus
> >  
> >  Flag that makes a 64bit dom0 boot in PVH mode. No 32bit support at present.
> >  
> > +### dom0hvm
> > +> `= <boolean>`
> > +
> > +> Default: `false`
> > +
> > +Flag that makes a dom0 boot in PVHv2 mode.
> 
> Considering sorting aspects this clearly wants to go at least ahead of
> dom0pvh.
>
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -75,6 +75,10 @@ unsigned long __read_mostly cr4_pv32_mask;
> >  static bool_t __initdata opt_dom0pvh;
> >  boolean_param("dom0pvh", opt_dom0pvh);
> >  
> > +/* Boot dom0 in HVM mode */
> > +static bool_t __initdata opt_dom0hvm;
> 
> Plain bool please.
> 
> > @@ -1495,6 +1499,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >      if ( opt_dom0pvh )
> >          domcr_flags |= DOMCRF_pvh | DOMCRF_hap;
> >  
> > +    if ( opt_dom0hvm ) {
> 
> Coding style.

Fixed all the above.
 
> > +        domcr_flags |= DOMCRF_hvm | (hvm_funcs.hap_supported ? DOMCRF_hap : 0);
> 
> So you mean to support PVHv2 on shadow (including for Dom0)
> right away. Are you also testing that?

I've added the following patch to my queue, in order to allow the user to 
select whether they want to use HAP or shadow, I've tested it locally and 
there seems to be no issues in building a PVHv2 Dom0 using shadow.

---

Comments

Jan Beulich Oct. 4, 2016, 6:54 a.m. UTC | #1

>>> On 03.10.16 at 12:09, <[e-mail]> wrote:
> I've added the following patch to my queue, in order to allow the user to 
> select whether they want to use HAP or shadow, I've tested it locally and 
> there seems to be no issues in building a PVHv2 Dom0 using shadow.

Hmm, two remarks: For one, I'm not convinced of the need to move
the definition. It being where it is now allows the string literal to be
discarded post boot. And considering that the option has presumably
been broken for PV for a long time and was never working for PVHv1,
I'm also unconvinced using it (and hence retaining its existence) is a
good idea - I'd much rather see "dom0hvm=hap" and
"dom0hvm=shadow" supported along with the booleans which can
be given to it right now.

> --- a/xen/include/asm-x86/setup.h
> +++ b/xen/include/asm-x86/setup.h
> @@ -51,6 +51,12 @@ void microcode_grab_module(
>  
>  extern uint8_t kbd_shift_flags;
>  
> +#ifdef CONFIG_SHADOW_PAGING
> +extern bool opt_dom0_shadow;
> +#else
> +#define opt_dom0_shadow 0

Please use "false" here.

Jan

Andrew Cooper Oct. 4, 2016, 7:09 a.m. UTC | #2

On 04/10/2016 07:54, Jan Beulich wrote:
>>>> On 03.10.16 at 12:09, <[e-mail]> wrote:
>> I've added the following patch to my queue, in order to allow the user to 
>> select whether they want to use HAP or shadow, I've tested it locally and 
>> there seems to be no issues in building a PVHv2 Dom0 using shadow.
> Hmm, two remarks: For one, I'm not convinced of the need to move
> the definition. It being where it is now allows the string literal to be
> discarded post boot. And considering that the option has presumably
> been broken for PV for a long time and was never working for PVHv1,
> I'm also unconvinced using it (and hence retaining its existence) is a
> good idea - I'd much rather see "dom0hvm=hap" and
> "dom0hvm=shadow" supported along with the booleans which can
> be given to it right now.

We already have a large number of dom0$foo options which are
inconsistent with underscores or no word breaks.

Could we introduce a dom0= instead and run it like the existing iommu=
to avoid gaining any new top level dom0$foo options?

~Andrew

Patch

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 80e20fa..17956e2 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -203,13 +203,6 @@  struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
     return setup_dom0_vcpu(dom0, 0, cpumask_first(&dom0_cpus));
 }
 
-#ifdef CONFIG_SHADOW_PAGING
-static bool_t __initdata opt_dom0_shadow;
-boolean_param("dom0_shadow", opt_dom0_shadow);
-#else
-#define opt_dom0_shadow 0
-#endif
-
 static char __initdata opt_dom0_ioports_disable[200] = "";
 string_param("dom0_ioports_disable", opt_dom0_ioports_disable);
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 9272318..252125d 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -79,6 +79,11 @@  boolean_param("dom0pvh", opt_dom0pvh);
 static bool_t __initdata opt_dom0hvm;
 boolean_param("dom0hvm", opt_dom0hvm);
 
+#ifdef CONFIG_SHADOW_PAGING
+bool __initdata opt_dom0_shadow;
+boolean_param("dom0_shadow", opt_dom0_shadow);
+#endif
+
 /* **** Linux config option: propagated to domain0. */
 /* "acpi=off":    Sisables both ACPI table parsing and interpreter. */
 /* "acpi=force":  Override the disable blacklist.                   */
@@ -1500,7 +1505,9 @@  void __init noreturn __start_xen(unsigned long mbi_p)
         domcr_flags |= DOMCRF_pvh | DOMCRF_hap;
 
     if ( opt_dom0hvm ) {
-        domcr_flags |= DOMCRF_hvm | (hvm_funcs.hap_supported ? DOMCRF_hap : 0);
+        domcr_flags |= DOMCRF_hvm |
+                       (hvm_funcs.hap_supported && !opt_dom0_shadow ?
+                       DOMCRF_hap : 0);
         config.emulation_flags = XEN_X86_EMU_LAPIC|XEN_X86_EMU_IOAPIC;
     }
 
diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h
index c65b79c..888d952 100644
--- a/xen/include/asm-x86/setup.h
+++ b/xen/include/asm-x86/setup.h
@@ -51,6 +51,12 @@  void microcode_grab_module(
 
 extern uint8_t kbd_shift_flags;
 
+#ifdef CONFIG_SHADOW_PAGING
+extern bool opt_dom0_shadow;
+#else
+#define opt_dom0_shadow 0
+#endif
+
 #ifdef NDEBUG
 # define highmem_start 0
 #else