[Qemu-devel] [PATCH 1/6] rtl8139: limit transmission buffer size in c+ mode

[Qemu-devel] [PATCH 1/6] rtl8139: limit transmission buffer size in c+ mode

[Jason Wang]

The tx buffer would be re-allocated for tx descriptor with big size
and without LS bit set, this would make guest driver could easily let
qemu to allocate unlimited.

In linux host, a glib failure were easy to be triggered:

GLib-ERROR **: gmem.c:176: failed to allocate 18446744071562067968 bytes

This patch fix this by adding a limit. As the spec didn't tell the maximum size
of buffer allowed, stick it to  current CP_TX_BUFFER_SIZE (65536).

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/rtl8139.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 05b8e1e..d9e742c 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -2063,11 +2063,10 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
 
     while (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize >= s->cplus_txbuffer_len)
     {
-        s->cplus_txbuffer_len += CP_TX_BUFFER_SIZE;
-        s->cplus_txbuffer = g_realloc(s->cplus_txbuffer, s->cplus_txbuffer_len);
-
-        DPRINTF("+++ C+ mode transmission buffer space changed to %d\n",
-            s->cplus_txbuffer_len);
+        /* The spec didn't tell the maximum size, stick to CP_TX_BUFFER_SIZE */
+        txsize = s->cplus_txbuffer_len - s->cplus_txbuffer_offset;
+        DPRINTF("+++ C+ mode transmission buffer overrun, truncated descriptor"
+                "length to %d\n", txsize);
     }
 
     if (!s->cplus_txbuffer)

[Qemu-devel] [PATCH 2/6] rtl8139: remove unused marco

[Jason Wang]

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/rtl8139.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index d9e742c..ca613ae 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -65,9 +65,6 @@
 
 #define PCI_FREQUENCY 33000000L
 
-/* debug RTL8139 card C+ mode only */
-//#define DEBUG_RTL8139CP 1
-
 #define SET_MASKED(input, mask, curr) \
     ( ( (input) & ~(mask) ) | ( (curr) & (mask) ) )
 

[Qemu-devel] [PATCH 3/6] rtl8139: support byte read to TxStatus registers

[Jason Wang]

Some drivers (such as win7) use byte read for TxStatus registers, so we need to
support this to let guest driver behave correctly.

For writing, only double-word access is allowed by spec.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/rtl8139.c |   33 +++++++++++++++++++++++++++++----
 1 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index ca613ae..a946e79 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -2495,11 +2495,30 @@ static void rtl8139_TxStatus_write(RTL8139State *s, uint32_t txRegOffset, uint32
     rtl8139_transmit(s);
 }
 
-static uint32_t rtl8139_TxStatus_read(RTL8139State *s, uint32_t txRegOffset)
+static uint32_t rtl8139_TxStatus_read(RTL8139State *s, uint8_t addr, int size)
 {
-    uint32_t ret = s->TxStatus[txRegOffset/4];
+    uint32_t reg = (addr - TxStatus0) / 4;
+    uint32_t offset = addr & 0x3;
+    uint32_t ret = 0;
+
+    if (addr & (size - 1)) {
+        DPRINTF("not implemented read for TxStatus addr=0x%x size=0x%x\n", addr,
+                size);
+        return ret;
+    }
 
-    DPRINTF("TxStatus read offset=0x%x val=0x%08x\n", txRegOffset, ret);
+    switch (size) {
+    case 1: /* fall through */
+    case 2: /* fall through */
+    case 4:
+        ret = (s->TxStatus[reg] >> offset * 8) & ((1 << (size * 8)) - 1);
+        DPRINTF("TxStatus[%d] read addr=0x%x size=0x%x val=0x%08x\n", reg, addr,
+                size, ret);
+        break;
+    default:
+        DPRINTF("unsupported size 0x%x of TxStatus reading\n", size);
+        break;
+    }
 
     return ret;
 }
