[PATCH RFC] x86+libxl: correct p2m (shadow) memory pool size calculation

Jan Beulich posted 1 patch 2 years ago
Failed in applying to current master (apply log)
There is a newer version of this series
[PATCH RFC] x86+libxl: correct p2m (shadow) memory pool size calculation
Posted by Jan Beulich 2 years ago
The reference "to shadow the resident processes" is applicable to
domains (potentially) running in shadow mode only. Adjust the
calculations accordingly.

In dom0_paging_pages() also take the opportunity and stop open-coding
DIV_ROUND_UP().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: I'm pretty sure I can't change a public libxl function (deprecated
     or not) like this, but I also don't know how I should go about
     doing so (short of introducing a brand new function and leaving the
     existing one broken).

--- a/tools/include/libxl_utils.h
+++ b/tools/include/libxl_utils.h
@@ -23,7 +23,10 @@ const
 #endif
 char *libxl_basename(const char *name); /* returns string from strdup */
 
-unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned int smp_cpus);
+unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb,
+                                               unsigned int smp_cpus,
+                                               libxl_domain_type type,
+                                               bool hap);
   /* deprecated; see LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONFIG in libxl.h */
 int libxl_name_to_domid(libxl_ctx *ctx, const char *name, uint32_t *domid);
 int libxl_domain_qualifier_to_domid(libxl_ctx *ctx, const char *name, uint32_t *domid);
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1194,10 +1194,17 @@ int libxl__domain_config_setdefault(libx
     }
 
     if (d_config->b_info.shadow_memkb == LIBXL_MEMKB_DEFAULT
-        && ok_to_default_memkb_in_create(gc))
+        && ok_to_default_memkb_in_create(gc)) {
+        bool hap = d_config->c_info.type != LIBXL_DOMAIN_TYPE_PV
+                   ? libxl_defbool_val(d_config->c_info.hap)
+                   : false;
+
         d_config->b_info.shadow_memkb =
             libxl_get_required_shadow_memory(d_config->b_info.max_memkb,
-                                             d_config->b_info.max_vcpus);
+                                             d_config->b_info.max_vcpus,
+                                             d_config->c_info.type,
+                                             hap);
+    }
 
     /* No IOMMU reservation is needed if passthrough mode is not 'sync_pt' */
     if (d_config->b_info.iommu_memkb == LIBXL_MEMKB_DEFAULT
--- a/tools/libs/light/libxl_utils.c
+++ b/tools/libs/light/libxl_utils.c
@@ -36,15 +36,21 @@ char *libxl_basename(const char *name)
     return strdup(name);
 }
 
-unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned int smp_cpus)
+unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb,
+                                               unsigned int smp_cpus,
+                                               libxl_domain_type type,
+                                               bool hap)
 {
     /* 256 pages (1MB) per vcpu,
-       plus 1 page per MiB of RAM for the P2M map,
-       plus 1 page per MiB of RAM to shadow the resident processes.
+       plus 1 page per MiB of RAM for the P2M map (for non-PV guests),
+       plus 1 page per MiB of RAM to shadow the resident processes (for shadow
+       mode guests).
        This is higher than the minimum that Xen would allocate if no value
        were given (but the Xen minimum is for safety, not performance).
      */
-    return 4 * (256 * smp_cpus + 2 * (maxmem_kb / 1024));
+    return 4 * (256 * smp_cpus +
+                ((type != LIBXL_DOMAIN_TYPE_PV) + !hap) *
+                (maxmem_kb / 1024));
 }
 
 char *libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid)
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -317,9 +317,12 @@ unsigned long __init dom0_paging_pages(c
     /* Copied from: libxl_get_required_shadow_memory() */
     unsigned long memkb = nr_pages * (PAGE_SIZE / 1024);
 
-    memkb = 4 * (256 * d->max_vcpus + 2 * (memkb / 1024));
+    memkb = 4 * (256 * d->max_vcpus +
+                 (paging_mode_enabled(d) +
+                  (opt_dom0_shadow || opt_pv_l1tf_hwdom)) *
+                 (memkb / 1024));
 
-    return ((memkb + 1023) / 1024) << (20 - PAGE_SHIFT);
+    return DIV_ROUND_UP(memkb, 1024) << (20 - PAGE_SHIFT);
 }
Re: [PATCH RFC] x86+libxl: correct p2m (shadow) memory pool size calculation
Posted by Roger Pau Monné 2 years ago
On Fri, Apr 22, 2022 at 12:57:03PM +0200, Jan Beulich wrote:
> The reference "to shadow the resident processes" is applicable to
> domains (potentially) running in shadow mode only. Adjust the
> calculations accordingly.
> 
> In dom0_paging_pages() also take the opportunity and stop open-coding
> DIV_ROUND_UP().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> RFC: I'm pretty sure I can't change a public libxl function (deprecated
>      or not) like this, but I also don't know how I should go about
>      doing so (short of introducing a brand new function and leaving the
>      existing one broken).

You have to play with LIBXL_API_VERSION, see for example:

1e3304005e libxl: Make libxl_retrieve_domain_configuration async

> 
> --- a/tools/include/libxl_utils.h
> +++ b/tools/include/libxl_utils.h
> @@ -23,7 +23,10 @@ const
>  #endif
>  char *libxl_basename(const char *name); /* returns string from strdup */
>  
> -unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned int smp_cpus);
> +unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb,
> +                                               unsigned int smp_cpus,
> +                                               libxl_domain_type type,
> +                                               bool hap);

Iff we are to change this anyway, we might as well rename the
function and introduce a proper
libxl_get_required_{paging,p2m}_memory?

It seems wrong to have a function explicitly named 'shadow' that takes
a 'hap' parameter.

If you introduce a new function there's no need to play with the
LIBXL_API_VERSION and you can just add a new LIBXL_HAVE_FOO define.

Thanks, Roger.
Re: [PATCH RFC] x86+libxl: correct p2m (shadow) memory pool size calculation
Posted by Jan Beulich 2 years ago
On 22.04.2022 13:14, Roger Pau Monné wrote:
> On Fri, Apr 22, 2022 at 12:57:03PM +0200, Jan Beulich wrote:
>> The reference "to shadow the resident processes" is applicable to
>> domains (potentially) running in shadow mode only. Adjust the
>> calculations accordingly.
>>
>> In dom0_paging_pages() also take the opportunity and stop open-coding
>> DIV_ROUND_UP().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> RFC: I'm pretty sure I can't change a public libxl function (deprecated
>>      or not) like this, but I also don't know how I should go about
>>      doing so (short of introducing a brand new function and leaving the
>>      existing one broken).
> 
> You have to play with LIBXL_API_VERSION, see for example:
> 
> 1e3304005e libxl: Make libxl_retrieve_domain_configuration async
> 
>>
>> --- a/tools/include/libxl_utils.h
>> +++ b/tools/include/libxl_utils.h
>> @@ -23,7 +23,10 @@ const
>>  #endif
>>  char *libxl_basename(const char *name); /* returns string from strdup */
>>  
>> -unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned int smp_cpus);
>> +unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb,
>> +                                               unsigned int smp_cpus,
>> +                                               libxl_domain_type type,
>> +                                               bool hap);
> 
> Iff we are to change this anyway, we might as well rename the
> function and introduce a proper
> libxl_get_required_{paging,p2m}_memory?
> 
> It seems wrong to have a function explicitly named 'shadow' that takes
> a 'hap' parameter.
> 
> If you introduce a new function there's no need to play with the
> LIBXL_API_VERSION and you can just add a new LIBXL_HAVE_FOO define.

With the original function deprecated, I don't see why I'd need to
make a new public function - my fallback plan was (as also suggested
by Jürgen) to make a new internal function.

Jan
Re: [PATCH RFC] x86+libxl: correct p2m (shadow) memory pool size calculation
Posted by Roger Pau Monné 2 years ago
On Fri, Apr 22, 2022 at 01:56:17PM +0200, Jan Beulich wrote:
> On 22.04.2022 13:14, Roger Pau Monné wrote:
> > On Fri, Apr 22, 2022 at 12:57:03PM +0200, Jan Beulich wrote:
> >> The reference "to shadow the resident processes" is applicable to
> >> domains (potentially) running in shadow mode only. Adjust the
> >> calculations accordingly.
> >>
> >> In dom0_paging_pages() also take the opportunity and stop open-coding
> >> DIV_ROUND_UP().
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> RFC: I'm pretty sure I can't change a public libxl function (deprecated
> >>      or not) like this, but I also don't know how I should go about
> >>      doing so (short of introducing a brand new function and leaving the
> >>      existing one broken).
> > 
> > You have to play with LIBXL_API_VERSION, see for example:
> > 
> > 1e3304005e libxl: Make libxl_retrieve_domain_configuration async
> > 
> >>
> >> --- a/tools/include/libxl_utils.h
> >> +++ b/tools/include/libxl_utils.h
> >> @@ -23,7 +23,10 @@ const
> >>  #endif
> >>  char *libxl_basename(const char *name); /* returns string from strdup */
> >>  
> >> -unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned int smp_cpus);
> >> +unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb,
> >> +                                               unsigned int smp_cpus,
> >> +                                               libxl_domain_type type,
> >> +                                               bool hap);
> > 
> > Iff we are to change this anyway, we might as well rename the
> > function and introduce a proper
> > libxl_get_required_{paging,p2m}_memory?
> > 
> > It seems wrong to have a function explicitly named 'shadow' that takes
> > a 'hap' parameter.
> > 
> > If you introduce a new function there's no need to play with the
> > LIBXL_API_VERSION and you can just add a new LIBXL_HAVE_FOO define.
> 
> With the original function deprecated, I don't see why I'd need to
> make a new public function - my fallback plan was (as also suggested
> by Jürgen) to make a new internal function.

Yes, that would be fine if there's no need to expose the new function
for external callers.

Thanks, Roger.

Re: [PATCH RFC] x86+libxl: correct p2m (shadow) memory pool size calculation
Posted by Juergen Gross 2 years ago
On 22.04.22 12:57, Jan Beulich wrote:
> The reference "to shadow the resident processes" is applicable to
> domains (potentially) running in shadow mode only. Adjust the
> calculations accordingly.
> 
> In dom0_paging_pages() also take the opportunity and stop open-coding
> DIV_ROUND_UP().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> RFC: I'm pretty sure I can't change a public libxl function (deprecated
>       or not) like this, but I also don't know how I should go about
>       doing so (short of introducing a brand new function and leaving the
>       existing one broken).

I'd modify the deprecated function to use the worst case scenario and
use a new function internally.


Juergen