Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37202 )
Change subject: libpayload: Make pci and endian handling -Wconversion safe ......................................................................
libpayload: Make pci and endian handling -Wconversion safe
Change-Id: Ibd1b179d647f105579bd74b071344668ca0a41ef Signed-off-by: Patrick Georgi pgeorgi@google.com --- M payloads/libpayload/include/endian.h M payloads/libpayload/include/pci.h M payloads/libpayload/libpci/libpci.c 3 files changed, 32 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/37202/1
diff --git a/payloads/libpayload/include/endian.h b/payloads/libpayload/include/endian.h index b387e66..b6b5322 100644 --- a/payloads/libpayload/include/endian.h +++ b/payloads/libpayload/include/endian.h @@ -36,12 +36,12 @@
static inline uint16_t swap_bytes16(uint16_t in) { - return ((in & 0xFF) << 8) | ((in & 0xFF00) >> 8); + return (uint16_t)((in & 0xFF) << 8) | ((in & 0xFF00) >> 8); }
static inline uint32_t swap_bytes32(uint32_t in) { - return ((in & 0xFF) << 24) | ((in & 0xFF00) << 8) | + return (uint16_t)((in & 0xFF) << 24) | ((in & 0xFF00) << 8) | ((in & 0xFF0000) >> 8) | ((in & 0xFF000000) >> 24); }
@@ -103,28 +103,30 @@ { uint8_t const *p = (uint8_t const *)pp;
- return ((p[0] << 8) | p[1]); + return (uint16_t)((p[0] << 8) | p[1]); }
static inline uint32_t be32dec(const void *pp) { uint8_t const *p = (uint8_t const *)pp;
- return (((unsigned)p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3]); + return (((uint32_t)p[0] << 24) | (uint32_t)(p[1] << 16) | + (uint32_t)(p[2] << 8) | p[3]); }
static inline uint16_t le16dec(const void *pp) { uint8_t const *p = (uint8_t const *)pp;
- return ((p[1] << 8) | p[0]); + return (uint16_t)((p[1] << 8) | p[0]); }
static inline uint32_t le32dec(const void *pp) { uint8_t const *p = (uint8_t const *)pp;
- return ((p[3] << 24) | (p[2] << 16) | (p[1] << 8) | p[0]); + return ((uint32_t)(p[3] << 24) | (uint32_t)(p[2] << 16) | + (uint32_t)(p[1] << 8) | p[0]); }
static inline void bebitenc(void *pp, uint32_t u, uint8_t b) diff --git a/payloads/libpayload/include/pci.h b/payloads/libpayload/include/pci.h index ff07d5c..a1bac4a 100644 --- a/payloads/libpayload/include/pci.h +++ b/payloads/libpayload/include/pci.h @@ -91,11 +91,11 @@ #define HEADER_TYPE_CARDBUS 2 #define HEADER_TYPE_MULTIFUNCTION 0x80
-#define PCI_ADDR(_bus, _dev, _fn, _reg) \ -(0x80000000 | (_bus << 16) | (_dev << 11) | (_fn << 8) | (_reg & ~3)) +#define PCI_DEV(_bus, _dev, _fn) (0x80000000 | \ +(uint32_t)(_bus << 16) | (uint32_t)(_dev << 11) | (uint32_t)(_fn << 8))
-#define PCI_DEV(_bus, _dev, _fn) \ -(0x80000000 | (_bus << 16) | (_dev << 11) | (_fn << 8)) +#define PCI_ADDR(_bus, _dev, _fn, _reg) \ +(PCI_DEV(_bus, _dev, _fn) | (uint8_t)(_reg & ~3))
#define PCI_BUS(_d) ((_d >> 16) & 0xff) #define PCI_SLOT(_d) ((_d >> 11) & 0x1f) diff --git a/payloads/libpayload/libpci/libpci.c b/payloads/libpayload/libpci/libpci.c index 82203a1..fd0332e 100644 --- a/payloads/libpayload/libpci/libpci.c +++ b/payloads/libpayload/libpci/libpci.c @@ -40,34 +40,34 @@ /* libpci interface */ u8 pci_read_byte(struct pci_dev *dev, int pos) { - return pci_read_config8(libpci_to_lb(dev), pos); + return pci_read_config8(libpci_to_lb(dev), (uint16_t)pos); }
u16 pci_read_word(struct pci_dev *dev, int pos) { - return pci_read_config16(libpci_to_lb(dev), pos); + return pci_read_config16(libpci_to_lb(dev), (uint16_t)pos); }
u32 pci_read_long(struct pci_dev *dev, int pos) { - return pci_read_config32(libpci_to_lb(dev), pos); + return pci_read_config32(libpci_to_lb(dev), (uint16_t)pos); }
int pci_write_byte(struct pci_dev *dev, int pos, u8 data) { - pci_write_config8(libpci_to_lb(dev), pos, data); + pci_write_config8(libpci_to_lb(dev), (uint16_t)pos, data); return 1; /* success */ }
int pci_write_word(struct pci_dev *dev, int pos, u16 data) { - pci_write_config16(libpci_to_lb(dev), pos, data); + pci_write_config16(libpci_to_lb(dev), (uint16_t)pos, data); return 1; /* success */ }
int pci_write_long(struct pci_dev *dev, int pos, u32 data) { - pci_write_config32(libpci_to_lb(dev), pos, data); + pci_write_config32(libpci_to_lb(dev), (uint16_t)pos, data); return 1; /* success */ }
@@ -110,29 +110,29 @@
char *funcp = strrchr(id, '.'); if (funcp) { - filter->func = strtoul(funcp+1, &endptr, 0); + filter->func = strtol(funcp+1, &endptr, 0); if (endptr[0] != '\0') return invalid_pci_device_string; }
char *devp = strrchr(id, ':'); if (!devp) { - filter->dev = strtoul(id, &endptr, 0); + filter->dev = strtol(id, &endptr, 0); } else { - filter->dev = strtoul(devp+1, &endptr, 0); + filter->dev = strtol(devp+1, &endptr, 0); } if (endptr != funcp) return invalid_pci_device_string; if (!devp) return NULL;
char *busp = strchr(id, ':'); if (busp == devp) { - filter->bus = strtoul(id, &endptr, 0); + filter->bus = strtol(id, &endptr, 0); } else { - filter->bus = strtoul(busp+1, &endptr, 0); + filter->bus = strtol(busp+1, &endptr, 0); } if (endptr != funcp) return invalid_pci_device_string; if (busp == devp) return NULL;
- filter->domain = strtoul(id, &endptr, 0); + filter->domain = strtol(id, &endptr, 0); if (endptr != busp) return invalid_pci_device_string;
return NULL; @@ -155,15 +155,15 @@ return 1; }
-static struct pci_dev *pci_scan_single_bus(struct pci_dev *dev, int bus) +static struct pci_dev *pci_scan_single_bus(struct pci_dev *dev, uint8_t bus) { int devfn; u32 val; unsigned char hdr;
for (devfn = 0; devfn < 0x100; devfn++) { - int func = devfn & 0x7; - int slot = (devfn >> 3) & 0x1f; + uint8_t func = devfn & 0x7; + uint8_t slot = (devfn >> 3) & 0x1f;
val = pci_read_config32(PCI_DEV(bus, slot, func), REG_VENDOR_ID); @@ -179,7 +179,7 @@ dev->dev = slot; dev->func = func; dev->vendor_id = val & 0xffff; - dev->device_id = val >> 16; + dev->device_id = (uint16_t)(val >> 16); dev->next = 0;
hdr = pci_read_config8(PCI_DEV(bus, slot, func), @@ -187,10 +187,10 @@ hdr &= 0x7F;
if (hdr == HEADER_TYPE_BRIDGE || hdr == HEADER_TYPE_CARDBUS) { - unsigned int busses; - busses = pci_read_config32(PCI_DEV(bus, slot, func), - REG_PRIMARY_BUS); - busses = (busses >> 8) & 0xFF; + uint8_t busses; + busses = (uint8_t)(pci_read_config32( + PCI_DEV(bus, slot, func), + REG_PRIMARY_BUS) >> 8);
/* Avoid recursion if the new bus is the same as * the old bus (insert lame The Who joke here) */
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37202 )
Change subject: libpayload: Make pci and endian handling -Wconversion safe ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... File payloads/libpayload/include/endian.h:
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... PS1, Line 39: return (uint16_t)((in & 0xFF) << 8) | ((in & 0xFF00) >> 8); every time I see stuff like this I refer to the byte order fallacy, is there certainty that we need this. https://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... File payloads/libpayload/include/pci.h:
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... PS1, Line 94: #define PCI_DEV(_bus, _dev, _fn) (0x80000000 | \ is there any reason for this to be a macro? I know that was the style when we started but ... why not just make it an inline at least and buy a little type safety? I think we might start adopting the rule that if it has parens, we make it a function.
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/libpci/... File payloads/libpayload/libpci/libpci.c:
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/libpci/... PS1, Line 43: return pci_read_config8(libpci_to_lb(dev), (uint16_t)pos); is it worth just forcing the parameter to be a uint?
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37202 )
Change subject: libpayload: Make pci and endian handling -Wconversion safe ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... File payloads/libpayload/include/endian.h:
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... PS1, Line 39: return (uint16_t)((in & 0xFF) << 8) | ((in & 0xFF00) >> 8);
every time I see stuff like this I refer to the byte order fallacy, is there certainty that we need […]
After reading the first paragraph I came to the conclusion that the article is wrong or misguided.
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... PS1, Line 44: 16_ uint32_t
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... File payloads/libpayload/include/pci.h:
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... PS1, Line 94: #define PCI_DEV(_bus, _dev, _fn) (0x80000000 | \
is there any reason for this to be a macro? I know that was the style when we started but ... […]
Could be a good idea, but maybe in a separate patch.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37202 )
Change subject: libpayload: Make pci and endian handling -Wconversion safe ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... File payloads/libpayload/include/endian.h:
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... PS1, Line 39: return (uint16_t)((in & 0xFF) << 8) | ((in & 0xFF00) >> 8);
After reading the first paragraph I came to the conclusion that the article is wrong or misguided.
I don't think the article's author has ever tried to poke a hardware register using C.
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/libpci/... File payloads/libpayload/libpci/libpci.c:
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/libpci/... PS1, Line 43: return pci_read_config8(libpci_to_lb(dev), (uint16_t)pos);
is it worth just forcing the parameter to be a uint?
I don't see why `pos` should be signed at all. However, libpci seems to use this interface...
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37202
to look at the new patch set (#2).
Change subject: libpayload: Make pci and endian handling -Wconversion safe ......................................................................
libpayload: Make pci and endian handling -Wconversion safe
Change-Id: Ibd1b179d647f105579bd74b071344668ca0a41ef Signed-off-by: Patrick Georgi pgeorgi@google.com --- M payloads/libpayload/include/endian.h M payloads/libpayload/include/pci.h M payloads/libpayload/libpci/libpci.c 3 files changed, 30 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/37202/2
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37202 )
Change subject: libpayload: Make pci and endian handling -Wconversion safe ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37202 )
Change subject: libpayload: Make pci and endian handling -Wconversion safe ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... File payloads/libpayload/include/endian.h:
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... PS1, Line 44: 16_
uint32_t
the entire function is now gone.
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... File payloads/libpayload/include/pci.h:
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/include... PS1, Line 94: #define PCI_DEV(_bus, _dev, _fn) (0x80000000 | \
Could be a good idea, but maybe in a separate patch.
Ack
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/libpci/... File payloads/libpayload/libpci/libpci.c:
https://review.coreboot.org/c/coreboot/+/37202/1/payloads/libpayload/libpci/... PS1, Line 43: return pci_read_config8(libpci_to_lb(dev), (uint16_t)pos);
I don't see why `pos` should be signed at all. However, libpci seems to use this interface...
Yes, we're bound to libpci's API
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37202 )
Change subject: libpayload: Make pci and endian handling -Wconversion safe ......................................................................
libpayload: Make pci and endian handling -Wconversion safe
Change-Id: Ibd1b179d647f105579bd74b071344668ca0a41ef Signed-off-by: Patrick Georgi pgeorgi@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37202 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Stefan Reinauer stefan.reinauer@coreboot.org --- M payloads/libpayload/include/endian.h M payloads/libpayload/include/pci.h M payloads/libpayload/libpci/libpci.c 3 files changed, 30 insertions(+), 28 deletions(-)
Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved
diff --git a/payloads/libpayload/include/endian.h b/payloads/libpayload/include/endian.h index dee45f2..037517c 100644 --- a/payloads/libpayload/include/endian.h +++ b/payloads/libpayload/include/endian.h @@ -86,28 +86,30 @@ { uint8_t const *p = (uint8_t const *)pp;
- return ((p[0] << 8) | p[1]); + return (uint16_t)((p[0] << 8) | p[1]); }
static inline uint32_t be32dec(const void *pp) { uint8_t const *p = (uint8_t const *)pp;
- return (((unsigned)p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3]); + return (((uint32_t)p[0] << 24) | (uint32_t)(p[1] << 16) | + (uint32_t)(p[2] << 8) | p[3]); }
static inline uint16_t le16dec(const void *pp) { uint8_t const *p = (uint8_t const *)pp;
- return ((p[1] << 8) | p[0]); + return (uint16_t)((p[1] << 8) | p[0]); }
static inline uint32_t le32dec(const void *pp) { uint8_t const *p = (uint8_t const *)pp;
- return ((p[3] << 24) | (p[2] << 16) | (p[1] << 8) | p[0]); + return ((uint32_t)(p[3] << 24) | (uint32_t)(p[2] << 16) | + (uint32_t)(p[1] << 8) | p[0]); }
static inline void bebitenc(void *pp, uint32_t u, uint8_t b) diff --git a/payloads/libpayload/include/pci.h b/payloads/libpayload/include/pci.h index ff07d5c..a1bac4a 100644 --- a/payloads/libpayload/include/pci.h +++ b/payloads/libpayload/include/pci.h @@ -91,11 +91,11 @@ #define HEADER_TYPE_CARDBUS 2 #define HEADER_TYPE_MULTIFUNCTION 0x80
-#define PCI_ADDR(_bus, _dev, _fn, _reg) \ -(0x80000000 | (_bus << 16) | (_dev << 11) | (_fn << 8) | (_reg & ~3)) +#define PCI_DEV(_bus, _dev, _fn) (0x80000000 | \ +(uint32_t)(_bus << 16) | (uint32_t)(_dev << 11) | (uint32_t)(_fn << 8))
-#define PCI_DEV(_bus, _dev, _fn) \ -(0x80000000 | (_bus << 16) | (_dev << 11) | (_fn << 8)) +#define PCI_ADDR(_bus, _dev, _fn, _reg) \ +(PCI_DEV(_bus, _dev, _fn) | (uint8_t)(_reg & ~3))
#define PCI_BUS(_d) ((_d >> 16) & 0xff) #define PCI_SLOT(_d) ((_d >> 11) & 0x1f) diff --git a/payloads/libpayload/libpci/libpci.c b/payloads/libpayload/libpci/libpci.c index 82203a1..fd0332e 100644 --- a/payloads/libpayload/libpci/libpci.c +++ b/payloads/libpayload/libpci/libpci.c @@ -40,34 +40,34 @@ /* libpci interface */ u8 pci_read_byte(struct pci_dev *dev, int pos) { - return pci_read_config8(libpci_to_lb(dev), pos); + return pci_read_config8(libpci_to_lb(dev), (uint16_t)pos); }
u16 pci_read_word(struct pci_dev *dev, int pos) { - return pci_read_config16(libpci_to_lb(dev), pos); + return pci_read_config16(libpci_to_lb(dev), (uint16_t)pos); }
u32 pci_read_long(struct pci_dev *dev, int pos) { - return pci_read_config32(libpci_to_lb(dev), pos); + return pci_read_config32(libpci_to_lb(dev), (uint16_t)pos); }
int pci_write_byte(struct pci_dev *dev, int pos, u8 data) { - pci_write_config8(libpci_to_lb(dev), pos, data); + pci_write_config8(libpci_to_lb(dev), (uint16_t)pos, data); return 1; /* success */ }
int pci_write_word(struct pci_dev *dev, int pos, u16 data) { - pci_write_config16(libpci_to_lb(dev), pos, data); + pci_write_config16(libpci_to_lb(dev), (uint16_t)pos, data); return 1; /* success */ }
int pci_write_long(struct pci_dev *dev, int pos, u32 data) { - pci_write_config32(libpci_to_lb(dev), pos, data); + pci_write_config32(libpci_to_lb(dev), (uint16_t)pos, data); return 1; /* success */ }
@@ -110,29 +110,29 @@
char *funcp = strrchr(id, '.'); if (funcp) { - filter->func = strtoul(funcp+1, &endptr, 0); + filter->func = strtol(funcp+1, &endptr, 0); if (endptr[0] != '\0') return invalid_pci_device_string; }
char *devp = strrchr(id, ':'); if (!devp) { - filter->dev = strtoul(id, &endptr, 0); + filter->dev = strtol(id, &endptr, 0); } else { - filter->dev = strtoul(devp+1, &endptr, 0); + filter->dev = strtol(devp+1, &endptr, 0); } if (endptr != funcp) return invalid_pci_device_string; if (!devp) return NULL;
char *busp = strchr(id, ':'); if (busp == devp) { - filter->bus = strtoul(id, &endptr, 0); + filter->bus = strtol(id, &endptr, 0); } else { - filter->bus = strtoul(busp+1, &endptr, 0); + filter->bus = strtol(busp+1, &endptr, 0); } if (endptr != funcp) return invalid_pci_device_string; if (busp == devp) return NULL;
- filter->domain = strtoul(id, &endptr, 0); + filter->domain = strtol(id, &endptr, 0); if (endptr != busp) return invalid_pci_device_string;
return NULL; @@ -155,15 +155,15 @@ return 1; }
-static struct pci_dev *pci_scan_single_bus(struct pci_dev *dev, int bus) +static struct pci_dev *pci_scan_single_bus(struct pci_dev *dev, uint8_t bus) { int devfn; u32 val; unsigned char hdr;
for (devfn = 0; devfn < 0x100; devfn++) { - int func = devfn & 0x7; - int slot = (devfn >> 3) & 0x1f; + uint8_t func = devfn & 0x7; + uint8_t slot = (devfn >> 3) & 0x1f;
val = pci_read_config32(PCI_DEV(bus, slot, func), REG_VENDOR_ID); @@ -179,7 +179,7 @@ dev->dev = slot; dev->func = func; dev->vendor_id = val & 0xffff; - dev->device_id = val >> 16; + dev->device_id = (uint16_t)(val >> 16); dev->next = 0;
hdr = pci_read_config8(PCI_DEV(bus, slot, func), @@ -187,10 +187,10 @@ hdr &= 0x7F;
if (hdr == HEADER_TYPE_BRIDGE || hdr == HEADER_TYPE_CARDBUS) { - unsigned int busses; - busses = pci_read_config32(PCI_DEV(bus, slot, func), - REG_PRIMARY_BUS); - busses = (busses >> 8) & 0xFF; + uint8_t busses; + busses = (uint8_t)(pci_read_config32( + PCI_DEV(bus, slot, func), + REG_PRIMARY_BUS) >> 8);
/* Avoid recursion if the new bus is the same as * the old bus (insert lame The Who joke here) */
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37202 )
Change subject: libpayload: Make pci and endian handling -Wconversion safe ......................................................................
Patch Set 5:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/452 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/451 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/450
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Patrick Georgi (Code Review)
-
ron minnich (Code Review)
-
Stefan Reinauer (Code Review)