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

[Don Slutz <dslutz@xxxxxxxxxxx>]

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

> 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,
> >               exit(-ERROR_FAIL);
> >           }
> >   
> > +        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,
>              exit(-ERROR_FAIL);
>          }
>
> +        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
> to?
Sure, with it in MBYTES just decimal should be enough.

   -Don Slutz

> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel