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

[Julien Grall <julien@xxxxxxx>]

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