Re: [PATCH v2 for-4.7 5/5] x86/hvm: Fix invalidation for emulated invlpg instructions

[George Dunlap <george.dunlap@citrix.com>]

On 09/05/16 19:27, Andrew Cooper wrote:
> hap_invlpg() is reachable from the instruction emulator, which means
> introspection and tests using hvm_fep can end up here.  As such, crashing the
> domain is not an appropriate action to take.
> 
> Fixing this involves rearranging the callgraph.
> 
> paging_invlpg() is now the central entry point.  It first checks for the
> non-canonical NOP case, and calls ino the paging subsystem.  If a real flush
> is needed, it will call the appropriate handler for the vcpu.  This allows the
> PV callsites of paging_invlpg() to be simplified.
> 
> The sole user of hvm_funcs.invlpg_intercept() is altered to use
> paging_invlpg() instead, allowing the .invlpg_intercept() hook to be removed.
> 
> For both VMX and SVM, the existing $VENDOR_invlpg_intercept() is split in
> half.  $VENDOR_invlpg_intercept() stays as the intercept handler only (which
> just calls paging_invlpg()), and new $VENDOR_invlpg() functions do the
> ASID/VPID management.  These later functions are made available in hvm_funcs
> for paging_invlpg() to use.
> 
> As a result, correct ASID/VPID management occurs for the hvmemul path, even if
> it did not originate from an real hardware intercept.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I haven't reviewed it in detail but it looks like a good idea; and based
on the other Reviewed-bys (and with or without Jan's suggested clean-up):

Acked-by: George Dunlap <george.dunlap@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> v2:
>  * Split performance improvement for AMD into previous patch
>  * Add vcpu parameter to new invlpg() function, and avoid assuming 'current'
>  * Modify paging_invlpg() to be void, and issue the PV TLB flush as well
> ---
>  xen/arch/x86/hvm/emulate.c    |  4 ++--
>  xen/arch/x86/hvm/svm/svm.c    | 11 +++++++----
>  xen/arch/x86/hvm/vmx/vmx.c    | 14 +++++++++-----
>  xen/arch/x86/mm.c             | 24 ++++++++++++++++++------
>  xen/arch/x86/mm/hap/hap.c     | 23 ++++++++++-------------
>  xen/include/asm-x86/hvm/hvm.h |  2 +-
>  xen/include/asm-x86/paging.h  | 11 ++---------
>  7 files changed, 49 insertions(+), 40 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index e6316be..b9cac8e 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1623,8 +1623,8 @@ static int hvmemul_invlpg(
>          rc = X86EMUL_OKAY;
>      }
>  
> -    if ( rc == X86EMUL_OKAY && is_canonical_address(addr) )
> -        hvm_funcs.invlpg_intercept(addr);
> +    if ( rc == X86EMUL_OKAY )
> +        paging_invlpg(current, addr);
>  
>      return rc;
>  }
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 081a5d8..679e615 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2222,10 +2222,13 @@ static void svm_invlpga_intercept(
>  
>  static void svm_invlpg_intercept(unsigned long vaddr)
>  {
> -    struct vcpu *curr = current;
>      HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(vaddr));
> -    if ( paging_invlpg(curr, vaddr) )
> -        svm_asid_g_invlpg(curr, vaddr);
> +    paging_invlpg(current, vaddr);
> +}
> +
> +static void svm_invlpg(struct vcpu *v, unsigned long vaddr)
> +{
> +    svm_asid_g_invlpg(v, vaddr);
>  }
>  
>  static struct hvm_function_table __initdata svm_function_table = {
> @@ -2258,12 +2261,12 @@ static struct hvm_function_table __initdata svm_function_table = {
>      .inject_trap          = svm_inject_trap,
>      .init_hypercall_page  = svm_init_hypercall_page,
>      .event_pending        = svm_event_pending,
> +    .invlpg               = svm_invlpg,
>      .cpuid_intercept      = svm_cpuid_intercept,
>      .wbinvd_intercept     = svm_wbinvd_intercept,
>      .fpu_dirty_intercept  = svm_fpu_dirty_intercept,
>      .msr_read_intercept   = svm_msr_read_intercept,
>      .msr_write_intercept  = svm_msr_write_intercept,
> -    .invlpg_intercept     = svm_invlpg_intercept,
>      .set_rdtsc_exiting    = svm_set_rdtsc_exiting,
>      .get_insn_bytes       = svm_get_insn_bytes,
>  
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index bc4410f..3acf1ab 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -81,7 +81,7 @@ static void vmx_wbinvd_intercept(void);
>  static void vmx_fpu_dirty_intercept(void);
>  static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content);
>  static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content);
> -static void vmx_invlpg_intercept(unsigned long vaddr);
> +static void vmx_invlpg(struct vcpu *v, unsigned long vaddr);
>  static int vmx_vmfunc_intercept(struct cpu_user_regs *regs);
>  
>  struct vmx_pi_blocking_vcpu {
> @@ -2137,6 +2137,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
>      .inject_trap          = vmx_inject_trap,
>      .init_hypercall_page  = vmx_init_hypercall_page,
>      .event_pending        = vmx_event_pending,
> +    .invlpg               = vmx_invlpg,
>      .cpu_up               = vmx_cpu_up,
>      .cpu_down             = vmx_cpu_down,
>      .cpuid_intercept      = vmx_cpuid_intercept,
> @@ -2144,7 +2145,6 @@ static struct hvm_function_table __initdata vmx_function_table = {
>      .fpu_dirty_intercept  = vmx_fpu_dirty_intercept,
>      .msr_read_intercept   = vmx_msr_read_intercept,
>      .msr_write_intercept  = vmx_msr_write_intercept,
> -    .invlpg_intercept     = vmx_invlpg_intercept,
>      .vmfunc_intercept     = vmx_vmfunc_intercept,
>      .handle_cd            = vmx_handle_cd,
>      .set_info_guest       = vmx_set_info_guest,
> @@ -2432,10 +2432,14 @@ static void vmx_dr_access(unsigned long exit_qualification,
>  
>  static void vmx_invlpg_intercept(unsigned long vaddr)
>  {
> -    struct vcpu *curr = current;
>      HVMTRACE_LONG_2D(INVLPG, /*invlpga=*/ 0, TRC_PAR_LONG(vaddr));
> -    if ( paging_invlpg(curr, vaddr) && cpu_has_vmx_vpid )
> -        vpid_sync_vcpu_gva(curr, vaddr);
> +    paging_invlpg(current, vaddr);
> +}
> +
> +static void vmx_invlpg(struct vcpu *v, unsigned long vaddr)
> +{
> +    if ( cpu_has_vmx_vpid )
> +        vpid_sync_vcpu_gva(v, vaddr);
>  }
>  
>  static int vmx_vmfunc_intercept(struct cpu_user_regs *regs)
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 2bb920b..03a4d5b 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3386,10 +3386,8 @@ long do_mmuext_op(
>          case MMUEXT_INVLPG_LOCAL:
>              if ( unlikely(d != pg_owner) )
>                  rc = -EPERM;
> -            else if ( !paging_mode_enabled(d)
> -                      ? __addr_ok(op.arg1.linear_addr)
> -                      : paging_invlpg(curr, op.arg1.linear_addr) )
> -                flush_tlb_one_local(op.arg1.linear_addr);
> +            else
> +                paging_invlpg(curr, op.arg1.linear_addr);
>              break;
>  
>          case MMUEXT_TLB_FLUSH_MULTI:
> @@ -4510,8 +4508,7 @@ static int __do_update_va_mapping(
>          switch ( (bmap_ptr = flags & ~UVMF_FLUSHTYPE_MASK) )
>          {
>          case UVMF_LOCAL:
> -            if ( !paging_mode_enabled(d) || paging_invlpg(v, va) )
> -                flush_tlb_one_local(va);
> +            paging_invlpg(v, va);
>              break;
>          case UVMF_ALL:
>              flush_tlb_one_mask(d->domain_dirty_cpumask, va);
> @@ -6478,6 +6475,21 @@ const unsigned long *__init get_platform_badpages(unsigned int *array_size)
>      return bad_pages;
>  }
>  
> +void paging_invlpg(struct vcpu *v, unsigned long va)
> +{
> +    if ( !is_canonical_address(va) )
> +        return;
> +
> +    if ( paging_mode_enabled(v->domain) &&
> +         !paging_get_hostmode(v)->invlpg(v, va) )
> +        return;
> +
> +    if ( is_pv_vcpu(v) )
> +        flush_tlb_one_local(va);
> +    else
> +        hvm_funcs.invlpg(v, va);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 4ab99bb..9c2cd49 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -680,23 +680,20 @@ static int hap_page_fault(struct vcpu *v, unsigned long va,
>  
>  /*
>   * HAP guests can handle invlpg without needing any action from Xen, so
> - * should not be intercepting it.
> + * should not be intercepting it.  However, we need to correctly handle
> + * getting here from instruction emulation.
>   */
>  static bool_t hap_invlpg(struct vcpu *v, unsigned long va)
>  {
> -    if (nestedhvm_enabled(v->domain)) {
> -        /* Emulate INVLPGA:
> -         * Must perform the flush right now or an other vcpu may
> -         * use it when we use the next VMRUN emulation, otherwise.
> -         */
> -        if ( vcpu_nestedhvm(v).nv_p2m )
> -            p2m_flush(v, vcpu_nestedhvm(v).nv_p2m);
> -        return 1;
> -    }
> +    /*
> +     * Emulate INVLPGA:
> +     * Must perform the flush right now or an other vcpu may
> +     * use it when we use the next VMRUN emulation, otherwise.
> +     */
> +    if ( nestedhvm_enabled(v->domain) && vcpu_nestedhvm(v).nv_p2m )
> +        p2m_flush(v, vcpu_nestedhvm(v).nv_p2m);
>  
> -    HAP_ERROR("Intercepted a guest INVLPG (%pv) with HAP enabled\n", v);
> -    domain_crash(v->domain);
> -    return 0;
> +    return 1;
>  }
>  
>  static void hap_update_cr3(struct vcpu *v, int do_locking)
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index add6ee8..ddbcbe8 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -154,6 +154,7 @@ struct hvm_function_table {
>      void (*init_hypercall_page)(struct domain *d, void *hypercall_page);
>  
>      int  (*event_pending)(struct vcpu *v);
> +    void (*invlpg)(struct vcpu *v, unsigned long vaddr);
>  
>      int  (*cpu_up_prepare)(unsigned int cpu);
>      void (*cpu_dead)(unsigned int cpu);
> @@ -172,7 +173,6 @@ struct hvm_function_table {
>      void (*fpu_dirty_intercept)(void);
>      int (*msr_read_intercept)(unsigned int msr, uint64_t *msr_content);
>      int (*msr_write_intercept)(unsigned int msr, uint64_t msr_content);
> -    void (*invlpg_intercept)(unsigned long vaddr);
>      int (*vmfunc_intercept)(struct cpu_user_regs *regs);
>      void (*handle_cd)(struct vcpu *v, unsigned long value);
>      void (*set_info_guest)(struct vcpu *v);
> diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
> index ab131cc..a1401ab 100644
> --- a/xen/include/asm-x86/paging.h
> +++ b/xen/include/asm-x86/paging.h
> @@ -237,15 +237,8 @@ paging_fault(unsigned long va, struct cpu_user_regs *regs)
>      return paging_get_hostmode(v)->page_fault(v, va, regs);
>  }
>  
> -/* Handle invlpg requests on vcpus.
> - * Returns 1 if the invlpg instruction should be issued on the hardware,
> - * or 0 if it's safe not to do so. */
> -static inline bool_t paging_invlpg(struct vcpu *v, unsigned long va)
> -{
> -    return (paging_mode_external(v->domain) ? is_canonical_address(va)
> -                                            : __addr_ok(va)) &&
> -           paging_get_hostmode(v)->invlpg(v, va);
> -}
> +/* Handle invlpg requests on vcpus. */
> +void paging_invlpg(struct vcpu *v, unsigned long va);
>  
>  /* Translate a guest virtual address to the frame number that the
>   * *guest* pagetables would map it to.  Returns INVALID_GFN if the guest
> 


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