From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <Ian.Campbell@citrix.com>
Subject: Re: [PATCH for 4.5 v7 1/1] Add mmio_hole_size
Date: Tue, 21 Oct 2014 08:52:33 +0100
Message-ID: <1413877953.20604.19.camel@citrix.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> <54458621.3080102@terremark.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <54458621.3080102@terremark.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Don Slutz <dslutz@verizon.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, xen-devel@lists.xen.org, Jan Beulich <jbeulich@suse.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>
List-Id: xen-devel@lists.xenproject.org

On Mon, 2014-10-20 at 18:01 -0400, Don Slutz wrote:


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

I'm not sure why the fact that HVM_BELOW_4G_MMIO_LENGTH happens to be
256M right now should be of interest to the casual reader of this code.
They can always lookup the #define.

Putting the value of a define next to every use of it somewhat defeats
the purpose of having a define.

Ian.