From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=56943 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PIRnG-0005Jq-9V for qemu-devel@nongnu.org; Tue, 16 Nov 2010 15:06:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PIRnE-0000QS-BY for qemu-devel@nongnu.org; Tue, 16 Nov 2010 15:06:18 -0500 Received: from mail-iw0-f173.google.com ([209.85.214.173]:47087) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PIRnE-0000QL-3r for qemu-devel@nongnu.org; Tue, 16 Nov 2010 15:06:16 -0500 Received: by iwn36 with SMTP id 36so1242347iwn.4 for ; Tue, 16 Nov 2010 12:06:15 -0800 (PST) Message-ID: <4CE2E435.9000207@codemonkey.ws> Date: Tue, 16 Nov 2010 14:06:13 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v2 1/2] rtl8139: add vlan tag insertion References: <1289267181-20613-1-git-send-email-benjamin.poirier@polymtl.ca> In-Reply-To: <1289267181-20613-1-git-send-email-benjamin.poirier@polymtl.ca> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Benjamin Poirier Cc: qemu-devel@nongnu.org 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 > Cc: Igor V. Kovalenko > --- > 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 > + > #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) >