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: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?  In which case it seems like it should be a BUILD_BUG_ON() of some sort.

 -George