-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Simpler memlayout #23
Conversation
dd13ef1
to
2065cce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand some details, but seems to make sense and to work fine.
@SergiiDmytruk can you check if this doesn't break QEMU and mark this PR as approved? |
@krystian-hebel Sure, however some things on QEMU branch are being questioned (https://review.coreboot.org/c/coreboot/+/57082/12) and those comments seem applicable to this PR as well (extra stuff in ramstage). Please take a look at the review, maybe you'll want to apply suggested changes here as well or know why they are wrong. I'll try to follow reviewer's comments and see if everything still works in QEMU. |
If changes from here will work with QEMU with !RAMSTAGE half removed then it can be simplified too. All of those comments come from lack of understanding HRMOR, so we may need to add some documentation on that, but I'd start with testing before doing anything with comments from gerrit. If we can get rid of that conditional it won't hurt to have more sections defined than necessary, I think Arthur's comment was just mentioning that we don't have to explicitly declare them if they are not used. |
Right, I was wondering why he didn't say anything about |
@krystian-hebel What modifications should suffice to make it work in QEMU? Changing
|
I think this should be enough. What is your command line for starting QEMU? Does it have separate bootblock and CBFS? |
It's all in one ROM-file:
|
@SergiiDmytruk can you push your current state of QEMU platform to new branch? I'd like to debug it and it would be best if we have exactly the same code base. |
@krystian-hebel Pushed as |
@SergiiDmytruk I tracked down the issue and added comment about how to fix it in https://review.coreboot.org/c/coreboot/+/57082. Other than that, in bootblock_crt0.S you should move code for saving FDT somewhere after HRMOR is modified and cache (re)initialized (but before %r3 is overwritten, or change all %r3 usages to another register) or Skiboot won't work. We may also use one of SPRs for saving this pointer (HSPRG?) - their content would be lost on hardware during deadman loop, but it doesn't have anything to pass in the first place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine in QEMU after fixing QEMU changes as suggested.
Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>
Thanks to 'wait' instruction and exception definition on POWER it is possible to use Decrementer Exception without actually implementing Decrementer Interrupt handler. This greatly simplifies previous implementation and makes it available to use in all stages. Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com> Change-Id: I74cc95a503d4f3642609eff3b781f0b1c3168a3d
Apparently this isn't a requirement for dead man loop. Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com> Change-Id: I8f90addfe69b280a486da1027c2be9d5e0e45406
d2ec14c
to
cc39cf7
Compare
No description provided.