Re: [PATCH v3 2/2] acpi: Add TPM2 interface definition.
On 03.05.2023 01:29, Jennifer Herbert wrote:
> On 02/05/2023 14:41, Jan Beulich wrote:
>> On 25.04.2023 19:47, Jennifer Herbert wrote:
>>> --- a/tools/libacpi/acpi2_0.h
>>> +++ b/tools/libacpi/acpi2_0.h
>>> @@ -121,6 +121,36 @@ struct acpi_20_tcpa {
>>> };
>>> #define ACPI_2_0_TCPA_LAML_SIZE (64*1024)
>>>
>>> +/*
>>> + * TPM2
>>> + */
>> Nit: While I'm willing to accept the comment style violation here as
>> (apparently) intentional, ...
>
> Well, I was trying to keep the file consistant. As far as I can tell,
> this styling is used thoughout the file - unless I'm misunderstanding
> your 'Nit'. (You object to a multi-line coment used for a single line? )
> But I'm codes style blind, so just say how you want it.
Right - strictly speaking those multi-line comments all ought to be single-
line ones, but aiui they're multi-line intentionally so they stand out.
Hence - as you say, for consistency - it's okay for this one to follow
suit.
>>> --- /dev/null
>>> +++ b/tools/libacpi/ssdt_tpm2.asl
>>> @@ -0,0 +1,36 @@
>>> +/*
>>> + * ssdt_tpm2.asl
>>> + *
>>> + * Copyright (c) 2018-2022, Citrix Systems, Inc.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU Lesser General Public License as published
>>> + * by the Free Software Foundation; version 2.1 only. with the special
>>> + * exception on linking described in file LICENSE.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU Lesser General Public License for more details.
>>> + */
>> While the full conversion to SPDX was done in the hypervisor only so far,
>> I think new tool stack source files would better use the much shorter
>> SPDX equivalent, too.
>
> OK, this is where I get a bit confused. I belive I copied the licence
> from ssdt_tpm.asl, for consistancy.
>
> So I think i need to use a 'LGPL-2.1-only' but then it says its using
> exceptions on linking as discribed in LICENSE, but um, which LICENSE
> file? So i'm not sure what exception I should be adding. Do you know?
First of all I think commit 68823df358e8 ("acpi: Re-license ACPI builder
files from GPLv2 to LGPLv2.1") was wrong using LICENSE; I'm pretty sure
COPYING was meant instead. And indeed the difference between libacpi's
COPYING and LICENCES/LGPL-2.1 look to be formatting plus an extra section
at the bottom of the latter; I haven't found any "special exception on
linking" anywhere. IOW I think using LGPL-2.1 here is what is wanted
(unlike e.g. GPL-2.0-only there's no LGPL-2.1-only afaics), the more that
you're contributing a new file (and of course provided you're okay to put
the new file under that license).
Jan
Re: [PATCH v3 2/2] acpi: Add TPM2 interface definition.
On 02/05/2023 14:41, Jan Beulich wrote:
On 25.04.2023 19:47, Jennifer Herbert wrote:
--- a/tools/libacpi/acpi2_0.h
+++ b/tools/libacpi/acpi2_0.h
@@ -121,6 +121,36 @@ struct acpi_20_tcpa {
};
#define ACPI_2_0_TCPA_LAML_SIZE (64*1024)
+/*
+ * TPM2
+ */
Nit: While I'm willing to accept the comment style violation here as
(apparently) intentional, ...
Well, I was trying to keep the file consistant. As far as I can tell,
this styling is used thoughout the file - unless I'm misunderstanding
your 'Nit'. (You object to a multi-line coment used for a single line? )
But I'm codes style blind, so just say how you want it.
+struct acpi_20_tpm2 {
+struct acpi_header header;
+uint16_t platform_class;
+uint16_t reserved;
+uint64_t control_area_address;
+uint32_t start_method;
+uint8_t start_method_params[12];
+uint32_t log_area_minimum_length;
+uint64_t log_area_start_address;
+};
+#define TPM2_ACPI_CLASS_CLIENT 0
+#define TPM2_START_METHOD_CRB 7
+
+/* TPM register I/O Mapped region, location of which defined in the
+ * TCG PC Client Platform TPM Profile Specification for TPM 2.0.
+ * See table 9 - Only Locality 0 is used here. This is emulated by QEMU.
+ * Definition of Register space is found in table 12.
+ */
... this comment wants adjusting to hypervisor style (/* on its own line),
as that looks to be the aimed-at style in this file.
Will do.
@@ -352,6 +353,7 @@ static int construct_secondary_tables(struct acpi_ctxt
*ctxt,
struct acpi_20_tcpa *tcpa;
unsigned char *ssdt;
void *lasa;
+struct acpi_20_tpm2 *tpm2;
Could I talk you into moving this up by two lines, such that it'll be
adjacent to "tcpa"?
No problem.
@@ -450,6 +452,43 @@ static int construct_secondary_tables(struct acpi_ctxt
*ctxt,
tcpa->header.length);
}
break;
+
+case 2:
+/* Check VID stored in bits 37:32 (3rd 16 bit word) of CRB
+ * identifier register. See table 16 of TCG PC client platform
+ * TPM profile specification for TPM 2.0.
+ */
Nit: This comment again wants a style adjustment.
ok
--- /dev/null
+++ b/tools/libacpi/ssdt_tpm2.asl
@@ -0,0 +1,36 @@
+/*
+ * ssdt_tpm2.asl
+ *
+ * Copyright (c) 2018-2022, Citrix Systems, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ */
While the full conversion to SPDX was done in the hypervisor only so far,
I think new tool stack source files would better use the much shorter
SPDX equivalent, too.
OK, this is where I get a bit confused. I belive I copied the licence
from ssdt_tpm.asl, for consistancy.
So I think i need to use a 'LGPL-2.1-only' but then it says its using
exceptions on linking as discribed in LICENSE, but um, which LICENSE
file? So i'm not sure what exception I should be adding. Do you know?
Then on top of Jason's R-b,
Acked-by: Jan Beulich
Jan
Thanks,
-jenny
Re: [PATCH v3 2/2] acpi: Add TPM2 interface definition.
On 25.04.2023 19:47, Jennifer Herbert wrote:
> --- a/tools/libacpi/acpi2_0.h
> +++ b/tools/libacpi/acpi2_0.h
> @@ -121,6 +121,36 @@ struct acpi_20_tcpa {
> };
> #define ACPI_2_0_TCPA_LAML_SIZE (64*1024)
>
> +/*
> + * TPM2
> + */
Nit: While I'm willing to accept the comment style violation here as
(apparently) intentional, ...
> +struct acpi_20_tpm2 {
> +struct acpi_header header;
> +uint16_t platform_class;
> +uint16_t reserved;
> +uint64_t control_area_address;
> +uint32_t start_method;
> +uint8_t start_method_params[12];
> +uint32_t log_area_minimum_length;
> +uint64_t log_area_start_address;
> +};
> +#define TPM2_ACPI_CLASS_CLIENT 0
> +#define TPM2_START_METHOD_CRB 7
> +
> +/* TPM register I/O Mapped region, location of which defined in the
> + * TCG PC Client Platform TPM Profile Specification for TPM 2.0.
> + * See table 9 - Only Locality 0 is used here. This is emulated by QEMU.
> + * Definition of Register space is found in table 12.
> + */
... this comment wants adjusting to hypervisor style (/* on its own line),
as that looks to be the aimed-at style in this file.
> @@ -352,6 +353,7 @@ static int construct_secondary_tables(struct acpi_ctxt
> *ctxt,
> struct acpi_20_tcpa *tcpa;
> unsigned char *ssdt;
> void *lasa;
> +struct acpi_20_tpm2 *tpm2;
Could I talk you into moving this up by two lines, such that it'll be
adjacent to "tcpa"?
> @@ -450,6 +452,43 @@ static int construct_secondary_tables(struct acpi_ctxt
> *ctxt,
> tcpa->header.length);
> }
> break;
> +
> +case 2:
> +/* Check VID stored in bits 37:32 (3rd 16 bit word) of CRB
> + * identifier register. See table 16 of TCG PC client platform
> + * TPM profile specification for TPM 2.0.
> + */
Nit: This comment again wants a style adjustment.
> --- /dev/null
> +++ b/tools/libacpi/ssdt_tpm2.asl
> @@ -0,0 +1,36 @@
> +/*
> + * ssdt_tpm2.asl
> + *
> + * Copyright (c) 2018-2022, Citrix Systems, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + */
While the full conversion to SPDX was done in the hypervisor only so far,
I think new tool stack source files would better use the much shorter
SPDX equivalent, too.
Then on top of Jason's R-b,
Acked-by: Jan Beulich
Jan
Re: [PATCH v3 2/2] acpi: Add TPM2 interface definition.
On 26/04/2023 21:29, Jason Andryuk wrote: On Tue, Apr 25, 2023 at 1:48 PM 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. Signed-off-by: Jennifer Herbert --- ... diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c index f39a8e584f..51272530fe 100644 --- a/tools/firmware/hvmloader/util.c +++ b/tools/firmware/hvmloader/util.c @@ -1009,6 +1009,15 @@ void hvmloader_acpi_build_tables(struct acpi_config *config, config->table_flags |= ACPI_HAS_TPM; config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS; break; + +case 2: +config->table_flags |= ACPI_HAS_TPM; +config->crb_id = (uint16_t *)TPM_CRB_INTF_ID; + +mem_hole_populate_ram(TPM_LOG_AREA_ADDRESS >> PAGE_SHIFT, + TPM_LOG_SIZE >> PAGE_SHIFT); +memset((void *)TPM_LOG_AREA_ADDRESS, 0, TPM_LOG_SIZE); TPM_LOG_AREA_ADDRESS is reserved in the e820 table since it is the high memory range after the ACPI data, correct? This is my understanding yes. We made sure to put it well clear of the qemu impremnted TPM, just incase it later decided to support more localitie levels, but still well within the RESERVED area, in the e820. -jenny Reviewed-by: Jason Andryuk Thanks, Jason
Re: [PATCH v3 2/2] acpi: Add TPM2 interface definition.
On Tue, Apr 25, 2023 at 1:48 PM 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. > > Signed-off-by: Jennifer Herbert > --- ... > diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c > index f39a8e584f..51272530fe 100644 > --- a/tools/firmware/hvmloader/util.c > +++ b/tools/firmware/hvmloader/util.c > @@ -1009,6 +1009,15 @@ void hvmloader_acpi_build_tables(struct acpi_config > *config, > config->table_flags |= ACPI_HAS_TPM; > config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS; > break; > + > +case 2: > +config->table_flags |= ACPI_HAS_TPM; > +config->crb_id = (uint16_t *)TPM_CRB_INTF_ID; > + > +mem_hole_populate_ram(TPM_LOG_AREA_ADDRESS >> PAGE_SHIFT, > + TPM_LOG_SIZE >> PAGE_SHIFT); > +memset((void *)TPM_LOG_AREA_ADDRESS, 0, TPM_LOG_SIZE); TPM_LOG_AREA_ADDRESS is reserved in the e820 table since it is the high memory range after the ACPI data, correct? Reviewed-by: Jason Andryuk Thanks, Jason
