Re: [PATCH 1/4] VMX: streamline entry.S code

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7952 bytes --]

On 23/08/2013 15:01, Jan Beulich wrote:
> - move stuff easily/better done in C into C code
> - re-arrange code paths so that no redundant GET_CURRENT() would remain
>   on the fast paths
> - move long latency operations earlier
> - slightly defer disabling interrupts on the VM entry path
> - use ENTRY() instead of open coding it
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This is quite tough reviewing as all the changes are mixed together, but
I think I have got it all now.

WRT moving things to C, and use of ENTRY(), fine.

>
> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -28,109 +28,66 @@
>  
>  #define VMRESUME     .byte 0x0f,0x01,0xc3
>  #define VMLAUNCH     .byte 0x0f,0x01,0xc2
> -#define VMREAD(off)  .byte 0x0f,0x78,0x47,((off)-UREGS_rip)
> -#define VMWRITE(off) .byte 0x0f,0x79,0x47,((off)-UREGS_rip)
>  
> -/* VMCS field encodings */
> -#define GUEST_RSP    0x681c
> -#define GUEST_RIP    0x681e
> -#define GUEST_RFLAGS 0x6820
> -
> -        ALIGN
> -.globl vmx_asm_vmexit_handler
> -vmx_asm_vmexit_handler:
> +ENTRY(vmx_asm_vmexit_handler)
>          push %rdi
>          push %rsi
>          push %rdx
>          push %rcx
>          push %rax
> +        mov  %cr2,%rax

I presume this is a long latency instruction.  Do you have a source of
numbers for this? (more for interest, as I can easily accept that it
would be a longer operation than the surrounding ones)

>          push %r8
>          push %r9
>          push %r10
>          push %r11
>          push %rbx
> +        GET_CURRENT(%rbx)

This seems a little less obvious.  I presume you are just breaking true
read-after-write data hazard on %rbx ?

>          push %rbp
>          push %r12
>          push %r13
>          push %r14
>          push %r15
>  
> -        GET_CURRENT(%rbx)
> -
>          movb $1,VCPU_vmx_launched(%rbx)
> -
> -        lea  UREGS_rip(%rsp),%rdi
> -        mov  $GUEST_RIP,%eax
> -        /*VMREAD(UREGS_rip)*/
> -        .byte 0x0f,0x78,0x07  /* vmread %rax,(%rdi) */
> -        mov  $GUEST_RSP,%eax
> -        VMREAD(UREGS_rsp)
> -        mov  $GUEST_RFLAGS,%eax
> -        VMREAD(UREGS_eflags)
> -
> -        mov  %cr2,%rax
>          mov  %rax,VCPU_hvm_guest_cr2(%rbx)
>  
> -#ifndef NDEBUG
> -        mov  $0xbeef,%ax
> -        mov  %ax,UREGS_error_code(%rsp)
> -        mov  %ax,UREGS_entry_vector(%rsp)
> -        mov  %ax,UREGS_saved_upcall_mask(%rsp)
> -        mov  %ax,UREGS_cs(%rsp)
> -        mov  %ax,UREGS_ds(%rsp)
> -        mov  %ax,UREGS_es(%rsp)
> -        mov  %ax,UREGS_fs(%rsp)
> -        mov  %ax,UREGS_gs(%rsp)
> -        mov  %ax,UREGS_ss(%rsp)
> -#endif
> -
>          mov  %rsp,%rdi
>          call vmx_vmexit_handler
>  
> -.globl vmx_asm_do_vmentry
> -vmx_asm_do_vmentry:

If you move the ENTRY(vmx_asm_do_vmentry) up from below, you should be
able to completely drop the jmp in it.  However...

> +.Lvmx_do_vmentry:
>          call vmx_intr_assist
>          call nvmx_switch_guest
>          ASSERT_NOT_IN_ATOMIC
>  
> -        GET_CURRENT(%rbx)
> -        cli

The movement of this cli indicates a possible issue.

If we have softirqs pending, we jump to .Lvmx_process_softirqs, which
calls do_softirq, and then jumps back to the top of .Lvmx_do_vmentry,
which reruns the top of do_vmentry with interrupts now enabled.

First of all, I cant see anything in vmx_intr_assist or
nvmx_switch_guest which should require calling multiple times on a
vmentry.  They are also expecting to be called with interrupts disabled
(although I cant spot anything obvious in the callpath which would be
affected).

Perhaps the jumps vmx_goto_emulator and vmx_process_softirqs should turn
into calls, to prevent repeatedly reruning the top of vmx_do_vmentry? 
Then, ENTRY(vmx_do_vmentry) and .Lvmx_do_vmentry could be collapsed
together.

