Rework Python modules, update OTP and LC CTRL

.github/workflows/build_test.yaml

@@ -76,10 +76,26 @@ jobs:
           sudo apt-get install -y clang-format-18
       - name: Check out QEMU
         uses: actions/checkout@v4
-      - name: Check execution
+      - name: Check C code format
         run: |
           scripts/opentitan/ot-format.sh --ci -i
 
+  lint-python:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Install tools
+        run: |
+          sudo apt-get update &&
+          sudo apt-get install -y python3-pip
+          # ubuntu "latest" is too old to require --break-system-packages ...
+          pip3 install pylint
+      - name: Check out QEMU
+        uses: actions/checkout@v4
+      - name: Lint Python code
+        run: |
+          pylint --rcfile scripts/opentitan/.pylintrc -d 'duplicate-code' -d 'fixme' \
+            scripts/opentitan/*.py python/qemu/jtag python/qemu/ot
+
   lint-clang:
     runs-on: ubuntu-latest
     needs: build-clang

docs/opentitan/jtagmbx.md

@@ -36,7 +36,7 @@ instantiated and listens for incoming connection on TCP port 3335.
 
 OpenTitan implementation provides JTAG/DTM/DMI/Mailbox stack available as Python modules:
 
-* jtag/tap module is available from `scripts/jtag` directory
+* jtag/tap module is available from `python/qemu/jtag` directory
 * dtm/dmi and jtag mailbox modules are available from `scripts/opentitan` directory
 
 Python snippet to create a communication channel with the VM JTAG mailbox:

docs/opentitan/lc_ctrl_dmi.md

@@ -33,7 +33,7 @@ instantiated and listens for incoming connection on TCP port 3335.
 
 OpenTitan implementation provides JTAG/DTM/DMI/Mailbox stack available as Python modules:
 
-* jtag/tap module is available from `scripts/jtag` directory
+* jtag/tap module is available from `python/qemu/jtag` directory
 * dtm/dmi and jtag mailbox modules are available from `scripts/opentitan` directory
 
 Python snippet to create a communication channel with the VM JTAG mailbox:

docs/opentitan/pymod.md

@@ -4,8 +4,8 @@ The communication ports of the OpenTitan machines exposed through CharDev device
 to access the devices from the local host (or also from a remote host when the CharDev is created
 using a TCP socket).
 
-* `scripts/jtag`: JTAG / TAP controller client, using the _Remote BitBang Protocol_
-* `scripts/opentitan/ot`: OpenTitan tools
+* `python/qemu/jtag`: JTAG / TAP controller client, using the _Remote BitBang Protocol_
+* `python/qemu/ot`: OpenTitan tools
   * `dtm`: Debug Transport Module support,
   * `dm`: RISC-V Debug Module support,
   * `lc_ctrl`: [Life Cycle controller](lc_ctrl_dmi.md) over JTAG/DMI support,

docs/opentitan/pyot.md

@@ -7,7 +7,7 @@
 ````text
 usage: pyot.py [-h] [-D DELAY] [-i ICOUNT] [-L LOG_FILE] [-M VARIANT] [-N LOG]
                [-m MACHINE] [-Q OPTS] [-q QEMU] [-P VCP] [-p DEVICE]
-               [-t TRACE] [-S FIRST_SOC] [-s] [-U] [-b file] [-c JSON] [-e]
+               [-t TRACE] [-S FIRST_SOC] [-s] [-U] [-b file] [-c HJSON] [-e]
                [-f RAW] [-g file] [-K] [-l file] [-O RAW] [-o VMEM] [-r ELF]
                [-w CSV] [-x file] [-X] [-F TEST] [-k SECONDS] [-z] [-R]
                [-T FACTOR] [-Z] [-v] [-V] [-d] [--quiet] [--log-time]
@@ -48,8 +48,8 @@ Virtual machine:
 
 Files:
   -b file, --boot file  bootloader 0 file
-  -c JSON, --config JSON
-                        path to configuration file
+  -c HJSON, --config HJSON
+                        path to HJSON configuration file
   -e, --embedded-flash  generate an embedded flash image file
   -f RAW, --flash RAW   SPI flash image file
   -g file, --otcfg file
@@ -92,8 +92,8 @@ Extras:
 This tool may be used in two ways, which can be combined:
 
 * From the command line, it is possible to run a QEMU test session for one application.
-* Using a JSON configuration file, it is possible to run several QEMU test sessions for each
-  specified test in the configuration file. This mode is enabled when a JSON config file is
+* Using a HJSON configuration file, it is possible to run several QEMU test sessions for each
+  specified test in the configuration file. This mode is enabled when a HJSON config file is
   specified.
 
 ### Virtual machine
@@ -136,8 +136,8 @@ This tool may be used in two ways, which can be combined:
 * `-b` / ` --boot`  specify a bootloader 0 file that can be added to the flash image file when
   a ROM extension file is specified with the `-x` option. This option is mutually exclusive with
   the `-f` option.
-* `-c` / `--config` specify a (H)JSON configuration file, see the
-  [Configuration](#Configurationfile) section for details.
+* `-c` / `--config` specify a HJSON configuration file, see the [Configuration](#Configurationfile)
+  section for details.
 * `-e` / `embedded-flash` generate an embedded flash image file, default is to provide ROM and
   application files as device options
 * `-f` / `--flash` specify a RAW image file that stores the embedded Flash content, which can be
@@ -191,85 +191,8 @@ This tool may be used in two ways, which can be combined:
 
 ## Configuration file
 
-### Legacy JSON syntax
-
-Sample config for running OpenTitan tests:
-````json
-{
-    "aliases": {
-        "BASEDIR": "${OT_DIR}/bazel-out/k8-fastbuild/bin"
-    },
-
-    "testdir": "${BASEDIR}/sw",
-
-    "default": {
-        "rom": "${BASEDIR}/sw/device/lib/testing/test_rom/test_rom_fpga_cw310.elf",
-        "otp": "${BASEDIR}/hw/ip/otp_ctrl/data/img_rma.24.vmem",
-        "timeout": 3,
-        "icount": 6
-    },
-
-    "include" : [
-        "**/*.fake_rsa_test_key_0.signed.bin"
-    ],
-
-    "exclude" : [
-        "alert_handler_*",
-        "ast_clk_out_*",
-        "clkmgr_off_*",
-        "i2c_*",
-        "manuf_cp_*",
-        "sensor_ctrl_*",
-        "spi_device_*",
-        "spi_passthru_*",
-        "usbdev_*"
-    ],
-
-    "suffixes": [
-        "_prog_fpga_cw310"
-    ],
-
-    "tests": {
-        "aes_idle_test": {
-            "opts": ["-global", "ot-aes.fast-mode=false"]
-        },
-        "alert_handler_lpg_reset_toggle_test" : {
-            "timeout": 10
-        },
-        "boot_data_functest": {
-            "icount": 1
-        },
-        "otbn_rsa_test": {
-            "timeout": 5
-        },
-        "mod_exp_otbn_functest_hardcoded": {
-            "icount": 0
-        },
-        "ecdsa_p256_functest": {
-            "timeout": 5
-        },
-        "ecdh_p256_functest": {
-            "timeout": 5
-        },
-        "entropy_src_csrng_test": {
-            "icount": ""
-        },
-        "csrng_edn_concurrency_test": {
-            "timeout": 15
-        },
-        "csrng_smoketest": {
-            "timeout": 5
-        }
-    }
-}
-````
-
-### HJSON syntax
-
-HJSON is a more user-friendly syntax that JSON. If the `hjson` module is available on the platform,
-this script uses it as a replacement for the system JSON module, hence supporting the improved
-syntax for configuration. It is encouraged to install the dependency-less HJSON module using a
-command such as `pip3 install hjson`.
+This script accepts HJSON configuration file to define how to run a test session.
+To install the dependency-less HJSON module, use a command such as `pip3 install hjson`.
 
 Sample config for running OpenTitan tests:
 ````hjson
