Re: [PATCH v5 05/14] tpm_crb: move ACPI table building to device interface

[Joelle van Dyne]

On Tue, Nov 14, 2023 at 8:44 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 11/14/23 11:37, Stefan Berger wrote:
> >
> >
> > On 11/13/23 21:09, Joelle van Dyne wrote:
> >> This logic is similar to TPM TIS ISA device. Since TPM CRB can only
> >> support TPM 2.0 backends, we check for this in realize.
> >>
> >> Signed-off-by: Joelle van Dyne <j@getutm.app>
> >> ---
> >>   hw/tpm/tpm_crb.h        |  2 ++
> >>   hw/i386/acpi-build.c    | 16 +---------------
> >>   hw/tpm/tpm_crb.c        | 16 ++++++++++++++++
> >>   hw/tpm/tpm_crb_common.c | 19 +++++++++++++++++++
> >>   4 files changed, 38 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h
> >> index 36863e1664..e6a86e3fd1 100644
> >> --- a/hw/tpm/tpm_crb.h
> >> +++ b/hw/tpm/tpm_crb.h
> >> @@ -73,5 +73,7 @@ void tpm_crb_init_memory(Object *obj, TPMCRBState
> >> *s, Error **errp);
> >>   void tpm_crb_mem_save(TPMCRBState *s, uint32_t *saved_regs, void
> >> *saved_cmdmem);
> >>   void tpm_crb_mem_load(TPMCRBState *s, const uint32_t *saved_regs,
> >>                         const void *saved_cmdmem);
> >> +void tpm_crb_build_aml(TPMIf *ti, Aml *scope, uint32_t baseaddr,
> >> uint32_t size,
> >> +                       bool build_ppi);
> >>
> >>   #endif /* TPM_TPM_CRB_H */
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 80db183b78..7491cee2af 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -1792,21 +1792,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >>
> >>   #ifdef CONFIG_TPM
> >>       if (TPM_IS_CRB(tpm)) {
> >> -        dev = aml_device("TPM");
> >> -        aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
> >> -        aml_append(dev, aml_name_decl("_STR",
> >> -                                      aml_string("TPM 2.0 Device")));
> >> -        crs = aml_resource_template();
> >> -        aml_append(crs, aml_memory32_fixed(TPM_CRB_ADDR_BASE,
> >> -                                           TPM_CRB_ADDR_SIZE,
> >> AML_READ_WRITE));
> >> -        aml_append(dev, aml_name_decl("_CRS", crs));
> >> -
> >> -        aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
> >> -        aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> >> -
> >> -        tpm_build_ppi_acpi(tpm, dev);
> >> -
> >> -        aml_append(sb_scope, dev);
> >> +        call_dev_aml_func(DEVICE(tpm), scope);
> >
> > For an x86_64 VM we have to call it directly otherwise the ACPI table
> > won't be there.
> >
> >          tpm_crb_build_aml(tpm, sb_scope, TPM_CRB_ADDR_BASE,
> > TPM_CRB_ADDR_SIZE, true);
> >
> >
> I looks like a good place for the moved code would be in hw/acpi/tpm.c

The change in v5 is that we call "call_dev_aml_func" which calls
"tpm_crb_build_aml" from tpm_crb.c (later moved to tpm_crb_common.c).
I think it's a better place for it because TPM TIS has a different
ACPI table and both can be handled correctly by call_dev_aml_func. In
the previous versions when we moved TPM CRB to ISA there was no need
to manually call "call_dev_aml_func" because the function for building
the ISA bus called it. However, I think this is still better in case
it is decided in the future to move the CRB device on ISA or another
bus and it makes the code more manageable with more TPM types.