Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FF-A] ACPI TPM2 Table for FF-A Implementation: #358

Open
wants to merge 1 commit into
base: feature/ffa_enablement
Choose a base branch
from

Conversation

Raymond-MS
Copy link

Description

Added the TCG2 ACPI changes for FF-A enablement in order to generate the correct ACPI table with FF-A information. This includes the .inf, .asl. and .c files.

For details on how to complete these options and their meaning refer to CONTRIBUTING.md.

  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
  • Includes documentation?
  • Backport to release branch?

How This Was Tested

Utilized the UEFI shell "acpiview" command to verify the TPM2 table is being populated properly.

Integration Instructions

N/A

…the correct ACPI table with FF-A information. This includes the .inf, .asl. and .c files.
SwSmiValue and allocated NVS region address.

Caution: This module requires additional review when modified.
This driver will have external input - variable and ACPINvs data in SMM mode.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description is not true anymore.


#include <Guid/TpmInstance.h>
#include <Guid/TpmNvsMm.h>
#include <Guid/PiSmmCommunicationRegionTable.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed?

ControlArea->Response = PcdGet64 (PcdTpmBaseAddress) + 0x80;
break;
default:
DEBUG ((DEBUG_ERROR, "TPM2 InterfaceType get error! %d\n", InterfaceType));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to set the return status code here?

break;
}

DEBUG ((DEBUG_INFO, "Tpm2 ACPI table size %d\n", mTpm2AcpiTemplate.Header.Length));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then check the status code then bail the installation if failed?

# populates registered SMI callback functions for Tcg2 physical presence
# and MemoryClear to handle the requests for ACPI method. It needs to be
# used together with Tcg2 MM drivers to exchange information on registered
# SwSmiValue and allocated NVS region address.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, let's update the description.

Tpm2CommandLib
Tcg2PhysicalPresenceLib
PcdLib
MmUnblockMemoryLib
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this lib? I do not think we called it anywhere?


[Guids]
gEfiTpmDeviceInstanceTpm20DtpmGuid ## PRODUCES ## GUID # TPM device identifier
gTpmNvsMmGuid ## CONSUMES
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used?


[Protocols]
gEfiAcpiTableProtocolGuid ## CONSUMES
gEfiMmCommunicationProtocolGuid ## CONSUMES
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used?

gEfiMmCommunicationProtocolGuid ## CONSUMES

[FixedPcd]
gEfiSecurityPkgTokenSpaceGuid.PcdSmiCommandIoPort ## CONSUMES
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used?

//
// Operational region for TPM access
//
OperationRegion (TPMR, SystemMemory, 0x10000010000, 0x5000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can change this over to use fixed PCD now, right?

Cacheable,
ReadWrite,
0x0,
0x10000010000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here?

@kuqin12
Copy link
Contributor

kuqin12 commented Nov 27, 2024

The change seems legit. There are a few functions looks largely alike with those in the ACPI driver. When we upstream this to EDK2, I think they will want to share these functions instead of duplicating another one.

@kuqin12
Copy link
Contributor

kuqin12 commented Nov 27, 2024

CI builds are failing, try to follow these guide to repro the failure locally: https://github.com/tianocore/tianocore.github.io/wiki/How-to-Build-With-Stuart#before-each-build-steps. There are logs published along with the pipeline builds. But it will not be efficient as the pipeline queue could be very long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants