From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.503.1610053232153147760 for ; Thu, 07 Jan 2021 13:00:32 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FO0aA7t4; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610053231; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8l4ZyfMGxrO8RnMWcWn//Xu+zx0a61cielVOVvCoQoo=; b=FO0aA7t4mOpjnu/hF45vufF2JPT4geg21dFEhqUMYsA4XKlEhJZ1CpZRUNW+q/9Idsn7nr cIhaImYHyLaSSoBV933YsEnFfom5tAv+PKpYidetcbjYtQyNDfMJA1CaRSEAm2j2Y6+MRM 2rm+EAqkdIC7r4+8NUtCTz8lCldgRaQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-389-22KdmIOKO7qhKnce8306rQ-1; Thu, 07 Jan 2021 16:00:29 -0500 X-MC-Unique: 22KdmIOKO7qhKnce8306rQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A83F98144E0; Thu, 7 Jan 2021 21:00:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-164.ams2.redhat.com [10.36.112.164]) by smtp.corp.redhat.com (Postfix) with ESMTP id 55B8874440; Thu, 7 Jan 2021 21:00:24 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface() From: "Laszlo Ersek" To: devel@edk2.groups.io, ankur.a.arora@oracle.com 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> Message-ID: <99d2244c-6279-c19e-cde9-888b997d91c5@redhat.com> Date: Thu, 7 Jan 2021 22:00:23 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/07/21 21:48, 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. > > Either way, I would advise against this approach. We should limit > ourselves to modifying only the contents of > SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib. > > 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. If you need "IsBSP" for a *different purpose* than unplugging the BSP itself, then you can fetch that easily: please see the PlatformSmmBspElection() function in "OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c". The logic there is so simple that you can simply copy it into SmmCpuFeaturesRendezvousExit() [OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c], to calculate IsBSP on the spot (rather than taking it from a parameter): MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr; ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE); IsBsp = (BOOLEAN)(ApicBaseMsr.Bits.BSP == 1); Thanks Laszlo > > *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). > > I think I'll skip reviewing this version beyond this email, as the > required changes appear quite intrusive. > > 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; >> >> >