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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan