Re: [XEN v4 07/11] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.03.2023 14:29, Julien Grall wrote:
> On 22/03/2023 06:59, Jan Beulich wrote:
>> On 21.03.2023 19:33, Ayan Kumar Halder wrote:
>>> On 21/03/2023 16:53, Jan Beulich wrote:
>>>> On 21.03.2023 17:15, Ayan Kumar Halder wrote:
>>>>> On 21/03/2023 14:22, Jan Beulich wrote:
>>>>>> (Using "unsigned long" for a 32-bit paddr_t is of
>>>>>> course suspicious as well - this ought to be uint32_t.)
>>>>> The problem with using uint32_t for paddr_t is that there are instances
>>>>> where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.
>>>>>
>>>>> For eg , handle_passthrough_prop()
>>>>>
>>>>>                printk(XENLOG_ERR "Unable to permit to dom%d access to"
>>>>>                       " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
>>>>>                       kinfo->d->domain_id,
>>>>>                       mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
>>>>>
>>>>> And in xen/include/xen/page-size.h,
>>>>>
>>>>> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
>>>>> #define PAGE_MASK           (~(PAGE_SIZE-1))
>>>>>
>>>>> Thus, the resulting types are unsigned long. This cannot be printed
>>>>> using %u for PRIpaddr.
>>>> Is there anything wrong with making PAGE_SIZE expand to (1 << PAGE_SHIFT)
>>>> when physical addresses are only 32 bits wide?
>>>
>>> I don't have a strong objection except that this is similar to what
>>> linux is doing today.
>>>
>>> https://elixir.bootlin.com/linux/latest/source/arch/arm/include/asm/page.h#L12
>>>
>>>>
>>>>> I remember some discussion (or comment) that the physical addresses
>>>>> should be represented using 'unsigned long'.
>>>> A reference would be helpful.
>>>
>>> https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00305.html
>>
>> I see. I guess this will be okay as long as only 32-bit arches elect to
>> use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON()
>> somewhere, accompanied by a suitable comment?
> 
> Hmmm... We definitely have 40-bits physical address space on Arm32. In 
> fact, my suggestion was not to define paddr_t as unsigned long for 
> everyone but only when PHYS_ADDR_T_32 is selected (AFAICT this is what 
> is done in this patch).
> 
> This is to avoid having to add cast everywhere we are using PAGE_* on 
> paddr_t and print it.
> 
> That said, I realize this means that for 64-bit, we would still use 
> 64-bit integer. I view it as a less a problem (at least on Arm) because 
> registers are always 64-bit. I am open to other suggestion.

It simply struck me as odd to use a 64-bit type for something that was
explicitly said is only going to be 32 bits wide. I would therefore
prefer if we could limit 32-bit paddr_t to 32-bit architectures for
now, as expressed before when asking for a respective BUILD_BUG_ON().
Especially if, as intended, the type definition moves to xen/types.h
(and hence isn't Arm-specific anymore).

Jan