Attention is currently required from: Martin Roth, Tim Wawrzynczak, Meera Ravindranath, Aamir Bohra, Andrey Petrov, Patrick Rudolph. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49474 )
Change subject: drivers/intel/fsp2_0: Add support for MP PPI services2 APIs ......................................................................
Patch Set 6:
(18 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49474/comment/736b11ad_9ffac746 PS6, Line 7: MP PPI services2 nit: MP services2 PPI
https://review.coreboot.org/c/coreboot/+/49474/comment/ba1ca19b_d9527816 PS6, Line 13: BUG=b:169196864
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/49474/comment/d108dbec_75935716 PS6, Line 267: : if FSP_PEIM_TO_PEIM_INTERFACE : source "src/drivers/intel/fsp2_0/ppi/Kconfig" : endif : Why was this dropped?
File src/drivers/intel/fsp2_0/include/fsp/ppi/mp_service_ppi.h:
https://review.coreboot.org/c/coreboot/+/49474/comment/be5cb13b_52936fc8 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 definition of this type. With V1 and V2 providing different definitions, it is unnecessarily going to create a dependency on the header files.
https://review.coreboot.org/c/coreboot/+/49474/comment/d12e10f3_29b912d7 PS6, Line 19: : /* : * get the number of logical processor in the platform : */ nit: Recommended comment style for single line comments is either /* ... */ or //
https://review.coreboot.org/c/coreboot/+/49474/comment/104bfe01_525f2ea9 PS6, Line 36: ) I think you are going to need an additional parameter here to decide whether the procedure should be run in parallel or in serial fashion. b/169114674. Are you planning to handle that as a follow-up?
https://review.coreboot.org/c/coreboot/+/49474/comment/4105c7b3_a9edf868 PS6, Line 41: ) What about timeout_usec?
https://review.coreboot.org/c/coreboot/+/49474/comment/897b4831_ddcc066a 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. Do we really want to have common helpers for these?
File src/drivers/intel/fsp2_0/ppi/Kconfig:
PS6: I think we can simplify this file somewhat.
1. I know I asked to use "choice". But, one thing I realized is that choice requires user prompt, which though harmless, is unnecessary. The user/mainboard does not really have an option to select one v/s other since it is pretty much governed by the FSP being used for the platform. Given, that I think we can just use configs without choice and add a check in mp_service_ppi.c to ensure that only one is selected.
2. "PLATFORM_USES.." and "FSP_USES.." looks really confusing. What do you think about: CB:50274 and CB:50275. I think we can add following configs in this file:
config MP_SERVICES_PPI (added in CB:50275) ...
config MP_SERVICES_PPI_V1 bool default n select MP_SERVICES_PPI help ... config MP_SERVICES_PPI_V2 bool default n select MP_SERVICES_PPI help ...
File src/drivers/intel/fsp2_0/ppi/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/49474/comment/05abea61_fba9605e PS6, Line 3: CONFIG_USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI And this can be CONFIG_MP_SERVICES_PPI
https://review.coreboot.org/c/coreboot/+/49474/comment/1ef7a4a2_5a4d547b PS6, Line 4: CONFIG_FSP_USES_MP_SERVICES_PPI ... and CONFIG_MP_SERVICES_PPI_V1
File src/drivers/intel/fsp2_0/ppi/mp_service1.c:
https://review.coreboot.org/c/coreboot/+/49474/comment/03fddcf4_6b1cb0c2 PS6, Line 13: remove extra blank lines.
https://review.coreboot.org/c/coreboot/+/49474/comment/cdcebf63_fb0f061c PS6, Line 61: /* : * EDK2 UEFIPKG Open Source MP Service PPI to be installed : */ Single line comment style: // or /* ... */
File src/drivers/intel/fsp2_0/ppi/mp_service2.c:
https://review.coreboot.org/c/coreboot/+/49474/comment/f8d1d2cc_9411a4a4 PS6, Line 14: extra blank line not required.
https://review.coreboot.org/c/coreboot/+/49474/comment/c1551958_1f543e81 PS6, Line 69: /* : * EDK2 UEFIPKG Open Source MP Services 2 PPI to be installed : */ : Use single line comment style.
File src/include/efi/efi_datatype.h:
https://review.coreboot.org/c/coreboot/+/49474/comment/eac097ec_8db29a67 PS6, Line 9: #include <Ppi/MpServices.h> : #include <Ppi/MpServices2.h> I don't think we should be including either of these here.
https://review.coreboot.org/c/coreboot/+/49474/comment/57a0b673_672df5d3 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.
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.
-
Furquan Shaikh (Code Review)