Argc/Argv support for SGX #605

csegarragonz · 2022-03-01T16:21:03Z

In this PR I implement the WASI calls to access argc and argv inside SGX: wasi_args_get and wasi_args_sizes_get. The immediate consequence is that now the test function: demo/argc_argv_test also runs on SGX.

The bulk of the implementation is almost copied from WAMR's (src/wamr/env.cpp), and requires no OCalls. The only major change to the OCall/ECall interface is that we now pass argc and argv to the ecallCallFunction call. Note that the OCall/ECall interface forces us to serialise argv into a byte array, which means some more work.

In addition, I re-factor the utility functions to run SGX tests to match the WAVM and WAMR counterparts: now we have execFunction, execWamrFunction, execSgxFunction, and execFuncWithPool, execFuncWithWamrPool, and execFuncWithSgxPool.

Shillaker

This looks good, but I'm concerned that this approach will lead to having two versions of a lot WAMR-related logic. I think a mixin shared between EnclaveWasmModule and WAMRWasmModule makes sense (e.g. WAMRModuleMixin or WAMRModuleCommon or something)

faasmcli/faasmcli/tasks/dev.py

Shillaker · 2022-03-02T08:57:33Z

include/enclave/inside/native.h

+//
+// Link to WAMR docs:
+// https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/doc/export_native_api.md#buffer-address-conversion-and-boundary-check
+// ----------------------------------------------


This looks a lot like the comment in https://github.com/faasm/faasm/blob/main/include/wamr/native.h, and feels like it's just repeating what's in the WAMR docs. Is it necessary and can we somehow avoid having this information in two places?

I have added the new relevant bits to the file you link, and removed this duplicated comment and added a reference to where it is explained.

Shillaker · 2022-03-02T08:58:21Z

include/enclave/inside/ocalls.h

+// In enclave release mode (i.e NDEBUG set) we disable debug logging
+#ifdef NDEBUG
+    void ocallLogDebug(const char* msg) { ; };
+#else


How come this has a different method signature to the declaration in debug mode? Shouldn't they be the same?

Both methods need to have different return values to prevent the RELEASE version from doing any kind of OCall. Whereas the second signature is that corresponding to an OCall.

Shillaker · 2022-03-02T08:59:03Z

include/enclave/outside/ecalls.h

-                                          uint32_t faaslet_id);
+                                          uint32_t faaslet_id,
+                                          uint32_t argc,
+                                          char** argv);


Still some kebab_case in this file, would be good to refactor to PascalCase in future.

Will do in the immediately subsequent PR.

src/enclave/inside/EnclaveWasmModule.cpp

Shillaker · 2022-03-02T09:06:08Z

src/enclave/inside/EnclaveWasmModule.cpp

+    }
+}
+
+void EnclaveWasmModule::prepareArgcArgv(uint32_t argcIn, char** argvIn)


Same point re. duplication with WARMWasmModule and mixin. We need to find a way to avoid duplicating these functions otherwise we'll end up with two copies of everything.

Unfortunately, this call in particular can not be abstracted out, as WAMR's (non-SGX) version is implemented in WasmModule and uses calls to faabric.

In addition, the method signature is different:

void EnclaveWasmModule::prepareArgcArgv(uint32_t argcIn, char** argvIn); void WAMRWasmModule::prepareArgcArgv(const faabric::Message& msg);

Ah ok I see, it's slightly confusing to have the same name and have it factored out as a separate function then, especially as it's only called in one place. I'd probably just inline the logic in the callFunction method.

Shillaker · 2022-03-02T09:08:03Z

src/enclave/inside/env.cpp

+    GET_EXECUTING_MODULE_AND_CHECK(exec_env);
+
+    *argcWasm = module->getArgc();
+    *argvBuffSizeWasm = module->getArgvBufferSize();


This looks very similar to

faasm/src/wamr/env.cpp

Line 26 in bcdbf54

uint32_t wasi_args_sizes_get(wasm_exec_env_t exec_env,

, potentially same point around moving some of this logic into a mixin.

I think native symbols aren't such good candidates to be included in a mixin. The native symbol registration doesn't work well with templated functions, and there are some methods (like logging and getting the executing module) that have different names and signatures.

That being said, after the new mixin structure the methods body is either trivial or implemented in the WAMRModuleMixin.

I don't mean we should merge the whole native symbol thing in, just the logic of the body of the function. This is how we share host interface logic between WAVM and WAMR normally, e.g. having a method like module->setUpArgcArgv(argcWasm, argvBuffSizeWasm) rather individual calls to getArgc and getArgvBufferSize. Admittedly here it will only save a couple of lines, but it's more the principle of absolutely minimising duplication.

…ution environment

csegarragonz · 2022-03-03T08:48:16Z

include/enclave/inside/EnclaveWasmModule.h

+
+    // ---- argc/arv ----
+
+    uint32_t getArgc();


The getters I need to implement in the actual modules, as the mixin does not have access to private members.

csegarragonz · 2022-03-03T08:52:28Z

include/wamr/WAMRModuleMixin.h

@@ -0,0 +1,66 @@
+#pragma once


@Shillaker this is the header file where are define the mixin (I plan on adding more calls currently in WAMRWasmModule as I need to implement them as well in EnclaveWasmModule).

Ok nice, great that we can share this stuff more now.

I just wanted to check, do you think we could actually do this via an intermediate superclass? I.e. have the following hierarchy:

WasmModule | WAMRModuleCommon | | WAMRWasmModule EnclaveWasmModule

We could then move the common properties between WAMRWasmModule and EnclaveWasmModule, like argc and argvBufferSize into WAMRModuleCommon and avoid the need for the stuff with underlying. We could also then potentially share methods like loadWasm.

I have a feeling we discussed this before, but can't remember why we couldn't do a superclass.

(Happy to continue discussion on Slack if easier)

Shillaker · 2022-03-03T09:26:34Z

src/enclave/inside/EnclaveWasmModule.cpp

+    }
+}
+
+void EnclaveWasmModule::prepareArgcArgv(uint32_t argcIn, char** argvIn)


