From: Tudor Ambarus <tudor.ambarus@linaro.org>
To: Paolo Bonzini <pbonzini@redhat.com>,
Sean Christopherson <seanjc@google.com>,
Joerg Roedel <joro@8bytes.org>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>,
Wanpeng Li <wanpengli@tencent.com>,
Jim Mattson <jmattson@google.com>,
Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
kvm@vger.kernel.org, iommu@lists.linux-foundation.org,
linux-kernel@vger.kernel.org,
Maxim Levitsky <mlevitsk@redhat.com>,
Lee Jones <joneslee@google.com>
Subject: Re: [PATCH v3 07/26] KVM: VMX: Move preemption timer <=> hrtimer dance to common x86
Date: Wed, 29 Mar 2023 16:22:33 +0100 [thread overview]
Message-ID: <ff078de7-1af8-19fd-2d8c-7a02792245cd@linaro.org> (raw)
In-Reply-To: <244097d2-3d14-6031-7733-62be75036d88@redhat.com>
On 3/29/23 14:47, Paolo Bonzini wrote:
Hi, Paolo!
> On 3/29/23 14:34, Tudor Ambarus wrote:
>> This patch fixes the bug reported at:
>> LINK:
>> https://syzkaller.appspot.com/bug?id=489beb3d76ef14cc6cd18125782dc6f86051a605
>>
>> One may find the strace at:
>> LINK:https://syzkaller.appspot.com/text?tag=CrashLog&x=1798b54ec80000
>> and the c reproducer at:
>> LINK:https://syzkaller.appspot.com/text?tag=ReproC&x=10365781c80000
>>
>> Since I've no experience with kvm, it would be helpful if one of you can
>> provide some guidance. Do you think it is worth to backport this patch
>> to stable (together with its prerequisite patches), or shall I try to
>> get familiar with the code and try to provide a less invasive fix?
>
> I think it is enough to fix the conflicts in vmx_pre_block and
> vmx_post_block, there are no prerequisites:
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 0718658268fe..895069038856 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7577,17 +7577,11 @@ static int vmx_pre_block(struct kvm_vcpu *vcpu)
> if (pi_pre_block(vcpu))
> return 1;
>
> - if (kvm_lapic_hv_timer_in_use(vcpu))
> - kvm_lapic_switch_to_sw_timer(vcpu);
> -
> return 0;
> }
>
> static void vmx_post_block(struct kvm_vcpu *vcpu)
> {
> - if (kvm_x86_ops.set_hv_timer)
> - kvm_lapic_switch_to_hv_timer(vcpu);
> -
> pi_post_block(vcpu);
> }
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fcfa3fedf84f..4eca3ec38afd 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10022,12 +10022,28 @@ static int vcpu_enter_guest(struct kvm_vcpu
> *vcpu)
>
> static inline int vcpu_block(struct kvm *kvm, struct kvm_vcpu *vcpu)
> {
> + bool hv_timer;
> +
> if (!kvm_arch_vcpu_runnable(vcpu) &&
> (!kvm_x86_ops.pre_block || static_call(kvm_x86_pre_block)(vcpu)
> == 0)) {
> + /*
> + * Switch to the software timer before halt-polling/blocking as
> + * the guest's timer may be a break event for the vCPU, and the
> + * hypervisor timer runs only when the CPU is in guest mode.
> + * Switch before halt-polling so that KVM recognizes an expired
> + * timer before blocking.
> + */
> + hv_timer = kvm_lapic_hv_timer_in_use(vcpu);
> + if (hv_timer)
> + kvm_lapic_switch_to_sw_timer(vcpu);
> +
> srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
> kvm_vcpu_block(vcpu);
> vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
>
> + if (hv_timer)
> + kvm_lapic_switch_to_hv_timer(vcpu);
> +
> if (kvm_x86_ops.post_block)
> static_call(kvm_x86_post_block)(vcpu);
>
> @@ -10266,6 +10282,11 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> r = -EINTR;
> goto out;
> }
> + /*
> + * It should be impossible for the hypervisor timer to be in
> + * use before KVM has ever run the vCPU.
> + */
> + WARN_ON_ONCE(kvm_lapic_hv_timer_in_use(vcpu));
> kvm_vcpu_block(vcpu);
> if (kvm_apic_accept_events(vcpu) < 0) {
> r = 0;
>
> The fix is due to the second "if" changing from
> kvm_x86_ops.set_hv_timer to hv_timer.
>
Thanks for the prompt answer! I fixed the conflicts as per your
suggestion and tested the patch with the reproducer on top of
stable/linux-5.15.y and I confirm the reproducer is silenced. Sent the
patch proposal (with you in To:) at:
https://lore.kernel.org/all/20230329151747.2938509-1-tudor.ambarus@linaro.org/T/#u
Feel free to ACK/NACK it.
Cheers,
ta
next prev parent reply other threads:[~2023-03-29 15:22 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-08 1:52 [PATCH v3 00/26] KVM: x86: Halt and APICv overhaul Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 01/26] KVM: fix avic_set_running for preemptable kernels Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 02/26] KVM: nVMX: Ensure vCPU honors event request if posting nested IRQ fails Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 03/26] KVM: VMX: Clean up PI pre/post-block WARNs Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 04/26] KVM: VMX: Handle PI wakeup shenanigans during vcpu_put/load Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 05/26] KVM: Drop unused kvm_vcpu.pre_pcpu field Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 06/26] KVM: Move x86 VMX's posted interrupt list_head to vcpu_vmx Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 07/26] KVM: VMX: Move preemption timer <=> hrtimer dance to common x86 Sean Christopherson
2023-03-29 12:34 ` Tudor Ambarus
2023-03-29 13:47 ` Paolo Bonzini
2023-03-29 15:22 ` Tudor Ambarus [this message]
2023-03-30 7:12 ` Tudor Ambarus
2021-12-08 1:52 ` [PATCH v3 08/26] KVM: x86: Unexport LAPIC's switch_to_{hv,sw}_timer() helpers Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 09/26] KVM: x86: Remove defunct pre_block/post_block kvm_x86_ops hooks Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 10/26] KVM: SVM: Signal AVIC doorbell iff vCPU is in guest mode Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 11/26] KVM: SVM: Don't bother checking for "running" AVIC when kicking for IPIs Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 12/26] KVM: SVM: Remove unnecessary APICv/AVIC update in vCPU unblocking path Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 13/26] KVM: SVM: Use kvm_vcpu_is_blocking() in AVIC load to handle preemption Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 14/26] KVM: SVM: Skip AVIC and IRTE updates when loading blocking vCPU Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 15/26] iommu/amd: KVM: SVM: Use pCPU to infer IsRun state for IRTE Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 16/26] KVM: VMX: Don't do full kick when triggering posted interrupt "fails" Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 17/26] KVM: VMX: Wake vCPU when delivering posted IRQ even if vCPU == this vCPU Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 18/26] KVM: VMX: Pass desired vector instead of bool for triggering posted IRQ Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 19/26] KVM: VMX: Fold fallback path into triggering posted IRQ helper Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 20/26] KVM: VMX: Don't do full kick when handling posted interrupt wakeup Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 21/26] KVM: SVM: Drop AVIC's intermediate avic_set_running() helper Sean Christopherson
2021-12-08 14:43 ` Paolo Bonzini
2021-12-08 15:03 ` Maxim Levitsky
2021-12-08 15:43 ` Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 22/26] KVM: SVM: Move svm_hardware_setup() and its helpers below svm_x86_ops Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 23/26] KVM: SVM: Nullify vcpu_(un)blocking() hooks if AVIC is disabled Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 24/26] KVM: x86: Skip APICv update if APICv is disable at the module level Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 25/26] KVM: x86: Drop NULL check on kvm_x86_ops.check_apicv_inhibit_reasons Sean Christopherson
2021-12-08 1:52 ` [PATCH v3 26/26] KVM: x86: Unexport __kvm_request_apicv_update() Sean Christopherson
2021-12-08 9:04 ` [PATCH v3 00/26] KVM: x86: Halt and APICv overhaul Paolo Bonzini
2021-12-08 14:51 ` Paolo Bonzini
2021-12-08 23:00 ` Maxim Levitsky
2021-12-08 23:16 ` Maxim Levitsky
2021-12-08 23:34 ` Maxim Levitsky
2021-12-09 0:04 ` Sean Christopherson
2021-12-09 6:36 ` Maxim Levitsky
2021-12-09 0:02 ` Sean Christopherson
2021-12-09 14:29 ` Paolo Bonzini
2021-12-09 14:48 ` Maxim Levitsky
2021-12-09 15:45 ` Sean Christopherson
2021-12-09 16:03 ` Maxim Levitsky
2021-12-09 1:37 ` Sean Christopherson
2021-12-09 6:31 ` Maxim Levitsky
2021-12-08 23:43 ` Sean Christopherson
2021-12-09 6:34 ` Maxim Levitsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ff078de7-1af8-19fd-2d8c-7a02792245cd@linaro.org \
--to=tudor.ambarus@linaro.org \
--cc=iommu@lists.linux-foundation.org \
--cc=jmattson@google.com \
--cc=joneslee@google.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mlevitsk@redhat.com \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=suravee.suthikulpanit@amd.com \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).