If an AP has been disabled on purpose due to a failure and is expected to never 
be enabled, then a broadcast mechanism would not be a good idea.
Instead, we should consider enhancing EFI_MP_SERVICES_ENABLEDISABLEAP to 
support a non-blocking operation so it can be called in a loop for a group of 
target APs.  Or a new API that allows a bitmask of APs to enable.

Mike

From: Zhao, Jason <jason.z...@intel.com>
Sent: Thursday, November 21, 2024 4:03 AM
To: Wu, Jiaxin <jiaxin...@intel.com>; Liu, Zhiguang <zhiguang....@intel.com>; 
devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Tan, Dun <dun....@intel.com>; 
Kumar, Rahul R <rahul.r.ku...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; 
Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming 
<gaolim...@byosoft.com.cn>
Cc: Kumar, Chandana C <chandana.c.ku...@intel.com>; Kuo, Donald 
<donald....@intel.com>; Zhao, Jason <jason.z...@intel.com>
Subject: RE: UefiCpuPkg: Proposal to enable/disable AP parallel

Hi Zhiguang,

May be a fool question.
If we follow the 1st solution, do we still have any API to enable/disable AP 
one by one?
How can we handle any scenario like only enable all Aps with specific core type 
(only big cores or only Atoms) if this API is changed to enable all Aps.

BRs/Jason
From: Wu, Jiaxin <jiaxin...@intel.com<mailto:jiaxin...@intel.com>>
Sent: Thursday, November 21, 2024 5:54 PM
To: Liu, Zhiguang <zhiguang....@intel.com<mailto:zhiguang....@intel.com>>; 
devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray 
<ray...@intel.com<mailto:ray...@intel.com>>; Tan, Dun 
<dun....@intel.com<mailto:dun....@intel.com>>; Kumar, Rahul R 
<rahul.r.ku...@intel.com<mailto:rahul.r.ku...@intel.com>>; Gerd Hoffmann 
<kra...@redhat.com<mailto:kra...@redhat.com>>; Kinney, Michael D 
<michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; Gao, Liming 
<gaolim...@byosoft.com.cn<mailto:gaolim...@byosoft.com.cn>>
Cc: Kumar, Chandana C 
<chandana.c.ku...@intel.com<mailto:chandana.c.ku...@intel.com>>; Zhao, Jason 
<jason.z...@intel.com<mailto:jason.z...@intel.com>>; Kuo, Donald 
<donald....@intel.com<mailto:donald....@intel.com>>
Subject: RE: UefiCpuPkg: Proposal to enable/disable AP parallel

Solution 1, it's not kind of spec violating. Instead, it's to add a new 
capability to the existing interface, and it's a compatible change, no impact 
to existing interface usage. I recall we have a guideline that prioritizes 
code-first approaches if there are no compatibility issues. Mike and Ray can 
comment on this. If no objection, I also prefer this way.

Solution 2 cannot handle the PPI case, leading to inconsistent behavior between 
the protocol and PPI for the mpservice2.

Solutions 3 and 4 are more like workarounds to address the specific issue.

Thanks,
Jiaxin

From: Liu, Zhiguang <zhiguang....@intel.com<mailto:zhiguang....@intel.com>>
Sent: Wednesday, November 20, 2024 3:13 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray 
<ray...@intel.com<mailto:ray...@intel.com>>; Wu, Jiaxin 
<jiaxin...@intel.com<mailto:jiaxin...@intel.com>>; Liu, Zhiguang 
<zhiguang....@intel.com<mailto:zhiguang....@intel.com>>; Tan, Dun 
<dun....@intel.com<mailto:dun....@intel.com>>; Kumar, Rahul R 
<rahul.r.ku...@intel.com<mailto:rahul.r.ku...@intel.com>>; Gerd Hoffmann 
<kra...@redhat.com<mailto:kra...@redhat.com>>; Kinney, Michael D 
<michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; Gao, Liming 
<gaolim...@byosoft.com.cn<mailto:gaolim...@byosoft.com.cn>>
Cc: Kumar, Chandana C 
<chandana.c.ku...@intel.com<mailto:chandana.c.ku...@intel.com>>; Zhao, Jason 
<jason.z...@intel.com<mailto:jason.z...@intel.com>>; Kuo, Donald 
<donald....@intel.com<mailto:donald....@intel.com>>
Subject: UefiCpuPkg: Proposal to enable/disable AP parallel

Hi MdePkg and UefiCpuPkg maintainers and reviewers

Recently, we met a performance issue when waking up disabled APs.
There is usage where BIOS needs to disable all APs, do something and then 
enable all APs.
Now, we are using the MpService PPI/Protocol EnableDisableAP(). This function 
can only enable/disable one AP each time.
To enable one AP, MP service needs to send INIT-SIPI-SIPI, which takes around 
10ms.
And now, we will have more than 10 APs in a client platform, and it will take 
more than 100ms.
The function definition of EnableDisableAP is:
typedef
EFI_STATUS
(EFIAPI *EFI_MP_SERVICES_ENABLEDISABLEAP)(
  IN  EFI_MP_SERVICES_PROTOCOL  *This,
  IN  UINTN                     ProcessorNumber,
  IN  BOOLEAN                   EnableAP,
  IN  UINT32                    *HealthFlag OPTIONAL
  );
The input parameter ProcessorNumber accepts a range from 0 to the total number 
of logical processors minus 1.

To support enable/disable AP parallel, I have below solutions:

Solution1:
Let input parameter ProcessorNumber accept a MAX_UINTN also. MAX_UINTN means to 
enable/disable all APs.
The draft PR is at https://github.com/tianocore/edk2/pull/6453 When the 
parameter is MAX_UINTN, EnableDisableAP() will enable/disable APs in a parallel 
way.
                However, we need to change below header files
                                MdePkg\Include\Protocol\MpService.h
                                MdePkg\Include\Ppi\MpServices.h
                                UefiCpuPkg\Include\Ppi\MpServices2.h
                The above two follow PI spec. We need to modify the PI spec 
first.

Solution2:
                Similar with solution1, but to avoid violating spec, add a new 
Protocol named MpServices2. Only change below header files.
                                UefiCpuPkg\Include\Ppi\MpServices2.h
UefiCpuPkg\Include\Protocol\MpServices2.h (new)
                The MdePkg part remains no change.

Solution3:
                MpService create new PPI/Protocol to only contain one function 
EnableDisableAllAps(), which will enable/disable all APs in a parallel way.

Solution4:
                Add PPI/Protocol notify in MpLib. The notify call back function 
will set WakeUpByInitSipiSipi to True. Similar code is removed in 
https://github.com/tianocore/edk2/pull/6303/commits/f1f8374381019169d421a65a896ab42ed5338c1e
                When users need to disable and then enable cores, the flow will 
be:

  1.  Send Init to all APs. (to disable all APs)
  2.  Do things that user need to do when all APs are disabled
  3.  Notify callback function
  4.  Use StartupAllAPs() from existing PPI/Protocol to wake up all APs.
The flow is similar with how S3 SMM code take control APs and then give the 
control back in old days.

Personally, I prefer solution1. It is simpler, but it does violate spec.
Please let me know your comments or any new idea, please share.

Thanks
Zhiguang



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120813): https://edk2.groups.io/g/devel/message/120813
Mute This Topic: https://groups.io/mt/109680758/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to