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

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

>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Thursday, July 16, 2015 2:02 AM
>
>>>> On 16.07.15 at 10:48, <ravi.sahita@xxxxxxxxx> wrote:
>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>Sent: Tuesday, July 14, 2015 1:53 AM
>>>>>> On 14.07.15 at 02:01, <ravi.sahita@xxxxxxxxx> wrote:
>>>>>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>>>Sent: Monday, July 13, 2015 1:01 AM
>>>>>>>> On 10.07.15 at 23:48, <ravi.sahita@xxxxxxxxx> wrote:
>>>>>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>>>>>Sent: Thursday, July 09, 2015 6:30 AM
>>>>>>>>>> On 01.07.15 at 20:09, <edmund.h.white@xxxxxxxxx> wrote:
>>>>>>>> @@ -294,6 +298,12 @@ struct arch_domain
>>>>>>>>      struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
>>>>>>>>      mm_lock_t nested_p2m_lock;
>>>>>>>>
>>>>>>>> +    /* altp2m: allow multiple copies of host p2m */
>>>>>>>> +    bool_t altp2m_active;
>>>>>>>> +    struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
>>>>>>>> +    mm_lock_t altp2m_lock;
>>>>>>>> +    uint64_t *altp2m_eptp;
>>>>>>>
>>>>>>>This is a non-insignificant increase of the structure size -
>>>>>>>perhaps all of these should hang off of struct arch_domain via a
>>>>>>>single, separately allocated pointer?
>>>>>>
>>>>>> Is this a nice-to-have - again we modelled after the nestedhvm
>>>>>> extensions to the struct.
>>>>>> This will affect a lot of our patch without really changing how
>>>>>> much memory is allocated.
>>>>>
>>>>>I understand that. To a certain point I can agree to limit changes
>>>>>to what is there at this stage. But you wanting to avoid addressing
>>>>>concerns basically everywhere it's not a bug overextends this. Just
>>>>>because the series was submitted late doesn't mean you should now
>>>>>expect us to give in on any controversy regarding aspects we would
>>>>>normally expect to be changed. This would basically encourage others
>>>>>to submit their stuff late too in the
>>>> future,
>>>>>hoping for relaxed review.
>>>>>
>>>>
>>>> Couple things - first, we have absorbed a lot of (good) feedback -
>>>> thanks for that.
>>>> Second, I don't think the series can be characterized as late
>>>> (feedback from others welcome).
>>>> V1 had almost the same structure and was submitted in January.
>>>
>>>Still we're at v3 only here, not v10 or beyond.
>>>
>>>> On this change - this will be a lot of effects on the code and we
>>>> would like to avoid this one.
>>>
>>>While this may be a lot of mechanical change, I don't this presenting
>>>any
>> major
>>>risk of breaking the code.
>>
>> On this one specific advice on how and where to implement such a
>> change would be great just so that we don't thrash on this change.
>
>I don't follow - what to do here was said quite explicitly (still visible in 
>the
>context above). I.e. I have no idea what additional advice you seek.

Ok that's fine - sorry if this was unclear - I was seeking if you had some 
specific feedback on how to allocate and manage the dynamic altp2m struct etc 
(if you had an opinion would be good to hear that). Will treat this as a 
Category 2 with lower priority than some of the other Category 2's in the other 
reviews (theres one from you and one from George). 

Thanks,
Ravi

>
>Jan


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