Re: [XEN v3 3/9] xen/drivers: ns16550: Use paddr_t for io_base/io_size

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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