| Message ID | 20240106223546.44460-1-wsh@wshooper.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net/vmnet: Pad short Ethernet frames | expand |
On Sun, Jan 7, 2024 at 7:19 AM William Hooper <wsh@wshooper.org> wrote: > > At least on macOS 12.7.2, vmnet doesn't pad Ethernet frames, such as the > host's ARP replies, to the minimum size (60 bytes before the frame check > sequence) defined in IEEE Std 802.3-2022, so guests' Ethernet device > drivers may drop them with "frame too short" errors. > > This patch calls eth_pad_short_frame() to add padding, as in net/tap.c > and net/slirp.c. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2058 > Signed-off-by: William Hooper <wsh@wshooper.org> > --- > net/vmnet-common.m | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Hi William, On 6/1/24 23:35, William Hooper wrote: > At least on macOS 12.7.2, vmnet doesn't pad Ethernet frames, such as the > host's ARP replies, to the minimum size (60 bytes before the frame check > sequence) defined in IEEE Std 802.3-2022, so guests' Ethernet device > drivers may drop them with "frame too short" errors. > > This patch calls eth_pad_short_frame() to add padding, as in net/tap.c > and net/slirp.c. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2058 > Signed-off-by: William Hooper <wsh@wshooper.org> > --- > net/vmnet-common.m | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/net/vmnet-common.m b/net/vmnet-common.m > index 2958283485..f8f7163226 100644 > --- a/net/vmnet-common.m > +++ b/net/vmnet-common.m > @@ -18,6 +18,7 @@ > #include "qemu/error-report.h" > #include "qapi/error.h" > #include "sysemu/runstate.h" > +#include "net/eth.h" > > #include <vmnet/vmnet.h> > #include <dispatch/dispatch.h> > @@ -150,10 +151,23 @@ static int vmnet_read_packets(VmnetState *s) > */ > static void vmnet_write_packets_to_qemu(VmnetState *s) > { > + uint8_t *pkt; > + size_t pktsz; > + uint8_t min_pkt[ETH_ZLEN]; > + size_t min_pktsz = sizeof(min_pkt); > + > while (s->packets_send_current_pos < s->packets_send_end_pos) { > - ssize_t size = qemu_send_packet_async(&s->nc, > - s->iov_buf[s->packets_send_current_pos].iov_base, > - s->packets_buf[s->packets_send_current_pos].vm_pkt_size, > + pkt = s->iov_buf[s->packets_send_current_pos].iov_base; > + pktsz = s->packets_buf[s->packets_send_current_pos].vm_pkt_size; > + > + if (net_peer_needs_padding(&s->nc)) { Don't we want to initialize min_pktsz here ... min_pktsz = sizeof(min_pkt); > + if (eth_pad_short_frame(min_pkt, &min_pktsz, pkt, pktsz)) { ... because eth_pad_short_frame() update it? > + pkt = min_pkt; > + pktsz = min_pktsz; > + } > + } > + > + ssize_t size = qemu_send_packet_async(&s->nc, pkt, pktsz, > vmnet_send_completed); > > if (size == 0) {
On Mon, Jan 8, 2024 at 7:36 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > Don't we want to initialize min_pktsz here ... > > min_pktsz = sizeof(min_pkt); > > > + if (eth_pad_short_frame(min_pkt, &min_pktsz, pkt, pktsz)) { > > ... because eth_pad_short_frame() update it? Thanks for the review. The results would be the same, since eth_pad_short_frame() sets min_pktsz, if at all, to ETH_ZLEN, the same value as the initializer. I have no objection to re-initializing min_pktsz for each packet, however, if only to reduce the risk of a bug being introduced if this behavior of eth_pad_short_frame() were ever to be changed. Would you like me to post a revised patch?
diff --git a/net/vmnet-common.m b/net/vmnet-common.m index 2958283485..f8f7163226 100644 --- a/net/vmnet-common.m +++ b/net/vmnet-common.m @@ -18,6 +18,7 @@ #include "qemu/error-report.h" #include "qapi/error.h" #include "sysemu/runstate.h" +#include "net/eth.h" #include <vmnet/vmnet.h> #include <dispatch/dispatch.h> @@ -150,10 +151,23 @@ static int vmnet_read_packets(VmnetState *s) */ static void vmnet_write_packets_to_qemu(VmnetState *s) { + uint8_t *pkt; + size_t pktsz; + uint8_t min_pkt[ETH_ZLEN]; + size_t min_pktsz = sizeof(min_pkt); + while (s->packets_send_current_pos < s->packets_send_end_pos) { - ssize_t size = qemu_send_packet_async(&s->nc, - s->iov_buf[s->packets_send_current_pos].iov_base, - s->packets_buf[s->packets_send_current_pos].vm_pkt_size, + pkt = s->iov_buf[s->packets_send_current_pos].iov_base; + pktsz = s->packets_buf[s->packets_send_current_pos].vm_pkt_size; + + if (net_peer_needs_padding(&s->nc)) { + if (eth_pad_short_frame(min_pkt, &min_pktsz, pkt, pktsz)) { + pkt = min_pkt; + pktsz = min_pktsz; + } + } + + ssize_t size = qemu_send_packet_async(&s->nc, pkt, pktsz, vmnet_send_completed); if (size == 0) {
At least on macOS 12.7.2, vmnet doesn't pad Ethernet frames, such as the host's ARP replies, to the minimum size (60 bytes before the frame check sequence) defined in IEEE Std 802.3-2022, so guests' Ethernet device drivers may drop them with "frame too short" errors. This patch calls eth_pad_short_frame() to add padding, as in net/tap.c and net/slirp.c. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2058 Signed-off-by: William Hooper <wsh@wshooper.org> --- net/vmnet-common.m | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)