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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-03-01 at 13:58 +0000, Julien Grall wrote:
> 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.
I have a good news.

Alignment of "*(.proc.info)" helps but I checked only yocto-qemuarm:
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792923264

I ran all tests here:
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792953524

Should I create a separate patch with ALIGN if the tests are passed?

~ Oleksii