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

[Julien Grall <julien@xxxxxxx>]

On 28/02/2023 12:38, Oleksii wrote:
> Hi Julien,
Hi,

> 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?
> >
> > > +
> > >    #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...
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