[PATCH v1] xen/arm: align *(.proc.info) in the linker script

[PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Oleksii Kurochko]

During testing of bug.h's macros generic implementation yocto-qemuarm
job crashed with data abort:

(XEN) Data Abort Trap. Syndrome=0x1830021
(XEN) Walking Hypervisor VA 0x2a5ca6 on CPU0 via TTBR 0x000000004014a000
(XEN) 1ST[0x000] = 0x0000000040149f7f
(XEN) 2ND[0x001] = 0x0040000040148f7f
(XEN) 3RD[0x0a5] = 0x00400000400b5fff
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.18-unstable  arm32  debug=y  Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     0020063c head.o#__lookup_processor_type+0x1c/0x44
(XEN) CPSR:   600001da MODE:Hypervisor
(XEN)      R0: 412fc0f1 R1: 002a5ca2 R2: 002a5cd2 R3: 600001da
(XEN)      R4: 002a7e9c R5: 00000011 R6: 00000000 R7: ffffffff
(XEN)      R8: 48008f20 R9: 00000000 R10:00000000 R11:002ffecc R12:00000000
(XEN) HYP: SP: 002ffeb8 LR: 00200618
(XEN)
(XEN)   VTCR_EL2: 00000000
(XEN)  VTTBR_EL2: 0000000000000000
(XEN)
(XEN)  SCTLR_EL2: 30cd187f
(XEN)    HCR_EL2: 00000038
(XEN)  TTBR0_EL2: 000000004014a000
(XEN)
(XEN)    ESR_EL2: 97830021
(XEN)  HPFAR_EL2: 00000000
(XEN)      HDFAR: 002a5ca6
(XEN)      HIFAR: 00000000
(XEN)
(XEN) Xen stack trace from sp=002ffeb8:
(XEN)    97830021 002a7e9c 00000000 00276a88 002fff54 002c8fc4 11112131 10011142
(XEN)    00000000 002a5500 00000000 00000000 00008f20 00002000 48000000 002e01f0
(XEN)    00000000 60000000 00000000 40000000 00000001 002e0208 002a7e8c 002a7e88
(XEN)    002b0ab4 002e31f0 00000000 60000000 00000003 ffffffff 00000000 002aa000
(XEN)    00200060 00000000 00000000 48000000 40010000 3fe10000 00000000 00200068
(XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)    00000000 00000000
(XEN) Xen call trace:
(XEN)    [<0020063c>] head.o#__lookup_processor_type+0x1c/0x44 (PC)
(XEN)    [<00200618>] lookup_processor_type+0xc/0x14 (LR)
(XEN)    [<002c8fc4>] start_xen+0xb8c/0x1138
(XEN)    [<00200068>] head.o#primary_switched+0x8/0x1c
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ****************************************
(XEN)
(XEN) Reboot in five seconds...
(XEN) Xen: Platform reset did not work properly!
(XEN) Xen: Platform reset did not work properly!

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/arm/xen.lds.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 3f7ebd19f3..1b392345bc 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -67,6 +67,7 @@ SECTIONS
        *(.data.rel.ro)
        *(.data.rel.ro.*)
 
+       . = ALIGN(4);
        __proc_info_start = .;
        *(.proc.info)
        __proc_info_end = .;
-- 
2.39.0

Re: [PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Julien Grall]

Hi Oleksii,

On 01/03/2023 16:14, Oleksii Kurochko wrote:
> During testing of bug.h's macros generic implementation yocto-qemuarm
> job crashed with data abort:

The commit message is lacking some information. You are telling us that 
there is an error when building with your series, but this doesn't tell 
me why this is the correct fix.

This is also why I asked to have the xen binary because I want to check 
whether this was a latent bug in Xen or your series effectively 
introduce a bug.

Note that regardless what I just wrote this is a good idea to align 
__proc_info_start. I will try to have a closer look later and propose a 
commit message and/or any action for your other series.

Cheers,

-- 
Julien Grall

Re: [PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Oleksii]

Hi Julien,

