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

Spike support for block device option #300

Merged
merged 4 commits into from
Apr 19, 2024
Merged

Spike support for block device option #300

merged 4 commits into from
Apr 19, 2024

Conversation

junsunchoi
Copy link
Contributor

Support for running spike with block device option when typing "marshal launch"

@junsunchoi junsunchoi requested a review from abejgonzalez April 18, 2024 23:30
wlutil/launch.py Outdated
raise ValueError("Spike does not support disk-based configurations")
riscv_lib_path = os.getenv('RISCV')
if not os.path.isfile(riscv_lib_path+'/lib/libspikedevices.so'):
raise ValueError("Spike does not support disk-based configurations without libspikedevices.so")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise ValueError("Spike does not support disk-based configurations without libspikedevices.so")
raise ValueError("Spike does not support disk-based configurations without libspikedevices.so installed by Chipyard")

wlutil/launch.py Outdated
Comment on lines 39 to 40
riscv_lib_path = os.getenv('RISCV')
if not os.path.isfile(riscv_lib_path+'/lib/libspikedevices.so'):
Copy link
Contributor

Choose a reason for hiding this comment

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

If 'RISCV' doesn't exist this returns None so the next line will break.

wlutil/launch.py Outdated

if 'spike' in config:
spikeBin = str(config['spike'])
else:
spikeBin = 'spike'

cmd = [spikeBin,
config.get('spike-args', ''),
spikeArgs, # config.get('spike-args', ''),
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. You still want to append the spike-args given by the config

' -p' + str(config['cpus']),
' -m' + str(int(config['mem'] / (1024*1024)))]

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Keep the space.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check that the docs don't need to be updated as well (maybe just grep around for spike and see what shows up)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you check the changes?

abejgonzalez
abejgonzalez previously approved these changes Apr 19, 2024
Copy link
Contributor

@abejgonzalez abejgonzalez left a comment

Choose a reason for hiding this comment

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

Small nits. Looks good though.

docs/source/internal/Build.rst Outdated Show resolved Hide resolved
docs/source/internal/Build.rst Outdated Show resolved Hide resolved
Co-authored-by: Abraham Gonzalez <[email protected]>
for Spike (that binary will boot on Qemu and FireSim as well). The build process
proceeds as follows:
Spike, Qemu, and FireSim. Spike now supports a disk. In order to use a disk,
``libspikedevices.so`` should be installed by Chipyard to the ``RISCV`` library path (i.e. ``$RISCV/lib`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
``libspikedevices.so`` should be installed by Chipyard to the ``RISCV`` library path (i.e. ``$RISCV/lib`).
``libspikedevices.so`` should be installed by Chipyard to the ``RISCV`` library path (i.e. ``$RISCV/lib``).

@junsunchoi junsunchoi merged commit 1a971fb into master Apr 19, 2024
8 checks passed
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.

2 participants