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

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thanks. Comment below:

> -----Original Message-----
> From: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> Sent: Thursday, November 7, 2019 5:51 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Lou, Yun <yun.lou@intel.com>
> Subject: RE: [PATCH V2 2/6] IntelSiliconPkg/Include: Add Platform Device
> Security Policy protocol
> 
> Same feedback as provided in patch1/6.
[Jiewen] Agree. See response in previous email.
I will add structure description in V3.

> In addition, is there a reason to add "EDKII" as prefix in the internal data
> structure names? E.g.
> +typedef struct {
> +  UINT32     Version; // 0x1
> +  UINT32     MeasurementPolicy;
> +  UINT32     AuthenticationPolicy;
> +} EDKII_DEVICE_SECURITY_POLICY; // this can be named simply as
> "DEVICE_SECURITY_POLICY" right?
[Jiewen] I believe it is common practice to add EDKII_ prefix to all data structure.

I did a search in MdeModulePkg.
I do see some EDKII protocol adds EDKII_ prefix for the data structure.
But some of them does not.

I feel better if we add it to avoid namespace confusing.
I am open for discussion.


> 
> Also, on the services "GetDevicePolicy" and "SetDeviceState", is it possible to
> have any other return states than EFI_SUCCESS?
[Jiewen] Good question. I can add something in V3.


> 
> Regards,
> 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 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/PlatformDeviceSecurityPolicy.h
> b/Silicon/Intel/IntelSiliconPkg/Include/Protocol/PlatformDeviceSecurityPolicy.h
> new file mode 100644
> index 0000000000..cb5a71ad41
> --- /dev/null
> +++
> b/Silicon/Intel/IntelSiliconPkg/Include/Protocol/PlatformDeviceSecurityPolicy.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