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: Sunday, July 19, 2015 11:18 PM
>
>>>> On 18.07.15 at 00:39, <ravi.sahita@xxxxxxxxx> wrote:
>>> 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).
>
>xmalloc() / xzalloc(). What other alternatives would you see?
>
>Jan

Ok - understood - as you must have seen, this change is not in our v6 - that is 
per our preface work plan -- we may not be able to get this change into the 
series proposed for 4.6.
Though I want to assure you the change can be made subsequently, to address 
your previous point, tonight I have prepared a delta patch for this change 
already but we need to test and that takes up a decent chunk of time. 
Are you ok if this mechanical change doesn't go into our 4.6 series? 

Thanks,
Ravi


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