Change in coreboot[master]: mmio: Use unsigned types for bit operations

[Hung-Te Lin (Code Review)]

Hung-Te Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40404 )

Change subject: mmio: Use unsigned types for bit operations
......................................................................

mmio: Use unsigned types for bit operations

When declaring bit fields with 31 bits (e.g: DEFINE_BITFIELD(MYREG, 30, 0) )
the calculation of mask value will go overflow (
 "error: integer overflow in expression '-2147483648 - 1' of type 'int'
  Results in '2147483647'").

To fix that, we want to bit field macros to always use unsigned
integers.

Change-Id: Ie3cddf9df60b83de4e21243bfde6b79729fb06ef
Changr-Id: Ib165700edba6f26fb885fad15c0e483bcb6a63a9
Signed-off-by: Hung-Te Lin 
---
M src/include/device/mmio.h
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/40404/1

diff --git a/src/include/device/mmio.h b/src/include/device/mmio.h
index 524284a..374c8ac 100644
--- a/src/include/device/mmio.h
+++ b/src/include/device/mmio.h
@@ -142,10 +142,10 @@
 #define DEFINE_BIT(name, bit) DEFINE_BITFIELD(name, bit, bit)

#define _BF_MASK(name, value) \
-	(((1 << name##_BITFIELD_SIZE) - 1) << name##_BITFIELD_SHIFT)
+	(((1U << name##_BITFIELD_SIZE) - 1) << name##_BITFIELD_SHIFT)

#define _BF_VALUE(name, value) \
-	((value) << name##_BITFIELD_SHIFT)
+	((unsigned int)(value) << name##_BITFIELD_SHIFT)

#define _BF_APPLY1(op, name, value, ...) (op(name, value))
 #define _BF_APPLY2(op, name, value, ...) ((op(name, value)) | \

Change in coreboot[master]: mmio: Use unsigned integers for bit operations

[Hung-Te Lin (Code Review)]

Hung-Te Lin has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/40404 )

Change subject: mmio: Use unsigned integers for bit operations
......................................................................

mmio: Use unsigned integers for bit operations

When declaring bit fields with 31 bits (e.g:
 DEFINE_BITFIELD(MYREG, 30, 0) ), the calculation of mask value
will go overflow:
 "error: integer overflow in expression '-2147483648 - 1' of
  type 'int' results in '2147483647'").

To fix that, the bit field macros should always use unsigned
integers.

Change-Id: Ie3cddf9df60b83de4e21243bfde6b79729fb06ef
Changr-Id: Ib165700edba6f26fb885fad15c0e483bcb6a63a9
Signed-off-by: Hung-Te Lin 
---
M src/include/device/mmio.h
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/40404/2

Change in coreboot[master]: mmio: Use unsigned integers for bit operations

[Hung-Te Lin (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40404

to look at the new patch set (#3).

Change subject: mmio: Use unsigned integers for bit operations
......................................................................

mmio: Use unsigned integers for bit operations

When declaring bit fields with 31 bits (e.g:
 DEFINE_BITFIELD(MYREG, 30, 0) ), the calculation of mask value
will go overflow:
 "error: integer overflow in expression '-2147483648 - 1' of
  type 'int' results in '2147483647'").

To fix that, the bit field macros should always use unsigned
integers.

Change-Id: Ie3cddf9df60b83de4e21243bfde6b79729fb06ef
Signed-off-by: Hung-Te Lin 
---
M src/include/device/mmio.h
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/40404/3

Change in coreboot[master]: mmio: Use unsigned integers for bit operations

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40404 )

Change subject: mmio: Use unsigned integers for bit operations
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/40404/3/src/include/device/mmio.h 
File src/include/device/mmio.h:

https://review.coreboot.org/c/coreboot/+/40404/3/src/include/device/mmio.h@1... 
PS3, Line 148: (unsigned int)(value)
nit: Would it maybe be useful to also mask this off with the size? That way you could use it with signed negative numbers and accidental misuse can't overflow into other fields.

[Hung-Te Lin (Code Review)]

Hello Huayang Duan, build bot (Jenkins), Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40404

to look at the new patch set (#4).

Change subject: mmio: Use unsigned types for bit operations
......................................................................

mmio: Use unsigned types for bit operations

When declaring bit fields with 31 bits (e.g: DEFINE_BITFIELD(MYREG, 30, 0) )
the calculation of mask value will go overflow (
 "error: integer overflow in expression '-2147483648 - 1' of type 'int'
  Results in '2147483647'").

And for bit fields with 32 bits (e.g: DEFINE_BITFIELD(MYREG, 31, 0) )
the error will be:
 "error: left shift count >= width of type [-Werror=shift-count-overflow]"

To fix these issues, we want to bit field macros to always use unsigned
integers, and use 64bit integer when creating mask value.

Change-Id: Ie3cddf9df60b83de4e21243bfde6b79729fb06ef
Signed-off-by: Hung-Te Lin 
---
M src/include/device/mmio.h
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/40404/4

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40404 )

Change subject: mmio: Use unsigned types for bit operations
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40404/3/src/include/device/mmio.h 
File src/include/device/mmio.h:

https://review.coreboot.org/c/coreboot/+/40404/3/src/include/device/mmio.h@1... 
PS3, Line 145: name##_BITFIELD_SIZE
one issue here is we can't handle 32bit field with this - but I don't want to change this to (u64)1U just for that case.... any better ideas?

Change in coreboot[master]: mmio: Fix failure in bit field macro when accessing >30 bits

[Hung-Te Lin (Code Review)]

Hello Huayang Duan, build bot (Jenkins), Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40404

to look at the new patch set (#5).

Change subject: mmio: Fix failure in bit field macro when accessing >30 bits
......................................................................

mmio: Fix failure in bit field macro when accessing >30 bits

For bit fields with 31 bits (e.g: DEFINE_BITFIELD(MYREG, 30, 0) ),
the calculation of mask value will go overflow:
 "error: integer overflow in expression '-2147483648 - 1' of
  type 'int' results in '2147483647'".

And for bit fields with 32 bits (e.g: DEFINE_BITFIELD(MYREG, 31, 0) ),
the error will be:
 "error: left shift count >= width of type [-Werror=shift-count-overflow]"

To fix these issues, the bit field macros should always use unsigned
integers, and use 64bit integer when creating mask value.

Change-Id: Ie3cddf9df60b83de4e21243bfde6b79729fb06ef
Signed-off-by: Hung-Te Lin 
---
M src/include/device/mmio.h
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/40404/5

Change in coreboot[master]: mmio: Fix failure in bit field macro when accessing >30 bits

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40404 )

Change subject: mmio: Fix failure in bit field macro when accessing >30 bits
......................................................................

Patch Set 5:

(1 comment)
> Patch Set 4:
>
> (1 comment)

https://review.coreboot.org/c/coreboot/+/40404/3/src/include/device/mmio.h 
File src/include/device/mmio.h:

https://review.coreboot.org/c/coreboot/+/40404/3/src/include/device/mmio.h@1... 
PS3, Line 145: name##_BITFIELD_SIZE
> one issue here is we can't handle 32bit field with this - but I don't want to change this to (u64)1U […]

Ack

Change in coreboot[master]: mmio: Fix failure in bit field macro when accessing >30 bits

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40404 )

Change subject: mmio: Fix failure in bit field macro when accessing >30 bits
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: mmio: Fix failure in bit field macro when accessing >30 bits

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40404 )

Change subject: mmio: Fix failure in bit field macro when accessing >30 bits
......................................................................

mmio: Fix failure in bit field macro when accessing >30 bits

For bit fields with 31 bits (e.g: DEFINE_BITFIELD(MYREG, 30, 0) ),
the calculation of mask value will go overflow:
 "error: integer overflow in expression '-2147483648 - 1' of
  type 'int' results in '2147483647'".

And for bit fields with 32 bits (e.g: DEFINE_BITFIELD(MYREG, 31, 0) ),
the error will be:
 "error: left shift count >= width of type [-Werror=shift-count-overflow]"

To fix these issues, the bit field macros should always use unsigned
integers, and use 64bit integer when creating mask value.

Change-Id: Ie3cddf9df60b83de4e21243bfde6b79729fb06ef
Signed-off-by: Hung-Te Lin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40404
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
---
M src/include/device/mmio.h
1 file changed, 2 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/src/include/device/mmio.h b/src/include/device/mmio.h
index b4f2ab6..a725a62 100644
--- a/src/include/device/mmio.h
+++ b/src/include/device/mmio.h
@@ -131,10 +131,10 @@
 #define DEFINE_BIT(name, bit) DEFINE_BITFIELD(name, bit, bit)

#define _BF_MASK(name, value) \
-	(((1 << name##_BITFIELD_SIZE) - 1) << name##_BITFIELD_SHIFT)
+	((u32)((1ULL << name##_BITFIELD_SIZE) - 1) << name##_BITFIELD_SHIFT)

#define _BF_VALUE(name, value) \
-	((value) << name##_BITFIELD_SHIFT)
+	(((u32)(value) << name##_BITFIELD_SHIFT) & _BF_MASK(name, 0))

#define _BF_APPLY1(op, name, value, ...) (op(name, value))
 #define _BF_APPLY2(op, name, value, ...) ((op(name, value)) | \