Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-03-07 at 14:16 +0100, Jan Beulich wrote:
> On 07.03.2023 14:13, Oleksii wrote:
> > > > > > > +
> > > > > > > +#define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > > > > > > {                   \
> > > > > > > +    BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > > > > > > BUG_LINE_HI_WIDTH));         \
> > > > > > > +    BUILD_BUG_ON((type) >=
> > > > > > > BUGFRAME_NR);                                     \
> > > > > > > +    asm volatile (
> > > > > > > _ASM_BUGFRAME_TEXT(second_frame)                         
> > > > > > > \
> > > > > > > +                   :: _ASM_BUGFRAME_INFO(type, line,
> > > > > > > ptr,
> > > > > > > msg)
> > > > > > > );            \
> > > > > > > +} while (0)
> > > > > 
> > > > > Isn't this tied to BUG_FRAME_STRUCT being defined (or not)?
> > > > > At
> > > > > least
> > > > > the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to
> > > > > use
> > > > > the generic struct: With how you have things right now
> > > > > BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check
> > > > > would
> > > > > also
> > > > > be at risk of causing false positives. Perhaps it's
> > > > > appropriate
> > > > > to
> > > > > have a separate #ifdef (incl the distinct identifier used),
> > > > > but
> > > > > that
> > > > > first BUILD_BUG_ON() needs abstracting out.
> > > Missed that. Thanks.
> > > I'll introduce that a separate #ifdef before BUG_FRAME:
> > > 
> > > #ifndef BUILD_BUG_ON_LINE_WIDTH
> > > #define BUILD_BUG_ON_LINE_WIDTH \
> > >         BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > > BUG_LINE_HI_WIDTH))
> > > #endif
> > I think even better will be to do in the following way:
> > 
> > #ifndef LINE_WIDTH
> > #define LINE_WIDTH (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)
> > #endif
> > 
> > #define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > {            
> > \
> >     BUILD_BUG_ON((line) >>
> > LINE_WIDTH);                               
> > \
> >     BUILD_BUG_ON((type) >=
> > BUGFRAME_NR);                              
> > \
> >     asm volatile (
> > _ASM_BUGFRAME_TEXT(second_frame)                   
> > \
> >                    :: _ASM_BUGFRAME_INFO(type, line, ptr, msg)
> > );     
> > \
> > } while (false)
> 
> Not sure - that'll still require arches to define LINE_WIDTH. What
> if they store the line number in a 32-bit field? Then defining
> LINE_WIDTH to 32 would yield undefined behavior here.
> 
It might be an issue.

Than it will be better to have function-like macros mentioned in
previous e-mail.

Thanks

~ Oleksii