[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [for-4.17 PATCH] acpi: Add TPM2 interface definition and make the TPM version configurable.


  • To: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>, "jbeulich@xxxxxxxx" <jbeulich@xxxxxxxx>, "wl@xxxxxxx" <wl@xxxxxxx>, Roger Pau Monne <roger.pau@xxxxxxxxxx>, Henry Wang <Henry.Wang@xxxxxxx>
  • From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • Date: Tue, 6 Sep 2022 14:18:51 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=h33p7WuFVa8KR5uNEATucKr0vqD/jF7WN4N9ZgbkFzw=; b=DN8mJgkUcg6q5ppLqtQVEZt1zfUvuK9JeNmlgrV6wWYUEtkZbSNgMLF1v7D4Ifde66uzZwdpHIsJlsIi2it2EJI5BjYZzwp/fbFztT0V15J67cA+nR0T3e9BXUpduGNjElf0fjroPb8CP25mVMvZD08TnqpTQnjqszP1U4kQ3PhUYz4D48KsG4tpSivBkLWt3orXIOlnyTnXJo0tNjJBcr3E/7Bv0UBj9SbB90DVpWrv/ejvcnCEpqy+4z3HqqEpUb165dQDiI4L+zTXbQVE6WDAknA3A2CBqRZ09mObdw8ogna3mdozkN8atqozJOKB+VMDy4GUQSiJhCCPTTD4Sw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VsTdL1ut+ZKIqBvEOA9e4m87TW0ALk1Gt2oPLTjj2ST2Cd/uPSxVIyqcaLY5rrlKdNOWq1tv0sqDUAztWqmDSe3sl2VywmsFz85cca3gAKVaZqsDDVJiGwBAcdBVec/dW7UoWyc1HQkRxkkDYTu+5jU0ZJS6ZHlwDUzNi+I79SVvDhOUmmHiVRtPyLzVx00ov++FPC/Ry9V8m8H6ysX1JBm9r6D6OXNowS/4jmgwe0VyN67Qr3Uq82YroNy0bkF71QyzoWcyMK716RBR5knW+rZOWCWuQ1LPo3OIYqVy9ge/lRk7dwGrTy5U+SF9YSvr3ccADdvSIFTlKdqjq1Ua6w==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 06 Sep 2022 21:50:39 +0000
  • Ironport-data: A9a23:lOwV2qsZPaLtP6jJHIAchoSVEOfnVGNfMUV32f8akzHdYApBsoF/q tZmKW2CafuCZDHxKI10Od6zpxsO7JeGzIcwTwU9qi89F38Q+JbJXdiXEBz9bniYRiHhoOOLz Cm8hv3odp1coqr0/0/1WlTZhSAgk/vOHtIQMcacUghpXwhoVSw9vhxqnu89k+ZAjMOwRgiAo rsemeWGULOe82MyYzl8B56r8ks15qyj4G1A5DTSWNgQ1LPgvyhNZH4gDfnZw0vQGuF8AuO8T uDf+7C1lkuxE8AFU47Nfh7TKyXmc5aKVeS8oiM+t5uK23CukhcawKcjXMfwXG8M49m/c3Kd/ /0W3XC4YV9B0qQhA43xWTEAe811FfUuFLMqvRFTGCFcpqHLWyKE/hlgMK05FY4W2dYpXDFSy eIBIQEsbhWliqWUxK3uH4GAhux7RCXqFKU2nyk8iBTmV7MhS52FRLjW79hF2jt2ntpJAfvVe 8seb3xocQjEZBpMfFwQDfrSns/x3iW5L2Ie9wnT/PVpi4TQ5FUZPLzFGdzZYNGVA+5SmV6Vv Dnu9GXlGBAKcteYzFJp91rz17KTxH6rAer+EpW1quRIgnnKl1ANCQ07U1zl+duFrkShDoc3x 0s8v3BGQbIJ3E6hQ8T5Xha4iGWZpRNaUN1Ve8Ul7Cmdx6yS5ByWbkAkQzhbeZoZvck5bTUw0 xmCmNaBLSxitviZRGyQ8p+QrCiuIm4FIGkafygGQAAZpd75r+kOYgnnS99iFOuwkYfzEDSon zSS9nFh2fMUkNIB0Li98RbfmTWwq5PVTwkzoALKQmai6QA/b4mgD2C11WXmAT97BN7xZjG8U LIswKByMMhm4UmxqRGw
  • Ironport-hdrordr: A9a23:U/49xqOSfzJHqMBcT2L155DYdb4zR+YMi2TDiHoddfUFSKalfp 6V98jzjSWE8wr4WBkb6LO90DHpewKQyXcH2/hqAV7EZnirhILIFvAp0WKG+VHd8kLFh4lgPM tbEpSWTeeAdWSS7vyKrzVQcexQpuVvmZrA7Yix854ud3ASV0gK1XYaNu/vKDwTeOAwP+tdKH Pz3Kp6jgvlXU5SQtWwB3EDUeSGjcbMjojabRkPAANiwBWSjBuzgYSKUiSw71M7aXdi0L0i+W /Kn0jS/aO4qcy2zRfayiv684lWot380dFObfb8yvT9aw+cyTpAVr4RHoFqjwpF5N1HL2xa1+ Ukli1QffibLUmhOF1d7yGdgjUImwxelkMKgWXo/UcL5/aJCg7SQvAx+76wOHHimjUdlcA536 RR022DsZ1LSRvGgSTm/tDNEwpnj0yuvBMZ4KcuZlFkIPwjgYVq3Poi1VIQFI1FEDPx6YghHu UrBMbA5OxOeVffa3zCpGFgzNGlQ3x2R369MwM/k93Q1yITkGFyzkMeysBalnAc9IglQ50B4+ jfKKxnmLxHU8dTZ6NgA+UKR9exFwX2MFrxGXPXJU6iGLAMOnrLpZKy6LIp5PuycJhN15c2kI SpaiItiYfzQTOaNSSj5uw6zvmWehTNYd3E8LAs27Fp/rvhWbHsLSqPDFgzjsrImYRsPvHm
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYwfuWslla8fUdWk+aob/m3t7waQ==
  • Thread-topic: [for-4.17 PATCH] acpi: Add TPM2 interface definition and make the TPM version configurable.

On 30/08/2022 21:27, Jennifer Herbert wrote:
> This patch introduces an optional TPM 2 interface definition to the ACPI 
> table,
> which is to be used as part of a vTPM 2 implementation.
> To enable the new interface - I have made the TPM interface version
> configurable in the acpi_config, with the default being the existing 
> 1.2.(TCPA)
> I have also added to hvmloader an option to utilise this new config, which can
> be triggered by setting the platform/tpm_verion xenstore key.
>
> Signed-off-by: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>

We're past the 4.17 feature submission deadline.  CC'ing Henry.

Henry: This is a fairly simple change and a critical building block for
getting Windows 11 support on Xen.  Given that feature freeze was
slipped several weeks for other reasons, this should be considered for
inclusion too.

Jenny: This needs splitting up into at least 2 patches.  Patch 1 should
be the rename of ACPI_HAS_{TCPA => TPM} and introduction of tpm_version
(inc suitable rearranging).  Patch 2 should be the introduction of TPM2.

> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index 581b35e5cf..e3af32581b 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -994,13 +994,24 @@ void hvmloader_acpi_build_tables(struct acpi_config 
> *config,
>      if ( !strncmp(xenstore_read("platform/acpi_laptop_slate", "0"), "1", 1)  
> )
>          config->table_flags |= ACPI_HAS_SSDT_LAPTOP_SLATE;
>  
> -    config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC |
> +    config->table_flags |= (ACPI_HAS_TPM | ACPI_HAS_IOAPIC |
>                              ACPI_HAS_WAET | ACPI_HAS_PMTIMER |
>                              ACPI_HAS_BUTTONS | ACPI_HAS_VGA |
>                              ACPI_HAS_8042 | ACPI_HAS_CMOS_RTC);
>      config->acpi_revision = 4;
>  
> -    config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
> +    if ( !strncmp(xenstore_read("platform/tpm_version", "0"), "2", 1)  ) {

Brace on new line.

Also, this is a new key, so needs an entry in
docs/misc/xenstore-paths.pandoc

> +
> +        config->tpm_version = 2;
> +        config->crb_hdr = (uint16_t *)ACPI_CRB_HDR_ADDRESS;
> +        config->tis_hdr = NULL;
> +    }
> +    else
> +    {
> +        config->tpm_version = 1;
> +        config->crb_hdr = NULL;
> +        config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
> +    }

This logic makes any value, including "0" mean "use TPM 1", which isn't
terribly good.  Furthermore, ACPI_HAS_TPM doesn't mean "has a TPM", it
means "probe for a TPM".

So what this actually wants to be is something more like this:

s = xenstore_read("platform/tpm-version");
config->tpm_version = stroll(s, NULL, 0);

switch ( config->tpm_version )
{
case 1:
    config->table_flags |= ACPI_HAS_TPM;
    config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
    break;
}

You don't need to set the NULL values because config is suitably zeroed
to begin with, and patch 2 will just add

case 2:
    config->table_flags |= ACPI_HAS_TPM;
    config->crb_hdr = (uint16_t *)ACPI_CRB_HDR_ADDRESS;
    break;

> diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
> index 2619ba32db..5754daa985 100644
> --- a/tools/libacpi/acpi2_0.h
> +++ b/tools/libacpi/acpi2_0.h
> @@ -121,6 +121,28 @@ struct acpi_20_tcpa {
>  };
>  #define ACPI_2_0_TCPA_LAML_SIZE (64*1024)
>  
> +/*
> + * TPM2
> + */
> +struct Acpi20TPM2 {

acpi_20_tpm2, consistent with everything else here.

> diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
> index fe2db66a62..478cbec5dd 100644
> --- a/tools/libacpi/build.c
> +++ b/tools/libacpi/build.c
> @@ -409,38 +412,86 @@ static int construct_secondary_tables(struct acpi_ctxt 
> *ctxt,
>          memcpy(ssdt, ssdt_laptop_slate, sizeof(ssdt_laptop_slate));
>          table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt);
>      }
> -
> -    /* TPM TCPA and SSDT. */
> -    if ( (config->table_flags & ACPI_HAS_TCPA) &&
> -         (config->tis_hdr[0] != 0 && config->tis_hdr[0] != 0xffff) &&
> -         (config->tis_hdr[1] != 0 && config->tis_hdr[1] != 0xffff) )
> +    /* TPM and SSDT. */
> +    if (config->table_flags & ACPI_HAS_TPM)
>      {

Style, here and lower down.

The end result wants to look something like:

if ( config->table_flags & ACPI_HAS_TPM )
{
    switch ( config->tpm_version )
    {
    case 1:
        if ( !config->tis_hdr || config->tis_hdr[0] == 0 ||
config->tis_hdr[0] == 0xffff )
            break;

        ssdt =
        ...
        break;
    }
}

In particular, I don't think the printf()'s are particularly useful for
bad internal input into a probe function.

> -        ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_tpm), 16);
> -        if (!ssdt) return -1;
> -        memcpy(ssdt, ssdt_tpm, sizeof(ssdt_tpm));
> -        table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt);
> -
> -        tcpa = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_tcpa), 16);
> -        if (!tcpa) return -1;
> -        memset(tcpa, 0, sizeof(*tcpa));
> -        table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, tcpa);
> -
> -        tcpa->header.signature = ACPI_2_0_TCPA_SIGNATURE;
> -        tcpa->header.length    = sizeof(*tcpa);
> -        tcpa->header.revision  = ACPI_2_0_TCPA_REVISION;
> -        fixed_strcpy(tcpa->header.oem_id, ACPI_OEM_ID);
> -        fixed_strcpy(tcpa->header.oem_table_id, ACPI_OEM_TABLE_ID);
> -        tcpa->header.oem_revision = ACPI_OEM_REVISION;
> -        tcpa->header.creator_id   = ACPI_CREATOR_ID;
> -        tcpa->header.creator_revision = ACPI_CREATOR_REVISION;
> -        if ( (lasa = ctxt->mem_ops.alloc(ctxt, ACPI_2_0_TCPA_LAML_SIZE, 16)) 
> != NULL )
> +        if (config-> tpm_version == 2)
> +        {
> +            if ( (config->crb_hdr) &&
> +                   (config->crb_hdr[0] != 0 && config->crb_hdr[0] != 0xffff))
> +            {
> +                ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_tpm2), 16);
> +                if (!ssdt) return -1;
> +                memcpy(ssdt, ssdt_tpm2, sizeof(ssdt_tpm2));
> +                table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt);
> +
> +                tpm2 = ctxt->mem_ops.alloc(ctxt, sizeof(struct Acpi20TPM2), 
> 16);
> +                if (!tpm2) return -1;
> +                memset(tpm2, 0, sizeof(*tpm2));
> +                table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, tpm2);
> +
> +                tpm2->header.signature = ACPI_2_0_TPM2_SIGNATURE;
> +                tpm2->header.length    = sizeof(*tpm2);
> +                tpm2->header.revision  = ACPI_2_0_TPM2_REVISION;
> +                fixed_strcpy(tpm2->header.oem_id, ACPI_OEM_ID);
> +                fixed_strcpy(tpm2->header.oem_table_id, ACPI_OEM_TABLE_ID);
> +                tpm2->header.oem_revision = ACPI_OEM_REVISION;
> +                tpm2->header.creator_id   = ACPI_CREATOR_ID;
> +                tpm2->header.creator_revision = ACPI_CREATOR_REVISION;
> +                tpm2->platform_class = TPM2_ACPI_CLASS_CLIENT;
> +                tpm2->control_area_address = TPM_CRB_ADDR_CTRL;
> +                tpm2->start_method = TPM2_START_METHOD_CRB;
> +                tpm2->log_area_minimum_length = TPM_LOG_AREA_MINIMUM_SIZE;
> +
> +                log = ctxt->mem_ops.alloc(ctxt, TPM_LOG_SIZE, 4096);
> +                if (!log) return -1;

This is buggy.

APCI table memory is covered by an E820_ACPI range (specifically, is
eligible to be used as general RAM after boot), while the TPM log should
be in an E820_RESERVED region.

To start with, it's probably fine to hardcode something in the 2M window
at 0xfed40000 to be fixed location for the log.

~Andrew

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.