Re: [edk2-devel] [PATCH v1 3/6] TGC2ACPI: Uninstall the TPM2 ACPI if present

["Yao, Jiewen" <jiewen.yao@intel.com>]

If there are two drivers to produce same resource, it should be a platform policy decision for which one takes precedent.

Maybe a platform wants to say: The ACPI is there, I want to skip myself, instead of override.

I feel uncomfortable to include such policy in TCG2 ACPI driver, to enforce the policy to override.
My suggestion is to keep SecurityPkg as is.

Yes, coreboot and slimboot will populate ACPI table. But it does not mean UniversalPayloadPkg must install them. I still believe that work should be done in UniversalPayloadPkg.



> -----Original Message-----
> From: Lakkimsetti, Subash <subash.lakkimsetti@intel.com>
> Sent: Thursday, March 23, 2023 12:38 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Qi1 <qi1.zhang@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1 3/6] TGC2ACPI: Uninstall the TPM2 ACPI
> if present
> 
> Hi Jiewen,
> 
> Bootloaders as SBL, coreboot supports multiple payloads and they would
> have TPM acpi tables populated at bootloader phase.
> These tables gets populated in bootloaders well before the payload gets
> loaded. Its population cannot be avoided due to generic implementations.
> 
> Uninstallation from universal payload package need a separate module to
> handle this.
> It is dependent on gEfiAcpiTableProtocolGuid & gEfiAcpiSdtProtocolGuid
> which are populated from AcpiTableDxe.
> 
> We might need add a dependency in TCG2ACPI on this module. Otherwise
> TPM2tables published from TCG2ACPI can be installed as per the dispatch
> order.
> 
> In order to adapt the TCG drivers for UEFI payload, TCG2ACPI has to updated
> to support payload. We can either follow one of the methods,
> *	Uninstall the TPM2 ACPI tables if they get published from bootloader
> phase. The current patch address it.
> *	Another method, Modify the ACPI tables updated from bootloader as
> per the EDK2 requirements. This also has to be done in TCG2ACPI.
> 
> Regards,
> Subash
> 
> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Wednesday, March 22, 2023 6:43 AM
> To: devel@edk2.groups.io; Lakkimsetti, Subash
> <subash.lakkimsetti@intel.com>
> Cc: Zhang, Qi1 <qi1.zhang@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1 3/6] TGC2ACPI: Uninstall the TPM2 ACPI
> if present
> 
> Question: Why not uninstall it in the universal payload package? or even not
> populate it?
> 
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Subash
> > Lakkimsetti
> > Sent: Wednesday, March 22, 2023 1:59 PM
> > To: devel@edk2.groups.io
> > Cc: Lakkimsetti, Subash <subash.lakkimsetti@intel.com>; Zhang, Qi1
> > <qi1.zhang@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> > Subject: [edk2-devel] [PATCH v1 3/6] TGC2ACPI: Uninstall the TPM2 ACPI
> > if present
> >
> > From: Subash Lakkimsetti <subash.lakkimsetti@intel.com>
> >
> > Bootloader supports multiple payload and TPM2 ACPI tables are updated
> > at bootloader phase. When UEFI is used payload these will be duplicates.
> > The tables are to be uninstalled before updating the TCG2ACPI tables
> > to avoid duplicates.
> >
> > Cc: Qi Zhang <qi1.zhang@intel.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Signed-off-by: Subash Lakkimsetti <subash.lakkimsetti@intel.com>
> > ---
> >  SecurityPkg/Tcg/Tcg2Acpi/Tcg2Acpi.c   | 251
> > ++++++++++++++++++++++++++
> >  SecurityPkg/Tcg/Tcg2Acpi/Tcg2Acpi.inf |   3 +
> >  2 files changed, 254 insertions(+)
> >
> > diff --git a/SecurityPkg/Tcg/Tcg2Acpi/Tcg2Acpi.c
> > b/SecurityPkg/Tcg/Tcg2Acpi/Tcg2Acpi.c
> > index e8822cbeb0..4b35796ba7 100644
> > --- a/SecurityPkg/Tcg/Tcg2Acpi/Tcg2Acpi.c
> > +++ b/SecurityPkg/Tcg/Tcg2Acpi/Tcg2Acpi.c
> > @@ -39,6 +39,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > #include <Library/Tpm2CommandLib.h>
> >
> >  #include <Library/UefiLib.h>
> >
> >  #include <Library/MmUnblockMemoryLib.h>
> >
> > +#include <IndustryStandard/Acpi.h>
> >
> > +#include <Protocol/AcpiSystemDescriptionTable.h>
> >
> >
> >
> >  //
> >
> >  // Physical Presence Interface Version supported by Platform
> >
> > @@ -867,6 +869,245 @@ PublishTpm2 (
> >    return Status;
> >
> >  }
> >
> >
> >
> > +/**
> >
> > +  Uninstall TPM2 SSDT ACPI table
> >
> > +
> >
> > +  This performs uninstallation of TPM2 SSDT tables published by
> >
> > +  bootloaders.
> >
> > +
> >
> > +  @retval   EFI_SUCCESS     The TPM2 ACPI table is uninstalled successfully
> if
> > found.
> >
> > +  @retval   Others          Operation error.
> >
> > +
> >
> > +**/
> >
> > +EFI_STATUS
> >
> > +UnInstallTpm2SSDTAcpiTables (
> >
> > +  )
> >
> > +{
> >
> > +  UINTN                    TableIndex;
> >
> > +  UINTN                    TableKey;
> >
> > +  EFI_ACPI_TABLE_VERSION   TableVersion;
> >
> > +  VOID                     *TableHeader;
> >
> > +  EFI_STATUS               Status;
> >
> > +  EFI_ACPI_SDT_PROTOCOL    *mAcpiSdtProtocol;
> >
> > +  EFI_ACPI_TABLE_PROTOCOL  *mAcpiTableProtocol;
> >
> > +  CHAR8                    TableIdString[8];
> >
> > +  UINT64                   TableIdSignature;
> >
> > +
> >
> > +  //
> >
> > +  // Determine whether there is a TPM2 SSDT already in the ACPI table.
> >
> > +  //
> >
> > +  Status             = EFI_SUCCESS;
> >
> > +  TableIndex         = 0;
> >
> > +  TableKey           = 0;
> >
> > +  TableHeader        = NULL;
> >
> > +  mAcpiTableProtocol = NULL;
> >
> > +  mAcpiSdtProtocol   = NULL;
> >
> > +
> >
> > +  //
> >
> > +  // Locate the EFI_ACPI_TABLE_PROTOCOL.
> >
> > +  //
> >
> > +  Status = gBS->LocateProtocol (
> >
> > +                  &gEfiAcpiTableProtocolGuid,
> >
> > +                  NULL,
> >
> > +                  (VOID **)&mAcpiTableProtocol
> >
> > +                  );
> >
> > +  if (EFI_ERROR (Status)) {
> >
> > +    DEBUG ((
> >
> > +      DEBUG_INFO,
> >
> > +      "UnInstallTpm2SSDTAcpiTables: Cannot locate the EFI ACPI Table
> > Protocol \n "
> >
> > +      ));
> >
> > +    return Status;
> >
> > +  }
> >
> > +
> >
> > +  //
> >
> > +  // Locate the EFI_ACPI_SDT_PROTOCOL.
> >
> > +  //
> >
> > +  Status = gBS->LocateProtocol (
> >
> > +                  &gEfiAcpiSdtProtocolGuid,
> >
> > +                  NULL,
> >
> > +                  (VOID **)&mAcpiSdtProtocol
> >
> > +                  );
> >
> > +  if (EFI_ERROR (Status)) {
> >
> > +    DEBUG ((
> >
> > +      DEBUG_INFO,
> >
> > +      "UnInstallTpm2SSDTAcpiTables: Cannot locate the EFI ACPI Sdt
> > + Protocol,
> > "
> >
> > +      "\n"
> >
> > +      ));
> >
> > +    return Status;
> >
> > +  }
> >
> > +
> >
> > +  while (!EFI_ERROR (Status)) {
> >
> > +    Status = mAcpiSdtProtocol->GetAcpiTable (
> >
> > +                                 TableIndex,
> >
> > +                                 (EFI_ACPI_SDT_HEADER
> > + **)&TableHeader,
> >
> > +                                 &TableVersion,
> >
> > +                                 &TableKey
> >
> > +                                 );
> >
> > +
> >
> > +    if (!EFI_ERROR (Status)) {
> >
> > +      TableIndex++;
> >
> > +
> >
> > +      if (((EFI_ACPI_SDT_HEADER *)TableHeader)->Signature ==
> > SIGNATURE_32 ('S', 'S', 'D', 'T')) {
> >
> > +        CopyMem ((VOID *)TableIdString, (VOID *)((EFI_ACPI_SDT_HEADER
> > *)TableHeader)->OemTableId, sizeof (TableIdString));
> >
> > +
> >
> > +        TableIdSignature = SIGNATURE_64 (
> >
> > +                             TableIdString[0],
> >
> > +                             TableIdString[1],
> >
> > +                             TableIdString[2],
> >
> > +                             TableIdString[3],
> >
> > +                             TableIdString[4],
> >
> > +                             TableIdString[5],
> >
> > +                             TableIdString[6],
> >
> > +                             TableIdString[7]
> >
> > +                             );
> >
> > +
> >
> > +        if (TableIdSignature == SIGNATURE_64 ('T', 'p', 'm', '2',
> > + 'T', 'a', 'b', 'l')) {
> >
> > +          DEBUG ((DEBUG_INFO, "Found Tpm2 SSDT Table for Physical
> > Presence\n"));
> >
> > +          break;
> >
> > +        }
> >
> > +      }
> >
> > +    }
> >
> > +  }
> >
> > +
> >
> > +  if (!EFI_ERROR (Status)) {
> >
> > +    //
> >
> > +    // A TPM2 SSDT is already in the ACPI table.
> >
> > +    //
> >
> > +    DEBUG ((
> >
> > +      DEBUG_INFO,
> >
> > +      "A TPM2 SSDT is already exist in the ACPI Table.\n"
> >
> > +      ));
> >
> > +
> >
> > +    //
> >
> > +    // Uninstall the origin TPM2 SSDT from the ACPI table.
> >
> > +    //
> >
> > +    Status = mAcpiTableProtocol->UninstallAcpiTable (
> >
> > +                                   mAcpiTableProtocol,
> >
> > +                                   TableKey
> >
> > +                                   );
> >
> > +    ASSERT_EFI_ERROR (Status);
> >
> > +
> >
> > +    if (EFI_ERROR (Status)) {
> >
> > +      DEBUG ((DEBUG_INFO, "UnInstall Tpm2SSDTAcpiTables failed \n
> > + "));
> >
> > +
> >
> > +      return Status;
> >
> > +    }
> >
> > +  }
> >
> > +
> >
> > +  return EFI_SUCCESS;
> >
> > +}
> >
> > +
> >
> > +/**
> >
> > +  Uninstall TPM2 table
> >
> > +
> >
> > +  This performs uninstallation of TPM2 tables published by
> >
> > +  bootloaders.
> >
> > +
> >
> > +  @retval   EFI_SUCCESS     The TPM2 table is uninstalled successfully if its
> > found.
> >
> > +  @retval   Others          Operation error.
> >
> > +
> >
> > +**/
> >
> > +EFI_STATUS
> >
> > +UnInstallTpm2Tables (
> >
> > +  )
> >
> > +{
> >
> > +  UINTN                    TableIndex;
> >
> > +  UINTN                    TableKey;
> >
> > +  EFI_ACPI_TABLE_VERSION   TableVersion;
> >
> > +  VOID                     *TableHeader;
> >
> > +  EFI_STATUS               Status;
> >
> > +  EFI_ACPI_SDT_PROTOCOL    *mAcpiSdtProtocol;
> >
> > +  EFI_ACPI_TABLE_PROTOCOL  *mAcpiTableProtocol;
> >
> > +
> >
> > +  //
> >
> > +  // Determine whether there is a TPM2 SSDT already in the ACPI table.
> >
> > +  //
> >
> > +  Status             = EFI_SUCCESS;
> >
> > +  TableIndex         = 0;
> >
> > +  TableKey           = 0;
> >
> > +  TableHeader        = NULL;
> >
> > +  mAcpiTableProtocol = NULL;
> >
> > +  mAcpiSdtProtocol   = NULL;
> >
> > +
> >
> > +  //
> >
> > +  // Locate the EFI_ACPI_TABLE_PROTOCOL.
> >
> > +  //
> >
> > +  Status = gBS->LocateProtocol (
> >
> > +                  &gEfiAcpiTableProtocolGuid,
> >
> > +                  NULL,
> >
> > +                  (VOID **)&mAcpiTableProtocol
> >
> > +                  );
> >
> > +  if (EFI_ERROR (Status)) {
> >
> > +    DEBUG ((
> >
> > +      DEBUG_INFO,
> >
> > +      "UnInstallTpm2Tables: Cannot locate the EFI ACPI Table Protocol \n "
> >
> > +      ));
> >
> > +    return Status;
> >
> > +  }
> >
> > +
> >
> > +  //
> >
> > +  // Locate the EFI_ACPI_SDT_PROTOCOL.
> >
> > +  //
> >
> > +  Status = gBS->LocateProtocol (
> >
> > +                  &gEfiAcpiSdtProtocolGuid,
> >
> > +                  NULL,
> >
> > +                  (VOID **)&mAcpiSdtProtocol
> >
> > +                  );
> >
> > +  if (EFI_ERROR (Status)) {
> >
> > +    DEBUG ((
> >
> > +      DEBUG_INFO,
> >
> > +      "UnInstallTpm2Tables: Cannot locate the EFI ACPI Sdt Protocol, "
> >
> > +      "\n"
> >
> > +      ));
> >
> > +    return Status;
> >
> > +  }
> >
> > +
> >
> > +  while (!EFI_ERROR (Status)) {
> >
> > +    Status = mAcpiSdtProtocol->GetAcpiTable (
> >
> > +                                 TableIndex,
> >
> > +                                 (EFI_ACPI_SDT_HEADER
> > + **)&TableHeader,
> >
> > +                                 &TableVersion,
> >
> > +                                 &TableKey
> >
> > +                                 );
> >
> > +
> >
> > +    if (!EFI_ERROR (Status)) {
> >
> > +      TableIndex++;
> >
> > +
> >
> > +      if (((EFI_ACPI_SDT_HEADER *)TableHeader)->Signature ==
> > EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_2_TABLE_SIGNATURE )
> {
> >
> > +        DEBUG ((DEBUG_INFO, "Found Tpm2 Table ..\n"));
> >
> > +        break;
> >
> > +      }
> >
> > +    }
> >
> > +  }
> >
> > +
> >
> > +  if (!EFI_ERROR (Status)) {
> >
> > +    //
> >
> > +    // A TPM2 SSDT is already in the ACPI table.
> >
> > +    //
> >
> > +    DEBUG ((
> >
> > +      DEBUG_INFO,
> >
> > +      "A TPM2 table  is already exist in the ACPI Table.\n"
> >
> > +      ));
> >
> > +
> >
> > +    //
> >
> > +    // Uninstall the origin TPM2 SSDT from the ACPI table.
> >
> > +    //
> >
> > +    Status = mAcpiTableProtocol->UninstallAcpiTable (
> >
> > +                                   mAcpiTableProtocol,
> >
> > +                                   TableKey
> >
> > +                                   );
> >
> > +    ASSERT_EFI_ERROR (Status);
> >
> > +
> >
> > +    if (EFI_ERROR (Status)) {
> >
> > +      DEBUG ((DEBUG_INFO, "UnInstall Tpm2Tables failed \n "));
> >
> > +
> >
> > +      return Status;
> >
> > +    }
> >
> > +  }
> >
> > +
> >
> > +  return EFI_SUCCESS;
> >
> > +}
> >
> > +
> >
> >  /**
> >
> >    The driver's entry point.
> >
> >
> >
> > @@ -894,6 +1135,16 @@ InitializeTcgAcpi (
> >      return EFI_UNSUPPORTED;
> >
> >    }
> >
> >
> >
> > +  //
> >
> > +  // Bootloader might pulish the TPM2 ACPT tables
> >
> > +  // Uninstall TPM tables if it exists
> >
> > +  //
> >
> > +  Status = UnInstallTpm2SSDTAcpiTables ();
> >
> > +  ASSERT_EFI_ERROR (Status);
> >
> > +
> >
> > +  Status = UnInstallTpm2Tables ();
> >
> > +  ASSERT_EFI_ERROR (Status);
> >
> > +
> >
> >    Status = PublishAcpiTable ();
> >
> >    ASSERT_EFI_ERROR (Status);
> >
> >
> >
> > diff --git a/SecurityPkg/Tcg/Tcg2Acpi/Tcg2Acpi.inf
> > b/SecurityPkg/Tcg/Tcg2Acpi/Tcg2Acpi.inf
> > index f1c6ae5b1c..7e639b0522 100644
> > --- a/SecurityPkg/Tcg/Tcg2Acpi/Tcg2Acpi.inf
> > +++ b/SecurityPkg/Tcg/Tcg2Acpi/Tcg2Acpi.inf
> > @@ -63,10 +63,13 @@
> >    gEfiTpmDeviceInstanceTpm20DtpmGuid                            ## PRODUCES
> > ## GUID       # TPM device identifier
> >
> >    gTpmNvsMmGuid                                                 ## CONSUMES
> >
> >    gEdkiiPiSmmCommunicationRegionTableGuid                       ## CONSUMES
> >
> > +  gEfiAcpiTableGuid
> >
> >
> >
> >  [Protocols]
> >
> >    gEfiAcpiTableProtocolGuid                                     ## CONSUMES
> >
> >    gEfiMmCommunicationProtocolGuid                               ## CONSUMES
> >
> > +  gEfiAcpiSdtProtocolGuid                        ## CONSUMES
> >
> > +
> >
> >
> >
> >  [FixedPcd]
> >
> >    gEfiSecurityPkgTokenSpaceGuid.PcdSmiCommandIoPort             ##
> > CONSUMES
> >
> > --
> > 2.39.1.windows.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#101580):
> > https://edk2.groups.io/g/devel/message/101580
> > Mute This Topic: https://groups.io/mt/97777996/1772286
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [jiewen.yao@intel.com] -=-=-=-=-=-=
> >