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: 2373 bytes --]

On Wed, Aug 12, 2009 at 03:04:17AM +0400, malc wrote:
> On Tue, 11 Aug 2009, Reimar D?ffinger wrote:
> 
> > This adds support for some kind of receive buffers "flexible
> > mode". The Intel documentation as I read it claims that no such mode exist
> > for receive, but the fact that those (working with real hardware I
> > expect) drivers use it contradicts that...
> > This _definitely_ is necessary to support these AppleIntel8255x drivers.
> > 
> > Signed-off-by: Reimar D?ffinger <Reimar.Doeffinger@gmx.de>
> > ---
> >  hw/eepro100.c |   27 ++++++++++++++++++++++++---
> >  1 files changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/eepro100.c b/hw/eepro100.c
> > index f619d36..5620bc7 100644
> > --- a/hw/eepro100.c
> > +++ b/hw/eepro100.c
> > @@ -153,6 +153,14 @@ typedef struct {
> >      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;
> 
> _t suffix aside (the whole file is quite tainted in this regard) and from
> skimming over the rest of the file it appears that this structure is
> shared with guest, even though it's neatly/(and for some definition of 
> natural)naturally aligned, i believe this is dodgy. Then again maybe just
> like _t the existing code already expects things to be rosy in this
> regard.

Well, I had mostly the same thoughts while working on this, but I
thought it preferable to keep with the current "style" even if it is
bad/problematic.
Discussing the best way to do it and then fixing all the code at a later
point seemed like a better idea (well, if the goal is to get this
applied without having to fix all those little issues with the current
code first).
Particularly in this case it is simple and still readable to just use
lduw_phys I think if you prefer...
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.
Also I expect it to be full of stuff that can be discussed for weeks,
i.e. typical bikeshed material...
And maybe this is the kind of thing Stefan Weil might want to take care
of?

P.S.: And I'd appreciate it if you wouldn't CC me, I am subscribed to the
list. Thanks.

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

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 2099459..1e2f670 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -142,25 +142,6 @@ typedef struct {
     //~ 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,
@@ -1075,11 +1056,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;
@@ -1096,11 +1072,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);
@@ -1523,29 +1498,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. */
@@ -1555,7 +1531,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);