From: Jan Beulich <jbeulich@suse.com>
To: Oleksii <oleksii.kurochko@gmail.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
Julien Grall <julien@xen.org>,
Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
Date: Tue, 14 Feb 2023 17:55:57 +0100 [thread overview]
Message-ID: <9cf03b23-586b-92e1-c0b1-578f3eadd2ad@suse.com> (raw)
In-Reply-To: <81fd6cf5ff59acf6ca8b66e093630e5accc45198.camel@gmail.com>
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
next prev parent reply other threads:[~2023-02-14 16:56 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-03 17:05 [PATCH v1 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-02-03 17:05 ` [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-02-13 12:24 ` Jan Beulich
2023-02-13 13:19 ` Julien Grall
2023-02-13 13:30 ` Jan Beulich
2023-02-13 13:56 ` Julien Grall
2023-02-13 15:02 ` Jan Beulich
2023-02-13 15:07 ` Julien Grall
2023-02-13 15:14 ` Jan Beulich
2023-02-13 13:33 ` Andrew Cooper
2023-02-13 13:55 ` Julien Grall
2023-02-14 16:22 ` Oleksii
2023-02-14 16:55 ` Jan Beulich [this message]
2023-02-15 15:06 ` Oleksii
2023-02-15 17:59 ` Oleksii
2023-02-16 7:31 ` Jan Beulich
2023-02-16 10:44 ` Oleksii
2023-02-16 12:09 ` Oleksii
2023-02-16 12:19 ` Andrew Cooper
2023-02-16 20:44 ` Oleksii
2023-02-17 7:12 ` Jan Beulich
2023-02-17 9:33 ` Oleksii
2023-02-16 14:55 ` Jan Beulich
2023-02-16 10:48 ` Andrew Cooper
2023-02-16 12:21 ` Oleksii
2023-02-16 14:53 ` Jan Beulich
2023-02-03 17:05 ` [PATCH v1 2/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-02-13 13:02 ` Jan Beulich
2023-02-14 16:28 ` Oleksii
2023-02-03 17:05 ` [PATCH v1 3/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
2023-02-13 13:10 ` Jan Beulich
2023-02-14 16:41 ` Oleksii
2023-02-03 17:05 ` [PATCH v1 4/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
2023-02-13 13:13 ` Jan Beulich
2023-02-14 16:44 ` Oleksii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9cf03b23-586b-92e1-c0b1-578f3eadd2ad@suse.com \
--to=jbeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=julien@xen.org \
--cc=oleksii.kurochko@gmail.com \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.