[Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimum

[Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimum

[Fabien Chouteau]

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 slirp/slirp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 1593be1..be30501 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;
@@ -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));
     }
 }
 
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimum

[Stefan Hajnoczi]

On Mon, Jun 27, 2011 at 1:41 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
>
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
>  slirp/slirp.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Any particular bug that this fixes?

There have been 64 byte minimum padding patches to several emulated
NICs.  There has also been discussion about where the best place to do
this is.  Why is this patch necessary?

Stefan

Re: [Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimum

[Fabien Chouteau]

On 27/06/2011 15:50, Stefan Hajnoczi wrote:
> On Mon, Jun 27, 2011 at 1:41 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
>>
>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>> ---
>>  slirp/slirp.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> Any particular bug that this fixes?
> 
> There have been 64 byte minimum padding patches to several emulated
> NICs.  There has also been discussion about where the best place to do
> this is.  Why is this patch necessary?
> 

This patch is necessary because some NICs are configured to drop short frames,
therefore the OS will not receive some of the packets generated by Qemu.

There's a first patch to fix this issue:
http://git.savannah.gnu.org/cgit/qemu.git/commit/?id=dbf3c4b4baceb91eb64d09f787cbe92d65188813

My patch fixes two other sources of short frames.

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimum

[Stefan Hajnoczi]

On Mon, Jun 27, 2011 at 3:23 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
> On 27/06/2011 15:50, Stefan Hajnoczi wrote:
>> On Mon, Jun 27, 2011 at 1:41 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
>>>
>>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>>> ---
>>>  slirp/slirp.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> Any particular bug that this fixes?
>>
>> There have been 64 byte minimum padding patches to several emulated
>> NICs.  There has also been discussion about where the best place to do
>> this is.  Why is this patch necessary?
>>
>
> This patch is necessary because some NICs are configured to drop short frames,
> therefore the OS will not receive some of the packets generated by Qemu.
>
> There's a first patch to fix this issue:
> http://git.savannah.gnu.org/cgit/qemu.git/commit/?id=dbf3c4b4baceb91eb64d09f787cbe92d65188813
>
> My patch fixes two other sources of short frames.

Thanks for the explanation.  I stepped back from the discussion on
where the right place to fix this is last time around.  Now I'm
wondering why do anything in slirp when the other sources (tap, ...)
aren't padding to 64 bytes?

Stefan

Re: [Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimum

[Fabien Chouteau]

On 27/06/2011 17:39, Stefan Hajnoczi wrote:
> On Mon, Jun 27, 2011 at 3:23 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
>> On 27/06/2011 15:50, Stefan Hajnoczi wrote:
>>> On Mon, Jun 27, 2011 at 1:41 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
>>>>
>>>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>>>> ---
>>>>  slirp/slirp.c |    4 ++--
>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> Any particular bug that this fixes?
>>>
>>> There have been 64 byte minimum padding patches to several emulated
>>> NICs.  There has also been discussion about where the best place to do
>>> this is.  Why is this patch necessary?
>>>
>>
>> This patch is necessary because some NICs are configured to drop short frames,
>> therefore the OS will not receive some of the packets generated by Qemu.
>>
>> There's a first patch to fix this issue:
>> http://git.savannah.gnu.org/cgit/qemu.git/commit/?id=dbf3c4b4baceb91eb64d09f787cbe92d65188813
>>
>> My patch fixes two other sources of short frames.
> 
> Thanks for the explanation.  I stepped back from the discussion on
> where the right place to fix this is last time around.  Now I'm
> wondering why do anything in slirp when the other sources (tap, ...)
> aren't padding to 64 bytes?

I think that packets generated by Qemu must follow RFC. For other sources, qemu
should keep original size when possible and put padding otherwise.

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimum

[Stefan Hajnoczi]

On Mon, Jun 27, 2011 at 5:12 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
> On 27/06/2011 17:39, Stefan Hajnoczi wrote:
>> On Mon, Jun 27, 2011 at 3:23 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
>>> On 27/06/2011 15:50, Stefan Hajnoczi wrote:
>>>> On Mon, Jun 27, 2011 at 1:41 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
>>>>>
>>>>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>>>>> ---
>>>>>  slirp/slirp.c |    4 ++--
>>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> Any particular bug that this fixes?
>>>>
>>>> There have been 64 byte minimum padding patches to several emulated
>>>> NICs.  There has also been discussion about where the best place to do
>>>> this is.  Why is this patch necessary?
>>>>
>>>
>>> This patch is necessary because some NICs are configured to drop short frames,
>>> therefore the OS will not receive some of the packets generated by Qemu.
>>>
>>> There's a first patch to fix this issue:
>>> http://git.savannah.gnu.org/cgit/qemu.git/commit/?id=dbf3c4b4baceb91eb64d09f787cbe92d65188813
>>>
>>> My patch fixes two other sources of short frames.
>>
>> Thanks for the explanation.  I stepped back from the discussion on
>> where the right place to fix this is last time around.  Now I'm
>> wondering why do anything in slirp when the other sources (tap, ...)
>> aren't padding to 64 bytes?
>
> I think that packets generated by Qemu must follow RFC. For other sources, qemu
> should keep original size when possible and put padding otherwise.

slirp interfaces to net.h which does not emulate Ethernet.  For
example, it doesn't carry the Frame Check Sequence (FCS) as per the
Ethernet standard.  NICs that want to report FCS to the guest
calculate it themselves, just like they do with 64-byte padding.  So I
find it weird to say we should honor Ethernet when the transport we
are using is not Ethernet.

This patch doesn't hurt but we'd be just as well off without it.

Did you do this to fix a bug?  If so, then something else in QEMU
needs to be fixed, not slirp.

Stefan

Re: [Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimuma

[Fabien Chouteau]

On 28/06/2011 10:34, Stefan Hajnoczi wrote:
> On Mon, Jun 27, 2011 at 5:12 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
>> On 27/06/2011 17:39, Stefan Hajnoczi wrote:
>>> On Mon, Jun 27, 2011 at 3:23 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
>>>> On 27/06/2011 15:50, Stefan Hajnoczi wrote:
>>>>> On Mon, Jun 27, 2011 at 1:41 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
>>>>>>
>>>>>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>>>>>> ---
>>>>>>  slirp/slirp.c |    4 ++--
>>>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> Any particular bug that this fixes?
>>>>>
>>>>> There have been 64 byte minimum padding patches to several emulated
>>>>> NICs.  There has also been discussion about where the best place to do
>>>>> this is.  Why is this patch necessary?
>>>>>
>>>>
>>>> This patch is necessary because some NICs are configured to drop short frames,
>>>> therefore the OS will not receive some of the packets generated by Qemu.
>>>>
>>>> There's a first patch to fix this issue:
>>>> http://git.savannah.gnu.org/cgit/qemu.git/commit/?id=dbf3c4b4baceb91eb64d09f787cbe92d65188813
>>>>
>>>> My patch fixes two other sources of short frames.
>>>
>>> Thanks for the explanation.  I stepped back from the discussion on
>>> where the right place to fix this is last time around.  Now I'm
>>> wondering why do anything in slirp when the other sources (tap, ...)
>>> aren't padding to 64 bytes?
>>
>> I think that packets generated by Qemu must follow RFC. For other sources, qemu
>> should keep original size when possible and put padding otherwise.
>
> slirp interfaces to net.h which does not emulate Ethernet.  For
> example, it doesn't carry the Frame Check Sequence (FCS) as per the
> Ethernet standard. NICs that want to report FCS to the guest
> calculate it themselves, just like they do with 64-byte padding.  So I
> find it weird to say we should honor Ethernet when the transport we
> are using is not Ethernet.

In this case it's always Ethernet frames, look at the two patched lines,
there's an Ethernet header:

uint8_t arp_req[max(ETH_HLEN + sizeof(struct arphdr), 64)];

slirp_output(slirp->opaque, buf, max(ip_data_len + ETH_HLEN, 64));

>
> This patch doesn't hurt but we'd be just as well off without it.
>
> Did you do this to fix a bug?  If so, then something else in QEMU
> needs to be fixed, not slirp.

When Qemu generates bad Ethernet frames, I think it's a bug.

And again, this is the extension of a previous patch. If this patch is not
valid then we should revert the first, it's also a question of consistency.

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimuma

[Stefan Hajnoczi]

On Tue, Jun 28, 2011 at 10:08 AM, Fabien Chouteau <chouteau@adacore.com> wrote:
> On 28/06/2011 10:34, Stefan Hajnoczi wrote:
>> This patch doesn't hurt but we'd be just as well off without it.
>>
>> Did you do this to fix a bug?  If so, then something else in QEMU
>> needs to be fixed, not slirp.
>
> When Qemu generates bad Ethernet frames, I think it's a bug.
>
> And again, this is the extension of a previous patch. If this patch is not
> valid then we should revert the first, it's also a question of consistency.

IMO the previous patch is not necessary either.

Since there is no net subsystem maintainer who can help decide which
way to go, I'm going to back off from this issue.  Please go ahead.

Stefan

Re: [Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimuma

[Fabien Chouteau]

On 28/06/2011 15:15, Stefan Hajnoczi wrote:
> On Tue, Jun 28, 2011 at 10:08 AM, Fabien Chouteau <chouteau@adacore.com> wrote:
>> On 28/06/2011 10:34, Stefan Hajnoczi wrote:
>>> This patch doesn't hurt but we'd be just as well off without it.
>>>
>>> Did you do this to fix a bug?  If so, then something else in QEMU
>>> needs to be fixed, not slirp.
>>
>> When Qemu generates bad Ethernet frames, I think it's a bug.
>>
>> And again, this is the extension of a previous patch. If this patch is not
>> valid then we should revert the first, it's also a question of consistency.
> 
> IMO the previous patch is not necessary either.
> 
> Since there is no net subsystem maintainer who can help decide which
> way to go, I'm going to back off from this issue.  Please go ahead.
> 

OK thanks.

Any other comments?

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimuma

[Fabien Chouteau]

On 30/06/2011 10:38, Fabien Chouteau wrote:
> On 28/06/2011 15:15, Stefan Hajnoczi wrote:
>> On Tue, Jun 28, 2011 at 10:08 AM, Fabien Chouteau <chouteau@adacore.com> wrote:
>>> On 28/06/2011 10:34, Stefan Hajnoczi wrote:
>>>> This patch doesn't hurt but we'd be just as well off without it.
>>>>
>>>> Did you do this to fix a bug?  If so, then something else in QEMU
>>>> needs to be fixed, not slirp.
>>>
>>> When Qemu generates bad Ethernet frames, I think it's a bug.
>>>
>>> And again, this is the extension of a previous patch. If this patch is not
>>> valid then we should revert the first, it's also a question of consistency.
>>
>> IMO the previous patch is not necessary either.
>>
>> Since there is no net subsystem maintainer who can help decide which
>> way to go, I'm going to back off from this issue.  Please go ahead.
>>
> 
> OK thanks.
> 
> Any other comments?
> 

This patch got no further comments, can it be applied please?

thanks,

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH] Make SLIRP Ethernet packets size to 64 bytes minimum

[Edgar E. Iglesias]

On Tue, Jun 28, 2011 at 09:34:16AM +0100, Stefan Hajnoczi wrote:
> On Mon, Jun 27, 2011 at 5:12 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
> > On 27/06/2011 17:39, Stefan Hajnoczi wrote:
> >> On Mon, Jun 27, 2011 at 3:23 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
> >>> On 27/06/2011 15:50, Stefan Hajnoczi wrote:
> >>>> On Mon, Jun 27, 2011 at 1:41 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
> >>>>>
> >>>>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> >>>>> ---
> >>>>>  slirp/slirp.c |    4 ++--
> >>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> Any particular bug that this fixes?
> >>>>
> >>>> There have been 64 byte minimum padding patches to several emulated
> >>>> NICs.  There has also been discussion about where the best place to do
> >>>> this is.  Why is this patch necessary?
> >>>>
> >>>
> >>> This patch is necessary because some NICs are configured to drop short frames,
> >>> therefore the OS will not receive some of the packets generated by Qemu.
> >>>
> >>> There's a first patch to fix this issue:
> >>> http://git.savannah.gnu.org/cgit/qemu.git/commit/?id=dbf3c4b4baceb91eb64d09f787cbe92d65188813
> >>>
> >>> My patch fixes two other sources of short frames.
> >>
> >> Thanks for the explanation.  I stepped back from the discussion on
> >> where the right place to fix this is last time around.  Now I'm
> >> wondering why do anything in slirp when the other sources (tap, ...)
> >> aren't padding to 64 bytes?
> >
> > I think that packets generated by Qemu must follow RFC. For other sources, qemu
> > should keep original size when possible and put padding otherwise.
> 
> slirp interfaces to net.h which does not emulate Ethernet.  For
> example, it doesn't carry the Frame Check Sequence (FCS) as per the
> Ethernet standard.  NICs that want to report FCS to the guest
> calculate it themselves, just like they do with 64-byte padding.  So I
> find it weird to say we should honor Ethernet when the transport we
> are using is not Ethernet.
> 
> This patch doesn't hurt but we'd be just as well off without it.
> 
> Did you do this to fix a bug?  If so, then something else in QEMU
> needs to be fixed, not slirp.

Hi,

IMO SLIRP currently does claim to emulate Ethernet.

Part of the problem is that QEMU's internal net router doesnt really
route but acts more like a bridge between different networks. Different
network wire formats dont get translated, they just pass through.

To be able to emulate different network types, I think translation/routing
will be necessary.

Last time this came up, if I am not misstaken MST suggested that we
add a translation macro or a function call to the netdevice models to be
called when receiving packets that would translate into the desired
wire format. I wasn't very happy about that because I didnt want to
clutter the device models with stuff that the hw equivalence doesnt
do but I dont have any better suggestions and I definitely think a receive
call function to translate is better than the current state so thats
what I would suggest now.

MST, Im CC:ing you, please correct me if I remeber wrong.

In the long run I think every netdevice should register the wire format
is uses so that the core networking code can translate accordingly.
That way we could support something injecting XXX talking to an
Ethernet device. The internal format may very well be ethernet a la tap
format, i.e ethernet without preamble, padding and CRC.

Cheers