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

[Fabien Chouteau <chouteau@adacore.com>]

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