[Qemu-devel] [PATCH v2 0/2] e1000e: Reimplement e1000 as a variant of e1000e
>From a hardware functionality and register perspective, the Intel 8254x Gigabit Ethernet Controller[1] is roughly a subset of the Intel 82574[2], making it practical to implement e1000 device emulation as an extension of e1000e. That would be a step towards eliminating the existing e1000 implementation--a bunch of semi-redundant code with its own bugs (like the faulty tx checksum offload I reported recently[3]). [1] https://www.intel.com/content/dam/doc/manual/pci-pci-x-family-gbe-controllers-software-dev-manual.pdf [2] https://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf [3] https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03008.html This patch series adds a new e1000-ng device but leaves the existing e1000 device alone. Linux 4.1 and Windows 10 e1000 guest drivers recognize the e1000-ng device and successfully pass traffic on a Linux host. Preliminary performance measurements are encouraging: with e1000-ng, single-threaded TCP iperf shows about a 2x speedup in tx and rx throughput vs e1000, and CPU usage is slighly lower. A ton of work is needed before e1000-ng will be ready to supplant e1000: testing with every random guest OS, fixing bugs, figuring out migration and other missing functionality. There's no way I can do this myself. I'd like to propose a more modest and achievable short-term goal: find and fix any regressions that might affect users of e1000e and e1000, so that the patches can be applied and folks can easily start trying out e1000-ng with their favorite guest OS. That means accepting (for now) known deficiencies in the new e1000-ng devices, which can be addressed by myself and (hopefully) others in follow-up patches. Does that sound reasonable? v2: - added new eecd_state fields to e1000e_vmstate and e1000_vmstate - got savevm/loadvm working with e1000-ng - leave MSI enabled for e1000-ng (though guest drivers seem to ignore it) Ed Swierk (2): e1000e: Infrastructure for e1000-ng e1000e: Add e1000-ng devices hw/net/e1000e.c | 277 +++ hw/net/e1000e_core.c | 249 + hw/net/e1000e_core.h | 25 - 3 files changed, 437 insertions(+), 114 deletions(-) -- 1.9.1
Re: [Qemu-devel] [PATCH v2 0/2] e1000e: Reimplement e1000 as a variant of e1000e
FWIW, it is generally recommended that new versions of patch series be posted as a new top level thread, not in-reply-to the previous version. That way the new posting gets attention near top of a dev's email inbox, as opposed to hidden away as a reply to a weeks old msg at the back of the inbox. On Wed, Nov 08, 2017 at 08:39:15PM -0800, Ed Swierk via Qemu-devel wrote: > From a hardware functionality and register perspective, the Intel > 8254x Gigabit Ethernet Controller[1] is roughly a subset of the Intel > 82574[2], making it practical to implement e1000 device emulation as > an extension of e1000e. > > That would be a step towards eliminating the existing e1000 > implementation--a bunch of semi-redundant code with its own bugs (like > the faulty tx checksum offload I reported recently[3]). > > [1] > https://www.intel.com/content/dam/doc/manual/pci-pci-x-family-gbe-controllers-software-dev-manual.pdf > [2] > https://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf > [3] https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03008.html > > This patch series adds a new e1000-ng device but leaves the existing > e1000 device alone. Linux 4.1 and Windows 10 e1000 guest drivers > recognize the e1000-ng device and successfully pass traffic on a Linux > host. > > Preliminary performance measurements are encouraging: with e1000-ng, > single-threaded TCP iperf shows about a 2x speedup in tx and rx > throughput vs e1000, and CPU usage is slighly lower. That is certainly attractive. > A ton of work is needed before e1000-ng will be ready to supplant > e1000: testing with every random guest OS, fixing bugs, figuring out > migration and other missing functionality. There's no way I can do > this myself. > > I'd like to propose a more modest and achievable short-term goal: find > and fix any regressions that might affect users of e1000e and e1000, > so that the patches can be applied and folks can easily start trying > out e1000-ng with their favorite guest OS. That means accepting (for > now) known deficiencies in the new e1000-ng devices, which can be > addressed by myself and (hopefully) others in follow-up patches. Does > that sound reasonable? My fear is that this approach of building a new e1000-ng device in parallel with having the existing e1000 device is going to cause long term pain, possibly never getting to a state where the e1000-ng device can replace the e1000 device. Any time there needs to be a "big bang" to switch from one impl to another completely different impl always causes trouble IME. With need for migration wire format & state compatibility, this is even more difficult. From a code review POV it will be essentially impossible to have confidence that the new impl can be a viable drop-in replacement for the old impl. Is there really no way that you can change the approach to do a more incremental conversion of the existing code base, but still end up in the same place at the very end ? eg just copy all the e1000.c code into the e1000e.c file to start with. Then gradually merge functional areas over a longish series of patches to eliminate the duplication. This would make it far more practical to identify where any regressions come from, and will give reviewers more confidence that we're not breaking migration compat. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v2 0/2] e1000e: Reimplement e1000 as a variant of e1000e
On Thu, Nov 9, 2017 at 5:53 AM, Daniel P. Berrange wrote: > My fear is that this approach of building a new e1000-ng device in > parallel with having the existing e1000 device is going to cause > long term pain, possibly never getting to a state where the e1000-ng > device can replace the e1000 device. Any time there needs to be a > "big bang" to switch from one impl to another completely different > impl always causes trouble IME. With need for migration wire format > & state compatibility, this is even more difficult. From a code review > POV it will be essentially impossible to have confidence that the new > impl can be a viable drop-in replacement for the old impl. > > Is there really no way that you can change the approach to do a more > incremental conversion of the existing code base, but still end up in > the same place at the very end ? > > eg just copy all the e1000.c code into the e1000e.c file to start with. > Then gradually merge functional areas over a longish series of patches > to eliminate the duplication. This would make it far more practical to > identify where any regressions come from, and will give reviewers more > confidence that we're not breaking migration compat. I agree an incremental conversion is the only realistic way to unearth potential regressions; testing won't be enough. In the past couple of days I've run into several cases where e1000 works but e1000-ng doesn't. Apparently e1000e guest drivers just happen not to tickle these bugs. So e1000-ng isn't ready for prime time anyway. I'll shelve this patch series for the moment. Meanwhile I'll post separate patches for the bugs I've encountered with e1000 UDP checksum offload. --Ed