[Qemu-devel] [PATCH] eepro100: pad to ensure minimum packet size

[Qemu-devel] [PATCH] eepro100: pad to ensure minimum packet size

[Bruce Rogers]

Recent gpxe e100pro drivers will drop small packets because the emulated
nic will report an error for small frames. In the qemu model we should
instead have the e100pro pad out the received frames to be the minimum
size and not report this case as an error.

Signed-off-by: Bruce Rogers <brogers@novell.com>
---
 hw/eepro100.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index edf48f6..03b6934 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1645,6 +1645,8 @@ static int nic_can_receive(VLANClientState *nc)
 #endif
 }
 
+#define MIN_BUF_SIZE      60 /* Min. octets in an ethernet frame sans FCS */
+
 static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size)
 {
     /* TODO:
@@ -1653,6 +1655,7 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
      */
     EEPRO100State *s = DO_UPCAST(NICState, nc, nc)->opaque;
     uint16_t rfd_status = 0xa000;
+    uint8_t min_buf[MIN_BUF_SIZE];
     static const uint8_t broadcast_macaddr[6] =
         { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
 
@@ -1660,15 +1663,15 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
         /* CSMA is disabled. */
         logout("%p received while CSMA is disabled\n", s);
         return -1;
-    } else if (size < 64 && (s->configuration[7] & BIT(0))) {
-        /* Short frame and configuration byte 7/0 (discard short receive) set:
-         * Short frame is discarded */
-        logout("%p received short frame (%zu byte)\n", s, size);
-        s->statistics.rx_short_frame_errors++;
-#if 0
-        return -1;
-#endif
-    } else if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] & BIT(3))) {
+    }
+    /* Pad to minimum Ethernet frame length */
+    if (size < sizeof(min_buf)) {
+        memcpy(min_buf, buf, size);
+        memset(&min_buf[size], 0, sizeof(min_buf) - size);
+        buf = min_buf;
+        size = sizeof(min_buf);
+    }
+    if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] & BIT(3))) {
         /* Long frame and configuration byte 18/3 (long receive ok) not set:
          * Long frames are discarded. */
         logout("%p received long frame (%zu byte), ignored\n", s, size);
@@ -1744,7 +1747,7 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
             "(%zu bytes); data truncated\n", rfd_size, size);
         size = rfd_size;
     }
