Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31549 )
Change subject: cpu/x86/pae/pgtbl: Add memset with PAE ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c File src/cpu/x86/pae/pgtbl.c:
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@122 PS7, Line 122: * @param pat The pattern to write to the pyhsical memory
It the same what regular memset does.
I don't get your point. Regular memset declares it as `int c` (c for char, I suppose). `unsigned int` makes it look like you need the space, IMHO. Also, memset(3) on my system mentions `byte` multiple times. So it doesn't seem hard to document what it does.
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@177 PS7, Line 177: ~((1UL << PDE_IDX_SHIFT) - 1)
yes, but that assumes no PAE 4KiB PDE mode.
You mean a PDE pointing to a page table (PT)?
That MASK for PAE 2MiB PDE mode is ~((1UL << PDE_IDX_SHIFT) - 1).
Right, I was confused by the macro name. *_ADDR_MASK here is just a PAGE_MASK, i.e. always masking the lowest 12 bits out. Don't know why these macros exist.
Just tried to reason what difference it makes... none. Because the masking here is a no-op anyway (dest is already 2MiB aligned).
https://review.coreboot.org/#/c/31549/8/src/cpu/x86/pae/pgtbl.c File src/cpu/x86/pae/pgtbl.c:
https://review.coreboot.org/#/c/31549/8/src/cpu/x86/pae/pgtbl.c@157 PS8, Line 157: ((dest < pgtbl_e) && (dest + length > pgtbl_e))) { This is usually simpler. If you have two ranges [a, b) and [s, e), they overlap if a < e and b > s. I find it easier to reason about the non-overlapping case, that is either [a, b) is completely below [s, e) or completely above:
(a < s && b <= s) || (a >= e && b > e)
b <= s implies a < s, so we can scratch the latter. same for the other pair:
(b <= s) || (a >= e)
inverted (overlapping case) that makes:
!((b <= s) || (a >= e)) !(b <= s) && !(a >= e) b > s && a < e
Your check seems to miss the case that [a, b) lies completely within [s, e).