Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing important FSP Hob structs header files for EagleStreamFspBinPkg #102

Open
johnnylinwiwynn opened this issue Feb 8, 2024 · 17 comments

Comments

@johnnylinwiwynn
Copy link

EagleStreamFspBinPkg/Include only provides UPD header files for FSP input interface for the bootloader, but doesn't provide any important Hob struct header files for the FSP output interface. Running
"find -iname hob.h" I can see there are quite many Hob header files for Client FSP, such as
/AlderLakeFspBinPkg/Client/AlderLakeS/Include/FirmwareVersionInfoHob.h
./AlderLakeFspBinPkg/Client/AlderLakeS/Include/MemInfoHob.h
./AlderLakeFspBinPkg/Client/AlderLakeS/Include/FspInfoHob.h
./AlderLakeFspBinPkg/Client/AlderLakeS/Include/SmbiosCacheInfoHob.h

Can Intel please also release the needed Hob header files for EagleStreamFspBinPkg? For example, these are the Hob header files for WW43 2022 EGS FSP:
https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/main/src/vendorcode/intel/fsp/fsp2_0/sapphirerapids_sp/

Thanks.

@65a
Copy link

65a commented Mar 15, 2024

Trying the current FSP with coreboot HEAD is completely broken on the board I am porting (Tyan S5652) and I think this is part of the reason. Please release updated headers, currently I'm seeing a number of strange issues that may be related to misinterpreted HOB blocks, including the inability to run option ROMs or boot a Linux payload, despite making it to EOP.

@johnnylinwiwynn
Copy link
Author

johnnylinwiwynn commented Mar 16, 2024

Some discussion on the change you can reference from
https://review.coreboot.org/c/coreboot/+/80360
At least with UEFI payload I can boot up OS.

https://github.com/johnnylinwiwynn/Intel_AC_OSF
FYI

@65a
Copy link

65a commented Mar 16, 2024

@johnnylinwiwynn two questions (I actually had read some of that gerrit change's comments trying to get things working):

  1. What is a TOCTOU mitigated FSP? I don't think this board has bootguard enforcement (I don't think I would get this far), but ME has been placed in the MFG-mode.
  2. How did you bring up graphics (or did you let OS bring up graphics)?
    I'll give it a try, thanks!

@65a
Copy link

65a commented Mar 16, 2024

Looking at my romstage setup and changing some things to match Archer city seems to have fixed things, I believe this was either using XAPIC by default, or configuring CXL, I was able to boot a bzImage. Thanks for that!

@LeanSheng
Copy link

LeanSheng commented Mar 16, 2024

@65a great to hear that it got resolved on your side!
I am also working on EGS and next generation Intel server platforms with coreboot.
Actually there is a slack group for Open Source Firmware, feel free to join:
https://join.slack.com/t/osfw/shared_invite/zt-2eyud9b4k-EulKQ3MwjTIAn0VAi_J_uA

You can then go to the coreboot channel and start discussions. There are many experienced and helpful folks there from all over the world :)

If not you could also drop me an email if you need something related:
[email protected]

@johnnylinwiwynn
Copy link
Author

johnnylinwiwynn commented Mar 17, 2024

@johnnylinwiwynn two questions (I actually had read some of that gerrit change's comments trying to get things working):

  1. What is a TOCTOU mitigated FSP? I don't think this board has bootguard enforcement (I don't think I would get this far), but ME has been placed in the MFG-mode.

  2. How did you bring up graphics (or did you let OS bring up graphics)?

I'll give it a try, thanks!

For some unknown reasons FSP needs to revert a commit related to a TOCTOU security pcd configuration otherwise Linux payload will reset and cannot boot, but UEFI payload is not affected by this. The FSP pcd is Intel NDA. I can share more if you have Intel FSP source code NDA.

I haven't tried to enable graphics since we use headless server only, but the community and firmware vender like 9elements, SysPro Consulting could help.

@65a
Copy link

65a commented Mar 17, 2024

I do not have an NDA, but thanks to your help I was able to boot a linux (also SeaBIOS but no keyboard, and u-boot with some issues) payload. I had success with XAPIC and late X2APIC (Called "XAPIC then X2APIC") mode, originally I was getting reboots in the default mode, but it may have also been some changes to try to configure CXL. I'll close my other issue , but it would still be nice if the FSP headers and Coreboot matched.

@65a
Copy link

65a commented Apr 19, 2024

As far as I can tell, Linux payload, and perhaps others, will only boot if coreboot is set to wipe memory. This results in one logged null pointer exception, but no other issues.

@65a
Copy link

65a commented Apr 19, 2024

TME is not working, as the policy part of the MSR is not programmed correctly. This may be a HOB/UPD issue, as copying Alderlake's coreboot init for TME doesn't produce any change.

@LeanSheng
Copy link

This is bad, let me try to reach out to my intel contact to fix this.

@nate-desimone
Copy link
Contributor

For some unknown reasons FSP needs to revert a commit related to a TOCTOU security pcd configuration otherwise Linux payload will reset and cannot boot, but UEFI payload is not affected by this. The FSP pcd is Intel NDA. I can share more if you have Intel FSP source code NDA.

Intel has root caused the issue with the TOCTOU mitigation & coreboot. A fix is currently under development and will be included in an upcoming FSP release.

@65a
Copy link

65a commented Jun 14, 2024

Great news! For what it's worth, I think the headers coreboot uses today do not match, especially the memory hob. For instance, the binary dump of the hob data shows things like the module serial numbers, but coreboot doesn't see this, and most DIMMs have the present boolean set to false. Another example is that SNC is apparently enabled, even though it shouldn't be (wasn't requested) and TME is configured improperly. This seems like the structs on the coreboot side do not match the actual hob, so there's a memory safety issue. Publishing the actual HOB structs as returned by the FSP would help in these cases, but glad you are also looking at the reset/TOCTOU issue.

@felixsinger
Copy link

For some unknown reasons FSP needs to revert a commit related to a TOCTOU security pcd configuration otherwise Linux payload will reset and cannot boot, but UEFI payload is not affected by this. The FSP pcd is Intel NDA. I can share more if you have Intel FSP source code NDA.

Intel has root caused the issue with the TOCTOU mitigation & coreboot. A fix is currently under development and will be included in an upcoming FSP release.

Amazing. Thank you!

@LeanSheng
Copy link

LeanSheng commented Oct 22, 2024

For some unknown reasons FSP needs to revert a commit related to a TOCTOU security pcd configuration otherwise Linux payload will reset and cannot boot, but UEFI payload is not affected by this. The FSP pcd is Intel NDA. I can share more if you have Intel FSP source code NDA.

Intel has root caused the issue with the TOCTOU mitigation & coreboot. A fix is currently under development and will be included in an upcoming FSP release.

@nate-desimone It has been 4 months now per your last update. Any update? :)

@65a
Copy link

65a commented Nov 27, 2024

+1, still very interested in updated headers and any fixed FSP. This thread seems to be now a mixture of TOCTOU and Headers discussion, I want to focus on the headers (For TOCTOU mitigation mitigation, I had success enabling Coreboot's CONFIG_SECURITY_CLEAR_DRAM_ON_REGULAR_BOOT, which is kinda weird).

Public headers are old problem:

The current headers available publicly are subtly wrong and create a memory safety risk when used with the public FSP. What I mean by that is:

Coreboot code:

printk(BIOS_DEBUG,
           "^^^  MEMMAP_SOCKET: %ld, ChannelDevice: %ld, MEMMAP_DIMM_DEVICE_INFO_STRUCT: %ld\n",
           sizeof(MEMMAP_SOCKET), sizeof(struct ChannelDevice),
           sizeof(MEMMAP_DIMM_DEVICE_INFO_STRUCT));
    printk(BIOS_DEBUG, "^^^  Element Offset: %ld\n",
           offsetof(SYSTEM_MEMORY_MAP_HOB, Element));
    printk(BIOS_DEBUG, "^^^  Socket Offset: %ld\n",
           offsetof(SYSTEM_MEMORY_MAP_HOB, Socket));
    printk(BIOS_DEBUG, "^^^  ChannelInfo Offset: %ld\n",
           offsetof(MEMMAP_SOCKET, ChannelInfo));
    printk(BIOS_DEBUG, "^^^  DimmInfo Offset: %ld\n",
           offsetof(struct ChannelDevice, DimmInfo));
    printk(BIOS_DEBUG, "^^^  DimmSize Offset: %ld\n",
           offsetof(struct DimmDevice, DimmSize));

    for (int s = 0; s < MAX_SOCKET; ++s) {
        if (!hob->Socket[s].SocketEnabled)
            continue;
        for (int ch = 0; ch < MAX_CH; ++ch) {
            if (!hob->Socket[s].ChannelInfo[ch].Enabled)
                continue;
            for (int dimm = 0; dimm < MAX_DIMM; ++dimm) {
                if (!hob->Socket[s].ChannelInfo[ch].DimmInfo[dimm].Present)
                    continue;
                printk(BIOS_DEBUG,
                       "\tsocket: %d, ch: %d, dimm: %d, enabled: %d, DimmSize: 0x%x\n",
                       s, ch, dimm,
                       hob->Socket[s].ChannelInfo[ch].DimmInfo[dimm].Enabled,
                       hob->Socket[s].ChannelInfo[ch].DimmInfo[dimm].DimmSize);
            }
        }
    }

    printk(BIOS_DEBUG, "\tBiosFisVersion: 0x%x\n", hob->BiosFisVersion);
    printk(BIOS_DEBUG, "\tMmiohBase: 0x%x\n", hob->MmiohBase);

Prints the hob data based on the structs in the current publicly available header. On a (thankfully) booting system with 16 dimms and an HBM Xeon Max, this code prints (during last HOB handoff, earlier are no better):

================== MEMORY MAP HOB DATA ==================
hob: 0x777f5000, structure size: 0x7c2d
        lowMemBase: 0x0, lowMemSize: 0x20, highMemBase: 0x40, highMemSize: 0x17e0
        memSize: 0x1800, memFreq: 0x12c0
        NumChPerMC: 2
        SystemMemoryMapElement Entries: 5, entry size: 19
                memory_map 0, SocketId: 0x0, BaseAddress: 0x0, ElementSize: 0x20, Type: 0x1
                memory_map 1, SocketId: 0x0, BaseAddress: 0x40, ElementSize: 0x7e0, Type: 0x1
                memory_map 2, SocketId: 0x0, BaseAddress: 0x820, ElementSize: 0x800, Type: 0x1
                memory_map 3, SocketId: 0x0, BaseAddress: 0x1020, ElementSize: 0x400, Type: 0x1
                memory_map 4, SocketId: 0x0, BaseAddress: 0x1420, ElementSize: 0x400, Type: 0x5
^^^  MEMMAP_SOCKET: 5845, ChannelDevice: 270, MEMMAP_DIMM_DEVICE_INFO_STRUCT: 126
^^^  Element Offset: 142
^^^  Socket Offset: 7239
^^^  ChannelInfo Offset: 3685
^^^  DimmInfo Offset: 18
^^^  DimmSize Offset: 14
        BiosFisVersion: 0x0
        MmiohBase: 0x0

Whatever bit happens to be at hob->Socket[s].SocketEnabled happens to be 0, I guess thankfully as it prevents further reading of the HOB incorrectly. Happy to provide raw hexdumps of the HOB as well, but it's quite clear to me the struct sizes/offsets are wrong, and the fact it can even boot is a bit of a matter of luck.

Really hopeful that Intel can release a matching FSP and header set, and thank you!

@LeanSheng
Copy link

+1, still very interested in updated headers and any fixed FSP. This thread seems to be now a mixture of TOCTOU and Headers discussion, I want to focus on the headers (For TOCTOU mitigation mitigation, I had success enabling Coreboot's CONFIG_SECURITY_CLEAR_DRAM_ON_REGULAR_BOOT, which is kinda weird).

Public headers are old problem:

The current headers available publicly are subtly wrong and create a memory safety risk when used with the public FSP. What I mean by that is:

Coreboot code:


printk(BIOS_DEBUG,

           "^^^  MEMMAP_SOCKET: %ld, ChannelDevice: %ld, MEMMAP_DIMM_DEVICE_INFO_STRUCT: %ld\n",

           sizeof(MEMMAP_SOCKET), sizeof(struct ChannelDevice),

           sizeof(MEMMAP_DIMM_DEVICE_INFO_STRUCT));

    printk(BIOS_DEBUG, "^^^  Element Offset: %ld\n",

           offsetof(SYSTEM_MEMORY_MAP_HOB, Element));

    printk(BIOS_DEBUG, "^^^  Socket Offset: %ld\n",

           offsetof(SYSTEM_MEMORY_MAP_HOB, Socket));

    printk(BIOS_DEBUG, "^^^  ChannelInfo Offset: %ld\n",

           offsetof(MEMMAP_SOCKET, ChannelInfo));

    printk(BIOS_DEBUG, "^^^  DimmInfo Offset: %ld\n",

           offsetof(struct ChannelDevice, DimmInfo));

    printk(BIOS_DEBUG, "^^^  DimmSize Offset: %ld\n",

           offsetof(struct DimmDevice, DimmSize));



    for (int s = 0; s < MAX_SOCKET; ++s) {

        if (!hob->Socket[s].SocketEnabled)

            continue;

        for (int ch = 0; ch < MAX_CH; ++ch) {

            if (!hob->Socket[s].ChannelInfo[ch].Enabled)

                continue;

            for (int dimm = 0; dimm < MAX_DIMM; ++dimm) {

                if (!hob->Socket[s].ChannelInfo[ch].DimmInfo[dimm].Present)

                    continue;

                printk(BIOS_DEBUG,

                       "\tsocket: %d, ch: %d, dimm: %d, enabled: %d, DimmSize: 0x%x\n",

                       s, ch, dimm,

                       hob->Socket[s].ChannelInfo[ch].DimmInfo[dimm].Enabled,

                       hob->Socket[s].ChannelInfo[ch].DimmInfo[dimm].DimmSize);

            }

        }

    }



    printk(BIOS_DEBUG, "\tBiosFisVersion: 0x%x\n", hob->BiosFisVersion);

    printk(BIOS_DEBUG, "\tMmiohBase: 0x%x\n", hob->MmiohBase);

Prints the hob data based on the structs in the current publicly available header. On a (thankfully) booting system with 16 dimms and an HBM Xeon Max, this code prints (during last HOB handoff, earlier are no better):


================== MEMORY MAP HOB DATA ==================

hob: 0x777f5000, structure size: 0x7c2d

        lowMemBase: 0x0, lowMemSize: 0x20, highMemBase: 0x40, highMemSize: 0x17e0

        memSize: 0x1800, memFreq: 0x12c0

        NumChPerMC: 2

        SystemMemoryMapElement Entries: 5, entry size: 19

                memory_map 0, SocketId: 0x0, BaseAddress: 0x0, ElementSize: 0x20, Type: 0x1

                memory_map 1, SocketId: 0x0, BaseAddress: 0x40, ElementSize: 0x7e0, Type: 0x1

                memory_map 2, SocketId: 0x0, BaseAddress: 0x820, ElementSize: 0x800, Type: 0x1

                memory_map 3, SocketId: 0x0, BaseAddress: 0x1020, ElementSize: 0x400, Type: 0x1

                memory_map 4, SocketId: 0x0, BaseAddress: 0x1420, ElementSize: 0x400, Type: 0x5

^^^  MEMMAP_SOCKET: 5845, ChannelDevice: 270, MEMMAP_DIMM_DEVICE_INFO_STRUCT: 126

^^^  Element Offset: 142

^^^  Socket Offset: 7239

^^^  ChannelInfo Offset: 3685

^^^  DimmInfo Offset: 18

^^^  DimmSize Offset: 14

        BiosFisVersion: 0x0

        MmiohBase: 0x0

Whatever bit happens to be at hob->Socket[s].SocketEnabled happens to be 0, I guess thankfully as it prevents further reading of the HOB incorrectly. Happy to provide raw hexdumps of the HOB as well, but it's quite clear to me the struct sizes/offsets are wrong, and the fact it can even boot is a bit of a matter of luck.

Really hopeful that Intel can release a matching FSP and header set, and thank you!

@65a
We 9elements have been following this issue every month and work closely with Intel on it. Intel engineer has been working very hard internally for the past few month to resolve the process hurdle in order to get it resolved asap. Let me follow up with Intel on this again. If you urgently need this, reach out to my email [email protected], we could share the required files after signing NDA. We also got some fixes in FSP and trying to update fixes back to Intel.

@65a
Copy link

65a commented Nov 29, 2024

@LeanSheng Thank you for your kind offer. I will wait for the public release, as I am not interested in the NDA. Looking forward to when this is updated! Glad to hear Intel is working on it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants