| Message ID | 20230208120529.22313-4-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for 32 bit physical address | expand |
On 08.02.2023 13:05, Ayan Kumar Halder wrote: > @@ -1166,8 +1166,9 @@ static const struct ns16550_config __initconst uart_config[] = > static int __init > pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) > { > - u64 orig_base = uart->io_base; > + paddr_t orig_base = uart->io_base; > unsigned int b, d, f, nextf, i; > + u64 pci_uart_io_base; uint64_t please (also elsewhere as needed), assuming the variable actually needs to survive. And if it needs to, please move it into a more narrow scope (and perhaps shorten its name). > @@ -1259,8 +1260,13 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) > else > size = len & PCI_BASE_ADDRESS_MEM_MASK; > > - uart->io_base = ((u64)bar_64 << 32) | > - (bar & PCI_BASE_ADDRESS_MEM_MASK); > + pci_uart_io_base = ((u64)bar_64 << 32) | > + (bar & PCI_BASE_ADDRESS_MEM_MASK); > + > + /* Truncation detected while converting to paddr_t */ > + BUG_ON((pci_uart_io_base >> (PADDR_SHIFT - 1)) > 1); Why would we want to crash during early boot at all? And then even at a point where it'll be hard to figure out what's going on, as we're only in the process of configuring the serial console? > @@ -1532,7 +1539,12 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) > else > #endif > { > - uart->io_base = simple_strtoull(conf, &conf, 0); > + uart_io_base = simple_strtoull(conf, &conf, 0); > + > + /* Truncation detected while converting to paddr_t */ > + BUG_ON((uart_io_base >> (PADDR_SHIFT - 1)) > 1); All the same here. Jan
Hi Jan, On 08/02/2023 13:52, Jan Beulich wrote: > On 08.02.2023 13:05, Ayan Kumar Halder wrote: >> @@ -1166,8 +1166,9 @@ static const struct ns16550_config __initconst uart_config[] = >> static int __init >> pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) >> { >> - u64 orig_base = uart->io_base; >> + paddr_t orig_base = uart->io_base; >> unsigned int b, d, f, nextf, i; >> + u64 pci_uart_io_base; > uint64_t please (also elsewhere as needed), assuming the variable > actually needs to survive. And if it needs to, please move it into > a more narrow scope (and perhaps shorten its name). Ack. > >> @@ -1259,8 +1260,13 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) >> else >> size = len & PCI_BASE_ADDRESS_MEM_MASK; >> >> - uart->io_base = ((u64)bar_64 << 32) | >> - (bar & PCI_BASE_ADDRESS_MEM_MASK); >> + pci_uart_io_base = ((u64)bar_64 << 32) | >> + (bar & PCI_BASE_ADDRESS_MEM_MASK); >> + >> + /* Truncation detected while converting to paddr_t */ >> + BUG_ON((pci_uart_io_base >> (PADDR_SHIFT - 1)) > 1); > Why would we want to crash during early boot at all? And then even at a > point where it'll be hard to figure out what's going on, as we're only > in the process of configuring the serial console? I do not understand the best way to return an error from pci_uart_config(). Out of the 4 invocations of pci_uart_config(), the return value is checked in two invocations only like this. if ( pci_uart_config(uart, 0, uart - ns16550_com) ) return true; pci_uart_config() returns 0 in case of success and -1 in case of error. But the caller seems to be checking the opposite. I could not use domain_crash() as I understand that pci_uart_config() is invoked before domain is created. > >> @@ -1532,7 +1539,12 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) >> else >> #endif >> { >> - uart->io_base = simple_strtoull(conf, &conf, 0); >> + uart_io_base = simple_strtoull(conf, &conf, 0); >> + >> + /* Truncation detected while converting to paddr_t */ >> + BUG_ON((uart_io_base >> (PADDR_SHIFT - 1)) > 1); > All the same here. I can return false from here and let ns16550_parse_port_config() return. I think that might be the correct thing to do here. - Ayan > > Jan
On 08.02.2023 18:07, Ayan Kumar Halder wrote: > Hi Jan, > > On 08/02/2023 13:52, Jan Beulich wrote: >> On 08.02.2023 13:05, Ayan Kumar Halder wrote: >>> @@ -1166,8 +1166,9 @@ static const struct ns16550_config __initconst uart_config[] = >>> static int __init >>> pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) >>> { >>> - u64 orig_base = uart->io_base; >>> + paddr_t orig_base = uart->io_base; >>> unsigned int b, d, f, nextf, i; >>> + u64 pci_uart_io_base; >> uint64_t please (also elsewhere as needed), assuming the variable >> actually needs to survive. And if it needs to, please move it into >> a more narrow scope (and perhaps shorten its name). > Ack. >> >>> @@ -1259,8 +1260,13 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) >>> else >>> size = len & PCI_BASE_ADDRESS_MEM_MASK; >>> >>> - uart->io_base = ((u64)bar_64 << 32) | >>> - (bar & PCI_BASE_ADDRESS_MEM_MASK); >>> + pci_uart_io_base = ((u64)bar_64 << 32) | >>> + (bar & PCI_BASE_ADDRESS_MEM_MASK); >>> + >>> + /* Truncation detected while converting to paddr_t */ >>> + BUG_ON((pci_uart_io_base >> (PADDR_SHIFT - 1)) > 1); >> Why would we want to crash during early boot at all? And then even at a >> point where it'll be hard to figure out what's going on, as we're only >> in the process of configuring the serial console? > > I do not understand the best way to return an error from pci_uart_config(). > > Out of the 4 invocations of pci_uart_config(), the return value is > checked in two invocations only like this. > > if ( pci_uart_config(uart, 0, uart - ns16550_com) ) > return true; > > pci_uart_config() returns 0 in case of success and -1 in case of error. > But the caller seems to be checking the opposite. The callers look a little odd, I agree, but iirc that's intentional. Since this is all PCI _and_ x86-only code, which you don't care all that much about, one option is to leave that code alone for the time being. The other is to properly propagate such an error through all involved code paths, e.g. by way of invoking PARSE_ERR_RET() in parse_namevalue_pairs() in the failure case. >>> @@ -1532,7 +1539,12 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) >>> else >>> #endif >>> { >>> - uart->io_base = simple_strtoull(conf, &conf, 0); >>> + uart_io_base = simple_strtoull(conf, &conf, 0); >>> + >>> + /* Truncation detected while converting to paddr_t */ >>> + BUG_ON((uart_io_base >> (PADDR_SHIFT - 1)) > 1); >> All the same here. > > I can return false from here and let ns16550_parse_port_config() return. > > I think that might be the correct thing to do here. Perhaps, and likely again by way of using PARSE_ERR_RET(). Jan
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index 092f6b9c4b..2aee5642f9 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -42,8 +42,8 @@ static struct ns16550 { int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq; - u64 io_base; /* I/O port or memory-mapped I/O address. */ - u64 io_size; + paddr_t io_base; /* I/O port or memory-mapped I/O address. */ + paddr_t io_size; int reg_shift; /* Bits to shift register offset by */ int reg_width; /* Size of access to use, the registers * themselves are still bytes */ @@ -1166,8 +1166,9 @@ static const struct ns16550_config __initconst uart_config[] = static int __init pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) { - u64 orig_base = uart->io_base; + paddr_t orig_base = uart->io_base; unsigned int b, d, f, nextf, i; + u64 pci_uart_io_base; /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */ for ( b = skip_amt ? 1 : 0; b < 0x100; b++ ) @@ -1259,8 +1260,13 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) else size = len & PCI_BASE_ADDRESS_MEM_MASK; - uart->io_base = ((u64)bar_64 << 32) | - (bar & PCI_BASE_ADDRESS_MEM_MASK); + pci_uart_io_base = ((u64)bar_64 << 32) | + (bar & PCI_BASE_ADDRESS_MEM_MASK); + + /* Truncation detected while converting to paddr_t */ + BUG_ON((pci_uart_io_base >> (PADDR_SHIFT - 1)) > 1); + + uart->io_base = pci_uart_io_base; } /* IO based */ else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) ) @@ -1468,6 +1474,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) int baud; const char *conf; char *name_val_pos; + u64 uart_io_base; conf = *str; name_val_pos = strchr(conf, '='); @@ -1532,7 +1539,12 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) else #endif { - uart->io_base = simple_strtoull(conf, &conf, 0); + uart_io_base = simple_strtoull(conf, &conf, 0); + + /* Truncation detected while converting to paddr_t */ + BUG_ON((uart_io_base >> (PADDR_SHIFT - 1)) > 1); + + uart->io_base = uart_io_base; } }
io_base and io_size represent physical addresses. So they should use paddr_t (instead of u64). However in future, paddr_t may be defined as u32. So when typecasting values from u64 to paddr_t, one should always check for any possible truncation. If any truncation is discovered, Xen needs to raise a BUG for this (as the address is provided either by reading the PCIE registers or parsing parameters from string, so the error is unrecoverable). Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Changes from - v1 - NA v2 - 1. Extracted the patch from "[XEN v2 05/11] xen/arm: Use paddr_t instead of u64 for address/size" into a separate patch of its own. xen/drivers/char/ns16550.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)