Re: [PATCH RESEND] libxl: Make domain build xc_domain_setmaxmem() call use max_memkb

[Kevin Alarcon Negy <kevin@xxxxxxxxxxxxx>]

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@xxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> >   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;
> >       }
>