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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

>> 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.
> 
> Everything you seem to suggest are clean ups. So I think it would be 
> better if they are first applied to Arm and then we move the code to 
> common afterwards.
> 
> This will make easier to confirm what changed and also tracking the 
> history (think git blame).
> 
> That said, I don't view the clean-ups as necessary in order to move the 
> code in common... They could be done afterwards by Oleksii or someone else.

I disagree: The wider the exposure / use of code, the more important it is
to be reasonably tidy. Cleaning up first and then moving is certainly fine
with me.

>>> +         ".pushsection .rodata.str, \"aMS\", %progbits, 1\n"               
>>>  \
>>> +         "2:\t.asciz " __stringify(file) "\n"                              
>>>  \
>>
>> file is always a string literal; really it always is __FILE__ in this
>> non-x86 implementation. So first preference ought to be to drop the
>> macro parameter and use __FILE__ here (same then for line vs __LINE__).
>> Yet even if not done like that, the __stringify() is largely unneeded
>> (unless we expect file names to have e.g. backslashes in their names)
>> and looks somewhat odd here. So how about
>>
>>           "2:\t.asciz \"" __FILE__ "\"\n"
>>
>> ? But wait - peeking ahead to the x86 patch I notice that __FILE__ and
>> __LINE__ need to remain arguments. But then the second preference would
>> still be
>>
>>           "2:\t.asciz \"" file "\"\n"
>>
>> imo. Yet maybe others disagree ...
> 
> I would prefer to keep the __stringify() version because it avoids 
> relying on file to always be a string literal.

... hiding possible mistakes (not passing a string literal is very
likely unintentional here after all).

Jan