All of lore.kernel.org
 help / color / mirror / Atom feed
From: Krish Sadhukhan <krish.sadhukhan@oracle.com>
To: Xiaoyao Li <xiaoyao.li@intel.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>
Subject: Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT
Date: Mon, 24 Feb 2020 16:27:34 -0800	[thread overview]
Message-ID: <716806df-c0e4-43d5-b082-627d2c312f53@oracle.com> (raw)
In-Reply-To: <d9744594-4a66-d867-f785-64ce4d42b848@intel.com>



On 02/24/2020 04:01 AM, Xiaoyao Li wrote:
> On 2/24/2020 6:16 PM, Vitaly Kuznetsov wrote:
>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>>
>>> Current kvm uses the 32-bit exit reason to check if it's any 
>>> specific VM
>>> EXIT, however only the low 16-bit of VM EXIT REASON acts as the basic
>>> exit reason.
>>>
>>> Introduce Macro basic(exit_reaso)
>>
>> "exit_reason"
>
> Ah, will correct it in v2.
>
>>>   to help retrieve the basic exit reason
>>> from VM EXIT REASON, and use the basic exit reason for checking and
>>> indexing the exit hanlder.
>>>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>> ---
>>>   arch/x86/kvm/vmx/vmx.c | 44 
>>> ++++++++++++++++++++++--------------------
>>>   arch/x86/kvm/vmx/vmx.h |  2 ++
>>>   2 files changed, 25 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>> index 9a6664886f2e..85da72d4dc92 100644
>>> --- a/arch/x86/kvm/vmx/vmx.c
>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>> @@ -1584,7 +1584,7 @@ static int skip_emulated_instruction(struct 
>>> kvm_vcpu *vcpu)
>>>        * i.e. we end up advancing IP with some random value.
>>>        */
>>>       if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
>>> -        to_vmx(vcpu)->exit_reason != EXIT_REASON_EPT_MISCONFIG) {
>>> +        basic(to_vmx(vcpu)->exit_reason) != 
>>> EXIT_REASON_EPT_MISCONFIG) {
>>
>> "basic" word is probably 'too basic' to be used for this purpose. Even
>> if we need a macro for it (I'm not really convinced it improves the
>> readability), I'd suggest we name it 'basic_exit_reason()' instead.
>
> Agreed.
>
>>>           rip = kvm_rip_read(vcpu);
>>>           rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
>>>           kvm_rip_write(vcpu, rip);
>>> @@ -5797,6 +5797,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>>>   {
>>>       struct vcpu_vmx *vmx = to_vmx(vcpu);
>>>       u32 exit_reason = vmx->exit_reason;
>>> +    u16 basic_exit_reason = basic(exit_reason);
>>
>> I don't think renaming local variable is needed, let's just do
>>
>> 'u16 exit_reason = basic_exit_reason(vmx->exit_reason)' and keep the
>> rest of the code as-is.
>
> No, we can't do this.
>
> It's not just renaming local variable, the full 32-bit exit reason is 
> used elsewhere in this function that needs the upper 16-bit.
>
> Here variable basic_exit_reason is added for the cases where only 
> basic exit reason number is needed.
>
>>>       u32 vectoring_info = vmx->idt_vectoring_info;
>>>         trace_kvm_exit(exit_reason, vcpu, KVM_ISA_VMX);
>>> @@ -5842,17 +5843,17 @@ static int vmx_handle_exit(struct kvm_vcpu 
>>> *vcpu,
>>>        * will cause infinite loop.
>>>        */
>>>       if ((vectoring_info & VECTORING_INFO_VALID_MASK) &&
>>> -            (exit_reason != EXIT_REASON_EXCEPTION_NMI &&
>>> -            exit_reason != EXIT_REASON_EPT_VIOLATION &&
>>> -            exit_reason != EXIT_REASON_PML_FULL &&
>>> -            exit_reason != EXIT_REASON_TASK_SWITCH)) {
>>> +            (basic_exit_reason != EXIT_REASON_EXCEPTION_NMI &&
>>> +             basic_exit_reason != EXIT_REASON_EPT_VIOLATION &&
>>> +             basic_exit_reason != EXIT_REASON_PML_FULL &&
>>> +             basic_exit_reason != EXIT_REASON_TASK_SWITCH)) {
>>>           vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>>           vcpu->run->internal.suberror = 
>>> KVM_INTERNAL_ERROR_DELIVERY_EV;
>>>           vcpu->run->internal.ndata = 3;
>>>           vcpu->run->internal.data[0] = vectoring_info;
>>>           vcpu->run->internal.data[1] = exit_reason;
>>>           vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
>>> -        if (exit_reason == EXIT_REASON_EPT_MISCONFIG) {
>>> +        if (basic_exit_reason == EXIT_REASON_EPT_MISCONFIG) {
>>>               vcpu->run->internal.ndata++;
>>>               vcpu->run->internal.data[3] =
>>>                   vmcs_read64(GUEST_PHYSICAL_ADDRESS);
>>> @@ -5884,32 +5885,32 @@ static int vmx_handle_exit(struct kvm_vcpu 
>>> *vcpu,
>>>           return 1;
>>>       }
>>>   -    if (exit_reason >= kvm_vmx_max_exit_handlers)
>>> +    if (basic_exit_reason >= kvm_vmx_max_exit_handlers)
>>>           goto unexpected_vmexit;
>>>   #ifdef CONFIG_RETPOLINE
>>> -    if (exit_reason == EXIT_REASON_MSR_WRITE)
>>> +    if (basic_exit_reason == EXIT_REASON_MSR_WRITE)
>>>           return kvm_emulate_wrmsr(vcpu);
>>> -    else if (exit_reason == EXIT_REASON_PREEMPTION_TIMER)
>>> +    else if (basic_exit_reason == EXIT_REASON_PREEMPTION_TIMER)
>>>           return handle_preemption_timer(vcpu);
>>> -    else if (exit_reason == EXIT_REASON_INTERRUPT_WINDOW)
>>> +    else if (basic_exit_reason == EXIT_REASON_INTERRUPT_WINDOW)
>>>           return handle_interrupt_window(vcpu);
>>> -    else if (exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>> +    else if (basic_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>>           return handle_external_interrupt(vcpu);
>>> -    else if (exit_reason == EXIT_REASON_HLT)
>>> +    else if (basic_exit_reason == EXIT_REASON_HLT)
>>>           return kvm_emulate_halt(vcpu);
>>> -    else if (exit_reason == EXIT_REASON_EPT_MISCONFIG)
>>> +    else if (basic_exit_reason == EXIT_REASON_EPT_MISCONFIG)
>>>           return handle_ept_misconfig(vcpu);
>>>   #endif
>>>   -    exit_reason = array_index_nospec(exit_reason,
>>> +    basic_exit_reason = array_index_nospec(basic_exit_reason,
>>>                        kvm_vmx_max_exit_handlers);
>>> -    if (!kvm_vmx_exit_handlers[exit_reason])
>>> +    if (!kvm_vmx_exit_handlers[basic_exit_reason])
>>>           goto unexpected_vmexit;
>>>   -    return kvm_vmx_exit_handlers[exit_reason](vcpu);
>>> +    return kvm_vmx_exit_handlers[basic_exit_reason](vcpu);
>>>     unexpected_vmexit:
>>> -    vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", 
>>> exit_reason);
>>> +    vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", 
>>> basic_exit_reason);
>>>       dump_vmcs();
>>>       vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>>       vcpu->run->internal.suberror =
>>> @@ -6241,13 +6242,14 @@ static void vmx_handle_exit_irqoff(struct 
>>> kvm_vcpu *vcpu,
>>>       enum exit_fastpath_completion *exit_fastpath)
>>>   {
>>>       struct vcpu_vmx *vmx = to_vmx(vcpu);
>>> +    u16 basic_exit_reason = basic(vmx->exit_reason);
>>
>> Here I'd suggest we also use the same
>>
>> 'u16 exit_reason = basic_exit_reason(vmx->exit_reason)'
>>
>> as above.
>>
>>>   -    if (vmx->exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>> +    if (basic_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>>           handle_external_interrupt_irqoff(vcpu);
>>> -    else if (vmx->exit_reason == EXIT_REASON_EXCEPTION_NMI)
>>> +    else if (basic_exit_reason == EXIT_REASON_EXCEPTION_NMI)
>>>           handle_exception_nmi_irqoff(vmx);
>>>       else if (!is_guest_mode(vcpu) &&
>>> -        vmx->exit_reason == EXIT_REASON_MSR_WRITE)
>>> +         basic_exit_reason == EXIT_REASON_MSR_WRITE)
>>>           *exit_fastpath = handle_fastpath_set_msr_irqoff(vcpu);
>>>   }
>>>   @@ -6621,7 +6623,7 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>>>       vmx->idt_vectoring_info = 0;
>>>         vmx->exit_reason = vmx->fail ? 0xdead : 
>>> vmcs_read32(VM_EXIT_REASON);
>>> -    if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
>>> +    if (basic(vmx->exit_reason) == EXIT_REASON_MCE_DURING_VMENTRY)
>>>           kvm_machine_check();
>>>         if (vmx->fail || (vmx->exit_reason & 
>>> VMX_EXIT_REASONS_FAILED_VMENTRY))
>>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>>> index 7f42cf3dcd70..c6ba33eedb59 100644
>>> --- a/arch/x86/kvm/vmx/vmx.h
>>> +++ b/arch/x86/kvm/vmx/vmx.h
>>> @@ -22,6 +22,8 @@ extern u32 get_umwait_control_msr(void);
>>>     #define X2APIC_MSR(r) (APIC_BASE_MSR + ((r) >> 4))
>>>   +#define basic(exit_reason) ((u16)(exit_reason))

We have a macro for bit 31,

     VMX_EXIT_REASONS_FAILED_VMENTRY                0x80000000


Does it make sense to define a macro like that instead ? Say,

     VMX_BASIC_EXIT_REASON        0x0000ffff

and then we do,

     u32 exit_reason = vmx->exit_reason;
     u16 basic_exit_reason = exit_reason & VMX_BASIC_EXIT_REASON;


>>> +
>>>   #ifdef CONFIG_X86_64
>>>   #define NR_SHARED_MSRS    7
>>>   #else
>>
>


  parent reply	other threads:[~2020-02-25  0:28 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-24  2:07 [PATCH 0/2] KVM: VMX: Use basic exit reason for cheking and indexing Xiaoyao Li
2020-02-24  2:07 ` [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT Xiaoyao Li
2020-02-24 10:16   ` Vitaly Kuznetsov
2020-02-24 12:01     ` Xiaoyao Li
2020-02-24 13:04       ` Vitaly Kuznetsov
2020-02-24 16:17         ` Sean Christopherson
2020-02-25  0:13           ` Xiaoyao Li
2020-02-25  6:13             ` Sean Christopherson
2020-02-25  6:41               ` Xiaoyao Li
2020-02-26 23:59                 ` Sean Christopherson
2020-02-27  8:35                   ` Xiaoyao Li
2020-02-27 23:57                     ` Sean Christopherson
2020-02-25  0:27       ` Krish Sadhukhan [this message]
2020-02-25 13:11         ` Vitaly Kuznetsov
2020-02-25 18:28           ` Krish Sadhukhan
2020-02-24  2:07 ` [PATCH 2/2] kvm: nvmx: Use basic(exit_reason) when checking specific EXIT_REASON Xiaoyao Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=716806df-c0e4-43d5-b082-627d2c312f53@oracle.com \
    --to=krish.sadhukhan@oracle.com \
    --cc=jmattson@google.com \
    --cc=joro@8bytes.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --cc=xiaoyao.li@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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.