[PATCH 3/5] firmware: Add RW section offset in scratch

Himanshu Chauhan hchauhan at ventanamicro.com
Thu Jan 19 02:35:30 PST 2023 

On Thu, Jan 19, 2023 at 04:43:35AM +0000, Jessica Clarke wrote:
> On 19 Jan 2023, at 04:29, Himanshu Chauhan <hchauhan at ventanamicro.com> wrote:
> > 
> > On Wed, Jan 18, 2023 at 04:23:06PM +0000, Jessica Clarke wrote:
> >> On 9 Jan 2023, at 08:40, Himanshu Chauhan <hchauhan at ventanamicro.com> wrote:
> >>> 
> >>> Add the RW section offset, provided by _fw_rw_offset symbol,
> >>> to the scratch structure. This will be used to program
> >>> separate pmp entry for RW section.
> >>> 
> >>> Signed-off-by: Himanshu Chauhan <hchauhan at ventanamicro.com>
> >>> ---
> >>> firmware/fw_base.S        |  5 +++++
> >>> include/sbi/sbi_scratch.h | 24 ++++++++++++++----------
> >>> 2 files changed, 19 insertions(+), 10 deletions(-)
> >>> 
> >>> diff --git a/firmware/fw_base.S b/firmware/fw_base.S
> >>> index 3f622b3..ce1f782 100644
> >>> --- a/firmware/fw_base.S
> >>> +++ b/firmware/fw_base.S
> >>> @@ -298,6 +298,11 @@ _scratch_init:
> >>> 	sub	a5, t3, a4
> >>> 	REG_S	a4, SBI_SCRATCH_FW_START_OFFSET(tp)
> >>> 	REG_S	a5, SBI_SCRATCH_FW_SIZE_OFFSET(tp)
> >>> +
> >>> +	/* Store R/W section's offset in scratch space */
> >>> +	lla	a4, _fw_rw_offset
> >>> +	REG_S	a4, SBI_SCRATCH_FW_RW_OFFSET(tp)
> >>> +
> >> 
> >> You can’t use LLA for an absolute symbol, especially one whose value
> >> isn’t guaranteed to be within 2^31 of the LLA’s address. Use the GOT,
> >> use a constant pool (for compatibility with older assemblers) or do the
> >> subtraction in the code instead of the linker script.
> > 
> > This code segment is executed after the relocation. The _fw_rw_offset
> > symbol also gets relocated.
> 
> No it doesn’t. _fw_rw_offset is absolute. It’s not subject to
> relocation. In fact executing this *after* relocation would totally
> screw up FW_PIC binaries, because they’d include the run-time
> relocation offset in the calculation of _fw_rw_offset, but
> _fw_rw_offset is the offset from _fw_base. That is, if your relocation
> offset (FreeBSD calls this “relocbase”) is 1 GiB then suddenly
> _fw_rw_offset is regarded as 1 GiB + some small value from the start of
> OpenSBI’s image, which seems rather nonsensical, when it should be just
> that small value.
> 
> > Thus the symbol will always be in 2Gig range
> > unless the text and RO-data go beyond that size.
> 
> That’s not even guaranteed at link time if you have a base address
> beyond 2 GiB. It only works with the default base address of 2 GiB
> because this lla is in the text segment and thus at a smaller offset
> from the start of the file than _fw_rw_offset’s value. For example,
> with a base address of 2 GiB, a 4 KiB text segment, and, for argument’s
> sake, 32 uncompressed instructions before this lla in the text segment,
> _fw_rw_offset will have value 4 KiB, and the lla will be at 2 GiB + 32
> * 4 bytes = 2 GiB + 128 bytes, which is less than 2 GiB above 4 KiB.
> But if you bump the base address up to 3 GiB then the lla is at 3 GiB +
> 128 bytes, which is more than 2 GiB above 4 KiB.
> 
> Use the GOT or a constant pool.
>
GOT your point :). Thanks for catching it! Will send new version.

Regards
Himanshu
 
