ast2600: Allow for SPL as large as 64Kb - 512b

[klauskiwi]

Recent changes[1] to the Aspeed U-boot allows for the SPL image to occupy the entire 64KB parity-checked SRAM.

[1] - AspeedTech-BMC/u-boot@a62cf9f

This patch adds a --spl_extra_sram argument to socsec and resocsec that allows the max BL1 image size to occupy 64KB (including signature) in case these type of images are being used.

(Based on code originally from @amboar )

[klauskiwi]

FYI, on the OpenBMC u-boot, this is the change that allows AST2600 SPL images to be as large as 64KB - https://github.com/openbmc/u-boot/commit/39dcd8063165b6bcb6feb8ec7285a6f59bbe392f

I suspect that this change will go upstream, thus the suggestion to not put this behind a command-line conditional.

-Klaus

[amboar]

Ugh, the GitHub Android app made me comment on the commit so I didn't see your comment on the pull req @klauskiwi.

You really need to discuss the related u-boot configuration change in the commit message.

[amboar]

Also the final 512 bytes is for the signature, not the header. The default header location is at 0x20 (immediately after the exception vectors) and is 0x20 bytes in length

[klauskiwi]

This last force-push resolves @amboar comments

[amboar]

Okay, thanks for updating the commit message. I hadn't realised that Aspeed had merged the config change into their tree.

This change still has the issue that running the patched socsec against a u-boot with an old (or just intentionally different) configuration may break the boot. I'd still prefer a switch so the caller has to explicitly acknowledge that they know what they're doing with large u-boot binaries.

Unfortunately it's not easy to extract the stack location from the raw SPL binary (it's a constant referenced in cpu_init_cp15) so we're sort-of relying on the user to tell us what they need.

[klauskiwi]

Okay, thanks for updating the commit message. I hadn't realised that Aspeed had merged the config change into their tree.

This change still has the issue that running the patched socsec against a u-boot with an old (or just intentionally different) configuration may break the boot. I'd still prefer a switch so the caller has to explicitly acknowledge that they know what they're doing with large u-boot binaries.

Unfortunately it's not easy to extract the stack location from the raw SPL binary (it's a constant referenced in cpu_init_cp15) so we're sort-of relying on the user to tell us what they need.

Thanks Andrew. I have to confess that by the time I force-pushed this patch I haven't yet seen your comment on the commit itself. I'll address that by re-working your patch.

-Klaus

[klauskiwi]

@johnydhuang any additional comments? Can you merge?

[amboar]

Sorry, I was out yesterday and was unable to provide input.

Given that ASPEED have changed the u-boot config I think we should do the following:

1. Add the optional argument
2. The default behaviour enforces the <= 60KiB requirement if the optional argument isn't supplied
3. When the tool is run, issue a warning about defaults changing if the optional argument isn't supplied
4. Include it in the next SDK release
5. Change the default behaviour if the argument isn't supplied to allow the up to <= 64KiB - 512b
6. Include the updated default it in the following SDK release

Further, I've thought about the option name some more. I think --stack-intersects-verification-region={true|false} is accurate and doesn't depend on changeable behaviours of the SoC. I suggest we change to that.

Regarding 2. and 5. we can implement this with parser.add_argument('--stack-intersects-verification-region', type=bool, default=None, ...), and then check with if args.stack_intersects_verified_region is None: to issue the warning.

The warning should probably look something like:

WARNING: --stack-intersects-verification-region={true|false} must be specified to ensure forwards compatibility.

[klauskiwi]

I'm hoping all comments / concerns are now addressed with the latest force-push: https://github.com/AspeedTech-BMC/socsec/compare/6d2526943e084b77433c1f7a0e5c45d648bc585a..46415a0ab48996ce4340b083dd1f8572a03f4bfe

[amboar]

Once the comment I've made above is fixed it looks like it's good to go.

[klauskiwi]

Once the comment I've made above is fixed it looks like it's good to go.

done!

thanks for the thorough review