From: Julien Grall <julien@xen.org>
To: Andrew Cooper <andrew.cooper3@citrix.com>,
Jan Beulich <jbeulich@suse.com>,
Oleksii Kurochko <oleksii.kurochko@gmail.com>
Cc: George Dunlap <george.dunlap@citrix.com>,
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: Mon, 13 Feb 2023 13:55:12 +0000 [thread overview]
Message-ID: <7e6e7db2-de4e-90aa-bcb3-69327d496c1e@xen.org> (raw)
In-Reply-To: <6ff62711-2826-72b9-5864-a37a56d8b74a@citrix.com>
Hi Andrew,
On 13/02/2023 13:33, Andrew Cooper wrote:
> On 13/02/2023 1:19 pm, Julien Grall wrote:
>>
>>
>> On 13/02/2023 12:24, 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()?
>>>
>>>> --- /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.
>
> I'm with Jan on this. vaddr_t is pure obfuscation, and you can do what
> you like with it in ARM, but you will find very firm objection to it
> finding its way into common or x86 code.
Talking about obfuscation...
42sh> ack "unsigned long addr" | wc -l
143
2/3 of this is on x86. Without looking at the code, it can be quite
difficult to figure out if this is meant to a virtual/physical
host/guest address.
>
> virtual addresses are spelt 'unsigned long'.
That's ok so long there are no way to mistakenly mix some parameters.
For instance in the past, the type of map_pages_to_xen() was:
int map_pages_to_xen(unsigned long virt,
unsigned long mfn,
unsigned long nr_mfns,
unsigned int flags)
Since then 'mfn' is now thankfully mfn_t... But before, it was easy to mix.
For sure, there are other way to do it like properly naming the
variable... But using a type as the advantage that it could be checked a
compilation.
I am open to other suggestions if you have a way to guarantee that
mistake can be avoided.
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2023-02-13 13: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 [this message]
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=7e6e7db2-de4e-90aa-bcb3-69327d496c1e@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.