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: Wed, 29 Oct 2014 15:03:01 +0800 Message-ID: <54509125.2070106@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> <544F41A7.3080606@intel.com> <544F76550200007800042B84@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: <544F76550200007800042B84@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/28 17:56, Jan Beulich wrote: >>>> On 28.10.14 at 08:11, wrote: >> 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. > > But it still takes people reading you reply time to read that. Please > ask back only when you think an earlier reply was ambiguous. > >>>>>> + 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); > > Yes. Finally. > >>>>>> @@ -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(); > > Except that "unsigned int" would seem more consistent. Fixed. > >>>> 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. > > A side effect is an effect you don't primarily intend with your change > (or more generally, with any particular operation). In the case here, > a BAR that previously got a value assigned may not anymore with > your change in place. An acceptable effect of your change would be > if the value it gets assigned is now different, but not assigning a value > at all is not acceptable. As I understand that value just need to align with BAR and size. Then any range should be fine. Here I think its not necessary to consider any space restriction, i.e, some device may just access under 4G. > >>>>> 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? > > Almost. You'd have to use 0x80000000, or other logic would break. > >> But what test you want to see? Just boot? I don't see any strong error from boot. > > Yes, guest boot, but including proper inspection that what your > code does is really how it should be. > Okay, I will go into details. Thanks Tiejun > Jan >