Re: [PATCH v3 2/4] xen: change to
On Tue, 2023-02-28 at 14:30 +0100, Jan Beulich wrote: > On 28.02.2023 14:07, Oleksii wrote: > > On Sat, 2023-02-25 at 16:47 +, Julien Grall wrote: > > > On 24/02/2023 11:31, Oleksii Kurochko wrote: > > > > --- a/xen/arch/arm/include/asm/bug.h > > > > +++ b/xen/arch/arm/include/asm/bug.h > > > > @@ -1,6 +1,8 @@ > > > > #ifndef __ARM_BUG_H__ > > > > #define __ARM_BUG_H__ > > > > > > > > +#include > > > > > > You are not adding new code in bug.h, so can you explain why this > > > is > > > now > > > necessary? > > I am not adding new code but inside 'struct bug_frame' there are > > uint16_t and uint32_t which are defined in . > > > > And after was changed to it started to > > complain > > on these types. > > Wouldn't xen/bug.h want to include xen/types.h anyway, and then > clearly > before including asm/bug.h? can be moved to but generic implementation doesn't use any specific from types. But probably you are right that it would be better move to ... > > Jan ~ Oleksii
Re: [PATCH v3 2/4] xen: change to
On 28/02/2023 12:38, Oleksii wrote: Hi Julien, Hi, On Sat, 2023-02-25 at 16:47 +, Julien Grall wrote: Hi Oleksii, On 24/02/2023 11:31, Oleksii Kurochko wrote: Since the generic version of bug.h stuff was introduced use instead of unnecessary Signed-off-by: Oleksii Kurochko --- Changes in V3: * Update patch 2 not to break compilation: move some parts from patches 3 and 4 to patch 2: * move some generic parts from to * add define BUG_FRAME_STRUCT in ARM's --- Changes in V2: * Put [PATCH v1 4/4] xen: change to as second patch, update the patch to change all to among the whole project to not break build. * Update the commit message. --- xen/arch/arm/include/asm/bug.h | 19 +++ xen/arch/arm/include/asm/div64.h | 2 +- xen/arch/arm/vgic/vgic-v2.c | 2 +- xen/arch/arm/vgic/vgic.c | 2 +- xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +- xen/arch/x86/include/asm/asm_defns.h | 2 +- xen/arch/x86/include/asm/bug.h | 19 ++- xen/drivers/cpufreq/cpufreq.c | 2 +- xen/include/xen/lib.h | 2 +- 9 files changed, 12 insertions(+), 40 deletions(-) diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h index f4088d0913..cacaf014ab 100644 --- a/xen/arch/arm/include/asm/bug.h +++ b/xen/arch/arm/include/asm/bug.h @@ -1,6 +1,8 @@ #ifndef __ARM_BUG_H__ #define __ARM_BUG_H__ +#include You are not adding new code in bug.h, so can you explain why this is now necessary? + #if defined(CONFIG_ARM_32) # include #elif defined(CONFIG_ARM_64) @@ -9,9 +11,7 @@ # error "unknown ARM variant" #endif -#define BUG_DISP_WIDTH 24 -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) Even if the values are the same as the one you defined in the common bug.h, it doesn't feel right to remove them as long as... +#define BUG_FRAME_STRUCT the arch is defining BUG_FRAME_STRUCT. So I would say the generic one should be defined within BUG_FRAME_STRUCT. One of the reason BUG_DISP_WIDTH, BUG_LINE_* were removed is that they don't use in ARM at all... Hmmm ok. But this sort of things should have been documented in the commit message even thought it doesn't feel this is related to what the patch is doing. Cheers, -- Julien Grall
Re: [PATCH v3 2/4] xen: change to
On 28.02.2023 14:07, Oleksii wrote: > On Sat, 2023-02-25 at 16:47 +, Julien Grall wrote: >> On 24/02/2023 11:31, Oleksii Kurochko wrote: >>> --- a/xen/arch/arm/include/asm/bug.h >>> +++ b/xen/arch/arm/include/asm/bug.h >>> @@ -1,6 +1,8 @@ >>> #ifndef __ARM_BUG_H__ >>> #define __ARM_BUG_H__ >>> >>> +#include >> >> You are not adding new code in bug.h, so can you explain why this is >> now >> necessary? > I am not adding new code but inside 'struct bug_frame' there are > uint16_t and uint32_t which are defined in . > > And after was changed to it started to complain > on these types. Wouldn't xen/bug.h want to include xen/types.h anyway, and then clearly before including asm/bug.h? Jan
Re: [PATCH v3 2/4] xen: change to
On Mon, 2023-02-27 at 15:29 +0100, Jan Beulich wrote:
> On 24.02.2023 12:31, Oleksii Kurochko wrote:
> > Since the generic version of bug.h stuff was introduced use
> >
> > instead of unnecessary
>
> You keep saying "unnecessary" here, but that's not really correct.
> Including asm/bug.h alone simply becomes meaningless. So how about
> "... instead of now useless (in isolation) "?
>
> > --- a/xen/arch/x86/include/asm/bug.h
> > +++ b/xen/arch/x86/include/asm/bug.h
> > @@ -1,19 +1,10 @@
> > #ifndef __X86_BUG_H__
> > #define __X86_BUG_H__
> >
> > -#define BUG_DISP_WIDTH 24
> > -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> > -
> > -#define BUGFRAME_run_fn 0
> > -#define BUGFRAME_warn 1
> > -#define BUGFRAME_bug 2
> > -#define BUGFRAME_assert 3
> > -
> > -#define BUGFRAME_NR 4
> > -
> > #ifndef __ASSEMBLY__
> >
> > +#define BUG_FRAME_STRUCT
> > +
> > struct bug_frame {
> > signed int loc_disp:BUG_DISP_WIDTH;
> > unsigned int line_hi:BUG_LINE_HI_WIDTH;
>
> Why would x86 continue to define its own bug_frame (and other items)?
>
Because x86 will be switched to generic one in the following patches of
the patch series and right now it defines only BUG_FRAME_STRUCT which
means that it will not use generic one implemetation now.
The idea of the patch was to rename to with
minimal required changes to keep Xen compilable.
And I am going to back:
-#define BUG_DISP_WIDTH24
-#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
-#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
for the same reason as in ARM. these defines are related to 'stuct
bug_frame' so should go with it.
These defines will be removed when an architecture will be switched to
generic implementation.
> Jan
~ Oleksii
Re: [PATCH v3 2/4] xen: change to
On Sat, 2023-02-25 at 16:47 +, Julien Grall wrote: > Hi Oleksii, > > On 24/02/2023 11:31, Oleksii Kurochko wrote: > > Since the generic version of bug.h stuff was introduced use > > > > instead of unnecessary > > > > Signed-off-by: Oleksii Kurochko > > --- > > Changes in V3: > > * Update patch 2 not to break compilation: move some parts from > > patches 3 and 4 > > to patch 2: > > * move some generic parts from to > > * add define BUG_FRAME_STRUCT in ARM's > > --- > > Changes in V2: > > * Put [PATCH v1 4/4] xen: change to as > > second patch, > > update the patch to change all to among > > the whole project > > to not break build. > > * Update the commit message. > > --- > > xen/arch/arm/include/asm/bug.h | 19 +++ > > xen/arch/arm/include/asm/div64.h | 2 +- > > xen/arch/arm/vgic/vgic-v2.c | 2 +- > > xen/arch/arm/vgic/vgic.c | 2 +- > > xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +- > > xen/arch/x86/include/asm/asm_defns.h | 2 +- > > xen/arch/x86/include/asm/bug.h | 19 ++- > > xen/drivers/cpufreq/cpufreq.c | 2 +- > > xen/include/xen/lib.h | 2 +- > > 9 files changed, 12 insertions(+), 40 deletions(-) > > > > diff --git a/xen/arch/arm/include/asm/bug.h > > b/xen/arch/arm/include/asm/bug.h > > index f4088d0913..cacaf014ab 100644 > > --- a/xen/arch/arm/include/asm/bug.h > > +++ b/xen/arch/arm/include/asm/bug.h > > @@ -1,6 +1,8 @@ > > #ifndef __ARM_BUG_H__ > > #define __ARM_BUG_H__ > > > > +#include > > You are not adding new code in bug.h, so can you explain why this is > now > necessary? I am not adding new code but inside 'struct bug_frame' there are uint16_t and uint32_t which are defined in . And after was changed to it started to complain on these types. > > > + > > #if defined(CONFIG_ARM_32) > > # include > > #elif defined(CONFIG_ARM_64) > > @@ -9,9 +11,7 @@ > > # error "unknown ARM variant" > > #endif > > > > -#define BUG_DISP_WIDTH 24 > > -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) > > -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) > > Even if the values are the same as the one you defined in the common > bug.h, it doesn't feel right to remove them as long as... > > > +#define BUG_FRAME_STRUCT > > the arch is defining BUG_FRAME_STRUCT. So I would say the generic one > should be defined within BUG_FRAME_STRUCT. One of the reason BUG_DISP_WIDTH, BUG_LINE_* were removed is that they don't use in ARM at all... But generally I agree that it should be part of "#ifndef BUG_FRAME_STRUCT" as it is 'struct bug_frame is dependent on it and these defines look 'struct bug_frame' specific. I'll update that in new version of the patch. > > Cheers, >
Re: [PATCH v3 2/4] xen: change to
Hi Julien, On Sat, 2023-02-25 at 16:47 +, Julien Grall wrote: > Hi Oleksii, > > On 24/02/2023 11:31, Oleksii Kurochko wrote: > > Since the generic version of bug.h stuff was introduced use > > > > instead of unnecessary > > > > Signed-off-by: Oleksii Kurochko > > --- > > Changes in V3: > > * Update patch 2 not to break compilation: move some parts from > > patches 3 and 4 > > to patch 2: > > * move some generic parts from to > > * add define BUG_FRAME_STRUCT in ARM's > > --- > > Changes in V2: > > * Put [PATCH v1 4/4] xen: change to as > > second patch, > > update the patch to change all to among > > the whole project > > to not break build. > > * Update the commit message. > > --- > > xen/arch/arm/include/asm/bug.h | 19 +++ > > xen/arch/arm/include/asm/div64.h | 2 +- > > xen/arch/arm/vgic/vgic-v2.c | 2 +- > > xen/arch/arm/vgic/vgic.c | 2 +- > > xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +- > > xen/arch/x86/include/asm/asm_defns.h | 2 +- > > xen/arch/x86/include/asm/bug.h | 19 ++- > > xen/drivers/cpufreq/cpufreq.c | 2 +- > > xen/include/xen/lib.h | 2 +- > > 9 files changed, 12 insertions(+), 40 deletions(-) > > > > diff --git a/xen/arch/arm/include/asm/bug.h > > b/xen/arch/arm/include/asm/bug.h > > index f4088d0913..cacaf014ab 100644 > > --- a/xen/arch/arm/include/asm/bug.h > > +++ b/xen/arch/arm/include/asm/bug.h > > @@ -1,6 +1,8 @@ > > #ifndef __ARM_BUG_H__ > > #define __ARM_BUG_H__ > > > > +#include > > You are not adding new code in bug.h, so can you explain why this is > now > necessary? > > > + > > #if defined(CONFIG_ARM_32) > > # include > > #elif defined(CONFIG_ARM_64) > > @@ -9,9 +11,7 @@ > > # error "unknown ARM variant" > > #endif > > > > -#define BUG_DISP_WIDTH 24 > > -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) > > -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) > > Even if the values are the same as the one you defined in the common > bug.h, it doesn't feel right to remove them as long as... > > > +#define BUG_FRAME_STRUCT > > the arch is defining BUG_FRAME_STRUCT. So I would say the generic one > should be defined within BUG_FRAME_STRUCT. > One of the reason BUG_DISP_WIDTH, BUG_LINE_* were removed is that they don't use in ARM at all... But generally I agree that it should be part of "#ifndef BUG_FRAME_STRUCT" as it is 'struct bug_frame is dependent on it and these defines look 'struct bug_frame' specific. I'll update that in new version of the patch. > Cheers, > ~ Oleksii
Re: [PATCH v3 2/4] xen: change to
On 24.02.2023 12:31, Oleksii Kurochko wrote:
> Since the generic version of bug.h stuff was introduced use
> instead of unnecessary
You keep saying "unnecessary" here, but that's not really correct.
Including asm/bug.h alone simply becomes meaningless. So how about
"... instead of now useless (in isolation) "?
> --- a/xen/arch/x86/include/asm/bug.h
> +++ b/xen/arch/x86/include/asm/bug.h
> @@ -1,19 +1,10 @@
> #ifndef __X86_BUG_H__
> #define __X86_BUG_H__
>
> -#define BUG_DISP_WIDTH24
> -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> -
> -#define BUGFRAME_run_fn 0
> -#define BUGFRAME_warn 1
> -#define BUGFRAME_bug2
> -#define BUGFRAME_assert 3
> -
> -#define BUGFRAME_NR 4
> -
> #ifndef __ASSEMBLY__
>
> +#define BUG_FRAME_STRUCT
> +
> struct bug_frame {
> signed int loc_disp:BUG_DISP_WIDTH;
> unsigned int line_hi:BUG_LINE_HI_WIDTH;
Why would x86 continue to define its own bug_frame (and other items)?
Jan
Re: [PATCH v3 2/4] xen: change to
Hi Oleksii, On 24/02/2023 11:31, Oleksii Kurochko wrote: Since the generic version of bug.h stuff was introduced use instead of unnecessary Signed-off-by: Oleksii Kurochko --- Changes in V3: * Update patch 2 not to break compilation: move some parts from patches 3 and 4 to patch 2: * move some generic parts from to * add define BUG_FRAME_STRUCT in ARM's --- Changes in V2: * Put [PATCH v1 4/4] xen: change to as second patch, update the patch to change all to among the whole project to not break build. * Update the commit message. --- xen/arch/arm/include/asm/bug.h | 19 +++ xen/arch/arm/include/asm/div64.h | 2 +- xen/arch/arm/vgic/vgic-v2.c | 2 +- xen/arch/arm/vgic/vgic.c | 2 +- xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +- xen/arch/x86/include/asm/asm_defns.h | 2 +- xen/arch/x86/include/asm/bug.h | 19 ++- xen/drivers/cpufreq/cpufreq.c| 2 +- xen/include/xen/lib.h| 2 +- 9 files changed, 12 insertions(+), 40 deletions(-) diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h index f4088d0913..cacaf014ab 100644 --- a/xen/arch/arm/include/asm/bug.h +++ b/xen/arch/arm/include/asm/bug.h @@ -1,6 +1,8 @@ #ifndef __ARM_BUG_H__ #define __ARM_BUG_H__ +#include You are not adding new code in bug.h, so can you explain why this is now necessary? + #if defined(CONFIG_ARM_32) # include #elif defined(CONFIG_ARM_64) @@ -9,9 +11,7 @@ # error "unknown ARM variant" #endif -#define BUG_DISP_WIDTH24 -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) Even if the values are the same as the one you defined in the common bug.h, it doesn't feel right to remove them as long as... +#define BUG_FRAME_STRUCT the arch is defining BUG_FRAME_STRUCT. So I would say the generic one should be defined within BUG_FRAME_STRUCT. Cheers, -- Julien Grall
[PATCH v3 2/4] xen: change to
Since the generic version of bug.h stuff was introduced use
instead of unnecessary
Signed-off-by: Oleksii Kurochko
---
Changes in V3:
* Update patch 2 not to break compilation: move some parts from patches 3 and 4
to patch 2:
* move some generic parts from to
* add define BUG_FRAME_STRUCT in ARM's
---
Changes in V2:
* Put [PATCH v1 4/4] xen: change to as second patch,
update the patch to change all to among the whole
project
to not break build.
* Update the commit message.
---
xen/arch/arm/include/asm/bug.h | 19 +++
xen/arch/arm/include/asm/div64.h | 2 +-
xen/arch/arm/vgic/vgic-v2.c | 2 +-
xen/arch/arm/vgic/vgic.c | 2 +-
xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +-
xen/arch/x86/include/asm/asm_defns.h | 2 +-
xen/arch/x86/include/asm/bug.h | 19 ++-
xen/drivers/cpufreq/cpufreq.c| 2 +-
xen/include/xen/lib.h| 2 +-
9 files changed, 12 insertions(+), 40 deletions(-)
diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
index f4088d0913..cacaf014ab 100644
--- a/xen/arch/arm/include/asm/bug.h
+++ b/xen/arch/arm/include/asm/bug.h
@@ -1,6 +1,8 @@
#ifndef __ARM_BUG_H__
#define __ARM_BUG_H__
+#include
+
#if defined(CONFIG_ARM_32)
# include
#elif defined(CONFIG_ARM_64)
@@ -9,9 +11,7 @@
# error "unknown ARM variant"
#endif
-#define BUG_DISP_WIDTH24
-#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
-#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
+#define BUG_FRAME_STRUCT
struct bug_frame {
signed int loc_disp;/* Relative address to the bug address */
@@ -26,13 +26,6 @@ struct bug_frame {
#define bug_line(b) ((b)->line)
#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
-#define BUGFRAME_run_fn 0
-#define BUGFRAME_warn 1
-#define BUGFRAME_bug2
-#define BUGFRAME_assert 3
-
-#define BUGFRAME_NR 4
-
/* 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
@@ -89,12 +82,6 @@ struct bug_frame {
unreachable(); \
} while (0)
-extern const struct bug_frame __start_bug_frames[],
- __stop_bug_frames_0[],
- __stop_bug_frames_1[],
- __stop_bug_frames_2[],
- __stop_bug_frames_3[];
-
#endif /* __ARM_BUG_H__ */
/*
* Local variables:
diff --git a/xen/arch/arm/include/asm/div64.h b/xen/arch/arm/include/asm/div64.h
index 1cd58bc51a..fc667a80f9 100644
--- a/xen/arch/arm/include/asm/div64.h
+++ b/xen/arch/arm/include/asm/div64.h
@@ -74,7 +74,7 @@
#elif __GNUC__ >= 4
-#include
+#include
/*
* If the divisor happens to be constant, we determine the appropriate
diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index 1a99d3a8b4..c90e88fddb 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -16,8 +16,8 @@
*/
#include
-#include
#include
+#include
#include
#include
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index f0f2ea5021..b9463a5f27 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -15,9 +15,9 @@
* along with this program. If not, see
