Re: [Xen-devel] [PATCH v3.1 10/15] xen/x86: split Dom0 build into PV and PVHv2

[Jan Beulich]

>>> On 28.11.16 at 18:49,  wrote:
> On Thu, Nov 17, 2016 at 03:49:22AM -0700, Jan Beulich wrote:
>> >>> On 16.11.16 at 19:02,  wrote:
>> > On Fri, Nov 11, 2016 at 09:53:49AM -0700, Jan Beulich wrote:
>> >> >>> On 29.10.16 at 10:59,  wrote:
>> >> > --- a/xen/arch/x86/setup.c
>> >> > +++ b/xen/arch/x86/setup.c
>> >> > @@ -67,6 +67,16 @@ unsigned long __read_mostly cr4_pv32_mask;
>> >> >  static bool_t __initdata opt_dom0pvh;
>> >> >  boolean_param("dom0pvh", opt_dom0pvh);
>> >> >  
>> >> > +/*
>> >> > + * List of parameters that affect Dom0 creation:
>> >> > + *
>> >> > + *  - hvm   Create a PVHv2 Dom0.
>> >> > + *  - shadowUse shadow paging for Dom0.
>> >> > + */
>> >> > +static void parse_dom0_param(char *s);
>> >> 
>> >> Please try to avoid such forward declarations.
> 
> OK, so would you prefer to place the custom_param call after the function 
> definition? I've done it that way (with the forward declaration) because it's 
> the way other options are already implemented.

I don't know where you've looked, but out of the 8 or so examples
I've looked at just now only one used a forward declaration, and
we've been asking others introducing new custom options the same
I'm asking you now. IOW - yes, the function should come first,
immediately followed by the custom_param().

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3.1 10/15] xen/x86: split Dom0 build into PV and PVHv2

[Roger Pau Monne]

On Thu, Nov 17, 2016 at 03:49:22AM -0700, Jan Beulich wrote:
> >>> On 16.11.16 at 19:02,  wrote:
> > On Fri, Nov 11, 2016 at 09:53:49AM -0700, Jan Beulich wrote:
> >> >>> On 29.10.16 at 10:59,  wrote:
> >> > --- a/xen/arch/x86/setup.c
> >> > +++ b/xen/arch/x86/setup.c
> >> > @@ -67,6 +67,16 @@ unsigned long __read_mostly cr4_pv32_mask;
> >> >  static bool_t __initdata opt_dom0pvh;
> >> >  boolean_param("dom0pvh", opt_dom0pvh);
> >> >  
> >> > +/*
> >> > + * List of parameters that affect Dom0 creation:
> >> > + *
> >> > + *  - hvm   Create a PVHv2 Dom0.
> >> > + *  - shadowUse shadow paging for Dom0.
> >> > + */
> >> > +static void parse_dom0_param(char *s);
> >> 
> >> Please try to avoid such forward declarations.

OK, so would you prefer to place the custom_param call after the function 
definition? I've done it that way (with the forward declaration) because it's 
the way other options are already implemented.
 
> >> > @@ -1543,6 +1574,14 @@ void __init noreturn __start_xen(unsigned long 
> >> > mbi_p)
> >> >  if ( opt_dom0pvh )
> >> >  domcr_flags |= DOMCRF_pvh | DOMCRF_hap;
> >> >  
> >> > +if ( dom0_hvm )
> >> > +{
> >> > +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;
> >> > +}
> >> 
> >> If you wire this up here already, instead of later in the series, what's
> >> the effect of someone using this option? Crash?
> > 
> > Most certainly. The BSP IP points to 0 at this point. I can wire this up 
> > later, 
> > but it's going to be strange IMHO.
> 
> Not any more "strange" than someone trying the option and getting
> some random and perhaps not immediately understandable crash.

I will add a panic here then, which I will then remove once this is finished.

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3.1 10/15] xen/x86: split Dom0 build into PV and PVHv2

[Jan Beulich]

>>> On 16.11.16 at 19:02,  wrote:
> On Fri, Nov 11, 2016 at 09:53:49AM -0700, Jan Beulich wrote:
>> >>> On 29.10.16 at 10:59,  wrote:
>> > --- a/xen/arch/x86/domain_build.c
>> > +++ b/xen/arch/x86/domain_build.c
>> > @@ -191,10 +191,8 @@ struct vcpu *__init alloc_dom0_vcpu0(struct domain 
> *dom0)
>> >  }
>> >  
>> >  #ifdef CONFIG_SHADOW_PAGING
>> > -static bool_t __initdata opt_dom0_shadow;
>> > +bool __initdata opt_dom0_shadow;
>> >  boolean_param("dom0_shadow", opt_dom0_shadow);
>> > -#else
>> > -#define opt_dom0_shadow 0
>> >  #endif
>> 
>> I think the new option parsing would better go here, avoiding the need
>> for this change. Making dom0_hvm visible globally is the less intrusive
>> variant.
> 
> I'm not sure I follow your point, even if dom0_hvm is defined here together 
> with the parsing, opt_dom0_shadow still needs to be made global, so it can be 
> accessed from setup.c which is where the domain_create call happens.

