Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common/block: Mark only TSEG range as IO_CACHEABLE ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@58 PS3, Line 58: MTRR: WB selected as default type.
I see. If WRCOMB type is removed, it means we've ran out of variabke MTRRs, which is not good.
Looking at the log it appears that below MTRR are also remain unclaimed in BIOS and OS space. so we are not running completely out
Coreboot log: 0x0000000000000000: PHYBASE6 0x0000000000000000: PHYMASK6: Disabled 0x0000000000000000: PHYBASE7 0x0000000000000000: PHYMASK7: Disabled 0x0000000000000000: PHYBASE8 0x0000000000000000: PHYMASK8: Disabled 0x0000000000000000: PHYBASE9 0x0000000000000000: PHYMASK9: Disabled
Yes, those MTRRs are left for OS usage. However, if you look up where the `MTRR: Removing WRCOMB type.` message is printed in `src/cpu/x86/mtrr/mtrr.c`, you will see that it only gets printed if `wb_deftype_count > bios_mtrrs && uc_deftype_count > bios_mtrrs`. This means we need more MTRRs than we can use in coreboot, so we drop the optional WRCOMB type to free up some MTRRs.
Yes Angel, my point why our MTRR logic can't make allocation like this instead (maximizing usage of of all 8 MTRRs available for BIOS usage).
Looks at the below MTRR range with this CL + some more modification inside 'src/cpu/x86/mtrr/mtrr.c' (which i need to clean to avoid WC is getting dropped and MTRR logic can use maximum BIOS MTRRs)
MTRR: default type WB/UC MTRR counts: 9/10. MTRR: WB selected as default type. MTRR: 0 base 0x0000000077000000 mask 0x00003fffff000000 type 0 MTRR: 1 base 0x0000000078000000 mask 0x00003ffffe000000 type 0 MTRR: 2 base 0x000000007a000000 mask 0x00003fffff000000 type 0 MTRR: 3 base 0x000000007b800000 mask 0x00003fffff800000 type 0 MTRR: 4 base 0x000000007c000000 mask 0x00003ffffc000000 type 0 MTRR: 5 base 0x0000000080000000 mask 0x00003ffff0000000 type 1 MTRR: 6 base 0x0000000090000000 mask 0x00003ffff0000000 type 0 MTRR: 7 base 0x00000000a0000000 mask 0x00003fffa0000000 type 0
Chrome OS log:
*snip* (it's the same as coreboot)
So, is there anything inside the 0x77000000 - 0x7b000000 memory range that needs to be marked as uncachable?
ME stolen memory which is used for HOST communication with security controller which i don't should be part of cacheable range, Also other reserved range if any like Tracehub etc no point of making at part of cachaable just to avoid MTRR running issue.
That sounds reasonable, but we should check if it's really necessary. I saw that, on Sandy Bridge, the SMRR can overlap with MTRRs just fine, because the settings in effect will be those of SMRR. If this is also the case on newer platforms, then we don't need to use MTRRs to describe the SMRR region. And if e.g. IMRs block accesses to the ME stolen memory and Tracehub ranges, we shouldn't need to mark them as uncachable either.
If after examining all regions inside the reserved memory range, there's something where MTRR caching settings are in effect, then we should get this patch in.
Yes, i believe there are IMR range which we should mark reserve hence we might need to make an entry in systemagent.c file unfortunately :)