Messages in this thread Patch in this message | | | Subject | Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT | From | Xiaoyao Li <> | Date | Tue, 25 Feb 2020 08:13:15 +0800 |
| |
On 2/25/2020 12:17 AM, Sean Christopherson wrote: > On Mon, Feb 24, 2020 at 02:04:46PM +0100, Vitaly Kuznetsov wrote: >> Xiaoyao Li <xiaoyao.li@intel.com> writes: >> >>> On 2/24/2020 6:16 PM, Vitaly Kuznetsov wrote: >>>> Xiaoyao Li <xiaoyao.li@intel.com> writes: >>>> >> >> ... >> >>>>> 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. >>> >> >> Can we do the other way around, i.e. introduce 'extended_exit_reason' >> and use it where all 32 bits are needed? I'm fine with the change, just >> trying to minimize the (unneeded) code churn. > > 100% agree. Even better than adding a second field to vcpu_vmx would be > to make it a union, though we'd probably want to call it something like > full_exit_reason in that case. That should give us compile-time checks on > exit_reason, e.g. if we try to query one of the upper bits using a u16, e.g.
I have thought about union, but it seems
union { u16 exit_reason; u32 full_exit_reason; }
is not a good name. Since there are many codes in vmx.c and nested.c assume that exit_reason stands for 32-bit EXIT REASON vmcs field as well as evmcs->vm_exit_reason and vmcs12->vm_exit_reason. Do we really want to also rename them to full_exit_reason?
Maybe we name it
union { u16 basic_exit_reason; u32 exit_reason; }
as what SDM defines?
> --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -5818,7 +5818,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, > if (is_guest_mode(vcpu) && nested_vmx_exit_reflected(vcpu, exit_reason)) > return nested_vmx_reflect_vmexit(vcpu, exit_reason); > > - if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) { > + if (vmx->full_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) { > dump_vmcs(); > vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY; > vcpu->run->fail_entry.hardware_entry_failure_reason > @@ -6620,11 +6620,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) > vmx->nested.nested_run_pending = 0; > 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) > + vmx->full_exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON); > + if (vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) > kvm_machine_check(); > > - if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)) > + if (vmx->fail || > + (vmx->full_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)) > return; > > vmx->loaded_vmcs->launched = 1; > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index 7f42cf3dcd70..60c09640ea59 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -260,7 +260,10 @@ struct vcpu_vmx { > int vpid; > bool emulation_required; > > - u32 exit_reason; > + union { > + u16 exit_reason; > + u32 full_exit_reason; > + } > > /* Posted interrupt descriptor */ > struct pi_desc pi_desc; > > > > > >> -- >> Vitaly >>
| |