@@ -2970,6 +2989,9 @@ static uint32_t rtl8139_io_readb(void *opaque, uint8_t addr)
         case MAR0 ... MAR0+7:
             ret = s->mult[addr - MAR0];
             break;
+        case TxStatus0 ... TxStatus0+4*4-1:
+            ret = rtl8139_TxStatus_read(s, addr, 1);
+            break;
         case ChipCmd:
             ret = rtl8139_ChipCmd_read(s);
             break;
@@ -3033,6 +3055,9 @@ static uint32_t rtl8139_io_readw(void *opaque, uint8_t addr)
 
     switch (addr)
     {
+        case TxAddr0 ... TxAddr0+4*4-1:
+            ret = rtl8139_TxStatus_read(s, addr, 2);
+            break;
         case IntrMask:
             ret = rtl8139_IntrMask_read(s);
             break;
@@ -3123,7 +3148,7 @@ static uint32_t rtl8139_io_readl(void *opaque, uint8_t addr)
             break;
 
         case TxStatus0 ... TxStatus0+4*4-1:
-            ret = rtl8139_TxStatus_read(s, addr-TxStatus0);
+            ret = rtl8139_TxStatus_read(s, addr, 4);
             break;
 
         case TxAddr0 ... TxAddr0+4*4-1:

[Qemu-devel] [PATCH 4/6] net: move compute_mcast_idx() to net.h

[Jason Wang]

Reduce duplicated codes.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/eepro100.c      |   25 -------------------------
 hw/ne2000.c        |   24 ------------------------
 hw/opencores_eth.c |   25 -------------------------
 hw/rtl8139.c       |   24 ------------------------
 net.c              |   23 +++++++++++++++++++++++
 net.h              |    3 +++
 6 files changed, 26 insertions(+), 98 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index e3ba719..02e6f7e 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -322,33 +322,8 @@ static const uint16_t eepro100_mdi_mask[] = {
     0xffff, 0xffff, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
 };
 
-#define POLYNOMIAL 0x04c11db6
-
 static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s);
 
-/* From FreeBSD */
-/* XXX: optimize */
-static unsigned compute_mcast_idx(const uint8_t * ep)
-{
-    uint32_t crc;
-    int carry, i, j;
-    uint8_t b;
-
-    crc = 0xffffffff;
-    for (i = 0; i < 6; i++) {
-        b = *ep++;
-        for (j = 0; j < 8; j++) {
-            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
-            crc <<= 1;
-            b >>= 1;
-            if (carry) {
-                crc = ((crc ^ POLYNOMIAL) | carry);
-            }
-        }
-    }
-    return (crc & BITS(7, 2)) >> 2;
-}
-
 /* Read a 16 bit control/status (CSR) register. */
 static uint16_t e100_read_reg2(EEPRO100State *s, E100RegisterOffset addr)
 {
diff --git a/hw/ne2000.c b/hw/ne2000.c
index 71452e1..d02e60c 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -150,30 +150,6 @@ static void ne2000_update_irq(NE2000State *s)
     qemu_set_irq(s->irq, (isr != 0));
 }
 
-#define POLYNOMIAL 0x04c11db6
-
-/* From FreeBSD */
-/* XXX: optimize */
-static int compute_mcast_idx(const uint8_t *ep)
-{
-    uint32_t crc;
-    int carry, i, j;
-    uint8_t b;
-
-    crc = 0xffffffff;
-    for (i = 0; i < 6; i++) {
-        b = *ep++;
-        for (j = 0; j < 8; j++) {
-            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
-            crc <<= 1;
-            b >>= 1;
-            if (carry)
-                crc = ((crc ^ POLYNOMIAL) | carry);
-        }
-    }
-    return (crc >> 26);
-}
-
 static int ne2000_buffer_full(NE2000State *s)
 {
     int avail, index, boundary;
diff --git a/hw/opencores_eth.c b/hw/opencores_eth.c
index 9b036cb..06ef712 100644
--- a/hw/opencores_eth.c
+++ b/hw/opencores_eth.c
@@ -351,31 +351,6 @@ static int open_eth_can_receive(VLANClientState *nc)
         (rx_desc(s)->len_flags & RXD_E);
 }
 
