[Qemu-devel] [PATCH V7 0/6] VMXNET3 paravirtual NIC device implementation

[Dmitry Fleytman]

NOTE: This is not a final patches for commit,
      they are just for the latest fixes review.
      This set of patches still misses test for the device.

      We didn't succeed to find any guide or sample for the 
      king of tests required (packets transmission).
      If someone can provide a pointer to the relevant
      information we'll be very grateful.

This set of patches implements VMWare VMXNET3 paravirtual NIC device.
The device supports of all the device features including offload capabilties,
VLANs and etc.
The device is tested on different OSes:
    Fedora 15
    Ubuntu 10.4
    Centos 6.2
    Windows 2008R2
    Windows 2008 64bit
    Windows 2008 32bit
    Windows 2003 64bit
    Windows 2003 32bit

Changes in V7:

   Reported-by: Michael S. Tsirkin <address@hidden>
   Issues reported by Michael S. Tsirkin reviewed and mostly fixed:

File vmware_utils.h:

...

> +static inline void
> +vmw_shmem_st64(target_phys_addr_t addr, uint64_t value)
> +{
> +    VMW_SHPRN("SHMEM store64: %" PRIx64 " (value %" PRIx64 ")", addr, value);
> +    stq_le_phys(addr, value);
> +}
> +

Pls remove these wrappers.  These are just memory stores. Our codebase
is too large as it is without every driver wrapping all standard calls.

[DF] Idea behind this macros is to have printout in each of them
[DF] Printouts are really needed when reverse-engineering windows guest drivers
[DF] Since there is no windows drivers code this is the only way to understand
[DF] sequences they use

> +/* MACROS for simplification of operations on array-style registers */

UPPERCASE ABUSE

[DF] Fixed

> +#define IS_MULTIREG_ADDR(addr, base, cnt, regsize)                 \
> +    (((addr + 1) > (base)) && ((addr) < (base) + (cnt) * (regsize)))


Same as range_covers_byte(base, cnt * regsize, addr)?

[DF] Fixed, thanks

> +
> +#define MULTIREG_IDX_BY_ADDR(addr, base, regsize)                  \
> +    (((addr) - (base)) / (regsize))
> +

Above two macros is all that's left. No objection but it does not say
what they do - want to add minimal documentation?
And please prefix with VMWARE_ or something.

[DF] Prefix added, macros documented

File vmxnet_utils.h (and related with the same problems):

...
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef _VMXNET_UTILS_H_
> +#define _VMXNET_UTILS_H_

Please do not start identifiers with _ followed by an uppercase
lattters.

[DF] Fixed

...
> +
> +struct eth_header {
> +    uint8_t  h_dest[ETH_ALEN];   /* destination eth addr */
> +    uint8_t  h_source[ETH_ALEN]; /* source ether addr    */
> +    uint16_t h_proto;            /* packet type ID field */
> +};
> +

And fix struct definitions to
1. follow qemu coding style
[DF] It's not clear what's wrong with the coding style. Please, elaborate.
2. start with vmxnet
[DF] Since this is generic network definitions with no device specifics
[DF] I'm not sure it makes sense to add vmxnet prefix.
[DF] I'd say it makes sense to put them into some generic header files under 
"net" directory instead
[DF] and clean up other devices that have their local definitions of the same 
structures.
[DF] Please, advise.


I also don't really understand why are these
functions split out - vmxnet is the only user, no?

[DF] Currently vmxnet3 is the only user, hovewer we have vmxnet2 implementation 
as well (not submitted yet)
[DF] and it uses the same header

File vmxnet_pkt.h:

...
> +
> +/* tx module context handle */
> +typedef void *VmxnetTxPktH;

This gets you zero type safety and makes debugging impossible.
Just use pointers like everyone does.

[DF] Handle type is added to hide VmxnetTxPkt structure into .c header
[DF] for better encapsulation.
[DF] Should we drop it anyway?

Files vmxnet3.*:

