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

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

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

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

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

> +#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?

> +            /*
> +             * 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? 

> +             */
> +            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?

Ian.