Subrata Banik has posted comments on this change. ( https://review.coreboot.org/25921 )
Change subject: Documentation/Intel: Add MultiProcessorInit documentation
......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
File Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md:
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
PS3, Line 7: for Intel 9th Gen (Cannon Lake) and beyond CPUs
> This is why you are doing it, but it seems to me that the result […]
i have made this clear why we need to create this API interface in this section.
Although i agree this is very generic implementation and dependent on mp_init.c so going forward we might feel a feed to make it generic, today we can't do because it has some EDK2 header dependencies which is only available for intel UDK2017 platform (CNL onwards).
btw, why you might need to have 2 document, i'm not going to explain how mp_init works on x86 platform, those are already in SDM, this document is to only talk about why we are creating EDK2 interface in coreboot and how intel thinks this can help our partners design.
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
PS3, Line 25: closed source in nature
> It might be just me, but to me "closed source in nature" sounds like […]
i don;t understand what you mean by can't be trusted ?
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
PS3, Line 29: get-rid of such close source
> If I understand it correctly (please correct me if not), it only […]
Yes, you are right
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
PS3, Line 35: It’s also possible for coreboot to extend its own
: support model and create a sets of APIs which later can be used by FSP to run CPU feature
: programming using coreboot published APIs
> I don't understand. coreboot already has such an API […]
i guess you have missed problem statement, you are referring to proposal and try to understand why i have made this proposal.
its clearly mentioned in problem statement why we even need this proposal
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
PS3, Line 68: 1. coreboot was using SkipMpInit=1 which will skip entire FSP CPU feature programming.
> This is decided per board and not for coreboot in general, AFAICS.
not sure what you mean by coreboot? this is a UPD which is intel fsp specific. not sure how this can be in general ?
--
To view, visit https://review.coreboot.org/25921
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6096ef31d8a523c00cbad39ab9d4884e735fde
Gerrit-Change-Number: 25921
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Comment-Date: Tue, 01 May 2018 11:43:09 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/25921 )
Change subject: Documentation/Intel: Add MultiProcessorInit documentation
......................................................................
Patch Set 3:
> (5 comments)
>
> Thanks for writing this up. The relation to the `SkipMpInit` wasn't
> clear to me before (I'm not 100% sure yet, but much closer).
>
> Please make it clear if you intend to force usage of this interface
> or if it is optional (for `SkipMpInit=1`).
Nothing is force here, its optional if you want to avoid FSP to use those interface which coreboot has published, then just set SkipMpInit=1, FSP won't call this APIs to run those feature programming over AP.
--
To view, visit https://review.coreboot.org/25921
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6096ef31d8a523c00cbad39ab9d4884e735fde
Gerrit-Change-Number: 25921
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Comment-Date: Tue, 01 May 2018 11:35:42 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No
Nico Huber has posted comments on this change. ( https://review.coreboot.org/25921 )
Change subject: Documentation/Intel: Add MultiProcessorInit documentation
......................................................................
Patch Set 3:
(5 comments)
Thanks for writing this up. The relation to the `SkipMpInit` wasn't
clear to me before (I'm not 100% sure yet, but much closer).
Please make it clear if you intend to force usage of this interface
or if it is optional (for `SkipMpInit=1`).
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
File Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md:
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
PS3, Line 7: for Intel 9th Gen (Cannon Lake) and beyond CPUs
This is why you are doing it, but it seems to me that the result
is 100% generic. Both, coreboot's MP API and EFI_MP_SERVICES_PPI,
are SoC and vendor independent. Hence I wouldn't expect this in
Documentation/Intel/ and the implementation in cpu/x86/ along-
side `mp_init.c`.
Maybe split this in two documents, one that states that coreboot
implements EFI_MP_SERVICES_PPI APIs and why (foreign code linked
with coreboot may rely on it) and one that explains how it's
working together with FSP?
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
PS3, Line 25: closed source in nature
It might be just me, but to me "closed source in nature" sounds like
"could not even be trusted if published".
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
PS3, Line 29: get-rid of such close source
If I understand it correctly (please correct me if not), it only
reduces closed-source overhead in case of `SkipMpInit=0`, right?
`SkipMpInit=1` will still work and run the least closed-source code?
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
PS3, Line 35: It’s also possible for coreboot to extend its own
: support model and create a sets of APIs which later can be used by FSP to run CPU feature
: programming using coreboot published APIs
I don't understand. coreboot already has such an API
(include/cpu/x86/mp.h). If it could be used by FSP later,
why can't it be used now?
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorI…
PS3, Line 68: 1. coreboot was using SkipMpInit=1 which will skip entire FSP CPU feature programming.
This is decided per board and not for coreboot in general, AFAICS.
--
To view, visit https://review.coreboot.org/25921
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6096ef31d8a523c00cbad39ab9d4884e735fde
Gerrit-Change-Number: 25921
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Comment-Date: Tue, 01 May 2018 11:26:51 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/25709 )
Change subject: arch/x86: Print CPU Index and APIC ID in case of exception
......................................................................
Patch Set 13:
Build Unstable
https://qa.coreboot.org/job/coreboot-gerrit/71076/ : UNSTABLE
https://qa.coreboot.org/job/coreboot-checkpatch/25251/ : SUCCESS
--
To view, visit https://review.coreboot.org/25709
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3776c9259747197a5f2410032f9b03786407fb
Gerrit-Change-Number: 25709
Gerrit-PatchSet: 13
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Comment-Date: Tue, 01 May 2018 08:01:29 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/25964 )
Change subject: arch/x86: Make cpu_index() available for bootblock
......................................................................
Patch Set 2:
Build Unstable
https://qa.coreboot.org/job/coreboot-gerrit/71073/ : UNSTABLE
--
To view, visit https://review.coreboot.org/25964
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I02b2387eb601abe2d64ed20ebf8a98572befd26c
Gerrit-Change-Number: 25964
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 01 May 2018 04:59:46 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/25709 )
Change subject: arch/x86: Print CPU Index and APIC ID in case of exception
......................................................................
Patch Set 12:
Build Unstable
https://qa.coreboot.org/job/coreboot-gerrit/71074/ : UNSTABLE
https://qa.coreboot.org/job/coreboot-checkpatch/25249/ : SUCCESS
--
To view, visit https://review.coreboot.org/25709
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3776c9259747197a5f2410032f9b03786407fb
Gerrit-Change-Number: 25709
Gerrit-PatchSet: 12
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 01 May 2018 04:52:33 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/25709 )
Change subject: arch/x86: Print CPU Index and APIC ID in case of exception
......................................................................
Patch Set 11:
Build Failed
https://qa.coreboot.org/job/coreboot-gerrit/71071/ : ABORTED
https://qa.coreboot.org/job/coreboot-checkpatch/25247/ : SUCCESS
--
To view, visit https://review.coreboot.org/25709
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3776c9259747197a5f2410032f9b03786407fb
Gerrit-Change-Number: 25709
Gerrit-PatchSet: 11
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 01 May 2018 04:43:21 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No