[PATCH v3] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic

[PATCH v3] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic

[Bin Meng]

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. This commit
reverses the RCTRL.RSF testing logic to match the manual.

Due to the reverse of the logic, certain guests may potentially
break if they don't program eTSEC to have RCTRL.RSF bit set.
When RCTRL.RSF is 0, short frames are silently dropped, however
as of today both slirp and tap networking do not pad short frames
(e.g.: an ARP packet) to the minimum frame size of 60 bytes. So
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)")

[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 v3:
- remove the slirp/tap networking workaround and only do the reverse

Changes in v2:
- rewrite the commit message and reverse the RCTRL.RSF test logic

 hw/net/fsl_etsec/rings.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index 121415a..fe055d3 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -502,7 +502,7 @@ 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)) {
+    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;
-- 
2.7.4

Re: [PATCH v3] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

On Wed, Feb 10, 2021 at 10:10:21AM +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. This commit
> reverses the RCTRL.RSF testing logic to match the manual.
> 
> Due to the reverse of the logic, certain guests may potentially
> break if they don't program eTSEC to have RCTRL.RSF bit set.
> When RCTRL.RSF is 0, short frames are silently dropped, however
> as of today both slirp and tap networking do not pad short frames
> (e.g.: an ARP packet) to the minimum frame size of 60 bytes. So
> 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)")
> 
> [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.

> 
> ---
> 
> Changes in v3:
> - remove the slirp/tap networking workaround and only do the reverse
> 
> Changes in v2:
> - rewrite the commit message and reverse the RCTRL.RSF test logic
> 
>  hw/net/fsl_etsec/rings.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index 121415a..fe055d3 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -502,7 +502,7 @@ 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)) {
> +    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;

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

Re: [PATCH v3] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic

[no-reply]

Patchew URL: https://patchew.org/QEMU/1612923021-19746-1-git-send-email-bmeng.cn@gmail.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1612923021-19746-1-git-send-email-bmeng.cn@gmail.com
Subject: [PATCH v3] hw/net: fsl_etsec: Reverse the RCTRL.RSF logic

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/cover.1612953419.git.pkrempa@redhat.com -> patchew/cover.1612953419.git.pkrempa@redhat.com
Switched to a new branch 'test'

=== OUTPUT BEGIN ===
checkpatch.pl: no revisions returned for revlist 'base..'
=== OUTPUT END ===

Test command exited with code: 255


The full log is available at
http://patchew.org/logs/1612923021-19746-1-git-send-email-bmeng.cn@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com