Change in coreboot[master]: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Hello Shreesh Chhabbi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to review the following change.
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/1
diff --git a/src/soc/intel/common/block/cpu/Kconfig b/src/soc/intel/common/block/cpu/Kconfig index 912760e..2b630d0 100644 --- a/src/soc/intel/common/block/cpu/Kconfig +++ b/src/soc/intel/common/block/cpu/Kconfig @@ -51,6 +51,14 @@ ENHANCED NEM guarantees that modified data is always kept in cache while clean data is replaced.
+config CAR_HAS_SF_MASKS + bool + depends on INTEL_CAR_NEM_ENHANCED + help + In the case of non-inclusive cache architecture Snoop Filter MSR + IA32_L3_SF_MASK_x programming is required along with the data ways. + This is applicable for TGL and beyond. + config COS_MAPPED_TO_MSB bool depends on INTEL_CAR_NEM_ENHANCED diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram.S b/src/soc/intel/common/block/cpu/car/cache_as_ram.S index 7408822..93f5f95 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram.S @@ -414,6 +414,35 @@ set_eviction_mask: mov %ebx, %ecx /* back up the number of ways */ mov %eax, %ebx /* back up the non-eviction mask*/ + mov %ecx, %edi /* back up the number of ways */ + +#if CONFIG(CAR_HAS_SF_MASKS) + /* + * If total number of available ways is X, SF mask is ((0x1 << X*2) - 1) + */ + mov $0x02, %eax + mul %ecx + mov %eax, %ecx + mov $0x01, %eax + shl %cl, %eax + subl $0x01, %eax /* Holds the SF mask */ + /* + * Program MSR 0x1891 IA32_CR_SF_QOS_MASK_1 with + * total number of LLC ways + */ + movl $IA32_CR_SF_QOS_MASK_1, %ecx + xorl %edx, %edx + wrmsr + /* + * Program MSR 0x1892 IA32_CR_SF_QOS_MASK_2 with + * total number of LLC ways + */ + movl $IA32_CR_SF_QOS_MASK_2, %ecx + xorl %edx, %edx + wrmsr + mov %edi, %ecx /* Restore number of ways */ +#endif + /* * Program MSR 0xC91 IA32_L3_MASK_1 * This MSR contain one bit per each way of LLC diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig index f53c18a..dd14ad2 100644 --- a/src/soc/intel/tigerlake/Kconfig +++ b/src/soc/intel/tigerlake/Kconfig @@ -24,6 +24,7 @@ select HAVE_SMI_HANDLER select IDT_IN_EVERY_STAGE select INTEL_CAR_NEM_ENHANCED if !INTEL_CAR_NEM + select CAR_HAS_SF_MASKS if INTEL_CAR_NEM_ENHANCED select COS_MAPPED_TO_MSB if INTEL_CAR_NEM_ENHANCED select INTEL_GMA_ACPI select INTEL_GMA_ADD_VBT if RUN_FSP_GOP
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/1/src/soc/intel/common/block/... PS1, Line 445: trailing whitespace
Hello build bot (Jenkins), Patrick Rudolph, Shreesh Chhabbi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#2).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48286/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/2/src/soc/intel/common/block/... PS2, Line 417: mov %ecx, %edi This seems weird. ebx was backed up to ecx and 2 lines below that ecx is backed up to edi. Why not directly back up ebx to edi?
BTW, Tim had another suggestion on your original CL (https://review.coreboot.org/c/coreboot/+/47983/5/src/soc/intel/common/block/...):
what about using %edx directly? ``` mov %eax, %edx mov $0x01, %eax shl %dl, %eax subl $0x01, %eax ```
https://review.coreboot.org/c/coreboot/+/48286/2/src/soc/intel/common/block/... PS2, Line 436: /* : * Program MSR 0x1892 IA32_CR_SF_QOS_MASK_2 with : * total number of LLC ways : */ : movl $IA32_CR_SF_QOS_MASK_2, %ecx : xorl %edx, %edx : wrmsr This can be dropped completely: https://review.coreboot.org/c/coreboot/+/47983/5/src/soc/intel/common/block/...
Hello build bot (Jenkins), Tim Wawrzynczak, Patrick Rudolph, Shreesh Chhabbi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#3).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 38 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/3/src/soc/intel/common/block/... PS3, Line 442: mov %edi, %ecx /* Restore number of ways */ This will have to be outside the #if because ecx is expected to have number of ways in the end. BTW, please see Tim's comment on previous patchset about using edx instead of ecx. That way you don't need to backup/restore to ecx.
Hello build bot (Jenkins), Tim Wawrzynczak, Patrick Rudolph, Shreesh Chhabbi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#4).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 39 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/4
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/3/src/soc/intel/common/block/... PS3, Line 442: mov %edi, %ecx /* Restore number of ways */
This will have to be outside the #if because ecx is expected to have number of ways in the end. […]
I moved it out Furquan. Good catch. Thanks. I will check Tim's comment.
Hello build bot (Jenkins), Tim Wawrzynczak, Patrick Rudolph, Shreesh Chhabbi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#5).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 35 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 422: This file mostly uses tabs after the command. It would be good to use that here and in the statements below as well.
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 426: dl Woops. Just remembered: shl only allows immediate or %cl. Sorry about the suggestion :(.
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 435: /* : * Program MSR 0x1892 IA32_CR_SF_QOS_MASK_2 with : * total number of LLC ways : */ : movl $IA32_CR_SF_QOS_MASK_2, %ecx : xorl %edx, %edx : wrmsr Like mentioned on the earlier CL, I think IA32_CR_SF_QOS_MASK_2 is expected to be kept untouched.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 426: dl
Woops. Just remembered: shl only allows immediate or %cl. Sorry about the suggestion :(.
Sorry, I misread the encoding
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 435: /* : * Program MSR 0x1892 IA32_CR_SF_QOS_MASK_2 with : * total number of LLC ways : */ : movl $IA32_CR_SF_QOS_MASK_2, %ecx : xorl %edx, %edx : wrmsr
Like mentioned on the earlier CL, I think IA32_CR_SF_QOS_MASK_2 is expected to be kept untouched.
Do we have a definition for the reset value for that MSR? I know the doc said to program them it as 0.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 426: dl
Sorry, I misread the encoding
``` mov $0x01, %eax shl %cl, %eax shl %cl, %eax subl $0x01, %eax ``` ?
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 426: dl
Hi Tim, Furquan, shall I back up into edi and restore? what do you suggest is right thing to do here?
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 435: /* : * Program MSR 0x1892 IA32_CR_SF_QOS_MASK_2 with : * total number of LLC ways : */ : movl $IA32_CR_SF_QOS_MASK_2, %ecx : xorl %edx, %edx : wrmsr
Do we have a definition for the reset value for that MSR? I know the doc said to program them it as […]
Yes Furquan, I was planning to now address this part now that other bugs were addressed in the patch. IA32_L3_MASK_2 and IA32_CR_SF_QOS_MASK_2 MSRs take effect only if IA32_PQR_ASSOC is programmed with value of 0x2. Currently we program it to 0x1 in the end. So I think that's why CPU team recommended to leave IA32_CR_SF_QOS_MASK_2 to default. I can remove programming of IA32_CR_SF_QOS_MASK_2. Tim from EDS document, reset default for IA32_CR_SF_QOS_MASK_x is 0FFFFFFFh.
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 435: /* : * Program MSR 0x1892 IA32_CR_SF_QOS_MASK_2 with : * total number of LLC ways : */ : movl $IA32_CR_SF_QOS_MASK_2, %ecx : xorl %edx, %edx : wrmsr
Yes Furquan, I was planning to now address this part now that other bugs were addressed in the patch […]
Do you both suggest this is ok?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 426: dl
Hi Tim, Furquan, shall I back up into edi and restore? what do you suggest is right thing to do here […]
What about the sequence I posted above? ``` mov $0x01, %eax shl %cl, %eax shl %cl, %eax subl $0x01, %eax ```
just do the shifting using the value from %cl two times instead of bothering with the multiply; this doesn't require modifying %eax or backing it up
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 435: /* : * Program MSR 0x1892 IA32_CR_SF_QOS_MASK_2 with : * total number of LLC ways : */ : movl $IA32_CR_SF_QOS_MASK_2, %ecx : xorl %edx, %edx : wrmsr
Do you both suggest this is ok?
The most authoritative info we have is from the document Shyam sent us; it said: ``` IA32_L3_SF_MASK_2 can be left to reset default. [To be updated with additional information] ```
so I guess I would just not program it
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 426: dl
What about the sequence I posted above? […]
Yes Tim. Thanks. mov $0x01, %eax -> eax = 0x1 shl %cl, %eax -> cl = 0xc; eax = 0x1000 shl %cl, %eax -> cl = 0xc; eax = 0x1000000 subl $0x01, %eax -> eax = 0xffffff
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 422:
This file mostly uses tabs after the command. […]
Ok Furquan.
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 435: /* : * Program MSR 0x1892 IA32_CR_SF_QOS_MASK_2 with : * total number of LLC ways : */ : movl $IA32_CR_SF_QOS_MASK_2, %ecx : xorl %edx, %edx : wrmsr
The most authoritative info we have is from the document Shyam sent us; it said: […]
Ok.
Hello build bot (Jenkins), Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#6).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 26 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/6
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/5/src/soc/intel/common/block/... PS5, Line 426: dl
Yes Tim. Thanks. […]
Ack
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48286/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/2/src/soc/intel/common/block/... PS2, Line 417: mov %ecx, %edi
This seems weird. ebx was backed up to ecx and 2 lines below that ecx is backed up to edi. […]
Ack
https://review.coreboot.org/c/coreboot/+/48286/2/src/soc/intel/common/block/... PS2, Line 436: /* : * Program MSR 0x1892 IA32_CR_SF_QOS_MASK_2 with : * total number of LLC ways : */ : movl $IA32_CR_SF_QOS_MASK_2, %ecx : xorl %edx, %edx : wrmsr
This can be dropped completely: https://review.coreboot. […]
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 6:
I think this modified NEM entry sequence looks good, but I think we should also make the additions to the NEM teardown too: "Set IA32_CR_PQR_ASSOC (MSR C8Fh) bits [32:33] to 00b and restore IA32_L3_MASK_1/2 and IA32_L3_SF_MASK_1/2 to their reset default values."
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/6/src/soc/intel/common/block/... PS6, Line 420: (0x1 << X) << X) I think saying (0x1 << (X * 2) - 1) or just saying that SF mask has double the number of bits than the number of ways is easier to understand.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48286/6//COMMIT_MSG@14 PS6, Line 14: [to be updated] Needs to be updated before submitting CL.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#7).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 29 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/7/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/7/src/soc/intel/common/block/... PS7, Line 416: edi I see you went back to using edi. Was that intentional? Or just a bad push?
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/7/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/7/src/soc/intel/common/block/... PS7, Line 416: edi
I see you went back to using edi. […]
Yes Furquan. I was testing the change yesterday. I observed the system hang with original change. I was looking at the code again, while writing to 0x1891 MSR, we are utilizing the ecx in below command. So it no longer holds the number of ways. Can you and Tim please provide your thoughts? movl $IA32_CR_SF_QOS_MASK_1, %ecx
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/7/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/7/src/soc/intel/common/block/... PS7, Line 416: edi
Yes Furquan. I was testing the change yesterday. I observed the system hang with original change. […]
Ah yes, that is right!! wrmsr requires register address in ecx.
This looks okay. Just one comment: Using edi to back up the number of ways can be done under #if CONFIG(CAR_HAS_SF_MASKS) and restoring back to ecx just before the #endif. This way, the save/restore would only be required for CAR_HAS_SF_MASKS.
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/7/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/7/src/soc/intel/common/block/... PS7, Line 416: edi
Ah yes, that is right!! wrmsr requires register address in ecx. […]
Yes that's very good catch. I will change now.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#8).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 29 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/8
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/8/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/8/src/soc/intel/common/block/... PS8, Line 418: bx This will have to be ecx since ebx actually holds the non-eviction mask at this point.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#9).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 29 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/9
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 9: Code-Review+2
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#10).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/common/block/cpu/car/exit_car.S M src/soc/intel/tigerlake/Kconfig 4 files changed, 37 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/10
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#11).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=Build volteer build and boot to Chrome OS.
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/common/block/cpu/car/exit_car.S M src/soc/intel/tigerlake/Kconfig 4 files changed, 37 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/11
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#12).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=Build volteer build and boot to Chrome OS on Delbin EVT.
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/common/block/cpu/car/exit_car.S M src/soc/intel/tigerlake/Kconfig 4 files changed, 37 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/12
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#13).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=Build volteer build and boot on Delbin EVT.
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/common/block/cpu/car/exit_car.S M src/soc/intel/tigerlake/Kconfig 4 files changed, 37 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/13
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/13/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/13/src/soc/intel/common/block... PS13, Line 99: Am I just the code to reset IA32_L3_MASK_x ?
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/13/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/13/src/soc/intel/common/block... PS13, Line 99:
Am I just the code to reset IA32_L3_MASK_x ?
Sorry I looked at the "CONFIG(INTEL_CAR_CQOS)" code section for teardown. Under "CONFIG(INTEL_CAR_NEM_ENHANCED)", it is not there. We need to add. I dumped the reset default value for IA32_L3_MASK_x. It is 0xFFF (all ways marked for eviction). I think we need to recalculate the available number of ways here since it could change between the processors. Tim do you have any suggestions to save it when we calculate it in NEM initialization code?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/13/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/13/src/soc/intel/common/block... PS13, Line 99:
Sorry I looked at the "CONFIG(INTEL_CAR_CQOS)" code section for teardown. […]
I agree, I think it's safest to just recalculate the available ways. Also since most of this is "straight-line" assembly, i.e., not much in the way of subroutines, etc. I would just crib the code from the cache_as_ram.S, adjusting register usage as need be .
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#14).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/common/block/cpu/car/exit_car.S M src/soc/intel/tigerlake/Kconfig 4 files changed, 85 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/14
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/13/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/13/src/soc/intel/common/block... PS13, Line 99:
I agree, I think it's safest to just recalculate the available ways. […]
Hi Tim, I added the code. Please help to review.
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/6/src/soc/intel/common/block/... PS6, Line 420: (0x1 << X) << X)
I think saying (0x1 << (X * 2) - 1) or just saying that SF mask has double the number of bits than t […]
Hi Furquan, I changed the comment.
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 14:
(3 comments)
`
https://review.coreboot.org/c/coreboot/+/48286/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48286/6//COMMIT_MSG@14 PS6, Line 14: [to be updated]
Needs to be updated before submitting CL.
Done
https://review.coreboot.org/c/coreboot/+/48286/7/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/7/src/soc/intel/common/block/... PS7, Line 416: edi
Yes that's very good catch. I will change now.
Done
https://review.coreboot.org/c/coreboot/+/48286/8/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/8/src/soc/intel/common/block/... PS8, Line 418: bx
This will have to be ecx since ebx actually holds the non-eviction mask at this point.
Ack
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#15).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=Build volteer build and boot on Delbin EVT.
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/common/block/cpu/car/exit_car.S M src/soc/intel/tigerlake/Kconfig 4 files changed, 85 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/15
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/15/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/15/src/soc/intel/common/block... PS15, Line 135: ebx shouldn't this be eax?, ebx is way size
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/15/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/15/src/soc/intel/common/block... PS15, Line 135: ebx
shouldn't this be eax?, ebx is way size
[Had discussion with Tim, to make the CAR exit code simpler, we planned to add Kconfig fields for reset default values for QOS_MASK & SF_MASK MSRs. Use them to reset the values in CAR exit code. Making this change now.]
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#16).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/cpu/Kconfig M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/common/block/cpu/car/exit_car.S M src/soc/intel/tigerlake/Kconfig 5 files changed, 87 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/16
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/16/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/16/src/soc/intel/common/block... PS16, Line 105: trailing whitespace
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Shreesh Chhabbi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48286
to look at the new patch set (#17).
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
Bug=b:171601324 BRANCH=volteer Test=[to be updated]
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/cpu/Kconfig M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/common/block/cpu/car/exit_car.S M src/soc/intel/tigerlake/Kconfig 5 files changed, 87 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/17
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 17:
(4 comments)
https://review.coreboot.org/c/coreboot/+/48286/17/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/48286/17/src/cpu/Kconfig@24 PS17, Line 24: hex Can you please add help text that outlines what this is and what it is used for?
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 100: IA32_L3_MASK_1 Questions: 1. CLOS selector is set to 0 above. Once that is done, do the L3_MASK and SF_QOS_MASK have any impact? Or are they effectively disabled?
2. If we don't ever configure these bits here, wouldn't it mean that the entity that uses it next is responsible for setting up the L3_MASK and SF_QOS_MASK before enabling CLOS selector again? Is that a dangerous assumption to make?
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 101: CONFIG_IA32_L3_MASK_1_DEFAULT Since this is used by all soc/intel/ platforms, what is getting programmed here for platforms other than TGL?
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... PS17, Line 255: 0xffff Isn't the default mask dependent on number of ways?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 101: CONFIG_IA32_L3_MASK_1_DEFAULT
Since this is used by all soc/intel/ platforms, what is getting programmed here for platforms other […]
If we don't have the values for other platforms yet, then this needs to be guarded by CONFIG_SOC_INTEL_TIGERLAKE
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... PS17, Line 255: 0xffff
Isn't the default mask dependent on number of ways?
The eNEM programming instructions said to set these MSRs back to their reset default values, which came from the EDS; I didn't say any special notes about the reset value being dependent on the cache size
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 101: CONFIG_IA32_L3_MASK_1_DEFAULT
If we don't have the values for other platforms yet, then this needs to be guarded by CONFIG_SOC_INT […]
I think it is generally wrong to include SoC specific config checks in common code. Since this is equally applicable to all SoCs using this CAR setup, it would be good to update all the impacted SoCs to provide the right configuration.
BTW, same comment as SF_MASK, why are we not using way # to calculate this mask at runtime?
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... PS17, Line 255: 0xffff
The eNEM programming instructions said to set these MSRs back to their reset default values, which c […]
Have we confirmed that? Both L3_MASK and SF_QOS_MASK are related to capacity bit masks which are different dependent on number of ways.
BTW, EDS Vol2 says that the default value for SF_QOS_MASK is 0FFFFFFFh which is different than what is being programmed here.
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 17:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 100: IA32_L3_MASK_1
Questions: […]
Yes you are right. CLOS selector definition says, if bits are set to 00b, no mask will be applied for L3 CQOS. That's how eNEM CAR teardown code is working so far. We thought that it would be ideal to program them to reset defaults back as mentioned in document. What do you suggest?
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 101: CONFIG_IA32_L3_MASK_1_DEFAULT
I think it is generally wrong to include SoC specific config checks in common code. […]
Ok. This current approach will cause maintenance issues right to keep track of reset defaults for each program. Since CLOS is 0, I think we don't need to touch the CAR exit code. What do you both suggest?
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... PS17, Line 255: 0xffff
Have we confirmed that? Both L3_MASK and SF_QOS_MASK are related to capacity bit masks which are dif […]
Hi Furquan, actually I printed values of L3_MASK & SF_QOS_MASK as post codes on EC console before eNEM code starts. I see the values of 0xfff and 0xffff respectively. I had asked about EDS reset default value mismatching with the one read with Eric. He had mentioned that it needs an update in the document.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 100: IA32_L3_MASK_1
Yes you are right. […]
I tried to dig up history on why these masks were not programmed in the first place for older platforms(eNEM guide for SKL also captures this requirement, but we never had the code do it) but unfortunately that CL seems to have been lost from coreboot gerrit.
Are there any side-effects of not programming the masks back to their default? In my opinion, the user of CLOS should be responsible for setting up the masks. But, I am not sure if I am missing any subtle behavior/expectation.
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... PS17, Line 255: 0xffff
Hi Furquan, actually I printed values of L3_MASK & SF_QOS_MASK as post codes on EC console before eN […]
That sounds really weird. SF_QOS_MASK sets bits for only 8*2 ways by default. Is that true for all SKUs?
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48286/15/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/15/src/soc/intel/common/block... PS15, Line 135: ebx
[Had discussion with Tim, to make the CAR exit code simpler, we planned to add Kconfig fields for re […]
Ack
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... PS17, Line 255: 0xffff
That sounds really weird. SF_QOS_MASK sets bits for only 8*2 ways by default. […]
That's good point Furquan. I checked on one SKU alone. Reset default value I see is 0xffff on that SKU and I can see that total number of ways is 12. Setting CLOS selector value to 00b is sufficient in my opinion.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 101: CONFIG_IA32_L3_MASK_1_DEFAULT
Ok. […]
If you decide to drop the programming from exit car, I would recommend adding a comment here indicating why we think it is safe to do so (even though it does not match Intel programming guidelines).
Also, please wait to hear from Tim to see what he thinks about this.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 101: CONFIG_IA32_L3_MASK_1_DEFAULT
If you decide to drop the programming from exit car, I would recommend adding a comment here indicat […]
It has I guess always seemed to work without reprogramming the L3_MASK anyways and logically what you say makes sense, it just worries me a little when somebody went to the trouble to call out a step (maybe it was just CYA, who knows) and we don't follow it.
I guess if we run through a bunch of reboot tests w/o the extra mask programming in teardown and don't have any issues, it's probably okay.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 101: CONFIG_IA32_L3_MASK_1_DEFAULT
It has I guess always seemed to work without reprogramming the L3_MASK anyways and logically what yo […]
Capturing next steps based on discussion: 1. Drop the code from exit_car.S (i.e. no need to add any new code) 2. Add a comment explaining why we are not configuring the masks to default on teardown 3. Perform tests as Tim requested.
Shreesh - does that make sense?
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 101: CONFIG_IA32_L3_MASK_1_DEFAULT
Capturing next steps based on discussion: […]
Yes Furquan. I agree with you & Tim. I will complete the patch. I am taking help from Ravi since my dev machine hit some issues.
Ravishankar Sarawadi has uploaded a new patch set (#18) to the change originally created by Shreesh Chhabbi. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
During CAR teardown code, MSRs IA32_L3_MASK_x & IA32_CR_SF_QOS_MASK_x are not being reset to default as per the doc NEM-Enhanced-Mode-Whitepaper-Tigerlake-draft-WW46.5. Resetting the value of IA32_PQR_ASSOC[32:33] to 00b is sufficient.
Bug=b:171601324 BRANCH=volteer Test=Build and boot to ChromeOS on Delbin.
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/common/block/cpu/car/exit_car.S M src/soc/intel/tigerlake/Kconfig 4 files changed, 31 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/18
Ravishankar Sarawadi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18:
Not sure why it's indicating merge conflict though I don's see any.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 415: use number of ways to prepare SF mask I think this comment should be on line 418 and the comment on line 418 should be here? When SF masks are not used, this comment doesn't really make sense here.
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 99: Are you planning to add a comment here indicating why masks are not programmed during teardown?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48286/18//COMMIT_MSG@15 PS18, Line 15: 32:33 We only reset bits `[1:0]` in teardown
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48286/18//COMMIT_MSG@15 PS18, Line 15: 32:33
We only reset bits `[1:0]` in teardown
That's a good point. Teardown path does not take into account COS_MAPPED_TO_MSB.
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48286/18//COMMIT_MSG@15 PS18, Line 15: 32:33
That's a good point. Teardown path does not take into account COS_MAPPED_TO_MSB.
Doesn't below code make edx Bits[0:1] = 00b and contents of edx will be written to Bits[32:63] of MSR?
and $~IA32_PQR_ASSOC_MASK, %edx wrmsr
IA32_PQR_ASSOC_MASK is defined as below. It has the value 0x3. #define IA32_PQR_ASSOC_MASK (1 << 0 | 1 << 1)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48286/18//COMMIT_MSG@15 PS18, Line 15: 32:33
Doesn't below code make edx Bits[0:1] = 00b and contents of edx will be written to Bits[32:63] of MS […]
You are correct. It does.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 94: /* Reset CLOS selector to 0 */ : mov $IA32_PQR_ASSOC, %ecx : rdmsr : and $~IA32_PQR_ASSOC_MASK, %edx : wrmsr Shouldn't we have the same logic here as in NEM programming, e.g., ``` mov $IA32_PQR_ASSOC, %ecx rdmsr #if CONFIG(COS_MAPPED_TO_MSB) and $~IA32_PQR_ASSOC_MASK, %edx #else and $~IA32_PQR_ASSOC_MASK, %eax #endif wrmsr ```
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 94: /* Reset CLOS selector to 0 */ : mov $IA32_PQR_ASSOC, %ecx : rdmsr : and $~IA32_PQR_ASSOC_MASK, %edx : wrmsr
Shouldn't we have the same logic here as in NEM programming, e.g., […]
Yes Tim, I think so. I was looking at following file. Comment captures that bits 33:32 are used in following MSR. So I think copying to edx was made purposefully for earlier programs as well.
src/include/cpu/x86/msr.h /* MSR bits 33:32 encode slot number 0-3 */ #define IA32_PQR_ASSOC_MASK (1 << 0 | 1 << 1)
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 94: /* Reset CLOS selector to 0 */ : mov $IA32_PQR_ASSOC, %ecx : rdmsr : and $~IA32_PQR_ASSOC_MASK, %edx : wrmsr
Yes Tim, I think so. I was looking at following file. […]
If we make following change, COS_MAPPED_TO_MSB has to be selected for all the programs. Right?
mov $IA32_PQR_ASSOC, %ecx rdmsr #if CONFIG(COS_MAPPED_TO_MSB) and $~IA32_PQR_ASSOC_MASK, %edx #else and $~IA32_PQR_ASSOC_MASK, %eax #endif wrmsr
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 94: /* Reset CLOS selector to 0 */ : mov $IA32_PQR_ASSOC, %ecx : rdmsr : and $~IA32_PQR_ASSOC_MASK, %edx : wrmsr
If we make following change, COS_MAPPED_TO_MSB has to be selected for all the programs. Right? […]
It's selected for JSL & TGL right now. When COS_MAPPED_TO_MSB is set, then we use bits 32 & 33 for setting the CLOS selector, otherwise (COS_MAPPED_TO_MSB is not set) it's bits 0 and 1 (https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...), so the teardown code should clear the correct set of bits.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 94: /* Reset CLOS selector to 0 */ : mov $IA32_PQR_ASSOC, %ecx : rdmsr : and $~IA32_PQR_ASSOC_MASK, %edx : wrmsr
If we make following change, COS_MAPPED_TO_MSB has to be selected for all the programs. Right? […]
There is definitely some inconsistency here. I started looking up the old documents to understand why we had bits 33:32 being cleared on older platforms (KBL, CML, APL, GLK, etc.). Here is what I found:
Doc#564550 (SKL eNEM): This refers to 0xc8f as LLC_WAY_MASK_CONTROL and talks about setting lower 2 bits for way selection.
Doc#567985 (APL eNEM): This refers to 0xc8f as IA32_PQR_ASSOC and talks about setting bits 33:32.
Looking at Intel SDM, IA32_PQR_ASSOC says that bits 33:32 represent class of service (CLOS).
Based on the above details, I wonder if bits 1:0 were used for way selection only on older platforms like SKL and newer platforms have actually switched to using bits 33:32 as per SDM. However, it is difficult to get a complete picture here since none of the Processor EDS have IA32_PQR_ASSOC(0xc8f) defined.
Given this inconsistency, I think we should: 1. Take this change for TGL to configure SF mask forward without any other change in exit_car.S 2. Let's track the definition of IA32_PQR_ASSOC for each generation and ensure we set up things correctly for all platforms. It might be the case that COS_MAPPED_TO_MSB would have to be inverted COS_MAPPED_TO_LSB and set only on older platforms like SKL. But, this requires more details from Intel.
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 18:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48286/18//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48286/18//COMMIT_MSG@15 PS18, Line 15: 32:33
You are correct. It does.
Done
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 94: /* Reset CLOS selector to 0 */ : mov $IA32_PQR_ASSOC, %ecx : rdmsr : and $~IA32_PQR_ASSOC_MASK, %edx : wrmsr
There is definitely some inconsistency here. […]
Yes Furquan. Agreed. We can do this.
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 99:
Are you planning to add a comment here indicating why masks are not programmed during teardown?
Done
Ravishankar Sarawadi has uploaded a new patch set (#19) to the change originally created by Shreesh Chhabbi. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
During CAR teardown code, MSRs IA32_L3_MASK_x & IA32_CR_SF_QOS_MASK_x are not being reset to default as per the doc NEM-Enhanced-Mode-Whitepaper-Tigerlake-draft-WW46.5. Resetting the value of IA32_PQR_ASSOC[32:33] to 00b is sufficient.
Bug=b:171601324 BRANCH=volteer Test=Build and boot to ChromeOS on Delbin.
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 30 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/48286/19
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 19:
(4 comments)
https://review.coreboot.org/c/coreboot/+/48286/17/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/48286/17/src/cpu/Kconfig@24 PS17, Line 24: hex
Can you please add help text that outlines what this is and what it is used for?
Done
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 100: IA32_L3_MASK_1
I tried to dig up history on why these masks were not programmed in the first place for older platfo […]
Done
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/common/block... PS17, Line 101: CONFIG_IA32_L3_MASK_1_DEFAULT
Yes Furquan. I agree with you & Tim. I will complete the patch. […]
Done
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/48286/17/src/soc/intel/tigerlake/Kc... PS17, Line 255: 0xffff
That's good point Furquan. I checked on one SKU alone. […]
Done
Ravishankar Sarawadi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 415: use number of ways to prepare SF mask
I think this comment should be on line 418 and the comment on line 418 should be here? When SF masks […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 94: /* Reset CLOS selector to 0 */ : mov $IA32_PQR_ASSOC, %ecx : rdmsr : and $~IA32_PQR_ASSOC_MASK, %edx : wrmsr
Yes Furquan. Agreed. We can do this.
Shreesh - are you raising a separate bug to follow-up on this inconsistency across platforms?
Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/48286/18/src/soc/intel/common/block... PS18, Line 94: /* Reset CLOS selector to 0 */ : mov $IA32_PQR_ASSOC, %ecx : rdmsr : and $~IA32_PQR_ASSOC_MASK, %edx : wrmsr
Shreesh - are you raising a separate bug to follow-up on this inconsistency across platforms?
Yes Furquan, I will raise a new bug for addressing the inconsistency in CLOS_Selector programming across all programs.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 19: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
Patch Set 19: Code-Review+2
Thanks Shreesh & Ravi!
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48286 )
Change subject: src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL ......................................................................
src/soc/intel: Add support for CAR_HAS_SF_MASKS and select for TGL
Program IA32_CR_SF_QOS_MASK_x MSRs under CAR_HAS_SF_MASKS config option. Select CAR_HAS_SF_MASKS for Tigerlake.
During CAR teardown code, MSRs IA32_L3_MASK_x & IA32_CR_SF_QOS_MASK_x are not being reset to default as per the doc NEM-Enhanced-Mode-Whitepaper-Tigerlake-draft-WW46.5. Resetting the value of IA32_PQR_ASSOC[32:33] to 00b is sufficient.
Bug=b:171601324 BRANCH=volteer Test=Build and boot to ChromeOS on Delbin.
Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Change-Id: Iabf7f387fb5887aca10158788599452c3f2df7e8 Signed-off-by: Shreesh Chhabbi shreesh.chhabbi@intel.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48286 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/common/block/cpu/car/cache_as_ram.S M src/soc/intel/tigerlake/Kconfig 3 files changed, 30 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/common/block/cpu/Kconfig b/src/soc/intel/common/block/cpu/Kconfig index 912760e..2b630d0 100644 --- a/src/soc/intel/common/block/cpu/Kconfig +++ b/src/soc/intel/common/block/cpu/Kconfig @@ -51,6 +51,14 @@ ENHANCED NEM guarantees that modified data is always kept in cache while clean data is replaced.
+config CAR_HAS_SF_MASKS + bool + depends on INTEL_CAR_NEM_ENHANCED + help + In the case of non-inclusive cache architecture Snoop Filter MSR + IA32_L3_SF_MASK_x programming is required along with the data ways. + This is applicable for TGL and beyond. + config COS_MAPPED_TO_MSB bool depends on INTEL_CAR_NEM_ENHANCED diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram.S b/src/soc/intel/common/block/cpu/car/cache_as_ram.S index b60e797..aaf6af7 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram.S @@ -412,8 +412,27 @@ subl $0x01, %eax
set_eviction_mask: - mov %ebx, %ecx /* back up the number of ways */ - mov %eax, %ebx /* back up the non-eviction mask */ + mov %ebx, %ecx /* back up number of ways */ + mov %eax, %ebx /* back up the non-eviction mask*/ +#if CONFIG(CAR_HAS_SF_MASKS) + mov %ecx, %edi /* use number of ways to prepare SF mask */ + /* + * SF mask is programmed with the double number of bits than + * the number of ways + */ + mov $0x01, %eax + shl %cl, %eax + shl %cl, %eax + subl $0x01, %eax /* contains SF mask */ + /* + * Program MSR 0x1891 IA32_CR_SF_QOS_MASK_1 with + * total number of LLC ways + */ + movl $IA32_CR_SF_QOS_MASK_1, %ecx + xorl %edx, %edx + wrmsr + mov %edi, %ecx /* restore number of ways */ +#endif /* * Program MSR 0xC91 IA32_L3_MASK_1 * This MSR contain one bit per each way of LLC @@ -431,7 +450,6 @@ movl $IA32_L3_MASK_1, %ecx xorl %edx, %edx wrmsr - /* * Program MSR 0xC92 IA32_L3_MASK_2 * This MSR contain one bit per each way of LLC @@ -460,7 +478,6 @@ movl $0x02, %eax #endif wrmsr - movl $CONFIG_DCACHE_RAM_BASE, %edi movl $CONFIG_DCACHE_RAM_SIZE, %ecx shr $0x02, %ecx diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig index 7629c7e..c7e10a9 100644 --- a/src/soc/intel/tigerlake/Kconfig +++ b/src/soc/intel/tigerlake/Kconfig @@ -25,6 +25,7 @@ select HAVE_SMI_HANDLER select IDT_IN_EVERY_STAGE select INTEL_CAR_NEM_ENHANCED if !INTEL_CAR_NEM + select CAR_HAS_SF_MASKS if INTEL_CAR_NEM_ENHANCED select COS_MAPPED_TO_MSB if INTEL_CAR_NEM_ENHANCED select INTEL_GMA_ACPI select INTEL_GMA_ADD_VBT if RUN_FSP_GOP
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
Ravishankar Sarawadi (Code Review)
-
Shreesh Chhabbi (Code Review)
-
Tim Wawrzynczak (Code Review)