Re: [Qemu-devel] [PATCH v7] rtl8139: add vlan support

[Jason Wang <jasowang@redhat.com>]

On 03/23/2011 07:11 AM, Benjamin Poirier wrote:
> Hello,
>
> Here is version 7 of my patchset to add vlan support to the emulated rtl8139
> nic.
>
> Changes since v6:
> 	* added check against guest requesting tagging on frames with len<  12
> 	* simplified tag extraction in receive function. dot1q_buf arg removed
> 	  from rtl8139_do_receive(). Frame is linearized in transfer_frame()
> 	  when loopback mode is on.
> 	* added an entry to file header
>
> I've ran the same tests as usual on linux and this time also freebsd 8.2, with
> and without vlanhwtso in the latter case. Jason, you're right that loopback
> mode is seldom used! It seems the bsd driver only uses it at probe time to
> identify a defect in some 8169 [1,2] and even then, that check has been
> disabled [3]. The linux driver doesn't support loopback mode (unless it's well
> hidden.)
>
> [1] http://lists.freebsd.org/pipermail/freebsd-emulation/2006-May/thread.html#2055
> [2] http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/re/if_re.c?rev=1.196;content-type=text%2Fplain
> [3] http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/dev/re/if_re.c#rev1.68
>
> Changes since v5:
> 	* moved all receive changes to "add vlan tag extraction"
> 	* fixed checkpatch.pl style issues
> 	* fixed bugs in receive case related to small buffers and loopback
> 	  mode. Moved "too small buffer" code back where it used to be, though
> 	  it is changed in content.
>
> Changes since v4:
> 	* removed alloca(), for real. Thanks to the reviewers for their
> 	  patience. This patchset now has more versions than the vlan header
> 	  has bytes!
> 	* corrected the unlikely, debug printf and long lines, as per comments
> 	* cleaned out ifdef's pertaining to ethernet checksum calculation.
> 	  According to a comment since removed they were related to an
> 	  "optimization":
> 	>  RTL8139 provides frame CRC with received packet, this feature
> 	>  seems to be ignored by most drivers, disabled by default
> 	  see commit ccf1d14
>
> I've tested v5 using x86_64 host/guest with the usual procedure. I've also ran
> the clang analyzer on the qemu code base, just for fun.
>
> Changes since v3:
> 	* removed alloca() and #include<net/ethernet.h>  as per comments
> 	* reordered patches to put extraction before insertion. Extraction
> 	  touches only the receive path but insertion touches both. The two
> 	  patches are now needed to have vlan functionnality.
>
> I've tested v4 with x86_64 host/guest. I used the same testing procedure as
> before. I've tested a plain configuration as well as one with tso + vlan
> offload, successfully.
>
> I had to hack around the Linux 8139cp driver to be able to enable tso on vlan
> which leads me to wonder, can someone with access to the C+ spec or a real
> card confirm that it can do tso and vlan offload at the same time? The patch
> I used for the kernel is at https://gist.github.com/851895.
>
> Changes since v2:
> insertion:
> 	* moved insertion later in the process, to handle tso
> 	* use qemu_sendv_packet() to insert the tag for us
> 	* added dot1q_buf parameter to rtl8139_do_receive() to avoid some
> 	  memcpy() in loopback mode. Note that the code path through that
> 	  function is unchanged when dot1q_buf is NULL.
>
> extraction:
> 	* reduced the amount of copying by moving the "frame too short" logic
> 	  after the removal of the vlan tag (as is done in e1000.c for
> 	  example). Unfortunately, that logic can no longer be shared betwen
> 	  C+ and C mode.
>
> I've posted v2 of these patches back in November
> http://article.gmane.org/gmane.comp.emulators.qemu/84252
>
> I've tested v3 on the following combinations of guest and hosts:
> host: x86_64, guest: x86_64
> host: x86_64, guest: ppc32
> host: ppc32, guest: ppc32
>
> Testing on the x86_64 host used '-net tap' and consisted of:
> * making an http transfert on the untagged interface.
> * ping -s 0-1472 to another host on a vlan.
> * making an scp upload to another host on a vlan.
>
> Testing on the ppc32 host used '-net socket' connected to an x86_64 qemu-kvm
> running the virtio nic and consisted of:
> * establishing an ssh connection between the two using an untagged interface.
> * ping -s 0-1472 between the two using a vlan.
> * making an scp transfer in both directions using a vlan.
>
> All that was successful. Nevertheless, it doesn't exercise all code paths so
> care is in order.
>
> Please note that the lack of vlan support in rtl8139 has taken a few people
> aback:
> https://bugzilla.redhat.com/show_bug.cgi?id=516587
> http://article.gmane.org/gmane.linux.network.general/14266
>
> Thanks,
> -Ben
>
Looks good to me.

Acked-by: Jason Wang <jasowang@redhat.com>