Re: [Qemu-devel] [PATCH v2 00/13] Introduce Intel 82574 GbE Controller Emulation (e1000e)

[Jason Wang]

On 02/23/2016 01:37 AM, Leonid Bloch wrote:
> Hello All,
>
> This is v2 of the patches, after the initial reviews.
>
> For convenience, the same patches are available at:
> https://github.com/daynix/qemu-e1000e/tree/e1000e-submit-v2
>
> Best regards,
> Leonid.
>
> Changes since v1:
>
> 1. PCI_PM_CAP_VER_1_1 is defined now in include/hw/pci/pci_regs.h and
>    not in include/standard-headers/linux/pci_regs.h.
> 2. Changes in naming and extra comments in hw/pci/pcie.c and in
>    include/hw/pci/pcie.h.
> 3. Defining pci_dsn_ver and pci_dsn_cap static const variables in
>    hw/pci/pcie.c, instead of PCI_DSN_VER and PCI_DSN_CAP symbolic
>    constants in include/hw/pci/pcie_regs.h.
> 4. Changing the vmxnet3_device_serial_num function in hw/net/vmxnet3.c
>    to avoid the cast when it is called.
> 5. Avoiding a preceding underscore in all the e1000e-related names.
> 6. Minor style changes.
>
> ===================
>
> Hello All,
>
> This series is the final code of the e1000e device emulation, that we
> have developed. Please review, and consider acceptance of these patches
> to the upstream QEMU repository.
>
> The code stability was verified by various traffic tests using Fedora 22
> Linux, and Windows Server 2012R2 guests. Also, Microsoft Hardware
> Certification Kit (HCK) tests were run on a Windows Server 2012R2 guest.

Sounds really cool.

>
> There was a discussion on the possibility of code sharing between the
> e1000e, and the existing e1000 devices. We have reviewed the final code
> for parts that may be shared between this device and the currently
> available e1000 emulation. The device specifications are very different,
> and there are almost no registers, nor functions, that were left as is
> from e1000. The ring descriptor structures were changed as well, by the
> introduction of extended and PS descriptors, as well as additional bits.

How about sharing the codes at function level somehow, e.g
e1000e_receive_filter() and receive_filter() in e1000, the only
difference is e1000's version has tracing.

>
> Additional differences stem from the fact that the e1000e device re-uses
> network packet abstractions introduced by the vmxnet3 device, while the
> e1000 has its own code for packet handling. BTW, it may be worth reusing
> those abstractions in e1000 as well. 

Right, but we have the risk to break e1000.

> (Following these changes the
> vmxnet3 device was successfully tested for possible regressions.)
>
> There are a few minor parts that may be shared, e.g. the default
> register handlers, and the ring management functions. The total amount
> of shared lines will be about 100--150, so we're not sure if it makes
> sense bothering, and taking a risk of breaking e1000, which is a good,
> old, and stable device.

Right, but if the changes in e1000 are small and direct, it may be
worth. I think we don't want to fix bug in two places just because of
codes duplications.

>
> Currently, the e1000e code is stand alone w.r.t. e1000.
>
> Please share your thoughts.

The patch looks good, see individual patches for comments.

One thing I'd expect to add is unit-test. Let's add one to test basic
tx/rx function for e1000e, you may want to refer tests/virtio-net-test.c
for an example.

Thanks