@@ -596,12 +519,12 @@ The script returns the error code of the most occurring error, or success (0)
 
 ## Examples
 
-* The most typical usage requires a JSON configuration file and produces an output CSV file. `-vv`,
+* The most typical usage requires a HJSON configuration file and produces an output CSV file. `-vv`,
   that is the information log level, should be enough to track execution without getting too many
   log messages.
 
   ````sh
-  ./scripts/opentitan/pyot.py -vv -c pyot.json -w pyot.csv
+  ./scripts/opentitan/pyot.py -vv -c pyot.hjson -w pyot.csv
   ````
 
   Note that results can be live-tracked from another terminal using a command like the following:

hw/jtag/tap_ctrl_rbb.c

@@ -29,35 +29,22 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu/ctype.h"
-#include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
-#include "qemu/sockets.h"
 #include "qapi/error.h"
 #include "qom/object.h"
 #include "chardev/char-fe.h"
 #include "chardev/char.h"
-#include "exec/gdbstub.h"
-#include "exec/hwaddr.h"
-#include "hw/boards.h"
-#include "hw/cpu/cluster.h"
 #include "hw/jtag/tap_ctrl.h"
 #include "hw/jtag/tap_ctrl_rbb.h"
 #include "hw/qdev-properties-system.h"
 #include "hw/qdev-properties.h"
 #include "hw/resettable.h"
-#include "monitor/monitor.h"
-#include "semihosting/semihost.h"
-#include "sysemu/hw_accel.h"
-#include "sysemu/replay.h"
 #include "sysemu/runstate.h"
 #include "trace.h"
 
 
