Re: [PATCH v2 07/12] x86/altp2m: add control of suppress_ve.

[Ed White <edmund.h.white@intel.com>]

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 <edmund.h.white@intel.com> wrote:
>>>> On 06/25/2015 11:04 PM, Jan Beulich wrote:
>>>>>>>> On 25.06.15 at 18:36, <edmund.h.white@intel.com> wrote:
>>>>>> On 06/25/2015 01:12 AM, Jan Beulich wrote:
>>>>>>>>>> On 24.06.15 at 19:53, <edmund.h.white@intel.com> wrote:
>>>>>>>> On 06/24/2015 07:38 AM, Jan Beulich wrote:
>>>>>>>>>>>> On 22.06.15 at 20:56, <edmund.h.white@intel.com> 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