Attention is currently required from: Furquan Shaikh, Martin Roth, Tim Wawrzynczak, Meera Ravindranath, Andrey Petrov, Patrick Rudolph. Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49474 )
Change subject: drivers/intel/fsp2_0: Add support for MP services2 PPI ......................................................................
Patch Set 6:
(17 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49474/comment/d50b2107_1e0b0eb3 PS6, Line 13:
BUG=b:169196864
Done
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/49474/comment/7a80a7d1_ee7a7431 PS6, Line 267: : if FSP_PEIM_TO_PEIM_INTERFACE : source "src/drivers/intel/fsp2_0/ppi/Kconfig" : endif :
Why was this dropped?
This gets sourced from src/Makefile.inc, it was causing lint error due to redundant symbol allocation.
File src/drivers/intel/fsp2_0/include/fsp/ppi/mp_service_ppi.h:
https://review.coreboot.org/c/coreboot/+/49474/comment/676a0287_f386623c PS6, Line 18: efi_pei_mp_services_ppi
I think it would be good to just set this to void so that we can void having to provide the definiti […]
Done
https://review.coreboot.org/c/coreboot/+/49474/comment/c7dc9f26_f7982d96 PS6, Line 19: : /* : * get the number of logical processor in the platform : */
nit: Recommended comment style for single line comments is either /* ... […]
Done
https://review.coreboot.org/c/coreboot/+/49474/comment/92a4d463_83342be8 PS6, Line 36: )
I think you are going to need an additional parameter here to decide whether the procedure should be […]
Ok. I can push a follow up CL for that.
https://review.coreboot.org/c/coreboot/+/49474/comment/d6006391_28429d7c PS6, Line 41: )
What about timeout_usec?
Added to params now. Initially it was default to 1000 * USECS_PER_MSEC as per mp_run_on_all_cpus
https://review.coreboot.org/c/coreboot/+/49474/comment/cc3f0132_4934a90f PS6, Line 49: /* : * switches the requested AP to be the BSP : */ : efi_return_status_t mp_switch_bsp(void); : : /* : * enable or disable an AP. This service may only be called from the BSP. : */ : efi_return_status_t mp_enable_disable_ap(void); :
These return FSP_UNSUPPORTED. […]
we can keep them as placeholders for now? Is it ok?
File src/drivers/intel/fsp2_0/ppi/Kconfig:
PS6:
I think we can simplify this file somewhat. […]
Done
File src/drivers/intel/fsp2_0/ppi/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/49474/comment/7f0b4316_043e75a7 PS6, Line 3: CONFIG_USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
And this can be CONFIG_MP_SERVICES_PPI
Done
https://review.coreboot.org/c/coreboot/+/49474/comment/72839fd8_7fbb82cb PS6, Line 4: CONFIG_FSP_USES_MP_SERVICES_PPI
... […]
Done
File src/drivers/intel/fsp2_0/ppi/mp_service1.c:
https://review.coreboot.org/c/coreboot/+/49474/comment/b37ace9f_c766a223 PS6, Line 13:
remove extra blank lines.
Done
https://review.coreboot.org/c/coreboot/+/49474/comment/33af0316_3ab7deb3 PS6, Line 61: /* : * EDK2 UEFIPKG Open Source MP Service PPI to be installed : */
Single line comment style: // or /* ... […]
Done
File src/drivers/intel/fsp2_0/ppi/mp_service2.c:
https://review.coreboot.org/c/coreboot/+/49474/comment/e539d564_fa56c1fc PS6, Line 14:
extra blank line not required.
Done
https://review.coreboot.org/c/coreboot/+/49474/comment/f25de25b_39d83a2b PS6, Line 69: /* : * EDK2 UEFIPKG Open Source MP Services 2 PPI to be installed : */ :
Use single line comment style.
Done
File src/include/efi/efi_datatype.h:
https://review.coreboot.org/c/coreboot/+/49474/comment/406f0b3c_ec40014f PS6, Line 9: #include <Ppi/MpServices.h> : #include <Ppi/MpServices2.h>
I don't think we should be including either of these here.
Done
https://review.coreboot.org/c/coreboot/+/49474/comment/90d2fa39_57172465 PS6, Line 59: #if CONFIG(FSP_USES_MP_SERVICES2_PPI) : typedef EDKII_PEI_MP_SERVICES2_PPI efi_pei_mp_services_ppi; : #else : typedef EFI_PEI_MP_SERVICES_PPI efi_pei_mp_services_ppi; : typedef EFI_PEI_SERVICES efi_pei_services; : #endif
I think these can be moved to mp_service1.c and mp_service2.c.
Done
File src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Ppi/MpServices2.h:
PS6:
This file is not really present in UDK2017. I don't think we should be adding this here.
Done