Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

[Bin Meng]

On Thu, Mar 11, 2021 at 11:33 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/3/11 11:12 上午, Bin Meng wrote:
> > On Thu, Mar 11, 2021 at 11:01 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2021/3/9 6:13 下午, Peter Maydell wrote:
> >>> On Tue, 9 Mar 2021 at 09:01, Bin Meng <bmeng.cn@gmail.com> wrote:
> >>>> Hi Jason,
> >>>>
> >>>> On Tue, Mar 9, 2021 at 5:00 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>>>> Hi Jason,
> >>>>>
> >>>>> On Tue, Mar 9, 2021 at 4:57 PM Jason Wang <jasowang@redhat.com> wrote:
> >>>>>> On 2021/3/9 4:35 下午, Bin Meng wrote:
> >>>>>>> Hi Jason,
> >>>>>>>
> >>>>>>> On Tue, Mar 9, 2021 at 4:23 PM Jason Wang <jasowang@redhat.com> wrote:
> >>>>>>>> On 2021/3/8 6:22 下午, Peter Maydell wrote:
> >>>>>>>>> I think the key thing we need to do here is make a decision
> >>>>>>>>> and be clear about what we're doing. There are three options
> >>>>>>>>> I can see:
> >>>>>>>>>
> >>>>>>>>> (1) we say that the net API demands that backends pad
> >>>>>>>>> packets they emit to the minimum ethernet frame length
> >>>>>>>>> unless they specifically are intending to emit a short frame,
> >>>>>>>>> and we fix any backends that don't comply (or equivalently,
> >>>>>>>>> add support in the core code for a backend to mark itself
> >>>>>>>>> as "I don't pad; please do it for me").
> >>>>>>>>>
> >>>>>>>>> (2) we say that the networking subsystem doesn't support
> >>>>>>>>> short packets, and just have the common code always enforce
> >>>>>>>>> padding short frames to the minimum length somewhere between
> >>>>>>>>> when it receives a packet from a backend and passes it to
> >>>>>>>>> a NIC model.
> >>>>>>>>>
> >>>>>>>>> (3) we say that it's the job of the NIC models to pad
> >>>>>>>>> short frames as they see them coming in.
> >>>>>>>> I'm not sure how much value we can gain from (1). So (2) looks 
> >>>>>>>> better to me.
> >>>>>>>>
> >>>>>>>> Bin or Philippe, want to send a new version?
> >>>>>>>>
> >>>>>>> I think this series does what (2) asks for. Or am I missing anything?
> >>>>>> It only did the padding for user/TAP.
> >>>> (hit send too soon ...)
> >>>>
> >>>> Ah, so we want this:
> >>>>
> >>>> if (sender->info->type != NET_CLIENT_DRIVER_NIC)
> >>>>
> >>>> correct?
> >>> No, option (2) is "always pad short packets regardless of
> >>> sender->info->type". Even if a NIC driver sends out a short
> >>> packet, we want to pad it, because we might be feeding it to
> >>> something that assumes it does not see short packets.
> >>>
> >>> thanks
> >>> -- PMM
> >>
> >> So I'm not sure this is correct. There're NIC that has its own logic
> >> that choose whether to pad the frame during TX (e.g e1000).
> > Yes, that's why I mentioned in v2's cover letter that we should
> > probably only do the padding for SLiRP and TAP.
>
>
> Actually it's a partail implementation of Peter's method 1. If we go
> that way, you need to make sure the packet is padded for every ethernet
> backend not just TAP and SLIRP.
>

This latest version series implemented the method 1, except for
providing a flag to indicate when NIC can send short frames and not
pad in the network codes.

>
> >   For NIC models, we can
> > still support sending short frames in QEMU.
>
>
> Then it will be padded as well.
>
>
> >
> >> And after a discussion 10 years ago [1]. Michael (cced) seems to want to
> >> keep the padding logic in the NIC itself (probably with a generic helper
> >> in the core). Since 1) the padding is only required for ethernet 2)
> >> virito-net doesn't need that (it can pass incomplete packet by design).
> >>
> > I did read this discussion before working on this patch series.
> > Providing a helper for NICs to call does NOT fix the issue for SLiRP
> > and TAP.
>
>
> I'm not sure I get here.
>
> For TX, the padding is controlled by the guest driver. So we just need
> to emulate what real NIC did, pad only when it was required explicitly
> by the driver.

Correct, version 2 of the RFC series do allow NIC models to send short
frames, but the latest version changed to pad short frames for every
network backends including NICs.

>
> For RX, if we receive short frames from an ethternet backend, we simply
> pad them to make sure it won't be dropped by the NIC model.
>

No, we don't do this. Hardware NICs don't do such. They either simply
drop the short frames, or receive it as it is.

> So we're actually fine here?
>

Regards,
Bin