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
