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

[Julien Grall <julien@xxxxxxx>]

Hi,

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.
> 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).
I am fine with both suggestions.

Cheers,

--
Julien Grall