-/* clang-format off */
-
 typedef enum {
     TEST_LOGIC_RESET,
     RUN_TEST_IDLE,
@@ -78,10 +65,6 @@ typedef enum {
     _TAP_STATE_COUNT
 } TAPState;
 
-#define TAP_CTRL_BYPASS_INST  0
-
-/* clang-format on */
-
 typedef TapDataHandler *tap_ctrl_data_reg_extender_t(uint64_t value);
 
 typedef struct TapCtrlRbbState {
@@ -135,6 +118,8 @@ typedef struct _TAPProcess {
 #define DEFAULT_JTAG_BITBANG_PORT "3335"
 #define MAX_PACKET_LENGTH         4096u
 
+#define TAP_CTRL_BYPASS_INST 0u
+
 /*
  * TAP controller state machine state/event matrix
  *
@@ -173,13 +158,13 @@ static const char TAPFSM_NAMES[_TAP_STATE_COUNT][18U] = {
 static void tap_ctrl_rbb_idcode_capture(TapDataHandler *tdh);
 
 /* Common TAP instructions */
-static const TapDataHandler tap_ctrl_rbb_bypass = {
+static const TapDataHandler TAP_CTRL_RBB_BYPASS = {
     .name = "bypass",
     .length = 1,
     .value = 0,
 };
 
-static const TapDataHandler tap_ctrl_rbb_idcode = {
+static const TapDataHandler TAP_CTRL_RBB_IDCODE = {
     .name = "idcode",
     .length = 32,
     .capture = &tap_ctrl_rbb_idcode_capture,
@@ -669,10 +654,10 @@ static void tap_ctrl_rbb_realize(DeviceState *dev, Error **errp)
 
     size_t irslots = 1u << tap->ir_length;
     tap_ctrl_rbb_register_handler(tap, TAP_CTRL_BYPASS_INST,
-                                  &tap_ctrl_rbb_bypass, true);
-    tap_ctrl_rbb_register_handler(tap, tap->idcode_inst, &tap_ctrl_rbb_idcode,
+                                  &TAP_CTRL_RBB_BYPASS, true);
+    tap_ctrl_rbb_register_handler(tap, tap->idcode_inst, &TAP_CTRL_RBB_IDCODE,
                                   true);
-    tap_ctrl_rbb_register_handler(tap, irslots - 1u, &tap_ctrl_rbb_bypass,
+    tap_ctrl_rbb_register_handler(tap, irslots - 1u, &TAP_CTRL_RBB_BYPASS,
                                   true);
     /* special case for ID code: opaque store the constant idcode value */
     TapDataHandler *tdh = tap_ctrl_rbb_get_data_handler(tap, tap->idcode_inst);

hw/opentitan/ot_alert.c

@@ -757,9 +757,9 @@ static void ot_alert_signal_tx(void *opaque, int n, int level)
 
     trace_ot_alert_signal_class(s->ot_id, alert, ACLASS(nclass), class_en);
 
-    if (class_en) {
-        DVAL(s->regs.intr->state) |= 1u << nclass;
+    DVAL(s->regs.intr->state) |= 1u << nclass;
 
+    if (class_en) {
         /* saturate (no roll over) */
         if (DVAL(aclass->accum_cnt) < CLASS_ACCUM_CNT_MASK) {
             DVAL(aclass->accum_cnt) += 1u;

hw/opentitan/ot_dm_tl.c

@@ -53,6 +53,7 @@ struct OtDMTLState {
     uint64_t tl_base;
     uint32_t dmi_addr;
     unsigned dmi_size;
+    bool enable;
     uint8_t role;
 };
 
@@ -136,6 +137,7 @@ static Property ot_dm_tl_properties[] = {
     DEFINE_PROP_UINT64("tl_addr", OtDMTLState, tl_base, 0),
     DEFINE_PROP_LINK("tl_dev", OtDMTLState, tl_dev, TYPE_SYS_BUS_DEVICE,
                      SysBusDevice *),
+    DEFINE_PROP_BOOL("enable", OtDMTLState, enable, true),
     DEFINE_PROP_UINT8("role", OtDMTLState, role, UINT8_MAX),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -148,9 +150,10 @@ static void ot_dm_tl_reset(DeviceState *dev)
     g_assert(dmtl->dmi_size);
 
     if (!dmtl->dtm_ok) {
+        RISCVDTMClass *dtmc = RISCV_DTM_GET_CLASS(OBJECT(dmtl->dtm));
         dmtl->dtm_ok =
-            riscv_dtm_register_dm(DEVICE(dmtl->dtm), RISCV_DEBUG_DEVICE(dev),
-                                  dmtl->dmi_addr, dmtl->dmi_size);
+            (*dtmc->register_dm)(DEVICE(dmtl->dtm), RISCV_DEBUG_DEVICE(dev),
+                                 dmtl->dmi_addr, dmtl->dmi_size, dmtl->enable);
     }
 
     if (dmtl->dtm_ok) {