From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v6][PATCH 3/7] tools/libxc: check if mmio BAR is out of reserved device memory maps Date: Fri, 12 Sep 2014 10:56:20 +0800 Message-ID: <541260D4.20301@intel.com> References: <1410328190-6372-1-git-send-email-tiejun.chen@intel.com> <1410328190-6372-4-git-send-email-tiejun.chen@intel.com> <5411DE130200007800034062@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5411DE130200007800034062@mail.emea.novell.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: Jan Beulich Cc: kevin.tian@intel.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2014/9/11 23:38, Jan Beulich wrote: >>>> On 10.09.14 at 07:49, wrote: >> +static int check_rdm_overlap(xc_interface *xch, uint64_t mmio_start, >> + uint64_t mmio_size) >> +{ >> + struct xen_mem_reserved_device_memory *map = NULL; >> + uint64_t rdm_start = 0, rdm_end = 0; >> + unsigned int i = 0; >> + int rc = 0; >> + /* Assume we have one entry if not enough we'll expand.*/ >> + uint32_t nr_entries = 1; >> + >> + /* We should check if mmio range is out of RDM mapping. */ >> + if ( (map = malloc(nr_entries * >> + sizeof(xen_mem_reserved_device_memory_t))) == NULL ) >> + { >> + PERROR("Could not allocate memory for map."); >> + return -1; >> + } >> + rc = xc_reserved_device_memory_map(xch, map, &nr_entries); >> + if ( rc < 0 ) >> + { >> + /* DRM doesn't exist. */ >> + if ( rc == -ENOENT ) >> + rc = 0; >> + else if ( rc == -ENOBUFS) > > I think you'd be better off handling both if() levels we're in here > with a single switch(). Okay. > >> + { >> + free(map); >> + /* Now we need more space to map all RDM mappings. */ >> + if ( (map = malloc(nr_entries * >> + sizeof(xen_mem_reserved_device_memory_t))) == NULL ) >> + { >> + PERROR("Could not allocate memory for map."); >> + return -1; >> + } >> + rc = xc_reserved_device_memory_map(xch, map, &nr_entries); >> + if ( rc < 0 ) >> + { >> + PERROR("Could not get DRM info on domain"); > > Please change to RDM here and elsewhere. Okay. > >> + free(map); >> + return rc; >> + } >> + } >> + else >> + PERROR("Could not get RDM info on domain"); > > Not returning here means looping for almost 4G iterations below. > >> + } >> + >> + for ( i = 0; i < rc; i++ ) >> + { >> + rdm_start = map[i].start_pfn << PAGE_SHIFT; >> + rdm_end = rdm_start + map[i].nr_pages * PAGE_SIZE; >> + if ( check_mmio_hole(rdm_start, map[i].nr_pages * PAGE_SIZE, > > First of all please be consistent: Either always use "<< PAGE_SHIFT" > or always use "* PAGE_SIZE". And then what you're doing here won't > do what you intend to in a 32-bit toolstack (due to the lack of casts). > Plus finally I think using XC_PAGE_* would be the better choice here > even if PAGE_* are #define-d as aliases thereof. > >> + mmio_start, mmio_size) ) > > Can you really use check_mmio_hole() here? I'm not certain that > its two one-offs are intentional (to allow for a gap between MMIO > and RAM), but these are certainly harmful to you. I don't understand what you mean here. But here we should check if RMRR is overlapping the whole MMIO, /* * 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; } > >> + { >> + PERROR("MMIO: [%lx]<->[%lx] overlap DRM [%lx]<->[%lx]\n", > > Please use a more conventional representation here, e.g. > > "MMIO [%lx,%lx) overlaps RDM [%lx,%lx)\n" Okay. I tried to address your comments but I assume its correct to use 'errno' directly as I'm asking in another email: @@ -54,6 +54,9 @@ #define VGA_HOLE_SIZE (0x20) +/* Record reserved device memory. */ +static struct xen_mem_reserved_device_memory *xmrdm = NULL; + 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) @@ -239,6 +242,70 @@ static int check_mmio_hole(uint64_t start, uint64_t memsize, return 1; } +/* + * Check whether there exists mmio overplap with the reserved device + * memory map + */ +static int check_rdm_overlap(xc_interface *xch, uint64_t mmio_start, + uint64_t mmio_size) +{ + uint64_t rdm_start = 0, rdm_end = 0; + unsigned int i = 0; + int rc = 0; + /* Assume we have one entry if not enough we'll expand.*/ + uint32_t nr_entries = 1; + + /* We should check if mmio range is out of RDM mapping. */ + if (!xmrdm) { + if ((xmrdm = malloc(nr_entries * + sizeof(xen_mem_reserved_device_memory_t))) == NULL) { + PERROR("Could not allocate memory for map."); + return -1; + } + + rc = xc_reserved_device_memory_map(xch, xmrdm, &nr_entries); + if (rc < 0 ) { + switch (errno) { + case ENOENT: /* RDM doesn't exist. */ + rc = 0; + break; + case ENOBUFS: /* Need more space */ + free(xmrdm); + /* Now we need more space to map all RDM mappings. */ + if ((xmrdm = malloc(nr_entries * + sizeof(xen_mem_reserved_device_memory_t))) == NULL) { + PERROR("Could not allocate memory for map."); + return -1; + } + rc = xc_reserved_device_memory_map(xch, xmrdm, &nr_entries); + if (rc) { + PERROR("Could not get RDM info on domain"); + free(xmrdm); + return rc; + } + break; + default: /* Failed to get RDM */ + PERROR("Could not get RDM info on domain"); + return rc; + } + } + } + + for (i = 0; i < rc; i++) { + rdm_start = xmrdm[i].start_pfn << XC_PAGE_SHIFT; + rdm_end = rdm_start + (xmrdm[i].nr_pages << XC_PAGE_SHIFT); + if (check_mmio_hole(rdm_start, (xmrdm[i].nr_pages << XC_PAGE_SHIFT), + mmio_start, mmio_size) ) { + PERROR("MMIO: [%lx,%lx) overlap RDM [%lx,%lx)\n", + mmio_start, (mmio_start + mmio_size), rdm_start, rdm_end); + free(xmrdm); + return -1; + } + } + + return rc; +} + static int setup_guest(xc_interface *xch, uint32_t dom, struct xc_hvm_build_args *args, char *image, unsigned long image_size) Thanks Tiejun