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 initial userspace support #467

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

vijaydhanraj
Copy link
Contributor

This is a draft PR that adds basic syscall support like exit, close, opendir, readdir and exec as well as init task and dm implementation. The purpose of this PR is to get feedback from the community and ensure the design aligns with principles of user-mode support described in https://github.com/coconut-svsm/svsm/blob/main/Documentation/docs/developer/DEVELOPMENT-PLAN.md as well syscall ABIs https://mail.8bytes.org/pipermail/svsm-devel/2024-June/000334.html.

PS: The changes are built on top of #456.

Copy link
Contributor

@Freax13 Freax13 left a comment

Choose a reason for hiding this comment

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

Some preliminary review comments.

kernel/src/string.rs Show resolved Hide resolved
kernel/src/syscall/class1.rs Outdated Show resolved Hide resolved
kernel/src/syscall/class1.rs Outdated Show resolved Hide resolved
kernel/src/fs/obj.rs Outdated Show resolved Hide resolved
kernel/src/fs/obj.rs Outdated Show resolved Hide resolved
kernel/src/syscall/class0.rs Outdated Show resolved Hide resolved
init/init.lds Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
init/.gitignore Outdated Show resolved Hide resolved
init/src/main.rs Outdated Show resolved Hide resolved
@vijaydhanraj vijaydhanraj force-pushed the initial_userspace_support branch 3 times, most recently from d860c4a to a40bb5b Compare September 29, 2024 17:34
@vijaydhanraj
Copy link
Contributor Author

Some preliminary review comments.

Thanks @Freax13 for the valuable comments. I have addressed all the earlier comments. Can you please take a look?

Copy link
Contributor

@Freax13 Freax13 left a comment

Choose a reason for hiding this comment

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

I don't have time for a full review right now, I'll do that sometime next week.

kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/syscall/class0.rs Outdated Show resolved Hide resolved
@Freax13
Copy link
Contributor

Freax13 commented Oct 7, 2024

Now that #456 has been merged, this PR can be rebased onto main and probably be marked as "ready to review" (instead of as a draft).

@vijaydhanraj
Copy link
Contributor Author

Now that #456 has been merged, this PR can be rebased onto main and probably be marked as "ready to review" (instead of as a draft).

Sure, will rebase, address your other comments and mark the PR ready to review.

@vijaydhanraj vijaydhanraj force-pushed the initial_userspace_support branch 2 times, most recently from 7493e6f to ac443c2 Compare October 9, 2024 05:52
@vijaydhanraj vijaydhanraj marked this pull request as ready for review October 9, 2024 05:56
kernel/src/cpu/idt/svsm.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
init/src/main.rs Outdated Show resolved Hide resolved
init/src/main.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/syscall/class1.rs Outdated Show resolved Hide resolved
kernel/src/task/tasks.rs Outdated Show resolved Hide resolved
init/src/main.rs Outdated Show resolved Hide resolved
kernel/src/cpu/idt/svsm.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/syscall/class0.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/syscall/class1.rs Outdated Show resolved Hide resolved
kernel/src/syscall/class1.rs Outdated Show resolved Hide resolved
syscall/src/call.rs Outdated Show resolved Hide resolved
@Freax13
Copy link
Contributor

Freax13 commented Oct 16, 2024

FYI this branch has conflicts with main. You might have to rebase your branch to resolve them.

kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/syscall/class1.rs Outdated Show resolved Hide resolved
syscall/src/def.rs Outdated Show resolved Hide resolved
@vijaydhanraj
Copy link
Contributor Author

FYI this branch has conflicts with main. You might have to rebase your branch to resolve them.

Yes, done.

kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Freax13 Freax13 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@joergroedel
Copy link
Member