-#define POLYNOMIAL 0x04c11db6
-
-/* From FreeBSD */
-/* XXX: optimize */
-static unsigned compute_mcast_idx(const uint8_t *ep)
-{
-    uint32_t crc;
-    int carry, i, j;
-    uint8_t b;
-
-    crc = 0xffffffff;
-    for (i = 0; i < 6; i++) {
-        b = *ep++;
-        for (j = 0; j < 8; j++) {
-            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
-            crc <<= 1;
-            b >>= 1;
-            if (carry) {
-                crc = ((crc ^ POLYNOMIAL) | carry);
-            }
-        }
-    }
-    return crc >> 26;
-}
-
 static ssize_t open_eth_receive(VLANClientState *nc,
         const uint8_t *buf, size_t size)
 {
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index a946e79..509a53e 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -708,30 +708,6 @@ static void rtl8139_update_irq(RTL8139State *s)
     qemu_set_irq(s->dev.irq[0], (isr != 0));
 }
 
-#define POLYNOMIAL 0x04c11db6
-
-/* From FreeBSD */
-/* XXX: optimize */
-static int compute_mcast_idx(const uint8_t *ep)
-{
-    uint32_t crc;
-    int carry, i, j;
-    uint8_t b;
-
-    crc = 0xffffffff;
-    for (i = 0; i < 6; i++) {
-        b = *ep++;
-        for (j = 0; j < 8; j++) {
-            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
-            crc <<= 1;
-            b >>= 1;
-            if (carry)
-                crc = ((crc ^ POLYNOMIAL) | carry);
-        }
-    }
-    return (crc >> 26);
-}
-
 static int rtl8139_RxWrap(RTL8139State *s)
 {
     /* wrapping enabled; assume 1.5k more buffer space if size < 65536 */
diff --git a/net.c b/net.c
index c34474f..1922d8a 100644
--- a/net.c
+++ b/net.c
@@ -1475,3 +1475,26 @@ int net_client_parse(QemuOptsList *opts_list, const char *optarg)
     default_net = 0;
     return 0;
 }
+
+/* From FreeBSD */
+/* XXX: optimize */
+unsigned compute_mcast_idx(const uint8_t *ep)
+{
+    uint32_t crc;
+    int carry, i, j;
+    uint8_t b;
+
+    crc = 0xffffffff;
+    for (i = 0; i < 6; i++) {
+        b = *ep++;
+        for (j = 0; j < 8; j++) {
+            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
+            crc <<= 1;
+            b >>= 1;
+            if (carry) {
+                crc = ((crc ^ POLYNOMIAL) | carry);
+            }
+        }
+    }
+    return crc >> 26;
+}
diff --git a/net.h b/net.h
index 75a8c15..64993b4 100644
--- a/net.h
+++ b/net.h
@@ -182,6 +182,9 @@ void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd);
 
 int net_handle_fd_param(Monitor *mon, const char *param);
 
+#define POLYNOMIAL 0x04c11db6
+unsigned compute_mcast_idx(const uint8_t *ep);
+
 #define vmstate_offset_macaddr(_state, _field)                       \
     vmstate_offset_array(_state, _field.a, uint8_t,                \
                          sizeof(typeof_field(_state, _field)))

[Qemu-devel] [PATCH 5/6] rtl8139: correctly check the opmode

[Jason Wang]

According to the spec, only when opmode is "Config. Register Write
Enable" could driver write to CONFIG0,1,3,4 and bits 13,12,8 of BMCR.

