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 7:
(13 comments)
Implementation looks good, not fond of the "documentation" ;)
https://review.coreboot.org/#/c/31549/7/Documentation/arch/x86/pae.md File Documentation/arch/x86/pae.md:
PS7: This is much too thin to be useful. It raises more questions than it answers. I instantly felt the urge to read the code instead.
Also, I would expect and prefer explanations of func- tion signatures in code comments. Linux manages to mix that with their Documentation/, maybe worth to have a look at, how.
If this is the style of documentation moving forward, I'd prefer to reconsider our decision to document more. Such documentation shims create maintenance burden on one side, whilst not providing much value on the other (beside the links to the source, but I know `git grep`).
https://review.coreboot.org/#/c/31549/7/Documentation/arch/x86/pae.md@3 PS7, Line 3: Due to missing x86_64 support it's required to use PAE enabled x86_32 code. If you read this out of context of the patch train (i.e. not with the memory clearing in mind), it makes no sense.
https://review.coreboot.org/#/c/31549/7/Documentation/arch/x86/pae.md@14 PS7, Line 14: struct pg_table *scratch, void *scratch2); Without any reasonable description of the parameters (pat, scratch, scratch2 most of all), this whole section merely is a "look for this in the source if you want to use it".
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
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
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 case, or even make it obvious as `uint8_t pat`.
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@125 PS7, Line 125: size_t What if `length >= 4GiB`?
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 have to care, but actually it shouldn't. Maybe make that clear with a `void *`?
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@140 PS7, Line 140: ((uintptr_t)dest < (uintptr_t)vmem_addr)); I don't understand this one. They are in different spaces, aren't they? e.g. if you wanted to erase 1MiB..3GiB (physical), you could still use 0x00200000 (2MiB) as virtual pointer?
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@141 PS7, Line 141: dest < `dest + length <=` ?
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@142 PS7, Line 142: > `>=` ?
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@163 PS7, Line 163: PDE_A | PDE_D | As far as I understood, the processor would not interpret these, only set them? If we don't interpret them, why set them?
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?