On Wed, 2023-03-01 at 16:21 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 01/03/2023 16:14, Oleksii Kurochko wrote:
> > During testing of bug.h's macros generic implementation yocto-
> > qemuarm
> > job crashed with data abort:
> 
> The commit message is lacking some information. You are telling us
> that 
> there is an error when building with your series, but this doesn't
> tell 
> me why this is the correct fix.
> 
> This is also why I asked to have the xen binary because I want to
> check 
> whether this was a latent bug in Xen or your series effectively 
> introduce a bug.
> 
> Note that regardless what I just wrote this is a good idea to align 
> __proc_info_start. I will try to have a closer look later and propose
> a 
> commit message and/or any action for your other series.
Regarding binaries please take a look here:
https://lore.kernel.org/xen-devel/aa2862eacccfb0574859bf4cda8f4992baa5d2e1.camel@gmail.com/

I am not sure if you get my answer as I had the message from delivery
server that it was blocked for some reason.


~ Oleksii

Re: [PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Julien Grall]

On 01/03/2023 16:38, Oleksii wrote:
> Hi Julien,

Hi,

> On Wed, 2023-03-01 at 16:21 +0000, Julien Grall wrote:
>> Hi Oleksii,
>>
>> On 01/03/2023 16:14, Oleksii Kurochko wrote:
>>> During testing of bug.h's macros generic implementation yocto-
>>> qemuarm
>>> job crashed with data abort:
>>
>> The commit message is lacking some information. You are telling us
>> that
>> there is an error when building with your series, but this doesn't
>> tell
>> me why this is the correct fix.
>>
>> This is also why I asked to have the xen binary because I want to
>> check
>> whether this was a latent bug in Xen or your series effectively
>> introduce a bug.
>>
>> Note that regardless what I just wrote this is a good idea to align
>> __proc_info_start. I will try to have a closer look later and propose
>> a
>> commit message and/or any action for your other series.
> Regarding binaries please take a look here:
> https://lore.kernel.org/xen-devel/aa2862eacccfb0574859bf4cda8f4992baa5d2e1.camel@gmail.com/
> 
> I am not sure if you get my answer as I had the message from delivery
> server that it was blocked for some reason.

I got the answer. The problem now is gitlab only keep the artifact for 
the latest build and it only provide a zImage (having the ELF would be 
easier).

I will try to reproduce the error on my end.

Cheers,

-- 
Julien Grall

Re: [PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Julien Grall]

Hi,

On 01/03/2023 17:50, Julien Grall wrote:
> 
> 
> On 01/03/2023 16:38, Oleksii wrote:
>> Hi Julien,
> 
> Hi,
> 
>> On Wed, 2023-03-01 at 16:21 +0000, Julien Grall wrote:
>>> Hi Oleksii,
>>>
>>> On 01/03/2023 16:14, Oleksii Kurochko wrote:
>>>> During testing of bug.h's macros generic implementation yocto-
>>>> qemuarm
>>>> job crashed with data abort:
>>>
>>> The commit message is lacking some information. You are telling us
>>> that
>>> there is an error when building with your series, but this doesn't
>>> tell
>>> me why this is the correct fix.
>>>
>>> This is also why I asked to have the xen binary because I want to
>>> check
>>> whether this was a latent bug in Xen or your series effectively
>>> introduce a bug.
>>>
>>> Note that regardless what I just wrote this is a good idea to align
>>> __proc_info_start. I will try to have a closer look later and propose
>>> a
>>> commit message and/or any action for your other series.
>> Regarding binaries please take a look here:
>> https://lore.kernel.org/xen-devel/aa2862eacccfb0574859bf4cda8f4992baa5d2e1.camel@gmail.com/
>>
>> I am not sure if you get my answer as I had the message from delivery
>> server that it was blocked for some reason.
> 
> I got the answer. The problem now is gitlab only keep the artifact for 
> the latest build and it only provide a zImage (having the ELF would be 
> easier).
> 
> I will try to reproduce the error on my end.

I managed to reproduce it. It looks like that after your bug patch, 
*(.rodata.*) will not be end on a 4-byte boundary. Before your patch, 
all the messages will be in .rodata.str. Now they are in .bug_frames.*, 
so there some difference in .rodata.*.

That said, it is not entirely clear why we never seen the issue before 
because my guessing there are no guarantee that .rodata.* will be 
suitably aligned.

Anyway, here a proposal for the commit message:

"
xen/arm: Ensure the start *(.proc.info) of is 4-byte aligned

The entries in *(.proc.info) are expected to be 4-byte aligned and the 
compiler will access them using 4-byte load instructions. On Arm32, the 
alignment is strictly enforced by the processor and will result to a 
data abort if it is not correct.

However, the linker script doesn't encode this requirement. So we are at 
the mercy of the compiler/linker to have aligned the previous sections 
suitably.

This was spotted when trying to use the upcoming generic bug 
infrastructure with the compiler provided by Yocto.

Link: 
https://lore.kernel.org/xen-devel/6735859208c6dcb7320f89664ae298005f70827b.camel@gmail.com/
"

If you are happy with the proposed commit message, then I can update it 
while committing.

Cheers,

-- 
Julien Grall

Re: [PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Oleksii]

Hi Julien,
> > > On Wed, 2023-03-01 at 16:21 +0000, Julien Grall wrote:
> > > > Hi Oleksii,
> > > > 
> > > > On 01/03/2023 16:14, Oleksii Kurochko wrote:
> > > > > During testing of bug.h's macros generic implementation
> > > > > yocto-
> > > > > qemuarm
> > > > > job crashed with data abort:
> > > > 
> > > > The commit message is lacking some information. You are telling
> > > > us
> > > > that
> > > > there is an error when building with your series, but this
> > > > doesn't
> > > > tell
> > > > me why this is the correct fix.
> > > > 
> > > > This is also why I asked to have the xen binary because I want
> > > > to
> > > > check
> > > > whether this was a latent bug in Xen or your series effectively
> > > > introduce a bug.
> > > > 
> > > > Note that regardless what I just wrote this is a good idea to
> > > > align
> > > > __proc_info_start. I will try to have a closer look later and
> > > > propose
> > > > a
> > > > commit message and/or any action for your other series.
> > > Regarding binaries please take a look here:
> > > https://lore.kernel.org/xen-devel/aa2862eacccfb0574859bf4cda8f4992baa5d2e1.camel@gmail.com/
> > > 
> > > I am not sure if you get my answer as I had the message from
> > > delivery
> > > server that it was blocked for some reason.
> > 
> > I got the answer. The problem now is gitlab only keep the artifact
> > for 
> > the latest build and it only provide a zImage (having the ELF would
> > be 
> > easier).
> > 
> > I will try to reproduce the error on my end.
> 
> I managed to reproduce it. It looks like that after your bug patch, 
> *(.rodata.*) will not be end on a 4-byte boundary. Before your patch,
> all the messages will be in .rodata.str. Now they are in
> .bug_frames.*, 
> so there some difference in .rodata.*.
> 
> That said, it is not entirely clear why we never seen the issue
> before 
> because my guessing there are no guarantee that .rodata.* will be 
> suitably aligned.
> 
> Anyway, here a proposal for the commit message:
> 
> "
> xen/arm: Ensure the start *(.proc.info) of is 4-byte aligned
> 
> The entries in *(.proc.info) are expected to be 4-byte aligned and
> the 
> compiler will access them using 4-byte load instructions. On Arm32,
> the 
> alignment is strictly enforced by the processor and will result to a 
> data abort if it is not correct.
> 
> However, the linker script doesn't encode this requirement. So we are
> at 
> the mercy of the compiler/linker to have aligned the previous
> sections 
> suitably.
> 
> This was spotted when trying to use the upcoming generic bug 
> infrastructure with the compiler provided by Yocto.
> 
> Link: 
> https://lore.kernel.org/xen-devel/6735859208c6dcb7320f89664ae298005f70827b.camel@gmail.com/
> "
> 
> If you are happy with the proposed commit message, then I can update
> it 
> while committing.
I am happy with the proposed commit message.

Thanks a lot.

~ Oleksii

Re: [PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Julien Grall]

Hi Oleksii,

