diff mbox series

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

Message ID 20231114020927.62315-6-j@getutm.app (mailing list archive)
State New, archived
Headers show
Series tpm: introduce TPM CRB SysBus device | expand

Commit Message

Joelle van Dyne Nov. 14, 2023, 2:09 a.m. UTC 
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(-)

Comments

Stefan Berger Nov. 14, 2023, 4:37 p.m. UTC | #1 
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);
Stefan Berger Nov. 14, 2023, 4:44 p.m. UTC | #2 
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
Joelle van Dyne Nov. 14, 2023, 7:29 p.m. UTC | #3 
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.
diff mbox series

Patch

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);
     }
 #endif
 
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 99c64dd72a..8d57295b15 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -19,6 +19,8 @@ 
 #include "qemu/module.h"
 #include "qapi/error.h"
 #include "exec/address-spaces.h"
+#include "hw/acpi/acpi_aml_interface.h"
+#include "hw/acpi/tpm.h"
 #include "hw/qdev-properties.h"
 #include "hw/pci/pci_ids.h"
 #include "hw/acpi/tpm.h"
@@ -121,6 +123,11 @@  static void tpm_crb_none_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (tpm_crb_none_get_version(TPM_IF(s)) != TPM_VERSION_2_0) {
+        error_setg(errp, "TPM CRB only supports TPM 2.0 backends");
+        return;
+    }
+
     tpm_crb_init_memory(OBJECT(s), &s->state, errp);
 
     /* only used for migration */
@@ -142,10 +149,17 @@  static void tpm_crb_none_realize(DeviceState *dev, Error **errp)
     }
 }
 
+static void build_tpm_crb_none_aml(AcpiDevAmlIf *adev, Aml *scope)
+{
+    tpm_crb_build_aml(TPM_IF(adev), scope, TPM_CRB_ADDR_BASE, TPM_CRB_ADDR_SIZE,
+                      true);
+}
+
 static void tpm_crb_none_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     TPMIfClass *tc = TPM_IF_CLASS(klass);
+    AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
 
     dc->realize = tpm_crb_none_realize;
     device_class_set_props(dc, tpm_crb_none_properties);
@@ -154,6 +168,7 @@  static void tpm_crb_none_class_init(ObjectClass *klass, void *data)
     tc->model = TPM_MODEL_TPM_CRB;
     tc->get_version = tpm_crb_none_get_version;
     tc->request_completed = tpm_crb_none_request_completed;
+    adevc->build_dev_aml = build_tpm_crb_none_aml;
 
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
@@ -166,6 +181,7 @@  static const TypeInfo tpm_crb_none_info = {
     .class_init  = tpm_crb_none_class_init,
     .interfaces = (InterfaceInfo[]) {
         { TYPE_TPM_IF },
+        { TYPE_ACPI_DEV_AML_IF },
         { }
     }
 };
diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c
index f96a8cf299..09ca55eece 100644
--- a/hw/tpm/tpm_crb_common.c
+++ b/hw/tpm/tpm_crb_common.c
@@ -241,3 +241,22 @@  void tpm_crb_mem_load(TPMCRBState *s, const uint32_t *saved_regs,
     memcpy(regs, saved_regs, A_CRB_DATA_BUFFER);
     memcpy(&regs[R_CRB_DATA_BUFFER], saved_cmdmem, CRB_CTRL_CMD_SIZE);
 }
+
+void tpm_crb_build_aml(TPMIf *ti, Aml *scope, uint32_t baseaddr, uint32_t size,
+                       bool build_ppi)
+{
+    Aml *dev, *crs;
+
+    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")));
+    aml_append(dev, aml_name_decl("_UID", aml_int(1)));
+    aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
+    crs = aml_resource_template();
+    aml_append(crs, aml_memory32_fixed(baseaddr, size, AML_READ_WRITE));
+    aml_append(dev, aml_name_decl("_CRS", crs));
+    if (build_ppi) {
+        tpm_build_ppi_acpi(ti, dev);
+    }
+    aml_append(scope, dev);
+}