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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

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.
Cheers,

--
Julien Grall