Re: [PATCH v2 for-4.5] libxl: account for romfile memory

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2014-11-25 at 12:43 +0000, Stefano Stabellini wrote:
> Account for the extra memory needed for the rom files of any emulated nics:
> QEMU uses xc_domain_populate_physmap_exact to allocate the memory for
> each them. Assume 256K each.

I suppose this will have to do for 4.5. Can we do something better in
the future -- like figuring out a way for guests to have
"not-really-RAM" allocations like this which are made by the toolstack
and happen to be backed by RAM not count or something.

> 
> This patch fixes a QEMU abort() when more than 4 emulated nics are
> assigned to a VM.

Are you also going to fix qemu to fail gracefully if it cannot deploy
option roms? abort() seems a bit extreme.

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Don Slutz <dslutz@verizon.com>
> CC: hanyandong <hanyandong@iie.ac.cn>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>

You missed Ian J. I've added him.

> 
> ---
> 
> Changes in v2:
> - remove double return statement;
> - check for return errors;
> - check for overflows.
> ---
>  tools/libxl/libxl.c          |   53 +++++++++++++++++++++++++++++++++++++-----
>  tools/libxl/libxl_dom.c      |    8 +++++--
>  tools/libxl/libxl_internal.h |    7 ++++++
>  3 files changed, 60 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index de23fec..2cdb768 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4527,13 +4527,40 @@ out:
>  
>  /******************************************************************************/
>  
> +int libxl__get_rom_memory_kb(libxl__gc *gc, uint32_t domid, libxl_domain_config *d_config)
> +{
> +    int i, romsize, rc;
> +    libxl_domain_config local_d_config;
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +
> +    if (d_config == NULL) {
> +        libxl_domain_config_init(&local_d_config);
> +        rc = libxl_retrieve_domain_configuration(ctx, domid, &local_d_config);
> +        if (rc < 0)
> +            return rc;
> +        d_config = &local_d_config;
> +    }

Perhaps we could store the answer to this function in XS when we build
the domain and simply read it back and account for it in the places
which use it?

Apart from being rather costly reparsing the json every time is going to
behave a bit strangely if NICs are plugged/unplugged at runtime and
ballooning is going on.

> +    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV)
> +        return 0;
> +
> +    for (i = 0, romsize = 0;
> +         i < d_config->num_nics && romsize < INT_MAX;

I don't think that romsize < INT_MAX is useful except in the case that
romsize+= results in romsize == INT_MAX. If you actually overflow then
romsize becomes negative which satisfies the condition (and in any case
you are into undefined behaviour territory there anyhow, I think).

Given that INT_MAX is a boat load of ROMs I'd be inclined to just limit
it to INT_MAX/2 or /4 or something.

Or you could do romsize < (INT_MAX - LIBXL_ROMSIZE_KB) I suppose.

> +    rc = xc_domain_setmaxmem(ctx->xch, domid,
> +                             max_memkb + LIBXL_MAXMEM_CONSTANT
> +                             + romsize);

Seems like we ought to have a helper to return the memory overheads,
which would be the constant + the romsize starting from now...

Ian.