[PATCH v2 0/3] xen/x86: support setting dom0_mem depending on host size

[PATCH v2 0/3] xen/x86: support setting dom0_mem depending on host size

[Juergen Gross]

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.

Changes since V1:
- replaced old patch 1 by new one
- rewritten patch 2 according to remarks by Jan Beulich
- changed patch 3 to allow config item on arm, too

Juergen Gross (3):
  xen: introduce parse_size_and_unit_or_int
  xen/x86: add dom0 memory sizing variants
  xen: add CONFIG item for default dom0 memory size

 docs/misc/xen-command-line.markdown |  19 ++++--
 xen/arch/arm/domain_build.c         |   7 +++
 xen/arch/x86/dom0_build.c           | 112 +++++++++++++++++++++++++++---------
 xen/common/Kconfig                  |  13 +++++
 xen/common/lib.c                    |  11 +++-
 xen/include/xen/lib.h               |   2 +
 6 files changed, 130 insertions(+), 34 deletions(-)

-- 
2.16.4


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

[PATCH v2 1/3] xen: introduce parse_size_and_unit_or_int

[Juergen Gross]

Introduce parse_size_and_unit_or_int() similar to parse_size_and_unit()
but not defaulting to kbytes in case the parameter is a number followed
by a specified character.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/lib.c      | 11 +++++++++--
 xen/include/xen/lib.h |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/xen/common/lib.c b/xen/common/lib.c
index 62330205fe..262e9f1053 100644
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -452,7 +452,8 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
 #endif
 }
 
-unsigned long long parse_size_and_unit(const char *s, const char **ps)
+unsigned long long parse_size_and_unit_or_int(const char *s, const char **ps,
+                                              char no_size)
 {
     unsigned long long ret;
     const char *s1;
@@ -477,7 +478,8 @@ unsigned long long parse_size_and_unit(const char *s, const char **ps)
         s1++;
         break;
     default:
-        ret <<= 10; /* default to kB */
+        if ( *s1 && *s1 != no_size )
+            ret <<= 10; /* default to kB */
         break;
     }
 
@@ -487,6 +489,11 @@ unsigned long long parse_size_and_unit(const char *s, const char **ps)
     return ret;
 }
 
+unsigned long long parse_size_and_unit(const char *s, const char **ps)
+{
+    return parse_size_and_unit_or_int(s, ps, '\0');
+}
+
 typedef void (*ctor_func_t)(void);
 extern const ctor_func_t __ctors_start[], __ctors_end[];
 
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 972fc843fa..4db6e6419c 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -153,6 +153,8 @@ unsigned long long simple_strtoull(
     const char *cp,const char **endp, unsigned int base);
 
 unsigned long long parse_size_and_unit(const char *s, const char **ps);
+unsigned long long parse_size_and_unit_or_int(const char *s, const char **ps,
+                                              char no_size);
 
 uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c);
 
-- 
2.16.4


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

Re: [PATCH v2 1/3] xen: introduce parse_size_and_unit_or_int

[Jan Beulich]

>>> On 06.12.18 at 09:06, <jgross@suse.com> wrote:
> @@ -477,7 +478,8 @@ unsigned long long parse_size_and_unit(const char *s, const char **ps)
>          s1++;
>          break;
>      default:
> -        ret <<= 10; /* default to kB */
> +        if ( *s1 && *s1 != no_size )
> +            ret <<= 10; /* default to kB */
>          break;

So did you figure anything wrong with simply special casing '%'
here? '%' is a form of "unit", after all. Perhaps demanding "ps"
to be non-NULL (to have some form of indication the caller will
check the suffix char) might be reasonable.

Jan



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

Re: [PATCH v2 1/3] xen: introduce parse_size_and_unit_or_int

[Juergen Gross]

On 06/12/2018 10:50, Jan Beulich wrote:
>>>> On 06.12.18 at 09:06, <jgross@suse.com> wrote:
>> @@ -477,7 +478,8 @@ unsigned long long parse_size_and_unit(const char *s, const char **ps)
>>          s1++;
>>          break;
>>      default:
>> -        ret <<= 10; /* default to kB */
>> +        if ( *s1 && *s1 != no_size )
>> +            ret <<= 10; /* default to kB */
>>          break;
> 
> So did you figure anything wrong with simply special casing '%'
> here? '%' is a form of "unit", after all. Perhaps demanding "ps"
> to be non-NULL (to have some form of indication the caller will
> check the suffix char) might be reasonable.

