Re: [Xen-devel] [PATCH 1/3] xen/x86: delay parsing of dom0_mem parameter until needed

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.11.18 at 17:40, <jgross@xxxxxxxx> wrote:
> Instead of parsing the dom0_mem command line parameter as custom
> parameter do that only when building dom0. This will enable a later
> addition of specifying the memory size by fractions of the host memory
> size, which isn't known when doing custom parameter parsing.

But you don't need to know memory size at the time of parsing. All
you need to do is store all specified values in __initdata variables
and do the calculations when available memory is known. The
reason I'd like to avoid going the route you chose is because ...

> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -24,6 +24,8 @@ static long __initdata dom0_nrpages;
>  static long __initdata dom0_min_nrpages;
>  static long __initdata dom0_max_nrpages = LONG_MAX;
>  
> +static char __initdata dom0_mem_par[64];

... I was really hoping to see go away all such statically dimensioned
arrays used to store command line pieces. This is even more so since
I've seen already in patch 2 you feel the need to bump its size.

> @@ -44,14 +46,20 @@ static long __initdata dom0_max_nrpages = LONG_MAX;
>   *  If +ve: The specified amount is an absolute value.
>   *  If -ve: The specified amount is subtracted from total available memory.
>   */
> -static long __init parse_amt(const char *s, const char **ps)
> +static unsigned long __init parse_amt(const char *s, const char **ps,
> +                                      unsigned long avail)
>  {
> -    long pages = parse_size_and_unit((*s == '-') ? s+1 : s, ps) >> 
> PAGE_SHIFT;
> -    return (*s == '-') ? -pages : pages;
> +    unsigned int minus = (*s == '-') ? 1 : 0;
> +    unsigned long pages = parse_size_and_unit(s + minus, ps) >> PAGE_SHIFT;
> +
> +    /* Negative specification means "all memory - specified amount". */
> +    return minus ? avail - pages : pages;
>  }

I don't think any of this should be done in a patch with the given
title.

> @@ -61,16 +69,16 @@ static int __init parse_dom0_mem(const char *s)
>  
>      do {
>          if ( !strncmp(s, "min:", 4) )
> -            dom0_min_nrpages = parse_amt(s+4, &s);
> +            dom0_min_nrpages = parse_amt(s + 4, &s, avail);
>          else if ( !strncmp(s, "max:", 4) )
> -            dom0_max_nrpages = parse_amt(s+4, &s);
> +            dom0_max_nrpages = parse_amt(s + 4, &s, avail);
>          else
> -            dom0_nrpages = parse_amt(s, &s);
> +            dom0_nrpages = parse_amt(s, &s, avail);
>      } while ( *s++ == ',' );
>  
>      return s[-1] ? -EINVAL : 0;
>  }
> -custom_param("dom0_mem", parse_dom0_mem);
> +string_param("dom0_mem", dom0_mem_par);

In the event of my objection to the delayed parsing getting overruled:
This would then belong next to the array. And if the array remained,
please make the suffix "_param", or even better be consistent with
other command line option holding variable names and call it
opt_dom0_mem.

> @@ -298,6 +306,10 @@ unsigned long __init dom0_compute_nr_pages(
>          (!iommu_hap_pt_share || !paging_mode_hap(d));
>      for ( ; ; need_paging = false )
>      {
> +        if ( dom0_mem_par[0] && parse_dom0_mem(avail) )
> +            printk("Invalid dom0_mem parameter value \"%s\", ignoring\n",
> +                   dom0_mem_par);

Looking at how the parsing function works I don't think the entire
command line would necessarily be ignored in case of error. I think
the log message shouldn't give a wrong impression.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel