[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH for 4.5 v7 1/1] Add mmio_hole_size

On 10/20/14 10:17, Ian Campbell wrote:
On Mon, 2014-10-13 at 08:51 -0400, Don Slutz wrote:
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 8bba21c..10e995a 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1111,6 +1111,21 @@ wallclock (i.e., real) time.
=back +=head3 Memory layout
+=over 4
+=item B<mmio_hole_size=BYTES>
+Specifies the size the MMIO hole below 4GiB will be.  Only valid for
+device_model_version = "qemu-xen".
+Cannot be smaller than 256MiB. Cannot be larger than 3840MiB.
+Known good large value is 3GiB (3221225472).
I think the literal values "3GiB"/"3840MiB" etc won't work, will they?


Is there any need to be able to specify this size at byte granularity?
All of the other xl options use MBYTES as there units, which is much
easier for people to deal with (e.g. 3072 vs 3221225272).

I do not have this need.  Will adjust to MBYTES

@@ -696,10 +698,28 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
              /* Switching here to the machine "pc" which does not add
               * the xen-platform device instead of the default "xenfv" machine.
-            flexarray_append(dm_args, "pc,accel=xen");
+            machinearg = libxl__sprintf(gc, "pc,accel=xen");
          } else {
-            flexarray_append(dm_args, "xenfv");
+            machinearg = libxl__sprintf(gc, "xenfv");
+        if (b_info->u.hvm.mmio_hole_size) {
+            uint64_t max_ram_below_4g = (1ULL << 32) -
+                b_info->u.hvm.mmio_hole_size;
+            if (max_ram_below_4g > HVM_BELOW_4G_MMIO_START) {
+                LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
+                           "max-ram-below-4g=%"PRIu64
+                           " (mmio_hole_size=%"PRIu64
+                           ") too big > %u ignored.\n",
Using the LOG macro would indent this code less and allow you to wrap in
a more sensible/readable way. Do you really need to print both
max-ram-below-4g and mmio_hole_size given that one is derived trivially
from the other.

Yes, I can switch to "LOG(WARN". I can drop the extra info (note: via xl you
should not get this message, it would be logged else where.)

diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index bbb03e2..2a71e9a 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -391,6 +391,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                         ("timeoffset",       string),
                                         ("hpet",             libxl_defbool),
                                         ("vpt_align",        libxl_defbool),
+                                       ("mmio_hole_size",   uint64),
Please make this a MemKB at the libxl interface level and convert
internally to whatever hvmloader expects. It seems that a more
conventional name would also have a _memkb suffix. Perhaps
mmio_hole_memkb? (size seems to be implicit to me).

Ok, Jan Beulich had proposed the name with the _size (on 27 Jun 2014), but he
did also say "(whether the xl config option should also get renamed I'm not
sure - the xl maintainers will know )", (so I am taking this as ok from Jan) I will
rename to mmio_hole_memkb here.

+#include <xen/hvm/e820.h>
#include "libxl.h"
  #include "libxl_utils.h"
@@ -1111,6 +1112,21 @@ static void parse_config_data(const char *config_source,
+ if (!xlu_cfg_get_long(config, "mmio_hole_size", &l, 0)) {
+            b_info->u.hvm.mmio_hole_size = (uint64_t) l;
This cast isn't needed, is it?

Nope.  Will drop.

+            /*
+             * With HVM_BELOW_4G_RAM_END == 0xF0000000, mmio_hole_size
+             * must be >= 256 MiB and <= 3840 MiB.
Isn't this just restating the if condition in a way which is liable to
get out of sync if the values of HVM_BELOW_4G_* ever changes?

Well, Konrad asked for this:

Subject:        Re: [Xen-devel] [PATCH for 4.5 v6 1/1] Add mmio_hole_size
Date:   Tue, 7 Oct 2014 10:08:13 -0400
From:   Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
To:     Don Slutz <dslutz@xxxxxxxxxxx>
CC: xen-devel@xxxxxxxxxxxxx, Ian Campbell <ian.campbell@xxxxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>


@@ -1111,6 +1112,17 @@ static void parse_config_data(const char *config_source,

+        if (!xlu_cfg_get_long(config, "mmio_hole_size", &l, 0)) {
+            b_info->u.hvm.mmio_hole_size = (uint64_t) l;
+            if (b_info->u.hvm.mmio_hole_size < HVM_BELOW_4G_MMIO_LENGTH ||

Oh boy, this check for 256MB is surely obfuscated by the macros.

Could you include a comment saying that HVM_BELOW_4G_MMIO_LENGTH resolves
to 256MB please?

+                b_info->u.hvm.mmio_hole_size > HVM_BELOW_4G_MMIO_START) {
+                fprintf(stderr, "ERROR: invalid value 0x%"PRIx64" (%"PRIu64
+                        ") for \"mmio_hole_size\"\n",


So I do not know which way to go here.

+             */
+            if (b_info->u.hvm.mmio_hole_size < HVM_BELOW_4G_MMIO_LENGTH ||
+                b_info->u.hvm.mmio_hole_size > HVM_BELOW_4G_MMIO_START) {
+                fprintf(stderr, "ERROR: invalid value 0x%"PRIx64" (%"PRIu64
+                        ") for \"mmio_hole_size\"\n",
+                        b_info->u.hvm.mmio_hole_size,
+                        b_info->u.hvm.mmio_hole_size);
Can we not trust people to be able to convert to/from hex if they need

Sure, with it in MBYTES just decimal should be enough.

   -Don Slutz


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.