Skip to content

Vsbox#8

Closed
CvvT wants to merge 12 commits intomainfrom
vsbox
Closed

Vsbox#8
CvvT wants to merge 12 commits intomainfrom
vsbox

Conversation

@CvvT
Copy link
Copy Markdown
Contributor

@CvvT CvvT commented Feb 11, 2025

Initial draft of platform for vsbox.

Run cargo doc --features platform_snp --open and navigate to the host module to see the interface design.

HostPunchthroughProvider: Interface for punchthrough requests
HostPunchthroughToken: A wrapper for PunchthroughToken that has lifetime
HyperCallInterface: Interface for hypercalls (See hypercall::HyperVInterface for example)

@CvvT CvvT requested a review from wdcui February 11, 2025 18:21
Comment thread Cargo.toml
resolver = "2"
members = [
"litebox",
"litebox-platform-linux-kernel",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shall we keep the names consistent with "_"?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I am to blame for this; I used to use - before deciding to make everything consistent in 1e6568d by switching to underscores (which turns out to be handled better by tooling and also make more sense in use ... declarations when specifying crates). Going forward, just using _ for all crate names should keep things nice, but yeah I believe @CvvT used - because I was using - at the point where he originally started working on this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I remember we have a similar error file for the posix shim. Can we use a single error file?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should be done in a separate PR to merge things into a shared crate litebox_linux_common or similar. I agree that currently this is essentially being duplicated.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(Fwiw, my suggestion for litebox_linux_common is a crate that also defines syscall punchthrough layer too, can explain more IRL if needed)

Comment thread .gitmodules
@@ -0,0 +1,3 @@
[submodule "litebox-platform-linux-kernel/sandbox_driver"]
path = litebox-platform-linux-kernel/sandbox_driver
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indexing by 4 spaces or 8 spaces?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the hypercall interface the same for SNP and VBS?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not clear to me why we need a host subfolder here.

}
}

const NR_SYSCALL_KILL: u64 = 62;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we define these two syscalls here? Do we need to define other syscall numbers as well?

punchthrough: SnpPunchthrough<'a>,
}

impl SnpPunchthroughToken<'_> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we define both HostPunchThrough and SnpPunchthrough?

Comment on lines +22 to +23
RecvPacket(&'a mut [u8]),
SendPacket(&'a [u8]),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As I understand it, these are intended for the IP interface stuff, yes? In that case, it should not be part of punchthrough since we already have the IPInterfaceProvider trait.

Comment on lines +141 to +147
OtherPunchthrough::Syscall0(ref v) => v.into(),
OtherPunchthrough::Syscall1(ref v) => v.into(),
OtherPunchthrough::Syscall2(ref v) => v.into(),
OtherPunchthrough::Syscall3(ref v) => v.into(),
OtherPunchthrough::Syscall4(ref v) => v.into(),
OtherPunchthrough::Syscall5(ref v) => v.into(),
OtherPunchthrough::Syscall6(ref v) => v.into(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of match *req, if you use match req then I think you don't need these refs (modern-ish Rust tends to use refs very rarely).

@jaybosamiya-ms
Copy link
Copy Markdown
Member

jaybosamiya-ms commented Feb 11, 2025

Quick question regarding the git-submodule for sandbox_driver; do we only need the .h files from there to set up bindings? If so, it may be a better idea for us to copy in the .h files, rather than deal with the complexity that comes from git-submodules especially for CI purposes (it is less of a complexity when things are public, but even then, it would lead to every user of litebox automatically cloning every submodule, even if not used, simply because of how Cargo does things conservatively). Having a sync_deps.sh or such script would make it easier for us to use a recent version, but it would allow us to decouple things better.

@CvvT
Copy link
Copy Markdown
Contributor Author

CvvT commented Feb 11, 2025

Quick question regarding the git-submodule for sandbox_driver; do we only need the .h files from there to set up bindings? If so, it may be a better idea for us to copy in the .h files, rather than deal with the complexity that comes from git-submodules especially for CI purposes (it is less of a complexity when things are public, but even then, it would lead to every user of litebox automatically cloning every submodule, even if not used, simply because of how Cargo does things conservatively). Having a sync_deps.sh or such script would make it easier for us to use a recent version, but it would allow us to decouple things better.

Thanks for the suggestion! Less complexity is better.


/// Interface for punchthrough requests
pub trait HostPunchthroughProvider<'a, InOut, Other> {
type Token: HostPunchthroughToken<'a, InOut>;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jaybosamiya-ms I was trying to use PunchthroughProvider but found that the associated PunchthroughToken does not have lifetime. The token may have shorter lifetime than the provider's.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Discussed this IRL, we need to de-restrict the situations that are allowed here. Fixed by #9 which also demonstrates how a (borrowing) punchthrough and/or punchthroughtoken can be implemented.

@CvvT CvvT closed this Feb 12, 2025
@CvvT
Copy link
Copy Markdown
Contributor Author

CvvT commented Feb 12, 2025

Move to a new one: #10

sangho2 pushed a commit that referenced this pull request Mar 11, 2026
macOS ARM64 does not restore x18 (platform-reserved register) from
ucontext on sigreturn. This caused a crash at far=0x8 when
exception_callback tried to load host_sp from [x18, #8] with x18=0.

Switch to x9 (caller-saved temp register) for passing host_tls through
sigreturn: set_signal_return writes to sigctx.__ss.__x[9], and both
exception_callback and interrupt_callback read host_tls from x9.

For the direct-branch path (switch_to_guest interrupt trampoline), add
'mov x9, x18' before branching to interrupt_callback so both paths are
consistent.
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