Re: [Qemu-devel] [PATCH V7 4/6] Adding common code for VMWARE network devices

[Stefan Hajnoczi]

On Fri, Nov 16, 2012 at 2:55 PM, Dmitry Fleytman <address@hidden> wrote:
> Signed-off-by: Dmitry Fleytman <address@hidden>
> Signed-off-by: Yan Vugenfirer <address@hidden>
> ---
>  hw/vmware_utils.h |  24 ++--
>  hw/vmxnet_debug.h | 121 +++++++++++++++++++
>  hw/vmxnet_utils.c | 219 +++++++++++++++++++++++++++++++++++
>  hw/vmxnet_utils.h | 340 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 692 insertions(+), 12 deletions(-)
>  create mode 100644 hw/vmxnet_debug.h
>  create mode 100644 hw/vmxnet_utils.c
>  create mode 100644 hw/vmxnet_utils.h
>
> diff --git a/hw/vmware_utils.h b/hw/vmware_utils.h
> index d88136a..7f449eb 100644
> --- a/hw/vmware_utils.h
> +++ b/hw/vmware_utils.h
> @@ -29,21 +29,21 @@
>   *
>   */
>  static inline void
> -vmw_shmem_read(target_phys_addr_t addr, void *buf, int len)
> +vmw_shmem_read(hwaddr addr, void *buf, int len)
>  {
>      VMW_SHPRN("SHMEM r: %" PRIx64 ", len: %d to %p", addr, len, buf);
>      cpu_physical_memory_read(addr, buf, len);
>  }

All changes to this file should be squashed with the previous patch.

> +/* #define VMXNET_DEBUG_CB */
> +#define VMXNET_DEBUG_WARNINGS
> +#define VMXNET_DEBUG_ERRORS
> +/* #define VMXNET_DEBUG_INTERRUPTS */
> +/* #define VMXNET_DEBUG_CONFIG */
> +/* #define VMXNET_DEBUG_RINGS */
> +/* #define VMXNET_DEBUG_PACKETS */
> +/* #define VMXNET_DEBUG_SHMEM_ACCESS */
> +
> +#ifdef VMXNET_DEBUG_SHMEM_ACCESS
> +#define VMW_SHPRN(fmt, ...)                                                  
>  \
> +    do {                                                                     
>  \
> +        printf("[%s][SH][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__,      
>  \
> +            ## __VA_ARGS__);                                                 
>  \
> +    } while (0)
> +#else
> +#define VMW_SHPRN(fmt, ...) do {} while (0)
> +#endif

Please use QEMU tracing.  It eliminates all this boilerplate and
conditional compilation.  Tracing can be enabled/disabled at runtime
and works with SystemTap/DTrace.  See docs/tracing.txt.

> diff --git a/hw/vmxnet_utils.h b/hw/vmxnet_utils.h
> new file mode 100644
> index 0000000..7fd9a01
> --- /dev/null
> +++ b/hw/vmxnet_utils.h
> @@ -0,0 +1,340 @@
> +/*
> + * QEMU VMWARE VMXNET* paravirtual NICs - network auxiliary code
> + *
> + * Copyright (c) 2012 Ravello Systems LTD (http://ravellosystems.com)
> + *
> + * Developed by Daynix Computing LTD (http://www.daynix.com)
> + *
> + * Authors:
> + * Dmitry Fleytman <address@hidden>
> + * Tamir Shomer <address@hidden>
> + * Yan Vugenfirer <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef _VMXNET_UTILS_H_
> +#define _VMXNET_UTILS_H_
> +
> +#include <sys/types.h>
> +#include <bswap.h>
> +#include <string.h>
> +#include <hwaddr.h>
> +#include <hw/virtio-net.h>
> +
> +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 */
> +};

Looks like it's copy-pasted stuff from /usr/include/linux/if_*.h,
/usr/include/netinet/*.h, and friends.  If the system-wide headers are
included names will collide for some of the macros at least.

Did you check if the slirp/ definitions can be reused?

I'd rather we import network header definitions once in a generic
place into the source tree.  That way vmxnet and other components
don't need to redefine these structs.