[Qemu-devel] [PATCH v2 1/2] rtl8139: add vlan tag insertion

[Qemu-devel] [PATCH v2 1/2] rtl8139: add vlan tag insertion

[Benjamin Poirier]

Add support to the emulated hardware to add vlan tags in packets going
from the guest to the network.

Signed-off-by: Benjamin Poirier <benjamin.poirier@polymtl.ca>
Cc: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
Changes since v1:
* moved the debug print statement inside the if block and reworded
  accordingly. (as suggested by Igor)

 hw/rtl8139.c |   45 ++++++++++++++++++++++++++++++++++-----------
 1 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index d92981d..b599945 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -47,6 +47,8 @@
  *                                  Darwin)
  */
 
+#include <net/ethernet.h>
+
 #include "hw.h"
 #include "pci.h"
 #include "qemu-timer.h"
@@ -58,6 +60,10 @@
 
 #define PCI_FREQUENCY 33000000L
 
+/* bytes in VLAN tag */
+#define VLAN_TCI_LEN 2
+#define VLAN_HDR_LEN (ETHER_TYPE_LEN + VLAN_TCI_LEN)
+
 /* debug RTL8139 card C+ mode only */
 //#define DEBUG_RTL8139CP 1
 
@@ -1913,7 +1919,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 
     cpu_physical_memory_read(cplus_tx_ring_desc,    (uint8_t *)&val, 4);
     txdw0 = le32_to_cpu(val);
-    /* TODO: implement VLAN tagging support, VLAN tag data is read to txdw1 */
     cpu_physical_memory_read(cplus_tx_ring_desc+4,  (uint8_t *)&val, 4);
     txdw1 = le32_to_cpu(val);
     cpu_physical_memory_read(cplus_tx_ring_desc+8,  (uint8_t *)&val, 4);
@@ -1925,9 +1930,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
            descriptor,
            txdw0, txdw1, txbufLO, txbufHI));
 
-    /* TODO: the following discard cast should clean clang analyzer output */
-    (void)txdw1;
-
 /* w0 ownership flag */
 #define CP_TX_OWN (1<<31)
 /* w0 end of ring flag */
@@ -1951,8 +1953,8 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 /* w0 bits 0...15 : buffer size */
 #define CP_TX_BUFFER_SIZE (1<<16)
 #define CP_TX_BUFFER_SIZE_MASK (CP_TX_BUFFER_SIZE - 1)
-/* w1 tag available flag */
-#define CP_RX_TAGC (1<<17)
+/* w1 add tag flag */
+#define CP_TX_TAGC (1<<17)
 /* w1 bits 0...15 : VLAN tag */
 #define CP_TX_VLAN_TAG_MASK ((1<<16) - 1)
 /* w2 low  32bit of Rx buffer ptr */
@@ -1978,12 +1980,21 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 
     DEBUG_PRINT(("RTL8139: +++ C+ Tx mode : transmitting from descriptor %d\n", descriptor));
 
+    int vlan_extra_size = 0;
     if (txdw0 & CP_TX_FS)
     {
         DEBUG_PRINT(("RTL8139: +++ C+ Tx mode : descriptor %d is first segment descriptor\n", descriptor));
 
         /* reset internal buffer offset */
         s->cplus_txbuffer_offset = 0;
+
+        if (txdw1 & CP_TX_TAGC)
+        {
+            vlan_extra_size = VLAN_HDR_LEN;
+
+            DEBUG_PRINT(("RTL8139: +++ C+ Tx mode : inserting vlan tag with "
+                    "tci: %u\n", bswap16(txdw1 & CP_TX_VLAN_TAG_MASK)));
+        }
     }
 
     int txsize = txdw0 & CP_TX_BUFFER_SIZE_MASK;
@@ -1992,14 +2003,15 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
     /* make sure we have enough space to assemble the packet */
     if (!s->cplus_txbuffer)
     {
-        s->cplus_txbuffer_len = CP_TX_BUFFER_SIZE;
+        s->cplus_txbuffer_len = CP_TX_BUFFER_SIZE + VLAN_HDR_LEN;
         s->cplus_txbuffer = qemu_malloc(s->cplus_txbuffer_len);
         s->cplus_txbuffer_offset = 0;
 
         DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer allocated space %d\n", s->cplus_txbuffer_len));
     }
 
