[for-4.20][PATCH 0/2] xen/arm CONFIG_PHYS_ADDR_T_32=y fixes

[for-4.20][PATCH 0/2] xen/arm CONFIG_PHYS_ADDR_T_32=y fixes

[Michal Orzel]

This series contains build fixes when CONFIG_PHYS_ADDR_T_32=y.

@Oleksii:
This is a release blocker.

Michal Orzel (2):
  device-tree: bootfdt: Fix build issue when CONFIG_PHYS_ADDR_T_32=y
  xen/arm: Fix build issue when CONFIG_PHYS_ADDR_T_32=y

 xen/arch/arm/include/asm/mm.h    | 2 +-
 xen/common/device-tree/bootfdt.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.25.1

[for-4.20][PATCH 1/2] device-tree: bootfdt: Fix build issue when CONFIG_PHYS_ADDR_T_32=y

[Michal Orzel]

On Arm32, when CONFIG_PHYS_ADDR_T_32 is set, a build failure is observed:
common/device-tree/bootfdt.c: In function 'build_assertions':
./include/xen/macros.h:47:31: error: static assertion failed: "!(alignof(struct membanks) != 8)"
   47 | #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); })
      |                               ^~~~~~~~~~~~~~
common/device-tree/bootfdt.c:31:5: note: in expansion of macro 'BUILD_BUG_ON'
   31 |     BUILD_BUG_ON(alignof(struct membanks) != 8);

When CONFIG_PHYS_ADDR_T_32 is set, paddr_t is defined as unsigned long,
therefore the struct membanks alignment is 4B. Fix it.

Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/common/device-tree/bootfdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 47386d4fffea..511700ccc2ba 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -27,8 +27,8 @@ static void __init __maybe_unused build_assertions(void)
      */
     BUILD_BUG_ON((offsetof(struct membanks, bank) !=
                  offsetof(struct meminfo, bank)));
-    /* Ensure "struct membanks" is 8-byte aligned */
-    BUILD_BUG_ON(alignof(struct membanks) != 8);
+    /* Ensure "struct membanks" is paddr aligned */
+    BUILD_BUG_ON(alignof(struct membanks) != sizeof(paddr_t));
 }
 
 static bool __init device_tree_node_is_available(const void *fdt, int node)
-- 
2.25.1

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

[Michal Orzel]

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
-- 
2.25.1

Re: [for-4.20][PATCH 1/2] device-tree: bootfdt: Fix build issue when CONFIG_PHYS_ADDR_T_32=y

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 1922 bytes --]

On Mon, 27 Jan 2025 at 07:46, Michal Orzel <michal.orzel@amd.com> wrote:

> On Arm32, when CONFIG_PHYS_ADDR_T_32 is set, a build failure is observed:
> common/device-tree/bootfdt.c: In function 'build_assertions':
> ./include/xen/macros.h:47:31: error: static assertion failed:
> "!(alignof(struct membanks) != 8)"
>    47 | #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond
> ")"); })
>       |                               ^~~~~~~~~~~~~~
> common/device-tree/bootfdt.c:31:5: note: in expansion of macro
> 'BUILD_BUG_ON'
>    31 |     BUILD_BUG_ON(alignof(struct membanks) != 8);
>
> When CONFIG_PHYS_ADDR_T_32 is set, paddr_t is defined as unsigned long,
> therefore the struct membanks alignment is 4B. Fix it.


Usually, we add a BUILD_BUG_ON when other parts of the code rely on a
specific property (in this case alignment). Can you explain in the commit
message why the new check is still ok?

Cheers,



>
> Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory
> bank structures")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>  xen/common/device-tree/bootfdt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/device-tree/bootfdt.c
> b/xen/common/device-tree/bootfdt.c
> index 47386d4fffea..511700ccc2ba 100644
> --- a/xen/common/device-tree/bootfdt.c
> +++ b/xen/common/device-tree/bootfdt.c
> @@ -27,8 +27,8 @@ static void __init __maybe_unused build_assertions(void)
>       */
>      BUILD_BUG_ON((offsetof(struct membanks, bank) !=
>                   offsetof(struct meminfo, bank)));
> -    /* Ensure "struct membanks" is 8-byte aligned */
> -    BUILD_BUG_ON(alignof(struct membanks) != 8);
> +    /* Ensure "struct membanks" is paddr aligned */
> +    BUILD_BUG_ON(alignof(struct membanks) != sizeof(paddr_t));
>  }
>
>  static bool __init device_tree_node_is_available(const void *fdt, int
> node)
> --
> 2.25.1
>
>