I thought a more general approach would be better. I can use the simple
'%' special case with the ps check in case you prefer that.


Juergen

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

Re: [PATCH v2 1/3] xen: introduce parse_size_and_unit_or_int

[Jan Beulich]

>>> On 06.12.18 at 11:01, <jgross@suse.com> wrote:
> On 06/12/2018 10:50, Jan Beulich wrote:
>>>>> On 06.12.18 at 09:06, <jgross@suse.com> wrote:
>>> @@ -477,7 +478,8 @@ unsigned long long parse_size_and_unit(const char *s, 
> const char **ps)
>>>          s1++;
>>>          break;
>>>      default:
>>> -        ret <<= 10; /* default to kB */
>>> +        if ( *s1 && *s1 != no_size )
>>> +            ret <<= 10; /* default to kB */
>>>          break;
>> 
>> So did you figure anything wrong with simply special casing '%'
>> here? '%' is a form of "unit", after all. Perhaps demanding "ps"
>> to be non-NULL (to have some form of indication the caller will
>> check the suffix char) might be reasonable.
> 
> I thought a more general approach would be better. I can use the simple
> '%' special case with the ps check in case you prefer that.

I'd indeed prefer that, first and foremost because that other
function's name becomes too long for my taste, but give others
a chance to voice differing opinions.

Jan



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

[PATCH v2 2/3] xen/x86: add dom0 memory sizing variants

[Juergen Gross]

Today the memory size of dom0 can be specified only in terms of bytes
(either an absolute value or "host-mem - value"). When dom0 shouldn't
be auto-ballooned this requires nearly always a manual adaption of the
Xen boot parameters to reflect the actual host memory size.

Add more possibilities to specify memory sizes. Today we have:

dom0_mem= List of ( min:<size> | max:<size> | <size> )

with <size> being a positive or negative size value (e.g. 1G).

Modify that to:

dom0_mem= List of ( min:<sz> | max:<sz> | <sz> )
<sz>: <size> | [<size>+]<frac>%
<frac>: integer value < 100

With the following semantics:

<frac>% specifies a fraction of host memory size in percent.
<sz> is a percentage of host memory plus an offset.

So <sz> being 1G+25% on a 256G host would result in 65G.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/misc/xen-command-line.markdown |  19 +++++--
 xen/arch/x86/dom0_build.c           | 106 +++++++++++++++++++++++++++---------
 2 files changed, 93 insertions(+), 32 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 9028bcde2e..6248b68595 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -759,17 +759,17 @@ Set the amount of memory for the initial domain (dom0). It must be
 greater than zero. This parameter is required.
 
 ### dom0\_mem (x86)
-> `= List of ( min:<size> | max:<size> | <size> )`
+> `= List of ( min:<sz> | max:<sz> | <sz> )`
 
 Set the amount of memory for the initial domain (dom0). If a size is
 positive, it represents an absolute value.  If a size is negative, it
 is subtracted from the total available memory.
 
-* `<size>` specifies the exact amount of memory.
-* `min:<size>` specifies the minimum amount of memory.
-* `max:<size>` specifies the maximum amount of memory.
+* `<sz>` specifies the exact amount of memory.
+* `min:<sz>` specifies the minimum amount of memory.
+* `max:<sz>` specifies the maximum amount of memory.
 
-If `<size>` is not specified, the default is all the available memory
+If `<sz>` is not specified, the default is all the available memory
 minus some reserve.  The reserve is 1/16 of the available memory or
 128 MB (whichever is smaller).
 
