Re: [for-4.20][PATCH 2/2] xen/arm: Fix build issue when CONFIG_PHYS_ADDR_T_32=y

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Mon Jan 27, 2025 at 5:14 PM GMT, Michal Orzel wrote:
>
>
> On 27/01/2025 18:03, Alejandro Vallejo wrote:
> > 
> > 
> > Hi,
> > 
> > On Mon Jan 27, 2025 at 10:45 AM GMT, Michal Orzel wrote:
> >> On Arm32, when CONFIG_PHYS_ADDR_T_32 is set, a build failure is observed:
> >> arch/arm/platforms/vexpress.c: In function 'vexpress_smp_init':
> >> arch/arm/platforms/vexpress.c:102:12: error: format '%lx' expects argument 
> >> of type 'long unsigned int', but argument 2 has type 'long long unsigned 
> >> int' [-Werror=format=]
> >>   102 |     printk("Set SYS_FLAGS to %"PRIpaddr" (%p)\n",
> >>
> >> When CONFIG_PHYS_ADDR_T_32 is set, paddr_t is defined as unsigned long.
> >> Commit 96f35de69e59 dropped __virt_to_maddr() which used paddr_t as a
> >> return type. Without a cast, the expression type is unsigned long long
> >> which causes the issue. Fix it.
> >>
> >> Fixes: 96f35de69e59 ("x86+Arm: drop (rename) __virt_to_maddr() / 
> >> __maddr_to_virt()")
> >> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> >> ---
> >>  xen/arch/arm/include/asm/mm.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> >> index f91ff088f6b1..a0d8e5afe977 100644
> >> --- a/xen/arch/arm/include/asm/mm.h
> >> +++ b/xen/arch/arm/include/asm/mm.h
> >> @@ -263,7 +263,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, 
> >> size_t len)
> >>
> >>  #define virt_to_maddr(va) ({                                        \
> >>      vaddr_t va_ = (vaddr_t)(va);                                    \
> >> -    (va_to_par(va_) & PADDR_MASK & PAGE_MASK) | (va_ & ~PAGE_MASK); \
> >> +    (paddr_t)((va_to_par(va_) & PADDR_MASK & PAGE_MASK) | (va_ & 
> >> ~PAGE_MASK)); \
> >>  })
> >>
> >>  #ifdef CONFIG_ARM_32
> > 
> > Out of curiosity, why not make va_to_par() and __va_to_par() return paddr_t
> > rather than uint64_t? Then this cast would be unnecessary and the expression
> > end up as unsigned long.
> > 
> > That would also cover any other uses outside this macro.
> > 
> > Unless I'm missing something else...
> va_to_par() returns uint64_t because PAR_EL1 on Arm64 or PAR on Arm32 system 
> registers are both 64bit.
> So, it would not make sense (also it would be confusing) for va_to_par() to 
> return already casted value.
> The function ends with _par so it should reflect the register size as the 
> name suggests. Macro is there
> to cast this value as it also takes into account PADDR_MASK and PAGE_MASK.
>
> ~Michal

I see. The point is to keep va_to_par() in sync with PAR's width then.

Fair enough then.

Cheers,
Alejandro