diff mbox

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

Message ID 20151222155118.GC18343@morn.lan (mailing list archive)
State New, archived
Headers show

Commit Message

Kevin O'Connor Dec. 22, 2015, 3:51 p.m. UTC 
On Tue, Dec 22, 2015 at 02:14:12AM +0000, Gonglei (Arei) wrote:
> > From: Kevin O'Connor [mailto:kevin@koconnor.net]
> > Sent: Tuesday, December 22, 2015 2:47 AM
> > To: Gonglei (Arei)
> > Cc: Xulei (Stone); Paolo Bonzini; qemu-devel; seabios@seabios.org;
> > Huangweidong (C); kvm@vger.kernel.org; Radim Krcmar
> > Subject: Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy
> > problem on qemu-kvm platform
> > 
> > On Mon, Dec 21, 2015 at 09:41:32AM +0000, Gonglei (Arei) wrote:
> > > 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
> > 
> > Does the SeaBIOS patch below help?  
> 
> Sorry, it doesn't work. What's worse is we cannot stop SeaBIOS stuck by
> Setting "CONFIG_ENTRY_EXTRASTACK=n" after applying this patch. 

Oops, can you try with the patch below instead?

> > I'm not familiar with how to "inject a
> > NMI" - can you describe the process in more detail?
> > 
> 
> 1. Qemu Command line:
> 
> #: /home/qemu/x86_64-softmmu/qemu-system-x86_64 -enable-kvm -m 4096 -smp 8 -name suse -vnc 0.0.0.0:10 \
> -device virtio-scsi-pci,id=scsi0 -drive file=/home/suse11_sp3_32_2,if=none,id=drive-scsi0-0-0-0,format=raw,cache=none,aio=native \
> -device scsi-hd,bus=scsi0.0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
> -chardev file,id=seabios,path=/home/seabios.log -device isa-debugcon,iobase=0x402,chardev=seabios \
> -monitor stdio -qmp unix:/tmp/qmp,server,nowait 
> 
> 2. Inject a NMI by QMP:
> 
> #: /home/qemu/scripts/qmp # ./qmp-shell /tmp/qmp
> Welcome to the QMP low-level shell!
> Connected to QEMU 2.5.0
> 
> (QEMU) system_reset
> {"return": {}}
> (QEMU) inject-nmi  
> {"return": {}}
> (QEMU) inject-nmi
> {"return": {}}
> 

I tried a few simple tests but was not able to reproduce.

-Kevin


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

Comments

Gonglei (Arei) Dec. 23, 2015, 6:40 a.m. UTC | #1 
> -----Original Message-----
> From: Kevin O'Connor [mailto:kevin@koconnor.net]
> Sent: Tuesday, December 22, 2015 11:51 PM
> To: Gonglei (Arei)
> Cc: Xulei (Stone); Paolo Bonzini; qemu-devel; seabios@seabios.org;
> Huangweidong (C); kvm@vger.kernel.org; Radim Krcmar
> Subject: Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy
> problem on qemu-kvm platform
> 
> On Tue, Dec 22, 2015 at 02:14:12AM +0000, Gonglei (Arei) wrote:
> > > From: Kevin O'Connor [mailto:kevin@koconnor.net]
> > > Sent: Tuesday, December 22, 2015 2:47 AM
> > > To: Gonglei (Arei)
> > > Cc: Xulei (Stone); Paolo Bonzini; qemu-devel; seabios@seabios.org;
> > > Huangweidong (C); kvm@vger.kernel.org; Radim Krcmar
> > > Subject: Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy
> > > problem on qemu-kvm platform
> > >
> > > On Mon, Dec 21, 2015 at 09:41:32AM +0000, Gonglei (Arei) wrote:
> > > > 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
> > >
> > > Does the SeaBIOS patch below help?
> >
> > Sorry, it doesn't work. What's worse is we cannot stop SeaBIOS stuck by
> > Setting "CONFIG_ENTRY_EXTRASTACK=n" after applying this patch.
> 
> Oops, can you try with the patch below instead?
> 

It works now. Thanks!

But do we need to check other possible situations
that maybe cause *extra stack* broken or overridden?


> > > I'm not familiar with how to "inject a
> > > NMI" - can you describe the process in more detail?
> > >
> >
> > 1. Qemu Command line:
> >
> > #: /home/qemu/x86_64-softmmu/qemu-system-x86_64 -enable-kvm -m 4096
> -smp 8 -name suse -vnc 0.0.0.0:10 \
> > -device virtio-scsi-pci,id=scsi0 -drive
> file=/home/suse11_sp3_32_2,if=none,id=drive-scsi0-0-0-0,format=raw,cache=
> none,aio=native \
> > -device scsi-hd,bus=scsi0.0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
> > -chardev file,id=seabios,path=/home/seabios.log -device
> isa-debugcon,iobase=0x402,chardev=seabios \
> > -monitor stdio -qmp unix:/tmp/qmp,server,nowait
> >
> > 2. Inject a NMI by QMP:
> >
> > #: /home/qemu/scripts/qmp # ./qmp-shell /tmp/qmp
> > Welcome to the QMP low-level shell!
> > Connected to QEMU 2.5.0
> >
> > (QEMU) system_reset
> > {"return": {}}
> > (QEMU) inject-nmi
> > {"return": {}}
> > (QEMU) inject-nmi
> > {"return": {}}
> >
> 
> I tried a few simple tests but was not able to reproduce.
> 
After reset the guest, then you inject an NMI when you see the grub surface
ASAP. 

Kevin, I sent you a picture in private. :)


Regards,
-Gonglei

> -Kevin
> 
> 
> --- a/src/romlayout.S
> +++ b/src/romlayout.S
> @@ -548,7 +548,10 @@ entry_post:
>          ENTRY_INTO32 _cfunc32flat_handle_post   // Normal entry point
> 
>          ORG 0xe2c3
> -        IRQ_ENTRY 02
> +        .global entry_02
> +entry_02:
> +        ENTRY handle_02  // NMI handler does not switch onto extra stack
> +        iretw
> 
>          ORG 0xe3fe
>          .global entry_13_official
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin O'Connor Dec. 23, 2015, 6:06 p.m. UTC | #2 
On Wed, Dec 23, 2015 at 06:40:12AM +0000, Gonglei (Arei) wrote:
> > From: Kevin O'Connor [mailto:kevin@koconnor.net]
> > On Tue, Dec 22, 2015 at 02:14:12AM +0000, Gonglei (Arei) wrote:
> > > Sorry, it doesn't work. What's worse is we cannot stop SeaBIOS stuck by
> > > Setting "CONFIG_ENTRY_EXTRASTACK=n" after applying this patch.
> > 
> > Oops, can you try with the patch below instead?
> > 
> 
> It works now. Thanks!
> 
> But do we need to check other possible situations
> that maybe cause *extra stack* broken or overridden?

I believe the issue is that an NMI could occur while SeaBIOS is
already using its extra stack.  The code is not prepared to switch
into the extra stack while already on the extra stack.  SeaBIOS is
careful to always disable IRQs while running C code to prevent this
issue, but disabling normal IRQs does not disable NMIs.  So, I believe
this issue is specific to the nature of NMIs.

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

Patch

--- a/src/romlayout.S
+++ b/src/romlayout.S
@@ -548,7 +548,10 @@  entry_post:
         ENTRY_INTO32 _cfunc32flat_handle_post   // Normal entry point
 
         ORG 0xe2c3
-        IRQ_ENTRY 02
+        .global entry_02
+entry_02:
+        ENTRY handle_02  // NMI handler does not switch onto extra stack
+        iretw
 
         ORG 0xe3fe
         .global entry_13_official