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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> --- /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.

>>> +};
>>> +
>>> +#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.

>>> +         ".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.

Jan