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 <m...@redhat.com>
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 <m...@redhat.com>
Implemented suggestions by Anthony Liguori
Reported-by: Anthony Liguori <aligu...@us.ibm.com>
Fixed incorrect checksum caclulation for some packets in SW offloads mode
Reported-by: Gerhard Wiesinger <li...@wiesinger.com>
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 <m...@redhat.com>
Patches regrouped as suggested by Paolo Bonzini
Reported-by: Paolo Bonzini <pbonz...@redhat.com>
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 <li...@wiesinger.com>
Various coding style adjustments and patch split-up as suggested by Anthony
Liguori
Reported-by: Anthony Liguori <aligu...@us.ibm.com>
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 <li...@wiesinger.com>
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
