Repository navigation
IPC4 fuzzing support - #7531
IPC4 fuzzing support#7531
Conversation
cujomalainey
left a comment
cujomalainey
left a comment
There was a problem hiding this comment.
mostly pretty good, just mostly patch cleanup
There was a problem hiding this comment.
this work should be consolidated with the commit that added it
|
Something must be weird with the state, you pushed the PR 1h ago, the commits 2 days ago, and my review was 1min ago but yet is marked as "outdated".... |
Can you please try something like this? (UNTESTED) I didn't expect any shell script change, pretty sure I anticipated this PR. Is there any? --- a/.github/workflows/ipc_fuzzer.yml
+++ b/.github/workflows/ipc_fuzzer.yml
@@ -42,15 +42,14 @@ jobs:
fuzz-seconds: 5
- # TODO, to add IPC4 support fix compilation of:
- #
- # ./scripts/fuzz.sh -t 1 -- -DCONFIG_IPC_MAJOR_4=y
- #
- # ... or of some other _IPC4_ -DOVERLAY_CONFIG=
- #
- # Then use a simple IPC3/IPC4 matrix like the one in zephyr.yml
simple-IPC3-fuzz_sh:
runs-on: ubuntu-22.04
+ strategy:
+ fail-fast: false
+ matrix:
+ # Keep these names short due to questionable Github UI choices
+ IPC: [ IPC3, IPC4 ]
+
steps:
- name: add i386 arch
run: |
@@ -78,4 +77,8 @@ jobs:
cd workspace
clang --verbose
set -x
- sof/scripts/fuzz.sh -o _.log -t 300
+ case '${{ matrix.IPC }}' in
+ IPC3) cmake_arg='-DCONFIG_IPC_MAJOR_3=y'
+ IPC4) cmake_arg='-DCONFIG_IPC_MAJOR_4=y'
+ esac
+ sof/scripts/fuzz.sh -o _.log -t 300 -- "$cmake_arg" |
__packed and __aligned are defined in compiler_attributes.h, which wasn't included, and when building for Zephyr that needs to include the zephyr/toolchain.h header which is the original source of those macros. Transitive headers were saving this, but building for posix/fuzzing works differently. Signed-off-by: Andy Ross <andyross@google.com>
CLK_MAX_CPU_HZ is defined in platform/lib/clk.h, unmasked by posix work that lacks the same transitive headers. Signed-off-by: Andy Ross <andyross@google.com>
This calls mailbox APIs when building for IPC4, but was missing the header Signed-off-by: Andy Ross <andyross@google.com>
|
Update, merge two patches which were vestigial versions of the same fix (this sat around in my tree for a while). Clean up the checkpatch warnings. Also filed bug #7549 to track the platform-dependent code discussion. I'm too timid to muck with CI files in the same PR, so no change to the workflow yaml here. But I did validate that fuzz.sh works in place if you just pass the kconfig (sorry, didn't realize you'd left the " |
|
Remaining checkpatch issue is a false positive. It's not possible to non-trivially use memcpy_s in that circumstance. The |
cujomalainey
left a comment
cujomalainey
left a comment
There was a problem hiding this comment.
just my request about the TODO, rest looks good
This was trying to read hardware registers, which obviously doesn't
work on other platforms. Stub with zeros for now.
Longer term: this whole message ("basefw_mem_state_info") might want
some rework. In addition to being heavily hardware-specific, it seems
like a comparatively large block without self-describing data
(e.g. id/val pairs), nor with a struct definition to specify it, nor
with any documentation that I can find.
Signed-off-by: Andy Ross <andyross@google.com>
Thanks for manually testing this, good enough for now. |
There was a problem hiding this comment.
No, it's to handle the case below where it may or may not be initialized to IMR depending on RIMAGE_MANIFEST.
There was a problem hiding this comment.
Yes, if RIMAGE_MANIFEST is defined, then desc is assigned immediately and there's no need for initialisation. If it isn't defined, then the function returns immediately and desc is never used, so there's again no need to initialise. BTW, if it isn't defined and there's a return statement right at the beginning, then compilers / analysers might complain about a whole bunch of unused variables and unreachable code.
The component model for IPC4 seems to be based on an array of manifest headers at the start of the firmware image. Those are rimage data, and not all platforms (in particular the fuzzer) are packing their output with rimage. Just stub this for now. In the source tree as it exists, there don't seem to be any extra components to build, nor is there any support I can see at the linker/rimage level to get them included anyway. Signed-off-by: Andy Ross <andyross@google.com>
Add new APIs needed for CONFIG_IPC_MAJOR_4=y builds. Note that many of these are fairly Intel-specific, meaning they probably don't belong in the platform layer. But in this case (fuzzing) it's probably worthwhile to enable as much code as possible vs. refactoring for purity. Future IPC4 hardware platforms will need to do that refactoring for us anyway. Signed-off-by: Andy Ross <andyross@google.com>
Don't use the IPC3 protocol whiteboxing for IPC4, the formats are different. Also IPC4 seems to send a LOT of zero-length empty replies. Those are causing spurious errors (memcmp_s fails when you pass it a null destination pointer, even when the length is zero), where the Intel IPC driver will just drop them. Suppress handling to match behavior, though this is probably a buglet worth investigating. Signed-off-by: Andy Ross <andyross@google.com>
Fuzzing produces (by design) an endless flood of protocol errors. Quiet the top level ones when in use, because otherwise the log output takes up most of the CPU time available that we'd rather spend on fuzzing. Same trick from IPC3. Signed-off-by: Andy Ross <andyross@google.com>
|
One known fail in https://sof-ci.01.org/sofpr/PR7531/build6911/devicetest/index.html , otherwise looks good. In review comments, @andyross @lyakh there's the one superfluous variable init that is not addressed. I do think this is valid, but I'm inclined to proceed with merge as the rest of the PR is good and we have tests passing. |
Hot on the heels of PR thesofproject#7531. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
|
Small #7579 adds IPC4 fuzzing to Github Actions, please review. |
Hot on the heels of PR #7531. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Support for IPC4 protocol fuzzing with the existing rig. Just select CONFIG_IPC_MAJOR_4=y and it will adjust as needed. It seems to have pretty decent coverage as far as I've been able to check manually, has run on my system for about two hours now (running 64 instances in parallel), and has uncovered one bug (in #7528 -- that will obviously want to merge before this does).
@marc-hb I didn't adjust the existing fuzz.sh script in this PR, it's still doing IPC3 by default. There are lots of ways to handle this: you could run the fuzzer twice in CI for each protocol, we could add new "posix" platform variants for each, etc... Let me know what you'd prefer.