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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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


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