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

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

[Stefano Stabellini]

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.

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

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>

---

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;
+    }
+
+    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++) {
+        if (d_config->nics[i].nictype == LIBXL_NIC_TYPE_VIF_IOEMU)
+            romsize += LIBXL_ROMSIZE_KB;
+    }
+
+    return romsize;
+}
+
 int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t max_memkb)
 {
     GC_INIT(ctx);
     char *mem, *endptr;
     uint32_t memorykb;
     char *dompath = libxl__xs_get_dompath(gc, domid);
-    int rc = 1;
+    int rc = 1, romsize;
 
     mem = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/memory/target", dompath));
     if (!mem) {
@@ -4550,11 +4577,18 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t max_memkb)
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "memory_static_max must be greater than or or equal to memory_dynamic_max\n");
         goto out;
     }
-    rc = xc_domain_setmaxmem(ctx->xch, domid, max_memkb + LIBXL_MAXMEM_CONSTANT);
+    rc = libxl__get_rom_memory_kb(gc, domid, NULL);
+    if (rc < 0)
+        goto out;
+    romsize = rc;
+    rc = xc_domain_setmaxmem(ctx->xch, domid,
+                             max_memkb + LIBXL_MAXMEM_CONSTANT
+                             + romsize);
     if (rc != 0) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
                 "xc_domain_setmaxmem domid=%d memkb=%d failed "
-                "rc=%d\n", domid, max_memkb + LIBXL_MAXMEM_CONSTANT, rc);
+                "rc=%d\n", domid, max_memkb + LIBXL_MAXMEM_CONSTANT +
+                romsize, rc);
         goto out;
     }
 
@@ -4683,7 +4717,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
         int32_t target_memkb, int relative, int enforce)
 {
     GC_INIT(ctx);
-    int rc = 1, abort_transaction = 0;
+    int rc = 1, abort_transaction = 0, romsize;
     uint32_t memorykb = 0, videoram = 0;
     uint32_t current_target_memkb = 0, new_target_memkb = 0;
     uint32_t current_max_memkb = 0;
@@ -4769,12 +4803,19 @@ retry_transaction:
 
     if (enforce) {
         memorykb = new_target_memkb;
+        rc = libxl__get_rom_memory_kb(gc, domid, NULL);
+        if (rc < 0) {
+            abort_transaction = 1;
+            goto out;
+        }
+        romsize = rc;
         rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
-                LIBXL_MAXMEM_CONSTANT);
+                LIBXL_MAXMEM_CONSTANT + romsize);
         if (rc != 0) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
                     "xc_domain_setmaxmem domid=%d memkb=%d failed "
-                    "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT, rc);
+                    "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT +
+                    romsize, rc);
             abort_transaction = 1;
             goto out;
         }
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 74ea84b..733f4c7 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -305,7 +305,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     libxl_domain_build_info *const info = &d_config->b_info;
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *xs_domid, *con_domid;
-    int rc;
+    int rc, romsize;
 
     if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
         LOG(ERROR, "Couldn't set max vcpu count");
@@ -405,8 +405,12 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
         }
     }
 
+	rc = libxl__get_rom_memory_kb(gc, domid, d_config);
+	if (rc < 0)
+		return rc;
+	romsize = rc;
     if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
-        LIBXL_MAXMEM_CONSTANT) < 0) {
+        LIBXL_MAXMEM_CONSTANT + romsize) < 0) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't set max memory");
         return ERROR_FAIL;
     }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4361421..33826ea 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -90,6 +90,7 @@
 #define LIBXL_XENCONSOLE_LIMIT 1048576
 #define LIBXL_XENCONSOLE_PROTOCOL "vt100"
 #define LIBXL_MAXMEM_CONSTANT 1024
+#define LIBXL_ROMSIZE_KB 256
 #define LIBXL_PV_EXTRA_MEMORY 1024
 #define LIBXL_HVM_EXTRA_MEMORY 2048
 #define LIBXL_MIN_DOM0_MEM (128*1024)
@@ -1023,6 +1024,12 @@ _hidden char * libxl__domain_pvcontrol_read(libxl__gc *gc,
 _hidden int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
                                           uint32_t domid, const char *cmd);
 