> -
>          mov  VCPU_processor(%rbx),%eax
> -        shl  $IRQSTAT_shift,%eax
>          lea  irq_stat+IRQSTAT_softirq_pending(%rip),%rdx
> -        cmpl $0,(%rdx,%rax,1)
> +        xor  %ecx,%ecx
> +        shl  $IRQSTAT_shift,%eax
> +        cli
> +        cmp  %ecx,(%rdx,%rax,1)
>          jnz  .Lvmx_process_softirqs
>  
> -        testb $0xff,VCPU_vmx_emulate(%rbx)
> -        jnz .Lvmx_goto_emulator
> -        testb $0xff,VCPU_vmx_realmode(%rbx)
> -UNLIKELY_START(nz, realmode)
> -        cmpw $0,VCPU_vm86_seg_mask(%rbx)
> +        cmp  %cl,VCPU_vmx_emulate(%rbx)
> +        jne .Lvmx_goto_emulator
> +        cmp  %cl,VCPU_vmx_realmode(%rbx)
> +UNLIKELY_START(ne, realmode)
> +        cmp  %cx,VCPU_vm86_seg_mask(%rbx)
>          jnz .Lvmx_goto_emulator
>          mov  %rsp,%rdi
>          call vmx_enter_realmode
>  UNLIKELY_END(realmode)
>  
> +        mov  %rsp,%rdi
>          call vmx_vmenter_helper
>          mov  VCPU_hvm_guest_cr2(%rbx),%rax
> -        mov  %rax,%cr2
> -
> -        lea  UREGS_rip(%rsp),%rdi
> -        mov  $GUEST_RIP,%eax
> -        /*VMWRITE(UREGS_rip)*/
> -        .byte 0x0f,0x79,0x07  /* vmwrite (%rdi),%rax */
> -        mov  $GUEST_RSP,%eax
> -        VMWRITE(UREGS_rsp)
> -        mov  $GUEST_RFLAGS,%eax
> -        VMWRITE(UREGS_eflags)
>  
> -        cmpb $0,VCPU_vmx_launched(%rbx)
>          pop  %r15
>          pop  %r14
>          pop  %r13
>          pop  %r12
>          pop  %rbp
> +        mov  %rax,%cr2
> +        cmpb $0,VCPU_vmx_launched(%rbx)

Again, I presume the move of "mov %rax,%cr2" is about the %rax data hazard?

~Andrew

>          pop  %rbx
>          pop  %r11
>          pop  %r10
> @@ -155,13 +112,17 @@ UNLIKELY_END(realmode)
>          call vm_launch_fail
>          ud2
>  
> +ENTRY(vmx_asm_do_vmentry)
> +        GET_CURRENT(%rbx)
> +        jmp  .Lvmx_do_vmentry
> +
>  .Lvmx_goto_emulator:
>          sti
>          mov  %rsp,%rdi
>          call vmx_realmode
> -        jmp  vmx_asm_do_vmentry
> +        jmp  .Lvmx_do_vmentry
>  
>  .Lvmx_process_softirqs:
>          sti
>          call do_softirq
> -        jmp  vmx_asm_do_vmentry
> +        jmp  .Lvmx_do_vmentry
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2375,6 +2375,12 @@ void vmx_vmexit_handler(struct cpu_user_
>      unsigned long exit_qualification, inst_len = 0;
>      struct vcpu *v = current;
>  
> +    regs->rip    = __vmread(GUEST_RIP);
> +    regs->rsp    = __vmread(GUEST_RSP);
> +    regs->rflags = __vmread(GUEST_RFLAGS);
> +
> +    hvm_invalidate_regs_fields(regs);
> +
>      if ( paging_mode_hap(v->domain) && hvm_paging_enabled(v) )
>          v->arch.hvm_vcpu.guest_cr[3] = v->arch.hvm_vcpu.hw_cr[3] =
>              __vmread(GUEST_CR3);
> @@ -2868,7 +2874,7 @@ out:
>          nvmx_idtv_handling();
>  }
>  
> -void vmx_vmenter_helper(void)
> +void vmx_vmenter_helper(const struct cpu_user_regs *regs)
>  {
>      struct vcpu *curr = current;
>      u32 new_asid, old_asid;
> @@ -2910,6 +2916,10 @@ void vmx_vmenter_helper(void)
>  
>   out:
>      HVMTRACE_ND(VMENTRY, 0, 1/*cycles*/, 0, 0, 0, 0, 0, 0, 0);
> +
> +    __vmwrite(GUEST_RIP,    regs->rip);
> +    __vmwrite(GUEST_RSP,    regs->rsp);
> +    __vmwrite(GUEST_RFLAGS, regs->rflags);
>  }
>  
>  /*
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -439,6 +439,21 @@ static inline void hvm_set_info_guest(st
>  
>  int hvm_debug_op(struct vcpu *v, int32_t op);
>  
> +static inline void hvm_invalidate_regs_fields(struct cpu_user_regs *regs)
> +{
> +#ifndef NDEBUG
> +    regs->error_code = 0xbeef;
> +    regs->entry_vector = 0xbeef;
> +    regs->saved_upcall_mask = 0xbf;
> +    regs->cs = 0xbeef;
> +    regs->ss = 0xbeef;
> +    regs->ds = 0xbeef;
> +    regs->es = 0xbeef;
> +    regs->fs = 0xbeef;
> +    regs->gs = 0xbeef;
> +#endif
> +}
> +
>  int hvm_hap_nested_page_fault(paddr_t gpa,
>                                bool_t gla_valid, unsigned long gla,
>                                bool_t access_r,
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 9393 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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