From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.1006.1614799372119416153 for ; Wed, 03 Mar 2021 11:22:52 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=YB4O/XcO; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.124.238.202] (unknown [131.107.174.202]) by linux.microsoft.com (Postfix) with ESMTPSA id 4103220B83EA; Wed, 3 Mar 2021 11:22:51 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 4103220B83EA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1614799371; bh=lRxrOaMAjUITBHvbz5887pJ875KxkQXDMz9BBczf/Dc=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=YB4O/XcOjzn8M7aYt55kloBXmFLU/SM/if0zTvBde0vQ1+JdPXUkQnvxTBZL4hxKA ovtaTdhuOur+T//bhXr/0Ra3elC5t2TBlnGsjRPjvTEDpaEPX4z2Z/+ILXbnoBDF31 02GQrM/bMB8I+WJDj/alYW9yfrgLQZ3/EEsKKcq8= Subject: Re: [edk2-devel] [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers To: devel@edk2.groups.io, nathaniel.l.desimone@intel.com Cc: Isaac Oram , Sai Chaganty , Liming Gao , Michael Kubacki References: <20210302022804.8641-1-nathaniel.l.desimone@intel.com> From: "Michael Kubacki" Message-ID: Date: Wed, 3 Mar 2021 11:22:51 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <20210302022804.8641-1-nathaniel.l.desimone@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Looked over the series at a high-level and the feature structure looks fine. For the series: Acked-by: Michael Kubacki I didn't look closely at implementation but there's a few things I noticed. There's a few typos. I'd suggest running a spell check to clean those up. Some quick examples: ServerManagement.h: "Generic Definations for Server Management Header File." typedef struct { LINERIZATION_TYPE Linearization; // L IpmiTransportPei.h: "IPMI Ttransport PPI Header File." There was a mix of function documentation styles though that might be something to clean up later. I saw some globals that did not appear necessary. For example, mIpmiTransport in SmmIpmiBaseLib.c seems to be located before every access. Thanks, Michael On 3/1/2021 6:27 PM, Nate DeSimone wrote: > From: Isaac Oram > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3242 > > Implement an open source generic IPMI transport driver. > Provides the ability to communicate with a BMC over IPMI > in MinPlatform board packages. > > New changes: > 1. Added GenericIpmi > 2. Added IPMI base libs > 3. Added transport PPI and protocol > 4. Updated IPMI command request and response data size from > UINT8 to UINT32 in IPMI transport layer to be compatible > with EDK2 industry standard IPMI commands. > 6. Added the completion code in the first byte of all IPMI > response data to be compatible with EDK2 industry standard > IPMI commands. > > Cc: Sai Chaganty > Cc: Liming Gao > Cc: Michael Kubacki > Signed-off-by: Isaac Oram > Co-authored-by: Nate DeSimone > > Isaac Oram (9): > IpmiFeaturePkg: Add IPMI driver Include headers > IpmiFeaturePkg: Add IpmiBaseLib and IpmiCommandLib > IpmiFeaturePkg: Add IpmiInit drivers > IpmiFeaturePkg: Add GenericIpmi driver common code > IpmiFeaturePkg: Add GenericIpmi PEIM > IpmiFeaturePkg: Add GenericIpmi DXE Driver > IpmiFeaturePkg: Add GenericIpmi SMM Driver > IpmiFeaturePkg: Add IPMI driver build files > Maintainers.txt: Add IpmiFeaturePkg maintainers > > .../GenericIpmi/Common/IpmiBmc.c | 297 +++++++++++ > .../GenericIpmi/Common/IpmiBmc.h | 44 ++ > .../GenericIpmi/Common/IpmiBmcCommon.h | 179 +++++++ > .../GenericIpmi/Common/IpmiHooks.c | 96 ++++ > .../GenericIpmi/Common/IpmiHooks.h | 81 +++ > .../GenericIpmi/Common/IpmiPhysicalLayer.h | 29 ++ > .../GenericIpmi/Common/KcsBmc.c | 483 ++++++++++++++++++ > .../GenericIpmi/Common/KcsBmc.h | 236 +++++++++ > .../GenericIpmi/Dxe/GenericIpmi.c | 46 ++ > .../GenericIpmi/Dxe/GenericIpmi.inf | 66 +++ > .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 452 ++++++++++++++++ > .../GenericIpmi/Pei/PeiGenericIpmi.c | 362 +++++++++++++ > .../GenericIpmi/Pei/PeiGenericIpmi.h | 138 +++++ > .../GenericIpmi/Pei/PeiGenericIpmi.inf | 58 +++ > .../GenericIpmi/Pei/PeiIpmiBmc.c | 277 ++++++++++ > .../GenericIpmi/Pei/PeiIpmiBmc.h | 38 ++ > .../GenericIpmi/Pei/PeiIpmiBmcDef.h | 156 ++++++ > .../GenericIpmi/Smm/SmmGenericIpmi.c | 208 ++++++++ > .../GenericIpmi/Smm/SmmGenericIpmi.inf | 53 ++ > .../IpmiFeaturePkg/Include/IpmiFeature.dsc | 9 +- > .../Include/Library/IpmiBaseLib.h | 50 ++ > .../Include/Library/IpmiCommandLib.h | 19 +- > .../Include/Ppi/IpmiTransportPpi.h | 68 +++ > .../Include/Protocol/IpmiTransportProtocol.h | 75 +++ > .../IpmiFeaturePkg/Include/ServerManagement.h | 337 ++++++++++++ > .../IpmiFeaturePkg/Include/SmStatusCodes.h | 98 ++++ > .../IpmiFeaturePkg/IpmiFeaturePkg.dec | 22 +- > .../IpmiFeaturePkg/IpmiInit/DxeIpmiInit.c | 4 +- > .../IpmiFeaturePkg/IpmiInit/DxeIpmiInit.inf | 6 +- > .../IpmiFeaturePkg/IpmiInit/PeiIpmiInit.c | 4 +- > .../IpmiFeaturePkg/IpmiInit/PeiIpmiInit.inf | 4 +- > .../Library/IpmiBaseLib/IpmiBaseLib.c | 155 ++++++ > .../Library/IpmiBaseLib/IpmiBaseLib.inf | 28 + > .../Library/IpmiBaseLibNull/IpmiBaseLibNull.c | 76 +++ > .../IpmiBaseLibNull/IpmiBaseLibNull.inf | 36 ++ > .../Library/IpmiCommandLib/IpmiCommandLib.inf | 4 +- > .../IpmiCommandLib/IpmiCommandLibNetFnApp.c | 7 +- > .../IpmiCommandLibNetFnChassis.c | 51 +- > .../IpmiCommandLibNetFnStorage.c | 7 +- > .../IpmiCommandLibNetFnTransport.c | 7 +- > .../Library/PeiIpmiBaseLib/PeiIpmiBaseLib.c | 111 ++++ > .../Library/PeiIpmiBaseLib/PeiIpmiBaseLib.inf | 30 ++ > .../Library/SmmIpmiBaseLib/SmmIpmiBaseLib.c | 180 +++++++ > .../Library/SmmIpmiBaseLib/SmmIpmiBaseLib.inf | 29 ++ > .../IpmiFeaturePkg/Readme.md | 4 +- > Maintainers.txt | 6 + > 46 files changed, 4694 insertions(+), 32 deletions(-) > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiPhysicalLayer.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/KcsBmc.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/KcsBmc.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.inf > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Library/IpmiBaseLib.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Ppi/IpmiTransportPpi.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Protocol/IpmiTransportProtocol.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/ServerManagement.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/SmStatusCodes.h > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLib/IpmiBaseLib.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLib/IpmiBaseLib.inf > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLibNull/IpmiBaseLibNull.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLibNull/IpmiBaseLibNull.inf > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/PeiIpmiBaseLib/PeiIpmiBaseLib.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/PeiIpmiBaseLib/PeiIpmiBaseLib.inf > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/SmmIpmiBaseLib/SmmIpmiBaseLib.c > create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/SmmIpmiBaseLib/SmmIpmiBaseLib.inf >