Removed the OutBHandler and MemAccessHandler abstractions and related implementations. #732
Conversation
simongdavies
added
the
kind/enhancement
simongdavies
requested review from
danbugs,
ludfjig,
dblnz,
devigned,
syntactically,
marosset and
jprendes
as code owners
simongdavies
changed the title
…ppers and to remove sync requirement Signed-off-by: Simon Davies <[email protected]>
Signed-off-by: Simon Davies <[email protected]>
Signed-off-by: Simon Davies <[email protected]>
Signed-off-by: Simon Davies <[email protected]>
Signed-off-by: Simon Davies <[email protected]>
…e new values to vm.initialise Signed-off-by: Simon Davies <[email protected]>
simongdavies
changed the title
There was a problem hiding this comment.
The solution that this PR implements is to remove the Sync constraint from the Hypervisor trait since its not possible to access a Sandbox concurrently from multiple threads.
That seems like a reasonable constraint, especially given the performance cliffs that we see with accessing a KVM partition from multiple threads (and how easy it seems to be for people to run into that). Is there a compelling reason not to make the Hypervisor trait non-Send as well, at least for the temporary future?
I do think that in general, we are going to need to be able to split up the actual hypervisor handle and the access to the host side of the memory mapping, in order to support the async use case, where the hypervisor handler(s) might (at least sometimes) need to be in a different thread running the guest vcpu whilst the host is also accessing memory buffers for async parameter passing. However, that is something we can address later.
There is probably some code in shared_mem that on the face of it does not seem necessary without multi threaded access but in the future when async guest io is implemented will still be needed so that is the only change made.
Thanks for leaving this in; indeed some of this was preemptively written.
| .take() | ||
| .ok_or_else(|| new_error!("mem_mgr should be initialized"))?; | ||
|
|
||
| let result = VirtualCPU::run( |
There was a problem hiding this comment.
I wonder if there is a way to simplify the somewhat windy code flow around here and remove the need to take the mem mgr option out, but perhaps that's for another day.
|
|
||
| /// The trait representing custom logic to handle the case when | ||
| /// a Hypervisor's virtual CPU (vCPU) informs Hyperlight a debug memory access | ||
| /// has been requested. | ||
| #[cfg(gdb)] | ||
| pub trait DbgMemAccessHandlerCaller: Send { |
There was a problem hiding this comment.
It looks like this trait also only has one (non-test) impl. Does it make sense to do the same thing as with the others and remove it?
Also, why remove the cfg(gdb) guards?
| // We need to handle the borrow checker issue where we need both: | ||
| // - &mut MemMgrWrapper (from self.mem_mgr.as_mut()) | ||
| // - &mut dyn Hypervisor (from self) | ||
| // We'll use a temporary approach to extract the mem_mgr temporarily |
There was a problem hiding this comment.
Is there a reason to cfg gate this instead of doing the same thing in both cases (e.g. a measurable performance impact?)
This pull fixes #495. It removes the OutBHandler and MemAccessHandler related traits, types and structs.
Since there is only a single implementation the functions that these types abstracted over (
handle_outbandhandle_mem_access) the code can be simplified by calling these functions directly.One implication of this change is that the
Hypervisornow contains aMemMgrWrapper<T>which ultimately contains aHostMappinginstance. Since the Hypervisor isSend + Syncthis change would requireHostMappingto beSend + Syncwhich is not automatically implemented as it contains a*mut u8 type. The solution that this PR implements is to remove theSyncconstraint from the Hypervisor trait since its not possible to access a Sandbox concurrently from multiple threads.There is probably some code in
shared_memthat on the face of it does not seem necessary without multi threaded access but in the future when async guest io is implemented will still be needed so that is the only change made.