From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from userp2130.oracle.com (userp2130.oracle.com [156.151.31.86]) by mx.groups.io with SMTP id smtpd.web11.289.1610734607089438022 for ; Fri, 15 Jan 2021 10:16:47 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=pv1lQlD8; spf=pass (domain: oracle.com, ip: 156.151.31.86, mailfrom: ankur.a.arora@oracle.com) Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 10FIFIh8184978; Fri, 15 Jan 2021 18:16:45 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=R8aUzpd10XXoq6qMl5y7E8T/nhFiWUs1c/5d35oknyI=; b=pv1lQlD8h8peLxhKS+UIb46pWcP2xVojvyObwSRsA5Hm7Gi2rGBpbPsAtSbdVvKGrhmL GOwtU9rEA4EfvhM8fpjVuo/eThnJtR22MMJkxd62x1ywrlmaB/w1u2wjQBWM/8nv9wNS hbfILal8pJHPn9CmxrgZ/OwapOavdn1I53abK1mqIZ407QHfeLOzfxrs2KuoDxi0HwbI LtmM0iUIMiRUqaSO/OJKjskPLzs5vXfwimftQbmEQLD7aLf/NR9F0GYIwpJQF+w+OuOQ pCrRIesSKcUv5xvWB/VVjLfg3ACZ6eDIdM7AtHRWdGxUqpTfSJ/xhMxt2DmDje2H18VZ Iw== Received: from aserp3030.oracle.com (aserp3030.oracle.com [141.146.126.71]) by userp2130.oracle.com with ESMTP id 360kvke40x-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 15 Jan 2021 18:16:45 +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 10FIGbl2135493; Fri, 15 Jan 2021 18:16:44 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by aserp3030.oracle.com with ESMTP id 360kf3nmsm-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 15 Jan 2021 18:16:44 +0000 Received: from abhmp0006.oracle.com (abhmp0006.oracle.com [141.146.116.12]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 10FIGM0h001834; Fri, 15 Jan 2021 18:16:22 GMT Received: from [192.168.0.108] (/70.36.60.91) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Fri, 15 Jan 2021 10:16:22 -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> From: "Ankur Arora" Message-ID: <3630f8e2-d9b0-b933-1226-682fe5ccaa86@oracle.com> Date: Fri, 15 Jan 2021 10:16:21 -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: <6d95cb8b-1ae9-91c8-4cae-f34ee2bade37@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-2101150110 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9865 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 phishscore=0 lowpriorityscore=0 bulkscore=0 priorityscore=1501 malwarescore=0 clxscore=1015 impostorscore=0 spamscore=0 mlxscore=0 suspectscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101150110 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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. >> >> Cc: Eric Dong >> Cc: Ray Ni >> Cc: Laszlo Ersek >> Cc: Rahul Kumar >> Cc: Boris Ostrovsky >> Cc: Aaron Young >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132 >> Signed-off-by: Ankur Arora >> --- >> UefiCpuPkg/Include/CpuHotPlugData.h | 21 +++++++++++++++++++++ >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 + >> UefiCpuPkg/UefiCpuPkg.dec | 5 +++++ >> UefiCpuPkg/UefiCpuPkg.uni | 4 ++++ >> 4 files changed, 31 insertions(+) >> >> diff --git a/UefiCpuPkg/Include/CpuHotPlugData.h b/UefiCpuPkg/Include/CpuHotPlugData.h >> index 6321a149fa52..86f964550655 100644 >> --- a/UefiCpuPkg/Include/CpuHotPlugData.h >> +++ b/UefiCpuPkg/Include/CpuHotPlugData.h >> @@ -2,6 +2,7 @@ >> Definition for a structure sharing information for CPU hot plug. >> >> Copyright (c) 2013 - 2015, Intel Corporation. All rights reserved.
>> +Copyright (c) 2021, Oracle Corporation.
>> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> **/ >> @@ -24,4 +25,24 @@ typedef struct { >> UINT32 SmrrSize; >> } CPU_HOT_PLUG_DATA; >> >> +typedef >> +VOID >> +(EFIAPI *CPU_HOT_EJECT_FN)( >> + IN UINTN ProcessorNum >> + ); >> + >> +#define CPU_EJECT_INVALID (MAX_UINT64) >> +#define CPU_EJECT_WORKER (MAX_UINT64-1) >> + >> +#define CPU_HOT_EJECT_DATA_REVISION_1 0x00000001 >> + >> +typedef struct { >> + UINT32 Revision; // Used for version identification for this structure >> + UINT32 ArrayLength; // The entries number of the following ApicId array and SmBase array >> + >> + UINT64 *ApicIdMap; // Pointer to CpuIndex->ApicId map. Holds APIC IDs for pending ejects >> + CPU_HOT_EJECT_FN Handler; // Handler for CPU ejection >> + UINT64 Reserved; >> +} CPU_HOT_EJECT_DATA; >> + >> #endif > > I'm sorry, I still don't understand -- why is it necessary to modify > UefiCpuPkg *at all*, for this feature? > > (1) The structure type for the data exchange should be in an OvmfPkg > header file. > > (2) The dynamic PCD for transferring the address of the structure should > be declared in the "OvmfPkg.dec" file. > > (3) The "handler" call is made > - from SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib, > - to CpuEject() in OvmfPkg/CpuHotplugSmm. > > First, this call should not need a function pointer at all -- the > CpuEject() logic should be flattened into > OvmfPkg/Library/SmmCpuFeaturesLib). Sure, it can be flattened -- but it's out of place in SmmCpuFeaturesLib. All of the logic pertaining to the unplug is in CpuHotPlugSmm, so it seems to make sense to locate the related ejection code along with it. If you are concerned about paying the additional cost of an indirect call then I think it should be possible to install the handler only when there is an actual unplug to be done and remove it after. > > 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. 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. Thanks Ankur > > It's possible I'm not seeing something; please explain. > > Thanks > Laszlo > >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf >> index 76b146299679..f79c874d74f1 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf >> @@ -131,6 +131,7 @@ >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress ## SOMETIMES_CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## SOMETIMES_PRODUCES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress ## SOMETIMES_PRODUCES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable ## CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmShadowStackSize ## SOMETIMES_CONSUMES >> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec >> index d83c084467b3..704ccc05f662 100644 >> --- a/UefiCpuPkg/UefiCpuPkg.dec >> +++ b/UefiCpuPkg/UefiCpuPkg.dec >> @@ -339,6 +339,11 @@ >> # @ValidList 0x80000001 | 0 >> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress|0x0|UINT64|0x60000011 >> >> + ## Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported. >> + # @Prompt The pointer to CPU Hot Eject Data. >> + # @ValidList 0x80000001 | 0 >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0x0|UINT64|0x60000017 >> + >> ## Indicates processor feature capabilities, each bit corresponding to a specific feature. >> # @Prompt Processor feature capabilities. >> # @ValidList 0x80000001 | 0 >> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni >> index 219c1963bf08..b1721f256632 100644 >> --- a/UefiCpuPkg/UefiCpuPkg.uni >> +++ b/UefiCpuPkg/UefiCpuPkg.uni >> @@ -140,6 +140,10 @@ >> >> #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotPlugDataAddress_HELP #language en-US "Contains the pointer to a CPU Hot Plug Data structure if CPU hot-plug is supported." >> >> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_PROMPT #language en-US "The pointer to CPU Hot Eject Data" >> + >> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_HELP #language en-US "Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported." >> + >> #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_PROMPT #language en-US "Number of reserved variable MTRRs" >> >> #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_HELP #language en-US "Specifies the number of variable MTRRs reserved for OS use." >> >