From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from aserp2120.oracle.com (aserp2120.oracle.com [141.146.126.78]) by mx.groups.io with SMTP id smtpd.web10.630.1610738527612187822 for ; Fri, 15 Jan 2021 11:22:07 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=NozDXwH2; spf=pass (domain: oracle.com, ip: 141.146.126.78, mailfrom: ankur.a.arora@oracle.com) Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 10FJE4lu005624; Fri, 15 Jan 2021 19:22:06 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2020-01-29; bh=kffFiZCVmm++tmyzV/gmUM7jyWlHebputQPJ4+1YVLg=; b=NozDXwH2SH5PVRoZy0Ld6Y5srw44wOjGYdQIA2Cw7YBrrjuuUzg9GHApGw74OiHSK8qI Oj/V+U0VlRRmK7WVzNLuvxf5fwgD+RnioYAthQoCn1Hl8cJOsOSDJHAdQJrOG877eTYX lWeloXwTon1Sn2gHj6itOWVg74bU3JlmnmsEhpwx6B5LdHTn46sW/ydZjxOpagwDxFwq H+9RKjG8p9yt+MmA2V15yAPOMSJo8Oq8pi0lho942xqT472OtND/xofXaPkrqrgRWJHX PvEZ5i2TXSrMgS5mqoKyhIpN+ErvbgUNb758+Qxf1Nzp/ZzU6Y6WcVHrDluh+G3Vongw mg== Received: from aserp3030.oracle.com (aserp3030.oracle.com [141.146.126.71]) by aserp2120.oracle.com with ESMTP id 360kd06dsw-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 15 Jan 2021 19:22:06 +0000 Received: from pps.filterd (aserp3030.oracle.com [127.0.0.1]) by aserp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 10FJKdSQ107889; Fri, 15 Jan 2021 19:22:06 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserp3030.oracle.com with ESMTP id 360kf3qewh-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 15 Jan 2021 19:22:06 +0000 Received: from abhmp0014.oracle.com (abhmp0014.oracle.com [141.146.116.20]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 10FJM4YB011978; Fri, 15 Jan 2021 19:22:04 GMT Received: from [192.168.0.108] (/70.36.60.91) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Fri, 15 Jan 2021 11:22:04 -0800 Subject: Re: [edk2-devel] [PATCH v3 04/10] UefiCpuPkg: add CPU ejection support To: Laszlo Ersek , devel@edk2.groups.io Cc: imammedo@redhat.com, Eric Dong , Ray Ni , Rahul Kumar , Boris Ostrovsky , Aaron Young References: <20210115074533.277448-1-ankur.a.arora@oracle.com> <20210115074533.277448-5-ankur.a.arora@oracle.com> <6d95cb8b-1ae9-91c8-4cae-f34ee2bade37@redhat.com> <3630f8e2-d9b0-b933-1226-682fe5ccaa86@oracle.com> <63e6cc00-ae8c-3f1e-226a-c9c03524da85@redhat.com> From: "Ankur Arora" Message-ID: <49cbf1b9-4d5e-ddeb-fd8a-271178093cdd@oracle.com> Date: Fri, 15 Jan 2021 11:22:02 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 MIME-Version: 1.0 In-Reply-To: <63e6cc00-ae8c-3f1e-226a-c9c03524da85@redhat.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9865 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 malwarescore=0 suspectscore=0 adultscore=0 spamscore=0 mlxlogscore=999 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101150117 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9865 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 impostorscore=0 bulkscore=0 adultscore=0 suspectscore=0 malwarescore=0 lowpriorityscore=0 clxscore=1015 mlxlogscore=999 mlxscore=0 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101150116 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-01-15 10:44 a.m., Laszlo Ersek wrote: > On 01/15/21 19:16, Ankur Arora wrote: >> On 2021-01-15 12:17 a.m., Laszlo Ersek wrote: >>> Hi Ankur, >>> >>> On 01/15/21 08:45, Ankur Arora wrote: >>>> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress, >>>> which would be used to share CPU ejection state between >>>> PiSmmCpuDxeSmm and OvmfPkg/CpuHotPlugSmm. >>>> [...] >>> >>> Second, if that's not possible -- please explain why --, then a function >>> pointer might be justified after all, but the information channel still >>> seems to have zero relevance for UefiCpuPkg. >> >> The reason for keeping the PCD in UefiCpuPkg was that its declaration and >> init are in SmmCpuFeaturesLib which gets folded into the >> UefiCpuPkg/PiSmmCpuDxe >> execution context and so the export happening via OvmfPkg.dec seemed off. > > No, it's not off, that's precisely the goal. SmmCpuFeaturesLib is a > dedicated platform customization interface for PiSmmCpuDxeSmm. (By > platform, I mean "firmware platform"; such as OvmfPkg.) > SmmCpuFeaturesLib exists becuase PiSmmCpuDxeSmm wants it to exist. > > If we can solve a task by hiding it entirely in SmmCpuFeaturesLib, in > connection with other parts of the firmware platform, we should do that. > That's why the SmmCpuFeaturesLib class was invented, with carefully > selected hook points. UefiCpuPkg in general is a core package, not a > firmware platform package, so we only modify UefiCpuPkg for platform > needs if that is absolutely unavoidable. > > We are modifying the SmmCpuFeaturesLib instance provided by OvmfPkg, so > we should strive for keeping the internals of that solution internal to > OvmfPkg -- such as a PCD declared in OvmfPkg.dec, a structure type > defined in an OvmfPkg include file, and so on. We're welcome to stuff as > much platform logic into PiSmmCpuDxeSmm through our platform's > SmmCpuFeaturesLib instance as possible, so long as we have an actual > justified purpose with that "stuffing", and we honor the interface > contracts. > >> That said, I guess your underlying point is that this adds unnecessary >> state to non OVMF builds (?), which it does, so I can move the PCD to OvmfPkg.dec. > > Yes, that's my point. Ideally, the diffstat of the series should > entirely stay within OvmfPkg. > > I would suggest even splitting off the last patch (for CpuDeadLoop()) > into its own submission. That patch could be merged sooner than, and > independently of, the unplug feature for OVMF. > > Is it OK with you if I ask you to submit a v4 like that, before I start > going through the series in detail? Sure. Let me send a v4 with these changes. Ankur > > A bit more feedback on folding this UefiCpuPkg content into OvmfPkg: > > - "OvmfPkg/Include/CpuHotUnplug.h" looks good to me, for the header file > (feel free to replace Unplug with Eject though, if you like the latter more) > > - in INF files, in every section, such as [Sources], [Pcds] etc, please > keep entries (filenames, PCD names) alphabetically sorted -- unless the > preexistent order already breaks this property > > - don't bother about a .uni file under OvmfPkg > > - in "OvmfPkg.dec", please find the PCD with the highest token value, > and for introducing the new PCD, use a new token value that's one > greater than the current maximum. > > Thank you! > Laszlo >