On 02/03/2023 07:34, Oleksii wrote:
> Hi Julien,
>>>> On Wed, 2023-03-01 at 16:21 +0000, Julien Grall wrote:
>>>>> Hi Oleksii,
>>>>>
>>>>> On 01/03/2023 16:14, Oleksii Kurochko wrote:
>>>>>> During testing of bug.h's macros generic implementation
>>>>>> yocto-
>>>>>> qemuarm
>>>>>> job crashed with data abort:
>>>>>
>>>>> The commit message is lacking some information. You are telling
>>>>> us
>>>>> that
>>>>> there is an error when building with your series, but this
>>>>> doesn't
>>>>> tell
>>>>> me why this is the correct fix.
>>>>>
>>>>> This is also why I asked to have the xen binary because I want
>>>>> to
>>>>> check
>>>>> whether this was a latent bug in Xen or your series effectively
>>>>> introduce a bug.
>>>>>
>>>>> Note that regardless what I just wrote this is a good idea to
>>>>> align
>>>>> __proc_info_start. I will try to have a closer look later and
>>>>> propose
>>>>> a
>>>>> commit message and/or any action for your other series.
>>>> Regarding binaries please take a look here:
>>>> https://lore.kernel.org/xen-devel/aa2862eacccfb0574859bf4cda8f4992baa5d2e1.camel@gmail.com/
>>>>
>>>> I am not sure if you get my answer as I had the message from
>>>> delivery
>>>> server that it was blocked for some reason.
>>>
>>> I got the answer. The problem now is gitlab only keep the artifact
>>> for
>>> the latest build and it only provide a zImage (having the ELF would
>>> be
>>> easier).
>>>
>>> I will try to reproduce the error on my end.
>>
>> I managed to reproduce it. It looks like that after your bug patch,
>> *(.rodata.*) will not be end on a 4-byte boundary. Before your patch,
>> all the messages will be in .rodata.str. Now they are in
>> .bug_frames.*,
>> so there some difference in .rodata.*.
>>
>> That said, it is not entirely clear why we never seen the issue
>> before
>> because my guessing there are no guarantee that .rodata.* will be
>> suitably aligned.
>>
>> Anyway, here a proposal for the commit message:
>>
>> "
>> xen/arm: Ensure the start *(.proc.info) of is 4-byte aligned
>>
>> The entries in *(.proc.info) are expected to be 4-byte aligned and
>> the
>> compiler will access them using 4-byte load instructions. On Arm32,
>> the
>> alignment is strictly enforced by the processor and will result to a
>> data abort if it is not correct.
>>
>> However, the linker script doesn't encode this requirement. So we are
>> at
>> the mercy of the compiler/linker to have aligned the previous
>> sections
>> suitably.
>>
>> This was spotted when trying to use the upcoming generic bug
>> infrastructure with the compiler provided by Yocto.
>>
>> Link:
>> https://lore.kernel.org/xen-devel/6735859208c6dcb7320f89664ae298005f70827b.camel@gmail.com/
>> "
>>
>> If you are happy with the proposed commit message, then I can update
>> it
>> while committing.
> I am happy with the proposed commit message.

Thanks. With that:

Reviewed-by: Julien Grall <jgrall@amazon.com>

I have addressed Jan's comment and committed the patch.

Cheers,

-- 
Julien Grall

Re: [PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Oleksii]

