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

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

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