[Qemu-devel] eepro100.c patches

[Qemu-devel] eepro100.c patches

[Stefan Weil]

Hi,

these patches are still missing in QEMU master
(and would also be good for 0.12.x because many
people wait for the multicast fix needed for
junos, see "junos qemu eepro100.c" at google):

089c60f8e467cfd567a946e005f10559b2fb9462
f925f386761fe1abb82aad8472c6fb388b022756
5a89b50ef52e7c9c3cf072638594880fa1a8f596
(apply them in this order to avoid conflicts)

The latest feedback from mst was positive, so I hope
there is nothing more in the way.

Kind regards,
Stefan

Re: [Qemu-devel] eepro100.c patches

[Anthony Liguori]

Stefan Weil wrote:
> Hi,
>
> these patches are still missing in QEMU master
> (and would also be good for 0.12.x because many
> people wait for the multicast fix needed for
> junos, see "junos qemu eepro100.c" at google):
>
> 089c60f8e467cfd567a946e005f10559b2fb9462
> f925f386761fe1abb82aad8472c6fb388b022756
> 5a89b50ef52e7c9c3cf072638594880fa1a8f596
> (apply them in this order to avoid conflicts)
>   

Commit ids are referring to?

None of the patches you have outstanding are 0.12.x material because 
they don't actually fix any bugs.

If you've got bug fixes patches that these depend on, then please send 
the whole thing out as a series.

Regards,

Anthony Liguori

Re: [Qemu-devel] eepro100.c patches

[Stefan Weil]

Anthony Liguori schrieb:
> Stefan Weil wrote:
>> Hi,
>>
>> these patches are still missing in QEMU master
>> (and would also be good for 0.12.x because many
>> people wait for the multicast fix needed for
>> junos, see "junos qemu eepro100.c" at google):
>>
>> 089c60f8e467cfd567a946e005f10559b2fb9462
>> f925f386761fe1abb82aad8472c6fb388b022756
>> 5a89b50ef52e7c9c3cf072638594880fa1a8f596
>> (apply them in this order to avoid conflicts)
>>   
>
> Commit ids are referring to?
>
> None of the patches you have outstanding are 0.12.x material because
> they don't actually fix any bugs.
>
> If you've got bug fixes patches that these depend on, then please send
> the whole thing out as a series.
>
> Regards,
>
> Anthony Liguori
>

Ah, sorry, I sent the commit ids which are not included in the patch mails.

These patches are missing:

* eepro100: Restructure code (new function tx_command)
  http://patchwork.ozlabs.org/patch/40965/

* eepro100: Better documentation for temporary data
  http://patchwork.ozlabs.org/patch/41072/

* eepro100: Fix multicast support
  http://patchwork.ozlabs.org/patch/33959/

The last one is a bug fix needed for junos and other
applications which depend on correct multicast handling.

I'll resent the patches as a series.

Regards,

Stefan Weil

[Qemu-devel] [PATCH 1/3] eepro100: Restructure code (new function tx_command)

[Stefan Weil]

Handling of transmit commands is rather complex,
so about 80 lines of code were moved from function
action_command to the new function tx_command.

The two new values "tx" and "cb_address" in the
eepro100 status structure made this possible without
passing too many parameters.

In addition, the moved code was cleaned a little bit:
old comments marked with //~ were removed, C++ style
comments were replaced by C style comments, C++ like
variable declarations after code were reordered.

Simplified mode is still broken. Nor did I fix
endianess issues. Both problems will be fixed in
additional patches (which need this one).

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |  192 +++++++++++++++++++++++++++++----------------------------
 1 files changed, 98 insertions(+), 94 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 2a9e3b5..5635f61 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -213,6 +213,10 @@ typedef struct {
     uint32_t ru_offset;         /* RU address offset */
     uint32_t statsaddr;         /* pointer to eepro100_stats_t */
 
+    /* Temporary data. */
+    eepro100_tx_t tx;
+    uint32_t cb_address;
+
     /* Statistical counters. Also used for wake-up packet (i82559). */
     eepro100_stats_t statistics;
 
@@ -748,17 +752,100 @@ static void dump_statistics(EEPRO100State * s)
     //~ missing("CU dump statistical counters");
 }
 
