Re: [PATCH v2 06/17] hw/riscv: add e-trace message helpers

[Konstantin Semichastnov <k.semichastnov@syntacore.com>]

On 11/11/25 14:46, Daniel Henrique Barboza wrote:
> Before making the trace encoder writing into the RAM sink we need a way
> to encode the messages/packets. The encoding is LSB (least significant
> bit) based. The doc "Efficient Trace for RISC-V", Chapter 7, mentions:
> 
> "The remainder of this section describes the contents of the payload
> portion which should be independent of the infrastructure. In each
> table, the fields are listed in transmission order: first field in
> the table is transmitted first, and multi-bit fields are transmitted
> LSB first."
> 
> The "RISC-V Trace Control Interface Specification" docs, Chapter 7,
> states when talking about the Trace RAM Sink:
> 
> "Trace data is placed in memory in LSB order (first byte of trace
> packet/data is placed on LSB)."
> 
> This means that the LSB encoding must be used to write into the RAM Sink
> memory, which is our goal.
> 
> The design we're going for is to have all these encoder helpers, along
> with the message formats, in a separated file. The trace encoder will
> make use of these helpers to blindly write a byte array with the packet
> desired, and then write it as is in the RAM Sink.
> 
> We'll start by modeling the synchronisation packet first, adding more
> formats as we increment the Trace Encoder capabilities.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   hw/riscv/meson.build         |  3 +-
>   hw/riscv/rv-trace-messages.c | 94 ++++++++++++++++++++++++++++++++++++
>   hw/riscv/rv-trace-messages.h | 25 ++++++++++
>   3 files changed, 121 insertions(+), 1 deletion(-)
>   create mode 100644 hw/riscv/rv-trace-messages.c
>   create mode 100644 hw/riscv/rv-trace-messages.h
> 
> diff --git a/hw/riscv/meson.build b/hw/riscv/meson.build
> index 2aadbe1e50..7d3576fcdf 100644
> --- a/hw/riscv/meson.build
> +++ b/hw/riscv/meson.build
> @@ -14,6 +14,7 @@ riscv_ss.add(when: 'CONFIG_RISCV_IOMMU', if_true: files(
>   	'riscv-iommu.c', 'riscv-iommu-pci.c', 'riscv-iommu-sys.c', 'riscv-iommu-hpm.c'))
>   riscv_ss.add(when: 'CONFIG_MICROBLAZE_V', if_true: files('microblaze-v-generic.c'))
>   riscv_ss.add(when: 'CONFIG_XIANGSHAN_KUNMINGHU', if_true: files('xiangshan_kmh.c'))
> -riscv_ss.add(when: 'CONFIG_RISCV_TRACE', if_true: files('trace-encoder.c', 'trace-ram-sink.c'))
> +riscv_ss.add(when: 'CONFIG_RISCV_TRACE', if_true: files('trace-encoder.c',
> +        'trace-ram-sink.c', 'rv-trace-messages.c'))
>   
>   hw_arch += {'riscv': riscv_ss}
> diff --git a/hw/riscv/rv-trace-messages.c b/hw/riscv/rv-trace-messages.c
> new file mode 100644
> index 0000000000..215135dd47
> --- /dev/null
> +++ b/hw/riscv/rv-trace-messages.c
> @@ -0,0 +1,94 @@
> +/*
> + * Helpers for RISC-V Trace Messages
> + *
> + * Copyright (C) 2025 Ventana Micro Systems Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "rv-trace-messages.h"
> +#include "qemu/bitops.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qapi/error.h"
> +#include "trace.h"
> +
> +typedef struct RVTraceMessageHeader {
> +    uint8_t length:5;
> +    uint8_t flow:2;
> +    uint8_t extend:1;
> +} RVTraceMessageHeader;
> +#define HEADER_SIZE 1
I suggest to move all such size macros
to single enum to keep them all in one place:

typedef enum {
     HEADER_SIZE              = 1,
     SYNC_PAYLOAD_SIZE_64BITS = 9,
     /* and other */
} RVTraceMessagePayloadSize;

> +
> +/*
> + * Format 3 subformat 0 without 'time' and 'context' fields
> + */
> +typedef struct RVTraceSyncPayload {
> +    uint8_t format:2;
> +    uint8_t subformat:2;
> +    uint8_t branch:1;
> +    uint8_t privilege:3;
> +    uint32_t addressLow;
> +    uint32_t addressHigh;
> +} RVTraceSyncPayload;
> +#define SYNC_PAYLOAD_SIZE_64BITS 9
> +
> +static void rv_etrace_write_bits(uint8_t *bytes, uint32_t bit_pos,
> +                                 uint32_t num_bits, uint32_t val)
Let's return num_bits here, so we could increment bit offset with return 
value.

> +{
> +    uint32_t pos, byte_index, byte_pos, byte_bits = 0;
> +
> +    if (!num_bits || 32 < num_bits) {
> +        return;
> +    }
> +
> +    for (pos = 0; pos < num_bits; pos += byte_bits) {
> +        byte_index = (bit_pos + pos) >> 3;
> +        byte_pos = (bit_pos + pos) & 0x7;
> +        byte_bits = (8 - byte_pos) < (num_bits - pos) ?
> +                    (8 - byte_pos) : (num_bits - pos);
> +        bytes[byte_index] &= ~(((1U << byte_bits) - 1) << byte_pos);
> +        bytes[byte_index] |= ((val >> pos) & ((1U << byte_bits) - 1)) << byte_pos;
I suggest to break this down a bit, because it very overloaded,
and this is very unclear without comments or references to
specification.

1. Let's add a reference to chapter 7 of e-trace spec in comment.

2. Let's use arithmetic operations to compute byte_index
and byte_pos:

     byte_index = (bit_pos + pos) / 8;
     byte_pos = (bit_pos + pos) % 8;

Compiler will optimize it anyway, but it is more clear what does
division and reminder mean compared to bitwise "shift" and "and".

3. Let's use macro MIN to comute byte_bits:

     byte_bits = MIN(8 - byte_pos, num_bits - pos);

4. Let's use extract32 and deposit32 to move bits from "val" to
"bytes" instead of manually shifting bits:

     uint8_t chunk = extract32(val, pos, byte_bits);
     bytes[byte_index] = deposit32(bytes[byte_index], byte_pos, 
byte_bits, chunk);


> +    }
> +}
> +
> +static void rv_etrace_write_header(uint8_t *buf, RVTraceMessageHeader header)
> +{
> +    /* flow and extend are always zero, i.e just write length */
> +    rv_etrace_write_bits(buf, 0, 5, header.length);
Flow and extend are, indeed, always zero, but we still need to write 
them to buffer.
Also, let's keep it as generic as possible, and write all fields from 
header, and not hardcoded zeroes.

     uint8_t bit_pos = 0;

     bit_pos += rv_etrace_write_bits(buf, bit_pos, 5, header.length);
     bit_pos += rv_etrace_write_bits(buf, bit_pos, 2, header.flow);
     bit_pos += rv_etrace_write_bits(buf, bit_pos, 1, header.extend);
> +}
> +
> +size_t rv_etrace_gen_encoded_sync_msg(uint8_t *buf, uint64_t pc,
> +                                      TracePrivLevel priv_level)
> +{
> +    RVTraceSyncPayload payload = {.format = 0b11,
> +                                  .subformat = 0b00,
> +                                  .branch = 1,
> +                                  .privilege = priv_level};
Let's assign addressLow and addressHigh right away:

     RVTraceSyncPayload payload = {.format = 0b11,
                                   .subformat = 0b00,
                                   .branch = 1,
                                   .privilege = priv_level,
                                   .addressLow = extract64(pc, 0, 32),
                                   .addressHigh = extract64(pc, 32, 32)};


> +    RVTraceMessageHeader header = {.flow = 0, .extend = 0,
> +                                   .length = SYNC_PAYLOAD_SIZE_64BITS};
> +    uint8_t bit_pos;
> +
> +    payload.addressLow = extract64(pc, 0, 32);
> +    payload.addressHigh = extract64(pc, 32, 32);
> +
> +    rv_etrace_write_header(buf, header);
> +    bit_pos = 8;
> +
> +    rv_etrace_write_bits(buf, bit_pos, 2, payload.format);
> +    bit_pos += 2;
> +    rv_etrace_write_bits(buf, bit_pos, 2, payload.subformat);
> +    bit_pos += 2;
> +    rv_etrace_write_bits(buf, bit_pos, 1, payload.branch);
> +    bit_pos += 1;
> +    rv_etrace_write_bits(buf, bit_pos, 3, payload.privilege);
> +    bit_pos += 3;
> +
> +    rv_etrace_write_bits(buf, bit_pos, 32, payload.addressLow);
> +    bit_pos += 32;
> +    rv_etrace_write_bits(buf, bit_pos, 32, payload.addressHigh);
Let's return num_bits value from rv_etrace_write_bits(), so we could 
just increment bit_pos with return value:


     uint8_t bit_pos = 0;

     bit_pos += rv_etrace_write_header(buf, header);

     bit_pos += rv_etrace_write_bits(buf, bit_pos, 2, payload.format);
     bit_pos += rv_etrace_write_bits(buf, bit_pos, 2, payload.subformat);
     bit_pos += rv_etrace_write_bits(buf, bit_pos, 1, payload.branch);
     bit_pos += rv_etrace_write_bits(buf, bit_pos, 3, payload.privilege);

     bit_pos += rv_etrace_write_bits(buf, bit_pos, 32,
                                     payload.addressLow);
     bit_pos += rv_etrace_write_bits(buf, bit_pos, 32,
                                     payload.addressHigh);

> +
> +    return HEADER_SIZE + SYNC_PAYLOAD_SIZE_64BITS;
> +}
> diff --git a/hw/riscv/rv-trace-messages.h b/hw/riscv/rv-trace-messages.h
> new file mode 100644
> index 0000000000..aeafea8849
> --- /dev/null
> +++ b/hw/riscv/rv-trace-messages.h
> @@ -0,0 +1,25 @@
> +/*
> + * Helpers for RISC-V Trace Messages
> + *
> + * Copyright (C) 2025 Ventana Micro Systems Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef RISCV_RV_TRACE_MESSAGES_H
> +#define RISCV_RV_TRACE_MESSAGES_H
> +
> +typedef enum {
> +    U = 0,
> +    S_HS = 1,
> +    RESERVED = 2,
> +    M = 3,
> +    D = 4,
> +    VU = 5,
> +    VS = 6,
> +} TracePrivLevel;
> +
> +size_t rv_etrace_gen_encoded_sync_msg(uint8_t *buf, uint64_t pc,
> +                                      TracePrivLevel priv_level);
> +
> +#endif