[-- Attachment #2: Type: text/html, Size: 2779 bytes --]

Re: [for-4.20][PATCH 1/2] device-tree: bootfdt: Fix build issue when CONFIG_PHYS_ADDR_T_32=y

[Michal Orzel]

On 27/01/2025 12:19, Julien Grall wrote:
> 	
> 
> 
> 
> 
> On Mon, 27 Jan 2025 at 07:46, Michal Orzel <michal.orzel@amd.com <mailto:michal.orzel@amd.com>> wrote:
> 
>     On Arm32, when CONFIG_PHYS_ADDR_T_32 is set, a build failure is observed:
>     common/device-tree/bootfdt.c: In function 'build_assertions':
>     ./include/xen/macros.h:47:31: error: static assertion failed: "!(alignof(struct membanks) != 8)"
>        47 | #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); })
>           |                               ^~~~~~~~~~~~~~
>     common/device-tree/bootfdt.c:31:5: note: in expansion of macro 'BUILD_BUG_ON'
>        31 |     BUILD_BUG_ON(alignof(struct membanks) != 8);
> 
>     When CONFIG_PHYS_ADDR_T_32 is set, paddr_t is defined as unsigned long,
>     therefore the struct membanks alignment is 4B. Fix it.
> 
> 
> Usually, we add a BUILD_BUG_ON when other parts of the code rely on a specific property (in this case alignment). Can you explain in the commit message why the new check is still ok?
Well, the change itself reflects the change in alignment requirement.
When paddr_t is 64b (Arm64, Arm32 with PA=40b) the alignment is 8B.
On Arm32 with PA=32b, the alignment is 4B because paddr_t is 4B.

AFAICT you requested this check back then, because struct membanks contains flexible array member 'bank' of type struct membank.
The alignment requirement of struct membanks becomes the requirement of struct membank whose largest type is paddr_t.
Let me know how you would like to have this written in commit msg.

~Michal

Re: [for-4.20][PATCH 0/2] xen/arm CONFIG_PHYS_ADDR_T_32=y fixes

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]


On 1/27/25 11:45 AM, Michal Orzel wrote:
> This series contains build fixes when CONFIG_PHYS_ADDR_T_32=y.
>
> @Oleksii:
> This is a release blocker.

Agree, we can't proceed with release if we have build issues. So with proper review:

R-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

>
> Michal Orzel (2):
>    device-tree: bootfdt: Fix build issue when CONFIG_PHYS_ADDR_T_32=y
>    xen/arm: Fix build issue when CONFIG_PHYS_ADDR_T_32=y
>
>   xen/arch/arm/include/asm/mm.h    | 2 +-
>   xen/common/device-tree/bootfdt.c | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
>

[-- Attachment #2: Type: text/html, Size: 1259 bytes --]

Re: [for-4.20][PATCH 1/2] device-tree: bootfdt: Fix build issue when CONFIG_PHYS_ADDR_T_32=y

[Luca Fancellu]

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:
> common/device-tree/bootfdt.c: In function 'build_assertions':
> ./include/xen/macros.h:47:31: error: static assertion failed: "!(alignof(struct membanks) != 8)"
>   47 | #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); })
>      |                               ^~~~~~~~~~~~~~
> common/device-tree/bootfdt.c:31:5: note: in expansion of macro 'BUILD_BUG_ON'
>   31 |     BUILD_BUG_ON(alignof(struct membanks) != 8);
> 
> When CONFIG_PHYS_ADDR_T_32 is set, paddr_t is defined as unsigned long,
> therefore the struct membanks alignment is 4B. Fix it.
> 
> Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---


Apart from Julien’s comments for which I’ll leave you both to agree on a solution, I’ve reproduced
the issue and tested that your change is fixing it and it’s not breaking a different setup (e.g. 64 bit).

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>

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

[Luca Fancellu]

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>

Re: [for-4.20][PATCH 1/2] device-tree: bootfdt: Fix build issue when CONFIG_PHYS_ADDR_T_32=y

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 2131 bytes --]

On Mon, 27 Jan 2025 at 09:52, Michal Orzel <michal.orzel@amd.com> wrote:

>
>
> On 27/01/2025 12:19, Julien Grall wrote:
> >
> >
> >
> >
> >
> > On Mon, 27 Jan 2025 at 07:46, Michal Orzel <michal.orzel@amd.com
> <mailto:michal.orzel@amd.com>> wrote:
> >
> >     On Arm32, when CONFIG_PHYS_ADDR_T_32 is set, a build failure is
> observed:
> >     common/device-tree/bootfdt.c: In function 'build_assertions':
> >     ./include/xen/macros.h:47:31: error: static assertion failed:
> "!(alignof(struct membanks) != 8)"
> >        47 | #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!("
> #cond ")"); })
> >           |                               ^~~~~~~~~~~~~~
> >     common/device-tree/bootfdt.c:31:5: note: in expansion of macro
> 'BUILD_BUG_ON'
> >        31 |     BUILD_BUG_ON(alignof(struct membanks) != 8);
> >
> >     When CONFIG_PHYS_ADDR_T_32 is set, paddr_t is defined as unsigned
> long,
> >     therefore the struct membanks alignment is 4B. Fix it.
> >
> >
> > Usually, we add a BUILD_BUG_ON when other parts of the code rely on a
> specific property (in this case alignment). Can you explain in the commit
> message why the new check is still ok?
> Well, the change itself reflects the change in alignment requirement.
> When paddr_t is 64b (Arm64, Arm32 with PA=40b) the alignment is 8B.
> On Arm32 with PA=32b, the alignment is 4B because paddr_t is 4B.
>
> AFAICT you requested this check back then, because struct membanks
> contains flexible array member 'bank' of type struct membank.
> The alignment requirement of struct membanks becomes the requirement of
> struct membank whose largest type is paddr_t.


