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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.01.2023 18:43, Ayan Kumar Halder wrote:
> One should now be able to use 'paddr_t' to represent address and size.
> Consequently, one should use 'PRIpaddr' as a format specifier for paddr_t.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> 
> Changes from -
> 
> v1 - 1. Rebased the patch.
> 
>  xen/arch/arm/domain_build.c        |  9 +++++----
>  xen/arch/arm/gic-v3.c              |  2 +-
>  xen/arch/arm/platforms/exynos5.c   | 26 +++++++++++++-------------
>  xen/drivers/char/exynos4210-uart.c |  2 +-
>  xen/drivers/char/ns16550.c         |  8 ++++----

Please make sure you Cc all maintainers.

> @@ -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.

If already you correct logic even in code not used on Arm (which I appreciate),
then there's actually also related command line handling which needs adjustment.
The use of simple_strtoul() to obtain ->io_base is bogus - this then needs to
be simple_strtoull() (perhaps in a separate prereq patch), and in the 32-bit-
paddr case you'd again need to check for truncation (in the patch here).

While doing the review I've noticed this

    uart->io_size = spcr->serial_port.bit_width;

in ns16550_acpi_uart_init(). This was introduced in 17b516196c55 ("ns16550:
add ACPI support for ARM only"), so Wei, Julien: Doesn't the right hand value
need DIV_ROUND_UP(, 8) to convert from bit count to byte count?

Jan