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

[Arun Menon <armenon@redhat.com>]

Hi Marc-André,

Thank you for the review.

On Thu, Mar 26, 2026 at 03:27:02PM +0400, marcandre.lureau@redhat.com wrote:
> 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?).

The Start has not changed from before.
Yes, I shall make changes to the other 2.

> 
> > +    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

Yes. Will do.

> 
> > [ ... 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.

Yes, I will move it into a separate function

> 
> > @@ -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.

Good catch. I did not think of what happens when the guest issues a
subsequent nextChunk while the tpm backend has not yet processed the
command.

I am wondering, checking if the start invoke bit is set will alone
prevent such a situation in the first place.

For example adding the following at the beginning,

case A_CRB_CTRL_START:
    if (s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE) {
        break;
    }

That way, a subsequent nextChunk or rspRetry will not be processed in
the first place. Do you see any problem with this approach? This will
also allow us to keep the migration logic as is.

> 
> > +                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 .

Will correct.

> 
> > +             * 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

Yes.

> 
> > +            } 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.

Sure, will add.

> 
> > @@ -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.

I will add this to the top of the block.

> 
> > +        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

yes, shall remove.

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

Regards,
Arun Menon