Reading this, it sounds like you want to check against the alignment of «
struct membank ». This is because the structure could gain a 64-bit field
in the future and this would not be caught by the BUILD_BUG_ON.


> Let me know how you would like to have this written in commit msg.


I think it needs to be rephrased to make clear the alignment of  struct
membanks should be the same as struct membank.

Cheers,


>
> ~Michal
>
>

[-- Attachment #2: Type: text/html, Size: 3193 bytes --]

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

[Alejandro Vallejo]

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

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

[Michal Orzel]

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

Re: [for-4.20][PATCH 1/2] device-tree: bootfdt: Fix build issue when CONFIG_PHYS_ADDR_T_32=y

[Michal Orzel]

On 27/01/2025 17:27, Julien Grall wrote:
> 	
> 
> 
> 
> 
> On Mon, 27 Jan 2025 at 09:52, Michal Orzel <michal.orzel@amd.com <mailto:michal.orzel@amd.com>> wrote:
> 
> 
> 
>     On 27/01/2025 12:19, Julien Grall wrote:
>     >       
>     >
>     >
>     >
>     >
>     > On Mon, 27 Jan 2025 at 07:46, Michal Orzel <michal.orzel@amd.com <mailto:michal.orzel@amd.com> <mailto:michal.orzel@amd.com <mailto:michal.orzel@amd.com>>> wrote:
>     >
>     >     On Arm32, when CONFIG_PHYS_ADDR_T_32 is set, a build failure is observed:
>     >     common/device-tree/bootfdt.c: In function 'build_assertions':
>     >     ./include/xen/macros.h:47:31: error: static assertion failed: "!(alignof(struct membanks) != 8)"
>     >        47 | #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); })
>     >           |                               ^~~~~~~~~~~~~~
>     >     common/device-tree/bootfdt.c:31:5: note: in expansion of macro 'BUILD_BUG_ON'
>     >        31 |     BUILD_BUG_ON(alignof(struct membanks) != 8);
>     >
>     >     When CONFIG_PHYS_ADDR_T_32 is set, paddr_t is defined as unsigned long,
>     >     therefore the struct membanks alignment is 4B. Fix it.
>     >
>     >
>     > Usually, we add a BUILD_BUG_ON when other parts of the code rely on a specific property (in this case alignment). Can you explain in the commit message why the new check is still ok?
>     Well, the change itself reflects the change in alignment requirement.
>     When paddr_t is 64b (Arm64, Arm32 with PA=40b) the alignment is 8B.
>     On Arm32 with PA=32b, the alignment is 4B because paddr_t is 4B.
> 
>     AFAICT you requested this check back then, because struct membanks contains flexible array member 'bank' of type struct membank.
>     The alignment requirement of struct membanks becomes the requirement of struct membank whose largest type is paddr_t.
> 
> 
> Reading this, it sounds like you want to check against the alignment of « struct membank ». This is because the structure could gain a 64-bit field in the future and this would not be caught by the BUILD_BUG_ON.
> 
> 
>     Let me know how you would like to have this written in commit msg.
> 
> 
> I think it needs to be rephrased to make clear the alignment of  struct membanks should be the same as struct membank.
Shouldn't this check therefore be changed to:
BUILD_BUG_ON(alignof(struct membanks) != alignof(struct membank));

~Michal

Re: [for-4.20][PATCH 1/2] device-tree: bootfdt: Fix build issue when CONFIG_PHYS_ADDR_T_32=y

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

Hi Michal,

On Mon, 27 Jan 2025 at 14:15, Michal Orzel <michal.orzel@amd.com> wrote:

> > I think it needs to be rephrased to make clear the alignment of  struct
> membanks should be the same as struct membank.
> Shouldn't this check therefore be changed to:
> BUILD_BUG_ON(alignof(struct membanks) != alignof(struct membank));



Yes it should be changed.

Cheers,


>
> ~Michal
>

[-- Attachment #2: Type: text/html, Size: 974 bytes --]

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

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>

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

[Alejandro Vallejo]

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