Good feedback. Thank you very much, Sami. Response inline.
I proposed some naming change. Please let us know if that is OK. Thank you Yao, Jiewen > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sami > Mujawar > Sent: Tuesday, October 19, 2021 9:21 PM > To: devel@edk2.groups.io; Xu, Min M <min.m...@intel.com> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Liming Gao > <gaolim...@byosoft.com.cn>; Liu, Zhiguang <zhiguang....@intel.com>; Yao, > Jiewen <jiewen....@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Lu, Ken > <ken...@intel.com>; nd <n...@arm.com>; Joey Gouly <joey.go...@arm.com> > Subject: Re: [edk2-devel] [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD- > Guest firmware > > Hi Min, Jiewen, > > Thank you for this patch. > > I think the protocol definition can be made architecturally neutral with > a few modifications marked inline as [SAMI]. > > I am fine with renaming the protocol to either > EFI_TEE_MEASUREMENT_PROTOCOL or EFI_CCAM_PROTOCOL. Similarly, some > of > the data structures, variables, etc. would need renaming as well. > > Please let me know if you have any queries. > > Regards, > > Sami Mujawar > > On 08/10/2021 06:21 AM, Min Xu via groups.io wrote: > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3625 > > > > If TD-Guest firmware supports measurement and an event is created, > > TD-Guest firmware is designed to report the event log with the same data > > structure in TCG-Platform-Firmware-Profile specification with > > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format. > > > > The TD-Guest firmware supports measurement, the TD Guest Firmware is > > designed to produce EFI_TD_PROTOCOL with new GUID > EFI_TD_PROTOCOL_GUID > > to report event log and provides hash capability. > > > > https://software.intel.com/content/dam/develop/external/us/en/documents/ > > intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf > > Section 4.3.2 includes the EFI_TD_PROTOCOL. > > > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > Cc: Zhiguang Liu <zhiguang....@intel.com> > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Ken Lu <ken...@intel.com> > > Reviewed-by: Jiewen Yao <jiewen....@intel.com> > > Signed-off-by: Min Xu <min.m...@intel.com> > > --- > > MdePkg/Include/Protocol/TdProtocol.h | 305 > +++++++++++++++++++++++++++ > > MdePkg/MdePkg.dec | 3 + > > 2 files changed, 308 insertions(+) > > create mode 100644 MdePkg/Include/Protocol/TdProtocol.h > > > > diff --git a/MdePkg/Include/Protocol/TdProtocol.h > b/MdePkg/Include/Protocol/TdProtocol.h > > new file mode 100644 > > index 000000000000..89b09928d33a > > --- /dev/null > > +++ b/MdePkg/Include/Protocol/TdProtocol.h > > @@ -0,0 +1,305 @@ > > +/** @file > > + If TD-Guest firmware supports measurement and an event is created, TD- > Guest > > + firmware is designed to report the event log with the same data structure > > + in TCG-Platform-Firmware-Profile specification with > > + EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format. > > + > > + The TD-Guest firmware supports measurement, the TD Guest Firmware is > designed > > + to produce EFI_TD_PROTOCOL with new GUID EFI_TD_PROTOCOL_GUID to > report > > + event log and provides hash capability. > > + > > +Copyright (c) 2020 - 2021, Intel Corporation. All rights reserved.<BR> > > +SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > + > > +#ifndef TD_PROTOCOL_H_ > > +#define TD_PROTOCOL_H_ > > + > > +#include <Uefi/UefiBaseType.h> > > +#include <IndustryStandard/UefiTcgPlatform.h> > > +#include <IndustryStandard/Tpm20.h> > [SAMI] Maybe the Tpm20.h include is not required here. Can you check, > please? [Jiewen] Right. I don’t think we do need it in this definition. I feel we just copy it from Tcg2Protocol.h. > > + > > + > > +#define EFI_TD_PROTOCOL_GUID \ > > + { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae, > 0x6b }} > > +extern EFI_GUID gEfiTdProtocolGuid; > > + > > +typedef struct _EFI_TD_PROTOCOL EFI_TD_PROTOCOL; > [SAMI] I think this could be renamed to either > EFI_TEE_MEASUREMENT_PROTOCOL or EFI_CCAM_PROTOCOL. Similarly, the > usage > of _TD_ would need to be replaced accordingly. [Jiewen] Agree. I propose "EFI_TD_PROTOCOL"->"EFI_TEE_MEASUREMENT_PROTOCOL" Also "TD"->"TEE" > > + > > +typedef struct { > > + UINT8 Major; > > + UINT8 Minor; > > +} EFI_TD_VERSION; > > + > > +typedef UINT32 EFI_TD_EVENT_LOG_BITMAP; > > +typedef UINT32 EFI_TD_EVENT_LOG_FORMAT; > > +typedef UINT32 EFI_TD_EVENT_ALGORITHM_BITMAP; > > +typedef UINT32 EFI_TD_MR_INDEX; > > + > > +#define EFI_TD_EVENT_LOG_FORMAT_TCG_2 0x00000002 > > +#define EFI_TD_BOOT_HASH_ALG_SHA384 0x00000004 > [SAMI] It is good that the values for these macros match that of TCG2. I > believe it should be possible to extend these to add macros for other > algorithms in the future. [Jiewen] Agree. If we change "TD"->"TEE", we can add other algo based upon real use case. E.g. EFI_TEE_EVENT_LOG_..., EFI_TEE_MR_INDEX, EFI_TEE_BOOT_HASH_... > > + > > +// > > +// This bit is shall be set when an event shall be extended but not logged. > > +// > > +#define EFI_TD_FLAG_EXTEND_ONLY 0x0000000000000001 > > +// > > +// This bit shall be set when the intent is to measure a PE/COFF image. > > +// > > +#define EFI_TD_FLAG_PE_COFF_IMAGE 0x0000000000000010 > > + > > +#define MR_INDEX_MRTD 0 > > +#define MR_INDEX_RTMR0 1 > > +#define MR_INDEX_RTMR1 2 > > +#define MR_INDEX_RTMR2 3 > > +#define MR_INDEX_RTMR3 4 > > + > [SAMI] I think these indexes could go to a TD specific include file Or > the indexes could be defined generically. May be it would be good to > introduce a PcdMaxMrIndex that is configurable for different > architectures. This may be useful should any asserts/checks are needed > in the code. [Jiewen] Right. This is TD specific index. We need rename it to TDX_MR_INDEX_*. I think we need add new fields in EFI_TD_BOOT_SERVICE_CAPABILITY. typedef UINT8 EFI_TEE_TYPE; // match https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/WorkArea.h, NONE = 0, AMD_SEV = 1, INTEL_TDX = 2, we can add more here. typedef UINT8 EFI_TEE_SUBTYPE; // TEE-type specific subtype. As such, the caller can know what event log / index it is using. E.g. If TeeType is TDX, then the INDEX matches the TDX RTMR. If TeeType is Realm, then the INDEX matches something else. I am not sure the usage of PcdMaxMrIndex. We SHALL NOT define PCD in a protocol in general. Would you please share your idea on how to use PcdMaxMrIndex? Then we can have better solution. > > +// > > +// This bit shall be set when the intent is to measure a PE/COFF image. > > +// > > +#define PE_COFF_IMAGE 0x0000000000000010 > > + > [SAMI] I think this macro is not needed. [Jiewen] This is to align with TCG2 protocol. I think we need to measurement UEFI image. Is there any concern to keep it? > > +#pragma pack (1) > > + > > +#define EFI_TD_EVENT_HEADER_VERSION 1 > > + > > +typedef struct { > > + // > > + // Size of the event header itself (sizeof(EFI_TD_EVENT_HEADER)). > > + // > > + UINT32 HeaderSize; > > + // > > + // Header version. For this version of this specification, the value > > shall be 1. > > + // > > + UINT16 HeaderVersion; > > + // > > + // Index of the MR that shall be extended. > > + // > > + EFI_TD_MR_INDEX MrIndex; > > + // > > + // Type of the event that shall be extended (and optionally logged). > > + // > > + UINT32 EventType; > > +} EFI_TD_EVENT_HEADER; > > + > > +typedef struct { > > + // > > + // Total size of the event including the Size component, the header and > > the > Event data. > > + // > > + UINT32 Size; > > + EFI_TD_EVENT_HEADER Header; > > + UINT8 Event[1]; > > +} EFI_TD_EVENT; > > + > > +#pragma pack() > > + > > + > > +typedef struct { > > + // > > + // Allocated size of the structure > > + // > > + UINT8 Size; > > + // > > + // Version of the EFI_TD_BOOT_SERVICE_CAPABILITY structure itself. > > + // For this version of the protocol, the Major version shall be set to 1 > > + // and the Minor version shall be set to 1. > > + // > > + EFI_TD_VERSION StructureVersion; > > + // > > + // Version of the EFI TD protocol. > > + // For this version of the protocol, the Major version shall be set to 1 > > + // and the Minor version shall be set to 1. > > + // > > + EFI_TD_VERSION ProtocolVersion; > [SAMI] Should the protocol version be 1.0 (Major.Minor), as this is the > first introduction. Same for the StructureVersion field above. [Jiewen] Copy/Past from TCG2. Sure. We can start from 1.0 (not 1.1). > > + // > > + // Supported hash algorithms > > + // > > + EFI_TD_EVENT_ALGORITHM_BITMAP HashAlgorithmBitmap; > > + // > > + // Bitmap of supported event log formats > > + // > > + EFI_TD_EVENT_LOG_BITMAP SupportedEventLogs; > > + > > + // > > + // False = TD not present > > + // > > + BOOLEAN TdPresentFlag; > [SAMI] I believe this would need to be renamed to something like > TeePresentFlag or CcaPresentFlag or a suitable alternative. [Jiewen] Agree. I propose to remove TdPresentFlag. Add "EFI_TEE_TYPE TeeType;" // 0 - None, 1 - SEV, 2 - TDX, ... > > +} EFI_TD_BOOT_SERVICE_CAPABILITY; > > + > > +/** > > + The EFI_TD_PROTOCOL GetCapability function call provides protocol > > + capability information and state information. > > + > > + @param[in] This Indicates the calling context > > + @param[in, out] ProtocolCapability The caller allocates memory for a > EFI_TD_BOOT_SERVICE_CAPABILITY > > + structure and sets the size field to > > the size of the > structure allocated. > > + The callee fills in the fields with > > the EFI protocol > capability information > > + and the current EFI TD state > > information up to the > number of fields which > > + fit within the size of the structure > > passed in. > > + > > + @retval EFI_SUCCESS Operation completed successfully. > > + @retval EFI_DEVICE_ERROR The command was unsuccessful. > > + The ProtocolCapability variable will not > > be populated. > > + @retval EFI_INVALID_PARAMETER One or more of the parameters are > incorrect. > > + The ProtocolCapability variable will not > > be populated. > > + @retval EFI_BUFFER_TOO_SMALL The ProtocolCapability variable is too > small to hold the full response. > > + It will be partially populated (required > > Size field will be set). > > +**/ > > +typedef > > +EFI_STATUS > > +(EFIAPI *EFI_TD_GET_CAPABILITY) ( > > + IN EFI_TD_PROTOCOL *This, > > + IN OUT EFI_TD_BOOT_SERVICE_CAPABILITY *ProtocolCapability > > + ); > > + > > +/** > > + The EFI_TD_PROTOCOL Get Event Log function call allows a caller to > > + retrieve the address of a given event log and its last entry. > > + > > + @param[in] This Indicates the calling context > > + @param[in] EventLogFormat The type of the event log for which the > information is requested. > > + @param[out] EventLogLocation A pointer to the memory address of the > event log. > > + @param[out] EventLogLastEntry If the Event Log contains more than one > entry, this is a pointer to the > > + address of the start of the last entry in > > the event log in > memory. > > + @param[out] EventLogTruncated If the Event Log is missing at least one > entry because an event would > > + have exceeded the area allocated for > > events, this value is > set to TRUE. > > + Otherwise, the value will be FALSE and > > the Event Log will > be complete. > > + > > + @retval EFI_SUCCESS Operation completed successfully. > > + @retval EFI_INVALID_PARAMETER One or more of the parameters are > incorrect > > + (e.g. asking for an event log whose > > format is not > supported). > > +**/ > > +typedef > > +EFI_STATUS > > +(EFIAPI *EFI_TD_GET_EVENT_LOG) ( > > + IN EFI_TD_PROTOCOL *This, > > + IN EFI_TD_EVENT_LOG_FORMAT EventLogFormat, > > + OUT EFI_PHYSICAL_ADDRESS *EventLogLocation, > > + OUT EFI_PHYSICAL_ADDRESS *EventLogLastEntry, > > + OUT BOOLEAN *EventLogTruncated > > + ); > > + > > +/** > > + The EFI_TD_PROTOCOL HashLogExtendEvent function call provides callers > with > > + an opportunity to extend and optionally log events without requiring > > + knowledge of actual TD commands. > > + The extend operation will occur even if this function cannot create an > > event > > + log entry (e.g. due to the event log being full). > > + > > + @param[in] This Indicates the calling context > > + @param[in] Flags Bitmap providing additional information. > > + @param[in] DataToHash Physical address of the start of the data > buffer to be hashed. > > + @param[in] DataToHashLen The length in bytes of the buffer > > referenced > by DataToHash. > > + @param[in] EfiTdEvent Pointer to data buffer containing > > information > about the event. > > + > > + @retval EFI_SUCCESS Operation completed successfully. > > + @retval EFI_DEVICE_ERROR The command was unsuccessful. > > + @retval EFI_VOLUME_FULL The extend operation occurred, but the > event could not be written to one or more event logs. > > + @retval EFI_INVALID_PARAMETER One or more of the parameters are > incorrect. > > + @retval EFI_UNSUPPORTED The PE/COFF image type is not supported. > > +**/ > > +typedef > > +EFI_STATUS > > +(EFIAPI * EFI_TD_HASH_LOG_EXTEND_EVENT) ( > > + IN EFI_TD_PROTOCOL *This, > > + IN UINT64 Flags, > > + IN EFI_PHYSICAL_ADDRESS DataToHash, > > + IN UINT64 DataToHashLen, > > + IN EFI_TD_EVENT *EfiTdEvent > > + ); > > + > > +/** > > + The EFI_TD_PROTOCOL MapPcrToMrIndex function call provides callers > > + the info on TPM PCR<-> measurement register mapping information. > > + > > + In current version, we use below mapping: > > + PCR0 -> MRTD (Index 0) > > + PCR1 -> RTMR0 (Index 1) > > + PCR2~6 -> RTMR1 (Index 2) > > + PCR7 -> RTMR0 (Index 1) > > + PCR8~15 -> RTMR2 (Index 3) > > + > [SAMI] I think different architecures may map the PCRs differently. I > think the comment could be reworded to a more generic representation of > the mapping. > Also, I need to check the mailing list if there is a patch that adds the > TD protocol implementaiton, and if it could be made generic as well. > Maybe the protocol implementation would need to use an architecture > specific library that provides the mapping function. > [/SAMI] [Jiewen] Agree. We should clear up the comment. The caller shall be generic enough to use this API to get the mapping. The caller shall NOT make any assumption. > > + @param[in] This Indicates the calling context > > + @param[in] PcrIndex TPM PCR index. > > + @param[out] MrIndex Measurement register index. > > + > > + @retval EFI_SUCCESS The MR index is returned. > > + @retval EFI_INVALID_PARAMETER The MrIndex is NULL. > > + @retval EFI_UNSUPPORTED The PcrIndex is invalid. > > +**/ > > +typedef > > +EFI_STATUS > > +(EFIAPI * EFI_TD_MAP_PCR_TO_MR_INDEX) ( > > + IN EFI_TD_PROTOCOL *This, > > + IN TCG_PCRINDEX PcrIndex, > > + OUT EFI_TD_MR_INDEX *MrIndex > > + ); > > + > > +struct _EFI_TD_PROTOCOL { > > + EFI_TD_GET_CAPABILITY GetCapability; > > + EFI_TD_GET_EVENT_LOG GetEventLog; > > + EFI_TD_HASH_LOG_EXTEND_EVENT HashLogExtendEvent; > > + EFI_TD_MAP_PCR_TO_MR_INDEX MapPcrToMrIndex; > > +}; > > + > > + > > +// > > +// TD event log > > +// > > + > > +#pragma pack(1) > > + > > +// > > +// Crypto Agile Log Entry Format. > > +// It is similar with TCG_PCR_EVENT2 except the field of MrIndex and > PCRIndex. > > +// > > +typedef struct { > > + EFI_TD_MR_INDEX MrIndex; > > + UINT32 EventType; > > + TPML_DIGEST_VALUES Digests; > > + UINT32 EventSize; > > + UINT8 Event[1]; > > +} TD_EVENT; > > + > > +// > > +// EFI TD Event Header > > +// It is similar with TCG_PCR_EVENT2_HDR except the field of MrIndex and > PCRIndex > > +// > > +typedef struct { > > + EFI_TD_MR_INDEX MrIndex; > > + UINT32 EventType; > > + TPML_DIGEST_VALUES Digests; > > + UINT32 EventSize; > > +} TD_EVENT_HDR; > > + > > +#pragma pack() > > + > > +// > > +// Log entries after Get Event Log service > > +// > > + > > + > > +typedef struct { > > + // > > + // The version of this structure. It shall be set ot 1. > [SAMI] It may be good to define a macro for the events table version, > similar to EFI_TCG2_FINAL_EVENTS_TABLE_VERSION. [Jiewen] Agree. > > + // > > + UINT64 Version; > > + // > > + // Number of events recorded after invocation of GetEventLog API > > + // > > + UINT64 NumberOfEvents; > > + // > > + // List of events of type TD_EVENT. > > + // > > + //TD_EVENT Event[1]; > > +} EFI_TD_FINAL_EVENTS_TABLE; > > + > > + > > +#define EFI_TD_FINAL_EVENTS_TABLE_GUID \ > > + {0xdd4a4648, 0x2de7, 0x4665, {0x96, 0x4d, 0x21, 0xd9, 0xef, 0x5f, 0xb4, > 0x46}} > > + > > +extern EFI_GUID gEfiTdFinalEventsTableGuid; > > + > > +#endif > > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec > > index 9cdc915ebae9..a31c44d3a689 100644 > > --- a/MdePkg/MdePkg.dec > > +++ b/MdePkg/MdePkg.dec > > @@ -1011,6 +1011,9 @@ > > ## Include/Protocol/PcdInfo.h > > gGetPcdInfoProtocolGuid = { 0x5be40f57, 0xfa68, 0x4610, { 0xbb, > > 0xbf, > 0xe9, 0xc5, 0xfc, 0xda, 0xd3, 0x65 } } > > > > + ## Include/Protocol/TdProtocol.h > > + gEfiTdProtocolGuid = { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7, > > 0x94, > 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b }} > > + > > # > > # Protocols defined in PI1.0. > > # > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#82358): https://edk2.groups.io/g/devel/message/82358 Mute This Topic: https://groups.io/mt/86163958/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
