On Thu, May 01, 2025, mlevitsk@xxxxxxxxxx wrote:
> On Tue, 2025-04-22 at 16:33 -0700, Sean Christopherson wrote:
> > > @@ -2653,11 +2654,17 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> > > if (vmx->nested.nested_run_pending &&
> > > (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS)) {
> > > kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
> > > - vmcs_write64(GUEST_IA32_DEBUGCTL, vmcs12->guest_ia32_debugctl);
> > > + new_debugctl = vmcs12->guest_ia32_debugctl;
> > > } else {
> > > kvm_set_dr(vcpu, 7, vcpu->arch.dr7);
> > > - vmcs_write64(GUEST_IA32_DEBUGCTL, vmx->nested.pre_vmenter_debugctl);
> > > + new_debugctl = vmx->nested.pre_vmenter_debugctl;
> > > }
> > > +
> > > + if (CC(!vmx_set_guest_debugctl(vcpu, new_debugctl, false))) {
> >
> > The consistency check belongs in nested_vmx_check_guest_state(), only needs to
> > check the VM_ENTRY_LOAD_DEBUG_CONTROLS case, and should be posted as a separate
> > patch.
>
> I can move it there. Can you explain why though you want this? Is it because of the
> order of checks specified in the PRM?
To be consistent with how KVM checks guest state. The two checks in prepare_vmcs02()
are special cases. vmx_guest_state_valid() consumes a huge variety of state, and
so replicating all of its logic for vmcs12 isn't worth doing. The check on the
kvm_set_msr() for guest_ia32_perf_global_ctrl exists purely so that KVM doesn't
simply ignore the return value.
And to a lesser degree, because KVM assumes that guest state has been sanitized
after nested_vmx_check_guest_state() is called. Violating that risks introducing
bugs, e.g. consuming vmcs12->guest_ia32_debugctl before it's been vetted could
theoretically be problematic.
> Currently GUEST_IA32_DEBUGCTL of the host is *written* in prepare_vmcs02.
> Should I also move this write to nested_vmx_check_guest_state?
No. nested_vmx_check_guest_state() verifies the incoming vmcs12 state,
prepare_vmcs02() merges the vmcs12 state with KVM's desires and fills vmcs02.
> Or should I write the value blindly in prepare_vmcs02 and then check the value
> of 'vmx->msr_ia32_debugctl' in nested_vmx_check_guest_state and fail if the value
> contains reserved bits?
I don't follow. nested_vmx_check_guest_state() is called before prepare_vmcs02().
> > > +bool vmx_set_guest_debugctl(struct kvm_vcpu *vcpu, u64 data, bool host_initiated)
> > > +{
> > > + u64 invalid = data & ~vmx_get_supported_debugctl(vcpu, host_initiated);
> > > +
> > > + if (invalid & (DEBUGCTLMSR_BTF|DEBUGCTLMSR_LBR)) {
> > > + kvm_pr_unimpl_wrmsr(vcpu, MSR_IA32_DEBUGCTLMSR, data);
> > > + data &= ~(DEBUGCTLMSR_BTF|DEBUGCTLMSR_LBR);
> > > + invalid &= ~(DEBUGCTLMSR_BTF|DEBUGCTLMSR_LBR);
> > > + }
> > > +
> > > + if (invalid)
> > > + return false;
> > > +
> > > + if (is_guest_mode(vcpu) && (get_vmcs12(vcpu)->vm_exit_controls &
> > > + VM_EXIT_SAVE_DEBUG_CONTROLS))
> > > + get_vmcs12(vcpu)->guest_ia32_debugctl = data;
> > > +
> > > + if (intel_pmu_lbr_is_enabled(vcpu) && !to_vmx(vcpu)->lbr_desc.event &&
> > > + (data & DEBUGCTLMSR_LBR))
> > > + intel_pmu_create_guest_lbr_event(vcpu);
> > > +
> > > + __vmx_set_guest_debugctl(vcpu, data);
> > > + return true;
> >
> > Return 0/-errno, not true/false.
>
> There are plenty of functions in this file and KVM that return boolean.
That doesn't make them "right". For helpers that are obvious predicates, then
absolutely use a boolean return value. The names for nested_vmx_check_eptp()
and vmx_control_verify() aren't very good, e.g. they should be
nested_vmx_is_valid_eptp() and vmx_is_valid_control(), but the intent is good.
But for flows like modifying guest state, KVM should return 0/-errno.
> e.g:
>
> static bool nested_vmx_check_eptp(struct kvm_vcpu *vcpu, u64 new_eptp)
> static inline bool vmx_control_verify(u32 control, u32 low, u32 high)
> static bool nested_evmcs_handle_vmclear(struct kvm_vcpu *vcpu, gpa_t vmptr)
> static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
> struct vmcs12 *vmcs12)
These two should return 0/-errno.
> static bool nested_vmx_check_eptp(struct kvm_vcpu *vcpu, u64 new_eptp)
> static bool nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
Probably should return 0/-errno, but nested_get_vmcs12_pages() is a bit of a mess.
> ...
>
>
> I personally think that functions that emulate hardware should return boolean
> values or some hardware specific status code (e.g VMX failure code) because
> the real hardware never returns -EINVAL and such.
Real hardware absolutely "returns" granular error codes. KVM even has informal
mappings between some of them, e.g. -EINVAL == #GP, -EFAULT == #PF, -EOPNOTSUPP == #UD,
BUG() == 3-strike #MC.
And hardware has many more ways to report errors to software. E.g. VMLAUNCH can
#UD, #GP(0), VM-Exit, VMfailInvalid, or VMFailValid with 30+ unique reasons. #MC
has a crazy number of possible error encodings. And so on and so forth.
Software visible error codes aside, comparing individual KVM functions to an
overall CPU is wildly misguided. A more appropriate comparison would be between
a KVM function and the ucode for a single instruction/operation. I highly, highly
doubt ucode flows are limited to binary yes/no outputs.
