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 15:21, Chen, Tiejun wrote:
> 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

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