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 Kernel syscall.* wrappers #173

Closed
frisitano opened this issue Jul 21, 2023 · 7 comments
Closed

Implement Kernel syscall.* wrappers #173

frisitano opened this issue Jul 21, 2023 · 7 comments

Comments

@frisitano
Copy link
Contributor

One of the constraints the virtual machine implements when exiting from a context is that the number of elements on the stack must be exactly 16. This has implications for syscall invocations where the syscall procedure returns more elements than it consumes. E.g. consumes one elmenent from the stack and returns 4. To address this we should add a prologue in the syscall.* wrapper that adds additional padding the top of the stack before the kernel procedure is invoked to ensure that unintended data is not overwritten.

Additionally syscall procedures should account for the fact that when the stack depth is 16 and items are dropped this results in 0's being padded to the bottom of the stack. If we need to return less elements then we consume then we should pad the top of the stack before exiting the root context.

@bobbinth
Copy link
Contributor

I think we can address this issue in the context of another issue. Specifically, we will probably want to cleanly separate "internal" from "user-facing" procedures so that users don't need to know when to use exec and when to use syscall. This will also allow us to do custom processing of inputs and outputs to hide the complexity of syscalls from the user.

One idea described in #89 (comment) is to move most (all?) modules in miden::sat to miden::sat::internal, and replace them with "user-facing" wrapper modules in miden::sat. For example, miden::sat::account could look something like this:

use.miden::sat::internal::account

export.get_id
  syscall.account::get_id
end
...

I think this will get us only partially there, and we'll need to modify this idea somewhat. The properties we want to achieve are as follows:

  1. User-facing procedures should have exactly the same interface as internal procedures.
  2. Internal procedures should not have to do any extra manipulation of inputs outputs to remain as efficient as possible.

One way to achieve the above is to put extra logic into procedures defined in kernel.masm module. So, in effect, input/output adjustment logic will be split between "user-facing" procedures and kernel procedures. Here is how it could work on the example of account::get_id procedure.

First, the user-facing miden::sat::account module would look like so:

use.miden::sat::kernel

#! Returns the account id.
#!
#! Operand stack:
#!   Inputs:  [...]
#!   Outputs: [acct_id, ...]
export.get_id
  push.0
  # => [0, ...]

  syscall.kernel::get_account_id
  # => [acct_id, ...]
end

And miden::sat::kernel module would look like so:

use.miden::sat::internal::account

#! Returns the account id.
#!
#! Operand stack:
#!   Inputs:  [0, ...]
#!   Outputs: [acct_id, ...]
export.get_account_id
  exec.account::get_id
  # => [acct_id, 0, ...]

  swap drop
  # => [acct_id, ...]
end

Basically, the user-facing wrapper pads the stack as needed (and may do other input processing) and makes the syscall, and then inside the kernel, we call the internal procedure and drop the padding.

Kernel procedures would also be the right place to enforce access control logic. For example, in miden::sat::kernel, we could have the following:

use.miden::sat::internal::account

export.set_account_item
  # get the hash of the caller
  padw caller

  # make sure the caller is a part of the account interface
  exec.account::authenticate_procedure
  dropw

  # update the account storage
  exec.account::set_item

  <drop padding>
end

I think the above would work fairly well, except for one downside: when we make updates, we'll need to update the entire procedure chain starting from user-facing procedures, to kernel procedures, to the internal procedures. But hopefully, such changes would be not very frequent.

@frisitano
Copy link
Contributor Author

frisitano commented Jul 23, 2023

This makes sense to me. One thing we may also want to consider in the context of this change is the visibility / scope of procedures. We probably don't want any of the internal procedures to be part of the external library interface. I believe we had discussed introducing variants of the export keyword that scope a procedure to the library as opposed to making it available externally - this would work something like export(lib).my_proc_name. Similarly we may not want the kernel procedures in kernel.masm to be part of the external interface of the library. Maybe we need to introduce a new keyword for kernel procs - export(kernel).my_kernl_proc. This isn't a blocker by any means but may improve the ergonomics of kernel packaging and distribution. We could also have other export scopes such as "super" / "test" / "mod".

@bobbinth
Copy link
Contributor

I believe we had discussed introducing variants of the export keyword that scope a procedure to the library as opposed to making it available externally - this would work something like export(lib).my_proc_name.

Yes, there is an issue for this already: 0xPolygonMiden/miden-vm#822 and I've been thinking about it. However, at of yet I haven't found a way to do this without adding quite a bit of complexity to the assembler. One of this issues is how procedure cache works. Once a procedure is in procedure cache, it can be called by any other procedure either by name, alias, or MAST root. So, technically, even non-exported procedures may be referenced right now if there were added to the procedure cache (this could happen under some conditions).

@frisitano
Copy link
Contributor Author

Something that hasn't been detailed in proposal above is the way in which account procedure are invoked using the call operation. The implications of this are similar to invocations of kernel procedures using the syscall operation. As such we will need to wrap account procedures to manage stack length during context switching.

@bobbinth
Copy link
Contributor

I think for accounts this will be a bit more challenging - or at least we won't be able to hide the complexity as easily. So, we'll need to come up with a convention which others would need to follow. There are several ways to do it. For example:

  • We could define a calling convention which would handle the calls generically (e.g., the caller saves top 16 elements of the stack to memory before making a call and then restores them after the call).
  • We could say that for every account there needs to be a wrapper module. This module would not be on-chain, but would be provided as a convenience for people who want to interact with the account.

There could be some other options too.

@frisitano
Copy link
Contributor Author

frisitano commented Jul 27, 2023

I think these are both viable options. I wonder if we could provide first class support for this in the Assembler. For example we could introduce a new token and procedure declaration pattern for call targets -export(call).proc_name.num_inputs.num_outputs.(num_locals). The compiler would then automatically compute and implement the required padding for this procedure and provide a wrapper than can be invoked using an exec in a nested module. This could also be used for syscall in the kernel.

@frisitano
Copy link
Contributor Author

closed by #174.

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

No branches or pull requests

2 participants