When building a domain, xc_domain_setmaxmem() is called with target_memkb (memory in domain config file).
If a config specifies maxmem > memory, any attempts to increase the domain memory size to its max,
outside of xl mem-set or xl mem-max, which already call xc_domain_setmaxmem() properly, will fail.
Changed xc_domain_setmaxmem() call inside libxl__build_pre() to use max_memkb.

Signed-off-by: Kevin Alarcon Negy <kevin@exostellar.io>
---
 tools/libs/light/libxl_dom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 94fef37401..16aa255aad 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -355,7 +355,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
         return ERROR_FAIL;
     }
 
-    if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb + size) < 0) {
+    if (xc_domain_setmaxmem(ctx->xch, domid, info->max_memkb + size) < 0) {
         LOGE(ERROR, "Couldn't set max memory");
         return ERROR_FAIL;
     }
-- 
2.25.1

On 08.08.23 00:16, Kevin Alarcon Negy wrote:
> When building a domain, xc_domain_setmaxmem() is called with target_memkb (memory in domain config file).
> If a config specifies maxmem > memory, any attempts to increase the domain memory size to its max,
> outside of xl mem-set or xl mem-max, which already call xc_domain_setmaxmem() properly, will fail.

But this is how it should work, no?

With your change the guest could easily balloon itself up to maxmem without it
having been allowed to do so.

The maxmem config option is meant to tell the domain how much memory it should
be prepared to use some time in the future. It isn't meant to allow the domain
to use right now.


Juergen

> Changed xc_domain_setmaxmem() call inside libxl__build_pre() to use max_memkb.
> 
> Signed-off-by: Kevin Alarcon Negy <kevin@exostellar.io>
> ---
>   tools/libs/light/libxl_dom.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index 94fef37401..16aa255aad 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -355,7 +355,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>           return ERROR_FAIL;
>       }
>   
> -    if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb + size) < 0) {
> +    if (xc_domain_setmaxmem(ctx->xch, domid, info->max_memkb + size) < 0) {
>           LOGE(ERROR, "Couldn't set max memory");
>           return ERROR_FAIL;
>       }

Apologies if I misused the "RESEND" subject line. The xen patch guide
[1] seemed to suggest using it as a way to ping.

Thanks for the feedback. I realize now that my misunderstanding in how
the original code should work is because of my confusion between
"maxmem" the config variable vs. "xl mem-max" command. I thought that
both should act exactly the same way. As in, "xl mem-max" calls
xc_domain_setmaxmem() and also sets the static-max variable [2]. I
know that maxmem (config variable) starts out as just the static-max
variable and does not result in an xc_domain_setmaxmem(maxmem) call
upon bootup, but it wasn't clear to me that this was intended. My
patch was intended to make both the config variable and the xl command
act in the same way.

Perhaps this distinction is better resolved with different naming? For
instance, instead of "maxmem" for the config variable, call it
"static-max" to match its internal meaning?

I appreciate your thoughts.
Kevin

[1] https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Resending_Patches
[2] https://github.com/xen-project/xen/blob/master/tools/libs/light/libxl_mem.c#L76

On Tue, Aug 8, 2023 at 3:14 AM Juergen Gross <jgross@suse.com> wrote:
>
> On 08.08.23 00:16, Kevin Alarcon Negy wrote:
> > When building a domain, xc_domain_setmaxmem() is called with target_memkb (memory in domain config file).
> > If a config specifies maxmem > memory, any attempts to increase the domain memory size to its max,
> > outside of xl mem-set or xl mem-max, which already call xc_domain_setmaxmem() properly, will fail.
>
> But this is how it should work, no?
>
> With your change the guest could easily balloon itself up to maxmem without it
> having been allowed to do so.
>
> The maxmem config option is meant to tell the domain how much memory it should
> be prepared to use some time in the future. It isn't meant to allow the domain
> to use right now.
>
>
> Juergen
>
> > Changed xc_domain_setmaxmem() call inside libxl__build_pre() to use max_memkb.
> >
> > Signed-off-by: Kevin Alarcon Negy <kevin@exostellar.io>
> > ---
> >   tools/libs/light/libxl_dom.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> > index 94fef37401..16aa255aad 100644
> > --- a/tools/libs/light/libxl_dom.c
> > +++ b/tools/libs/light/libxl_dom.c
> > @@ -355,7 +355,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> >           return ERROR_FAIL;
> >       }
> >
> > -    if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb + size) < 0) {
> > +    if (xc_domain_setmaxmem(ctx->xch, domid, info->max_memkb + size) < 0) {
> >           LOGE(ERROR, "Couldn't set max memory");
> >           return ERROR_FAIL;
> >       }
>

On 12.08.23 04:32, Kevin Alarcon Negy wrote:
> Apologies if I misused the "RESEND" subject line. The xen patch guide
> [1] seemed to suggest using it as a way to ping.
> 
> Thanks for the feedback. I realize now that my misunderstanding in how
> the original code should work is because of my confusion between
> "maxmem" the config variable vs. "xl mem-max" command. I thought that
> both should act exactly the same way. As in, "xl mem-max" calls
> xc_domain_setmaxmem() and also sets the static-max variable [2]. I
> know that maxmem (config variable) starts out as just the static-max
> variable and does not result in an xc_domain_setmaxmem(maxmem) call
> upon bootup, but it wasn't clear to me that this was intended. My
> patch was intended to make both the config variable and the xl command
> act in the same way.
> 
> Perhaps this distinction is better resolved with different naming? For
> instance, instead of "maxmem" for the config variable, call it
> "static-max" to match its internal meaning?

While you are right with "static-max" explaining the semantics for
someone familiar with the internals better, I'm not sure this applies
to Xen users, too.

Additionally we would need to support both names after doing the switch,
as we don't want to break existing config files using "maxmem".

So changing the parameter name would not really help IMHO.


Juergen

On 08.08.2023 00:16, Kevin Alarcon Negy wrote:
> When building a domain, xc_domain_setmaxmem() is called with target_memkb (memory in domain config file).
> If a config specifies maxmem > memory, any attempts to increase the domain memory size to its max,
> outside of xl mem-set or xl mem-max, which already call xc_domain_setmaxmem() properly, will fail.
> Changed xc_domain_setmaxmem() call inside libxl__build_pre() to use max_memkb.
> 
> Signed-off-by: Kevin Alarcon Negy <kevin@exostellar.io>
> ---
>  tools/libs/light/libxl_dom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

A note on why you resent would have been useful here. Is this perhaps
more a ping than a resend?

Jan

> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index 94fef37401..16aa255aad 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -355,7 +355,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>          return ERROR_FAIL;
>      }
>  
> -    if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb + size) < 0) {
> +    if (xc_domain_setmaxmem(ctx->xch, domid, info->max_memkb + size) < 0) {
>          LOGE(ERROR, "Couldn't set max memory");
>          return ERROR_FAIL;
>      }