i#4014 dr$sim phys: Check privileges for pagemap #5531
Conversation
Solves issues with requiring precise placement of physical markers prior to their corresponding regular virtual address entry (ifetch entries being inserted between the marker and memref by raw2trace; delayed branches needing to carry their marker; page-spanning extra markers needing to land in the right place) by switching to a scheme where markers come in pairs with a physical and a virtual. Such a pair has flexibility on where it can appear in the trace so long as it is prior to the regular entry with that virtual address. This eliminates all work and complexity for raw2trace to move the markers around. Solves issues with capacity where the tracer runs out of pre-allocated space inside the main buffer for marker-heavy output by switching to use a separately-allocated-and-emitted "v2p" buffer for the physical,virtual pairs. This is made possible by the flexible location. Two complexities of this scheme are solved: the buffer is page-aligned and heap-allocated for buffer handoff; the initial thread header is added to the v2p buffer and skipped in the regular buffer to avoid the pairs appearing before the top-level metadata. Refactors header insertion and buffer output into separate functions out of the large memtrace() function. Multi-output was tested on SPEC2K6 gzip which hits the 1-page limit 4 times during its run with a scaled-down test input set workload. The view tool, invariant checker, release notes, and regression test are updated. The invariant checker now also checks that the physical and virtual pair member share their bottom bits. are identical. Includes a significant fix for physical offline tracing: Elision and address displacement optimizations are disabled for physical addresses as the final address is required at tracing time. Issue: #4014
Since the kernel lets an unprivileged user read the pagemap file but supplies 0 values for the physical pages (and 0 is a possible legitimate value), we add an explicit check for privileges and fail up front if physical addresses are requested but not available. This is a change in behavior where before execution would continue with a warning. Adds a test of missing privileges. Fixes a sentinel issue where a 0 physical page was stored as 1 in the table: but 1 is a possible valid number as well, so -1 is used instead. Issue: #4014
|
Unfortunately Github doesn't support chained PR's very well...this is on top of PR #5528. It doesn't seem to run the tests. But I think it is still worth starting the review process as there is another PR to be built on top of this. |
|
Since #5528 is ready for merge now, can we merge it first and resolve the conflicts? |
Isn't the diff shown the diff vs the other one? I assumed whatever conflicts are purely for it running the tests or something. |
I merged it in. I wish Github had better support here to expedite reviewing chained PR's. |
|
Will wait for review before pushing any changes for the test failures to avoid changing the code during review. Not sure I understand why the sudo tests would fail now but not before, since the sudo tests failing in these runs didn't really change in this PR. |
|
This PR adds sudo to the online phys tests, which wasn't there before (only for offline which is x86-restricted) -- which is why it fails on Jenkins which does not have password-less sudo and fails like this: How should this best be handled? Should Jenkins set an env var or sthg saying it doesn't have pw-less sudo, and our cmake code checks that before enabling sudo tests? |
I have runsuite only enabling sudo tests for CI_TRIGGER being set: so just for Github Actions. |
Since the kernel lets an unprivileged user read the pagemap file but
supplies 0 values for the physical pages (and 0 is a possible
legitimate value), we add an explicit check for privileges and fail up
front if physical addresses are requested but not available. This is
a change in behavior where before execution would continue with a
warning.
Adds a test of missing privileges.
Fixes a sentinel issue where a 0 physical page was stored as 1 in the
table: but 1 is a possible valid number as well, so -1 is used
instead.
Issue: #4014