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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/06/15 19:56, Ed White wrote:
> The existing ept_set_entry() and ept_get_entry() routines are extended
> to optionally set/get suppress_ve and renamed. New ept_set_entry() and
> ept_get_entry() routines are provided as wrappers, where set preserves
> suppress_ve for an existing entry and sets it for a new entry.
>
> Additional function pointers are added to p2m_domain to allow direct
> access to the extended routines.
>
> Signed-off-by: Ed White <edmund.h.white@intel.com>
> ---
>  xen/arch/x86/mm/p2m-ept.c | 40 +++++++++++++++++++++++++++++++++-------
>  xen/include/asm-x86/p2m.h | 13 +++++++++++++
>  2 files changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 5de3387..e7719cf 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -649,14 +649,15 @@ bool_t ept_handle_misconfig(uint64_t gpa)
>  }
>  
>  /*
> - * ept_set_entry() computes 'need_modify_vtd_table' for itself,
> + * ept_set_entry_sve() computes 'need_modify_vtd_table' for itself,
>   * by observing whether any gfn->mfn translations are modified.
>   *
>   * Returns: 0 for success, -errno for failure
>   */
>  static int
> -ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, 
> -              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma)
> +ept_set_entry_sve(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, 
> +                  unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,
> +                  unsigned int sve)

I would be tempted to name this _ept_set_entry() rather than using a
_sve suffix.

unsigned int sve would be better int sve using -1/0/1, as it is
logically a trinay value (copy existing, clear, set).

>  {
>      ept_entry_t *table, *ept_entry = NULL;
>      unsigned long gfn_remainder = gfn;
> @@ -802,7 +803,11 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>          ept_p2m_type_to_flags(p2m, &new_entry, p2mt, p2ma);
>      }
>  
> -    new_entry.suppress_ve = 1;
> +    if ( sve != ~0 )
> +        new_entry.suppress_ve = !!sve;
> +    else
> +        new_entry.suppress_ve = is_epte_valid(&old_entry) ?
> +                                    old_entry.suppress_ve : 1;
>  
>      rc = atomic_write_ept_entry(ept_entry, new_entry, target);
>      if ( unlikely(rc) )(set/clear/copy).
> @@ -847,10 +852,18 @@ out:
>      return rc;
>  }
>  
> +static int
> +ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, 
> +              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma)
> +{
> +    return ept_set_entry_sve(p2m, gfn, mfn, order, p2mt, p2ma, ~0);
> +}
> +
>  /* Read ept p2m entries */
> -static mfn_t ept_get_entry(struct p2m_domain *p2m,
> -                           unsigned long gfn, p2m_type_t *t, p2m_access_t* a,
> -                           p2m_query_t q, unsigned int *page_order)
> +static mfn_t ept_get_entry_sve(struct p2m_domain *p2m,
> +                               unsigned long gfn, p2m_type_t *t, p2m_access_t* a,
> +                               p2m_query_t q, unsigned int *page_order,
> +                               unsigned int *sve)

This should be a bool_t * as it is very definitely a boolean output.

~Andrew