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

mm: Add unit tests for kernel mapping, guestmem ops, address checks #170

Closed
wants to merge 3 commits into from

Conversation

Zildj1an
Copy link
Contributor

Add unit tests to evaluate offline correct behavior of memory components: kernel mappings (address_space.rs), read and write operations (guestmem.rs) and valid physical address checking (memory.rs).

Copy link
Member

@00xc 00xc left a comment

Choose a reason for hiding this comment

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

At least one test is failing inside the SVSM, so we need that fixed at the minimum. I would also really want the Miri addition to go in, as I'm running Miri tests nightly.

src/mm/memory.rs Outdated Show resolved Hide resolved
src/mm/guestmem.rs Outdated Show resolved Hide resolved
src/mm/guestmem.rs Outdated Show resolved Hide resolved
src/mm/guestmem.rs Outdated Show resolved Hide resolved
src/mm/guestmem.rs Outdated Show resolved Hide resolved
src/mm/address_space.rs Show resolved Hide resolved
@Zildj1an
Copy link
Contributor Author

At least one test is failing inside the SVSM, so we need that fixed at the minimum. I would also really want the Miri addition to go in, as I'm running Miri tests nightly.

I foresee the complexity of these and future tests growing considerably if we want all to be able to run inside the SVSM. My intention when I wrote and added these tests was to run them offline. For example, I need to either implement new functions that use KERNEL_MAPPING_TEST instead, or I have to consider the ramifications of using KERNEL_MAPPING uninitialized, etc. So I'm wondering, Is there a way we can leave some tests for offline testing only?

@00xc
Copy link
Member

00xc commented Nov 30, 2023

I foresee the complexity of these and future tests growing considerably if we want all to be able to run inside the SVSM. My intention when I wrote and added these tests was to run them offline. For example, I need to either implement new functions that use KERNEL_MAPPING_TEST instead, or I have to consider the ramifications of using KERNEL_MAPPING uninitialized, etc. So I'm wondering, Is there a way we can leave some tests for offline testing only?

Yes, there are already some tests that are ignored inside the SVSM, for example in mm/alloc.rs. You can do that with:

#[test]
#[cfg_attr(test_in_svsm, ignore = "<REASON>")]
fn my_test() {}

I think you can also omit the reason.

Add unit tests to evaluate offline correct behavior of valid physical
address checking (memory.rs).

Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
Add unit tests to evaluate offline correct behavior of read and write
operations (guestmem.rs).

Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
Add unit tests to evaluate offline correct behavior of memory components:
kernel mappings (address_space.rs).

Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
@Zildj1an
Copy link
Contributor Author

I foresee the complexity of these and future tests growing considerably if we want all to be able to run inside the SVSM. My intention when I wrote and added these tests was to run them offline. For example, I need to either implement new functions that use KERNEL_MAPPING_TEST instead, or I have to consider the ramifications of using KERNEL_MAPPING uninitialized, etc. So I'm wondering, Is there a way we can leave some tests for offline testing only?

Yes, there are already some tests that are ignored inside the SVSM, for example in mm/alloc.rs. You can do that with:

#[test]
#[cfg_attr(test_in_svsm, ignore = "<REASON>")]
fn my_test() {}

I think you can also omit the reason.

Thanks, I'm doing this, opening new clean PR

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