+static void tx_command(EEPRO100State *s)
+{
+    uint32_t tbd_array = le32_to_cpu(s->tx.tx_desc_addr);
+    uint16_t tcb_bytes = (le16_to_cpu(s->tx.tcb_bytes) & 0x3fff);
+    /* Sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes. */
+    uint8_t buf[2600];
+    uint16_t size = 0;
+    uint32_t tbd_address = s->cb_address + 0x10;
+    TRACE(RXTX, logout
+        ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count %u\n",
+         tbd_array, tcb_bytes, s->tx.tbd_count));
+
+    if (tcb_bytes > 2600) {
+        logout("TCB byte count too large, using 2600\n");
+        tcb_bytes = 2600;
+    }
+    if (!((tcb_bytes > 0) || (tbd_array != 0xffffffff))) {
+        logout
+            ("illegal values of TBD array address and TCB byte count!\n");
+    }
+    assert(tcb_bytes <= sizeof(buf));
+    while (size < tcb_bytes) {
+        uint32_t tx_buffer_address = ldl_phys(tbd_address);
+        uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
+        //~ uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
+        tbd_address += 8;
+        TRACE(RXTX, logout
+            ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
+             tx_buffer_address, tx_buffer_size));
+        tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
+        cpu_physical_memory_read(tx_buffer_address, &buf[size],
+                                 tx_buffer_size);
+        size += tx_buffer_size;
+    }
+    if (tbd_array == 0xffffffff) {
+        /* Simplified mode. Was already handled by code above. */
+    } else {
+        /* Flexible mode. */
+        uint8_t tbd_count = 0;
+        if (s->has_extended_tcb_support && !(s->configuration[6] & BIT(4))) {
+            /* Extended Flexible TCB. */
+            for (; tbd_count < 2; tbd_count++) {
+                uint32_t tx_buffer_address = ldl_phys(tbd_address);
+                uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
+                uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
+                tbd_address += 8;
+                TRACE(RXTX, logout
+                    ("TBD (extended flexible mode): buffer address 0x%08x, size 0x%04x\n",
+                     tx_buffer_address, tx_buffer_size));
+                tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
+                cpu_physical_memory_read(tx_buffer_address, &buf[size],
+                                         tx_buffer_size);
+                size += tx_buffer_size;
+                if (tx_buffer_el & 1) {
+                    break;
+                }
+            }
+        }
+        tbd_address = tbd_array;
+        for (; tbd_count < s->tx.tbd_count; tbd_count++) {
+            uint32_t tx_buffer_address = ldl_phys(tbd_address);
+            uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
+            uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
+            tbd_address += 8;
+            TRACE(RXTX, logout
+                ("TBD (flexible mode): buffer address 0x%08x, size 0x%04x\n",
+                 tx_buffer_address, tx_buffer_size));
+            tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
+            cpu_physical_memory_read(tx_buffer_address, &buf[size],
+                                     tx_buffer_size);
+            size += tx_buffer_size;
+            if (tx_buffer_el & 1) {
+                break;
+            }
+        }
+    }
+    TRACE(RXTX, logout("%p sending frame, len=%d,%s\n", s, size, nic_dump(buf, size)));
+    qemu_send_packet(&s->nic->nc, buf, size);
+    s->statistics.tx_good_frames++;
+    /* Transmit with bad status would raise an CX/TNO interrupt.
+     * (82557 only). Emulation never has bad status. */
+    //~ eepro100_cx_interrupt(s);
+}
+
 static void action_command(EEPRO100State *s)
 {
     for (;;) {
-        uint32_t cb_address = s->cu_base + s->cu_offset;
-        eepro100_tx_t tx;
-        cpu_physical_memory_read(cb_address, (uint8_t *) & tx, sizeof(tx));
-        uint16_t status = le16_to_cpu(tx.status);
-        uint16_t command = le16_to_cpu(tx.command);
+        s->cb_address = s->cu_base + s->cu_offset;
+        cpu_physical_memory_read(s->cb_address, (uint8_t *)&s->tx, sizeof(s->tx));
+        uint16_t status = le16_to_cpu(s->tx.status);
+        uint16_t command = le16_to_cpu(s->tx.command);
         logout
             ("val=0x%02x (cu start), status=0x%04x, command=0x%04x, link=0x%08x\n",
-             val, status, command, tx.link);
+             val, status, command, s->tx.link);
         bool bit_el = ((command & 0x8000) != 0);
         bool bit_s = ((command & 0x4000) != 0);
         bool bit_i = ((command & 0x2000) != 0);
@@ -766,17 +853,17 @@ static void action_command(EEPRO100State *s)
         bool success = true;
         //~ bool bit_sf = ((command & 0x0008) != 0);
         uint16_t cmd = command & 0x0007;
-        s->cu_offset = le32_to_cpu(tx.link);
+        s->cu_offset = le32_to_cpu(s->tx.link);
         switch (cmd) {
         case CmdNOp:
             /* Do nothing. */
             break;
         case CmdIASetup:
-            cpu_physical_memory_read(cb_address + 8, &s->conf.macaddr.a[0], 6);
+            cpu_physical_memory_read(s->cb_address + 8, &s->conf.macaddr.a[0], 6);
             TRACE(OTHER, logout("macaddr: %s\n", nic_dump(&s->macaddr[0], 6)));
             break;
         case CmdConfigure:
-            cpu_physical_memory_read(cb_address + 8, &s->configuration[0],
+            cpu_physical_memory_read(s->cb_address + 8, &s->configuration[0],
                                      sizeof(s->configuration));
             TRACE(OTHER, logout("configuration: %s\n", nic_dump(&s->configuration[0], 16)));
             break;
@@ -784,95 +871,12 @@ static void action_command(EEPRO100State *s)
             //~ missing("multicast list");
             break;
         case CmdTx:
-            (void)0;
-            uint32_t tbd_array = le32_to_cpu(tx.tx_desc_addr);
-            uint16_t tcb_bytes = (le16_to_cpu(tx.tcb_bytes) & 0x3fff);
-            TRACE(RXTX, logout
-                ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count %u\n",
-                 tbd_array, tcb_bytes, tx.tbd_count));
-
             if (bit_nc) {
                 missing("CmdTx: NC = 0");
                 success = false;
                 break;
             }
-            //~ assert(!bit_sf);
-            if (tcb_bytes > 2600) {
-                logout("TCB byte count too large, using 2600\n");
-                tcb_bytes = 2600;
-            }
-            /* Next assertion fails for local configuration. */
-            //~ assert((tcb_bytes > 0) || (tbd_array != 0xffffffff));
-            if (!((tcb_bytes > 0) || (tbd_array != 0xffffffff))) {
-                logout
-                    ("illegal values of TBD array address and TCB byte count!\n");
-            }
-            // sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes
-            uint8_t buf[2600];
-            uint16_t size = 0;
-            uint32_t tbd_address = cb_address + 0x10;
-            assert(tcb_bytes <= sizeof(buf));
-            while (size < tcb_bytes) {
-                uint32_t tx_buffer_address = ldl_phys(tbd_address);
-                uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
-                //~ uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
-                tbd_address += 8;
-                TRACE(RXTX, logout
-                    ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
-                     tx_buffer_address, tx_buffer_size));
-                tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
-                cpu_physical_memory_read(tx_buffer_address, &buf[size],
-                                         tx_buffer_size);
-                size += tx_buffer_size;
-            }
-            if (tbd_array == 0xffffffff) {
-                /* Simplified mode. Was already handled by code above. */
-            } else {
-                /* Flexible mode. */
-                uint8_t tbd_count = 0;
-                if (s->has_extended_tcb_support && !(s->configuration[6] & BIT(4))) {
-                    /* Extended Flexible TCB. */
-                    for (; tbd_count < 2; tbd_count++) {
-                        uint32_t tx_buffer_address = ldl_phys(tbd_address);
-                        uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
-                        uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
-                        tbd_address += 8;
-                        TRACE(RXTX, logout
-                            ("TBD (extended flexible mode): buffer address 0x%08x, size 0x%04x\n",
-                             tx_buffer_address, tx_buffer_size));
-                        tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
-                        cpu_physical_memory_read(tx_buffer_address, &buf[size],
-                                                 tx_buffer_size);
-                        size += tx_buffer_size;
-                        if (tx_buffer_el & 1) {
-                            break;
-                        }
-                    }
-                }
-                tbd_address = tbd_array;
-                for (; tbd_count < tx.tbd_count; tbd_count++) {
-                    uint32_t tx_buffer_address = ldl_phys(tbd_address);
-                    uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
-                    uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
-                    tbd_address += 8;
-                    TRACE(RXTX, logout
-                        ("TBD (flexible mode): buffer address 0x%08x, size 0x%04x\n",
-                         tx_buffer_address, tx_buffer_size));
-                    tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
-                    cpu_physical_memory_read(tx_buffer_address, &buf[size],
-                                             tx_buffer_size);
-                    size += tx_buffer_size;
-                    if (tx_buffer_el & 1) {
-                        break;
-                    }
-                }
-            }
-            TRACE(RXTX, logout("%p sending frame, len=%d,%s\n", s, size, nic_dump(buf, size)));
-            qemu_send_packet(&s->nic->nc, buf, size);
-            s->statistics.tx_good_frames++;
-            /* Transmit with bad status would raise an CX/TNO interrupt.
-             * (82557 only). Emulation never has bad status. */
-            //~ eepro100_cx_interrupt(s);
+            tx_command(s);
             break;
         case CmdTDR:
             TRACE(OTHER, logout("load microcode\n"));
@@ -885,7 +889,7 @@ static void action_command(EEPRO100State *s)
             break;
         }
         /* Write new status. */
