Re: [Qemu-devel] [PATCH V8 0/5] VMXNET3 paravirtual NIC device implementation

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, Dec 07, 2012 at 01:15:04PM +0200, Dmitry Fleytman wrote:
> > +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?
> 
> [DF] Yes, you are right. This is copy-pasted from different places.
> [DF] Slips definishing do not fully cover our needs.
> 
> 
> 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.
> 
> [DF] Exaclty! Our intention is to create generic header with network definitions and make everyone use it.
> [DF] We can move our header to some shared place if you want, however I'd do it in parallel with cleanup
> [DF] of similar definitions in existing code and this is a big change that os out of scope of these patches.

Please put it in a generic place in include/qemu/....  Please
double-check copyright headers you need when copy-pasting code from
various system headers.

> > + 
> > *===========================================================================*/
> 
> Is this huge comment box a sign that the code should be split into a
> foo_tx.c and an foo_rx.c file?
> 
> [DF] As for me this file is not that big to be splitted (<800 lines), however I'll do this if you insist :)

It came to mind because I find comment boxes ugly.  They tend to be
pointless "*********** PUBLIC METHODS ***********" or they show that the
code should really be split.  Just a pet peeve but please do split it if
possible.

> > +static inline void vmxnet3_flush_shmem_changes(void)
> > +{
> > +    /*
> > +     * Flush shared memory changes
> > +     * Needed before sending interrupt to guest to ensure
> > +     * it gets consistent memory state
> > +     */
> > +    smp_wmb();
> > +}
> 
> It's useful to document why a memory barrier is being used in each
> instance.  Therefore hiding smp_wmb() inside a wrapper function isn't
> great.
> 
> [DF] Fixed.
> 
> Also, it's suspicious that smb_wmb() is used but no other barriers are
> used.  What about a read memory barrier when accessing shared memory
> written by the guest?
> 
> [DF] VMWARE interface build a in safe way - you always read out data buffer (packet descriptor) with memcopy,
> [DF] and then check its last byte to see whether it was fully filled by driver.
> [DF] In this case device doesn't need read barriers. Drivers need write barriers of course to secure
> [DF] proper order of writes.

That's not portable, you're making assumptions about memory ordering
which varies from architecture to architecture.  Some of the barriers
compile out on x86 because the memory model is strong.

QEMU is portable across host architectures so you need to use the read
memory barrier.

Stefan