[PATCH for-4.7 0/4] Fixes for invlpg handling for HVM guests

[PATCH for-4.7 0/4] Fixes for invlpg handling for HVM guests

[Andrew Cooper]

Turns out there are a lot of broken corner cases.

Andrew Cooper (4):
  x86/hvm: Always return the linear address from
    hvm_virtual_to_linear_addr()
  x86/hvm: Raise #SS faults for %ss-based segmentation violations
  x86/hvm: Correct the emulated interaction of invlpg with segments
  x86/hvm: Fix invalidation for emulated invlpg instructions

 xen/arch/x86/hvm/emulate.c      | 20 ++++++++++++++++++--
 xen/arch/x86/hvm/hvm.c          | 37 +++++++++++++++++++++++--------------
 xen/arch/x86/hvm/svm/svm.c      | 11 +++++++----
 xen/arch/x86/hvm/vmx/vmx.c      | 14 +++++++++-----
 xen/arch/x86/mm.c               | 13 +++++++++++++
 xen/arch/x86/mm/hap/hap.c       | 23 ++++++++++-------------
 xen/arch/x86/mm/shadow/common.c |  3 ++-
 xen/include/asm-x86/hvm/hvm.h   |  4 ++--
 xen/include/asm-x86/paging.h    |  7 +------
 9 files changed, 85 insertions(+), 47 deletions(-)

-- 
2.1.4


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

[PATCH for-4.7 1/4] x86/hvm: Always return the linear address from hvm_virtual_to_linear_addr()

[Andrew Cooper]

Some callers need the linear address (with appropriate segment base), whether
or not the limit or canonical check succeeds.

While modifying the function, change the return type to bool_t to match its
semantics.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/hvm.c        | 37 +++++++++++++++++++++++--------------
 xen/include/asm-x86/hvm/hvm.h |  2 +-
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 82e2ed1..863d134 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2411,7 +2411,7 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
     return X86EMUL_EXCEPTION;
 }
 
