From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v3][PATCH 13/16] tools/libxl: detect and avoid conflicts with RDM Date: Mon, 15 Jun 2015 09:50:49 +0800 Message-ID: <557E2F79.7050502@intel.com> References: <1433985325-16676-1-git-send-email-tiejun.chen@intel.com> <1433985325-16676-14-git-send-email-tiejun.chen@intel.com> <20150612163944.GZ14606@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150612163944.GZ14606@zion.uk.xensource.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: Wei Liu Cc: kevin.tian@intel.com, ian.campbell@citrix.com, andrew.cooper3@citrix.com, tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, jbeulich@suse.com, yang.z.zhang@intel.com, Ian.Jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 2015/6/13 0:39, Wei Liu wrote: > On Thu, Jun 11, 2015 at 09:15:22AM +0800, Tiejun Chen wrote: > [...] >> +++ b/tools/libxc/xc_hvm_build_x86.c >> @@ -21,6 +21,7 @@ >> #include >> #include >> #include >> +#include >> >> #include "xg_private.h" >> #include "xc_private.h" >> @@ -270,7 +271,7 @@ static int setup_guest(xc_interface *xch, >> >> elf_parse_binary(&elf); >> v_start = 0; >> - v_end = args->mem_size; >> + v_end = args->lowmem_end; > > Why is this needed? This was left to handle something inside modules_init(). But I think this change can be removed now. >> >> if ( nr_pages > target_pages ) >> memflags |= XENMEMF_populate_on_demand; >> @@ -754,6 +755,8 @@ int xc_hvm_build_target_mem(xc_interface *xch, >> args.mem_size = (uint64_t)memsize << 20; >> args.mem_target = (uint64_t)target << 20; >> args.image_file_name = image_name; > [...] >> >> +static struct xen_reserved_device_memory >> +*xc_device_get_rdm(libxl__gc *gc, >> + uint32_t flag, >> + uint16_t seg, >> + uint8_t bus, >> + uint8_t devfn, >> + unsigned int *nr_entries) >> +{ >> + struct xen_reserved_device_memory *xrdm; >> + int rc; >> + >> + rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn, >> + NULL, nr_entries); > > xc_reserved_device_memory_map dereferences nr_entries. You need to make > sure there is no garbage value in nr_entries. I.e. you need to > initialise nr_entries to 0 before passing it in. Sure, so what about this? /* * We really can't presume how many entries we can get in advance. */ if (*nr_entries) *nr_entries = 0; > >> + assert(rc <= 0); >> + /* "0" means we have no any rdm entry. */ >> + if (!rc) >> + goto out; >> + >> + if (errno == ENOBUFS) { >> + xrdm = malloc(*nr_entries * sizeof(xen_reserved_device_memory_t)); >> + if (!xrdm) { >> + LOG(ERROR, "Could not allocate RDM buffer!\n"); >> + goto out; >> + } >> + rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn, >> + xrdm, nr_entries); >> + if (rc) { >> + LOG(ERROR, "Could not get reserved device memory maps.\n"); >> + *nr_entries = 0; >> + free(xrdm); >> + xrdm = NULL; >> + } >> + } else >> + LOG(ERROR, "Could not get reserved device memory maps.\n"); >> + >> + out: >> + return xrdm; >> +} >> + >> +/* >> + * Check whether there exists rdm hole in the specified memory range. >> + * Returns true if exists, else returns false. >> + */ >> +static bool overlaps_rdm(uint64_t start, uint64_t memsize, >> + uint64_t rdm_start, uint64_t rdm_size) >> +{ >> + return (start + memsize > rdm_start) && (start < rdm_start + rdm_size); >> +} >> + >> +/* >> + * Check reported RDM regions and handle potential gfn conflicts according >> + * to user preferred policy. >> + * >> + * RMRR can reside in address space beyond 4G theoretically, but we never >> + * see this in real world. So in order to avoid breaking highmem layout >> + * we don't solve highmem conflict. Note this means highmem rmrr could still >> + * be supported if no conflict. >> + * >> + * But in the case of lowmem, RMRR probably scatter the whole RAM space. >> + * Especially multiple RMRR entries would worsen this to lead a complicated >> + * memory layout. And then its hard to extend hvm_info_table{} to work >> + * hvmloader out. So here we're trying to figure out a simple solution to >> + * avoid breaking existing layout. So when a conflict occurs, >> + * >> + * #1. Above a predefined boundary (default 2G) >> + * - Move lowmem_end below reserved region to solve conflict; >> + * >> + * #2. Below a predefined boundary (default 2G) >> + * - Check strict/relaxed policy. >> + * "strict" policy leads to fail libxl. Note when both policies >> + * are specified on a given region, 'strict' is always preferred. >> + * "relaxed" policy issue a warning message and also mask this entry >> + * INVALID to indicate we shouldn't expose this entry to hvmloader. >> + */ > > This looks sensible. Thanks. > >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c >> index 4364ba4..85d74fd 100644 >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c >> @@ -1374,6 +1374,9 @@ static void parse_config_data(const char *config_source, >> if (!xlu_cfg_get_long (config, "videoram", &l, 0)) >> b_info->video_memkb = l * 1024; >> >> + if (!xlu_cfg_get_long (config, "rdm_mem_boundary", &l, 0)) >> + b_info->rdm_mem_boundary_memkb = l * 1024; >> + > > Maybe you need to rearrange this patch series a bit more. The toolstack > side patches have mixed libxc, libxl and xl changes which is a bit hard > for me to tell what is needed by what. We can discuss this if you have > questions. Okay, just let me try firstly. Thanks Tiejun > > Wei. > >> if (!xlu_cfg_get_long(config, "max_event_channels", &l, 0)) >> b_info->event_channels = l; >> >> -- >> 1.9.1 >