On Mon, Jan 7, 2013 at 4:14 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
> 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.
>
>
Done.
> > > +
> > >
> *===========================================================================*/
> >
> > 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.
>
>
Did it.
> > > +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
>
Oh, I see, thanks for explanation.
Fixed.
--
Dmitry Fleytman
Technology Expert and Consultant,
Daynix Computing Ltd.
Cell: +972-54-2819481
Skype: dmitry.fleytman
