| Message ID | c5844665-8ed7-d91f-a41c-9e4eb3e2bcc2@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] x86+libxl: correct p2m (shadow) memory pool size calculation | expand |
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
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.
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
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.
--- 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); }
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).