Messages in this thread | | | Date | Wed, 2 Dec 2020 15:03:01 -0500 (EST) | From | Nicolas Pitre <> | Subject | Re: [PATCH] arch/riscv: enable XIP |
| |
On Wed, 2 Dec 2020, Vitaly Wool wrote:
> On Wed, Dec 2, 2020 at 7:06 PM Nicolas Pitre <nico@fluxnic.net> wrote: > > > > On Wed, 2 Dec 2020, Vitaly Wool wrote: > > > > > Introduce XIP (eXecute In Place) support for RISC-V platforms. > > > It allows code to be executed directly from non-volatile storage > > > directly addressable by the CPU, such as QSPI NOR flash which can > > > be found on many RISC-V platforms. This makes way for significant > > > optimization of RAM footprint. The XIP kernel is not compressed > > > since it has to run directly from flash, so it will occupy more > > > space on the non-volatile storage to The physical flash address > > > used to link the kernel object files and for storing it has to > > > be known at compile time and is represented by a Kconfig option. > > > > > > XIP on RISC-V will currently only work on MMU-enabled kernels. > > > > > > Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.com> > > > > That's nice! > > > > Suggestion for a future enhancement: > > To save on ROM storage, and given that the .data segment has to be > > copied to RAM anyway, you could store .data compressed and decompress it > > to RAM instead. See commit ca8b5d97d6bf for inspiration. In fact, many > > parts there could be shared. > > Thanks! That's in my TODO list. > > > More comments below. > > > > > +#define __XIP_FIXUP(addr) \ > > > + (((long)(addr) >= CONFIG_XIP_PHYS_ADDR && \ > > > + (long)(addr) <= CONFIG_XIP_PHYS_ADDR + SZ_16M) ? \ > > > + (long)(addr) - CONFIG_XIP_PHYS_ADDR + CONFIG_XIP_PHYS_RAM_BASE - XIP_OFFSET : \ > > > + (long)(addr)) > > > > Here you should cast to unsigned long instead. > > Right, or (just thought of it) uintptr_t for that matter. Does that sound right?
Sure.
> > > +#ifdef CONFIG_XIP_KERNEL > > > + la a0, _trampoline_pg_dir > > > + lw t0, _xip_fixup > > > + add a0, a0, t0 > > [...] > > > +_xip_fixup: > > > + .dword CONFIG_XIP_PHYS_RAM_BASE - CONFIG_XIP_PHYS_ADDR - XIP_OFFSET > > > +#endif > > > > Here _xip_fixup is a dword but you're loading it as a word. > > This won't work for both rv32 and rv64. > > Well, at this point I believe it does, as long as we use little > endian.
... and the fixup is not larger than 32 bits.
> 64-bit version has been verified.
Won't work if ROM and RAM are far-enough apart.
> I do not argue though that it isn't nice and should be fixed. > > > > +SECTIONS > > > +{ > > > + /* Beginning of code and text segment */ > > > + . = XIP_VIRT_ADDR(CONFIG_XIP_PHYS_ADDR); > > > + _xiprom = .; > > > + _start = .; > > > + HEAD_TEXT_SECTION > > > + INIT_TEXT_SECTION(PAGE_SIZE) > > > + /* we have to discard exit text and such at runtime, not link time */ > > > + .exit.text : > > > + { > > > + EXIT_TEXT > > > + } > > > + > > > + .text : { > > > + _text = .; > > > + _stext = .; > > > + TEXT_TEXT > > > + SCHED_TEXT > > > + CPUIDLE_TEXT > > > + LOCK_TEXT > > > + KPROBES_TEXT > > > + ENTRY_TEXT > > > + IRQENTRY_TEXT > > > + SOFTIRQENTRY_TEXT > > > + *(.fixup) > > > + _etext = .; > > > + } > > > + RO_DATA(L1_CACHE_BYTES) > > > + .srodata : { > > > + *(.srodata*) > > > + } > > > + .init.rodata : { > > > + INIT_SETUP(16) > > > + INIT_CALLS > > > + CON_INITCALL > > > + INIT_RAM_FS > > > + } > > > + _exiprom = ALIGN(PAGE_SIZE); /* End of XIP ROM area */ > > > > Why do you align this to a page size? > > TBH I just cut the corners here and below, did not have to worry about > partial pages and such.
You're wasting ROM space though.
> > > + > > > + > > > +/* > > > + * From this point, stuff is considered writable and will be copied to RAM > > > + */ > > > + __data_loc = ALIGN(PAGE_SIZE); /* location in file */ > > > > Same question here? > > > > > + . = PAGE_OFFSET; /* location in memory */ > > > + > > > + _sdata = .; /* Start of data section */ > > > + _data = .; > > > + RW_DATA(L1_CACHE_BYTES, PAGE_SIZE, THREAD_SIZE) > > > + _edata = .; > > > + __start_ro_after_init = .; > > > + .data.ro_after_init : AT(ADDR(.data.ro_after_init) - LOAD_OFFSET) { > > > + *(.data..ro_after_init) > > > + } > > > + __end_ro_after_init = .; > > > + > > > + . = ALIGN(PAGE_SIZE); > > > > And again here?
Forget this particular one. It is fine.
Nicolas
| |