Bug 241808 - bhyve e1000 broken after r354288
Summary: bhyve e1000 broken after r354288
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bhyve (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-virtualization mailing list
URL: https://reviews.freebsd.org/D22286
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-08 17:20 UTC by Aleksandr Fedorov
Modified: 2019-11-12 21:08 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Aleksandr Fedorov 2019-11-08 17:20:09 UTC
I found a problem trying to run the following command:

sh /usr/share/examples/bhyve/vmrun.sh -c 2 -m 4096M -n e1000 -t tap1000 -d freebsd-12-0.img freebsd-0

   Autoboot in 9 seconds, hit [Enter] to boot or any other key to stop                                                                                                                            
                                                                                                                                                                                                  
Loading kernel...                                                                                                                                                                                 
/boot/kernel/kernel text=0x1678a68 data=0x1cd288+0x768b40 syms=[0x8+0x174cd8+0x8+0x19224a]                                                                                                        
Loading configured modules...                                                                                                                                                                     
/boot/entropy size=0x1000                                                                                                                                                                         
Error return from kevent change: No such file or directory                
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                                                        
                                                          ---<<BOOT>>---                                                                                                                          
Copyright (c) 1992-2018 The FreeBSD Project.                                                                                                                                                      
Copyright (c) 1979, 1980, 1983, 1986, 1988, 1989, 1991, 1992, 1993, 1994                                                                                                                          
        The Regents of the University of California. All rights reserved.                                                                                                                         
FreeBSD is a registered trademark of The FreeBSD Foundation.                                                                                                                                      
FreeBSD 12.0-RELEASE r341666 GENERIC amd64                                                                                                                                                        
FreeBSD clang version 6.0.1 (tags/RELEASE_601/final 335540) (based on LLVM 6.0.1) 

....

driver bug: Unable to set devclass (class: atkbdc devname: (unknown))
Unhandled ps2 mouse command 0xe1
                                psm0: <PS/2 Mouse> irq 12 on atkbdc0
psm0: [GIANT-LOCKED]
psm0: model Generic PS/2 mouse, device ID 0
Error return from kevent change: No such file or directory
                                                          Error return from kevent change: No such file or directory
                                                                                                                    Error return from kevent change: No such file or directory
                                                                                                                                                                              Error return from ke
vent change: No such file or directory
                                      Error return from kevent change: No such file or directory
                                                                                                uart0: <16550 or compatible> port 0x3f8-0x3ff irq 4 flags 0x10 on acpi0
uart0: console (9600,n,8,1)
uart1: <16550 or compatible> port 0x2f8-0x2ff irq 3 on acpi0
vga0: <Generic ISA VGA> at port 0x3b0-0x3bb iomem 0xb0000-0xb7fff pnpid PNP0900 on isa0



As you can see, kevent () returns with an error when trying to update the list of events: https://svnweb.freebsd.org/base/head/usr.sbin/bhyve/mevent.c?view=markup#l487

The guest virtual machine does not respond to the keyboard, etc.

So, I added some debugging output to find out the cause of the error.

Index: usr.sbin/bhyve/mevent.c                                                                                                                                                                    
===================================================================                                                                                                                               
--- usr.sbin/bhyve/mevent.c     (revision 354546)                                                                                                                                                 
+++ usr.sbin/bhyve/mevent.c     (working copy)                                                                                                                                                    
@@ -200,6 +200,7 @@                                                                                                                                                                               
        mevent_qlock();                                                                                                                                                                           
                                                                                                                                                                                                  
        LIST_FOREACH_SAFE(mevp, &change_head, me_list, tmpp) {
+
                if (mevp->me_closefd) {
                        /*
                         * A close of the file descriptor will remove the
@@ -218,6 +219,8 @@
                        kev[i].flags = mevent_kq_flags(mevp);
                        kev[i].fflags = mevent_kq_fflags(mevp);
                        kev[i].udata = mevp;
+                       printf("%s: fd %d, flags: %04X\r\n", __func__, 
+                                       mevp->me_fd, kev[i].flags);
                        i++;
                }
  
@@ -264,6 +267,8 @@
                return (NULL);
        }
  
+       printf("%s fd: %d\r\n", __func__, tfd);
+
        mevp = NULL;
  
        mevent_qlock();
@@ -338,12 +343,15 @@
        if (evp->me_state == MEV_DEL_PENDING)
                return (EINVAL);
  
+       printf("%s: old state: %d new state: %d fd: %d\r\n", 
+                       __func__, evp->me_state, newstate, evp->me_fd);
+
        /*
         * No update needed if state isn't changing
         */
        if (evp->me_state == newstate)
                return (0);
-
+
        mevent_qlock();
  
        evp->me_state = newstate;
@@ -484,6 +492,9 @@
                if (numev) {
                        ret = kevent(mfd, changelist, numev, NULL, 0, NULL);
                        if (ret == -1) {
+                               for (int i = 0; i < numev; i++)
+                                       printf("%s: %d: descr: %lu flags: %04X\r\n", 
+                                                       __func__, i, changelist[i].ident, changelist[i].flags);
                                perror("Error return from kevent change");
                        }
                }


Guest kernel output:

   Autoboot in 9 seconds, hit [Enter] to boot or any other key to stop                                                                                                                            
                                                                                                                                                                                                  
Loading kernel...                                                                                                                                                                                 
/boot/kernel/kernel text=0x1678a68 data=0x1cd288+0x768b40 syms=[0x8+0x174cd8+0x8+0x19224a]                                                                                                        
Loading configured modules...                                                                                                                                                                     
/boot/entropy size=0x1000                                                                                                                                                                         
mevent_add_state fd: 0      <-- Add stdin to kqueue                                                                                                                                                                      
mevent_add_state fd: 4      <-- Add tapX descriptor to kqueue (https://svnweb.freebsd.org/base/head/usr.sbin/bhyve/net_backends.c?view=markup#l223)                                                                                                                                                                     
mevent_update: old state: 5 new state: 2 fd: 4 <-- Update tapX descriptor state from MEV_ADD_DISABLED to MEV_ENABLE  
                                                                                                                                                 
mevent_add_state fd: 19                                                                                                                                                                           
mevent_add_state fd: 7                                                                                                                                                                            
mevent_build: fd 7, flags: 0001                                                                                                                                                                   
mevent_build: fd 19, flags: 0001       
                                                                                                                                                           
mevent_build: fd 4, flags: 0004 <--- !!!!! Build eventlist for kevent() (tapX descriptor) - struct kevent::flags == EV_ENABLE, But before that, there was no call to kevent () with the EV_ADD flag for this descriptor!!!!  
                                                                                                                                                             
mevent_build: fd 0, flags: 0001                                                                                                                                                                   
mevent_dispatch: 0: descr: 7 flags: 0001                                                                                                                                                          
mevent_dispatch: 1: descr: 19 flags: 0001                                                                                                                                                         
mevent_dispatch: 2: descr: 4 flags: 0004                                                                                                                                                          
mevent_dispatch: 3: descr: 0 flags: 0001                                                                                                                                                          
Error return from kevent change: No such file or directory  <-- We got an error!!!
                                                                                                                                      
                                                          ---<<BOOT>>---                                                                                                                          
Copyright (c) 1992-2018 The FreeBSD Project.                                                                                                                                                      
Copyright (c) 1979, 1980, 1983, 1986, 1988, 1989, 1991, 1992, 1993, 1994 

As I understand, the problem is that the e1000 backend activates the reception of packets too soon - before the kevent () system call is called to add descriptor to kqueue from mevent_dispatch().

If I move the netbe_rx_enable() call to e82545_rx_enable(), the guest machine is working fine:

Index: usr.sbin/bhyve/pci_e82545.c
===================================================================
--- usr.sbin/bhyve/pci_e82545.c (revision 354546)
+++ usr.sbin/bhyve/pci_e82545.c (working copy)
@@ -1526,6 +1526,7 @@
 {
 
        sc->esc_rx_enabled = 1;
+       netbe_rx_enable(sc->esc_be);
 }
 
 static void
@@ -2351,8 +2352,6 @@
                net_genmac(pi, sc->esc_mac.octet);
        }
 
-       netbe_rx_enable(sc->esc_be);
-
        /* H/w initiated reset */
        e82545_reset(sc, 0);
Comment 1 Vincenzo Maffione freebsd_committer 2019-11-08 21:44:03 UTC
Very interesting, thanks for catching it.
I think the real problem here (even before r354288) is that nothing prevents a call to kevent(fd, EV_ENABLE) to happen before kevent(fd, EVADD) is called, as you are reporting.

On mevent_add() and mevent_update(), mevent_notify() is called to wakeup the I/O thread, that will call kevent(changelist) to update the kernel.
A race condition is possible where the client calls mevent_add() and mevent_update(EV_ENABLE) before the I/O thread has the chance to wake up and calls mevent_build()+kevent(changelist), which is exactly what happens in your case.
r354288 only makes this race condition more likely (this explains why I did not catch the bug on my machine when I tested e1000).
Your fix makes the race condition less likely, but I think it's still there.
I tried to rework and simplify mevent.c to make sure EV_ADD is always called before EV_ENABLE or EV_DISABLE.

Let me know what you think about that:
https://reviews.freebsd.org/D22286
Comment 2 Vincenzo Maffione freebsd_committer 2019-11-08 21:55:31 UTC
(In reply to Aleksandr Fedorov from comment #0)

By the way it makes sense to move the call to netbe_rx_enable() where you suggest, and also call netbe_rx_disable() in the corresponding rx_disable function.
Comment 3 commit-hook freebsd_committer 2019-11-12 21:08:49 UTC
A commit references this bug:

Author: vmaffione
Date: Tue Nov 12 21:07:51 UTC 2019
New revision: 354659
URL: https://svnweb.freebsd.org/changeset/base/354659

Log:
  bhyve: rework mevent processing to fix a race condition

  At the end of both mevent_add() and mevent_update(), mevent_notify()
  is called to wakeup the I/O thread, that will call kevent(changelist)
  to update the kernel.
  A race condition is possible where the client calls mevent_add() and
  mevent_update(EV_ENABLE) before the I/O thread has the chance to wake
  up and call mevent_build()+kevent(changelist) in response to mevent_add().
  The mevent_add() is therefore ignored by the I/O thread, and
  kevent(fd, EV_ENABLE) is called before kevent(fd, EV_ADD), resuliting
  in a failure of the kevent(fd, EV_ENABLE) call.

  PR:	241808
  Reviewed by:	jhb, markj
  MFC with:	r354288
  Differential Revision:	https://reviews.freebsd.org/D22286

Changes:
  head/usr.sbin/bhyve/mevent.c