Hi Omkar,

Please find my feedback inline marked [SAMI].

Regards,

Sami Mujawar


On 24/08/2021 06:34 AM, Omkar Anand Kulkarni wrote:
> Add the protocol definition of the MM_HEST_ERROR_SOURCE_DESC_PROTOCOL
> protocol. This protocol can be implemented by MM drivers to publish
> error source descriptors that have to be populated into HEST table.
>
> Co-authored-by: Thomas Abraham <thomas.abraham@arm.com>
> Signed-off-by: Omkar Anand Kulkarni <omkar.kulkarni@arm.com>
> ---
>   ArmPlatformPkg/ArmPlatformPkg.dec                     |  3 +
>   ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h | 64 ++++++++++++++++++++
>   2 files changed, 67 insertions(+)
>
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
> index 3a25ddcdc8ca..446d93b880f9 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -127,3 +127,6 @@
>     gArmPlatformTokenSpaceGuid.PcdPL031RtcPpmAccuracy|300000000|UINT32|0x00000022
>   
>     gArmPlatformTokenSpaceGuid.PcdWatchdogCount|0x0|UINT32|0x00000033
> +
> +[Protocols.common]
> +  gMmHestErrorSourceDescProtocolGuid = { 0x560bf236, 0xa4a8, 0x4d69, { 0xbc, 0xf6, 0xc2, 0x97, 0x24, 0x10, 0x9d, 0x91 } }
> diff --git a/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h b/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
> new file mode 100644
> index 000000000000..86662ea7af57
> --- /dev/null
> +++ b/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
[SAMI] Can this file be rename to MmHestErrorSourceProtocol.h?
> @@ -0,0 +1,64 @@
> +/** @file
> +  MM protocol to get the secure error source descriptor information.
> +
> +  MM Drivers must implement this protocol in order to publish secure side
> +  error source descriptor information to OSPM through the HEST ACPI table.
> +
> +  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef MM_HEST_ERROR_SOURCE_DESC_
> +#define MM_HEST_ERROR_SOURCE_DESC_
> +
> +#define MM_HEST_ERROR_SOURCE_DESC_PROTOCOL_GUID \
> +  { \
> +    0x560bf236, 0xa4a8, 0x4d69, { 0xbc, 0xf6, 0xc2, 0x97, 0x24, 0x10, 0x9d, 0x91 } \
> +  }
> +
> +typedef struct MmHestErrorSourceDescProtocol
> +                 MM_HEST_ERROR_SOURCE_DESC_PROTOCOL;
[SAMI] Can this be renamed to EDKII_MM_HEST_ERROR_SOURCE_PROTOCOL ?
> +
> +/**
> +  Get HEST Secure Error Source Descriptors.
> +
> +  The MM drivers implementing this protocol must convey the total count and
> +  total length of the error sources the driver has along with the actual error
> +  source descriptor(s).
> +
> +  Passing NULL as Buffer parameter shall return EFI_INVALID_PARAMETR with the
> +  total length and count of the error source descriptor(s) it supports.
[SAMI]I think the error code EFI_BUFFER_TOO_SMALL should be returned 
when the Buffer is NULL and *ErrorSourcesLength is 0.
i.e. To determine the size of required buffer, the caller shall call 
this interface with Buffer = NULL and *ErrorSourcesLength = 0. In 
response this interface shall return an error code EFI_BUFFER_TOO_SMALL 
with the required buffer length in ErrorSourcesLength and count in 
ErrorSourcesCount.

The EFI_INVALID_PARAMETR should be returned when a parameter is invalid. 
e.g. ErrorSourcesLength should be an IN OUT parameter and cannot be NULL.
The ErrorSourcesCount could be an optional parameter and can be NULL.

Can you revisit the design of this interface, please?
[/SAMI]
> +
> +  @param[in]   This                MM_HEST_ERROR_SOURCE_DESC_PROTOCOL instance.
> +  @param[out]  Buffer              Buffer to be appended with the error
> +                                   source descriptors information.
> +  @param[out]  ErrorSourcesLength  Total length of all the error source
> +                                   descriptors.
> +  @param[out]  ErrorSourceCount    Count of total error source descriptors
> +                                   supported by the driver.
> +
> +  retval  EFI_SUCCESS           If the Buffer is valid and is filled with valid
> +                                Error Source descriptor data.
> +  retval  EFI_INVALID_PARAMTER  Buffer is NULL.
> +  retval  Other                 If no error source descriptor information is
> +                                available.
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *MM_HEST_GET_ERROR_SOURCE_DESCRIPTORS) (
[SAMI] Can this be renamed to EDKII_MM_HEST_GET_ERROR_SOURCE_DESCRIPTORS?
> +  IN  MM_HEST_ERROR_SOURCE_DESC_PROTOCOL *This,
> +  OUT VOID                               **Buffer,
> +  OUT UINTN                              *ErrorSourcesLength,
> +  OUT UINTN                              *ErrorSourcesCount
> +  );
> +
> +//
> +// Protocol declaration
> +//
> +struct MmHestErrorSourceDescProtocol {
> +  MM_HEST_GET_ERROR_SOURCE_DESCRIPTORS GetHestErrorSourceDescriptors;
> +};
> +
> +extern EFI_GUID gMmHestErrorSourceDescProtocolGuid;
> +
> +#endif // MM_HEST_ERROR_SOURCE_DESC_