As "io_size" and "uart->io_size" are both u64, so there will be no truncation.
Thus, one can remove the ASSERT() and extra assignment.

In an earlier commit (7c1de0038895cbc75ebd0caffc5b0f3f03c5ad51),
"ns16550.io_size" was u32 and "io_size" was u64. Thus, the ASSERT() was needed
to check if the values are the same.
However, in a later commit (c9f8e0aee507bec25104ca5535fde38efae6c6bc),
"ns16550.io_size" was changed to u64. Thus, the ASSERT() became redundant.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/drivers/char/ns16550.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 01a05c9aa8..58d0ccd889 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1747,7 +1747,6 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
     struct ns16550 *uart;
     int res;
     u32 reg_shift, reg_width;
-    u64 io_size;
 
     uart = &ns16550_com[0];
 
@@ -1758,14 +1757,10 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
     uart->parity    = UART_PARITY_NONE;
     uart->stop_bits = 1;
 
-    res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
+    res = dt_device_get_address(dev, 0, &uart->io_base, &uart->io_size);
     if ( res )
         return res;
 
-    uart->io_size = io_size;
-
-    ASSERT(uart->io_size == io_size); /* Detect truncation */
-
     res = dt_property_read_u32(dev, "reg-shift", &reg_shift);
     if ( !res )
         uart->reg_shift = 0;
-- 
2.17.1

Hi Ayan,

Title: It suggests that this is modifying arch/arm whereas you are 
updating the Arm part of the ns16550 driver.

In addition to that, from a reader PoV, it is more important to emphase 
on the fact the truncation check is removed rather than the extra 
assignment.

So I would suggest the following title:

xen/ns16550: Remove unneeded truncation check in the DT init code

On 01/12/2022 17:31, Ayan Kumar Halder wrote:
> As "io_size" and "uart->io_size" are both u64, so there will be no truncation.
> Thus, one can remove the ASSERT() and extra assignment.
> 
> In an earlier commit (7c1de0038895cbc75ebd0caffc5b0f3f03c5ad51),

Please use 12-digit hash and provide the commit title.

> "ns16550.io_size" was u32 and "io_size" was u64. Thus, the ASSERT() was needed
> to check if the values are the same.
> However, in a later commit (c9f8e0aee507bec25104ca5535fde38efae6c6bc),

Ditto.

> "ns16550.io_size" was changed to u64. Thus, the ASSERT() became redundant.

Those two paragraphs explaining your reasoning why the truncation check 
is removed. So I think they should be moved first. Then you can add the 
initial paragraph to explain the resolution.

However... I wonder whether it would not be better to switch 'io_size' 
to paddr_t because, as you said earlier one, on 32-bit ARMv8-R the 
address is 32-bit. Therefore:
  1. it sounds pointless to store the size using 64-bit
  2. the truncation check still make sense (maybe hardened) in the 
32-bit ARMv8-R to catch buggy DT.

Cheers,

-- 
Julien Grall

On 03/12/2022 18:05, Julien Grall wrote:
> Hi Ayan,

Hi Julien,

>
> Title: It suggests that this is modifying arch/arm whereas you are 
> updating the Arm part of the ns16550 driver.
>
> In addition to that, from a reader PoV, it is more important to 
> emphase on the fact the truncation check is removed rather than the 
> extra assignment.
>
> So I would suggest the following title:
>
> xen/ns16550: Remove unneeded truncation check in the DT init code
Ack
>
> On 01/12/2022 17:31, Ayan Kumar Halder wrote:
>> As "io_size" and "uart->io_size" are both u64, so there will be no 
>> truncation.
>> Thus, one can remove the ASSERT() and extra assignment.
>>
>> In an earlier commit (7c1de0038895cbc75ebd0caffc5b0f3f03c5ad51),
>
> Please use 12-digit hash and provide the commit title.
Ack
>
>> "ns16550.io_size" was u32 and "io_size" was u64. Thus, the ASSERT() 
>> was needed
>> to check if the values are the same.
>> However, in a later commit (c9f8e0aee507bec25104ca5535fde38efae6c6bc),
>
> Ditto.
Ack
>
>> "ns16550.io_size" was changed to u64. Thus, the ASSERT() became 
>> redundant.
>
> Those two paragraphs explaining your reasoning why the truncation 
> check is removed. So I think they should be moved first. Then you can 
> add the initial paragraph to explain the resolution.
>
> However... I wonder whether it would not be better to switch 'io_size' 
> to paddr_t because, as you said earlier one, on 32-bit ARMv8-R the 
> address is 32-bit. Therefore:
There are some more drivers where this kind of change (ie using paddr_t 
instead of u64) is required. Thus, I wish to send it in a serie where I 
will introduce CONFIG_ARM_PA_32  (to add support for 32 bit physical 
addresses). Also ...
>  1. it sounds pointless to store the size using 64-bit
>  2. the truncation check still make sense (maybe hardened) in the 
> 32-bit ARMv8-R to catch buggy DT.

Yes, but we need a common check for all the drivers/code as the DT gives 
us 64 bit address (ie u64) and this needs to be translated to paddr_t 
(which can be u64 or u32).

Again, as part of serie to introduce CONFIG_ARM_PA_32, I will provide 
the following function to do address translation :-

--- a/xen/arch/arm/include/asm/platform.h
+++ b/xen/arch/arm/include/asm/platform.h
@@ -42,6 +42,32 @@ struct platform_desc {
      unsigned int dma_bitsize;
  };

+int translate_dt_address_size(u64 *dt_addr, u64 *dt_size, paddr_t *addr,
+                               paddr_t *size)
+{
+#ifdef CONFIG_ARM_PA_32
+    if ( dt_addr && (*dt_addr >> PADDR_SHIFT) )
+    {
+        dprintk(XENLOG_ERR, "Error in DT. Invalid address\n");
+        return -ENXIO;
+    }
+
+    if ( dt_size && (*dt_size >> PADDR_SHIFT) )
+    {
+        dprintk(XENLOG_ERR, "Error in DT. Invalid size\n");
+        return -ENXIO;
+    }
+#endif
+
+    if ( dt_addr && addr )
+        *addr = (paddr_t) (*dt_addr);
+
+    if ( dt_size && size )
+        *size = (paddr_t) (*dt_size);
+
+    return 0;
+}

And the drivers would invoke it as follows. For eg exynos5.c

diff --git a/xen/arch/arm/platforms/exynos5.c 
b/xen/arch/arm/platforms/exynos5.c
index 6560507092..15d1df9104 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -42,8 +42,9 @@ static int exynos5_init_time(void)
      void __iomem *mct;
      int rc;
      struct dt_device_node *node;
-    u64 mct_base_addr;
-    u64 size;
+    paddr_t mct_base_addr;
+    paddr_t size;
+    uint64_t dt_mct_base_addr, dt_size;

      node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-mct");
      if ( !node )
@@ -52,14 +53,19 @@ static int exynos5_init_time(void)
          return -ENXIO;
      }

-    rc = dt_device_get_address(node, 0, &mct_base_addr, &size);
+    rc = dt_device_get_address(node, 0, &dt_mct_base_addr, &dt_size);
      if ( rc )
      {
          dprintk(XENLOG_ERR, "Error in \"samsung,exynos4210-mct\"\n");
          return -ENXIO;
      }

-    dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
+    rc = translate_dt_address_size(&dt_mct_base_addr, &dt_size, 
&mct_base_addr,
+                                   &size);
+    if ( rc )
+        rteturn rc;
+
+    dprintk(XENLOG_INFO, "mct_base_addr: 0x%"PRIpaddr" size: 
0x%"PRIpaddr"\n",
              mct_base_addr, size);

So if this sounds reasonable, we can still remove the truncation as part 
of the current patch.

If you agree, I can send v2 with an updated commit message.

- Ayan

>
> Cheers,
>

Hi Ayan,

On 03/12/2022 19:08, Ayan Kumar Halder wrote:
> 
> On 03/12/2022 18:05, Julien Grall wrote:
>> Hi Ayan,
> 
> Hi Julien,
> 
>>
>> Title: It suggests that this is modifying arch/arm whereas you are 
>> updating the Arm part of the ns16550 driver.
>>
>> In addition to that, from a reader PoV, it is more important to 
>> emphase on the fact the truncation check is removed rather than the 
>> extra assignment.
>>
>> So I would suggest the following title:
>>
>> xen/ns16550: Remove unneeded truncation check in the DT init code
> Ack
>>
>> On 01/12/2022 17:31, Ayan Kumar Halder wrote:
>>> As "io_size" and "uart->io_size" are both u64, so there will be no 
>>> truncation.
>>> Thus, one can remove the ASSERT() and extra assignment.
>>>
>>> In an earlier commit (7c1de0038895cbc75ebd0caffc5b0f3f03c5ad51),
>>
>> Please use 12-digit hash and provide the commit title.
> Ack
>>
>>> "ns16550.io_size" was u32 and "io_size" was u64. Thus, the ASSERT() 
>>> was needed
>>> to check if the values are the same.
>>> However, in a later commit (c9f8e0aee507bec25104ca5535fde38efae6c6bc),
>>
>> Ditto.
> Ack
>>
>>> "ns16550.io_size" was changed to u64. Thus, the ASSERT() became 
>>> redundant.
>>
>> Those two paragraphs explaining your reasoning why the truncation 
>> check is removed. So I think they should be moved first. Then you can 
>> add the initial paragraph to explain the resolution.
>>
>> However... I wonder whether it would not be better to switch 'io_size' 
>> to paddr_t because, as you said earlier one, on 32-bit ARMv8-R the 
>> address is 32-bit. Therefore:
> There are some more drivers where this kind of change (ie using paddr_t 
> instead of u64) is required. Thus, I wish to send it in a serie where I 
> will introduce CONFIG_ARM_PA_32  (to add support for 32 bit physical 
> addresses). Also ...
>>  1. it sounds pointless to store the size using 64-bit
>>  2. the truncation check still make sense (maybe hardened) in the 
>> 32-bit ARMv8-R to catch buggy DT.
> 
> Yes, but we need a common check for all the drivers/code as the DT gives 
> us 64 bit address (ie u64) and this needs to be translated to paddr_t 
> (which can be u64 or u32).
> 
> Again, as part of serie to introduce CONFIG_ARM_PA_32, I will provide 
> the following function to do address translation :-

That series is not on the ML and therefore I haven't had a chance to 
fully review the outcome. So...

> If you agree, I can send v2 with an updated commit message.

... I think it would be best if this patch is part of your upcoming series.

Cheers,

-- 
Julien Grall