Re: [PATCH v2 19/25] KVM: nVMX: Add FRED VMCS fields

-- > Signed-off-by: Xin Li xin3.li@intel.com > Tested-by: Shan Kang shan.kang@intel.com > --- > > Change since v1: > * Remove hyperv TLFS related changes (Jeremi Piotrowski). > * Use kvm_cpu_cap_has() instead of cpu_feature_enabled() (Chao Gao). > --- > Documentation/virt/kvm/x86/nested-vmx.rst | 18 +++++ > arch/x86/kvm/vmx/nested.c | 91 +++++++++++++++++++---- > arch/x86/kvm/vmx/vmcs12.c | 18 +++++ > arch/x86/kvm/vmx/vmcs12.h | 36 +++++++++ > arch/x86/kvm/vmx/vmcs_shadow_fields.h | 4 + > 5 files changed, 152 insertions(+), 15 deletions(-) > > diff --git a/Documentation/virt/kvm/x86/nested-vmx.rst b/Documentation/virt/kvm/x86/nested-vmx.rst > index e64ef231f310..87fa9f3877ab 100644 > --- a/Documentation/virt/kvm/x86/nested-vmx.rst > +++ b/Documentation/virt/kvm/x86/nested-vmx.rst > @@ -218,6 +218,24 @@ struct shadow_vmcs is ever changed. > u16 host_gs_selector; > u16 host_tr_selector; > u64 secondary_vm_exit_controls; > + u64 guest_ia32_fred_config; > + u64 guest_ia32_fred_rsp1; > + u64 guest_ia32_fred_rsp2; > + u64 guest_ia32_fred_rsp3; > + u64 guest_ia32_fred_stklvls; > + u64 guest_ia32_fred_ssp1; > + u64 guest_ia32_fred_ssp2; > + u64 guest_ia32_fred_ssp3; > + u64 host_ia32_fred_config; > + u64 host_ia32_fred_rsp1; > + u64 host_ia32_fred_rsp2; > + u64 host_ia32_fred_rsp3; > + u64 host_ia32_fred_stklvls; > + u64 host_ia32_fred_ssp1; > + u64 host_ia32_fred_ssp2; > + u64 host_ia32_fred_ssp3; > + u64 injected_event_data; > + u64 original_event_data; > }; > > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 94da6a0a2f81..f9c1fbeac302 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -686,6 +686,9 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu, > > nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0, > MSR_KERNEL_GS_BASE, MSR_TYPE_RW); > + > + nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0, > + MSR_IA32_FRED_RSP0, MSR_TYPE_RW); > #endif > nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0, > MSR_IA32_SPEC_CTRL, MSR_TYPE_RW); > @@ -2498,6 +2501,8 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct loaded_vmcs *vmcs0 > vmcs12->vm_entry_instruction_len); > vmcs_write32(GUEST_INTERRUPTIBILITY_INFO, > vmcs12->guest_interruptibility_info); > + if (kvm_cpu_cap_has(X86_FEATURE_FRED)) This is wrong, vmcs02 should be set from vmcs12 if and only if the field is enabled in L1's VMX configuration, i.e. iff nested_cpu_has(vmcs12, ???). Note, the ??? should be tied to whatever VMX MSR feature flag enumerates INJECTED_EVENT_DATA. KVM's clearing of X86_FEATURE_FRED when one or more pieces is missing is a software decision, i.e. not archictectural. > + vmcs_write64(INJECTED_EVENT_DATA, vmcs12->injected_event_data); > vmx->loaded_vmcs->nmi_known_unmasked = > !(vmcs12->guest_interruptibility_info & GUEST_INTR_STATE_NMI); > } else { > @@ -2548,6 +2553,17 @@ static void prepare_vmcs02_rare(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12) > vmcs_writel(GUEST_GDTR_BASE, vmcs12->guest_gdtr_base); > vmcs_writel(GUEST_IDTR_BASE, vmcs12->guest_idtr_base); > > + if (kvm_cpu_cap_has(X86_FEATURE_FRED)) { Same thing here. > + vmcs_write64(GUEST_IA32_FRED_CONFIG, vmcs12->guest_ia32_fred_config); > + vmcs_write64(GUEST_IA32_FRED_RSP1, vmcs12->guest_ia32_fred_rsp1); > + vmcs_write64(GUEST_IA32_FRED_RSP2, vmcs12->guest_ia32_fred_rsp2); > + vmcs_write64(GUEST_IA32_FRED_RSP3, vmcs12->guest_ia32_fred_rsp3); > + vmcs_write64(GUEST_IA32_FRED_STKLVLS, vmcs12->guest_ia32_fred_stklvls); > + vmcs_write64(GUEST_IA32_FRED_SSP1, vmcs12->guest_ia32_fred_ssp1); > + vmcs_write64(GUEST_IA32_FRED_SSP2, vmcs12->guest_ia32_fred_ssp2); > + vmcs_write64(GUEST_IA32_FRED_SSP3, vmcs12->guest_ia32_fred_ssp3); > + } > + > vmx->segment_cache.bitmask = 0; > } > > @@ -3835,6 +3851,22 @@ vmcs12_guest_cr4(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > vcpu->arch.cr4_guest_owned_bits)); > } > > +static inline unsigned long > +nested_vmx_get_event_data(struct kvm_vcpu *vcpu, bool for_ex_vmexit) Heh, two form letters for the price of one: #1 Do not use "inline" for functions that are visible only to the local compilation unit. "inline" is just a hint, and modern compilers are smart enough to inline functions when appropriate without a hint. A longer explanation/rant here: https://lore.kernel.org/all/ZAdfX+S323JVWNZC@google.com #2 Do not wrap before the function name. Linus has a nice explanation/rant on this[*]. [*] https://lore.kernel.org/all/CAHk-=wjoLAYG446ZNHfg=GhjSY6nFmuB_wA8fYd5iLBNXjo... > +{ > + struct kvm_queued_exception *ex = for_ex_vmexit ? > + &vcpu->arch.exception_vmexit : &vcpu->arch.exception; > + > + if (ex->has_payload) > + return ex->payload; > + else if (ex->vector == PF_VECTOR) > + return vcpu->arch.cr2; > + else if (ex->vector == DB_VECTOR) > + return (vcpu->arch.dr6 & ~DR6_BT) ^ DR6_ACTIVE_LOW; > + else > + return 0; I'll circle back to this on the next version, i.e. after it's reworked to account for the suggested payload changes. I highly doubt it's correct as-is. > static void vmcs12_save_pending_event(struct kvm_vcpu *vcpu, > struct vmcs12 *vmcs12, > u32 vm_exit_reason, u32 exit_intr_info) > @@ -3842,6 +3874,8 @@ static void vmcs12_save_pending_event(struct kvm_vcpu *vcpu, > u32 idt_vectoring; > unsigned int nr; > > + vmcs12->original_event_data = 0; > + > /* > * Per the SDM, VM-Exits due to double and triple faults are never > * considered to occur during event delivery, even if the double/triple > @@ -3880,6 +3914,12 @@ static void vmcs12_save_pending_event(struct kvm_vcpu *vcpu, > vcpu->arch.exception.error_code; > } > > + idt_vectoring |= vcpu->arch.exception.nested ? > + INTR_INFO_NESTED_EXCEPTION_MASK : 0; Please stop using ternary operators this way. It's less readable and the same number of lines as: if (vcpu->arch.exception.nested) idt_vectoring |= INTR_INFO_NESTED_EXCEPTION_MASK; > + > + vmcs12->original_event_data = > + nested_vmx_get_event_data(vcpu, false); > + > vmcs12->idt_vectoring_info_field = idt_vectoring; > } else if (vcpu->arch.nmi_injected) { > vmcs12->idt_vectoring_info_field = > @@ -3970,19 +4010,7 @@ static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu) > struct kvm_queued_exception *ex = &vcpu->arch.exception_vmexit; > u32 intr_info = ex->vector | INTR_INFO_VALID_MASK; > struct vmcs12 *vmcs12 = get_vmcs12(vcpu); > - unsigned long exit_qual; > - > - if (ex->has_payload) { > - exit_qual = ex->payload; > - } else if (ex->vector == PF_VECTOR) { > - exit_qual = vcpu->arch.cr2; > - } else if (ex->vector == DB_VECTOR) { > - exit_qual = vcpu->arch.dr6; > - exit_qual &= ~DR6_BT; > - exit_qual ^= DR6_ACTIVE_LOW; > - } else { > - exit_qual = 0; > - } > + unsigned long exit_qual = nested_vmx_get_event_data(vcpu, true); This can't possibly be correct, EXIT_QUAL and EVENT_DATA aren't equivalent, e.g. the former doesn't have XFD_ERR, but the latter does. > /* > * Unlike AMD's Paged Real Mode, which reports an error code on #PF > @@ -4003,10 +4031,12 @@ static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu) > intr_info |= INTR_INFO_DELIVER_CODE_MASK; > } > > - if (kvm_exception_is_soft(ex->vector)) > + if (kvm_exception_is_soft(ex->vector)) { > intr_info |= INTR_TYPE_SOFT_EXCEPTION; > - else > + } else { > intr_info |= INTR_TYPE_HARD_EXCEPTION; > + intr_info |= ex->nested ? INTR_INFO_NESTED_EXCEPTION_MASK : 0; Again, if (ex->nested) intr_info |= INTR_INFO_NESTED_EXCEPTION_MASK; > + if (kvm_cpu_cap_has(X86_FEATURE_FRED)) { And here > + vmcs12->guest_ia32_fred_config = vmcs_read64(GUEST_IA32_FRED_CONFIG); > + vmcs12->guest_ia32_fred_rsp1 = vmcs_read64(GUEST_IA32_FRED_RSP1); > + vmcs12->guest_ia32_fred_rsp2 = vmcs_read64(GUEST_IA32_FRED_RSP2); > + vmcs12->guest_ia32_fred_rsp3 = vmcs_read64(GUEST_IA32_FRED_RSP3); > + vmcs12->guest_ia32_fred_stklvls = vmcs_read64(GUEST_IA32_FRED_STKLVLS); > + vmcs12->guest_ia32_fred_ssp1 = vmcs_read64(GUEST_IA32_FRED_SSP1); > + vmcs12->guest_ia32_fred_ssp2 = vmcs_read64(GUEST_IA32_FRED_SSP2); > + vmcs12->guest_ia32_fred_ssp3 = vmcs_read64(GUEST_IA32_FRED_SSP3); > + } > + > vmcs12->guest_pending_dbg_exceptions = > vmcs_readl(GUEST_PENDING_DBG_EXCEPTIONS); > > @@ -4625,6 +4675,17 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu, > vmcs_write32(GUEST_IDTR_LIMIT, 0xFFFF); > vmcs_write32(GUEST_GDTR_LIMIT, 0xFFFF); > > + if (kvm_cpu_cap_has(X86_FEATURE_FRED)) { And here