Attention is currently required from: Arthur Heymans, Julius Werner, Jérémy Compostella, Kapil Porwal, Nico Huber.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81960?usp=email )
Change subject: arch/x86: Enable long mode entry into payload for x86_64 support
......................................................................
Patch Set 2:
(1 comment)
File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/81960/comment/b40dd853_76a9e902 :
PS1, Line 25: if (CONFIG(PAYLOAD_X86_64_SUPPORT)) {
> > [Subrata] I don't under why all of sudden the "multiple entry point approach" considered critical. The problem of supporting 64-bit direct entry can be handled w/o that as well. Having multiple entry point approach like kernel may not be that critical at this stage. we can jump into 64-bit entry point using boot.c code (CB:81960). For that we don't need to support multiple entry point approach. If you wish to implement multiple entry point approach, please do that w/o blocking the current task is my request.
> >
>
> So multiple entry points is for sure not a must to proceed I'd say. The point I tried to make was that you don't want different ways to inform coreboot ramstage to straight jump to 64bit payload entry (even if there is only one 64bit entry). So we should carefully discuss the route we want to take.
>
> So what do you think of the following suggestion:
>
> - Add a 64bit entry (maybe call it x86_long_mode, to avoid confusion?) in cbfs_payload_segment_type: maybe PAYLOAD_SEGMENT_X86_LONG_MODE_ENTRY ?
> - Add a way to cbfstool to mark the entry of the payload as such (similar to your --64 patch, but then not with file attr, but as payload segment)
> - 64bit ramstage coreboot at runtime decides to jump to PAYLOAD_SEGMENT_X86_LONG_MODE_ENTRY directly if found
> - If not found keep the old 32bit entering code
[Subrata] I really like this approach and isn't this is what I've tried since morning till now? I have used file attribute (over segment_type) as I felt this is more meaningful as we are injecting it at header.
If you wish to submit those CLs, please feel free to do so, I don't mind deleting my cls.
>
> Then later iterations (that are unimportant to your use case, I assume):
> - cbfstool can detect multiple entry points and make the --64 code obsolete
> - Add multiple entry points to libpayload
> - More flexibility in coreboot: i.e. trampoline to 64bit payload with 32bit coreboot ramstage?
>
> This allows you to move forward with jumping straight to a 64bit payload, while also allowing for some flexible backwards compatibility in some cases.
[Subrata] Yes, this is the way to move forward and solving the one problem at a time to unblock others.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81960?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ic5e6f0af11c05e8b075b8c20880c012747a1df9b
Gerrit-Change-Number: 81960
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 18 Apr 2024 18:10:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Julius Werner, Jérémy Compostella, Kapil Porwal, Nico Huber.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81960?usp=email )
Change subject: arch/x86: Enable long mode entry into payload for x86_64 support
......................................................................
Patch Set 2:
(1 comment)
File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/81960/comment/558e9a38_38396150 :
PS1, Line 25: if (CONFIG(PAYLOAD_X86_64_SUPPORT)) {
> [Subrata] I don't under why all of sudden the "multiple entry point approach" considered critical. The problem of supporting 64-bit direct entry can be handled w/o that as well. Having multiple entry point approach like kernel may not be that critical at this stage. we can jump into 64-bit entry point using boot.c code (CB:81960). For that we don't need to support multiple entry point approach. If you wish to implement multiple entry point approach, please do that w/o blocking the current task is my request.
>
So multiple entry points is for sure not a must to proceed I'd say. The point I tried to make was that you don't want different ways to inform coreboot ramstage to straight jump to 64bit payload entry (even if there is only one 64bit entry). So we should carefully discuss the route we want to take.
So what do you think of the following suggestion:
- Add a 64bit entry (maybe call it x86_long_mode, to avoid confusion?) in cbfs_payload_segment_type: maybe PAYLOAD_SEGMENT_X86_LONG_MODE_ENTRY ?
- Add a way to cbfstool to mark the entry of the payload as such (similar to your --64 patch, but then not with file attr, but as payload segment)
- 64bit ramstage coreboot at runtime decides to jump to PAYLOAD_SEGMENT_X86_LONG_MODE_ENTRY directly if found
- If not found keep the old 32bit entering code
Then later iterations (that are unimportant to your use case, I assume):
- cbfstool can detect multiple entry points and make the --64 code obsolete
- Add multiple entry points to libpayload
- More flexibility in coreboot: i.e. trampoline to 64bit payload with 32bit coreboot ramstage?
This allows you to move forward with jumping straight to a 64bit payload, while also allowing for some flexible backwards compatibility in some cases.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81960?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ic5e6f0af11c05e8b075b8c20880c012747a1df9b
Gerrit-Change-Number: 81960
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Thu, 18 Apr 2024 18:03:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Andrey Petrov, Chen, Gang C, Jincheng Li, Jérémy Compostella, Martin L Roth, Ronak Kanabar, Shuo Liu.
Appukuttan V K has uploaded a new patch set (#29) to the change originally created by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/80323?usp=email )
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: drivers/intel/fsp2_0: Default to 64-bits for FSP 2.4
......................................................................
drivers/intel/fsp2_0: Default to 64-bits for FSP 2.4
Sets`PLATFORM_USES_FSP2_X86_32' to `n' by default if FSP 2.4 is
enabled as 64-bits FSP should be norm moving forward.
BUG=b:329034258
TEST=verified on Lunar Lake RVP board (lnlrvp)
Change-Id: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec98
Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com>
---
M src/drivers/intel/fsp2_0/Kconfig
1 file changed, 4 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/80323/29
--
To view, visit https://review.coreboot.org/c/coreboot/+/80323?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec98
Gerrit-Change-Number: 80323
Gerrit-PatchSet: 29
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Chen, Gang C <gang.c.chen(a)intel.com>
Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Chen, Gang C <gang.c.chen(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Jincheng Li <jincheng.li(a)intel.com>
Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Arthur Heymans.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81968?usp=email )
Change subject: libpayload: Add initial patch or 64-bit compilation
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Why duplicate the x86 code into x64? Why not capture the differences with preprocessor as done in coreboot?
I just submit something for your view and i felt we are back/forth talking about we need to implement ABI first.
I really wish to submit the clean code but i pull something from my downstream repo that I'm working to enable this support.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81968?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I69fda47bedf1a14807b1515c4aed6e3a1d5b8585
Gerrit-Change-Number: 81968
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 18 Apr 2024 18:02:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Julius Werner, Jérémy Compostella, Kapil Porwal, Nico Huber.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81960?usp=email )
Change subject: arch/x86: Enable long mode entry into payload for x86_64 support
......................................................................
Patch Set 2:
(2 comments)
Patchset:
PS2:
> Subrata, please slow down. It's not just about implementing it and making it
> work. coreboot and the payload should be independent and interchangeable. If
> you want to create a 64-bit payload that can't be loaded by a current core-
> boot (or one from the release branches?) for no obvious reason, that's ok, but
> then you don't have to do it upstream. I would rather see it upstream, though.
> Adding a 64-bit ABI and handover is something we have to do anyway, for X86S.
> So any effort in that direction would be really appreciated, as long as we
> don't unnecessarily sarcifice compatibility.
>
> The last handover convention worked for what? 20 years, maybe? Please see that
> as a goal for a 64-bit version. We should design and discuss something that can
> last 20 years without binary incompatibilities. And then implement it. In the
> mean time, you can use the old convention as long as your CPU supports protected
> mode.
>
> Beside strengthening the ecosystem, compatibility is also good for other reasons.
> For instance, when someone debugs a regression and doesn't know if the problem
> is in the payload or in coreboot. In the past it was usually possible to try
> a new coreboot with an old payload build and vice versa.
Nico, I totally agree with you that this support has to be added such a way that we can maintain the code for next several year. But there has to be start point and I'm starting that as I have to ensure 64-bit support in AP FW.
I don't understand where are you seeing the below issues
1. compatibility are broken
2. ABI are not stable
yes, we can't achieve everything overnight but when someone is willing to put the energy and implementing supporting why so much back and forth communication. Give the feedback to improve the code and please don't say, "slow down".
if you wish we don't work in upstream that is a feedback which we would like to consider. I felt the community was not aware that 64-bit support will knock our door one day and we don't have answer to that question.
File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/81960/comment/fb44ffc6_29a799f0 :
PS1, Line 25: if (CONFIG(PAYLOAD_X86_64_SUPPORT)) {
> > > > > > > > > Would it be possible to make it an offset? Then we could have a 32-bit entry
> > > > > > > > > point followed by a 64-bit one, and a coreboot that knows the new ABI could skip
> > > > > > > > > the first and jump directly to the new entry point. This would allow the most
> > > > > > > > > compatibility between coreboot and payloads, as we could say the 64-bit entry
> > > > > > > > > point is optional on both ends (except for X86S, ofc.).
> > > > > > > > >
> > > > > > > > > I guess the complexity of such a 32-bit entry point, that switches to long mode,
> > > > > > > > > would depend a lot on the expectations on page tables in the new ABI.
> > > > > > > >
> > > > > > >
> > > > > > > I guess a minimal expectation of at least 4G identity mapped is reasonable. Iirc UPL does that too. I like the idea of multiple entry points over a CBFS attribute as it's indeed the best way to be remain compatible. Linux already has exactly this btw. Question remains on how to achieve this? Have an expectation on the namespace in the ELF file which cbfstool can pick up? .text._entry64?
> > > > > >
> > > > > > can we review CB:81964 that adds the payload cbfs attribute for 64-bit. Then this CL has now updated to fetch the cbfs attribute rather reading any static Kconfig.
> > > > >
> > > > > following kernel concurrent 32-bit and 64-bit entry implementation may not be desired in AP FW as out configuration are very much static. We have to anyway select Kconfig in SoC to know if we are building 32-bit coreboot vs 64-bit coreboot hence, rather than complicating the flow, not sure what else we will bring by adding kernel like implementation while transferring control from coreboot to payload.
> > > > >
> > > >
> > > > My previous comment explained that building coreboot and payload together is not the only way of doing things and should not be assumed... Entering 64bit mode from in the payload itself with a 32bit entry point can be done in 12 instructions. That's not too complicated and should be considered with the dual entry point strategy.
> > > >
> > > > > Anyway, lets review this CL to ensure we have atleast some ways to launch 64-bit payload from a 64-bit coreboot w/o lowering into 32-bit mode. Later we can desire more scalable approach for handoff. btw, i will bring x64 support cls next for libpayload.
> > > >
> > > > Let's not rush this. An ABI needs to be well designed and well documented. I don't think the fix/improve later approach is good here, as it implies breaking the ABI that worked at some point. If you want a 64bit libpayload, maybe start with a 32bit entry point that jumps to 64bit, which should work as is. In the meanwhile let's have a discussion on the ML on what the ABI would look like?
> > >
> > > I respectfully disagree with your approach.
> > >
> > > The problem statement is straightforward:
> > >
> > > coreboot currently always enters payload/libpayload in 32-bit mode, even when Coreboot is built natively for 64-bit mode. As explained, we need to ensure that we have support for 64-bit direct entry without trunking.
> > >
> >
> > Why do you need that without trunking? Are you talking about HW that does not have 32bit support
>
> [Subrata] Intel's claims about next generation SoC is legacy free like 64-bit is only supported.
>
> or do you want to load/jump to the payload above 4G?
>
> [Subrata] This depends on the memory layout for the target platform. if we don't see any free/available memory < 4GB then jumping to 64-bit mode is only option.
>
> >
> > > I agree with you that we should add things to the payload CBFS to make things dynamic so that Coreboot can decide to jump either directly into 64-bit mode or fall back to 32-bit.
> > >
> > > I am not sure what is being broken by this approach. We are enabling 64-bit mode for payload and testing it seamlessly in parallel to 32-bit entry.
> > >
> >
> > If we decide on a multiple entry point approach, then you end up with 2 ways: one with multiple entry points and one with an attribute indicating in which mode the payload works. That's 2 ways to do the same and since ABI needs to be stable that's not a good situation.
>
> [Subrata] I don't under why all of sudden the "multiple entry point approach" considered critical. The problem of supporting 64-bit direct entry can be handled w/o that as well. Having multiple entry point approach like kernel may not be that critical at this stage. we can jump into 64-bit entry point using boot.c code (CB:81960). For that we don't need to support multiple entry point approach. If you wish to implement multiple entry point approach, please do that w/o blocking the current task is my request.
>
> >
> > > If you wish to improve how the entry point should be handled in the future, that can be done separately. I am not sure why I need to hold off my activity or do things by your means.
> > >
> >
> > Improving probably means breaking an ABI compatibility in the future. That's not a good idea.
>
> [Subrata] I don't understand what makes you feel that any improvement means breaking things. We can make this approach of CBFS attribute deprecated as and when the multiple entry point approach is ready. multiple entry point approach can be designed such way that it won't cause any issue in existing 32bit/64bit entry call.
>
> >
> > > As I stated earlier, I will provide the libpayload 64-bit support code so you can examine how the entry point would appear. I ask that you do not combine this effort with unified payload. Unified payload is still in the planning stages, but enabling 64-bit support in coreboot/payload has business value; thus, the priority should not be the same at your end as it is at mine.
> >
> > The unified payload stuff indeed should not trump your business needs and I'm not suggesting to wait for that to happen. However I do think that thoughtful design and discussion needs to happen around the introduction of a ABI, before moving forward. In my opinion that should be a priority before merging something that works.
>
> [Subrata] I don't follow why you need to introduce a new ABI. The 64-bit calling convention is defined and as mentioned the x86_64 libpayload work has also completed where we are able to jump into head.S in 64-bit.
>
> Sorry, I might be missing but are you suggesting to implement 64-bit libpayload before implementing the long mode entry code ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/81960?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ic5e6f0af11c05e8b075b8c20880c012747a1df9b
Gerrit-Change-Number: 81960
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 18 Apr 2024 18:01:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Julius Werner.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81964?usp=email )
Change subject: util/cbfstool: Add --64 option for 64-bit payload stitching
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> > > Why not parse the ELF for architecture?
> > >
> > > Also it's confusing/meaningless for !x86 payloads.
> >
> > can you please elaborate a bit your review comments ?
> >
> > for sure, I should have guarded those against x86 arch alone. But are you suggesting not to use any cmdline (like --64) and use ELF arch to know if the payload is 64-bit binary and then insert specific magic data into the header ?
>
> I think you only need a cmdline if you cannot figure it out differently. OTOH the ELF does not conclusively say something about the entry point. For other type of payloads like flat binaries it's not possible to know this either I suppose.
32-bit depthcharge
```
readelf -h /build/rex/firmware/rex/depthcharge/depthcharge.elf
ELF Header:
Magic: 7f 45 4c 46 01 01 01 00 00 00 00 00 00 00 00 00
Class: ELF32
Data: 2's complement, little endian
Version: 1 (current)
OS/ABI: UNIX - System V
ABI Version: 0
Type: EXEC (Executable file)
Machine: Intel 80386
```
64-bit depthcharge
```
readelf -h /build/rex/firmware/rex/depthcharge/depthcharge.elf
ELF Header:
Magic: 7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00
Class: ELF64
Data: 2's complement, little endian
Version: 1 (current)
OS/ABI: UNIX - System V
ABI Version: 0
Type: EXEC (Executable file)
Machine: Advanced Micro Devices X86-64
Version: 0x1
```
I believe in that case, I don't need any cmdline.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81964?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I41562041d6c09869c7966ea31503f002ca1caefa
Gerrit-Change-Number: 81964
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 18 Apr 2024 17:55:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Julius Werner, Jérémy Compostella, Kapil Porwal, Subrata Banik.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81960?usp=email )
Change subject: arch/x86: Enable long mode entry into payload for x86_64 support
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
Subrata, please slow down. It's not just about implementing it and making it
work. coreboot and the payload should be independent and interchangeable. If
you want to create a 64-bit payload that can't be loaded by a current core-
boot (or one from the release branches?) for no obvious reason, that's ok, but
then you don't have to do it upstream. I would rather see it upstream, though.
Adding a 64-bit ABI and handover is something we have to do anyway, for X86S.
So any effort in that direction would be really appreciated, as long as we
don't unnecessarily sarcifice compatibility.
The last handover convention worked for what? 20 years, maybe? Please see that
as a goal for a 64-bit version. We should design and discuss something that can
last 20 years without binary incompatibilities. And then implement it. In the
mean time, you can use the old convention as long as your CPU supports protected
mode.
Beside strengthening the ecosystem, compatibility is also good for other reasons.
For instance, when someone debugs a regression and doesn't know if the problem
is in the payload or in coreboot. In the past it was usually possible to try
a new coreboot with an old payload build and vice versa.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81960?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ic5e6f0af11c05e8b075b8c20880c012747a1df9b
Gerrit-Change-Number: 81960
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 18 Apr 2024 17:51:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81897?usp=email )
Change subject: Haswell NRI: Implement fast boot path
......................................................................
Patch Set 1: Code-Review-1
(1 comment)
Patchset:
PS1:
Something seems wrong in the fast boot path, I can reach the payload but booting from a SATA SSD is extremely slow (and probably hangs).
--
To view, visit https://review.coreboot.org/c/coreboot/+/81897?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I06f6cd39ceecdca104fae89159f28e85cf7ff4e6
Gerrit-Change-Number: 81897
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 18 Apr 2024 17:48:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Subrata Banik.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81968?usp=email )
Change subject: libpayload: Add initial patch or 64-bit compilation
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Why duplicate the x86 code into x64? Why not capture the differences with preprocessor as done in coreboot?
--
To view, visit https://review.coreboot.org/c/coreboot/+/81968?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I69fda47bedf1a14807b1515c4aed6e3a1d5b8585
Gerrit-Change-Number: 81968
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Comment-Date: Thu, 18 Apr 2024 17:47:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Julius Werner, Subrata Banik.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81964?usp=email )
Change subject: util/cbfstool: Add --64 option for 64-bit payload stitching
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> > Why not parse the ELF for architecture?
> >
> > Also it's confusing/meaningless for !x86 payloads.
>
> can you please elaborate a bit your review comments ?
>
> for sure, I should have guarded those against x86 arch alone. But are you suggesting not to use any cmdline (like --64) and use ELF arch to know if the payload is 64-bit binary and then insert specific magic data into the header ?
I think you only need a cmdline if you cannot figure it out differently. OTOH the ELF does not conclusively say something about the entry point. For other type of payloads like flat binaries it's not possible to know this either I suppose.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81964?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I41562041d6c09869c7966ea31503f002ca1caefa
Gerrit-Change-Number: 81964
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Thu, 18 Apr 2024 17:37:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment