* [PATCH] x86: correct create_bounce_frame
@ 2017-05-02 13:22 Jan Beulich
2017-05-02 14:12 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-05-02 13:22 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Julien Grall
[-- Attachment #1: Type: text/plain, Size: 5924 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.
--- 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,16 +313,15 @@ __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
movq UREGS_rcx+8(%rsp),%rax
@@ -345,15 +344,20 @@ 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(.Lft2, domain_crash_page_fault_48)
+ _ASM_EXTABLE(.Lft3, domain_crash_page_fault_40)
+ _ASM_EXTABLE(.Lft4, domain_crash_page_fault_24)
+ _ASM_EXTABLE(.Lft5, domain_crash_page_fault_32)
+ _ASM_EXTABLE(.Lft6, domain_crash_page_fault_16)
+ _ASM_EXTABLE(.Lft7, domain_crash_page_fault_16)
_ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
_ASM_EXTABLE(.Lft13, domain_crash_page_fault)
+ .pushsection .fixup, "ax", @progbits
+domain_crash_page_fault_48:
+ addq $8,%rsi
+domain_crash_page_fault_40:
+ addq $8,%rsi
domain_crash_page_fault_32:
addq $8,%rsi
domain_crash_page_fault_24:
@@ -380,6 +384,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: 5956 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.
--- 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,16 +313,15 @@ __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
movq UREGS_rcx+8(%rsp),%rax
@@ -345,15 +344,20 @@ 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(.Lft2, domain_crash_page_fault_48)
+ _ASM_EXTABLE(.Lft3, domain_crash_page_fault_40)
+ _ASM_EXTABLE(.Lft4, domain_crash_page_fault_24)
+ _ASM_EXTABLE(.Lft5, domain_crash_page_fault_32)
+ _ASM_EXTABLE(.Lft6, domain_crash_page_fault_16)
+ _ASM_EXTABLE(.Lft7, domain_crash_page_fault_16)
_ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
_ASM_EXTABLE(.Lft13, domain_crash_page_fault)
+ .pushsection .fixup, "ax", @progbits
+domain_crash_page_fault_48:
+ addq $8,%rsi
+domain_crash_page_fault_40:
+ addq $8,%rsi
domain_crash_page_fault_32:
addq $8,%rsi
domain_crash_page_fault_24:
@@ -380,6 +384,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] x86: correct create_bounce_frame
2017-05-02 13:22 [PATCH] x86: correct create_bounce_frame Jan Beulich
@ 2017-05-02 14:12 ` Andrew Cooper
2017-05-02 14:33 ` Jan Beulich
2017-05-03 15:42 ` Jan Beulich
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Cooper @ 2017-05-02 14:12 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Julien Grall
On 02/05/17 14:22, 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.
I'm not sure this a useful change to make. HYPERVISOR_VIRT_START has at
minimum page alignment, and we have just aligned %rsi down to a 16 byte
boundary.
>
> 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.
I think it is certainly worth taking, if only to correct the comment.
>
> --- 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,16 +313,15 @@ __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
For consistency, this should become movq %rax, 1*8(%rsi) and later
0*8(%rsi) for .Lft13, so all deltas from (%rsi) are expressed in terms
of slots.
> movq UREGS_rcx+8(%rsp),%rax
> @@ -345,15 +344,20 @@ 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(.Lft2, domain_crash_page_fault_48)
> + _ASM_EXTABLE(.Lft3, domain_crash_page_fault_40)
> + _ASM_EXTABLE(.Lft4, domain_crash_page_fault_24)
> + _ASM_EXTABLE(.Lft5, domain_crash_page_fault_32)
> + _ASM_EXTABLE(.Lft6, domain_crash_page_fault_16)
> + _ASM_EXTABLE(.Lft7, domain_crash_page_fault_16)
> _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
Given that you have altered the notation for a delta from (%rsi) (which
is a good improvement), these labels should be similarly altered for
consistency.
~Andrew
> _ASM_EXTABLE(.Lft13, domain_crash_page_fault)
>
> + .pushsection .fixup, "ax", @progbits
> +domain_crash_page_fault_48:
> + addq $8,%rsi
> +domain_crash_page_fault_40:
> + addq $8,%rsi
> domain_crash_page_fault_32:
> addq $8,%rsi
> domain_crash_page_fault_24:
> @@ -380,6 +384,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] x86: correct create_bounce_frame
2017-05-02 14:12 ` Andrew Cooper
@ 2017-05-02 14:33 ` Jan Beulich
2017-05-03 15:42 ` Jan Beulich
1 sibling, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2017-05-02 14:33 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Julien Grall
>>> On 02.05.17 at 16:12, <andrew.cooper3@citrix.com> wrote:
> On 02/05/17 14:22, 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.
>
> I'm not sure this a useful change to make. HYPERVISOR_VIRT_START has at
> minimum page alignment, and we have just aligned %rsi down to a 16 byte
> boundary.
I view this as mainly making sure it documents itself. And with what
you say the adjustment may still make sense: The aligned down %rsi
may be HYPERVISOR_VIRT_START, which is okay (for our purposes),
but would be rejected by the old code. In the end what exactly is
being used here would only matter if the addresses immediately
below HYPERVISOR_VIRT_START were accessible, i.e. if that wouldn't
sit exactly on the canonical boundary.
>> @@ -313,16 +313,15 @@ __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
>
> For consistency, this should become movq %rax, 1*8(%rsi) and later
> 0*8(%rsi) for .Lft13, so all deltas from (%rsi) are expressed in terms
> of slots.
I did consider this, and specifically left it untouched. Anyone
reading this code should understand that 1*8=8 and 0*8=0.
While we certainly don't care much about old gas, there was a
time when a non-blank displacement actually resulted in mode
1 (with an 8-bit displacement of zero) being used instead of
mode 0.
>> @@ -345,15 +344,20 @@ 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(.Lft2, domain_crash_page_fault_48)
>> + _ASM_EXTABLE(.Lft3, domain_crash_page_fault_40)
>> + _ASM_EXTABLE(.Lft4, domain_crash_page_fault_24)
>> + _ASM_EXTABLE(.Lft5, domain_crash_page_fault_32)
>> + _ASM_EXTABLE(.Lft6, domain_crash_page_fault_16)
>> + _ASM_EXTABLE(.Lft7, domain_crash_page_fault_16)
>> _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
>
> Given that you have altered the notation for a delta from (%rsi) (which
> is a good improvement), these labels should be similarly altered for
> consistency.
And I did consider this too - I don't want to use '*' in label names
(only relatively recent gas knows to consistently deal quoted
symbol names), and I also don't want to use _2by8 or some such.
I could probably be talked into using the slot count alone here,
but would that really make much of a difference?
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] x86: correct create_bounce_frame
2017-05-02 14:12 ` Andrew Cooper
2017-05-02 14:33 ` Jan Beulich
@ 2017-05-03 15:42 ` Jan Beulich
2017-05-03 18:58 ` Andrew Cooper
1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-05-03 15:42 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Julien Grall
>>> On 02.05.17 at 16:12, <andrew.cooper3@citrix.com> wrote:
> On 02/05/17 14:22, Jan Beulich wrote:
>> @@ -345,15 +344,20 @@ 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(.Lft2, domain_crash_page_fault_48)
>> + _ASM_EXTABLE(.Lft3, domain_crash_page_fault_40)
>> + _ASM_EXTABLE(.Lft4, domain_crash_page_fault_24)
>> + _ASM_EXTABLE(.Lft5, domain_crash_page_fault_32)
>> + _ASM_EXTABLE(.Lft6, domain_crash_page_fault_16)
>> + _ASM_EXTABLE(.Lft7, domain_crash_page_fault_16)
>> _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
>
> Given that you have altered the notation for a delta from (%rsi) (which
> is a good improvement), these labels should be similarly altered for
> consistency.
Actually, looking at this again I'm no longer sure factoring out the
8 here would be a good idea. I'm also not sure I understand you
saying "Given that you have altered the notation for a delta from
(%rsi)" - the notation didn't change at all, the numeric tag has
always been expressing the adjustment needed for %rsi to
represent the actual failing memory address.
So together with my earlier reply I'm not of the opinion that any
of the adjustments you ask for are actually warranted.
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] x86: correct create_bounce_frame
2017-05-03 15:42 ` Jan Beulich
@ 2017-05-03 18:58 ` Andrew Cooper
2017-05-04 7:41 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2017-05-03 18:58 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Julien Grall
On 03/05/17 16:42, Jan Beulich wrote:
>>>> On 02.05.17 at 16:12, <andrew.cooper3@citrix.com> wrote:
>> On 02/05/17 14:22, Jan Beulich wrote:
>>> @@ -345,15 +344,20 @@ 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(.Lft2, domain_crash_page_fault_48)
>>> + _ASM_EXTABLE(.Lft3, domain_crash_page_fault_40)
>>> + _ASM_EXTABLE(.Lft4, domain_crash_page_fault_24)
>>> + _ASM_EXTABLE(.Lft5, domain_crash_page_fault_32)
>>> + _ASM_EXTABLE(.Lft6, domain_crash_page_fault_16)
>>> + _ASM_EXTABLE(.Lft7, domain_crash_page_fault_16)
>>> _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
>> Given that you have altered the notation for a delta from (%rsi) (which
>> is a good improvement), these labels should be similarly altered for
>> consistency.
> Actually, looking at this again I'm no longer sure factoring out the
> 8 here would be a good idea. I'm also not sure I understand you
> saying "Given that you have altered the notation for a delta from
> (%rsi)" - the notation didn't change at all, the numeric tag has
> always been expressing the adjustment needed for %rsi to
> represent the actual failing memory address.
First of all, this point is not obvious at all to people reading the
code who don't already know the meaning. If it were new code today, I'd
insist on a comment explaining what the numeric tag means.
As for the change in notation, you very much have changed it. You have
factored 8 out of it, which visually emphases the number of stack slots
away from (%rsi) is used.
The change in notation is, in principle, a good thing because it makes
the code easier to read and correlate with the comment.
However, such a change is only an improvement if it is implemented
consistently (hence the request to use 1*8 and 0*8 for the other (%rsi)
references), and if it doesn't complicate other areas of the code. By
expressing the (%rsi) references in terms of slots, but the fixup lables
in terms of bytes, you have taken a complicated and non-obvious
relationship and made it even more complicated to review, which is a net
detriment (and hence the request to express the labels in the same units
as the code they refer to).
~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] x86: correct create_bounce_frame
2017-05-03 18:58 ` Andrew Cooper
@ 2017-05-04 7:41 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2017-05-04 7:41 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Julien Grall
>>> On 03.05.17 at 20:58, <andrew.cooper3@citrix.com> wrote:
> On 03/05/17 16:42, Jan Beulich wrote:
>>>>> On 02.05.17 at 16:12, <andrew.cooper3@citrix.com> wrote:
>>> On 02/05/17 14:22, Jan Beulich wrote:
>>>> @@ -345,15 +344,20 @@ 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(.Lft2, domain_crash_page_fault_48)
>>>> + _ASM_EXTABLE(.Lft3, domain_crash_page_fault_40)
>>>> + _ASM_EXTABLE(.Lft4, domain_crash_page_fault_24)
>>>> + _ASM_EXTABLE(.Lft5, domain_crash_page_fault_32)
>>>> + _ASM_EXTABLE(.Lft6, domain_crash_page_fault_16)
>>>> + _ASM_EXTABLE(.Lft7, domain_crash_page_fault_16)
>>>> _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
>>> Given that you have altered the notation for a delta from (%rsi) (which
>>> is a good improvement), these labels should be similarly altered for
>>> consistency.
>> Actually, looking at this again I'm no longer sure factoring out the
>> 8 here would be a good idea. I'm also not sure I understand you
>> saying "Given that you have altered the notation for a delta from
>> (%rsi)" - the notation didn't change at all, the numeric tag has
>> always been expressing the adjustment needed for %rsi to
>> represent the actual failing memory address.
>
> First of all, this point is not obvious at all to people reading the
> code who don't already know the meaning. If it were new code today, I'd
> insist on a comment explaining what the numeric tag means.
I'm not at all opposed to adding a comment, if you think that helps.
> As for the change in notation, you very much have changed it. You have
> factored 8 out of it, which visually emphases the number of stack slots
> away from (%rsi) is used.
That's notation of the mainline code, not the fixup one. The fixup
code continues to be a sequence of additions of 8 to %rsi.
> The change in notation is, in principle, a good thing because it makes
> the code easier to read and correlate with the comment.
>
> However, such a change is only an improvement if it is implemented
> consistently (hence the request to use 1*8 and 0*8 for the other (%rsi)
> references),
As said before, if this is the only way to get an ack on the patch, I
can do this part despite not agreeing with the argument. As we've
had such situations a number of times in the past, I think we finally
need to have a more general discussion here. I'll kick off a separate
thread.
> and if it doesn't complicate other areas of the code. By
> expressing the (%rsi) references in terms of slots, but the fixup lables
> in terms of bytes, you have taken a complicated and non-obvious
> relationship and made it even more complicated to review, which is a net
> detriment (and hence the request to express the labels in the same units
> as the code they refer to).
So did you really consider how the alternative would look like:
domain_crash_page_fault_6:
addq $8,%rsi
domain_crash_page_fault_5:
addq $8,%rsi
domain_crash_page_fault_4:
addq $8,%rsi
domain_crash_page_fault_3:
addq $8,%rsi
domain_crash_page_fault_2:
addq $8,%rsi
domain_crash_page_fault_1:
addq $8,%rsi
domain_crash_page_fault:
...
I don't think this is any more or any less clear than what we have.
As said, using label names of the form
domain_crash_page_fault_<N>*8 is not going to work with other
than pretty recent binutils, and I'm unconvinced the alternative
of e.g. domain_crash_page_fault_<N>x8 is really helpful. Plus in
either case the question then would whether (a) you'd expect the
final label to use 0 to replace <N> (instead of not having any tag
at all, as it is currently) and (b) whether you'd insist on all the 8s
to become 1*8.
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
end of thread, other threads:[~2017-05-04 7:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-02 13:22 [PATCH] x86: correct create_bounce_frame Jan Beulich
2017-05-02 14:12 ` Andrew Cooper
2017-05-02 14:33 ` Jan Beulich
2017-05-03 15:42 ` Jan Beulich
2017-05-03 18:58 ` Andrew Cooper
2017-05-04 7:41 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.