Change in coreboot[master]: src/soc/intel/common/block: Mark only TSEG range as IO_CACHEABLE
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common/block: Mark only TSEG range as IO_CACHEABLE ......................................................................
src/soc/intel/common/block: Mark only TSEG range as IO_CACHEABLE
This patch ensures that the TSEG region is only mapped as cacheable so that one can perform SMRAM relocation faster.
Ideally don't need to mark the entire TOP_OF_RAM till BGSM range (used for ME stolen memory, PTT, DPR, PRMRR, TSEG etc) as cacheable as no executable code exist there except TSEG region. Hence only mark TSEG range as cacheable (+ reserved) and other ranges as reserve alone.
TEST=Able to build and boot ICL, TGL RVP.
Without this CL:
PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a
With this CL:
PCI: 00:00.0 resource base 77000000 size 4000000 align 0 gran 0 limit 0 flags f0000200 index 9 PCI: 00:00.0 resource base 7b000000 size 800000 align 0 gran 0 limit 0 flags f0004200 index a PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index b
Change-Id: I64c14b14caf0a53219fdc02ec6bbd375955a0c8e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/systemagent/systemagent.c 1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44014/1
diff --git a/src/soc/intel/common/block/systemagent/systemagent.c b/src/soc/intel/common/block/systemagent/systemagent.c index e12e07c..b15ca01 100644 --- a/src/soc/intel/common/block/systemagent/systemagent.c +++ b/src/soc/intel/common/block/systemagent/systemagent.c @@ -173,8 +173,13 @@
sa_get_mem_map(dev, &sa_map_values[0]);
- /* top_of_ram -> BGSM */ + /* top_of_ram -> TSEG */ base_k = top_of_ram; + size_k = sa_map_values[SA_TSEG_REG] - base_k; + mmio_resource(dev, index++, base_k / KiB, size_k / KiB); + + /* TSEG -> BGSM */ + base_k = sa_map_values[SA_TSEG_REG]; size_k = sa_map_values[SA_BGSM_REG] - base_k; reserved_ram_resource(dev, index++, base_k / KiB, size_k / KiB);
Hello Furquan Shaikh, Wonkyu Kim, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44014
to look at the new patch set (#2).
Change subject: src/soc/intel/common/block: Mark only TSEG range as IO_CACHEABLE ......................................................................
src/soc/intel/common/block: Mark only TSEG range as IO_CACHEABLE
This patch ensures that the TSEG region is only mapped as cacheable so that one can perform SMRAM relocation faster.
Ideally don't need to mark the entire TOP_OF_RAM till BGSM range (used for ME stolen memory, PTT, DPR, PRMRR, TSEG etc) as cacheable as no executable code exist there except TSEG region. Hence only mark TSEG range as cacheable (+ reserved) and other ranges as reserve alone.
TEST=Able to build and boot ICL, TGL RVP.
Without this CL :
PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a
With this CL :
PCI: 00:00.0 resource base 77000000 size 4000000 align 0 gran 0 limit 0 flags f0000200 index 9 PCI: 00:00.0 resource base 7b000000 size 800000 align 0 gran 0 limit 0 flags f0004200 index a PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index b
Change-Id: I64c14b14caf0a53219fdc02ec6bbd375955a0c8e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/systemagent/systemagent.c 1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44014/2
Angel Pons 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 2: Code-Review+1
V Sowmya 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 2: Code-Review+2
Nico Huber 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 2:
(4 comments)
So far, this doesn't seem to take MTRR allocations into account. Please don't submit before this has been analyzed.
https://review.coreboot.org/c/coreboot/+/44014/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/2//COMMIT_MSG@12 PS2, Line 12: Ideally don't need to mark the entire TOP_OF_RAM till BGSM range (used for : ME stolen memory, PTT, DPR, PRMRR, TSEG etc) as cacheable as no executable code : exist there except TSEG region. This explains why we can do it. But why should we?
https://review.coreboot.org/c/coreboot/+/44014/2//COMMIT_MSG@12 PS2, Line 12: Ideally don't need to mark the entire TOP_OF_RAM till BGSM range (used for : ME stolen memory, PTT, DPR, PRMRR, TSEG etc) as cacheable as no executable code : exist there except TSEG region. Hence only mark TSEG range as cacheable (+ reserved) : and other ranges as reserve alone. : Please break lines before 72 chars.
https://review.coreboot.org/c/coreboot/+/44014/2//COMMIT_MSG@17 PS2, Line 17: TEST=Able to build and boot ICL, TGL RVP. Did you compare timestamps yet?
https://review.coreboot.org/c/coreboot/+/44014/2//COMMIT_MSG@19 PS2, Line 19: Without this CL : : : PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 : PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a : : With this CL : : : PCI: 00:00.0 resource base 77000000 size 4000000 align 0 gran 0 limit 0 flags f0000200 index 9 : PCI: 00:00.0 resource base 7b000000 size 800000 align 0 gran 0 limit 0 flags f0004200 index a : PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index b This is already obvious from the code. But how about MTRR allocations? If we run out of variable MTRRs, it might make things worse.
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 2:
(2 comments)
Patch Set 2:
(4 comments)
So far, this doesn't seem to take MTRR allocations into account. Please don't submit before this has been analyzed.
MTRR allocation based on DRAM is taking care the resource input from here. And https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/... will setup DRAM MTRR range. right ?
https://review.coreboot.org/c/coreboot/+/44014/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/2//COMMIT_MSG@17 PS2, Line 17: TEST=Able to build and boot ICL, TGL RVP.
Did you compare timestamps yet?
Yes, i will share the boot time stamp.
https://review.coreboot.org/c/coreboot/+/44014/2//COMMIT_MSG@19 PS2, Line 19: Without this CL : : : PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 : PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a : : With this CL : : : PCI: 00:00.0 resource base 77000000 size 4000000 align 0 gran 0 limit 0 flags f0000200 index 9 : PCI: 00:00.0 resource base 7b000000 size 800000 align 0 gran 0 limit 0 flags f0004200 index a : PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index b
This is already obvious from the code. But how about MTRR allocations? If […]
we don't ideally as index count is still meeting this requirement https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/...
I will share the MTRR snapshot as well with and without this CL
Nico Huber 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 2:
So far, this doesn't seem to take MTRR allocations into account. Please don't submit before this has been analyzed.
MTRR allocation based on DRAM is taking care the resource input from here. And https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/... will setup DRAM MTRR range. right ?
This has nothing to do with DRAM (I could only make guesses what that comment tries to tell us, it seems wrong). But you are right that the call there sets MTRRs up.
Before I put more effort into this, please explain why we should perform the change.
Nico Huber 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 2:
Please also note that this effectively reverts your own patch CB:21539 and treats things differently than other platforms (where DPR is marked cacheable for instance).
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 2:
Patch Set 2:
Please also note that this effectively reverts your own patch CB:21539 and treats things differently than other platforms (where DPR is marked cacheable for instance).
Thanks Nico for digging, if you see CB:21539, that CL was meant to add any reserve range into DRAM resource. The assumption there was that Top_of_RAM (0x7700_0000) till TSEG (0x7b00_0000) is just reserved and TSEG (0x7b00_0000) till BGSM (0x7b80_0000) is cacheable + reserved.
But CB:36216 CL breaks that assumption and making Top_of_RAM till BGSM entire range reserved and cachable.
Current CL is basically to restore the previous assumption where we don't really need to make such bigger memory range cacheable
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 2:
Patch Set 2:
Patch Set 2:
Please also note that this effectively reverts your own patch CB:21539 and treats things differently than other platforms (where DPR is marked cacheable for instance).
Thanks Nico for digging, if you see CB:21539, that CL was meant to add any reserve range into DRAM resource. The assumption there was that Top_of_RAM (0x7700_0000) till TSEG (0x7b00_0000) is just reserved and TSEG (0x7b00_0000) till BGSM (0x7b80_0000) is cacheable + reserved.
But CB:36216 CL breaks that assumption and making Top_of_RAM till BGSM entire range reserved and cachable.
Current CL is basically to restore the previous assumption where we don't really need to make such bigger memory range cacheable
my point is "TSEG - DPR - reserved - top_of_memory == 0" is not valid assumption to make as CB:36216 does
Nico Huber 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 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Please also note that this effectively reverts your own patch CB:21539 and treats things differently than other platforms (where DPR is marked cacheable for instance).
Thanks Nico for digging, if you see CB:21539, that CL was meant to add any reserve range into DRAM resource. The assumption there was that Top_of_RAM (0x7700_0000) till TSEG (0x7b00_0000) is just reserved and TSEG (0x7b00_0000) till BGSM (0x7b80_0000) is cacheable + reserved.
But CB:36216 CL breaks that assumption and making Top_of_RAM till BGSM entire range reserved and cachable.
That's both not true.
Current CL is basically to restore the previous assumption where we don't really need to make such bigger memory range cacheable
Making a bigger memory range cacheable has usually better results, though. Unless something must not be cached. Which nobody said yet.
my point is "TSEG - DPR - reserved - top_of_memory == 0" is not valid assumption to make as CB:36216 does
Not true. I don't have time for this.
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 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Please also note that this effectively reverts your own patch CB:21539 and treats things differently than other platforms (where DPR is marked cacheable for instance).
Thanks Nico for digging, if you see CB:21539, that CL was meant to add any reserve range into DRAM resource. The assumption there was that Top_of_RAM (0x7700_0000) till TSEG (0x7b00_0000) is just reserved and TSEG (0x7b00_0000) till BGSM (0x7b80_0000) is cacheable + reserved.
But CB:36216 CL breaks that assumption and making Top_of_RAM till BGSM entire range reserved and cachable.
That's both not true.
i don't understand your comments here. i have provided the value so a simple math will tell that the assumption made by CB:36216 is not true
TSEG = 0x7b000000 Assuming you don't know the value of DPR and reseved top_of_memory = 0x77000000 so the result would be 0x4000000 its not 0 right ?
Current CL is basically to restore the previous assumption where we don't really need to make such bigger memory range cacheable
Making a bigger memory range cacheable has usually better results, though. Unless something must not be cached. Which nobody said yet.
But do we have that much bigger cache either right?
my point is "TSEG - DPR - reserved - top_of_memory == 0" is not valid assumption to make as CB:36216 does
Not true.
I have given the calculation above, can you please help to understand why its still not true ?
I don't have time for this.
I don't know what to say.
Angel Pons 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 2:
The thing with MTRRs is that there's a limited number of them. If we run out of MTRRs, we can't properly describe memory regions.
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Please also note that this effectively reverts your own patch CB:21539 and treats things differently than other platforms (where DPR is marked cacheable for instance).
Thanks Nico for digging, if you see CB:21539, that CL was meant to add any reserve range into DRAM resource. The assumption there was that Top_of_RAM (0x7700_0000) till TSEG (0x7b00_0000) is just reserved and TSEG (0x7b00_0000) till BGSM (0x7b80_0000) is cacheable + reserved.
But CB:36216 CL breaks that assumption and making Top_of_RAM till BGSM entire range reserved and cachable.
That's both not true.
i don't understand your comments here. i have provided the value so a simple math will tell that the assumption made by CB:36216 is not true
TSEG = 0x7b000000 Assuming you don't know the value of DPR and reseved top_of_memory = 0x77000000 so the result would be 0x4000000 its not 0 right ?
The thing with these numbers is that they may change between platforms and between different configurations (e.g. iGPU enabled/disabled).
Current CL is basically to restore the previous assumption where we don't really need to make such bigger memory range cacheable
Making a bigger memory range cacheable has usually better results, though. Unless something must not be cached. Which nobody said yet.
But do we have that much bigger cache either right?
Marking a memory range as cacheable does not mean it will always be put in cache. The cache size does not matter when marking a memory range as cacheable. For things like memory-mapped registers, we do not want caching: if writes hit the cache, they don't reach the actual registers; for reads, the value of some registers can be changed by the hardware and the cache would not be updated.
my point is "TSEG - DPR - reserved - top_of_memory == 0" is not valid assumption to make as CB:36216 does
Not true.
I have given the calculation above, can you please help to understand why its still not true ?
AFAIUI, you said that "TSEG - DPR - reserved - top_of_memory == 0" is not a valid assumption. Why so? In which case does this assumption not hold?
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Maulik V Vaghela, Angel Pons, Aamir Bohra, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44014
to look at the new patch set (#3).
Change subject: src/soc/intel/common/block: Mark only TSEG range as IO_CACHEABLE ......................................................................
src/soc/intel/common/block: Mark only TSEG range as IO_CACHEABLE
This patch ensures that the TSEG region is only mapped as cacheable so that one can perform SMRAM relocation faster.
Ideally don't need to mark the entire TOP_OF_RAM till BGSM range (used for ME stolen memory, PTT, DPR, PRMRR, TSEG etc) as cacheable as no executable code exist there except TSEG region. Hence only mark TSEG range as cacheable (+ reserved) and other ranges as reserve alone.
TEST=Able to build and boot ICL, TGL RVP.
Without this CL :
PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a
MTRR: Physical address space: 0x0000000000000000 - 0x00000000000a0000 size 0x000a0000 type 6 0x00000000000a0000 - 0x00000000000c0000 size 0x00020000 type 0 0x00000000000c0000 - 0x000000007b800000 size 0x7b740000 type 6 0x000000007b800000 - 0x0000000080000000 size 0x04800000 type 0 0x0000000080000000 - 0x0000000090000000 size 0x10000000 type 1 0x0000000090000000 - 0x0000000100000000 size 0x70000000 type 0 0x0000000100000000 - 0x0000000480400000 size 0x380400000 type 6
MTRR: WB selected as default type. MTRR: 0 base 0x000000007b800000 mask 0x00003fffff800000 type 0 MTRR: 1 base 0x000000007c000000 mask 0x00003ffffc000000 type 0 MTRR: 2 base 0x0000000080000000 mask 0x00003ffff0000000 type 1 MTRR: 3 base 0x0000000090000000 mask 0x00003ffff0000000 type 0 MTRR: 4 base 0x00000000a0000000 mask 0x00003fffe0000000 type 0 MTRR: 5 base 0x00000000c0000000 mask 0x00003fffc0000000 type 0
With this CL :
PCI: 00:00.0 resource base 77000000 size 4000000 align 0 gran 0 limit 0 flags f0000200 index 9 PCI: 00:00.0 resource base 7b000000 size 800000 align 0 gran 0 limit 0 flags f0004200 index a PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index b
MTRR: Physical address space: 0x0000000000000000 - 0x00000000000a0000 size 0x000a0000 type 6 0x00000000000a0000 - 0x00000000000c0000 size 0x00020000 type 0 0x00000000000c0000 - 0x0000000077000000 size 0x76f40000 type 6 0x0000000077000000 - 0x000000007b000000 size 0x04000000 type 0 0x000000007b000000 - 0x000000007b800000 size 0x00800000 type 6 0x000000007b800000 - 0x0000000080000000 size 0x04800000 type 0 0x0000000080000000 - 0x0000000090000000 size 0x10000000 type 1 0x0000000090000000 - 0x0000000100000000 size 0x70000000 type 0 0x0000000100000000 - 0x0000000480400000 size 0x380400000 type 6
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 0x00003fff80000000 type 0
Change-Id: I64c14b14caf0a53219fdc02ec6bbd375955a0c8e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/systemagent/systemagent.c 1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44014/3
Angel Pons 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:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@27 PS3, Line 27: 0x00000000000c0000 - 0x000000007b800000 size 0x7b740000 type 6 Before:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@50 PS3, Line 50: 0x00000000000c0000 - 0x0000000077000000 size 0x76f40000 type 6 : 0x0000000077000000 - 0x000000007b000000 size 0x04000000 type 0 : 0x000000007b000000 - 0x000000007b800000 size 0x00800000 type 6 After:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@58 PS3, Line 58: MTRR: WB selected as default type. I don't see any Type 1 MTRRs in here. Could you please share the full bootlog? You can put it on https://paste.flashrom.org (email can be `none`)
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:
Patch Set 2:
The thing with MTRRs is that there's a limited number of them. If we run out of MTRRs, we can't properly describe memory regions.
Valid point, today we have 6 MTRR max to provide the BIOS resource snapshot which should be able to manage the required BIOS used memory description
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Please also note that this effectively reverts your own patch CB:21539 and treats things differently than other platforms (where DPR is marked cacheable for instance).
Thanks Nico for digging, if you see CB:21539, that CL was meant to add any reserve range into DRAM resource. The assumption there was that Top_of_RAM (0x7700_0000) till TSEG (0x7b00_0000) is just reserved and TSEG (0x7b00_0000) till BGSM (0x7b80_0000) is cacheable + reserved.
But CB:36216 CL breaks that assumption and making Top_of_RAM till BGSM entire range reserved and cachable.
That's both not true.
i don't understand your comments here. i have provided the value so a simple math will tell that the assumption made by CB:36216 is not true
TSEG = 0x7b000000 Assuming you don't know the value of DPR and reseved top_of_memory = 0x77000000 so the result would be 0x4000000 its not 0 right ?
The thing with these numbers is that they may change between platforms and between different configurations (e.g. iGPU enabled/disabled).
yes, also based on debug options like tracehub, PRMRR etc those number would chance.
Current CL is basically to restore the previous assumption where we don't really need to make such bigger memory range cacheable
Making a bigger memory range cacheable has usually better results, though. Unless something must not be cached. Which nobody said yet.
But do we have that much bigger cache either right?
Marking a memory range as cacheable does not mean it will always be put in cache. The cache size does not matter when marking a memory range as cacheable. For things like memory-mapped registers, we do not want caching: if writes hit the cache, they don't reach the actual registers; for reads, the value of some registers can be changed by the hardware and the cache would not be updated.
my point is "TSEG - DPR - reserved - top_of_memory == 0" is not valid assumption to make as CB:36216 does
Not true.
I have given the calculation above, can you please help to understand why its still not true ?
AFAIUI, you said that "TSEG - DPR - reserved - top_of_memory == 0" is not a valid assumption. Why so? In which case does this assumption not hold?
this reserve size should accounted for all reserve range available (this was old implementation which got stabled after CB:36216 hence we can ignore this part) if that not happen then this assumption won't hold good.
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 don't see any Type 1 MTRRs in here. […]
Please check the mask value for MTRR: 5 its basically ignoring WC to accommodate the BIOS MTRRs
MTRR: Removing WRCOMB type. WB/UC MTRR counts: 9/10 > 8. MTRR: default type WB/UC MTRR counts: 6/9. 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 0x00003fff80000000 type 0
Angel Pons 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: -Code-Review
(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.
Please check the mask value for MTRR: 5 its basically ignoring WC to accommodate the BIOS MTRRs […]
I see. If WRCOMB type is removed, it means we've ran out of variabke MTRRs, which is not good.
So, is there anything inside the 0x77000000 - 0x7b000000 memory range that needs to be marked as uncachable?
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
Chrome OS log:
[ 0.039794] MTRR variable ranges enabled: [ 0.043754] 0 base 000077000000 mask 3FFFFF000000 uncachable [ 0.049512] 1 base 000078000000 mask 3FFFFE000000 uncachable [ 0.055269] 2 base 00007A000000 mask 3FFFFF000000 uncachable [ 0.061027] 3 base 00007B800000 mask 3FFFFF800000 uncachable [ 0.066792] 4 base 00007C000000 mask 3FFFFC000000 uncachable [ 0.072550] 5 base 000080000000 mask 3FFF80000000 uncachable [ 0.078309] 6 disabled [ 0.080802] 7 disabled [ 0.083295] 8 disabled [ 0.085788] 9 disabled
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.
Angel Pons 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.
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.
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 :)
Aaron Durbin 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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@10 PS3, Line 10: that one can perform SMRAM relocation faster. But this was already happening.
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@15 PS3, Line 15: range as cacheable (+ reserved) and other ranges as reserve alone. What's the motivation? If the order of operations is maintained (us doing mpinit w/ smm relocation early) one can just omit tseg from being a reserved ram resource because we've done all that work already. i.e. keep one memory map for init and the one we expose to OS as different.
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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@10 PS3, Line 10: that one can perform SMRAM relocation faster.
But this was already happening.
yes, it just to only make TSEG range notthe entire stolen range cacheable, i will update the commit msg to make it clear
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@15 PS3, Line 15: range as cacheable (+ reserved) and other ranges as reserve alone.
What's the motivation? If the order of operations is maintained (us doing mpinit w/ smm relocation e […]
Do you mean to remove TSEG region reservation like below ? I could see if we are not marking this TSEG range reserved that PCI enumeration is giving that region to other device as BAR and creating problem (like i saw TSEG region has given to SATA and system is keep on rebooting or its given to IGD and accessing IGD region is causing MCE)
+#if 0 /* TSEG -> BGSM */ base_k = sa_map_values[SA_TSEG_REG]; size_k = sa_map_values[SA_BGSM_REG] - base_k; reserved_ram_resource(dev, index++, base_k / KiB, size_k / KiB); - +#endif
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Maulik V Vaghela, Angel Pons, Aamir Bohra, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44014
to look at the new patch set (#4).
Change subject: src/soc/intel/common/block: Mark only TSEG range as IO_CACHEABLE ......................................................................
src/soc/intel/common/block: Mark only TSEG range as IO_CACHEABLE
This patch ensures that the TSEG region is only mapped as cacheable so that one can perform SMRAM relocation faster.
Ideally don't need to mark the entire TOP_OF_RAM till BGSM range (used for ME stolen memory, PTT, DPR, PRMRR, TSEG etc) as cacheable as no executable code exist there except TSEG region. Hence only mark TSEG range as cacheable (+ reserved) and other ranges as reserve alone.
TEST=Able to build and boot ICL, TGL RVP.
Without this CL :
PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a
MTRR: Physical address space: 0x0000000000000000 - 0x00000000000a0000 size 0x000a0000 type 6 0x00000000000a0000 - 0x00000000000c0000 size 0x00020000 type 0 0x00000000000c0000 - 0x000000007b800000 size 0x7b740000 type 6 0x000000007b800000 - 0x0000000080000000 size 0x04800000 type 0 0x0000000080000000 - 0x0000000090000000 size 0x10000000 type 1 0x0000000090000000 - 0x0000000100000000 size 0x70000000 type 0 0x0000000100000000 - 0x0000000480400000 size 0x380400000 type 6
MTRR: WB selected as default type. MTRR: 0 base 0x000000007b800000 mask 0x00003fffff800000 type 0 MTRR: 1 base 0x000000007c000000 mask 0x00003ffffc000000 type 0 MTRR: 2 base 0x0000000080000000 mask 0x00003ffff0000000 type 1 MTRR: 3 base 0x0000000090000000 mask 0x00003ffff0000000 type 0 MTRR: 4 base 0x00000000a0000000 mask 0x00003fffe0000000 type 0 MTRR: 5 base 0x00000000c0000000 mask 0x00003fffc0000000 type 0
With this CL :
PCI: 00:00.0 resource base 77000000 size 4000000 align 0 gran 0 limit 0 flags f0000200 index 9 PCI: 00:00.0 resource base 7b000000 size 800000 align 0 gran 0 limit 0 flags f0004200 index a PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index b
MTRR: Physical address space: 0x0000000000000000 - 0x00000000000a0000 size 0x000a0000 type 6 0x00000000000a0000 - 0x00000000000c0000 size 0x00020000 type 0 0x00000000000c0000 - 0x0000000077000000 size 0x76f40000 type 6 0x0000000077000000 - 0x000000007b000000 size 0x04000000 type 0 0x000000007b000000 - 0x000000007b800000 size 0x00800000 type 6 0x000000007b800000 - 0x0000000080000000 size 0x04800000 type 0 0x0000000080000000 - 0x0000000090000000 size 0x10000000 type 1 0x0000000090000000 - 0x0000000100000000 size 0x70000000 type 0 0x0000000100000000 - 0x0000000480400000 size 0x380400000 type 6
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 0x00003fff80000000 type 0
Change-Id: I64c14b14caf0a53219fdc02ec6bbd375955a0c8e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/systemagent/systemagent.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44014/4
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Maulik V Vaghela, Angel Pons, Aamir Bohra, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44014
to look at the new patch set (#5).
Change subject: src/soc/intel/common: Make top_of_ram till BGSM region mmio_resource ......................................................................
src/soc/intel/common: Make top_of_ram till BGSM region mmio_resource
Ideally don't need to mark the entire top_of_ram till BGSM range (used for ME stolen memory, PTT, DPR, PRMRR, TSEG etc) as cacheable for OS usage as coreboot already done with mpinit w/ smm relocation early.
TEST=Able to build and boot ICL, TGL RVP.
Without this CL :
PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a
MTRR: Physical address space: 0x0000000000000000 - 0x00000000000a0000 size 0x000a0000 type 6 0x00000000000a0000 - 0x00000000000c0000 size 0x00020000 type 0 0x00000000000c0000 - 0x000000007b800000 size 0x7b740000 type 6 0x000000007b800000 - 0x0000000080000000 size 0x04800000 type 0 0x0000000080000000 - 0x0000000090000000 size 0x10000000 type 1 0x0000000090000000 - 0x0000000100000000 size 0x70000000 type 0 0x0000000100000000 - 0x0000000480400000 size 0x380400000 type 6
MTRR: WB selected as default type. MTRR: 0 base 0x000000007b800000 mask 0x00003fffff800000 type 0 MTRR: 1 base 0x000000007c000000 mask 0x00003ffffc000000 type 0 MTRR: 2 base 0x0000000080000000 mask 0x00003ffff0000000 type 1 MTRR: 3 base 0x0000000090000000 mask 0x00003ffff0000000 type 0 MTRR: 4 base 0x00000000a0000000 mask 0x00003fffe0000000 type 0 MTRR: 5 base 0x00000000c0000000 mask 0x00003fffc0000000 type 0
With this CL :
PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0000200 index 9 PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a
MTRR: Physical address space: 0x0000000000000000 - 0x00000000000a0000 size 0x000a0000 type 6 0x00000000000a0000 - 0x00000000000c0000 size 0x00020000 type 0 0x00000000000c0000 - 0x0000000077000000 size 0x76f40000 type 6 0x0000000077000000 - 0x0000000080000000 size 0x09000000 type 0 0x0000000080000000 - 0x0000000090000000 size 0x10000000 type 1 0x0000000090000000 - 0x0000000100000000 size 0x70000000 type 0 0x0000000100000000 - 0x0000000480400000 size 0x380400000 type 6
MTRR: WB selected as default type. MTRR: 0 base 0x0000000077000000 mask 0x00003fffff000000 type 0 MTRR: 1 base 0x0000000078000000 mask 0x00003ffff8000000 type 0 MTRR: 2 base 0x0000000080000000 mask 0x00003ffff0000000 type 1 MTRR: 3 base 0x0000000090000000 mask 0x00003ffff0000000 type 0 MTRR: 4 base 0x00000000a0000000 mask 0x00003fffe0000000 type 0 MTRR: 5 base 0x00000000c0000000 mask 0x00003fffc0000000 type 0
Change-Id: I64c14b14caf0a53219fdc02ec6bbd375955a0c8e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/systemagent/systemagent.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44014/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till BGSM region mmio_resource ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/44014/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/2//COMMIT_MSG@12 PS2, Line 12: Ideally don't need to mark the entire TOP_OF_RAM till BGSM range (used for : ME stolen memory, PTT, DPR, PRMRR, TSEG etc) as cacheable as no executable code : exist there except TSEG region. Hence only mark TSEG range as cacheable (+ reserved) : and other ranges as reserve alone. :
Please break lines before 72 chars.
Done
https://review.coreboot.org/c/coreboot/+/44014/2//COMMIT_MSG@19 PS2, Line 19: Without this CL : : : PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 : PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a : : With this CL : : : PCI: 00:00.0 resource base 77000000 size 4000000 align 0 gran 0 limit 0 flags f0000200 index 9 : PCI: 00:00.0 resource base 7b000000 size 800000 align 0 gran 0 limit 0 flags f0004200 index a : PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index b
we don't ideally as index count is still meeting this requirement https://github. […]
Done
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@10 PS3, Line 10: that one can perform SMRAM relocation faster.
yes, it just to only make TSEG range notthe entire stolen range cacheable, i will update the commit […]
Done
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@15 PS3, Line 15: range as cacheable (+ reserved) and other ranges as reserve alone.
Do you mean to remove TSEG region reservation like below ? I could see if we are not marking this TS […]
@Aaron, i guess you meant this https://review.coreboot.org/c/coreboot/+/44014/5/src/soc/intel/common/block/... this looks good now. thanks
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. […]
No change in MTRR layout hence we are good.
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Maulik V Vaghela, Angel Pons, Aamir Bohra, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44014
to look at the new patch set (#6).
Change subject: src/soc/intel/common: Make top_of_ram till BGSM region mmio_resource ......................................................................
src/soc/intel/common: Make top_of_ram till BGSM region mmio_resource
Ideally don't need to mark the entire top_of_ram till BGSM range (used for ME stolen memory, PTT, DPR, PRMRR, TSEG etc) as cacheable for OS usage as coreboot already done with mpinit w/ smm relocation early.
TEST=Able to build and boot ICL, TGL RVP.
Without this CL :
PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a
With this CL :
PCI: 00:00.0 resource base 77000000 size 8c00000 align 0 gran 0 limit 0 flags f0000200 index 9
No changes observed with MTRRs snapshot.
Change-Id: I64c14b14caf0a53219fdc02ec6bbd375955a0c8e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/systemagent/systemagent.c 1 file changed, 2 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44014/6
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Maulik V Vaghela, Angel Pons, Aamir Bohra, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44014
to look at the new patch set (#7).
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource
Ideally don't need to mark the entire top_of_ram till TOLUD range (used for stolen memory like GFX and ME, PTT, DPR, PRMRR, TSEG etc) as cacheable for OS usage as coreboot already done with mpinit w/ smm relocation early.
TEST=Able to build and boot ICL, TGL RVP.
Without this CL :
PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a
With this CL :
PCI: 00:00.0 resource base 77000000 size 8c00000 align 0 gran 0 limit 0 flags f0000200 index 9
No changes observed with MTRRs snapshot.
Change-Id: I64c14b14caf0a53219fdc02ec6bbd375955a0c8e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/systemagent/systemagent.c 1 file changed, 2 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44014/7
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Maulik V Vaghela, Angel Pons, Aamir Bohra, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44014
to look at the new patch set (#8).
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource
Ideally don't need to mark the entire top_of_ram till TOLUD range (used for stolen memory like GFX and ME, PTT, DPR, PRMRR, TSEG etc) as cacheable for OS usage as coreboot already done with mpinit w/ smm relocation early.
TEST=Able to build and boot ICL, TGL RVP.
Without this CL :
PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a
With this CL :
PCI: 00:00.0 resource base 77000000 size 8c00000 align 0 gran 0 limit 0 flags f0000200 index 9
No changes observed with MTRRs snapshot.
Change-Id: I64c14b14caf0a53219fdc02ec6bbd375955a0c8e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/systemagent/systemagent.c 1 file changed, 2 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44014/8
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@15 PS3, Line 15: range as cacheable (+ reserved) and other ranges as reserve alone.
@Aaron, i guess you meant this https://review.coreboot. […]
That is what I meant. If we've already utilized TSEG from early mtrr solution and it was marked cacheable we get the speed up. After that we finalize our MTRR solution for boot. Looks like we do what I noted when !CONFIG(USE_INTEL_FSP_MP_INIT). When CONFIG(USE_INTEL_FSP_MP_INIT) = y then that assumption breaks down looking at post_cpus_init() in src/soc/intel/common/block/cpu/mp_init.c
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 8: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44014/8/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/44014/8/src/soc/intel/common/block/... PS8, Line 146: * It's probably best to replace this paragraph with the expected flow as discussed in the CL comments. That way the assumptions are documented here.
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Maulik V Vaghela, Angel Pons, Aamir Bohra, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44014
to look at the new patch set (#9).
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource
Ideally don't need to mark the entire top_of_ram till TOLUD range (used for stolen memory like GFX and ME, PTT, DPR, PRMRR, TSEG etc) as cacheable for OS usage as coreboot already done with mpinit w/ smm relocation early.
TEST=Able to build and boot ICL, TGL RVP.
Without this CL :
PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a
With this CL :
PCI: 00:00.0 resource base 77000000 size 8c00000 align 0 gran 0 limit 0 flags f0000200 index 9
No changes observed with MTRRs snapshot.
Change-Id: I64c14b14caf0a53219fdc02ec6bbd375955a0c8e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/systemagent/systemagent.c 1 file changed, 6 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44014/9
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@15 PS3, Line 15: range as cacheable (+ reserved) and other ranges as reserve alone.
That is what I meant. […]
When CONFIG(USE_INTEL_FSP_MP_INIT) = y then that assumption breaks down looking at post_cpus_init() in src/soc/intel/common/block/cpu/mp_init.c
This is the problem today when user selects USE_INTEL_FSP_MP_INIT, we don't even run MTRR programming on MP and just booted to OS which is wrong, i have clarified that in other CL [https://review.coreboot.org/c/coreboot/+/44058/1/src/soc/intel/tigerlake/fsp...] will get this fixed soon
https://review.coreboot.org/c/coreboot/+/44014/8/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/44014/8/src/soc/intel/common/block/... PS8, Line 146: *
It's probably best to replace this paragraph with the expected flow as discussed in the CL comments. […]
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@15 PS3, Line 15: range as cacheable (+ reserved) and other ranges as reserve alone.
When CONFIG(USE_INTEL_FSP_MP_INIT) = y then that assumption breaks down looking at post_cpus_init […]
Also Aaron, as you brought this point, i could ask your feedback about how you would recommend the flow when user selects USE_INTEL_FSP_MP_INIT, in those cases, CB don't run mp init earlier w/ smm and FSP-S will bring APs up. But after FSP-S done and control comes back, we might need to run init_cpus() to bring those AP in coreboot context so it can run post_cpus_init with same assumption.
Can we do something like this? 1. Do init_cpus() after FSP-S exit if USE_INTEL_FSP_MP_INIT is selected => to bring all cores back into cb context 2. Remove USE_INTEL_FSP_MP_INIT check inside post_cpus_init() as we would like all cores to have MTRR updated based on DRAM snapshot else i could see it still has old MTRR snapshot (before DRAM) after booting to OS incase USE_INTEL_FSP_MP_INIT is selected
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@15 PS3, Line 15: range as cacheable (+ reserved) and other ranges as reserve alone.
Also Aaron, as you brought this point, i could ask your feedback about how you would recommend the f […]
We'd have to do something like you suggested, yes.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9: Code-Review+1
Let's see what others have to say.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9: Code-Review+1
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44014/3//COMMIT_MSG@15 PS3, Line 15: range as cacheable (+ reserved) and other ranges as reserve alone.
We'd have to do something like you suggested, yes.
Sure, i have created few CLs in the same line, kindly help to review if you have some time. https://review.coreboot.org/c/coreboot/+/44076/2 https://review.coreboot.org/c/coreboot/+/44077/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9:
Patch Set 9: Code-Review+1
Let's see what others have to say.
Can you please help to look once, its been few days and looks good so far
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9: Code-Review+1
Patch Set 9: Code-Review+1
Let's see what others have to say.
I'm still not sure *why* we should change this (or why we should keep the current setup).
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9:
Patch Set 9: Code-Review+1
Patch Set 9: Code-Review+1
Let's see what others have to say.
I'm still not sure *why* we should change this (or why we should keep the current setup).
Angel, the point of making this change would be, we don't need to make those ranges mark cacheable specifically reserve HW range for OS to consume as coreboot has done TSEG/SMM relocation hence benefit has already before, anything we are marking here is for OS usage or telling OS then do we see any need to say those ranges are cacaheable either rather this CL make those HW reserve range just IO_RESERVE.
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9: Code-Review+1
I have tested this patch on Jasper Lake and normal boot, reboot and S3 works fine with the patch.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9: Code-Review+2
Patch Set 9:
Patch Set 9: Code-Review+1
Patch Set 9: Code-Review+1
Let's see what others have to say.
I'm still not sure *why* we should change this (or why we should keep the current setup).
Angel, the point of making this change would be, we don't need to make those ranges mark cacheable specifically reserve HW range for OS to consume as coreboot has done TSEG/SMM relocation hence benefit has already before, anything we are marking here is for OS usage or telling OS then do we see any need to say those ranges are cacaheable either rather this CL make those HW reserve range just IO_RESERVE.
Ack
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9:
Patch Set 9: Code-Review+2
Patch Set 9:
Patch Set 9: Code-Review+1
Patch Set 9: Code-Review+1
Let's see what others have to say.
I'm still not sure *why* we should change this (or why we should keep the current setup).
Angel, the point of making this change would be, we don't need to make those ranges mark cacheable specifically reserve HW range for OS to consume as coreboot has done TSEG/SMM relocation hence benefit has already before, anything we are marking here is for OS usage or telling OS then do we see any need to say those ranges are cacaheable either rather this CL make those HW reserve range just IO_RESERVE.
Ack
Thanks Angel
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 9: Code-Review+2
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource
Ideally don't need to mark the entire top_of_ram till TOLUD range (used for stolen memory like GFX and ME, PTT, DPR, PRMRR, TSEG etc) as cacheable for OS usage as coreboot already done with mpinit w/ smm relocation early.
TEST=Able to build and boot ICL, TGL RVP.
Without this CL :
PCI: 00:00.0 resource base 77000000 size 4800000 align 0 gran 0 limit 0 flags f0004200 index 9 PCI: 00:00.0 resource base 7b800000 size 4400000 align 0 gran 0 limit 0 flags f0000200 index a
With this CL :
PCI: 00:00.0 resource base 77000000 size 8c00000 align 0 gran 0 limit 0 flags f0000200 index 9
No changes observed with MTRRs snapshot.
Change-Id: I64c14b14caf0a53219fdc02ec6bbd375955a0c8e Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44014 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Maulik V Vaghela maulik.v.vaghela@intel.com Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/soc/intel/common/block/systemagent/systemagent.c 1 file changed, 6 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, but someone else must approve Maulik V Vaghela: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/common/block/systemagent/systemagent.c b/src/soc/intel/common/block/systemagent/systemagent.c index 195a8e7..39ac53f 100644 --- a/src/soc/intel/common/block/systemagent/systemagent.c +++ b/src/soc/intel/common/block/systemagent/systemagent.c @@ -158,8 +158,7 @@ * These are the host memory ranges that should be added: * - 0 -> 0xa0000: cacheable * - 0xc0000 -> top_of_ram : cacheable - * - top_of_ram -> BGSM: cacheable with standard MTRRs and reserved - * - BGSM -> TOLUD: not cacheable with standard MTRRs and reserved + * - top_of_ram -> TOLUD: not cacheable with standard MTRRs and reserved * - 4GiB -> TOUUD: cacheable * * The default SMRAM space is reserved so that the range doesn't @@ -173,9 +172,10 @@ * is not omitted the mtrr code will setup the area as cacheable * causing VGA access to not work. * - * The TSEG region is mapped as cacheable so that one can perform - * SMRAM relocation faster. Once the SMRR is enabled the SMRR takes - * precedence over the existing MTRRs covering this region. + * Don't need to mark the entire top_of_ram till TOLUD range (used + * for stolen memory like GFX and ME, PTT, DPR, PRMRR, TSEG etc) as + * cacheable for OS usage as coreboot already done with mpinit w/ smm + * relocation early. * * It should be noted that cacheable entry types need to be added in * order. The reason is that the current MTRR code assumes this and @@ -206,13 +206,8 @@
sa_get_mem_map(dev, &sa_map_values[0]);
- /* top_of_ram -> BGSM */ + /* top_of_ram -> TOLUD */ base_k = top_of_ram; - size_k = sa_map_values[SA_BGSM_REG] - base_k; - reserved_ram_resource(dev, index++, base_k / KiB, size_k / KiB); - - /* BGSM -> TOLUD */ - base_k = sa_map_values[SA_BGSM_REG]; size_k = sa_map_values[SA_TOLUD_REG] - base_k; mmio_resource(dev, index++, base_k / KiB, size_k / KiB);
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44014 )
Change subject: src/soc/intel/common: Make top_of_ram till TOLUD region mmio_resource ......................................................................
Patch Set 10:
(1 comment)
Patchset:
PS10:
Patch Set 9:
Patch Set 9: Code-Review+1
Patch Set 9: Code-Review+1
Let's see what others have to say.
I'm still not sure *why* we should change this (or why we should keep the current setup).
Angel, the point of making this change would be, we don't need to make those ranges mark cacheable specifically reserve HW range for OS to consume as coreboot has done TSEG/SMM relocation hence benefit has already before, anything we are marking here is for OS usage or telling OS then do we see any need to say those ranges are cacaheable either rather this CL make those HW reserve range just IO_RESERVE.
Those ranges are reserved by the OS. This has nothing to do with marking them cacheable. It's not because you don't need them to be cacheable that it is a good idea to set them up UC. You want to use as little MTRR as possible and marking that region as UC blows up the MTRR solution. It should be marked as reserved_ram instead of mmio_resource, since that is what it really is.
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
Furquan Shaikh (Code Review)
-
Maulik V Vaghela (Code Review)
-
Nico Huber (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)