[Qemu-devel] SeaBios: Fix reset procedure reentrancy problem on qemu-kvm platform

Commit Message

Gonglei (Arei) Dec. 19, 2015, 12:03 p.m. UTC

Hi Kevin,


> -----Original Message-----
> From: Kevin O'Connor [mailto:[e-mail]]
> 
> On Fri, Dec 18, 2015 at 03:04:58AM +0000, Gonglei (Arei) wrote:
> > Hi Kevin & Paolo,
> >
> > Luckily, I reproduced this problem last night. And I got the below log when
> SeaBIOS is stuck.
> [...]
> > [2015-12-18 10:38:10]  gonglei: finish while
> [...]
> > <...>-31509 [035] 154753.180077: kvm_exit: reason EXCEPTION_NMI rip 0x3
> info 0 80000306
> > <...>-31509 [035] 154753.180077: kvm_emulate_insn: 0:3:f0 53 (real)
> > <...>-31509 [035] 154753.180077: kvm_inj_exception: #UD (0x0)
> > <...>-31509 [035] 154753.180077: kvm_entry: vcpu 0
> 
> This is an odd finding.  It seems to indicate that the code is caught
> in an infinite irq loop once irqs are enabled.  What doesn't make
> sense is that an NMI shouldn't depend on the cpu irq enable flag.

Maybe the root cause is not NMI but INTR, so yield() can open hardware interrupt,
And then execute interrupt handler, but the interrupt handler make the SeaBIOS
stack broken, so that the BSP can't execute the instruction and occur exception,
VM_EXIT to Kmod, which is an infinite loop. But I don't have any proofs except
the surface phenomenon.

Kevin, can we drop yield() in smp_setup() ?


Is it really useful and allowable for SeaBIOS? Maybe for other components?
I'm not sure. Because we found that when SeaBIOS is booting, if we inject a
NMI by QMP, the guest will *stuck*. And the kvm tracing log is the same with
the current problem.


Regards,
-Gonglei

> Also, I can't explain why rip would be 0x03, nor why a #UD in an
> exception handler wouldn't result in a triple fault.  Maybe someone
> with more kvm knowledge could help here.
> 
> I did notice that you appear to be running with SeaBIOS v1.8.1 - I
> recommend you upgrade to the latest.  There were two important fixes
> in this area (8b9942fa and 3156b71a).  I don't think either of these
> fixes would explain the log above, but it would be best to eliminate
> the possibility.
> 
> -Kevin
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Kevin O'Connor Dec. 19, 2015, 3:11 p.m. UTC | #1

On Sat, Dec 19, 2015 at 12:03:15PM +0000, Gonglei (Arei) wrote:
> Maybe the root cause is not NMI but INTR, so yield() can open hardware interrupt,
> And then execute interrupt handler, but the interrupt handler make the SeaBIOS
> stack broken, so that the BSP can't execute the instruction and occur exception,
> VM_EXIT to Kmod, which is an infinite loop. But I don't have any proofs except
> the surface phenomenon.

I can't see any reason why allowing interrupts at this location would
be a problem.

> Kevin, can we drop yield() in smp_setup() ?

It's possible to eliminate this instance of yield, but I think it
would just push the crash to the next time interrupts are enabled.

> Is it really useful and allowable for SeaBIOS? Maybe for other components?
> I'm not sure. Because we found that when SeaBIOS is booting, if we inject a
> NMI by QMP, the guest will *stuck*. And the kvm tracing log is the same with
> the current problem.

If you apply the patches you had to prevent that NMI crash problem,
does it also prevent the above crash?

-Kevin
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gonglei (Arei) Dec. 20, 2015, 9:49 a.m. UTC | #2

> -----Original Message-----
> From: Kevin O'Connor [mailto:[e-mail]]
> Sent: Saturday, December 19, 2015 11:12 PM
> On Sat, Dec 19, 2015 at 12:03:15PM +0000, Gonglei (Arei) wrote:
> > Maybe the root cause is not NMI but INTR, so yield() can open hardware
> interrupt,
> > And then execute interrupt handler, but the interrupt handler make the
> SeaBIOS
> > stack broken, so that the BSP can't execute the instruction and occur
> exception,
> > VM_EXIT to Kmod, which is an infinite loop. But I don't have any proofs except
> > the surface phenomenon.
> 
> I can't see any reason why allowing interrupts at this location would
> be a problem.
> 
Does it have any relationship with *extra stack* of SeaBIOS?

> > Kevin, can we drop yield() in smp_setup() ?
> 
> It's possible to eliminate this instance of yield, but I think it
> would just push the crash to the next time interrupts are enabled.
> 
Perhaps. I'm not sure.

> > Is it really useful and allowable for SeaBIOS? Maybe for other components?
> > I'm not sure. Because we found that when SeaBIOS is booting, if we inject a
> > NMI by QMP, the guest will *stuck*. And the kvm tracing log is the same with
> > the current problem.
> 
> If you apply the patches you had to prevent that NMI crash problem,
> does it also prevent the above crash?
> 
Yes, but we cannot prevent the NMI injection (though I'll submit some patches to
forbid users' NMI injection after NMI_EN disabled by RTC bit7 of port 0x70).


Regards,
-Gonglei
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin O'Connor Dec. 20, 2015, 2:33 p.m. UTC | #3

On Sun, Dec 20, 2015 at 09:49:54AM +0000, Gonglei (Arei) wrote:
> > From: Kevin O'Connor [mailto:[e-mail]]
> > Sent: Saturday, December 19, 2015 11:12 PM
> > On Sat, Dec 19, 2015 at 12:03:15PM +0000, Gonglei (Arei) wrote:
> > > Maybe the root cause is not NMI but INTR, so yield() can open hardware
> > interrupt,
> > > And then execute interrupt handler, but the interrupt handler make the
> > SeaBIOS
> > > stack broken, so that the BSP can't execute the instruction and occur
> > exception,
> > > VM_EXIT to Kmod, which is an infinite loop. But I don't have any proofs except
> > > the surface phenomenon.
> > 
> > I can't see any reason why allowing interrupts at this location would
> > be a problem.
> > 
> Does it have any relationship with *extra stack* of SeaBIOS?

None that I can see.  Also, the kvm trace seems to show the code
trying to execute at rip=0x03 - that will crash long before the extra
stack is used.

> > > Kevin, can we drop yield() in smp_setup() ?
> > 
> > It's possible to eliminate this instance of yield, but I think it
> > would just push the crash to the next time interrupts are enabled.
> > 
> Perhaps. I'm not sure.
> 
> > > Is it really useful and allowable for SeaBIOS? Maybe for other components?
> > > I'm not sure. Because we found that when SeaBIOS is booting, if we inject a
> > > NMI by QMP, the guest will *stuck*. And the kvm tracing log is the same with
> > > the current problem.
> > 
> > If you apply the patches you had to prevent that NMI crash problem,
> > does it also prevent the above crash?
> > 
> Yes, but we cannot prevent the NMI injection (though I'll submit some patches to
> forbid users' NMI injection after NMI_EN disabled by RTC bit7 of port 0x70).
> 

-Kevin
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gonglei (Arei) Dec. 21, 2015, 9:41 a.m. UTC | #4

Dear Kevin,

> -----Original Message-----
> From: Kevin O'Connor [mailto:[e-mail]]
> Sent: Sunday, December 20, 2015 10:33 PM
> To: Gonglei (Arei)
> Cc: Xulei (Stone); Paolo Bonzini; qemu-devel; [e-mail];
> Huangweidong (C); [e-mail]; Radim Krcmar
> Subject: Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy
> problem on qemu-kvm platform
> 
> On Sun, Dec 20, 2015 at 09:49:54AM +0000, Gonglei (Arei) wrote:
> > > From: Kevin O'Connor [mailto:[e-mail]]
> > > Sent: Saturday, December 19, 2015 11:12 PM
> > > On Sat, Dec 19, 2015 at 12:03:15PM +0000, Gonglei (Arei) wrote:
> > > > Maybe the root cause is not NMI but INTR, so yield() can open hardware
> > > interrupt,
> > > > And then execute interrupt handler, but the interrupt handler make the
> > > SeaBIOS
> > > > stack broken, so that the BSP can't execute the instruction and occur
> > > exception,
> > > > VM_EXIT to Kmod, which is an infinite loop. But I don't have any proofs
> except
> > > > the surface phenomenon.
> > >
> > > I can't see any reason why allowing interrupts at this location would
> > > be a problem.
> > >
> > Does it have any relationship with *extra stack* of SeaBIOS?
> 
> None that I can see.  Also, the kvm trace seems to show the code
> trying to execute at rip=0x03 - that will crash long before the extra
> stack is used.
> 
When the gurb of OS is booting, then the softirq and C function send_disk_op()
may use extra stack of SeaBIOS. If we inject a NMI, romlayout.S: irqentry_extrastack
is invoked, and the extra stack will be used again. And the stack of first calling
will be broken, so that the SeaBIOS stuck. 

You can easily reproduce the problem.

1. start on guest
2. reset the guest
3. inject a NMI when the guest show the grub surface
4. then the guest stuck

If we disabled extra stack by setting

 CONFIG_ENTRY_EXTRASTACK=n

Then the problem is gone.

Besides, I have another thought:

Is it possible when one cpu is using the extra stack, but other cpus (APs)
still be waked up by hardware interrupt after yield() or br->flags = F_IF 
and used the extra stack again?


Regards,
-Gonglei

> > > > Kevin, can we drop yield() in smp_setup() ?
> > >
> > > It's possible to eliminate this instance of yield, but I think it
> > > would just push the crash to the next time interrupts are enabled.
> > >
> > Perhaps. I'm not sure.
> >
> > > > Is it really useful and allowable for SeaBIOS? Maybe for other
> components?
> > > > I'm not sure. Because we found that when SeaBIOS is booting, if we inject
> a
> > > > NMI by QMP, the guest will *stuck*. And the kvm tracing log is the same
> with
> > > > the current problem.
> > >
> > > If you apply the patches you had to prevent that NMI crash problem,
> > > does it also prevent the above crash?
> > >
> > Yes, but we cannot prevent the NMI injection (though I'll submit some
> patches to
> > forbid users' NMI injection after NMI_EN disabled by RTC bit7 of port 0x70).
> >
> 
> -Kevin
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/src/fw/smp.c b/src/fw/smp.c
index 579acdb..dd23eda 100644
--- a/src/fw/smp.c
+++ b/src/fw/smp.c
@@ -136,7 +136,6 @@  smp_setup(void)
             "  jc 1b\n"
             : "+m" (SMPLock), "+m" (SMPStack)
             : : "cc", "memory");
-    yield();
 
     // Restore memory.
     *(u64*)BUILD_AP_BOOT_ADDR = old;