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

[Julien Grall <julien@xxxxxxx>]

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