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

[Juergen Gross <jgross@xxxxxxxx>]

On 28/11/2018 16:12, Jan Beulich wrote:
>>>> 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 ...

Okay, so this would require additional dom0_min_frac, dom0_max_frac and
dom0_frac (or similar). I can change it this way if you like that
better.

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

Uh, the bumping was a leftover from a previous version. But nevertheless
I can understand your concern here.

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

Going the other route will result in merging patches 1 and 2, I guess.

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

The message is referencing the dom0_mem value only. And this would be
ignored. I don't see your concern here.


Juergen


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