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,