Thanks for that work, it is pretty impressive! Although it is hard to review on all levels due to its size:

 .github/workflows/rust.yml     |  2 +-
 Cargo.lock                     | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 Cargo.toml                     |  5 +++++
 Makefile                       | 16 ++++++++++++++--
 dm/Cargo.toml                  | 15 +++++++++++++++
 dm/build.rs                    |  6 ++++++
 dm/dm.lds                      | 37 +++++++++++++++++++++++++++++++++++++
 dm/src/main.rs                 | 25 +++++++++++++++++++++++++
 elf/src/program_header.rs      |  3 +++
 init/Cargo.toml                | 16 ++++++++++++++++
 init/build.rs                  |  6 ++++++
 init/init.lds                  | 37 +++++++++++++++++++++++++++++++++++++
 init/src/main.rs               | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/src/cpu/idt/svsm.rs     | 12 ++++++++----
 kernel/src/error.rs            | 27 +++++++++++++++++++++++++++
 kernel/src/fs/api.rs           |  1 +
 kernel/src/fs/filesystem.rs    | 60 +++++++++++++++++++++++++++++++++++++++++++++++++-----------
 kernel/src/fs/mod.rs           |  2 ++
 kernel/src/fs/obj.rs           | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/src/mm/guestmem.rs      | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/src/string.rs           | 11 +++++++++++
 kernel/src/svsm.rs             | 13 +++++++------
 kernel/src/syscall/class0.rs   | 42 ++++++++++++++++++++++++++++++++++++++++++
 kernel/src/syscall/class1.rs   | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/src/syscall/handlers.rs | 21 ---------------------
 kernel/src/syscall/mod.rs      |  6 ++++--
 kernel/src/syscall/obj.rs      | 13 ++++++++++++-
 kernel/src/task/exec.rs        | 17 ++++++++++++-----
 kernel/src/task/schedule.rs    |  8 ++++++--
 kernel/src/task/tasks.rs       | 19 +++++++++++++++++--
 syscall/Cargo.toml             |  1 +
 syscall/src/call.rs            | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 syscall/src/class0.rs          | 33 +++++++++++++++++++++++++++++++++
 syscall/src/class1.rs          | 38 ++++++++++++++++++++++++++++++++++++++
 syscall/src/def.rs             | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 syscall/src/lib.rs             | 10 ++++++++--
 syscall/src/numbers.rs         | 10 ----------
 syscall/src/obj.rs             | 15 +++++++++++++++
 38 files changed, 939 insertions(+), 69 deletions(-)

Can you please split this into smaller PRs which we can merge one at a time? Please start with the system call interface and implementation. This PR can be rebased then on what has already been merged.

@joergroedel joergroedel added the in-review PR is under active review and not yet approved label Oct 23, 2024
@vijaydhanraj
Copy link
Contributor Author

Sure, I can split this into smaller PRs. Would splitting commits into the below PRs be helpful?

  • PR#8: Introduce a simple template for initial device model

    • dm: Add initial device model project
    • init: Extend opendir() and readdir() for dm discovery
  • PR#7: Add the initial version of init

  • PR#6: Introduce exec syscall

    • syscall: Implement exec()
    • kernel/syscall: Implement EXEC
    • kernel/task: Return TID in exec_user()
  • PR#5: Introduce opendir and readdir syscalls

    • syscall/class1: Implement opendir() and readdir()
    • kernel/syscall: Implement OPENDIR and READDIR
    • kernel/fs/filesystem: Add opendir()
    • kernel/fs: Add FsObj
    • kernel/fs/filesystem: Add find_dir()
    • svsm: Initialize FS earlier
  • PR#4: Introduce exit and open syscalls

    • syscall/obj: Close an ObjHandle when dropped
    • kernel/syscall: Implement close syscall
    • syscall: Introduce exit syscall
  • PR#3: Introduce syscall interface and error code

    • syscall: Define error code
    • syscall: Add syscall macro
  • PR#2: Introduce UserPtr wrapper around GuestPtr

  • PR#1: Few bookkeeping and clean-up commits

    • kernel/cpu/idt: Remove SYS_HELLO for implementing formal syscalls
    • kernel/syscall: Rename handlers.rs to class0.rs
    • syscall: Rename numbers.rs to def.rs
    • syscall: Add Obj trait

@Freax13
Copy link
Contributor

Freax13 commented Oct 25, 2024

FYI, we merged SMAP support in #473, this PR will have to be adjusted accordingly.

peterfang and others added 2 commits October 27, 2024 14:53
Add a new trait, Obj, to ensure all of the object handles follow a
common interface. This helps with implementing common syscalls that can
be used for different object types.

Signed-off-by: Peter Fang <[email protected]>
Rename numbers.rs to def.rs so that this file can be extended in the
future to contain more definitions, like MMFlag, syscall error code
which are common for both kernel and user.

Signed-off-by: Chuanxiao Dong <[email protected]>
cxdong and others added 11 commits October 27, 2024 14:53
The syscall handlers in handlers.rs currently is for class0 so rename
the file to class0.rs. The plan is to introduce other syscall handlers
files for different classes.

Signed-off-by: Chuanxiao Dong <[email protected]>
SYS_HELLO is a test syscall and occupies the syscall number 0, which is
for SYS_EXIT according to the spec. Remove the SYS_HELLO.

Signed-off-by: Chuanxiao Dong <[email protected]>
Add macro to define 5 raw syscalls, which take different number of
input arguement. The syscall ABI follows the published system call
spec[1]. To trigger syscall to kernel, int 0x80 is used according to the
kernel implementation. The syscall macro is derived from [2].

