| Message ID | 1477731601-10926-11-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 29.10.16 at 10:59, <roger.pau@citrix.com> 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. > + * - shadow Use 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
On Fri, Nov 11, 2016 at 09:53:49AM -0700, Jan Beulich wrote: > >>> On 29.10.16 at 10:59, <roger.pau@citrix.com> 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. > > + * - shadow Use 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.
>>> On 16.11.16 at 19:02, <roger.pau@citrix.com> wrote: > On Fri, Nov 11, 2016 at 09:53:49AM -0700, Jan Beulich wrote: >> >>> On 29.10.16 at 10:59, <roger.pau@citrix.com> 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. >> > + * - shadow Use 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
On Thu, Nov 17, 2016 at 03:49:22AM -0700, Jan Beulich wrote: > >>> On 16.11.16 at 19:02, <roger.pau@citrix.com> wrote: > > On Fri, Nov 11, 2016 at 09:53:49AM -0700, Jan Beulich wrote: > >> >>> On 29.10.16 at 10:59, <roger.pau@citrix.com> 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. > >> > + * - shadow Use 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.
>>> On 28.11.16 at 18:49, <roger.pau@citrix.com> wrote: > On Thu, Nov 17, 2016 at 03:49:22AM -0700, Jan Beulich wrote: >> >>> On 16.11.16 at 19:02, <roger.pau@citrix.com> wrote: >> > On Fri, Nov 11, 2016 at 09:53:49AM -0700, Jan Beulich wrote: >> >> >>> On 29.10.16 at 10:59, <roger.pau@citrix.com> 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. >> >> > + * - shadow Use 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
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 > `= <boolean>` 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. + * - shadow Use 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 initial domain 0. */ dom0 = domain_create(0, domcr_flags, 0, &config); if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h index c65b79c..c4179d1 100644 --- a/xen/include/asm-x86/setup.h +++ b/xen/include/asm-x86/setup.h @@ -57,4 +57,10 @@ extern uint8_t kbd_shift_flags; extern unsigned long highmem_start; #endif +#ifdef CONFIG_SHADOW_PAGING +extern bool opt_dom0_shadow; +#else +#define opt_dom0_shadow 0 +#endif + #endif
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é <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- 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(-)