On Wed, Jan 18, 2023 at 1:15 PM Jan Beulich <[email protected]> wrote:
> On 18.01.2023 12:15, Ayan Kumar Halder wrote:
> > On 18/01/2023 08:40, Jan Beulich wrote:
> >> On 17.01.2023 18:43, Ayan Kumar Halder wrote:
> >>> @@ -1166,7 +1166,7 @@ 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;
> >>>
> >>> /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on
> bus 0. */
> >>> @@ -1259,7 +1259,7 @@ 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) |
> >>> + uart->io_base = (paddr_t) ((u64)bar_64 << 32) |
> >>> (bar &
> PCI_BASE_ADDRESS_MEM_MASK);
> >>> }
> >> This looks wrong to me: You shouldn't blindly truncate to 32 bits. You
> need
> >> to refuse acting on 64-bit BARs with the upper address bits non-zero.
> >
> > Yes, I was treating this like others (where Xen does not detect for
> > truncation while getting the address/size from device-tree and
> > typecasting it to paddr_t).
> >
> > However in this case, as Xen is reading from PCI registers, so it needs
> > to check for truncation.
> >
> > I think the following change should do good.
> >
> > @@ -1180,6 +1180,7 @@ pci_uart_config(struct ns16550 *uart, bool_t
> > skip_amt, unsigned int idx)
> > unsigned int bar_idx = 0, port_idx = idx;
> > uint32_t bar, bar_64 = 0, len, len_64;
> > u64 size = 0;
> > + uint64_t io_base = 0;
> > const struct ns16550_config_param *param = uart_param;
> >
> > nextf = (f || (pci_conf_read16(PCI_SBDF(0, b, d, f),
> > @@ -1260,8 +1261,11 @@ pci_uart_config(struct ns16550 *uart, bool_t
> > skip_amt, unsigned int idx)
> > else
> > size = len & PCI_BASE_ADDRESS_MEM_MASK;
> >
> > - uart->io_base = (paddr_t) ((u64)bar_64 << 32) |
> > + io_base = ((u64)bar_64 << 32) |
> > (bar & PCI_BASE_ADDRESS_MEM_MASK);
> > +
> > + uart->io_base = (paddr_t) io_base;
> > + ASSERT(uart->io_base == io_base); /* Detect
> > truncation */
> > }
> > /* IO based */
> > else if ( !param->mmio && (bar &
> > PCI_BASE_ADDRESS_SPACE_IO) )
>
> An assertion can only ever check assumption on behavior elsewhere in Xen.
> Anything external needs handling properly, including in non-debug builds.
>
Except in this case, it's detecting the result of the compiler cast just
above it, isn't it? In which case it seems like it should be a
BUILD_BUG_ON() of some sort.
-George
