Re: [PATCH v4 01/10] hw/tpm: Add TPM CRB chunking fields

[Arun Menon <armenon@redhat.com>]

Hi Marc-André,

Thanks for the review.

On Fri, Apr 17, 2026 at 06:51:14PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Apr 17, 2026 at 6:36 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >
> >
> >
> > On 4/17/26 6:05 AM, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Tue, Apr 14, 2026 at 12:30 PM Arun Menon <armenon@redhat.com> wrote:
> > >>
> > >> From: Arun Menon <armenon@redhat.com>
> > >>
> > >> - Add new fields to the CRB Interface Identifier and the CRB
> > >>    Control Start registers.
> > >> - CRB_CTRL_START now has 2 new settings, that can be toggled using the
> > >>    nextChunk and crbRspRetry bits.
> > >> - CapCRBChunk bit (10) was Reserved1 previously. The field is reused in
> > >>    this revision of the specification.
> > >> - Refer to section 6.4.2.2 of [1]
> > >>
> > >> [1] https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p07_Pub.pdf
> > >>
> > >> Signed-off-by: Arun Menon <armenon@redhat.com>
> > >> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > >> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >> ---
> > >>   hw/tpm/tpm_crb.c      | 3 +++
> > >>   include/hw/acpi/tpm.h | 5 ++++-
> > >>   2 files changed, 7 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > >> index 8723536f93..0a1c7ecdc6 100644
> > >> --- a/hw/tpm/tpm_crb.c
> > >> +++ b/hw/tpm/tpm_crb.c
> > >> @@ -59,6 +59,7 @@ DECLARE_INSTANCE_CHECKER(CRBState, CRB,
> > >>   #define CRB_INTF_CAP_FIFO_NOT_SUPPORTED 0b0
> > >>   #define CRB_INTF_CAP_CRB_SUPPORTED 0b1
> > >>   #define CRB_INTF_IF_SELECTOR_CRB 0b1
> > >> +#define CRB_INTF_CAP_CRB_CHUNK 0b1
> > >>
> > >>   #define CRB_CTRL_CMD_SIZE (TPM_CRB_ADDR_SIZE - A_CRB_DATA_BUFFER)
> > >>
> > >> @@ -262,6 +263,8 @@ static void tpm_crb_reset(void *dev)
> > >>                        CapCRB, CRB_INTF_CAP_CRB_SUPPORTED);
> > >>       ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> > >>                        InterfaceSelector, CRB_INTF_IF_SELECTOR_CRB);
> > >> +    ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> > >> +                     CapCRBChunk, CRB_INTF_CAP_CRB_CHUNK);
> > >
> > > We should not expose that new capability to guests without an explicit
> > > "cap-chunk=on".
> >
> > You mean put this on the QEMU command line? This new capability is
> > really only needed if the user chooses PQC algorithm support (but then I
> > was also going to allow for 7kb writes to NVRAM spaces rather than 1kb
> > writes so far) and if the user chooses a swtpm profile with such support
> > but then fails to set this flag, some transmissions won't work. What
> > would be the problem if we exposed this new feature all the time and if
> > we can avoid state backwards-incompatibilities, which IMO is the most
> > important aspect, if we detect that chunking was not used by the driver
> 
> In general, we shouldn't suddenly expose device capabilities to
> guests. Starting with 11.1 machine, the cap-chunk feature can be
> enabled by default though.

You are right, we do not need to keep track of 2 properties. I will
remove migrate_buffers hw_compat property.

IIUC, you meant we need to expose cap-chunk i.e. remove "x-".
And the whole logic of exposing it based on machine type should also be
moved to earlier i.e. [PATCH v4 01/10] hw/tpm: Add TPM CRB chunking fields
instead of [PATCH v4 06/10] hw/tpm: Add support for VM migration with TPM CRB chunking.

> 
> > and only write additional state if found absolutely necessary? I would
> > prefer to untangle the user's choice of enabled algorithm from the
> > choice of such a flag. But then there's the aspect of OS drivers that
> > could use chunking even when not absolutely necessary (?).
> 

Reards,
Arun Menon