-        stw_phys(cb_address, status | 0x8000 | (success ? 0x2000 : 0));
+        stw_phys(s->cb_address, status | 0x8000 | (success ? 0x2000 : 0));
         if (bit_i) {
             /* CU completed action. */
             eepro100_cx_interrupt(s);
-- 
1.6.5

[Qemu-devel] [PATCH 2/3] eepro100: Better documentation for temporary data

[Stefan Weil]

Michael S. Tsirkin suggested more documentation for
two temporary status values. Well, here it is.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 5635f61..fb455a4 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -213,9 +213,10 @@ typedef struct {
     uint32_t ru_offset;         /* RU address offset */
     uint32_t statsaddr;         /* pointer to eepro100_stats_t */
 
-    /* Temporary data. */
-    eepro100_tx_t tx;
-    uint32_t cb_address;
+    /* Temporary status information (no need to save these values),
+     * used while processing CU commands. */
+    eepro100_tx_t tx;           /* transmit buffer descriptor */
+    uint32_t cb_address;        /* = cu_base + cu_offset */
 
     /* Statistical counters. Also used for wake-up packet (i82559). */
     eepro100_stats_t statistics;
-- 
1.6.5

[Qemu-devel] [PATCH 3/3] eepro100: Fix multicast support

[Stefan Weil]

* Handling of multicast list was missing.
* Multicast all was missing.
* Promiscuous mode for multicast frames was wrong.

This patch is a step to synchronize my maintainer version
of eepro100.c (git://repo.or.cz/qemu/ar7.git) with the
version integrated in QEMU.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |   52 +++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index fb455a4..5a77409 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -141,7 +141,7 @@ typedef struct {
     uint16_t status;
     uint16_t command;
     uint32_t link;              /* void * */
-    uint32_t tx_desc_addr;      /* transmit buffer decsriptor array address. */
+    uint32_t tbd_array_addr;    /* transmit buffer descriptor array address. */
     uint16_t tcb_bytes;         /* transmit command block byte count (in lower 14 bits */
     uint8_t tx_threshold;       /* transmit threshold */
     uint8_t tbd_count;          /* TBD number */
@@ -268,7 +268,7 @@ static void stl_le_phys(target_phys_addr_t addr, uint32_t val)
 
 /* From FreeBSD */
 /* XXX: optimize */
-static int compute_mcast_idx(const uint8_t * ep)
+static unsigned compute_mcast_idx(const uint8_t * ep)
 {
     uint32_t crc;
     int carry, i, j;
@@ -286,7 +286,7 @@ static int compute_mcast_idx(const uint8_t * ep)
             }
         }
     }
-    return (crc >> 26);
+    return (crc & BITS(7, 2)) >> 2;
 }
 
 #if defined(DEBUG_EEPRO100)
@@ -636,6 +636,8 @@ static void nic_reset(void *opaque)
 {
     EEPRO100State *s = opaque;
     TRACE(OTHER, logout("%p\n", s));
+    /* TODO: Clearing of multicast table for selective reset, too? */
+    memset(&s->mult[0], 0, sizeof(s->mult));
     nic_selective_reset(s);
 }
 
@@ -755,7 +757,7 @@ static void dump_statistics(EEPRO100State * s)
 
 static void tx_command(EEPRO100State *s)
 {
-    uint32_t tbd_array = le32_to_cpu(s->tx.tx_desc_addr);
+    uint32_t tbd_array = le32_to_cpu(s->tx.tbd_array_addr);
     uint16_t tcb_bytes = (le16_to_cpu(s->tx.tcb_bytes) & 0x3fff);
     /* Sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes. */
     uint8_t buf[2600];
@@ -837,6 +839,22 @@ static void tx_command(EEPRO100State *s)
     //~ eepro100_cx_interrupt(s);
 }
 
+static void set_multicast_list(EEPRO100State *s)
+{
+    uint16_t multicast_count = s->tx.tbd_array_addr & BITS(13, 0);
+    uint16_t i;
+    memset(&s->mult[0], 0, sizeof(s->mult));
+    TRACE(OTHER, logout("multicast list, multicast count = %u\n", multicast_count));
+    for (i = 0; i < multicast_count; i += 6) {
+        uint8_t multicast_addr[6];
+        cpu_physical_memory_read(s->cb_address + 10 + i, multicast_addr, 6);
+        TRACE(OTHER, logout("multicast entry %s\n", nic_dump(multicast_addr, 6)));
+        unsigned mcast_idx = compute_mcast_idx(multicast_addr);
+        assert(mcast_idx < 64);
+        s->mult[mcast_idx >> 3] |= (1 << (mcast_idx & 7));
+    }
+}
+
 static void action_command(EEPRO100State *s)
 {
     for (;;) {
@@ -869,7 +887,7 @@ static void action_command(EEPRO100State *s)
             TRACE(OTHER, logout("configuration: %s\n", nic_dump(&s->configuration[0], 16)));
             break;
         case CmdMulticastList:
-            //~ missing("multicast list");
+            set_multicast_list(s);
             break;
         case CmdTx:
             if (bit_nc) {
@@ -1652,17 +1670,25 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
         /* Broadcast frame. */
         TRACE(RXTX, logout("%p received broadcast, len=%zu\n", s, size));
         rfd_status |= 0x0002;
-    } else if (buf[0] & 0x01) { // !!!
+    } else if (buf[0] & 0x01) {
         /* Multicast frame. */
-        TRACE(RXTX, logout("%p received multicast, len=%zu\n", s, size));
-        /* TODO: check multicast all bit. */
+        TRACE(RXTX, logout("%p received multicast, len=%zu,%s\n", s, size, nic_dump(buf, size)));
         if (s->configuration[21] & BIT(3)) {
-            missing("Multicast All bit");
-        }
-        int mcast_idx = compute_mcast_idx(buf);
-        if (!(s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7)))) {
-            return size;
+          /* Multicast all bit is set, receive all multicast frames. */
+        } else {
+          unsigned mcast_idx = compute_mcast_idx(buf);
+          assert(mcast_idx < 64);
+          if (s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7))) {
+            /* Multicast frame is allowed in hash table. */
+          } else if (s->configuration[15] & 1) {
+              /* Promiscuous: receive all. */
+              rfd_status |= 0x0004;
+          } else {
+              TRACE(RXTX, logout("%p multicast ignored\n", s));
+              return -1;
+          }
         }
+        /* TODO: Next not for promiscuous mode? */
         rfd_status |= 0x0002;
     } else if (s->configuration[15] & 1) {
         /* Promiscuous: receive all. */
-- 
1.6.5

Re: [Qemu-devel] [PATCH 2/3] eepro100: Better documentation for temporary data

[Andreas Färber]

Hello,

Am 20.12.2009 um 16:52 schrieb Stefan Weil:

> Michael S. Tsirkin suggested more documentation for
> two temporary status values. Well, here it is.
>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
> hw/eepro100.c |    7 ++++---
> 1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 5635f61..fb455a4 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -213,9 +213,10 @@ typedef struct {
>     uint32_t ru_offset;         /* RU address offset */
>     uint32_t statsaddr;         /* pointer to eepro100_stats_t */
>
> -    /* Temporary data. */
> -    eepro100_tx_t tx;
> -    uint32_t cb_address;

Since you just introduced this in patch 1/3, please add the verbose  
documentation to the patch introducing these variables.

Andreas

> +    /* Temporary status information (no need to save these values),
> +     * used while processing CU commands. */
> +    eepro100_tx_t tx;           /* transmit buffer descriptor */
> +    uint32_t cb_address;        /* = cu_base + cu_offset */
>
>     /* Statistical counters. Also used for wake-up packet (i82559). */
>     eepro100_stats_t statistics;
> -- 
> 1.6.5
>
>
>

Re: [Qemu-devel] [PATCH 2/3] eepro100: Better documentation for temporary data

[Stefan Weil]

Andreas Färber schrieb:
> Hello,
>
> Am 20.12.2009 um 16:52 schrieb Stefan Weil:
>
>> Michael S. Tsirkin suggested more documentation for
>> two temporary status values. Well, here it is.
>>
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>> ---
>> hw/eepro100.c |    7 ++++---
>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index 5635f61..fb455a4 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -213,9 +213,10 @@ typedef struct {
>>     uint32_t ru_offset;         /* RU address offset */
>>     uint32_t statsaddr;         /* pointer to eepro100_stats_t */
>>
>> -    /* Temporary data. */
>> -    eepro100_tx_t tx;
>> -    uint32_t cb_address;
>
> Since you just introduced this in patch 1/3, please add the verbose
> documentation to the patch introducing these variables.
>
> Andreas

Hi Andreas,

in this special case I won't resend the patch.
It was already sent too often :-)

Regards
Stefan

[Qemu-devel] Re: [PATCH 1/3] eepro100: Restructure code (new function tx_command)

[Michael S. Tsirkin]

On Sun, Dec 20, 2009 at 04:52:22PM +0100, Stefan Weil wrote:
> Handling of transmit commands is rather complex,
> so about 80 lines of code were moved from function
> action_command to the new function tx_command.
> 
> The two new values "tx" and "cb_address" in the
> eepro100 status structure made this possible without
> passing too many parameters.
> 
> In addition, the moved code was cleaned a little bit:
> old comments marked with //~ were removed, C++ style
> comments were replaced by C style comments, C++ like
> variable declarations after code were reordered.
> 
> Simplified mode is still broken. Nor did I fix
> endianess issues. Both problems will be fixed in
> additional patches (which need this one).
> 
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>

Thanks, applied.
I merged this with the following commit: no reason
to change the same line twice.

> ---
>  hw/eepro100.c |  192 +++++++++++++++++++++++++++++----------------------------
>  1 files changed, 98 insertions(+), 94 deletions(-)
> 
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 2a9e3b5..5635f61 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -213,6 +213,10 @@ typedef struct {
>      uint32_t ru_offset;         /* RU address offset */
>      uint32_t statsaddr;         /* pointer to eepro100_stats_t */
>  
> +    /* Temporary data. */
> +    eepro100_tx_t tx;
> +    uint32_t cb_address;
> +
>      /* Statistical counters. Also used for wake-up packet (i82559). */
>      eepro100_stats_t statistics;
>  
> @@ -748,17 +752,100 @@ static void dump_statistics(EEPRO100State * s)
>      //~ missing("CU dump statistical counters");
>  }
>  
> +static void tx_command(EEPRO100State *s)
> +{
> +    uint32_t tbd_array = le32_to_cpu(s->tx.tx_desc_addr);
> +    uint16_t tcb_bytes = (le16_to_cpu(s->tx.tcb_bytes) & 0x3fff);
> +    /* Sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes. */
> +    uint8_t buf[2600];
> +    uint16_t size = 0;
> +    uint32_t tbd_address = s->cb_address + 0x10;
> +    TRACE(RXTX, logout
> +        ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count %u\n",
> +         tbd_array, tcb_bytes, s->tx.tbd_count));
> +
> +    if (tcb_bytes > 2600) {
> +        logout("TCB byte count too large, using 2600\n");
> +        tcb_bytes = 2600;
> +    }
> +    if (!((tcb_bytes > 0) || (tbd_array != 0xffffffff))) {
> +        logout
> +            ("illegal values of TBD array address and TCB byte count!\n");
> +    }
> +    assert(tcb_bytes <= sizeof(buf));
> +    while (size < tcb_bytes) {
> +        uint32_t tx_buffer_address = ldl_phys(tbd_address);
> +        uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
> +        //~ uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
> +        tbd_address += 8;
> +        TRACE(RXTX, logout
> +            ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
> +             tx_buffer_address, tx_buffer_size));
> +        tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
> +        cpu_physical_memory_read(tx_buffer_address, &buf[size],
> +                                 tx_buffer_size);
> +        size += tx_buffer_size;
> +    }
> +    if (tbd_array == 0xffffffff) {
> +        /* Simplified mode. Was already handled by code above. */
> +    } else {
> +        /* Flexible mode. */
> +        uint8_t tbd_count = 0;
> +        if (s->has_extended_tcb_support && !(s->configuration[6] & BIT(4))) {
> +            /* Extended Flexible TCB. */
> +            for (; tbd_count < 2; tbd_count++) {
> +                uint32_t tx_buffer_address = ldl_phys(tbd_address);
> +                uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
> +                uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
> +                tbd_address += 8;
> +                TRACE(RXTX, logout
> +                    ("TBD (extended flexible mode): buffer address 0x%08x, size 0x%04x\n",
> +                     tx_buffer_address, tx_buffer_size));
> +                tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
> +                cpu_physical_memory_read(tx_buffer_address, &buf[size],
> +                                         tx_buffer_size);
> +                size += tx_buffer_size;
> +                if (tx_buffer_el & 1) {
> +                    break;
> +                }
> +            }
> +        }
> +        tbd_address = tbd_array;
> +        for (; tbd_count < s->tx.tbd_count; tbd_count++) {
> +            uint32_t tx_buffer_address = ldl_phys(tbd_address);
> +            uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
> +            uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
> +            tbd_address += 8;
> +            TRACE(RXTX, logout
> +                ("TBD (flexible mode): buffer address 0x%08x, size 0x%04x\n",
> +                 tx_buffer_address, tx_buffer_size));
> +            tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
> +            cpu_physical_memory_read(tx_buffer_address, &buf[size],
> +                                     tx_buffer_size);
> +            size += tx_buffer_size;
> +            if (tx_buffer_el & 1) {
> +                break;
> +            }
> +        }
> +    }
> +    TRACE(RXTX, logout("%p sending frame, len=%d,%s\n", s, size, nic_dump(buf, size)));
> +    qemu_send_packet(&s->nic->nc, buf, size);
> +    s->statistics.tx_good_frames++;
> +    /* Transmit with bad status would raise an CX/TNO interrupt.
> +     * (82557 only). Emulation never has bad status. */
> +    //~ eepro100_cx_interrupt(s);
> +}
> +
>  static void action_command(EEPRO100State *s)
>  {
>      for (;;) {
> -        uint32_t cb_address = s->cu_base + s->cu_offset;
> -        eepro100_tx_t tx;
> -        cpu_physical_memory_read(cb_address, (uint8_t *) & tx, sizeof(tx));
> -        uint16_t status = le16_to_cpu(tx.status);
> -        uint16_t command = le16_to_cpu(tx.command);
> +        s->cb_address = s->cu_base + s->cu_offset;
> +        cpu_physical_memory_read(s->cb_address, (uint8_t *)&s->tx, sizeof(s->tx));
> +        uint16_t status = le16_to_cpu(s->tx.status);
> +        uint16_t command = le16_to_cpu(s->tx.command);
>          logout
>              ("val=0x%02x (cu start), status=0x%04x, command=0x%04x, link=0x%08x\n",
> -             val, status, command, tx.link);
> +             val, status, command, s->tx.link);
>          bool bit_el = ((command & 0x8000) != 0);
>          bool bit_s = ((command & 0x4000) != 0);
>          bool bit_i = ((command & 0x2000) != 0);
> @@ -766,17 +853,17 @@ static void action_command(EEPRO100State *s)
>          bool success = true;
>          //~ bool bit_sf = ((command & 0x0008) != 0);
>          uint16_t cmd = command & 0x0007;
> -        s->cu_offset = le32_to_cpu(tx.link);
> +        s->cu_offset = le32_to_cpu(s->tx.link);
>          switch (cmd) {
>          case CmdNOp:
>              /* Do nothing. */
>              break;
>          case CmdIASetup:
> -            cpu_physical_memory_read(cb_address + 8, &s->conf.macaddr.a[0], 6);
> +            cpu_physical_memory_read(s->cb_address + 8, &s->conf.macaddr.a[0], 6);
>              TRACE(OTHER, logout("macaddr: %s\n", nic_dump(&s->macaddr[0], 6)));
>              break;
>          case CmdConfigure:
> -            cpu_physical_memory_read(cb_address + 8, &s->configuration[0],
> +            cpu_physical_memory_read(s->cb_address + 8, &s->configuration[0],
>                                       sizeof(s->configuration));
>              TRACE(OTHER, logout("configuration: %s\n", nic_dump(&s->configuration[0], 16)));
>              break;
> @@ -784,95 +871,12 @@ static void action_command(EEPRO100State *s)
>              //~ missing("multicast list");
>              break;
>          case CmdTx:
> -            (void)0;
> -            uint32_t tbd_array = le32_to_cpu(tx.tx_desc_addr);
> -            uint16_t tcb_bytes = (le16_to_cpu(tx.tcb_bytes) & 0x3fff);
> -            TRACE(RXTX, logout
> -                ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count %u\n",
> -                 tbd_array, tcb_bytes, tx.tbd_count));
> -
>              if (bit_nc) {
>                  missing("CmdTx: NC = 0");
>                  success = false;
>                  break;
>              }
> -            //~ assert(!bit_sf);
> -            if (tcb_bytes > 2600) {
> -                logout("TCB byte count too large, using 2600\n");
> -                tcb_bytes = 2600;
> -            }
> -            /* Next assertion fails for local configuration. */
> -            //~ assert((tcb_bytes > 0) || (tbd_array != 0xffffffff));
> -            if (!((tcb_bytes > 0) || (tbd_array != 0xffffffff))) {
> -                logout
> -                    ("illegal values of TBD array address and TCB byte count!\n");
> -            }
> -            // sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes
> -            uint8_t buf[2600];
> -            uint16_t size = 0;
> -            uint32_t tbd_address = cb_address + 0x10;
> -            assert(tcb_bytes <= sizeof(buf));
> -            while (size < tcb_bytes) {
> -                uint32_t tx_buffer_address = ldl_phys(tbd_address);
> -                uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
> -                //~ uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
> -                tbd_address += 8;
> -                TRACE(RXTX, logout
> -                    ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
> -                     tx_buffer_address, tx_buffer_size));
> -                tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
> -                cpu_physical_memory_read(tx_buffer_address, &buf[size],
> -                                         tx_buffer_size);
> -                size += tx_buffer_size;
> -            }
> -            if (tbd_array == 0xffffffff) {
> -                /* Simplified mode. Was already handled by code above. */
> -            } else {
> -                /* Flexible mode. */
> -                uint8_t tbd_count = 0;
> -                if (s->has_extended_tcb_support && !(s->configuration[6] & BIT(4))) {
> -                    /* Extended Flexible TCB. */
> -                    for (; tbd_count < 2; tbd_count++) {
> -                        uint32_t tx_buffer_address = ldl_phys(tbd_address);
> -                        uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
> -                        uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
> -                        tbd_address += 8;
> -                        TRACE(RXTX, logout
> -                            ("TBD (extended flexible mode): buffer address 0x%08x, size 0x%04x\n",
> -                             tx_buffer_address, tx_buffer_size));
> -                        tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
> -                        cpu_physical_memory_read(tx_buffer_address, &buf[size],
> -                                                 tx_buffer_size);
> -                        size += tx_buffer_size;
> -                        if (tx_buffer_el & 1) {
> -                            break;
> -                        }
> -                    }
> -                }
> -                tbd_address = tbd_array;
> -                for (; tbd_count < tx.tbd_count; tbd_count++) {
> -                    uint32_t tx_buffer_address = ldl_phys(tbd_address);
> -                    uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
> -                    uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
> -                    tbd_address += 8;
> -                    TRACE(RXTX, logout
> -                        ("TBD (flexible mode): buffer address 0x%08x, size 0x%04x\n",
> -                         tx_buffer_address, tx_buffer_size));
> -                    tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
> -                    cpu_physical_memory_read(tx_buffer_address, &buf[size],
> -                                             tx_buffer_size);
> -                    size += tx_buffer_size;
> -                    if (tx_buffer_el & 1) {
> -                        break;
> -                    }
> -                }
> -            }
> -            TRACE(RXTX, logout("%p sending frame, len=%d,%s\n", s, size, nic_dump(buf, size)));
> -            qemu_send_packet(&s->nic->nc, buf, size);
> -            s->statistics.tx_good_frames++;
> -            /* Transmit with bad status would raise an CX/TNO interrupt.
> -             * (82557 only). Emulation never has bad status. */
> -            //~ eepro100_cx_interrupt(s);
> +            tx_command(s);
>              break;
>          case CmdTDR:
>              TRACE(OTHER, logout("load microcode\n"));
> @@ -885,7 +889,7 @@ static void action_command(EEPRO100State *s)
>              break;
>          }
>          /* Write new status. */
> -        stw_phys(cb_address, status | 0x8000 | (success ? 0x2000 : 0));
> +        stw_phys(s->cb_address, status | 0x8000 | (success ? 0x2000 : 0));
>          if (bit_i) {
>              /* CU completed action. */
>              eepro100_cx_interrupt(s);
> -- 
> 1.6.5
> 
> 

Re: [Qemu-devel] eepro100.c patches

[Anthony Liguori]

Stefan Weil wrote:
> Anthony Liguori schrieb:
>   
>> Stefan Weil wrote:
>>     
>>> Hi,
>>>
>>> these patches are still missing in QEMU master
>>> (and would also be good for 0.12.x because many
>>> people wait for the multicast fix needed for
>>> junos, see "junos qemu eepro100.c" at google):
>>>
>>> 089c60f8e467cfd567a946e005f10559b2fb9462
>>> f925f386761fe1abb82aad8472c6fb388b022756
>>> 5a89b50ef52e7c9c3cf072638594880fa1a8f596
>>> (apply them in this order to avoid conflicts)
>>>   
>>>       
>> Commit ids are referring to?
>>
>> None of the patches you have outstanding are 0.12.x material because
>> they don't actually fix any bugs.
>>
>> If you've got bug fixes patches that these depend on, then please send
>> the whole thing out as a series.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>     
>
> Ah, sorry, I sent the commit ids which are not included in the patch mails.
>
> These patches are missing:
>
> * eepro100: Restructure code (new function tx_command)
>   http://patchwork.ozlabs.org/patch/40965/
>
> * eepro100: Better documentation for temporary data
>   http://patchwork.ozlabs.org/patch/41072/
>
> * eepro100: Fix multicast support
>   http://patchwork.ozlabs.org/patch/33959/
>
> The last one is a bug fix needed for junos and other
> applications which depend on correct multicast handling.
>
> I'll resent the patches as a series.
>   

Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] eepro100.c patches