-int hvm_virtual_to_linear_addr(
+bool_t hvm_virtual_to_linear_addr(
     enum x86_segment seg,
     struct segment_register *reg,
     unsigned long offset,
@@ -2421,6 +2421,7 @@ int hvm_virtual_to_linear_addr(
     unsigned long *linear_addr)
 {
     unsigned long addr = offset, last_byte;
+    bool_t okay = 0;
 
     if ( !(current->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE) )
     {
@@ -2431,7 +2432,7 @@ int hvm_virtual_to_linear_addr(
         addr = (uint32_t)(addr + reg->base);
         last_byte = (uint32_t)addr + bytes - !!bytes;
         if ( last_byte < addr )
-            return 0;
+            goto out;
     }
     else if ( addr_size != 64 )
     {
@@ -2439,15 +2440,21 @@ int hvm_virtual_to_linear_addr(
          * COMPATIBILITY MODE: Apply segment checks and add base.
          */
 
+        /*
+         * Hardware truncates to 32 bits in compatibility mode.
+         * It does not truncate to 16 bits in 16-bit address-size mode.
+         */
+        addr = (uint32_t)(addr + reg->base);
+
         switch ( access_type )
         {
         case hvm_access_read:
             if ( (reg->attr.fields.type & 0xa) == 0x8 )
-                return 0; /* execute-only code segment */
+                goto out; /* execute-only code segment */
             break;
         case hvm_access_write:
             if ( (reg->attr.fields.type & 0xa) != 0x2 )
-                return 0; /* not a writable data segment */
+                goto out; /* not a writable data segment */
             break;
         default:
             break;
@@ -2464,16 +2471,10 @@ int hvm_virtual_to_linear_addr(
 
             /* Check first byte and last byte against respective bounds. */
             if ( (offset <= reg->limit) || (last_byte < offset) )
-                return 0;
+                goto out;
         }
         else if ( (last_byte > reg->limit) || (last_byte < offset) )
-            return 0; /* last byte is beyond limit or wraps 0xFFFFFFFF */
-
-        /*
-         * Hardware truncates to 32 bits in compatibility mode.
-         * It does not truncate to 16 bits in 16-bit address-size mode.
-         */
-        addr = (uint32_t)(addr + reg->base);
+            goto out; /* last byte is beyond limit or wraps 0xFFFFFFFF */
     }
     else
     {
@@ -2487,11 +2488,19 @@ int hvm_virtual_to_linear_addr(
         last_byte = addr + bytes - !!bytes;
         if ( !is_canonical_address(addr) || last_byte < addr ||
              !is_canonical_address(last_byte) )
-            return 0;
+            goto out;
     }
 
+    /* All checks ok. */
+    okay = 1;
+
+ out:
+    /*
+     * Always return the correct linear address, even if a permission check
+     * failed.  The permissions failure is not relevant to some callers.
+     */
     *linear_addr = addr;
-    return 1;
+    return okay;
 }
 
 struct hvm_write_map {
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 7b7ff3f..add6ee8 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -471,7 +471,7 @@ enum hvm_access_type {
     hvm_access_read,
     hvm_access_write
 };
-int hvm_virtual_to_linear_addr(
+bool_t hvm_virtual_to_linear_addr(
     enum x86_segment seg,
     struct segment_register *reg,
     unsigned long offset,
-- 
2.1.4


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

Re: [PATCH for-4.7 1/4] x86/hvm: Always return the linear address from hvm_virtual_to_linear_addr()

[Jan Beulich]

>>> On 09.05.16 at 15:15, <andrew.cooper3@citrix.com> wrote:
> Some callers need the linear address (with appropriate segment base), whether
> or not the limit or canonical check succeeds.
> 
> While modifying the function, change the return type to bool_t to match its
> semantics.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

[PATCH for-4.7 2/4] x86/hvm: Raise #SS faults for %ss-based segmentation violations

[Andrew Cooper]

Raising #GP under such circumstances is architecturally wrong.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/emulate.c      | 3 ++-
 xen/arch/x86/mm/shadow/common.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index be1e7c2..ee5cf1f 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -566,7 +566,8 @@ static int hvmemul_virtual_to_linear(
 
     /* This is a singleton operation: fail it with an exception. */
     hvmemul_ctxt->exn_pending = 1;
-    hvmemul_ctxt->trap.vector = TRAP_gp_fault;
+    hvmemul_ctxt->trap.vector =
+        (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault;
     hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
     hvmemul_ctxt->trap.error_code = 0;
     hvmemul_ctxt->trap.insn_len = 0;
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 559d4a4..226e32d 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -148,7 +148,8 @@ static int hvm_translate_linear_addr(
 
     if ( !okay )
     {
-        hvm_inject_hw_exception(TRAP_gp_fault, 0);
+        hvm_inject_hw_exception(
+            (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault, 0);
         return X86EMUL_EXCEPTION;
     }
 
-- 
2.1.4


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

Re: [PATCH for-4.7 2/4] x86/hvm: Raise #SS faults for %ss-based segmentation violations

[Jan Beulich]

>>> On 09.05.16 at 15:15, <andrew.cooper3@citrix.com> wrote:
> Raising #GP under such circumstances is architecturally wrong.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH for-4.7 2/4] x86/hvm: Raise #SS faults for %ss-based segmentation violations

[Wei Liu]

CC George as well

On Mon, May 09, 2016 at 02:15:40PM +0100, Andrew Cooper wrote:
> Raising #GP under such circumstances is architecturally wrong.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/hvm/emulate.c      | 3 ++-
>  xen/arch/x86/mm/shadow/common.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index be1e7c2..ee5cf1f 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -566,7 +566,8 @@ static int hvmemul_virtual_to_linear(
>  
>      /* This is a singleton operation: fail it with an exception. */
>      hvmemul_ctxt->exn_pending = 1;
> -    hvmemul_ctxt->trap.vector = TRAP_gp_fault;
> +    hvmemul_ctxt->trap.vector =
> +        (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault;
>      hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
>      hvmemul_ctxt->trap.error_code = 0;
>      hvmemul_ctxt->trap.insn_len = 0;
> diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
> index 559d4a4..226e32d 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -148,7 +148,8 @@ static int hvm_translate_linear_addr(
>  
>      if ( !okay )
>      {
> -        hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +        hvm_inject_hw_exception(
> +            (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault, 0);
>          return X86EMUL_EXCEPTION;
>      }
>  
> -- 
> 2.1.4
> 

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

Re: [PATCH for-4.7 2/4] x86/hvm: Raise #SS faults for %ss-based segmentation violations

[David Vrabel]

On 09/05/16 14:15, Andrew Cooper wrote:
> Raising #GP under such circumstances is architecturally wrong.

You should include a reference to the relevant Intel and AMD manuals here.

David

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/hvm/emulate.c      | 3 ++-
>  xen/arch/x86/mm/shadow/common.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index be1e7c2..ee5cf1f 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -566,7 +566,8 @@ static int hvmemul_virtual_to_linear(
>  
>      /* This is a singleton operation: fail it with an exception. */
>      hvmemul_ctxt->exn_pending = 1;
> -    hvmemul_ctxt->trap.vector = TRAP_gp_fault;
> +    hvmemul_ctxt->trap.vector =
> +        (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault;
>      hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
>      hvmemul_ctxt->trap.error_code = 0;
>      hvmemul_ctxt->trap.insn_len = 0;
> diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
> index 559d4a4..226e32d 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -148,7 +148,8 @@ static int hvm_translate_linear_addr(
>  
>      if ( !okay )
>      {
> -        hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +        hvm_inject_hw_exception(
> +            (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault, 0);
>          return X86EMUL_EXCEPTION;
>      }
>  
> 


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

[PATCH for-4.7 3/4] x86/hvm: Correct the emulated interaction of invlpg with segments

[Andrew Cooper]

The `invlpg` instruction is documented to take a memory address, and is not
documented to suffer faults from segmentation violations.

Experimentally, and subsequently confirmed by both Intel and AMD, the
instruction does take into account segment bases, but will happily invalidate
a TLB entry for a mapping beyond the segment limit.

The emulation logic will currently raise #GP/#SS faults for segment limit
violations, or non-canonical addresses, which doesn't match hardware's
behaviour.  Instead, squash exceptions generated by
hvmemul_virtual_to_linear() and proceed with invalidation.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/emulate.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index ee5cf1f..e6316be 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1608,7 +1608,22 @@ static int hvmemul_invlpg(
     rc = hvmemul_virtual_to_linear(
         seg, offset, 1, &reps, hvm_access_none, hvmemul_ctxt, &addr);
 
-    if ( rc == X86EMUL_OKAY )
+    if ( rc == X86EMUL_EXCEPTION )
+    {
+        /*
+         * `invlpg` takes segment bases into account, but is not subject to
+         * faults from segment type/limit checks, and is specified as a NOP
+         * when issued on non-canonical addresses.
+         *
+         * hvmemul_virtual_to_linear() raises exceptions for type/limit
+         * violations, so squash them.
+         */
+        hvmemul_ctxt->exn_pending = 0;
+        hvmemul_ctxt->trap = (struct hvm_trap){};
+        rc = X86EMUL_OKAY;
+    }
+
+    if ( rc == X86EMUL_OKAY && is_canonical_address(addr) )
         hvm_funcs.invlpg_intercept(addr);
 
     return rc;
-- 
2.1.4


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

Re: [PATCH for-4.7 3/4] x86/hvm: Correct the emulated interaction of invlpg with segments

[Jan Beulich]

>>> On 09.05.16 at 15:15, <andrew.cooper3@citrix.com> wrote:
> The `invlpg` instruction is documented to take a memory address, and is not
> documented to suffer faults from segmentation violations.
> 
> Experimentally, and subsequently confirmed by both Intel and AMD, the
> instruction does take into account segment bases, but will happily invalidate
> a TLB entry for a mapping beyond the segment limit.

How about non-canonical addresses? If those don't #GP (which is
what I assume), is such an INVLPG a NOP, or does it invalidate
something (e.g. the translation for the address truncated to 48
bits)? In that latter case we might need to also make our code
behave that way...

> The emulation logic will currently raise #GP/#SS faults for segment limit
> violations, or non-canonical addresses, which doesn't match hardware's
> behaviour.  Instead, squash exceptions generated by
> hvmemul_virtual_to_linear() and proceed with invalidation.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

albeit I think ...

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1608,7 +1608,22 @@ static int hvmemul_invlpg(
>      rc = hvmemul_virtual_to_linear(
>          seg, offset, 1, &reps, hvm_access_none, hvmemul_ctxt, &addr);
>  
> -    if ( rc == X86EMUL_OKAY )
> +    if ( rc == X86EMUL_EXCEPTION )
> +    {
> +        /*
> +         * `invlpg` takes segment bases into account, but is not subject to
> +         * faults from segment type/limit checks, and is specified as a NOP
> +         * when issued on non-canonical addresses.
> +         *
> +         * hvmemul_virtual_to_linear() raises exceptions for type/limit
> +         * violations, so squash them.
> +         */
> +        hvmemul_ctxt->exn_pending = 0;
> +        hvmemul_ctxt->trap = (struct hvm_trap){};

... this does more work than is really needed.

Jan


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

Re: [PATCH for-4.7 3/4] x86/hvm: Correct the emulated interaction of invlpg with segments

[Andrew Cooper]

On 09/05/16 14:42, Jan Beulich wrote:
>>>> On 09.05.16 at 15:15, <andrew.cooper3@citrix.com> wrote:
>> The `invlpg` instruction is documented to take a memory address, and is not
>> documented to suffer faults from segmentation violations.
>>
>> Experimentally, and subsequently confirmed by both Intel and AMD, the
>> instruction does take into account segment bases, but will happily invalidate
>> a TLB entry for a mapping beyond the segment limit.
> How about non-canonical addresses? If those don't #GP (which is
> what I assume), is such an INVLPG a NOP

They are explicitly documented by both Intel and AMD as being a NOP when
passed a non-canonical address.  Experimentation confirms this.

(I am just putting the finishing touches on an XTF test for all of this).

> , or does it invalidate
> something (e.g. the translation for the address truncated to 48
> bits)? In that latter case we might need to also make our code
> behave that way...
>
>> The emulation logic will currently raise #GP/#SS faults for segment limit
>> violations, or non-canonical addresses, which doesn't match hardware's
>> behaviour.  Instead, squash exceptions generated by
>> hvmemul_virtual_to_linear() and proceed with invalidation.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> albeit I think ...
>
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -1608,7 +1608,22 @@ static int hvmemul_invlpg(
>>      rc = hvmemul_virtual_to_linear(
>>          seg, offset, 1, &reps, hvm_access_none, hvmemul_ctxt, &addr);
>>  
>> -    if ( rc == X86EMUL_OKAY )
>> +    if ( rc == X86EMUL_EXCEPTION )
>> +    {
>> +        /*
>> +         * `invlpg` takes segment bases into account, but is not subject to
>> +         * faults from segment type/limit checks, and is specified as a NOP
>> +         * when issued on non-canonical addresses.
>> +         *
>> +         * hvmemul_virtual_to_linear() raises exceptions for type/limit
>> +         * violations, so squash them.
>> +         */
>> +        hvmemul_ctxt->exn_pending = 0;
>> +        hvmemul_ctxt->trap = (struct hvm_trap){};
> ... this does more work than is really needed.

For sanity sake, I would prefer not to leave stale information in the
emulation context.  This path is definitely not a hotpath.

~Andrew

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

Re: [PATCH for-4.7 3/4] x86/hvm: Correct the emulated interaction of invlpg with segments

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 09 May 2016 14:16
> To: Xen-devel
> Cc: Andrew Cooper; Jan Beulich; Paul Durrant; Wei Liu
> Subject: [PATCH for-4.7 3/4] x86/hvm: Correct the emulated interaction of
> invlpg with segments
> 
> The `invlpg` instruction is documented to take a memory address, and is not
> documented to suffer faults from segmentation violations.
> 
> Experimentally, and subsequently confirmed by both Intel and AMD, the
> instruction does take into account segment bases, but will happily invalidate
> a TLB entry for a mapping beyond the segment limit.
> 
> The emulation logic will currently raise #GP/#SS faults for segment limit
> violations, or non-canonical addresses, which doesn't match hardware's
> behaviour.  Instead, squash exceptions generated by
> hvmemul_virtual_to_linear() and proceed with invalidation.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/hvm/emulate.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index ee5cf1f..e6316be 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1608,7 +1608,22 @@ static int hvmemul_invlpg(
>      rc = hvmemul_virtual_to_linear(
>          seg, offset, 1, &reps, hvm_access_none, hvmemul_ctxt, &addr);
> 
> -    if ( rc == X86EMUL_OKAY )
> +    if ( rc == X86EMUL_EXCEPTION )
> +    {
> +        /*
> +         * `invlpg` takes segment bases into account, but is not subject to
> +         * faults from segment type/limit checks, and is specified as a NOP
> +         * when issued on non-canonical addresses.
> +         *
> +         * hvmemul_virtual_to_linear() raises exceptions for type/limit
> +         * violations, so squash them.
> +         */
> +        hvmemul_ctxt->exn_pending = 0;
> +        hvmemul_ctxt->trap = (struct hvm_trap){};
> +        rc = X86EMUL_OKAY;
> +    }
> +
> +    if ( rc == X86EMUL_OKAY && is_canonical_address(addr) )
>          hvm_funcs.invlpg_intercept(addr);
> 
>      return rc;
> --
> 2.1.4


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

[PATCH for-4.7 4/4] x86/hvm: Fix invalidation for emulated invlpg instructions

[Andrew Cooper]

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
applicability of invalidation based on virtual address, and optionally calls
into the paging invalidation logic.  For HVM domains, it also makes ASID/VPID
management calls.

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.

Signed-off-by: Andrew Cooper <andrew.cooper3@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>
---
 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             | 13 +++++++++++++
 xen/arch/x86/mm/hap/hap.c     | 23 ++++++++++-------------
 xen/include/asm-x86/hvm/hvm.h |  2 +-
 xen/include/asm-x86/paging.h  |  7 +------
 7 files changed, 43 insertions(+), 31 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 7634c3f..1082e4b 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));
-    paging_invlpg(curr, vaddr);
-    svm_asid_g_invlpg(curr, vaddr);
+    paging_invlpg(current, vaddr);
+}
+
+static void svm_invlpg(unsigned long vaddr)
+{
+    svm_asid_g_invlpg(current, 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..a41a981 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(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(unsigned long vaddr)
+{
+    if ( cpu_has_vmx_vpid )
+        vpid_sync_vcpu_gva(current, 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..daf5376 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6478,6 +6478,19 @@ const unsigned long *__init get_platform_badpages(unsigned int *array_size)
     return bad_pages;
 }
 
+bool_t paging_invlpg(struct vcpu *v, unsigned long va)
+{
+    bool_t invl = paging_mode_external(v->domain)
+        ? is_canonical_address(va) : __addr_ok(va);
+
+    if ( invl )
+        invl = paging_get_hostmode(v)->invlpg(v, va);
+    if ( invl && is_hvm_domain(v->domain) )
+        hvm_funcs.invlpg(va);
+
+    return invl;
+}
+
 /*
  * 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..9e1ff7f 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)(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..7847d2c 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -240,12 +240,7 @@ paging_fault(unsigned long va, struct cpu_user_regs *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);
-}
+bool_t 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
-- 
2.1.4


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

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

[Jan Beulich]

>>> On 09.05.16 at 15:15, <andrew.cooper3@citrix.com> wrote:
> --- 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));
> -    paging_invlpg(curr, vaddr);
> -    svm_asid_g_invlpg(curr, vaddr);
> +    paging_invlpg(current, vaddr);
> +}
> +
> +static void svm_invlpg(unsigned long vaddr)
> +{
> +    svm_asid_g_invlpg(current, vaddr);

Since paging_invlpg() already gets passed a struct vcpu *, having
it hand it to ->invlpg() would seem quite natural.

> @@ -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 )

So the previous dependency on paging_invlpg()'s return value gets
moved into that function (making the call here conditional). While
that's fine for VMX, SVM previously didn't pay attention to that
return value, and hence you're altering behavior in a way not
spelled out in the description (and to be honest I'm also not 100%
certain that change is correct).

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -6478,6 +6478,19 @@ const unsigned long *__init get_platform_badpages(unsigned int *array_size)
>      return bad_pages;
>  }
>  
> +bool_t paging_invlpg(struct vcpu *v, unsigned long va)
> +{
> +    bool_t invl = paging_mode_external(v->domain)
> +        ? is_canonical_address(va) : __addr_ok(va);
> +
> +    if ( invl )
> +        invl = paging_get_hostmode(v)->invlpg(v, va);
> +    if ( invl && is_hvm_domain(v->domain) )

has_hvm_container_domain() (or paging_mode_external(),
implying the former)?

Jan


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

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

[Andrew Cooper]

On 09/05/16 14:57, Jan Beulich wrote:
>>>> On 09.05.16 at 15:15, <andrew.cooper3@citrix.com> wrote:
>> --- 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));
>> -    paging_invlpg(curr, vaddr);
>> -    svm_asid_g_invlpg(curr, vaddr);
>> +    paging_invlpg(current, vaddr);
>> +}
>> +
>> +static void svm_invlpg(unsigned long vaddr)
>> +{
>> +    svm_asid_g_invlpg(current, vaddr);
> Since paging_invlpg() already gets passed a struct vcpu *, having
> it hand it to ->invlpg() would seem quite natural.

Ok.

>
>> @@ -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 )
> So the previous dependency on paging_invlpg()'s return value gets
> moved into that function (making the call here conditional). While
> that's fine for VMX, SVM previously didn't pay attention to that
> return value, and hence you're altering behavior in a way not
> spelled out in the description (and to be honest I'm also not 100%
> certain that change is correct).

I thought I had included this in the commit message, but it appears it
got lost.

AMD is over-the-top with its TLB flushing generally, and will allocate a
new ASID rather than shooting a single mapping out of the current ASID. 
The new svm_invlpg() should be a simple invlpga() call with the in-use
ASID.  However, the instruction emulator forwards invlpga to invlpg and
loses the ASID, meaning that the only reason it functions is because
this case over-flushes at the moment.  I opted not to break that usecase.

However, for the cases where paging_invlpg() returns 0, there is no need
to allocate an ASID at all, as there are no mapping needing invalidation
which could have been cached in the TLB.  As such, this change is a
performance improvement, albeit a minor one as the common case is dealt
with directly in hardware without Xen's interaction.

>
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -6478,6 +6478,19 @@ const unsigned long *__init get_platform_badpages(unsigned int *array_size)
>>      return bad_pages;
>>  }
>>  
>> +bool_t paging_invlpg(struct vcpu *v, unsigned long va)
>> +{
>> +    bool_t invl = paging_mode_external(v->domain)
>> +        ? is_canonical_address(va) : __addr_ok(va);
>> +
>> +    if ( invl )
>> +        invl = paging_get_hostmode(v)->invlpg(v, va);
>> +    if ( invl && is_hvm_domain(v->domain) )
> has_hvm_container_domain() (or paging_mode_external(),
> implying the former)?

I could have sworn I already fixed this...  Will do for v2.

~Andrew

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

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

[Andrew Cooper]

On 09/05/16 16:14, Tim Deegan wrote:
> Hi,
>
> At 14:15 +0100 on 09 May (1462803342), 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
>> applicability of invalidation based on virtual address, and optionally calls
>> into the paging invalidation logic.  For HVM domains, it also makes ASID/VPID
>> management calls.
> This reshuffle looks fine, but leaves the return value looking pretty
> strange.

I suppose it does.  This looks to be better option.

> For HVM guests it's longer correct (since the hardware
> operation has moved inside paging_invlpg() it should always return 0)
> but none of the callers actually check it, so yay?
>
> I think it might be better to make that return value internal to
> paging_invlpg() and have it DTRT for PV guests as it now does for HVM
> ones, e.g.:
>
>     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(va);
>     }
>
> with appropriate simplifications at the PV callsites.
>
> I simplified the canonical/__addr_ok test there because I don't think
> we care about the speed of guest invlpg of Xen addresses; I suspect we
> could remove it entirely, and turn a fast emulated NOP into a slow one.

A PV guest should not be able to invlpg Xen addresses at all, which is
why the checks were recently added in c/s 828e114f7 "x86/mmuext: tighten
TLB flush address checks".  I will see if I can untangle this as well
without avoiding the __addr_ok() check.

~Andrew

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