Thanks all for your comments. Let me reply one by one.
Hi Jason,
Yes, we still support enable/disable AP one by one. Caller can still give an AP
processer ID to disable/enable this AP. Just add a capability to allow caller
give a all Fs value to disable/enable all APs.
Hi Jiaxin,
Solution 2 will also change the PPI mpservice2. PPI mpservice2 will provide
exact same interface as Protocol mpservice2.
Hi Mike,
I understand your concern, However, I think it is a very corner case where AP
is disable on purpose and is expected to never be enabled. I am not sure if we
should take effect for such corner case.
If we need to consider such case, bitmask may be not a good solution because we
don't know how many cores there are in future. (A UINT64 bit misk can only
handle 64-core CPU)
Supportting a non-blocking operation is a good idea for me, let me write the
solution down in detail.
EFI_MP_SERVICES_ENABLEDISABLEAP: (enable case)
Send INIT IPI to AP
Set AP to state CpuStateAfterInitIpi
return
EFI_MP_SERVICES_ENABLEDONE:
Delay 10ms
For loop to check each cores:
If this core's state is CpuStateAfterInitIpi
Send SIPI IPI to this core to wake up it.
Set AP to state CpuStateIdle
The solution needs to add a new interface EnableDone().
After calling EnableDisalbeAp in a for loop to enable APs, caller need to call
the new interface to really enable cores.
To be compatible with original EnableDisalbeAp, we can use BIT63 in the input
ProcessorNumber. If BIT63 is not set, it is a blocking operation like before,
and no need to call EnableDone(). If BIT63 is set, it is a non-block operation
and need caller to call EnableDone() later.
Another similar idea (Ray also mentioned offline to me before) is that we don't
provide the new interface. The similar thing should be done in next first call
of MpService.
For example, after user calling EnableDisalbeAp() to enable some APs in a for
loop, another module calls StartupThisAP() to let a specific AP to something.
Inside StartupThisAP(), MpLib first check the AP state, and if any core's state
is CpuStateAfterInitIpi, MpLib will delay 10ms, and wake up these APs, and
change the state to CpuStateIdle. After that, MpLib do the original work.
I didn't bring up this idea because I think it is a little complicated. In PEI
phase, we don't know who the last caller of MpService is. For example, if a
caller enables an AP, and then PEI phase ends, how DXE know that? We need
additional field in the MP_HAND_OFF Hob to record such state.
Mike, please help comment on these two new solutions. Or if you have other good
idea, please share.
Thanks
Zhiguang
From: Kinney, Michael D <michael.d.kin...@intel.com>
Sent: Friday, November 22, 2024 2:44 AM
To: Zhao, Jason <jason.z...@intel.com>; 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>; Gao, Liming
<gaolim...@byosoft.com.cn>
Cc: Kumar, Chandana C <chandana.c.ku...@intel.com>; Kuo, Donald
<donald....@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>
Subject: RE: UefiCpuPkg: Proposal to enable/disable AP parallel
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<mailto:jason.z...@intel.com>>
Sent: Thursday, November 21, 2024 4:03 AM
To: Wu, Jiaxin <jiaxin...@intel.com<mailto:jiaxin...@intel.com>>; 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>>; Kuo, Donald
<donald....@intel.com<mailto:donald....@intel.com>>; Zhao, Jason
<jason.z...@intel.com<mailto: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 (#120843): https://edk2.groups.io/g/devel/message/120843
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]
-=-=-=-=-=-=-=-=-=-=-=-
