Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 16: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... PS16, Line 93: BIOS_WARNING Nit: why is this WARNING, when not caching at all is only NOTICE?
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... PS16, Line 93: d Nit: %u (actually PRIu32, but %u is compatible to x86_64 anyway and looks better)
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... PS16, Line 94: mtrr_size / KiB); Nit: looks like it fits 96 chars exactly.
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... PS16, Line 111: } Why did the if turn into a loop? it seems easy to prove that it can't get better after the second iteration...
4 cases: 1. size_covered = end - base = size // maximum 2. size_covered = end - mtrr_base 3. size_covered = mtrr_base + mtrr_size - base 4. size_covered = mtrr_base + mtrr_size - mtrr_base = mtrr_size
The result of 2. is always worse than that of 4.
And the result of 4. is invariant.
In the first iteration we can only hit 1. or 3.
From the second iteration on we can only hit 2. or 4.
And if we hit 1. or 2., it's the last iteration.
So possible runs are (1|34*2).
1 => done, 34.. => can't get better, 32 => done
if (mtrr_base + mtrr_size < end && /* not 1 */ mtrr_base + mtrr_size - base < MIN(mtrr_base + 2 * mtrr_size, end) - (mtrr_base + mtrr_size) /* better of 3 or 2/4 */) mtrr_base += mtrr_size;
obviously doesn't look better than the loop... how about
mtrr_base = ALIGN_DOWN(base, mtrr_size); if (mtrr_base + mtrr_size < end) { printk(BIOS_NOTICE, "PROG_RUN: Limiting XIP cache to %uKiB!", mtrr_size / KiB); /* Check if we can cover a bigger range by aligning up. */ const uint32_t alt_base = ALIGN_UP(base, mtrr_size); const uint32_t lower_coverage = mtrr_base + mtrr_size - base; const uint32_t upper_coverage = MIN(alt_base + mtrr_size, end) - alt_base; if (upper_coverage > lower_coverage) mtrr_base = alt_base; }
Soooo, the loop is not wrong, and an exhaustive search doesn't hurt more than my urge for explicitness. I won't block it if you prefer it.