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

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

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.