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
