Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set qemu cpu option from ELF attribute #1233

Merged
merged 3 commits into from
May 13, 2023
Merged

Set qemu cpu option from ELF attribute #1233

merged 3 commits into from
May 13, 2023

Conversation

kito-cheng
Copy link
Collaborator

This could help multi-lib testing, but the price is slightly increase the testing time since it will need to extract ELF attribute from binary before running qemu.

But I think the cost is acceptable compare to make build system more complicate, and actually we already use this approach in our internal stuffs for years.

Fix #1206.

@kito-cheng
Copy link
Collaborator Author

@vineetgarc

pos64 = val.find("rv64")
# MAGIC WORKAROUND
# Some version of pyelftools has issue for parsing
# Tag number =5, it will wrongly parse it become
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space

@@ -26,6 +29,7 @@ def parse_opt(argv):
parser = argparse.ArgumentParser()
parser.add_argument('-march', '--with-arch', type=str, dest='march')
parser.add_argument('-selftest', action='store_true')
parser.add_argument('--with-elf', type=str)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not --get-riscv-tag?

scripts/wrapper/qemu/riscv64-unknown-linux-gnu-run Outdated Show resolved Hide resolved
@vineetgarc
Copy link

vineetgarc commented Apr 20, 2023

This could help multi-lib testing, but the price is slightly increase the testing time since it will need to extract ELF attribute from binary before running qemu.

But I think the cost is acceptable compare to make build system more complicate, and actually we already use this approach in our internal stuffs for years.

Fix #1206.

I'm still a bit wary of cost (delay) considering qemu runs are not currently parallelized.
I was thinking that a non-multilib can benefit from this change (as in rely on elf attribute) but the multilib cases are simple (at least today) and only need rv64 / rv32 for QEMU_CPU, nothing fancier (most other things are enabled by default in qemu anyways).

And multilib is what takes forover so we can help speed it up by avoiding all this (using additional makefile foo/bar) and with equivalent of following in run script.

-QEMU_CPU=${QEMU_CPU} qemu-riscv$xlen -r 5.10 "${qemu_args[@]}" -L ${RISC_V_SYSROOT} "$@"
+QEMU_CPU=rv$xlen qemu-riscv$xlen -r 5.10 "${qemu_args[@]}" -L ${RISC_V_SYSROOT} "$@"

@cmuellner
Copy link
Collaborator

cmuellner commented Apr 20, 2023

I'm still a bit wary of cost (delay) considering qemu runs are not currently parallelized.

make -j$(nproc) report SIM=qemu runs a QEMU process on each core here.

I was thinking of proposing rv$XLEN as well, because QEMU enables many extensions by default.
However, this breaks for e.g. vendor extensions (and all standard extensions that are not enabled), which is certainly not what we want.

@kito-cheng
Copy link
Collaborator Author

This could help multi-lib testing, but the price is slightly increase the testing time since it will need to extract ELF attribute from binary before running qemu.
But I think the cost is acceptable compare to make build system more complicate, and actually we already use this approach in our internal stuffs for years.
Fix #1206.

I'm still a bit wary of cost (delay) considering qemu runs are not currently parallelized. I was thinking that a non-multilib can benefit from this change (as in rely on elf attribute) but the multilib cases are simple (at least today) and only need rv64 / rv32 for QEMU_CPU, nothing fancier (most other things are enabled by default in qemu anyways).

Not sure what yo mean about not parallelized? Gcc test suite should be parallelized just by -j I think?

And multilib is what takes forover so we can help speed it up by avoiding all this (using additional makefile foo/bar) and with equivalent of following in run script.

-QEMU_CPU=${QEMU_CPU} qemu-riscv$xlen -r 5.10 "${qemu_args[@]}" -L ${RISC_V_SYSROOT} "$@"
+QEMU_CPU=rv$xlen qemu-riscv$xlen -r 5.10 "${qemu_args[@]}" -L ${RISC_V_SYSROOT} "$@"

That seems can’t handle those extension not enabled by default?

@vineetgarc
Copy link

I'm still a bit wary of cost (delay) considering qemu runs are not currently parallelized.

make -j$(nproc) report SIM=qemu runs a QEMU process on each core here.

Ah I'm stupid. I was not passing -j to test run :-(

I was thinking of proposing rv$XLEN as well, because QEMU enables many extensions by default. However, this breaks for e.g. vendor extensions (and all standard extensions that are not enabled), which is certainly not what we want.

The point is those vendor extensions are not run as part of standard multilib runs. But anyways elf parsing feels more future proof so yes fine by me.

This could help multi-lib testing, but the price is slightly increase
the testing time since it will need to extract ELF attribute from binary
before running qemu.

But I think the cost is acceptable compare to make build system more
complicate, and actually we already use this approach in our internal stuffs
for years.
@kito-cheng kito-cheng merged commit ac4e8dd into master May 13, 2023
@kito-cheng kito-cheng deleted the attr-qemu branch May 13, 2023 06:21
kito-cheng added a commit that referenced this pull request May 19, 2023
We use some non-standard python package in #1233, so we must make sure
it available before run the test.

Fix #1252
kito-cheng added a commit that referenced this pull request May 19, 2023
We use some non-standard python package in #1233, so we must make sure
it available before run the test.

Fix #1252
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multilib toolchain dejagnu testing broken chapter 2
3 participants