Add a helper function and several wrapping macros to consolidate the
copy-paste code in vmx_compute_secondary_exec_control() for adjusting
controls that are dependent on guest CPUID bits.
No functional change intended.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 128 +++++++++++++----------------------------
1 file changed, 41 insertions(+), 87 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 5180529f6531..b786cfb74f4f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4073,6 +4073,38 @@ u32 vmx_exec_control(struct vcpu_vmx *vmx)
}
+static inline void
+vmx_adjust_secondary_exec_control(struct vcpu_vmx *vmx, u32 *exec_control,
+ u32 control, bool enabled, bool exiting)
+{
+ if (enabled == exiting)
+ *exec_control &= ~control;
+ if (nested) {
+ if (enabled)
+ vmx->nested.msrs.secondary_ctls_high |= control;
+ else
+ vmx->nested.msrs.secondary_ctls_high &= ~control;
+ }
+}
+
+#define vmx_adjust_sec_exec_control(vmx, exec_control, name, feat_name, ctrl_name, exiting) \
+({ \
+ bool __enabled; \
+ \
+ if (cpu_has_vmx_##name()) { \
+ __enabled = guest_cpuid_has(&(vmx)->vcpu, \
+ X86_FEATURE_##feat_name); \
+ vmx_adjust_secondary_exec_control(vmx, exec_control, \
+ SECONDARY_EXEC_##ctrl_name, __enabled, exiting); \
+ } \
+})
+
+#define vmx_adjust_sec_exec_feature(vmx, exec_control, lname, uname) \
+ vmx_adjust_sec_exec_control(vmx, exec_control, lname, uname, ENABLE_##uname, false)
+
+#define vmx_adjust_sec_exec_exiting(vmx, exec_control, lname, uname) \
+ vmx_adjust_sec_exec_control(vmx, exec_control, lname, uname, uname##_EXITING, true)
+
static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
{
struct kvm_vcpu *vcpu = &vmx->vcpu;
@@ -4121,33 +4153,12 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
vcpu->arch.xsaves_enabled = xsaves_enabled;
- if (!xsaves_enabled)
- exec_control &= ~SECONDARY_EXEC_XSAVES;
-
- if (nested) {
- if (xsaves_enabled)
- vmx->nested.msrs.secondary_ctls_high |=
- SECONDARY_EXEC_XSAVES;
- else
- vmx->nested.msrs.secondary_ctls_high &=
- ~SECONDARY_EXEC_XSAVES;
- }
+ vmx_adjust_secondary_exec_control(vmx, &exec_control,
+ SECONDARY_EXEC_XSAVES,
+ xsaves_enabled, false);
}
- if (cpu_has_vmx_rdtscp()) {
- bool rdtscp_enabled = guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP);
- if (!rdtscp_enabled)
- exec_control &= ~SECONDARY_EXEC_ENABLE_RDTSCP;
-
- if (nested) {
- if (rdtscp_enabled)
- vmx->nested.msrs.secondary_ctls_high |=
- SECONDARY_EXEC_ENABLE_RDTSCP;
- else
- vmx->nested.msrs.secondary_ctls_high &=
- ~SECONDARY_EXEC_ENABLE_RDTSCP;
- }
- }
+ vmx_adjust_sec_exec_feature(vmx, &exec_control, rdtscp, RDTSCP);
/*
* Expose INVPCID if and only if PCID is also exposed to the guest.
@@ -4157,71 +4168,14 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
*/
if (!guest_cpuid_has(vcpu, X86_FEATURE_PCID))
guest_cpuid_clear(vcpu, X86_FEATURE_INVPCID);
+ vmx_adjust_sec_exec_feature(vmx, &exec_control, invpcid, INVPCID);
- if (cpu_has_vmx_invpcid()) {
- /* Exposing INVPCID only when PCID is exposed */
- bool invpcid_enabled =
- guest_cpuid_has(vcpu, X86_FEATURE_INVPCID);
- if (!invpcid_enabled)
- exec_control &= ~SECONDARY_EXEC_ENABLE_INVPCID;
+ vmx_adjust_sec_exec_exiting(vmx, &exec_control, rdrand, RDRAND);
+ vmx_adjust_sec_exec_exiting(vmx, &exec_control, rdseed, RDSEED);
- if (nested) {
- if (invpcid_enabled)
- vmx->nested.msrs.secondary_ctls_high |=
- SECONDARY_EXEC_ENABLE_INVPCID;
- else
- vmx->nested.msrs.secondary_ctls_high &=
- ~SECONDARY_EXEC_ENABLE_INVPCID;
- }
- }
-
- if (cpu_has_vmx_rdrand()) {
- bool rdrand_enabled = guest_cpuid_has(vcpu, X86_FEATURE_RDRAND);
- if (rdrand_enabled)
- exec_control &= ~SECONDARY_EXEC_RDRAND_EXITING;
-
- if (nested) {
- if (rdrand_enabled)
- vmx->nested.msrs.secondary_ctls_high |=
- SECONDARY_EXEC_RDRAND_EXITING;
- else
- vmx->nested.msrs.secondary_ctls_high &=
- ~SECONDARY_EXEC_RDRAND_EXITING;
- }
- }
-
- if (cpu_has_vmx_rdseed()) {
- bool rdseed_enabled = guest_cpuid_has(vcpu, X86_FEATURE_RDSEED);
- if (rdseed_enabled)
- exec_control &= ~SECONDARY_EXEC_RDSEED_EXITING;
-
- if (nested) {
- if (rdseed_enabled)
- vmx->nested.msrs.secondary_ctls_high |=
- SECONDARY_EXEC_RDSEED_EXITING;
- else
- vmx->nested.msrs.secondary_ctls_high &=
- ~SECONDARY_EXEC_RDSEED_EXITING;
- }
- }
-
- if (cpu_has_vmx_waitpkg()) {
- bool waitpkg_enabled =
- guest_cpuid_has(vcpu, X86_FEATURE_WAITPKG);
-
- if (!waitpkg_enabled)
- exec_control &= ~SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE;
-
- if (nested) {
- if (waitpkg_enabled)
- vmx->nested.msrs.secondary_ctls_high |=
- SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE;
- else
- vmx->nested.msrs.secondary_ctls_high &=
- ~SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE;
- }
- }
+ vmx_adjust_sec_exec_control(vmx, &exec_control, waitpkg, WAITPKG,
+ ENABLE_USR_WAIT_PAUSE, false);
vmx->secondary_exec_control = exec_control;
}
--
2.28.0
On 23/09/20 18:50, Sean Christopherson wrote:
> Add a helper function and several wrapping macros to consolidate the
> copy-paste code in vmx_compute_secondary_exec_control() for adjusting
> controls that are dependent on guest CPUID bits.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/vmx/vmx.c | 128 +++++++++++++----------------------------
> 1 file changed, 41 insertions(+), 87 deletions(-)
The diffstat is enticing but the code a little less so... Can you just
add documentation above vmx_adjust_secondary_exec_control that explains
the how/why?
Paolo
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 5180529f6531..b786cfb74f4f 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4073,6 +4073,38 @@ u32 vmx_exec_control(struct vcpu_vmx *vmx)
> }
>
>
> +static inline void
> +vmx_adjust_secondary_exec_control(struct vcpu_vmx *vmx, u32 *exec_control,
> + u32 control, bool enabled, bool exiting)
> +{
> + if (enabled == exiting)
> + *exec_control &= ~control;
> + if (nested) {
> + if (enabled)
> + vmx->nested.msrs.secondary_ctls_high |= control;
> + else
> + vmx->nested.msrs.secondary_ctls_high &= ~control;
> + }
> +}
> +
> +#define vmx_adjust_sec_exec_control(vmx, exec_control, name, feat_name, ctrl_name, exiting) \
> +({ \
> + bool __enabled; \
> + \
> + if (cpu_has_vmx_##name()) { \
> + __enabled = guest_cpuid_has(&(vmx)->vcpu, \
> + X86_FEATURE_##feat_name); \
> + vmx_adjust_secondary_exec_control(vmx, exec_control, \
> + SECONDARY_EXEC_##ctrl_name, __enabled, exiting); \
> + } \
> +})
> +
> +#define vmx_adjust_sec_exec_feature(vmx, exec_control, lname, uname) \
> + vmx_adjust_sec_exec_control(vmx, exec_control, lname, uname, ENABLE_##uname, false)
> +
> +#define vmx_adjust_sec_exec_exiting(vmx, exec_control, lname, uname) \
> + vmx_adjust_sec_exec_control(vmx, exec_control, lname, uname, uname##_EXITING, true)
> +
> static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
> {
On Wed, Sep 23, 2020 at 07:20:17PM +0200, Paolo Bonzini wrote:
> On 23/09/20 18:50, Sean Christopherson wrote:
> > Add a helper function and several wrapping macros to consolidate the
> > copy-paste code in vmx_compute_secondary_exec_control() for adjusting
> > controls that are dependent on guest CPUID bits.
> >
> > No functional change intended.
> >
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > arch/x86/kvm/vmx/vmx.c | 128 +++++++++++++----------------------------
> > 1 file changed, 41 insertions(+), 87 deletions(-)
>
> The diffstat is enticing but the code a little less so... Can you just
> add documentation above vmx_adjust_secondary_exec_control that explains
> the how/why?
Ya, I'd be more than happy to add a big comment.
On 23/09/20 19:22, Sean Christopherson wrote:
> On Wed, Sep 23, 2020 at 07:20:17PM +0200, Paolo Bonzini wrote:
>> On 23/09/20 18:50, Sean Christopherson wrote:
>>> Add a helper function and several wrapping macros to consolidate the
>>> copy-paste code in vmx_compute_secondary_exec_control() for adjusting
>>> controls that are dependent on guest CPUID bits.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Sean Christopherson <[email protected]>
>>> ---
>>> arch/x86/kvm/vmx/vmx.c | 128 +++++++++++++----------------------------
>>> 1 file changed, 41 insertions(+), 87 deletions(-)
>>
>> The diffstat is enticing but the code a little less so... Can you just
>> add documentation above vmx_adjust_secondary_exec_control that explains
>> the how/why?
>
> Ya, I'd be more than happy to add a big comment.
>
Ok, I'll wait for v2 of this patch only.
Paolo