Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Maxim Levitsky <mlevitsk@redhat.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 10/14] KVM: x86: Make Hyper-V emulation optional
In-Reply-To: <ZWfl3ahamXPPoIGB@google.com>
References: <20231025152406.1879274-1-vkuznets@redhat.com>
 <20231025152406.1879274-11-vkuznets@redhat.com>
 <ZWfl3ahamXPPoIGB@google.com>
Date:   Thu, 30 Nov 2023 16:11:43 +0100
Message-ID: <87y1efmg28.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Sean Christopherson <seanjc@google.com> writes:

...

>
>>  static bool nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
>>  {
>> @@ -3552,11 +3563,13 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
>>  	if (!nested_vmx_check_permission(vcpu))
>>  		return 1;
>>  
>> +#ifdef CONFIG_KVM_HYPERV
>>  	evmptrld_status = nested_vmx_handle_enlightened_vmptrld(vcpu, launch);
>>  	if (evmptrld_status == EVMPTRLD_ERROR) {
>>  		kvm_queue_exception(vcpu, UD_VECTOR);
>>  		return 1;
>>  	}
>> +#endif
>>  
>>  	kvm_pmu_trigger_event(vcpu, PERF_COUNT_HW_BRANCH_INSTRUCTIONS);
>
> This fails to build with CONFIG_KVM_HYPERV=n && CONFIG_KVM_WERROR=y:
>
> arch/x86/kvm/vmx/nested.c:3577:9: error: variable 'evmptrld_status' is uninitialized when used here [-Werror,-Wuninitialized]
>         if (CC(evmptrld_status == EVMPTRLD_VMFAIL))
>                ^~~~~~~~~~~~~~~
>
> Sadly, simply wrapping with an #ifdef also fails because then evmptrld_status
> becomes unused.  I'd really prefer to avoid having to tag it __maybe_unused, and
> adding more #ifdef would also be ugly.  Any ideas?

A couple,

- we can try putting all eVMPTR logic under 'if (1)' just to create a
  block where we can define evmptrld_status. Not sure this is nicer than
  another #ifdef wrapping evmptrld_status and I'm not sure what to do
  with kvm_pmu_trigger_event() -- can it just go above
  nested_vmx_handle_enlightened_vmptrld()?

- we can add a helper, e.g. 'evmptr_is_vmfail()' and make it just return
  'false' when !CONFIG_KVM_HYPERV.

- rewrite this as a switch to avoid the need for having the local
  variable, (untested)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index c5ec0ef51ff7..b26ce7d596e9 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3553,22 +3553,23 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
        enum nvmx_vmentry_status status;
        struct vcpu_vmx *vmx = to_vmx(vcpu);
        u32 interrupt_shadow = vmx_get_interrupt_shadow(vcpu);
-       enum nested_evmptrld_status evmptrld_status;
 
        if (!nested_vmx_check_permission(vcpu))
                return 1;
 
-       evmptrld_status = nested_vmx_handle_enlightened_vmptrld(vcpu, launch);
-       if (evmptrld_status == EVMPTRLD_ERROR) {
+       switch (nested_vmx_handle_enlightened_vmptrld(vcpu, launch)) {
+       case EVMPTRLD_ERROR:
                kvm_queue_exception(vcpu, UD_VECTOR);
                return 1;
+       case EVMPTRLD_VMFAIL:
+               trace_kvm_nested_vmenter_failed("evmptrld_status", 0);
+               return nested_vmx_failInvalid(vcpu);
+       default:
+               break;
        }
 
        kvm_pmu_trigger_event(vcpu, PERF_COUNT_HW_BRANCH_INSTRUCTIONS);
 
-       if (CC(evmptrld_status == EVMPTRLD_VMFAIL))
-               return nested_vmx_failInvalid(vcpu);
-
        if (CC(!evmptr_is_valid(vmx->nested.hv_evmcs_vmptr) &&
               vmx->nested.current_vmptr == INVALID_GPA))
                return nested_vmx_failInvalid(vcpu);

Unfortunately, I had to open code CC() ;-(

Or maybe just another "#ifdef" is not so ugly after all? :-)

-- 
Vitaly