Change in coreboot[master]: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR
This adds a macro to find an available MTRR(s) to set up CAR. This added complexity is not required on bootpaths without bootguard but with bootguard MTRR's have already been set up by the ACM so we need to figure out at runtime which ones are available.
Change-Id: I7d5442c75464cfb2b3611c63a472c8ee521c014d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 43 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/37190/1
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 0992d85..d1ae6ca 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 @@ -23,6 +23,31 @@ #include <rules.h> #include <intelblocks/msr.h>
+ /* puts 1 to eax if not found, 0 if found */ + .macro find_free_mtrr + /* Figure put how many MTRRs we have */ + mov $MTRR_CAP_MSR, %ecx + rdmsr + movzb %al, %ebx /* Number of variable MTRRs */ + + /* Find a free variable MTRR */ + movl $MTRR_PHYS_MASK(0), %ecx +1: + rdmsr + test $MTRR_PHYS_MASK_VALID, %eax + jz 2f + addl $2, %ecx + dec %ebx + jnz 1b + + movl $1, %eax + jmp 3f +2: + movl $0, %eax + decl %ecx +3: + .endm + .global bootblock_pre_c_entry bootblock_pre_c_entry:
@@ -93,15 +118,19 @@ post_code(0x24)
#if ((CONFIG_DCACHE_RAM_SIZE & (CONFIG_DCACHE_RAM_SIZE - 1)) == 0) + find_free_mtrr + cmp $1, %eax + jne 1f + jmp .halt_forever +1: /* Configure CAR region as write-back (WB) */ - mov $MTRR_PHYS_BASE(0), %ecx mov $CONFIG_DCACHE_RAM_BASE, %eax or $MTRR_TYPE_WRBACK, %eax xor %edx,%edx wrmsr
/* Configure the MTRR mask for the size region */ - mov $MTRR_PHYS_MASK(0), %ecx + inc %ecx mov $CONFIG_DCACHE_RAM_SIZE, %eax /* size mask */ dec %eax not %eax @@ -109,14 +138,18 @@ movl %esi, %edx /* edx <- MTRR_PHYS_MASK_HIGH */ wrmsr #elif (CONFIG_DCACHE_RAM_SIZE == 768 * KiB) /* 768 KiB */ + find_free_mtrr + cmp $1, %eax + jne 1f + jmp .halt_forever +1: /* Configure CAR region as write-back (WB) */ - mov $MTRR_PHYS_BASE(0), %ecx mov $CONFIG_DCACHE_RAM_BASE, %eax or $MTRR_TYPE_WRBACK, %eax xor %edx,%edx wrmsr
- mov $MTRR_PHYS_MASK(0), %ecx + incl %ecx mov $(512 * KiB), %eax /* size mask */ dec %eax not %eax @@ -124,13 +157,17 @@ movl %esi, %edx /* edx <- MTRR_PHYS_MASK_HIGH */ wrmsr
- mov $MTRR_PHYS_BASE(1), %ecx + find_free_mtrr + cmp $1, %eax + jne 1f + jmp .halt_forever +1: mov $(CONFIG_DCACHE_RAM_BASE + 512 * KiB), %eax or $MTRR_TYPE_WRBACK, %eax xor %edx,%edx wrmsr
- mov $MTRR_PHYS_MASK(1), %ecx + incl %ecx mov $(256 * KiB), %eax /* size mask */ dec %eax not %eax
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37190
to look at the new patch set (#2).
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR
This adds a macro to find an available MTRR(s) to set up CAR. This added complexity is not required on bootpaths without bootguard but with bootguard MTRR's have already been set up by the ACM so we need to figure out at runtime which ones are available.
Change-Id: I7d5442c75464cfb2b3611c63a472c8ee521c014d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 41 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/37190/2
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37190
to look at the new patch set (#3).
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR
This adds a macro to find an available MTRR(s) to set up CAR. This added complexity is not required on bootpaths without bootguard but with bootguard MTRR's have already been set up by the ACM so we need to figure out at runtime which ones are available.
Change-Id: I7d5442c75464cfb2b3611c63a472c8ee521c014d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 43 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/37190/3
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 3:
(1 comment)
looks mostly reasonable, just that one cmp $1, %eax...
https://review.coreboot.org/c/coreboot/+/37190/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/3/src/soc/intel/common/block/... PS3, Line 142: cmp $1, %eax this test looks odd and isn't really specified as valid output of find_free_mtrr (its comment also defines what's going on in ebx and ecx but nothing about eax)
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37190/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/3/src/soc/intel/common/block/... PS3, Line 142: cmp $1, %eax
this test looks odd and isn't really specified as valid output of find_free_mtrr (its comment also defines what's going on in ebx and ecx but nothing about eax)
In some previous version success/fail was put into %eax. Thx
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37190
to look at the new patch set (#4).
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR
This adds a macro to find an available MTRR(s) to set up CAR. This added complexity is not required on bootpaths without bootguard but with bootguard MTRR's have already been set up by the ACM so we need to figure out at runtime which ones are available.
Change-Id: I7d5442c75464cfb2b3611c63a472c8ee521c014d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 43 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/37190/4
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37190/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/3/src/soc/intel/common/block/... PS3, Line 142: cmp $1, %eax
this test looks odd and isn't really specified as valid output of find_free_mtrr (its comment also […]
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 123: jne 1f je .halt_forever?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 4:
(8 comments)
I'm not sure if the number labels work as expected. They can be avoided.
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 27: * macro: find_free_mtrr : * Clobbers %eax, %ebx, %ecx, %edx. : * If not found %ebx is 0. : * If found MTRR_BASE is at %ecx. Is this macro returning a tuple as (%ebx, %ecx) ? The comment is not very clear.
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 32: .macro find_free_mtrr nit: Maybe don't indent this?
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 43: 2f What does this refer to? Maybe call it "mtrr_found" instead ?
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 46: 1b Is this a loop?
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 48: decl why decl here?
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 122: cmp $0, %ebx I'd use the idiomatic way:
test %ebx, %ebx
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 123: jne 1f
je . […]
Or even jz .halt_forever
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 133: inc %ecx This looks wrong. Why is the mask the result of incrementing ecx?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 27: * macro: find_free_mtrr : * Clobbers %eax, %ebx, %ecx, %edx. : * If not found %ebx is 0. : * If found MTRR_BASE is at %ecx.
Is this macro returning a tuple as (%ebx, %ecx) ? The comment is not very clear.
Yes, how could it be more clear?
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 43: 2f
What does this refer to? Maybe call it "mtrr_found" instead ?
2f means 2 in forward direction, i.e. the next 2: in line 47
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 46: 1b
Is this a loop?
1 backwards, line 40
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 48: decl
why decl here?
MTRRs come in pairs, one BASE followed by one MASK reg. We've checked a bit in the MASK reg here, but want to return BASE, hence -1.
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 133: inc %ecx
This looks wrong. […]
See above, returned %ecx is the BASE reg, +1 for the MASK reg.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 4: Code-Review+1
(5 comments)
Okay, I think I understand it better now.
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 27: * macro: find_free_mtrr : * Clobbers %eax, %ebx, %ecx, %edx. : * If not found %ebx is 0. : * If found MTRR_BASE is at %ecx.
Yes, how could it be more clear?
I would describe both return values, one is the free MTRR count and the other one is the MTRR base.
macro: find_free_mtrr Note: the MTRR MSRs are contiguous, and alternating between BASE and MASK: MTRR_PHYS_MASK(0) = MTRR_PHYS_BASE(0) + 1 MTRR_PHYS_BASE(n) = MTRR_PHYS_BASE(0) + 2 * n MTRR_PHYS_MASK(n) = MTRR_PHYS_MASK(0) + 2 * n Clobbers %eax, %ebx, %ecx, %edx. Returns: %ebx: Number of free MTRRs. If zero, there are no free MTRRs left. %ecx: If %ebx is not zero, the MTRR_BASE of the first available MTRR.
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 43: 2f
2f means 2 in forward direction, i.e. […]
right. Personally, I'd prefer names so that code self-documents
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 46: 1b
1 backwards, line 40
Ack. I'd also use a self-documenting name
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 48: decl
MTRRs come in pairs, one BASE followed by one MASK reg. We've checked a bit […]
Right. I would add a comment (suggestions welcome):
Decrement %ecx to obtain the MTRR base from the MTRR mask.
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 133: inc %ecx
See above, returned %ecx is the BASE reg, +1 for the MASK reg.
Right. I wonder why "inc" is used here, but "incl" is used twice on the codepath below.
Hello Patrick Rudolph, Angel Pons, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37190
to look at the new patch set (#5).
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR
This adds a macro to find an available MTRR(s) to set up CAR. This added complexity is not required on bootpaths without bootguard but with bootguard MTRR's have already been set up by the ACM so we need to figure out at runtime which ones are available.
Change-Id: I7d5442c75464cfb2b3611c63a472c8ee521c014d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 44 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/37190/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37190/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/5/src/soc/intel/common/block/... PS5, Line 31: * It should be checked agaist 0. 'agaist' may be misspelled - perhaps 'against'?
Hello Patrick Rudolph, Angel Pons, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37190
to look at the new patch set (#6).
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR
This adds a macro to find an available MTRR(s) to set up CAR. This added complexity is not required on bootpaths without bootguard but with bootguard MTRR's have already been set up by the ACM so we need to figure out at runtime which ones are available.
Change-Id: I7d5442c75464cfb2b3611c63a472c8ee521c014d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 44 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/37190/6
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 27: * macro: find_free_mtrr : * Clobbers %eax, %ebx, %ecx, %edx. : * If not found %ebx is 0. : * If found MTRR_BASE is at %ecx.
Note: the MTRR MSRs are contiguous, and alternating between BASE and MASK: MTRR_PHYS_MASK(0) = MTRR_PHYS_BASE(0) + 1 MTRR_PHYS_BASE(n) = MTRR_PHYS_BASE(0) + 2 * n MTRR_PHYS_MASK(n) = MTRR_PHYS_MASK(0) + 2 * n
That applies to the whole file, not just this macro.
https://review.coreboot.org/c/coreboot/+/37190/4/src/soc/intel/common/block/... PS4, Line 43: 2f
right. […]
Are you sure one can use names in an assembler macro?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 6: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/37190/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/6/src/soc/intel/common/block/... PS6, Line 26: /* The comment looks rather weird now that the macro declaration isn't indented.
https://review.coreboot.org/c/coreboot/+/37190/6/src/soc/intel/common/block/... PS6, Line 35: put out
https://review.coreboot.org/c/coreboot/+/37190/6/src/soc/intel/common/block/... PS6, Line 161: find_free_mtrr Looks like the suggestions didn't reach this block
Arthur Heymans has uploaded a new patch set (#7) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR
This adds a macro to find an available MTRR(s) to set up CAR. This added complexity is not required on bootpaths without bootguard but with bootguard MTRR's have already been set up by the ACM so we need to figure out at runtime which ones are available.
Change-Id: I7d5442c75464cfb2b3611c63a472c8ee521c014d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 45 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/37190/7
Attention is currently required from: Arthur Heymans, Nico Huber, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 7: Code-Review+1
(8 comments)
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/comment/e700b346_971c034e PS4, Line 32: .macro find_free_mtrr
nit: Maybe don't indent this?
Done
https://review.coreboot.org/c/coreboot/+/37190/comment/d6652561_d2eaa0a6 PS4, Line 43: 2f
Are you sure one can use names in an assembler macro?
It works if the macro is only used once per file, but it probably won't work if used more than once
https://review.coreboot.org/c/coreboot/+/37190/comment/60d68a1b_b7523c6c PS4, Line 48: decl
Right. I would add a comment (suggestions welcome): […]
Ack
https://review.coreboot.org/c/coreboot/+/37190/comment/1cfee80b_516702d7 PS4, Line 122: cmp $0, %ebx
I'd use the idiomatic way: […]
Done
https://review.coreboot.org/c/coreboot/+/37190/comment/cd41c250_c467300c PS4, Line 123: jne 1f
Or even jz . […]
Done
https://review.coreboot.org/c/coreboot/+/37190/comment/a1dcc856_383ce52e PS4, Line 133: inc %ecx
Right. I wonder why "inc" is used here, but "incl" is used twice on the codepath below.
Ack
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/comment/266975f4_3bc79092 PS6, Line 161: find_free_mtrr
Looks like the suggestions didn't reach this block
Ack
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/comment/745aa384_c00f1d7f PS7, Line 152: cmp $0, %ebx : jne 1f : jmp .halt_forever Simpler:
test %ebx, %ebx jz .halt_forever
Attention is currently required from: Arthur Heymans, Nico Huber, Patrick Rudolph. Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37190
to look at the new patch set (#8).
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR
This adds a macro to find an available MTRR(s) to set up CAR. This added complexity is not required on bootpaths without bootguard but with bootguard MTRR's have already been set up by the ACM so we need to figure out at runtime which ones are available.
Change-Id: I7d5442c75464cfb2b3611c63a472c8ee521c014d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 44 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/37190/8
Attention is currently required from: Nico Huber, Angel Pons, Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 8:
(7 comments)
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/comment/5dac7898_44cd87e5 PS4, Line 27: * macro: find_free_mtrr : * Clobbers %eax, %ebx, %ecx, %edx. : * If not found %ebx is 0. : * If found MTRR_BASE is at %ecx.
Note: the MTRR MSRs are contiguous, and alternating between BASE and MASK: […]
Done
https://review.coreboot.org/c/coreboot/+/37190/comment/7afebbd9_202e0554 PS4, Line 32: .macro find_free_mtrr
nit: Maybe don't indent this?
Both seem to be present in the codebase. Looks a bit better if not indented.
https://review.coreboot.org/c/coreboot/+/37190/comment/dc1c3412_9fba33b1 PS4, Line 43: 2f
right. Personally, I'd prefer names so that code self-documents
I'm not sure if that is a good idea in a macro.
https://review.coreboot.org/c/coreboot/+/37190/comment/0764f059_8d8489a7 PS4, Line 123: jne 1f
Or even jz .halt_forever
je is the same as jz.
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/comment/3a266b16_7b7f61de PS6, Line 26: /*
The comment looks rather weird now that the macro declaration isn't indented.
Done
https://review.coreboot.org/c/coreboot/+/37190/comment/35c7e5ea_4b44566e PS6, Line 35: put
out
Done
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/comment/1effd5a5_94f4cdf6 PS7, Line 152: cmp $0, %ebx : jne 1f : jmp .halt_forever
Simpler: […]
Done
Attention is currently required from: Nico Huber, Arthur Heymans, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 9:
(1 comment)
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/37190/comment/312f05cf_7a8af489 PS4, Line 123: jne 1f
Or even jz .halt_forever […]
https://stackoverflow.com/questions/14267081/difference-between-je-jne-and-j...
Yes, they're the same machine operation, but have different semantic meaning. One should use them depending on what one is doing. If one is testing for something being equal to zero, `jz` and `jnz` are more appropriate. `je` and `jne` are more appropriate immediately after a `cmp` instruction.
Attention is currently required from: Nico Huber, Arthur Heymans, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 9: Code-Review+1
Attention is currently required from: Nico Huber, Arthur Heymans. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
Patch Set 9: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37190 )
Change subject: soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR ......................................................................
soc/intel/common/cache_as_ram.S: Add macro to find a free MTRR
This adds a macro to find an available MTRR(s) to set up CAR. This added complexity is not required on bootpaths without bootguard but with bootguard MTRR's have already been set up by the ACM so we need to figure out at runtime which ones are available.
Change-Id: I7d5442c75464cfb2b3611c63a472c8ee521c014d Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/37190 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/soc/intel/common/block/cpu/car/cache_as_ram.S 1 file changed, 44 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
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 3817fa5..db1345a 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 @@ -12,6 +12,35 @@ .section .init, "ax", @progbits
.code32 + +/* + * macro: find_free_mtrr + * Clobbers: %eax, %ebx, %ecx, %edx. + * Returns: + * %ebx contains the number of freely available MTRR's. + * It should be checked against 0. + * %ecx holds the MTRR_BASE of the free MTRR. + */ +.macro find_free_mtrr + /* Figure out how many MTRRs we have */ + mov $MTRR_CAP_MSR, %ecx + rdmsr + movzb %al, %ebx /* Number of variable MTRRs */ + + /* Find a free variable MTRR */ + movl $MTRR_PHYS_MASK(0), %ecx +1: + rdmsr + test $MTRR_PHYS_MASK_VALID, %eax + jz 2f + addl $2, %ecx + dec %ebx + jnz 1b +2: + /* %ecx needs to hold the MTRR_BASE */ + decl %ecx +.endm + .global bootblock_pre_c_entry bootblock_pre_c_entry:
@@ -82,15 +111,18 @@ post_code(0x24)
#if ((CONFIG_DCACHE_RAM_SIZE & (CONFIG_DCACHE_RAM_SIZE - 1)) == 0) + find_free_mtrr + test %ebx, %ebx + jz .halt_forever + /* Configure CAR region as write-back (WB) */ - mov $MTRR_PHYS_BASE(0), %ecx mov $CONFIG_DCACHE_RAM_BASE, %eax or $MTRR_TYPE_WRBACK, %eax xor %edx,%edx wrmsr
/* Configure the MTRR mask for the size region */ - mov $MTRR_PHYS_MASK(0), %ecx + inc %ecx mov $CONFIG_DCACHE_RAM_SIZE, %eax /* size mask */ dec %eax not %eax @@ -98,14 +130,17 @@ movl %esi, %edx /* edx <- MTRR_PHYS_MASK_HIGH */ wrmsr #elif (CONFIG_DCACHE_RAM_SIZE == 768 * KiB) /* 768 KiB */ + find_free_mtrr + test %ebx, %ebx + jz .halt_forever + /* Configure CAR region as write-back (WB) */ - mov $MTRR_PHYS_BASE(0), %ecx mov $CONFIG_DCACHE_RAM_BASE, %eax or $MTRR_TYPE_WRBACK, %eax xor %edx,%edx wrmsr
- mov $MTRR_PHYS_MASK(0), %ecx + incl %ecx mov $(512 * KiB), %eax /* size mask */ dec %eax not %eax @@ -113,13 +148,16 @@ movl %esi, %edx /* edx <- MTRR_PHYS_MASK_HIGH */ wrmsr
- mov $MTRR_PHYS_BASE(1), %ecx + find_free_mtrr + test %ebx, %ebx + jz .halt_forever +1: mov $(CONFIG_DCACHE_RAM_BASE + 512 * KiB), %eax or $MTRR_TYPE_WRBACK, %eax xor %edx,%edx wrmsr
- mov $MTRR_PHYS_MASK(1), %ecx + incl %ecx mov $(256 * KiB), %eax /* size mask */ dec %eax not %eax
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)