On Tue, 2023-02-14 at 17:55 +0100, Jan Beulich wrote:
> On 14.02.2023 17:22, Oleksii wrote:
> > On Mon, 2023-02-13 at 13:24 +0100, Jan Beulich wrote:
> > > On 03.02.2023 18:05, Oleksii Kurochko wrote:
> > > > --- a/xen/common/Kconfig
> > > > +++ b/xen/common/Kconfig
> > > > @@ -92,6 +92,12 @@ config STATIC_MEMORY
> > > >
> > > > If unsure, say N.
> > > >
> > > > +config GENERIC_DO_BUG_FRAME
> > > > + bool
> > > > + help
> > > > + Generic do_bug_frame() function is needed to handle
> > > > the
> > > > type of bug
> > > > + frame and print an information about it.
> > >
> > > Generally help text for prompt-less functions is not very useful.
> > > Assuming
> > > it is put here to inform people actually reading the source file,
> > > I'm
> > > okay
> > > for it to be left here, but please at least drop the stray "an".
> > > I
> > > also
> > > think this may want moving up in the file, e.g. ahead of all the
> > > HAS_*
> > > controls (which, as you will notice, all have no help text
> > > either).
> > > Plus
> > > finally how about shorter and more applicable GENERIC_BUG_FRAME -
> > > after
> > > all what becomes generic is more than just do_bug_frame()?
> > Thanks for the comments. I will take them into account.
> > Right now only do_bug_frame() is expected to be generic.
>
> Hmm, do you mean to undo part of what you've done? I didn't think
> you would. Yet in what you've done so far, the struct an several
> macros are also generalized. Hence the "DO" in the name is too
> narrow from my pov.
>
No, I don't undo part of what I have done.
I misunderstood your initial message. So yeah, I will remove "DO" I
think it will be more correct.
> > > > --- /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?
> > Sure.
> >
> > >
> > > > + 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 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.
> > >
> > It will better to change it to 'uint16_t' as I don't see any reason
> > to
> > use 'uint32_t' with bitfield here.
> > I'll check if we need the alignment. If there is '.p2align 2' we
> > really don't need it.
>
> See Julien's replies any my responses: FTAOD I did _not_ ask to
> remove
> the field.
I didn't see his reply so I'll read it too.
>
> > > > +};
> > > > +
> > > > +#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> > > > +#define bug_file(b) ((const void *)(b) + (b)->file_disp);
> > > > +#define bug_line(b) ((b)->line)
> > > > +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
> > > > +#endif /* BUG_FRAME_STUFF */
> > > > +
> > > > +#ifndef BUG_FRAME
> > > > +/* Many versions of GCC doesn't support the asm %c parameter
> > > > which
> > > > would
> > > > + * be preferable to this unpleasantness. We use mergeable
> > > > string
> > > > + * sections to avoid multiple copies of the string appearing
> > > > in
> > > > the
> > > > + * Xen image. BUGFRAME_run_fn needs to be handled separately.
> > > > + */
> > >
> > > When generalizing the logic, I wonder in how far the comment
> > > doesn't
> > > want re-wording some. For example, while Arm prefixes constant
> > > insn
> > > operands with # (and x86 uses $), there's no such prefix in RISC-
> > > V.
> > > At
> > > which point there's no need to use %c in the first place. (Which
> > > in
> > > turn means x86'es more compact representation may also be usable
> > > there.
> > > And yet in turn the question arises whether RISC-V wouldn't
> > > better
> > > have
> > > its own derivation of the machinery, rather than trying to
> > > generalize
> > > things. RISC-V's would then likely be closer to x86'es, just
> > > without
> > > the use of %c on asm() operands. Which might then suggest to
> > > rather
> > > generalize x86'es variant down the road.)
> > ARM version is more portable because of the '%c' modifier which is
> > not
> > present everywhere, so I decided to use it as a generic
> > implementation.
> > Moreover I don't see any reason why we can't switch x86
> > implementation
> > to 'generic/ARM'.
>
> That would increase data size on x86 for no gain, from all I can
> tell.
You are right. It will increase data size.
One more point regarding portability is that x86 uses an 'i' constraint
modifier that GCC won't allow when PIE is enabled, so it doesn't look
like the best option to use as generic.
>
> > > > + ".hword " __stringify(line) ",
> > > > 0\n" \
> > >
> > > Hmm, .hword. How generic is support for that in assemblers? I
> > > notice
> > > even
> > > very old gas has support for it, but architectures may not
> > > consider
> > > it
> > > two bytes wide. (On x86, for example, it's bogus to be two bytes,
> > > since
> > > .word also produces 2 bytes of data. And indeed MIPS and NDS32
> > > override it
> > > in gas to produce 1 byte of data only, at least in certain
> > > cases.)
> > > I'd
> > > like to suggest to use a fourth .long here, and to drop the
> > > padding
> > > field
> > > from struct bug_frame in exchange.
> > Changing .hword to .half can make the code more portable and
> > generic,
> > as .half is a more standard and widely supported assembler
> > directive
> > for declaring 16-bit data.
>
> And how is "half" better than "hword" in the mentioned respect? Half
> a word is still a byte on x86 (due to its 16-bit history).
>
> That said - from all I can tell by briefly looking at gas sources,
> there's no ".half" there.
Then, it will still be an issue. So then the best solution will be to
change it to the type recommended before.
>
> Jan