[Stefan Weil]

Anthony Liguori schrieb:
> Stefan Weil wrote:
>> Anthony Liguori schrieb:
>>  
>>> Stefan Weil wrote:
>>>    
>>>> Hi,
>>>>
>>>> these patches are still missing in QEMU master
>>>> (and would also be good for 0.12.x because many
>>>> people wait for the multicast fix needed for
>>>> junos, see "junos qemu eepro100.c" at google):
>>>>
>>>> 089c60f8e467cfd567a946e005f10559b2fb9462
>>>> f925f386761fe1abb82aad8472c6fb388b022756
>>>> 5a89b50ef52e7c9c3cf072638594880fa1a8f596
>>>> (apply them in this order to avoid conflicts)
>>>>         
>>> Commit ids are referring to?
>>>
>>> None of the patches you have outstanding are 0.12.x material because
>>> they don't actually fix any bugs.
>>>
>>> If you've got bug fixes patches that these depend on, then please send
>>> the whole thing out as a series.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>     
>>
>> Ah, sorry, I sent the commit ids which are not included in the patch
>> mails.
>>
>> These patches are missing:
>>
>> * eepro100: Restructure code (new function tx_command)
>>   http://patchwork.ozlabs.org/patch/40965/
>>
>> * eepro100: Better documentation for temporary data
>>   http://patchwork.ozlabs.org/patch/41072/
>>
>> * eepro100: Fix multicast support
>>   http://patchwork.ozlabs.org/patch/33959/
>>
>> The last one is a bug fix needed for junos and other
>> applications which depend on correct multicast handling.
>>
>> I'll resent the patches as a series.
>>   
>
> Thanks.
>
> Regards,
>
> Anthony Liguori

Hi,

I hope that it is possible to apply these patches now.
Please give feedback if anything is missing.

Regards,

Stefan Weil