[PATCH 10/10] KVM: VMX: Convert vcpu_vmx.exit_reason to a union

[Sean Christopherson]

diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index e64da06c7009..2d9a005d11ab 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -93,6 +93,29 @@ struct pt_desc {
struct pt_ctx guest;
};
+union vmx_exit_reason {
+ struct {
+ u32 basic : 16;
+ u32 reserved16 : 1;
+ u32 reserved17 : 1;
+ u32 reserved18 : 1;
+ u32 reserved19 : 1;
+ u32 reserved20 : 1;
+ u32 reserved21 : 1;
+ u32 reserved22 : 1;
+ u32 reserved23 : 1;
+ u32 reserved24 : 1;
+ u32 reserved25 : 1;
+ u32 reserved26 : 1;
+ u32 enclave_mode : 1;
+ u32 smi_pending_mtf : 1;
+ u32 smi_from_vmx_root : 1;
+ u32 reserved30 : 1;
+ u32 failed_vmentry : 1;

Just wondering, is there any particular benefit in using 'u32' instead
of 'u16' here?

Not that I know of. Paranoia that the compiler will do something weird?

+ };
+ u32 full;
+};
+
/*
* The nested_vmx structure is part of vcpu_vmx, and holds information we need
* for correct emulation of VMX (i.e., nested VMX) on this vcpu.
@@ -263,7 +286,7 @@ struct vcpu_vmx {
int vpid;
bool emulation_required;
- u32 exit_reason;
+ union vmx_exit_reason exit_reason;
/* Posted interrupt descriptor */
struct pi_desc pi_desc;

--
Vitaly

[Sean Christopherson]

Use a u16 for nested_vmx_enter_non_root_mode()'s local "exit_reason" to
make it clear the intermediate code is only responsible for setting the
basic exit reason, e.g. FAILED_VMENTRY is unconditionally OR'd in when
emulating a failed VM-Entry.
No functional change intended.
---
arch/x86/kvm/vmx/nested.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 1848ca0116c0..8fbbe2152ab7 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3182,7 +3182,7 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
struct vcpu_vmx *vmx = to_vmx(vcpu);
struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
bool evaluate_pending_interrupts;
- u32 exit_reason = EXIT_REASON_INVALID_STATE;
+ u16 exit_reason = EXIT_REASON_INVALID_STATE;
u32 exit_qual;
evaluate_pending_interrupts = exec_controls_get(vmx) &
@@ -3308,7 +3308,7 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
return NVMX_VMENTRY_VMEXIT;
load_vmcs12_host_state(vcpu, vmcs12);
- vmcs12->vm_exit_reason = exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
+ vmcs12->vm_exit_reason = VMX_EXIT_REASONS_FAILED_VMENTRY | exit_reason;

My personal preference would be to do
(u32)exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY
instead but maybe I'm just not in love with implicit type convertion in C.

Either way works for me. Paolo?

[Paolo Bonzini]

Post by Sean Christopherson

load_vmcs12_host_state(vcpu, vmcs12);
- vmcs12->vm_exit_reason = exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
+ vmcs12->vm_exit_reason = VMX_EXIT_REASONS_FAILED_VMENTRY | exit_reason;

My personal preference would be to do
(u32)exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY
instead but maybe I'm just not in love with implicit type convertion in C.

Either way works for me. Paolo?

Flip a coin? :) I think your version is fine.

Paolo

[Sean Christopherson]

