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: 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:
>>>>>> +        altp2m_vcpu_reset(v);
>>>>>> +        vcpu_altp2m(v).p2midx = 0;
>>>>>> +        atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
>>>>>> +
>>>>>> +        ap2m_vcpu_update_eptp(v);
>>>>>
>>>>>We're in vendor independent code here - either the function is
>>>>>misnamed, or it shouldn't be called directly from here.
>>>>
>>>> Would it be reasonable to add if hap_enabled and cpu_has_vmx checks
>>>> like other code in this file that invokes ept specific ops?
>>>> Otherwise it seems ok that the function would be called from here
>>>> for p2m_altp2m interactions such as switching altp2m by id etc.
>>>> Open to any other suggestions from you, or we would like to leave it
>>>> as it is.
>>>
>>>Imo such should be abstracted out properly (if it's indeed
>>>EPT-specific), or
>> the
>>>function be renamed.
>>>
>>
>> Renaming may make sense - checking first - Would a name like
>> altp2m_vcpu_update_p2m() make sense?
>
>Sounds fine to me.
>

Thanks


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


>
>Jan


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