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.

Sign up for GitHub

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

Jump to bottom

qemu handy cpu page size call proposal. #1433

Merged
merged 2 commits into from
Aug 23, 2023
Merged

qemu handy cpu page size call proposal. #1433

merged 2 commits into from
Aug 23, 2023

Conversation

devnexen
Copy link
Contributor

No description provided.

@domenukk
Copy link
Member

@andreafioraldi should take a look but a few thoughts from my side:

  • Usually rust getters are not prepended with get_
  • Maybe hardcoding the page size for usermode isn't a good idea, how about sysconf(_SC_PAGE_SIZE); or similar?
  • I found that a similar function exists but is commented out, here:

    LibAFL/libafl_qemu/src/emu.rs

    Line 878 in c31ca2c

    //#[must_use]

@andreafioraldi
Copy link
Member

Maybe hardcoding the page size for usermode isn't a good idea, how about sysconf(_SC_PAGE_SIZE); or similar?

It is for architectures with fixed page size, we don't support others like PPC atm anyway. Calling a qemu_sys function every time a pointer to page is done (e.g. every mem access for snapshots) is not a good idea, but we can insert a cfg based on target arch and return a constant on mainstream archs like x86 or arm.

Usually rust getters are not prepended with get_

Yes, @devnexen just page_size please

@domenukk
Copy link
Member

FWIW aarch64 uses both: Asahi Linux / M1 = 16384, other phones have 4k. IIRC Linux servers also experiment with 16k and 64k sized pages.

The request could alternatively be done a once_cell so we need to access it only.. once?

@domenukk
Copy link
Member

Of course it might be that 16k pages destroy all other assumptions inside qemu_user... :D

@devnexen devnexen force-pushed the qemu_page_size branch 2 times, most recently from aa1f818 to 56896dd Compare August 22, 2023 19:22
domenukk
domenukk reviewed Aug 23, 2023
View reviewed changes
libafl_qemu/src/emu.rs Outdated
@@ -14,6 +14,10 @@ use std::{
};
use std::{slice::from_raw_parts, str::from_utf8_unchecked};

use once_cell::sync::OnceCell;
Copy link
Member

Choose a reason for hiding this comment

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

Std::cell::OnceCell -> no create needed anymore

domenukk
domenukk reviewed Aug 23, 2023
View reviewed changes
libafl_qemu/src/emu.rs Outdated
@@ -1,4 +1,4 @@
//! Expose QEMU user `LibAFL` C api to Rust
// Expose QEMU user `LibAFL` C api to Rust

Copy link
Member

Choose a reason for hiding this comment

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

Why not use this as doc string anymore? Cannot hurt?

@domenukk
Copy link
Member

@andreafioraldi okay like this?

@domenukk domenukk merged commit d338b30 into AFLplusplus:main Aug 23, 2023
@andreafioraldi
Copy link
Member

@andreafioraldi okay like this?

Not it was not, don't merge qemu PRs...

Gonna revert this, the page_size method is useful, but SNAPSHOT_PAGE_SIZE can remain a constant

andreafioraldi added a commit that referenced this pull request Aug 24, 2023
@andreafioraldi
Revert "qemu: add cpu page_size call (#1433)"
83e7358 
This reverts commit d338b30.
@andreafioraldi andreafioraldi mentioned this pull request Aug 24, 2023
Merged
@devnexen
Copy link
Contributor Author

@andreafioraldi okay like this?

Not it was not, don't merge qemu PRs...

Gonna revert this, the page_size method is useful, but SNAPSHOT_PAGE_SIZE can remain a constant

This is different from the constant in snapshot tough.

@andreafioraldi
Copy link
Member

Yes, but it needs to use another variable. In addition, the purpose of calling qemu_target_page_size in system mode is that there are architectures in qemu that can change page size during the execution of the guest, so it can't be stored in a once cell. Gonna make page_size simply return a call to qemu_target_page_size every time.

@devnexen
Copy link
Contributor Author

oh ok it looks more a fix than a revert. cheers.

@domenukk
Copy link
Member

Sorrey :)

andreafioraldi added a commit that referenced this pull request Aug 25, 2023
@andreafioraldi
qemu: Fix cpu page size function for full-system (#1452)
04c8d52 
* Revert "qemu: add cpu page_size call (#1433)"

This reverts commit d338b30.

* Reintroduce page_size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domenukk domenukk domenukk left review comments

@andreafioraldi andreafioraldi Awaiting requested review from andreafioraldi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@devnexen @domenukk @andreafioraldi