Re: [Xen-devel] [PATCH v4 18/24] x86/shadow: Avoid raising faults behind the emulators back

2016-12-02 Thread Andrew Cooper 
On 02/12/16 11:37, Jan Beulich wrote:
>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -3373,18 +3373,36 @@ static int sh_page_fault(struct vcpu *v,
>>  
>>  r = x86_emulate(&emul_ctxt.ctxt, emul_ops);
>>  
>> -/*
>> - * The previous lack of inject_{sw,hw}*() hooks caused exceptions raised
>> - * by the emulator itself to become X86EMUL_UNHANDLEABLE.  Such 
>> exceptions
>> - * now set event_pending instead.  Exceptions raised behind the back of
>> - * the emulator don't yet set event_pending.
>> - *
>> - * For now, cause such cases to return to the X86EMUL_UNHANDLEABLE path,
>> - * for no functional change from before.  Future patches will fix this
>> - * properly.
>> - */
>>  if ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending )
>> -r = X86EMUL_UNHANDLEABLE;
>> +{
>> +/*
>> + * This emulation covers writes to shadow pagetables.  We tolerate 
>> #PF
>> + * (from accesses spanning pages, concurrent paging updated from
>> + * vcpus, etc) and #GP[0]/#SS[0] (from segmentation errors).  
>> Anything
>> + * else is an emulation bug, or a guest playing with the instruction
>> + * stream under Xen's feet.
>> + */
>> +if ( emul_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
>> + ((emul_ctxt.ctxt.event.vector == TRAP_page_fault) ||
>> +  (((emul_ctxt.ctxt.event.vector == TRAP_gp_fault) ||
>> +(emul_ctxt.ctxt.event.vector == TRAP_stack_error)) &&
>> +   emul_ctxt.ctxt.event.error_code == 0)) )
>> +{
>> +if ( has_hvm_container_domain(d) )
>> +hvm_inject_event(&emul_ctxt.ctxt.event);
>> +else
>> +pv_inject_event(&emul_ctxt.ctxt.event);
>> +
>> +goto emulate_done;
>> +}
>> +else
> ... the else following a goto is kind of pointless and imo makes the
> code slightly harder to follow.

Oh - good point. This is actually a slight behavioural change, as it
skips the trace.  I will drop the goto.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 18/24] x86/shadow: Avoid raising faults behind the emulators back

2016-12-02 Thread Jan Beulich 
>>> On 01.12.16 at 17:56,  wrote:
> Use x86_emul_{hw_exception,pagefault}() rather than
> {pv,hvm}_inject_page_fault() and hvm_inject_hw_exception() to cause raised
> faults to be known to the emulator.  This requires altering the callers of
> x86_emulate() to properly re-inject the event.
> 
> Signed-off-by: Andrew Cooper 
> Acked-by: Tim Deegan 

Reviewed-by: Jan Beulich 
albeit ...

> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3373,18 +3373,36 @@ static int sh_page_fault(struct vcpu *v,
>  
>  r = x86_emulate(&emul_ctxt.ctxt, emul_ops);
>  
> -/*
> - * The previous lack of inject_{sw,hw}*() hooks caused exceptions raised
> - * by the emulator itself to become X86EMUL_UNHANDLEABLE.  Such 
> exceptions
> - * now set event_pending instead.  Exceptions raised behind the back of
> - * the emulator don't yet set event_pending.
> - *
> - * For now, cause such cases to return to the X86EMUL_UNHANDLEABLE path,
> - * for no functional change from before.  Future patches will fix this
> - * properly.
> - */
>  if ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending )
> -r = X86EMUL_UNHANDLEABLE;
> +{
> +/*
> + * This emulation covers writes to shadow pagetables.  We tolerate 
> #PF
> + * (from accesses spanning pages, concurrent paging updated from
> + * vcpus, etc) and #GP[0]/#SS[0] (from segmentation errors).  
> Anything
> + * else is an emulation bug, or a guest playing with the instruction
> + * stream under Xen's feet.
> + */
> +if ( emul_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
> + ((emul_ctxt.ctxt.event.vector == TRAP_page_fault) ||
> +  (((emul_ctxt.ctxt.event.vector == TRAP_gp_fault) ||
> +(emul_ctxt.ctxt.event.vector == TRAP_stack_error)) &&
> +   emul_ctxt.ctxt.event.error_code == 0)) )
> +{
> +if ( has_hvm_container_domain(d) )
> +hvm_inject_event(&emul_ctxt.ctxt.event);
> +else
> +pv_inject_event(&emul_ctxt.ctxt.event);
> +
> +goto emulate_done;
> +}
> +else

... the else following a goto is kind of pointless and imo makes the
code slightly harder to follow.

Jan

> +{
> +SHADOW_PRINTK(
> +"Unexpected event (type %u, vector %#x) from emulation\n",
> +emul_ctxt.ctxt.event.type, emul_ctxt.ctxt.event.vector);
> +r = X86EMUL_UNHANDLEABLE;
> +}
> +}
>  
>  /*
>   * NB. We do not unshadow on X86EMUL_EXCEPTION. It's not clear that it



___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v4 18/24] x86/shadow: Avoid raising faults behind the emulators back

2016-12-01 Thread Andrew Cooper 
Use x86_emul_{hw_exception,pagefault}() rather than
{pv,hvm}_inject_page_fault() and hvm_inject_hw_exception() to cause raised
faults to be known to the emulator.  This requires altering the callers of
x86_emulate() to properly re-inject the event.

Signed-off-by: Andrew Cooper 
Acked-by: Tim Deegan 
---
CC: Jan Beulich 

v4:
 * Fix the commit message following v3's changes.
 * s/is_hvm_vcpu/has_hvm_container_domain/
 * Adjust description of permitted exceptions.
 * Disallow #GP/#SS with non-zero error codes.
v3:
 * Split out #DB handling to an earlier part of the series
 * Don't inject #GP faults for unexpected events, but do reenter the guest.
v2:
 * New
---
 xen/arch/x86/mm/shadow/common.c | 13 ++---
 xen/arch/x86/mm/shadow/multi.c  | 40 +---
 2 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index f07803b..e509cc1 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -162,8 +162,9 @@ static int hvm_translate_linear_addr(
 
 if ( !okay )
 {
-hvm_inject_hw_exception(
-(seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault, 0);
+x86_emul_hw_exception(
+(seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault,
+0, &sh_ctxt->ctxt);
 return X86EMUL_EXCEPTION;
 }
 
@@ -323,7 +324,7 @@ pv_emulate_read(enum x86_segment seg,
 
 if ( (rc = copy_from_user(p_data, (void *)offset, bytes)) != 0 )
 {
-pv_inject_page_fault(0, offset + bytes - rc); /* Read fault. */
+x86_emul_pagefault(0, offset + bytes - rc, ctxt); /* Read fault. */
 return X86EMUL_EXCEPTION;
 }
 
@@ -1720,10 +1721,8 @@ static mfn_t emulate_gva_to_mfn(struct vcpu *v, unsigned 
long vaddr,
 gfn = paging_get_hostmode(v)->gva_to_gfn(v, NULL, vaddr, &pfec);
 if ( gfn == gfn_x(INVALID_GFN) )
 {
-if ( is_hvm_vcpu(v) )
-hvm_inject_page_fault(pfec, vaddr);
-else
-pv_inject_page_fault(pfec, vaddr);
+x86_emul_pagefault(pfec, vaddr, &sh_ctxt->ctxt);
+
 return _mfn(BAD_GVA_TO_GFN);
 }
 
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 7d9081b..fc18d65 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3373,18 +3373,36 @@ static int sh_page_fault(struct vcpu *v,
 
 r = x86_emulate(&emul_ctxt.ctxt, emul_ops);
 
-/*
- * The previous lack of inject_{sw,hw}*() hooks caused exceptions raised
- * by the emulator itself to become X86EMUL_UNHANDLEABLE.  Such exceptions
- * now set event_pending instead.  Exceptions raised behind the back of
- * the emulator don't yet set event_pending.
- *
- * For now, cause such cases to return to the X86EMUL_UNHANDLEABLE path,
- * for no functional change from before.  Future patches will fix this
- * properly.
- */
 if ( r == X86EMUL_EXCEPTION && emul_ctxt.ctxt.event_pending )
-r = X86EMUL_UNHANDLEABLE;
+{
+/*
+ * This emulation covers writes to shadow pagetables.  We tolerate #PF
+ * (from accesses spanning pages, concurrent paging updated from
+ * vcpus, etc) and #GP[0]/#SS[0] (from segmentation errors).  Anything
+ * else is an emulation bug, or a guest playing with the instruction
+ * stream under Xen's feet.
+ */
+if ( emul_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
+ ((emul_ctxt.ctxt.event.vector == TRAP_page_fault) ||
+  (((emul_ctxt.ctxt.event.vector == TRAP_gp_fault) ||
+(emul_ctxt.ctxt.event.vector == TRAP_stack_error)) &&
+   emul_ctxt.ctxt.event.error_code == 0)) )
+{
+if ( has_hvm_container_domain(d) )
+hvm_inject_event(&emul_ctxt.ctxt.event);
+else
+pv_inject_event(&emul_ctxt.ctxt.event);
+
+goto emulate_done;
+}
+else
+{
+SHADOW_PRINTK(
+"Unexpected event (type %u, vector %#x) from emulation\n",
+emul_ctxt.ctxt.event.type, emul_ctxt.ctxt.event.vector);
+r = X86EMUL_UNHANDLEABLE;
+}
+}
 
 /*
  * NB. We do not unshadow on X86EMUL_EXCEPTION. It's not clear that it
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel