[Qemu-devel] [PATCH] Intel 8255x/eepro100 compatibility patches

[Qemu-devel] [PATCH] Intel 8255x/eepro100 compatibility patches

[Reimar Döffinger]

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

Hello everyone,
I have been playing around a bit with the OS X/darwin network drivers for these
cards and noticed that they seem to differ quite a bit from the Linux ones.
If you're interested, the source of the core part is here:
http://www.opensource.apple.com/source/AppleIntel8255x/AppleIntel8255x-19/i82557Private.cpp
Attached is a series of patches that makes things work with at least
some version of that (sorry, I only tried some binary I found on the
net, didn't compile from source).
In addition, I also used the documentation from here:
http://www.intel.com/design/network/manuals/8255X_OpenSDM.htm

The first patch does not set the SBAck flag for MDI interrupts when
interrupts are actually disabled for MDI.
I think (have not tested yet) that this is not necessary to fix
anything, but according to the documentation the current code is wrong.

The second patch is to ensure that a driver will not
accidentally/incorrectly change the RU/CU state with a write.
This is incomplete and a bit ugly, but good enough for these drivers.

The third patch 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 drivers.

And the last patch expands received data shorter than 60 bytes so no
short-frame detection is incorrectly triggered, and of course also
throws away all short-frame detection code since it makes no sense.
It was also wrong since size is without CRC and thus the short frame
limit is 60, not 64. And related to that (but without a patch), I think
that the
>    } else if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] & 8)) {
check is wrong, too, and the + 4 should not be there...
Back to the patch, e.g. the rtl8139 driver also expands those frames
(and I just copied the code for that).
This too _definitely_ is necessary to support these drivers.

Hope this is interesting to someone and maybe we can even get these
merged...

Thanks,
Reimar Döffinger

[-- Attachment #2: 0001-Setting-the-MDI-SCBAck-flag-when-interrupts-for-MDI-.patch --]
[-- Type: text/plain, Size: 998 bytes --]

>From cdd96658f21e571c077fee08a4ba64e79d49c0b8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Reimar=20D=C3=B6ffinger?= <reimar@hokum.(none)>
Date: Sun, 9 Aug 2009 21:36:45 +0200
Subject: [PATCH 1/4] Setting the MDI SCBAck flag when interrupts for MDI are disabled is
 wrong, even if it does not seem to cause any real issue with known
 drivers.

---
 hw/eepro100.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index ec31a6a..bf5d920 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1043,9 +1043,7 @@ static void eepro100_write_mdi(EEPRO100State * s, uint32_t val)
             }
             data = s->mdimem[reg];
         }
-        /* Emulation takes no time to finish MDI transaction.
-         * Set MDI bit in SCB status register. */
-        s->mem[SCBAck] |= 0x08;
+        /* Emulation takes no time to finish MDI transaction. */
         val |= BIT(28);
         if (raiseint) {
             eepro100_mdi_interrupt(s);
-- 
1.6.4


[-- Attachment #3: 0002-Hack-to-make-sure-that-drivers-like-AppleIntel8255x-.patch --]
[-- Type: text/plain, Size: 1035 bytes --]

>From 84f2d6ecf89c1dbd5f83676251aa2f646e2270b1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Reimar=20D=C3=B6ffinger?= <reimar@hokum.(none)>
Date: Sun, 9 Aug 2009 21:39:36 +0200
Subject: [PATCH 2/4] Hack to make sure that drivers like AppleIntel8255x will not meddle
 with the RU/CU state when the ACK the interrupt with a 16 bit write.

---
 hw/eepro100.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index bf5d920..f619d36 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1249,7 +1249,11 @@ static void eepro100_write1(EEPRO100State * s, uint32_t addr, uint8_t val)
 static void eepro100_write2(EEPRO100State * s, uint32_t addr, uint16_t val)
 {
     if (addr <= sizeof(s->mem) - sizeof(val)) {
+        ru_state_t rtmp = get_ru_state(s);
+        cu_state_t ctmp = get_cu_state(s);
         memcpy(&s->mem[addr], &val, sizeof(val));
+        set_cu_state(s, ctmp);
+        set_ru_state(s, rtmp);
     }
 
     logout("addr=%s val=0x%04x\n", regname(addr), val);
-- 
1.6.4


[-- Attachment #4: 0003-Add-support-for-receiving-via-receive-buffers.patch --]
[-- Type: text/plain, Size: 3486 bytes --]

>From 670b2dcb0a0914415f58eabf56c304f72c9c23b1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Reimar=20D=C3=B6ffinger?= <reimar@hokum.(none)>
Date: Sun, 9 Aug 2009 20:06:46 +0200
Subject: [PATCH 3/4] 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.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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;
+
 typedef struct {
     uint32_t tx_good_frames, tx_max_collisions, tx_late_collisions,
         tx_underruns, tx_lost_crs, tx_deferred, tx_single_collisions,
@@ -218,6 +226,7 @@ typedef struct {
     /* (ru_base + ru_offset) address the RFD in the Receive Frame Area. */
     uint32_t ru_base;           /* RU base address */
     uint32_t ru_offset;         /* RU address offset */
+    uint32_t rbd_addr;
     uint32_t statsaddr;         /* pointer to eepro100_stats_t */
     eepro100_stats_t statistics;        /* statistical counters */
 #if 0
@@ -843,6 +852,7 @@ static void eepro100_ru_command(EEPRO100State * s, uint8_t val)
         }
         set_ru_state(s, ru_ready);
         s->ru_offset = s->pointer;
+        s->rbd_addr = 0;
         logout("val=0x%02x (rx start)\n", val);
         break;
     case RX_RESUME:
@@ -1512,7 +1522,19 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
     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);
-    uint16_t rfd_size = le16_to_cpu(rx.size);
+    uint32_t rfd_size = le16_to_cpu(rx.size);
+    uint32_t dst_addr = s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, packet);
+    if (rfd_command & 8) {
+        // argh! Flexible mode. Intel docs say it is not support 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);
+    }
     assert(size <= rfd_size);
     if (size < 64) {
         rfd_status |= 0x0080;
@@ -1528,8 +1550,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
     assert(!(s->configuration[18] & 4));
     /* TODO: check stripping enable bit. */
     //~ assert(!(s->configuration[17] & 1));
-    cpu_physical_memory_write(s->ru_base + s->ru_offset +
-                              offsetof(eepro100_rx_t, packet), buf, 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);
-- 
1.6.4


[-- Attachment #5: 0004-Short-frames-do-not-exist-so-remove-code-to-handle-t.patch --]
[-- Type: text/plain, Size: 2959 bytes --]

>From da19bed5b3ac1b409f0db91c881983360fbde946 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Reimar=20D=C3=B6ffinger?= <reimar@hokum.(none)>
Date: Sun, 9 Aug 2009 22:57:06 +0200
Subject: [PATCH 4/4] Short frames do not exist, so remove code to handle them.
 Also expand packets that are smaller than the shor frame limit,
 otherwise the OS X network stack seems to discard them.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
---
 hw/eepro100.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 5620bc7..d2c18cc 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1452,6 +1452,8 @@ static int nic_can_receive(VLANClientState *vc)
     //~ return !eepro100_buffer_full(s);
 }
 
+#define MIN_BUF_SIZE 60
+
 static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size)
 {
     /* TODO:
@@ -1459,6 +1461,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
      * - Interesting packets should set bit 29 in power management driver register.
      */
     EEPRO100State *s = vc->opaque;
+    uint8_t buf1[MIN_BUF_SIZE];
     uint16_t rfd_status = 0xa000;
     static const uint8_t broadcast_macaddr[6] =
         { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
@@ -1466,16 +1469,18 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
     /* TODO: check multiple IA bit. */
     assert(!(s->configuration[20] & BIT(6)));
 
+    /* Short frames can not happen on virtual hardware, so expand them. */
+    if (size < MIN_BUF_SIZE) {
+        memcpy(buf1, buf, size);
+        memset(buf1 + size, 0, MIN_BUF_SIZE - size);
+        buf = buf1;
+        size = MIN_BUF_SIZE;
+    }
+
     if (s->configuration[8] & 0x80) {
         /* CSMA is disabled. */
         logout("%p received while CSMA is disabled\n", s);
         return -1;
-    } else if (size < 64 && (s->configuration[7] & 1)) {
-        /* Short frame and configuration byte 7/0 (discard short receive) set:
-         * Short frame is discarded */
-        logout("%p received short frame (%d byte)\n", s, size);
-        s->statistics.rx_short_frame_errors++;
-        //~ return -1;
     } else if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] & 8)) {
         /* Long frame and configuration byte 18/3 (long receive ok) not set:
          * Long frames are discarded. */
@@ -1536,9 +1541,6 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
         s->rbd_addr = le32_to_cpu(rbd.link);
     }
     assert(size <= rfd_size);
-    if (size < 64) {
-        rfd_status |= 0x0080;
-    }
     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),
-- 
1.6.4

Re: [Qemu-devel] [PATCH] Intel 8255x/eepro100 compatibility patches

[Stefan Weil]

Reimar Döffinger schrieb:
> Hello everyone,
> I have been playing around a bit with the OS X/darwin network drivers for these
> cards and noticed that they seem to differ quite a bit from the Linux ones.
> If you're interested, the source of the core part is here:
> http://www.opensource.apple.com/source/AppleIntel8255x/AppleIntel8255x-19/i82557Private.cpp
> Attached is a series of patches that makes things work with at least
> some version of that (sorry, I only tried some binary I found on the
> net, didn't compile from source).
> In addition, I also used the documentation from here:
> http://www.intel.com/design/network/manuals/8255X_OpenSDM.htm
>
> The first patch does not set the SBAck flag for MDI interrupts when
> interrupts are actually disabled for MDI.
> I think (have not tested yet) that this is not necessary to fix
> anything, but according to the documentation the current code is wrong.
>
> The second patch is to ensure that a driver will not
> accidentally/incorrectly change the RU/CU state with a write.
> This is incomplete and a bit ugly, but good enough for these drivers.
>
> The third patch 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 drivers.
>
> And the last patch expands received data shorter than 60 bytes so no
> short-frame detection is incorrectly triggered, and of course also
> throws away all short-frame detection code since it makes no sense.
> It was also wrong since size is without CRC and thus the short frame
> limit is 60, not 64. And related to that (but without a patch), I think
> that the
>   
>>    } else if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] & 8)) {
>>     
> check is wrong, too, and the + 4 should not be there...
> Back to the patch, e.g. the rtl8139 driver also expands those frames
> (and I just copied the code for that).
> This too _definitely_ is necessary to support these drivers.
>
> Hope this is interesting to someone and maybe we can even get these
> merged...
>
> Thanks,
> Reimar Döffinger
>   


Hi,

this is interesting to me. Maybe you want to try a newer version of
eepro100.c. The latest version is in http://repo.or.cz/w/qemu/ar7.git,
http://repo.or.cz/w/qemu/ar7.git?a=blob;f=hw/eepro100.c

It has some really important improvements. I want to get it ready
for inclusion in the official QEMU by the end of August.

Regards
Stefan Weil

Re: [Qemu-devel] [PATCH] Intel 8255x/eepro100 compatibility patches

[Reimar Döffinger]

On Mon, Aug 10, 2009 at 06:36:58AM +0200, Stefan Weil wrote:
> this is interesting to me. Maybe you want to try a newer version of
> eepro100.c. The latest version is in http://repo.or.cz/w/qemu/ar7.git,
> http://repo.or.cz/w/qemu/ar7.git?a=blob;f=hw/eepro100.c
> 
> It has some really important improvements. I want to get it ready
> for inclusion in the official QEMU by the end of August.

I'll see if I can find some time, but after a quick look I don't think
it has any changes relevant in this case, and the patches even apply
cleanly.
I also remembered another strangeness: MDI reads never generate an
interrupt (the corresponding flag is ignored), was that code (MID
interrupts) ever tested with some real-world driver (I expect there
is no driver using that feature?).

Re: [Qemu-devel] [PATCH] Intel 8255x/eepro100 compatibility patches

[Reimar Döffinger]

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

On Sun, Aug 09, 2009 at 11:14:33PM +0200, Reimar Döffinger wrote:
> Attached is a series of patches that makes things work with at least
> some version of that (sorry, I only tried some binary I found on the
> net, didn't compile from source).
> In addition, I also used the documentation from here:
> http://www.intel.com/design/network/manuals/8255X_OpenSDM.htm

Another change, currently if the hardware runs out of receive buffers qemu
crashes with an assert.
Simply setting the RU state to "no resources" seems to work (though it
is hard to provoke the situation and thus hard to test well), at least
it can't be any worse than crashing due to an assert (IMO)...

[-- Attachment #2: 0001-Set-the-RU-state-to-ru_no_resources-instead-of-asser.patch --]
[-- Type: text/plain, Size: 1114 bytes --]

>From eb3dcfa9d65df83c207a74970953cad95d2da1ad Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Reimar=20D=C3=B6ffinger?= <reimar@hokum.(none)>
Date: Tue, 11 Aug 2009 17:19:01 +0200
Subject: [PATCH] Set the RU state to ru_no_resources instead of asserting when we used up
 the last receive buffer.
 This should not usually happen with good drivers, but it can happen with
 the OS X drivers at least.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
---
 hw/eepro100.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index d2c18cc..8b343c1 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1558,7 +1558,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
     s->ru_offset = le32_to_cpu(rx.link);
     if (rfd_command & 0x8000) {
         /* EL bit is set, so this was the last frame. */
-        assert(0);
+        set_ru_state(s, ru_no_resources);
     }
     if (rfd_command & 0x4000) {
         /* S bit is set. */
-- 
1.6.4

Re: [Qemu-devel] [PATCH] Intel 8255x/eepro100 compatibility patches

[Anthony Liguori]

Reimar Döffinger wrote:
> On Sun, Aug 09, 2009 at 11:14:33PM +0200, Reimar Döffinger wrote:
>   
>> Attached is a series of patches that makes things work with at least
>> some version of that (sorry, I only tried some binary I found on the
>> net, didn't compile from source).
>> In addition, I also used the documentation from here:
>> http://www.intel.com/design/network/manuals/8255X_OpenSDM.htm
>>     
>
> Another change, currently if the hardware runs out of receive buffers qemu
> crashes with an assert.
> Simply setting the RU state to "no resources" seems to work (though it
> is hard to provoke the situation and thus hard to test well), at least
> it can't be any worse than crashing due to an assert (IMO)...
>   
Please send this as a series of patches with each patch in an email.

Regards,

Anthony Liguori

[Qemu-devel] Intel 8255x/eepro100 compatibility patches

[Reimar Döffinger]

Hello,

On Tue, Aug 11, 2009 at 03:26:25PM -0500, Anthony Liguori wrote:
> Please send this as a series of patches with each patch in an email.

Ok, resending as a series of patches, hopefully this is the proper format now.
Repeating myself a bit from the original mail:
I have been playing around a bit with the OS X/darwin network drivers for these
cards and noticed that they seem to differ quite a bit from the Linux ones.
If you're interested, the source of the core part is here:
http://www.opensource.apple.com/source/AppleIntel8255x/AppleIntel8255x-19/i82557Private.cpp
Attached is a series of patches that makes things work with at least
some version of that (sorry, I only tried some binary I found on the
net, didn't compile from source).
In addition, I also used the documentation from here:
http://www.intel.com/design/network/manuals/8255X_OpenSDM.htm

Hope this is interesting to someone and maybe we can even get these
merged...

Thanks,
Reimar Döffinger

[Qemu-devel] [PATCH 1/5] Setting the MDI SCBAck flag when interrupts for MDI are disabled is wrong, even if it does not seem to cause any real issue with known drivers.

[Reimar Döffinger]

This makes the emulation not set the SBAck flag for MDI interrupts when
interrupts are actually disabled for MDI.
I think (have not tested yet) that this is not necessary to fix
anything, but according to the documentation the current code is wrong.

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
---
 hw/eepro100.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index ec31a6a..bf5d920 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1043,9 +1043,7 @@ static void eepro100_write_mdi(EEPRO100State * s, uint32_t val)
             }
             data = s->mdimem[reg];
         }
-        /* Emulation takes no time to finish MDI transaction.
-         * Set MDI bit in SCB status register. */
-        s->mem[SCBAck] |= 0x08;
+        /* Emulation takes no time to finish MDI transaction. */
         val |= BIT(28);
         if (raiseint) {
             eepro100_mdi_interrupt(s);
-- 
1.6.4

[Qemu-devel] [PATCH 2/5] Hack to make sure that drivers like AppleIntel8255x will not meddle with the RU/CU state when the ACK the interrupt with a 16 bit write.

[Reimar Döffinger]

This patch ensures that a driver will not accidentally/incorrectly change the
RU/CU state with a write.
This is incomplete and a bit ugly, but good enough for these drivers.
The reason this is an issue is that the drivers ACK interrupts with a 16 bit
write to the status word, with the lower bits having a value of 0.

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
---
 hw/eepro100.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index bf5d920..f619d36 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1249,7 +1249,11 @@ static void eepro100_write1(EEPRO100State * s, uint32_t addr, uint8_t val)
 static void eepro100_write2(EEPRO100State * s, uint32_t addr, uint16_t val)
 {
     if (addr <= sizeof(s->mem) - sizeof(val)) {
+        ru_state_t rtmp = get_ru_state(s);
+        cu_state_t ctmp = get_cu_state(s);
         memcpy(&s->mem[addr], &val, sizeof(val));
+        set_cu_state(s, ctmp);
+        set_ru_state(s, rtmp);
     }
 
     logout("addr=%s val=0x%04x\n", regname(addr), val);
-- 
1.6.4

[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]

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;
+
 typedef struct {
     uint32_t tx_good_frames, tx_max_collisions, tx_late_collisions,
         tx_underruns, tx_lost_crs, tx_deferred, tx_single_collisions,
@@ -218,6 +226,7 @@ typedef struct {
     /* (ru_base + ru_offset) address the RFD in the Receive Frame Area. */
     uint32_t ru_base;           /* RU base address */
     uint32_t ru_offset;         /* RU address offset */
+    uint32_t rbd_addr;
     uint32_t statsaddr;         /* pointer to eepro100_stats_t */
     eepro100_stats_t statistics;        /* statistical counters */
 #if 0
@@ -843,6 +852,7 @@ static void eepro100_ru_command(EEPRO100State * s, uint8_t val)
         }
         set_ru_state(s, ru_ready);
         s->ru_offset = s->pointer;
+        s->rbd_addr = 0;
         logout("val=0x%02x (rx start)\n", val);
         break;
     case RX_RESUME:
@@ -1512,7 +1522,19 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
     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);
-    uint16_t rfd_size = le16_to_cpu(rx.size);
+    uint32_t rfd_size = le16_to_cpu(rx.size);
+    uint32_t dst_addr = s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, packet);
+    if (rfd_command & 8) {
+        // argh! Flexible mode. Intel docs say it is not support 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);
+    }
     assert(size <= rfd_size);
     if (size < 64) {
         rfd_status |= 0x0080;
@@ -1528,8 +1550,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
     assert(!(s->configuration[18] & 4));
     /* TODO: check stripping enable bit. */
     //~ assert(!(s->configuration[17] & 1));
-    cpu_physical_memory_write(s->ru_base + s->ru_offset +
-                              offsetof(eepro100_rx_t, packet), buf, 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);
-- 
1.6.4

[Qemu-devel] [PATCH 4/5] Short frames do not exist, so remove code to handle them. Also expand packets that are smaller than the shor frame limit, otherwise the OS X network stack seems to discard them.

[Reimar Döffinger]

This patch expands received data shorter than 60 bytes so no
short-frame detection is incorrectly triggered, and of course also
throws away all short-frame detection code since it makes no sense.
It was also wrong since size is without CRC and thus the short frame
limit is 60, not 64. And related to that (but without a patch), I think
that the
>    } else if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] & 8)) {
check is wrong, too, and the + 4 should not be there...
Back to the patch, e.g. the rtl8139 driver also expands those frames
(and I just copied the code for that).
This too _definitely_ is necessary to support these AppleIntel8255x drivers.

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
---
 hw/eepro100.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 5620bc7..d2c18cc 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1452,6 +1452,8 @@ static int nic_can_receive(VLANClientState *vc)
     //~ return !eepro100_buffer_full(s);
 }
 
+#define MIN_BUF_SIZE 60
+
 static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size)
 {
     /* TODO:
@@ -1459,6 +1461,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
      * - Interesting packets should set bit 29 in power management driver register.
      */
     EEPRO100State *s = vc->opaque;
+    uint8_t buf1[MIN_BUF_SIZE];
     uint16_t rfd_status = 0xa000;
     static const uint8_t broadcast_macaddr[6] =
         { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
@@ -1466,16 +1469,18 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
     /* TODO: check multiple IA bit. */
     assert(!(s->configuration[20] & BIT(6)));
 
+    /* Short frames can not happen on virtual hardware, so expand them. */
+    if (size < MIN_BUF_SIZE) {
+        memcpy(buf1, buf, size);
+        memset(buf1 + size, 0, MIN_BUF_SIZE - size);
+        buf = buf1;
+        size = MIN_BUF_SIZE;
+    }
+
     if (s->configuration[8] & 0x80) {
         /* CSMA is disabled. */
         logout("%p received while CSMA is disabled\n", s);
         return -1;
-    } else if (size < 64 && (s->configuration[7] & 1)) {
-        /* Short frame and configuration byte 7/0 (discard short receive) set:
-         * Short frame is discarded */
-        logout("%p received short frame (%d byte)\n", s, size);
-        s->statistics.rx_short_frame_errors++;
-        //~ return -1;
     } else if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] & 8)) {
         /* Long frame and configuration byte 18/3 (long receive ok) not set:
          * Long frames are discarded. */
@@ -1536,9 +1541,6 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
         s->rbd_addr = le32_to_cpu(rbd.link);
     }
     assert(size <= rfd_size);
-    if (size < 64) {
-        rfd_status |= 0x0080;
-    }
     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),
-- 
1.6.4

[Qemu-devel] [PATCH 5/5] Set the RU state to ru_no_resources instead of asserting when we used up the last receive buffer. This should not usually happen with good drivers, but it can happen with the OS X drivers at least.

[Reimar Döffinger]

Currently if the hardware runs out of receive buffers qemu
crashes with an assert.
Simply setting the RU state to "no resources" seems to work (though it
is hard to provoke the situation and thus hard to test well), at least
it can't be any worse than crashing due to an assert (IMO)...

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
---
 hw/eepro100.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index d2c18cc..8b343c1 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1558,7 +1558,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
     s->ru_offset = le32_to_cpu(rx.link);
     if (rfd_command & 0x8000) {
         /* EL bit is set, so this was the last frame. */
-        assert(0);
+        set_ru_state(s, ru_no_resources);
     }
     if (rfd_command & 0x4000) {
         /* S bit is set. */
-- 
1.6.4

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.

[malc]

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.

> +
>  typedef struct {
>      uint32_t tx_good_frames, tx_max_collisions, tx_late_collisions,
>          tx_underruns, tx_lost_crs, tx_deferred, tx_single_collisions,
> @@ -218,6 +226,7 @@ typedef struct {
>      /* (ru_base + ru_offset) address the RFD in the Receive Frame Area. */
>      uint32_t ru_base;           /* RU base address */
>      uint32_t ru_offset;         /* RU address offset */
> +    uint32_t rbd_addr;
>      uint32_t statsaddr;         /* pointer to eepro100_stats_t */
>      eepro100_stats_t statistics;        /* statistical counters */
>  #if 0
> @@ -843,6 +852,7 @@ static void eepro100_ru_command(EEPRO100State * s, uint8_t val)
>          }
>          set_ru_state(s, ru_ready);
>          s->ru_offset = s->pointer;
> +        s->rbd_addr = 0;
>          logout("val=0x%02x (rx start)\n", val);
>          break;
>      case RX_RESUME:
> @@ -1512,7 +1522,19 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
>      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);
> -    uint16_t rfd_size = le16_to_cpu(rx.size);
> +    uint32_t rfd_size = le16_to_cpu(rx.size);
> +    uint32_t dst_addr = s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, packet);
> +    if (rfd_command & 8) {
> +        // argh! Flexible mode. Intel docs say it is not support but the Mac OS driver uses it anyway.
                                                            support_ed_ perhaps?

> +        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);
> +    }
>      assert(size <= rfd_size);
>      if (size < 64) {
>          rfd_status |= 0x0080;
> @@ -1528,8 +1550,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
>      assert(!(s->configuration[18] & 4));
>      /* TODO: check stripping enable bit. */
>      //~ assert(!(s->configuration[17] & 1));
> -    cpu_physical_memory_write(s->ru_base + s->ru_offset +
> -                              offsetof(eepro100_rx_t, packet), buf, 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);
> 

-- 
mailto:av1474@comtv.ru

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]

[-- 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);

Re: [Qemu-devel] [PATCH 6/5] Implement the trivial diagnose CU and RU abort commands. These are necessary to make the device work with OpenSolaris 0609 (111b).

[Reimar Döffinger]

With these changes in addition, OpenSolaris boots and works with the
eepro100 network device (without it, qemu crashes due to asserts).

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
---
 hw/eepro100.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 2099459..7c951c0 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -771,6 +771,10 @@ static void eepro100_cu_command(EEPRO100State * s,
uint8_t val)
             /* Starting with offset 8, the command contains
              * 64 dwords microcode which we just ignore here. */
             break;
+        case CmdDiagnose:
+            logout("diagnose\n");
+            status = 0; // make sure error flag is not set
+            break;
         default:
             missing("undefined command");
         }
@@ -864,6 +868,9 @@ static void eepro100_ru_command(EEPRO100State * s,
uint8_t val)
         }
         set_ru_state(s, ru_ready);
         break;
+    case RX_ABORT:
+        set_ru_state(s, ru_idle);
+        break;
     case RX_ADDR_LOAD:
         /* Load RU base. */
         logout("val=0x%02x (RU base address)\n", val);
-- 
1.6.4

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.

[malc]

On Wed, 12 Aug 2009, Reimar D?ffinger wrote:

> On Wed, Aug 12, 2009 at 03:04:17AM +0400, malc wrote:
> > On Tue, 11 Aug 2009, Reimar D?ffinger wrote:
> > 

[..snip..]

> 
> 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.

I sort of thought that would be the case.

> 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).

Sure.

> 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.

Well, Anthony changed the list settings to CC everyone, not CCing someone
is actually more work (couple of keystrokes) than doing it, so all your
Cc credits do belong to him.

-- 
mailto:av1474@comtv.ru

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.

[Anthony Liguori]

malc wrote:
>> P.S.: And I'd appreciate it if you wouldn't CC me, I am subscribed to the
>> list. Thanks.
>>     
>
> Well, Anthony changed the list settings to CC everyone, not CCing someone
> is actually more work (couple of keystrokes) than doing it, so all your
> Cc credits do belong to him.
>   

I'm happy to accept it too :-)

Regards,

Anthony Liguori

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]

[-- 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);
 

Re: [Qemu-devel] Intel 8255x/eepro100 compatibility patches

[Reimar Döffinger]

On Tue, Aug 11, 2009 at 11:13:51PM +0200, Reimar Döffinger wrote:
> On Tue, Aug 11, 2009 at 03:26:25PM -0500, Anthony Liguori wrote:
> > Please send this as a series of patches with each patch in an email.
> 
> Ok, resending as a series of patches, hopefully this is the proper format now.
> Repeating myself a bit from the original mail:
> I have been playing around a bit with the OS X/darwin network drivers for these
> cards and noticed that they seem to differ quite a bit from the Linux ones.
> If you're interested, the source of the core part is here:
> http://www.opensource.apple.com/source/AppleIntel8255x/AppleIntel8255x-19/i82557Private.cpp
> Attached is a series of patches that makes things work with at least
> some version of that (sorry, I only tried some binary I found on the
> net, didn't compile from source).
> In addition, I also used the documentation from here:
> http://www.intel.com/design/network/manuals/8255X_OpenSDM.htm
> 
> Hope this is interesting to someone and maybe we can even get these
> merged...

I know it hasn't been that long, but still: ping?
Btw. I have tested my patched version with OSX (10.5), OpenSolaris (0906), Ubuntu (9.10
alpha) and FreeBSD 7.2 (admittedly only x86/x86_64) and found no issues.

Re: [Qemu-devel] [PATCH] Intel 8255x/eepro100 compatibility patches

[Reimar Döffinger]

On Mon, Aug 10, 2009 at 06:36:58AM +0200, Stefan Weil wrote:
> this is interesting to me. Maybe you want to try a newer version of
> eepro100.c. The latest version is in http://repo.or.cz/w/qemu/ar7.git,
> http://repo.or.cz/w/qemu/ar7.git?a=blob;f=hw/eepro100.c
> 
> It has some really important improvements. I want to get it ready
> for inclusion in the official QEMU by the end of August.

Any hints on what those are? The only ones not in upstream seem mostly
cosmetic to me (not that they aren't a good idea), and changing the
VM save format, which I think should better wait for further changes,
e.g. most of the statistics stuff should not be saved because it is not
and for what I can tell stuff like rx_crc_errors never will be used (not
to mention the "complete" thing which makes no sense at all to store).
I'd suggest a patch, but my local tree is a bit too much of a mess with
all those pending patches that I am too lazy.