| Message ID | 20210929155330.5597-4-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SVM: Add initial GHCB protocol version 2 support | expand |
On Wed, Sep 29, 2021, Joerg Roedel wrote: > #define PFERR_PRESENT_BIT 0 > #define PFERR_WRITE_BIT 1 > @@ -908,6 +913,8 @@ struct kvm_vcpu_arch { > #if IS_ENABLED(CONFIG_HYPERV) > hpa_t hv_root_tdp; > #endif > + > + enum ap_reset_hold_type reset_hold_type; Apologies for very belated feedback... This living in kvm_vcpu_arch came about from feedback (see bottom) that _if_ kvm_emulate_ap_reset_hold() is in x86.c, so should the hold type information. But clearing the hold in SEV here... > void sev_es_unmap_ghcb(struct vcpu_svm *svm) > { > + /* Clear any indication that the vCPU is in a type of AP Reset Hold */ > + svm->vcpu.arch.reset_hold_type = AP_RESET_HOLD_NONE; > + > if (!svm->ghcb) > return; makes this completely unbalanced, i.e. common x86 doesn't clear the reset_hold_type when the vCPU is awakened, despite it being set in common x86. More at the end. > -int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu) > +int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu, > + enum ap_reset_hold_type type) > { > int ret = kvm_skip_emulated_instruction(vcpu); > > + vcpu->arch.reset_hold_type = type; > + > return __kvm_vcpu_halt(vcpu, KVM_MP_STATE_AP_RESET_HOLD, KVM_EXIT_AP_RESET_HOLD) && ret; > } > EXPORT_SYMBOL_GPL(kvm_emulate_ap_reset_hold); ... On Thu, Jul 15, 2021, Tom Lendacky wrote: > On 7/15/21 10:45 AM, Sean Christopherson wrote: > > On Thu, Jul 15, 2021, Tom Lendacky wrote: > >> On 7/14/21 3:17 PM, Sean Christopherson wrote: > >>>> + case GHCB_MSR_AP_RESET_HOLD_REQ: > >>>> + svm->ap_reset_hold_type = AP_RESET_HOLD_MSR_PROTO; > >>>> + ret = kvm_emulate_ap_reset_hold(&svm->vcpu); > >>> > >>> The hold type feels like it should be a param to kvm_emulate_ap_reset_hold(). > >> > >> I suppose it could be, but then the type would have to be tracked in the > >> kvm_vcpu_arch struct instead of the vcpu_svm struct, so I opted for the > >> latter. Maybe a helper function, sev_ap_reset_hold(), that sets the type > >> and then calls kvm_emulate_ap_reset_hold(), but I'm not seeing a big need > >> for it. > > > > Huh. Why is kvm_emulate_ap_reset_hold() in x86.c? That entire concept is very > > much SEV specific. And if anyone argues its not SEV specific, then the hold type > > should also be considered generic, i.e. put in kvm_vcpu_arch. > > That was based on review comments where it was desired that the halt be > identified as specifically from the AP reset hold vs a normal halt. The reason I emphasized "if", is that IMO this patch goes in the wrong direction. My feedback here was that kvm_emulate_ap_reset_hold() and reset_hold_type should tied together. I completely agree with the review comments Tom mentioned, but IMO adding a common kvm_emulate_ap_reset_hold() was the wrong solution. That's very much an SEV specific concept, as demonstrated by this patch. Rather than put more stuff into x86 that really belongs to SEV, what about moving kvm_emulate_ap_reset_hold() into sev.c and instead exporting __kvm_vcpu_halt()? Note, there's a conflict there with a proposed function rename[*], but it's minor and should be trivial to resolve depending how which series wins the race. [*] https://lkml.kernel.org/r/20211009021236.4122790-13-seanjc@google.com
On Wed, Oct 13, 2021 at 10:04:05PM +0000, Sean Christopherson wrote: > Rather than put more stuff into x86 that really belongs to SEV, what about moving > kvm_emulate_ap_reset_hold() into sev.c and instead exporting __kvm_vcpu_halt()? Did that in a separate patch and fixed things up. Also replaced the kvm_ with and sev_ prefix and the function now takes an vcpu_svm parameter. New version coming soon. Thanks, Joerg
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 88f0326c184a..501a55cecd73 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -237,6 +237,11 @@ enum x86_intercept_stage; KVM_GUESTDBG_INJECT_DB | \ KVM_GUESTDBG_BLOCKIRQ) +enum ap_reset_hold_type { + AP_RESET_HOLD_NONE, + AP_RESET_HOLD_NAE_EVENT, + AP_RESET_HOLD_MSR_PROTO, +}; #define PFERR_PRESENT_BIT 0 #define PFERR_WRITE_BIT 1 @@ -908,6 +913,8 @@ struct kvm_vcpu_arch { #if IS_ENABLED(CONFIG_HYPERV) hpa_t hv_root_tdp; #endif + + enum ap_reset_hold_type reset_hold_type; }; struct kvm_lpage_info { @@ -1690,7 +1697,8 @@ int kvm_fast_pio(struct kvm_vcpu *vcpu, int size, unsigned short port, int in); int kvm_emulate_cpuid(struct kvm_vcpu *vcpu); int kvm_emulate_halt(struct kvm_vcpu *vcpu); int kvm_vcpu_halt(struct kvm_vcpu *vcpu); -int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu); +int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu, + enum ap_reset_hold_type type); int kvm_emulate_wbinvd(struct kvm_vcpu *vcpu); void kvm_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg); diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 159b22bb74e4..69653d7838e3 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2246,6 +2246,9 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm) void sev_es_unmap_ghcb(struct vcpu_svm *svm) { + /* Clear any indication that the vCPU is in a type of AP Reset Hold */ + svm->vcpu.arch.reset_hold_type = AP_RESET_HOLD_NONE; + if (!svm->ghcb) return; @@ -2405,6 +2408,11 @@ static u64 ghcb_msr_version_info(void) return msr; } +static u64 ghcb_msr_ap_rst_resp(u64 value) +{ + return (u64)GHCB_MSR_AP_RESET_HOLD_RESP | (value << GHCB_DATA_LOW); +} + static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm) { struct vmcb_control_area *control = &svm->vmcb->control; @@ -2451,6 +2459,16 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm) break; } + case GHCB_MSR_AP_RESET_HOLD_REQ: + ret = kvm_emulate_ap_reset_hold(&svm->vcpu, AP_RESET_HOLD_MSR_PROTO); + + /* + * Preset the result to a non-SIPI return and then only set + * the result to non-zero when delivering a SIPI. + */ + svm->vmcb->control.ghcb_gpa = ghcb_msr_ap_rst_resp(0); + + break; case GHCB_MSR_TERM_REQ: { u64 reason_set, reason_code; @@ -2536,7 +2554,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) ret = svm_invoke_exit_handler(vcpu, SVM_EXIT_IRET); break; case SVM_VMGEXIT_AP_HLT_LOOP: - ret = kvm_emulate_ap_reset_hold(vcpu); + ret = kvm_emulate_ap_reset_hold(vcpu, AP_RESET_HOLD_NAE_EVENT); break; case SVM_VMGEXIT_AP_JUMP_TABLE: { struct kvm_sev_info *sev = &to_kvm_svm(vcpu->kvm)->sev_info; @@ -2671,13 +2689,23 @@ void sev_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector) return; } - /* - * Subsequent SIPI: Return from an AP Reset Hold VMGEXIT, where - * the guest will set the CS and RIP. Set SW_EXIT_INFO_2 to a - * non-zero value. - */ - if (!svm->ghcb) - return; - - ghcb_set_sw_exit_info_2(svm->ghcb, 1); + /* Subsequent SIPI */ + switch (vcpu->arch.reset_hold_type) { + case AP_RESET_HOLD_NAE_EVENT: + /* + * Return from an AP Reset Hold VMGEXIT, where the guest will + * set the CS and RIP. Set SW_EXIT_INFO_2 to a non-zero value. + */ + ghcb_set_sw_exit_info_2(svm->ghcb, 1); + break; + case AP_RESET_HOLD_MSR_PROTO: + /* + * Return from an AP Reset Hold VMGEXIT, where the guest will + * set the CS and RIP. Set GHCB data field to a non-zero value. + */ + svm->vmcb->control.ghcb_gpa = ghcb_msr_ap_rst_resp(1); + break; + default: + break; + } } diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 46ee9bf61df4..1756511b873e 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -8672,10 +8672,13 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu) } EXPORT_SYMBOL_GPL(kvm_emulate_halt); -int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu) +int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu, + enum ap_reset_hold_type type) { int ret = kvm_skip_emulated_instruction(vcpu); + vcpu->arch.reset_hold_type = type; + return __kvm_vcpu_halt(vcpu, KVM_MP_STATE_AP_RESET_HOLD, KVM_EXIT_AP_RESET_HOLD) && ret; } EXPORT_SYMBOL_GPL(kvm_emulate_ap_reset_hold);