Upl #10570
Draft
Upl #10570
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.
|
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
Drop the space at the end of the 'Found options node' message. Provide the address of the CHobList and produce a message if it is not found. Signed-off-by: Simon Glass <[email protected]>
There is no 'name' property in the spec. Use the description instead, until we can decide what to do within the spec. Signed-off-by: Simon Glass <[email protected]>
This code uses %lx to print a 32-bit value, which is not correct. Fix this and tidy up a missing newline and extra space. Signed-off-by: Simon Glass <[email protected]>
The data-offset property holds the offset of the data measured from the end of the FIT metadata, i.e. the totalsize property. There doesn't seem to be a way to read this at present. For now, assume a size of 4K.
EDK2 boots OK from U-Boot's qemu-x86 build (32-bit, using the IA32 architecture). Signed-off-by: Simon Glass <[email protected]>
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