Oh, I had overlooked that use.

>> > --- a/xen/arch/x86/setup.c
>> > +++ b/xen/arch/x86/setup.c
>> > @@ -67,6 +67,16 @@ unsigned long __read_mostly cr4_pv32_mask;
>> >  static bool_t __initdata opt_dom0pvh;
>> >  boolean_param("dom0pvh", opt_dom0pvh);
>> >  
>> > +/*
>> > + * List of parameters that affect Dom0 creation:
>> > + *
>> > + *  - hvm   Create a PVHv2 Dom0.
>> > + *  - shadowUse shadow paging for Dom0.
>> > + */
>> > +static void parse_dom0_param(char *s);
>> 
>> Please try to avoid such forward declarations.
>> 
>> > @@ -1543,6 +1574,14 @@ void __init noreturn __start_xen(unsigned long 
>> > mbi_p)
>> >  if ( opt_dom0pvh )
>> >  domcr_flags |= DOMCRF_pvh | DOMCRF_hap;
>> >  
>> > +if ( dom0_hvm )
>> > +{
>> > +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;
>> > +}
>> 
>> If you wire this up here already, instead of later in the series, what's
>> the effect of someone using this option? Crash?
> 
> Most certainly. The BSP IP points to 0 at this point. I can wire this up 
> later, 
> but it's going to be strange IMHO.

Not any more "strange" than someone trying the option and getting
some random and perhaps not immediately understandable crash.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3.1 10/15] xen/x86: split Dom0 build into PV and PVHv2

[Roger Pau Monne]

On Fri, Nov 11, 2016 at 09:53:49AM -0700, Jan Beulich wrote:
> >>> On 29.10.16 at 10:59,  wrote:
> > --- a/xen/arch/x86/domain_build.c
> > +++ b/xen/arch/x86/domain_build.c
> > @@ -191,10 +191,8 @@ struct vcpu *__init alloc_dom0_vcpu0(struct domain 
> > *dom0)
> >  }
> >  
> >  #ifdef CONFIG_SHADOW_PAGING
> > -static bool_t __initdata opt_dom0_shadow;
> > +bool __initdata opt_dom0_shadow;
> >  boolean_param("dom0_shadow", opt_dom0_shadow);
> > -#else
> > -#define opt_dom0_shadow 0
> >  #endif
> 
> I think the new option parsing would better go here, avoiding the need
> for this change. Making dom0_hvm visible globally is the less intrusive
> variant.

I'm not sure I follow your point, even if dom0_hvm is defined here together 
with the parsing, opt_dom0_shadow still needs to be made global, so it can be 
accessed from setup.c which is where the domain_create call happens.
 
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -67,6 +67,16 @@ unsigned long __read_mostly cr4_pv32_mask;
> >  static bool_t __initdata opt_dom0pvh;
> >  boolean_param("dom0pvh", opt_dom0pvh);
> >  
> > +/*
> > + * List of parameters that affect Dom0 creation:
> > + *
> > + *  - hvm   Create a PVHv2 Dom0.
> > + *  - shadowUse shadow paging for Dom0.
> > + */
> > +static void parse_dom0_param(char *s);
> 
> Please try to avoid such forward declarations.
> 
> > @@ -1543,6 +1574,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >  if ( opt_dom0pvh )
> >  domcr_flags |= DOMCRF_pvh | DOMCRF_hap;
> >  
> > +if ( dom0_hvm )
> > +{
> > +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;
> > +}
> 
> If you wire this up here already, instead of later in the series, what's
> the effect of someone using this option? Crash?

Most certainly. The BSP IP points to 0 at this point. I can wire this up later, 
but it's going to be strange IMHO.

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3.1 10/15] xen/x86: split Dom0 build into PV and PVHv2

[Jan Beulich]

>>> On 29.10.16 at 10:59,  wrote:
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -191,10 +191,8 @@ struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
>  }
>  
>  #ifdef CONFIG_SHADOW_PAGING
> -static bool_t __initdata opt_dom0_shadow;
> +bool __initdata opt_dom0_shadow;
>  boolean_param("dom0_shadow", opt_dom0_shadow);
> -#else
> -#define opt_dom0_shadow 0
>  #endif

I think the new option parsing would better go here, avoiding the need
for this change. Making dom0_hvm visible globally is the less intrusive
variant.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -67,6 +67,16 @@ unsigned long __read_mostly cr4_pv32_mask;
>  static bool_t __initdata opt_dom0pvh;
>  boolean_param("dom0pvh", opt_dom0pvh);
>  
> +/*
> + * List of parameters that affect Dom0 creation:
> + *
> + *  - hvm   Create a PVHv2 Dom0.
> + *  - shadowUse shadow paging for Dom0.
> + */
> +static void parse_dom0_param(char *s);