On Thu, 2023-03-02 at 14:50 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 02/03/2023 07:34, Oleksii wrote:
> > Hi Julien,
> > > > > On Wed, 2023-03-01 at 16:21 +0000, Julien Grall wrote:
> > > > > > Hi Oleksii,
> > > > > > 
> > > > > > On 01/03/2023 16:14, Oleksii Kurochko wrote:
> > > > > > > During testing of bug.h's macros generic implementation
> > > > > > > yocto-
> > > > > > > qemuarm
> > > > > > > job crashed with data abort:
> > > > > > 
> > > > > > The commit message is lacking some information. You are
> > > > > > telling
> > > > > > us
> > > > > > that
> > > > > > there is an error when building with your series, but this
> > > > > > doesn't
> > > > > > tell
> > > > > > me why this is the correct fix.
> > > > > > 
> > > > > > This is also why I asked to have the xen binary because I
> > > > > > want
> > > > > > to
> > > > > > check
> > > > > > whether this was a latent bug in Xen or your series
> > > > > > effectively
> > > > > > introduce a bug.
> > > > > > 
> > > > > > Note that regardless what I just wrote this is a good idea
> > > > > > to
> > > > > > align
> > > > > > __proc_info_start. I will try to have a closer look later
> > > > > > and
> > > > > > propose
> > > > > > a
> > > > > > commit message and/or any action for your other series.
> > > > > Regarding binaries please take a look here:
> > > > > https://lore.kernel.org/xen-devel/aa2862eacccfb0574859bf4cda8f4992baa5d2e1.camel@gmail.com/
> > > > > 
> > > > > I am not sure if you get my answer as I had the message from
> > > > > delivery
> > > > > server that it was blocked for some reason.
> > > > 
> > > > I got the answer. The problem now is gitlab only keep the
> > > > artifact
> > > > for
> > > > the latest build and it only provide a zImage (having the ELF
> > > > would
> > > > be
> > > > easier).
> > > > 
> > > > I will try to reproduce the error on my end.
> > > 
> > > I managed to reproduce it. It looks like that after your bug
> > > patch,
> > > *(.rodata.*) will not be end on a 4-byte boundary. Before your
> > > patch,
> > > all the messages will be in .rodata.str. Now they are in
> > > .bug_frames.*,
> > > so there some difference in .rodata.*.
> > > 
> > > That said, it is not entirely clear why we never seen the issue
> > > before
> > > because my guessing there are no guarantee that .rodata.* will be
> > > suitably aligned.
> > > 
> > > Anyway, here a proposal for the commit message:
> > > 
> > > "
> > > xen/arm: Ensure the start *(.proc.info) of is 4-byte aligned
> > > 
> > > The entries in *(.proc.info) are expected to be 4-byte aligned
> > > and
> > > the
> > > compiler will access them using 4-byte load instructions. On
> > > Arm32,
> > > the
> > > alignment is strictly enforced by the processor and will result
> > > to a
> > > data abort if it is not correct.
> > > 
> > > However, the linker script doesn't encode this requirement. So we
> > > are
> > > at
> > > the mercy of the compiler/linker to have aligned the previous
> > > sections
> > > suitably.
> > > 
> > > This was spotted when trying to use the upcoming generic bug
> > > infrastructure with the compiler provided by Yocto.
> > > 
> > > Link:
> > > https://lore.kernel.org/xen-devel/6735859208c6dcb7320f89664ae298005f70827b.camel@gmail.com/
> > > "
> > > 
> > > If you are happy with the proposed commit message, then I can
> > > update
> > > it
> > > while committing.
> > I am happy with the proposed commit message.
> 
> Thanks. With that:
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> 
> I have addressed Jan's comment and committed the patch.
> 
Thanks a lot.

Not generic bug feature is unblock.
I'll wait for comments till tomorrow.
If it won't be any that will sent new patch series.

~ Oleksii

Re: [PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Jan Beulich]

On 01.03.2023 21:38, Julien Grall wrote:
> On 01/03/2023 17:50, Julien Grall wrote:
>> I got the answer. The problem now is gitlab only keep the artifact for 
>> the latest build and it only provide a zImage (having the ELF would be 
>> easier).
>>
>> I will try to reproduce the error on my end.
> 
> I managed to reproduce it. It looks like that after your bug patch, 
> *(.rodata.*) will not be end on a 4-byte boundary. Before your patch, 
> all the messages will be in .rodata.str. Now they are in .bug_frames.*, 
> so there some difference in .rodata.*.

Strings in .bug_frames.*? This sounds like a mistake, which - if indeed
the case - will want investigating before the conversion series is
actually considered for committing.

> That said, it is not entirely clear why we never seen the issue before 
> because my guessing there are no guarantee that .rodata.* will be 
> suitably aligned.
> 
> Anyway, here a proposal for the commit message:
> 
> "
> xen/arm: Ensure the start *(.proc.info) of is 4-byte aligned
> 
> The entries in *(.proc.info) are expected to be 4-byte aligned and the 
> compiler will access them using 4-byte load instructions. On Arm32, the 
> alignment is strictly enforced by the processor and will result to a 
> data abort if it is not correct.
> 
> However, the linker script doesn't encode this requirement. So we are at 
> the mercy of the compiler/linker to have aligned the previous sections 
> suitably.

