Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h

[Julien Grall <julien@xxxxxxx>]

On 01/03/2023 12:31, Oleksii wrote:
> Hi Julien,
Hi,

> > > > > On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > > > > > The following changes were made:
> > > > > > * make GENERIC_BUG_FRAME mandatory for ARM
> > > > > I have asked in patch #1 but will ask it again because I think
> > > > > this
> > > > > should be recorded in the commit message. Can you outline why
> > > > > it is
> > > > > not
> > > > > possible to completely switch to the generic version?
> > > > I have just tried to remove it on arm64 and it seems to work.
> > > > This
> > > > was
> > > > only tested with GCC 10 though. But given the generic version is
> > > > not
> > > > not
> > > > using the %c modifier, then I wouldn't expect any issue.
> > > >
> > > > Cheers,
> > > I tried to switch ARM to generic implementation.
> > >
> > > Here is the patch: [1]
> > This is similar to the patch I wrote to test with generic
> > implementation
> > on Arm (see my other reply).
> >
> > [...]
> >
> > > (it will be merged with patch 3 if it is OK )
> > >
> > > And looks like we can switch ARM to generic implementation as all
> > > tests
> > > passed:
> > > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396
> > Thanks for checking with the gitlab CI!
> >
> > > The only issue is with yocto-arm:
> > > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396/failures
> > > But I am not sure that it is because of my patch
> > This looks unrelated to me. This is happening because there is a data
> > abort before PSCI (used to reboot the platform) is properly setup. I
> > think we should consider to only print once the error rather than
> > every
> > few iterations (not a request for you).
> >
> > That said, I am a bit puzzled why this issue is only happening in the
> > Yocto test (the Debian one pass). Do you know if the test is passing
> > in
> > the normal CI?
> I checked several pipelines on the normal CI and it is fine.
> > If yes, what other modifications did you do?
> It looks like the issue happens after switch ARM to generic
> implementation of bug.h:
> -
> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792379063/failures
> [ failure ]
> -
> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792381665/failures
> [ passed ]
>
> The difference between builds is only in the commit ' check if ARM can
> be fully switched to generic implementation '.
> For second one it is reverted so it looks like we can't switch ARM to
> generic implementation now as it is required addiotional investigation.
Thanks. Looking at the error again, it looks like the data abort is 
because we are accessing an unaligned address.
From a brief look at arch/arm/xen.lds.S, I can at least see one case of 
misalignment (not clear why it would only happen now though). Can you try:
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 3f7ebd19f3ed..fb8155bd729f 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 = .;

> There is no other significant changes ( only the changes mentioned in
> the current mail thread ).
>
> Could we go ahead without switching ARM to generic implementation to
> not block other RISC-V patch series?
Given this is an alignment issue (Arm32 is more sensible to this than 
the other architecture, but this is still a potential problem for the 
other archs), I would really like to understand whether this is an issue 
in the common code or in the Arm linker script.
Cheers,

--
Julien Grall