...
> +
> +    if (zero_region) {
> +        vmw_shmem_set(pa, 0, size*cell_size);

spaces around *

[DF] Fixed

...
> +#define vmxnet3_ring_dump(macro, ring_name, ridx, r)                         
> \
> +    macro("%s#%d: base %" PRIx64 " size %lu cell_size %lu gen %d next %lu",  
> \
> +          (ring_name), (ridx),                                               
> \
> +          (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)

make macros upper case

[DF] Fixed

...
> +static inline void vmxnet3_flush_shmem_changes(void)
> +{
> +    /*
> +     * Flush shared memory changes
> +     * Needed before transferring control to guest

what does 'transferring control to guest' mean?
[DF] Changed to 
[DF]    /*
[DF]     * Flush shared memory changes
[DF]     * Needed before sending interrupt to guest to ensure
[DF]     * it gets consistent memory state
[DF]     */

...
> +     */
> +    smp_wmb();
> +}

Don't use wrappers like this. They just hide bugs. For example
it's not helpful before an interrupt in the function below.

[DF] I guess you are talking about vmxnet3_complete_packet()
[DF] Strictly speaking barrier is a must because we change shared memory in 
[DF] vmxnet3_complete_packet()

[DF] And the wrapper is a good thing because its name explains its effect
[DF] in a formal way as opposed to comments

...
> +    switch (status) {
> +    case VMXNET3_PKT_STATUS_OK: {

don't put {} around cases: they align incorrectly
if it's too big move to a function.

[DF] Fixed

...
> +static bool
> +vmxnet3_send_packet(VMXNET3State *s, uint32_t qidx)
> +{
> +    size_t bytes_sent = 0;
> +    bool res = true;

why = true? don't initialize just because.

[DF] Fixed

...
> +/*
> + * VMWARE headers we got from Linux kernel do not fully comply QEMU coding
> + * standards in sense of types and defines used.
> + * Since we didn't want to change VMWARE code, following set of typedefs
> + * and defines needed to compile these headers with QEMU introduced.
> + */

No need for this now.
You can export headers and put them under linux-headers.

[DF] Not sure it is possible because the header as-is is not stand-alone and 
won't compile
[DF] without changes. We extracted definitions we use from their header and 
dropped unused
[DF] and kernel-specific stuff.
[DF} Please, advise.

...

> +        if (VMXNET3_OM_TSO == s->offload_mode) {

Don't do Yoda style like this

[DF] "Yoda" style removed everywhere


Changes in V6:
   Fixed most of problems pointed out by Michael S. Tsirkin
   The only issue still open is creation of shared place
   with generic network structures and functions. Currently
   all generic network code introduced by VMXNET3 resides in
   vmxnet_utils.c/h files. It could be moved to some shared location however
   we believe it is a matter of separate refactoring as there are a lot of 
copy-pasted
   definitions in almost every device and code cleanup efforts requred in order
   to create truly shared codebase.

     Reported-by: Michael S. Tsirkin <address@hidden>

   Implemented suggestions by Anthony Liguori 

     Reported-by: Anthony Liguori <address@hidden>

   Fixed incorrect checksum caclulation for some packets in SW offloads mode

     Reported-by: Gerhard Wiesinger <address@hidden>

Changes in V5:
   MSI-X save/load implemented in the device instead of pci bus as
   suggested by Michael S. Tsirkin

     Reported-by: Michael S. Tsirkin <address@hidden>

   Patches regrouped as suggested by Paolo Bonzini

     Reported-by: Paolo Bonzini <address@hidden>

Changes in V4:
   Fixed a few problems uncovered by NETIO test suit
   Assertion on failure to initialize MSI/MSI-X replaced with warning 
   message and fallback to Legacy/MSI respectively   

     Reported-by: Gerhard Wiesinger <address@hidden>

   Various coding style adjustments and patch split-up as suggested by Anthony 
Liguori
     
     Reported-by: Anthony Liguori <address@hidden>

   Live migration support added

Changes in V3:
   Fixed crash when net device that is used as network fronted has no
   virtio HDR support.
   Task offloads emulation for cases when net device that is used as 
   network fronted has no virtio HDR support.

     Reported-by: Gerhard Wiesinger  <address@hidden>

Changes in V2:
   License text changed accoring to community suggestions
   Standard license header from GPLv2+ - licensed QEMU files used


Dmitry Fleytman (6):
  Adding utility function net_checksum_add_cont() that allows checksum 
       calculation of scattered data with odd chunk sizes
  Adding utility function iov_net_csum_add() for iovec checksum
    calculation     Adding utility function iov_rebuild() for smart
    iovec copy
  Adding common definitions for VMWARE devices
  Adding common code for VMWARE network devices
  Adding packet abstraction for VMWARE network devices
  Adding VMXNET3 device implementation

 Makefile.objs           |    1 +
 default-configs/pci.mak |    1 +
 hw/Makefile.objs        |    1 +
 hw/pci.h                |    1 +
 hw/vmware_utils.h       |  143 +++
 hw/vmxnet3.c            | 2442 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/vmxnet3.h            |  762 +++++++++++++++
 hw/vmxnet_debug.h       |  121 +++
 hw/vmxnet_pkt.c         |  776 +++++++++++++++
 hw/vmxnet_pkt.h         |  311 ++++++
 hw/vmxnet_utils.c       |  219 +++++
 hw/vmxnet_utils.h       |  340 +++++++
 iov.c                   |   53 +
 iov.h                   |   13 +
 net/checksum.c          |   13 +-
 net/checksum.h          |   14 +-
 tests/Makefile          |    2 +-
 17 files changed, 5205 insertions(+), 8 deletions(-)
 create mode 100644 hw/vmware_utils.h
 create mode 100644 hw/vmxnet3.c
 create mode 100644 hw/vmxnet3.h
 create mode 100644 hw/vmxnet_debug.h
 create mode 100644 hw/vmxnet_pkt.c
 create mode 100644 hw/vmxnet_pkt.h
 create mode 100644 hw/vmxnet_utils.c
 create mode 100644 hw/vmxnet_utils.h

-- 
1.7.11.7