May I suggest "aligned/padded", because it's really the tail of the
previous section which matters?

Jan

> This was spotted when trying to use the upcoming generic bug 
> infrastructure with the compiler provided by Yocto.
> 
> Link: 
> https://lore.kernel.org/xen-devel/6735859208c6dcb7320f89664ae298005f70827b.camel@gmail.com/
> "
> 
> If you are happy with the proposed commit message, then I can update it 
> while committing.
> 
> Cheers,
> 

Re: [PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Julien Grall]

Hi,

On 02/03/2023 09:45, Jan Beulich wrote:
> On 01.03.2023 21:38, Julien Grall wrote:
>> On 01/03/2023 17:50, Julien Grall wrote:
>>> I got the answer. The problem now is gitlab only keep the artifact for
>>> the latest build and it only provide a zImage (having the ELF would be
>>> easier).
>>>
>>> I will try to reproduce the error on my end.
>>
>> I managed to reproduce it. It looks like that after your bug patch,
>> *(.rodata.*) will not be end on a 4-byte boundary. Before your patch,
>> all the messages will be in .rodata.str. Now they are in .bug_frames.*,
>> so there some difference in .rodata.*.
> 
> Strings in .bug_frames.*? This sounds like a mistake, which - if indeed
> the case - will want investigating before the conversion series is
> actually considered for committing.

No. I misread the code. But there are definitely a difference in size:

Before:

Section Headers:
   [Nr] Name              Type            Addr     Off    Size   ES Flg 
Lk Inf Al
   [ 0]                   NULL            00000000 000000 000000 00 
0   0  0
   [ 1] .text             PROGBITS        00200000 008000 07e7a8 00 WAX 
0   0 32
   [ 2] .rodata           PROGBITS        0027f000 087000 02acc8 00   A 
0   0 16
   [ 3] .note.gnu.build-i NOTE            002a9cc8 0b1cc8 000024 00   A 
0   0  4
   [ 4] .data.ro_after_in PROGBITS        002aa000 0b2000 001000 00  WA 
0   0  8
   [ 5] .data.read_mostly PROGBITS        002ab000 0b3000 001118 00  WA 
0   0  8
   [ 6] .data             PROGBITS        002ad000 0b5000 006ca8 00  WA 
0   0 4096
   [ 7] .arch.info        PROGBITS        002b3ca8 0bbca8 000208 00   A 
0   0  4
   [ 8] .dev.info         PROGBITS        002b3eb0 0bbeb0 000070 00   A 
0   0  4
   [ 9] .init.text        PROGBITS        002b4000 0bc000 016054 00  AX 
0   0  8
   [10] .init.data        PROGBITS        002d0000 0d8000 030008 00  WA 
0   0 32768
   [11] .bss              NOBITS          00308000 108008 0394c0 00  WA 
0   0 4096
   [12] .debug_abbrev     PROGBITS        00000000 108008 03006e 00 
0   0  1
   [13] .debug_info       PROGBITS        00000000 138076 27b6b5 00 
0   0  1
   [14] .debug_str        PROGBITS        00000000 3b372b 01a123 01  MS 
0   0  1
   [15] .debug_line       PROGBITS        00000000 3cd84e 0a59e6 00 
0   0  1
   [16] .debug_frame      PROGBITS        00000000 473234 018cc8 00 
0   0  4
   [17] .debug_loc        PROGBITS        00000000 48befc 108473 00 
0   0  1
   [18] .debug_ranges     PROGBITS        00000000 594370 031450 00 
0   0  8
   [19] .debug_aranges    PROGBITS        00000000 5c57c0 004dd0 00 
0   0  8
   [20] .comment          PROGBITS        00000000 5ca590 00005d 01  MS 
0   0  1
   [21] .ARM.attributes   ARM_ATTRIBUTES  00000000 5ca5ed 000037 00 
0   0  1
   [22] .symtab           SYMTAB          00000000 5ca624 022d60 10 
23 7573  4
   [23] .strtab           STRTAB          00000000 5ed384 00c631 00 
