Re: [XEN v2 05/11] xen/arm: Use paddr_t instead of u64 for address/size

[George Dunlap <george.dunlap@xxxxxxxxx>]

On Wed, Jan 18, 2023 at 1:58 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:

On 18.01.2023 14:34, George Dunlap wrote:
> On Wed, Jan 18, 2023 at 1:15 PM Jan Beulich <jbeulich@xxxxxxxx> 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