* [PATCH v2] x86: correct create_bounce_frame
@ 2017-05-04 13:35 Jan Beulich
2017-05-05 8:41 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-05-04 13:35 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Julien Grall
[-- Attachment #1: Type: text/plain, Size: 6975 bytes --]
Commit d9b7ef209a7 ("x86: drop failsafe callback invocation from
assembly") didn't go quite far enough with the cleanup it did: The
changed maximum frame size should also have been reflected in the early
address range check (which has now been pointed out to have been wrong
anyway, using 60 instead of 0x60), and it should have updated the
comment ahead of the function.
Also adjust the lower bound - all is fine (for our purposes) if the
initial guest kernel stack pointer points right at the hypervisor base
address, as only memory _below_ that address is going to be written.
Additionally limit the number of times %rsi is being adjusted to what
is really needed.
Finally move exception fixup code into the designated .fixup section.
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This corrects the code which did result in XSA-215 on Xen 4.6 and
older. For that reason I at least want to explore whether this is a
change we want to take for 4.9.
---
v2: Change domain_crash_page_fault_* tags and add comment ahead of the
labels. Convert 8(%rsi) to 1*8(%rsi) and (%rsi) to 0*8(%rsi).
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -258,7 +258,7 @@ int80_slow_path:
jmp handle_exception_saved
/* CREATE A BASIC EXCEPTION FRAME ON GUEST OS STACK: */
-/* { RCX, R11, [DS-GS,] [CR2,] [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
+/* { RCX, R11, [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
/* %rdx: trap_bounce, %rbx: struct vcpu */
/* On return only %rbx and %rdx are guaranteed non-clobbered. */
create_bounce_frame:
@@ -276,9 +276,9 @@ create_bounce_frame:
movq UREGS_rsp+8(%rsp),%rsi
andb $0xfc,UREGS_cs+8(%rsp) # Indicate kernel context to guest.
2: andq $~0xf,%rsi # Stack frames are 16-byte aligned.
- movq $HYPERVISOR_VIRT_START,%rax
+ movq $HYPERVISOR_VIRT_START+1,%rax
cmpq %rax,%rsi
- movq $HYPERVISOR_VIRT_END+60,%rax
+ movq $HYPERVISOR_VIRT_END+8*8,%rax
sbb %ecx,%ecx # In +ve address space? Then okay.
cmpq %rax,%rsi
adc %ecx,%ecx # Above Xen private area? Then okay.
@@ -286,13 +286,13 @@ UNLIKELY_START(g, create_bounce_frame_ba
lea UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_sp)(%rip), %rdi
jmp asm_domain_crash_synchronous /* Does not return */
__UNLIKELY_END(create_bounce_frame_bad_sp)
- subq $40,%rsi
+ subq $7*8,%rsi
movq UREGS_ss+8(%rsp),%rax
ASM_STAC
movq VCPU_domain(%rbx),%rdi
-.Lft2: movq %rax,32(%rsi) # SS
+.Lft2: movq %rax,6*8(%rsi) # SS
movq UREGS_rsp+8(%rsp),%rax
-.Lft3: movq %rax,24(%rsi) # RSP
+.Lft3: movq %rax,5*8(%rsi) # RSP
movq VCPU_vcpu_info(%rbx),%rax
pushq VCPUINFO_upcall_mask(%rax)
testb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
@@ -301,7 +301,7 @@ __UNLIKELY_END(create_bounce_frame_bad_s
popq %rax
shlq $32,%rax # Bits 32-39: saved_upcall_mask
movw UREGS_cs+8(%rsp),%ax # Bits 0-15: CS
-.Lft4: movq %rax,8(%rsi) # CS / saved_upcall_mask
+.Lft4: movq %rax,3*8(%rsi) # CS / saved_upcall_mask
shrq $32,%rax
testb $0xFF,%al # Bits 0-7: saved_upcall_mask
setz %ch # %ch == !saved_upcall_mask
@@ -313,20 +313,19 @@ __UNLIKELY_END(create_bounce_frame_bad_s
testb $1 << VMASST_TYPE_architectural_iopl,DOMAIN_vm_assist(%rdi)
cmovnzl VCPU_iopl(%rbx),%ecx # Bits 13:12 (EFLAGS.IOPL)
orl %ecx,%eax # Fold EFLAGS.IOPL into %eax
-.Lft5: movq %rax,16(%rsi) # RFLAGS
+.Lft5: movq %rax,4*8(%rsi) # RFLAGS
movq UREGS_rip+8(%rsp),%rax
-.Lft6: movq %rax,(%rsi) # RIP
+.Lft6: movq %rax,2*8(%rsi) # RIP
testb $TBF_EXCEPTION_ERRCODE,TRAPBOUNCE_flags(%rdx)
jz 1f
subq $8,%rsi
movl TRAPBOUNCE_error_code(%rdx),%eax
-.Lft7: movq %rax,(%rsi) # ERROR CODE
+.Lft7: movq %rax,2*8(%rsi) # ERROR CODE
1:
- subq $16,%rsi
movq UREGS_r11+8(%rsp),%rax
-.Lft12: movq %rax,8(%rsi) # R11
+.Lft12: movq %rax,1*8(%rsi) # R11
movq UREGS_rcx+8(%rsp),%rax
-.Lft13: movq %rax,(%rsi) # RCX
+.Lft13: movq %rax,0*8(%rsi) # RCX
ASM_CLAC
/* Rewrite our stack frame and return to guest-OS mode. */
/* IA32 Ref. Vol. 3: TF, VM, RF and NT flags are cleared on trap. */
@@ -345,24 +344,30 @@ UNLIKELY_START(z, create_bounce_frame_ba
__UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
movq %rax,UREGS_rip+8(%rsp)
ret
- _ASM_EXTABLE(.Lft2, domain_crash_page_fault_32)
- _ASM_EXTABLE(.Lft3, domain_crash_page_fault_24)
- _ASM_EXTABLE(.Lft4, domain_crash_page_fault_8)
- _ASM_EXTABLE(.Lft5, domain_crash_page_fault_16)
- _ASM_EXTABLE(.Lft6, domain_crash_page_fault)
- _ASM_EXTABLE(.Lft7, domain_crash_page_fault)
- _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
- _ASM_EXTABLE(.Lft13, domain_crash_page_fault)
+ _ASM_EXTABLE(.Lft2, domain_crash_page_fault_6x8)
+ _ASM_EXTABLE(.Lft3, domain_crash_page_fault_5x8)
+ _ASM_EXTABLE(.Lft4, domain_crash_page_fault_4x8)
+ _ASM_EXTABLE(.Lft5, domain_crash_page_fault_3x8)
+ _ASM_EXTABLE(.Lft6, domain_crash_page_fault_2x8)
+ _ASM_EXTABLE(.Lft7, domain_crash_page_fault_2x8)
+ _ASM_EXTABLE(.Lft12, domain_crash_page_fault_1x8)
+ _ASM_EXTABLE(.Lft13, domain_crash_page_fault_0x8)
-domain_crash_page_fault_32:
+ .pushsection .fixup, "ax", @progbits
+ # Numeric tags below represent the intended overall %rsi adjustment.
+domain_crash_page_fault_6x8:
addq $8,%rsi
-domain_crash_page_fault_24:
+domain_crash_page_fault_5x8:
addq $8,%rsi
-domain_crash_page_fault_16:
+domain_crash_page_fault_4x8:
addq $8,%rsi
-domain_crash_page_fault_8:
+domain_crash_page_fault_3x8:
addq $8,%rsi
-domain_crash_page_fault:
+domain_crash_page_fault_2x8:
+ addq $8,%rsi
+domain_crash_page_fault_1x8:
+ addq $8,%rsi
+domain_crash_page_fault_0x8:
ASM_CLAC
movq %rsi,%rdi
call show_page_walk
@@ -380,6 +385,7 @@ ENTRY(dom_crash_sync_extable)
orb %al,UREGS_cs(%rsp)
xorl %edi,%edi
jmp asm_domain_crash_synchronous /* Does not return */
+ .popsection
ENTRY(common_interrupt)
SAVE_ALL CLAC
[-- Attachment #2: x86-bounce-frame-size.patch --]
[-- Type: text/plain, Size: 7007 bytes --]
x86: correct create_bounce_frame
Commit d9b7ef209a7 ("x86: drop failsafe callback invocation from
assembly") didn't go quite far enough with the cleanup it did: The
changed maximum frame size should also have been reflected in the early
address range check (which has now been pointed out to have been wrong
anyway, using 60 instead of 0x60), and it should have updated the
comment ahead of the function.
Also adjust the lower bound - all is fine (for our purposes) if the
initial guest kernel stack pointer points right at the hypervisor base
address, as only memory _below_ that address is going to be written.
Additionally limit the number of times %rsi is being adjusted to what
is really needed.
Finally move exception fixup code into the designated .fixup section.
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This corrects the code which did result in XSA-215 on Xen 4.6 and
older. For that reason I at least want to explore whether this is a
change we want to take for 4.9.
---
v2: Change domain_crash_page_fault_* tags and add comment ahead of the
labels. Convert 8(%rsi) to 1*8(%rsi) and (%rsi) to 0*8(%rsi).
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -258,7 +258,7 @@ int80_slow_path:
jmp handle_exception_saved
/* CREATE A BASIC EXCEPTION FRAME ON GUEST OS STACK: */
-/* { RCX, R11, [DS-GS,] [CR2,] [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
+/* { RCX, R11, [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
/* %rdx: trap_bounce, %rbx: struct vcpu */
/* On return only %rbx and %rdx are guaranteed non-clobbered. */
create_bounce_frame:
@@ -276,9 +276,9 @@ create_bounce_frame:
movq UREGS_rsp+8(%rsp),%rsi
andb $0xfc,UREGS_cs+8(%rsp) # Indicate kernel context to guest.
2: andq $~0xf,%rsi # Stack frames are 16-byte aligned.
- movq $HYPERVISOR_VIRT_START,%rax
+ movq $HYPERVISOR_VIRT_START+1,%rax
cmpq %rax,%rsi
- movq $HYPERVISOR_VIRT_END+60,%rax
+ movq $HYPERVISOR_VIRT_END+8*8,%rax
sbb %ecx,%ecx # In +ve address space? Then okay.
cmpq %rax,%rsi
adc %ecx,%ecx # Above Xen private area? Then okay.
@@ -286,13 +286,13 @@ UNLIKELY_START(g, create_bounce_frame_ba
lea UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_sp)(%rip), %rdi
jmp asm_domain_crash_synchronous /* Does not return */
__UNLIKELY_END(create_bounce_frame_bad_sp)
- subq $40,%rsi
+ subq $7*8,%rsi
movq UREGS_ss+8(%rsp),%rax
ASM_STAC
movq VCPU_domain(%rbx),%rdi
-.Lft2: movq %rax,32(%rsi) # SS
+.Lft2: movq %rax,6*8(%rsi) # SS
movq UREGS_rsp+8(%rsp),%rax
-.Lft3: movq %rax,24(%rsi) # RSP
+.Lft3: movq %rax,5*8(%rsi) # RSP
movq VCPU_vcpu_info(%rbx),%rax
pushq VCPUINFO_upcall_mask(%rax)
testb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
@@ -301,7 +301,7 @@ __UNLIKELY_END(create_bounce_frame_bad_s
popq %rax
shlq $32,%rax # Bits 32-39: saved_upcall_mask
movw UREGS_cs+8(%rsp),%ax # Bits 0-15: CS
-.Lft4: movq %rax,8(%rsi) # CS / saved_upcall_mask
+.Lft4: movq %rax,3*8(%rsi) # CS / saved_upcall_mask
shrq $32,%rax
testb $0xFF,%al # Bits 0-7: saved_upcall_mask
setz %ch # %ch == !saved_upcall_mask
@@ -313,20 +313,19 @@ __UNLIKELY_END(create_bounce_frame_bad_s
testb $1 << VMASST_TYPE_architectural_iopl,DOMAIN_vm_assist(%rdi)
cmovnzl VCPU_iopl(%rbx),%ecx # Bits 13:12 (EFLAGS.IOPL)
orl %ecx,%eax # Fold EFLAGS.IOPL into %eax
-.Lft5: movq %rax,16(%rsi) # RFLAGS
+.Lft5: movq %rax,4*8(%rsi) # RFLAGS
movq UREGS_rip+8(%rsp),%rax
-.Lft6: movq %rax,(%rsi) # RIP
+.Lft6: movq %rax,2*8(%rsi) # RIP
testb $TBF_EXCEPTION_ERRCODE,TRAPBOUNCE_flags(%rdx)
jz 1f
subq $8,%rsi
movl TRAPBOUNCE_error_code(%rdx),%eax
-.Lft7: movq %rax,(%rsi) # ERROR CODE
+.Lft7: movq %rax,2*8(%rsi) # ERROR CODE
1:
- subq $16,%rsi
movq UREGS_r11+8(%rsp),%rax
-.Lft12: movq %rax,8(%rsi) # R11
+.Lft12: movq %rax,1*8(%rsi) # R11
movq UREGS_rcx+8(%rsp),%rax
-.Lft13: movq %rax,(%rsi) # RCX
+.Lft13: movq %rax,0*8(%rsi) # RCX
ASM_CLAC
/* Rewrite our stack frame and return to guest-OS mode. */
/* IA32 Ref. Vol. 3: TF, VM, RF and NT flags are cleared on trap. */
@@ -345,24 +344,30 @@ UNLIKELY_START(z, create_bounce_frame_ba
__UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
movq %rax,UREGS_rip+8(%rsp)
ret
- _ASM_EXTABLE(.Lft2, domain_crash_page_fault_32)
- _ASM_EXTABLE(.Lft3, domain_crash_page_fault_24)
- _ASM_EXTABLE(.Lft4, domain_crash_page_fault_8)
- _ASM_EXTABLE(.Lft5, domain_crash_page_fault_16)
- _ASM_EXTABLE(.Lft6, domain_crash_page_fault)
- _ASM_EXTABLE(.Lft7, domain_crash_page_fault)
- _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
- _ASM_EXTABLE(.Lft13, domain_crash_page_fault)
+ _ASM_EXTABLE(.Lft2, domain_crash_page_fault_6x8)
+ _ASM_EXTABLE(.Lft3, domain_crash_page_fault_5x8)
+ _ASM_EXTABLE(.Lft4, domain_crash_page_fault_4x8)
+ _ASM_EXTABLE(.Lft5, domain_crash_page_fault_3x8)
+ _ASM_EXTABLE(.Lft6, domain_crash_page_fault_2x8)
+ _ASM_EXTABLE(.Lft7, domain_crash_page_fault_2x8)
+ _ASM_EXTABLE(.Lft12, domain_crash_page_fault_1x8)
+ _ASM_EXTABLE(.Lft13, domain_crash_page_fault_0x8)
-domain_crash_page_fault_32:
+ .pushsection .fixup, "ax", @progbits
+ # Numeric tags below represent the intended overall %rsi adjustment.
+domain_crash_page_fault_6x8:
addq $8,%rsi
-domain_crash_page_fault_24:
+domain_crash_page_fault_5x8:
addq $8,%rsi
-domain_crash_page_fault_16:
+domain_crash_page_fault_4x8:
addq $8,%rsi
-domain_crash_page_fault_8:
+domain_crash_page_fault_3x8:
addq $8,%rsi
-domain_crash_page_fault:
+domain_crash_page_fault_2x8:
+ addq $8,%rsi
+domain_crash_page_fault_1x8:
+ addq $8,%rsi
+domain_crash_page_fault_0x8:
ASM_CLAC
movq %rsi,%rdi
call show_page_walk
@@ -380,6 +385,7 @@ ENTRY(dom_crash_sync_extable)
orb %al,UREGS_cs(%rsp)
xorl %edi,%edi
jmp asm_domain_crash_synchronous /* Does not return */
+ .popsection
ENTRY(common_interrupt)
SAVE_ALL CLAC
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86: correct create_bounce_frame
2017-05-04 13:35 [PATCH v2] x86: correct create_bounce_frame Jan Beulich
@ 2017-05-05 8:41 ` Andrew Cooper
2017-05-05 9:38 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2017-05-05 8:41 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Julien Grall
On 04/05/17 14:35, Jan Beulich wrote:
> Commit d9b7ef209a7 ("x86: drop failsafe callback invocation from
> assembly") didn't go quite far enough with the cleanup it did: The
> changed maximum frame size should also have been reflected in the early
> address range check (which has now been pointed out to have been wrong
> anyway, using 60 instead of 0x60), and it should have updated the
> comment ahead of the function.
>
> Also adjust the lower bound - all is fine (for our purposes) if the
> initial guest kernel stack pointer points right at the hypervisor base
> address, as only memory _below_ that address is going to be written.
>
> Additionally limit the number of times %rsi is being adjusted to what
> is really needed.
>
> Finally move exception fixup code into the designated .fixup section.
>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Thankyou. This code is now much easier to review. On that note,
> ---
> This corrects the code which did result in XSA-215 on Xen 4.6 and
> older. For that reason I at least want to explore whether this is a
> change we want to take for 4.9.
> ---
> v2: Change domain_crash_page_fault_* tags and add comment ahead of the
> labels. Convert 8(%rsi) to 1*8(%rsi) and (%rsi) to 0*8(%rsi).
>
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -258,7 +258,7 @@ int80_slow_path:
> jmp handle_exception_saved
>
> /* CREATE A BASIC EXCEPTION FRAME ON GUEST OS STACK: */
> -/* { RCX, R11, [DS-GS,] [CR2,] [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
> +/* { RCX, R11, [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
> /* %rdx: trap_bounce, %rbx: struct vcpu */
> /* On return only %rbx and %rdx are guaranteed non-clobbered. */
> create_bounce_frame:
> @@ -276,9 +276,9 @@ create_bounce_frame:
> movq UREGS_rsp+8(%rsp),%rsi
> andb $0xfc,UREGS_cs+8(%rsp) # Indicate kernel context to guest.
> 2: andq $~0xf,%rsi # Stack frames are 16-byte aligned.
> - movq $HYPERVISOR_VIRT_START,%rax
> + movq $HYPERVISOR_VIRT_START+1,%rax
> cmpq %rax,%rsi
> - movq $HYPERVISOR_VIRT_END+60,%rax
> + movq $HYPERVISOR_VIRT_END+8*8,%rax
> sbb %ecx,%ecx # In +ve address space? Then okay.
> cmpq %rax,%rsi
> adc %ecx,%ecx # Above Xen private area? Then okay.
> @@ -286,13 +286,13 @@ UNLIKELY_START(g, create_bounce_frame_ba
> lea UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_sp)(%rip), %rdi
> jmp asm_domain_crash_synchronous /* Does not return */
> __UNLIKELY_END(create_bounce_frame_bad_sp)
> - subq $40,%rsi
> + subq $7*8,%rsi
> movq UREGS_ss+8(%rsp),%rax
> ASM_STAC
> movq VCPU_domain(%rbx),%rdi
> -.Lft2: movq %rax,32(%rsi) # SS
> +.Lft2: movq %rax,6*8(%rsi) # SS
> movq UREGS_rsp+8(%rsp),%rax
> -.Lft3: movq %rax,24(%rsi) # RSP
> +.Lft3: movq %rax,5*8(%rsi) # RSP
> movq VCPU_vcpu_info(%rbx),%rax
> pushq VCPUINFO_upcall_mask(%rax)
> testb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
> @@ -301,7 +301,7 @@ __UNLIKELY_END(create_bounce_frame_bad_s
> popq %rax
> shlq $32,%rax # Bits 32-39: saved_upcall_mask
> movw UREGS_cs+8(%rsp),%ax # Bits 0-15: CS
> -.Lft4: movq %rax,8(%rsi) # CS / saved_upcall_mask
> +.Lft4: movq %rax,3*8(%rsi) # CS / saved_upcall_mask
> shrq $32,%rax
> testb $0xFF,%al # Bits 0-7: saved_upcall_mask
> setz %ch # %ch == !saved_upcall_mask
> @@ -313,20 +313,19 @@ __UNLIKELY_END(create_bounce_frame_bad_s
> testb $1 << VMASST_TYPE_architectural_iopl,DOMAIN_vm_assist(%rdi)
> cmovnzl VCPU_iopl(%rbx),%ecx # Bits 13:12 (EFLAGS.IOPL)
> orl %ecx,%eax # Fold EFLAGS.IOPL into %eax
> -.Lft5: movq %rax,16(%rsi) # RFLAGS
> +.Lft5: movq %rax,4*8(%rsi) # RFLAGS
> movq UREGS_rip+8(%rsp),%rax
> -.Lft6: movq %rax,(%rsi) # RIP
> +.Lft6: movq %rax,2*8(%rsi) # RIP
> testb $TBF_EXCEPTION_ERRCODE,TRAPBOUNCE_flags(%rdx)
> jz 1f
> subq $8,%rsi
> movl TRAPBOUNCE_error_code(%rdx),%eax
> -.Lft7: movq %rax,(%rsi) # ERROR CODE
> +.Lft7: movq %rax,2*8(%rsi) # ERROR CODE
> 1:
> - subq $16,%rsi
> movq UREGS_r11+8(%rsp),%rax
> -.Lft12: movq %rax,8(%rsi) # R11
> +.Lft12: movq %rax,1*8(%rsi) # R11
> movq UREGS_rcx+8(%rsp),%rax
> -.Lft13: movq %rax,(%rsi) # RCX
> +.Lft13: movq %rax,0*8(%rsi) # RCX
> ASM_CLAC
> /* Rewrite our stack frame and return to guest-OS mode. */
> /* IA32 Ref. Vol. 3: TF, VM, RF and NT flags are cleared on trap. */
> @@ -345,24 +344,30 @@ UNLIKELY_START(z, create_bounce_frame_ba
> __UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
> movq %rax,UREGS_rip+8(%rsp)
> ret
> - _ASM_EXTABLE(.Lft2, domain_crash_page_fault_32)
> - _ASM_EXTABLE(.Lft3, domain_crash_page_fault_24)
> - _ASM_EXTABLE(.Lft4, domain_crash_page_fault_8)
> - _ASM_EXTABLE(.Lft5, domain_crash_page_fault_16)
> - _ASM_EXTABLE(.Lft6, domain_crash_page_fault)
> - _ASM_EXTABLE(.Lft7, domain_crash_page_fault)
> - _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
> - _ASM_EXTABLE(.Lft13, domain_crash_page_fault)
> + _ASM_EXTABLE(.Lft2, domain_crash_page_fault_6x8)
> + _ASM_EXTABLE(.Lft3, domain_crash_page_fault_5x8)
> + _ASM_EXTABLE(.Lft4, domain_crash_page_fault_4x8)
> + _ASM_EXTABLE(.Lft5, domain_crash_page_fault_3x8)
Do you perhaps mean to swap the labels for 4 and 5?
~Andrew
> + _ASM_EXTABLE(.Lft6, domain_crash_page_fault_2x8)
> + _ASM_EXTABLE(.Lft7, domain_crash_page_fault_2x8)
> + _ASM_EXTABLE(.Lft12, domain_crash_page_fault_1x8)
> + _ASM_EXTABLE(.Lft13, domain_crash_page_fault_0x8)
>
> -domain_crash_page_fault_32:
> + .pushsection .fixup, "ax", @progbits
> + # Numeric tags below represent the intended overall %rsi adjustment.
> +domain_crash_page_fault_6x8:
> addq $8,%rsi
> -domain_crash_page_fault_24:
> +domain_crash_page_fault_5x8:
> addq $8,%rsi
> -domain_crash_page_fault_16:
> +domain_crash_page_fault_4x8:
> addq $8,%rsi
> -domain_crash_page_fault_8:
> +domain_crash_page_fault_3x8:
> addq $8,%rsi
> -domain_crash_page_fault:
> +domain_crash_page_fault_2x8:
> + addq $8,%rsi
> +domain_crash_page_fault_1x8:
> + addq $8,%rsi
> +domain_crash_page_fault_0x8:
> ASM_CLAC
> movq %rsi,%rdi
> call show_page_walk
> @@ -380,6 +385,7 @@ ENTRY(dom_crash_sync_extable)
> orb %al,UREGS_cs(%rsp)
> xorl %edi,%edi
> jmp asm_domain_crash_synchronous /* Does not return */
> + .popsection
>
> ENTRY(common_interrupt)
> SAVE_ALL CLAC
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86: correct create_bounce_frame
2017-05-05 8:41 ` Andrew Cooper
@ 2017-05-05 9:38 ` Jan Beulich
2017-05-05 10:18 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-05-05 9:38 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Julien Grall
>>> On 05.05.17 at 10:41, <andrew.cooper3@citrix.com> wrote:
> On 04/05/17 14:35, Jan Beulich wrote:
>> @@ -345,24 +344,30 @@ UNLIKELY_START(z, create_bounce_frame_ba
>> __UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
>> movq %rax,UREGS_rip+8(%rsp)
>> ret
>> - _ASM_EXTABLE(.Lft2, domain_crash_page_fault_32)
>> - _ASM_EXTABLE(.Lft3, domain_crash_page_fault_24)
>> - _ASM_EXTABLE(.Lft4, domain_crash_page_fault_8)
>> - _ASM_EXTABLE(.Lft5, domain_crash_page_fault_16)
>> - _ASM_EXTABLE(.Lft6, domain_crash_page_fault)
>> - _ASM_EXTABLE(.Lft7, domain_crash_page_fault)
>> - _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
>> - _ASM_EXTABLE(.Lft13, domain_crash_page_fault)
>> + _ASM_EXTABLE(.Lft2, domain_crash_page_fault_6x8)
>> + _ASM_EXTABLE(.Lft3, domain_crash_page_fault_5x8)
>> + _ASM_EXTABLE(.Lft4, domain_crash_page_fault_4x8)
>> + _ASM_EXTABLE(.Lft5, domain_crash_page_fault_3x8)
>
> Do you perhaps mean to swap the labels for 4 and 5?
Oh, yes, of course. I remember that something was odd when
doing the conversion, but I wasn't able to spot it because things
looked well ordered.
Having made a mistake like this I wonder whether it wouldn't be
better to move these next to their instructions anyway? The
way we build them now both insn and extable generation could
actually be bundled into a macro, eliminating any risk for the
two parts to go out of sync ...
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86: correct create_bounce_frame
2017-05-05 9:38 ` Jan Beulich
@ 2017-05-05 10:18 ` Andrew Cooper
2017-05-05 10:24 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2017-05-05 10:18 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Julien Grall
On 05/05/17 10:38, Jan Beulich wrote:
>>>> On 05.05.17 at 10:41, <andrew.cooper3@citrix.com> wrote:
>> On 04/05/17 14:35, Jan Beulich wrote:
>>> @@ -345,24 +344,30 @@ UNLIKELY_START(z, create_bounce_frame_ba
>>> __UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
>>> movq %rax,UREGS_rip+8(%rsp)
>>> ret
>>> - _ASM_EXTABLE(.Lft2, domain_crash_page_fault_32)
>>> - _ASM_EXTABLE(.Lft3, domain_crash_page_fault_24)
>>> - _ASM_EXTABLE(.Lft4, domain_crash_page_fault_8)
>>> - _ASM_EXTABLE(.Lft5, domain_crash_page_fault_16)
>>> - _ASM_EXTABLE(.Lft6, domain_crash_page_fault)
>>> - _ASM_EXTABLE(.Lft7, domain_crash_page_fault)
>>> - _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
>>> - _ASM_EXTABLE(.Lft13, domain_crash_page_fault)
>>> + _ASM_EXTABLE(.Lft2, domain_crash_page_fault_6x8)
>>> + _ASM_EXTABLE(.Lft3, domain_crash_page_fault_5x8)
>>> + _ASM_EXTABLE(.Lft4, domain_crash_page_fault_4x8)
>>> + _ASM_EXTABLE(.Lft5, domain_crash_page_fault_3x8)
>> Do you perhaps mean to swap the labels for 4 and 5?
> Oh, yes, of course. I remember that something was odd when
> doing the conversion, but I wasn't able to spot it because things
> looked well ordered.
>
> Having made a mistake like this I wonder whether it wouldn't be
> better to move these next to their instructions anyway? The
> way we build them now both insn and extable generation could
> actually be bundled into a macro, eliminating any risk for the
> two parts to go out of sync ...
Do you see such a construct being used anywhere else? I can't spot any
obvious candidates.
For post 4.9, I will be submitting a series which reimplements this
logic in C, so I wouldn't expend too much effort in this area.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86: correct create_bounce_frame
2017-05-05 10:18 ` Andrew Cooper
@ 2017-05-05 10:24 ` Jan Beulich
2017-05-05 10:29 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-05-05 10:24 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Julien Grall
>>> On 05.05.17 at 12:18, <andrew.cooper3@citrix.com> wrote:
> On 05/05/17 10:38, Jan Beulich wrote:
>>>>> On 05.05.17 at 10:41, <andrew.cooper3@citrix.com> wrote:
>>> On 04/05/17 14:35, Jan Beulich wrote:
>>>> @@ -345,24 +344,30 @@ UNLIKELY_START(z, create_bounce_frame_ba
>>>> __UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
>>>> movq %rax,UREGS_rip+8(%rsp)
>>>> ret
>>>> - _ASM_EXTABLE(.Lft2, domain_crash_page_fault_32)
>>>> - _ASM_EXTABLE(.Lft3, domain_crash_page_fault_24)
>>>> - _ASM_EXTABLE(.Lft4, domain_crash_page_fault_8)
>>>> - _ASM_EXTABLE(.Lft5, domain_crash_page_fault_16)
>>>> - _ASM_EXTABLE(.Lft6, domain_crash_page_fault)
>>>> - _ASM_EXTABLE(.Lft7, domain_crash_page_fault)
>>>> - _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
>>>> - _ASM_EXTABLE(.Lft13, domain_crash_page_fault)
>>>> + _ASM_EXTABLE(.Lft2, domain_crash_page_fault_6x8)
>>>> + _ASM_EXTABLE(.Lft3, domain_crash_page_fault_5x8)
>>>> + _ASM_EXTABLE(.Lft4, domain_crash_page_fault_4x8)
>>>> + _ASM_EXTABLE(.Lft5, domain_crash_page_fault_3x8)
>>> Do you perhaps mean to swap the labels for 4 and 5?
>> Oh, yes, of course. I remember that something was odd when
>> doing the conversion, but I wasn't able to spot it because things
>> looked well ordered.
>>
>> Having made a mistake like this I wonder whether it wouldn't be
>> better to move these next to their instructions anyway? The
>> way we build them now both insn and extable generation could
>> actually be bundled into a macro, eliminating any risk for the
>> two parts to go out of sync ...
>
> Do you see such a construct being used anywhere else? I can't spot any
> obvious candidates.
No, just here. But it's eight places.
> For post 4.9, I will be submitting a series which reimplements this
> logic in C, so I wouldn't expend too much effort in this area.
True, yet I'm touching all these lines now anyway... Would you
be opposed to moving them and/or macroizing the whole thing?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86: correct create_bounce_frame
2017-05-05 10:24 ` Jan Beulich
@ 2017-05-05 10:29 ` Andrew Cooper
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2017-05-05 10:29 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Julien Grall
On 05/05/17 11:24, Jan Beulich wrote:
>>>> On 05.05.17 at 12:18, <andrew.cooper3@citrix.com> wrote:
>> On 05/05/17 10:38, Jan Beulich wrote:
>>>>>> On 05.05.17 at 10:41, <andrew.cooper3@citrix.com> wrote:
>>>> On 04/05/17 14:35, Jan Beulich wrote:
>>>>> @@ -345,24 +344,30 @@ UNLIKELY_START(z, create_bounce_frame_ba
>>>>> __UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
>>>>> movq %rax,UREGS_rip+8(%rsp)
>>>>> ret
>>>>> - _ASM_EXTABLE(.Lft2, domain_crash_page_fault_32)
>>>>> - _ASM_EXTABLE(.Lft3, domain_crash_page_fault_24)
>>>>> - _ASM_EXTABLE(.Lft4, domain_crash_page_fault_8)
>>>>> - _ASM_EXTABLE(.Lft5, domain_crash_page_fault_16)
>>>>> - _ASM_EXTABLE(.Lft6, domain_crash_page_fault)
>>>>> - _ASM_EXTABLE(.Lft7, domain_crash_page_fault)
>>>>> - _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
>>>>> - _ASM_EXTABLE(.Lft13, domain_crash_page_fault)
>>>>> + _ASM_EXTABLE(.Lft2, domain_crash_page_fault_6x8)
>>>>> + _ASM_EXTABLE(.Lft3, domain_crash_page_fault_5x8)
>>>>> + _ASM_EXTABLE(.Lft4, domain_crash_page_fault_4x8)
>>>>> + _ASM_EXTABLE(.Lft5, domain_crash_page_fault_3x8)
>>>> Do you perhaps mean to swap the labels for 4 and 5?
>>> Oh, yes, of course. I remember that something was odd when
>>> doing the conversion, but I wasn't able to spot it because things
>>> looked well ordered.
>>>
>>> Having made a mistake like this I wonder whether it wouldn't be
>>> better to move these next to their instructions anyway? The
>>> way we build them now both insn and extable generation could
>>> actually be bundled into a macro, eliminating any risk for the
>>> two parts to go out of sync ...
>> Do you see such a construct being used anywhere else? I can't spot any
>> obvious candidates.
> No, just here. But it's eight places.
>
>> For post 4.9, I will be submitting a series which reimplements this
>> logic in C, so I wouldn't expend too much effort in this area.
> True, yet I'm touching all these lines now anyway... Would you
> be opposed to moving them and/or macroizing the whole thing?
Moving the _ASM_EXTABLE() labels is quick and looks like a good thing to do.
Your call on whether macroizing the access is worth the effort or not.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-05 10:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-04 13:35 [PATCH v2] x86: correct create_bounce_frame Jan Beulich
2017-05-05 8:41 ` Andrew Cooper
2017-05-05 9:38 ` Jan Beulich
2017-05-05 10:18 ` Andrew Cooper
2017-05-05 10:24 ` Jan Beulich
2017-05-05 10:29 ` Andrew Cooper
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).