Messages in this thread | | | From | Vitaly Kuznetsov <> | Subject | Re: [PATCH 01/10] KVM: nVMX: Move reflection check into nested_vmx_reflect_vmexit() | Date | Tue, 17 Mar 2020 18:00:07 +0100 |
| |
Sean Christopherson <sean.j.christopherson@intel.com> writes:
> On Mon, Mar 16, 2020 at 10:33:27PM -0700, Sean Christopherson wrote: >> On Fri, Mar 13, 2020 at 01:12:33PM +0100, Vitaly Kuznetsov wrote: >> > Sean Christopherson <sean.j.christopherson@intel.com> writes: >> > >> > > -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; >> >> 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.
-- Vitaly
| |