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.
Jan
