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

[Jason Wang]

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.


>   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.

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.

So we're actually fine here?

Thanks


> > Thanks
> >
> > [1]
> > https://patchwork.ozlabs.org/project/qemu-devel/patch/1284842625-13920-1-git-send-email-stefanha@linux.vnet.ibm.com/
> Regards,
> Bin