From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [RFC][PATCH 10/13] tools: extend XENMEM_set_memory_map Date: Fri, 15 May 2015 07:16:31 +0100 Message-ID: <5555AB5F020000780007A57F@mail.emea.novell.com> References: <1428657724-3498-1-git-send-email-tiejun.chen@intel.com> <1428657724-3498-11-git-send-email-tiejun.chen@intel.com> <5535206C0200007800073D05@mail.emea.novell.com> <5555608B.2060108@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5555608B.2060108@intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tiejun Chen Cc: tim@xen.org, kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org >>> On 15.05.15 at 04:57, wrote: > On 2015/4/20 21:51, Jan Beulich wrote: >>>>> On 10.04.15 at 11:22, wrote: >>> --- a/tools/libxl/libxl_dom.c >>> +++ b/tools/libxl/libxl_dom.c >>> @@ -787,6 +787,70 @@ out: >>> return rc; >>> } >>> >>> +static int libxl__domain_construct_memmap(libxl_ctx *ctx, >>> + libxl_domain_config *d_config, >>> + uint32_t domid, >>> + struct xc_hvm_build_args *args, >>> + int num_pcidevs, >>> + libxl_device_pci *pcidevs) >>> +{ >>> + unsigned int nr = 0, i; >>> + /* We always own at least one lowmem entry. */ >>> + unsigned int e820_entries = 1; >>> + uint64_t highmem_end = 0, highmem_size = args->mem_size - args->lowmem_size; >>> + struct e820entry *e820 = NULL; >>> + >>> + /* Add all rdm entries. */ >>> + e820_entries += d_config->num_rdms; >>> + >>> + /* If we should have a highmem range. */ >>> + if (highmem_size) >>> + { >>> + highmem_end = (1ull<<32) + highmem_size; >>> + e820_entries++; >>> + } >>> + >>> + e820 = malloc(sizeof(struct e820entry) * e820_entries); >>> + if (!e820) { >>> + return -1; >>> + } >>> + >>> + /* Low memory */ >>> + e820[nr].addr = 0x100000; >>> + e820[nr].size = args->lowmem_size - 0x100000; >>> + e820[nr].type = E820_RAM; >> >> If you really mean it to be this lax (not covering the low 1Mb), then >> you need to explain why in a comment (and the consuming side >> should also have a similar explanation then). >> > > Okay, here may need this, > > /* > > * Low RAM starts at least from 1M to make sure all standard regions > > * of the PC memory map, like BIOS, VGA memory-mapped I/O and vgabios, > > * have enough space. > */ > #define GUEST_LOW_MEM_START_DEFAULT 0x100000 But this only states a generic fact, but doesn't explain why you can lump together all the different things below 1Mb into a single E820 entry. >>> + nr++; >>> + >>> + /* RDM mapping */ >>> + for (i = 0; i < d_config->num_rdms; i++) { >>> + /* >>> + * We should drop this kind of rdm entry. >>> + */ >>> + if (d_config->rdms[i].flag == LIBXL_RDM_RESERVE_FLAG_INVALID) >>> + continue; >>> + >>> + e820[nr].addr = d_config->rdms[i].start; >>> + e820[nr].size = d_config->rdms[i].size; >>> + e820[nr].type = E820_RESERVED; >>> + nr++; >>> + } >> >> Is this guaranteed not to produce overlapping entries? >> > > Right, I would add this at the beginning, > > if (e820_entries >= E820MAX) { > LOG(ERROR, "Ooops! Too many entries in the memory map!\n"); > return -1; > } That would be a protection against too many entries, but not against overlapping ones. Jan