| Message ID | 20220811210605.402337-9-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: never write to memory from kvm_vcpu_check_block | expand |
On Thu, Aug 11, 2022, Paolo Bonzini wrote: > Do not return true from kvm_apic_has_events, and consequently from > kvm_vcpu_has_events, if the vCPU is not going to process an INIT. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/i8259.c | 2 +- > arch/x86/kvm/lapic.h | 2 +- > arch/x86/kvm/x86.c | 5 +++++ > arch/x86/kvm/x86.h | 5 ----- > 5 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 293ff678fff5..1ce4ebc41118 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -2042,6 +2042,7 @@ void __user *__x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, > u32 size); > bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu); > bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu); > +bool kvm_vcpu_latch_init(struct kvm_vcpu *vcpu); > > bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq, > struct kvm_vcpu **dest_vcpu); > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c > index e1bb6218bb96..177555eea54e 100644 > --- a/arch/x86/kvm/i8259.c > +++ b/arch/x86/kvm/i8259.c > @@ -29,9 +29,9 @@ > #include <linux/mm.h> > #include <linux/slab.h> > #include <linux/bitops.h> > -#include "irq.h" > +#include <linux/kvm_host.h> > > -#include <linux/kvm_host.h> > +#include "irq.h" > #include "trace.h" > > #define pr_pic_unimpl(fmt, ...) \ > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > index 117a46df5cc1..12577ddccdfc 100644 > --- a/arch/x86/kvm/lapic.h > +++ b/arch/x86/kvm/lapic.h > @@ -225,7 +225,7 @@ static inline bool kvm_vcpu_apicv_active(struct kvm_vcpu *vcpu) > > static inline bool kvm_apic_has_events(struct kvm_vcpu *vcpu) > { > - return lapic_in_kernel(vcpu) && vcpu->arch.apic->pending_events; > + return lapic_in_kernel(vcpu) && vcpu->arch.apic->pending_events && !kvm_vcpu_latch_init(vcpu); Blech, the kvm_apic_has_events() name is awful (as is pending_events), e.g. it really should be kvm_apic_has_pending_sipi_or_init(). To avoid the odd kvm_vcpu_latch_init() declaration and the long line above, what if we open code this in kvm_vcpu_has_events() like we do for NMI, SMI, etc...? And as follow-up, I would love to rename kvm_vcpu_latch_init() => kvm_vcpu_init_blocked(), kvm_apic_has_events(), and pending_events. E.g. for this patch just do: diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 9f11b505cbee..559900736a71 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -12533,7 +12533,8 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu) if (!list_empty_careful(&vcpu->async_pf.done)) return true; - if (kvm_apic_has_events(vcpu)) + /* comment explaning that SIPIs are dropped in this case. */ + if (kvm_apic_has_events(vcpu) && !kvm_vcpu_latch_init(vcpu)) return true; if (vcpu->arch.pv.pv_unhalted)
On Wed, 2022-08-17 at 00:07 +0000, Sean Christopherson wrote: > On Thu, Aug 11, 2022, Paolo Bonzini wrote: > > Do not return true from kvm_apic_has_events, and consequently from > > kvm_vcpu_has_events, if the vCPU is not going to process an INIT. > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > arch/x86/include/asm/kvm_host.h | 1 + > > arch/x86/kvm/i8259.c | 2 +- > > arch/x86/kvm/lapic.h | 2 +- > > arch/x86/kvm/x86.c | 5 +++++ > > arch/x86/kvm/x86.h | 5 ----- > > 5 files changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index 293ff678fff5..1ce4ebc41118 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -2042,6 +2042,7 @@ void __user *__x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, > > u32 size); > > bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu); > > bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu); > > +bool kvm_vcpu_latch_init(struct kvm_vcpu *vcpu); > > > > bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq, > > struct kvm_vcpu **dest_vcpu); > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c > > index e1bb6218bb96..177555eea54e 100644 > > --- a/arch/x86/kvm/i8259.c > > +++ b/arch/x86/kvm/i8259.c > > @@ -29,9 +29,9 @@ > > #include <linux/mm.h> > > #include <linux/slab.h> > > #include <linux/bitops.h> > > -#include "irq.h" > > +#include <linux/kvm_host.h> > > > > -#include <linux/kvm_host.h> > > +#include "irq.h" > > #include "trace.h" > > > > #define pr_pic_unimpl(fmt, ...) \ > > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > > index 117a46df5cc1..12577ddccdfc 100644 > > --- a/arch/x86/kvm/lapic.h > > +++ b/arch/x86/kvm/lapic.h > > @@ -225,7 +225,7 @@ static inline bool kvm_vcpu_apicv_active(struct kvm_vcpu *vcpu) > > > > static inline bool kvm_apic_has_events(struct kvm_vcpu *vcpu) > > { > > - return lapic_in_kernel(vcpu) && vcpu->arch.apic->pending_events; > > + return lapic_in_kernel(vcpu) && vcpu->arch.apic->pending_events && !kvm_vcpu_latch_init(vcpu); > > Blech, the kvm_apic_has_events() name is awful (as is pending_events), e.g. it > really should be kvm_apic_has_pending_sipi_or_init(). 110% agree. > > To avoid the odd kvm_vcpu_latch_init() declaration and the long line above, what > if we open code this in kvm_vcpu_has_events() like we do for NMI, SMI, etc...? > > And as follow-up, I would love to rename kvm_vcpu_latch_init() => kvm_vcpu_init_blocked(), > kvm_apic_has_events(), and pending_events. > > E.g. for this patch just do: > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 9f11b505cbee..559900736a71 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -12533,7 +12533,8 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu) > if (!list_empty_careful(&vcpu->async_pf.done)) > return true; > > - if (kvm_apic_has_events(vcpu)) > + /* comment explaning that SIPIs are dropped in this case. */ > + if (kvm_apic_has_events(vcpu) && !kvm_vcpu_latch_init(vcpu)) > return true; I personally don't know if I prefer this or the original patch. > > if (vcpu->arch.pv.pv_unhalted) > While reviwing this, I noticed that we have this code: static bool svm_apic_init_signal_blocked(struct kvm_vcpu *vcpu) { struct vcpu_svm *svm = to_svm(vcpu); /* * TODO: Last condition latch INIT signals on vCPU when * vCPU is in guest-mode and vmcb12 defines intercept on INIT. * To properly emulate the INIT intercept, * svm_check_nested_events() should call nested_svm_vmexit() * if an INIT signal is pending. */ return !gif_set(svm) || (vmcb_is_intercept(&svm->vmcb->control, INTERCEPT_INIT)); } Is this workaround still needed? svm_check_nested_events does check the apic's INIT/SIPI status. Currently the '.apic_init_signal_blocked' is called from kvm_vcpu_latch_init which itself is currently called from kvm_vcpu_latch_init which happens after we would vmexit if INIT is intercepted by nested hypervisor. Best regards, Maxim Levitsky
On 8/17/22 16:11, Maxim Levitsky wrote: > > While reviwing this, I noticed that we have this code: > > > static bool svm_apic_init_signal_blocked(struct kvm_vcpu *vcpu) > { > struct vcpu_svm *svm = to_svm(vcpu); > > /* > * TODO: Last condition latch INIT signals on vCPU when > * vCPU is in guest-mode and vmcb12 defines intercept on INIT. > * To properly emulate the INIT intercept, > * svm_check_nested_events() should call nested_svm_vmexit() > * if an INIT signal is pending. > */ > return !gif_set(svm) || > (vmcb_is_intercept(&svm->vmcb->control, INTERCEPT_INIT)); > } > > Is this workaround still needed? svm_check_nested_events does check > the apic's INIT/SIPI status. > > Currently the '.apic_init_signal_blocked' is called from > kvm_vcpu_latch_init which itself is currently called from > kvm_vcpu_latch_init which happens after we would vmexit if INIT is > intercepted by nested hypervisor. No, it shouldn't be needed anymore. Paolo
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 293ff678fff5..1ce4ebc41118 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -2042,6 +2042,7 @@ void __user *__x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size); bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu); bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu); +bool kvm_vcpu_latch_init(struct kvm_vcpu *vcpu); bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq, struct kvm_vcpu **dest_vcpu); diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c index e1bb6218bb96..177555eea54e 100644 --- a/arch/x86/kvm/i8259.c +++ b/arch/x86/kvm/i8259.c @@ -29,9 +29,9 @@ #include <linux/mm.h> #include <linux/slab.h> #include <linux/bitops.h> -#include "irq.h" +#include <linux/kvm_host.h> -#include <linux/kvm_host.h> +#include "irq.h" #include "trace.h" #define pr_pic_unimpl(fmt, ...) \ diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h index 117a46df5cc1..12577ddccdfc 100644 --- a/arch/x86/kvm/lapic.h +++ b/arch/x86/kvm/lapic.h @@ -225,7 +225,7 @@ static inline bool kvm_vcpu_apicv_active(struct kvm_vcpu *vcpu) static inline bool kvm_apic_has_events(struct kvm_vcpu *vcpu) { - return lapic_in_kernel(vcpu) && vcpu->arch.apic->pending_events; + return lapic_in_kernel(vcpu) && vcpu->arch.apic->pending_events && !kvm_vcpu_latch_init(vcpu); } static inline bool kvm_lowest_prio_delivery(struct kvm_lapic_irq *irq) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 0f9f24793b8a..5e9358ea112b 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -12529,6 +12529,11 @@ static inline bool kvm_guest_apic_has_interrupt(struct kvm_vcpu *vcpu) static_call(kvm_x86_guest_apic_has_interrupt)(vcpu)); } +bool kvm_vcpu_latch_init(struct kvm_vcpu *vcpu) +{ + return is_smm(vcpu) || static_call(kvm_x86_apic_init_signal_blocked)(vcpu); +} + static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu) { if (!list_empty_careful(&vcpu->async_pf.done)) diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h index 1926d2cb8e79..c333e7cf933a 100644 --- a/arch/x86/kvm/x86.h +++ b/arch/x86/kvm/x86.h @@ -267,11 +267,6 @@ static inline bool kvm_check_has_quirk(struct kvm *kvm, u64 quirk) return !(kvm->arch.disabled_quirks & quirk); } -static inline bool kvm_vcpu_latch_init(struct kvm_vcpu *vcpu) -{ - return is_smm(vcpu) || static_call(kvm_x86_apic_init_signal_blocked)(vcpu); -} - void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip); u64 get_kvmclock_ns(struct kvm *kvm);
Do not return true from kvm_apic_has_events, and consequently from kvm_vcpu_has_events, if the vCPU is not going to process an INIT. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/kvm/i8259.c | 2 +- arch/x86/kvm/lapic.h | 2 +- arch/x86/kvm/x86.c | 5 +++++ arch/x86/kvm/x86.h | 5 ----- 5 files changed, 8 insertions(+), 7 deletions(-)