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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

virtual addresses are spelt 'unsigned long'.

~Andrew