| Message ID | 1309178511-20027-1-git-send-email-chouteau@adacore.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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.
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
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.
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
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?
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,
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
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)); } }
Signed-off-by: Fabien Chouteau <chouteau@adacore.com> --- slirp/slirp.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)