Re: [PATCH V2 2/6] IntelSiliconPkg/Include: Add Platform Device Security Policy protocol

["Ni, Ray" <ray.ni@intel.com>]

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.yao@intel.com>
> Sent: Thursday, October 31, 2019 8:31 PM
> 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 2/6] IntelSiliconPkg/Include: Add Platform Device
> Security Policy protocol
> 
> 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/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