Re: [RFC v2 5/7] test/qtest: Add test for tpm crb chunking

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

Hi

On Thu, Mar 26, 2026 at 3:27 PM <marcandre.lureau@redhat.com> wrote:
>
> On Thu, 19 Mar 2026 19:23:14 +0530, Arun Menon <armenon@redhat.com> wrote:
>
> Hi
>
> >
> > diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
> > index 2cb2dd47961..dfc1fd70b7f 100644
> > --- a/tests/qtest/tpm-util.c
> > +++ b/tests/qtest/tpm-util.c
> > @@ -14,16 +14,42 @@
> >
> >  #include "qemu/osdep.h"
> >  #include <glib/gstdio.h>
> > +#include "system/memory.h"
>
> Use qemu/bswap.h instead
>
> >
> >  #include "hw/acpi/tpm.h"
> >  #include "libqtest.h"
> >  #include "tpm-util.h"
> >  #include "qobject/qdict.h"
> >
> > +#define CRB_ADDR_START (TPM_CRB_ADDR_BASE + A_CRB_CTRL_START)
> > +#define CRB_CTRL_STS (TPM_CRB_ADDR_BASE + A_CRB_CTRL_STS)
> > +#define CRB_CTRL_CMD_SIZE (TPM_CRB_ADDR_BASE + A_CRB_CTRL_CMD_SIZE)
>
> Shadows tpm.h macro. Use CRB_ADDR_CTRL_STS ?
>
> > +
> > +#define BIT_START_INVOKE  (1 << 0)
>
> Similar (although in tpm_crb.c unit)
>
> > [ ... skip 11 lines ... ]
> > +            break;
> > +        }
> > +        if (g_get_monotonic_time() >= end_time) {
> > +            break;
> > +        }
> > +    }
>
> Consider adding a g_assert() here
>
> > @@ -31,24 +57,74 @@ void tpm_util_crb_transfer(QTestState *s,
> > [ ... skip 30 lines ... ]
> > +
> > +    qtest_writeb(s, TPM_CRB_ADDR_BASE + A_CRB_LOC_CTRL, 1);
> > +
> > +    for (size_t i = 0 ; i < req_size; i += chunk_size) {
> > +        bool last_chunk = false;
> > +        size_t current_chunk_size = chunk_size ;
>
> Trailing whitespace before the semicolon.
>
> > +
> > +        if (i + chunk_size > req_size) {
> > +            last_chunk = true;
> > +            current_chunk_size = req_size - i;
> >          }
> > -        if (g_get_monotonic_time() >= end_time) {
> > -            break;
> > +
> > +        qtest_memwrite(s, caddr, req + i, current_chunk_size);
> > +
> > +        if (last_chunk) {
> > +            qtest_writel(s, CRB_ADDR_START , BIT_START_INVOKE);
>
> Trailing whitespace before the comma.
>
> Style: should be tpm_sts per QEMU naming conventions.
> Style: declarations should be at the top of the function.
> Same: declaration mixed with statements.
>
> Style: should be tpm_sts per QEMU naming conventions.
> Style: declarations should be at the top of the function.
> Same: declaration mixed with statements.
>

This is the first time I used "b4 review tui", this is strange. I'll
check what happened :)

I'd recommend others to try it, especially those like me who lack a
good "all-in-terminal" workflow for reviews.