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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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