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

[Julien Grall <julien@xxxxxxx>]

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