-
Notifications
You must be signed in to change notification settings - Fork 49
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
[Draft] cc rebase #1310
base: main
Are you sure you want to change the base?
[Draft] cc rebase #1310
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall, I'm seeing very little (if any) documentations. confidential computing is a heavy feature and introduces some breaking changes in the APIs. Please add documentations otherwise the affected code will become obscure to those who don't work in this area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another global comment is that there are some heavy code repetition. Some could be optimized.
This series of changes aim to create a common base for running different CC architectures, and the updates will involve: 1)private and shared memory isolation. 2)hypercalls update for using shared memory. 3)vm initialization flow update. 4)use one more vcpu to handle io and scheduling inside kernel.
Now cc_all and cc_debug can compile with cc feature.
In normal vm, there is the common heap and an additional IO heap if enabled EnableIOBuf config. In cc, there are 4 heaps: 1)shared heap: qkernel stores in it the shared data(IObuffer, sharedspace etc.), then qvisor uses it after vm is launched. 2)IO heap: considered as shared heap here, used if enabled EnableIOBuf config. 3)guest private heap: qvisor stores necessary data during initialization (initial pagetable, gdt, kernel etc.) then qkernel uses it as the default heap. 4)host init heap: an additional heap is used by the qvisor for storing data before launching vm. It will not be used after vm launched.
Now qvisor will switch to different vm creation flow based on the CCMode config. If any cc mode is enabled, sharedspace will be initialized after vm is launched. Additionally, map the host initial heap to kvm if cc is compiled but not enabled.
Instead of setting registers to pass parameters, in cc mode, a sharapara page is used. This method only works when feature cc is compiled and CCMode config is not None.
Add a data structure p2pmap in the mappable which stores the mapping of private and shared memory, writeback if map shared and not readonly when unmapping the data. Besides, need to sync updates when fsync and writing with fd.
Instead of using cstring for passing parameters in call, in cc, sharestring is used to allocate string in shared memory directly.
Add a struct taskWrapper, has minimal host required data(ready, queueid and taskAddr), which should be allocated in the shared memory and read by the host.
1)Store Timer in shared heap, since the value of the btree in timestore is a reference 2)Make timekeepr shared, the timekeeper is initialized and stored in the sharespace, and cloned when the guest creates a TimeKeeperClock. The internal timekeeper is used by host. ProcessOnce() ->TIMER_STORE.Trigger() -> get the timer by GetFirst() then timer.Fire() ->Timeout() -> Now() 3)In the same Timeout function, the listener of the Timer is triggered to update the vdso, the update by host is banned now. 4)Store FdWaitInfo in shared heap, it may be set by the kernel and check by host in ProcessOnce() -> FD_NOTIFIER.HostEpollWait() -> FdNotify()
Now CCMode::NormalEmu can be set in the config to enable unidentical mapping. Private memory is mapped 30gb higher on the host.
@@ -368,6 +370,149 @@ impl Thread { | |||
return Ok(nt); | |||
} | |||
|
|||
#[cfg(feature = "cc")] | |||
pub fn Clone(&self, opts: &CloneOptions, stackAddr: u64, taskWrapperAddr: u64) -> Result<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the taskWrapper
approach also requires you to repeat so code so a lot, even if the code is doing exactly the same.
if SHARESPACE.hiberMgr.ContainersPage(addr) { | ||
let _ret = HostSpace::SwapInPage(addr); | ||
} | ||
|
||
#[cfg(feature = "cc")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding swapping pages:
IIRC swapping is not supported when cc is enabled:
- since it's possible to swap in/out page before cc is enabled (the second condition), you need to make sure all pages are swapped back when you enable cc, otherwise it's not possible to swap in after that point.
- could you make sure SwapOutPage causes a panic with cc enabled?
pub submitq: QMutex<VecDeque<UringEntry>>, | ||
#[cfg(feature = "cc")] | ||
pub submitq: ArrayQueue<UringEntry>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not Mutex in CC? And why ArrayQueue instead of VecDeque?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.ArrayQueue is from Crossbeam crate, which is designed for lock free concurrency.
2.ArrayQueue instead of VecDeque: avoid dynamic growth, arrayqueue will return an error when the queue is full.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds promising. Perhaps consider doing the same also for non-cc? (in the future, not here)
} | ||
|
||
impl CCMode { | ||
pub fn from(value: u64) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why mapping from numeric values? Aren't this enum user defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used for reusing the sharedspace u64 passed to the kernel as the CCMode when cc is enabled.
@@ -13,21 +13,40 @@ | |||
// limitations under the License. | |||
|
|||
use super::super::common::*; | |||
#[cfg(not(feature = "cc"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, this is pretty noisy, perhaps you could simply just import them, the current config won't warn unused imports anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will warn unused imports...
fromCtx.mm.VcpuLeave(); | ||
toCtx.mm.VcpuEnter(); | ||
|
||
assert!(from.task_wrapper_addr > 0, "switch from.task_wrapper_addr > 0"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On log messages, please try to use plain human language, not code.
To enable cc mode, compiling with make cc_all/ cc_release/ cc_debug.
Set CCMode in config.json as Normal or NormalEmu . Normal uses identical mapping and NormalEmu uses unidentical mapping.