Skip to content
This repository has been archived by the owner on Nov 28, 2023. It is now read-only.

Add boot_argument(), returns the value an a* register had at boot time #94

Closed
wants to merge 4 commits into from

Conversation

SimonSapin
Copy link
Contributor

Fixes #92

With this PR, the code below succeeds in QEMU and Fdt can be used to find the address of the simulated UART that prints to stdout on the host.

use fdt::Fdt;

pub fn parse() -> Fdt<'static> {
    /// https://doc.coreboot.org/arch/riscv/index.html#stage-handoff-protocol
    const FDT_ARG_POSITION: usize = 1;
    let ptr = riscv_rt::boot_argument(FDT_ARG_POSITION).unwrap() as *const u8;
    unsafe { Fdt::from_ptr(ptr).expect("Failed to parse FDT") }
}

@SimonSapin
Copy link
Contributor Author

This unconditionally reserves 8 * XLEN in .data. Hopefully that’s considered cheap enough to have even when not used. 8 is the total number of argument registers that exist, but since QEMU / coreboot only use 3 we could also reduce to that.

almindor
almindor previously approved these changes Jun 25, 2022
Copy link
Contributor

@almindor almindor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems sensible to me. I'll wait for @Disasm to approve as well tho.

Copy link
Member

@Disasm Disasm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work well in multi-core environments since multiple cores will execute the same code at the same time. If different argument values are passed on different cores, we will get garbage in the end.

@Disasm
Copy link
Member

Disasm commented Jun 25, 2022

I checked binary sizes and this PR adds 44 bytes of code for riscv32imac-unknown-none-elf while passing a0..a2 to main adds 12 bytes

This avoids a data race on multicore
@SimonSapin
Copy link
Contributor Author

I’d added a branch on hartid that I believe should fix the multicore issue.

What does the source code look like for passing to main in your comparison?

@Disasm
Copy link
Member

Disasm commented Jun 25, 2022

I created a draft PR for this: #95. Currently the proc-macro part is missing.

STORE a5, 5*REGBYTES(t1)
STORE a6, 6*REGBYTES(t1)
STORE a7, 7*REGBYTES(t1)
fence rw, rw
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not confident about this (single) fence, or about using plain reads in fn boot_arguments.

At first I started emulating the behavior of AtomicUsize: https://rust.godbolt.org/z/3djWjhPcG. But why is the fence instruction placed before stores and after loads? I’d expected fences to be between things in order to separate them.

On the Rust size I ended up with this, since AtomicUsize is disabled on targets without the A extension. The portable-atomic crate would be another option for doing the same.

/// Simulate AtomicUsize::load(Ordering::Acquire)
unsafe fn atomic_load(ptr: *const usize) -> usize {
    let value;

    // Only lw v.s. ld changes but asm! wants a string literal
    #[cfg(target_pointer_width = "32")]
    core::arch::asm!(
        "fence r, rw",
        "lw {value}, 0({ptr})",
        ptr = in(reg) ptr,
        value = out(reg) value,
    );
    #[cfg(not(target_pointer_width = "32"))]
    core::arch::asm!(
        "fence r, rw",
        "ld {value}, 0({ptr})",
        ptr = in(reg) ptr,
        value = out(reg) value,
    );
    value
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Position of fence instructions depends on the ordering. With SeqCst you get fences before and after the load.

@SimonSapin
Copy link
Contributor Author

Closing in favor of #95

@SimonSapin SimonSapin closed this Jun 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessing parameters passed in register by the previous boot stage
3 participants