Currently, we allow modifying to those registers also when 8139 is in
"Auto-load" mode and "93C46 (93C56) Programming" mode. This patch
fixes this.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/rtl8139.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 509a53e..2e3da0b 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -332,8 +332,10 @@ enum CSCRBits {
 };
 
 enum Cfg9346Bits {
-    Cfg9346_Lock = 0x00,
-    Cfg9346_Unlock = 0xC0,
+    Cfg9346_Normal = 0x00,
+    Cfg9346_Autoload = 0x40,
+    Cfg9346_Programming = 0x80,
+    Cfg9346_ConfigWrite = 0xC0,
 };
 
 typedef enum {
@@ -1451,7 +1453,7 @@ static uint32_t rtl8139_IntrMitigate_read(RTL8139State *s)
 
 static int rtl8139_config_writable(RTL8139State *s)
 {
-    if (s->Cfg9346 & Cfg9346_Unlock)
+    if ((s->Cfg9346 & Chip9346_op_mask) == Cfg9346_ConfigWrite)
     {
         return 1;
     }

[Qemu-devel] [PATCH 6/6] rtl8139: do the network/host communication only in normal operating mode

[Jason Wang]

According the spec, the card works in network/host communication mode only when
both EEM1 and EEM0 are unset in 93C46 Command Register (normal op
mode). So this patch check these bits before trying to receive packets.

As some guest driver (such as linux, see cp_init_hw() in 8139cp.c)
allocate rx ring after the recevier were enabled, this would cause our
emulation codes tries to dma into guest memory when the rx descriptor
is not properly configured. This patch fixes this.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/rtl8139.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 2e3da0b..97a4b77 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -791,6 +791,9 @@ static int rtl8139_can_receive(VLANClientState *nc)
       return 1;
     if (!rtl8139_receiver_enabled(s))
       return 1;
+    /* network/host communication happens only in normal mode */
+    if ((s->Cfg9346 & Chip9346_op_mask) != Cfg9346_Normal)
+	return 0;
 
     if (rtl8139_cp_receiver_enabled(s)) {
         /* ??? Flow control not implemented in c+ mode.
@@ -833,6 +836,12 @@ static ssize_t rtl8139_do_receive(VLANClientState *nc, const uint8_t *buf, size_
         return -1;
     }
 
+    /* check whether we are in normal mode */
+    if ((s->Cfg9346 & Chip9346_op_mask) != Cfg9346_Normal) {
+        DPRINTF("not in normal op mode\n");
+        return -1;
+    }
+
     /* XXX: check this */
     if (s->RxConfig & AcceptAllPhys) {
         /* promiscuous: receive all */

Re: [Qemu-devel] [PATCH 1/6] rtl8139: limit transmission buffer size in c+ mode

[Stefan Hajnoczi]

On Mon, Mar 05, 2012 at 11:08:24AM +0800, Jason Wang wrote:
> The tx buffer would be re-allocated for tx descriptor with big size
> and without LS bit set, this would make guest driver could easily let
> qemu to allocate unlimited.
> 
> In linux host, a glib failure were easy to be triggered:
> 
> GLib-ERROR **: gmem.c:176: failed to allocate 18446744071562067968 bytes
> 
> This patch fix this by adding a limit. As the spec didn't tell the maximum size
> of buffer allowed, stick it to  current CP_TX_BUFFER_SIZE (65536).
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/rtl8139.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index 05b8e1e..d9e742c 100644
> --- a/hw/rtl8139.c
> +++ b/hw/rtl8139.c
> @@ -2063,11 +2063,10 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
> 
>      while (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize >= s->cplus_txbuffer_len)
>      {
> -        s->cplus_txbuffer_len += CP_TX_BUFFER_SIZE;
> -        s->cplus_txbuffer = g_realloc(s->cplus_txbuffer, s->cplus_txbuffer_len);
> -
> -        DPRINTF("+++ C+ mode transmission buffer space changed to %d\n",
> -            s->cplus_txbuffer_len);
> +        /* The spec didn't tell the maximum size, stick to CP_TX_BUFFER_SIZE */
> +        txsize = s->cplus_txbuffer_len - s->cplus_txbuffer_offset;
> +        DPRINTF("+++ C+ mode transmission buffer overrun, truncated descriptor"
> +                "length to %d\n", txsize);

This makes sense to me since a 64 KB MTU is not possible - it's a safe
upper limit that will not impact guests.

Please change this while statement to an if statement and drop the
s->cplus_txbuffer check (which is always true since we allocate
immediately prior, if it is not already allocated).

Stefan

[Qemu-devel] [1/6 V2 PATCH] rtl8139: limit transmission buffer size in c+ mode

[Jason Wang]

The tx buffer would be re-allocated for tx descriptor with big size
and without LS bit set, this would make guest driver could easily let
qemu to allocate unlimited.

In linux host, a glib failure were easy to be triggered:

GLib-ERROR **: gmem.c:176: failed to allocate 18446744071562067968 bytes

This patch fix this by adding a limit. As the spec didn't tell the maximum size
of buffer allowed, stick it to current CP_TX_BUFFER_SIZE (65536).

Changes from V1:

Drop the while statement and s->cplus_txbuffer check.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/rtl8139.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 05b8e1e..be4bc6a 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -2061,13 +2061,12 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
             s->cplus_txbuffer_len);
     }
 
-    while (s->cplus_txbuffer && s->cplus_txbuffer_offset + txsize >= s->cplus_txbuffer_len)
+    if (s->cplus_txbuffer_offset + txsize >= s->cplus_txbuffer_len)
     {
-        s->cplus_txbuffer_len += CP_TX_BUFFER_SIZE;
-        s->cplus_txbuffer = g_realloc(s->cplus_txbuffer, s->cplus_txbuffer_len);
-
-        DPRINTF("+++ C+ mode transmission buffer space changed to %d\n",
-            s->cplus_txbuffer_len);
+        /* The spec didn't tell the maximum size, stick to CP_TX_BUFFER_SIZE */
+        txsize = s->cplus_txbuffer_len - s->cplus_txbuffer_offset;
+        DPRINTF("+++ C+ mode transmission buffer overrun, truncated descriptor"
+                "length to %d\n", txsize);
     }
 
     if (!s->cplus_txbuffer)

Re: [Qemu-devel] [1/6 V2 PATCH] rtl8139: limit transmission buffer size in c+ mode

[Stefan Hajnoczi]

On Wed, Mar 07, 2012 at 11:17:48AM +0800, Jason Wang wrote:
> The tx buffer would be re-allocated for tx descriptor with big size
> and without LS bit set, this would make guest driver could easily let
> qemu to allocate unlimited.
> 
> In linux host, a glib failure were easy to be triggered:
> 
> GLib-ERROR **: gmem.c:176: failed to allocate 18446744071562067968 bytes
> 
> This patch fix this by adding a limit. As the spec didn't tell the maximum size
> of buffer allowed, stick it to current CP_TX_BUFFER_SIZE (65536).
> 
> Changes from V1:
> 
> Drop the while statement and s->cplus_txbuffer check.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/rtl8139.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [1/6 V2 PATCH] rtl8139: limit transmission buffer size in c+ mode

[Michael S. Tsirkin]

On Wed, Mar 07, 2012 at 08:13:51AM +0000, Stefan Hajnoczi wrote:
> On Wed, Mar 07, 2012 at 11:17:48AM +0800, Jason Wang wrote:
> > The tx buffer would be re-allocated for tx descriptor with big size
> > and without LS bit set, this would make guest driver could easily let
> > qemu to allocate unlimited.
> > 
> > In linux host, a glib failure were easy to be triggered:
> > 
> > GLib-ERROR **: gmem.c:176: failed to allocate 18446744071562067968 bytes
> > 
> > This patch fix this by adding a limit. As the spec didn't tell the maximum size
> > of buffer allowed, stick it to current CP_TX_BUFFER_SIZE (65536).
> > 
> > Changes from V1:
> > 
> > Drop the while statement and s->cplus_txbuffer check.
> > 
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/rtl8139.c |   11 +++++------
> >  1 files changed, 5 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Applied, thanks everyone.