Hi all,
Today's linux-next merge of the kvm tree got a conflict in:
virt/kvm/arm/arch_timer.c
between commit:
36e5cfd410ad ("KVM: arm/arm64: Properly handle arch-timer IRQs after vtimer_save_state")
from Linus' tree and commits:
70450a9fbe06 ("KVM: arm/arm64: Don't cache the timer IRQ level")
13e59ece5b30 ("KVM: arm/arm64: Fix incorrect timer_is_pending logic")
from the kvm tree.
I fixed it up (I think - see below) and can carry the fix as
necessary. This is now fixed as far as linux-next is concerned, but any
non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging. You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc virt/kvm/arm/arch_timer.c
index cc29a8148328,fb6bd9b9845e..000000000000
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@@ -92,27 -92,18 +92,26 @@@ static irqreturn_t kvm_arch_timer_handl
{
struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id;
struct arch_timer_context *vtimer;
+ u32 cnt_ctl;
- if (!vcpu) {
- pr_warn_once("Spurious arch timer IRQ on non-VCPU thread\n");
- return IRQ_NONE;
- }
+ /*
+ * We may see a timer interrupt after vcpu_put() has been called which
+ * sets the CPU's vcpu pointer to NULL, because even though the timer
+ * has been disabled in vtimer_save_state(), the hardware interrupt
+ * signal may not have been retired from the interrupt controller yet.
+ */
+ if (!vcpu)
+ return IRQ_HANDLED;
vtimer = vcpu_vtimer(vcpu);
- if (!vtimer->irq.level) {
- cnt_ctl = read_sysreg_el0(cntv_ctl);
- cnt_ctl &= ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT |
- ARCH_TIMER_CTRL_IT_MASK;
- if (cnt_ctl == (ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT))
- kvm_timer_update_irq(vcpu, true, vtimer);
- }
-
- if (unlikely(!irqchip_in_kernel(vcpu->kvm)))
- if (kvm_timer_should_fire(vtimer))
++ cnt_ctl = read_sysreg_el0(cntv_ctl);
++ cnt_ctl &= ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT |
++ ARCH_TIMER_CTRL_IT_MASK;
++ if (cnt_ctl == (ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT))
+ kvm_timer_update_irq(vcpu, true, vtimer);
+
+ if (static_branch_unlikely(&userspace_irqchip_in_use) &&
+ unlikely(!irqchip_in_kernel(vcpu->kvm)))
kvm_vtimer_update_mask_user(vcpu);
return IRQ_HANDLED;
Hi,
On Thu, Feb 01, 2018 at 12:55:12PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the kvm tree got a conflict in:
>
> virt/kvm/arm/arch_timer.c
>
> between commit:
>
> 36e5cfd410ad ("KVM: arm/arm64: Properly handle arch-timer IRQs after vtimer_save_state")
>
> from Linus' tree and commits:
>
> 70450a9fbe06 ("KVM: arm/arm64: Don't cache the timer IRQ level")
> 13e59ece5b30 ("KVM: arm/arm64: Fix incorrect timer_is_pending logic")
>
> from the kvm tree.
>
> I fixed it up (I think - see below) and can carry the fix as
> necessary. This is now fixed as far as linux-next is concerned, but any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging. You may also want to consider
> cooperating with the maintainer of the conflicting tree to minimise any
> particularly complex conflicts.
>
While the suggested fix is functional it does result in some code
duplication, and the better resolution is the following:
diff --cc virt/kvm/arm/arch_timer.c
index cc29a8148328,fb6bd9b9845e..000000000000
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@@ -92,27 -92,18 +92,22 @@@ static irqreturn_t kvm_arch_timer_handl
{
struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id;
struct arch_timer_context *vtimer;
- u32 cnt_ctl;
- if (!vcpu) {
- pr_warn_once("Spurious arch timer IRQ on non-VCPU thread\n");
- return IRQ_NONE;
- }
+ /*
+ * We may see a timer interrupt after vcpu_put() has been called which
+ * sets the CPU's vcpu pointer to NULL, because even though the timer
+ * has been disabled in vtimer_save_state(), the hardware interrupt
+ * signal may not have been retired from the interrupt controller yet.
+ */
+ if (!vcpu)
+ return IRQ_HANDLED;
vtimer = vcpu_vtimer(vcpu);
- if (!vtimer->irq.level) {
- cnt_ctl = read_sysreg_el0(cntv_ctl);
- cnt_ctl &= ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT |
- ARCH_TIMER_CTRL_IT_MASK;
- if (cnt_ctl == (ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT))
- kvm_timer_update_irq(vcpu, true, vtimer);
- }
+ if (kvm_timer_should_fire(vtimer))
+ kvm_timer_update_irq(vcpu, true, vtimer);
- if (unlikely(!irqchip_in_kernel(vcpu->kvm)))
+ if (static_branch_unlikely(&userspace_irqchip_in_use) &&
+ unlikely(!irqchip_in_kernel(vcpu->kvm)))
kvm_vtimer_update_mask_user(vcpu);
return IRQ_HANDLED;
Thanks,
-Christoffer
Hi Christoffer,
On Thu, 1 Feb 2018 11:47:07 +0100 Christoffer Dall <[email protected]> wrote:
>
> While the suggested fix is functional it does result in some code
> duplication, and the better resolution is the following:
OK, I will use that resolution form tomorrow on.
Someone needs to remember to let Linus know when the pull request is
sent.
--
Cheers,
Stephen Rothwell
On Fri, Feb 02, 2018 at 12:22:27AM +1100, Stephen Rothwell wrote:
> Hi Christoffer,
>
> On Thu, 1 Feb 2018 11:47:07 +0100 Christoffer Dall <[email protected]> wrote:
> >
> > While the suggested fix is functional it does result in some code
> > duplication, and the better resolution is the following:
>
> OK, I will use that resolution form tomorrow on.
>
> Someone needs to remember to let Linus know when the pull request is
> sent.
>
I've sent a separate notice to Paolo and Radim, so I'm sure they'll take
care of it.
Thanks,
-Christoffer
On 01/02/2018 08:22, Stephen Rothwell wrote:
> Hi Christoffer,
>
> On Thu, 1 Feb 2018 11:47:07 +0100 Christoffer Dall <[email protected]> wrote:
>>
>> While the suggested fix is functional it does result in some code
>> duplication, and the better resolution is the following:
>
> OK, I will use that resolution form tomorrow on.
>
> Someone needs to remember to let Linus know when the pull request is
> sent.
It should be fixed in the KVM tree before it reaches Linus (when we
merge a topic branch that is common between x86/pti & KVM).
Thanks,
Paolo
2018-02-01 09:21-0500, Paolo Bonzini:
> On 01/02/2018 08:22, Stephen Rothwell wrote:
> > Hi Christoffer,
> >
> > On Thu, 1 Feb 2018 11:47:07 +0100 Christoffer Dall <[email protected]> wrote:
> >>
> >> While the suggested fix is functional it does result in some code
> >> duplication, and the better resolution is the following:
> >
> > OK, I will use that resolution form tomorrow on.
> >
> > Someone needs to remember to let Linus know when the pull request is
> > sent.
>
> It should be fixed in the KVM tree before it reaches Linus (when we
> merge a topic branch that is common between x86/pti & KVM).
I wasn't sure if the pti top branch is final, so I pulled hyper-v topic
branch that also also contains v4.15. This and the SEV feature
conflicts should be gone now,
thanks.
On 01/02/2018 10:22, Radim Krčmář wrote:
> 2018-02-01 09:21-0500, Paolo Bonzini:
>> On 01/02/2018 08:22, Stephen Rothwell wrote:
>>> Hi Christoffer,
>>>
>>> On Thu, 1 Feb 2018 11:47:07 +0100 Christoffer Dall <[email protected]> wrote:
>>>>
>>>> While the suggested fix is functional it does result in some code
>>>> duplication, and the better resolution is the following:
>>>
>>> OK, I will use that resolution form tomorrow on.
>>>
>>> Someone needs to remember to let Linus know when the pull request is
>>> sent.
>>
>> It should be fixed in the KVM tree before it reaches Linus (when we
>> merge a topic branch that is common between x86/pti & KVM).
>
> I wasn't sure if the pti top branch is final, so I pulled hyper-v topic
> branch that also also contains v4.15. This and the SEV feature
> conflicts should be gone now,
The pti topic branch is now updated with the v3 patches. So if v3 is
good for you, it is final. :)
Paolo
Hi Radim,
On Thu, 1 Feb 2018 16:22:44 +0100 Radim Krčmář <[email protected]> wrote:
>
> I wasn't sure if the pti top branch is final, so I pulled hyper-v topic
> branch that also also contains v4.15. This and the SEV feature
> conflicts should be gone now,
That merge would have been a good place to add the following merge
resolution fix patch I have been carrying:
From: Eric Biggers <[email protected]>
Subject: KVM: x86: don't forget vcpu_put() in kvm_arch_vcpu_ioctl_set_sregs()
Date: Thu, 21 Dec 2017 01:30:30 +0100
Due to a bad merge resolution between commit f29810335965 ("KVM/x86:
Check input paging mode when cs.l is set") and commit b4ef9d4e8cb8
("KVM: Move vcpu_load to arch-specific kvm_arch_vcpu_ioctl_set_sregs"),
there is a case in kvm_arch_vcpu_ioctl_set_sregs() where vcpu_put() is
not called after vcpu_get(). Fix it.
Reported-by: syzbot <[email protected]>
Signed-off-by: Eric Biggers <[email protected]>
---
arch/x86/kvm/x86.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ea3a98196753..f4e8b5089b28 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7624,7 +7624,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
goto out;
if (kvm_valid_sregs(vcpu, sregs))
- return -EINVAL;
+ goto out;
apic_base_msr.data = sregs->apic_base;
apic_base_msr.host_initiated = true;
--
Cheers,
Stephen Rothwell
2018-02-02 11:20+1100, Stephen Rothwell:
> Hi Radim,
>
> On Thu, 1 Feb 2018 16:22:44 +0100 Radim Krčmář <[email protected]> wrote:
> >
> > I wasn't sure if the pti top branch is final, so I pulled hyper-v topic
> > branch that also also contains v4.15. This and the SEV feature
> > conflicts should be gone now,
>
> That merge would have been a good place to add the following merge
> resolution fix patch I have been carrying:
Yes, should have been there ... I've slapped the patch on top, thanks
for noticing.
> From: Eric Biggers <[email protected]>
> Subject: KVM: x86: don't forget vcpu_put() in kvm_arch_vcpu_ioctl_set_sregs()
> Date: Thu, 21 Dec 2017 01:30:30 +0100
>
> Due to a bad merge resolution between commit f29810335965 ("KVM/x86:
> Check input paging mode when cs.l is set") and commit b4ef9d4e8cb8
> ("KVM: Move vcpu_load to arch-specific kvm_arch_vcpu_ioctl_set_sregs"),
> there is a case in kvm_arch_vcpu_ioctl_set_sregs() where vcpu_put() is
> not called after vcpu_get(). Fix it.
>
> Reported-by: syzbot <[email protected]>
> Signed-off-by: Eric Biggers <[email protected]>
> ---
> arch/x86/kvm/x86.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ea3a98196753..f4e8b5089b28 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7624,7 +7624,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
> goto out;
>
> if (kvm_valid_sregs(vcpu, sregs))
> - return -EINVAL;
> + goto out;
>
> apic_base_msr.data = sregs->apic_base;
> apic_base_msr.host_initiated = true;
>
> --
> Cheers,
> Stephen Rothwell