Re: [Qemu-devel] [PATCH 3/5] Add support for receiving via receive buffers. While the Intel documentation claims this is unsupported, the OS X drivers use it, causing an assertion failure since rx buffer size is 0.

["Reimar Döffinger" <Reimar.Doeffinger@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On Wed, Aug 12, 2009 at 02:35:53AM +0200, Reimar Döffinger wrote:
> I attached a proof-of concept patch that converts parts of it, but doing
> the same for the tx_t and statistics_t structs would get a bit messy
> without changing the overall code a bit.

It's not really that bad, attached is a patch that changes all of them
so no structs are directly read from/written to memory.
It also includes a change to make the statistics dumping actually work
(currently the driver never gets notified that they were dumped, and for
the 82558 and 82559 devices parts of the statistics were left
uninitialized. I know this really belongs in a separate patch but as
long as I don't know when or if ever my patches get accepted I am too
lazy for that.
For now I allow myself to misuse this list mostly as a backup for my
code :-P

[-- Attachment #2: avoid_structs.diff --]
[-- Type: text/plain, Size: 11151 bytes --]

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 7c951c0..c192599 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -126,41 +126,6 @@ enum speedo_offsets {
     SCBFlow = 24,
 };
 
-/* A speedo3 transmit buffer descriptor with two buffers... */
-typedef struct {
-    uint16_t status;
-    uint16_t command;
-    uint32_t link;              /* void * */
-    uint32_t tx_desc_addr;      /* transmit buffer decsriptor 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 */
-    //~ /* This constitutes two "TBD" entries: hdr and data */
-    //~ uint32_t tx_buf_addr0;  /* void *, header of frame to be transmitted.  */
-    //~ int32_t  tx_buf_size0;  /* Length of Tx hdr. */
-    //~ uint32_t tx_buf_addr1;  /* void *, data to be transmitted.  */
-    //~ int32_t  tx_buf_size1;  /* Length of Tx data. */
-} eepro100_tx_t;
-
-/* Receive frame descriptor. */
-typedef struct {
-    int16_t status;
-    uint16_t command;
-    uint32_t link;              /* struct RxFD * */
-    uint32_t rx_buf_addr;       /* void * */
-    uint16_t count;
-    uint16_t size;
-    char packet[MAX_ETH_FRAME_SIZE + 4];
-} eepro100_rx_t;
-
-/* Receive buffer descriptor. */
-typedef struct {
-    uint32_t count;
-    uint32_t link;
-    uint32_t buffer;
-    uint32_t size;
-} eepro100_rbd_t;
-
 typedef struct {
     uint32_t tx_good_frames, tx_max_collisions, tx_late_collisions,
         tx_underruns, tx_lost_crs, tx_deferred, tx_single_collisions,
@@ -229,6 +194,7 @@ typedef struct {
     uint32_t rbd_addr;
     uint32_t statsaddr;         /* pointer to eepro100_stats_t */
     eepro100_stats_t statistics;        /* statistical counters */
+    int stats_size;
 #if 0
     uint16_t status;
 #endif
@@ -621,26 +587,29 @@ static void set_ru_state(EEPRO100State * s, ru_state_t state)
     s->mem[SCBStatus] = (s->mem[SCBStatus] & 0xc3) + (state << 2);
 }
 
-static void dump_statistics(EEPRO100State * s)
+static void dump_statistics(EEPRO100State * s, int reset)
 {
     /* Dump statistical data. Most data is never changed by the emulation
      * and always 0, so we first just copy the whole block and then those
      * values which really matter.
      * Number of data should check configuration!!!
      */
-    cpu_physical_memory_write(s->statsaddr, (uint8_t *) & s->statistics, 64);
+    uint8_t zeros[80];
+    assert(s->stats_size <= sizeof(zeros));
+    memset(zeros, 0, sizeof(zeros));
+    cpu_physical_memory_write(s->statsaddr, zeros, s->stats_size);
     stl_phys(s->statsaddr + 0, s->statistics.tx_good_frames);
     stl_phys(s->statsaddr + 36, s->statistics.rx_good_frames);
     stl_phys(s->statsaddr + 48, s->statistics.rx_resource_errors);
     stl_phys(s->statsaddr + 60, s->statistics.rx_short_frame_errors);
     //~ stw_phys(s->statsaddr + 76, s->statistics.xmt_tco_frames);
     //~ stw_phys(s->statsaddr + 78, s->statistics.rcv_tco_frames);
-    //~ missing("CU dump statistical counters");
+    stl_phys(s->statsaddr + s->stats_size, reset ? 0xA007 : 0xA005);
+    memset(&s->statistics, 0, sizeof(s->statistics));
 }
 
 static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
 {
-    eepro100_tx_t tx;
     uint32_t cb_address;
     switch (val) {
     case CU_NOP:
@@ -658,19 +627,21 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
         s->cu_offset = s->pointer;
       next_command:
         cb_address = s->cu_base + s->cu_offset;
-        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);
+        uint16_t status       = lduw_phys(cb_address);
+        uint16_t command      = lduw_phys(cb_address +  2);
+        s->cu_offset          = ldl_phys (cb_address +  4);
+        uint32_t tbd_array    = ldl_phys (cb_address +  8);
+        uint16_t tcb_bytes    = lduw_phys(cb_address + 12) & 0x3fff;
+        uint8_t  tx_tbd_count = ldub_phys(cb_address + 15);
         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->cu_offset);
         bool bit_el = ((command & 0x8000) != 0);
         bool bit_s = ((command & 0x4000) != 0);
         bool bit_i = ((command & 0x2000) != 0);
         bool bit_nc = ((command & 0x0010) != 0);
         //~ bool bit_sf = ((command & 0x0008) != 0);
         uint16_t cmd = command & 0x0007;
-        s->cu_offset = le32_to_cpu(tx.link);
         switch (cmd) {
         case CmdNOp:
             /* Do nothing. */
@@ -689,11 +660,9 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
             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);
             logout
                 ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count %u\n",
-                 tbd_array, tcb_bytes, tx.tbd_count);
+                 tbd_array, tcb_bytes, tx_tbd_count);
             assert(!bit_nc);
             //~ assert(!bit_sf);
             assert(tcb_bytes <= 2600);
@@ -744,7 +713,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
                     }
                 }
                 tbd_address = tbd_array;
-                for (; tbd_count < tx.tbd_count; tbd_count++) {
+                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);
@@ -821,7 +790,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
         break;
     case CU_SHOWSTATS:
         /* Dump statistical counters. */
-        dump_statistics(s);
+        dump_statistics(s, 0);
         break;
     case CU_CMD_BASE:
         /* Load CU base. */
@@ -830,8 +799,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
         break;
     case CU_DUMPSTATS:
         /* Dump and reset statistical counters. */
-        dump_statistics(s);
-        memset(&s->statistics, 0, sizeof(s->statistics));
+        dump_statistics(s, 1);
         break;
     case CU_SRESUME:
         /* CU static resume. */
@@ -1082,11 +1050,6 @@ static void eepro100_write_mdi(EEPRO100State * s, uint32_t val)
 #define PORT_DUMP               3
 #define PORT_SELECTION_MASK     3
 
-typedef struct {
-    uint32_t st_sign;           /* Self Test Signature */
-    uint32_t st_result;         /* Self Test Results */
-} eepro100_selftest_t;
-
 static uint32_t eepro100_read_port(EEPRO100State * s)
 {
     return 0;
@@ -1103,11 +1066,10 @@ static void eepro100_write_port(EEPRO100State * s, uint32_t val)
         break;
     case PORT_SELFTEST:
         logout("selftest address=0x%08x\n", address);
-        eepro100_selftest_t data;
-        cpu_physical_memory_read(address, (uint8_t *) & data, sizeof(data));
-        data.st_sign = 0xffffffff;
-        data.st_result = 0;
-        cpu_physical_memory_write(address, (uint8_t *) & data, sizeof(data));
+        // self-test signature, driver initializes to 0
+        stl_phys(address,     0xffffffff);
+        // self-test result (failure bitmask), driver initializes to 0xffffffff
+        stl_phys(address + 4, 0);
         break;
     case PORT_SELECTIVE_RESET:
         logout("selective reset, selftest address=0x%08x\n", address);
@@ -1530,29 +1492,30 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
     }
     //~ !!!
 //~ $3 = {status = 0x0, command = 0xc000, link = 0x2d220, rx_buf_addr = 0x207dc, count = 0x0, size = 0x5f8, packet = {0x0 <repeats 1518 times>}}
-    eepro100_rx_t rx;
-    cpu_physical_memory_read(s->ru_base + s->ru_offset, (uint8_t *) & rx,
-                             offsetof(eepro100_rx_t, packet));
-    uint16_t rfd_command = le16_to_cpu(rx.command);
-    uint32_t rfd_size = le16_to_cpu(rx.size);
-    uint32_t dst_addr = s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, packet);
+    uint32_t rx = s->ru_base + s->ru_offset;
+    // Read and update the receive frame descriptor (RFD)
+    stw_phys (rx, rfd_status);
+    uint16_t rfd_command = lduw_phys(rx +  2);
+    s->ru_offset         = ldl_phys (rx +  4);
+    uint32_t rfd_rbd     = ldl_phys (rx +  8);
+    stw_phys (rx + 12, size);
+    uint32_t rfd_size    = lduw_phys(rx + 14);
+    uint32_t dst_addr    =           rx + 16;
     if (rfd_command & 8) {
         // argh! Flexible mode. Intel docs say it is not supported but the Mac OS driver uses it anyway.
-        eepro100_rbd_t rbd;
-        if (!s->rbd_addr)
-            s->rbd_addr = le32_to_cpu(rx.rx_buf_addr);
-        cpu_physical_memory_read(s->rbd_addr, (uint8_t *) & rbd, sizeof(rbd));
-        rfd_size = le32_to_cpu(rbd.size);
-        dst_addr = le32_to_cpu(rbd.buffer);
-        stl_phys(s->rbd_addr + offsetof(eepro100_rbd_t, count), size | 0x8000);
-        s->rbd_addr = le32_to_cpu(rbd.link);
+        // Read and update the receive buffer descriptor (RBD)
+        if (s->rbd_addr)
+            // Only the RBD address in the first RFD is valid, if we have a
+            // link value from a previous RBD follow that instead.
+            rfd_rbd = s->rbd_addr;
+        stl_phys(rfd_rbd, size | 0x8000);
+        s->rbd_addr = ldl_phys(rfd_rbd +  4);
+        dst_addr    = ldl_phys(rfd_rbd +  8);
+        rfd_size    = ldl_phys(rfd_rbd + 12);
     }
     assert(size <= rfd_size);
     logout("command 0x%04x, link 0x%08x, addr 0x%08x, size %u\n", rfd_command,
-           rx.link, rx.rx_buf_addr, rfd_size);
-    stw_phys(s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, status),
-             rfd_status);
-    stw_phys(s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, count), size);
+           rfd_link, rfd_rbd, rfd_size);
     /* Early receive interrupt not supported. */
     //~ eepro100_er_interrupt(s);
     /* Receive CRC Transfer not supported. */
@@ -1562,7 +1525,6 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
     cpu_physical_memory_write(dst_addr, buf, size);
     s->statistics.rx_good_frames++;
     eepro100_fr_interrupt(s);
-    s->ru_offset = le32_to_cpu(rx.link);
     if (rfd_command & 0x8000) {
         /* EL bit is set, so this was the last frame. */
         set_ru_state(s, ru_no_resources);
@@ -1772,6 +1734,23 @@ static void nic_init(PCIDevice *pci_dev, uint32_t device)
     s = &d->eepro100;
     s->device = device;
     s->pci_dev = &d->dev;
+    s->stats_size = 64;
+    switch (device) {
+    case i82551:
+        break;
+    case i82557B:
+    case i82557C:
+        break;
+    case i82558B:
+        s->stats_size = 76;
+        break;
+    case i82559C:
+    case i82559ER:
+        s->stats_size = 80;
+        break;
+    default:
+        assert(!"Unknown device set");
+    }
 
     pci_reset(s);