* [PATCH v2] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic
@ 2021-02-09 1:22 Bin Meng
2021-02-09 2:02 ` David Gibson
2021-02-09 9:48 ` Peter Maydell
0 siblings, 2 replies; 6+ messages in thread
From: Bin Meng @ 2021-02-09 1:22 UTC (permalink / raw)
To: Peter Maydell, David Gibson, Greg Kurz, Jason Wang
Cc: Bin Meng, qemu-ppc, qemu-devel
From: Bin Meng <bin.meng@windriver.com>
Per MPC8548ERM [1] chapter 14.5.3.4.1:
When RCTRL.RSF is 1, frames less than 64 bytes are accepted upon
a DA match. But currently QEMU does the opposite.
When RCTRL.RSF is 0, short frames are silently dropped, however
we cannot drop such frames in QEMU as of today, due to both slirp
and tap networking do not pad short frames (e.g.: an ARP packet)
to the minimum frame size of 60 bytes.
If eTSEC is programmed to reject short frames, ARP requests will be
dropped, preventing the guest from becoming visible on the network.
The same issue was reported on e1000 and vmxenet3 before, see:
commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")
Ideally this should be fixed on the slirp/tap networking side to
pad short frames to the minimum frame length, but I am not sure
whether that's doable.
This commit reverses the RCTRL.RSF testing logic to match the spec.
The log message is updated to mention the reject short frames
functionality is unimplemented.
[1] https://www.nxp.com/docs/en/reference-manual/MPC8548ERM.pdf
Fixes: eb1e7c3e5146 ("Add Enhanced Three-Speed Ethernet Controller (eTSEC)")
Signed-off-by: Bin Meng <bin.meng@windriver.com>
---
Changes in v2:
- rewrite the commit message and reverse the RCTRL.RSF test logic
hw/net/fsl_etsec/rings.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index 121415a..f89aa7f 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -502,10 +502,17 @@ ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
return -1;
}
- if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
+ /*
+ * Both slirp and tap networking do not pad short frames
+ * (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
+ *
+ * If eTSEC is programmed to reject short frames, ARP requests
+ * will be dropped, preventing the guest from becoming visible
+ * on the network.
+ */
+ if (!(etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
/* CRC is not in the packet yet, so short frame is below 60 bytes */
- RING_DEBUG("%s: Drop short frame\n", __func__);
- return -1;
+ RING_DEBUG("%s: Drop short frame not implemented\n", __func__);
}
rx_init_frame(etsec, buf, size);
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic
2021-02-09 1:22 [PATCH v2] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic Bin Meng
@ 2021-02-09 2:02 ` David Gibson
2021-02-09 9:48 ` Peter Maydell
1 sibling, 0 replies; 6+ messages in thread
From: David Gibson @ 2021-02-09 2:02 UTC (permalink / raw)
To: Bin Meng
Cc: Peter Maydell, Jason Wang, Bin Meng, Greg Kurz, qemu-devel,
qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 3056 bytes --]
On Tue, Feb 09, 2021 at 09:22:41AM +0800, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
>
> Per MPC8548ERM [1] chapter 14.5.3.4.1:
>
> When RCTRL.RSF is 1, frames less than 64 bytes are accepted upon
> a DA match. But currently QEMU does the opposite.
>
> When RCTRL.RSF is 0, short frames are silently dropped, however
> we cannot drop such frames in QEMU as of today, due to both slirp
> and tap networking do not pad short frames (e.g.: an ARP packet)
> to the minimum frame size of 60 bytes.
>
> If eTSEC is programmed to reject short frames, ARP requests will be
> dropped, preventing the guest from becoming visible on the network.
>
> The same issue was reported on e1000 and vmxenet3 before, see:
>
> commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
> commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")
>
> Ideally this should be fixed on the slirp/tap networking side to
> pad short frames to the minimum frame length, but I am not sure
> whether that's doable.
>
> This commit reverses the RCTRL.RSF testing logic to match the spec.
> The log message is updated to mention the reject short frames
> functionality is unimplemented.
>
> [1] https://www.nxp.com/docs/en/reference-manual/MPC8548ERM.pdf
>
> Fixes: eb1e7c3e5146 ("Add Enhanced Three-Speed Ethernet Controller (eTSEC)")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
Applied to ppc-for-6.0.
Thanks for the excellent commit message with the links to the relevant
documentation.
>
> ---
>
> Changes in v2:
> - rewrite the commit message and reverse the RCTRL.RSF test logic
>
> hw/net/fsl_etsec/rings.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index 121415a..f89aa7f 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -502,10 +502,17 @@ ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
> return -1;
> }
>
> - if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> + /*
> + * Both slirp and tap networking do not pad short frames
> + * (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
> + *
> + * If eTSEC is programmed to reject short frames, ARP requests
> + * will be dropped, preventing the guest from becoming visible
> + * on the network.
> + */
> + if (!(etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> /* CRC is not in the packet yet, so short frame is below 60 bytes */
> - RING_DEBUG("%s: Drop short frame\n", __func__);
> - return -1;
> + RING_DEBUG("%s: Drop short frame not implemented\n", __func__);
> }
>
> rx_init_frame(etsec, buf, size);
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic
2021-02-09 1:22 [PATCH v2] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic Bin Meng
2021-02-09 2:02 ` David Gibson
@ 2021-02-09 9:48 ` Peter Maydell
2021-02-09 9:54 ` Bin Meng
2021-02-10 0:11 ` David Gibson
1 sibling, 2 replies; 6+ messages in thread
From: Peter Maydell @ 2021-02-09 9:48 UTC (permalink / raw)
To: Bin Meng
Cc: Jason Wang, Bin Meng, QEMU Developers, Greg Kurz, qemu-ppc,
David Gibson
On Tue, 9 Feb 2021 at 01:22, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> Per MPC8548ERM [1] chapter 14.5.3.4.1:
>
> When RCTRL.RSF is 1, frames less than 64 bytes are accepted upon
> a DA match. But currently QEMU does the opposite.
>
> When RCTRL.RSF is 0, short frames are silently dropped, however
> we cannot drop such frames in QEMU as of today, due to both slirp
> and tap networking do not pad short frames (e.g.: an ARP packet)
> to the minimum frame size of 60 bytes.
>
> If eTSEC is programmed to reject short frames, ARP requests will be
> dropped, preventing the guest from becoming visible on the network.
>
> The same issue was reported on e1000 and vmxenet3 before, see:
>
> commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
> commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")
>
> Ideally this should be fixed on the slirp/tap networking side to
> pad short frames to the minimum frame length, but I am not sure
> whether that's doable.
>
> This commit reverses the RCTRL.RSF testing logic to match the spec.
> The log message is updated to mention the reject short frames
> functionality is unimplemented.
>
> [1] https://www.nxp.com/docs/en/reference-manual/MPC8548ERM.pdf
>
> Fixes: eb1e7c3e5146 ("Add Enhanced Three-Speed Ethernet Controller (eTSEC)")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> - if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> + /*
> + * Both slirp and tap networking do not pad short frames
> + * (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
> + *
> + * If eTSEC is programmed to reject short frames, ARP requests
> + * will be dropped, preventing the guest from becoming visible
> + * on the network.
> + */
> + if (!(etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> /* CRC is not in the packet yet, so short frame is below 60 bytes */
> - RING_DEBUG("%s: Drop short frame\n", __func__);
> - return -1;
> + RING_DEBUG("%s: Drop short frame not implemented\n", __func__);
> }
This change is doing two things at once.
One of them is an entirely uncontroversial bug fix: we
got the sense of the RCTRL_RSF test the wrong way round.
The other is different: it is working around a bug elsewhere in QEMU.
If there's a problem with packets that should not be short
frames being presented to ethernet devices as short frames,
please fix that bug at the source. I don't think we should
take any more device-model workarounds for it. We have lots
and lots of ethernet device models: it will be much more
effort to try to fix them all one by one as people encounter
this bug than it would be to just fix the code that's creating
bogus short frames.
David, could you drop this from your queue, please ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic
2021-02-09 9:48 ` Peter Maydell
@ 2021-02-09 9:54 ` Bin Meng
2021-02-10 0:11 ` David Gibson
1 sibling, 0 replies; 6+ messages in thread
From: Bin Meng @ 2021-02-09 9:54 UTC (permalink / raw)
To: Peter Maydell
Cc: Jason Wang, Bin Meng, QEMU Developers, Greg Kurz, qemu-ppc,
David Gibson
Hi Peter,
On Tue, Feb 9, 2021 at 5:48 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 9 Feb 2021 at 01:22, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > Per MPC8548ERM [1] chapter 14.5.3.4.1:
> >
> > When RCTRL.RSF is 1, frames less than 64 bytes are accepted upon
> > a DA match. But currently QEMU does the opposite.
> >
> > When RCTRL.RSF is 0, short frames are silently dropped, however
> > we cannot drop such frames in QEMU as of today, due to both slirp
> > and tap networking do not pad short frames (e.g.: an ARP packet)
> > to the minimum frame size of 60 bytes.
> >
> > If eTSEC is programmed to reject short frames, ARP requests will be
> > dropped, preventing the guest from becoming visible on the network.
> >
> > The same issue was reported on e1000 and vmxenet3 before, see:
> >
> > commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
> > commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")
> >
> > Ideally this should be fixed on the slirp/tap networking side to
> > pad short frames to the minimum frame length, but I am not sure
> > whether that's doable.
> >
> > This commit reverses the RCTRL.RSF testing logic to match the spec.
> > The log message is updated to mention the reject short frames
> > functionality is unimplemented.
> >
> > [1] https://www.nxp.com/docs/en/reference-manual/MPC8548ERM.pdf
> >
> > Fixes: eb1e7c3e5146 ("Add Enhanced Three-Speed Ethernet Controller (eTSEC)")
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
>
>
> > - if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> > + /*
> > + * Both slirp and tap networking do not pad short frames
> > + * (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
> > + *
> > + * If eTSEC is programmed to reject short frames, ARP requests
> > + * will be dropped, preventing the guest from becoming visible
> > + * on the network.
> > + */
> > + if (!(etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> > /* CRC is not in the packet yet, so short frame is below 60 bytes */
> > - RING_DEBUG("%s: Drop short frame\n", __func__);
> > - return -1;
> > + RING_DEBUG("%s: Drop short frame not implemented\n", __func__);
> > }
>
> This change is doing two things at once.
We may have to do that. If we just reverse the testing logic, I bet
lots of guest software will break because of the reversion. Doing 2
things at one won't breaks such bisectability.
>
> One of them is an entirely uncontroversial bug fix: we
> got the sense of the RCTRL_RSF test the wrong way round.
>
> The other is different: it is working around a bug elsewhere in QEMU.
>
> If there's a problem with packets that should not be short
> frames being presented to ethernet devices as short frames,
> please fix that bug at the source. I don't think we should
> take any more device-model workarounds for it.
See above, if only fixing the testing logic, the fix may immediately
break guest software.
Or someone could fix the slirp or tap networking ASAP?
> We have lots
> and lots of ethernet device models: it will be much more
> effort to try to fix them all one by one as people encounter
> this bug than it would be to just fix the code that's creating
> bogus short frames.
>
> David, could you drop this from your queue, please ?
Regards,
Bin
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic
2021-02-09 9:48 ` Peter Maydell
2021-02-09 9:54 ` Bin Meng
@ 2021-02-10 0:11 ` David Gibson
2021-02-10 1:37 ` Bin Meng
1 sibling, 1 reply; 6+ messages in thread
From: David Gibson @ 2021-02-10 0:11 UTC (permalink / raw)
To: Peter Maydell
Cc: Jason Wang, Bin Meng, Greg Kurz, QEMU Developers, qemu-ppc,
Bin Meng
[-- Attachment #1: Type: text/plain, Size: 3379 bytes --]
On Tue, Feb 09, 2021 at 09:48:18AM +0000, Peter Maydell wrote:
> On Tue, 9 Feb 2021 at 01:22, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > Per MPC8548ERM [1] chapter 14.5.3.4.1:
> >
> > When RCTRL.RSF is 1, frames less than 64 bytes are accepted upon
> > a DA match. But currently QEMU does the opposite.
> >
> > When RCTRL.RSF is 0, short frames are silently dropped, however
> > we cannot drop such frames in QEMU as of today, due to both slirp
> > and tap networking do not pad short frames (e.g.: an ARP packet)
> > to the minimum frame size of 60 bytes.
> >
> > If eTSEC is programmed to reject short frames, ARP requests will be
> > dropped, preventing the guest from becoming visible on the network.
> >
> > The same issue was reported on e1000 and vmxenet3 before, see:
> >
> > commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
> > commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")
> >
> > Ideally this should be fixed on the slirp/tap networking side to
> > pad short frames to the minimum frame length, but I am not sure
> > whether that's doable.
> >
> > This commit reverses the RCTRL.RSF testing logic to match the spec.
> > The log message is updated to mention the reject short frames
> > functionality is unimplemented.
> >
> > [1] https://www.nxp.com/docs/en/reference-manual/MPC8548ERM.pdf
> >
> > Fixes: eb1e7c3e5146 ("Add Enhanced Three-Speed Ethernet Controller (eTSEC)")
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
>
>
> > - if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> > + /*
> > + * Both slirp and tap networking do not pad short frames
> > + * (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
> > + *
> > + * If eTSEC is programmed to reject short frames, ARP requests
> > + * will be dropped, preventing the guest from becoming visible
> > + * on the network.
> > + */
> > + if (!(etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> > /* CRC is not in the packet yet, so short frame is below 60 bytes */
> > - RING_DEBUG("%s: Drop short frame\n", __func__);
> > - return -1;
> > + RING_DEBUG("%s: Drop short frame not implemented\n", __func__);
> > }
>
> This change is doing two things at once.
Oops, I missed that.
> One of them is an entirely uncontroversial bug fix: we
> got the sense of the RCTRL_RSF test the wrong way round.
>
> The other is different: it is working around a bug elsewhere in QEMU.
>
> If there's a problem with packets that should not be short
> frames being presented to ethernet devices as short frames,
> please fix that bug at the source. I don't think we should
> take any more device-model workarounds for it. We have lots
> and lots of ethernet device models: it will be much more
> effort to try to fix them all one by one as people encounter
> this bug than it would be to just fix the code that's creating
> bogus short frames.
>
> David, could you drop this from your queue, please ?
Done.
>
> thanks
> -- PMM
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic
2021-02-10 0:11 ` David Gibson
@ 2021-02-10 1:37 ` Bin Meng
0 siblings, 0 replies; 6+ messages in thread
From: Bin Meng @ 2021-02-10 1:37 UTC (permalink / raw)
To: David Gibson
Cc: Peter Maydell, Jason Wang, Bin Meng, Greg Kurz, QEMU Developers,
qemu-ppc
Hi David, Peter,
On Wed, Feb 10, 2021 at 9:16 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Tue, Feb 09, 2021 at 09:48:18AM +0000, Peter Maydell wrote:
> > On Tue, 9 Feb 2021 at 01:22, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > Per MPC8548ERM [1] chapter 14.5.3.4.1:
> > >
> > > When RCTRL.RSF is 1, frames less than 64 bytes are accepted upon
> > > a DA match. But currently QEMU does the opposite.
> > >
> > > When RCTRL.RSF is 0, short frames are silently dropped, however
> > > we cannot drop such frames in QEMU as of today, due to both slirp
> > > and tap networking do not pad short frames (e.g.: an ARP packet)
> > > to the minimum frame size of 60 bytes.
> > >
> > > If eTSEC is programmed to reject short frames, ARP requests will be
> > > dropped, preventing the guest from becoming visible on the network.
> > >
> > > The same issue was reported on e1000 and vmxenet3 before, see:
> > >
> > > commit 78aeb23eded2 ("e1000: Pad short frames to minimum size (60 bytes)")
> > > commit 40a87c6c9b11 ("vmxnet3: Pad short frames to minimum size (60 bytes)")
> > >
> > > Ideally this should be fixed on the slirp/tap networking side to
> > > pad short frames to the minimum frame length, but I am not sure
> > > whether that's doable.
> > >
> > > This commit reverses the RCTRL.RSF testing logic to match the spec.
> > > The log message is updated to mention the reject short frames
> > > functionality is unimplemented.
> > >
> > > [1] https://www.nxp.com/docs/en/reference-manual/MPC8548ERM.pdf
> > >
> > > Fixes: eb1e7c3e5146 ("Add Enhanced Three-Speed Ethernet Controller (eTSEC)")
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >
> >
> > > - if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> > > + /*
> > > + * Both slirp and tap networking do not pad short frames
> > > + * (e.g.: an ARP packet) to the minimum frame size of 60 bytes.
> > > + *
> > > + * If eTSEC is programmed to reject short frames, ARP requests
> > > + * will be dropped, preventing the guest from becoming visible
> > > + * on the network.
> > > + */
> > > + if (!(etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
> > > /* CRC is not in the packet yet, so short frame is below 60 bytes */
> > > - RING_DEBUG("%s: Drop short frame\n", __func__);
> > > - return -1;
> > > + RING_DEBUG("%s: Drop short frame not implemented\n", __func__);
> > > }
> >
> > This change is doing two things at once.
>
> Oops, I missed that.
>
> > One of them is an entirely uncontroversial bug fix: we
> > got the sense of the RCTRL_RSF test the wrong way round.
> >
> > The other is different: it is working around a bug elsewhere in QEMU.
> >
> > If there's a problem with packets that should not be short
> > frames being presented to ethernet devices as short frames,
> > please fix that bug at the source. I don't think we should
> > take any more device-model workarounds for it. We have lots
> > and lots of ethernet device models: it will be much more
> > effort to try to fix them all one by one as people encounter
> > this bug than it would be to just fix the code that's creating
> > bogus short frames.
> >
> > David, could you drop this from your queue, please ?
>
> Done.
OK, I will only do the reverse then.
Regards,
Bin
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-10 1:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-09 1:22 [PATCH v2] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic Bin Meng
2021-02-09 2:02 ` David Gibson
2021-02-09 9:48 ` Peter Maydell
2021-02-09 9:54 ` Bin Meng
2021-02-10 0:11 ` David Gibson
2021-02-10 1:37 ` Bin Meng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).