Change in coreboot[master]: libpayload: Cache physical location of serial-console struct
Hello Julius Werner,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/43577
to review the following change.
Change subject: libpayload: Cache physical location of serial-console struct ......................................................................
libpayload: Cache physical location of serial-console struct
In the presence of self-relocating payloads, it's safer to keep physical addresses in `libsysinfo`.
Change-Id: Icd30e95c6b8115d16dd793914fb01a1a9da1854f Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/drivers/serial/8250.c M payloads/libpayload/drivers/serial/ipq40xx.c M payloads/libpayload/drivers/serial/ipq806x.c M payloads/libpayload/drivers/serial/qcom_qupv3_serial.c M payloads/libpayload/drivers/serial/qcs405.c M payloads/libpayload/drivers/serial/s5p.c M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c 8 files changed, 16 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/43577/1
diff --git a/payloads/libpayload/drivers/serial/8250.c b/payloads/libpayload/drivers/serial/8250.c index b0e9d03..1df34c4 100644 --- a/payloads/libpayload/drivers/serial/8250.c +++ b/payloads/libpayload/drivers/serial/8250.c @@ -132,9 +132,9 @@
void serial_console_init(void) { - if (!lib_sysinfo.serial) + if (!lib_sysinfo.cb_serial) return; - cb_serial = *lib_sysinfo.serial; + cb_serial = *(struct cb_serial *)phys_to_virt(lib_sysinfo.cb_serial);
serial_init();
diff --git a/payloads/libpayload/drivers/serial/ipq40xx.c b/payloads/libpayload/drivers/serial/ipq40xx.c index 5a9079b..bc5ebbb 100644 --- a/payloads/libpayload/drivers/serial/ipq40xx.c +++ b/payloads/libpayload/drivers/serial/ipq40xx.c @@ -553,9 +553,7 @@ /* For simplicity's sake, let's rely on coreboot initializing the UART. */ void serial_console_init(void) { - struct cb_serial *sc_ptr = lib_sysinfo.serial; - - if (!sc_ptr) + if (!lib_sysinfo.cb_serial) return;
consin.havekey = serial_havechar; diff --git a/payloads/libpayload/drivers/serial/ipq806x.c b/payloads/libpayload/drivers/serial/ipq806x.c index ef4ce80..93e2129 100644 --- a/payloads/libpayload/drivers/serial/ipq806x.c +++ b/payloads/libpayload/drivers/serial/ipq806x.c @@ -343,9 +343,9 @@ /* For simplicity's sake, let's rely on coreboot initializing the UART. */ void serial_console_init(void) { - struct cb_serial *sc_ptr = lib_sysinfo.serial; + struct cb_serial *sc_ptr = phys_to_virt(lib_sysinfo.cb_serial);
- if (!sc_ptr) + if (!lib_sysinfo.cb_serial) return;
base_uart_addr = (void *) sc_ptr->baseaddr; diff --git a/payloads/libpayload/drivers/serial/qcom_qupv3_serial.c b/payloads/libpayload/drivers/serial/qcom_qupv3_serial.c index 3d0e6de..321ff46 100644 --- a/payloads/libpayload/drivers/serial/qcom_qupv3_serial.c +++ b/payloads/libpayload/drivers/serial/qcom_qupv3_serial.c @@ -275,7 +275,8 @@
static struct qup_regs *uart_base_address(void) { - return (void *)(uintptr_t)lib_sysinfo.serial->baseaddr; + const struct cb_serial *const serial = phys_to_virt(lib_sysinfo.cb_serial); + return phys_to_virt(serial->baseaddr); }
static void uart_qupv3_tx_flush(void) @@ -332,7 +333,7 @@
void serial_console_init(void) { - if (!lib_sysinfo.serial) + if (!lib_sysinfo.cb_serial) return;
console_add_output_driver(&consout); diff --git a/payloads/libpayload/drivers/serial/qcs405.c b/payloads/libpayload/drivers/serial/qcs405.c index 1a7b9e9..2ed6af1 100644 --- a/payloads/libpayload/drivers/serial/qcs405.c +++ b/payloads/libpayload/drivers/serial/qcs405.c @@ -541,9 +541,9 @@ /* For simplicity's sake, let's rely on coreboot initializing the UART. */ void serial_console_init(void) { - struct cb_serial *sc_ptr = lib_sysinfo.serial; + struct cb_serial *sc_ptr = phys_to_virt(lib_sysinfo.cb_serial);
- if (!sc_ptr) + if (!lib_sysinfo.cb_serial) return;
uart_board_param.uart_dm_base = (void *)(uintptr_t)sc_ptr->baseaddr; diff --git a/payloads/libpayload/drivers/serial/s5p.c b/payloads/libpayload/drivers/serial/s5p.c index 6ca5dc4..7a6f0e1 100644 --- a/payloads/libpayload/drivers/serial/s5p.c +++ b/payloads/libpayload/drivers/serial/s5p.c @@ -90,10 +90,12 @@
void serial_init(void) { - if (!lib_sysinfo.serial || !lib_sysinfo.serial->baseaddr) + const struct cb_serial *const serial = phys_to_virt(lib_sysinfo.cb_serial); + + if (!lib_sysinfo.cb_serial || !serial->baseaddr) return;
- uart_regs = (struct s5p_uart *)lib_sysinfo.serial->baseaddr; + uart_regs = (struct s5p_uart *)serial->baseaddr; }
void serial_console_init(void) diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h index 31a3974..03300b5 100644 --- a/payloads/libpayload/include/sysinfo.h +++ b/payloads/libpayload/include/sysinfo.h @@ -39,8 +39,6 @@
#include <coreboot_tables.h>
-struct cb_serial; - /* * All pointers in here shall be virtual. * @@ -49,7 +47,7 @@ */ struct sysinfo_t { unsigned int cpu_khz; - struct cb_serial *serial; + uintptr_t cb_serial; unsigned short ser_ioport; unsigned long ser_base; // for mmapped serial
diff --git a/payloads/libpayload/libc/coreboot.c b/payloads/libpayload/libc/coreboot.c index cb47a83..5ecdd51 100644 --- a/payloads/libpayload/libc/coreboot.c +++ b/payloads/libpayload/libc/coreboot.c @@ -86,7 +86,7 @@
static void cb_parse_serial(void *ptr, struct sysinfo_t *info) { - info->serial = ((struct cb_serial *)ptr); + info->cb_serial = virt_to_phys(ptr); }
static void cb_parse_vboot_workbuf(unsigned char *ptr, struct sysinfo_t *info)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43577 )
Change subject: libpayload: Cache physical location of serial-console struct ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43577/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/serial/8250.c:
https://review.coreboot.org/c/coreboot/+/43577/1/payloads/libpayload/drivers... PS1, Line 137: cb_serial = *(struct cb_serial *)phys_to_virt(lib_sysinfo.cb_serial); nit: might be nice to have a wrapper function for this (e.g. cb_get_serial() or something). Then drivers like this wouldn't need a local copy at all they could just keep using that.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43577 )
Change subject: libpayload: Cache physical location of serial-console struct ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43577 )
Change subject: libpayload: Cache physical location of serial-console struct ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43577/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/serial/ipq40xx.c:
https://review.coreboot.org/c/coreboot/+/43577/2/payloads/libpayload/drivers... PS2, Line 556: !lib_sysinfo.cb_serial Shouldn't this be `phys_to_virt(lib_sysinfo.cb_serial)` instead? ipq40xx and ipq806x do not agree
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43577 )
Change subject: libpayload: Cache physical location of serial-console struct ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43577/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/serial/ipq40xx.c:
https://review.coreboot.org/c/coreboot/+/43577/2/payloads/libpayload/drivers... PS2, Line 556: !lib_sysinfo.cb_serial
Shouldn't this be `phys_to_virt(lib_sysinfo. […]
Nvm, I see it now. It was late ._.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43577 )
Change subject: libpayload: Cache physical location of serial-console struct ......................................................................
libpayload: Cache physical location of serial-console struct
In the presence of self-relocating payloads, it's safer to keep physical addresses in `libsysinfo`.
Change-Id: Icd30e95c6b8115d16dd793914fb01a1a9da1854f Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/43577 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/drivers/serial/8250.c M payloads/libpayload/drivers/serial/ipq40xx.c M payloads/libpayload/drivers/serial/ipq806x.c M payloads/libpayload/drivers/serial/qcom_qupv3_serial.c M payloads/libpayload/drivers/serial/qcs405.c M payloads/libpayload/drivers/serial/s5p.c M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c 8 files changed, 16 insertions(+), 17 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/serial/8250.c b/payloads/libpayload/drivers/serial/8250.c index b0e9d03..1df34c4 100644 --- a/payloads/libpayload/drivers/serial/8250.c +++ b/payloads/libpayload/drivers/serial/8250.c @@ -132,9 +132,9 @@
void serial_console_init(void) { - if (!lib_sysinfo.serial) + if (!lib_sysinfo.cb_serial) return; - cb_serial = *lib_sysinfo.serial; + cb_serial = *(struct cb_serial *)phys_to_virt(lib_sysinfo.cb_serial);
serial_init();
diff --git a/payloads/libpayload/drivers/serial/ipq40xx.c b/payloads/libpayload/drivers/serial/ipq40xx.c index 5a9079b..bc5ebbb 100644 --- a/payloads/libpayload/drivers/serial/ipq40xx.c +++ b/payloads/libpayload/drivers/serial/ipq40xx.c @@ -553,9 +553,7 @@ /* For simplicity's sake, let's rely on coreboot initializing the UART. */ void serial_console_init(void) { - struct cb_serial *sc_ptr = lib_sysinfo.serial; - - if (!sc_ptr) + if (!lib_sysinfo.cb_serial) return;
consin.havekey = serial_havechar; diff --git a/payloads/libpayload/drivers/serial/ipq806x.c b/payloads/libpayload/drivers/serial/ipq806x.c index ef4ce80..93e2129 100644 --- a/payloads/libpayload/drivers/serial/ipq806x.c +++ b/payloads/libpayload/drivers/serial/ipq806x.c @@ -343,9 +343,9 @@ /* For simplicity's sake, let's rely on coreboot initializing the UART. */ void serial_console_init(void) { - struct cb_serial *sc_ptr = lib_sysinfo.serial; + struct cb_serial *sc_ptr = phys_to_virt(lib_sysinfo.cb_serial);
- if (!sc_ptr) + if (!lib_sysinfo.cb_serial) return;
base_uart_addr = (void *) sc_ptr->baseaddr; diff --git a/payloads/libpayload/drivers/serial/qcom_qupv3_serial.c b/payloads/libpayload/drivers/serial/qcom_qupv3_serial.c index 3d0e6de..321ff46 100644 --- a/payloads/libpayload/drivers/serial/qcom_qupv3_serial.c +++ b/payloads/libpayload/drivers/serial/qcom_qupv3_serial.c @@ -275,7 +275,8 @@
static struct qup_regs *uart_base_address(void) { - return (void *)(uintptr_t)lib_sysinfo.serial->baseaddr; + const struct cb_serial *const serial = phys_to_virt(lib_sysinfo.cb_serial); + return phys_to_virt(serial->baseaddr); }
static void uart_qupv3_tx_flush(void) @@ -332,7 +333,7 @@
void serial_console_init(void) { - if (!lib_sysinfo.serial) + if (!lib_sysinfo.cb_serial) return;
console_add_output_driver(&consout); diff --git a/payloads/libpayload/drivers/serial/qcs405.c b/payloads/libpayload/drivers/serial/qcs405.c index 1a7b9e9..2ed6af1 100644 --- a/payloads/libpayload/drivers/serial/qcs405.c +++ b/payloads/libpayload/drivers/serial/qcs405.c @@ -541,9 +541,9 @@ /* For simplicity's sake, let's rely on coreboot initializing the UART. */ void serial_console_init(void) { - struct cb_serial *sc_ptr = lib_sysinfo.serial; + struct cb_serial *sc_ptr = phys_to_virt(lib_sysinfo.cb_serial);
- if (!sc_ptr) + if (!lib_sysinfo.cb_serial) return;
uart_board_param.uart_dm_base = (void *)(uintptr_t)sc_ptr->baseaddr; diff --git a/payloads/libpayload/drivers/serial/s5p.c b/payloads/libpayload/drivers/serial/s5p.c index 6ca5dc4..7a6f0e1 100644 --- a/payloads/libpayload/drivers/serial/s5p.c +++ b/payloads/libpayload/drivers/serial/s5p.c @@ -90,10 +90,12 @@
void serial_init(void) { - if (!lib_sysinfo.serial || !lib_sysinfo.serial->baseaddr) + const struct cb_serial *const serial = phys_to_virt(lib_sysinfo.cb_serial); + + if (!lib_sysinfo.cb_serial || !serial->baseaddr) return;
- uart_regs = (struct s5p_uart *)lib_sysinfo.serial->baseaddr; + uart_regs = (struct s5p_uart *)serial->baseaddr; }
void serial_console_init(void) diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h index c3d8c7f..188b2c3 100644 --- a/payloads/libpayload/include/sysinfo.h +++ b/payloads/libpayload/include/sysinfo.h @@ -41,8 +41,6 @@
#include <coreboot_tables.h>
-struct cb_serial; - /* * All pointers in here shall be virtual. * @@ -51,7 +49,7 @@ */ struct sysinfo_t { unsigned int cpu_khz; - struct cb_serial *serial; + uintptr_t cb_serial; unsigned short ser_ioport; unsigned long ser_base; // for mmapped serial
diff --git a/payloads/libpayload/libc/coreboot.c b/payloads/libpayload/libc/coreboot.c index cb47a83..5ecdd51 100644 --- a/payloads/libpayload/libc/coreboot.c +++ b/payloads/libpayload/libc/coreboot.c @@ -86,7 +86,7 @@
static void cb_parse_serial(void *ptr, struct sysinfo_t *info) { - info->serial = ((struct cb_serial *)ptr); + info->cb_serial = virt_to_phys(ptr); }
static void cb_parse_vboot_workbuf(unsigned char *ptr, struct sysinfo_t *info)
-
Angel Pons (Code Review) -
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)