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.web11.751.1610054382073082439 for ; Thu, 07 Jan 2021 13:19:42 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=JtD4VX00; 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 107L96JB169125; Thu, 7 Jan 2021 21:19:34 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=6dqIvaSGgnzEYJ4HNWqewfKgWXfRBrJNaH9XqyPhDhc=; b=JtD4VX00pYQHJSnf7YCiTlNW76qj/1M9w+o5ZmNunUYyINEDWxgEKsWXledCwNVHDGlQ 6iaM6u77Z/FkihNybDKw5TrvAzKPlHilpTasgM+M5QMALAvY3e2gvhK7+/iWLjfvdhwg RkaA9JqlbrZW5raQGgENuBGClmGuNRb/WD/lhD1pX3fq4xQ08zilX/kOCzGK9MVsJAwL JijxJyNoKkwh9/JppV77FVmFT0s91jKml3n+KQ/3/JWvu6TXFgN36EmIM8buzqx5OJvF ZwMCynQd8Y5WkwJLuD7q9UkGnXWO4PgKZJTW7wPB9lcDBkmOxpCu3wict0gikIlTqen3 ng== Received: from aserp3030.oracle.com (aserp3030.oracle.com [141.146.126.71]) by aserp2120.oracle.com with ESMTP id 35wepmecx1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 07 Jan 2021 21:19:34 +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 107LAWvA051108; Thu, 7 Jan 2021 21:19:34 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserp3030.oracle.com with ESMTP id 35v4rege80-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 07 Jan 2021 21:19:34 +0000 Received: from abhmp0010.oracle.com (abhmp0010.oracle.com [141.146.116.16]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 107LJV0s000618; Thu, 7 Jan 2021 21:19:32 GMT Received: from [10.159.232.136] (/10.159.232.136) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Thu, 07 Jan 2021 13:19:31 -0800 Subject: Re: [edk2-devel] [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface() To: Laszlo Ersek , devel@edk2.groups.io Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com, Jian J Wang , Michael D Kinney , Liming Gao , Zhiguang Liu , Eric Dong , Ray Ni , Rahul Kumar , Aaron Young References: <20210107195515.106158-1-ankur.a.arora@oracle.com> <20210107195515.106158-6-ankur.a.arora@oracle.com> From: "Ankur Arora" Message-ID: <6893343b-bfb0-2b2b-4954-72a2ad838c48@oracle.com> Date: Thu, 7 Jan 2021 13:19:29 -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: X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9857 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 bulkscore=0 suspectscore=0 spamscore=0 adultscore=0 malwarescore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101070123 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9857 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0 spamscore=0 impostorscore=0 phishscore=0 lowpriorityscore=0 suspectscore=0 priorityscore=1501 mlxscore=0 malwarescore=0 clxscore=1015 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101070123 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-01-07 12:48 p.m., Laszlo Ersek wrote: > On 01/07/21 20:55, Ankur Arora wrote: >> Add MmRegisterShutdownInterface(), which is used to register a callback, >> that gets used to do the final ejection as part of CPU hot-unplug. >> >> Cc: Jian J Wang >> Cc: Michael D Kinney >> Cc: Liming Gao >> Cc: Zhiguang Liu >> Cc: Eric Dong >> Cc: Ray Ni >> Cc: Laszlo Ersek >> Cc: Rahul Kumar >> Cc: Igor Mammedov >> Cc: Boris Ostrovsky >> Cc: Aaron Young >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132 >> Signed-off-by: Ankur Arora >> --- >> >> Not sure this is the right way to register a callback to be called >> from SmmCpuFeaturesRendezvousExit(). Happy to hear suggestions for >> a better way to accomplish this. > > No, it's not. > > SmmCpuFeaturesRendezvousExit() is an interface in the > "UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h" class. Each platform is > supposed to create its own instance of this library class. For example, > OVMF's instance is at: > > OvmfPkg/Library/SmmCpuFeaturesLib > > You can modify the SmmCpuFeaturesRendezvousExit() function > implementation in that library instance. > > I don't think you can easily modify the function *declaration* though, > as that would break a whole lot of platforms that exist outside of edk2. > > Additionally, I don't think you can modify EFI_MM_SYSTEM_TABLE. That's a > structure defined in the Platform Init specification. You'd first need > to standardize (via the Platform Init Working Group of the UEFI Forum) > the proposed change, and a compatible way to upgrade would have to be > found. An alternative would be the "code first" procedure, which exists > so that code can be contributed ahead of changing the published > standards. But I'm unsure (I don't remember) how that procedure works in > practice. Thanks, yeah I'm definitely not looking to do that. I did look through the spec but not enough to get any kind of familiarity with it. > > Either way, I would advise against this approach. We should limit > ourselves to modifying only the contents of > SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib. That was my initial approach. The problem that ran into was that which CPUs to eject depends on state held in OvmfPkg/CpuHotPlugSmm (see patch 8) which, I could not figure out a way to make that available given that the libraries are linked statically. The other way I can think of doing this would be to just reprobe this state from QEMU. The problem with that is that then there's zero shared state between the removal and the ejection. > > You should basically migrate the contents of CpuUnplugExitWork() (in > patch#8) into SmmCpuFeaturesRendezvousExit(), without changing the > prototype of SmmCpuFeaturesRendezvousExit(). This might require you to > calculate "IsBSP" on the spot, possibly with the help of some global > data that you set up in advance. The reason for trying to capture IsBSP was that mSmmMpSyncData->BspIndex has gone at this point. If, as you say, there are significant problems with changing the prototype, then I can figure out a way to avoid that, especially given that we get the BSP by reading an APIC MSR anyway. > > *However*, regarding "IsBSP" itself -- on the QEMU platform, the BSP is > neither unpluggable, nor switchable with any one of the APs. A removal > for the BSP will never be attempted, so it's fine to add much simpler > code that recognizes such an attempt (as a sanity check), and hangs hard > on it. If that would lead to grave complications, then don't bother with > it; just assume (or enforce elsewhere) that the BSP is never being > unplugged. > > Please see the apic_designate_bsp() call sites in the QEMU source code > -- there is exactly one of them, in "target/i386/cpu.c": > > /* We hard-wire the BSP to the first CPU. */ > apic_designate_bsp(cpu->apic_state, s->cpu_index == 0); > > > By adding the business logic to SmmCpuFeaturesRendezvousExit() instead > of CpuUnplugExitWork, said logic will actually be included in the > PiSmmCpuDxeSmm binary -- but that's fine. That's why the > SmmCpuFeaturesRendezvousExit() API exists, as a platform customization > hook for PiSmmCpuDxeSmm. > > > A formal comment in closing -- modifying two packages in the same patch, > such as OvmfPkg and UefiCpuPkg, is almost always forbidden. It is > permitted exceptionally, when there is absolutely no way to solve an > issue with a gradual approach (i.e., by modifying the packages in > question with separate patches). Thanks for letting me know. I'll avoid that in future versions. > > I think I'll skip reviewing this version beyond this email, as the > required changes appear quite intrusive. Could you take a look at patch 8? That's really the crux of the series and your comments on that would be helpful. Thanks Ankur > > Thanks > Laszlo > >> >> --- >> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 1 + >> MdePkg/Include/Pi/PiMmCis.h | 16 ++++++++++++++++ >> MdePkg/Include/Pi/PiSmmCis.h | 2 ++ >> MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c | 11 +++++++++++ >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 1 + >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 1 + >> 6 files changed, 32 insertions(+) >> >> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> index cfa9922cbdb5..9d883bb06633 100644 >> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c >> @@ -39,6 +39,7 @@ EFI_SMM_SYSTEM_TABLE2 gSmmCoreSmst = { >> SmmFreePool, >> SmmAllocatePages, >> SmmFreePages, >> + NULL, // SmmShutdownAp >> NULL, // SmmStartupThisAp >> 0, // CurrentlyExecutingCpu >> 0, // NumberOfCpus >> diff --git a/MdePkg/Include/Pi/PiMmCis.h b/MdePkg/Include/Pi/PiMmCis.h >> index fdf0591a03d6..237bd8dcba76 100644 >> --- a/MdePkg/Include/Pi/PiMmCis.h >> +++ b/MdePkg/Include/Pi/PiMmCis.h >> @@ -77,6 +77,14 @@ EFI_STATUS >> IN OUT VOID *ProcArguments OPTIONAL >> ); >> >> +typedef >> +EFI_STATUS >> +(EFIAPI *EFI_MM_SHUTDOWN_AP)( >> + IN UINTN CpuNumber, >> + IN BOOLEAN IsBSP >> + ); >> + >> + >> /** >> Function prototype for protocol install notification. >> >> @@ -242,6 +250,13 @@ VOID >> IN CONST EFI_MM_ENTRY_CONTEXT *MmEntryContext >> ); >> >> +EFI_STATUS >> +EFIAPI >> +MmRegisterShutdownInterface ( >> + IN EFI_MM_SHUTDOWN_AP Procedure >> + ); >> + >> + >> /// >> /// Management Mode System Table (MMST) >> /// >> @@ -282,6 +297,7 @@ struct _EFI_MM_SYSTEM_TABLE { >> /// >> /// MP service >> /// >> + EFI_MM_SHUTDOWN_AP MmShutdownAp; >> EFI_MM_STARTUP_THIS_AP MmStartupThisAp; >> >> /// >> diff --git a/MdePkg/Include/Pi/PiSmmCis.h b/MdePkg/Include/Pi/PiSmmCis.h >> index 06ef4aecd7b5..296dc01f6703 100644 >> --- a/MdePkg/Include/Pi/PiSmmCis.h >> +++ b/MdePkg/Include/Pi/PiSmmCis.h >> @@ -49,6 +49,7 @@ EFI_STATUS >> IN UINTN TableSize >> ); >> >> +typedef EFI_MM_SHUTDOWN_AP EFI_SMM_SHUTDOWN_AP; >> typedef EFI_MM_STARTUP_THIS_AP EFI_SMM_STARTUP_THIS_AP; >> typedef EFI_MM_NOTIFY_FN EFI_SMM_NOTIFY_FN; >> typedef EFI_MM_REGISTER_PROTOCOL_NOTIFY EFI_SMM_REGISTER_PROTOCOL_NOTIFY; >> @@ -137,6 +138,7 @@ struct _EFI_SMM_SYSTEM_TABLE2 { >> /// >> /// MP service >> /// >> + EFI_SMM_SHUTDOWN_AP SmmShutdownAp; >> EFI_SMM_STARTUP_THIS_AP SmmStartupThisAp; >> >> /// >> diff --git a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c >> index 27f9d526e396..c7d81a0dc193 100644 >> --- a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c >> +++ b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c >> @@ -55,3 +55,14 @@ MmServicesTableLibConstructor ( >> >> return EFI_SUCCESS; >> } >> + >> +EFI_STATUS >> +EFIAPI >> +MmRegisterShutdownInterface ( >> + IN EFI_MM_SHUTDOWN_AP Procedure >> + ) >> +{ >> + gMmst->MmShutdownAp = Procedure; >> + >> + return EFI_SUCCESS; >> +} >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> index db68e1316ec5..f2f67e85e5e9 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> @@ -22,6 +22,7 @@ SMM_CPU_PRIVATE_DATA mSmmCpuPrivateData = { >> NULL, // Pointer to CpuSaveStateSize array >> NULL, // Pointer to CpuSaveState array >> { {0} }, // SmmReservedSmramRegion >> + NULL, // SmmShutdownAp >> { >> SmmStartupThisAp, // SmmCoreEntryContext.SmmStartupThisAp >> 0, // SmmCoreEntryContext.CurrentlyExecutingCpu >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h >> index b8aa9e1769d3..7672834a2f70 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h >> @@ -247,6 +247,7 @@ typedef struct { >> VOID **CpuSaveState; >> >> EFI_SMM_RESERVED_SMRAM_REGION SmmReservedSmramRegion[1]; >> + EFI_SMM_SHUTDOWN_AP SmmShutdownAp; >> EFI_SMM_ENTRY_CONTEXT SmmCoreEntryContext; >> EFI_SMM_ENTRY_POINT SmmCoreEntry; >> >> >