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

Implement enclave memory management #436

Open
wants to merge 20 commits into
base: emm-dev
Choose a base branch
from

Conversation

ClawSeven
Copy link

No description provided.

@ClawSeven ClawSeven marked this pull request as draft June 9, 2023 11:14
@ClawSeven ClawSeven changed the base branch from v2.0.0-preview to emm-dev August 15, 2023 02:06
@ClawSeven ClawSeven marked this pull request as ready for review August 15, 2023 02:06
use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
use sgx_types::marker::ContiguousMemory;

pub struct ReentrantMutex<T: ?Sized> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest name change to SpinReentrantMutex

}
}

pub struct ReentrantMutexGuard<'a, T: 'a + ?Sized> {
Copy link
Contributor

Choose a reason for hiding this comment

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

pub struct SpinReentrantMutexGuard<'a, T: 'a + ?Sized> {
    lock: &'a SpinReentrantMutex<T>,
}

}

#[inline]
fn acquire_lock(&self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

fn acquire_lock(&self) {
     while self
            .lock
            .compare_exchange_weak(false, true, Ordering::Acquire, Ordering::Relaxed)
            .is_err()
    {
        while self.is_locked() {
            spin_loop();
        }
    }
}

Copy link
Author

Choose a reason for hiding this comment

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

Done

@ClawSeven ClawSeven force-pushed the emm-dev branch 4 times, most recently from e9588a9 to 60caea5 Compare August 29, 2023 09:35
use sgx_types::marker::ContiguousMemory;

bitflags! {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use impl_bitflags! instead of bitflags!.

Copy link
Author

Choose a reason for hiding this comment

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

Done

if #[cfg(not(any(feature = "sim", feature = "hyper")))] {
pub use hw::*;
} else {
pub use sw::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

sw mod is not defined

Copy link
Author

Choose a reason for hiding this comment

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

Done, sw is removed.

// Free block for allocating memory with exact size
#[repr(C)]
#[derive(Debug)]
struct BlockFree {
Copy link
Contributor

Choose a reason for hiding this comment

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

struct Payload {
    ptr: Option<NonNull<u8>>,
}

union BlockPtr {
    link: Link,
    payload: Payload,
}

struct Block {
    size: usize,
    ptr: BlockPtr
}

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Author

Choose a reason for hiding this comment

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

Here we use From trait to refactor code.

}

let mut cursor_next = cursor.peek_next();
while !cursor_next.is_null() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to traverse the linked list twice?

for block in &mut exact_blocks {
block.write(SinglyLinkedList::new(BlockFreeAda::new()));
}
unsafe { transmute(exact_blocks) }
Copy link
Contributor

Choose a reason for hiding this comment

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

unsafe { MaybeUninit::array_assume_init(exact_blocks)}

Copy link
Author

Choose a reason for hiding this comment

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

Done, nice suggestion!

};
}

#[repr(C)]
Copy link
Contributor

Choose a reason for hiding this comment

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

remove #[repr(C)]

Copy link
Author

Choose a reason for hiding this comment

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

Done


#[repr(C)]
#[derive(Debug)]
pub struct BitArray {
Copy link
Contributor

Choose a reason for hiding this comment

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

pub struct BitArray<'a> {
    bits: usize,
    data: Box<[u8], &'a dyn Allocator>,
}

Copy link
Author

Choose a reason for hiding this comment

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

Excellent! I wasn't previously aware of the unsafe impl<A> Allocator for &A implementation.

impl Ema {
/// Initialize Emanode with null eaccept map,
/// and start address must be page aligned
pub fn new(
Copy link
Contributor

Choose a reason for hiding this comment

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

impl Clone for Ema {
}

Copy link
Author

@ClawSeven ClawSeven Nov 30, 2023

Choose a reason for hiding this comment

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

Initially, I contemplated utilizing the Clone trait, but upon deeper analysis, I've come to realize that its semantics diverge from our implementation. The inherent implication of the default Clone is that it performs a "field-by-field clone using the default allocator." In contrast, within the context of our split() function, our objective is to "clone all fields, excluding the bitmap, utilizing the inner allocator." This distinction in behavior necessitates a more tailored approach to suit our specific cloning needs.

Copy link
Author

Choose a reason for hiding this comment

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

If we were to implement the Clone trait according to our specific semantics, there's a heightened risk that developers might inadvertently misuse the clone() method.

}
}

pub fn new_options(options: &EmaOptions) -> OsResult<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

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

new_options rename to new

Copy link
Author

Choose a reason for hiding this comment

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

Done

/// Split current bit array at specified position, return a new allocated bit array
/// corresponding to the bits at the range of [pos, end).
/// And the current bit array manages the bits at the range of [0, pos).
pub fn split(&mut self, pos: usize) -> OsResult<BitArray> {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a waste of memory

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it is a temporary but convenient solution.

@yangfh2004
Copy link

@ClawSeven
Hi, I notice two issues with your PR:

  1. your emm sample code cannot be built with BUILD_STD=cargo
  2. I merged your commits in this PR into the current v2.0.0-preview branch to support the latest rust toolchain as this fork branch, however, I see problems with httpreq sample code, it will have pointer alignment issues while doing ocall "u_getaddrinfo_ocall", the pointer address of the argument and the return value is misaligned, which will cause panic in the repo. It seems that your implementation has some conflicts with the recent Rust toolchain.
    Please take a look, thank you!

@volcano0dr
Copy link
Contributor

@ClawSeven Hi, I notice two issues with your PR:

  1. your emm sample code cannot be built with BUILD_STD=cargo
  2. I merged your commits in this PR into the current v2.0.0-preview branch to support the latest rust toolchain as this fork branch, however, I see problems with httpreq sample code, it will have pointer alignment issues while doing ocall "u_getaddrinfo_ocall", the pointer address of the argument and the return value is misaligned, which will cause panic in the repo. It seems that your implementation has some conflicts with the recent Rust toolchain.
    Please take a look, thank you!

Is upgrading Intel SGX SDK to 2.21 to enable EDMM?

@yangfh2004
Copy link

@ClawSeven Hi, I notice two issues with your PR:

  1. your emm sample code cannot be built with BUILD_STD=cargo
  2. I merged your commits in this PR into the current v2.0.0-preview branch to support the latest rust toolchain as this fork branch, however, I see problems with httpreq sample code, it will have pointer alignment issues while doing ocall "u_getaddrinfo_ocall", the pointer address of the argument and the return value is misaligned, which will cause panic in the repo. It seems that your implementation has some conflicts with the recent Rust toolchain.
    Please take a look, thank you!

Is upgrading Intel SGX SDK to 2.21 to enable EDMM?

Thanks a lot for your reply, we are using SGX2 on 3rd Gen Xeon Scalable Processor with MKTME, and we will need large dynamic heap allocations. We are now using v1.1.6 with Intel SGX SDK 2.21. We are trying to upgrade to v2.0.0 with support to the later Rust toolchain.

@yangfh2004
Copy link

yangfh2004 commented Feb 19, 2024

@ClawSeven Hi, I notice two issues with your PR:

  1. your emm sample code cannot be built with BUILD_STD=cargo
  2. I merged your commits in this PR into the current v2.0.0-preview branch to support the latest rust toolchain as this fork branch, however, I see problems with httpreq sample code, it will have pointer alignment issues while doing ocall "u_getaddrinfo_ocall", the pointer address of the argument and the return value is misaligned, which will cause panic in the repo. It seems that your implementation has some conflicts with the recent Rust toolchain.
    Please take a look, thank you!

The second issue has an easy fix, essentially, all problems are due to the alignment checks in dereference of raw pointers. We just need to use read_unaligned or write_unaligned

I also fixed the first issue. Rust_STD_Features in the Makefile needs to include thread.

@ClawSeven
Copy link
Author

@yangfh2004 , Hi, thanks for your two issue.

  1. Regarding the first issue, I have managed to replicate it and have confirmed that the Makefile for the EMM sample code indeed requires thread feature activation. I will address and resolve this promptly.

  2. As for the second issue, I am unable to reproduce the scenario you've described. In my environment, the httpreq sample code works well without any panic. Could you please provide the version of your toolchain so that I can further investigate this matter?

@ClawSeven
Copy link
Author

@yangfh2004 Btw, I have tested the httpreq with rustc 1.78.0-nightly (397937d81 2024-02-22). It also worked well.

@yangfh2004
Copy link

@ClawSeven Hi, our team is using your emm branch, which works OK. But we want to request some supports to the simulation mode (SW mode) so that we can build and run our enclave without sgx drivers. Do you have a chance to add SW supports to your branch? Thanks

@ClawSeven
Copy link
Author

@yangfh2004 Hello, I'm pleased to know that the EDMM feature has been integrated into your project. Unfortunately, the simulation mode (SW mode) for EDMM is not on our current roadmap, primarily because Intel does not offer support for EDMM in SW mode, and I am currently at full capacity, preventing me from undertaking the implementation of this feature.

However, conceptually speaking, I believe the SW mode could be relatively straightforward to implement using system calls like mmap and munmap.

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.

3 participants