Re: [Xen-devel] [PATCH v6 05/15] x86/altp2m: basic data structures and support routines.

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

>From: George Dunlap [mailto:george.dunlap@xxxxxxxxxx]
>Sent: Tuesday, July 21, 2015 5:47 AM
>
>On 07/21/2015 11:13 AM, Jan Beulich wrote:
>>>>> On 21.07.15 at 01:58, <edmund.h.white@xxxxxxxxx> wrote:
>>> Add the basic data structures needed to support alternate p2m's and
>>> the functions to initialise them and tear them down.
>>>
>>> Although Intel hardware can handle 512 EPTP's per hardware thread
>>> concurrently, only 10 per domain are supported in this patch for
>>> performance reasons.
>>>
>>> The iterator in hap_enable() does need to handle 512, so that is now
>>> uint16_t.
>>
>> Sigh - this one is still here (and the respective code unchanged).
>> I'm not going to NAK the patch just because of this, but it really
>> looks like you aren't trying to address comments even when they're
>> trivial (and quick) to carry out and testing of the change comes as a
>> side effect of you needing to test all the other changes as well.
>>
>>> --- a/xen/arch/x86/hvm/Makefile
>>> +++ b/xen/arch/x86/hvm/Makefile
>>> @@ -1,6 +1,7 @@
>>>  subdir-y += svm
>>>  subdir-y += vmx
>>>
>>> +obj-y += altp2m.o
>>
>> Wasn't the outcome of the earlier discussion to put this in x86/mm, or
>> possibly not even introduce a new file?
>
>That was my recommendation [1] in response to v5, but there was no
>response.  The mail seems to have been seen, however, since Andrew's
>Reviewed-by was dropped.  (Perhaps they didn't notice the additional
>comment further down.)
>
>[1] marc.info/?i=<55A53159.4010703@xxxxxxxxxxxxx>
>

We got these and hopefully all the straightforward ones in the next rev.

>> Overall the situation didn't really change from v5 - the code from a
>> pure functionality pov looks okay, but I don't see myself giving in on
>> all the "minor" issues the patch introduces. If some were left
>> adjusting of which really takes time to or risks breaking the code,
>> I'd (reluctantly) give my ack, but not this way, I'm afraid.
>
>This is a bit puzzling, and somewhat frustrating too -- to have carefully 
>combed
>through past versions, seen the comments and discussion, and then carefully
>combed through this one to find that nearly none of them have been
>addressed, even minor ones.

Sorry we did the ABI and some minor ones first in v6, the next rev should have 
these and patch 10 sorted out.
Thanks to you both for the reviews.

Ravi

>
> -George


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