Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35187 )
Change subject: [NOTFORMERGE] soc/intel exit_car.S ......................................................................
[NOTFORMERGE] soc/intel exit_car.S
Change-Id: Icd369029d9bbf0aa72923513952e1aacf88c0f40 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/exit_car.S M src/cpu/intel/car/non-evict/exit_car.S M src/cpu/intel/car/p4-netburst/exit_car.S M src/drivers/amd/agesa/cache_as_ram.S M src/soc/amd/common/block/cpu/car/exit_car.S M src/soc/intel/common/block/cpu/car/exit_car.S 6 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/35187/1
diff --git a/src/arch/x86/exit_car.S b/src/arch/x86/exit_car.S index 769a758..8ae7a66 100644 --- a/src/arch/x86/exit_car.S +++ b/src/arch/x86/exit_car.S @@ -34,6 +34,7 @@ call gdt_init
/* chipset_teardown_car() is expected to disable cache-as-ram. */ + /* ..... */ call chipset_teardown_car
/* Enable caching if not already enabled. */ @@ -42,6 +43,7 @@ mov %eax, %cr0
/* Ensure cache is clean. */ + /* ..... */ invd
/* Set up new stack. */ diff --git a/src/cpu/intel/car/non-evict/exit_car.S b/src/cpu/intel/car/non-evict/exit_car.S index 8adc5f6..ee71d81 100644 --- a/src/cpu/intel/car/non-evict/exit_car.S +++ b/src/cpu/intel/car/non-evict/exit_car.S @@ -29,6 +29,7 @@ post_code(0x30)
/* Disable cache. */ + /* ..... */ movl %cr0, %eax orl $CR0_CacheDisable, %eax movl %eax, %cr0 diff --git a/src/cpu/intel/car/p4-netburst/exit_car.S b/src/cpu/intel/car/p4-netburst/exit_car.S index 3b99128..ba57546 100644 --- a/src/cpu/intel/car/p4-netburst/exit_car.S +++ b/src/cpu/intel/car/p4-netburst/exit_car.S @@ -27,6 +27,7 @@ post_code(0x30)
/* Disable cache. */ + /* ..... */ movl %cr0, %eax orl $CR0_CacheDisable, %eax movl %eax, %cr0 diff --git a/src/drivers/amd/agesa/cache_as_ram.S b/src/drivers/amd/agesa/cache_as_ram.S index 4f0bb3f..37dfd3f 100644 --- a/src/drivers/amd/agesa/cache_as_ram.S +++ b/src/drivers/amd/agesa/cache_as_ram.S @@ -131,6 +131,7 @@ #endif
/* Disable cache */ + /* ..... */ movl %cr0, %eax orl $CR0_CacheDisable, %eax movl %eax, %cr0 diff --git a/src/soc/amd/common/block/cpu/car/exit_car.S b/src/soc/amd/common/block/cpu/car/exit_car.S index f9d056e..9a36df0 100644 --- a/src/soc/amd/common/block/cpu/car/exit_car.S +++ b/src/soc/amd/common/block/cpu/car/exit_car.S @@ -23,6 +23,7 @@ pop %esp
/* Disable cache */ + /* ..... */ movl %cr0, %eax orl $CR0_CacheDisable, %eax movl %eax, %cr0 diff --git a/src/soc/intel/common/block/cpu/car/exit_car.S b/src/soc/intel/common/block/cpu/car/exit_car.S index ab7886c..8aa8bfb 100644 --- a/src/soc/intel/common/block/cpu/car/exit_car.S +++ b/src/soc/intel/common/block/cpu/car/exit_car.S @@ -44,6 +44,10 @@ */ pop %ebx
+ + /* Disable cache ??? */ + + /* Disable MTRRs. */ mov $(MTRR_DEF_TYPE_MSR), %ecx rdmsr @@ -55,6 +59,7 @@ car_nem_teardown:
/* invalidate cache contents. */ + /* maybe redundant ? */ invd
/* Knock down bit 1 then bit 0 of NEM control not combining steps. */ @@ -69,6 +74,8 @@ .global car_cqos_teardown car_cqos_teardown:
+ /* inconsistent, no invd here */ + /* Go back to all-evicting mode, set both masks to all-1s */ mov $MSR_L2_QOS_MASK(0), %ecx rdmsr @@ -91,6 +98,7 @@ car_nem_enhanced_teardown:
/* invalidate cache contents. */ + /* maybe redundant ? */ invd
/* Knock down bit 1 then bit 0 of NEM control not combining steps. */
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35187 )
Change subject: [NOTFORMERGE] soc/intel exit_car.S ......................................................................
Patch Set 1:
Is it intentional that soc/intel does not toggle cache-disable bit CR0.CD while doing CAR teardown? Or did I just not find it?
Also, are there some compelling reasons to support "config FSP_CAR" and late_car_teardown()?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35187 )
Change subject: [NOTFORMERGE] soc/intel exit_car.S ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... PS2, Line 48: /* Disable cache ??? */ Transactions would still hit in the cache even if cache is disabled.
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... PS2, Line 62: /* maybe redundant ? */ Not necessarily. What were you assuming at this point? valid cache lines do not become invalid on a cache disable operation.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35187 )
Change subject: [NOTFORMERGE] soc/intel exit_car.S ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... PS2, Line 48: /* Disable cache ??? */
Transactions would still hit in the cache even if cache is disabled.
Perhaps so, see comment on arch/x86/exit_car.S line 36.
There will be no CR0.CD 1->0 transition when we return to arch/x86/exit_car.S, if we skip CR0.CD 0->1 transition here. Everyone else seems to do it. I thought both those transitions have some significance wrt MTRR reproramming.
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... PS2, Line 62: /* maybe redundant ? */
Not necessarily. […]
There is invd once we return to caller, arch/x86/exit_car.S line 45. And one of these three paths for soc/intel does not have it either.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35187 )
Change subject: [NOTFORMERGE] soc/intel exit_car.S ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... PS2, Line 48: /* Disable cache ??? */
Perhaps so, see comment on arch/x86/exit_car.S line 36.
"/* chipset_teardown_car() is expected to disable cache-as-ram. */"
That one? That doesn't say cache. It says cache as ram.
There will be no CR0.CD 1->0 transition when we return to arch/x86/exit_car.S, if we skip CR0.CD 0->1 transition here. Everyone else seems to do it. I thought both those transitions have some significance wrt MTRR reproramming.
Not really. x86 is kinda horrible here. With CR0.CD=1 it means fills (for fresh reads or fresh writes) won't go into cache. But cache is snooped for either transaction. A read can hit in cache, and so can a write (which I think flushes the line out).
As for MTRR programming proper we're disabling those below. I 99%sure that would stop the snooping since there's nothing indicating to the processor to attempt the snoop (MTRRs are disabled). Because of this data is abandoned in cache and the invd knocks down all the valid bits.
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... PS2, Line 62: /* maybe redundant ? */
There is invd once we return to caller, arch/x86/exit_car.S line 45. […]
Double invd doesn't matter unless there's a dirty line which the ABI basically says you can only use registers. That's why we keep the return value in ebx -- and not a stack. Multiple invds are ok in the current sequence, but the arch/x86/exit_car.S doesn't assume implementation details of the chipset. I do agree that invd should be on line 77 for consistency. This assumes everything dirty has been flushed from cache to memory.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35187 )
Change subject: [NOTFORMERGE] soc/intel exit_car.S ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... PS2, Line 48: /* Disable cache ??? */
Perhaps so, see comment on arch/x86/exit_car.S line 36. […]
Patrick Rudolph CB:34791 Aug 08 20:26
My tests on Sandy Bridge showed that MTRRs are only updated on CR0.CD=1 to CR0.CD=0 transitions. All of AMD and early Intel CPUs follow this scheme and update MTRRs with cache disabled. Isn't that transition required any more starting with APL?
From that commit comments, I did not find reference to Intel docs describing NEM at a satisfactory level of details. Instead I read about random lockups in ramstage (MP init) and consistent delay being triggered inside vboot payload after we had only added WB MTRRs in postcar frame. With access to BWG you probably can find exact paragraphs showing that it is no longer required to toggle CR0.CD to correcly exit non-evict mode??
The sequence for CONFIG_INTEL_CAR_NEM below is the same for sandy/ivy, except that the implementation Google provided does set CR0.CD here first. Having peeked into some random FSP-M disassembly, CR0.CD was definetly getting set at several locations, but with a 5 minute effort it's hard to see code that is on TempRamExit() execution path. That's the closed-source CAR teardown implementation.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35187 )
Change subject: [NOTFORMERGE] soc/intel exit_car.S ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/exit_car.S:
https://review.coreboot.org/c/coreboot/+/35187/2/src/soc/intel/common/block/... PS2, Line 48: /* Disable cache ??? */
Patrick Rudolph CB:34791 Aug 08 20:26
My tests on Sandy Bridge showed that MTRRs are only updated on CR0.CD=1 to CR0.CD=0 transitions. All of AMD and early Intel CPUs follow this scheme and update MTRRs with cache disabled. Isn't that transition required any more starting with APL?
The above observation has never been my understanding recently.
Not related to CD=1, but DEF_MTRR.E=0:
E (MTRRs enabled) flag, bit 11 — MTRRs are enabled when set; all MTRRs are disabled when clear, and the UC memory type is applied to all of physical memory.
So once MTRRs are disable there won't be snooping in the caches.
There's also this line eluding to behavior which aligns to my understanding that I previously described:
Enter the no-fill cache mode. (Set the CD flag in control register CR0 to 1 and the NW flag to 0.)
But this one is a gem and aligns to my microarchitectural implementation defined behavior for older processors:
Some older IA-32 processors used the UC memory type when loading the PDPTEs. Some processors may use the UC memory type if CR0.CD = 1 or if the MTRRs are disabled. These behaviors are model-specific and not architectural.
There's a fuller description in 11.5.1 Cache Control Registers and Bits, but this is a snippet:
CD flag, bit 30 of control register CR0 — Controls caching of system memory locations (see Section 2.5, “Control Registers”). If the CD flag is clear, caching is enabled for the whole of system memory, but may be restricted for individual pages or regions of memory by other cache-control mechanisms. When the CD flag is set, caching is restricted in the processor’s caches (cache hierarchy) for the P6 and more recent processor families and prevented for the Pentium processor (see note below). With the CD flag set, however, the caches will still respond to snoop traffic. Caches should be explicitly flushed to insure memory coherency. For highest processor performance, both the CD and the NW flags in control register CR0 should be cleared. Table 11-5 shows the interaction of the CD and NW flags. The effect of setting the CD flag is somewhat different for processor families starting with P6 family than the Pentium processor (see Table 11-5). To insure memory coherency after the CD flag is set, the caches should be explicitly flushed (see Section 11.5.3, “Preventing Caching”). Setting the CD flag for the P6 and more recent processor families modify cache line fill and update behaviour. Also, setting the CD flag on these processors do not force strict ordering of memory accesses unless the MTRRs are disabled and/or all memory is referenced as uncached (see Section 8.2.5, “Strengthening or Weakening the Memory-Ordering Model”).
From that commit comments, I did not find reference to Intel docs describing NEM at a satisfactory level of details. Instead I read about random lockups in ramstage (MP init) and consistent delay being triggered inside vboot payload after we had only added WB MTRRs in postcar frame. With access to BWG you probably can find exact paragraphs showing that it is no longer required to toggle CR0.CD to correcly exit non-evict mode??
Perhaps. However, many many paragraphs are just copied forward w/o necessarily relating to reality. That said, if it makes you feel comfortable we can actually disable the cache and invalidate it. It's just not necessary in my recent memory and understanding of implementation.
The sequence for CONFIG_INTEL_CAR_NEM below is the same for sandy/ivy, except that the implementation Google provided does set CR0.CD here first. Having peeked into some random FSP-M disassembly, CR0.CD was definetly getting set at several locations, but with a 5 minute effort it's hard to see code that is on TempRamExit() execution path. That's the closed-source CAR teardown implementation.
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35187 )
Change subject: [NOTFORMERGE] soc/intel exit_car.S ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)