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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.03.2023 12:32, Oleksii wrote:
> On Mon, 2023-03-06 at 11:17 +0100, Jan Beulich wrote:
>>> On 03.03.2023 11:38, Oleksii Kurochko wrote:
>>>>> --- a/xen/common/Kconfig
>>>>> +++ b/xen/common/Kconfig
>>>>> @@ -28,6 +28,9 @@ config ALTERNATIVE_CALL
>>>>>  config ARCH_MAP_DOMAIN_PAGE
>>>>>         bool
>>>>>  
>>>>> +config GENERIC_BUG_FRAME
>>>>> +       bool
>>>
>>> With Arm now also going to use the generic logic, do we actually
>>> need
>>> this
>>> control anymore (provided things have been proven to work on Arm
>>> for
>>> the
>>> compiler range we support there)? It looks like because of the way
>>> the
>>> series is partitioned it may be necessary transiently, but it
>>> should
>>> be
>>> possible to drop it again in a new 5th patch.
> We still need it because RISC-V doesn't ready yet to use do_bug_frame()
> from xen/common/bug.c as it requires find_text_region(),
> virtual_region() and panic().
> The mentioned functionality isn't ready now for RISC-V so RISC-V will
> use only generic things from <xen/bug.h> only for some time.

Oh, right. So let's leave it for the time being, but consider dropping
it once RISC-V is more complete.

>>>>> --- /dev/null
>>>>> +++ b/xen/include/xen/bug.h
>>>>> @@ -0,0 +1,158 @@
>>>>> +#ifndef __XEN_BUG_H__
>>>>> +#define __XEN_BUG_H__
>>>>> +
>>>>> +#define BUGFRAME_run_fn 0
>>>>> +#define BUGFRAME_warn   1
>>>>> +#define BUGFRAME_bug    2
>>>>> +#define BUGFRAME_assert 3
>>>>> +
>>>>> +#define BUGFRAME_NR     4
>>>>> +
>>>>> +#ifndef BUG_FRAME_STRUCT
>>>
>>> This check won't help when it comes ahead of ...
>>>
>>>>> +#define BUG_DISP_WIDTH    24
>>>>> +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
>>>>> +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
>>>>> +#endif
>>>>> +
>>>>> +#include <asm/bug.h>
>>>
>>> ... this. But is the #ifdef actually necessary? Or can the #define-
>>> s
>>> be moved ...
>>>
>>>>> +#ifndef BUG_DEBUGGER_TRAP_FATAL
>>>>> +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
>>>>> +#endif
>>>>> +
>>>>> +#ifndef __ASSEMBLY__
>>>>> +
>>>>> +#include <xen/lib.h>
>>>>> +
>>>>> +#ifndef BUG_FRAME_STRUCT
>>>
>>> ... here? (I guess having them defined early, but unconditionally
>>> is
>>> better. If an arch wants to override them, they can #undef and then
>>> #define.)
> We can't move it to #indef __ASSEMBLY__ because in this case x86
> compilation will fail as in x86's <asm/bug.h> BUG_DISP_WIDTH,
> BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH are used in case when the header
> is included in assembly code.

Oh, of course.

> I agree that there is no any sense to have the defines under "#ifndef
> BUG_FRAME_STUCT" before <asm/bug.h> but it is necessary to define them
> before <asm/bug.h>. The defines were put under "#ifndef
> BUG_FRAME_STUCT" because it seems to me that logically the defines
> should go only with definition of 'struct bug_frame'.
> 
> So it looks like the only one way we have. It is define them
> unconditionally before <asm/bug.h> and #undef if it will be necessary
> for specific architecture.
> As an option we can move the defines to #ifndef BUG_FRAME_STRUCT under
> #ifndef __ASSEMBLY__ and then define them explicitly in x86's
> <asm/bug.h> for case when the header is included some where in assembly
> code. But this option looks really weird.

Indeed.

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

But then please make this a function-like macro.

I'm also not convinced of the #ifndef - an arch using an entirely
different layout should have no need for this check. So I'd rather
attach the #define to what's inside #ifndef BUG_FRAME_STRUCT and
have an #else there to supply a #define to <empty> alternatively.

Jan