Closed n0toose closed 2 months ago
Attention: Patch coverage is 71.73913%
with 13 lines
in your changes missing coverage. Please review.
Project coverage is 67.03%. Comparing base (
f8b1426
) to head (11f5a44
).
Files | Patch % | Lines |
---|---|---|
src/vm.rs | 71.42% | 12 Missing :warning: |
src/arch/mod.rs | 0.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Oops, test fails because I forgot to move the proof-of-concept sysinfo
function into the aarch64 variant. It should work locally on x86_64
.
I'm not sure if "Draft" PRs in the context of hermit-os are interpreted as "Please don't look at this / review this PR (yet)" or "This needs some extra work before it can be merged" (as I've seen both in the context of OSS projects before). In my case, this PR needs some extra work and could benefit from feedback. I have the following question before finalizing this PR:
How do we deal with sysinfo on x86_64? (Do we use sysinfo instead of CPUID, or should it be exclusively an aarch64 thing? If yes, do we throw out the CPUID code?)
This is not urgent or a call for attention - just a clarification of my intentions and why the PR has stalled. I'll work on something else in the meantime :)
- [ ] Does this work on aarch64 (Raspberry Pi)?
I assume it does. Looks like your approach is fine.
- [ ] How do we deal with sysinfo on x86_64? (Do we use sysinfo instead of CPUID, or should it be exclusively an
aarch64
thing? If yes, do we throw out the CPUID code?)
I like the current approach you've taken
- [ ] Investigate potential conflict with https://github.com/hermit-os/uhyve/pull/688
I don't see a conflict here.
- [ ]
FrequencyDetectionFailed
duplication?
True. You should create an error outside of the arch
mods and import it in arch::x86...
- [ ] Better design, so that we don't have to convert u64 into u32?
- Possibly negligible, the current test suite does not expect frequencies over
As written above, this is fine as is
Apparently, the example of "would it brake on macos-latest" was not a hypothetical - the test would fail in that case, because the frequency value is not OK (see action). This should have been fixed here and perhaps warrants some further investigation before merging: https://github.com/GuillaumeGomez/sysinfo/pull/1023
https://github.com/hermit-os/uhyve/pull/701#discussion_r1662674228
This is a proof-of-concept, there is some more testing necessary and some questions as to how this feature should be implemented: https://github.com/hermit-os/uhyve/issues/690
TODOs
aarch64
thing? If yes, do we throw out the CPUID code?)FrequencyDetectionFailed
duplication?I don't think that there's a "serious" case where it would return a zero, so this would technically turn the alternatives (e.g. CPUID) into dead code 99.99% of the time.
Nice-to-haves
sysinfo
crate adds a new dependency onwindows
crates. Perhaps a feature upstream that would remove that dependency would be great, as Windows is not a target.Add detect_freq_from_sysinfo()
Shows a real-time value of the base frequency, similarly to /proc/cpuinfo on Linux.
Using sysinfo as an alternative to the CPUID-provided value may be useful for some systems where the CPUID feature is not available. A potential flaw of this implementation is that the frequency is passed once to before the initialization of the kernel and cannot be changed later.
If the host system is in "power-saving mode" (e.g. in laptops) and then switches to "performance mode", the application will not have any knowledge of this change. IIRC, Jonathan thinks this is OK for now.
Use sysinfo by default on x86_64 instead of CPUID.
Currently for demonstration reasons, we may want to amend this in a later revision.
Fixes: https://github.com/hermit-os/uhyve/issues/690