From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mx.groups.io with SMTP id smtpd.web09.1013.1655169988266274148 for ; Mon, 13 Jun 2022 18:26:28 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="unable to parse pub key" header.i=@intel.com header.s=intel header.b=d2MkRvps; spf=pass (domain: intel.com, ip: 134.134.136.65, mailfrom: hao.a.wu@intel.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1655169988; x=1686705988; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=Dqk1Nzq6qaelXmubCplYkgDo9iS/Eqc9ZSSOtAbfYfM=; b=d2MkRvpsTDVejPb6osmy2HGEftnWKHUGbTeb5zKRQZgLO4mv0h+x6Ee1 +hd/jvjGl9Z9qKdsM3jeOOI10b+l5bheYieruHQuIYzWtNSunQ4L7VAge vgGjqamvqE4UD24WnjS6kzvasg4cOKO/bmxRecuiMQamytkxwvMmJA2F3 RJRU3cGblvOfUSI3GX7Dcuw9DB6qCxcYRYMj/8BeMpNJHzgDVbHQwjtqE QMUwoIvUQZ6MWR+nfWUlKPetf1BmmpieiTDkdEBjNDrBvbTpfOoLEbajy EPGVRDUugo39amwuruUw3PClTc1Rw8PLprRbvuzafYTAngeQL7RVmBuIM Q==; X-IronPort-AV: E=McAfee;i="6400,9594,10377"; a="279506888" X-IronPort-AV: E=Sophos;i="5.91,298,1647327600"; d="scan'208";a="279506888" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Jun 2022 18:26:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.91,298,1647327600"; d="scan'208";a="726556168" Received: from orsmsx606.amr.corp.intel.com ([10.22.229.19]) by fmsmga001.fm.intel.com with ESMTP; 13 Jun 2022 18:26:27 -0700 Received: from orsmsx609.amr.corp.intel.com (10.22.229.22) by ORSMSX606.amr.corp.intel.com (10.22.229.19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.27; Mon, 13 Jun 2022 18:26:26 -0700 Received: from orsedg603.ED.cps.intel.com (10.7.248.4) by orsmsx609.amr.corp.intel.com (10.22.229.22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.27 via Frontend Transport; Mon, 13 Jun 2022 18:26:26 -0700 Received: from NAM04-MW2-obe.outbound.protection.outlook.com (104.47.73.173) by edgegateway.intel.com (134.134.137.100) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2308.27; Mon, 13 Jun 2022 18:26:26 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=NfOfyY1qdBlNa4LHBIdr48mpUm7v7xblWJD9BUznHGuoylf6SSauQ9vqWm/GG0Mozocn8tfjdmTvzSnNb55W24fpx+qt/StEx9Tikfs50++50r59Bc02uGR6HUbw2XV2MEUxO4TtzhTEAWIlp4HRxJTlsfxo10TWr5GvjeUf3fVT2nNpUFrm8NeA7bBsmaDjNW3zETxGv90Y4ARdSjAJfMNyqF+cajiVkZPZrWhpO7TZCDo2o7TjB8UOGIm6dql9JirQZOWjRp77rm8WJpd/S+CGDspO/XT65ysDQSB6dVbKFICYXGSOWAFbxt4VcGWCUIVNQPFfEPAH7SEmpxvyog== 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=Vr5WyoqprMcx5eXWXPMd8y8+NuAmp+seUHqxU9RfyuY=; b=lDoj2w1zY/ZfFVYVCs1BsGpfaA61Uf0E+hiCSkUWnMJOW+qmVCSIUJpcS4FQWNmC15hvFcNYc2kMcIQQzuaOcOQPFMZFR7kwMeEj7FGnIgJdjP4od+X4Sobm9xaSgPXeIRZZh9pmIFomcF1NKaSzT+662FcWo7TGRa4U9EYEp6piBRq2bAtzM926ZreV3u1HqnD7I3i5eucvXQv+Fe7spNu02HM3KVGybDXdkQ1GDaGhrzYHH51AOXXw1+S1igUldZRUwzPF8tqTa7orz0HlR1iU/fzNnGxpqIJ0CgAIro6qzFI1mGokS6qleFFSSXw36HJgcbmtGXflYzXwpAE/zA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none Received: from DM6PR11MB4025.namprd11.prod.outlook.com (2603:10b6:5:197::31) by BY5PR11MB4385.namprd11.prod.outlook.com (2603:10b6:a03:1c0::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5332.12; Tue, 14 Jun 2022 01:26:24 +0000 Received: from DM6PR11MB4025.namprd11.prod.outlook.com ([fe80::c473:f30f:6b1f:c5ec]) by DM6PR11MB4025.namprd11.prod.outlook.com ([fe80::c473:f30f:6b1f:c5ec%5]) with mapi id 15.20.5332.020; Tue, 14 Jun 2022 01:26:24 +0000 From: "Wu, Hao A" To: "Czajkowski, Maciej" , "devel@edk2.groups.io" CC: "Ni, Ray" Subject: Re: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use PCI_DEVICE_PPI to manage AHCI device Thread-Topic: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use PCI_DEVICE_PPI to manage AHCI device Thread-Index: AQHYeaNbITTSYmoSMkeMRh0OHsejLq1GQYHggAAlyXCABvXhAIAAvwHw Date: Tue, 14 Jun 2022 01:26:24 +0000 Message-ID: References: <20220606124529.2152-1-maciej.czajkowski@intel.com> <20220606124529.2152-3-maciej.czajkowski@intel.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 5c02ef9b-ae4b-468e-6a53-08da4da4e567 x-ms-traffictypediagnostic: BY5PR11MB4385:EE_ x-microsoft-antispam-prvs: x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: LFNkww/qJJkXvlYKGwSJZksk0ig/U6F86MbeZv2P7nveUSVtGjgvnIXAxAt+oKZrQ6CBRH4WFSWjkJpn9m+tAV3cJ5nj/CEphOnwAn/OGJRXfBQ7kyB9qTRLBhMahsiATv0+piUfiTKoKQ/Pi1GRkw75mZ+o1Y4Hx4JjCZxbs88q3OYIVchBtGIfXo2MI9/TRMy6ZPeAe8hoSiWwRp1CKcUdjoLK4vTxzMzpTOuPRFt2+J8GMCHtQ1o1pG5dwJo7IkWPq7uvRIIDcKv6vu+y0r89dYKehi9/FwaBv/TbjBK95urrjSh074RpvNvMxU30uuagDsx0lSJtGrjbhZoKcqc1uTlR3C8N7a9x5rH9bGIXpLW8sQi1ivsH7N3J2uyZi3QHLWWGPnlotMhIubAAOU2qcSuteXXVOE/MOvg1ynipLb6JoHc1Hg2gVUpqn35KMmqJ3igngH5FNQCr4mwfJH0UI8BcNZjTz+YSdfsFWWTvk37xdOoeFjanDUJqp5buF3vIE+TJZbhHbuXmIAFe53ilU+sMNImS3DrgnfHuEp7RoVGMsUgQ6ZyK7mlHDmmhL8hlZL4Jdqo3JVOy9q5GKBfERcYtAob6h8YJPgd38gSDbWvINfUJXAZ5yYJaKDcySqzMX22y7QKd7xBnPR+GXt1HuLuCdHRdWPCOFTAltKEIDBMGb67Ygu5QgXvmxL6qFgAJevF92ZIkMZTj8jMhhJ9fztQ4K9kuJNdn1fNnvILZsLvvQl2N2YUKSJVxjxJWvzJyoHuo+6ZibhUtXumKKQhKf8QEcxI128biW5OzqKqyn3YeeH7sPJmdRlwTCITz2esqpHTgWZPSuDrGCcEEjw== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM6PR11MB4025.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230016)(366004)(5660300002)(7696005)(52536014)(83380400001)(9686003)(6506007)(53546011)(26005)(966005)(508600001)(33656002)(38100700002)(8936002)(66556008)(64756008)(66476007)(30864003)(66946007)(107886003)(86362001)(38070700005)(316002)(186003)(110136005)(66446008)(71200400001)(76116006)(4326008)(8676002)(19627235002)(55016003)(2906002)(82960400001)(122000001)(579004)(559001);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?f2RmSs5mEzrIv6bgKoRzcQ25VkJiBrrSCKU2Ls/3UV4/YICFFaE53lFTQ3LK?= =?us-ascii?Q?mLWzanSylMNyFfMXQHvypYuXgU5wNsrB24kRlCNDV08aPotFYNIdJ3UeQJkD?= =?us-ascii?Q?uTNCC/EJqfXI/rSStTRpfrd+eNsNaqFjLlSr1j7KzS0IxF4s5Qp8iSW3k256?= =?us-ascii?Q?hWnBtC4rE+9ZBq7RIzDdPlNsPyKjQdyJbADeJSnzHburTVB6Okj+XkwwSn0E?= =?us-ascii?Q?3Hn+8pUQ5JwskbLzm+YRVslEPMffvD/Ks8HH5G0FYKNY2dMGqAvoiN0f0nVX?= =?us-ascii?Q?kbzYp9PXf1Sjds7hJ24uh1uSUTYl6H3Wcd/jGIruyfvyeTpJqKNC50vwU3k9?= =?us-ascii?Q?gBvW3d0+q6o4cBLlIunqmyXFi+0CF0wdujipQi9hvUk4Khew3I6mPbWsbP2b?= =?us-ascii?Q?4Sgqez8wLjN17nPNAKXJBcbRekrkm0M5nX5Y9WdbD0XFrET+XEAZzIzbGBTn?= =?us-ascii?Q?igiFhAy+nMtEnE7LlMkhUc0OH995oTau+xPl9YZ3Fm93Z2XxkRI0y59QFEec?= =?us-ascii?Q?+ZyFWxIfqAtrQ9InVhEOLgtBotIwFMPHNxU1DzOnMOfYWbJ9yZj8i/QRZ+Pj?= =?us-ascii?Q?20jesqX05FqTebPyoVfGH5Vb7UYrbS9slTIdcAyPB6meZmDL2l2dpNgdVELN?= =?us-ascii?Q?qm3+/3DhZy3m7zSv1gwBQ+XkQ21/F/VMHXgdbSZntbpSga2TPeKk4RB0Hp+3?= =?us-ascii?Q?y4mZyYQq3JIhqk7LhnPyfVqg8fE2Ewoikgsr0632iQQ1zHsk4SfA3VuhI/vX?= =?us-ascii?Q?K+524jUsO7aXRKaNkm9kspJVH7Ms3wwaP0fghDOlVH0PRsymMZzS4rJmwxZQ?= =?us-ascii?Q?cjYlBXepCm/uBVQdnLBGDZrlSBsdBO+xwjM6baOSUYD2dAGTdI2pK4Buk9me?= =?us-ascii?Q?oeBKAlINlErV0Y7NiUqr47mYMqUisMQ6ndPWDg9pQ13dFSldlUpDXsBjWzwV?= =?us-ascii?Q?RU4fyXcA4EGJspo6mJwRwh/RB/+qlRBpDMW6RoWTWCI1LEw7J3U3jChKli29?= =?us-ascii?Q?U/tgyGCgr787mN1HFDLAPWQaJ8fh/sO8iEe1vzIRyVPJKFDHqSqzaFGtTITF?= =?us-ascii?Q?xE79cQVrfG5Y57rx2BBBbCV/5UZo1r/nroLoNhDk0+5eGJ75+iL/xtYe7AeH?= =?us-ascii?Q?DvtOixG87m0dhNhO1/6oNryJTX+LPJiI8iYtM6tUJyR31Bqsx7N622UrbK3A?= =?us-ascii?Q?c8uSQTwqNxq2JkEXHXduDN0LrI3KF8BEaeMoS7WZ6ynALR66kykUhC5caN5B?= =?us-ascii?Q?gKEy8kIdiBThN4qdytCka5jGJVUxlM75D48GFYc/a9Z+f75Ulo65wn/4GDdV?= =?us-ascii?Q?uJvye+bpWYEvSzBPwjVweEyjGjurAB2gRClK0YOeuaVN49BmnOS2A6LaSsU0?= =?us-ascii?Q?oPLM9yhgHALWJZIwFE9GifWCi7VcUfpI1W5j4fr/nN+p+NWPaQ32yd39NXTL?= =?us-ascii?Q?toiQUN4euNtvd1JVhFl07JsdgYKQ9KjIH/8xxAPRCdgyRw0QLvNBrCA6akLL?= =?us-ascii?Q?fYi0zB9qye6gDY89WERaW2AORFnePXK5+YuvqQSFvA3exa8RpBdYXB+yyfoO?= =?us-ascii?Q?XTrkcQTesIOgDKMW90c0tqdvxdEMX2XRZXxRZWZDIzHHQvpjG/MLx0LfQtls?= =?us-ascii?Q?wYQtgTYCQAGGaBpTZPOtve+Y0L66ExLRl18IR0aiWam9kxTn3loxWScmh89u?= =?us-ascii?Q?BXV2FJoTTEHcrHmzMQ3hJJHHL6UcBHACe455sta5hg7cQhF3ORNfAE7nXk6o?= =?us-ascii?Q?/s9dHHBbOQ=3D=3D?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB4025.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 5c02ef9b-ae4b-468e-6a53-08da4da4e567 X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Jun 2022 01:26:24.4925 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: FebxKG8akroy97FF7x+DJkwoVK+l4d8EH9kF8YHpq7sThZPfO+/gwni0ygxAc1fwRYzy/FAGx5g5SacFDTxeIg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR11MB4385 Return-Path: hao.a.wu@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks. For 2 (DevicePathLib PEIM instance), please ensure it is done before mergin= g this series. For 3 (IOMMU codes in storage device PEIMs): Yes, you are right that we still need to consider the EDKII_ATA_AHCI_HOST_C= ONTROLLER_PPI case. As far as I can recall, this PPI was added for the support of OPAL/HDD Pass= word S3 unlock feature. So my take is that the PPI producers are mainly Int= el Client platforms. I think in order to add a library (or directly use services in EDKII_PCI_DE= VICE_PPI) to abstract the IOMMU related logics in AhciPei, both 2 condition= s below should be met: a) Retire the EDKII_ATA_AHCI_HOST_CONTROLLER_PPI This should be no hard, as there are not many producers of this PPI. b) A public reference PciBusPei module is needed in edk2 like PciBusDxe for= DXE case As I mentioned earlier, an enforcement for producers of EDKII_PCI_DEVICE_PP= I to add IOMMU support is required. If there is no reference module (like PciBusDxe for DXE), such enforcement = cannot be guaranteed. Meanwhile, adding a reference PciBusPei implementation might not be easy. As I went through the RFC discussion (https://edk2.groups.io/g/rfc/topic/86= 658203), it seems to me that the PEI phase enumeration requirement varies d= ramatically between platforms. Not sure what is the long-term goal for the current silicon code that produ= ces of EDKII_PCI_DEVICE_PPI, if the target is to eventually put it in edk2 = (not edk2-platform), then I see the feasibility of handling PEI phase IOMMU= in one common place. For 5 (GitHub Pull Request to trigger the CI tests), since 2 is not done ye= t, my take is that the build tests are likely to fail. Need to wait for Dev= icePathLib PEIM instance being merged for this. Best Regards, Hao Wu > -----Original Message----- > From: Czajkowski, Maciej > Sent: Monday, June 13, 2022 9:20 PM > To: Wu, Hao A ; devel@edk2.groups.io > Cc: Ni, Ray > Subject: RE: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use > PCI_DEVICE_PPI to manage AHCI device >=20 > Hello, >=20 > 1. Yes, I will try to fix that in the v2 patch. > 2. We have a review opened to add such instance - > https://edk2.groups.io/g/devel/message/89970 > 3. For now it will be implemented in the silicon code, so you are right -= we > should keep them. Also, it would require a larger library refactor to con= sume > such code from PCI_DEVICE_PPI if we are going to still support both > PCI_DEVICE_PPI and AHCI_HOST_CONTROLLER_PPI. However, what are you > thoughts about future of the library? If we can get rid of > AHCI_HOST_CONTROLLER_PPI, I think that it is possible to remove the > IOMMU code. > 4. It has been run in the simulation environment, and a BlockIo read has = been > performed in PEI phase - and it was performed successfully. > 5. Sure, will do that for v2 patch. >=20 > -----Original Message----- > From: Wu, Hao A > Sent: czwartek, 9 czerwca 2022 05:08 > To: devel@edk2.groups.io; Wu, Hao A ; Czajkowski, > Maciej > Cc: Ni, Ray > Subject: RE: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use > PCI_DEVICE_PPI to manage AHCI device >=20 > For "3) Could you help to check if the DMA memory related codes in > MdeModulePkg\Bus\Ata\AhciPei\DmaMem.c can be covered by the 'PciIo' > service in EDKII_PCI_DEVICE_PPI?" > After a second thought, my take is that there will be no PciBusPei > implementation added in edk2. > So there will be no enforcement for producers of EDKII_PCI_DEVICE_PPI to > add IOMMU support like in PciBusDxe. >=20 > If my above understanding is correct, then I think we might still need to= keep > those IOMMU support codes in AhciPei PEIM. >=20 > Best Regards, > Hao Wu >=20 > > -----Original Message----- > > From: devel@edk2.groups.io On Behalf Of Wu, > Hao > > A > > Sent: Thursday, June 9, 2022 10:48 AM > > To: Czajkowski, Maciej ; > > devel@edk2.groups.io > > Cc: Ni, Ray > > Subject: Re: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use > > PCI_DEVICE_PPI to manage AHCI device > > > > Couple of general level comments/questions: > > 1) The implementation of functions > > AtaAhciPciDevicePpiInstallationCallback() & > > AtaAhciInitPrivateDataFromPciDevice() has many duplications. Is it > > possible to abstract a separate function to reduce duplicated codes? > > 2) What DevicePathLib instance should be used for the PEI case? As far > > as I know, current DevicePathLib instances in edk2 do not support PEIM. > > 3) Could you help to check if the DMA memory related codes in > > MdeModulePkg\Bus\Ata\AhciPei\DmaMem.c can be covered by the 'PciIo' > > service in EDKII_PCI_DEVICE_PPI? > > 4) May I know what kind of tests are performed for this patch? Would > > like to ensure the origin gEdkiiPeiAtaAhciHostControllerPpiGuid path is= not > broken. > > 5) Could you help to create a GitHub Pull Request to trigger the CI > > tests for this series? > > > > More inline comments below: > > > > > > > -----Original Message----- > > > From: Czajkowski, Maciej > > > Sent: Monday, June 6, 2022 8:45 PM > > > To: devel@edk2.groups.io > > > Cc: Wu, Hao A ; Ni, Ray > > > Subject: [edk2-devel][PATCH v1 2/2] MdeModulePkg/AhciPei: Use > > > PCI_DEVICE_PPI to manage AHCI device > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3907 > > > > > > This change modifies AhciPei library to allow usage both > > > EDKII_PCI_DEVICE_PPI and > EDKII_PEI_ATA_AHCI_HOST_CONTROLLER_PPI to > > > manage ATA HDD working under AHCI mode. > > > > > > Cc: Hao A Wu > > > Cc: Ray Ni > > > Signed-off-by: Maciej Czajkowski > > > --- > > > MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c | 615 +++++++++++++++-- > --- > > > MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c | 44 -- > > > MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf | 5 +- > > > 3 files changed, 458 insertions(+), 206 deletions(-) > > > > > > diff --git a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c > > > b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c > > > index 208b7e9a3606..31bb3c0760ab 100644 > > > --- a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c > > > +++ b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.c > > > @@ -9,6 +9,47 @@ > > > **/ > > > > > > > > > > > > #include "AhciPei.h" > > > > > > +#include > > > > > > +#include > > > > > > +#include > > > > > > + > > > > > > +/** > > > > > > + Callback for EDKII_ATA_AHCI_HOST_CONTROLLER_PPI installation. > > > > > > + > > > > > > + @param[in] PeiServices Pointer to PEI Services Table. > > > > > > + @param[in] NotifyDescriptor Pointer to the descriptor for the > Notification > > > > > > + event that caused this function to = execute. > > > > > > + @param[in] Ppi Pointer to the PPI data associated = with this > > function. > > > > > > + > > > > > > + @retval EFI_SUCCESS The function completes successfully > > > > > > + > > > > > > +**/ > > > > > > +EFI_STATUS > > > > > > +EFIAPI > > > > > > +AtaAhciHostControllerPpiInstallationCallback ( > > > > > > + IN EFI_PEI_SERVICES **PeiServices, > > > > > > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, > > > > > > + IN VOID *Ppi > > > > > > + ); > > > > > > + > > > > > > +/** > > > > > > + Callback for EDKII_PCI_DEVICE_PPI installation. > > > > > > + > > > > > > + @param[in] PeiServices Pointer to PEI Services Table. > > > > > > + @param[in] NotifyDescriptor Pointer to the descriptor for the > Notification > > > > > > + event that caused this function to = execute. > > > > > > + @param[in] Ppi Pointer to the PPI data associated = with this > > function. > > > > > > + > > > > > > + @retval EFI_SUCCESS The function completes successfully > > > > > > + > > > > > > +**/ > > > > > > +EFI_STATUS > > > > > > +EFIAPI > > > > > > +AtaAhciPciDevicePpiInstallationCallback ( > > > > > > + IN EFI_PEI_SERVICES **PeiServices, > > > > > > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, > > > > > > + IN VOID *Ppi > > > > > > + ); > > > > > > Could you help to put the above 2 function declarations in AhciPei.h > > to keep consistency? >=20 > Sure, will fix that in v2 patch. >=20 > > > > > > > > > > > > > > > > EFI_PEI_PPI_DESCRIPTOR mAhciAtaPassThruPpiListTemplate =3D { > > > > > > (EFI_PEI_PPI_DESCRIPTOR_PPI | > > > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), > > > > > > @@ -40,6 +81,18 @@ EFI_PEI_NOTIFY_DESCRIPTOR > > > mAhciEndOfPeiNotifyListTemplate =3D { > > > AhciPeimEndOfPei > > > > > > }; > > > > > > > > > > > > +EFI_PEI_NOTIFY_DESCRIPTOR mAtaAhciHostControllerNotify =3D { > > > > > > + (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | > > > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), > > > > > > + &gEdkiiPeiAtaAhciHostControllerPpiGuid, > > > > > > + AtaAhciHostControllerPpiInstallationCallback > > > > > > +}; > > > > > > + > > > > > > +EFI_PEI_NOTIFY_DESCRIPTOR mPciDevicePpiNotify =3D { > > > > > > + (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | > > > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), > > > > > > + &gEdkiiPeiPciDevicePpiGuid, > > > > > > + AtaAhciPciDevicePpiInstallationCallback > > > > > > +}; > > > > > > + > > > > > > /** > > > > > > Free the DMA resources allocated by an ATA AHCI controller. > > > > > > > > > > > > @@ -111,33 +164,28 @@ AhciPeimEndOfPei ( } > > > > > > > > > > > > /** > > > > > > - Entry point of the PEIM. > > > > > > + Initialize and install PrivateData PPIs. > > > > > > > > > > > > - @param[in] FileHandle Handle of the file being invoked. > > > > > > - @param[in] PeiServices Describes the list of possible PEI Servi= ces. > > > > > > - > > > > > > - @retval EFI_SUCCESS PPI successfully installed. > > > > > > + @param[in] Private A pointer to the > > > PEI_AHCI_CONTROLLER_PRIVATE_DATA data > > > > > > + structure. > > > > > > > > > > > > + @retval EFI_SUCCESS AHCI controller initialized and PPIs > > > + installed > > > > > > + @retval others Failed to initialize AHCI controller > > > > > > **/ > > > > > > EFI_STATUS > > > > > > -EFIAPI > > > > > > -AtaAhciPeimEntry ( > > > > > > - IN EFI_PEI_FILE_HANDLE FileHandle, > > > > > > - IN CONST EFI_PEI_SERVICES **PeiServices > > > > > > +AtaAhciInitPrivateData ( > > > > > > + IN UINTN MmioBase, > > > > > > + IN EFI_DEVICE_PATH_PROTOCOL *DevicePath, > > > > > > + IN UINTN DevicePathLength > > > > > > ) > > > > > > { > > > > > > - EFI_STATUS Status; > > > > > > - EFI_BOOT_MODE BootMode; > > > > > > - EDKII_ATA_AHCI_HOST_CONTROLLER_PPI *AhciHcPpi; > > > > > > - UINT8 Controller; > > > > > > - UINTN MmioBase; > > > > > > - UINTN DevicePathLength; > > > > > > - EFI_DEVICE_PATH_PROTOCOL *DevicePath; > > > > > > - UINT32 PortBitMap; > > > > > > - PEI_AHCI_CONTROLLER_PRIVATE_DATA *Private; > > > > > > - UINT8 NumberOfPorts; > > > > > > + EFI_STATUS Status; > > > > > > + UINT32 PortBitMap; > > > > > > + UINT8 NumberOfPorts; > > > > > > + PEI_AHCI_CONTROLLER_PRIVATE_DATA *Private; > > > > > > + EFI_BOOT_MODE BootMode; > > > > > > > > > > > > - DEBUG ((DEBUG_INFO, "%a: Enters.\n", __FUNCTION__)); > > > > > > + DEBUG ((DEBUG_INFO, "Initializing private data for ATA\n")); > > > > > > > > > > > > // > > > > > > // Get the current boot mode. > > > > > > @@ -149,19 +197,149 @@ AtaAhciPeimEntry ( > > > } > > > > > > > > > > > > // > > > > > > - // Locate the ATA AHCI host controller PPI. > > > > > > - // > > > > > > - Status =3D PeiServicesLocatePpi ( > > > > > > - &gEdkiiPeiAtaAhciHostControllerPpiGuid, > > > > > > - 0, > > > > > > - NULL, > > > > > > - (VOID **)&AhciHcPpi > > > > > > - ); > > > > > > + // Check validity of the device path of the ATA AHCI controller. > > > > > > + // > > > > > > + Status =3D AhciIsHcDevicePathValid (DevicePath, DevicePathLength); > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + DEBUG (( > > > > > > + DEBUG_ERROR, > > > > > > + "%a: The device path is invalid.\n", > > > > > > + __FUNCTION__ > > > > > > + )); > > > > > > + return Status; > > > > > > + } > > > > > > + > > > > > > + // > > > > > > + // For S3 resume performance consideration, not all ports on an > > > + ATA AHCI > > > > > > + // controller will be enumerated/initialized. The driver consumes > > > + the > > > > > > + // content within S3StorageDeviceInitList LockBox to get the > > > + ports that > > > > > > + // will be enumerated/initialized during S3 resume. > > > > > > + // > > > > > > + if (BootMode =3D=3D BOOT_ON_S3_RESUME) { > > > > > > + NumberOfPorts =3D AhciS3GetEumeratePorts (DevicePath, > > > + DevicePathLength, > > > &PortBitMap); > > > > > > + if (NumberOfPorts =3D=3D 0) { > > > > > > + return EFI_SUCCESS; > > > > > > + } > > > > > > + } else { > > > > > > + PortBitMap =3D MAX_UINT32; > > > > > > + } > > > > > > + > > > > > > + // > > > > > > + // Memory allocation for controller private data. > > > > > > + // > > > > > > + Private =3D AllocateZeroPool (sizeof > > > + (PEI_AHCI_CONTROLLER_PRIVATE_DATA)); > > > > > > + if (Private =3D=3D NULL) { > > > > > > + DEBUG (( > > > > > > + DEBUG_ERROR, > > > > > > + "%a: Fail to allocate private data.\n", > > > > > > + __FUNCTION__ > > > > > > + )); > > > > > > + return EFI_OUT_OF_RESOURCES; > > > > > > + } > > > > > > + > > > > > > + // > > > > > > + // Initialize controller private data. > > > > > > + // > > > > > > + Private->Signature =3D > > > AHCI_PEI_CONTROLLER_PRIVATE_DATA_SIGNATURE; > > > > > > + Private->MmioBase =3D MmioBase; > > > > > > + Private->DevicePathLength =3D DevicePathLength; > > > > > > + Private->DevicePath =3D DevicePath; > > > > > > + Private->PortBitMap =3D PortBitMap; > > > > > > + InitializeListHead (&Private->DeviceList); > > > > > > + > > > > > > + Status =3D AhciModeInitialization (Private); > > > > > > if (EFI_ERROR (Status)) { > > > > > > - DEBUG ((DEBUG_ERROR, "%a: Failed to locate > > AtaAhciHostControllerPpi.\n", > > > __FUNCTION__)); > > > > > > - return EFI_UNSUPPORTED; > > > > > > + return Status; > > > > > > + } > > > > > > + > > > > > > + Private->AtaPassThruMode.Attributes =3D > > > EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL | > > > > > > + > > > + EFI_ATA_PASS_THRU_ATTRIBUTES_LOGICAL; > > > > > > + Private->AtaPassThruMode.IoAlign =3D sizeof (UINTN); > > > > > > + Private->AtaPassThruPpi.Revision =3D > > > EDKII_PEI_ATA_PASS_THRU_PPI_REVISION; > > > > > > + Private->AtaPassThruPpi.Mode =3D &Private->AtaPassThruMod= e; > > > > > > + Private->AtaPassThruPpi.PassThru =3D AhciAtaPassThruPassThru; > > > > > > + Private->AtaPassThruPpi.GetNextPort =3D > AhciAtaPassThruGetNextPort; > > > > > > + Private->AtaPassThruPpi.GetNextDevice =3D > > > + AhciAtaPassThruGetNextDevice; > > > > > > + Private->AtaPassThruPpi.GetDevicePath =3D > > > + AhciAtaPassThruGetDevicePath; > > > > > > + CopyMem ( > > > > > > + &Private->AtaPassThruPpiList, > > > > > > + &mAhciAtaPassThruPpiListTemplate, > > > > > > + sizeof (EFI_PEI_PPI_DESCRIPTOR) > > > > > > + ); > > > > > > + Private->AtaPassThruPpiList.Ppi =3D &Private->AtaPassThruPpi; > > > > > > + PeiServicesInstallPpi (&Private->AtaPassThruPpiList); > > > > > > + > > > > > > + Private->BlkIoPpi.GetNumberOfBlockDevices =3D > > > + AhciBlockIoGetDeviceNo; > > > > > > + Private->BlkIoPpi.GetBlockDeviceMediaInfo =3D > > > + AhciBlockIoGetMediaInfo; > > > > > > + Private->BlkIoPpi.ReadBlocks =3D AhciBlockIoReadBlock= s; > > > > > > + CopyMem ( > > > > > > + &Private->BlkIoPpiList, > > > > > > + &mAhciBlkIoPpiListTemplate, > > > > > > + sizeof (EFI_PEI_PPI_DESCRIPTOR) > > > > > > + ); > > > > > > + Private->BlkIoPpiList.Ppi =3D &Private->BlkIoPpi; > > > > > > + PeiServicesInstallPpi (&Private->BlkIoPpiList); > > > > > > + > > > > > > + Private->BlkIo2Ppi.Revision =3D > > > EFI_PEI_RECOVERY_BLOCK_IO2_PPI_REVISION; > > > > > > + Private->BlkIo2Ppi.GetNumberOfBlockDevices =3D > > > + AhciBlockIoGetDeviceNo2; > > > > > > + Private->BlkIo2Ppi.GetBlockDeviceMediaInfo =3D > > > + AhciBlockIoGetMediaInfo2; > > > > > > + Private->BlkIo2Ppi.ReadBlocks =3D AhciBlockIoReadBloc= ks2; > > > > > > + CopyMem ( > > > > > > + &Private->BlkIo2PpiList, > > > > > > + &mAhciBlkIo2PpiListTemplate, > > > > > > + sizeof (EFI_PEI_PPI_DESCRIPTOR) > > > > > > + ); > > > > > > + Private->BlkIo2PpiList.Ppi =3D &Private->BlkIo2Ppi; > > > > > > + PeiServicesInstallPpi (&Private->BlkIo2PpiList); > > > > > > + > > > > > > + if (Private->TrustComputingDevices !=3D 0) { > > > > > > + DEBUG (( > > > > > > + DEBUG_INFO, > > > > > > + "%a: Security Security Command PPI will be produced.\n", > > > > > > + __FUNCTION__ > > > > > > + )); > > > > > > + Private->StorageSecurityPpi.Revision =3D > > > EDKII_STORAGE_SECURITY_PPI_REVISION; > > > > > > + Private->StorageSecurityPpi.GetNumberofDevices =3D > > > AhciStorageSecurityGetDeviceNo; > > > > > > + Private->StorageSecurityPpi.GetDevicePath =3D > > > AhciStorageSecurityGetDevicePath; > > > > > > + Private->StorageSecurityPpi.ReceiveData =3D > > > AhciStorageSecurityReceiveData; > > > > > > + Private->StorageSecurityPpi.SendData =3D > > AhciStorageSecuritySendData; > > > > > > + CopyMem ( > > > > > > + &Private->StorageSecurityPpiList, > > > > > > + &mAhciStorageSecurityPpiListTemplate, > > > > > > + sizeof (EFI_PEI_PPI_DESCRIPTOR) > > > > > > + ); > > > > > > + Private->StorageSecurityPpiList.Ppi =3D > > > + &Private->StorageSecurityPpi; > > > > > > + PeiServicesInstallPpi (&Private->StorageSecurityPpiList); > > > > > > } > > > > > > > > > > > > + CopyMem ( > > > > > > + &Private->EndOfPeiNotifyList, > > > > > > + &mAhciEndOfPeiNotifyListTemplate, > > > > > > + sizeof (EFI_PEI_NOTIFY_DESCRIPTOR) > > > > > > + ); > > > > > > + PeiServicesNotifyPpi (&Private->EndOfPeiNotifyList); > > > > > > + > > > > > > + return EFI_SUCCESS; > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + Initialize AHCI controller from > > > + EDKII_ATA_AHCI_HOST_CONTROLLER_PPI > > > instance. > > > > > > + > > > > > > + @param[in] AhciHcPpi Pointer to the AHCI Host Controller PPI inst= ance. > > > > > > + > > > > > > + @retval EFI_SUCCESS PPI successfully installed. > > > > > > +**/ > > > > > > +EFI_STATUS > > > > > > +AtaAhciInitPrivateDataFromHostControllerPpi ( > > > > > > + IN EDKII_ATA_AHCI_HOST_CONTROLLER_PPI *AhciHcPpi > > > > > > + ) > > > > > > +{ > > > > > > + UINT8 Controller; > > > > > > + UINTN MmioBase; > > > > > > + UINTN DevicePathLength; > > > > > > + EFI_DEVICE_PATH_PROTOCOL *DevicePath; > > > > > > + EFI_STATUS Status; > > > > > > + > > > > > > Controller =3D 0; > > > > > > MmioBase =3D 0; > > > > > > while (TRUE) { > > > > > > @@ -193,65 +371,7 @@ AtaAhciPeimEntry ( > > > return Status; > > > > > > } > > > > > > > > > > > > - // > > > > > > - // Check validity of the device path of the ATA AHCI controller. > > > > > > - // > > > > > > - Status =3D AhciIsHcDevicePathValid (DevicePath, DevicePathLength= ); > > > > > > - if (EFI_ERROR (Status)) { > > > > > > - DEBUG (( > > > > > > - DEBUG_ERROR, > > > > > > - "%a: The device path is invalid for Controller %d.\n", > > > > > > - __FUNCTION__, > > > > > > - Controller > > > > > > - )); > > > > > > - Controller++; > > > > > > - continue; > > > > > > - } > > > > > > - > > > > > > - // > > > > > > - // For S3 resume performance consideration, not all ports on an = ATA > AHCI > > > > > > - // controller will be enumerated/initialized. The driver consume= s the > > > > > > - // content within S3StorageDeviceInitList LockBox to get the por= ts that > > > > > > - // will be enumerated/initialized during S3 resume. > > > > > > - // > > > > > > - if (BootMode =3D=3D BOOT_ON_S3_RESUME) { > > > > > > - NumberOfPorts =3D AhciS3GetEumeratePorts (DevicePath, > > DevicePathLength, > > > &PortBitMap); > > > > > > - if (NumberOfPorts =3D=3D 0) { > > > > > > - // > > > > > > - // No ports need to be enumerated for this controller. > > > > > > - // > > > > > > - Controller++; > > > > > > - continue; > > > > > > - } > > > > > > - } else { > > > > > > - PortBitMap =3D MAX_UINT32; > > > > > > - } > > > > > > - > > > > > > - // > > > > > > - // Memory allocation for controller private data. > > > > > > - // > > > > > > - Private =3D AllocateZeroPool (sizeof > > (PEI_AHCI_CONTROLLER_PRIVATE_DATA)); > > > > > > - if (Private =3D=3D NULL) { > > > > > > - DEBUG (( > > > > > > - DEBUG_ERROR, > > > > > > - "%a: Fail to allocate private data for Controller %d.\n", > > > > > > - __FUNCTION__, > > > > > > - Controller > > > > > > - )); > > > > > > - return EFI_OUT_OF_RESOURCES; > > > > > > - } > > > > > > - > > > > > > - // > > > > > > - // Initialize controller private data. > > > > > > - // > > > > > > - Private->Signature =3D > > > AHCI_PEI_CONTROLLER_PRIVATE_DATA_SIGNATURE; > > > > > > - Private->MmioBase =3D MmioBase; > > > > > > - Private->DevicePathLength =3D DevicePathLength; > > > > > > - Private->DevicePath =3D DevicePath; > > > > > > - Private->PortBitMap =3D PortBitMap; > > > > > > - InitializeListHead (&Private->DeviceList); > > > > > > - > > > > > > - Status =3D AhciModeInitialization (Private); > > > > > > + Status =3D AtaAhciInitPrivateData (MmioBase, DevicePath, > > > + DevicePathLength); > > > > > > if (EFI_ERROR (Status)) { > > > > > > DEBUG (( > > > > > > DEBUG_ERROR, > > > > > > @@ -260,86 +380,261 @@ AtaAhciPeimEntry ( > > > Controller, > > > > > > Status > > > > > > )); > > > > > > - Controller++; > > > > > > - continue; > > > > > > - } > > > > > > - > > > > > > - Private->AtaPassThruMode.Attributes =3D > > > EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL | > > > > > > - EFI_ATA_PASS_THRU_ATTRIBUT= ES_LOGICAL; > > > > > > - Private->AtaPassThruMode.IoAlign =3D sizeof (UINTN); > > > > > > - Private->AtaPassThruPpi.Revision =3D > > > EDKII_PEI_ATA_PASS_THRU_PPI_REVISION; > > > > > > - Private->AtaPassThruPpi.Mode =3D &Private->AtaPassThruM= ode; > > > > > > - Private->AtaPassThruPpi.PassThru =3D AhciAtaPassThruPassThr= u; > > > > > > - Private->AtaPassThruPpi.GetNextPort =3D > AhciAtaPassThruGetNextPort; > > > > > > - Private->AtaPassThruPpi.GetNextDevice =3D > AhciAtaPassThruGetNextDevice; > > > > > > - Private->AtaPassThruPpi.GetDevicePath =3D > AhciAtaPassThruGetDevicePath; > > > > > > - CopyMem ( > > > > > > - &Private->AtaPassThruPpiList, > > > > > > - &mAhciAtaPassThruPpiListTemplate, > > > > > > - sizeof (EFI_PEI_PPI_DESCRIPTOR) > > > > > > - ); > > > > > > - Private->AtaPassThruPpiList.Ppi =3D &Private->AtaPassThruPpi; > > > > > > - PeiServicesInstallPpi (&Private->AtaPassThruPpiList); > > > > > > - > > > > > > - Private->BlkIoPpi.GetNumberOfBlockDevices =3D > AhciBlockIoGetDeviceNo; > > > > > > - Private->BlkIoPpi.GetBlockDeviceMediaInfo =3D > AhciBlockIoGetMediaInfo; > > > > > > - Private->BlkIoPpi.ReadBlocks =3D AhciBlockIoReadBlo= cks; > > > > > > - CopyMem ( > > > > > > - &Private->BlkIoPpiList, > > > > > > - &mAhciBlkIoPpiListTemplate, > > > > > > - sizeof (EFI_PEI_PPI_DESCRIPTOR) > > > > > > - ); > > > > > > - Private->BlkIoPpiList.Ppi =3D &Private->BlkIoPpi; > > > > > > - PeiServicesInstallPpi (&Private->BlkIoPpiList); > > > > > > - > > > > > > - Private->BlkIo2Ppi.Revision =3D > > > EFI_PEI_RECOVERY_BLOCK_IO2_PPI_REVISION; > > > > > > - Private->BlkIo2Ppi.GetNumberOfBlockDevices =3D > AhciBlockIoGetDeviceNo2; > > > > > > - Private->BlkIo2Ppi.GetBlockDeviceMediaInfo =3D > AhciBlockIoGetMediaInfo2; > > > > > > - Private->BlkIo2Ppi.ReadBlocks =3D AhciBlockIoReadBl= ocks2; > > > > > > - CopyMem ( > > > > > > - &Private->BlkIo2PpiList, > > > > > > - &mAhciBlkIo2PpiListTemplate, > > > > > > - sizeof (EFI_PEI_PPI_DESCRIPTOR) > > > > > > - ); > > > > > > - Private->BlkIo2PpiList.Ppi =3D &Private->BlkIo2Ppi; > > > > > > - PeiServicesInstallPpi (&Private->BlkIo2PpiList); > > > > > > - > > > > > > - if (Private->TrustComputingDevices !=3D 0) { > > > > > > + } else { > > > > > > DEBUG (( > > > > > > DEBUG_INFO, > > > > > > - "%a: Security Security Command PPI will be produced for > > Controller %d.\n", > > > > > > + "%a: Controller %d has been successfully initialized.\n", > > > > > > __FUNCTION__, > > > > > > Controller > > > > > > )); > > > > > > - Private->StorageSecurityPpi.Revision =3D > > > EDKII_STORAGE_SECURITY_PPI_REVISION; > > > > > > - Private->StorageSecurityPpi.GetNumberofDevices =3D > > > AhciStorageSecurityGetDeviceNo; > > > > > > - Private->StorageSecurityPpi.GetDevicePath =3D > > > AhciStorageSecurityGetDevicePath; > > > > > > - Private->StorageSecurityPpi.ReceiveData =3D > > > AhciStorageSecurityReceiveData; > > > > > > - Private->StorageSecurityPpi.SendData =3D > > AhciStorageSecuritySendData; > > > > > > - CopyMem ( > > > > > > - &Private->StorageSecurityPpiList, > > > > > > - &mAhciStorageSecurityPpiListTemplate, > > > > > > - sizeof (EFI_PEI_PPI_DESCRIPTOR) > > > > > > - ); > > > > > > - Private->StorageSecurityPpiList.Ppi =3D &Private->StorageSecur= ityPpi; > > > > > > - PeiServicesInstallPpi (&Private->StorageSecurityPpiList); > > > > > > } > > > > > > > > > > > > - CopyMem ( > > > > > > - &Private->EndOfPeiNotifyList, > > > > > > - &mAhciEndOfPeiNotifyListTemplate, > > > > > > - sizeof (EFI_PEI_NOTIFY_DESCRIPTOR) > > > > > > - ); > > > > > > - PeiServicesNotifyPpi (&Private->EndOfPeiNotifyList); > > > > > > - > > > > > > - DEBUG (( > > > > > > - DEBUG_INFO, > > > > > > - "%a: Controller %d has been successfully initialized.\n", > > > > > > - __FUNCTION__, > > > > > > - Controller > > > > > > - )); > > > > > > Controller++; > > > > > > } > > > > > > > > > > > > return EFI_SUCCESS; > > > > > > } > > > > > > + > > > > > > +/** > > > > > > + Callback for EDKII_ATA_AHCI_HOST_CONTROLLER_PPI installation. > > > > > > + > > > > > > + @param[in] PeiServices Pointer to PEI Services Table. > > > > > > + @param[in] NotifyDescriptor Pointer to the descriptor for the > Notification > > > > > > + event that caused this function to = execute. > > > > > > + @param[in] Ppi Pointer to the PPI data associated = with this > > function. > > > > > > + > > > > > > + @retval EFI_SUCCESS The function completes successfully > > > > > > Please help to add the descriptions for function returning error status= . >=20 > Sure, will fix that in v2 patch. >=20 > > > > > > > > > > + > > > > > > +**/ > > > > > > +EFI_STATUS > > > > > > +EFIAPI > > > > > > +AtaAhciHostControllerPpiInstallationCallback ( > > > > > > + IN EFI_PEI_SERVICES **PeiServices, > > > > > > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, > > > > > > + IN VOID *Ppi > > > > > > + ) > > > > > > +{ > > > > > > + EDKII_ATA_AHCI_HOST_CONTROLLER_PPI *AhciHcPpi; > > > > > > + > > > > > > + if (Ppi =3D=3D NULL) { > > > > > > + return EFI_INVALID_PARAMETER; > > > > > > + } > > > > > > + > > > > > > + AhciHcPpi =3D (EDKII_ATA_AHCI_HOST_CONTROLLER_PPI*) Ppi; > > > > > > + > > > > > > + return AtaAhciInitPrivateDataFromHostControllerPpi (AhciHcPpi); > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + Callback for EDKII_PCI_DEVICE_PPI installation. > > > > > > + > > > > > > + @param[in] PeiServices Pointer to PEI Services Table. > > > > > > + @param[in] NotifyDescriptor Pointer to the descriptor for the > Notification > > > > > > + event that caused this function to = execute. > > > > > > + @param[in] Ppi Pointer to the PPI data associated = with this > > function. > > > > > > + > > > > > > + @retval EFI_SUCCESS The function completes successfully > > > > > > Please help to add the descriptions for function returning error status= . >=20 > Sure, will fix that in v2 patch. >=20 > > > > > > > > > > + > > > > > > +**/ > > > > > > +EFI_STATUS > > > > > > +EFIAPI > > > > > > +AtaAhciPciDevicePpiInstallationCallback ( > > > > > > + IN EFI_PEI_SERVICES **PeiServices, > > > > > > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, > > > > > > + IN VOID *Ppi > > > > > > + ) > > > > > > +{ > > > > > > + EDKII_PCI_DEVICE_PPI *PciDevice; > > > > > > + PCI_TYPE00 PciData; > > > > > > + UINTN MmioBase; > > > > > > + EFI_DEVICE_PATH_PROTOCOL *DevicePath; > > > > > > + UINTN DevicePathLength; > > > > > > + EFI_STATUS Status; > > > > > > + > > > > > > + PciDevice =3D (EDKII_PCI_DEVICE_PPI*) Ppi; > > > > > > + > > > > > > + // > > > > > > + // Now further check the PCI header: Base Class (offset 0x0B) and > > > > > > + // Sub Class (offset 0x0A). This controller should be an SATA > > > + controller > > > > > > + // > > > > > > + Status =3D PciDevice->PciIo.Pci.Read ( > > > > > > + &PciDevice->PciIo, > > > > > > + EfiPciIoWidthUint8, > > > > > > + PCI_CLASSCODE_OFFSET, > > > > > > + sizeof (PciData.Hdr.ClassCode), > > > > > > + PciData.Hdr.ClassCode > > > > > > + ); > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + return EFI_UNSUPPORTED; > > > > > > + } > > > > > > + > > > > > > + if (!IS_PCI_IDE (&PciData) && !IS_PCI_SATADPA (&PciData)) { > > > > > > + return EFI_UNSUPPORTED; > > > > > > + } > > > > > > + > > > > > > + Status =3D PciDevice->PciIo.Attributes ( > > > > > > + &PciDevice->PciIo, > > > > > > + EfiPciIoAttributeOperationSet, > > > > > > + EFI_PCI_DEVICE_ENABLE, > > > > > > + NULL > > > > > > + ); > > > > > > Do you think we need to align with AtaAtapiPassThru DXE counterpart > > when enabling the controller? > > MdeModulePkg\Bus\Ata\AtaAtapiPassThru\AtaAtapiPassThru.c - > > AtaAtapiPassThruStart(): > > Status =3D PciIo->Attributes ( > > PciIo, > > EfiPciIoAttributeOperationSupported, > > 0, > > &EnabledPciAttributes > > ); > > if (!EFI_ERROR (Status)) { > > EnabledPciAttributes &=3D (UINT64)EFI_PCI_DEVICE_ENABLE; > > Status =3D PciIo->Attributes ( > > PciIo, > > EfiPciIoAttributeOperationEnable, > > EnabledPciAttributes, > > NULL > > ); > > } >=20 > Yes, I think it is a good idea. Will be changed in v2 patch. > > > > > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + return EFI_UNSUPPORTED; > > > > > > + } > > > > > > + > > > > > > + Status =3D PciDevice->PciIo.Pci.Read ( > > > > > > + &PciDevice->PciIo, > > > > > > + EfiPciIoWidthUint32, > > > > > > + 0x24, > > > > > > + sizeof (UINTN), > > > > > > + &MmioBase > > > > > > + ); > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + return EFI_UNSUPPORTED; > > > > > > + } > > > > > > + > > > > > > + DevicePathLength =3D GetDevicePathSize (PciDevice->DevicePath); > > > > > > + DevicePath =3D PciDevice->DevicePath; > > > > > > + > > > > > > + Status =3D AtaAhciInitPrivateData (MmioBase, DevicePath, > > > + DevicePathLength); > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + DEBUG (( > > > > > > + DEBUG_INFO, > > > > > > + "%a: Failed to init controller, with Status - %r\n", > > > > > > + __FUNCTION__, > > > > > > + Status > > > > > > + )); > > > > > > + } > > > > > > + > > > > > > + return EFI_SUCCESS; > > > > > > +} > > > > > > + > > > > > > > > > Missing function description comments for > > AtaAhciInitPrivateDataFromPciDevice. >=20 > I will fix that in v2 patch. >=20 > > > > > > > +EFI_STATUS > > > > > > +AtaAhciInitPrivateDataFromPciDevice ( > > > > > > + VOID > > > > > > + ) > > > > > > +{ > > > > > > + EFI_STATUS Status; > > > > > > + UINTN ControllerIndex; > > > > > > + EDKII_PCI_DEVICE_PPI *PciDevice; > > > > > > + PCI_TYPE00 PciData; > > > > > > + UINTN MmioBase; > > > > > > + EFI_DEVICE_PATH_PROTOCOL *DevicePath; > > > > > > + UINTN DevicePathLength; > > > > > > + > > > > > > + Status =3D EFI_SUCCESS; > > > > > > + for (ControllerIndex =3D 0; Status !=3D EFI_NOT_FOUND; > > > + ControllerIndex++ ) { > > > > > > + Status =3D PeiServicesLocatePpi ( > > > > > > + &gEdkiiPeiPciDevicePpiGuid, > > > > > > + ControllerIndex, > > > > > > + NULL, > > > > > > + (VOID**) &PciDevice > > > > > > + ); > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + continue; > > > > > > + } > > > > > > + > > > > > > + // > > > > > > + // Now further check the PCI header: Base Class (offset 0x0B) > > > + and > > > > > > + // Sub Class (offset 0x0A). This controller should be an SATA > > > + controller > > > > > > + // > > > > > > + Status =3D PciDevice->PciIo.Pci.Read ( > > > > > > + &PciDevice->PciIo, > > > > > > + EfiPciIoWidthUint8, > > > > > > + PCI_CLASSCODE_OFFSET, > > > > > > + sizeof (PciData.Hdr.ClassCode), > > > > > > + PciData.Hdr.ClassCode > > > > > > + ); > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + continue; > > > > > > + } > > > > > > + > > > > > > + if (!IS_PCI_IDE (&PciData) && !IS_PCI_SATADPA (&PciData)) { > > > > > > + continue; > > > > > > + } > > > > > > + > > > > > > + Status =3D PciDevice->PciIo.Attributes ( > > > > > > + &PciDevice->PciIo, > > > > > > + EfiPciIoAttributeOperationSet, > > > > > > + EFI_PCI_DEVICE_ENABLE, > > > > > > + NULL > > > > > > + ); > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + continue; > > > > > > + } > > > > > > + > > > > > > + Status =3D PciDevice->PciIo.Pci.Read ( > > > > > > + &PciDevice->PciIo, > > > > > > + EfiPciIoWidthUint32, > > > > > > + 0x24, > > > > > > + sizeof (UINTN), > > > > > > + &MmioBase > > > > > > + ); > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + continue; > > > > > > + } > > > > > > + > > > > > > + DevicePathLength =3D GetDevicePathSize (PciDevice->DevicePath); > > > > > > + DevicePath =3D PciDevice->DevicePath; > > > > > > + > > > > > > + Status =3D AtaAhciInitPrivateData (MmioBase, DevicePath, > > > + DevicePathLength); > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + DEBUG (( > > > > > > + DEBUG_INFO, > > > > > > + "%a: Failed to init controller, with Status - %r\n", > > > > > > + __FUNCTION__, > > > > > > + Status > > > > > > + )); > > > > > > + } > > > > > > + // > > > > > > + // Override the status to continue the for loop > > > > > > + // > > > > > > + Status =3D EFI_SUCCESS; > > > > > > + } > > > > > > + > > > > > > + return EFI_SUCCESS; > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + Entry point of the PEIM. > > > > > > + > > > > > > + @param[in] FileHandle Handle of the file being invoked. > > > > > > + @param[in] PeiServices Describes the list of possible PEI Servi= ces. > > > > > > + > > > > > > + @retval EFI_SUCCESS PPI successfully installed. > > > > > > + > > > > > > +**/ > > > > > > +EFI_STATUS > > > > > > +EFIAPI > > > > > > +AtaAhciPeimEntry ( > > > > > > + IN EFI_PEI_FILE_HANDLE FileHandle, > > > > > > + IN CONST EFI_PEI_SERVICES **PeiServices > > > > > > + ) > > > > > > +{ > > > > > > + EFI_STATUS Status; > > > > > > + EDKII_ATA_AHCI_HOST_CONTROLLER_PPI *AhciHcPpi; > > > > > > + > > > > > > + DEBUG ((DEBUG_INFO, "%a: Enters.\n", __FUNCTION__)); > > > > > > + > > > > > > + PeiServicesNotifyPpi (&mAtaAhciHostControllerNotify); > > > > > > + > > > > > > + PeiServicesNotifyPpi (&mPciDevicePpiNotify); > > > > > > + > > > > > > + Status =3D AtaAhciInitPrivateDataFromPciDevice (); > > > > > > + > > > > > > + // > > > > > > + // Locate the ATA AHCI host controller PPI. > > > > > > + // > > > > > > + Status =3D PeiServicesLocatePpi ( > > > > > > + &gEdkiiPeiAtaAhciHostControllerPpiGuid, > > > > > > + 0, > > > > > > + NULL, > > > > > > + (VOID **)&AhciHcPpi > > > > > > + ); > > > > > > + if (!EFI_ERROR (Status)) { > > > > > > + DEBUG ((DEBUG_ERROR, "%a: Using AtaAhciHostControllerPpi to > > > + initialize > > > private data.\n", __FUNCTION__)); > > > > > > Suggest to use DEBUG_INFO here. >=20 > Ok, will fix that in v2 patch. >=20 > > > > Best Regards, > > Hao Wu > > > > > > > > > > + Status =3D AtaAhciInitPrivateDataFromHostControllerPpi > > > + (AhciHcPpi); > > > > > > + } > > > > > > + > > > > > > + return Status; > > > > > > +} > > > > > > diff --git a/MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c > > > b/MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c > > > index 81f8743d40d8..cf0955929dde 100644 > > > --- a/MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c > > > +++ b/MdeModulePkg/Bus/Ata/AhciPei/DevicePath.c > > > @@ -38,50 +38,6 @@ EFI_DEVICE_PATH_PROTOCOL > > > mAhciEndDevicePathNodeTemplate =3D { > > > } > > > > > > }; > > > > > > > > > > > > -/** > > > > > > - Returns the 16-bit Length field of a device path node. > > > > > > - > > > > > > - Returns the 16-bit Length field of the device path node specified = by > Node. > > > > > > - Node is not required to be aligned on a 16-bit boundary, so it is > > > recommended > > > > > > - that a function such as ReadUnaligned16() be used to extract the > > > contents of > > > > > > - the Length field. > > > > > > - > > > > > > - If Node is NULL, then ASSERT(). > > > > > > - > > > > > > - @param Node A pointer to a device path node data structure. > > > > > > - > > > > > > - @return The 16-bit Length field of the device path node specified = by > Node. > > > > > > - > > > > > > -**/ > > > > > > -UINTN > > > > > > -DevicePathNodeLength ( > > > > > > - IN CONST VOID *Node > > > > > > - ) > > > > > > -{ > > > > > > - ASSERT (Node !=3D NULL); > > > > > > - return ReadUnaligned16 ((UINT16 *)&((EFI_DEVICE_PATH_PROTOCOL > > > *)(Node))->Length[0]); > > > > > > -} > > > > > > - > > > > > > -/** > > > > > > - Returns a pointer to the next node in a device path. > > > > > > - > > > > > > - If Node is NULL, then ASSERT(). > > > > > > - > > > > > > - @param Node A pointer to a device path node data structure. > > > > > > - > > > > > > - @return a pointer to the device path node that follows the device > > > path node > > > > > > - specified by Node. > > > > > > - > > > > > > -**/ > > > > > > -EFI_DEVICE_PATH_PROTOCOL * > > > > > > -NextDevicePathNode ( > > > > > > - IN CONST VOID *Node > > > > > > - ) > > > > > > -{ > > > > > > - ASSERT (Node !=3D NULL); > > > > > > - return (EFI_DEVICE_PATH_PROTOCOL *)((UINT8 *)(Node) + > > > DevicePathNodeLength (Node)); > > > > > > -} > > > > > > - > > > > > > /** > > > > > > Get the size of the current device path instance. > > > > > > > > > > > > diff --git a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf > > > b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf > > > index 912ff7a8ba4f..788660b33299 100644 > > > --- a/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf > > > +++ b/MdeModulePkg/Bus/Ata/AhciPei/AhciPei.inf > > > @@ -50,11 +50,13 @@ [LibraryClasses] > > > TimerLib > > > > > > LockBoxLib > > > > > > PeimEntryPoint > > > > > > + DevicePathLib > > > > > > > > > > > > [Ppis] > > > > > > gEdkiiPeiAtaAhciHostControllerPpiGuid ## CONSUMES > > > > > > gEdkiiIoMmuPpiGuid ## CONSUMES > > > > > > gEfiEndOfPeiSignalPpiGuid ## CONSUMES > > > > > > + gEdkiiPeiPciDevicePpiGuid ## CONSUMES > > > > > > gEdkiiPeiAtaPassThruPpiGuid ## SOMETIMES_PRODUC= ES > > > > > > gEfiPeiVirtualBlockIoPpiGuid ## SOMETIMES_PRODUC= ES > > > > > > gEfiPeiVirtualBlockIo2PpiGuid ## SOMETIMES_PRODUC= ES > > > > > > @@ -65,8 +67,7 @@ [Guids] > > > > > > > > > [Depex] > > > > > > gEfiPeiMemoryDiscoveredPpiGuid AND > > > > > > - gEfiPeiMasterBootModePpiGuid AND > > > > > > - gEdkiiPeiAtaAhciHostControllerPpiGuid > > > > > > + gEfiPeiMasterBootModePpiGuid > > > > > > > > > > > > [UserExtensions.TianoCore."ExtraFiles"] > > > > > > AhciPeiExtra.uni > > > > > > -- > > > 2.27.0.windows.1 > > > > > > > >=20 > >