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

[Don Slutz <dslutz@verizon.com>]

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@oracle.com>
To: 	Don Slutz <dslutz@verizon.com>
CC: 	xen-devel@lists.xen.org, Ian Campbell <ian.campbell@citrix.com>, 
Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Ian Jackson 
<ian.jackson@eu.citrix.com>, Jan Beulich <jbeulich@suse.com>, Boris 
Ostrovsky <boris.ostrovsky@oracle.com>


...

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