Re: [PATCH 06/11] tpm_crb: move ACPI table building to device interface

[Stefan Berger <stefanb@linux.ibm.com>]

On 7/13/23 14:10, Joelle van Dyne wrote:
> In that case, do you think we should have a check in "realize" to make
> sure the backend is 2.0?
> 
Maybe. I think at the moment it would simply not work (with existing drivers) without terminating QEMU on it due to the misconfiguration. On libvirt level we intercept this case and notify the user that the combination doesn't work. Leaving it like this would be an option...

    Stefan

> On Thu, Jul 13, 2023 at 9:08 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 7/12/23 23:51, Joelle van Dyne wrote:
>>> This logic is similar to TPM TIS ISA device.
>>>
>>> Signed-off-by: Joelle van Dyne <j@getutm.app>
>>> ---
>>>    hw/i386/acpi-build.c | 23 -----------------------
>>>    hw/tpm/tpm_crb.c     | 28 ++++++++++++++++++++++++++++
>>>    2 files changed, 28 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index 9c74fa17ad..b767df39df 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -1441,9 +1441,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>>>        uint32_t nr_mem = machine->ram_slots;
>>>        int root_bus_limit = 0xFF;
>>>        PCIBus *bus = NULL;
>>> -#ifdef CONFIG_TPM
>>> -    TPMIf *tpm = tpm_find();
>>> -#endif
>>>        bool cxl_present = false;
>>>        int i;
>>>        VMBusBridge *vmbus_bridge = vmbus_bridge_find();
>>> @@ -1793,26 +1790,6 @@ 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);
>>> -    }
>>> -#endif
>>> -
>>>        if (pcms->sgx_epc.size != 0) {
>>>            uint64_t epc_base = pcms->sgx_epc.base;
>>>            uint64_t epc_size = pcms->sgx_epc.size;
>>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
>>> index 6144081d30..14feb9857f 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"
>>> @@ -116,10 +118,34 @@ static void tpm_crb_isa_realize(DeviceState *dev, Error **errp)
>>>        }
>>>    }
>>>
>>> +static void build_tpm_crb_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
>>> +{
>>> +    Aml *dev, *crs;
>>> +    CRBState *s = CRB(adev);
>>> +    TPMIf *ti = TPM_IF(s);
>>> +
>>> +    dev = aml_device("TPM");
>>> +    if (tpm_crb_isa_get_version(ti) == TPM_VERSION_2_0) {
>>> +        aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
>>> +        aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device")));
>>> +    } else {
>>> +        aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
>>> +    }
>>
>> CRB only exists for TPM 2.0 and that's why we didn't have a different case here before.
>>
>> CRB only has MSFT0101: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_crb.c#L820
>> TIS has PNP0C31: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_tis.c
>>
>> You should remove the check for TPM_VERSION_2_0.
>>
>>      Stefan
>>> +    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(TPM_CRB_ADDR_BASE, TPM_CRB_ADDR_SIZE,
>>> +                                      AML_READ_WRITE));
>>> +    aml_append(dev, aml_name_decl("_CRS", crs));
>>> +    tpm_build_ppi_acpi(ti, dev);
>>> +    aml_append(scope, dev);
>>> +}
>>> +
>>>    static void tpm_crb_isa_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_isa_realize;
>>>        device_class_set_props(dc, tpm_crb_isa_properties);
>>> @@ -128,6 +154,7 @@ static void tpm_crb_isa_class_init(ObjectClass *klass, void *data)
>>>        tc->model = TPM_MODEL_TPM_CRB;
>>>        tc->get_version = tpm_crb_isa_get_version;
>>>        tc->request_completed = tpm_crb_isa_request_completed;
>>> +    adevc->build_dev_aml = build_tpm_crb_isa_aml;
>>>
>>>        set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>>    }
>>> @@ -139,6 +166,7 @@ static const TypeInfo tpm_crb_isa_info = {
>>>        .class_init  = tpm_crb_isa_class_init,
>>>        .interfaces = (InterfaceInfo[]) {
>>>            { TYPE_TPM_IF },
>>> +        { TYPE_ACPI_DEV_AML_IF },
>>>            { }
>>>        }
>>>    };