-    while (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize >= s->cplus_txbuffer_len)
+    while (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize +
+        vlan_extra_size >= s->cplus_txbuffer_len)
     {
         s->cplus_txbuffer_len += CP_TX_BUFFER_SIZE;
         s->cplus_txbuffer = qemu_realloc(s->cplus_txbuffer, s->cplus_txbuffer_len);
@@ -2025,6 +2037,20 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
     DEBUG_PRINT(("RTL8139: +++ C+ mode transmit reading %d bytes from host memory at %016" PRIx64 " to offset %d\n",
                  txsize, (uint64_t)tx_addr, s->cplus_txbuffer_offset));
 
+    if (vlan_extra_size && txsize >= 2 * ETHER_ADDR_LEN)
+    {
+        /* copy addresses */
+        cpu_physical_memory_read(tx_addr, s->cplus_txbuffer, 2 *
+            ETHER_ADDR_LEN);
+        tx_addr += 2 * ETHER_ADDR_LEN;
+        txsize -= 2 * ETHER_ADDR_LEN;
+        /* insert vlan tag */
+        *(uint16_t *)(s->cplus_txbuffer + 2 * ETHER_ADDR_LEN) =
+            cpu_to_be16(ETHERTYPE_VLAN);
+        *(uint16_t *)(s->cplus_txbuffer + 2 * ETHER_ADDR_LEN + ETHER_TYPE_LEN)
+            = cpu_to_le16(txdw1 & CP_TX_VLAN_TAG_MASK);
+        s->cplus_txbuffer_offset += 2 * ETHER_ADDR_LEN + VLAN_HDR_LEN;
+    }
     cpu_physical_memory_read(tx_addr, s->cplus_txbuffer + s->cplus_txbuffer_offset, txsize);
     s->cplus_txbuffer_offset += txsize;
 
@@ -2053,9 +2079,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
     /* update ring data */
     val = cpu_to_le32(txdw0);
     cpu_physical_memory_write(cplus_tx_ring_desc,    (uint8_t *)&val, 4);
-    /* TODO: implement VLAN tagging support, VLAN tag data is read to txdw1 */
-//    val = cpu_to_le32(txdw1);
-//    cpu_physical_memory_write(cplus_tx_ring_desc+4,  &val, 4);
 
     /* Now decide if descriptor being processed is holding the last segment of packet */
     if (txdw0 & CP_TX_LS)
-- 
1.7.2.3

[Qemu-devel] [PATCH v2 2/2] rtl8139: add vlan tag extraction

[Benjamin Poirier]

Add support to the emulated hardware to remove vlan tags in packets
going from the network to the guest.

Signed-off-by: Benjamin Poirier <benjamin.poirier@polymtl.ca>
Cc: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

---
Changes since v1:
* moved the debug print statement inside the if block and reworded
  accordingly. (as suggested by Igor)

AFAIK, extraction is optional to get vlans working. The driver
requests rx detagging but should not assume that it was done. Under
Linux, the mac layer will catch the vlan ethertype. I only added this
part for completeness (to emulate the hardware more truthfully..?).

 hw/rtl8139.c |   40 +++++++++++++++++++++++++++++++++++++---
 1 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index b599945..7762dbb 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -1024,6 +1024,43 @@ static ssize_t rtl8139_do_receive(VLANClientState *nc, const uint8_t *buf, size_
 
         target_phys_addr_t rx_addr = rtl8139_addr64(rxbufLO, rxbufHI);
 
+        if (s->CpCmd & CPlusRxVLAN && size >= ETHER_ADDR_LEN * 2 +
+            VLAN_HDR_LEN && be16_to_cpup((uint16_t *) &buf[ETHER_ADDR_LEN *
+                2]) == ETHERTYPE_VLAN)
+        {
+            size_t new_size = size - VLAN_HDR_LEN;
+
+            rxdw1 &= ~CP_RX_VLAN_TAG_MASK;
+            rxdw1 |= CP_RX_TAVA |
+                le16_to_cpup((uint16_t *)&buf[ETHER_HDR_LEN]);
+
+            if (buf == buf1 || new_size < MIN_BUF_SIZE)
+            {
+                /* move the end and pad */
+                memmove((uint8_t *)buf + ETHER_ADDR_LEN * 2, buf +
+                    ETHER_ADDR_LEN * 2 + VLAN_HDR_LEN, new_size -
+                    ETHER_ADDR_LEN * 2);
+                memset((uint8_t *)buf + new_size, 0, MIN_BUF_SIZE - new_size);
+                size = MIN_BUF_SIZE;
+            }
+            else
+            {
+                /* move the beginning */
+                memmove((uint8_t *)buf + VLAN_HDR_LEN, buf, ETHER_ADDR_LEN *
+                    2);
+                buf += VLAN_HDR_LEN;
+                size = new_size;
+            }
+
+            DEBUG_PRINT(("RTL8139: C+ Rx mode : extracted vlan tag with tci: "
+                    "%u\n", bswap16(rxdw1 & CP_RX_VLAN_TAG_MASK)));
+        }
+        else
+        {
+            /* reset VLAN tag flag */
+            rxdw1 &= ~CP_RX_TAVA;
+        }
+
         /* receive/copy to target memory */
         cpu_physical_memory_write( rx_addr, buf, size );
 
@@ -1082,9 +1119,6 @@ static ssize_t rtl8139_do_receive(VLANClientState *nc, const uint8_t *buf, size_
         rxdw0 &= ~CP_RX_BUFFER_SIZE_MASK;
         rxdw0 |= (size+4);
 
-        /* reset VLAN tag flag */
-        rxdw1 &= ~CP_RX_TAVA;
-
         /* update ring data */
         val = cpu_to_le32(rxdw0);
         cpu_physical_memory_write(cplus_rx_ring_desc,    (uint8_t *)&val, 4);
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH v2 1/2] rtl8139: add vlan tag insertion

[Anthony Liguori]

On 11/08/2010 07:46 PM, Benjamin Poirier wrote:
> Add support to the emulated hardware to add vlan tags in packets going
> from the guest to the network.
>
> Signed-off-by: Benjamin Poirier<benjamin.poirier@polymtl.ca>
> Cc: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
> ---
> Changes since v1:
> * moved the debug print statement inside the if block and reworded
>    accordingly. (as suggested by Igor)
>
>   hw/rtl8139.c |   45 ++++++++++++++++++++++++++++++++++-----------
>   1 files changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index d92981d..b599945 100644
> --- a/hw/rtl8139.c
> +++ b/hw/rtl8139.c
> @@ -47,6 +47,8 @@
>    *                                  Darwin)
>    */
>
> +#include<net/ethernet.h>
> +
>   #include "hw.h"
>   #include "pci.h"
>   #include "qemu-timer.h"
> @@ -58,6 +60,10 @@
>
>   #define PCI_FREQUENCY 33000000L
>
> +/* bytes in VLAN tag */
> +#define VLAN_TCI_LEN 2
> +#define VLAN_HDR_LEN (ETHER_TYPE_LEN + VLAN_TCI_LEN)
> +
>   /* debug RTL8139 card C+ mode only */
>   //#define DEBUG_RTL8139CP 1
>
> @@ -1913,7 +1919,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>
>       cpu_physical_memory_read(cplus_tx_ring_desc,    (uint8_t *)&val, 4);
>       txdw0 = le32_to_cpu(val);
> -    /* TODO: implement VLAN tagging support, VLAN tag data is read to txdw1 */
>       cpu_physical_memory_read(cplus_tx_ring_desc+4,  (uint8_t *)&val, 4);
>       txdw1 = le32_to_cpu(val);
>       cpu_physical_memory_read(cplus_tx_ring_desc+8,  (uint8_t *)&val, 4);
> @@ -1925,9 +1930,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>              descriptor,
>              txdw0, txdw1, txbufLO, txbufHI));
>
> -    /* TODO: the following discard cast should clean clang analyzer output */
> -    (void)txdw1;
> -
>   /* w0 ownership flag */
>   #define CP_TX_OWN (1<<31)
>   /* w0 end of ring flag */
> @@ -1951,8 +1953,8 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>   /* w0 bits 0...15 : buffer size */
>   #define CP_TX_BUFFER_SIZE (1<<16)
>   #define CP_TX_BUFFER_SIZE_MASK (CP_TX_BUFFER_SIZE - 1)
> -/* w1 tag available flag */
> -#define CP_RX_TAGC (1<<17)
> +/* w1 add tag flag */
> +#define CP_TX_TAGC (1<<17)
>   /* w1 bits 0...15 : VLAN tag */
>   #define CP_TX_VLAN_TAG_MASK ((1<<16) - 1)
>   /* w2 low  32bit of Rx buffer ptr */
> @@ -1978,12 +1980,21 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>
>       DEBUG_PRINT(("RTL8139: +++ C+ Tx mode : transmitting from descriptor %d\n", descriptor));
>
> +    int vlan_extra_size = 0;
>       if (txdw0&  CP_TX_FS)
>       {
>           DEBUG_PRINT(("RTL8139: +++ C+ Tx mode : descriptor %d is first segment descriptor\n", descriptor));
>
>           /* reset internal buffer offset */
>           s->cplus_txbuffer_offset = 0;
> +
> +        if (txdw1&  CP_TX_TAGC)
> +        {
> +            vlan_extra_size = VLAN_HDR_LEN;
> +
> +            DEBUG_PRINT(("RTL8139: +++ C+ Tx mode : inserting vlan tag with "
> +                    "tci: %u\n", bswap16(txdw1&  CP_TX_VLAN_TAG_MASK)));
> +        }
>       }
>
>       int txsize = txdw0&  CP_TX_BUFFER_SIZE_MASK;
> @@ -1992,14 +2003,15 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>       /* make sure we have enough space to assemble the packet */
>       if (!s->cplus_txbuffer)
>       {
> -        s->cplus_txbuffer_len = CP_TX_BUFFER_SIZE;
> +        s->cplus_txbuffer_len = CP_TX_BUFFER_SIZE + VLAN_HDR_LEN;
>           s->cplus_txbuffer = qemu_malloc(s->cplus_txbuffer_len);
>           s->cplus_txbuffer_offset = 0;
>
>           DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer allocated space %d\n", s->cplus_txbuffer_len));
>       }
>
> -    while (s->cplus_txbuffer&&  s->cplus_txbuffer_offset + txsize>= s->cplus_txbuffer_len)
> +    while (s->cplus_txbuffer&&  s->cplus_txbuffer_offset + txsize +
> +        vlan_extra_size>= s->cplus_txbuffer_len)
>       {
>           s->cplus_txbuffer_len += CP_TX_BUFFER_SIZE;
>           s->cplus_txbuffer = qemu_realloc(s->cplus_txbuffer, s->cplus_txbuffer_len);
> @@ -2025,6 +2037,20 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>       DEBUG_PRINT(("RTL8139: +++ C+ mode transmit reading %d bytes from host memory at %016" PRIx64 " to offset %d\n",
>                    txsize, (uint64_t)tx_addr, s->cplus_txbuffer_offset));
>
> +    if (vlan_extra_size&&  txsize>= 2 * ETHER_ADDR_LEN)
> +    {
> +        /* copy addresses */
> +        cpu_physical_memory_read(tx_addr, s->cplus_txbuffer, 2 *
> +            ETHER_ADDR_LEN);
> +        tx_addr += 2 * ETHER_ADDR_LEN;
> +        txsize -= 2 * ETHER_ADDR_LEN;
> +        /* insert vlan tag */
> +        *(uint16_t *)(s->cplus_txbuffer + 2 * ETHER_ADDR_LEN) =
> +            cpu_to_be16(ETHERTYPE_VLAN);
> +        *(uint16_t *)(s->cplus_txbuffer + 2 * ETHER_ADDR_LEN + ETHER_TYPE_LEN)
> +            = cpu_to_le16(txdw1&  CP_TX_VLAN_TAG_MASK);
>    

This looks wrong.  You check for txsize >= 2 * ETHER_ADDR_LEN but then 
you assign at 2 * ETHER_ADDR_LEN.  This is a potential overflow, no?

Regards,

Anthony Liguori

> +        s->cplus_txbuffer_offset += 2 * ETHER_ADDR_LEN + VLAN_HDR_LEN;
> +    }
>       cpu_physical_memory_read(tx_addr, s->cplus_txbuffer + s->cplus_txbuffer_offset, txsize);
>       s->cplus_txbuffer_offset += txsize;
>
> @@ -2053,9 +2079,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>       /* update ring data */
>       val = cpu_to_le32(txdw0);
>       cpu_physical_memory_write(cplus_tx_ring_desc,    (uint8_t *)&val, 4);
> -    /* TODO: implement VLAN tagging support, VLAN tag data is read to txdw1 */
> -//    val = cpu_to_le32(txdw1);
> -//    cpu_physical_memory_write(cplus_tx_ring_desc+4,&val, 4);
>
>       /* Now decide if descriptor being processed is holding the last segment of packet */
>       if (txdw0&  CP_TX_LS)
>    

Re: [Qemu-devel] [PATCH v2 1/2] rtl8139: add vlan tag insertion

[Benjamin Poirier]

On 16/11/10 03:06 PM, Anthony Liguori wrote:
> On 11/08/2010 07:46 PM, Benjamin Poirier wrote:
>> Add support to the emulated hardware to add vlan tags in packets going
>> from the guest to the network.
>>
>> Signed-off-by: Benjamin Poirier<benjamin.poirier@polymtl.ca>
>> Cc: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>> ---
>> Changes since v1:
>> * moved the debug print statement inside the if block and reworded
>>    accordingly. (as suggested by Igor)
>>
>>   hw/rtl8139.c |   45 ++++++++++++++++++++++++++++++++++-----------
>>   1 files changed, 34 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
>> index d92981d..b599945 100644
>> --- a/hw/rtl8139.c
>> +++ b/hw/rtl8139.c
>> @@ -47,6 +47,8 @@
>>    *                                  Darwin)
>>    */
>>
>> +#include<net/ethernet.h>
>> +
>>   #include "hw.h"
>>   #include "pci.h"
>>   #include "qemu-timer.h"
>> @@ -58,6 +60,10 @@
>>
>>   #define PCI_FREQUENCY 33000000L
>>
>> +/* bytes in VLAN tag */
>> +#define VLAN_TCI_LEN 2
>> +#define VLAN_HDR_LEN (ETHER_TYPE_LEN + VLAN_TCI_LEN)
>> +
>>   /* debug RTL8139 card C+ mode only */
>>   //#define DEBUG_RTL8139CP 1
>>
>> @@ -1913,7 +1919,6 @@ static int
>> rtl8139_cplus_transmit_one(RTL8139State *s)
>>
>>       cpu_physical_memory_read(cplus_tx_ring_desc,    (uint8_t *)&val,
>> 4);
>>       txdw0 = le32_to_cpu(val);
>> -    /* TODO: implement VLAN tagging support, VLAN tag data is read to
>> txdw1 */
>>       cpu_physical_memory_read(cplus_tx_ring_desc+4,  (uint8_t *)&val,
>> 4);
>>       txdw1 = le32_to_cpu(val);
>>       cpu_physical_memory_read(cplus_tx_ring_desc+8,  (uint8_t *)&val,
>> 4);
>> @@ -1925,9 +1930,6 @@ static int
>> rtl8139_cplus_transmit_one(RTL8139State *s)
>>              descriptor,
>>              txdw0, txdw1, txbufLO, txbufHI));
>>
>> -    /* TODO: the following discard cast should clean clang analyzer
>> output */
>> -    (void)txdw1;
>> -
>>   /* w0 ownership flag */
>>   #define CP_TX_OWN (1<<31)
>>   /* w0 end of ring flag */
>> @@ -1951,8 +1953,8 @@ static int
>> rtl8139_cplus_transmit_one(RTL8139State *s)
>>   /* w0 bits 0...15 : buffer size */
>>   #define CP_TX_BUFFER_SIZE (1<<16)
>>   #define CP_TX_BUFFER_SIZE_MASK (CP_TX_BUFFER_SIZE - 1)
>> -/* w1 tag available flag */
>> -#define CP_RX_TAGC (1<<17)
>> +/* w1 add tag flag */
>> +#define CP_TX_TAGC (1<<17)
>>   /* w1 bits 0...15 : VLAN tag */
>>   #define CP_TX_VLAN_TAG_MASK ((1<<16) - 1)
>>   /* w2 low  32bit of Rx buffer ptr */
>> @@ -1978,12 +1980,21 @@ static int
>> rtl8139_cplus_transmit_one(RTL8139State *s)
>>
>>       DEBUG_PRINT(("RTL8139: +++ C+ Tx mode : transmitting from
>> descriptor %d\n", descriptor));
>>
>> +    int vlan_extra_size = 0;
>>       if (txdw0&  CP_TX_FS)
>>       {
>>           DEBUG_PRINT(("RTL8139: +++ C+ Tx mode : descriptor %d is
>> first segment descriptor\n", descriptor));
>>
>>           /* reset internal buffer offset */
>>           s->cplus_txbuffer_offset = 0;
>> +
>> +        if (txdw1&  CP_TX_TAGC)
>> +        {
>> +            vlan_extra_size = VLAN_HDR_LEN;
>> +
>> +            DEBUG_PRINT(("RTL8139: +++ C+ Tx mode : inserting vlan
>> tag with "
>> +                    "tci: %u\n", bswap16(txdw1&  CP_TX_VLAN_TAG_MASK)));
>> +        }
>>       }
>>
>>       int txsize = txdw0&  CP_TX_BUFFER_SIZE_MASK;
>> @@ -1992,14 +2003,15 @@ static int
>> rtl8139_cplus_transmit_one(RTL8139State *s)
>>       /* make sure we have enough space to assemble the packet */
>>       if (!s->cplus_txbuffer)
>>       {
>> -        s->cplus_txbuffer_len = CP_TX_BUFFER_SIZE;
>> +        s->cplus_txbuffer_len = CP_TX_BUFFER_SIZE + VLAN_HDR_LEN;
>>           s->cplus_txbuffer = qemu_malloc(s->cplus_txbuffer_len);
>>           s->cplus_txbuffer_offset = 0;
>>
>>           DEBUG_PRINT(("RTL8139: +++ C+ mode transmission buffer
>> allocated space %d\n", s->cplus_txbuffer_len));
>>       }
>>
>> -    while (s->cplus_txbuffer&&  s->cplus_txbuffer_offset + txsize>=
>> s->cplus_txbuffer_len)
>> +    while (s->cplus_txbuffer&&  s->cplus_txbuffer_offset + txsize +
>> +        vlan_extra_size>= s->cplus_txbuffer_len)
>>       {
>>           s->cplus_txbuffer_len += CP_TX_BUFFER_SIZE;
>>           s->cplus_txbuffer = qemu_realloc(s->cplus_txbuffer,
>> s->cplus_txbuffer_len);
>> @@ -2025,6 +2037,20 @@ static int
>> rtl8139_cplus_transmit_one(RTL8139State *s)
>>       DEBUG_PRINT(("RTL8139: +++ C+ mode transmit reading %d bytes
>> from host memory at %016" PRIx64 " to offset %d\n",
>>                    txsize, (uint64_t)tx_addr, s->cplus_txbuffer_offset));
>>
>> +    if (vlan_extra_size&&  txsize>= 2 * ETHER_ADDR_LEN)
>> +    {
>> +        /* copy addresses */
>> +        cpu_physical_memory_read(tx_addr, s->cplus_txbuffer, 2 *
>> +            ETHER_ADDR_LEN);
>> +        tx_addr += 2 * ETHER_ADDR_LEN;
>> +        txsize -= 2 * ETHER_ADDR_LEN;
>> +        /* insert vlan tag */
>> +        *(uint16_t *)(s->cplus_txbuffer + 2 * ETHER_ADDR_LEN) =
>> +            cpu_to_be16(ETHERTYPE_VLAN);
>> +        *(uint16_t *)(s->cplus_txbuffer + 2 * ETHER_ADDR_LEN +
>> ETHER_TYPE_LEN)
>> +            = cpu_to_le16(txdw1&  CP_TX_VLAN_TAG_MASK);
>>    
> 
> This looks wrong.  You check for txsize >= 2 * ETHER_ADDR_LEN but then
> you assign at 2 * ETHER_ADDR_LEN.  This is a potential overflow, no?
> 

No, the check is to protect the read. Concerning the writes,
cplus_txbuffer is allocated with size of at least CP_TX_BUFFER_SIZE +
VLAN_HDR_LEN > 2 * ETHER_ADDR_LEN + VLAN_HDR_LEN (ie. 64k + 4 > 12 + 4),
please see previous hunk.

Thanks,
-Ben

> Regards,
> 
> Anthony Liguori
> 
>> +        s->cplus_txbuffer_offset += 2 * ETHER_ADDR_LEN + VLAN_HDR_LEN;
>> +    }
>>       cpu_physical_memory_read(tx_addr, s->cplus_txbuffer +
>> s->cplus_txbuffer_offset, txsize);
>>       s->cplus_txbuffer_offset += txsize;
>>
>> @@ -2053,9 +2079,6 @@ static int
>> rtl8139_cplus_transmit_one(RTL8139State *s)
>>       /* update ring data */
>>       val = cpu_to_le32(txdw0);
>>       cpu_physical_memory_write(cplus_tx_ring_desc,    (uint8_t
>> *)&val, 4);
>> -    /* TODO: implement VLAN tagging support, VLAN tag data is read to
>> txdw1 */
>> -//    val = cpu_to_le32(txdw1);
>> -//    cpu_physical_memory_write(cplus_tx_ring_desc+4,&val, 4);
>>
>>       /* Now decide if descriptor being processed is holding the last
>> segment of packet */
>>       if (txdw0&  CP_TX_LS)
>>    
>