Re: [PATCH V2 1/6] IntelSiliconPkg/Include: Add Intel PciSecurity definition.

["Chaganty, Rangasai V" <rangasai.v.chaganty@intel.com>]

Hi Jiewen, 
Few comments:
1. Can we put a reference to the spec at the file header?
2. Can we group all the macros at the top followed by structure definitions?
3. Is it possible to add some high level description above the structure definition that describes the structure?
4. I see line 80 is commented out. Can we remove that line?
5. Please add some description about the change after line 5.

Thanks,
Sai

-----Original Message-----
From: Yao, Jiewen 
Sent: Thursday, October 31, 2019 5:31 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Lou, Yun <yun.lou@intel.com>
Subject: [PATCH V2 1/6] IntelSiliconPkg/Include: Add Intel PciSecurity definition.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2303

Cc: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Yun Lou <yun.lou@intel.com>
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/IntelPciSecurity.h | 66 ++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/IntelPciSecurity.h b/Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/IntelPciSecurity.h
new file mode 100644
index 0000000000..a8c5483165
--- /dev/null
+++ b/Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/IntelPciSecurity.h
@@ -0,0 +1,66 @@
+/** @file
+  Intel PCI security data structure
+
+Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __INTEL_PCI_SECURITY_H__
+#define __INTEL_PCI_SECURITY_H__
+
+#pragma pack(1)
+
+typedef struct {
+  UINT16  CapId;           // 0x23: DVSEC
+  UINT16  CapVersion:4;    // 1
+  UINT16  NextOffset:12;
+  UINT16  DvSecVendorId;   // 0x8086
+  UINT16  DvSecRevision:4; // 1
+  UINT16  DvSecLength:12;
+  UINT16  DvSecId;         // 0x3E: Measure
+} INTEL_PCI_DIGEST_CAPABILITY_HEADER;
+
+#define INTEL_PCI_CAPID_DVSEC                0x23
+#define INTEL_PCI_DVSEC_VENDORID_INTEL       0x8086
+#define INTEL_PCI_DVSEC_DVSECID_MEASUREMENT  0x3E
+
+typedef union {
+  struct {
+    UINT8   DigestModified:1;         // RW1C
+    UINT8   Reserved0:7;
+  } Bits;
+  UINT8 Data;
+} INTEL_PCI_DIGEST_DATA_MODIFIED;
+
+#define INTEL_PCI_DIGEST_MODIFIED        BIT0
+
+typedef union {
+  struct {
+    UINT8   Digest0Valid:1;          // RO
+    UINT8   Digest0Locked:1;         // RO
+    UINT8   Digest1Valid:1;          // RO
+    UINT8   Digest1Locked:1;         // RO
+    UINT8   Reserved1:4;
+  } Bits;
+  UINT8 Data;
+} INTEL_PCI_DIGEST_DATA_VALID;
+
+#define INTEL_PCI_DIGEST_0_VALID         BIT0
+#define INTEL_PCI_DIGEST_0_LOCKED        BIT1
+#define INTEL_PCI_DIGEST_1_VALID         BIT2
+#define INTEL_PCI_DIGEST_1_LOCKED        BIT3
+
+typedef struct {
+  INTEL_PCI_DIGEST_DATA_MODIFIED   Modified;   // RW1C
+  INTEL_PCI_DIGEST_DATA_VALID      Valid;      // RO
+  UINT16                           TcgAlgId;   // RO
+  UINT8                            FirmwareID; // RO
+  UINT8                            Reserved;
+//UINT8                            Digest[];
+} INTEL_PCI_DIGEST_CAPABILITY_STRUCTURE;
+
+#pragma pack()
+
+#endif
+
-- 
2.19.2.windows.1