[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.