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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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.

 -George

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