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.