Move the call to nested_vmx_exit_reflected() from vmx_handle_exit() into
nested_vmx_reflect_vmexit() and change the semantics of the return value
for nested_vmx_reflect_vmexit() to indicate whether or not the exit was
reflected into L1. nested_vmx_exit_reflected() and
nested_vmx_reflect_vmexit() are intrinsically tied together, calling one
without simultaneously calling the other makes little sense.
No functional change intended.
---
arch/x86/kvm/vmx/nested.h | 16 +++++++++++-----
arch/x86/kvm/vmx/vmx.c | 4 ++--
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index 21d36652f213..6bc379cf4755 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -72,12 +72,16 @@ static inline bool nested_ept_ad_enabled(struct kvm_vcpu *vcpu)
}
/*
- * Reflect a VM Exit into L1.
+ * Conditionally reflect a VM-Exit into L1. Returns %true if the VM-Exit was
+ * reflected into L1.
*/
-static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
- u32 exit_reason)
+static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
+ u32 exit_reason)
{
- u32 exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
+ u32 exit_intr_info;
+
+ if (!nested_vmx_exit_reflected(vcpu, exit_reason))
+ return false;

(unrelated to your patch)
It's probably just me but 'nested_vmx_exit_reflected()' name always
makes me thinkg 'the vmexit WAS [already] reflected' and not 'the vmexit
NEEDS to be reflected'. 'nested_vmx_exit_needs_reflecting()' maybe?

Not just you. It'd be nice if the name some how reflected (ha) that the
logic is mostly based on whether or not L1 expects the exit, with a few
exceptions. E.g. something like

if (!l1_expects_vmexit(...) && !is_system_vmexit(...))
return false;

The downside of that is the logic is split, which is probably a net loss?

[Sean Christopherson]

Post by Sean Christopherson

-static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
- u32 exit_reason)
+static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
+ u32 exit_reason)
{
- u32 exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
+ u32 exit_intr_info;
+
+ if (!nested_vmx_exit_reflected(vcpu, exit_reason))
+ return false;

(unrelated to your patch)
It's probably just me but 'nested_vmx_exit_reflected()' name always
makes me thinkg 'the vmexit WAS [already] reflected' and not 'the vmexit
NEEDS to be reflected'. 'nested_vmx_exit_needs_reflecting()' maybe?

Not just you. It'd be nice if the name some how reflected (ha) that the
logic is mostly based on whether or not L1 expects the exit, with a few
exceptions. E.g. something like
if (!l1_expects_vmexit(...) && !is_system_vmexit(...))
return false;

Doh, the system VM-Exit logic is backwards, it should be

if (!l1_expects_vmexit(...) || is_system_vmexit(...))
return false;

Post by Sean Christopherson
The downside of that is the logic is split, which is probably a net loss?

[Vitaly Kuznetsov]

Post by Sean Christopherson

Post by Sean Christopherson

-static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
- u32 exit_reason)
+static inline bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
+ u32 exit_reason)
{
- u32 exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
+ u32 exit_intr_info;
+
+ if (!nested_vmx_exit_reflected(vcpu, exit_reason))
+ return false;

(unrelated to your patch)
It's probably just me but 'nested_vmx_exit_reflected()' name always
makes me thinkg 'the vmexit WAS [already] reflected' and not 'the vmexit
NEEDS to be reflected'. 'nested_vmx_exit_needs_reflecting()' maybe?

Not just you. It'd be nice if the name some how reflected (ha) that the
logic is mostly based on whether or not L1 expects the exit, with a few
exceptions. E.g. something like
if (!l1_expects_vmexit(...) && !is_system_vmexit(...))
return false;

Doh, the system VM-Exit logic is backwards, it should be
if (!l1_expects_vmexit(...) || is_system_vmexit(...))
return false;

Post by Sean Christopherson
The downside of that is the logic is split, which is probably a net loss?

Yea,

(just thinking out loud below)

the problem with the split is that we'll have to handle the same exit
reason twice, e.g. EXIT_REASON_EXCEPTION_NMI (is_nmi() check goes to
is_system_vmexit() and vmcs12->exception_bitmap check goes to
l1_expects_vmexit()). Also, we have two 'special' cases: vmx->fail and
nested_run_pending. While the former belongs to to l1_expects_vmexit(),
the later doesn't belong to either (but we can move it to
nested_vmx_reflect_vmexit() I believe).

On the other hand, I'm a great fan of splitting checkers ('pure'
functions) from actors (functions with 'side-effects') and
nested_vmx_exit_reflected() while looking like a checker does a lot of
'acting': nested_mark_vmcs12_pages_dirty(), trace printk.

[Paolo Bonzini]

Post by Vitaly Kuznetsov
On the other hand, I'm a great fan of splitting checkers ('pure'
functions) from actors (functions with 'side-effects') and
nested_vmx_exit_reflected() while looking like a checker does a lot of
'acting': nested_mark_vmcs12_pages_dirty(), trace printk.

Good idea (trace_printk is not a big deal, but
nested_mark_vmcs12_pages_dirty should be done outside). I'll send a
patch, just to show that I can still write KVM code. :)

Paolo

[Sean Christopherson]

Post by Paolo Bonzini

Post by Vitaly Kuznetsov
On the other hand, I'm a great fan of splitting checkers ('pure'
functions) from actors (functions with 'side-effects') and
nested_vmx_exit_reflected() while looking like a checker does a lot of
'acting': nested_mark_vmcs12_pages_dirty(), trace printk.

Good idea (trace_printk is not a big deal, but
nested_mark_vmcs12_pages_dirty should be done outside). I'll send a
patch, just to show that I can still write KVM code. :)

LOL, don't jinx yourself.

[Paolo Bonzini]

Use a u16 to hold the exit reason in vmx_handle_exit_irqoff(), as the
checks for INTR/NMI/WRMSR expect to encounter only the basic exit reason
in vmx->exit_reason.

"No functional change intended."
"Opportunistically align the params to handle_external_interrupt_irqoff()."

Ahah that's perhaps a bit too much, but "no functional change intended"
makes sense.

Paolo

[Paolo Bonzini]

Post by Sean Christopherson

+union vmx_exit_reason {
+ struct {
+ u32 basic : 16;
+ u32 reserved16 : 1;
+ u32 reserved17 : 1;
+ u32 reserved18 : 1;
+ u32 reserved19 : 1;
+ u32 reserved20 : 1;
+ u32 reserved21 : 1;
+ u32 reserved22 : 1;
+ u32 reserved23 : 1;
+ u32 reserved24 : 1;
+ u32 reserved25 : 1;
+ u32 reserved26 : 1;
+ u32 enclave_mode : 1;
+ u32 smi_pending_mtf : 1;
+ u32 smi_from_vmx_root : 1;
+ u32 reserved30 : 1;
+ u32 failed_vmentry : 1;

Just wondering, is there any particular benefit in using 'u32' instead
of 'u16' here?

Not that I know of. Paranoia that the compiler will do something weird?

Since we have an "u32 full" it makes sense to have u32 here too, it
documents that we're matching an u32 field in the other side of the union.

Paolo