Re: [Xen-devel] [PATCH v4 09/14] xen/x86: split Dom0 build into PV and PVHv2

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, Dec 09, 2016 at 09:07:16AM -0700, Jan Beulich wrote:
> >>> On 30.11.16 at 17:49, <roger.pau@xxxxxxxxxx> wrote:
> > --- 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.
> 
> Would you mind marking dom0_shadow deprecated at once? In fact
> I wouldn't mind if it was removed from the documentation altogether,
> the more that it still has no description at all.

Sure, AFAICT it's just removing it from the documentation and a single usage
in construct_dom0_pv (the one in compute_dom0_nr_pages needs to stay for PVHv2
Dom0).

> > @@ -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");
> 
> Why again is it that you call the function "hvm" but mean "pvh"?

This was to differentiate between the current "pvh" functions in this file that
refer to PVHv1. I could name them pvhv2, but IMHO hvm seems clearer and
shorter.

> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -187,6 +187,35 @@ static void __init parse_acpi_param(char *s)
> >      }
> >  }
> >  
> > +/*
> > + * List of parameters that affect Dom0 creation:
> > + *
> > + *  - hvm               Create a PVHv2 Dom0.
> > + *  - shadow            Use shadow paging for Dom0.
> > + */
> > +static bool __initdata dom0_hvm;
> > +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 );
> > +}
> > +custom_param("dom0", parse_dom0_param);
> 
> I continue to think that this should live in domain_build.c, and
> dom0_hvm be the one off variable which needs to be global. After
> all we intend to extend the "dom0=" quite a bit (presumably to
> subsume everything which the various "dom0..." options now do),
> and all that stuff lives there anyway.

opt_dom0_shadow is also needed by setup.c in order to pass the right DOMCRF
flags. I don't really mind putting it here or in setup.c, but ATM both options
will need to be exported one way or another.

> > --- 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
> 
> "false" please, to match up with "bool".

Fixed, thanks.

Roger.


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