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

Support per-device arguments and device factory reuse #1655

Merged
merged 1 commit into from
May 1, 2024

Conversation

liuyu81
Copy link

@liuyu81 liuyu81 commented Apr 26, 2024

Device arguments (or sargs) is a new feature introduced in #1522. It allows passing extra sargs from CLI --device=factory[,sargs] option down to MMIO device plugins.

While the use case is definitely sound, previous implementation does not really support instantiating multiple devices from the same factory with different sets of arguments. Another limitation (of previous implementation), is that -- due to the injection of per-device sargs globally into device_factory_t -- it prohibits device factory resue across multiple sim_t instances.

As proposed in #1652, we have created this PR to address the above limitations. The fix is three fold,

  • we removed sargs from device_factory_t, and introduced a new type alias device_factory_sargs_t to capture <device_factory_t *, sargs> pairs, this is used to instantiate sim_t instances;
  • we changed the signature of device_factory_t::generate_fdt / device_factory_t::parse_from_fdt to take on an extra sargs argument, for instantiating devices with per-device arguments;
  • we made device_factory_t const and potentially resuable across multiple sim_t instances.

Copy link
Collaborator

@jerryz123 jerryz123 left a comment

Choose a reason for hiding this comment

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

This overall looks good to me. This repo uses a rebase flow for PRs. Rebase your changes on master, instead of merging master into your feature branch.

riscv/clint.cc Outdated Show resolved Hide resolved
@liuyu81 liuyu81 force-pushed the master branch 2 times, most recently from fffff31 to cebc2b7 Compare April 30, 2024 01:29
As proposed in riscv-software-src#1652, we made the following changes to MMIO device (factory)
plugin API, to mitigate current limitations and facilitate factory reuse.

- removed `sargs` from `device_factory_t`, and introduced a new type alias
  `device_factory_sargs_t` to capture `<device_factory_t *, sargs>` pairs,
  this is used to instantiate sim_t instances;
- changed the signature of `device_factory_t::generate_fdt` and
  `device_factory_t::parse_from_fdt` to take on an extra `sargs` argument,
  for instantiating devices with per-device arguments;
- made `device_factory_t` const and potentially resuable across multiple
  `sim_t` instances.
@liuyu81
Copy link
Author

liuyu81 commented Apr 30, 2024

Thanks for the review. I have marked the sargs of clint/ns16550/plic as UNUSED, and rebased my changes on master.

@aswaterman
Copy link
Collaborator

@jerryz123 I don't have time to review this right now, but feel free to merge if you think it's good to go.

@jerryz123 jerryz123 merged commit 37b0dc0 into riscv-software-src:master May 1, 2024
3 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.

None yet

3 participants