From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH for 4.5 v7 1/1] Add mmio_hole_size Date: Mon, 20 Oct 2014 18:01:05 -0400 Message-ID: <54458621.3080102@terremark.com> References: <1413204679-7345-1-git-send-email-dslutz@verizon.com> <1413204679-7345-2-git-send-email-dslutz@verizon.com> <1413814677.13796.10.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1413814677.13796.10.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , Don Slutz , Jan Beulich Cc: Boris Ostrovsky , Stefano Stabellini , Ian Jackson , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 >> + >> +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 >> >> #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 To: Don Slutz CC: xen-devel@lists.xen.org, Ian Campbell , Stefano Stabellini , Ian Jackson , Jan Beulich , Boris Ostrovsky ... > @@ -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. >