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: Tue, 28 Oct 2014 15:11:35 +0800 Message-ID: <544F41A7.3080606@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> <544DF06F.5000106@intel.com> <544E24D70200007800042579@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: <544E24D70200007800042579@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/27 17:56, Jan Beulich wrote: >>>> On 27.10.14 at 08:12, wrote: >> 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; > > Please stop echoing back what was requested. I try to fix inline to make sure I'm addressing all comments inline properly. If you think this is correct please just ignore that. > >>>> + 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; >> } > > Is "a simple single return statement" ambiguous in any way? This > > static bool check_mmio_hole_conflict(uint64_t start, uint64_t memsize, > uint64_t mmio_start, uint64_t mmio_size) > { > return start + memsize > mmio_start && start < mmio_start + mmio_size; > } > > is how I think this should be. Thanks for your show. > >>>> +} >>>> + >>>> +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(); > > And (see also below) "unsigned int". It's bogus anyway to have the > function return the count by normal means by the actual array via a > global variable. I think you ought to switch to a consistent model. > >>>> + 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? > > Shifting a 32-bit quantity left still yields a 32-bit quantity, no matter > whether the result is then stored in a 64-bit variable. You need to > up-cast the left side of the shift first. Do you mean this? rdm_start = (uint64_t)rdm_map[j].start_pfn << PAGE_SHIFT; rdm_end = rdm_start + ((uint64_t)rdm_map[j].nr_pages << PAGE_SHIFT); > >>>> @@ -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 > > If only you checked for these negative values... May I can simplify these failed cases handle with within hvm_get_reserved_device_memory_map() like, if ( rc ) return 0; Because actually we don't need any negative return value again. So '0' is always fine. So here, unsigned long nr_rdm_entries = hvm_get_reserved_device_memory_map(); > >> 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. > > Resulting in the BAR not getting a value assigned afaict. Certainly > not what we want as a side effect of your changes. I don't understand what a side effect is. I just to try to make sure BAR space skip any conflict range but they are still in these resource ranges. > >>> 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. > > Below whose MMIO? The host's or the guest's? In the latter case, > just (in order to test your code) increase the range reserved for > MMIO enough to cover the RMRR range. In my platform, RMRR region: base_addr ab80a000 end_address ab81dfff RMRR region: base_addr ad000000 end_address af7fffff So I guess you hope I change this #define PCI_MEM_START 0xf0000000 to #define PCI_MEM_START 0xa0000000 right? But what test you want to see? Just boot? Thanks Tiejun