+/* Returns the amount of extra mem required to allocate roms or an libxl
+ * error code on error.
+ * The *d_config parameter is optional.
+ */
+_hidden int libxl__get_rom_memory_kb(libxl__gc *gc, uint32_t domid, libxl_domain_config *d_config);
+
 /* from xl_device */
 _hidden char *libxl__device_disk_string_of_backend(libxl_disk_backend backend);
 _hidden char *libxl__device_disk_string_of_format(libxl_disk_format format);
-- 
1.7.10.4

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

[Ian Campbell]

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.

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

[Stefano Stabellini]

On Tue, 25 Nov 2014, Ian Campbell wrote:
> 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.

Actually Wei suggested a better alternative: I could call
xc_domain_setmaxmem directly from QEMU. That makes much more sense.


> > 
> > ---
> > 
> > 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.
> 

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

[Ian Campbell]

On Tue, 2014-11-25 at 16:49 +0000, Stefano Stabellini wrote:
> On Tue, 25 Nov 2014, Ian Campbell wrote:
> > 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.
> 
> Actually Wei suggested a better alternative: I could call
> xc_domain_setmaxmem directly from QEMU. That makes much more sense.

xl mem-set would do it again, but not taking qemu's extras into account,
unless you communicate the overhead somehow...

> 
> 
> > > 
> > > ---
> > > 
> > > 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.
> > 

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

[Wei Liu]

On Tue, Nov 25, 2014 at 04:56:02PM +0000, Ian Campbell wrote:
> On Tue, 2014-11-25 at 16:49 +0000, Stefano Stabellini wrote:
> > On Tue, 25 Nov 2014, Ian Campbell wrote:
> > > 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.
> > 
> > Actually Wei suggested a better alternative: I could call
> > xc_domain_setmaxmem directly from QEMU. That makes much more sense.
> 
> xl mem-set would do it again, but not taking qemu's extras into account,
> unless you communicate the overhead somehow...
> 

Use a xenstore key? Like /vm/$UUID/XXX.

The key can be written by QEMU (because it knows the extra ram used by
all ROMs) and read by libxl.

(Haven't followed this closely, just my two cents)

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

[Stefano Stabellini]

On Tue, 25 Nov 2014, Ian Campbell wrote:
> On Tue, 2014-11-25 at 16:49 +0000, Stefano Stabellini wrote:
> > On Tue, 25 Nov 2014, Ian Campbell wrote:
> > > 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.
> > 
> > Actually Wei suggested a better alternative: I could call
> > xc_domain_setmaxmem directly from QEMU. That makes much more sense.
> 
> xl mem-set would do it again, but not taking qemu's extras into account,
> unless you communicate the overhead somehow...

We could start reading the current maxmem and add to it in
libxl_set_memory_target. Or we could write the maxmem to xenstore and
read it back again.  Given that the allocations are only done by QEMU at
initialization time, I don't think we need to worry about concurrency
here.


> > 
> > > > 
> > > > ---
> > > > 
> > > > 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.
> > > 
> 
> 

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

[Ian Campbell]

On Tue, 2014-11-25 at 17:05 +0000, Stefano Stabellini wrote:
> On Tue, 25 Nov 2014, Ian Campbell wrote:
> > On Tue, 2014-11-25 at 16:49 +0000, Stefano Stabellini wrote:
> > > On Tue, 25 Nov 2014, Ian Campbell wrote:
> > > > 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.
> > > 
> > > Actually Wei suggested a better alternative: I could call
> > > xc_domain_setmaxmem directly from QEMU. That makes much more sense.
> > 
> > xl mem-set would do it again, but not taking qemu's extras into account,
> > unless you communicate the overhead somehow...
> 
> We could start reading the current maxmem and add to it in
> libxl_set_memory_target. Or we could write the maxmem to xenstore and
> read it back again.  Given that the allocations are only done by QEMU at
> initialization time, I don't think we need to worry about concurrency
> here.

Might work, but it's a bit scary for 4.5, I would expect there to be
subtle knock on effects from this sort of thing :-/

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

[Stefano Stabellini]

On Wed, 26 Nov 2014, Ian Campbell wrote:
> On Tue, 2014-11-25 at 17:05 +0000, Stefano Stabellini wrote:
> > On Tue, 25 Nov 2014, Ian Campbell wrote:
> > > On Tue, 2014-11-25 at 16:49 +0000, Stefano Stabellini wrote:
> > > > On Tue, 25 Nov 2014, Ian Campbell wrote:
> > > > > 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.
> > > > 
> > > > Actually Wei suggested a better alternative: I could call
> > > > xc_domain_setmaxmem directly from QEMU. That makes much more sense.
> > > 
> > > xl mem-set would do it again, but not taking qemu's extras into account,
> > > unless you communicate the overhead somehow...
> > 
> > We could start reading the current maxmem and add to it in
> > libxl_set_memory_target. Or we could write the maxmem to xenstore and
> > read it back again.  Given that the allocations are only done by QEMU at
> > initialization time, I don't think we need to worry about concurrency
> > here.
> 
> Might work, but it's a bit scary for 4.5, I would expect there to be
> subtle knock on effects from this sort of thing :-/
 
Given that this is not a regression, we could wait for 4.6 to commit the
fix and then if it doesn't cause any unwanted side effects, we could
backport it?

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

[Ian Campbell]

On Wed, 2014-11-26 at 12:39 +0000, Stefano Stabellini wrote:
> On Wed, 26 Nov 2014, Ian Campbell wrote:
> > On Tue, 2014-11-25 at 17:05 +0000, Stefano Stabellini wrote:
> > > On Tue, 25 Nov 2014, Ian Campbell wrote:
> > > > On Tue, 2014-11-25 at 16:49 +0000, Stefano Stabellini wrote:
> > > > > On Tue, 25 Nov 2014, Ian Campbell wrote:
> > > > > > 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.
> > > > > 
> > > > > Actually Wei suggested a better alternative: I could call
> > > > > xc_domain_setmaxmem directly from QEMU. That makes much more sense.
> > > > 
> > > > xl mem-set would do it again, but not taking qemu's extras into account,
> > > > unless you communicate the overhead somehow...
> > > 
> > > We could start reading the current maxmem and add to it in
> > > libxl_set_memory_target. Or we could write the maxmem to xenstore and
> > > read it back again.  Given that the allocations are only done by QEMU at
> > > initialization time, I don't think we need to worry about concurrency
> > > here.
> > 
> > Might work, but it's a bit scary for 4.5, I would expect there to be
> > subtle knock on effects from this sort of thing :-/
>  
> Given that this is not a regression, we could wait for 4.6 to commit the
> fix and then if it doesn't cause any unwanted side effects, we could
> backport it?

That's not a bad plan. Release noting "you can't create more than 4
emulated NICs" doesn't seem so bad to me.

Ian.

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

[Konrad Rzeszutek Wilk]

On Wed, Nov 26, 2014 at 01:04:00PM +0000, Ian Campbell wrote:
> On Wed, 2014-11-26 at 12:39 +0000, Stefano Stabellini wrote:
> > On Wed, 26 Nov 2014, Ian Campbell wrote:
> > > On Tue, 2014-11-25 at 17:05 +0000, Stefano Stabellini wrote:
> > > > On Tue, 25 Nov 2014, Ian Campbell wrote:
> > > > > On Tue, 2014-11-25 at 16:49 +0000, Stefano Stabellini wrote:
> > > > > > On Tue, 25 Nov 2014, Ian Campbell wrote:
> > > > > > > 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.
> > > > > > 
> > > > > > Actually Wei suggested a better alternative: I could call
> > > > > > xc_domain_setmaxmem directly from QEMU. That makes much more sense.
> > > > > 
> > > > > xl mem-set would do it again, but not taking qemu's extras into account,
> > > > > unless you communicate the overhead somehow...
> > > > 
> > > > We could start reading the current maxmem and add to it in
> > > > libxl_set_memory_target. Or we could write the maxmem to xenstore and
> > > > read it back again.  Given that the allocations are only done by QEMU at
> > > > initialization time, I don't think we need to worry about concurrency
> > > > here.
> > > 
> > > Might work, but it's a bit scary for 4.5, I would expect there to be
> > > subtle knock on effects from this sort of thing :-/
> >  
> > Given that this is not a regression, we could wait for 4.6 to commit the
> > fix and then if it doesn't cause any unwanted side effects, we could
> > backport it?
> 
> That's not a bad plan. Release noting "you can't create more than 4
> emulated NICs" doesn't seem so bad to me.

<wipes out the sweat from his forehead>

4.6 it is then!
> 
> Ian.
> 

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

[Don Slutz]

On 11/26/14 09:40, Konrad Rzeszutek Wilk wrote:
> On Wed, Nov 26, 2014 at 01:04:00PM +0000, Ian Campbell wrote:
>> On Wed, 2014-11-26 at 12:39 +0000, Stefano Stabellini wrote:
>>> On Wed, 26 Nov 2014, Ian Campbell wrote:
>>>> On Tue, 2014-11-25 at 17:05 +0000, Stefano Stabellini wrote:
>>>>> On Tue, 25 Nov 2014, Ian Campbell wrote:
>>>>>> On Tue, 2014-11-25 at 16:49 +0000, Stefano Stabellini wrote:
>>>>>>> On Tue, 25 Nov 2014, Ian Campbell wrote:
>>>>>>>> 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.
>>>>>>> Actually Wei suggested a better alternative: I could call
>>>>>>> xc_domain_setmaxmem directly from QEMU. That makes much more sense.
>>>>>> xl mem-set would do it again, but not taking qemu's extras into account,
>>>>>> unless you communicate the overhead somehow...
>>>>> We could start reading the current maxmem and add to it in
>>>>> libxl_set_memory_target. Or we could write the maxmem to xenstore and
>>>>> read it back again.  Given that the allocations are only done by QEMU at
>>>>> initialization time, I don't think we need to worry about concurrency
>>>>> here.
>>>> Might work, but it's a bit scary for 4.5, I would expect there to be
>>>> subtle knock on effects from this sort of thing :-/
>>>   
>>> Given that this is not a regression, we could wait for 4.6 to commit the
>>> fix and then if it doesn't cause any unwanted side effects, we could
>>> backport it?
>> That's not a bad plan. Release noting "you can't create more than 4
>> emulated NICs" doesn't seem so bad to me.

The Release note needs to include that e1000 & rtl8139 have this 
restriction.
vmxnet3 does not (and cannot be used for PXE boot).

    -Don Slutz

> <wipes out the sweat from his forehead>
>
> 4.6 it is then!
>> Ian.
>>

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

[Ian Campbell]

On Tue, 2014-11-25 at 16:49 +0000, Stefano Stabellini wrote:
> Actually Wei suggested a better alternative: I could call
> xc_domain_setmaxmem directly from QEMU. That makes much more sense.

Do you have links to the patches for the final approach you decided to
take here? Or maybe just resend?

Either way, I'd really appreciate any hints about which patches *aren't*
relevant to this issue any more too, so I can forget about them.

Ian.

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

[Don Slutz]

On 11/25/14 07:43, 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 have seen that this is no longer planned for 4.5, but I do think that 
libxl will
still be changed for this.

so

>   int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t max_memkb)
>   {
>       GC_INIT(ctx);
>       char *mem, *endptr;
>       uint32_t memorykb;
>       char *dompath = libxl__xs_get_dompath(gc, domid);
> -    int rc = 1;
> +    int rc = 1, romsize;
>   
>       mem = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/memory/target", dompath));
>       if (!mem) {
> @@ -4550,11 +4577,18 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t max_memkb)
>           LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "memory_static_max must be greater than or or equal to memory_dynamic_max\n");
>           goto out;
>       }
> -    rc = xc_domain_setmaxmem(ctx->xch, domid, max_memkb + LIBXL_MAXMEM_CONSTANT);
> +    rc = libxl__get_rom_memory_kb(gc, domid, NULL);
> +    if (rc < 0)
> +        goto out;
> +    romsize = rc;
> +    rc = xc_domain_setmaxmem(ctx->xch, domid,
> +                             max_memkb + LIBXL_MAXMEM_CONSTANT
> +                             + romsize);

Keeping LIBXL_MAXMEM_CONSTANT is wrong.  Should be dropped.

>       if (rc != 0) {
>           LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
>                   "xc_domain_setmaxmem domid=%d memkb=%d failed "
> -                "rc=%d\n", domid, max_memkb + LIBXL_MAXMEM_CONSTANT, rc);
> +                "rc=%d\n", domid, max_memkb + LIBXL_MAXMEM_CONSTANT +
> +                romsize, rc);
>           goto out;
>       }
>   

And here.

> @@ -4683,7 +4717,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
>           int32_t target_memkb, int relative, int enforce)
>   {
>       GC_INIT(ctx);
> -    int rc = 1, abort_transaction = 0;
> +    int rc = 1, abort_transaction = 0, romsize;
>       uint32_t memorykb = 0, videoram = 0;
>       uint32_t current_target_memkb = 0, new_target_memkb = 0;
>       uint32_t current_max_memkb = 0;
> @@ -4769,12 +4803,19 @@ retry_transaction:
>   
>       if (enforce) {
>           memorykb = new_target_memkb;
> +        rc = libxl__get_rom_memory_kb(gc, domid, NULL);
> +        if (rc < 0) {
> +            abort_transaction = 1;
> +            goto out;
> +        }
> +        romsize = rc;
>           rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
> -                LIBXL_MAXMEM_CONSTANT);
> +                LIBXL_MAXMEM_CONSTANT + romsize);

