Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 25 Oct 2022 17:21:04 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Emanuele Giuseppe Esposito <eesposit@redhat.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, Bandan Das <bsd@redhat.com>,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] KVM: vmx/nested: avoid blindly setting
 SECONDARY_EXEC_ENCLS_EXITING when sgx is enabled
Message-ID: <Y1gbAA0JVo9g3joi@google.com>
References: <20221025123749.2201649-1-eesposit@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221025123749.2201649-1-eesposit@redhat.com>
Precedence: bulk

Shortlog scope is still wrong, should be "KVM: nVMX:"

The shortlog is also somewhat is misleading/confusing, as it's not at all obvious
that "sgx enabled" means "KVM's sgx_module param is enabled" and not "SGX is enabled
in the system".

E.g.

  KVM: nVMX: Advertise ENCLS_EXITING to L1 iff SGX is fully supported


On Tue, Oct 25, 2022, Emanuele Giuseppe Esposito wrote:
> Currently vmx

s/vmx/KVM

> enables SECONDARY_EXEC_ENCLS_EXITING even when sgx is not set in the host MSR.

"sgx is not set in the host MSR" is ambiguous.  "sgx ... in the host MSR" could
easily refer to the SGX_ENABLED bit in IA32_FEATURE_CONTROL, it could refer to
the ENCLS_EXITING bit in the allowed-1 half of IA32_VMX_PROCBASED_CTLS2, etc...

In other words, please be more precise.

This statement is also wrong in that it implies that KVM _always_ sets ENCLS_EXITING,
whereas the bug is purely limited to nested virtualization.

E.g.

Clear enable_sgx if ENCLS-exiting is not supported, i.e. if SGX cannot be
virtualized.  This fixes a bug where KVM would advertise ENCLS-exiting to
L1 and propagate the control from vmcs12 to vmcs02 even if ENCLS-exiting
isn't supported in secondary execution controls, e.g. because SGX isn't
fully enabled, and thus induce an unexpected VM-Fail in L1.

> When booting a guest, KVM checks that the cpuid bit is actually set
> in vmx.c, and if not, it does not enable the feature.

Again, this is nothing to do with the failure.