>
> Thanks in advance,
> Dmitry.
>
>
> Changes since RFCv2:
>
> 1. Device functionality verified using Microsoft Hardware Certification Test 
> Kit (HCK)
> 2. Introduced a number of performance improvements
> 3. The code was cleaned, and rebased to the latest master
> 4. Patches verified with checkpatch.pl
>
> ===================
>
> Changes since RFCv1:
>
> 1. Added support for all the device features:
>   - Interrupt moderation.
>   - RSS.
>   - Multiqueue.
> 2. Simulated exact PCI/PCIe configuration space layout.
> 3. Made fixes needed to pass Microsoft's HW certification tests (HCK).
>
> This series is still an RFC, because the following tasks are not done yet:
>
> 1. See which code can be shared between this device and the existing e1000 
> device.
> 2. Rebase patches to the latest master (current base is v2.3.0).
>
> Please share your thoughts,
> Thanks, Dmitry.
>
> ===================
>
> Hello qemu-devel,
>
> This patch series is an RFC for the new networking device emulation
> we're developing for QEMU.
>
> This new device emulates the Intel 82574 GbE Controller and works
> with unmodified Intel e1000e drivers from the Linux/Windows kernels.
>
> The status of the current series is "Functional Device Ready, work
> on Extended Features in Progress".
>
> More precisely, these patches represent a functional device, which
> is recognized by the standard Intel drivers, and is able to transfer
> TX/RX packets with CSO/TSO offloads, according to the spec.
>
> Extended features not supported yet (work in progress):
>   1. TX/RX Interrupt moderation mechanisms
>   2. RSS
>   3. Full-featured multi-queue (use of multiqueued network backend)
>
> Also, there will be some code refactoring and performance
> optimization efforts.
>
> This series was tested on Linux (Fedora 22) and Windows (2012R2)
> guests, using Iperf, with TX/RX and TCP/UDP streams, and various
> packet sizes.
>
> More thorough testing, including data streams with different MTU
> sizes, and Microsoft Certification (HLK) tests, are pending missing
> features' development.
>
> See commit messages (esp. "net: Introduce e1000e device emulation")
> for more information about the development approaches and the
> architecture options chosen for this device.
>
> This series is based upon v2.3.0 tag of the upstream QEMU repository,
> and it will be rebased to latest before the final submission.
>
> Please share your thoughts - any feedback is highly welcomed :)
>
> Best Regards,
> Dmitry Fleytman.
>
> Dmitry Fleytman (13):
>   msix: make msix_clr_pending() visible for clients
>   pci: Introduce define for PM capability version 1.1
>   pcie: Add support for PCIe CAP v1
>   pcie: Introduce function for DSN capability creation
>   vmxnet3: Use generic function for DSN capability definition
>   net: Introduce Toeplitz hash calculator
>   net: Add macros for MAC address tracing
>   vmxnet3: Use common MAC address tracing macros
>   net_pkt: Name vmxnet3 packet abstractions more generic
>   rtl8139: Move more TCP definitions to common header
>   net_pkt: Extend packet abstraction as required by e1000e functionality
>   e1000_regs: Add definitions for Intel 82574-specific bits
>   net: Introduce e1000e device emulation
>
>  MAINTAINERS                |   14 +
>  default-configs/pci.mak    |    1 +
>  docs/specs/e1000e-spec.txt |   17 +
>  hw/net/Makefile.objs       |    5 +-
>  hw/net/e1000_regs.h        |  351 ++++-
>  hw/net/e1000e.c            |  718 +++++++++
>  hw/net/e1000e_core.c       | 3673 
> ++++++++++++++++++++++++++++++++++++++++++++
>  hw/net/e1000e_core.h       |  230 +++
>  hw/net/net_rx_pkt.c        |  599 ++++++++
>  hw/net/net_rx_pkt.h        |  365 +++++
>  hw/net/net_tx_pkt.c        |  631 ++++++++
>  hw/net/net_tx_pkt.h        |  191 +++
>  hw/net/rtl8139.c           |    5 -
>  hw/net/vmxnet3.c           |  108 +-
>  hw/net/vmxnet_debug.h      |    3 -
>  hw/net/vmxnet_rx_pkt.c     |  187 ---
>  hw/net/vmxnet_rx_pkt.h     |  176 ---
>  hw/net/vmxnet_tx_pkt.c     |  581 -------
>  hw/net/vmxnet_tx_pkt.h     |  148 --
>  hw/pci/msix.c              |    2 +-
>  hw/pci/pcie.c              |   94 +-
>  include/hw/pci/msix.h      |    1 +
>  include/hw/pci/pci_regs.h  |    2 +
>  include/hw/pci/pcie.h      |    5 +
>  include/hw/pci/pcie_regs.h |    5 +-
>  include/net/checksum.h     |   49 +-
>  include/net/eth.h          |  161 +-
>  include/net/net.h          |    5 +
>  net/checksum.c             |    7 +-
>  net/eth.c                  |  410 ++++-
>  tests/Makefile             |    4 +-
>  trace-events               |  210 +++
>  32 files changed, 7681 insertions(+), 1277 deletions(-)
>  create mode 100644 docs/specs/e1000e-spec.txt
>  create mode 100644 hw/net/e1000e.c
>  create mode 100644 hw/net/e1000e_core.c
>  create mode 100644 hw/net/e1000e_core.h
>  create mode 100644 hw/net/net_rx_pkt.c
>  create mode 100644 hw/net/net_rx_pkt.h
>  create mode 100644 hw/net/net_tx_pkt.c
>  create mode 100644 hw/net/net_tx_pkt.h
>  delete mode 100644 hw/net/vmxnet_rx_pkt.c
>  delete mode 100644 hw/net/vmxnet_rx_pkt.h
>  delete mode 100644 hw/net/vmxnet_tx_pkt.c
>  delete mode 100644 hw/net/vmxnet_tx_pkt.h
>