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

[Jan Beulich <jbeulich@xxxxxxxx>]

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