From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sahita, Ravi" Subject: Re: [PATCH v3 05/13] x86/altp2m: basic data structures and support routines. Date: Thu, 16 Jul 2015 08:48:35 +0000 Message-ID: References: <1435774177-6345-1-git-send-email-edmund.h.white@intel.com> <1435774177-6345-6-git-send-email-edmund.h.white@intel.com> <559E936E020000780008ED56@mail.emea.novell.com> <55A38C75020000780008FF5C@mail.emea.novell.com> <55A4EA2702000078000907DC@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A4EA2702000078000907DC@mail.emea.novell.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Tim Deegan , "Sahita, Ravi" , Wei Liu , George Dunlap , Andrew Cooper , Ian Jackson , "White, Edmund H" , "xen-devel@lists.xen.org" , "tlengyel@novetta.com" , Daniel De Graaf List-Id: xen-devel@lists.xenproject.org >From: Jan Beulich [mailto:JBeulich@suse.com] >Sent: Tuesday, July 14, 2015 1:53 AM > >>>> On 14.07.15 at 02:01, wrote: >>>From: Jan Beulich [mailto:JBeulich@suse.com] >>>Sent: Monday, July 13, 2015 1:01 AM >>>>>> On 10.07.15 at 23:48, wrote: >>>>> From: Jan Beulich [mailto:JBeulich@suse.com] >>>>>Sent: Thursday, July 09, 2015 6:30 AM >>>>>>>> On 01.07.15 at 20:09, 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