Re: [PATCH 07/37] xen/x86: use paddr_t for addresses in NUMA node structure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.01.2022 07:33, Wei Chen wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 2022年1月18日 23:23
>>
>> On 23.09.2021 14:02, Wei Chen wrote:
>>> @@ -249,24 +250,26 @@ static int __init numa_emulation(u64 start_pfn,
>> u64 end_pfn)
>>>  void __init numa_initmem_init(unsigned long start_pfn, unsigned long
>> end_pfn)
>>>  {
>>>      int i;
>>> +    paddr_t start, end;
>>>
>>>  #ifdef CONFIG_NUMA_EMU
>>>      if ( numa_fake && !numa_emulation(start_pfn, end_pfn) )
>>>          return;
>>>  #endif
>>>
>>> +    start = pfn_to_paddr(start_pfn);
>>> +    end = pfn_to_paddr(end_pfn);
>>
>> Nit: Would be slightly neater if these were the initializers of the
>> variables.
> 
> But if this function returns in numa_fake && !numa_emulation,
> will the two pfn_to_paddr operations be waste?

And what harm would that do?

>>> @@ -441,7 +441,7 @@ void __init srat_parse_regions(u64 addr)
>>>         acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat))
>>>             return;
>>>
>>> -   srat_region_mask = pdx_init_mask(addr);
>>> +   srat_region_mask = pdx_init_mask((u64)addr);
>>
>> I don't see the need for a cast here.
>>
> 
> current addr type has been changed to paddr_t, but pdx_init_mask
> accept parameter type is u64. I know paddr_t is a typedef of
> u64 on Arm64/32, or unsinged long on x86. In current stage,
> their machine byte-lengths are the same. But in case of future
> changes I think an explicit case here maybe better? 

It may only ever be an up-cast, yet the compiler would do a widening
conversion (according to the usual type conversion rules) for us
anyway no matter whether there's a cast. Down-casts (in the general
compiler case, i.e. considering a wider set than just gcc and clang)
sometimes need making explicit to silence compiler warnings about
truncation, but I've not observed any compiler warning when widening
values.

Jan