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: Wed, 27 Aug 2014 15:21:13 +0800 Message-ID: <53FD86E9.4020205@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53FD9C03020000780002DEFF@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: kevin.tian@intel.com, ian.campbell@citrix.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 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