On Wed, Jan 18, 2023 at 1:58 PM Jan Beulich <jbeul...@suse.com> wrote:
> On 18.01.2023 14:34, George Dunlap wrote:
> > On Wed, Jan 18, 2023 at 1:15 PM Jan Beulich <jbeul...@suse.com> 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?
>
> Not really, no - it checks whether there was truncation, but the
> absence (or presence) thereof is still a property of the underlying
> system, not one in Xen.
>
Ah, gotcha. Ayan, it might be helpful to take a look at the 'Handling
unexpected conditions' section of our CODING_STYLE [1] for a discussion of
when (and when not) to use ASSERT().
-George
[1] https://github.com/xen-project/xen/blob/master/CODING_STYLE
