Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33964 )
Change subject: cpu/x86/pae/pgtbl: Use CRx_TYPE type for CRx ......................................................................
Patch Set 3:
(2 comments)
If I don't miss anything this is just a style change? I wouldn't block this, but we'd need some people that think the new version is better.
https://review.coreboot.org/c/coreboot/+/33964/3/src/cpu/x86/pae/pgtbl.c File src/cpu/x86/pae/pgtbl.c:
https://review.coreboot.org/c/coreboot/+/33964/3/src/cpu/x86/pae/pgtbl.c@82 PS3, Line 82: write_cr3(cr3); I would simply put a cast here. Or ignore it. Was there a warning or why did you start this?
Reasoning below.
https://review.coreboot.org/c/coreboot/+/33964/3/src/cpu/x86/pae/pgtbl.c@201 PS3, Line 201: paging_enable_pae_cr3((CRx_TYPE)pdp); So... `pdp` is a pointer. What one usually should do is to first cast it to `uintptr_t` to be sure that the bit width matches (no warning about casting to integer of different width) and then cast it to the targeted integer type, e.g.
(CRx_TYPE)(uintptr_t)pdp
The direct cast to `CRx_TYPE` works, too, because it is defined to have the same width. But you have to have its definition in mind to see that the code is correct. Personally, I find the original version more legible.