xen/x86: support setting dom0_mem depending on host size

Quoted 1 line

Quoted 1 line · On 22.11.18 at 17:40, <[email protected]> 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.

xen/arch/x86/dom0_build.c +1 -1

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

xen/arch/x86/dom0_build.c +1 -2

@@ -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);

Quoted 5 lines

Quoted 1 line

Quoted 1 line · On 22.11.18 at 17:40, <[email protected]> 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 ...

Quoted 7 lines

xen/arch/x86/dom0_build.c +0 -0

@@ -24,6 +24,8 @@ static long __initdata dom0_nrpages;
 static long __initdata dom0_min_nrpages;
 static long __initdata dom0_max_nrpages = LONG_MAX;
 

xen/arch/x86/dom0_build.c static long __initdata dom0_nrpages; · line 27

Quoted 1 line

xen/arch/x86/dom0_build.c +1 -0

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

Quoted 12 lines

xen/arch/x86/dom0_build.c +3 -3

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

xen/arch/x86/dom0_build.c static long __initdata dom0_max_nrpages = LONG_MAX; · line 57

Quoted 5 lines

xen/arch/x86/dom0_build.c +4 -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.

Quoted 9 lines

xen/arch/x86/dom0_build.c +1 -2

@@ -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);

xen/arch/x86/dom0_build.c static int __init parse_dom0_mem(const char *s) · line 81

Quoted 10 lines

xen/arch/x86/dom0_build.c +3 -2

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

