[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [XEN v1] xen/Arm: Remove the extra assignment



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.