Jiewen, I am fine with the 1/6 patch that doesn't contain enough comments to describe the meaning of each field in each structure because I can reach out to the referenced spec to understand them.
This 2/6 patch introduces a new protocol but contains very few comments (almost none) for each structure each field and I cannot reach out to any spec to understand them. So can you please add comments to each field of structures like EDKII_DEVICE_SECURITY_POLICY and EDKII_DEVICE_SECURITY_STATE? Also can you please add comments to all the macros defined in this patch to explain the meaning and more important how they are going to impact the logic. In general, can you please add enough comments so that a PCI/USB BUS driver developer can understand the whole flow how this protocol supports the device security? Thanks, Ray > -----Original Message----- > From: Yao, Jiewen <jiewen....@intel.com> > Sent: Thursday, October 31, 2019 8:31 PM > To: devel@edk2.groups.io > Cc: Ni, Ray <ray...@intel.com>; Chaganty, Rangasai V > <rangasai.v.chaga...@intel.com>; Lou, Yun <yun....@intel.com> > Subject: [PATCH V2 2/6] IntelSiliconPkg/Include: Add Platform Device > Security Policy protocol > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2303 > > Cc: Ray Ni <ray...@intel.com> > Cc: Rangasai V Chaganty <rangasai.v.chaga...@intel.com> > Cc: Yun Lou <yun....@intel.com> > Signed-off-by: Jiewen Yao <jiewen....@intel.com> > --- > > Silicon/Intel/IntelSiliconPkg/Include/Protocol/PlatformDeviceSecurityPolicy.h > | 84 ++++++++++++++++++++ > 1 file changed, 84 insertions(+) > > diff --git > a/Silicon/Intel/IntelSiliconPkg/Include/Protocol/PlatformDeviceSecurityPolic > y.h > b/Silicon/Intel/IntelSiliconPkg/Include/Protocol/PlatformDeviceSecurityPolic > y.h > new file mode 100644 > index 0000000000..cb5a71ad41 > --- /dev/null > +++ > b/Silicon/Intel/IntelSiliconPkg/Include/Protocol/PlatformDeviceSecurityPolic > y.h > @@ -0,0 +1,84 @@ > +/** @file > + Device Security Policy Protocol definition > + > + Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > + > +#ifndef __EDKII_DEVICE_SECURITY_POLICY_PROTOCOL_H__ > +#define __EDKII_DEVICE_SECURITY_POLICY_PROTOCOL_H__ > + > +#include <Uefi.h> > +#include <Protocol/DeviceSecurity.h> > + > +typedef struct _EDKII_DEVICE_SECURITY_POLICY_PROTOCOL > EDKII_DEVICE_SECURITY_POLICY_PROTOCOL; > + > +typedef struct { > + UINT32 Version; // 0x1 > + UINT32 MeasurementPolicy; > + UINT32 AuthenticationPolicy; > +} EDKII_DEVICE_SECURITY_POLICY; > + > +// BIT0 means if the action is needed or NOT > +#define EDKII_DEVICE_MEASUREMENT_POLICY_REQUIRED BIT0 > +#define EDKII_DEVICE_AUTHENTICATION_POLICY_REQUIRED BIT0 > + > +typedef struct { > + UINT32 Version; // 0x1 > + UINT32 MeasurementState; > + UINT32 AuthenticationState; > +} EDKII_DEVICE_SECURITY_STATE; > + > +// All zero means success > +#define EDKII_DEVICE_SECURITY_STATE_SUCCESS 0 > +#define EDKII_DEVICE_SECURITY_STATE_ERROR BIT31 > +#define EDKII_DEVICE_SECURITY_STATE_ERROR_UEFI_UNSUPPORTED > (EDKII_DEVICE_SECURITY_STATE_ERROR + 0x0) > +#define > EDKII_DEVICE_SECURITY_STATE_ERROR_UEFI_GET_POLICY_PROTOCOL > (EDKII_DEVICE_SECURITY_STATE_ERROR + 0x1) > +#define EDKII_DEVICE_SECURITY_STATE_ERROR_PCI_NO_CAPABILITIES > (EDKII_DEVICE_SECURITY_STATE_ERROR + 0x10) > +#define EDKII_DEVICE_SECURITY_STATE_ERROR_TCG_EXTEND_TPM_PCR > (EDKII_DEVICE_SECURITY_STATE_ERROR + 0x20) > + > +/** > + This function returns the device security policy associated with the > device. > + > + @param[in] This The protocol instance pointer. > + @param[in] DeviceId The Identifier for the device. > + @param[out] DeviceSecurityPolicy The Device Security Policy associated > with the device. > + > + @retval EFI_SUCCESS The device security policy is returned > +**/ > +typedef > +EFI_STATUS > +(EFIAPI *EDKII_DEVICE_SECURITY_GET_DEVICE_POLICY) ( > + IN EDKII_DEVICE_SECURITY_POLICY_PROTOCOL *This, > + IN EDKII_DEVICE_IDENTIFIER *DeviceId, > + OUT EDKII_DEVICE_SECURITY_POLICY **DeviceSecurityPolicy > + ); > + > +/** > + This function sets the device state based upon the authentication result. > + > + @param[in] This The protocol instance pointer. > + @param[in] DeviceId The Identifier for the device. > + @param[in] DeviceSecurityState The Device Security state associated > with the device. > + > + @retval EFI_SUCCESS The device state is set > +**/ > +typedef > +EFI_STATUS > +(EFIAPI *EDKII_DEVICE_SECURITY_SET_DEVICE_STATE) ( > + IN EDKII_DEVICE_SECURITY_POLICY_PROTOCOL *This, > + IN EDKII_DEVICE_IDENTIFIER *DeviceId, > + IN EDKII_DEVICE_SECURITY_STATE *DeviceSecurityState > + ); > + > +struct _EDKII_DEVICE_SECURITY_POLICY_PROTOCOL { > + UINT32 Version; // 0x1 > + EDKII_DEVICE_SECURITY_GET_DEVICE_POLICY GetDevicePolicy; > + EDKII_DEVICE_SECURITY_SET_DEVICE_STATE SetDeviceState; > +}; > + > +extern EFI_GUID gEdkiiDeviceSecurityPolicyProtocolGuid; > + > +#endif > -- > 2.19.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#50165): https://edk2.groups.io/g/devel/message/50165 Mute This Topic: https://groups.io/mt/40117802/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
