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

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 :|

On Thu, Nov 9, 2017 at 5:53 AM, Daniel P. Berrange <berrange@redhat.com> 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