Please try to avoid such forward declarations.

> @@ -1543,6 +1574,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  if ( opt_dom0pvh )
>  domcr_flags |= DOMCRF_pvh | DOMCRF_hap;
>  
> +if ( dom0_hvm )
> +{
> +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;
> +}

If you wire this up here already, instead of later in the series, what's
the effect of someone using this option? Crash?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v3.1 10/15] xen/x86: split Dom0 build into PV and PVHv2

[Roger Pau Monne]

Split the Dom0 builder into two different functions, one for PV (and classic
PVH), and another one for PVHv2. Introduce a new command line parameter
called 'dom0' that can be used to request the creation of a PVHv2 Dom0 by
setting the 'hvm' sub-option.

Signed-off-by: Roger Pau Monné 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
---
Changes since v2:
 - Fix coding style.
 - Introduce a new dom0 option that allows passing several parameters.
   Currently supported ones are hvm and shadow.

Changes since RFC:
 - Add documentation for the new command line option.
 - Simplify the logic in construct_dom0.
---
 docs/misc/xen-command-line.markdown | 17 
 xen/arch/x86/domain_build.c | 28 ++
 xen/arch/x86/setup.c| 39 +
 xen/include/asm-x86/setup.h |  6 ++
 4 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index 87c3023..006e90c 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -656,6 +656,23 @@ affinities to prefer but be not limited to the specified 
node(s).
 
 Pin dom0 vcpus to their respective pcpus
 
+### dom0
+> `= List of [ hvm | shadow ]`
+
+> Sub-options:
+
+> `hvm`
+
+> Default: `false`
+
+Flag that makes a dom0 boot in PVHv2 mode.
+
+> `shadow`
+
+> Default: `false`
+
+Flag that makes a dom0 use shadow paging.
+
 ### dom0pvh
 > `= `
 
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 1e557b9..2c9ebf2 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -191,10 +191,8 @@ struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
 }
 
 #ifdef CONFIG_SHADOW_PAGING
-static bool_t __initdata opt_dom0_shadow;
+bool __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] = "";
@@ -951,7 +949,7 @@ static int __init setup_permissions(struct domain *d)
 return rc;
 }
 
-int __init construct_dom0(
+static int __init construct_dom0_pv(
 struct domain *d,
 const module_t *image, unsigned long image_headroom,
 module_t *initrd,
@@ -1655,6 +1653,28 @@ out:
 return rc;
 }
 
+static int __init construct_dom0_hvm(struct domain *d, const module_t *image,
+ unsigned long image_headroom,
+ module_t *initrd,
+ void *(*bootstrap_map)(const module_t *),
+ char *cmdline)
+{
+
+printk("** Building a PVH Dom0 **\n");
+
+return 0;
+}
+
+int __init construct_dom0(struct domain *d, const module_t *image,
+  unsigned long image_headroom, module_t *initrd,
+  void *(*bootstrap_map)(const module_t *),
+  char *cmdline)
+{
+
+return (is_hvm_domain(d) ? construct_dom0_hvm : construct_dom0_pv)
+   (d, image, image_headroom, initrd,bootstrap_map, cmdline);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 72e7f24..64d4c89 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -67,6 +67,16 @@ unsigned long __read_mostly cr4_pv32_mask;
 static bool_t __initdata opt_dom0pvh;
 boolean_param("dom0pvh", opt_dom0pvh);
 
+/*
+ * List of parameters that affect Dom0 creation:
+ *
+ *  - hvm   Create a PVHv2 Dom0.
+ *  - shadowUse shadow paging for Dom0.
+ */
+static void parse_dom0_param(char *s);
+custom_param("dom0", parse_dom0_param);
+static bool __initdata dom0_hvm;
+
 /*  Linux config option: propagated to domain0. */
 /* "acpi=off":Sisables both ACPI table parsing and interpreter. */
 /* "acpi=force":  Override the disable blacklist.   */
@@ -187,6 +197,27 @@ static void __init parse_acpi_param(char *s)
 }
 }
 
+static void __init parse_dom0_param(char *s)
+{
+char *ss;
+
+do {
+
+ss = strchr(s, ',');
+if ( ss )
+*ss = '\0';
+
+if ( !strcmp(s, "hvm") )
+dom0_hvm = true;
+#ifdef CONFIG_SHADOW_PAGING
+else if ( !strcmp(s, "shadow") )
+opt_dom0_shadow = true;
+#endif
+
+s = ss + 1;
+} while ( ss );
+}
+
 static const module_t *__initdata initial_images;
 static unsigned int __initdata nr_initial_images;
 
@@ -1543,6 +1574,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 if ( opt_dom0pvh )
 domcr_flags |= DOMCRF_pvh | DOMCRF_hap;
 
+if ( dom0_hvm )
+{
+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;
+}
+
 /* Create