Re: [PATCH 0 of 2] Add libxc API that sets mem_access type for an array of gfns

[Tim Deegan <tim@xen.org>]

Hi Aravindh,

At 15:02 -0700 on 04 May (1336143748), Aravindh Puthiyaparambil wrote:
> diff -r 9dda0efd8ce1 -r c180942992bf xen/arch/x86/mm/p2m-ept.c
> --- a/xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200
> +++ b/xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700
> @@ -274,10 +274,13 @@ static int ept_next_level(struct p2m_dom
>  /*
>   * ept_set_entry() computes 'need_modify_vtd_table' for itself,
>   * by observing whether any gfn->mfn translations are modified.
> + * If sync_deferred is not NULL, then the caller will take care of
> + * calling ept_sync_domain() in the cases where it can be deferred.
>   */
>  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)
> +              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,
> +              bool_t *sync_deferred)
>  {
>      ept_entry_t *table, *ept_entry = NULL;
>      unsigned long gfn_remainder = gfn;
> @@ -309,6 +312,9 @@ ept_set_entry(struct p2m_domain *p2m, un
>             (target == 1 && hvm_hap_has_2mb(d)) ||
>             (target == 0));
> 
> +    if (sync_deferred)

Xen coding style has spaces inside those parentheses.

> +        *sync_deferred = 1;
> +
>      table = map_domain_page(ept_get_asr(d));
> 
>      for ( i = ept_get_wl(d); i > target; i-- )
> @@ -345,8 +351,12 @@ ept_set_entry(struct p2m_domain *p2m, un
>          ept_entry_t new_entry = { .epte = 0 };
> 
>          /* No need to flush if the old entry wasn't valid */
> -        if ( !is_epte_present(ept_entry) )
> +        if ( !is_epte_present(ept_entry) || (!target && sync_deferred) )
> +        {
>              needs_sync = 0;
> +            if ( sync_deferred )
> +                *sync_deferred = 0;

I don't think you should do this -- you call this function in a loop and
you may need to sync because of an earlier iteration.  Better to only
write a 1 to *sync_deferred once you know there's a sync to be done, and
never to write a zero.

> +        }

One comment on the rest of the patch: your calling function calls
ept_sync_domain() directly if sync_deferred == 1.  That's correct for
now but it would be cleaner to use a sync() function pointer in the
struct p2m_domain, the same as the other implementation-specific calls.

Other than that, I think this looks pretty good to go in after 4.2 has
branched.

Cheers,

Tim.