Skip to content

[DRAFT] Adapt kmac_app_agent to dynamic app interface#30230

Draft
rswarbrick wants to merge 6 commits into
lowRISC:masterfrom
rswarbrick:kmac-new-interface-dv
Draft

[DRAFT] Adapt kmac_app_agent to dynamic app interface#30230
rswarbrick wants to merge 6 commits into
lowRISC:masterfrom
rswarbrick:kmac-new-interface-dv

Conversation

@rswarbrick

@rswarbrick rswarbrick commented May 27, 2026

Copy link
Copy Markdown
Contributor

This DV change is designed to adapt the kmac_app_agent to a more sensible interface after the changes in #30143 were merged. I have split out several parts into separate PRs:

Once those are merged the remaining commit is as follows:

[kmac,dv] Rewrite kmac_app_agent to be more general

This work adapts the agent to match the newly defined interface with
minimal changes. This should allow OTBN to interact with KMAC through
the interface in a more complicated way than existing users.

The agent that was in place isn't really able to handle the tweaked
protocol, so this commit rewrites it to allow extending things
smoothly.

Upsettingly, the commit is enormous! I couldn't figure out how to
split it up into smaller pieces. :-(

Important changes:

  • The agent gets split in two ("kmac_app_device_agent" and
    "kmac_app_host_agent"). These have different sequencers,
    corresponding to the fact that they fundamentally drive different
    sorts of items.

  • We no longer use push_pull_agent. It was already a slightly awkward
    fit (I think) and can't really support the new protocol.

  • kmac_app_device_driver now watches app requests and creates
    sequence items to represent these requests. They get
    broadcast through m_req_port. That port is connected to the
    sequencer's m_req_fifo, which can be consumed by the sequence that
    sends responses.

  • Sequence items are a bit more structured:

    • A request is sent in multiple beats, which each get a
      kmac_app_req_item where the last has m_last set.

    • The items representing the beats of a request are grouped into
      a "packet", represented by a kmac_app_req_packet_item. This
      contains a queue with the kmac_app_req_item objects for the
      beats.

    • A response is represented by a kmac_app_rsp_item.

    • The monitor sees complete transactions (a request followed by a
      response) and represents this as a kmac_app_mon_item, which
      contains a kmac_app_req_packet_item and a kmac_app_rsp_item.

  • The signals in kmac_app_if are now driven through clocking blocks
    in both directions, depending on an if_mode variable.

  • We don't use phase sequences any more. I think this is not really
    recommended and we can just start the "device responder" sequences
    from the test: probably a bit clearer. For example, see
    keymgr_base_test.sv (three lines of code).

Minor ("while we're at it") changes:

  • It also exposes the request and response as inout ports, which
    makes it a bit easier to use. See hw/ip/keymgr/dv/tb.sv for an
    example of the change.

  • Any new or dramatically-altered classes use out-of-block
    definitions and I've added meaningful documentation comments to
    functions, tasks and class variables.

  • The imports in kmac_app_agent_pkg are a bit more precise (mainly
    because I was concentrating to make sure I got the right thing
    from the right place).

  • The StrbAlignLSB_A assertions in kmac_app_intf were more
    complicated than needed. Group them into a single assertion. Also,
    express the assertions with default clocking/disable to make them
    easier to understand.

  • Rather than using field macros for the new classes, I've defined
    do_print/do_copy manually. This is probably a bit more efficient at
    runtime and (more importantly) lets us be explicit about things
    like radixes. For example, kmac_app_req_item always uses
    hexadecimal for its m_data field.

  • Since the strobe line is always required to be contiguous and
    aligned to the lsb, kmac_app_req_item represents it as just the
    number of valid bytes (m_num_bytes). This makes some of the code
    deailing with the item a bit simpler.

    For example, see kmac_app_host_seq: handling partial words when
    cfg.inject_zero_in_host_strb is now rather less mysterious.

  • When porting the function from kmac_app_item, I renamed
    get_is_kmac_rsp_data_invalid to just is_kmac_rsp_data_invalid.
    There's no need for the "double accessor name".

  • Since I was having to touch the code anyway, I've made the way
    scoreboards consume items from the agent a bit simpler. For
    example, keymgr_scoreboard now has two imp imports (for requests
    and transactions) and doesn't have to manually maintain fifos in
    the same way.

@rswarbrick rswarbrick requested a review from etterli May 27, 2026 18:49

@etterli etterli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This first draft looks good! Most of the required changes to the other IPs can be found in the extra commits of #30143. But I assume you already know this :)

Comment thread hw/dv/sv/kmac_app_agent/kmac_app_host_driver.sv Outdated
Comment thread hw/ip/kmac/dv/env/kmac_env_pkg.sv Outdated
@rswarbrick rswarbrick force-pushed the kmac-new-interface-dv branch 3 times, most recently from cc20f07 to d57a440 Compare June 2, 2026 16:18
@rswarbrick

Copy link
Copy Markdown
Contributor Author

Force-push rebases onto latest version of #30195

@rswarbrick

Copy link
Copy Markdown
Contributor Author

I've just rebased this PR past the enormous changes that have landed (and tweaks to the DV code from the design team). I'm going to allow this to run through CI and then will review/hack on the code at this end a little bit until I'm convinced it's ready for review.

@etterli

etterli commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

In case you are looking for the initial smoke test of the dynamic app interface, you can find it on this branch: https://github.com/etterli/opentitan/tree/otbn-kmac-if-combo

@rswarbrick

Copy link
Copy Markdown
Contributor Author

This first force-push is prompted by trying to improve kmac_app_req_item, with the following changes:

  • Implement do_compare for kmac_app_req_item
  • Use print_field instead of print_field_int in kmac_app_req_item for
    fields that might be wider than 64 bits.
  • Improve name for kmac_app_req_packet_item::get_share_byte_queue
  • Make kmac_app_req_item::add_share_to_byte_queue use unsigned bytes
  • Improve documentation around byte_length_c to explain when partial requests are ok.
  • Drop a stray "unsigned" from m_last.
  • Constrain m_delay not to be ridiculously large.
  • Add m_using_masked_interface to kmac_app_host_seq to configure
    whether m_data_s1 can be nonzero, and whether partial beats are
    allowed in the middle of a burst.
  • Allow termination to with an explicit empty message.
  • Configure m_using_masked_interface when starting a kmac_app_host_seq
    from kmac_base_vseq.

@rswarbrick rswarbrick force-pushed the kmac-new-interface-dv branch from 1cdfaf1 to 69b9f76 Compare June 12, 2026 17:17
This probably shouldn't be a signed value (we want bytes that contain
129, not -127!)

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
There's no need to encode the name of the environment into agents that
it instantiates.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
These queues contain digests, rather than shares. I suspect the
original name was because it's a queue of digests which, in turn, have
multiple shares, so it's a "share digest" queue. But the name looks a
lot like it's a queue of shares, which is wrong.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
No functional change, but this matches the conventional name used in
OpenTitan for interfaces.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
@rswarbrick

Copy link
Copy Markdown
Contributor Author

Changes in the latest force-push:

  • Fix the silly signedness problems that caused CI failures last time
  • Split out some of the changes in the big commit into precursors in the PR (to be reviewed and merged separately, I hope!)
  • Fix fields missing from uvm_object_utils calls in
    kmac_app_agent_cfg. Rather than adding the fields to the macro
    calls, implement do_print, do_copy and do_compare.
  • Fix grammar errors in description of zero_delays and rsp_digest_hs.
  • Association fix in kmac_app_host_seq: I had written A && B || C when
    I meant A && (B || C).
  • Fix a width mismatch in kmac_app_device_driver, which was using
    AppDigestW for a width of a signal that is actually MsgStrbW bits
    wide.

@rswarbrick rswarbrick force-pushed the kmac-new-interface-dv branch from 69b9f76 to fea2cf9 Compare June 12, 2026 19:00
This should be a little cleaner than the version we were using before.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
@rswarbrick rswarbrick force-pushed the kmac-new-interface-dv branch from fea2cf9 to 73df79f Compare June 12, 2026 19:41
@rswarbrick

Copy link
Copy Markdown
Contributor Author

Latest force-push pulls the change to the connections to kmac_app_if into a separate commit.

@rswarbrick

Copy link
Copy Markdown
Contributor Author

Latest force-push addresses a few minor things, and also fixes a silly error in keymgr_env:

  • Fix broken name in keymgr_env (I messed up a rebase and undid a
    rename from an earlier commit)

  • Fix description of the DoneAssertAfterLast_A assertion in
    kmac_app_if. This is talking about a "done" signal, which has now
    been renamed to "rsp_valid".

  • Improve radix for printing various fields in kmac_app_req_item and
    kmac_app_rsp_item.

  • Add do_compare for kmac_app_rsp_item and add a constraint on the
    delay between request and response.

  • Add do_compare to kmac_app_req_packet_item

@rswarbrick rswarbrick force-pushed the kmac-new-interface-dv branch from 73df79f to 5a589a0 Compare June 12, 2026 20:48
This work adapts the agent to match the newly defined interface with
minimal changes. This should allow OTBN to interact with KMAC through
the interface in a more complicated way than existing users.

The agent that was in place isn't really able to handle the tweaked
protocol, so this commit rewrites it to allow extending things
smoothly.

Upsettingly, the commit is enormous! I couldn't figure out how to
split it up into smaller pieces. :-(

Important changes:

 - The agent gets split in two ("kmac_app_device_agent" and
   "kmac_app_host_agent"). These have different sequencers,
   corresponding to the fact that they fundamentally drive different
   sorts of items.

 - We no longer use push_pull_agent. It was already a slightly awkward
   fit (I think) and can't really support the new protocol.

 - kmac_app_device_driver now watches app requests and creates
   sequence items to represent these requests. They get
   broadcast through m_req_port. That port is connected to the
   sequencer's m_req_fifo, which can be consumed by the sequence that
   sends responses.

 - Sequence items are a bit more structured:

     + A request is sent in multiple beats, which each get a
       kmac_app_req_item where the last has m_last set.

     + The items representing the beats of a request are grouped into
       a "packet", represented by a kmac_app_req_packet_item. This
       contains a queue with the kmac_app_req_item objects for the
       beats.

     + A response is represented by a kmac_app_rsp_item.

     + The monitor sees complete transactions (a request followed by a
       response) and represents this as a kmac_app_mon_item, which
       contains a kmac_app_req_packet_item and a kmac_app_rsp_item.

 - The signals in kmac_app_if are now driven through clocking blocks
   in both directions, depending on an if_mode variable.

 - We don't use phase sequences any more. I think this is not really
   recommended and we can just start the "device responder" sequences
   from the test: probably a bit clearer. For example, see
   keymgr_base_test.sv (three lines of code).

Minor ("while we're at it") changes:

 - It also exposes the request and response as inout ports, which
   makes it a bit easier to use. See hw/ip/keymgr/dv/tb.sv for an
   example of the change.

 - Any new or dramatically-altered classes use out-of-block
   definitions and I've added meaningful documentation comments to
   functions, tasks and class variables.

 - The imports in kmac_app_agent_pkg are a bit more precise (mainly
   because I was concentrating to make sure I got the right thing
   from the right place).

 - The StrbAlignLSB_A assertions in kmac_app_intf were more
   complicated than needed. Group them into a single assertion. Also,
   express the assertions with default clocking/disable to make them
   easier to understand.

 - Rather than using field macros for the new classes, I've defined
   do_print/do_copy manually. This is probably a bit more efficient at
   runtime and (more importantly) lets us be explicit about things
   like radixes. For example, kmac_app_req_item always uses
   hexadecimal for its m_data field.

 - Since the strobe line is always required to be contiguous and
   aligned to the lsb, kmac_app_req_item represents it as just the
   number of valid bytes (m_num_bytes). This makes some of the code
   deailing with the item a bit simpler.

   For example, see kmac_app_host_seq: handling partial words when
   cfg.inject_zero_in_host_strb is now rather less mysterious.

 - When porting the function from kmac_app_item, I renamed
   get_is_kmac_rsp_data_invalid to just is_kmac_rsp_data_invalid.
   There's no need for the "double accessor name".

 - Since I was having to touch the code anyway, I've made the way
   scoreboards consume items from the agent a bit simpler. For
   example, keymgr_scoreboard now has two imp imports (for requests
   and transactions) and doesn't have to manually maintain fifos in
   the same way.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
@rswarbrick rswarbrick force-pushed the kmac-new-interface-dv branch from 5a589a0 to fc7404e Compare June 12, 2026 20:52
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

Successfully merging this pull request may close these issues.

2 participants