Re: [Xen-devel] [RFC][PATCH 10/13] tools: extend XENMEM_set_memory_map

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Apr 14, 2015 at 08:42:39AM +0800, Chen, Tiejun wrote:
> On 2015/4/13 19:02, Wei Liu wrote:
> >On Mon, Apr 13, 2015 at 10:09:51AM +0800, Chen, Tiejun wrote:
> >[...]
> >>>Hardcoded value?
> >>
> >>Yes. Actually, we intend to use this to present that lowmem entry,
> >>
> >>tools/firmware/hvmloader/e820.c:
> >>
> >>     /* Low RAM goes here. Reserve space for special pages. */
> >>     ...
> >>     e820[nr].addr = 0x100000;
> >>
> >
> >I don't like the idea of having two hardcoded values in different
> 
> Just one place since based on our logic, hvmloader doesn't have this setting
> anymore and actually it really grab that info from here. Please refer to
> patch #13.
> 
> >locations. Please put this value into a header file and reference it
> >here and in hvmloader.
> 
> Anyway, I'd like to define this here directly since no one consumes this
> again.
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 5134b33..af747e6 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -787,6 +787,7 @@ out:
>      return rc;
>  }
> 
> +#define GUEST_LOW_MEM_START_DEFAULT 0x100000
>  static int libxl__domain_construct_memmap(libxl__gc *gc,
>                                            libxl_domain_config *d_config,
>                                            uint32_t domid,
> @@ -812,8 +813,8 @@ static int libxl__domain_construct_memmap(libxl__gc *gc,
>      e820 = libxl__malloc(gc, sizeof(struct e820entry) * e820_entries);
> 
>      /* Low memory */
> -    e820[nr].addr = 0x100000;
> -    e820[nr].size = args->lowmem_size - 0x100000;
> +    e820[nr].addr = GUEST_LOW_MEM_START_DEFAULT;
> +    e820[nr].size = args->lowmem_size - GUEST_LOW_MEM_START_DEFAULT;
>      e820[nr].type = E820_RAM;
>      nr++;
> 
> Is this fine to you?
> 

Having a #define without explaining why it is chosen is still fragile.
As Jan pointed out this needs more explanation.

Wei.

> Thanks
> Tiejun

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel