| Message ID | 20211103143929.15264-1-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] KVM: x86: inhibit APICv when KVM_GUESTDBG_BLOCKIRQ active | expand |
On Wed, Nov 03, 2021, Maxim Levitsky wrote: > KVM_GUESTDBG_BLOCKIRQ relies on interrupts being injected using > standard kvm's inject_pending_event, and not via APICv/AVIC. > > Since this is a debug feature, just inhibit APICv/AVIC while > KVM_GUESTDBG_BLOCKIRQ is in use on at least one vCPU. Very clever! > Fixes: 61e5f69ef0837 ("KVM: x86: implement KVM_GUESTDBG_BLOCKIRQ") > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- With the below nits resolved (tested on Intel w/ APICv): Reviewed-and-tested-by: Sean Christopherson <seanjc@google.com> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index ac83d873d65b0..5d30cea58182e 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -10703,6 +10703,25 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, > return ret; > } > > +static void kvm_arch_vcpu_guestdbg_update_apicv_inhibit(struct kvm *kvm) > +{ > + struct kvm_vcpu *vcpu = NULL; vcpu doesn't need to be initialized. > + int i; Nit, I'd prefer we use reverse fir tree when it's convenient, i.e. bool block_irq_used = false; struct kvm_vcpu *vcpu; int i; > + bool block_irq_used = false; > + > + down_write(&kvm->arch.apicv_update_lock); > + > + kvm_for_each_vcpu(i, vcpu, kvm) { > + if (vcpu->guest_debug & KVM_GUESTDBG_BLOCKIRQ) { > + block_irq_used = true; > + break; > + } > + } > + __kvm_request_apicv_update(kvm, !block_irq_used, > + APICV_INHIBIT_REASON_BLOCKIRQ); Heh, this indentation is still messed up, I think you need to change your if (r == -ENOCOFFEE) maxim_get_coffee(); to while (r == -ENOCOFFEE) r = maxim_get_coffee(); > + up_write(&kvm->arch.apicv_update_lock); > +} > + > int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, > struct kvm_guest_debug *dbg) > { > @@ -10755,6 +10774,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, > > static_call(kvm_x86_update_exception_bitmap)(vcpu); > > + kvm_arch_vcpu_guestdbg_update_apicv_inhibit(vcpu->kvm); > + > r = 0; > > out: > -- > 2.26.3 >
On Thu, 2021-11-04 at 19:26 +0000, Sean Christopherson wrote: > On Wed, Nov 03, 2021, Maxim Levitsky wrote: > > KVM_GUESTDBG_BLOCKIRQ relies on interrupts being injected using > > standard kvm's inject_pending_event, and not via APICv/AVIC. > > > > Since this is a debug feature, just inhibit APICv/AVIC while > > KVM_GUESTDBG_BLOCKIRQ is in use on at least one vCPU. > > Very clever! Thanks! It is now possible to enjoy this, after we broke our back making APICv/AVIC inhibition actually work... > > > Fixes: 61e5f69ef0837 ("KVM: x86: implement KVM_GUESTDBG_BLOCKIRQ") > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > With the below nits resolved (tested on Intel w/ APICv): > > Reviewed-and-tested-by: Sean Christopherson <seanjc@google.com> > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index ac83d873d65b0..5d30cea58182e 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -10703,6 +10703,25 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, > > return ret; > > } > > > > +static void kvm_arch_vcpu_guestdbg_update_apicv_inhibit(struct kvm *kvm) > > +{ > > + struct kvm_vcpu *vcpu = NULL; > > vcpu doesn't need to be initialized. True, fixed in v4 > > > + int i; > > Nit, I'd prefer we use reverse fir tree when it's convenient, i.e. Fixed in v4 > > bool block_irq_used = false; > struct kvm_vcpu *vcpu; > int i; > > > + bool block_irq_used = false; > > + > > + down_write(&kvm->arch.apicv_update_lock); > > + > > + kvm_for_each_vcpu(i, vcpu, kvm) { > > + if (vcpu->guest_debug & KVM_GUESTDBG_BLOCKIRQ) { > > + block_irq_used = true; > > + break; > > + } > > + } > > + __kvm_request_apicv_update(kvm, !block_irq_used, > > + APICV_INHIBIT_REASON_BLOCKIRQ); > > Heh, this indentation is still messed up, I think you need to change your > > if (r == -ENOCOFFEE) > maxim_get_coffee(); > > to > > while (r == -ENOCOFFEE) > r = maxim_get_coffee(); Yep :-) > > > + up_write(&kvm->arch.apicv_update_lock); > > +} > > + > > int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, > > struct kvm_guest_debug *dbg) > > { > > @@ -10755,6 +10774,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, > > > > static_call(kvm_x86_update_exception_bitmap)(vcpu); > > > > + kvm_arch_vcpu_guestdbg_update_apicv_inhibit(vcpu->kvm); > > + > > r = 0; > > > > out: > > -- > > 2.26.3 > > Thanks for the review, Best regards, Maxim Levitsky
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 88fce6ab4bbd7..8f6e15b95a4d8 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1034,6 +1034,7 @@ struct kvm_x86_msr_filter { #define APICV_INHIBIT_REASON_IRQWIN 3 #define APICV_INHIBIT_REASON_PIT_REINJ 4 #define APICV_INHIBIT_REASON_X2APIC 5 +#define APICV_INHIBIT_REASON_BLOCKIRQ 6 struct kvm_arch { unsigned long n_used_mmu_pages; diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c index 8052d92069e01..affc0ea98d302 100644 --- a/arch/x86/kvm/svm/avic.c +++ b/arch/x86/kvm/svm/avic.c @@ -904,7 +904,8 @@ bool svm_check_apicv_inhibit_reasons(ulong bit) BIT(APICV_INHIBIT_REASON_NESTED) | BIT(APICV_INHIBIT_REASON_IRQWIN) | BIT(APICV_INHIBIT_REASON_PIT_REINJ) | - BIT(APICV_INHIBIT_REASON_X2APIC); + BIT(APICV_INHIBIT_REASON_X2APIC) | + BIT(APICV_INHIBIT_REASON_BLOCKIRQ); return supported & BIT(bit); } diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 71f54d85f104c..e4fc9ff7cd944 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -7565,7 +7565,8 @@ static void hardware_unsetup(void) static bool vmx_check_apicv_inhibit_reasons(ulong bit) { ulong supported = BIT(APICV_INHIBIT_REASON_DISABLE) | - BIT(APICV_INHIBIT_REASON_HYPERV); + BIT(APICV_INHIBIT_REASON_HYPERV) | + BIT(APICV_INHIBIT_REASON_BLOCKIRQ); return supported & BIT(bit); } diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index ac83d873d65b0..5d30cea58182e 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10703,6 +10703,25 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, return ret; } +static void kvm_arch_vcpu_guestdbg_update_apicv_inhibit(struct kvm *kvm) +{ + struct kvm_vcpu *vcpu = NULL; + int i; + bool block_irq_used = false; + + down_write(&kvm->arch.apicv_update_lock); + + kvm_for_each_vcpu(i, vcpu, kvm) { + if (vcpu->guest_debug & KVM_GUESTDBG_BLOCKIRQ) { + block_irq_used = true; + break; + } + } + __kvm_request_apicv_update(kvm, !block_irq_used, + APICV_INHIBIT_REASON_BLOCKIRQ); + up_write(&kvm->arch.apicv_update_lock); +} + int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, struct kvm_guest_debug *dbg) { @@ -10755,6 +10774,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, static_call(kvm_x86_update_exception_bitmap)(vcpu); + kvm_arch_vcpu_guestdbg_update_apicv_inhibit(vcpu->kvm); + r = 0; out:
KVM_GUESTDBG_BLOCKIRQ relies on interrupts being injected using standard kvm's inject_pending_event, and not via APICv/AVIC. Since this is a debug feature, just inhibit APICv/AVIC while KVM_GUESTDBG_BLOCKIRQ is in use on at least one vCPU. Fixes: 61e5f69ef0837 ("KVM: x86: implement KVM_GUESTDBG_BLOCKIRQ") Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/kvm/svm/avic.c | 3 ++- arch/x86/kvm/vmx/vmx.c | 3 ++- arch/x86/kvm/x86.c | 21 +++++++++++++++++++++ 4 files changed, 26 insertions(+), 2 deletions(-)