From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings Date: Thu, 28 Aug 2014 10:24:06 +0800 Message-ID: <53FE92C6.6050605@intel.com> References: <1409050980-21933-1-git-send-email-tiejun.chen@intel.com> <1409050980-21933-4-git-send-email-tiejun.chen@intel.com> <53FC774D.8000501@citrix.com> <53FC9BB1020000780002D986@mail.emea.novell.com> <53FD3669.3070705@intel.com> <53FD9C03020000780002DEFF@mail.emea.novell.com> <53FD86E9.4020205@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53FD86E9.4020205@intel.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 , ian.campbell@citrix.com Cc: kevin.tian@intel.com, stefano.stabellini@eu.citrix.com, Andrew Cooper , ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2014/8/27 15:21, Chen, Tiejun wrote: > On 2014/8/27 14:51, Jan Beulich wrote: >>>>> On 27.08.14 at 03:37, wrote: >>> On 2014/8/26 20:37, Jan Beulich wrote: >>>> For example, I had also asked you to adjust your patch titles, yet >>> >>> Are you sure? I recheck all e-mails you replied to me but I don't find >>> this comment. >> >> http://lists.xenproject.org/archives/html/xen-devel/2014-08/msg00925.html >> > > Seems in another e-mail thread, but I guess '(not just here)' means that > should be valid here as well. > >>>> they still come in the same bogus form (namely with colons rather >>>> than slashes as prefix separators - this ones should e.g. start with >>>> "xen/x86:", albeit I personally dislike the xen/ prefix and tend to >>>> strip it). >>> >>> Anyway, I think you'd like to change all titles as follows: >> >> Along those lines, albeit some of the prefixes continue to be >> overly long: >> >>> 1> xen/vtd/rmrr: export acpi_rmrr_units >>> 2> xen/vtd/rmrr: introduce acpi_rmrr_unit_entries >> >> In these two, the trailing rmrr is meaningless: rmrr is not really >> a sub-component, and the rest of the title already establishes >> enough context. > > Right. > > 1> xen/vtd: export acpi_rmrr_units > 2> xen/vtd: introduce acpi_rmrr_unit_entries > >> >>> 3> xen/x86: define a new hypercall to get RMRR mappings >> >> To avoid needless extra rounds, iirc the hypercall was requested >> to no longer be RMRR-centric. Hence the title shouldn't be either. >> >>> 4> tools/libxc: introduce hypercall for xc_reserved_device_memory_map >>> 5> tools/libxc: check if mmio BAR is out of RMRR mappings >> >> Similarly, this wouldn't be RMRR specific the either. > > 3> xen/x86: define a new hypercall to get reserved device memory map > >> >>> 6> hvm_info_table: introduce nr_reserved_device_memory_map >> >> Here the prefix is rather odd: Knowing most of the sub-component >> placement throughout the code base quite well, I can't really identify >> which sub-component this is about. Please remember that the >> primary purpose of these prefixes is to make it easy to identify >> roughly which area of the code base a change affects. Hence this >> should neither be too fine grained (like you seemed to be picking >> e.g. individual header file names as prefixes) nor too coarse grained. > > Agreed, so thanks for your guide. > >> >> Just take on for yourself the viewing angle a maintainer or potential >> reviewer would take: Is this an area I should be looking at or I am >> interested in? > > Hope this is fine, > > 6> tools/libxc: introduce nr_reserved_device_memory_map into hvm_info_table > >> >>> 7> xen/x86: support xc_reserved_device_memory_map in compat case >>> 8> tools/firmware/hvmloader: introduce hypercall for >>> xc_reserved_device_memory_map >>> 9> tools/firmware/hvmloader: check to reserve RMRR >>> mappings in e820 >> >> For these two just "hvmloader:" would seem to provide enough >> context. > > 8> hvmloader: check to reserve all device reserved memory in e820 > 9> hvmloader: introduce hypercall for xc_reserved_device_memory_map > > Thanks > Tiejun Jan, Andrew and Ian, I hope I already cover all comments based v5, 1> Refine patch titles from Jan 2> Improve one patch description from Ian 3> Remove 'static' from Andrew 4> Use i which is the actual number of entries written from Andrew 5> Move checking acpi_rmrr_unit_entries before the copy_from_guest() from Andrew. And I also posted those fixed code fragments inline as well. If you guys have no more comments, could I send a new series to review? Thanks Tiejun