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

[Blue Swirl]

Thanks, applied all.

On Wed, Mar 23, 2011 at 1:11 AM, Benjamin Poirier
<address@hidden> 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
>
>