Ah ok I see, it's slightly confusing to have the same name and have it factored out as a separate function then, especially as it's only called in one place. I'd probably just inline the logic in the callFunction method.

Shillaker · 2022-03-03T09:30:09Z

src/enclave/inside/env.cpp

+    GET_EXECUTING_MODULE_AND_CHECK(exec_env);
+
+    *argcWasm = module->getArgc();
+    *argvBuffSizeWasm = module->getArgvBufferSize();


I don't mean we should merge the whole native symbol thing in, just the logic of the body of the function. This is how we share host interface logic between WAVM and WAMR normally, e.g. having a method like module->setUpArgcArgv(argcWasm, argvBuffSizeWasm) rather individual calls to getArgc and getArgvBufferSize. Admittedly here it will only save a couple of lines, but it's more the principle of absolutely minimising duplication.

Shillaker · 2022-03-03T09:35:36Z

include/wamr/WAMRModuleMixin.h

@@ -0,0 +1,66 @@
+#pragma once


Ok nice, great that we can share this stuff more now.

I just wanted to check, do you think we could actually do this via an intermediate superclass? I.e. have the following hierarchy:

WasmModule | WAMRModuleCommon | | WAMRWasmModule EnclaveWasmModule

We could then move the common properties between WAMRWasmModule and EnclaveWasmModule, like argc and argvBufferSize into WAMRModuleCommon and avoid the need for the stuff with underlying. We could also then potentially share methods like loadWasm.

I have a feeling we discussed this before, but can't remember why we couldn't do a superclass.

(Happy to continue discussion on Slack if easier)

Shillaker · 2022-03-03T09:41:30Z

include/wamr/WAMRModuleMixin.h

+#include <vector>
+
+template<typename T>
+struct WAMRModuleMixin


Can this be a class? It gives us more flexibility around scoping the members.

csegarragonz · 2022-03-03T12:12:21Z

After our offline discussion I have added a comment in the mixin header file, explaining our design choices.

csegarragonz added 2 commits March 1, 2022 16:17

enclave/inside: no-op log debug ocall in release builds

422ff21

enclave/inside: add note on symbol registration

7a82451

csegarragonz marked this pull request as draft March 1, 2022 16:21

enclave: add support for argc/argv inside enclaves

359fa09

csegarragonz force-pushed the barebones branch from bcb7336 to 359fa09 Compare March 1, 2022 16:29

csegarragonz added 2 commits March 1, 2022 17:41

enclave: implement wasi_args_get

067fe95

tests: include sgx in function to test argc/argv

f2ac4d3

csegarragonz self-assigned this Mar 1, 2022

csegarragonz added the wasm/wamr-sgx SGX related stuff. label Mar 1, 2022

csegarragonz marked this pull request as ready for review March 1, 2022 17:43

csegarragonz added 2 commits March 1, 2022 17:50

nits

a87f71b

cli: generate sgx codegen for new supported function

f8a509c

csegarragonz force-pushed the barebones branch 2 times, most recently from 142bff0 to 7079b23 Compare March 1, 2022 19:03

tests: disable anything sgx-related in sanitised builds

8179320

csegarragonz force-pushed the barebones branch from 7079b23 to 8179320 Compare March 1, 2022 19:21

csegarragonz requested a review from Shillaker March 1, 2022 22:59

Shillaker requested changes Mar 2, 2022

View reviewed changes

csegarragonz added 6 commits March 2, 2022 11:03

cli: fix nosgx comparison

6f9051f

wamr: introduce WAMRModuleMixin to abstract shared logic

cf04545

ocalls: add comment to ocall log debug method

423328d

enclave: remove wasm namespace inside EnclaveWasmModule

ea0aa72

enclave: add error message when can't find registered module for exec…

f8490c2

…ution environment

wamr: unify notes on wamr symbol registrattion

c2b511f

csegarragonz requested a review from Shillaker March 3, 2022 08:46

csegarragonz commented Mar 3, 2022

View reviewed changes

Shillaker reviewed Mar 3, 2022

View reviewed changes

wamr: add comment in the mixin header file

ee5a46e

csegarragonz requested a review from Shillaker March 3, 2022 12:11

Shillaker approved these changes Mar 4, 2022

View reviewed changes

csegarragonz merged commit 46699e6 into main Mar 4, 2022

csegarragonz deleted the barebones branch March 4, 2022 08:27

Argc/Argv support for SGX #605

Argc/Argv support for SGX #605

Uh oh!

Conversation

csegarragonz commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shillaker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csegarragonz Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csegarragonz Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csegarragonz Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shillaker Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shillaker Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csegarragonz commented Mar 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

csegarragonz commented Mar 1, 2022 •

edited

Loading

Shillaker left a comment •

edited

Loading

csegarragonz Mar 3, 2022 •

edited

Loading

csegarragonz Mar 3, 2022 •

edited

Loading

csegarragonz Mar 3, 2022 •

edited

Loading

Shillaker Mar 3, 2022 •

edited

Loading

Shillaker Mar 3, 2022 •

edited

Loading