From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v8][PATCH 06/17] tools/libxc: check if modules space is overlapping with reserved device memory Date: Mon, 08 Dec 2014 15:49:48 +0800 Message-ID: <5485581C.1060809@intel.com> References: <1417425875-9634-1-git-send-email-tiejun.chen@intel.com> <1417425875-9634-7-git-send-email-tiejun.chen@intel.com> <20141202195506.GF357@laptop.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20141202195506.GF357@laptop.dumpdata.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: Konrad Rzeszutek Wilk Cc: kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, tim@xen.org, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, jbeulich@suse.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2014/12/3 3:55, Konrad Rzeszutek Wilk wrote: > On Mon, Dec 01, 2014 at 05:24:24PM +0800, Tiejun Chen wrote: >> In case of reserved device memory overlapping with ram, it also probably > > s/also// Fixed. >> overlap with modules space so we need to check these reserved device > s/overlap/overlaps/ Fixed. > > What is 'modules space'? Please see modules_init(), and looks it includs acpi and smbios currently. > >> memory as well. > > s/reserved device memory/E820_RSV/ ? I don't find we have such a definition. > >> >> Signed-off-by: Tiejun Chen >> --- >> tools/libxc/xc_hvm_build_x86.c | 94 +++++++++++++++++++++++++++++++++++------- >> 1 file changed, 79 insertions(+), 15 deletions(-) >> >> diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c >> index c81a25b..ddcf06d 100644 >> --- a/tools/libxc/xc_hvm_build_x86.c >> +++ b/tools/libxc/xc_hvm_build_x86.c >> @@ -54,9 +54,82 @@ >> >> #define VGA_HOLE_SIZE (0x20) >> >> +/* >> + * Check whether there exists mmio hole in the specified memory range. >> + * Returns 1 if exists, else returns 0. >> + */ >> +static int check_mmio_hole(uint64_t start, uint64_t memsize, >> + uint64_t mmio_start, uint64_t mmio_size) >> +{ >> + if ( start + memsize <= mmio_start || start >= mmio_start + mmio_size ) >> + return 0; >> + else >> + return 1; >> +} >> + >> +/* Getting all reserved device memory map info. */ >> +static struct xen_reserved_device_memory >> +*xc_get_reserved_device_memory_map(xc_interface *xch, unsigned int nr_entries, >> + uint32_t dom) >> +{ >> + struct xen_reserved_device_memory *xrdm = NULL; >> + int rc = xc_reserved_device_memory_map(xch, dom, xrdm, &nr_entries); >> + >> + if ( rc < 0 ) >> + { >> + if ( errno == ENOBUFS ) >> + { >> + if ( (xrdm = malloc(nr_entries * >> + sizeof(xen_reserved_device_memory_t))) == NULL ) >> + { >> + PERROR("Could not allocate memory."); >> + return 0; >> + } >> + rc = xc_reserved_device_memory_map(xch, dom, xrdm, &nr_entries); >> + if ( rc ) >> + { >> + PERROR("Could not get reserved device memory maps."); >> + free(xrdm); >> + return 0; > > Uhhh, is that the right error to return? > > Don't you mean ERR_PTR logic? Or 'return NULL' ? OOPS, return NULL. > > >> + } >> + } >> + else >> + PERROR("Could not get reserved device memory maps."); >> + } >> + >> + return xrdm; >> +} >> + >> +static int xc_check_modules_space(xc_interface *xch, uint64_t *mstart_out, >> + uint64_t *mend_out, uint32_t dom) >> +{ >> + unsigned int i = 0, nr_entries = 0; >> + uint64_t rdm_start = 0, rdm_end = 0; >> + struct xen_reserved_device_memory *rdm_map = >> + xc_get_reserved_device_memory_map(xch, nr_entries, dom); >> + > > You need to check whether 'rdm_map' is NULL. You're right. Actually nr_entries is always 0 if rdm_map is NULL with my original design. But now this should be checked as you mentioned, so + if ( !rdm_map ) + return 0; + > >> + for ( i = 0; i < nr_entries; i++ ) >> + { >> + rdm_start = (uint64_t)rdm_map[i].start_pfn << XC_PAGE_SHIFT; >> + rdm_end = rdm_start + ((uint64_t)rdm_map[i].nr_pages << XC_PAGE_SHIFT); >> + >> + /* Just use check_mmio_hole() to check modules ranges. */ >> + if ( check_mmio_hole(rdm_start, >> + rdm_end - rdm_start, >> + *mstart_out, *mend_out) ) >> + return -1; >> + } >> + >> + free(rdm_map); >> + >> + return 0; >> +} >> + >> static int modules_init(struct xc_hvm_build_args *args, >> uint64_t vend, struct elf_binary *elf, >> - uint64_t *mstart_out, uint64_t *mend_out) >> + uint64_t *mstart_out, uint64_t *mend_out, >> + xc_interface *xch, >> + uint32_t dom) >> { >> #define MODULE_ALIGN 1UL << 7 >> #define MB_ALIGN 1UL << 20 >> @@ -80,6 +153,10 @@ static int modules_init(struct xc_hvm_build_args *args, >> if ( *mend_out > vend ) >> return -1; >> >> + /* Is it overlapping with reserved device memory? */ >> + if ( xc_check_modules_space(xch, mstart_out, mend_out, dom) ) >> + return -1; >> + >> if ( args->acpi_module.length != 0 ) >> args->acpi_module.guest_addr_out = *mstart_out; >> if ( args->smbios_module.length != 0 ) >> @@ -226,19 +303,6 @@ static int loadmodules(xc_interface *xch, >> return rc; >> } >> >> -/* >> - * Check whether there exists mmio hole in the specified memory range. >> - * Returns 1 if exists, else returns 0. >> - */ >> -static int check_mmio_hole(uint64_t start, uint64_t memsize, >> - uint64_t mmio_start, uint64_t mmio_size) >> -{ >> - if ( start + memsize <= mmio_start || start >= mmio_start + mmio_size ) >> - return 0; >> - else >> - return 1; >> -} >> - > > This movement of 'check_mmio_hole' needs to be a seperate patch. Okay. Thanks Tiejun > >> static int setup_guest(xc_interface *xch, >> uint32_t dom, struct xc_hvm_build_args *args, >> char *image, unsigned long image_size) >> @@ -282,7 +346,7 @@ static int setup_guest(xc_interface *xch, >> goto error_out; >> } >> >> - if ( modules_init(args, v_end, &elf, &m_start, &m_end) != 0 ) >> + if ( modules_init(args, v_end, &elf, &m_start, &m_end, xch, dom) != 0 ) >> { >> ERROR("Insufficient space to load modules."); >> goto error_out; >> -- >> 1.9.1 >> >