From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
xen-devel@lists.xenproject.org,
Oleksii Kurochko <oleksii.kurochko@gmail.com>
Subject: Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
Date: Mon, 13 Feb 2023 13:56:43 +0000 [thread overview]
Message-ID: <1b079086-ffb7-2716-8092-b69ede4aec8c@xen.org> (raw)
In-Reply-To: <e0ab939d-30f0-f9d9-1913-6e63e7023d0a@suse.com>
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/common/bug.c
>>>> @@ -0,0 +1,88 @@
>>>> +#include <xen/bug.h>
>>>> +#include <xen/errno.h>
>>>> +#include <xen/types.h>
>>>> +#include <xen/kernel.h>
>>>
>>> Please order headers alphabetically unless there's anything preventing
>>> that from being done.
>>>
>>>> +#include <xen/string.h>
>>>> +#include <xen/virtual_region.h>
>>>> +
>>>> +#include <asm/processor.h>
>>>> +
>>>> +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
>>>
>>> I know Arm is using vaddr_t and RISC-V now also has it, but in UNIX-like
>>> environments that's redundant with "unsigned long", and it's also
>>> redundant with C99's uintptr_t. Hence when seeing the type I always
>>> wonder whether it's really a host virtual address which is meant (and
>>> not perhaps a guest one, which in principle could differ from the host
>>> one for certain guest types). In any event the existence of this type
>>> should imo not be a prereq to using this generic piece of infrastructure
>>
>> C spec aside, the use "unsigned long" is quite overloaded within Xen.
>> Although, this has improved since we introduced mfn_t/gfn_t.
>>
>> In the future, I could imagine us to also introduce typesafe for
>> vaddr_t, reducing further the risk to mix different meaning of unsigned
>> long.
>>
>> Therefore, I think the introduction of vaddr_t should be a prereq for
>> using the generic piece of infrastructure.
>
> Would be nice if other maintainers could share their thoughts here. I,
> for one, would view a typesafe gaddr_t as reasonable, and perhaps also
> a typesafe gvaddr_t, but hypervisor addresses should be fine as void *
> or unsigned long.
See my answer to Andrew.
>
>>>> --- /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.
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2023-02-13 13:57 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 [this message]
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
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=1b079086-ffb7-2716-8092-b69ede4aec8c@xen.org \
--to=julien@xen.org \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=jbeulich@suse.com \
--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.