[1]: https://mail.8bytes.org/pipermail/svsm-devel/2024-June/000334.html
[2]: https://gitlab.redox-os.org/redox-os/syscall/-/blob/master/src/arch/x86_64.rs

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Chuanxiao Dong <[email protected]>
Define the raw error code according to syscall spec, and an enum
SysCallError to conver the raw error code to rust style error.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Chuanxiao Dong <[email protected]>
Add exit() to be used as SYS_EXIT, which takes one exit code
as an input parameter according to the spec.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Chuanxiao Dong <[email protected]>
The close syscall is implemented by using the obj_remove, to remove the
ObjHandle associated witht the input object id from the current task.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Chuanxiao Dong <[email protected]>
Implement Drop for the ObjHandle to close the underlying kernel object
when drop the ObjHandle in the user mode.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Chuanxiao Dong <[email protected]>
FS_ROOT needs to be initialized early enough for process root directory
to take effect when setting up BSP's idle task.

Signed-off-by: Peter Fang <[email protected]>
Add find_dir() which looks for a directory entry using a relative path.

Signed-off-by: Peter Fang <[email protected]>
Add FsObj which is the primary kernel object used for Class 1 syscalls.
This object represents both a file and a directory entry. Also, add
as_fs() to the Obj trait that allows for these objects to downcast back
to FsObjs.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Peter Fang <[email protected]>
Add opendir() which allows a Directory object to be returned.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Peter Fang <[email protected]>
@vijaydhanraj
Copy link
Contributor Author

Hi @joergroedel as suggested, split this PR and added a PR #502 to introduce the syscall interface implementation.

@vijaydhanraj
Copy link
Contributor Author

FYI, we merged SMAP support in #473, this PR will have to be adjusted accordingly.

Hi @Freax13 thanks, I have rebased this PR.

@Freax13
Copy link
Contributor

Freax13 commented Oct 28, 2024

Hi @Freax13 thanks, I have rebased this PR.

I meant to say that we now need to execute stac & clac in the UserPtr methods.

@vijaydhanraj
Copy link
Contributor Author

Got it, will add the stac and clac as part of the read/write_ref UserPtr methods and update the PR.

@vijaydhanraj
Copy link
Contributor Author

@Freax13 I have updated the PR to use stac & clac in UserPtr methods. If the changes look fine, please let me know if this should be spined out as a separate PR?

@Freax13
Copy link
Contributor

Freax13 commented Oct 29, 2024

@Freax13 I have updated the PR to use stac & clac in UserPtr methods. If the changes look fine, please let me know if this should be spined out as a separate PR?

Personally, I don't have a problem with large PRs, but @joergroedel might object.

From your comment above:

  • PR#2: Introduce UserPtr wrapper around GuestPtr

It probably makes sense to add the changes to this PR.

kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
kernel/src/mm/guestmem.rs Outdated Show resolved Hide resolved
vijaydhanraj and others added 9 commits October 29, 2024 08:18
In addition to ensuring invalid memory access, the UserPtr
wrapper around GuestPtr validates the address passed from
the userspace to make sure it isn't pointing to kernel memory.

Signed-off-by: Vijay Dhanraj <[email protected]>
Implement the syscall handlers for SYS_OPENDIR and SYS_READDIR based on
the ABI spec. SYS_OPENDIR creates an FsObj representing a directory
entry.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Peter Fang <[email protected]>
Implement the OPENDIR and READDIR syscall interface in user space.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Peter Fang <[email protected]>
If the user process is still running after scheduling, return its TID.
Else, indicate that it's already terminated through TaskError.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Peter Fang <[email protected]>
Implement the syscall handler for SYS_EXEC. This commit also provides
the ability to specify a new root directory for the process. This is
done because EXEC allows the parent process to specify the root
directory of the new process. New threads automatically inherits its
parent's root directory.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Peter Fang <[email protected]>
Implement the EXEC syscall interface in user space.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Peter Fang <[email protected]>
Add the initial version of init. Currently it only does exec() on a
dummy file. This is addressed in the future commits.

Introduced "make user" to compile the init process.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Peter Fang <[email protected]>
Use opendir() and readdir() for dm discovery. Currently, it naively runs
the first file discovered under /bin.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Peter Fang <[email protected]>
The device model is a user mode program which manages VM's life cycles
and handles various vmexits events. The initial version just calls exit()
syscall without doing anything else.

As this is a user mode program, not sure what is the preferred way to
put this project, just put it as a sub folder in coconut-svsm and in the
workspace. If in the workspace is not preferred, this can be changed in
the future.

Co-developed-by: Vijay Dhanraj <[email protected]>
Signed-off-by: Chuanxiao Dong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review PR is under active review and not yet approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants