| Message ID | d80c136720c156d6ef83f27f1ce8dca5dba5b5a0.1677233393.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | introduce generic implementation of macros from bug.h | expand |
Hi Oleksii, 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? Cheers,
On 25/02/2023 16:49, Julien Grall wrote: > Hi Oleksii, > > 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,
Hi Julien, On Sat, 2023-02-25 at 16:49 +0000, Julien Grall wrote: > Hi Oleksii, > > 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 haven't tried to switch ARM too because of comment regarding 'i' in <asm/bug.h>: /* * GCC will not allow to use "i" when PIE is enabled (Xen doesn't set the * flag but instead rely on the default value from the compiler). So the * easiest way to implement run_in_exception_handler() is to pass the to * be called function in a fixed register. */ #define run_in_exception_handler(fn) do { \ So I decided to leave ARM implementation as is. But I'll try to switch it for the test and look at tests. If tests won't fail that I'll switch ARM to generic implementation too. > > Cheers, > ~ Oleksii
Hi Julien, On Sat, 2023-02-25 at 17:05 +0000, Julien Grall wrote: > > > On 25/02/2023 16:49, Julien Grall wrote: > > Hi Oleksii, > > > > 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] diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h index e6cc37e1d6..ffb0f569fc 100644 --- a/xen/arch/arm/include/asm/bug.h +++ b/xen/arch/arm/include/asm/bug.h @@ -1,8 +1,6 @@ #ifndef __ARM_BUG_H__ #define __ARM_BUG_H__ -#include <xen/types.h> - #if defined(CONFIG_ARM_32) # include <asm/arm32/bug.h> #elif defined(CONFIG_ARM_64) @@ -11,63 +9,7 @@ # error "unknown ARM variant" #endif -#define BUG_FRAME_STRUCT - -struct bug_frame { - signed int loc_disp; /* Relative address to the bug address */ - signed int file_disp; /* Relative address to the filename */ - signed int msg_disp; /* Relative address to the predicate (for ASSERT) */ - uint16_t line; /* Line number */ - uint32_t pad0:16; /* Padding for 8-bytes align */ -}; - -#define bug_ptr(b) ((const void *)(b) + (b)->file_disp) -#define bug_line(b) ((b)->line) -#define bug_msg(b) ((const char *)(b) + (b)->msg_disp) - -/* Many versions of GCC doesn't support the asm %c parameter which would - * be preferable to this unpleasantness. We use mergeable string - * sections to avoid multiple copies of the string appearing in the - * Xen image. BUGFRAME_run_fn needs to be handled separately. - */ -#define BUG_FRAME(type, line, file, has_msg, msg) do { \ - BUILD_BUG_ON((line) >> 16); \ - BUILD_BUG_ON((type) >= BUGFRAME_NR); \ - asm ("1:"BUG_INSTR"\n" \ - ".pushsection .rodata.str, \"aMS\", %progbits, 1\n" \ - "2:\t.asciz " __stringify(file) "\n" \ - "3:\n" \ - ".if " #has_msg "\n" \ - "\t.asciz " #msg "\n" \ - ".endif\n" \ - ".popsection\n" \ - ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ - "4:\n" \ - ".p2align 2\n" \ - ".long (1b - 4b)\n" \ - ".long (2b - 4b)\n" \ - ".long (3b - 4b)\n" \ - ".hword " __stringify(line) ", 0\n" \ - ".popsection"); \ -} while (0) - -/* - * GCC will not allow to use "i" when PIE is enabled (Xen doesn't set the - * flag but instead rely on the default value from the compiler). So the - * easiest way to implement run_in_exception_handler() is to pass the to - * be called function in a fixed register. - */ -#define run_in_exception_handler(fn) do { \ - asm ("mov " __stringify(BUG_FN_REG) ", %0\n" \ - "1:"BUG_INSTR"\n" \ - ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) "," \ - " \"a\", %%progbits\n" \ - "2:\n" \ - ".p2align 2\n" \ - ".long (1b - 2b)\n" \ - ".long 0, 0, 0\n" \ - ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG) ); \ -} while (0) +#define BUG_ASM_CONST "c" #endif /* __ARM_BUG_H__ */ ... (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 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 Is this enough from a verification point of view? [1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/5ff7a06e1d354e1e42bde1c203f3cf05a3653ad6 https://gitlab.com/xen-project/people/olkur/xen/-/commit/5ff7a06e1d354e1e42bde1c203f3cf05a3653ad6
Hi Oleksii, On 28/02/2023 15:09, Oleksii wrote: > On Sat, 2023-02-25 at 16:49 +0000, Julien Grall wrote: >> Hi Oleksii, >> >> 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 haven't tried to switch ARM too because of comment regarding 'i' in > <asm/bug.h>: > /* > * GCC will not allow to use "i" when PIE is enabled (Xen doesn't set > the > * flag but instead rely on the default value from the compiler). So > the > * easiest way to implement run_in_exception_handler() is to pass the > to > * be called function in a fixed register. > */ I would expect this comment to be valid for any arch. So if there is a need to deal with PIE, then we would not be able to use "i" in the BUG frame. Note that we are now explicitly compiling Xen without PIE (see Config.mk). Cheers,
On 28/02/2023 17:21, Oleksii wrote: > Hi Julien, Hi Oleksii, > On Sat, 2023-02-25 at 17:05 +0000, Julien Grall wrote: >> >> >> On 25/02/2023 16:49, Julien Grall wrote: >>> Hi Oleksii, >>> >>> 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? If yes, what other modifications did you do? Cheers,
On Tue, 2023-02-28 at 17:48 +0000, Julien Grall wrote: > Hi Oleksii, > > On 28/02/2023 15:09, Oleksii wrote: > > On Sat, 2023-02-25 at 16:49 +0000, Julien Grall wrote: > > > Hi Oleksii, > > > > > > 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 haven't tried to switch ARM too because of comment regarding 'i' > > in > > <asm/bug.h>: > > /* > > * GCC will not allow to use "i" when PIE is enabled (Xen doesn't > > set > > the > > * flag but instead rely on the default value from the compiler). > > So > > the > > * easiest way to implement run_in_exception_handler() is to pass > > the > > to > > * be called function in a fixed register. > > */ > > I would expect this comment to be valid for any arch. So if there is > a > need to deal with PIE, then we would not be able to use "i" in the > BUG > frame. > > Note that we are now explicitly compiling Xen without PIE (see > Config.mk). Then it looks like some architectures isn't expected to be compiled with PIE. I mean that x86's bug.h is used 'i' and there is no any alternative version in case of PIE. If Xen should be compilable with PIE then we have to use ARM implementation of bug.h everywhere. ( based on comment about 'i' with PIE ). Now I am totally confused... ~ Oleksii
Hi, On 01/03/2023 08:58, Oleksii wrote: > On Tue, 2023-02-28 at 17:48 +0000, Julien Grall wrote: >> Hi Oleksii, >> >> On 28/02/2023 15:09, Oleksii wrote: >>> On Sat, 2023-02-25 at 16:49 +0000, Julien Grall wrote: >>>> Hi Oleksii, >>>> >>>> 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 haven't tried to switch ARM too because of comment regarding 'i' >>> in >>> <asm/bug.h>: >>> /* >>> * GCC will not allow to use "i" when PIE is enabled (Xen doesn't >>> set >>> the >>> * flag but instead rely on the default value from the compiler). >>> So >>> the >>> * easiest way to implement run_in_exception_handler() is to pass >>> the >>> to >>> * be called function in a fixed register. >>> */ >> >> I would expect this comment to be valid for any arch. So if there is >> a >> need to deal with PIE, then we would not be able to use "i" in the >> BUG >> frame. >> >> Note that we are now explicitly compiling Xen without PIE (see >> Config.mk). > Then it looks like some architectures isn't expected to be compiled > with PIE. I mean that x86's bug.h is used 'i' and there is no any > alternative version in case of PIE. > > If Xen should be compilable with PIE then we have to use ARM > implementation of bug.h everywhere. ( based on comment about 'i' with > PIE ). The comment was added because until commit ecd6b9759919 ("Config.mk: correct PIE-related option(s) in EMBEDDED_EXTRA_CFLAGS") we would let the compiler to decide whether PIE should be enabled. Now we are forcing -fno-pie for Xen on any architecture (the flag is added at the top-level in Config.mk). > > Now I am totally confused... My point was not about using the Arm implementation everywhere. My point was that we disable even for Arm and therefore it is fine to use the common version. If in the future we need to support PIE in Xen (I am not aware of any effort yet), then we could decide to update the common BUG framework. But for now, I don't think this is something you need to care in your series. Cheers,
Hi Julien, > > > > > > > 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. 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? ~ Oleksii
On Wed, 2023-03-01 at 09:31 +0000, Julien Grall wrote: > Hi, > > On 01/03/2023 08:58, Oleksii wrote: > > On Tue, 2023-02-28 at 17:48 +0000, Julien Grall wrote: > > > Hi Oleksii, > > > > > > On 28/02/2023 15:09, Oleksii wrote: > > > > On Sat, 2023-02-25 at 16:49 +0000, Julien Grall wrote: > > > > > Hi Oleksii, > > > > > > > > > > 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 haven't tried to switch ARM too because of comment regarding > > > > 'i' > > > > in > > > > <asm/bug.h>: > > > > /* > > > > * GCC will not allow to use "i" when PIE is enabled (Xen > > > > doesn't > > > > set > > > > the > > > > * flag but instead rely on the default value from the > > > > compiler). > > > > So > > > > the > > > > * easiest way to implement run_in_exception_handler() is to > > > > pass > > > > the > > > > to > > > > * be called function in a fixed register. > > > > */ > > > > > > I would expect this comment to be valid for any arch. So if there > > > is > > > a > > > need to deal with PIE, then we would not be able to use "i" in > > > the > > > BUG > > > frame. > > > > > > Note that we are now explicitly compiling Xen without PIE (see > > > Config.mk). > > Then it looks like some architectures isn't expected to be compiled > > with PIE. I mean that x86's bug.h is used 'i' and there is no any > > alternative version in case of PIE. > > > > If Xen should be compilable with PIE then we have to use ARM > > implementation of bug.h everywhere. ( based on comment about 'i' > > with > > PIE ). > > The comment was added because until commit ecd6b9759919 ("Config.mk: > correct PIE-related option(s) in EMBEDDED_EXTRA_CFLAGS") we would let > the compiler to decide whether PIE should be enabled. > > Now we are forcing -fno-pie for Xen on any architecture (the flag is > added at the top-level in Config.mk). > > > > > Now I am totally confused... > > My point was not about using the Arm implementation everywhere. My > point > was that we disable even for Arm and therefore it is fine to use the > common version. > > If in the future we need to support PIE in Xen (I am not aware of any > effort yet), then we could decide to update the common BUG framework. > But for now, I don't think this is something you need to care in your > series. > Thanks for clarification. > Cheers, >
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,
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
Hi Oleksii, On 01/03/2023 15:16, Oleksii wrote: > On Wed, 2023-03-01 at 13:58 +0000, Julien Grall wrote: >> On 01/03/2023 12:31, Oleksii wrote: >> 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? Yes please. But, to be honest, I am not entirely sure what is not aligned before hand. Do you know if it is possible to download the Xen binary from gitlab? Cheers,
Hi Julien, > On 01/03/2023 15:16, Oleksii wrote: > > On Wed, 2023-03-01 at 13:58 +0000, Julien Grall wrote: > > > On 01/03/2023 12:31, Oleksii wrote: > > > 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? > > Yes please. But, to be honest, I am not entirely sure what is not > aligned before hand. Do you know if it is possible to download the > Xen > binary from gitlab? It is possible. Please go to the link of the job: https://gitlab.com/xen-project/people/olkur/xen/-/jobs/3856617252 And on the right you will find 'Job artificats' where you can click 'Download'. Or in case if you need a particular binary can click 'Browse' and go to Artifcats/Binaries/: https://gitlab.com/xen-project/people/olkur/xen/-/jobs/3856617252/artifacts/browse/binaries/ ~ Oleksii
On Wed, 2023-03-01 at 15:21 +0000, Julien Grall wrote: > Hi Oleksii, > > On 01/03/2023 15:16, Oleksii wrote: > > On Wed, 2023-03-01 at 13:58 +0000, Julien Grall wrote: > > > On 01/03/2023 12:31, Oleksii wrote: > > > 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? > > Yes please. But, to be honest, I am not entirely sure what is not > aligned before hand. Do you know if it is possible to download the > Xen > binary from gitlab? It is possible. Please go to the link of the job: https://gitlab.com/xen-project/people/olkur/xen/-/jobs/3856617252 And on the right you will find 'Job artificats' where you can click 'Download'. Or in case if you need a particular binary can click 'Browse' and go to Artifcats/Binaries/: https://gitlab.com/xen-project/people/olkur/xen/-/jobs/3856617252/artifacts/browse/binaries/ ~ Oleksii
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 239d3aed3c..aad6644a7b 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -12,6 +12,7 @@ config ARM_64 config ARM def_bool y + select GENERIC_BUG_FRAME select HAS_ALTERNATIVE select HAS_DEVICE_TREE select HAS_PASSTHROUGH diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c index a2fc1c22cb..61c61132c7 100644 --- a/xen/arch/arm/arm32/traps.c +++ b/xen/arch/arm/arm32/traps.c @@ -48,7 +48,7 @@ void do_trap_undefined_instruction(struct cpu_user_regs *regs) if ( instr != BUG_OPCODE ) goto die; - if ( do_bug_frame(regs, pc) ) + if ( do_bug_frame(regs, pc) < 0 ) goto die; regs->pc += 4; diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h index cacaf014ab..e6cc37e1d6 100644 --- a/xen/arch/arm/include/asm/bug.h +++ b/xen/arch/arm/include/asm/bug.h @@ -21,8 +21,7 @@ struct bug_frame { uint32_t pad0:16; /* Padding for 8-bytes align */ }; -#define bug_loc(b) ((const void *)(b) + (b)->loc_disp) -#define bug_file(b) ((const void *)(b) + (b)->file_disp); +#define bug_ptr(b) ((const void *)(b) + (b)->file_disp) #define bug_line(b) ((b)->line) #define bug_msg(b) ((const char *)(b) + (b)->msg_disp) @@ -70,18 +69,6 @@ struct bug_frame { ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG) ); \ } while (0) -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "") - -#define BUG() do { \ - BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, ""); \ - unreachable(); \ -} while (0) - -#define assert_failed(msg) do { \ - BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \ - unreachable(); \ -} while (0) - #endif /* __ARM_BUG_H__ */ /* * Local variables: diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h index 883dae368e..c6518008ec 100644 --- a/xen/arch/arm/include/asm/traps.h +++ b/xen/arch/arm/include/asm/traps.h @@ -69,8 +69,6 @@ void do_cp(struct cpu_user_regs *regs, const union hsr hsr); void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr); void do_trap_hvc_smccc(struct cpu_user_regs *regs); -int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc); - void noreturn do_unexpected_trap(const char *msg, const struct cpu_user_regs *regs); void do_trap_hyp_sync(struct cpu_user_regs *regs); diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 061c92acbd..9c6eb66422 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1197,85 +1197,6 @@ void do_unexpected_trap(const char *msg, const struct cpu_user_regs *regs) panic("CPU%d: Unexpected Trap: %s\n", smp_processor_id(), msg); } -int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc) -{ - const struct bug_frame *bug = NULL; - const char *prefix = "", *filename, *predicate; - unsigned long fixup; - int id = -1, lineno; - const struct virtual_region *region; - - region = find_text_region(pc); - if ( region ) - { - for ( id = 0; id < BUGFRAME_NR; id++ ) - { - const struct bug_frame *b; - unsigned int i; - - for ( i = 0, b = region->frame[id].bugs; - i < region->frame[id].n_bugs; b++, i++ ) - { - if ( ((vaddr_t)bug_loc(b)) == pc ) - { - bug = b; - goto found; - } - } - } - } - found: - if ( !bug ) - return -ENOENT; - - if ( id == BUGFRAME_run_fn ) - { - void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG; - - fn(regs); - return 0; - } - - /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ - filename = bug_file(bug); - if ( !is_kernel(filename) ) - return -EINVAL; - fixup = strlen(filename); - if ( fixup > 50 ) - { - filename += fixup - 47; - prefix = "..."; - } - lineno = bug_line(bug); - - switch ( id ) - { - case BUGFRAME_warn: - printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); - show_execution_state(regs); - return 0; - - case BUGFRAME_bug: - printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); - show_execution_state(regs); - panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno); - - case BUGFRAME_assert: - /* ASSERT: decode the predicate string pointer. */ - predicate = bug_msg(bug); - if ( !is_kernel(predicate) ) - predicate = "<unknown>"; - - printk("Assertion '%s' failed at %s%s:%d\n", - predicate, prefix, filename, lineno); - show_execution_state(regs); - panic("Assertion '%s' failed at %s%s:%d\n", - predicate, prefix, filename, lineno); - } - - return -EINVAL; -} - #ifdef CONFIG_ARM_64 static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr) { @@ -1292,7 +1213,7 @@ static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr) switch ( hsr.brk.comment ) { case BRK_BUG_FRAME_IMM: - if ( do_bug_frame(regs, regs->pc) ) + if ( do_bug_frame(regs, regs->pc) < 0 ) goto die; regs->pc += 4;
The following changes were made: * make GENERIC_BUG_FRAME mandatory for ARM * As do_bug_frame() returns -EINVAL in case something goes wrong otherwise id of bug frame. Thereby 'if' cases where do_bug_frame() was updated to check if the returned value is less than 0 * Change macros bug_file() to bug_ptr() to align it with generic implementation. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V3: * As prototype and what do_bug_frame() returns was changed so patch 3 and 4 was updated to use a new version of do_bug_frame --- Changes in V2: * Rename bug_file() in ARM implementation to bug_ptr() as generic do_bug_frame() uses bug_ptr(). * Remove generic parts from bug.h * Remove declaration of 'int do_bug_frame(...)' from <asm/traps.h> as it was introduced in <xen/bug.h> --- xen/arch/arm/Kconfig | 1 + xen/arch/arm/arm32/traps.c | 2 +- xen/arch/arm/include/asm/bug.h | 15 +----- xen/arch/arm/include/asm/traps.h | 2 - xen/arch/arm/traps.c | 81 +------------------------------- 5 files changed, 4 insertions(+), 97 deletions(-)