Re: [Xen-devel] [PATCH] x86: correct create_bounce_frame

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.05.17 at 16:12, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel