Re: [PATCH] arch/riscv: enable XIP

[Nicolas Pitre]

On Wed, 2 Dec 2020, Vitaly Wool wrote:

> On Wed, Dec 2, 2020 at 7:06 PM Nicolas Pitre <nico@xxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >
> > 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