> Jess
> 
> > IMHO, I don't see a need
> > to go via GOT.
> > 
> > Regards
> > Himanshu
> > 
> >> Jess
> >> 
> >>> 	/* Store next arg1 in scratch space */
> >>> 	MOV_3R	s0, a0, s1, a1, s2, a2
> >>> 	call	fw_next_arg1
> >>> diff --git a/include/sbi/sbi_scratch.h b/include/sbi/sbi_scratch.h
> >>> index 40a3bc9..2966188 100644
> >>> --- a/include/sbi/sbi_scratch.h
> >>> +++ b/include/sbi/sbi_scratch.h
> >>> @@ -18,26 +18,28 @@
> >>> #define SBI_SCRATCH_FW_START_OFFSET		(0 * __SIZEOF_POINTER__)
> >>> /** Offset of fw_size member in sbi_scratch */
> >>> #define SBI_SCRATCH_FW_SIZE_OFFSET		(1 * __SIZEOF_POINTER__)
> >>> +/** Offset (in sbi_scratch) of the R/W Offset */
> >>> +#define SBI_SCRATCH_FW_RW_OFFSET		(2 * __SIZEOF_POINTER__)
> >>> /** Offset of next_arg1 member in sbi_scratch */
> >>> -#define SBI_SCRATCH_NEXT_ARG1_OFFSET		(2 * __SIZEOF_POINTER__)
> >>> +#define SBI_SCRATCH_NEXT_ARG1_OFFSET		(3 * __SIZEOF_POINTER__)
> >>> /** Offset of next_addr member in sbi_scratch */
> >>> -#define SBI_SCRATCH_NEXT_ADDR_OFFSET		(3 * __SIZEOF_POINTER__)
> >>> +#define SBI_SCRATCH_NEXT_ADDR_OFFSET		(4 * __SIZEOF_POINTER__)
> >>> /** Offset of next_mode member in sbi_scratch */
> >>> -#define SBI_SCRATCH_NEXT_MODE_OFFSET		(4 * __SIZEOF_POINTER__)
> >>> +#define SBI_SCRATCH_NEXT_MODE_OFFSET		(5 * __SIZEOF_POINTER__)
> >>> /** Offset of warmboot_addr member in sbi_scratch */
> >>> -#define SBI_SCRATCH_WARMBOOT_ADDR_OFFSET	(5 * __SIZEOF_POINTER__)
> >>> +#define SBI_SCRATCH_WARMBOOT_ADDR_OFFSET	(6 * __SIZEOF_POINTER__)
> >>> /** Offset of platform_addr member in sbi_scratch */
> >>> -#define SBI_SCRATCH_PLATFORM_ADDR_OFFSET	(6 * __SIZEOF_POINTER__)
> >>> +#define SBI_SCRATCH_PLATFORM_ADDR_OFFSET	(7 * __SIZEOF_POINTER__)
> >>> /** Offset of hartid_to_scratch member in sbi_scratch */
> >>> -#define SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET	(7 * __SIZEOF_POINTER__)
> >>> +#define SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET	(8 * __SIZEOF_POINTER__)
> >>> /** Offset of trap_exit member in sbi_scratch */
> >>> -#define SBI_SCRATCH_TRAP_EXIT_OFFSET		(8 * __SIZEOF_POINTER__)
> >>> +#define SBI_SCRATCH_TRAP_EXIT_OFFSET		(9 * __SIZEOF_POINTER__)
> >>> /** Offset of tmp0 member in sbi_scratch */
> >>> -#define SBI_SCRATCH_TMP0_OFFSET			(9 * __SIZEOF_POINTER__)
> >>> +#define SBI_SCRATCH_TMP0_OFFSET			(10 * __SIZEOF_POINTER__)
> >>> /** Offset of options member in sbi_scratch */
> >>> -#define SBI_SCRATCH_OPTIONS_OFFSET		(10 * __SIZEOF_POINTER__)
> >>> +#define SBI_SCRATCH_OPTIONS_OFFSET		(11 * __SIZEOF_POINTER__)
> >>> /** Offset of extra space in sbi_scratch */
> >>> -#define SBI_SCRATCH_EXTRA_SPACE_OFFSET		(11 * __SIZEOF_POINTER__)
> >>> +#define SBI_SCRATCH_EXTRA_SPACE_OFFSET		(12 * __SIZEOF_POINTER__)
> >>> /** Maximum size of sbi_scratch (4KB) */
> >>> #define SBI_SCRATCH_SIZE			(0x1000)
> >>> 
> >>> @@ -53,6 +55,8 @@ struct sbi_scratch {
> >>> 	unsigned long fw_start;
> >>> 	/** Size (in bytes) of firmware linked to OpenSBI library */
> >>> 	unsigned long fw_size;
> >>> +	/** Offset (in bytes) of the R/W section */
> >>> +	unsigned long fw_rw_offset;
> >>> 	/** Arg1 (or 'a1' register) of next booting stage for this HART */
> >>> 	unsigned long next_arg1;
> >>> 	/** Address of next booting stage for this HART */
> >>> -- 
> >>> 2.39.0
> >>> 
> >>> 
> >>> -- 
> >>> opensbi mailing list
> >>> opensbi at lists.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/opensbi
>

More information about the opensbi mailing list