[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |