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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Jan,

On 18/01/2023 08:40, Jan Beulich wrote:
> 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.
Ack.
> > @@ -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) )
> 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).
Agreed this needs to be done in a separate prereq patch.
> 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?
Yes, I think it should be

uart->io_size = DIV_ROUND_UP(spcr->serial_port.bit_width, BITS_PER_BYTE);

However, Julien/Wei can confirm this.

- Ayan

> Jan