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

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

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.

Ed