gromero commented on code in PR #12125: URL: https://github.com/apache/tvm/pull/12125#discussion_r943725515
########## apps/microtvm/zephyr/template_project/src/host_driven/fvp/semihost.h: ########## @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#ifndef TVM_APPS_MICROTVM_ZEPHYR_HOST_DRIVEN_SEMIHOST_H_ +#define TVM_APPS_MICROTVM_ZEPHYR_HOST_DRIVEN_SEMIHOST_H_ + +#include <kernel.h> +#include <unistd.h> +#include <zephyr.h> + +static uint32_t semihost_cmd(uint32_t opcode, void* arg); + +void init_semihosting(); + +ssize_t read_semihost(uint8_t* data, size_t size); + +ssize_t write_semihost(void* unused_context, const uint8_t* data, size_t size); + +int32_t get_sim_clk(); Review Comment: I don't see this function being used. Could we remove it? (also from `semihost.c`)? ########## apps/microtvm/zephyr/template_project/CMakeLists.txt.template: ########## @@ -21,7 +21,7 @@ cmake_minimum_required(VERSION 3.13.1) set(ENV{QEMU_BIN_PATH} "${CMAKE_SOURCE_DIR}/qemu-hack") -set(QEMU_PIPE "\${QEMU_PIPE}") # QEMU_PIPE is set by the calling TVM instance. +set(QEMU_PIPE <QEMU_PIPE> CACHE PATH "path to qemu pipe") Review Comment: Could we s/path to qemu pipe/Path to QEMU pipe/? ########## apps/microtvm/zephyr/template_project/fvp-hack/FVP_Corstone_SSE-300_Ethos-U55: ########## @@ -0,0 +1,44 @@ +#!/bin/bash -e +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +set -x + +ARGS=( "$(basename $0)" ) + +if [ "${FVP_BIN_PATH}" != "" ]; then + ARGS=( ${FVP_BIN_PATH}/${ARGS[0]} ) +fi + +ARGS=( "${ARGS[@]}" + --iris-server + --print-port-number Review Comment: remove trailing space ########## apps/microtvm/zephyr/template_project/microtvm_api_server.py: ########## @@ -578,49 +623,78 @@ def generate_project(self, model_library_format_path, standalone_crt_dir, projec def build(self, options): BUILD_DIR.mkdir() - check_call(["cmake", ".."], cwd=BUILD_DIR) + zephyr_board = _find_board_from_cmake_file(API_SERVER_DIR / CMAKELIST_FILENAME) + emu_platform = _find_platform_from_cmake_file(API_SERVER_DIR / CMAKELIST_FILENAME) + + env = dict(os.environ) Review Comment: I think this cast to `dict()` is not necessary For instance, if `env = os.environ` you can access `env[ZEPHYR_BASE]`. ########## apps/microtvm/zephyr/template_project/microtvm_api_server.py: ########## @@ -166,6 +169,16 @@ def _find_board_from_cmake_file(cmake_file: Union[str, pathlib.Path]) -> str: return zephyr_board +def _find_platform_from_cmake_file(cmake_file: Union[str, pathlib.Path]) -> str: + emu_platform = None + with open(API_SERVER_DIR / CMAKELIST_FILENAME) as cmake_f: + for line in cmake_f: + if line.startswith("set(EMU_PLATFORM"): + emu_platform = line.strip("\n").strip("set(EMU_PLATFORM ").strip(")") Review Comment: It's possible to use a capture group as I suggested in: https://github.com/apache/tvm/pull/12338#discussion_r940861161 but it's up to you, won't block on this. ########## apps/microtvm/zephyr/template_project/microtvm_api_server.py: ########## @@ -904,5 +982,192 @@ def _wait_for_qemu(self): raise ValueError(f"{item} not expected.") +class ZephyrFvpMakeResult(enum.Enum): + FVP_STARTED = "fvp_started" + MICROTVM_API_SERVER_INIT = "fvp_initialized" + MAKE_FAILED = "make_failed" + EOF = "eof" + + +class BlockingStream: + """Reimplementation of Stream class from Iris with blocking semantics.""" + + def __init__(self): + self.q = queue.Queue() + self.unread = None + + def read(self, n=-1, timeout_sec=None): + assert ( + n != -1 + ), "expect firmware to open stdin using raw mode, and therefore expect sized read requests" + + data = b"" + if self.unread: + data = data + self.unread + self.unread = None + + while len(data) < n: + try: + # When there is some data to return, fetch as much as possible, then return what we can. + # When there is no data yet to return, block. + data += self.q.get(block=not len(data), timeout=timeout_sec) + except queue.Empty: + break + + if len(data) > n: + self.unread = data[n:] + data = data[:n] + + return data + + readline = read + + def write(self, data): + self.q.put(data) + + +class ZephyrFvpTransport: + """A transport class that communicates with the ARM FVP via Iris server.""" + + def __init__(self, options): + self.options = options + self.proc = None + self._queue = queue.Queue() + self._import_iris() + + def _import_iris(self): + # Location as seen in the FVP_Corstone_SSE-300_11.15_24 tar. + iris_lib_path = ( + pathlib.Path(self.options["arm_fvp_path"]).parent.parent.parent + / "Iris" + / "Python" + / "iris" + ) + + sys.path.insert(0, str(iris_lib_path.parent)) + try: + import iris.NetworkModelInitializer + finally: + sys.path.pop(0) + + self._iris_lib = iris + + def _convertStringToU64Array(strValue): + numBytes = len(strValue) + if numBytes == 0: + return [] + + numU64 = (numBytes + 7) // 8 + # Extend the string ending with '\0', so that the string length is multiple of 8. + # E.g. 'hello' is extended to: 'hello'+\0\0\0 + strExt = strValue.ljust(8 * numU64, b"\0") + # Convert the string to a list of uint64_t in little endian + return struct.unpack("<{}Q".format(numU64), strExt) + + iris.iris.convertStringToU64Array = _convertStringToU64Array + + def open(self): + args = ["ninja"] + if self.options.get("verbose"): + args.append("-v") + args.append("run") + env = dict(os.environ) + env["FVP_BIN_PATH"] = str(pathlib.Path(self.options["arm_fvp_path"]).parent) Review Comment: Should we add `arm_fvp_path` as required or optional in the server options? It seems required here -- or is a check missing that would use `options.get()`? But if it's a required option by FVP I'm not sure how to add it since this option doesn't apply to QEMU or other boards that don't use FVP. I think currently there is no way to inform that complexity using the server options ... so if it's indeed require at least assert or throw an error if `arm_fvp_path` is not given here? I also wonder why we have both `FVP_BIN_PATH` and `ARMFVP_BIN_PATH` here. The fvp-hack script uses FVP_BIN_PATH. Couldn't we use only the `ARMFVP_BIN_PATH` here and in the fvp-hack. In this case the Python code here would set `ARMFVP_BIN_PATH` to use the fvp-hack script and the fvp-hack, by its turn, would be called by Zephyr build / run system (when subprocess() is called). Then the fvp-hack, when called by Zephyr, would add the iris flags etc? Or at least add a comment why there are two similar bin paths to the FVP, saying how that's used by the fvp-hack script.. ########## apps/microtvm/zephyr/template_project/fvp-hack/FVP_Corstone_SSE-300_Ethos-U55: ########## @@ -0,0 +1,44 @@ +#!/bin/bash -e +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +set -x + +ARGS=( "$(basename $0)" ) + +if [ "${FVP_BIN_PATH}" != "" ]; then + ARGS=( ${FVP_BIN_PATH}/${ARGS[0]} ) +fi + +ARGS=( "${ARGS[@]}" Review Comment: remove trailing space in this line ########## apps/microtvm/zephyr/template_project/fvp-hack/FVP_Corstone_SSE-300_Ethos-U55: ########## @@ -0,0 +1,44 @@ +#!/bin/bash -e +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +set -x + +ARGS=( "$(basename $0)" ) + +if [ "${FVP_BIN_PATH}" != "" ]; then + ARGS=( ${FVP_BIN_PATH}/${ARGS[0]} ) +fi + +ARGS=( "${ARGS[@]}" + --iris-server Review Comment: remove trailing space ########## apps/microtvm/zephyr/template_project/src/host_driven/fvp/semihost.h: ########## @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#ifndef TVM_APPS_MICROTVM_ZEPHYR_HOST_DRIVEN_SEMIHOST_H_ +#define TVM_APPS_MICROTVM_ZEPHYR_HOST_DRIVEN_SEMIHOST_H_ + +#include <kernel.h> +#include <unistd.h> +#include <zephyr.h> + +static uint32_t semihost_cmd(uint32_t opcode, void* arg); + +void init_semihosting(); + +ssize_t read_semihost(uint8_t* data, size_t size); Review Comment: this should be `semihost_read()` ########## apps/microtvm/zephyr/template_project/src/host_driven/fvp/semihost.h: ########## @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#ifndef TVM_APPS_MICROTVM_ZEPHYR_HOST_DRIVEN_SEMIHOST_H_ +#define TVM_APPS_MICROTVM_ZEPHYR_HOST_DRIVEN_SEMIHOST_H_ + +#include <kernel.h> +#include <unistd.h> +#include <zephyr.h> + +static uint32_t semihost_cmd(uint32_t opcode, void* arg); + +void init_semihosting(); + +ssize_t read_semihost(uint8_t* data, size_t size); + +ssize_t write_semihost(void* unused_context, const uint8_t* data, size_t size); Review Comment: this should be `semihost_write()` ########## apps/microtvm/zephyr/template_project/microtvm_api_server.py: ########## @@ -483,10 +509,14 @@ def _generate_cmake_args(self, mlf_extracted_path, options) -> str: if options.get("west_cmd"): cmake_args += f"set(WEST {options['west_cmd']})\n" - if self._is_qemu(options["zephyr_board"]): + if self._is_qemu(options["zephyr_board"], options.get("use_fvp")): # Some boards support more than one emulator, so ensure QEMU is set. cmake_args += f"set(EMU_PLATFORM qemu)\n" + if self._is_fvp(options["zephyr_board"], options.get("use_fvp")): + cmake_args += f"set(EMU_PLATFORM armfvp)\n" Review Comment: no s-tring is necessary here ########## apps/microtvm/zephyr/template_project/fvp-hack/FVP_Corstone_SSE-300_Ethos-U55: ########## @@ -0,0 +1,44 @@ +#!/bin/bash -e +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +set -x + +ARGS=( "$(basename $0)" ) + +if [ "${FVP_BIN_PATH}" != "" ]; then + ARGS=( ${FVP_BIN_PATH}/${ARGS[0]} ) +fi + +ARGS=( "${ARGS[@]}" + --iris-server + --print-port-number + -C cpu0.semihosting-enable=1 Review Comment: remove trailing space ########## apps/microtvm/zephyr/template_project/src/host_driven/fvp/semihost.h: ########## @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#ifndef TVM_APPS_MICROTVM_ZEPHYR_HOST_DRIVEN_SEMIHOST_H_ +#define TVM_APPS_MICROTVM_ZEPHYR_HOST_DRIVEN_SEMIHOST_H_ + +#include <kernel.h> +#include <unistd.h> +#include <zephyr.h> + +static uint32_t semihost_cmd(uint32_t opcode, void* arg); Review Comment: Usually static functions should not go into header files, since they are defined and used as per each translation unit (source file). But I suspect you didn't want it to be static, so `static`could be removed from here and from `semihost.c`? Otherwise the following warning happens in `main.c` which also includes `semihost.h` but doesn't define -- because it does't use it -- `semihost_cmd` in: ``` In file included from /home/gromero/git/tvm/tests/micro/zephyr/workspace_mps3_an547/2022-08-11T14-40-30/project/src/main.c:49: /home/gromero/git/tvm/tests/micro/zephyr/workspace_mps3_an547/2022-08-11T14-40-30/project/src/main.c: At top level: /home/gromero/git/tvm/tests/micro/zephyr/workspace_mps3_an547/2022-08-11T14-40-30/project/src/semihost.h:32:17: warning: 'semihost_cmd' declared 'static' but never defined [-Wunused-function] 32 | static uint32_t semihost_cmd(uint32_t opcode, void* arg); | ^~~~ ``` ########## apps/microtvm/zephyr/template_project/microtvm_api_server.py: ########## @@ -483,10 +509,14 @@ def _generate_cmake_args(self, mlf_extracted_path, options) -> str: if options.get("west_cmd"): cmake_args += f"set(WEST {options['west_cmd']})\n" - if self._is_qemu(options["zephyr_board"]): + if self._is_qemu(options["zephyr_board"], options.get("use_fvp")): # Some boards support more than one emulator, so ensure QEMU is set. cmake_args += f"set(EMU_PLATFORM qemu)\n" + if self._is_fvp(options["zephyr_board"], options.get("use_fvp")): + cmake_args += f"set(EMU_PLATFORM armfvp)\n" + cmake_args += f"set(ARMFVP_FLAGS -I)\n" Review Comment: likewise regarding s-string ########## apps/microtvm/zephyr/template_project/microtvm_api_server.py: ########## @@ -578,49 +623,78 @@ def generate_project(self, model_library_format_path, standalone_crt_dir, projec def build(self, options): BUILD_DIR.mkdir() - check_call(["cmake", ".."], cwd=BUILD_DIR) + zephyr_board = _find_board_from_cmake_file(API_SERVER_DIR / CMAKELIST_FILENAME) + emu_platform = _find_platform_from_cmake_file(API_SERVER_DIR / CMAKELIST_FILENAME) + + env = dict(os.environ) + if self._is_fvp(zephyr_board, emu_platform == "armfvp"): + env["ARMFVP_BIN_PATH"] = str(API_SERVER_DIR / "fvp-hack") + env["ARMFVP_BIN_PATH"] = os.path.realpath(env["ARMFVP_BIN_PATH"]) + st = os.stat(env["ARMFVP_BIN_PATH"] + "/FVP_Corstone_SSE-300_Ethos-U55") + os.chmod( + env["ARMFVP_BIN_PATH"] + "/FVP_Corstone_SSE-300_Ethos-U55", + st.st_mode | stat.S_IEXEC, + ) - args = ["make", "-j2"] + print("ENV", env) Review Comment: Is this a leftover and should be removed? ########## apps/microtvm/zephyr/template_project/microtvm_api_server.py: ########## @@ -578,49 +623,78 @@ def generate_project(self, model_library_format_path, standalone_crt_dir, projec def build(self, options): BUILD_DIR.mkdir() - check_call(["cmake", ".."], cwd=BUILD_DIR) + zephyr_board = _find_board_from_cmake_file(API_SERVER_DIR / CMAKELIST_FILENAME) + emu_platform = _find_platform_from_cmake_file(API_SERVER_DIR / CMAKELIST_FILENAME) + + env = dict(os.environ) + if self._is_fvp(zephyr_board, emu_platform == "armfvp"): + env["ARMFVP_BIN_PATH"] = str(API_SERVER_DIR / "fvp-hack") + env["ARMFVP_BIN_PATH"] = os.path.realpath(env["ARMFVP_BIN_PATH"]) + st = os.stat(env["ARMFVP_BIN_PATH"] + "/FVP_Corstone_SSE-300_Ethos-U55") + os.chmod( + env["ARMFVP_BIN_PATH"] + "/FVP_Corstone_SSE-300_Ethos-U55", + st.st_mode | stat.S_IEXEC, + ) - args = ["make", "-j2"] + print("ENV", env) + check_call(["cmake", "-GNinja", ".."], cwd=BUILD_DIR, env=env) + + args = ["ninja"] if options.get("verbose"): - args.append("VERBOSE=1") - check_call(args, cwd=BUILD_DIR) + args.append("-v") + check_call(args, cwd=BUILD_DIR, env=env) # A list of all zephyr_board values which are known to launch using QEMU. Many platforms which # launch through QEMU by default include "qemu" in their name. However, not all do. This list # includes those tested platforms which do not include qemu. - _KNOWN_QEMU_ZEPHYR_BOARDS = ("mps2_an521", "mps3_an547") + _KNOWN_QEMU_ZEPHYR_BOARDS = ["mps2_an521", "mps3_an547"] + + # A list of all zephyr_board values which are known to launch using ARM FVP (this script configures + # Zephyr to use that launch method). + _KNOWN_FVP_ZEPHYR_BOARDS = ["mps3_an547"] @classmethod - def _is_qemu(cls, board: str) -> bool: - return "qemu" in board or board in cls._KNOWN_QEMU_ZEPHYR_BOARDS + def _is_fvp(cls, board, use_fvp): + assert not ( Review Comment: Could this assert be simplified by: ``` if use_fvp: assert board in cls._KNOWN_FVP_ZEPHYR_BOARDS, "FVP can be used to emulate this board on Zephyr" return True return False ``` ? ########## apps/microtvm/zephyr/template_project/microtvm_api_server.py: ########## @@ -578,49 +623,78 @@ def generate_project(self, model_library_format_path, standalone_crt_dir, projec def build(self, options): BUILD_DIR.mkdir() - check_call(["cmake", ".."], cwd=BUILD_DIR) + zephyr_board = _find_board_from_cmake_file(API_SERVER_DIR / CMAKELIST_FILENAME) + emu_platform = _find_platform_from_cmake_file(API_SERVER_DIR / CMAKELIST_FILENAME) + + env = dict(os.environ) + if self._is_fvp(zephyr_board, emu_platform == "armfvp"): + env["ARMFVP_BIN_PATH"] = str(API_SERVER_DIR / "fvp-hack") Review Comment: I think you can avoid str() cast since `API_SERVER_DIR` is already of `pathlib.Path()` type. Why it's necessary to `chmod` the `FVP_Corstone_SSE-300_Ethos-U55` bash script? It's already add as an executable to the repo. So, if all this holds, I think the code from like 631 to 637 can be simply something like: ``` - env["ARMFVP_BIN_PATH"] = str(API_SERVER_DIR / "fvp-hack") - env["ARMFVP_BIN_PATH"] = os.path.realpath(env["ARMFVP_BIN_PATH"]) - st = os.stat(env["ARMFVP_BIN_PATH"] + "/FVP_Corstone_SSE-300_Ethos-U55") - os.chmod( - env["ARMFVP_BIN_PATH"] + "/FVP_Corstone_SSE-300_Ethos-U55", - st.st_mode | stat.S_IEXEC, - ) + env["ARMFVP_BIN_PATH"] = (API_SERVER_DIR / "fvp-hack").resolve() ``` ? ########## apps/microtvm/zephyr/template_project/microtvm_api_server.py: ########## @@ -344,6 +357,18 @@ def _get_nrf_device_args(options): type="str", help="Path to the CMSIS directory.", ), + server.ProjectOption( + "arm_fvp_path", + optional=["generate_project"], Review Comment: I see this options is accessed directly in open() (so used by `open_transport` when FVP is used), hence it seems a required option for `open_transport` Project API method? Could you please clarify its domain / use? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
