| Message ID | 20250127104556.175641-3-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm CONFIG_PHYS_ADDR_T_32=y fixes | expand |
Hi Michal, > On 27 Jan 2025, at 10:45, Michal Orzel <michal.orzel@amd.com> 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@amd.com> > --- I’ve tested this one and it fix the compilation issue on the above setup, I’ve also tested that it doesn’t introduce issues on other setup (e.g. arm64) Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Tested-by: Luca Fancellu <luca.fancellu@arm.com>
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@amd.com> > --- > 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... Cheers, Alejandro
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@amd.com> >> --- >> 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
On Mon, 27 Jan 2025, Luca Fancellu wrote: > Hi Michal, > > > On 27 Jan 2025, at 10:45, Michal Orzel <michal.orzel@amd.com> 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@amd.com> > > --- > > I’ve tested this one and it fix the compilation issue on the above setup, I’ve also tested > that it doesn’t introduce issues on other setup (e.g. arm64) > > Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > Tested-by: Luca Fancellu <luca.fancellu@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
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@amd.com> > >> --- > >> 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
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
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@amd.com> --- xen/arch/arm/include/asm/mm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)