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

[Ayan Kumar Halder <ayankuma@amd.com>]

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