-    if (size < 64) {
+    if (size < MIN_BUF_SIZE) {
         rfd_status |= 0x0080;
     }
     TRACE(OTHER, logout("command 0x%04x, link 0x%08x, addr 0x%08x, size %u\n",
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH] eepro100: pad to ensure minimum packet size

[Stefan Weil]

Am 11.02.2011 20:36, schrieb Bruce Rogers:
> Recent gpxe e100pro drivers will drop small packets because the emulated
> nic will report an error for small frames. In the qemu model we should
> instead have the e100pro pad out the received frames to be the minimum
> size and not report this case as an error.
>
> Signed-off-by: Bruce Rogers <brogers@novell.com>
> ---
> hw/eepro100.c | 23 +++++++++++++----------
> 1 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index edf48f6..03b6934 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -1645,6 +1645,8 @@ static int nic_can_receive(VLANClientState *nc)
> #endif
> }
>
> +#define MIN_BUF_SIZE 60 /* Min. octets in an ethernet frame sans FCS */
> +
> static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, 
> size_t size)
> {
> /* TODO:
> @@ -1653,6 +1655,7 @@ static ssize_t nic_receive(VLANClientState *nc, 
> const uint8_t * buf, size_t size
> */
> EEPRO100State *s = DO_UPCAST(NICState, nc, nc)->opaque;
> uint16_t rfd_status = 0xa000;
> + uint8_t min_buf[MIN_BUF_SIZE];
> static const uint8_t broadcast_macaddr[6] =
> { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>
> @@ -1660,15 +1663,15 @@ static ssize_t nic_receive(VLANClientState 
> *nc, const uint8_t * buf, size_t size
> /* CSMA is disabled. */
> logout("%p received while CSMA is disabled\n", s);
> return -1;
> - } else if (size < 64 && (s->configuration[7] & BIT(0))) {
> - /* Short frame and configuration byte 7/0 (discard short receive) set:
> - * Short frame is discarded */
> - logout("%p received short frame (%zu byte)\n", s, size);
> - s->statistics.rx_short_frame_errors++;
> -#if 0
> - return -1;
> -#endif
> - } else if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] 
> & BIT(3))) {
> + }
> + /* Pad to minimum Ethernet frame length */
> + if (size < sizeof(min_buf)) {
> + memcpy(min_buf, buf, size);
> + memset(&min_buf[size], 0, sizeof(min_buf) - size);
> + buf = min_buf;
> + size = sizeof(min_buf);
> + }
> + if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] & 
> BIT(3))) {
> /* Long frame and configuration byte 18/3 (long receive ok) not set:
> * Long frames are discarded. */
> logout("%p received long frame (%zu byte), ignored\n", s, size);
> @@ -1744,7 +1747,7 @@ static ssize_t nic_receive(VLANClientState *nc, 
> const uint8_t * buf, size_t size
> "(%zu bytes); data truncated\n", rfd_size, size);
> size = rfd_size;
> }
> - if (size < 64) {
> + if (size < MIN_BUF_SIZE) {
> rfd_status |= 0x0080;
> }
> TRACE(OTHER, logout("command 0x%04x, link 0x%08x, addr 0x%08x, size %u\n",


Could you please give more details of the test scenario which fails?
I'd like to reproduce it here and find a better solution.

The configuration bit "discard short frame" exists in real hardware,
so removing the code which emulates this behavior is not the correct
solution - even if it helps in a special case.

No acknowledge for this patch from me.

Regards,
Stefan Weil

Re: [Qemu-devel] [PATCH] eepro100: pad to ensure minimum packet size

[Bruce Rogers]

 >>> On 2/13/2011 at 05:17 AM, Stefan Weil <weil@mail.berlios.de> wrote: 
> Am 11.02.2011 20:36, schrieb Bruce Rogers:
>> Recent gpxe e100pro drivers will drop small packets because the emulated
>> nic will report an error for small frames. In the qemu model we should
>> instead have the e100pro pad out the received frames to be the minimum
>> size and not report this case as an error.
>>
>> Signed-off-by: Bruce Rogers <brogers@novell.com>
>> ---
>> hw/eepro100.c | 23 +++++++++++++----------
>> 1 files changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index edf48f6..03b6934 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -1645,6 +1645,8 @@ static int nic_can_receive(VLANClientState *nc)
>> #endif
>> }
>>
>> +#define MIN_BUF_SIZE 60 /* Min. octets in an ethernet frame sans FCS */
>> +
>> static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, 
>> size_t size)
>> {
>> /* TODO:
>> @@ -1653,6 +1655,7 @@ static ssize_t nic_receive(VLANClientState *nc, 
>> const uint8_t * buf, size_t size
>> */
>> EEPRO100State *s = DO_UPCAST(NICState, nc, nc)->opaque;
>> uint16_t rfd_status = 0xa000;
>> + uint8_t min_buf[MIN_BUF_SIZE];
>> static const uint8_t broadcast_macaddr[6] =
>> { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>>
>> @@ -1660,15 +1663,15 @@ static ssize_t nic_receive(VLANClientState 
>> *nc, const uint8_t * buf, size_t size
>> /* CSMA is disabled. */
>> logout("%p received while CSMA is disabled\n", s);
>> return -1;
>> - } else if (size < 64 && (s->configuration[7] & BIT(0))) {
>> - /* Short frame and configuration byte 7/0 (discard short receive) set:
>> - * Short frame is discarded */
>> - logout("%p received short frame (%zu byte)\n", s, size);
>> - s->statistics.rx_short_frame_errors++;
>> -#if 0
>> - return -1;
>> -#endif
>> - } else if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] 
>> & BIT(3))) {
>> + }
>> + /* Pad to minimum Ethernet frame length */
>> + if (size < sizeof(min_buf)) {
>> + memcpy(min_buf, buf, size);
>> + memset(&min_buf[size], 0, sizeof(min_buf) - size);
>> + buf = min_buf;
>> + size = sizeof(min_buf);
>> + }
>> + if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] & 
>> BIT(3))) {
>> /* Long frame and configuration byte 18/3 (long receive ok) not set:
>> * Long frames are discarded. */
>> logout("%p received long frame (%zu byte), ignored\n", s, size);
>> @@ -1744,7 +1747,7 @@ static ssize_t nic_receive(VLANClientState *nc, 
>> const uint8_t * buf, size_t size
>> "(%zu bytes); data truncated\n", rfd_size, size);
>> size = rfd_size;
>> }
>> - if (size < 64) {
>> + if (size < MIN_BUF_SIZE) {
>> rfd_status |= 0x0080;
>> }
>> TRACE(OTHER, logout("command 0x%04x, link 0x%08x, addr 0x%08x, size %u\n",
> 
> 
> Could you please give more details of the test scenario which fails?
> I'd like to reproduce it here and find a better solution.
> 
> The configuration bit "discard short frame" exists in real hardware,
> so removing the code which emulates this behavior is not the correct
> solution - even if it helps in a special case.
> 
> No acknowledge for this patch from me.
> 
> Regards,
> Stefan Weil

What I'm doing is using the current v1.0.1 gpxe based epro100 rom to pxe boot using the user mode networking stack. For example, the following (in tree) command line fails to pxe boot successfully, when the above mentioned pxe rom is used:

x86_64-softmmu/qemu-system-x86_64 -boot n -bootp http://boot.kernel.org/bko/pxelinux.0 -net user -net nic,model=i82559er

(It doesn't have to be using http protocol however - using a local tftp path to pxelinux.0 fails for me as well.)

I found the gpxe git commit which causes the failure to be the following:

commit cdcb4165bdabf3adbc4b2e5937c279614d769aa9
Author: Thomas Miletich <thomas.miletich@gmail.com>
Date:   Tue Oct 6 23:57:49 2009 +0200

    [eepro100] Convert to native gPXE API
    
    This version is Based on Michael Decker's GSoC 2008 code.
    A number cleanups and fixes were applied.


This commit causes packets which are short, such as come from the slirp code, to be rejected.

Bruce