@@ -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`
 
+> `<sz>` is: `<size> | [<size>+]<frac>%`
+> `<frac>` is an integer < 100
+
+* `<frac>` specifies a fraction of host memory size in percent.
+
+So `<sz>` being `1G+25%` on a 256 GB host would result in 65 GB.
+
 If you use this option then it is highly recommended that you disable
 any dom0 autoballooning feature present in your toolstack. See the
 _xl.conf(5)_ man page or [Xen Best
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 5e2ad4bd56..8de71346db 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -20,17 +20,42 @@
 #include <asm/p2m.h>
 #include <asm/setup.h>
 
-static long __initdata dom0_nrpages;
-static long __initdata dom0_min_nrpages;
-static long __initdata dom0_max_nrpages = LONG_MAX;
+struct memsize {
+    long nr_pages;
+    unsigned int percent;
+    bool minus;
+};
+
+static struct memsize __initdata dom0_size;
+static struct memsize __initdata dom0_min_size;
+static struct memsize __initdata dom0_max_size = { .nr_pages = LONG_MAX };
+
+static bool __init memsize_gt_zero(const struct memsize *sz)
+{
+    return !sz->minus && sz->nr_pages;
+}
+
+static unsigned long __init get_memsize(const struct memsize *sz,
+                                        unsigned long avail)
+{
+    unsigned long pages;
+
+    pages = sz->nr_pages + sz->percent * avail / 100;
+    return sz->minus ? avail - pages : pages;
+}
 
 /*
  * dom0_mem=[min:<min_amt>,][max:<max_amt>,][<amt>]
- * 
+ *
  * <min_amt>: The minimum amount of memory which should be allocated for dom0.
  * <max_amt>: The maximum amount of memory which should be allocated for dom0.
  * <amt>:     The precise amount of memory to allocate for dom0.
- * 
+ *
+ * The format of <min_amt>, <max_amt> and <amt> is as follows:
+ * <size> | <frac>% | <size>+<frac>%
+ * <size> is a size value like 1G (1 GByte), <frac> is percentage of host
+ * memory (so 1G+10% means 10 percent of host memory + 1 GByte).
+ *
  * Notes:
  *  1. <amt> is clamped from below by <min_amt> and from above by available
  *     memory and <max_amt>
@@ -39,19 +64,53 @@ static long __initdata dom0_max_nrpages = LONG_MAX;
  *  4. If <amt> is not specified, it is calculated as follows:
  *     "All of memory is allocated to domain 0, minus 1/16th which is reserved
  *      for uses such as DMA buffers (the reservation is clamped to 128MB)."
- * 
+ *
  * Each value can be specified as positive or negative:
  *  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 int __init parse_amt(const char *s, const char **ps, struct memsize *sz)
 {
-    long pages = parse_size_and_unit((*s == '-') ? s+1 : s, ps) >> PAGE_SHIFT;
-    return (*s == '-') ? -pages : pages;
+    unsigned long val;
+    struct memsize tmp = { };
+
+    tmp.minus = (*s == '-');
+    if ( tmp.minus )
+        s++;
+
+    /* Avoid accessing s[-1] in case value starts with '%'. */
+    if ( *s == '%' )
+        return -EINVAL;
+
+    while ( isdigit(*s) )
+    {
+        val = parse_size_and_unit_or_int(s, ps, '%');
+        s = *ps;
+        if ( *s == '%' )
+        {
+            if ( !isdigit(s[-1]) || val >= 100 )
+                return -EINVAL;
+            tmp.percent = val;
+            s++;
+        }
+        else
+            tmp.nr_pages = val >> PAGE_SHIFT;
+        if ( *s == '+' )
+            s++;
+    }
+    *ps = s;
+    if ( *s && *s != ',' )
+        return -EINVAL;
+
+    *sz = tmp;
+
+    return 0;
 }
 
 static int __init parse_dom0_mem(const char *s)
 {
+    int ret;
+
     /* xen-shim uses shim_mem parameter instead of dom0_mem */
     if ( pv_shim )
     {
@@ -61,14 +120,14 @@ static int __init parse_dom0_mem(const char *s)
 
     do {
         if ( !strncmp(s, "min:", 4) )
-            dom0_min_nrpages = parse_amt(s+4, &s);
+            ret = parse_amt(s + 4, &s, &dom0_min_size);
         else if ( !strncmp(s, "max:", 4) )
-            dom0_max_nrpages = parse_amt(s+4, &s);
+            ret = parse_amt(s + 4, &s, &dom0_max_size);
         else
-            dom0_nrpages = parse_amt(s, &s);
-    } while ( *s++ == ',' );
+            ret = parse_amt(s, &s, &dom0_size);
+    } while ( *s++ == ',' && !ret );
 
-    return s[-1] ? -EINVAL : 0;
+    return s[-1] ? -EINVAL : ret;
 }
 custom_param("dom0_mem", parse_dom0_mem);
 
@@ -298,9 +357,9 @@ unsigned long __init dom0_compute_nr_pages(
         (!iommu_hap_pt_share || !paging_mode_hap(d));
     for ( ; ; need_paging = false )
     {
-        nr_pages = dom0_nrpages;
-        min_pages = dom0_min_nrpages;
-        max_pages = dom0_max_nrpages;
+        nr_pages = get_memsize(&dom0_size, avail);
+        min_pages = get_memsize(&dom0_min_size, avail);
+        max_pages = get_memsize(&dom0_max_size, avail);
 
         /*
          * If allocation isn't specified, reserve 1/16th of available memory
@@ -308,14 +367,9 @@ unsigned long __init dom0_compute_nr_pages(
          * maximum of 128MB.
          */
         if ( !nr_pages )
-            nr_pages = -(pv_shim ? pv_shim_mem(avail)
+            nr_pages = avail - (pv_shim ? pv_shim_mem(avail)
                                  : min(avail / 16, 128UL << (20 - PAGE_SHIFT)));
 
-        /* Negative specification means "all memory - specified amount". */
-        if ( (long)nr_pages  < 0 ) nr_pages  += avail;
-        if ( (long)min_pages < 0 ) min_pages += avail;
-        if ( (long)max_pages < 0 ) max_pages += avail;
-
         /* Clamp according to min/max limits and available memory. */
         nr_pages = max(nr_pages, min_pages);
         nr_pages = min(nr_pages, max_pages);
@@ -329,8 +383,8 @@ unsigned long __init dom0_compute_nr_pages(
     }
 
     if ( is_pv_domain(d) &&
-         (parms->p2m_base == UNSET_ADDR) && (dom0_nrpages <= 0) &&
-         ((dom0_min_nrpages <= 0) || (nr_pages > min_pages)) )
+         (parms->p2m_base == UNSET_ADDR) && !memsize_gt_zero(&dom0_size) &&
+         (!memsize_gt_zero(&dom0_min_size) || (nr_pages > min_pages)) )
     {
         /*
          * Legacy Linux kernels (i.e. such without a XEN_ELFNOTE_INIT_P2M
@@ -356,7 +410,7 @@ unsigned long __init dom0_compute_nr_pages(
         {
             end = sizeof_long >= sizeof(end) ? 0 : 1UL << (8 * sizeof_long);
             nr_pages = (end - vend) / (2 * sizeof_long);
-            if ( dom0_min_nrpages > 0 && nr_pages < min_pages )
+            if ( memsize_gt_zero(&dom0_min_size) && nr_pages < min_pages )
                 nr_pages = min_pages;
             printk("Dom0 memory clipped to %lu pages\n", nr_pages);
         }
-- 
2.16.4


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

Re: [PATCH v2 2/3] xen/x86: add dom0 memory sizing variants

[Jan Beulich]

>>> On 06.12.18 at 09:06, <jgross@suse.com> wrote:
> Today the memory size of dom0 can be specified only in terms of bytes
> (either an absolute value or "host-mem - value"). When dom0 shouldn't
> be auto-ballooned this requires nearly always a manual adaption of the
> Xen boot parameters to reflect the actual host memory size.
> 
> Add more possibilities to specify memory sizes. Today we have:
> 
> dom0_mem= List of ( min:<size> | max:<size> | <size> )
> 
> with <size> being a positive or negative size value (e.g. 1G).
> 
> Modify that to:
> 
> dom0_mem= List of ( min:<sz> | max:<sz> | <sz> )
> <sz>: <size> | [<size>+]<frac>%
> <frac>: integer value < 100
> 
> With the following semantics:
> 
> <frac>% specifies a fraction of host memory size in percent.
> <sz> is a percentage of host memory plus an offset.
> 
> So <sz> being 1G+25% on a 256G host would result in 65G.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

I notice though that ...

> -static long __init parse_amt(const char *s, const char **ps)
> +static int __init parse_amt(const char *s, const char **ps, struct memsize *sz)
>  {
> -    long pages = parse_size_and_unit((*s == '-') ? s+1 : s, ps) >> PAGE_SHIFT;
> -    return (*s == '-') ? -pages : pages;
> +    unsigned long val;
> +    struct memsize tmp = { };
> +
> +    tmp.minus = (*s == '-');
> +    if ( tmp.minus )
> +        s++;
> +
> +    /* Avoid accessing s[-1] in case value starts with '%'. */
> +    if ( *s == '%' )
> +        return -EINVAL;
> +
> +    while ( isdigit(*s) )
> +    {
> +        val = parse_size_and_unit_or_int(s, ps, '%');
> +        s = *ps;
> +        if ( *s == '%' )
> +        {
> +            if ( !isdigit(s[-1]) || val >= 100 )
> +                return -EINVAL;
> +            tmp.percent = val;
> +            s++;
> +        }
> +        else
> +            tmp.nr_pages = val >> PAGE_SHIFT;
> +        if ( *s == '+' )
> +            s++;
> +    }

... you allow more flexibility here than you document (i.e. also
<percentage>+<basesize>). You may want to consider
refusing something like 1G+10%+10%, though.

Jan



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

Re: [PATCH v2 2/3] xen/x86: add dom0 memory sizing variants

[Juergen Gross]

On 06/12/2018 12:08, Jan Beulich wrote:
>>>> On 06.12.18 at 09:06, <jgross@suse.com> wrote:
>> Today the memory size of dom0 can be specified only in terms of bytes
>> (either an absolute value or "host-mem - value"). When dom0 shouldn't
>> be auto-ballooned this requires nearly always a manual adaption of the
>> Xen boot parameters to reflect the actual host memory size.
>>
>> Add more possibilities to specify memory sizes. Today we have:
>>
>> dom0_mem= List of ( min:<size> | max:<size> | <size> )
>>
>> with <size> being a positive or negative size value (e.g. 1G).
>>
>> Modify that to:
>>
>> dom0_mem= List of ( min:<sz> | max:<sz> | <sz> )
>> <sz>: <size> | [<size>+]<frac>%
>> <frac>: integer value < 100
>>
>> With the following semantics:
>>
>> <frac>% specifies a fraction of host memory size in percent.
>> <sz> is a percentage of host memory plus an offset.
>>
>> So <sz> being 1G+25% on a 256G host would result in 65G.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> I notice though that ...
> 
>> -static long __init parse_amt(const char *s, const char **ps)
>> +static int __init parse_amt(const char *s, const char **ps, struct memsize *sz)
>>  {
>> -    long pages = parse_size_and_unit((*s == '-') ? s+1 : s, ps) >> PAGE_SHIFT;
>> -    return (*s == '-') ? -pages : pages;
>> +    unsigned long val;
>> +    struct memsize tmp = { };
>> +
>> +    tmp.minus = (*s == '-');
>> +    if ( tmp.minus )
>> +        s++;
>> +
>> +    /* Avoid accessing s[-1] in case value starts with '%'. */
>> +    if ( *s == '%' )
>> +        return -EINVAL;
>> +
>> +    while ( isdigit(*s) )
>> +    {
>> +        val = parse_size_and_unit_or_int(s, ps, '%');
>> +        s = *ps;
>> +        if ( *s == '%' )
>> +        {
>> +            if ( !isdigit(s[-1]) || val >= 100 )
>> +                return -EINVAL;
>> +            tmp.percent = val;
>> +            s++;
>> +        }
>> +        else
>> +            tmp.nr_pages = val >> PAGE_SHIFT;
>> +        if ( *s == '+' )
>> +            s++;
>> +    }
> 
> ... you allow more flexibility here than you document (i.e. also
> <percentage>+<basesize>). You may want to consider
> refusing something like 1G+10%+10%, though.

Okay, should be fairly easy.

Can I keep your R-b: with adding something like:

+    bool percent = false;
...
-    while ( isdigit(*s) )
+    while ( isdigit(*s) && !percent )
...
         if ( *s == '%' )
         {
+            percent = true;
...


Juergen


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

Re: [PATCH v2 2/3] xen/x86: add dom0 memory sizing variants

[Jan Beulich]

>>> On 06.12.18 at 12:20, <jgross@suse.com> wrote:
> On 06/12/2018 12:08, Jan Beulich wrote:
>>>>> On 06.12.18 at 09:06, <jgross@suse.com> wrote:
>>> Today the memory size of dom0 can be specified only in terms of bytes
>>> (either an absolute value or "host-mem - value"). When dom0 shouldn't
>>> be auto-ballooned this requires nearly always a manual adaption of the
>>> Xen boot parameters to reflect the actual host memory size.
>>>
>>> Add more possibilities to specify memory sizes. Today we have:
>>>
>>> dom0_mem= List of ( min:<size> | max:<size> | <size> )
>>>
>>> with <size> being a positive or negative size value (e.g. 1G).
>>>
>>> Modify that to:
>>>
>>> dom0_mem= List of ( min:<sz> | max:<sz> | <sz> )
>>> <sz>: <size> | [<size>+]<frac>%
>>> <frac>: integer value < 100
>>>
>>> With the following semantics:
>>>
>>> <frac>% specifies a fraction of host memory size in percent.
>>> <sz> is a percentage of host memory plus an offset.
>>>
>>> So <sz> being 1G+25% on a 256G host would result in 65G.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> 
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> 
>> I notice though that ...
>> 
>>> -static long __init parse_amt(const char *s, const char **ps)
>>> +static int __init parse_amt(const char *s, const char **ps, struct memsize *sz)
>>>  {
>>> -    long pages = parse_size_and_unit((*s == '-') ? s+1 : s, ps) >> PAGE_SHIFT;
>>> -    return (*s == '-') ? -pages : pages;
>>> +    unsigned long val;
>>> +    struct memsize tmp = { };
>>> +
>>> +    tmp.minus = (*s == '-');
>>> +    if ( tmp.minus )
>>> +        s++;
>>> +
>>> +    /* Avoid accessing s[-1] in case value starts with '%'. */
>>> +    if ( *s == '%' )
>>> +        return -EINVAL;
>>> +
>>> +    while ( isdigit(*s) )
>>> +    {
>>> +        val = parse_size_and_unit_or_int(s, ps, '%');
>>> +        s = *ps;
>>> +        if ( *s == '%' )
>>> +        {
>>> +            if ( !isdigit(s[-1]) || val >= 100 )
>>> +                return -EINVAL;
>>> +            tmp.percent = val;
>>> +            s++;
>>> +        }
>>> +        else
>>> +            tmp.nr_pages = val >> PAGE_SHIFT;
>>> +        if ( *s == '+' )
>>> +            s++;
>>> +    }
>> 
>> ... you allow more flexibility here than you document (i.e. also
>> <percentage>+<basesize>). You may want to consider
>> refusing something like 1G+10%+10%, though.
> 
> Okay, should be fairly easy.
> 
> Can I keep your R-b: with adding something like:
> 
> +    bool percent = false;
> ...
> -    while ( isdigit(*s) )
> +    while ( isdigit(*s) && !percent )
> ...
>          if ( *s == '%' )
>          {
> +            percent = true;
> ...

Something like this, yes. The double percent value was just an
example though, "1G+10%+1G" then too would better either work
as written, or be refused.

Jan



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

[PATCH v2 3/3] xen: add CONFIG item for default dom0 memory size

[Juergen Gross]

With being able to specify a dom0_mem value depending on host memory
size on x86 make it easy for distros to specify a default dom0 size by
adding a CONFIG_DOM0_MEM item which presets the dom0_mem boot parameter
value.

It will be used only if no dom0_mem parameter was specified in the
boot parameters.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/arm/domain_build.c |  7 +++++++
 xen/arch/x86/dom0_build.c   |  6 ++++++
 xen/common/Kconfig          | 13 +++++++++++++
 3 files changed, 26 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b0ec3f0b72..d2c63a89ca 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -32,9 +32,12 @@ static unsigned int __initdata opt_dom0_max_vcpus;
 integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
 
 static u64 __initdata dom0_mem;
+static bool __initdata dom0_mem_set;
 
 static int __init parse_dom0_mem(const char *s)
 {
+    dom0_mem_set = true;
+
     dom0_mem = parse_size_and_unit(s, &s);
 
     return *s ? -EINVAL : 0;
@@ -2114,6 +2117,10 @@ int __init construct_dom0(struct domain *d)
     BUG_ON(d->domain_id != 0);
 
     printk("*** LOADING DOMAIN 0 ***\n");
+
+    if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
+        parse_dom0_mem(CONFIG_DOM0_MEM);
+
     if ( dom0_mem <= 0 )
     {
         warning_add("PLEASE SPECIFY dom0_mem PARAMETER - USING 512M FOR NOW\n");
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 8de71346db..71b670275b 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -29,6 +29,7 @@ struct memsize {
 static struct memsize __initdata dom0_size;
 static struct memsize __initdata dom0_min_size;
 static struct memsize __initdata dom0_max_size = { .nr_pages = LONG_MAX };
+static bool __initdata dom0_mem_set;
 
 static bool __init memsize_gt_zero(const struct memsize *sz)
 {
@@ -111,6 +112,8 @@ static int __init parse_dom0_mem(const char *s)
 {
     int ret;
 
+    dom0_mem_set = true;
+
     /* xen-shim uses shim_mem parameter instead of dom0_mem */
     if ( pv_shim )
     {
@@ -333,6 +336,9 @@ unsigned long __init dom0_compute_nr_pages(
     unsigned long avail = 0, nr_pages, min_pages, max_pages;
     bool need_paging;
 
+    if ( !dom0_mem_set && CONFIG_DOM0_MEM[0] )
+        parse_dom0_mem(CONFIG_DOM0_MEM);
+
     for_each_node_mask ( node, dom0_nodes )
         avail += avail_domheap_pages_region(node, 0, 0) +
                  initial_images_nrpages(node);
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 68132a3a10..155a9a45e8 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -323,4 +323,17 @@ config CMDLINE_OVERRIDE
 
 	  This is used to work around broken bootloaders. This should
 	  be set to 'N' under normal conditions.
+
+config DOM0_MEM
+	string "Default value for dom0_mem boot parameter"
+	default ""
+	---help---
+	  Sets a default value for dom0_mem, e.g. "512M".
+	  The specified string will be used for the dom0_mem parameter in
+	  case it was not specified on the command line.
+
+	  See docs/misc/xen-command-line.markdown for the supported syntax.
+
+	  Leave empty if you are not sure what to specify.
+	
 endmenu
-- 
2.16.4


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

Re: [PATCH v2 3/3] xen: add CONFIG item for default dom0 memory size

[Jan Beulich]

>>> On 06.12.18 at 09:06, <jgross@suse.com> wrote:
> With being able to specify a dom0_mem value depending on host memory
> size on x86 make it easy for distros to specify a default dom0 size by
> adding a CONFIG_DOM0_MEM item which presets the dom0_mem boot parameter
> value.
> 
> It will be used only if no dom0_mem parameter was specified in the
> boot parameters.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH v2 1/3] xen: introduce parse_size_and_unit_or_int

[Juergen Gross]

On 06/12/2018 11:15, Jan Beulich wrote:
>>>> On 06.12.18 at 11:01, <jgross@suse.com> wrote:
>> On 06/12/2018 10:50, Jan Beulich wrote:
>>>>>> On 06.12.18 at 09:06, <jgross@suse.com> wrote:
>>>> @@ -477,7 +478,8 @@ unsigned long long parse_size_and_unit(const char *s, 
>> const char **ps)
>>>>          s1++;
>>>>          break;
>>>>      default:
>>>> -        ret <<= 10; /* default to kB */
>>>> +        if ( *s1 && *s1 != no_size )
>>>> +            ret <<= 10; /* default to kB */
>>>>          break;
>>>
>>> So did you figure anything wrong with simply special casing '%'
>>> here? '%' is a form of "unit", after all. Perhaps demanding "ps"
>>> to be non-NULL (to have some form of indication the caller will
>>> check the suffix char) might be reasonable.
>>
>> I thought a more general approach would be better. I can use the simple
>> '%' special case with the ps check in case you prefer that.
> 
> I'd indeed prefer that, first and foremost because that other
> function's name becomes too long for my taste, but give others
> a chance to voice differing opinions.

In case the function name is your main concern I could rename it to
parse_size_or_int() or similar.

I'm fine either way, so lets wait for some days and act then according
to the comments.


Juergen

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