From: Jan Kiszka <jan.kiszka@siemens.com>
The spec does not justify clearing of any E1000_ICR_OTHER_CAUSES when
E1000_ICR_OTHER is set in EIAC. In fact, removing this code fixes the
issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: Avoid
receiver overrun interrupt bursts") and was worked around by
745d0bd3af99 ("e1000e: Remove Other from EIAC").
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
This resolves the issue I reported on February 18 ("e1000e: MSI-X
problem with recent Linux drivers").
hw/net/e1000e_core.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index ecf9b15555..d38f025c0f 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg)
effective_eiac = core->mac[EIAC] & cause;
- if (effective_eiac == E1000_ICR_OTHER) {
- effective_eiac |= E1000_ICR_OTHER_CAUSES;
- }
-
core->mac[ICR] &= ~effective_eiac;
if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
--
2.13.6
Hi, This series failed docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. Type: series Message-id: 267f42a5-b7ce-379e-ffd1-f2611393d2ff@web.de Subject: [Qemu-devel] [PATCH] e1000e: Do not auto-clear ICR bits which aren't set in EIAC === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-mingw@fedora === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 665d663f3d e1000e: Do not auto-clear ICR bits which aren't set in EIAC === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-9hyebnaz/src/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' BUILD fedora make[1]: Entering directory '/var/tmp/patchew-tester-tmp-9hyebnaz/src' GEN /var/tmp/patchew-tester-tmp-9hyebnaz/src/docker-src.2018-04-01-17.24.18.15141/qemu.tar Cloning into '/var/tmp/patchew-tester-tmp-9hyebnaz/src/docker-src.2018-04-01-17.24.18.15141/qemu.tar.vroot'... done. Checking out files: 13% (841/6066) Checking out files: 14% (850/6066) Checking out files: 15% (910/6066) Checking out files: 16% (971/6066) Checking out files: 17% (1032/6066) Checking out files: 18% (1092/6066) Checking out files: 19% (1153/6066) Checking out files: 19% (1172/6066) Checking out files: 20% (1214/6066) Checking out files: 21% (1274/6066) Checking out files: 22% (1335/6066) Checking out files: 23% (1396/6066) Checking out files: 24% (1456/6066) Checking out files: 25% (1517/6066) Checking out files: 26% (1578/6066) Checking out files: 27% (1638/6066) Checking out files: 28% (1699/6066) Checking out files: 29% (1760/6066) Checking out files: 30% (1820/6066) Checking out files: 30% (1860/6066) Checking out files: 31% (1881/6066) Checking out files: 32% (1942/6066) Checking out files: 33% (2002/6066) Checking out files: 34% (2063/6066) Checking out files: 35% (2124/6066) Checking out files: 36% (2184/6066) Checking out files: 37% (2245/6066) Checking out files: 38% (2306/6066) Checking out files: 39% (2366/6066) Checking out files: 40% (2427/6066) Checking out files: 41% (2488/6066) Checking out files: 42% (2548/6066) Checking out files: 43% (2609/6066) Checking out files: 44% (2670/6066) Checking out files: 44% (2693/6066) Checking out files: 45% (2730/6066) Checking out files: 46% (2791/6066) Checking out files: 46% (2815/6066) Checking out files: 47% (2852/6066) Checking out files: 48% (2912/6066) Checking out files: 49% (2973/6066) Checking out files: 50% (3033/6066) Checking out files: 51% (3094/6066) Checking out files: 52% (3155/6066) Checking out files: 52% (3203/6066) Checking out files: 53% (3215/6066) Checking out files: 54% (3276/6066) Checking out files: 55% (3337/6066) Checking out files: 56% (3397/6066) Checking out files: 57% (3458/6066) Checking out files: 57% (3463/6066) Checking out files: 58% (3519/6066) Checking out files: 59% (3579/6066) Checking out files: 60% (3640/6066) Checking out files: 61% (3701/6066) Checking out files: 62% (3761/6066) Checking out files: 63% (3822/6066) Checking out files: 64% (3883/6066) Checking out files: 65% (3943/6066) Checking out files: 66% (4004/6066) Checking out files: 67% (4065/6066) Checking out files: 68% (4125/6066) Checking out files: 68% (4142/6066) Checking out files: 69% (4186/6066) Checking out files: 70% (4247/6066) Checking out files: 71% (4307/6066) Checking out files: 72% (4368/6066) Checking out files: 73% (4429/6066) Checking out files: 74% (4489/6066) Checking out files: 74% (4507/6066) Checking out files: 75% (4550/6066) Checking out files: 76% (4611/6066) Checking out files: 77% (4671/6066) Checking out files: 78% (4732/6066) Checking out files: 79% (4793/6066) Checking out files: 80% (4853/6066) Checking out files: 81% (4914/6066) Checking out files: 82% (4975/6066) Checking out files: 83% (5035/6066) Checking out files: 84% (5096/6066) Checking out files: 85% (5157/6066) Checking out files: 86% (5217/6066) Checking out files: 87% (5278/6066) Checking out files: 87% (5326/6066) Checking out files: 88% (5339/6066) Checking out files: 89% (5399/6066) Checking out files: 90% (5460/6066) Checking out files: 91% (5521/6066) Checking out files: 92% (5581/6066) Checking out files: 93% (5642/6066) Checking out files: 94% (5703/6066) Checking out files: 95% (5763/6066) Checking out files: 96% (5824/6066) Checking out files: 97% (5885/6066) Checking out files: 98% (5945/6066) Checking out files: 99% (6006/6066) Checking out files: 100% (6066/6066) Checking out files: 100% (6066/6066), done. Your branch is up-to-date with 'origin/test'. Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-9hyebnaz/src/docker-src.2018-04-01-17.24.18.15141/qemu.tar.vroot/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into '/var/tmp/patchew-tester-tmp-9hyebnaz/src/docker-src.2018-04-01-17.24.18.15141/qemu.tar.vroot/ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' tar: /var/tmp/patchew-tester-tmp-9hyebnaz/src/docker-src.2018-04-01-17.24.18.15141/qemu.tar: Wrote only 2048 of 10240 bytes tar: Error is not recoverable: exiting now failed to create tar file COPY RUNNER RUN test-mingw in qemu:fedora tar: Unexpected EOF in archive tar: rmtlseek not stopped at a record boundary tar: Error is not recoverable: exiting now /var/tmp/qemu/run: line 32: prep_fail: command not found Packages installed: PyYAML-3.12-5.fc27.x86_64 SDL-devel-1.2.15-29.fc27.x86_64 bc-1.07.1-3.fc27.x86_64 bison-3.0.4-8.fc27.x86_64 bzip2-1.0.6-24.fc27.x86_64 ccache-3.3.6-1.fc27.x86_64 clang-5.0.1-3.fc27.x86_64 findutils-4.6.0-16.fc27.x86_64 flex-2.6.1-5.fc27.x86_64 gcc-7.3.1-5.fc27.x86_64 gcc-c++-7.3.1-5.fc27.x86_64 gettext-0.19.8.1-12.fc27.x86_64 git-2.14.3-3.fc27.x86_64 glib2-devel-2.54.3-2.fc27.x86_64 hostname-3.18-4.fc27.x86_64 libaio-devel-0.3.110-9.fc27.x86_64 libasan-7.3.1-5.fc27.x86_64 libfdt-devel-1.4.6-1.fc27.x86_64 libubsan-7.3.1-5.fc27.x86_64 llvm-5.0.1-3.fc27.x86_64 make-4.2.1-4.fc27.x86_64 mingw32-SDL-1.2.15-9.fc27.noarch mingw32-bzip2-1.0.6-9.fc27.noarch mingw32-curl-7.54.1-2.fc27.noarch mingw32-glib2-2.54.1-1.fc27.noarch mingw32-gmp-6.1.2-2.fc27.noarch mingw32-gnutls-3.5.13-2.fc27.noarch mingw32-gtk2-2.24.31-4.fc27.noarch mingw32-gtk3-3.22.16-1.fc27.noarch mingw32-libjpeg-turbo-1.5.1-3.fc27.noarch mingw32-libpng-1.6.29-2.fc27.noarch mingw32-libssh2-1.8.0-3.fc27.noarch mingw32-libtasn1-4.13-1.fc27.noarch mingw32-nettle-3.3-3.fc27.noarch mingw32-pixman-0.34.0-3.fc27.noarch mingw32-pkg-config-0.28-9.fc27.x86_64 mingw64-SDL-1.2.15-9.fc27.noarch mingw64-bzip2-1.0.6-9.fc27.noarch mingw64-curl-7.54.1-2.fc27.noarch mingw64-glib2-2.54.1-1.fc27.noarch mingw64-gmp-6.1.2-2.fc27.noarch mingw64-gnutls-3.5.13-2.fc27.noarch mingw64-gtk2-2.24.31-4.fc27.noarch mingw64-gtk3-3.22.16-1.fc27.noarch mingw64-libjpeg-turbo-1.5.1-3.fc27.noarch mingw64-libpng-1.6.29-2.fc27.noarch mingw64-libssh2-1.8.0-3.fc27.noarch mingw64-libtasn1-4.13-1.fc27.noarch mingw64-nettle-3.3-3.fc27.noarch mingw64-pixman-0.34.0-3.fc27.noarch mingw64-pkg-config-0.28-9.fc27.x86_64 nettle-devel-3.4-1.fc27.x86_64 perl-5.26.1-403.fc27.x86_64 pixman-devel-0.34.0-4.fc27.x86_64 python3-3.6.2-13.fc27.x86_64 sparse-0.5.1-2.fc27.x86_64 tar-1.29-7.fc27.x86_64 which-2.21-4.fc27.x86_64 zlib-devel-1.2.11-4.fc27.x86_64 Environment variables: TARGET_LIST= PACKAGES=ccache gettext git tar PyYAML sparse flex bison python3 bzip2 hostname glib2-devel pixman-devel zlib-devel SDL-devel libfdt-devel gcc gcc-c++ llvm clang make perl which bc findutils libaio-devel nettle-devel libasan libubsan mingw32-pixman mingw32-glib2 mingw32-gmp mingw32-SDL mingw32-pkg-config mingw32-gtk2 mingw32-gtk3 mingw32-gnutls mingw32-nettle mingw32-libtasn1 mingw32-libjpeg-turbo mingw32-libpng mingw32-curl mingw32-libssh2 mingw32-bzip2 mingw64-pixman mingw64-glib2 mingw64-gmp mingw64-SDL mingw64-pkg-config mingw64-gtk2 mingw64-gtk3 mingw64-gnutls mingw64-nettle mingw64-libtasn1 mingw64-libjpeg-turbo mingw64-libpng mingw64-curl mingw64-libssh2 mingw64-bzip2 J=8 V= HOSTNAME=45f12165d142 DEBUG= SHOW_ENV=1 PWD=/ HOME=/root CCACHE_DIR=/var/tmp/ccache DISTTAG=f27container QEMU_CONFIGURE_OPTS=--python=/usr/bin/python3 FGC=f27 TEST_DIR=/tmp/qemu-test SHLVL=1 FEATURES=mingw clang pyyaml asan dtc PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin MAKEFLAGS= -j8 EXTRA_CONFIGURE_OPTS= _=/usr/bin/env /var/tmp/qemu/run: line 52: cd: /tmp/qemu-test/src/tests/docker: No such file or directory /var/tmp/qemu/run: line 57: /test-mingw: No such file or directory Traceback (most recent call last): File "./tests/docker/docker.py", line 407, in <module> sys.exit(main()) File "./tests/docker/docker.py", line 404, in main return args.cmdobj.run(args, argv) File "./tests/docker/docker.py", line 261, in run return Docker().run(argv, args.keep, quiet=args.quiet) File "./tests/docker/docker.py", line 229, in run quiet=quiet) File "./tests/docker/docker.py", line 147, in _do_check return subprocess.check_call(self._command + cmd, **kwargs) File "/usr/lib64/python2.7/subprocess.py", line 186, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['docker', 'run', '--label', 'com.qemu.instance.uuid=1df5a2a035f311e8b32652540069c830', '-u', '0', '--security-opt', 'seccomp=unconfined', '--rm', '--net=none', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=8', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/root/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-9hyebnaz/src/docker-src.2018-04-01-17.24.18.15141:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 127 make[1]: *** [tests/docker/Makefile.include:129: docker-run] Error 1 make[1]: Leaving directory '/var/tmp/patchew-tester-tmp-9hyebnaz/src' make: *** [tests/docker/Makefile.include:163: docker-run-test-mingw@fedora] Error 2 real 0m52.962s user 0m10.088s sys 0m7.637s === OUTPUT END === Test command exited with code: 2 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Hi, This series failed docker-build@min-glib build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. Type: series Message-id: 267f42a5-b7ce-379e-ffd1-f2611393d2ff@web.de Subject: [Qemu-devel] [PATCH] e1000e: Do not auto-clear ICR bits which aren't set in EIAC === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-build@min-glib === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 665d663f3d e1000e: Do not auto-clear ICR bits which aren't set in EIAC === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-0v9s6yo6/src/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' BUILD min-glib make[1]: Entering directory '/var/tmp/patchew-tester-tmp-0v9s6yo6/src' GEN /var/tmp/patchew-tester-tmp-0v9s6yo6/src/docker-src.2018-04-01-17.32.05.20960/qemu.tar Cloning into '/var/tmp/patchew-tester-tmp-0v9s6yo6/src/docker-src.2018-04-01-17.32.05.20960/qemu.tar.vroot'... done. Checking out files: 42% (2568/6066) Checking out files: 43% (2609/6066) Checking out files: 44% (2670/6066) Checking out files: 45% (2730/6066) Checking out files: 45% (2751/6066) Checking out files: 46% (2791/6066) Checking out files: 47% (2852/6066) Checking out files: 48% (2912/6066) Checking out files: 49% (2973/6066) Checking out files: 50% (3033/6066) Checking out files: 51% (3094/6066) Checking out files: 52% (3155/6066) Checking out files: 53% (3215/6066) Checking out files: 54% (3276/6066) Checking out files: 55% (3337/6066) Checking out files: 56% (3397/6066) Checking out files: 56% (3430/6066) Checking out files: 57% (3458/6066) Checking out files: 58% (3519/6066) Checking out files: 59% (3579/6066) Checking out files: 60% (3640/6066) Checking out files: 61% (3701/6066) Checking out files: 62% (3761/6066) Checking out files: 63% (3822/6066) Checking out files: 64% (3883/6066) Checking out files: 65% (3943/6066) Checking out files: 66% (4004/6066) Checking out files: 67% (4065/6066) Checking out files: 68% (4125/6066) Checking out files: 69% (4186/6066) Checking out files: 70% (4247/6066) Checking out files: 71% (4307/6066) Checking out files: 72% (4368/6066) Checking out files: 73% (4429/6066) Checking out files: 74% (4489/6066) Checking out files: 75% (4550/6066) Checking out files: 76% (4611/6066) Checking out files: 77% (4671/6066) Checking out files: 78% (4732/6066) Checking out files: 79% (4793/6066) Checking out files: 80% (4853/6066) Checking out files: 81% (4914/6066) Checking out files: 82% (4975/6066) Checking out files: 83% (5035/6066) Checking out files: 84% (5096/6066) Checking out files: 85% (5157/6066) Checking out files: 86% (5217/6066) Checking out files: 87% (5278/6066) Checking out files: 88% (5339/6066) Checking out files: 89% (5399/6066) Checking out files: 90% (5460/6066) Checking out files: 91% (5521/6066) Checking out files: 92% (5581/6066) Checking out files: 93% (5642/6066) Checking out files: 94% (5703/6066) Checking out files: 95% (5763/6066) Checking out files: 96% (5824/6066) Checking out files: 97% (5885/6066) Checking out files: 98% (5945/6066) Checking out files: 99% (6006/6066) Checking out files: 100% (6066/6066) Checking out files: 100% (6066/6066), done. Your branch is up-to-date with 'origin/test'. Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-0v9s6yo6/src/docker-src.2018-04-01-17.32.05.20960/qemu.tar.vroot/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into '/var/tmp/patchew-tester-tmp-0v9s6yo6/src/docker-src.2018-04-01-17.32.05.20960/qemu.tar.vroot/ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' tar: /var/tmp/patchew-tester-tmp-0v9s6yo6/src/docker-src.2018-04-01-17.32.05.20960/qemu.tar: Wrote only 2048 of 10240 bytes tar: Error is not recoverable: exiting now failed to create tar file COPY RUNNER RUN test-build in qemu:min-glib tar: Unexpected EOF in archive tar: Unexpected EOF in archive tar: Error is not recoverable: exiting now /var/tmp/qemu/run: line 32: prep_fail: command not found Environment variables: HOSTNAME=0696834df35f MAKEFLAGS= -j8 J=8 CCACHE_DIR=/var/tmp/ccache EXTRA_CONFIGURE_OPTS= V= SHOW_ENV=1 PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin PWD=/ TARGET_LIST= SHLVL=1 HOME=/root TEST_DIR=/tmp/qemu-test FEATURES= dtc DEBUG= _=/usr/bin/env /var/tmp/qemu/run: line 52: cd: /tmp/qemu-test/src/tests/docker: No such file or directory /var/tmp/qemu/run: line 57: /test-build: No such file or directory /var/tmp/qemu/run: line 57: exec: /test-build: cannot execute: No such file or directory Traceback (most recent call last): File "./tests/docker/docker.py", line 407, in <module> sys.exit(main()) File "./tests/docker/docker.py", line 404, in main return args.cmdobj.run(args, argv) File "./tests/docker/docker.py", line 261, in run return Docker().run(argv, args.keep, quiet=args.quiet) File "./tests/docker/docker.py", line 229, in run quiet=quiet) File "./tests/docker/docker.py", line 147, in _do_check return subprocess.check_call(self._command + cmd, **kwargs) File "/usr/lib64/python2.7/subprocess.py", line 186, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['docker', 'run', '--label', 'com.qemu.instance.uuid=30d753c235f411e88d2d52540069c830', '-u', '0', '--security-opt', 'seccomp=unconfined', '--rm', '--net=none', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=8', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/root/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-0v9s6yo6/src/docker-src.2018-04-01-17.32.05.20960:/var/tmp/qemu:z,ro', 'qemu:min-glib', '/var/tmp/qemu/run', 'test-build']' returned non-zero exit status 126 make[1]: *** [tests/docker/Makefile.include:129: docker-run] Error 1 make[1]: Leaving directory '/var/tmp/patchew-tester-tmp-0v9s6yo6/src' make: *** [tests/docker/Makefile.include:163: docker-run-test-build@min-glib] Error 2 real 0m48.674s user 0m9.563s sys 0m7.082s === OUTPUT END === Test command exited with code: 2 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On 2018-04-01 23:17, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > The spec does not justify clearing of any E1000_ICR_OTHER_CAUSES when > E1000_ICR_OTHER is set in EIAC. In fact, removing this code fixes the > issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: Avoid > receiver overrun interrupt bursts") and was worked around by > 745d0bd3af99 ("e1000e: Remove Other from EIAC"). > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > > This resolves the issue I reported on February 18 ("e1000e: MSI-X > problem with recent Linux drivers"). > > hw/net/e1000e_core.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > index ecf9b15555..d38f025c0f 100644 > --- a/hw/net/e1000e_core.c > +++ b/hw/net/e1000e_core.c > @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg) > > effective_eiac = core->mac[EIAC] & cause; > > - if (effective_eiac == E1000_ICR_OTHER) { > - effective_eiac |= E1000_ICR_OTHER_CAUSES; > - } > - > core->mac[ICR] &= ~effective_eiac; > > if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { > Ping for this - as well as https://patchwork.ozlabs.org/patch/895476. Given that q35 uses e1000e by default and many Linux kernel versions no longer work, this should likely go into upcoming and stable versions Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux
On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2018-04-01 23:17, Jan Kiszka wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> The spec does not justify clearing of any E1000_ICR_OTHER_CAUSES when >> E1000_ICR_OTHER is set in EIAC. In fact, removing this code fixes the >> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: Avoid >> receiver overrun interrupt bursts") and was worked around by >> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> --- >> >> This resolves the issue I reported on February 18 ("e1000e: MSI-X >> problem with recent Linux drivers"). >> >> hw/net/e1000e_core.c | 4 ---- >> 1 file changed, 4 deletions(-) >> >> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >> index ecf9b15555..d38f025c0f 100644 >> --- a/hw/net/e1000e_core.c >> +++ b/hw/net/e1000e_core.c >> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg) >> >> effective_eiac = core->mac[EIAC] & cause; >> >> - if (effective_eiac == E1000_ICR_OTHER) { >> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >> - } >> - >> core->mac[ICR] &= ~effective_eiac; >> >> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >> > > Ping for this - as well as https://patchwork.ozlabs.org/patch/895476. > > Given that q35 uses e1000e by default and many Linux kernel versions no > longer work, this should likely go into upcoming and stable versions I'd rather not put it into 2.12 at this point in the release cycle unless it's a regression from 2.11, I think. thanks -- PMM
On 2018-04-16 17:29, Peter Maydell wrote: > On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> On 2018-04-01 23:17, Jan Kiszka wrote: >>> From: Jan Kiszka <jan.kiszka@siemens.com> >>> >>> The spec does not justify clearing of any E1000_ICR_OTHER_CAUSES when >>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code fixes the >>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: Avoid >>> receiver overrun interrupt bursts") and was worked around by >>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>> --- >>> >>> This resolves the issue I reported on February 18 ("e1000e: MSI-X >>> problem with recent Linux drivers"). >>> >>> hw/net/e1000e_core.c | 4 ---- >>> 1 file changed, 4 deletions(-) >>> >>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>> index ecf9b15555..d38f025c0f 100644 >>> --- a/hw/net/e1000e_core.c >>> +++ b/hw/net/e1000e_core.c >>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg) >>> >>> effective_eiac = core->mac[EIAC] & cause; >>> >>> - if (effective_eiac == E1000_ICR_OTHER) { >>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>> - } >>> - >>> core->mac[ICR] &= ~effective_eiac; >>> >>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>> >> >> Ping for this - as well as https://patchwork.ozlabs.org/patch/895476. >> >> Given that q35 uses e1000e by default and many Linux kernel versions no >> longer work, this should likely go into upcoming and stable versions > > I'd rather not put it into 2.12 at this point in the release > cycle unless it's a regression from 2.11, I think. Second ping - nothing hit the repo so far, nor did I receive feedback. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux
On 2018-05-22 09:00, Jan Kiszka wrote: > On 2018-04-16 17:29, Peter Maydell wrote: >> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>> On 2018-04-01 23:17, Jan Kiszka wrote: >>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>> >>>> The spec does not justify clearing of any E1000_ICR_OTHER_CAUSES when >>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code fixes the >>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: Avoid >>>> receiver overrun interrupt bursts") and was worked around by >>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>>> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>> --- >>>> >>>> This resolves the issue I reported on February 18 ("e1000e: MSI-X >>>> problem with recent Linux drivers"). >>>> >>>> hw/net/e1000e_core.c | 4 ---- >>>> 1 file changed, 4 deletions(-) >>>> >>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>> index ecf9b15555..d38f025c0f 100644 >>>> --- a/hw/net/e1000e_core.c >>>> +++ b/hw/net/e1000e_core.c >>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg) >>>> >>>> effective_eiac = core->mac[EIAC] & cause; >>>> >>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>> - } >>>> - >>>> core->mac[ICR] &= ~effective_eiac; >>>> >>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>> >>> >>> Ping for this - as well as https://patchwork.ozlabs.org/patch/895476. >>> >>> Given that q35 uses e1000e by default and many Linux kernel versions no >>> longer work, this should likely go into upcoming and stable versions >> >> I'd rather not put it into 2.12 at this point in the release >> cycle unless it's a regression from 2.11, I think. > > Second ping - nothing hit the repo so far, nor did I receive feedback. > And another ping. For both. These days I had to help someone with a broken QEMU setup that failed installing from network. It turned out that "modprobe e1000e IntMode=0" was needed to workaround the issues my patches address. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux
Hi Jan, On 06/12/2018 02:22 PM, Jan Kiszka wrote: > On 2018-05-22 09:00, Jan Kiszka wrote: >> On 2018-04-16 17:29, Peter Maydell wrote: >>> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>> On 2018-04-01 23:17, Jan Kiszka wrote: >>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>> >>>>> The spec does not justify clearing of any E1000_ICR_OTHER_CAUSES when >>>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code fixes the >>>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: Avoid >>>>> receiver overrun interrupt bursts") and was worked around by >>>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>>>> >>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>> --- >>>>> >>>>> This resolves the issue I reported on February 18 ("e1000e: MSI-X >>>>> problem with recent Linux drivers"). >>>>> >>>>> hw/net/e1000e_core.c | 4 ---- >>>>> 1 file changed, 4 deletions(-) >>>>> >>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>> index ecf9b15555..d38f025c0f 100644 >>>>> --- a/hw/net/e1000e_core.c >>>>> +++ b/hw/net/e1000e_core.c >>>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg) >>>>> >>>>> effective_eiac = core->mac[EIAC] & cause; >>>>> >>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>> - } >>>>> - >>>>> core->mac[ICR] &= ~effective_eiac; >>>>> >>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>> >>>> >>>> Ping for this - as well as https://patchwork.ozlabs.org/patch/895476. >>>> >>>> Given that q35 uses e1000e by default and many Linux kernel versions no >>>> longer work, this should likely go into upcoming and stable versions >>> >>> I'd rather not put it into 2.12 at this point in the release >>> cycle unless it's a regression from 2.11, I think. >> >> Second ping - nothing hit the repo so far, nor did I receive feedback. >> > > And another ping. For both. > > These days I had to help someone with a broken QEMU setup that failed > installing from network. It turned out that "modprobe e1000e IntMode=0" > was needed to workaround the issues my patches address. What about the IMS register? It is set just after. Looking at b38636b8372, can you test this patch? -- >8 -- diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index c93c4661ed..a484b68a5a 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -2022,13 +2022,13 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg) effective_eiac = core->mac[EIAC] & cause; - if (effective_eiac == E1000_ICR_OTHER) { - effective_eiac |= E1000_ICR_OTHER_CAUSES; - } - core->mac[ICR] &= ~effective_eiac; if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { + if (effective_eiac == E1000_ICR_OTHER) { + effective_eiac |= E1000_ICR_OTHER_CAUSES; + } + core->mac[IMS] &= ~effective_eiac; } } -- Thanks, Phil.
On 2018-06-12 20:11, Philippe Mathieu-Daudé wrote: > Hi Jan, > > On 06/12/2018 02:22 PM, Jan Kiszka wrote: >> On 2018-05-22 09:00, Jan Kiszka wrote: >>> On 2018-04-16 17:29, Peter Maydell wrote: >>>> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>>> On 2018-04-01 23:17, Jan Kiszka wrote: >>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>> >>>>>> The spec does not justify clearing of any E1000_ICR_OTHER_CAUSES when >>>>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code fixes the >>>>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: Avoid >>>>>> receiver overrun interrupt bursts") and was worked around by >>>>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>>>>> >>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>> --- >>>>>> >>>>>> This resolves the issue I reported on February 18 ("e1000e: MSI-X >>>>>> problem with recent Linux drivers"). >>>>>> >>>>>> hw/net/e1000e_core.c | 4 ---- >>>>>> 1 file changed, 4 deletions(-) >>>>>> >>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>> index ecf9b15555..d38f025c0f 100644 >>>>>> --- a/hw/net/e1000e_core.c >>>>>> +++ b/hw/net/e1000e_core.c >>>>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg) >>>>>> >>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>> >>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>> - } >>>>>> - >>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>> >>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>> >>>>> >>>>> Ping for this - as well as https://patchwork.ozlabs.org/patch/895476. >>>>> >>>>> Given that q35 uses e1000e by default and many Linux kernel versions no >>>>> longer work, this should likely go into upcoming and stable versions >>>> >>>> I'd rather not put it into 2.12 at this point in the release >>>> cycle unless it's a regression from 2.11, I think. >>> >>> Second ping - nothing hit the repo so far, nor did I receive feedback. >>> >> >> And another ping. For both. >> >> These days I had to help someone with a broken QEMU setup that failed >> installing from network. It turned out that "modprobe e1000e IntMode=0" >> was needed to workaround the issues my patches address. > > What about the IMS register? It is set just after. > > Looking at b38636b8372, can you test this patch? > > -- >8 -- > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > index c93c4661ed..a484b68a5a 100644 > --- a/hw/net/e1000e_core.c > +++ b/hw/net/e1000e_core.c > @@ -2022,13 +2022,13 @@ e1000e_msix_notify_one(E1000ECore *core, > uint32_t cause, uint32_t int_cfg) > > effective_eiac = core->mac[EIAC] & cause; > > - if (effective_eiac == E1000_ICR_OTHER) { > - effective_eiac |= E1000_ICR_OTHER_CAUSES; > - } > - > core->mac[ICR] &= ~effective_eiac; > > if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { > + if (effective_eiac == E1000_ICR_OTHER) { > + effective_eiac |= E1000_ICR_OTHER_CAUSES; > + } > + > core->mac[IMS] &= ~effective_eiac; > } > } > Before testing this: What would be the reasoning for this change? I need to refresh my caches, the debugging session is now too long ago again. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux
On 06/12/2018 03:30 PM, Jan Kiszka wrote: > On 2018-06-12 20:11, Philippe Mathieu-Daudé wrote: >> Hi Jan, >> >> On 06/12/2018 02:22 PM, Jan Kiszka wrote: >>> On 2018-05-22 09:00, Jan Kiszka wrote: >>>> On 2018-04-16 17:29, Peter Maydell wrote: >>>>> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>>>> On 2018-04-01 23:17, Jan Kiszka wrote: >>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>> >>>>>>> The spec does not justify clearing of any E1000_ICR_OTHER_CAUSES when >>>>>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code fixes the >>>>>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: Avoid >>>>>>> receiver overrun interrupt bursts") and was worked around by >>>>>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>>>>>> >>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>> --- >>>>>>> >>>>>>> This resolves the issue I reported on February 18 ("e1000e: MSI-X >>>>>>> problem with recent Linux drivers"). >>>>>>> >>>>>>> hw/net/e1000e_core.c | 4 ---- >>>>>>> 1 file changed, 4 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>>> index ecf9b15555..d38f025c0f 100644 >>>>>>> --- a/hw/net/e1000e_core.c >>>>>>> +++ b/hw/net/e1000e_core.c >>>>>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg) >>>>>>> >>>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>>> >>>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>> - } >>>>>>> - >>>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>>> >>>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>>> >>>>>> >>>>>> Ping for this - as well as https://patchwork.ozlabs.org/patch/895476. >>>>>> >>>>>> Given that q35 uses e1000e by default and many Linux kernel versions no >>>>>> longer work, this should likely go into upcoming and stable versions >>>>> >>>>> I'd rather not put it into 2.12 at this point in the release >>>>> cycle unless it's a regression from 2.11, I think. >>>> >>>> Second ping - nothing hit the repo so far, nor did I receive feedback. >>>> >>> >>> And another ping. For both. >>> >>> These days I had to help someone with a broken QEMU setup that failed >>> installing from network. It turned out that "modprobe e1000e IntMode=0" >>> was needed to workaround the issues my patches address. >> >> What about the IMS register? It is set just after. >> >> Looking at b38636b8372, can you test this patch? >> >> -- >8 -- >> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >> index c93c4661ed..a484b68a5a 100644 >> --- a/hw/net/e1000e_core.c >> +++ b/hw/net/e1000e_core.c >> @@ -2022,13 +2022,13 @@ e1000e_msix_notify_one(E1000ECore *core, >> uint32_t cause, uint32_t int_cfg) >> >> effective_eiac = core->mac[EIAC] & cause; >> >> - if (effective_eiac == E1000_ICR_OTHER) { >> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >> - } >> - >> core->mac[ICR] &= ~effective_eiac; >> >> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >> + if (effective_eiac == E1000_ICR_OTHER) { >> + effective_eiac |= E1000_ICR_OTHER_CAUSES; >> + } >> + >> core->mac[IMS] &= ~effective_eiac; >> } >> } >> > > Before testing this: What would be the reasoning for this change? Not breaking the purpose of b38636b8372 :) > > I need to refresh my caches, the debugging session is now too long ago > again. > > Jan >
On 2018-06-12 20:38, Philippe Mathieu-Daudé wrote: > On 06/12/2018 03:30 PM, Jan Kiszka wrote: >> On 2018-06-12 20:11, Philippe Mathieu-Daudé wrote: >>> Hi Jan, >>> >>> On 06/12/2018 02:22 PM, Jan Kiszka wrote: >>>> On 2018-05-22 09:00, Jan Kiszka wrote: >>>>> On 2018-04-16 17:29, Peter Maydell wrote: >>>>>> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>>>>> On 2018-04-01 23:17, Jan Kiszka wrote: >>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>> >>>>>>>> The spec does not justify clearing of any E1000_ICR_OTHER_CAUSES when >>>>>>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code fixes the >>>>>>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: Avoid >>>>>>>> receiver overrun interrupt bursts") and was worked around by >>>>>>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>>>>>>> >>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>> --- >>>>>>>> >>>>>>>> This resolves the issue I reported on February 18 ("e1000e: MSI-X >>>>>>>> problem with recent Linux drivers"). >>>>>>>> >>>>>>>> hw/net/e1000e_core.c | 4 ---- >>>>>>>> 1 file changed, 4 deletions(-) >>>>>>>> >>>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>>>> index ecf9b15555..d38f025c0f 100644 >>>>>>>> --- a/hw/net/e1000e_core.c >>>>>>>> +++ b/hw/net/e1000e_core.c >>>>>>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg) >>>>>>>> >>>>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>>>> >>>>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>>> - } >>>>>>>> - >>>>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>>>> >>>>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>>>> >>>>>>> >>>>>>> Ping for this - as well as https://patchwork.ozlabs.org/patch/895476. >>>>>>> >>>>>>> Given that q35 uses e1000e by default and many Linux kernel versions no >>>>>>> longer work, this should likely go into upcoming and stable versions >>>>>> >>>>>> I'd rather not put it into 2.12 at this point in the release >>>>>> cycle unless it's a regression from 2.11, I think. >>>>> >>>>> Second ping - nothing hit the repo so far, nor did I receive feedback. >>>>> >>>> >>>> And another ping. For both. >>>> >>>> These days I had to help someone with a broken QEMU setup that failed >>>> installing from network. It turned out that "modprobe e1000e IntMode=0" >>>> was needed to workaround the issues my patches address. >>> >>> What about the IMS register? It is set just after. >>> >>> Looking at b38636b8372, can you test this patch? >>> >>> -- >8 -- >>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>> index c93c4661ed..a484b68a5a 100644 >>> --- a/hw/net/e1000e_core.c >>> +++ b/hw/net/e1000e_core.c >>> @@ -2022,13 +2022,13 @@ e1000e_msix_notify_one(E1000ECore *core, >>> uint32_t cause, uint32_t int_cfg) >>> >>> effective_eiac = core->mac[EIAC] & cause; >>> >>> - if (effective_eiac == E1000_ICR_OTHER) { >>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>> - } >>> - >>> core->mac[ICR] &= ~effective_eiac; >>> >>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>> + if (effective_eiac == E1000_ICR_OTHER) { >>> + effective_eiac |= E1000_ICR_OTHER_CAUSES; >>> + } >>> + >>> core->mac[IMS] &= ~effective_eiac; >>> } >>> } >>> >> >> Before testing this: What would be the reasoning for this change? > > Not breaking the purpose of b38636b8372 :) I disagree on this expansion of bit 31 ("other causes"). I see no indication in the spec that setting this bit for autoclear has more impact than on the very same bit itself. Therefore I'm asking for a reasoning - based on the spec. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux
Cc'ing Jason who is also listed as co-maintainer: ./scripts/get_maintainer.pl -f hw/net/e1000e_core.c Dmitry Fleytman <dmitry.fleytman@gmail.com> (maintainer:e1000e) Jason Wang <jasowang@redhat.com> (odd fixer:Network devices) On 06/12/2018 03:43 PM, Jan Kiszka wrote: > On 2018-06-12 20:38, Philippe Mathieu-Daudé wrote: >> On 06/12/2018 03:30 PM, Jan Kiszka wrote: >>> On 2018-06-12 20:11, Philippe Mathieu-Daudé wrote: >>>> Hi Jan, >>>> >>>> On 06/12/2018 02:22 PM, Jan Kiszka wrote: >>>>> On 2018-05-22 09:00, Jan Kiszka wrote: >>>>>> On 2018-04-16 17:29, Peter Maydell wrote: >>>>>>> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>>>>>> On 2018-04-01 23:17, Jan Kiszka wrote: >>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>> >>>>>>>>> The spec does not justify clearing of any E1000_ICR_OTHER_CAUSES when >>>>>>>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code fixes the >>>>>>>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: Avoid >>>>>>>>> receiver overrun interrupt bursts") and was worked around by >>>>>>>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>>>>>>>> >>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> This resolves the issue I reported on February 18 ("e1000e: MSI-X >>>>>>>>> problem with recent Linux drivers"). >>>>>>>>> >>>>>>>>> hw/net/e1000e_core.c | 4 ---- >>>>>>>>> 1 file changed, 4 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>>>>> index ecf9b15555..d38f025c0f 100644 >>>>>>>>> --- a/hw/net/e1000e_core.c >>>>>>>>> +++ b/hw/net/e1000e_core.c >>>>>>>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg) >>>>>>>>> >>>>>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>>>>> >>>>>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>>>> - } >>>>>>>>> - >>>>>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>>>>> >>>>>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>>>>> >>>>>>>> >>>>>>>> Ping for this - as well as https://patchwork.ozlabs.org/patch/895476. >>>>>>>> >>>>>>>> Given that q35 uses e1000e by default and many Linux kernel versions no >>>>>>>> longer work, this should likely go into upcoming and stable versions >>>>>>> >>>>>>> I'd rather not put it into 2.12 at this point in the release >>>>>>> cycle unless it's a regression from 2.11, I think. >>>>>> >>>>>> Second ping - nothing hit the repo so far, nor did I receive feedback. >>>>>> >>>>> >>>>> And another ping. For both. >>>>> >>>>> These days I had to help someone with a broken QEMU setup that failed >>>>> installing from network. It turned out that "modprobe e1000e IntMode=0" >>>>> was needed to workaround the issues my patches address. >>>> >>>> What about the IMS register? It is set just after. >>>> >>>> Looking at b38636b8372, can you test this patch? >>>> >>>> -- >8 -- >>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>> index c93c4661ed..a484b68a5a 100644 >>>> --- a/hw/net/e1000e_core.c >>>> +++ b/hw/net/e1000e_core.c >>>> @@ -2022,13 +2022,13 @@ e1000e_msix_notify_one(E1000ECore *core, >>>> uint32_t cause, uint32_t int_cfg) >>>> >>>> effective_eiac = core->mac[EIAC] & cause; >>>> >>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>> - } >>>> - >>>> core->mac[ICR] &= ~effective_eiac; >>>> >>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>> + if (effective_eiac == E1000_ICR_OTHER) { >>>> + effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>> + } >>>> + >>>> core->mac[IMS] &= ~effective_eiac; >>>> } >>>> } >>>> >>> >>> Before testing this: What would be the reasoning for this change? >> >> Not breaking the purpose of b38636b8372 :) > > I disagree on this expansion of bit 31 ("other causes"). I see no > indication in the spec that setting this bit for autoclear has more > impact than on the very same bit itself. Therefore I'm asking for a > reasoning - based on the spec. Interrupt Cause Registers Other bits in this register are the legacy indication of interrupts as the MDIC complete, management and link status change. There is a specific Other Cause bit that is set if one of these bits are set, this bit can be mapped to a specific MSI-X interrupt message. I spent half an hour reading the relevant parts of the spec and can't figure out, so I'll let the authors of b38636b8372 to review your patch. Regards, Phil.
On 2018年06月13日 03:00, Philippe Mathieu-Daudé wrote: > Cc'ing Jason who is also listed as co-maintainer: > > ./scripts/get_maintainer.pl -f hw/net/e1000e_core.c > Dmitry Fleytman <dmitry.fleytman@gmail.com> (maintainer:e1000e) > Jason Wang <jasowang@redhat.com> (odd fixer:Network devices) > > On 06/12/2018 03:43 PM, Jan Kiszka wrote: >> On 2018-06-12 20:38, Philippe Mathieu-Daudé wrote: >>> On 06/12/2018 03:30 PM, Jan Kiszka wrote: >>>> On 2018-06-12 20:11, Philippe Mathieu-Daudé wrote: >>>>> Hi Jan, >>>>> >>>>> On 06/12/2018 02:22 PM, Jan Kiszka wrote: >>>>>> On 2018-05-22 09:00, Jan Kiszka wrote: >>>>>>> On 2018-04-16 17:29, Peter Maydell wrote: >>>>>>>> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>>>>>>> On 2018-04-01 23:17, Jan Kiszka wrote: >>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>> >>>>>>>>>> The spec does not justify clearing of any E1000_ICR_OTHER_CAUSES when >>>>>>>>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code fixes the >>>>>>>>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: Avoid >>>>>>>>>> receiver overrun interrupt bursts") and was worked around by >>>>>>>>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>>>>>>>>> >>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> This resolves the issue I reported on February 18 ("e1000e: MSI-X >>>>>>>>>> problem with recent Linux drivers"). >>>>>>>>>> >>>>>>>>>> hw/net/e1000e_core.c | 4 ---- >>>>>>>>>> 1 file changed, 4 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>>>>>> index ecf9b15555..d38f025c0f 100644 >>>>>>>>>> --- a/hw/net/e1000e_core.c >>>>>>>>>> +++ b/hw/net/e1000e_core.c >>>>>>>>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore *core, uint32_t cause, uint32_t int_cfg) >>>>>>>>>> >>>>>>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>>>>>> >>>>>>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>>>>> - } >>>>>>>>>> - >>>>>>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>>>>>> >>>>>>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>>>>>> >>>>>>>>> Ping for this - as well as https://patchwork.ozlabs.org/patch/895476. >>>>>>>>> >>>>>>>>> Given that q35 uses e1000e by default and many Linux kernel versions no >>>>>>>>> longer work, this should likely go into upcoming and stable versions >>>>>>>> I'd rather not put it into 2.12 at this point in the release >>>>>>>> cycle unless it's a regression from 2.11, I think. >>>>>>> Second ping - nothing hit the repo so far, nor did I receive feedback. >>>>>>> >>>>>> And another ping. For both. >>>>>> >>>>>> These days I had to help someone with a broken QEMU setup that failed >>>>>> installing from network. It turned out that "modprobe e1000e IntMode=0" >>>>>> was needed to workaround the issues my patches address. >>>>> What about the IMS register? It is set just after. >>>>> >>>>> Looking at b38636b8372, can you test this patch? >>>>> >>>>> -- >8 -- >>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>> index c93c4661ed..a484b68a5a 100644 >>>>> --- a/hw/net/e1000e_core.c >>>>> +++ b/hw/net/e1000e_core.c >>>>> @@ -2022,13 +2022,13 @@ e1000e_msix_notify_one(E1000ECore *core, >>>>> uint32_t cause, uint32_t int_cfg) >>>>> >>>>> effective_eiac = core->mac[EIAC] & cause; >>>>> >>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>> - } >>>>> - >>>>> core->mac[ICR] &= ~effective_eiac; >>>>> >>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>> + if (effective_eiac == E1000_ICR_OTHER) { >>>>> + effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>> + } >>>>> + >>>>> core->mac[IMS] &= ~effective_eiac; >>>>> } >>>>> } >>>>> >>>> Before testing this: What would be the reasoning for this change? >>> Not breaking the purpose of b38636b8372 :) >> I disagree on this expansion of bit 31 ("other causes"). I see no >> indication in the spec that setting this bit for autoclear has more >> impact than on the very same bit itself. Therefore I'm asking for a >> reasoning - based on the spec. > Interrupt Cause Registers > > Other bits in this register are the legacy indication of > interrupts as the MDIC complete, management and link status > change. There is a specific Other Cause bit that is set if > one of these bits are set, this bit can be mapped to a > specific MSI-X interrupt message. > > I spent half an hour reading the relevant parts of the spec and can't > figure out, so I'll let the authors of b38636b8372 to review your patch. > > Regards, > > Phil. Looking at EIAC part of the spec: Bits 24:20 in this register enables clearing of the corresponding bit in ICR following interrupt generation. When a bit is set, the corresponding bit in ICR and in IMS is automatically cleared following an interrupt. It looks to me that only the other bit itself need to be cleared. Thanks
Hi Jason, On 06/12/2018 11:18 PM, Jason Wang wrote: > On 2018年06月13日 03:00, Philippe Mathieu-Daudé wrote: >> Cc'ing Jason who is also listed as co-maintainer: >> >> ./scripts/get_maintainer.pl -f hw/net/e1000e_core.c >> Dmitry Fleytman <dmitry.fleytman@gmail.com> (maintainer:e1000e) >> Jason Wang <jasowang@redhat.com> (odd fixer:Network devices) >> >> On 06/12/2018 03:43 PM, Jan Kiszka wrote: >>> On 2018-06-12 20:38, Philippe Mathieu-Daudé wrote: >>>> On 06/12/2018 03:30 PM, Jan Kiszka wrote: >>>>> On 2018-06-12 20:11, Philippe Mathieu-Daudé wrote: >>>>>> Hi Jan, >>>>>> >>>>>> On 06/12/2018 02:22 PM, Jan Kiszka wrote: >>>>>>> On 2018-05-22 09:00, Jan Kiszka wrote: >>>>>>>> On 2018-04-16 17:29, Peter Maydell wrote: >>>>>>>>> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>> wrote: >>>>>>>>>> On 2018-04-01 23:17, Jan Kiszka wrote: >>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>>> >>>>>>>>>>> The spec does not justify clearing of any >>>>>>>>>>> E1000_ICR_OTHER_CAUSES when >>>>>>>>>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code >>>>>>>>>>> fixes the >>>>>>>>>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: >>>>>>>>>>> Avoid >>>>>>>>>>> receiver overrun interrupt bursts") and was worked around by >>>>>>>>>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>>> --- >>>>>>>>>>> >>>>>>>>>>> This resolves the issue I reported on February 18 ("e1000e: >>>>>>>>>>> MSI-X >>>>>>>>>>> problem with recent Linux drivers"). >>>>>>>>>>> >>>>>>>>>>> hw/net/e1000e_core.c | 4 ---- >>>>>>>>>>> 1 file changed, 4 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>>>>>>> index ecf9b15555..d38f025c0f 100644 >>>>>>>>>>> --- a/hw/net/e1000e_core.c >>>>>>>>>>> +++ b/hw/net/e1000e_core.c >>>>>>>>>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore >>>>>>>>>>> *core, uint32_t cause, uint32_t int_cfg) >>>>>>>>>>> >>>>>>>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>>>>>>> >>>>>>>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>>>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>>>>>> - } >>>>>>>>>>> - >>>>>>>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>>>>>>> >>>>>>>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>>>>>>> >>>>>>>>>> Ping for this - as well as >>>>>>>>>> https://patchwork.ozlabs.org/patch/895476. >>>>>>>>>> >>>>>>>>>> Given that q35 uses e1000e by default and many Linux kernel >>>>>>>>>> versions no >>>>>>>>>> longer work, this should likely go into upcoming and stable >>>>>>>>>> versions >>>>>>>>> I'd rather not put it into 2.12 at this point in the release >>>>>>>>> cycle unless it's a regression from 2.11, I think. >>>>>>>> Second ping - nothing hit the repo so far, nor did I receive >>>>>>>> feedback. >>>>>>>> >>>>>>> And another ping. For both. >>>>>>> >>>>>>> These days I had to help someone with a broken QEMU setup that >>>>>>> failed >>>>>>> installing from network. It turned out that "modprobe e1000e >>>>>>> IntMode=0" >>>>>>> was needed to workaround the issues my patches address. >>>>>> What about the IMS register? It is set just after. >>>>>> >>>>>> Looking at b38636b8372, can you test this patch? >>>>>> >>>>>> -- >8 -- >>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>> index c93c4661ed..a484b68a5a 100644 >>>>>> --- a/hw/net/e1000e_core.c >>>>>> +++ b/hw/net/e1000e_core.c >>>>>> @@ -2022,13 +2022,13 @@ e1000e_msix_notify_one(E1000ECore *core, >>>>>> uint32_t cause, uint32_t int_cfg) >>>>>> >>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>> >>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>> - } >>>>>> - >>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>> >>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>> + if (effective_eiac == E1000_ICR_OTHER) { >>>>>> + effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>> + } >>>>>> + >>>>>> core->mac[IMS] &= ~effective_eiac; >>>>>> } >>>>>> } >>>>>> >>>>> Before testing this: What would be the reasoning for this change? >>>> Not breaking the purpose of b38636b8372 :) >>> I disagree on this expansion of bit 31 ("other causes"). I see no >>> indication in the spec that setting this bit for autoclear has more >>> impact than on the very same bit itself. Therefore I'm asking for a >>> reasoning - based on the spec. >> Interrupt Cause Registers >> >> Other bits in this register are the legacy indication of >> interrupts as the MDIC complete, management and link status >> change. There is a specific Other Cause bit that is set if >> one of these bits are set, this bit can be mapped to a >> specific MSI-X interrupt message. >> >> I spent half an hour reading the relevant parts of the spec and can't >> figure out, so I'll let the authors of b38636b8372 to review your patch. >> >> Regards, >> >> Phil. > > Looking at EIAC part of the spec: > > Bits 24:20 in this register enables clearing of the corresponding bit in > ICR following > interrupt generation. When a bit is set, the corresponding bit in ICR > and in IMS is > automatically cleared following an interrupt. Thanks for looking at this. > It looks to me that only the other bit itself need to be cleared. So no need to set the other_causes bits, thus Jan patch is correct? Thanks, Phil.
On 2018年06月13日 10:26, Philippe Mathieu-Daudé wrote: > Hi Jason, > > On 06/12/2018 11:18 PM, Jason Wang wrote: >> On 2018年06月13日 03:00, Philippe Mathieu-Daudé wrote: >>> Cc'ing Jason who is also listed as co-maintainer: >>> >>> ./scripts/get_maintainer.pl -f hw/net/e1000e_core.c >>> Dmitry Fleytman <dmitry.fleytman@gmail.com> (maintainer:e1000e) >>> Jason Wang <jasowang@redhat.com> (odd fixer:Network devices) >>> >>> On 06/12/2018 03:43 PM, Jan Kiszka wrote: >>>> On 2018-06-12 20:38, Philippe Mathieu-Daudé wrote: >>>>> On 06/12/2018 03:30 PM, Jan Kiszka wrote: >>>>>> On 2018-06-12 20:11, Philippe Mathieu-Daudé wrote: >>>>>>> Hi Jan, >>>>>>> >>>>>>> On 06/12/2018 02:22 PM, Jan Kiszka wrote: >>>>>>>> On 2018-05-22 09:00, Jan Kiszka wrote: >>>>>>>>> On 2018-04-16 17:29, Peter Maydell wrote: >>>>>>>>>> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>> wrote: >>>>>>>>>>> On 2018-04-01 23:17, Jan Kiszka wrote: >>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>>>> >>>>>>>>>>>> The spec does not justify clearing of any >>>>>>>>>>>> E1000_ICR_OTHER_CAUSES when >>>>>>>>>>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code >>>>>>>>>>>> fixes the >>>>>>>>>>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: >>>>>>>>>>>> Avoid >>>>>>>>>>>> receiver overrun interrupt bursts") and was worked around by >>>>>>>>>>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>>>> --- >>>>>>>>>>>> >>>>>>>>>>>> This resolves the issue I reported on February 18 ("e1000e: >>>>>>>>>>>> MSI-X >>>>>>>>>>>> problem with recent Linux drivers"). >>>>>>>>>>>> >>>>>>>>>>>> hw/net/e1000e_core.c | 4 ---- >>>>>>>>>>>> 1 file changed, 4 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>>>>>>>> index ecf9b15555..d38f025c0f 100644 >>>>>>>>>>>> --- a/hw/net/e1000e_core.c >>>>>>>>>>>> +++ b/hw/net/e1000e_core.c >>>>>>>>>>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore >>>>>>>>>>>> *core, uint32_t cause, uint32_t int_cfg) >>>>>>>>>>>> >>>>>>>>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>>>>>>>> >>>>>>>>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>>>>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>>>>>>> - } >>>>>>>>>>>> - >>>>>>>>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>>>>>>>> >>>>>>>>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>>>>>>>> >>>>>>>>>>> Ping for this - as well as >>>>>>>>>>> https://patchwork.ozlabs.org/patch/895476. >>>>>>>>>>> >>>>>>>>>>> Given that q35 uses e1000e by default and many Linux kernel >>>>>>>>>>> versions no >>>>>>>>>>> longer work, this should likely go into upcoming and stable >>>>>>>>>>> versions >>>>>>>>>> I'd rather not put it into 2.12 at this point in the release >>>>>>>>>> cycle unless it's a regression from 2.11, I think. >>>>>>>>> Second ping - nothing hit the repo so far, nor did I receive >>>>>>>>> feedback. >>>>>>>>> >>>>>>>> And another ping. For both. >>>>>>>> >>>>>>>> These days I had to help someone with a broken QEMU setup that >>>>>>>> failed >>>>>>>> installing from network. It turned out that "modprobe e1000e >>>>>>>> IntMode=0" >>>>>>>> was needed to workaround the issues my patches address. >>>>>>> What about the IMS register? It is set just after. >>>>>>> >>>>>>> Looking at b38636b8372, can you test this patch? >>>>>>> >>>>>>> -- >8 -- >>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>>> index c93c4661ed..a484b68a5a 100644 >>>>>>> --- a/hw/net/e1000e_core.c >>>>>>> +++ b/hw/net/e1000e_core.c >>>>>>> @@ -2022,13 +2022,13 @@ e1000e_msix_notify_one(E1000ECore *core, >>>>>>> uint32_t cause, uint32_t int_cfg) >>>>>>> >>>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>>> >>>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>> - } >>>>>>> - >>>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>>> >>>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>>> + if (effective_eiac == E1000_ICR_OTHER) { >>>>>>> + effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>> + } >>>>>>> + >>>>>>> core->mac[IMS] &= ~effective_eiac; >>>>>>> } >>>>>>> } >>>>>>> >>>>>> Before testing this: What would be the reasoning for this change? >>>>> Not breaking the purpose of b38636b8372 :) >>>> I disagree on this expansion of bit 31 ("other causes"). I see no >>>> indication in the spec that setting this bit for autoclear has more >>>> impact than on the very same bit itself. Therefore I'm asking for a >>>> reasoning - based on the spec. >>> Interrupt Cause Registers >>> >>> Other bits in this register are the legacy indication of >>> interrupts as the MDIC complete, management and link status >>> change. There is a specific Other Cause bit that is set if >>> one of these bits are set, this bit can be mapped to a >>> specific MSI-X interrupt message. >>> >>> I spent half an hour reading the relevant parts of the spec and can't >>> figure out, so I'll let the authors of b38636b8372 to review your patch. >>> >>> Regards, >>> >>> Phil. >> Looking at EIAC part of the spec: >> >> Bits 24:20 in this register enables clearing of the corresponding bit in >> ICR following >> interrupt generation. When a bit is set, the corresponding bit in ICR >> and in IMS is >> automatically cleared following an interrupt. > Thanks for looking at this. > >> It looks to me that only the other bit itself need to be cleared. > So no need to set the other_causes bits, thus Jan patch is correct? > > Thanks, > > Phil. > Yes, I think so. But I think we can wait for few days to see if Dmitry have come comments. Thanks
On 2018年06月13日 10:29, Jason Wang wrote: > > > On 2018年06月13日 10:26, Philippe Mathieu-Daudé wrote: >> Hi Jason, >> >> On 06/12/2018 11:18 PM, Jason Wang wrote: >>> On 2018年06月13日 03:00, Philippe Mathieu-Daudé wrote: >>>> Cc'ing Jason who is also listed as co-maintainer: >>>> >>>> ./scripts/get_maintainer.pl -f hw/net/e1000e_core.c >>>> Dmitry Fleytman <dmitry.fleytman@gmail.com> (maintainer:e1000e) >>>> Jason Wang <jasowang@redhat.com> (odd fixer:Network devices) >>>> >>>> On 06/12/2018 03:43 PM, Jan Kiszka wrote: >>>>> On 2018-06-12 20:38, Philippe Mathieu-Daudé wrote: >>>>>> On 06/12/2018 03:30 PM, Jan Kiszka wrote: >>>>>>> On 2018-06-12 20:11, Philippe Mathieu-Daudé wrote: >>>>>>>> Hi Jan, >>>>>>>> >>>>>>>> On 06/12/2018 02:22 PM, Jan Kiszka wrote: >>>>>>>>> On 2018-05-22 09:00, Jan Kiszka wrote: >>>>>>>>>> On 2018-04-16 17:29, Peter Maydell wrote: >>>>>>>>>>> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>>> wrote: >>>>>>>>>>>> On 2018-04-01 23:17, Jan Kiszka wrote: >>>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>>>>> >>>>>>>>>>>>> The spec does not justify clearing of any >>>>>>>>>>>>> E1000_ICR_OTHER_CAUSES when >>>>>>>>>>>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code >>>>>>>>>>>>> fixes the >>>>>>>>>>>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: >>>>>>>>>>>>> Avoid >>>>>>>>>>>>> receiver overrun interrupt bursts") and was worked around by >>>>>>>>>>>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>>>>> --- >>>>>>>>>>>>> >>>>>>>>>>>>> This resolves the issue I reported on February 18 ("e1000e: >>>>>>>>>>>>> MSI-X >>>>>>>>>>>>> problem with recent Linux drivers"). >>>>>>>>>>>>> >>>>>>>>>>>>> hw/net/e1000e_core.c | 4 ---- >>>>>>>>>>>>> 1 file changed, 4 deletions(-) >>>>>>>>>>>>> >>>>>>>>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>>>>>>>>> index ecf9b15555..d38f025c0f 100644 >>>>>>>>>>>>> --- a/hw/net/e1000e_core.c >>>>>>>>>>>>> +++ b/hw/net/e1000e_core.c >>>>>>>>>>>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore >>>>>>>>>>>>> *core, uint32_t cause, uint32_t int_cfg) >>>>>>>>>>>>> >>>>>>>>>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>>>>>>>>> >>>>>>>>>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>>>>>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>>>>>>>> - } >>>>>>>>>>>>> - >>>>>>>>>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>>>>>>>>> >>>>>>>>>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>>>>>>>>> >>>>>>>>>>>> Ping for this - as well as >>>>>>>>>>>> https://patchwork.ozlabs.org/patch/895476. >>>>>>>>>>>> >>>>>>>>>>>> Given that q35 uses e1000e by default and many Linux kernel >>>>>>>>>>>> versions no >>>>>>>>>>>> longer work, this should likely go into upcoming and stable >>>>>>>>>>>> versions >>>>>>>>>>> I'd rather not put it into 2.12 at this point in the release >>>>>>>>>>> cycle unless it's a regression from 2.11, I think. >>>>>>>>>> Second ping - nothing hit the repo so far, nor did I receive >>>>>>>>>> feedback. >>>>>>>>>> >>>>>>>>> And another ping. For both. >>>>>>>>> >>>>>>>>> These days I had to help someone with a broken QEMU setup that >>>>>>>>> failed >>>>>>>>> installing from network. It turned out that "modprobe e1000e >>>>>>>>> IntMode=0" >>>>>>>>> was needed to workaround the issues my patches address. >>>>>>>> What about the IMS register? It is set just after. >>>>>>>> >>>>>>>> Looking at b38636b8372, can you test this patch? >>>>>>>> >>>>>>>> -- >8 -- >>>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>>>> index c93c4661ed..a484b68a5a 100644 >>>>>>>> --- a/hw/net/e1000e_core.c >>>>>>>> +++ b/hw/net/e1000e_core.c >>>>>>>> @@ -2022,13 +2022,13 @@ e1000e_msix_notify_one(E1000ECore *core, >>>>>>>> uint32_t cause, uint32_t int_cfg) >>>>>>>> >>>>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>>>> >>>>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>>> - } >>>>>>>> - >>>>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>>>> >>>>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>>>> + if (effective_eiac == E1000_ICR_OTHER) { >>>>>>>> + effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>>> + } >>>>>>>> + >>>>>>>> core->mac[IMS] &= ~effective_eiac; >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>> Before testing this: What would be the reasoning for this change? >>>>>> Not breaking the purpose of b38636b8372 :) >>>>> I disagree on this expansion of bit 31 ("other causes"). I see no >>>>> indication in the spec that setting this bit for autoclear has more >>>>> impact than on the very same bit itself. Therefore I'm asking for a >>>>> reasoning - based on the spec. >>>> Interrupt Cause Registers >>>> >>>> Other bits in this register are the legacy indication of >>>> interrupts as the MDIC complete, management and link status >>>> change. There is a specific Other Cause bit that is set if >>>> one of these bits are set, this bit can be mapped to a >>>> specific MSI-X interrupt message. >>>> >>>> I spent half an hour reading the relevant parts of the spec and can't >>>> figure out, so I'll let the authors of b38636b8372 to review your >>>> patch. >>>> >>>> Regards, >>>> >>>> Phil. >>> Looking at EIAC part of the spec: >>> >>> Bits 24:20 in this register enables clearing of the corresponding >>> bit in >>> ICR following >>> interrupt generation. When a bit is set, the corresponding bit in ICR >>> and in IMS is >>> automatically cleared following an interrupt. >> Thanks for looking at this. >> >>> It looks to me that only the other bit itself need to be cleared. >> So no need to set the other_causes bits, thus Jan patch is correct? >> >> Thanks, >> >> Phil. >> > > Yes, I think so. But I think we can wait for few days to see if Dmitry > have come comments. > > Thanks > Ok, I applied this first. Thanks
On 2018-06-15 04:07, Jason Wang wrote: > > > On 2018年06月13日 10:29, Jason Wang wrote: >> >> >> On 2018年06月13日 10:26, Philippe Mathieu-Daudé wrote: >>> Hi Jason, >>> >>> On 06/12/2018 11:18 PM, Jason Wang wrote: >>>> On 2018年06月13日 03:00, Philippe Mathieu-Daudé wrote: >>>>> Cc'ing Jason who is also listed as co-maintainer: >>>>> >>>>> ./scripts/get_maintainer.pl -f hw/net/e1000e_core.c >>>>> Dmitry Fleytman <dmitry.fleytman@gmail.com> (maintainer:e1000e) >>>>> Jason Wang <jasowang@redhat.com> (odd fixer:Network devices) >>>>> >>>>> On 06/12/2018 03:43 PM, Jan Kiszka wrote: >>>>>> On 2018-06-12 20:38, Philippe Mathieu-Daudé wrote: >>>>>>> On 06/12/2018 03:30 PM, Jan Kiszka wrote: >>>>>>>> On 2018-06-12 20:11, Philippe Mathieu-Daudé wrote: >>>>>>>>> Hi Jan, >>>>>>>>> >>>>>>>>> On 06/12/2018 02:22 PM, Jan Kiszka wrote: >>>>>>>>>> On 2018-05-22 09:00, Jan Kiszka wrote: >>>>>>>>>>> On 2018-04-16 17:29, Peter Maydell wrote: >>>>>>>>>>>> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>>>> wrote: >>>>>>>>>>>>> On 2018-04-01 23:17, Jan Kiszka wrote: >>>>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>>>>>> >>>>>>>>>>>>>> The spec does not justify clearing of any >>>>>>>>>>>>>> E1000_ICR_OTHER_CAUSES when >>>>>>>>>>>>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code >>>>>>>>>>>>>> fixes the >>>>>>>>>>>>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e: >>>>>>>>>>>>>> Avoid >>>>>>>>>>>>>> receiver overrun interrupt bursts") and was worked around by >>>>>>>>>>>>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC"). >>>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>>>>>>>>> --- >>>>>>>>>>>>>> >>>>>>>>>>>>>> This resolves the issue I reported on February 18 ("e1000e: >>>>>>>>>>>>>> MSI-X >>>>>>>>>>>>>> problem with recent Linux drivers"). >>>>>>>>>>>>>> >>>>>>>>>>>>>> hw/net/e1000e_core.c | 4 ---- >>>>>>>>>>>>>> 1 file changed, 4 deletions(-) >>>>>>>>>>>>>> >>>>>>>>>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>>>>>>>>>> index ecf9b15555..d38f025c0f 100644 >>>>>>>>>>>>>> --- a/hw/net/e1000e_core.c >>>>>>>>>>>>>> +++ b/hw/net/e1000e_core.c >>>>>>>>>>>>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore >>>>>>>>>>>>>> *core, uint32_t cause, uint32_t int_cfg) >>>>>>>>>>>>>> >>>>>>>>>>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>>>>>>>>>> >>>>>>>>>>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>>>>>>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>>>>>>>>> - } >>>>>>>>>>>>>> - >>>>>>>>>>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>>>>>>>>>> >>>>>>>>>>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>>>>>>>>>> >>>>>>>>>>>>> Ping for this - as well as >>>>>>>>>>>>> https://patchwork.ozlabs.org/patch/895476. >>>>>>>>>>>>> >>>>>>>>>>>>> Given that q35 uses e1000e by default and many Linux kernel >>>>>>>>>>>>> versions no >>>>>>>>>>>>> longer work, this should likely go into upcoming and stable >>>>>>>>>>>>> versions >>>>>>>>>>>> I'd rather not put it into 2.12 at this point in the release >>>>>>>>>>>> cycle unless it's a regression from 2.11, I think. >>>>>>>>>>> Second ping - nothing hit the repo so far, nor did I receive >>>>>>>>>>> feedback. >>>>>>>>>>> >>>>>>>>>> And another ping. For both. >>>>>>>>>> >>>>>>>>>> These days I had to help someone with a broken QEMU setup that >>>>>>>>>> failed >>>>>>>>>> installing from network. It turned out that "modprobe e1000e >>>>>>>>>> IntMode=0" >>>>>>>>>> was needed to workaround the issues my patches address. >>>>>>>>> What about the IMS register? It is set just after. >>>>>>>>> >>>>>>>>> Looking at b38636b8372, can you test this patch? >>>>>>>>> >>>>>>>>> -- >8 -- >>>>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c >>>>>>>>> index c93c4661ed..a484b68a5a 100644 >>>>>>>>> --- a/hw/net/e1000e_core.c >>>>>>>>> +++ b/hw/net/e1000e_core.c >>>>>>>>> @@ -2022,13 +2022,13 @@ e1000e_msix_notify_one(E1000ECore *core, >>>>>>>>> uint32_t cause, uint32_t int_cfg) >>>>>>>>> >>>>>>>>> effective_eiac = core->mac[EIAC] & cause; >>>>>>>>> >>>>>>>>> - if (effective_eiac == E1000_ICR_OTHER) { >>>>>>>>> - effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>>>> - } >>>>>>>>> - >>>>>>>>> core->mac[ICR] &= ~effective_eiac; >>>>>>>>> >>>>>>>>> if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) { >>>>>>>>> + if (effective_eiac == E1000_ICR_OTHER) { >>>>>>>>> + effective_eiac |= E1000_ICR_OTHER_CAUSES; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> core->mac[IMS] &= ~effective_eiac; >>>>>>>>> } >>>>>>>>> } >>>>>>>>> >>>>>>>> Before testing this: What would be the reasoning for this change? >>>>>>> Not breaking the purpose of b38636b8372 :) >>>>>> I disagree on this expansion of bit 31 ("other causes"). I see no >>>>>> indication in the spec that setting this bit for autoclear has more >>>>>> impact than on the very same bit itself. Therefore I'm asking for a >>>>>> reasoning - based on the spec. >>>>> Interrupt Cause Registers >>>>> >>>>> Other bits in this register are the legacy indication of >>>>> interrupts as the MDIC complete, management and link status >>>>> change. There is a specific Other Cause bit that is set if >>>>> one of these bits are set, this bit can be mapped to a >>>>> specific MSI-X interrupt message. >>>>> >>>>> I spent half an hour reading the relevant parts of the spec and can't >>>>> figure out, so I'll let the authors of b38636b8372 to review your >>>>> patch. >>>>> >>>>> Regards, >>>>> >>>>> Phil. >>>> Looking at EIAC part of the spec: >>>> >>>> Bits 24:20 in this register enables clearing of the corresponding >>>> bit in >>>> ICR following >>>> interrupt generation. When a bit is set, the corresponding bit in ICR >>>> and in IMS is >>>> automatically cleared following an interrupt. >>> Thanks for looking at this. >>> >>>> It looks to me that only the other bit itself need to be cleared. >>> So no need to set the other_causes bits, thus Jan patch is correct? >>> >>> Thanks, >>> >>> Phil. >>> >> >> Yes, I think so. But I think we can wait for few days to see if Dmitry >> have come comments. >> >> Thanks >> > > Ok, I applied this first. Thanks. Did you already have a look at https://patchwork.ozlabs.org/patch/895476 as well? Jan
© 2016 - 2024 Red Hat, Inc.