On 15.09.2022 22:40, Jennifer Herbert wrote:
First of all - please follow patch submission guidelines and
send patches To: the list only, with maintainers Cc:-ed.
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -994,13 +994,22 @@ 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);
Did you not mean to drop ACPI_HAS_TPM here when ...
> config->acpi_revision = 4;
>
> - config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
> + s = xenstore_read("platform/tpm_version", "0");
> + config->tpm_version = strtoll(s, NULL, 0);
> +
> + switch( config->tpm_version )
> + {
> + case 1:
> + config->table_flags |= ACPI_HAS_TPM;
... you now OR it in here? Or else what use is this statement?
As to the use of strtoll() - I realize we have nothing else in
hvmloader, but I'm still weary of the overflow potential. Just
a remark, not really something to act upon.
> @@ -78,8 +78,8 @@ struct acpi_config {
> struct acpi_numa numa;
> const struct hvm_info_table *hvminfo;
>
> + uint8_t tpm_version;
> const uint16_t *tis_hdr;
> -
> /*
> * Address where acpi_info should be placed.
> * This must match the OperationRegion(BIOS, SystemMemory, ....)
Please don't remove the blank line here.
Jan
