Re: [Xen-devel] [PATCH v3 5/9] x86/vvmx: make updating shadow EPTP value more efficient

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/10/17 15:38, George Dunlap wrote:
> On 10/03/2017 04:21 PM, Sergey Dyasli wrote:
>> At the moment, the shadow EPTP value is written unconditionally in
>> ept_handle_violation().
>>
>> Instead, write the value on vmentry to the guest; but only write it if
>> the value needs updating.
>>
>> To detect this, add a flag to the nestedvcpu struct, stale_np2m, to
>> indicate when such an action is necessary.  Set it when the nested p2m
>> changes or when the np2m is flushed by an IPI, and clear it when we
>> write the new value.
>>
>> Since an IPI invalidating the p2m may happen between
>> nvmx_switch_guest() and vmx_vmenter, but we can't perform the vmwrite
>> with interrupts disabled, check the flag just before entering the
>> guest and restart the vmentry if it's set.
>>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
>> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Looks good to me; just one question...
>
>> ---
>> v2 --> v3:
>> - current pointer is now calculated only once in nvmx_eptp_update()
>> ---
>>  xen/arch/x86/hvm/nestedhvm.c   |  2 ++
>>  xen/arch/x86/hvm/vmx/entry.S   |  6 ++++++
>>  xen/arch/x86/hvm/vmx/vmx.c     | 14 +++++++-------
>>  xen/arch/x86/hvm/vmx/vvmx.c    | 22 ++++++++++++++++++++++
>>  xen/arch/x86/mm/p2m.c          | 10 ++++++++--
>>  xen/include/asm-x86/hvm/vcpu.h |  1 +
>>  6 files changed, 46 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
>> index f2f7469d86..74a464d162 100644
>> --- a/xen/arch/x86/hvm/nestedhvm.c
>> +++ b/xen/arch/x86/hvm/nestedhvm.c
>> @@ -56,6 +56,7 @@ nestedhvm_vcpu_reset(struct vcpu *v)
>>      nv->nv_vvmcxaddr = INVALID_PADDR;
>>      nv->nv_flushp2m = 0;
>>      nv->nv_p2m = NULL;
>> +    nv->stale_np2m = false;
>>  
>>      hvm_asid_flush_vcpu_asid(&nv->nv_n2asid);
>>  
>> @@ -107,6 +108,7 @@ nestedhvm_flushtlb_ipi(void *info)
>>       */
>>      hvm_asid_flush_core();
>>      vcpu_nestedhvm(v).nv_p2m = NULL;
>> +    vcpu_nestedhvm(v).stale_np2m = true;
>>  }
>>  
>>  void
>> diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
>> index 53eedc6363..9fb8f89220 100644
>> --- a/xen/arch/x86/hvm/vmx/entry.S
>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>> @@ -79,6 +79,8 @@ UNLIKELY_END(realmode)
>>  
>>          mov  %rsp,%rdi
>>          call vmx_vmenter_helper
>> +        cmp  $0,%eax
>> +        jne .Lvmx_vmentry_restart
>>          mov  VCPU_hvm_guest_cr2(%rbx),%rax
>>  
>>          pop  %r15
>> @@ -117,6 +119,10 @@ ENTRY(vmx_asm_do_vmentry)
>>          GET_CURRENT(bx)
>>          jmp  .Lvmx_do_vmentry
>>  
>> +.Lvmx_vmentry_restart:
>> +        sti
>> +        jmp  .Lvmx_do_vmentry
>> +
>>  .Lvmx_goto_emulator:
>>          sti
>>          mov  %rsp,%rdi
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 9cfa9b6965..c9a4111267 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3249,12 +3249,6 @@ static void ept_handle_violation(ept_qual_t q, 
>> paddr_t gpa)
>>      case 0:         // Unhandled L1 EPT violation
>>          break;
>>      case 1:         // This violation is handled completly
>> -        /*Current nested EPT maybe flushed by other vcpus, so need
>> -         * to re-set its shadow EPTP pointer.
>> -         */
>> -        if ( nestedhvm_vcpu_in_guestmode(current) &&
>> -                        nestedhvm_paging_mode_hap(current ) )
>> -            __vmwrite(EPT_POINTER, get_shadow_eptp(current));
>>          return;
>>      case -1:        // This vioaltion should be injected to L1 VMM
>>          vcpu_nestedhvm(current).nv_vmexit_pending = 1;
>> @@ -4203,13 +4197,17 @@ static void lbr_fixup(void)
>>          bdw_erratum_bdf14_fixup();
>>  }
>>  
>> -void vmx_vmenter_helper(const struct cpu_user_regs *regs)
>> +int vmx_vmenter_helper(const struct cpu_user_regs *regs)
> ...Andy, did you want a comment here explaining what the return value is
> supposed to mean? (And/or changing this to a bool?)

Definitely a comment please (especially as it is logically inverted from
what I would have expected originally).

Bool depending on whether it actually has boolean properties or not
(which will depend on how the comment ends up looking).

~Andrew

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