xen/arch/x86/dom0_build.c unsigned long __init dom0_compute_nr_pages( · line 311

Quoted 7 lines

xen/arch/x86/dom0_build.c +3 -0

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

Quoted 1 line

Quoted 1 line · On 28.11.18 at 16:33, <[email protected]> wrote:

Quoted 12 lines · Jan Beulich

Quoted 12 lines

Quoted 1 line

Quoted 1 line · On 22.11.18 at 17:40, <[email protected]> wrote:

xen/arch/x86/dom0_build.c +3 -3

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

Quoted 8 lines

xen/arch/x86/dom0_build.c static long __initdata dom0_max_nrpages = LONG_MAX; · line 57

Quoted 5 lines

xen/arch/x86/dom0_build.c +4 -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.

Quoted 11 lines

xen/arch/x86/dom0_build.c unsigned long __init dom0_compute_nr_pages( · line 311

Quoted 7 lines

xen/arch/x86/dom0_build.c +3 -0

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

Quoted 1 line

Quoted 1 line · On 22.11.18 at 17:40, <[email protected]> wrote:

docs/misc/xen-command-line.markdown +0 -0

@@ -777,13 +777,20 @@ The amount of memory will be at least the minimum but never more than
 the maximum (i.e., `max` overrides the `min` option).  If there isn't
 enough memory then as much as possible is allocated.
 

xen/arch/x86/dom0_build.c +2 -1

@@ -50,7 +55,28 @@ static unsigned long __init parse_amt(const char *s, const char **ps,
                                       unsigned long avail)
 {
     unsigned int minus = (*s == '-') ? 1 : 0;
-    unsigned long pages = parse_size_and_unit(s + minus, ps) >> PAGE_SHIFT;
+    unsigned long val, pages = 0;
+

Quoted 13 lines

Quoted 1 line

Quoted 1 line · On 22.11.18 at 17:40, <[email protected]> wrote:

docs/misc/xen-command-line.markdown +1 -1

@@ -777,13 +777,20 @@ The amount of memory will be at least the minimum but never more than
 the maximum (i.e., `max` overrides the `min` option).  If there isn't
 enough memory then as much as possible is allocated.
 
-`max:<size>` also sets the maximum reservation (the maximum amount of
+`max:<sz>` also sets the maximum reservation (the maximum amount of
 memory dom0 can balloon up to).  If this is omitted then the maximum
 reservation is unlimited.
 
 For example, to set dom0's initial memory allocation to 512MB but
 allow it to balloon up as far as 1GB use `dom0_mem=512M,max:1G`
 

docs/misc/xen-command-line.markdown The amount of memory will be at least the minimum but never more than · line 788

Quoted 2 lines

docs/misc/xen-command-line.markdown +2 -0

+> `<sz>` is: `<size> | [<size>+]<frac>%`
+> `<frac>` is an integer < 100

It is probably fine to state it like this here, but the implementation should take care of not allowing <size>+<frac>% to be greater than available memory.

Quoted 13 lines

xen/arch/x86/dom0_build.c +7 -1

@@ -50,7 +55,28 @@ static unsigned long __init parse_amt(const char *s, const char **ps,
                                       unsigned long avail)
 {
     unsigned int minus = (*s == '-') ? 1 : 0;
-    unsigned long pages = parse_size_and_unit(s + minus, ps) >> PAGE_SHIFT;
+    unsigned long val, pages = 0;
+
+    /* Avoid accessing s[-1] in case value starts with '%'. */
+    if ( *s == '%' )
+        return 0;
+
+    s += minus;

xen/arch/x86/dom0_build.c static unsigned long __init parse_amt(const char *s, const char **ps, · line 69

Quoted 5 lines

xen/arch/x86/dom0_build.c +5 -0

+    while ( isdigit(*s) )
+    {
+        val = parse_size_and_unit(s, ps);
+        s = *ps;
+        if ( *s == '%' && isdigit(*(s - 1)) && val < (100 << 10) )

In the comment above you talk about s[-1] - why do you use a more complicated (even if just slightly) expression here?

Review static unsigned long __init parse_amt(const char *s, const char **ps, · line 71

Quoted 3 lines

(quoted diff) +2 -0

+        {
+            pages += (val >> 10) * avail / 100;

The scaling by 10 bits here and above make it rather desirable to slightly change parse_size_and_unit() instead: I think it could easily be made not do the scaling if it finds % as the next input character. Otherwise at least a brief comment should be put here.

Despite the comments - I very much like the added flexibility.

Quoted 1 line

Quoted 1 line · On 28.11.18 at 16:43, <[email protected]> wrote:

Quoted 13 lines · Jan Beulich

Quoted 13 lines

Quoted 1 line

Quoted 1 line · On 22.11.18 at 17:40, <[email protected]> wrote:

xen/arch/x86/dom0_build.c +7 -1

@@ -50,7 +55,28 @@ static unsigned long __init parse_amt(const char *s, const char **ps,
                                       unsigned long avail)
 {
     unsigned int minus = (*s == '-') ? 1 : 0;
-    unsigned long pages = parse_size_and_unit(s + minus, ps) >> PAGE_SHIFT;
+    unsigned long val, pages = 0;
+
+    /* Avoid accessing s[-1] in case value starts with '%'. */
+    if ( *s == '%' )
+        return 0;
+
+    s += minus;

Quoted 8 lines

xen/arch/x86/dom0_build.c static unsigned long __init parse_amt(const char *s, const char **ps, · line 69

Quoted 5 lines

xen/arch/x86/dom0_build.c +5 -0

+    while ( isdigit(*s) )
+    {
+        val = parse_size_and_unit(s, ps);
+        s = *ps;
+        if ( *s == '%' && isdigit(*(s - 1)) && val < (100 << 10) )

In the comment above you talk about s[-1] - why do you use a more complicated (even if just slightly) expression here?

I dislike using s[-1] in code. In case you prefer it I can use it here.

Quoted 1 line

Quoted 1 line · On 22.11.18 at 17:40, <[email protected]> wrote:

xen/arch/x86/Kconfig +0 -0

@@ -176,6 +176,15 @@ config PV_SHIM_EXCLUSIVE
 	  firmware, and will not function correctly in other scenarios.

Quoted 6 lines

Quoted 1 line

Quoted 1 line · On 22.11.18 at 17:40, <[email protected]> wrote:

xen/arch/x86/Kconfig +0 -0

@@ -176,6 +176,15 @@ config PV_SHIM_EXCLUSIVE
 	  firmware, and will not function correctly in other scenarios.
 

xen/arch/x86/Kconfig config PV_SHIM_EXCLUSIVE · line 186

Quoted 9 lines

xen/arch/x86/Kconfig +8 -0

 	  If unsure, say N.
+
+config DOM0_MEM
+	string "Default value for dom0_mem boot parameter"
+	---help---
+	  Sets a default value for dom0_mem, e.g. "min:1G,max:64G,256M+10%".
+	  See docs/misc/xen-command-line.markdown for the supported syntax.
+
+	  Leave empty if you are not sure what to specify.

The latest here I'm with Julien - this should not be x86-specific.

Setting the memory size of dom0 on a server for the non autoballooning case requires always specification of a boot parameter today. The value to set will depend mostly on the host memory size.

In order to support that scenario add the possibility to set dom0_mem depending on the amount of physical memory by allowing to specify a percentage of host memory (e.g. 10%) with an offset (like 1G+10%).

To make it easy for a distributor to use such a setting as the default make the standard setting for dom0_mem configurable via Kconfig.

Juergen Gross (3):
xen/x86: delay parsing of dom0_mem parameter until needed xen/x86: add dom0 memory sizing variants xen/x86: add CONFIG item for default dom0 memory size

 docs/misc/xen-command-line.markdown | 21 ++++++++----
 xen/arch/x86/Kconfig                |  9 +++++
 xen/arch/x86/dom0_build.c           | 67 +++++++++++++++++++++++++++----------
 3 files changed, 73 insertions(+), 24 deletions(-)

Signature · 1 line

2.16.4

Hi Juergen,

I don't mean to scope-creep your series, and I think it is fine if you don't want to do it, but it would be fantastic if you took the opportunity to make dom0_mem common across architectures.

On ARM, we parse it in xen/arch/arm/domain_build.c:parse_dom0_mem. I think the ARM and x86 implementations should be the same.