HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33964
Change subject: cpu/x86/pae/pgtbl: Use CRx_TYPE type for CRx ......................................................................
cpu/x86/pae/pgtbl: Use CRx_TYPE type for CRx
Change-Id: I8108398071a6ff7bceb90741581522023a786afd Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/cpu/x86/pae/pgtbl.c M src/include/cpu/x86/pae.h 2 files changed, 5 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/33964/1
diff --git a/src/cpu/x86/pae/pgtbl.c b/src/cpu/x86/pae/pgtbl.c index f54a1c3..5d23ce5 100644 --- a/src/cpu/x86/pae/pgtbl.c +++ b/src/cpu/x86/pae/pgtbl.c @@ -76,7 +76,7 @@ struct pde pdp[512]; } __packed;
-void paging_enable_pae_cr3(uintptr_t cr3) +void paging_enable_pae_cr3(CRx_TYPE cr3) { /* Load the page table address */ write_cr3(cr3); @@ -198,7 +198,7 @@ /* Get pointer to PD that's not identity mapped */ pd = &pgtbl_buf->pd[((uintptr_t)vmem_addr) >> PDE_IDX_SHIFT];
- paging_enable_pae_cr3((uintptr_t)pdp); + paging_enable_pae_cr3((CRx_TYPE)pdp);
do { const size_t len = MIN(length, s2MiB - offset); @@ -279,7 +279,7 @@ | ((i & 0x3ff) << 21) | 0xE3; pd[i].addr_hi = (window >> 1); } - paging_enable_pae_cr3((uintptr_t)pdp); + paging_enable_pae_cr3((CRx_TYPE)pdp); } mapped_window[index] = window; } @@ -376,7 +376,7 @@ return -1; }
- paging_enable_pae_cr3((uintptr_t)_pdpt); + paging_enable_pae_cr3((CRx_TYPE)_pdpt);
return 0; } diff --git a/src/include/cpu/x86/pae.h b/src/include/cpu/x86/pae.h index 72bae53..6464503 100644 --- a/src/include/cpu/x86/pae.h +++ b/src/include/cpu/x86/pae.h @@ -22,7 +22,7 @@
/* Enable paging with cr3 value for page directory pointer table as well as PAE option in cr4. */ -void paging_enable_pae_cr3(uintptr_t cr3); +void paging_enable_pae_cr3(CRx_TYPE cr3); /* Enable paging as well as PAE option in cr4. */ void paging_enable_pae(void); /* Disable paging as well as PAE option in cr4. */
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33964
to look at the new patch set (#2).
Change subject: cpu/x86/pae/pgtbl: Use CRx_TYPE type for CRx ......................................................................
cpu/x86/pae/pgtbl: Use CRx_TYPE type for CRx
Change-Id: I8108398071a6ff7bceb90741581522023a786afd Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/cpu/x86/pae/pgtbl.c M src/include/cpu/x86/pae.h 2 files changed, 6 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/33964/2
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.
HAOUAS Elyes 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:
I've started this because CRx_TYPE is defined as uint64_t if we have x86_64 and uint32_t if x86_32.
Your are right, I got only those warning:
src/cpu/x86/pae/pgtbl.c 153:21: warning: format '%p' expects argument of type 'void *', but argument 4 has type 'struct pg_table *' [-Wformat=] src/cpu/x86/pae/pgtbl.c 153:41: note: format string is defined here src/cpu/x86/pae/pgtbl.c 204:22: note: in expansion of macro 'MIN' src/cpu/x86/pae/pgtbl.c 204:40: warning: conversion to 'long unsigned int' from 'ssize_t' {aka 'long int'} may change the sign of the result [-Wsign-conversion] src/cpu/x86/pae/pgtbl.c 210:17: warning: conversion from 'long long unsigned int' to 'uint32_t' {aka 'unsigned int'} may change value [-Wconversion] src/cpu/x86/pae/pgtbl.c 211:17: warning: conversion from 'uint64_t' {aka 'long long unsigned int'} to 'uint32_t' {aka 'unsigned int'} may change value [-Wconversion] src/cpu/x86/pae/pgtbl.c 217:15: warning: conversion to 'uint64_t' {aka 'long long unsigned int'} from 'ssize_t' {aka 'long int'} may change the sign of the result [-Wsign-conversion] src/cpu/x86/pae/pgtbl.c 217:46: warning: conversion to 'long unsigned int' from 'ssize_t' {aka 'long int'} may change the sign of the result [-Wsign-conversion] src/cpu/x86/pae/pgtbl.c 219:20: warning: pointer of type 'void *' used in arithmetic [-Wpointer-arith] src/cpu/x86/pae/pgtbl.c 271:21: warning: conversion to 'uint32_t' {aka 'unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion] src/cpu/x86/pae/pgtbl.c 279:6: warning: conversion to 'long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion] src/cpu/x86/pae/pgtbl.c 301:10: warning: unsigned conversion from 'int' to 'unsigned int' changes value from '-2049' to '4294965247' [-Wsign-conversion] src/cpu/x86/pae/pgtbl.c 309:11: warning: conversion from 'uint64_t' {aka 'long long unsigned int'} to 'unsigned int' may change value [-Wconversion] src/cpu/x86/pae/pgtbl.c 310:11: warning: conversion from 'uint64_t' {aka 'long long unsigned int'} to 'unsigned int' may change value [-Wconversion] src/cpu/x86/pae/pgtbl.c 369:39: note: in expansion of macro 'REGION_SIZE' src/cpu/x86/pae/pgtbl.c 374:43: note: in expansion of macro 'REGION_SIZE' src/cpu/x86/pae/pgtbl.c 517:14: note: in expansion of macro 'MIN' src/cpu/x86/pae/pgtbl.c 531:23: warning: unsigned conversion from 'int' to 'uint32_t' {aka 'unsigned int'} changes value from '-2147483648' to '2147483648' [-Wsign-conversion] src/cpu/x86/pae/pgtbl.c 98:6: warning: unsigned conversion from 'int' to 'uint32_t' {aka 'unsigned int'} changes value from '-2147483648' to '2147483648' [-Wsign-conversion]
so, maybe I have to drop this change
HAOUAS Elyes 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: Code-Review-1
HAOUAS Elyes has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/33964 )
Change subject: cpu/x86/pae/pgtbl: Use CRx_TYPE type for CRx ......................................................................
Removed Code-Review-1 by HAOUAS Elyes ehaouas@noos.fr
HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33964 )
Change subject: cpu/x86/pae/pgtbl: Use CRx_TYPE type for CRx ......................................................................
Abandoned
-
HAOUAS Elyes (Code Review) -
Nico Huber (Code Review)