From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ed White Subject: Re: [PATCH v2 07/12] x86/altp2m: add control of suppress_ve. Date: Mon, 06 Jul 2015 11:43:48 -0700 Message-ID: <559ACC64.5050705@intel.com> References: <1434999372-3688-1-git-send-email-edmund.h.white@intel.com> <1434999372-3688-8-git-send-email-edmund.h.white@intel.com> <558ADCEE0200007800088FF7@mail.emea.novell.com> <558AEEA8.1000305@intel.com> <558BD42502000078000895BD@mail.emea.novell.com> <558C2E05.3040300@intel.com> <558D079F0200007800089F82@mail.emea.novell.com> <558D7D88.1050100@intel.com> <559ABC64.4080503@intel.com> <559AC926.7010707@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <559AC926.7010707@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: Ravi Sahita , Wei Liu , Ian Jackson , Tim Deegan , "xen-devel@lists.xen.org" , Jan Beulich , Andrew Cooper , tlengyel@novetta.com, Daniel De Graaf List-Id: xen-devel@lists.xenproject.org On 07/06/2015 11:29 AM, George Dunlap wrote: > On 07/06/2015 06:35 PM, Ed White wrote: >> On 07/06/2015 10:12 AM, George Dunlap wrote: >>> On Fri, Jun 26, 2015 at 5:27 PM, Ed White wrote: >>>> On 06/25/2015 11:04 PM, Jan Beulich wrote: >>>>>>>> On 25.06.15 at 18:36, wrote: >>>>>> On 06/25/2015 01:12 AM, Jan Beulich wrote: >>>>>>>>>> On 24.06.15 at 19:53, wrote: >>>>>>>> On 06/24/2015 07:38 AM, Jan Beulich wrote: >>>>>>>>>>>> On 22.06.15 at 20:56, wrote: >>>>>>>>>> --- a/xen/include/asm-x86/p2m.h >>>>>>>>>> +++ b/xen/include/asm-x86/p2m.h >>>>>>>>>> @@ -237,6 +237,19 @@ struct p2m_domain { >>>>>>>>>> p2m_access_t *p2ma, >>>>>>>>>> p2m_query_t q, >>>>>>>>>> unsigned int *page_order); >>>>>>>>>> + int (*set_entry_full)(struct p2m_domain *p2m, >>>>>>>>>> + unsigned long gfn, >>>>>>>>>> + mfn_t mfn, unsigned int >>>>>>>> page_order, >>>>>>>>>> + p2m_type_t p2mt, >>>>>>>>>> + p2m_access_t p2ma, >>>>>>>>>> + unsigned int sve); >>>>>>>>>> + mfn_t (*get_entry_full)(struct p2m_domain *p2m, >>>>>>>>>> + unsigned long gfn, >>>>>>>>>> + p2m_type_t *p2mt, >>>>>>>>>> + p2m_access_t *p2ma, >>>>>>>>>> + p2m_query_t q, >>>>>>>>>> + unsigned int *page_order, >>>>>>>>>> + unsigned int *sve); >>>>>>>>> >>>>>>>>> I have to admit that I find the _full suffixes here pretty odd. Based >>>>>>>>> on the functionality, they should be _sve. But then it seems >>>>>>>>> questionable how they could be useful to the generic p2m layer >>>>>>>>> anyway, i.e. why there would need to be such hooks in the first >>>>>>>>> place. >>>>>>>> >>>>>>>> I did originally use _sve suffixes. I changed them because there >>>>>>>> may be some future case where these routines control some other >>>>>>>> EPTE bit too. I made them hooks because I thought calling ept... >>>>>>>> functions directly would be a layering violation. >>>>>>> >>>>>>> Indeed it would. But thinking about it more, I would suggest to >>>>>>> extend the existing accessors rather than adding new ones. >>>>>>> Just consider what would result when further such return values >>>>>>> are going to be needed in the future: I don't see us adding >>>>>>> _fuller, _fullest, etc variants. Perhaps just make the new output >>>>>>> an optional generic "flags" one. One might even consider folding >>>>>>> it with order, or even consolidate all the outputs into a single >>>>>>> structure. >>>>>> >>>>>> The new functions are called in 3 places only, so changing them >>>>>> later would have minimal impact. The existing functions are called >>>>>> in many, many places. I *really* don't want to go changing the >>>>>> amount of existing code that doing what you suggest would entail >>>>>> at this late stage. >>>>> >>>>> I continue to think differently (and I don't consider "at this late >>>>> stage" a particularly relevant argument), but the maintainer will >>>>> have the final say anyway - George? >>>>> >>>> >>>> The patch as it is now doesn't disturb (and risk breaking) any >>>> existing code. I'd much rather stick with that for 4.6, even if >>>> only on the condition that I have to change it later. If I do >>>> what you suggest, that sets me up to fail to get anything in >>>> 4.6. That may not matter to you, but it matters to me. >>> >>> Sorry, I've just gotten up to speed enough to figure out what the >>> question is about. >>> >>> For future reference: what has the highest risk of breaking existing >>> code is touching the codepath, not doing an almost entirely mechanical >>> change. From that perspective, you have changed all paths through >>> [gs]et_entry() already (on Intel boxes at least). I wouldn't have >>> considered a global search-and-replace where the defaults are always >>> the same (and propagation of the interface through the generic and AMD >>> function signatures) as a particularly invasive change -- at least, >>> not any more than the code you have here. >>> >>> It looks like the existing p2m->set_entry() function is only called in >>> 6 places -- 5 times in p2m.c and once in mem_sharing.c; and >>> p2m->get_entry() is called in about two dozen places, all in p2m.c >>> (and again one in mem_sharing.c). If you change the [gs]et_entry() >>> hooks, but have p2m_set_entry() pass in the default, it shouldn't be >>> that big of an impact (particularly as the get_entry() will just be >>> passing NULL). >>> >>> I do think that avoiding magic numbers is important, at least for the >>> default; for example: >>> >>> #define P2M_SUPPRESS_VE_DEFAULT (-1) >>> >>> Another option would be to make an enum with {default, clear, set}, >>> but that's probably overkill. >>> >> >> I certainly don't want to speak for Jan, but my reading of his >> comments suggests that wouldn't be enough to satisfy him. He >> seemed to me to object to the whole idea of adding something >> specifically to handle suppress_ve, and thought any change should >> offer a more general 'control extra (E)PTE bits' interface. > > I understood Jan's objection to be to adding two extra hooks ("But then > it seems questionable how they could be useful to the generic p2m layer > anyway, i.e. why there would need to be such hooks in the first > place."), instead of just adding an extra field to the existing > [gs]et_p2m_entry() ("But thinking about it more, I would suggest to > extend the existing accessors rather than adding new ones.") > > He does suggest the idea of making the interface generic, by for example > making it an extentable "flags" argument, or by changing the whole thing > to accept a pointer to a struct, rather than adding more and more > arguments that need to be set (and which, like p2m_access_t, almost > everybody just either uses the default or passes on what was passed to > them), add a pointer to a struct ("One might even consider folding it > with order, or even consolidate all the outputs into a single > structure.") But I think that may be a clean-up thing we do next round. > > The first quote above isn't 100% clear, so I can see why you might think > he meant not to expose SVE directly. > >> If the requirement is only to add control of suppress_ve, I honestly >> don't understand what is wrong with the way I have already done it. >> There is certainly precedent for adding extra p2m hook functions that >> are VMX-specific (look at the PML patch series), and I haven't >> changed lots of code that I have no way to test, which is one of >> the concerns I have about changing set/get everywhere. >> >> If the objection is to me wrapping the existing EPT set/get functions, >> I could add entirely separate functions that only manipulate >> suppress_ve. The reason I didn't is that I would need to duplicate >> a lot of the code in the existing functions. > > The objection isn't to the wrapping; the objection is to adding new > hooks that are *almost entirely identical* to the old hooks, but have > one extra parameter. > > The PML series added new hooks that were *completely new* in functionality. > >> I want to be clear: you are the maintainers, and in the end you have >> final say; however, I've been developing system software for a long >> time and I really don't understand why you think requiring a design >> that changes more source code for no functional effect is a good >> idea. > > If it were simply a matter of making a new function call (by adding _ to > the front or _internal to the end), then yeah, this wrapper scheme would > probably be better than going around changing all the entries that don't > use the extra value. But p2m->set_entry() is *already* the internal > function which is wrapped by p2m_set_entry(). > > Introducing yet another layer -- particularly in a hooked interface like > this -- just seems clunky. It's not the worst thing in the world; if I > thought this would be the difference between making it or not, I might > just say fix it later. But I don't think it will; and these little > things add up. > I don't want to change set/get everywhere, and Tim already made it clear that coupling suppress_ve with p2m_type_t is not acceptable. How can I provide an implementation that does not do either of the above but does allow access to suppress_ve in a way that is acceptable? Tell me and I will do it. Ed