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

[Ed White <edmund.h.white@xxxxxxxxx>]

On 07/06/2015 10:12 AM, George Dunlap wrote:
> On Fri, Jun 26, 2015 at 5:27 PM, Ed White <edmund.h.white@xxxxxxxxx> wrote:
>> On 06/25/2015 11:04 PM, Jan Beulich wrote:
>>>>>> On 25.06.15 at 18:36, <edmund.h.white@xxxxxxxxx> wrote:
>>>> On 06/25/2015 01:12 AM, Jan Beulich wrote:
>>>>>>>> On 24.06.15 at 19:53, <edmund.h.white@xxxxxxxxx> wrote:
>>>>>> On 06/24/2015 07:38 AM, Jan Beulich wrote:
>>>>>>>>>> On 22.06.15 at 20:56, <edmund.h.white@xxxxxxxxx> 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.

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.

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.

Ed

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