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

[Benjamin Poirier]

On 08/11/10 11:34 AM, Stefan Hajnoczi wrote:
> On Sun, Nov 7, 2010 at 9:25 PM, Benjamin Poirier
> <address@hidden> 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 <address@hidden>
>> Cc: Igor V. Kovalenko <address@hidden>
>> ---
>>  hw/rtl8139.c |   46 +++++++++++++++++++++++++++++++++++-----------
>>  1 files changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
>> index d92981d..ac294da 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,22 @@ 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));
>>
>> +        DEBUG_PRINT(("RTL8139: +++ C+ Tx mode : add vlan tag: %u tci: %u\n",
>> +                !!(txdw1 & CP_TX_TAGC), bswap16(txdw1 &
>> +                    CP_TX_VLAN_TAG_MASK)));
> 
> Please use leXX_to_cpu() and cpu_to_leXX() instead of bswapXX() for
> little-endian conversion.  This is more explicit than unconditional
> byteswapping and work on both big- and little-endian QEMU host
> architectures.

I believe the use of flat out swapping is warranted here. The data has
been adjusted for host-endianness when the register was read:
        txdw1 = le32_to_cpu(val);
We have to mask and swap to accomodate the format used by the card's
registers, which is always the same regardless of machine endianness.

txdw1 is extracted from a 32 bit register like such (when the descriptor
specifies tx tagging), in increasing address order:
tci8-15 tci0-7 0x01 0x00

-Ben

> 
> Stefan
>