[Qemu-devel] Re: [PATCH] slirp: ensure minimum packet size

[Qemu-devel] Re: [PATCH] slirp: ensure minimum packet size

[Hervé Poussineau]

Hi,

qemu-devel-request@nongnu.org a écrit :
> Date: Thu, 10 Feb 2011 15:54:28 -0700
> From: "Bruce Rogers" <brogers@novell.com>
> Subject: [Qemu-devel] [PATCH] slirp: ensure minimum packet size
> To: <qemu-devel@nongnu.org>
> Message-ID: <4D540A3402000048000A9C8C@novprvoes0310.provo.novell.com>
> Content-Type: text/plain; charset=US-ASCII
>
> With recent gpxe eepro100 drivers, short packets are rejected,
> so ensure the minimum ethernet packet size.
>
> Signed-off-by: Bruce Rogers <brogers@novell.com>
> ---
>  slirp/slirp.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 332d83b..b611cf7 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -697,7 +697,7 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
>          return;
>      
>      if (!memcmp(slirp->client_ethaddr, zero_ethaddr, ETH_ALEN)) {
> -        uint8_t arp_req[ETH_HLEN + sizeof(struct arphdr)];
> +        uint8_t arp_req[max(ETH_HLEN + sizeof(struct arphdr), 64)];
>          struct ethhdr *reh = (struct ethhdr *)arp_req;
>          struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN);
>          const struct ip *iph = (const struct ip *)ip_data;
>   
Ack for this part.
Slirp must not generate frames which are not 802.3 compliant, ie less 
than 64 bytes.
A similar fix has already been done at 
http://git.qemu.org/qemu.git/commit/?id=dbf3c4b4baceb91eb64d09f787cbe92d65188813
On a side note, maybe you need to add a memset to clear the unused part 
of the buffer?

> @@ -734,7 +734,7 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
>          memcpy(&eh->h_source[2], &slirp->vhost_addr, 4);
>          eh->h_proto = htons(ETH_P_IP);
>          memcpy(buf + sizeof(struct ethhdr), ip_data, ip_data_len);
> -        slirp_output(slirp->opaque, buf, ip_data_len + ETH_HLEN);
> +        slirp_output(slirp->opaque, buf, max(ip_data_len + ETH_HLEN, 64));
>      }
>  }
>  
>   
Nack for this part.
Here, you're limiting the Ethernet frame to at most 64 bytes. You 
probably meant min(ip_data_len + ETH_HLEN, 64).

Even with min(), I'm not sure if it is slirp responsability to increase 
buffer size to meet Ethernet standards.

Hervé

[Qemu-devel] Re: [PATCH] slirp: ensure minimum packet size

[Bruce Rogers]

 >>> On 2/12/2011 at 12:51 AM, Hervé Poussineau<hpoussin@reactos.org> wrote: 
> Hi,
> 
> qemu-devel-request@nongnu.org a écrit :
>> Date: Thu, 10 Feb 2011 15:54:28 -0700
>> From: "Bruce Rogers" <brogers@novell.com>
>> Subject: [Qemu-devel] [PATCH] slirp: ensure minimum packet size
>> To: <qemu-devel@nongnu.org>
>> Message-ID: <4D540A3402000048000A9C8C@novprvoes0310.provo.novell.com>
>> Content-Type: text/plain; charset=US-ASCII
>>
>> With recent gpxe eepro100 drivers, short packets are rejected,
>> so ensure the minimum ethernet packet size.
>>
>> Signed-off-by: Bruce Rogers <brogers@novell.com>
>> ---
>>  slirp/slirp.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/slirp/slirp.c b/slirp/slirp.c
>> index 332d83b..b611cf7 100644
>> --- a/slirp/slirp.c
>> +++ b/slirp/slirp.c
>> @@ -697,7 +697,7 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int 
> ip_data_len)
>>          return;
>>      
>>      if (!memcmp(slirp->client_ethaddr, zero_ethaddr, ETH_ALEN)) {
>> -        uint8_t arp_req[ETH_HLEN + sizeof(struct arphdr)];
>> +        uint8_t arp_req[max(ETH_HLEN + sizeof(struct arphdr), 64)];
>>          struct ethhdr *reh = (struct ethhdr *)arp_req;
>>          struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN);
>>          const struct ip *iph = (const struct ip *)ip_data;
>>   
> Ack for this part.
> Slirp must not generate frames which are not 802.3 compliant, ie less 
> than 64 bytes.
> A similar fix has already been done at 
> http://git.qemu.org/qemu.git/commit/?id=dbf3c4b4baceb91eb64d09f787cbe92d6518
> 8813
> On a side note, maybe you need to add a memset to clear the unused part 
> of the buffer?
> 
>> @@ -734,7 +734,7 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int 
> ip_data_len)
>>          memcpy(&eh->h_source[2], &slirp->vhost_addr, 4);
>>          eh->h_proto = htons(ETH_P_IP);
>>          memcpy(buf + sizeof(struct ethhdr), ip_data, ip_data_len);
>> -        slirp_output(slirp->opaque, buf, ip_data_len + ETH_HLEN);
>> +        slirp_output(slirp->opaque, buf, max(ip_data_len + ETH_HLEN, 64));
>>      }
>>  }
>>  
>>   
> Nack for this part.
> Here, you're limiting the Ethernet frame to at most 64 bytes. You 
> probably meant min(ip_data_len + ETH_HLEN, 64).
> 
> Even with min(), I'm not sure if it is slirp responsability to increase 
> buffer size to meet Ethernet standards.
> 
> Hervé

Yeah - obviously that should have been min. ;)

Anyways, I think these patches are misguided, along with the one you referenced above.

Instead it appears the right solution to the failures that these changes are trying to solve, is to have the emulated nics add padding to receive data to ensure the minimum frame size that would have been enforced on a physical network.

I have submitted a patch to do that for the eepro100 case which I was interested in. Other more common emulated nics are already working that way, but some remain without that change.

Bruce