[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxx> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.