diff mbox series

[v5,3/4] acpi: add fw_cfg file for TPM and PPI virtual memory device

Message ID 20180626122343.13473-4-marcandre.lureau@redhat.com
State New
Headers show
Series Add support for TPM Physical Presence interface | expand

Commit Message

Marc-André Lureau June 26, 2018, 12:23 p.m. UTC 
From: Stefan Berger <stefanb@linux.vnet.ibm.com>

To avoid having to hard code the base address of the PPI virtual
memory device we introduce a fw_cfg file etc/tpm/config that holds the
base address of the PPI device, the version of the PPI interface and
the version of the attached TPM.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
[ Marc-André: renamed to etc/tpm/config, made it static, document it ]
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---

v4:
 - add ACPI only if PPI is enabled
v3:
 - renamed to etc/tpm/config, made it static, document it
---
 include/hw/acpi/tpm.h |  3 +++
 hw/i386/acpi-build.c  | 19 +++++++++++++++++++
 docs/specs/tpm.txt    | 20 ++++++++++++++++++++
 3 files changed, 42 insertions(+)

Comments

Igor Mammedov June 27, 2018, 12:01 p.m. UTC | #1 
On Tue, 26 Jun 2018 14:23:42 +0200
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> 
> To avoid having to hard code the base address of the PPI virtual
> memory device we introduce a fw_cfg file etc/tpm/config that holds the
> base address of the PPI device, the version of the PPI interface and
> the version of the attached TPM.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> [ Marc-André: renamed to etc/tpm/config, made it static, document it ]
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ---
> 
> v4:
>  - add ACPI only if PPI is enabled
> v3:
>  - renamed to etc/tpm/config, made it static, document it
> ---
>  include/hw/acpi/tpm.h |  3 +++
>  hw/i386/acpi-build.c  | 19 +++++++++++++++++++
>  docs/specs/tpm.txt    | 20 ++++++++++++++++++++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> index c082df7d1d..f79d68a77a 100644
> --- a/include/hw/acpi/tpm.h
> +++ b/include/hw/acpi/tpm.h
> @@ -193,4 +193,7 @@ REG32(CRB_DATA_BUFFER, 0x80)
>  #define TPM_PPI_ADDR_SIZE           0x400
>  #define TPM_PPI_ADDR_BASE           0xFED45000
>  
> +#define TPM_PPI_VERSION_NONE        0
> +#define TPM_PPI_VERSION_1_30        1
> +
>  #endif /* HW_ACPI_TPM_H */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9bc6d97ea1..d9320845ed 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -119,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
>      bool pcihp_bridge_en;
>  } AcpiBuildPciBusHotplugState;
>  
> +typedef struct FWCfgTPMConfig {
> +    uint32_t tpmppi_address;
is 32bit enough (what if on ARM or somewhere else area would be above 4Gb)?
to be future proof I'd make it 64bit field so we won't have to change ABI
later on.

> +    uint8_t tpm_version;
> +    uint8_t tpmppi_version;
> +} QEMU_PACKED FWCfgTPMConfig;
> +
>  static void init_common_fadt_data(Object *o, AcpiFadtData *data)
>  {
>      uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> @@ -2873,6 +2879,8 @@ void acpi_setup(void)
>      AcpiBuildTables tables;
>      AcpiBuildState *build_state;
>      Object *vmgenid_dev;
> +    TPMIf *tpm;
> +    static FWCfgTPMConfig tpm_config;
>  
>      if (!pcms->fw_cfg) {
>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> @@ -2907,6 +2915,17 @@ void acpi_setup(void)
what about vart-arm machine?
(it also has some TPM stuff but it doesn't use acpi_setup())
maybe add common helper and reuse it for both?


>      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>  
> +    tpm = tpm_find();
> +    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
> +        tpm_config = (FWCfgTPMConfig) {
> +            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
> +            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
Have it actually been tested on big endian host?

> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
ditto

(that's why I don't welcome packed structures in random places)

how about adding unit-tests to series to make sure that it works
across different hosts that travis builds on and that thing won't
fall apart in future?

> +        };
> +        fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
> +                        &tpm_config, sizeof tpm_config);
> +    }
> +
>      vmgenid_dev = find_vmgenid_dev();
>      if (vmgenid_dev) {
>          vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> index c230c4c93e..2ddb768084 100644
> --- a/docs/specs/tpm.txt
> +++ b/docs/specs/tpm.txt
> @@ -20,6 +20,26 @@ QEMU files related to TPM TIS interface:
>   - hw/tpm/tpm_tis.h
>  
>  
> += fw_cfg interface =
> +
> +The bios/firmware may use the "etc/tpm/config" fw_cfg entry for
> +configuring the guest appropriately.
> +
> +The entry of 6 bytes has the following content, in little-endian:
> +
> +    #define TPM_VERSION_UNSPEC          0
> +    #define TPM_VERSION_1_2             1
> +    #define TPM_VERSION_2_0             2
> +
> +    #define TPM_PPI_VERSION_NONE        0
> +    #define TPM_PPI_VERSION_1_30        1
> +
> +    struct FWCfgTPMConfig {
> +        uint32_t tpmppi_address;         /* PPI memory location */
> +        uint8_t tpm_version;             /* TPM version */
> +        uint8_t tpmppi_version;          /* PPI version */
> +    };
> +
>  = ACPI Interface =
>  
>  The TPM device is defined with ACPI ID "PNP0C31". QEMU builds a SSDT and passes
Stefan Berger June 27, 2018, 12:59 p.m. UTC | #2 
On 06/27/2018 08:01 AM, Igor Mammedov wrote:
> On Tue, 26 Jun 2018 14:23:42 +0200
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>
>> To avoid having to hard code the base address of the PPI virtual
>> memory device we introduce a fw_cfg file etc/tpm/config that holds the
>> base address of the PPI device, the version of the PPI interface and
>> the version of the attached TPM.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> [ Marc-André: renamed to etc/tpm/config, made it static, document it ]
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> ---
>>
>> v4:
>>   - add ACPI only if PPI is enabled
>> v3:
>>   - renamed to etc/tpm/config, made it static, document it
>> ---
>>   include/hw/acpi/tpm.h |  3 +++
>>   hw/i386/acpi-build.c  | 19 +++++++++++++++++++
>>   docs/specs/tpm.txt    | 20 ++++++++++++++++++++
>>   3 files changed, 42 insertions(+)
>>
>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>> index c082df7d1d..f79d68a77a 100644
>> --- a/include/hw/acpi/tpm.h
>> +++ b/include/hw/acpi/tpm.h
>> @@ -193,4 +193,7 @@ REG32(CRB_DATA_BUFFER, 0x80)
>>   #define TPM_PPI_ADDR_SIZE           0x400
>>   #define TPM_PPI_ADDR_BASE           0xFED45000
>>   
>> +#define TPM_PPI_VERSION_NONE        0
>> +#define TPM_PPI_VERSION_1_30        1
>> +
>>   #endif /* HW_ACPI_TPM_H */
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 9bc6d97ea1..d9320845ed 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -119,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
>>       bool pcihp_bridge_en;
>>   } AcpiBuildPciBusHotplugState;
>>   
>> +typedef struct FWCfgTPMConfig {
>> +    uint32_t tpmppi_address;
> is 32bit enough (what if on ARM or somewhere else area would be above 4Gb)?
> to be future proof I'd make it 64bit field so we won't have to change ABI
> later on.
>
>> +    uint8_t tpm_version;
>> +    uint8_t tpmppi_version;
>> +} QEMU_PACKED FWCfgTPMConfig;
>> +
>>   static void init_common_fadt_data(Object *o, AcpiFadtData *data)
>>   {
>>       uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
>> @@ -2873,6 +2879,8 @@ void acpi_setup(void)
>>       AcpiBuildTables tables;
>>       AcpiBuildState *build_state;
>>       Object *vmgenid_dev;
>> +    TPMIf *tpm;
>> +    static FWCfgTPMConfig tpm_config;
>>   
>>       if (!pcms->fw_cfg) {
>>           ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>> @@ -2907,6 +2915,17 @@ void acpi_setup(void)
> what about vart-arm machine?
> (it also has some TPM stuff but it doesn't use acpi_setup())
> maybe add common helper and reuse it for both?

I have never used ARM with it. I am not sure whether the firmware on ARM 
is instrumented to have support for PPI. If someone wanted to enable 
that, I would leave these parts up to them.

>
>
>>       fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>>                       tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>>   
>> +    tpm = tpm_find();
>> +    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
>> +        tpm_config = (FWCfgTPMConfig) {
>> +            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
>> +            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
> Have it actually been tested on big endian host?

At some point I did, yes.

>
>> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
> ditto
>
> (that's why I don't welcome packed structures in random places)

I thought a packed structure at least ensures that the offsets of the 
fields are agreed upon by 32 and 64bit, no ? So the firmware can use 64 
bit or 32bit and the offsets would be ensured.

>
> how about adding unit-tests to series to make sure that it works
> across different hosts that travis builds on and that thing won't
> fall apart in future?

>> +        };
>> +        fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
>> +                        &tpm_config, sizeof tpm_config);
>> +    }
>> +
>>       vmgenid_dev = find_vmgenid_dev();
>>       if (vmgenid_dev) {
>>           vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
>> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
>> index c230c4c93e..2ddb768084 100644
>> --- a/docs/specs/tpm.txt
>> +++ b/docs/specs/tpm.txt
>> @@ -20,6 +20,26 @@ QEMU files related to TPM TIS interface:
>>    - hw/tpm/tpm_tis.h
>>   
>>   
>> += fw_cfg interface =
>> +
>> +The bios/firmware may use the "etc/tpm/config" fw_cfg entry for
>> +configuring the guest appropriately.
>> +
>> +The entry of 6 bytes has the following content, in little-endian:
>> +
>> +    #define TPM_VERSION_UNSPEC          0
>> +    #define TPM_VERSION_1_2             1
>> +    #define TPM_VERSION_2_0             2
>> +
>> +    #define TPM_PPI_VERSION_NONE        0
>> +    #define TPM_PPI_VERSION_1_30        1
>> +
>> +    struct FWCfgTPMConfig {
>> +        uint32_t tpmppi_address;         /* PPI memory location */
>> +        uint8_t tpm_version;             /* TPM version */
>> +        uint8_t tpmppi_version;          /* PPI version */
>> +    };
>> +
>>   = ACPI Interface =
>>   
>>   The TPM device is defined with ACPI ID "PNP0C31". QEMU builds a SSDT and passes
Igor Mammedov June 27, 2018, 2:26 p.m. UTC | #3 
On Wed, 27 Jun 2018 08:59:55 -0400
Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:

> On 06/27/2018 08:01 AM, Igor Mammedov wrote:
> > On Tue, 26 Jun 2018 14:23:42 +0200
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >  
> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >>
> >> To avoid having to hard code the base address of the PPI virtual
> >> memory device we introduce a fw_cfg file etc/tpm/config that holds the
> >> base address of the PPI device, the version of the PPI interface and
> >> the version of the attached TPM.
> >>
> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> [ Marc-André: renamed to etc/tpm/config, made it static, document it ]
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> ---
> >>
> >> v4:
> >>   - add ACPI only if PPI is enabled
> >> v3:
> >>   - renamed to etc/tpm/config, made it static, document it
> >> ---
> >>   include/hw/acpi/tpm.h |  3 +++
> >>   hw/i386/acpi-build.c  | 19 +++++++++++++++++++
> >>   docs/specs/tpm.txt    | 20 ++++++++++++++++++++
> >>   3 files changed, 42 insertions(+)
> >>
> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> index c082df7d1d..f79d68a77a 100644
> >> --- a/include/hw/acpi/tpm.h
> >> +++ b/include/hw/acpi/tpm.h
> >> @@ -193,4 +193,7 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >>   #define TPM_PPI_ADDR_SIZE           0x400
> >>   #define TPM_PPI_ADDR_BASE           0xFED45000
> >>   
> >> +#define TPM_PPI_VERSION_NONE        0
> >> +#define TPM_PPI_VERSION_1_30        1
> >> +
> >>   #endif /* HW_ACPI_TPM_H */
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 9bc6d97ea1..d9320845ed 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -119,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
> >>       bool pcihp_bridge_en;
> >>   } AcpiBuildPciBusHotplugState;
> >>   
> >> +typedef struct FWCfgTPMConfig {
> >> +    uint32_t tpmppi_address;  
> > is 32bit enough (what if on ARM or somewhere else area would be above 4Gb)?
> > to be future proof I'd make it 64bit field so we won't have to change ABI
> > later on.
> >  
> >> +    uint8_t tpm_version;
> >> +    uint8_t tpmppi_version;
> >> +} QEMU_PACKED FWCfgTPMConfig;
> >> +
> >>   static void init_common_fadt_data(Object *o, AcpiFadtData *data)
> >>   {
> >>       uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> >> @@ -2873,6 +2879,8 @@ void acpi_setup(void)
> >>       AcpiBuildTables tables;
> >>       AcpiBuildState *build_state;
> >>       Object *vmgenid_dev;
> >> +    TPMIf *tpm;
> >> +    static FWCfgTPMConfig tpm_config;
> >>   
> >>       if (!pcms->fw_cfg) {
> >>           ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> >> @@ -2907,6 +2915,17 @@ void acpi_setup(void)  
> > what about vart-arm machine?
> > (it also has some TPM stuff but it doesn't use acpi_setup())
> > maybe add common helper and reuse it for both?  
> 
> I have never used ARM with it. I am not sure whether the firmware on ARM 
> is instrumented to have support for PPI. If someone wanted to enable 
> that, I would leave these parts up to them.
> 
> >
> >  
> >>       fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
> >>                       tables.tcpalog->data, acpi_data_len(tables.tcpalog));
> >>   
> >> +    tpm = tpm_find();
> >> +    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
> >> +        tpm_config = (FWCfgTPMConfig) {
> >> +            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
> >> +            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),  
> > Have it actually been tested on big endian host?  
> 
> At some point I did, yes.
> 
> >  
> >> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)  
> > ditto
> >
> > (that's why I don't welcome packed structures in random places)  
> 
> I thought a packed structure at least ensures that the offsets of the 
> fields are agreed upon by 32 and 64bit, no ? So the firmware can use 64 
> bit or 32bit and the offsets would be ensured.

I think layout for this structure is:
  0-3 : tpmppi_address
    4 : tpm_version
    5 : tpmppi_version

but that's not a problem,

  unit8_t foo = cpu_to_le32(bar)

wouldn't it produce different results depending on host endianness?

> 
> >
> > how about adding unit-tests to series to make sure that it works
> > across different hosts that travis builds on and that thing won't
> > fall apart in future?  
> 
> >> +        };
> >> +        fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
> >> +                        &tpm_config, sizeof tpm_config);
> >> +    }
> >> +
> >>       vmgenid_dev = find_vmgenid_dev();
> >>       if (vmgenid_dev) {
> >>           vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
> >> diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> >> index c230c4c93e..2ddb768084 100644
> >> --- a/docs/specs/tpm.txt
> >> +++ b/docs/specs/tpm.txt
> >> @@ -20,6 +20,26 @@ QEMU files related to TPM TIS interface:
> >>    - hw/tpm/tpm_tis.h
> >>   
> >>   
> >> += fw_cfg interface =
> >> +
> >> +The bios/firmware may use the "etc/tpm/config" fw_cfg entry for
> >> +configuring the guest appropriately.
> >> +
> >> +The entry of 6 bytes has the following content, in little-endian:
> >> +
> >> +    #define TPM_VERSION_UNSPEC          0
> >> +    #define TPM_VERSION_1_2             1
> >> +    #define TPM_VERSION_2_0             2
> >> +
> >> +    #define TPM_PPI_VERSION_NONE        0
> >> +    #define TPM_PPI_VERSION_1_30        1
> >> +
> >> +    struct FWCfgTPMConfig {
> >> +        uint32_t tpmppi_address;         /* PPI memory location */
> >> +        uint8_t tpm_version;             /* TPM version */
> >> +        uint8_t tpmppi_version;          /* PPI version */
> >> +    };
> >> +
> >>   = ACPI Interface =
> >>   
> >>   The TPM device is defined with ACPI ID "PNP0C31". QEMU builds a SSDT and passes  
> 
>
Marc-André Lureau June 28, 2018, 12:42 p.m. UTC | #4 
Hi

On Wed, Jun 27, 2018 at 4:26 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Wed, 27 Jun 2018 08:59:55 -0400
> Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
>
>> On 06/27/2018 08:01 AM, Igor Mammedov wrote:
>> > On Tue, 26 Jun 2018 14:23:42 +0200
>> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>> >
>> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> >>
>> >> To avoid having to hard code the base address of the PPI virtual
>> >> memory device we introduce a fw_cfg file etc/tpm/config that holds the
>> >> base address of the PPI device, the version of the PPI interface and
>> >> the version of the attached TPM.
>> >>
>> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> >> [ Marc-André: renamed to etc/tpm/config, made it static, document it ]
>> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >>
>> >> ---
>> >>
>> >> v4:
>> >>   - add ACPI only if PPI is enabled
>> >> v3:
>> >>   - renamed to etc/tpm/config, made it static, document it
>> >> ---
>> >>   include/hw/acpi/tpm.h |  3 +++
>> >>   hw/i386/acpi-build.c  | 19 +++++++++++++++++++
>> >>   docs/specs/tpm.txt    | 20 ++++++++++++++++++++
>> >>   3 files changed, 42 insertions(+)
>> >>
>> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>> >> index c082df7d1d..f79d68a77a 100644
>> >> --- a/include/hw/acpi/tpm.h
>> >> +++ b/include/hw/acpi/tpm.h
>> >> @@ -193,4 +193,7 @@ REG32(CRB_DATA_BUFFER, 0x80)
>> >>   #define TPM_PPI_ADDR_SIZE           0x400
>> >>   #define TPM_PPI_ADDR_BASE           0xFED45000
>> >>
>> >> +#define TPM_PPI_VERSION_NONE        0
>> >> +#define TPM_PPI_VERSION_1_30        1
>> >> +
>> >>   #endif /* HW_ACPI_TPM_H */
>> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> >> index 9bc6d97ea1..d9320845ed 100644
>> >> --- a/hw/i386/acpi-build.c
>> >> +++ b/hw/i386/acpi-build.c
>> >> @@ -119,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
>> >>       bool pcihp_bridge_en;
>> >>   } AcpiBuildPciBusHotplugState;
>> >>
>> >> +typedef struct FWCfgTPMConfig {
>> >> +    uint32_t tpmppi_address;
>> > is 32bit enough (what if on ARM or somewhere else area would be above 4Gb)?
>> > to be future proof I'd make it 64bit field so we won't have to change ABI
>> > later on.
>> >
>> >> +    uint8_t tpm_version;
>> >> +    uint8_t tpmppi_version;
>> >> +} QEMU_PACKED FWCfgTPMConfig;
>> >> +
>> >>   static void init_common_fadt_data(Object *o, AcpiFadtData *data)
>> >>   {
>> >>       uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
>> >> @@ -2873,6 +2879,8 @@ void acpi_setup(void)
>> >>       AcpiBuildTables tables;
>> >>       AcpiBuildState *build_state;
>> >>       Object *vmgenid_dev;
>> >> +    TPMIf *tpm;
>> >> +    static FWCfgTPMConfig tpm_config;
>> >>
>> >>       if (!pcms->fw_cfg) {
>> >>           ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>> >> @@ -2907,6 +2915,17 @@ void acpi_setup(void)
>> > what about vart-arm machine?
>> > (it also has some TPM stuff but it doesn't use acpi_setup())
>> > maybe add common helper and reuse it for both?
>>
>> I have never used ARM with it. I am not sure whether the firmware on ARM
>> is instrumented to have support for PPI. If someone wanted to enable
>> that, I would leave these parts up to them.
>>
>> >
>> >
>> >>       fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>> >>                       tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>> >>
>> >> +    tpm = tpm_find();
>> >> +    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
>> >> +        tpm_config = (FWCfgTPMConfig) {
>> >> +            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
>> >> +            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
>> > Have it actually been tested on big endian host?
>>
>> At some point I did, yes.
>>
>> >
>> >> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
>> > ditto
>> >
>> > (that's why I don't welcome packed structures in random places)
>>
>> I thought a packed structure at least ensures that the offsets of the
>> fields are agreed upon by 32 and 64bit, no ? So the firmware can use 64
>> bit or 32bit and the offsets would be ensured.
>
> I think layout for this structure is:
>   0-3 : tpmppi_address
>     4 : tpm_version
>     5 : tpmppi_version
>
> but that's not a problem,
>
>   unit8_t foo = cpu_to_le32(bar)
>
> wouldn't it produce different results depending on host endianness?

My understanding of C conversion from longer int to shorter ones is
that excessive higher order bits are dropped.

I think we could just remove the cpu_to_le32() call.
Igor Mammedov June 28, 2018, 1:06 p.m. UTC | #5 
On Thu, 28 Jun 2018 14:42:34 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Wed, Jun 27, 2018 at 4:26 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Wed, 27 Jun 2018 08:59:55 -0400
> > Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
> >  
> >> On 06/27/2018 08:01 AM, Igor Mammedov wrote:  
> >> > On Tue, 26 Jun 2018 14:23:42 +0200
> >> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >> >  
> >> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> >>
> >> >> To avoid having to hard code the base address of the PPI virtual
> >> >> memory device we introduce a fw_cfg file etc/tpm/config that holds the
> >> >> base address of the PPI device, the version of the PPI interface and
> >> >> the version of the attached TPM.
> >> >>
> >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> >> [ Marc-André: renamed to etc/tpm/config, made it static, document it ]
> >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >>
> >> >> ---
> >> >>
> >> >> v4:
> >> >>   - add ACPI only if PPI is enabled
> >> >> v3:
> >> >>   - renamed to etc/tpm/config, made it static, document it
> >> >> ---
> >> >>   include/hw/acpi/tpm.h |  3 +++
> >> >>   hw/i386/acpi-build.c  | 19 +++++++++++++++++++
> >> >>   docs/specs/tpm.txt    | 20 ++++++++++++++++++++
> >> >>   3 files changed, 42 insertions(+)
> >> >>
> >> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> >> index c082df7d1d..f79d68a77a 100644
> >> >> --- a/include/hw/acpi/tpm.h
> >> >> +++ b/include/hw/acpi/tpm.h
> >> >> @@ -193,4 +193,7 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >> >>   #define TPM_PPI_ADDR_SIZE           0x400
> >> >>   #define TPM_PPI_ADDR_BASE           0xFED45000
> >> >>
> >> >> +#define TPM_PPI_VERSION_NONE        0
> >> >> +#define TPM_PPI_VERSION_1_30        1
> >> >> +
> >> >>   #endif /* HW_ACPI_TPM_H */
> >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> >> index 9bc6d97ea1..d9320845ed 100644
> >> >> --- a/hw/i386/acpi-build.c
> >> >> +++ b/hw/i386/acpi-build.c
> >> >> @@ -119,6 +119,12 @@ typedef struct AcpiBuildPciBusHotplugState {
> >> >>       bool pcihp_bridge_en;
> >> >>   } AcpiBuildPciBusHotplugState;
> >> >>
> >> >> +typedef struct FWCfgTPMConfig {
> >> >> +    uint32_t tpmppi_address;  
> >> > is 32bit enough (what if on ARM or somewhere else area would be above 4Gb)?
> >> > to be future proof I'd make it 64bit field so we won't have to change ABI
> >> > later on.
> >> >  
> >> >> +    uint8_t tpm_version;
> >> >> +    uint8_t tpmppi_version;
> >> >> +} QEMU_PACKED FWCfgTPMConfig;
> >> >> +
> >> >>   static void init_common_fadt_data(Object *o, AcpiFadtData *data)
> >> >>   {
> >> >>       uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> >> >> @@ -2873,6 +2879,8 @@ void acpi_setup(void)
> >> >>       AcpiBuildTables tables;
> >> >>       AcpiBuildState *build_state;
> >> >>       Object *vmgenid_dev;
> >> >> +    TPMIf *tpm;
> >> >> +    static FWCfgTPMConfig tpm_config;
> >> >>
> >> >>       if (!pcms->fw_cfg) {
> >> >>           ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> >> >> @@ -2907,6 +2915,17 @@ void acpi_setup(void)  
> >> > what about vart-arm machine?
> >> > (it also has some TPM stuff but it doesn't use acpi_setup())
> >> > maybe add common helper and reuse it for both?  
> >>
> >> I have never used ARM with it. I am not sure whether the firmware on ARM
> >> is instrumented to have support for PPI. If someone wanted to enable
> >> that, I would leave these parts up to them.
> >>  
> >> >
> >> >  
> >> >>       fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
> >> >>                       tables.tcpalog->data, acpi_data_len(tables.tcpalog));
> >> >>
> >> >> +    tpm = tpm_find();
> >> >> +    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
> >> >> +        tpm_config = (FWCfgTPMConfig) {
> >> >> +            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
> >> >> +            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),  
> >> > Have it actually been tested on big endian host?  
> >>
> >> At some point I did, yes.
> >>  
> >> >  
> >> >> +            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)  
> >> > ditto
> >> >
> >> > (that's why I don't welcome packed structures in random places)  
> >>
> >> I thought a packed structure at least ensures that the offsets of the
> >> fields are agreed upon by 32 and 64bit, no ? So the firmware can use 64
> >> bit or 32bit and the offsets would be ensured.  
> >
> > I think layout for this structure is:
> >   0-3 : tpmppi_address
> >     4 : tpm_version
> >     5 : tpmppi_version
> >
> > but that's not a problem,
> >
> >   unit8_t foo = cpu_to_le32(bar)
> >
> > wouldn't it produce different results depending on host endianness?  
> 
> My understanding of C conversion from longer int to shorter ones is
> that excessive higher order bits are dropped.
> 
> I think we could just remove the cpu_to_le32() call.
Yep, that should fix the bug
diff mbox series

Patch

diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index c082df7d1d..f79d68a77a 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -193,4 +193,7 @@  REG32(CRB_DATA_BUFFER, 0x80)
 #define TPM_PPI_ADDR_SIZE           0x400
 #define TPM_PPI_ADDR_BASE           0xFED45000
 
+#define TPM_PPI_VERSION_NONE        0
+#define TPM_PPI_VERSION_1_30        1
+
 #endif /* HW_ACPI_TPM_H */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9bc6d97ea1..d9320845ed 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -119,6 +119,12 @@  typedef struct AcpiBuildPciBusHotplugState {
     bool pcihp_bridge_en;
 } AcpiBuildPciBusHotplugState;
 
+typedef struct FWCfgTPMConfig {
+    uint32_t tpmppi_address;
+    uint8_t tpm_version;
+    uint8_t tpmppi_version;
+} QEMU_PACKED FWCfgTPMConfig;
+
 static void init_common_fadt_data(Object *o, AcpiFadtData *data)
 {
     uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
@@ -2873,6 +2879,8 @@  void acpi_setup(void)
     AcpiBuildTables tables;
     AcpiBuildState *build_state;
     Object *vmgenid_dev;
+    TPMIf *tpm;
+    static FWCfgTPMConfig tpm_config;
 
     if (!pcms->fw_cfg) {
         ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
@@ -2907,6 +2915,17 @@  void acpi_setup(void)
     fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
                     tables.tcpalog->data, acpi_data_len(tables.tcpalog));
 
+    tpm = tpm_find();
+    if (tpm && object_property_get_bool(OBJECT(tpm), "ppi", &error_abort)) {
+        tpm_config = (FWCfgTPMConfig) {
+            .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE),
+            .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())),
+            .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE)
+        };
+        fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config",
+                        &tpm_config, sizeof tpm_config);
+    }
+
     vmgenid_dev = find_vmgenid_dev();
     if (vmgenid_dev) {
         vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
index c230c4c93e..2ddb768084 100644
--- a/docs/specs/tpm.txt
+++ b/docs/specs/tpm.txt
@@ -20,6 +20,26 @@  QEMU files related to TPM TIS interface:
  - hw/tpm/tpm_tis.h
 
 
+= fw_cfg interface =
+
+The bios/firmware may use the "etc/tpm/config" fw_cfg entry for
+configuring the guest appropriately.
+
+The entry of 6 bytes has the following content, in little-endian:
+
+    #define TPM_VERSION_UNSPEC          0
+    #define TPM_VERSION_1_2             1
+    #define TPM_VERSION_2_0             2
+
+    #define TPM_PPI_VERSION_NONE        0
+    #define TPM_PPI_VERSION_1_30        1
+
+    struct FWCfgTPMConfig {
+        uint32_t tpmppi_address;         /* PPI memory location */
+        uint8_t tpm_version;             /* TPM version */
+        uint8_t tpmppi_version;          /* PPI version */
+    };
+
 = ACPI Interface =
 
 The TPM device is defined with ACPI ID "PNP0C31". QEMU builds a SSDT and passes