Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/8/27 14:51, Jan Beulich wrote:
>>>> On 27.08.14 at 03:37, <tiejun.chen@intel.com> 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