Subject: Re: [PATCH v2 1/5] [ppc] Process dynamic relocations for kernel

From: Josh Poimboeuf <[email-masked]>
Date: Thu, 10 Nov 2011 15:44:57 -0600

On Thu, 2011-11-10 at 08:01 +0530, Suzuki Poulose wrote:
> >> How about using clean_dcache_range() to flush the range runtime
> >> address range [ _stext, _end ] ? That would flush the entire
> >> instructions.
> >
> > Wouldn't that result in more cache flushing than the original solution?
> >
> > For example, my kernel is 3.5MB.  Assuming a 32 byte cache line size,
> > clean_dcache_range(_stext, _end) would result in about 115,000 dcbst's
> > (3.5MB / 32).
> 
> Oops ! You are right. We could go back to the clean_dcache_all() or the
> initial approach that you suggested. (dcbst).
> 
> I am not sure how do we flush the entire dcache(only). Could you post a
> patch which does the same ?

It turns out that my original idea of giving a 64k address range to
clean_dcache_range() wouldn't work, because dcbst only flushes the line
if the given address is in the cache already.

Also, I experimented with a simple clean_dcache_all() function in
misc_32.S:

#define L1_CACHE_SIZE   (32 * 1024)
#define L1_CACHE_LINES  (L1_CACHE_SIZE / L1_CACHE_BYTES) 
_GLOBAL(clean_dcache_all)
        lis     r3, _start@h
        ori     r3, r3, _start@l
        li      r4, (L1_CACHE_LINES * 2)
        mtctr   r4
1:      lwz     r5, 0(r3)
        addi    r3, r3, L1_CACHE_BYTES
        bdnz    1b
        sync
        blr

But this approach has some issues:

1. It should probably be made more platform-independent with respect to
d-cache size.  I'm not sure the best way to achieve that.

2. The _start address is the kernel virtual address, not the physical
address, but relocate() is running without TLB address translation
enabled.  Although we could easily circumvent this problem by clearing
the d-cache directly in relocate() (or in head_44x.S) using physical
addresses.

3. Chicken/egg issue:  the _start address might be stale because we
haven't yet flushed the d-cache and invalidated the i-cache.

I also discovered that calling flush_instruction_cache() from relocate()
wouldn't work for all platforms, for similar reasons.

We could overcome these issues with more code, but the added complexity
might not be worth it (premature optimization and all).  My original
patch at least has the benefit of being simple.

> 
> Another option is to, change the current mapping to 'Write Through' before
> calling the relocate() and revert back to the original setting after relocate().

True, that's another option.  Although since TLB handling is
platform-specific, I think it would have to be handled by the caller in
head_44x.S, rather than within relocate().

> > That also may suggest a bigger can of worms.  A grep of the powerpc code
> > shows many uses of KERNELBASE.  For a relocatable kernel, nobody should
> > be relying on KERNELBASE except for the early relocation code.  Are we
> > sure that all the other usages of KERNELBASE are "safe"?
> >
> I think we could simply replace the occurrences of KERNELBASE (after the relocate())
>   with '_stext' which would give the virtual start address of the kernel.

Yeah, that would work.

Josh