Patrick Rudolph 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:
(12 comments)
https://review.coreboot.org/#/c/31549/7/Documentation/arch/x86/pae.md File Documentation/arch/x86/pae.md:
https://review.coreboot.org/#/c/31549/7/Documentation/arch/x86/pae.md@13 PS7, Line 13: uint64_t
Can’t this bee offset_t or size_t?
No
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@114 PS7, Line 114: vmem_addr
documentation shim is already out of sync
Done
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@115 PS7, Line 115: aligned 2 MiB
either `2MiB aligned` or `aligned to 2MiB`, afaik
Done
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@116 PS7, Line 116: are
is
Done
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
AIUI, this is used as a byte pattern? Please mention that in […]
It the same what regular memset does.
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@123 PS7, Line 123: *
Document the return value? Use CB_SUCCESS macro?
Done
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@125 PS7, Line 125: size_t
What if `length >= 4GiB`?
Done
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@126 PS7, Line 126: struct pg_table
This `struct pg_table *` makes it look like the caller would […]
Done
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@128 PS7, Line 128: ssize_t offset;
Why not `offset_t`?
it does not exist
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@132 PS7, Line 132: page-tables
Above you spell it *pagetables*. I’d prefer *page tables* though. […]
Done
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@140 PS7, Line 140: ((uintptr_t)dest < (uintptr_t)vmem_addr));
However, `pgtbl_buf` and `vmem_addr` must not overlap because of […]
Yes thats true, I didn't thought about that case.
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@177 PS7, Line 177: ~((1UL << PDE_IDX_SHIFT) - 1)
PDE_ADDR_MASK?
yes, but that assumes no PAE 4KiB PDE mode. That MASK for PAE 2MiB PDE mode is ~((1UL << PDE_IDX_SHIFT) - 1).