From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-BN7-obe.outbound.protection.outlook.com (NAM10-BN7-obe.outbound.protection.outlook.com [40.107.92.62]) by mx.groups.io with SMTP id smtpd.web10.2025.1676618858761024503 for ; Thu, 16 Feb 2023 23:27:39 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@amd.com header.s=selector1 header.b=Y99qJ8B/; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: amd.com, ip: 40.107.92.62, mailfrom: abner.chang@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=f6h8QUYKICSXGADzz+A8npScwIaeEVdJLvaENi5LwCGzAMBO63CEz65Q+A88TKuRxw2n5j0jVlTFlfN7Xqq1scB3q6UUjcJ88v6Q9mzCokoF3O4wGh0YxNuiQnr/Nib0PYF2oY0FoTJf+xez+aeRt8zJp5doSRj/mKNiG5vC6C4x+4BSaK72N+i8H1aq3QbPaYIfleS2HCaxNplQaG7vqO+hDenVxwtnORNuXeA+J+RNGxKoEraEM0R0WCahMunObCuYc2jyFIY+zOJ7NRbGtB70vCnIsqdv0g0WWFud6PH4J1aoK52mUdDl630h0bsXwuKuVCtaC6v5B3VIr/JEXQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=VDymsohXOOcXzPlBKtJndC6es26r3zQUEmLC/5FWKZA=; b=Ke/vTr3KiO31PJc4fFhb13yEm3/phCGI87+l3e8tC0/n7gCOmG+bhvRjd5Oi8cywCSKLZeSRLanffaupf8unX7/zl/jrjE3kd694Y7Cvj4DQXtNSAKKlDRC2vPwkI5iSPQu9O97oyfm7+vDUo2nwD293FBwrItqNUFa/PrHLnmN5V43+3Ep4NFQTkuihKsqkgEG8CV3iv5bjgBYJb82+4Nv8ziNHfUrIor4chQGnKrUn9qApkc1jk/1zu5acjr/AAbPnRlm8kA8CyGQDAjOXbmfaaX0+uSfopHjrnzJ+GI719fY+WVLw0m6VTkHW/NDn6yxP963vqfoq5gADjM2+WA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=VDymsohXOOcXzPlBKtJndC6es26r3zQUEmLC/5FWKZA=; b=Y99qJ8B/gE4D8luPE/eQNVIu48mIE3/Cv2H+yPim7Wp6xiKZquKM2Gmie+7X4QFVRF1SLBA/mO32wqOMSJH1vlAqv9n2sPR+WdTQUp7oLW4GcJijN1ONgeRAsY15XA2s5Sl1O93LhTrvfyzZCY150B0ATCRdffxzNZw5SmBPfY0= Received: from MN2PR12MB3966.namprd12.prod.outlook.com (2603:10b6:208:165::18) by CH3PR12MB8235.namprd12.prod.outlook.com (2603:10b6:610:120::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6086.26; Fri, 17 Feb 2023 07:27:35 +0000 Received: from MN2PR12MB3966.namprd12.prod.outlook.com ([fe80::d606:ab63:cf3:5d36]) by MN2PR12MB3966.namprd12.prod.outlook.com ([fe80::d606:ab63:cf3:5d36%5]) with mapi id 15.20.6086.024; Fri, 17 Feb 2023 07:27:35 +0000 From: "Chang, Abner" To: "Oram, Isaac W" , "devel@edk2.groups.io" CC: "Gao, Liming" , "Desimone, Nathaniel L" , Nickle Wang , Igor Kulchytskyy , "Attar, AbdulLateef (Abdul Lateef)" , Leif Lindholm , "Kinney, Michael D" Subject: Re: [edk2-platforms][PATCH 0/7] Implementation of IPMI Protocol Thread-Topic: [edk2-platforms][PATCH 0/7] Implementation of IPMI Protocol Thread-Index: AQHZOxB5eqabOzzljEG04y6Y095cja7QuNeQgAHnw9A= Date: Fri, 17 Feb 2023 07:27:35 +0000 Message-ID: References: <20230207162236.1406-1-abner.chang@amd.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_Enabled=true; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_SetDate=2023-02-17T07:27:32Z; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_Method=Standard; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_Name=General; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_SiteId=3dd8961f-e488-4e60-8e11-a82d994e183d; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_ActionId=b00a007c-3eba-470c-b1d7-d7205227bee3; MSIP_Label_4342314e-0df4-4b58-84bf-38bed6170a0f_ContentBits=1 authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com; x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN2PR12MB3966:EE_|CH3PR12MB8235:EE_ x-ms-office365-filtering-correlation-id: f0713b35-6d8c-4a45-2ab5-08db10b87094 x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: vk9eub98R6BugVJiX68IQULZpJRIGf3/C5K0Avj+sHOpQKrpF5H67L4HuGyUpcWgU03mCS34LLnlzAgqnSpiM2ek92H2gP9RJ/ZNUriCDWAnAcTFd4I7/uXaDxgbCNWHyQUR+/syZnbhYzftWZSc6rUoU0lK4Kq+gfApJMZ5Yed2OhR+q4SaqDoWgscqurDHuG+m/c6dW3DZ6Ab5OfZyVU40Am5PweXiDH1j1fWX9jiZVyTDDPbKn74g0lOqTJhK9j82dmWown0lwFPCPR/Ci5LWFPNo981/n9wNNV6s/dnzJX/zp4v8bASH8fHnk8EN+GIZ/JCFkKcmcBt2DR/oSYkKRYwXkL/DlUwQbO5guZhIbJGidpzCwoeJ8yIDMBJ+UOp2hXklayfcD0IwNh98MTCdDCQY2UUBiNI2ddvIKOSxgV5nsVZ/S0vJDu8wc0KLwwxw55VmwQfwvO53jI7LS5ipk/28iZ65a0NYjna8F6YDf+sOaNxKGSrzfEniXAkZlztTIL1Nw02+YbhrelHa+F68chuFxpOkLji2akoFLsxtkLz2otJ+O3+yRMI0704XHTboCyImHC2mjaL0HHbzsFg7vp4wPVMgLWO1oKnNdEIfdVdEo3MEsBmp8BwVwzUZRj6jj94VFHR/fOMB7AAQdPFyZm7jEC+opiNddYmfOznfmxP9fpKvnsi9h/fpkBID x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MN2PR12MB3966.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230025)(4636009)(346002)(39860400002)(136003)(396003)(376002)(366004)(451199018)(478600001)(2906002)(6506007)(26005)(9686003)(966005)(53546011)(186003)(7696005)(52536014)(38100700002)(71200400001)(86362001)(8936002)(41300700001)(122000001)(5660300002)(33656002)(38070700005)(54906003)(316002)(110136005)(83380400001)(19627235002)(55016003)(66556008)(66946007)(8676002)(76116006)(64756008)(66476007)(66446008)(4326008);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?PG3pwWv48xRbzoDqiBU/EcDO5JzakBF6lbqAWDSoipaMfLLjIVGgijy9hteU?= =?us-ascii?Q?wdGf6/PwHZFZ1X/e52LWe/+CgqzB5VlGPd8I7sUmVK0NOueq77Cc+gBZeD4L?= =?us-ascii?Q?ZhRMtu1/afu8o73xfDtSfjFYyFVNB/lmMVpYiYrgs5NAMva6C3+je+aXNqS0?= =?us-ascii?Q?NFetpTvnV0gwpvTXa5Uq6biCQ/HIX43M7CW0XsOMC5H1ZL+PB65ffJRzMFsy?= =?us-ascii?Q?CNs6BsEWhII8V3tMHt1TOVJVT/vtriNwdy5btHWSr6qlbkmTNl6sTBTJVSqT?= =?us-ascii?Q?GyOF8yI/io6c+hV/R7paxU9QeueHyeawra5hEYSakc9nTe8I5UFD+R6D3RGT?= =?us-ascii?Q?gyvOK6Jp98ytZHFZqtd7z4E1PCYCys/Qc0Qv2dbFRUWC4EnVv8BEHV+D40AA?= =?us-ascii?Q?ABury6PB2uB8woXoE+x8AXAmE6GhMlLzd5LbmXDYKo+AvkhZ8sMr7xr/tPxd?= =?us-ascii?Q?fXSdY5Is7lypoFwRO9lcTohxTPVm7xVngIvSS9/qLWKFmOI3PZLNtndKmRFK?= =?us-ascii?Q?e0MyFzxlZHEsdgD3+0pDcCveSVBqh5VkiqTKOC36/2OrxsBBcO2Ml0rmwT48?= =?us-ascii?Q?ehOwyu6Mxxgav0uxA7bCr4p00XcAqbMSdeci1zcVnNQUxHqKEnVX5oQ9abUc?= =?us-ascii?Q?yD/yf2TTbE4PKGhbjAo/ToMimNKgYY6+HRJYFp5mCydATUTsDVHttXmCsk/r?= =?us-ascii?Q?m5RgE7CYfD3UhnEi7xsgbegQVypOxC1P+msMo0JS49KUCCC1B0Weh1ZsppDp?= =?us-ascii?Q?z2HctFUoEBX1MMkzxkd/+Bd5MKqxLUMxN6HLt2XiVm61vihDn+KM79sQgn9u?= =?us-ascii?Q?QDyQrNxvtkaKiXCzhhD/SHiNexa8IV+YXrJggj99x2TDIP1T1eRWYG7QC3yy?= =?us-ascii?Q?qK+zQrUx73+Bz1XJbs6kPcugnjHGdcKURDNW6gDcy3v6OyUXCV+irpkyp3qx?= =?us-ascii?Q?aTJdbxAzw6i2uTmt5HtvKd1sSGfWDWa2kdfd8FlJaPGa/ujZPJvl/q9vHTi5?= =?us-ascii?Q?xQyAQW0kmEco3hKmIYJEN7igOeuXXsRb/u5dZH1FG6Uu5HMjAhEhI6uFg05y?= =?us-ascii?Q?D6lYZ0q/PaU9eoFUG9DZkVWAZa38ipCAQy0TR05bSx+RrodZJa6I7+AUtMrD?= =?us-ascii?Q?e8faOGWMLw9+UPX88o0LSHVsVtIeFXEokYRe+rJcmydVFx8ziuGO0B3NUcw2?= =?us-ascii?Q?/QhRi4CFiQrxtXaDlwly6Bbtet8JG7pfzUt5oJkgAJHX3NA3CgbRiUN03Sst?= =?us-ascii?Q?RH5YGJc/WrIXD+NB0WD6yLhubyezMLZQ5YUnVPNMowGmVAMWK4R1i9e3Ohkz?= =?us-ascii?Q?7W76yxUK1VqWxsIuC8MeJ+uY4cZ8OdwzdKaNcYl21CvjJDz55qB/gr7avepA?= =?us-ascii?Q?i6kXQQx5PTyrTMPfEglRoEeTiWT8hy/E3xdDJ2oNvAxFmCaomgSJ2qDNSJSc?= =?us-ascii?Q?2dgKU06ybG+zKrGyFkdqTuJTftsNfimRmQYLu47RxmLW2Gk8QnGWlF0P0LfI?= =?us-ascii?Q?48FywuSr5I030id3SIgYop5gd0zuOJOlHqSbm92hNvQEiV1RAu0QHGIF2xei?= =?us-ascii?Q?P/o1kAhLAau0H+bLSg4=3D?= MIME-Version: 1.0 X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MN2PR12MB3966.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: f0713b35-6d8c-4a45-2ab5-08db10b87094 X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Feb 2023 07:27:35.1217 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: tlYJAX7a056TUL+CFYbPm6RB7/OFd049jlEBiU7FhcaSjQHzNAlc+jBtu3q5aYqgc6STI2RRcHNLCcQyY+A/TQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB8235 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable [AMD Official Use Only - General] Hi Isaac, Thanks for spending time on this. Please see my feedback below, > -----Original Message----- > From: Oram, Isaac W > Sent: Thursday, February 16, 2023 9:42 AM > To: Chang, Abner ; devel@edk2.groups.io > Cc: Gao, Liming ; Desimone, Nathaniel L > ; Nickle Wang ; > Igor Kulchytskyy ; Attar, AbdulLateef (Abdul Lateef) > ; Leif Lindholm ; > Kinney, Michael D > Subject: RE: [edk2-platforms][PATCH 0/7] Implementation of IPMI Protocol >=20 > Caution: This message originated from an External Source. Use proper > caution when opening attachments, clicking links, or responding. >=20 >=20 > In looking at the code, this is what I see: >=20 > edk2: > There is an IpmiLib.h library class which abstracts the > IpmiPpi.h/IpmiProtocol.h use > There is an IpmiPpi.h/IpmiProtocol.h which provide an interface t= o send > a command "via IPMI" > Current IpmiPpi.h/IpmiProtocol.h implementations use IpmiBaseLib > library class to send commands >=20 > edk2-platforms/Features/Intel/OutOfBandManagement/IpmiFeaturePkg: > There is an IpmiBaseLib library class which abstracts > IpmiTransportPpi.h/IpmiTransportProtocol.h use > There is an IpmiTransportPpi.h/IpmiTransportProtocol.h that abstr= acts > the transport protocol. > Currently IpmiTransportPpi.h/IpmiTransportProtocol.h are implemen= ted > in "GenericIpmi". >=20 > It appears to me that they are meant to be the same API. The > IpmiFeaturePkg transport version adds a function to GetBmcStatus, but it > seems like IpmiSubmitCommand is meant to be the same function > throughout. > I don't find any use of the GetBmcStatus function that is in the > IpmiFeaturePkg but not the edk2 API. I am not sure what to make of that.= I > think we need more feedback from current consumers to resolve that. > >=20 > When I look at our internal reference version which is an ancestor to the > edk2-platforms/Feature/Intel.../IpmiFeaturePkg version, there is no use o= f > the IpmiLib.h/IpmiPpi.h/IpmiProtocol.h. There are additional fields in > interfaces, like GetBmcStatus and data values, but they seem to have been > redesigned out when open sourcing the interfaces. I don't really know the history of IpmiFeaturePkg and no idea why the imple= mentation is sort of duplicated with the Ipmi stuff defined in edk2. >=20 > My main questions: > To your understanding, are the IpmiLib.h/IpmiPpi.h/IpmiProtocol.h and the > IpmiBaseLib.h/IpmiTransportPpi.h/IpmiTransportProtocol.h simply two > versions of basically the same with a library API abstracting a dynamic p= hase > specific IPMI transport API? > Why does ManageabilityPkg use IpmiBaseLib and not IpmiLib? Is it more > than a temporary solution? So what I was done is to connect the implementation between edk2 and edk2-p= latforms to avoid the design changes on both side. There are IpmiLib implementation under MdeModulePkg/Library, those are DxeIpmiLibIpmiProtocol Library PeiIpmiLibIpmiPpi Library SmmIpmiLibSmmIpmiProtocol Library These libraries provide the abstract library of IpmiProtocol and this imple= mentation makes sense to me for the usage of IPMI high level driver. The drivers under ManageabilityPkg is the implementation of IpmiProtocol, s= o it uses IpmiBaseLib as IpmiBaseLib is the abstract API of IpmiTransport p= rotocol. So that is, IpmiLib->IpmiProtocol->IpmiBaseLib->IpmiTransport (GenericIpmi). Again, the= implementation of IpmiProtocol under ManageabilityPkg is the connection be= tween existing IpmiLib and IpmiBaseLib. More feedbacks in the each patch. Thanks Abner >=20 >=20 > High level feedback: > 0001 - I think we should not make this change. It impacts any existing u= sers > for no reason I can justify. > 0002 - Not sure - per my understanding of the stack, it doesn't quite fit= the > code in an obvious way. > 0003 - More description of the design and duplication, the roles of the l= ibrary > class and the PPI/protocols, and the relationship to other IPMI code in t= he > repos. > 0004 - Minor feedback only > 0005/0006 - I think that we should skip the step of using IpmiBaseLib and= just > implement the modified version of GenericIpmi to support the edk2 API > stack design. >=20 > I will send more specific details in response to each commit shortly. >=20 > Regards, > Isaac >=20 > -----Original Message----- > From: abner.chang@amd.com > Sent: Tuesday, February 7, 2023 8:22 AM > To: devel@edk2.groups.io > Cc: Gao, Liming ; Oram, Isaac W > ; Desimone, Nathaniel L > ; Nickle Wang ; > Igor Kulchytskyy ; Abdul Lateef Attar > ; Leif Lindholm ; Kinney, > Michael D > Subject: [edk2-platforms][PATCH 0/7] Implementation of IPMI Protocol >=20 > From: Abner Chang >=20 > This change implementes IPMI Protocol and PPI in the new introduced > ManageabilityPkg (described in below email) > https://edk2.groups.io/g/devel/message/95579?p=3D%2C%2C%2C20%2C0%2C > 0%2C0%3A%3ACreated%2C%2CManageability%2C20%2C2%2C0%2C94572748 >=20 > BZ #4336: > The change also fixes the confusion (Patch 1/7) of IpmiSubmitCommand() > deinfed in IPMI Transport protocol. >=20 > You can skip reviewing on patch 2/7 as it is an image file. >=20 > Signed-off-by: Abner Chang > Cc: Liming Gao > Cc: Isaac Oram > Cc: Nate DeSimone > Cc: Nickle Wang > Cc: Igor Kulchytskyy > Cc: Abdul Lateef Attar > Cc: Leif Lindholm > Cc: Michael D Kinney >=20 > Abner Chang (7): > IpmiFeaturePkg: Rename IpmiSubmitCommand function > ManageabilityPkg: Add diagrams > ManageabilityPkg: Add Readme file > ManageabilityPkg: Initial package > ManageabilityPkg: Implement Ipmi Protocol/Ppi > IpmiProtocol: Add to Manageability Package > edk2-platforms: Maintainers.txt >=20 > .../ManageabilityPkg/ManageabilityPkg.dec | 18 ++++ > .../Include/CommonLibs.dsc.inc | 52 +++++++++ > .../ManageabilityPkg/ManageabilityPkg.dsc | 27 +++++ > .../IpmiProtocol/Dxe/IpmiProtocolDxe.inf | 37 +++++++ > .../Universal/IpmiProtocol/Pei/IpmiPpiPei.inf | 38 +++++++ > .../IpmiProtocol/Smm/IpmiProtocolSmm.inf | 40 +++++++ > .../Include/Library/IpmiBaseLib.h | 2 +- > .../Include/Ppi/IpmiTransportPpi.h | 2 +- > .../Include/Protocol/IpmiTransportProtocol.h | 2 +- > .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 2 +- > .../GenericIpmi/Pei/PeiGenericIpmi.c | 2 +- > .../GenericIpmi/Smm/SmmGenericIpmi.c | 2 +- > .../Library/IpmiBaseLib/IpmiBaseLib.c | 4 +- > .../Library/IpmiBaseLibNull/IpmiBaseLibNull.c | 2 +- > .../IpmiCommandLib/IpmiCommandLibNetFnApp.c | 26 ++--- > .../IpmiCommandLibNetFnChassis.c | 12 +-- > .../IpmiCommandLibNetFnStorage.c | 24 ++--- > .../IpmiCommandLibNetFnTransport.c | 8 +- > .../Library/PeiIpmiBaseLib/PeiIpmiBaseLib.c | 4 +- > .../Library/SmmIpmiBaseLib/SmmIpmiBaseLib.c | 4 +- > .../Universal/IpmiProtocol/Dxe/IpmiProtocol.c | 97 +++++++++++++++++ > .../Universal/IpmiProtocol/Pei/IpmiPpi.c | 102 ++++++++++++++++++ > .../Universal/IpmiProtocol/Smm/IpmiProtocol.c | 98 +++++++++++++++++ > .../Ipmi/Library/IpmiLibKcs/IpmiLibKcs.c | 12 +-- > Features/ManageabilityPkg/Readme.md | 37 +++++++ > .../Media/ManageabilityDriverStack.svg | 1 + > Maintainers.txt | 9 +- > 27 files changed, 608 insertions(+), 56 deletions(-) create mode 100644 > Features/ManageabilityPkg/ManageabilityPkg.dec > create mode 100644 > Features/ManageabilityPkg/Include/CommonLibs.dsc.inc > create mode 100644 Features/ManageabilityPkg/ManageabilityPkg.dsc > create mode 100644 > Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocolDxe.in > f > create mode 100644 > Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiPei.inf > create mode 100644 > Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocolSmm > .inf > create mode 100644 > Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocol.c > create mode 100644 > Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpi.c > create mode 100644 > Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocol.c > create mode 100644 Features/ManageabilityPkg/Readme.md > create mode 100644 > Features/ManageabilityPkg/Documents/Media/ManageabilityDriverStack.sv > g >=20 > -- > 2.37.1.windows.1