And here.

>           if (rc != 0) {
>               LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
>                       "xc_domain_setmaxmem domid=%d memkb=%d failed "
> -                    "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT, rc);
> +                    "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT +
> +                    romsize, rc);
>               abort_transaction = 1;
>               goto out;
>           }
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 74ea84b..733f4c7 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -305,7 +305,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>       libxl_domain_build_info *const info = &d_config->b_info;
>       libxl_ctx *ctx = libxl__gc_owner(gc);
>       char *xs_domid, *con_domid;
> -    int rc;
> +    int rc, romsize;
>   
>       if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
>           LOG(ERROR, "Couldn't set max vcpu count");
> @@ -405,8 +405,12 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>           }
>       }
>   
> +	rc = libxl__get_rom_memory_kb(gc, domid, d_config);
> +	if (rc < 0)
> +		return rc;
> +	romsize = rc;
>       if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
> -        LIBXL_MAXMEM_CONSTANT) < 0) {
> +        LIBXL_MAXMEM_CONSTANT + romsize) < 0) {
>           LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't set max memory");
>           return ERROR_FAIL;
>       }
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 4361421..33826ea 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -90,6 +90,7 @@
>   #define LIBXL_XENCONSOLE_LIMIT 1048576
>   #define LIBXL_XENCONSOLE_PROTOCOL "vt100"
>   #define LIBXL_MAXMEM_CONSTANT 1024
> +#define LIBXL_ROMSIZE_KB 256
>   #define LIBXL_PV_EXTRA_MEMORY 1024
>   #define LIBXL_HVM_EXTRA_MEMORY 2048

This should be "romsize" + 1024 as far as I know.

>   #define LIBXL_MIN_DOM0_MEM (128*1024)
> @@ -1023,6 +1024,12 @@ _hidden char * libxl__domain_pvcontrol_read(libxl__gc *gc,
>   _hidden int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
>                                             uint32_t domid, const char *cmd);
>   
> +/* Returns the amount of extra mem required to allocate roms or an libxl
> + * error code on error.
> + * The *d_config parameter is optional.
> + */
> +_hidden int libxl__get_rom_memory_kb(libxl__gc *gc, uint32_t domid, libxl_domain_config *d_config);
> +
>   /* from xl_device */
>   _hidden char *libxl__device_disk_string_of_backend(libxl_disk_backend backend);
>   _hidden char *libxl__device_disk_string_of_format(libxl_disk_format format);

     -Don Slutz