[Qemu-devel] [PATCH] hw/eepro100.c: Use extended TBD only where applicable

[Qemu-devel] [PATCH] hw/eepro100.c: Use extended TBD only where applicable

[Naphtali Sprei]

Bug fix for segfault when run as i82551 HW:
Use Extended TBD only when HW supports it (i82558 and up).

Added assertions to guard from such buffer overflow
Introduce the MAX_TCB_BYTE_COUNT macro
Allocate buf big enough as HW needs (MAX_ETH_FRAME_SIZE -> MAX_TCB_BYTE_COUNT)


I don't feel 100% OK with the "s->device >= i82558B" condition
since it relies on the numeric (hex) value of those defines, which currently
is correct, but changes (which I don't forsee now) might break it.


Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
---
 hw/eepro100.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index ec31a6a..907c0a1 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -715,8 +715,8 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
             } else {
                 /* Flexible mode. */
                 uint8_t tbd_count = 0;
-                if (!(s->configuration[6] & BIT(4))) {
-                    /* Extended TCB. */
+                if ((s->device >= i82558B) && !(s->configuration[6] & BIT(4))) {
+                    /* Extended Flexible TCB. */
                     assert(tcb_bytes == 0);
                     for (; tbd_count < 2; tbd_count++) {
                         uint32_t tx_buffer_address = ldl_phys(tbd_address);
@@ -724,7 +724,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
                         uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
                         tbd_address += 8;
                         logout
-                            ("TBD (extended mode): buffer address 0x%08x, size 0x%04x\n",
+                            ("TBD (extended flexible mode): buffer address 0x%08x, size 0x%04x\n",
                              tx_buffer_address, tx_buffer_size);
                         cpu_physical_memory_read(tx_buffer_address, &buf[size],
                                                  tx_buffer_size);
-- 
1.6.3.3

Re: [Qemu-devel] [PATCH] hw/eepro100.c: Use extended TBD only where applicable

[Reimar Döffinger]

On Thu, Aug 13, 2009 at 03:01:20PM +0300, Naphtali Sprei wrote:
> I don't feel 100% OK with the "s->device >= i82558B" condition
> since it relies on the numeric (hex) value of those defines, which currently
> is correct, but changes (which I don't forsee now) might break it.

I think it might be best to add a
switch (device)
to nic_init and set a flag in the device context there.
There is other device-dependent stuff that needs to be added to fix some
issues, e.g. I have just added this code in order to fix the statistics
counters:
@@ -1772,6 +1751,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] [PATCH] hw/eepro100.c: Use extended TBD only where applicable

[Reimar Döffinger]

On Thu, Aug 13, 2009 at 03:01:20PM +0300, Naphtali Sprei wrote:
> Bug fix for segfault when run as i82551 HW:
> Use Extended TBD only when HW supports it (i82558 and up).
> 
> Added assertions to guard from such buffer overflow
> Introduce the MAX_TCB_BYTE_COUNT macro
> Allocate buf big enough as HW needs (MAX_ETH_FRAME_SIZE -> MAX_TCB_BYTE_COUNT)
> 
> 
> I don't feel 100% OK with the "s->device >= i82558B" condition
> since it relies on the numeric (hex) value of those defines, which currently
> is correct, but changes (which I don't forsee now) might break it.

It seems this was applied. Unfortunately this breaks things on FreeBSD.
There seem to be multiple issues.
First, the intel document says the 82551, 82550, 82559 models are all
supersets of the 82558. Or in other words: they all support this
feature.
Only the 82557 does not.
But then even for that the FreeBSD driver will fail.
The reason for that is this line:
    eeprom_contents[0xa] = 0x4000;
the value here must be 0x01000 for all 82557 models it seems.

Re: [Qemu-devel] [PATCH] hw/eepro100.c: Use extended TBD only where applicable

[Reimar Döffinger]

On Sat, Aug 29, 2009 at 08:17:55PM +0200, Reimar Döffinger wrote:
> The reason for that is this line:
>     eeprom_contents[0xa] = 0x4000;
> the value here must be 0x01000 for all 82557 models it seems.

Sorry, thinko.
eeprom_contents[5] must be 0x0100 for all 82557 models.

[Qemu-devel] [PATCH] Fix for commit 3f9cb1c14dc368f41447db5f78d6248c4f100ad4

[Naphtali Sprei]

Here's a patch to fix the issue introduced by me, as Reimar Döffinger pointed out,

Reimar Döffinger wrote:
> On Thu, Aug 13, 2009 at 03:01:20PM +0300, Naphtali Sprei wrote:
>> Bug fix for segfault when run as i82551 HW:
>> Use Extended TBD only when HW supports it (i82558 and up).
>>
>> Added assertions to guard from such buffer overflow
>> Introduce the MAX_TCB_BYTE_COUNT macro
>> Allocate buf big enough as HW needs (MAX_ETH_FRAME_SIZE -> MAX_TCB_BYTE_COUNT)
>>
>>
>> I don't feel 100% OK with the "s->device >= i82558B" condition
>> since it relies on the numeric (hex) value of those defines, which currently
>> is correct, but changes (which I don't forsee now) might break it.
> 
> It seems this was applied. Unfortunately this breaks things on FreeBSD.
> There seem to be multiple issues.
> First, the intel document says the 82551, 82550, 82559 models are all
> supersets of the 82558. Or in other words: they all support this
> feature.
> Only the 82557 does not.
> But then even for that the FreeBSD driver will fail.
> The reason for that is this line:
>     eeprom_contents[0xa] = 0x4000;
> the value here must be 0x01000 for all 82557 models it seems.

 Correct the logic of determining devices that supports
 extended TxCB: only the 82557 do not support it.

Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
---
 hw/eepro100.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index c374931..287b462 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -575,6 +575,11 @@ static uint16_t eepro100_read_command(EEPRO100State * s)
 }
 #endif
 
+static bool device_supports_eTxCB(EEPRO100State * s)
+{
+    return (s->device != i82557B && s->device != i82557C);
+}
+
 /* Commands that can be put in a command list entry. */
 enum commands {
     CmdNOp = 0,
@@ -717,7 +722,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
             } else {
                 /* Flexible mode. */
                 uint8_t tbd_count = 0;
-                if ((s->device >= i82558B) && !(s->configuration[6] & BIT(4))) {
+                if (device_supports_eTxCB(s) && !(s->configuration[6] & BIT(4))) {
                     /* Extended Flexible TCB. */
                     assert(tcb_bytes == 0);
                     for (; tbd_count < 2; tbd_count++) {
-- 
1.6.3.3