From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v7][RFC][PATCH 05/13] hvmloader/mmio: reconcile guest mmio with reserved device memory Date: Mon, 27 Oct 2014 15:12:47 +0800 Message-ID: <544DF06F.5000106@intel.com> References: <1414136077-18599-1-git-send-email-tiejun.chen@intel.com> <1414136077-18599-6-git-send-email-tiejun.chen@intel.com> <544A81610200007800041FFE@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: <544A81610200007800041FFE@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: yang.z.zhang@intel.com, kevin.tian@intel.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 2014/10/24 22:42, Jan Beulich wrote: >>>> On 24.10.14 at 09:34, wrote: >> --- a/tools/firmware/hvmloader/pci.c >> +++ b/tools/firmware/hvmloader/pci.c >> @@ -37,6 +37,44 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0; >> enum virtual_vga virtual_vga = VGA_none; >> unsigned long igd_opregion_pgbase = 0; >> >> +unsigned int need_skip_rmrr = 0; > > Static (and without initializer)? static unsigned int need_skip_rmrr; > >> + >> +/* >> + * Check whether there exists mmio hole in the specified memory range. >> + * Returns 1 if exists, else returns 0. >> + */ >> +static int check_mmio_hole_confliction(uint64_t start, uint64_t memsize, > > I don't think the word "confliction" exists. "conflict" please. s/check_mmio_hole_confliction/check_mmio_hole_conflict/g > >> + uint64_t mmio_start, uint64_t mmio_size) >> +{ >> + if ( start + memsize <= mmio_start || start >= mmio_start + mmio_size ) >> + return 0; >> + else >> + return 1; > > Make this a simple single return statement? static int check_mmio_hole_conflict(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; return 1; } > >> +} >> + >> +static int check_reserved_device_memory_map(uint64_t mmio_base, >> + uint64_t mmio_max) >> +{ >> + uint32_t i = 0; > > Pointless initializer. > >> + uint64_t rdm_start, rdm_end; >> + int nr_entries = -1; > > And again. > >> + >> + nr_entries = hvm_get_reserved_device_memory_map(); > > It's completely unclear why this can't be the variable's initializer. uint32_t i; uint64_t rdm_start, rdm_end; int nr_rdm_entries = hvm_get_reserved_device_memory_map(); > >> + >> + for ( i = 0; i < nr_entries; i++ ) >> + { >> + rdm_start = rdm_map[i].start_pfn << PAGE_SHIFT; >> + rdm_end = rdm_start + (rdm_map[i].nr_pages << PAGE_SHIFT); > > I'm pretty certain I pointed out before that you can't simply shift > these fields - you risk losing significant bits. I tried to go back looking into something but just found you were saying I shouldn't use PAGE_SHIFT and PAGE_SIZE at the same time. If I'm still missing could you show me what you expect? > >> + if ( check_mmio_hole_confliction(rdm_start, (rdm_end - rdm_start), > > Pointless parentheses. if ( check_mmio_hole_conflict(rdm_start, rdm_end - rdm_start, > >> + mmio_base, mmio_max - mmio_base) ) >> + { >> + need_skip_rmrr++; >> + } >> + } >> + >> + return nr_entries; >> +} >> + >> void pci_setup(void) >> { >> uint8_t is_64bar, using_64bar, bar64_relocate = 0; >> @@ -58,7 +96,9 @@ void pci_setup(void) >> uint32_t bar_reg; >> uint64_t bar_sz; >> } *bars = (struct bars *)scratch_start; >> - unsigned int i, nr_bars = 0; >> + unsigned int i, j, nr_bars = 0; >> + int nr_entries = 0; > > And another pointless initializer. Plus as a count of something this int nr_rdm_entries; > surely wants to be "unsigned int". Also I guess the variable name nr_rdm_entries should be literally unsigned int but this value always be set from hvm_get_reserved_device_memory_map(), nr_rdm_entries = hvm_get_reserved_device_memory_map() I hope that return value can be negative value in some failed case > is too generic - nr_rdm_entries perhaps? Okay, s/nr_entries/nr_rdm_entries/g > >> @@ -363,11 +411,29 @@ void pci_setup(void) >> bar_data &= ~PCI_BASE_ADDRESS_IO_MASK; >> } >> >> + reallocate_mmio: >> base = (resource->base + bar_sz - 1) & ~(uint64_t)(bar_sz - 1); >> bar_data |= (uint32_t)base; >> bar_data_upper = (uint32_t)(base >> 32); >> base += bar_sz; >> >> + if ( need_skip_rmrr ) >> + { >> + for ( j = 0; j < nr_entries; j++ ) >> + { >> + rdm_start = rdm_map[j].start_pfn << PAGE_SHIFT; >> + rdm_end = rdm_start + (rdm_map[j].nr_pages << PAGE_SHIFT); >> + if ( check_mmio_hole_confliction(rdm_start, >> + (rdm_end - rdm_start), >> + base, bar_sz) ) > > "base" was already updated by this point. Looks I should insert these code fragments between bar_data_upper = (uint32_t)(base >> 32); and base += bar_sz; So (See below) > >> + { >> + resource->base = rdm_end; >> + need_skip_rmrr--; >> + goto reallocate_mmio; > > If you ever get here, the earlier determination of whether the MMIO > hole is large enough may get invalidated. I.e. I'm afraid this approach > is still to simplistic. Also to way you do the retry, the resulting bar_data Here move 'reallocate_mmio' downward one line, and s/resource->base = rdm_end;/base = (rdm_end + bar_sz - 1) & ~(uint64_t)(bar_sz - 1); Then, ... base = (resource->base + bar_sz - 1) & ~(uint64_t)(bar_sz - 1); reallocate_mmio: bar_data |= (uint32_t)base; bar_data_upper = (uint32_t)(base >> 32); if ( need_skip_rmrr ) { for ( j = 0; j < nr_rdm_entries; j++ ) { rdm_start = rdm_map[j].start_pfn << PAGE_SHIFT; rdm_end = rdm_start + (rdm_map[j].nr_pages << PAGE_SHIFT); if ( check_mmio_hole_conflict(rdm_start, rdm_end - rdm_start, base, bar_sz) ) { base = (rdm_end + bar_sz - 1) & ~(uint64_t)(bar_sz - 1); need_skip_rmrr--; goto reallocate_mmio; } } } base += bar_sz; Additionally, actually there are some original codes just following my codes: if ( need_skip_rmrr ) { ... } base += bar_sz; if ( (base < resource->base) || (base > resource->max) ) { printf("pci dev %02x:%x bar %02x size "PRIllx": no space for " "resource!\n", devfn>>3, devfn&7, bar_reg, PRIllx_arg(bar_sz)); continue; } This can guarantee we don't overwhelm the previous mmio range. > and bar_data_upper will likely end up being garbage. > > Did you actually _test_ this code? Actually in my real case those RMRR ranges are always below MMIO. Thanks Tiejun