0   0  1
   [24] .shstrtab         STRTAB          00000000 5f99b5 00010b 00 
0   0  1

After:

   [Nr] Name              Type            Addr     Off    Size   ES Flg 
Lk Inf Al
   [ 0]                   NULL            00000000 000000 000000 00 
0   0  0
   [ 1] .text             PROGBITS        00200000 008000 07e670 00 WAX 
0   0 32
   [ 2] .rodata           PROGBITS        0027f000 087000 02b3e8 00   A 
0   0 16
   [ 3] .note.gnu.build-i NOTE            002aa3e8 0b23e8 000024 00   A 
0   0  4
   [ 4] .data.ro_after_in PROGBITS        002ab000 0b3000 001000 00  WA 
0   0  8
   [ 5] .data.read_mostly PROGBITS        002ac000 0b4000 001118 00  WA 
0   0  8
   [ 6] .data             PROGBITS        002ae000 0b6000 006ca8 00  WA 
0   0 4096
   [ 7] .arch.info        PROGBITS        002b4ca8 0bcca8 000208 00   A 
0   0  4
   [ 8] .dev.info         PROGBITS        002b4eb0 0bceb0 000070 00   A 
0   0  4
   [ 9] .init.text        PROGBITS        002b5000 0bd000 016054 00  AX 
0   0  8
   [10] .init.data        PROGBITS        002d0000 0d8000 030008 00  WA 
0   0 32768
   [11] .bss              NOBITS          00308000 108008 0394c0 00  WA 
0   0 4096
   [12] .debug_abbrev     PROGBITS        00000000 108008 02fe48 00 
0   0  1
   [13] .debug_info       PROGBITS        00000000 137e50 27ac8f 00 
0   0  1
   [14] .debug_str        PROGBITS        00000000 3b2adf 01a107 01  MS 
0   0  1
   [15] .debug_line       PROGBITS        00000000 3ccbe6 0a590e 00 
0   0  1
   [16] .debug_frame      PROGBITS        00000000 4724f4 018da0 00 
0   0  4
   [17] .debug_loc        PROGBITS        00000000 48b294 10822c 00 
0   0  1
   [18] .debug_ranges     PROGBITS        00000000 5934c0 031028 00 
0   0  8
   [19] .debug_aranges    PROGBITS        00000000 5c44e8 004dd8 00 
0   0  8
   [20] .comment          PROGBITS        00000000 5c92c0 00005d 01  MS 
0   0  1
   [21] .ARM.attributes   ARM_ATTRIBUTES  00000000 5c931d 000037 00 
0   0  1
   [22] .symtab           SYMTAB          00000000 5c9354 0220f0 10 
23 7374  4
   [23] .strtab           STRTAB          00000000 5eb444 00c677 00 
0   0  1
   [24] .shstrtab         STRTAB          00000000 5f7abb 00010b 00 
0   0  1

It is not entirely clear why. Anyway, it doesn't matter too much.

Note that the size of Xen grew a little bit on Arm (+0.03%). I am not 
too concerned though as we now consolidated the BUG infrastructure. So 
that's a plus.

> 
>> That said, it is not entirely clear why we never seen the issue before
>> because my guessing there are no guarantee that .rodata.* will be
>> suitably aligned.
>>
>> Anyway, here a proposal for the commit message:
>>
>> "
>> xen/arm: Ensure the start *(.proc.info) of is 4-byte aligned
>>
>> The entries in *(.proc.info) are expected to be 4-byte aligned and the
>> compiler will access them using 4-byte load instructions. On Arm32, the
>> alignment is strictly enforced by the processor and will result to a
>> data abort if it is not correct.
>>
>> However, the linker script doesn't encode this requirement. So we are at
>> the mercy of the compiler/linker to have aligned the previous sections
>> suitably.
> 
> May I suggest "aligned/padded", because it's really the tail of the
> previous section which matters?

Sure.

Cheers,

-- 
Julien Grall

Re: [PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Jan Beulich]

On 02.03.2023 12:01, Julien Grall wrote:
> On 02/03/2023 09:45, Jan Beulich wrote:
>> On 01.03.2023 21:38, Julien Grall wrote:
>>> I managed to reproduce it. It looks like that after your bug patch,
>>> *(.rodata.*) will not be end on a 4-byte boundary. Before your patch,
>>> all the messages will be in .rodata.str. Now they are in .bug_frames.*,
>>> so there some difference in .rodata.*.
>>
>> Strings in .bug_frames.*? This sounds like a mistake, which - if indeed
>> the case - will want investigating before the conversion series is
>> actually considered for committing.
> 
> No. I misread the code. But there are definitely a difference in size:
> 
> Before:
> 
> Section Headers:
>    [Nr] Name              Type            Addr     Off    Size   ES Flg 
> Lk Inf Al
>    [ 0]                   NULL            00000000 000000 000000 00 
> 0   0  0
>    [ 1] .text             PROGBITS        00200000 008000 07e7a8 00 WAX 
> 0   0 32
>    [ 2] .rodata           PROGBITS        0027f000 087000 02acc8 00   A 
> 0   0 16
>[...]
> After:
> 
>    [Nr] Name              Type            Addr     Off    Size   ES Flg 
> Lk Inf Al
>    [ 0]                   NULL            00000000 000000 000000 00 
> 0   0  0
>    [ 1] .text             PROGBITS        00200000 008000 07e670 00 WAX 
> 0   0 32
>    [ 2] .rodata           PROGBITS        0027f000 087000 02b3e8 00   A 
> 0   0 16

I still find this concerning (as in: at least needing explanation), as
the bug frame representation shrinks in size: Entries for assertions
remain to be 4 ".long"-s, while all other entries use only two now. So
in a release build the size of all .bug_frame.* together should halve,
while in debug builds it should shrink at least some. And Oleksii's
series doesn't add meaningful other contributions to .rodata, iirc.

Jan

Re: [PATCH v1] xen/arm: align *(.proc.info) in the linker script

[Julien Grall]

Hi Jan,

On 02/03/2023 11:21, Jan Beulich wrote:
> On 02.03.2023 12:01, Julien Grall wrote:
>> On 02/03/2023 09:45, Jan Beulich wrote:
>>> On 01.03.2023 21:38, Julien Grall wrote:
>>>> I managed to reproduce it. It looks like that after your bug patch,
>>>> *(.rodata.*) will not be end on a 4-byte boundary. Before your patch,
>>>> all the messages will be in .rodata.str. Now they are in .bug_frames.*,
>>>> so there some difference in .rodata.*.
>>>
>>> Strings in .bug_frames.*? This sounds like a mistake, which - if indeed
>>> the case - will want investigating before the conversion series is
>>> actually considered for committing.
>>
>> No. I misread the code. But there are definitely a difference in size:
>>
>> Before:
>>
>> Section Headers:
>>     [Nr] Name              Type            Addr     Off    Size   ES Flg
>> Lk Inf Al
>>     [ 0]                   NULL            00000000 000000 000000 00
>> 0   0  0
>>     [ 1] .text             PROGBITS        00200000 008000 07e7a8 00 WAX
>> 0   0 32
>>     [ 2] .rodata           PROGBITS        0027f000 087000 02acc8 00   A
>> 0   0 16
>> [...]
>> After:
>>
>>     [Nr] Name              Type            Addr     Off    Size   ES Flg
>> Lk Inf Al
>>     [ 0]                   NULL            00000000 000000 000000 00
>> 0   0  0
>>     [ 1] .text             PROGBITS        00200000 008000 07e670 00 WAX
>> 0   0 32
>>     [ 2] .rodata           PROGBITS        0027f000 087000 02b3e8 00   A
>> 0   0 16
> 
> I still find this concerning (as in: at least needing explanation), as
> the bug frame representation shrinks in size: Entries for assertions
> remain to be 4 ".long"-s, while all other entries use only two now. So
> in a release build the size of all .bug_frame.* together should halve,
> while in debug builds it should shrink at least some. And Oleksii's
> series doesn't add meaningful other contributions to .rodata, iirc.

Doh, I inverted the two tables. So .rodata is shrinking but .txt is 
slightly increasing.

Cheers,

-- 
Julien Grall