Re: [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h>

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Sat, 2023-02-25 at 16:47 +0000, 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
> > <xen/bug.h>
> > instead of unnecessary <asm/bug.h>
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > 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 <asm/bug.h> to <xen/bug.h>
> >     * add define BUG_FRAME_STRUCT in ARM's <asm/bug.h>
> > ---
> > Changes in V2:
> >   * Put [PATCH v1 4/4] xen: change <asm/bug.h> to <xen/bug.h> as
> > second patch,
> >     update the patch to change all <asm/bug.h> to <xen/bug.h> 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 <xen/types.h>
> 
> 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 <xen/types.h>.

And after <asm/bug.h> was changed to <xen/bug.h> it started to complain
on these types.
> 
> > +
> >   #if defined(CONFIG_ARM_32)
> >   # include <asm/arm32/bug.h>
> >   #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,
>