| Message ID | 1614333786-74258-2-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: Pad short frames to minimum size (60 bytes) | expand |
On 2/26/21 11:03 AM, Bin Meng wrote: > From: Bin Meng <bin.meng@windriver.com> > > The minimum Ethernet frame length is 60 bytes, and we should pad > frames whose length is smaller to the minimum size. > > This commit fixes the issue as seen with various ethernet models, > that ARP requests get dropped, preventing the guest from becoming > visible on the network. Is it also used in commit 18995b9808d ("Send a RARP packet after migration.")? > The following 2 commits that attempted to workaround this issue > in e1000 and vmxenet3 before, should be reverted. > > commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)") > commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)") > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > net/net.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/net/net.c b/net/net.c > index b038370..34004da 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -638,6 +638,7 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender, > NetPacketSent *sent_cb) > { > NetQueue *queue; > + uint8_t min_buf[60]; Can you add a definition instead of a magic value? Maybe ETH_FRAME_MIN_LEN in "net/eth.h"? > int ret; > > #ifdef DEBUG_NET > @@ -649,6 +650,14 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender, > return size; > } > > + /* 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); > + } > + > /* Let filters handle the packet first */ > ret = filter_receive(sender, NET_FILTER_DIRECTION_TX, > sender, flags, buf, size, sent_cb); >
On 26/02/2021 10:29, Philippe Mathieu-Daudé wrote: > On 2/26/21 11:03 AM, Bin Meng wrote: >> From: Bin Meng <bin.meng@windriver.com> >> >> The minimum Ethernet frame length is 60 bytes, and we should pad >> frames whose length is smaller to the minimum size. >> >> This commit fixes the issue as seen with various ethernet models, >> that ARP requests get dropped, preventing the guest from becoming >> visible on the network. > > Is it also used in commit 18995b9808d ("Send a RARP packet after > migration.")? > >> The following 2 commits that attempted to workaround this issue >> in e1000 and vmxenet3 before, should be reverted. >> >> commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)") >> commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)") >> >> Signed-off-by: Bin Meng <bin.meng@windriver.com> >> --- >> >> net/net.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/net/net.c b/net/net.c >> index b038370..34004da 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -638,6 +638,7 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender, >> NetPacketSent *sent_cb) >> { >> NetQueue *queue; >> + uint8_t min_buf[60]; > > Can you add a definition instead of a magic value? > Maybe ETH_FRAME_MIN_LEN in "net/eth.h"? > >> int ret; >> >> #ifdef DEBUG_NET >> @@ -649,6 +650,14 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender, >> return size; >> } >> >> + /* 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); >> + } >> + >> /* Let filters handle the packet first */ >> ret = filter_receive(sender, NET_FILTER_DIRECTION_TX, >> sender, flags, buf, size, sent_cb); >> Looks like this pattern is also used in a few other cards. From the SPARC/PPC world: https://gitlab.com/qemu-project/qemu/-/blob/master/hw/net/pcnet.c#L1003 https://gitlab.com/qemu-project/qemu/-/blob/master/hw/net/sungem.c#L587 https://gitlab.com/qemu-project/qemu/-/blob/master/hw/net/sunhme.c#L778 I can easily review/test patches that remove the same code from the cards. ATB, Mark.
On Fri, 26 Feb 2021 at 10:03, Bin Meng <bmeng.cn@gmail.com> wrote: > > From: Bin Meng <bin.meng@windriver.com> > > The minimum Ethernet frame length is 60 bytes, and we should pad > frames whose length is smaller to the minimum size. > > This commit fixes the issue as seen with various ethernet models, > that ARP requests get dropped, preventing the guest from becoming > visible on the network. > > The following 2 commits that attempted to workaround this issue > in e1000 and vmxenet3 before, should be reverted. > > commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)") > commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)") > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- Is it better to do this here, or in the places which create network packets? Doing it centrally has the advantage of being just one place to change which then means senders and receivers don't need to think about it. On the other hand it means we don't have any equivalent of really actually sending a short frame and having the modelled ethernet device implement the handling of the short frame. thanks -- PMM
On Sat, Feb 27, 2021 at 2:57 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 26 Feb 2021 at 10:03, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > From: Bin Meng <bin.meng@windriver.com> > > > > The minimum Ethernet frame length is 60 bytes, and we should pad > > frames whose length is smaller to the minimum size. > > > > This commit fixes the issue as seen with various ethernet models, > > that ARP requests get dropped, preventing the guest from becoming > > visible on the network. > > > > The following 2 commits that attempted to workaround this issue > > in e1000 and vmxenet3 before, should be reverted. > > > > commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)") > > commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)") > > > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > > --- > > Is it better to do this here, or in the places which create > network packets? After looking at the eTSEC manual further, I think we should put these codes in the send path. > Doing it centrally has the advantage of > being just one place to change which then means senders > and receivers don't need to think about it. On the other > hand it means we don't have any equivalent of really actually > sending a short frame and having the modelled ethernet device > implement the handling of the short frame. Maybe the best choice would be each ethernet model duplicates the codes in their send path? I have not checked e1000 and vmxnet3 manual, but it seems workaround this issue in the receive path is wrong. Regards, Bin
diff --git a/net/net.c b/net/net.c index b038370..34004da 100644 --- a/net/net.c +++ b/net/net.c @@ -638,6 +638,7 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender, NetPacketSent *sent_cb) { NetQueue *queue; + uint8_t min_buf[60]; int ret; #ifdef DEBUG_NET @@ -649,6 +650,14 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender, return size; } + /* 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); + } + /* Let filters handle the packet first */ ret = filter_receive(sender, NET_FILTER_DIRECTION_TX, sender, flags, buf, size, sent_cb);