From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v3][PATCH 07/16] hvmloader/pci: skip reserved ranges Date: Wed, 17 Jun 2015 15:54:31 +0800 Message-ID: <558127B7.5060800@intel.com> References: <1433985325-16676-1-git-send-email-tiejun.chen@intel.com> <1433985325-16676-8-git-send-email-tiejun.chen@intel.com> <557A9000.600@intel.com> <557FEC6A.1080003@intel.com> <55800B21020000780008561E@mail.emea.novell.com> <55811D50.2070002@intel.com> <55813BA60200007800085D93@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: <55813BA60200007800085D93@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: "tim@xen.org" , Kevin Tian , "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 List-Id: xen-devel@lists.xenproject.org On 2015/6/17 15:19, Jan Beulich wrote: >>>> On 17.06.15 at 09:10, wrote: >> On 2015/6/16 17:40, Jan Beulich wrote: >>>>>> On 16.06.15 at 11:29, wrote: >>>> I'm trying to walk into this direction: >>>> >>>> /* >>>> * We'll skip all space overlapping with reserved memory later, >>>> * so we need to increase mmio_total to compensate them. >>>> */ >>>> for ( j = 0; j < memory_map.nr_map ; j++ ) >>>> { >>>> uint64_t conflict_size = 0; >>>> if ( memory_map.map[j].type != E820_RAM ) >>>> { >>>> reserved_start = memory_map.map[j].addr; >>>> reserved_size = memory_map.map[j].size; >>>> reserved_end = reserved_start + reserved_size; >>>> if ( check_overlap(pci_mem_start, pci_mem_end - pci_mem_start, >>>> reserved_start, reserved_size) ) >>>> { >>>> /* >>>> * Calculate how much mmio range conflict with >>>> * reserved device memory. >>>> */ >>>> conflict_size += reserved_size; >>>> >>>> /* >>>> * But we may need to subtract those sizes beyond the >>>> * pci memory, [pci_mem_start, pci_mem_end]. >>>> */ >>>> if ( reserved_start < pci_mem_start ) >>>> conflict_size -= (pci_mem_start - reserved_start); >>>> if ( reserved_end > pci_mem_end ) >>>> conflict_size -= (reserved_end - pci_mem_end); >>>> } >>>> } >>>> >>>> if ( conflict_size ) >>>> { >>>> uint64_t conflict_size = max_t( >>>> uint64_t, conflict_size, max_bar_sz); >>>> conflict_size &= ~(conflict_size - 1); >>>> mmio_total += conflict_size; >>>> } >>>> } >>> >>> This last thing goes in the right direction, but is complete overkill >>> when you have a small reserved region and a huge BAR. You >> >> Yeah, this may waste some spaces in this worst case but I this think >> this can guarantee our change don't impact on the original expectation, >> right? > > "Some space" may be multiple Gb (e.g. the frame buffer of a graphics Sure. > card), which is totally unacceptable. > But then I don't understand what's your way. How can we fit all pci devices just with "the smallest power-of-2 region enclosing the reserved device memory"? For example, the whole pci memory is sitting at [0xa0000000, 0xa2000000]. And there are two PCI devices, A and B. Note each device needs to be allocated with 0x1000000. So if without concerning RMRR, A. [0xa0000000,0xa1000000] B. [0xa1000000,0xa2000000] But if one RMRR resides at [0xa0f00000, 0xa1f00000] which obviously generate its own alignment with 0x1000000. So the pci memory is expended as [0xa0000000, 0xa3000000], right? Then actually the whole pci memory can be separated three segments like, #1. [0xa0000000, 0xa0f00000] #2. [0xa0f00000, 0xa1f00000] -> RMRR would occupy #3. [0xa1f00000, 0xa3000000] So just #3 can suffice to allocate but just for one device, right? If I'm wrong please correct me. >>> ought to work out the smallest power-of-2 region enclosing the >> >> Okay. I remember the smallest size of a given PCI I/O space is 8 bytes, >> and the smallest size of a PCI memory space is 16 bytes. So >> >> /* At least 16 bytes to align a PCI BAR size. */ >> uint64_t align = 16; >> >> reserved_start = memory_map.map[j].addr; >> reserved_size = memory_map.map[j].size; >> >> reserved_start = (reserved_star + align) & ~(align - 1); >> reserved_size = (reserved_size + align) & ~(align - 1); >> >> Is this correct? > > Simply aligning the region doesn't help afaict. You need to fit it > with the other MMIO allocations. I guess you're saying just those mmio allocations conflicting with RMRR? But we don't know these exact addresses until we finalize to allocate them, right? Thanks Tiejun > >>> reserved range (albeit there are tricky corner cases to consider). >>> >> >> Yeah, its a little tricky since RMRR always owns a fixed start address, >> so we can't reorder them with all pci bars. I just think at least we >> should provide a correct solution now, then further look into what can >> be optimized. So I think we'd better get conflict_size with >> max(conflict_size, max_bar_sz), right? > > As per above - no, this is not an option. > > Jan > > >