Upl #10570
Draft
Upl #10570
+279
−199
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The fields in this HOB are UINT8s, so print them as such. This avoids printing invalid/undefined memory, and confusing developers. Signed-off-by: Benjamin Doron <[email protected]>
For coreboot, it's important we drop the unnecessary spaces in the FIT's FDT as it compares whole strings. EDK2 won't mind, it compares with AsciiStrnCmp(). Signed-off-by: Benjamin Doron <[email protected]>
Drop some phase-scoped library definitions that are already defined at the phase-agnostic arch-scope. The most important change here is moving the SizeOfIoSpace PCD definition to an IA32 and X64 scope. Otherwise, PciHostBridgeDxe throws asserts or strange errors for a 0x0-0x0 IO space. The above still needs to be implemented for other architectures! Also, move PcdDxeIplSwitchToLongMode out of the X64 scope, where it makes no sense. The only reason this ever worked is because it's MdeModule's default too. Signed-off-by: Benjamin Doron <[email protected]>
These are unused references. I believe that leaving these in was confusing the linker, causing `ud2` to be emitted into the code. Signed-off-by: Benjamin Doron <[email protected]>
This is a "upl-custom" parameter. It's not even specced, it can't be required. Signed-off-by: Benjamin Doron <[email protected]>
FDTs are a structured data format, where order should not be relevant. When encountering the "reserved-memory" block, don't just increase the depth and accept any internal "memory@" node inside (the default behaviour). That would cause imminent exceptions. Instead, start skipping over any node with a depth greater than this. Signed-off-by: Benjamin Doron <[email protected]>
Again, FDTs are not meant to have strict orders. Encounter the GMA string and PCI RB node number in any order, then handle them at the end. Signed-off-by: Benjamin Doron <[email protected]>
According to the spec, "isa" is under the root. Also, serial ports can be purely memory-mapped, in which case they're also under the root. Signed-off-by: Benjamin Doron <[email protected]>
Signed-off-by: Benjamin Doron <[email protected]>
Per the discussion in the UPL meeting on December 10, the load property is for the FIT image, not whole FIT binary. To support Platform Init implementations that load images individually, we fix this discrepancy. However, EDK2-based implementations load the entire image at once. Therefore, account for this when using the "load" property. Signed-off-by: Benjamin Doron <[email protected]>
While Platform Init implementations are recommended to implement "pci-rb" support - it's necessary, I believe for PCIe segmenting - we do have a sufficiently capable PCI driver stack that it's not always required. However, the assumption that these nodes will be there, that GUID HOBs will be initialised, leads to a NULL pointer exception if they're not. So, fix this. This also contains another enhancement, removing node order effects from parsing: we defer parsing any "pci-rb" nodes until the end, by which point we know the parameters that need to be passed in as arguments. Signed-off-by: Benjamin Doron <[email protected]>
While EDK2-based implementations of UPL Platform Init copy the entire FIT binary into memory, the coreboot implementation loads entries in the images node into memory individually. This means that the assumption that all FVs directly follow each other is incorrect. It's also assumed that payloads don't need access to their FIT header. Therefore, add support for parsing `/options/upl-images@<addr>/image`. Signed-off-by: Benjamin Doron <[email protected]>
Follow the UPL spec as it converges on the FIT spec's definition. Signed-off-by: Benjamin Doron <[email protected]>
DNM: This is not the way to handle it! Better to stash addr-cells and size-cells, and have a 'FDT native length to CPU native endianness' function, which take one of these as an additional argument, calls the relevant FDT getter, and possibly increments the pointer if necessary. Signed-off-by: Benjamin Doron <[email protected]>
Update to version 1.7.2 which has setprop_bool(). Take from https://github.com/dgibson/dtc.git commit: 18f4f30 ("build: fix -Dtools=false build") Signed-off-by: Simon Glass <[email protected]>
It is simpler to use the provided Fdt class rather than call each libfdt
function directory. Update the initial FIT-creation to use this
approach.
The existing code has a few bugs where one more byte is written from a
property than exists. For example:
Fdt.setprop(ParentNode, 'compression', bytes('none', 'utf-8'),
len('none') + 1)
sets the length of the property to one larger than the length of the
string, relying on Python to have a nul byte there. Fix these by using
the setprop_str() method instead.
Signed-off-by: Simon Glass <[email protected]>
Tianocore requires its other two images to be loaded, so mention this in the configuration node. Signed-off-by: Simon Glass <[email protected]>
The Tianocore image is firmware, not flat_binary, so update it. Tianocore requires its other two images to be loaded, so mention this in the configuration node. Signed-off-by: Simon Glass <[email protected]>
The Tianocore image is probably best classed as an 'efi' Operating System. Add the 'os' property since this is required. Signed-off-by: Simon Glass <[email protected]>
At present Tianocore is started as a 32-bit image, even on 64-bit machines. So for now, set the architecture accordingly. Note that FIT uses "i386" rather than "x86" Signed-off-by: Simon Glass <[email protected]>
When developing this script it is helpful to get a full traceback when something goes wrong. Raise an exception to ensure this. Signed-off-by: Simon Glass <[email protected]>
This property is an offset from the end of the FIT structure, not the start of the file. Correct the calculations. Signed-off-by: Simon Glass <[email protected]>
Add the 'arch' property since this is required.
This shows a hang with the payload: python UefiPayloadPkg/UniversalPayloadBuild.py -t GCC5 --Fit -a IA32 Using U-Boot from here: https://ci.u-boot.org/u-boot/u-boot/-/commits/upl $ ./scripts/build-qemu.sh -a x86 -w -u /home/sglass/dev/edk2/Build/UefiPayloadPkgIA32/UniversalPayload.fit -Brs Formatting 'upl.img', fmt=raw size=8388608 mkfs.fat 4.2 (2021-01-31) Copying upl.fit Running qemu-system-i386 -drive if=virtio,file=upl.img,format=raw,id=hd0 -display none -serial mon:stdio U-Boot 2025.01-rc3-00318-g3f2c7280549e-dirty (Dec 27 2024 - 07:50:54 +1300) CPU: QEMU Virtual CPU version 2.5+ DRAM: 512 MiB Core: 21 devices, 14 uclasses, devicetree: separate, universal payload active Loading Environment from nowhere... OK Model: QEMU x86 (I440FX) Net: eth_initialize() No ethernet found. Hit any key to stop autoboot: 0 3612672 bytes read in 5 ms (689.1 MiB/s) addr 100000 calling upl_create() upl_add_graphics() returned 0 - done upl_create() data 101000 size e000 upl_add_fit 0 calling oftree_new() oftree_new() created tree at 1ed1b428 tree 1ed1b428 ser compat 16550 skip_existing 1 Working FDT set to 1ed1b428 / { #size-cells = <0x00000001>; #address-cells = <0x00000001>; framebuffer@0xd0000000 { format = "a8b8g8r8"; stride = <0x00001400>; height = <0x00000400>; width = <0x00000500>; reg = <0xd0000000 0x00500000>; compatible = "simple-framebuffer"; }; isa { #address-cells = <0x00000002>; #size-cells = <0x00000001>; compatible = "isa"; serial { access-type = "io"; reg-io-width = <0x00000001>; reg = <0x000003f8 0x00000008>; current-speed = <0x0001c200>; clock-frequency = <0x001c2000>; compatible = "ns8250"; }; }; chosen { stdout-path = "/isa/serial"; }; options { upl-images { conf-offset = <0x00000330>; image@801000 { description = "Uefi Universal Payload"; reg = <0x00801000 0x0000e000>; }; }; upl-params { acpi-nvs-size = <0x00000000>; addr-width = <0x00000024>; bootmode = "default"; compatible = "upl"; }; }; }; img 1ed1b3b0 load 801000 entry 805cd4 count 1 00101000: 4d 5a 00 00 00 00 00 00 00 00 00 00 00 00 00 00 MZ.............. loading from 101000 to 801000 size e000 Using 'conf-1' configuration Trying 'tianocore' firmware subimage Description: Uefi Universal Payload Created: 2024-12-26 20:05:55 UTC Type: Firmware Compression: uncompressed Data Start: 0x00101000 Data Size: 57344 Bytes = 56 KiB Architecture: Unknown Architecture OS: EFI Firmware Load Address: 0x00801000 Loading firmware from 0x00101000 to 0x00801000 Trying 'uefi-fv' loadables subimage Description: UEFI Firmware Volume Created: 2024-12-26 20:05:55 UTC Type: Firmware Compression: uncompressed Data Start: 0x00110000 Data Size: 3178496 Bytes = 3 MiB Architecture: Unknown Architecture OS: EFI Firmware Load Address: 0x0080f000 Loading loadables from 0x00110000 to 0x0080f000 Trying 'bds-fv' loadables subimage Description: BDS Firmware Volume Created: 2024-12-26 20:05:55 UTC Type: Firmware Compression: uncompressed Data Start: 0x00418000 Data Size: 368640 Bytes = 360 KiB Architecture: Unknown Architecture OS: EFI Firmware Load Address: 0x00b17000 Loading loadables from 0x00418000 to 0x00b17000 UPL: handoff at 1ed1b428 size 1000 Starting at 805cd4 ... 00805cd4: 55 89 e5 57 56 53 81 ec 6c 01 00 00 8b 7d 08 e8 U..WVS..l....}.. 64-bit entry...Hi... 1ED1B428 p 80ACAC 0 (hangs) Signed-off-by: Simon Glass <[email protected]>
|
PR can not be merged due to conflict. Please rebase and resubmit |
github-actions
bot
added
the
impact:breaking-change
| @@ -54,6 +54,7 @@ def BuildConfNode(Fdt, ParentNode, MultiImage): | |||
|
|
|||
| Fdt.setprop(ConfNode1, 'require-fit', b'', 0) | |||
| Fdt.setprop_str(ConfNode1, 'firmware', 'tianocore') | |||
| Fdt.setprop_str(ConfNode1, 'loadables', 'uefi-fv\0bds-fv') | |||
There was a problem hiding this comment.
This is a good idea, but we should do it properly by iterating MultiImage in BuildFitImage() below
| @@ -130,10 +130,10 @@ def BuildUniversalPayload(Args): | |||
| BuildDir = os.path.join(os.environ['WORKSPACE'], os.path.normpath("Build/UefiPayloadPkg{}").format (Args.Arch)) | |||
| if Args.Arch == 'X64': | |||
| BuildArch = "X64" | |||
| FitArch = "x86_64" | |||
| FitArch = "i386" | |||
There was a problem hiding this comment.
No, this is a coreboot-specific point. coreboot starts its payloads in 32-bit on x86, without condition on what coreboot is compiled as. So, coreboot will build UPL passing in -a IA32. EDK2-based Platform Init might call UPL already in long mode, or it might do a jump from their 32-bit PEI, to our 32-bit SEC, and let us do the mode switch.
U-Boot should build with -a IA32 or -a X64 depending on the bitness of the instructions right before you jump.
| @@ -63,6 +63,7 @@ def BuildFvImageNode(Fdt, InfoHeader, ParentNode, DataOffset, DataSize, Descript | |||
| Fdt.setprop_str(ParentNode, 'project', 'tianocore') | |||
| Fdt.setprop_str(ParentNode, 'arch',Arch) | |||
| Fdt.setprop_str(ParentNode, 'type', 'firmware') | |||
| Fdt.setprop_u64(ParentNode, 'load', InfoHeader.LoadAddr + DataOffset - InfoHeader.DataOffset) | |||
There was a problem hiding this comment.
FVs aren't place-specific. Do we need a load property if we don't care?
There was a problem hiding this comment.
This is going to break PayloadLoaderPeim and coreboot. If the changes have to be made, I'll likely be making coreboot's, but please sync this with FitPayloadLoaderPeim. This is also getting hard to read, (well, harder, after what I did to load) and I'm not sure if we can avoid that
| @@ -61,8 +61,11 @@ def BuildFvImageNode(Fdt, InfoHeader, ParentNode, DataOffset, DataSize, Descript | |||
| Fdt.setprop_u32(ParentNode, 'data-offset', DataOffset) | |||
| Fdt.setprop_str(ParentNode, 'compression', 'none') | |||
| Fdt.setprop_str(ParentNode, 'project', 'tianocore') | |||
| Fdt.setprop_str(ParentNode, 'arch',Arch) | |||
| if InfoHeader.Arch is not None: | |||
There was a problem hiding this comment.
Can this or the below ever be None? Also, arch's value is the same, as this is what the caller passed in
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Changes to bring UPL in line with the spec and make it work with U-Boot
How This Was Tested
Using U-Boot branch 'upl' at sjg.u-boot.org
./scripts/build-qemu.sh -a x86 -w -u ~dev/edk2/Build/UefiPayloadPkgIA32/UniversalPayload.fit -rs
Integration Instructions
N/A