Re: [PATCHv4 1/2] x86/ept: invalidate guest physical mappings on VMENTER

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

On 14/12/15 14:39, David Vrabel wrote:
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index eef0372..6e0cf89 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1089,9 +1089,10 @@ static void ept_memory_type_changed(struct p2m_domain *p2m)
>  
>  static void __ept_sync_domain(void *info)
>  {
> -    struct ept_data *ept = &((struct p2m_domain *)info)->ept;
> -
> -    __invept(INVEPT_SINGLE_CONTEXT, ept_get_eptp(ept), 0);
> +    /*
> +     * The invalidate will be done before VMENTER (see
> +     * vmx_vmenter_helper()).
> +     */
>  }
>  
>  void ept_sync_domain(struct p2m_domain *p2m)
> @@ -1107,16 +1108,10 @@ void ept_sync_domain(struct p2m_domain *p2m)
>      if ( nestedhvm_enabled(d) && !p2m_is_nestedp2m(p2m) )
>          p2m_flush_nestedp2m(d);
>  
> -    /*
> -     * Flush active cpus synchronously. Flush others the next time this domain
> -     * is scheduled onto them. We accept the race of other CPUs adding to
> -     * the ept_synced mask before on_selected_cpus() reads it, resulting in
> -     * unnecessary extra flushes, to avoid allocating a cpumask_t on the stack.
> -     */
> -    cpumask_and(ept_get_synced_mask(ept),
> -                d->domain_dirty_cpumask, &cpu_online_map);
> +    /* May need to invalidate on all PCPUs. */
> +    cpumask_setall(ept->invalidate);
>  
> -    on_selected_cpus(ept_get_synced_mask(ept),
> +    on_selected_cpus(d->domain_dirty_cpumask,
>                       __ept_sync_domain, p2m, 1);

You can drop __ept_sync_domain() entirely by using
smp_send_event_check_mask() instead, which is a no-op IPI (and slightly
less overhead while holding the IPI lock).

>  }
>  
> @@ -1182,10 +1177,14 @@ int ept_p2m_init(struct p2m_domain *p2m)
>          p2m->flush_hardware_cached_dirty = ept_flush_pml_buffers;
>      }
>  
> -    if ( !zalloc_cpumask_var(&ept->synced_mask) )
> +    if ( !zalloc_cpumask_var(&ept->invalidate) )
>          return -ENOMEM;
>  
> -    on_each_cpu(__ept_sync_domain, p2m, 1);
> +    /*
> +     * Assume an initial invalidation is required, in case an EP4TA is
> +     * reused.
> +     */
> +    cpumask_setall(ept->invalidate);
>  
>      return 0;
>  }
> @@ -1193,7 +1192,7 @@ int ept_p2m_init(struct p2m_domain *p2m)
>  void ept_p2m_uninit(struct p2m_domain *p2m)
>  {
>      struct ept_data *ept = &p2m->ept;
> -    free_cpumask_var(ept->synced_mask);
> +    free_cpumask_var(ept->invalidate);
>  }
>  
>  static void ept_dump_p2m_table(unsigned char key)
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index a8d4d5b..e778d86 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -67,7 +67,7 @@ struct ept_data {
>          };
>          u64 eptp;
>      };
> -    cpumask_var_t synced_mask;
> +    cpumask_var_t invalidate;

Could you include a small comment here to describe the behaviour?  Perhaps:

/* Whether an INVEPT should be issued on VMENTER? */

~Andrew