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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.02.2023 14:56, Julien Grall wrote:
> On 13/02/2023 13:30, Jan Beulich wrote:
>> On 13.02.2023 14:19, Julien Grall wrote:
>>> On 13/02/2023 12:24, Jan Beulich wrote:
>>>> On 03.02.2023 18:05, Oleksii Kurochko wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/include/xen/bug.h
>>>>> @@ -0,0 +1,127 @@
>>>>> +#ifndef __XEN_BUG_H__
>>>>> +#define __XEN_BUG_H__
>>>>> +
>>>>> +#define BUG_DISP_WIDTH    24
>>>>> +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
>>>>> +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
>>>>> +
>>>>> +#define BUGFRAME_run_fn 0
>>>>> +#define BUGFRAME_warn   1
>>>>> +#define BUGFRAME_bug    2
>>>>> +#define BUGFRAME_assert 3
>>>>> +
>>>>> +#define BUGFRAME_NR     4
>>>>> +
>>>>> +#ifndef __ASSEMBLY__
>>>>> +
>>>>> +#include <xen/errno.h>
>>>>> +#include <xen/stringify.h>
>>>>> +#include <xen/types.h>
>>>>> +#include <xen/lib.h>
>>>>
>>>> Again - please sort headers.
>>>>
>>>>> +#include <asm/bug.h>
>>>>> +
>>>>> +#ifndef BUG_FRAME_STUFF
>>>>> +struct bug_frame {
>>>>
>>>> Please can we have a blank line between the above two ones and then 
>>>> similarly
>>>> ahead of the #endif?
>>>>
>>>>> +    signed int loc_disp;    /* Relative address to the bug address */
>>>>> +    signed int file_disp;   /* Relative address to the filename */
>>>>> +    signed int msg_disp;    /* Relative address to the predicate (for 
>>>>> ASSERT) */
>>>>> +    uint16_t line;          /* Line number */
>>>>> +    uint32_t pad0:16;       /* Padding for 8-bytes align */
>>>>
>>>> Already the original comment in Arm code is wrong: The padding doesn't
>>>> guarantee 8-byte alignment; it merely guarantees a multiple of 8 bytes
>>>> size.
>>>> Aiui there's also no need for 8-byte alignment anywhere here (in
>>>> fact there's ".p2align 2" in the generator macros).
>>>
>>> I would rather keep the pad0 here.
>>
>> May I ask why that is?
> 
> It makes clear of the padding (which would have been hidden) when using 
> .p2align 2.

But you realize that I didn't ask to drop the member? Besides (later in
the reply to Oleksii) suggesting to make "line" uint32_t, you zapped the
relevant further part of my reply here:

"I also wonder why this needs to be a named bitfield: Either make it
 plain uint16_t, or if you use a bitfield, then omit the name."

I thought that's clear enough as a request to change representation,
but not asking for plain removal. The part of my reply that you commented
on is merely asking to not move a bogus comment (whether it gets corrected
up front or while being moved is secondary to me).

Jan