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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.02.2023 16:07, Julien Grall wrote:
> On 13/02/2023 15:02, Jan Beulich wrote:
>> 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?
> 
> I misunderstood your first reply. But the second reply...
> 
>   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,
> 
> ... "May I ask why that is?" added to the confusion because it implied 
> that you disagree on keep the pad0.

Hmm, yes, I can see how that may have been ambiguous: I understood that
reply of yours as requesting to retain the _name_ (i.e. objecting to my
thought of using an unnamed bitfield instead).

Jan