Re: [RFC v2 4/7] hw/tpm: Implement TPM CRB chunking logic

[marcandre.lureau@redhat.com]

On Thu, 19 Mar 2026 19:23:13 +0530, Arun Menon <armenon@redhat.com> wrote:

Hi,

>
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 5ea1a4a9706..e61c04aee0b 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -80,6 +82,8 @@ enum crb_ctrl_req {
>  
>  enum crb_start {
>      CRB_START_INVOKE = BIT(0),
> +    CRB_START_RESP_RETRY = BIT(1),

Hmm, maybe we should rename that field, since it is "Start" in the spec (it changed?).

> +    CRB_START_NEXT_CHUNK = BIT(2),
>  };

And follow the spec naming here "RspRetry" -> RSP_RETRY

> @@ -122,6 +126,68 @@ static uint8_t tpm_crb_get_active_locty(CRBState *s)
> [ ... skip 23 lines ... ]
> +            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS, tpmSts, 1);
> +            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 0);
> +            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, nextChunk, 0);
> +            tpm_crb_clear_internal_buffers(s);
> +            error_report("Command size '%d' less than TPM header size '%d'",
> +                         total_request_size, TPM_HEADER_SIZE);

Use PRIu32 to avoid sign sign-mismatch

> [ ... skip 28 lines ... ]
> +    if (to_copy < CRB_CTRL_CMD_SIZE) {
> +        memset((guint8 *)mem + to_copy, 0, CRB_CTRL_CMD_SIZE - to_copy);
> +    }
> +
> +    s->response_offset += to_copy;
> +    memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);

Those lines are repeated below and could be factorized.

> @@ -152,20 +218,48 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
> [ ... skip 21 lines ... ]
> +            if (!(s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE)) {
> +                if (!tpm_crb_append_command_request(s)) {
> +                    break;
> +                }
> +                ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 1);
> +                g_byte_array_set_size(s->response_buffer, s->be_buffer_size);

This pre-allocates the response buffer before the backend has written
anything. Since response_buffer->len > 0 now, a subsequent nextChunk
from the guest would enter tpm_crb_fill_command_response() and serve
uninitialized data.

Before this patch we would serve guest input data, which wasn't great either.


We could add a separate flag to track response readiness instead. This
will need to be migrated too.

> +                s->cmd = (TPMBackendCmd) {
> +                    .in = s->command_buffer->data,
> +                    .in_len = s->command_buffer->len,
> +                    .out = s->response_buffer->data,
> +                    .out_len = s->response_buffer->len,
> +                };
> +                tpm_backend_deliver_request(s->tpmbe, &s->cmd);
> +            }
> +        } else if (val & CRB_START_NEXT_CHUNK) {
> +            /*
> +             * nextChunk is used both while sending and receiving data.
> +             * To distinguish between the two, response_buffer is checked

Missing .

> +             * If it does not have data, then that means we have not yet
> +             * sent the command to the tpm backend, and therefore call
> +             * tpm_crb_append_command_request()
> +             */
> +            if (s->response_buffer->len > 0 &&
> +                s->response_offset < s->response_buffer->len) {
> +                    tpm_crb_fill_command_response(s);

Indentation <4

> +            } else {
> +                if (!tpm_crb_append_command_request(s)) {
> +                    break;
> +                }
> +            }
> +            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, nextChunk, 0);
> +        } else if (val & CRB_START_RESP_RETRY) {
> +            if (s->response_buffer->len > 0) {

I'd suggest adding a trace here.

> @@ -205,13 +299,36 @@ static const MemoryRegionOps tpm_crb_memory_ops = {
> [ ... skip 15 lines ... ]
> +
> +        /*
> +         * Send the first chunk. Subsequent chunks will be sent using
> +         * tpm_crb_fill_command_response()
> +         */
> +        uint32_t to_copy = MIN(CRB_CTRL_CMD_SIZE, s->response_buffer->len);

QEMU coding style: declaration not mixed with statements.

> +        memcpy(mem, s->response_buffer->data, to_copy);
> +
> +        if (to_copy < CRB_CTRL_CMD_SIZE) {
> +            memset((guint8 *)mem + to_copy, 0, CRB_CTRL_CMD_SIZE - to_copy);
> +        }
> +        s->response_offset += to_copy;
>      }
>      memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);

Consider factorizing

> +    ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 0);

redundant clear

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>