On 17/10/2023 08:46, Jan Beulich wrote:
On 16.10.2023 19:05, Nicola Vetrini wrote:
On 16/10/2023 16:50, Jan Beulich wrote:
On 09.10.2023 08:54, Nicola Vetrini wrote:
--- a/xen/arch/x86/include/asm/compat.h
+++ b/xen/arch/x86/include/asm/compat.h
@@ -13,6 +13,7 @@ typedef unsigned long full_ptr_t;
struct domain;
#ifdef CONFIG_PV32
+extern unsigned long cr4_pv32_mask;
Why is this needed? Didn't we say declarations aren't needed when the
only consumer is assembly code? (I also wonder how this header is any
more "appropriate" - see the changelog entry - than about any other
one.)
It was pointed out to me [1] that compat.h might be more appropriate
than setup.h
(probably because the asm code referencing it is under x86_64/compat).
Hmm. I agree setup.h isn't appropriate.
Further, while it's true that this variable is used in asm, it's also
used in x86/setup.c, hence
the need for a declaration.
But that's the file where the variable is defined. IOW no risk of
definition and (non-existing) declaration going out of sync.
This is an aspect specific to this variable, that unfortunately the rule
does not
capture. I'll deviate this in the next version of this series.
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -13,6 +13,7 @@ extern char __2M_rwdata_start[],
__2M_rwdata_end[];
extern unsigned long xenheap_initial_phys_start;
extern uint64_t boot_tsc_stamp;
+extern char cpu0_stack[STACK_SIZE];
Same question here.
This one is a bit more nuanced (I wouldn't oppose deviating this), but
there is indeed one use.
Still same here then.
Same as above; it can be argued that there's no risk of anything going
out of sync.
--- a/xen/common/symbols.c
+++ b/xen/common/symbols.c
@@ -21,23 +21,6 @@
#include <xen/guest_access.h>
#include <xen/errno.h>
-#ifdef SYMBOLS_ORIGIN
-extern const unsigned int symbols_offsets[];
-#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
-#else
-extern const unsigned long symbols_addresses[];
-#define symbols_address(n) symbols_addresses[n]
-#endif
-extern const unsigned int symbols_num_syms;
-extern const u8 symbols_names[];
-
-extern const struct symbol_offset symbols_sorted_offsets[];
-
-extern const u8 symbols_token_table[];
-extern const u16 symbols_token_index[];
-
-extern const unsigned int symbols_markers[];
-
/* expand a compressed symbol data into the resulting uncompressed
string,
given the offset to where the symbol is in the compressed stream
*/
static unsigned int symbols_expand_symbol(unsigned int off, char
*result)
--- a/xen/include/xen/symbols.h
+++ b/xen/include/xen/symbols.h
@@ -33,4 +33,22 @@ struct symbol_offset {
uint32_t stream; /* .. in the compressed stream.*/
uint32_t addr; /* .. and in the fixed size address array. */
};
+
+#ifdef SYMBOLS_ORIGIN
+extern const unsigned int symbols_offsets[];
+#define symbols_address(n) (SYMBOLS_ORIGIN + symbols_offsets[n])
+#else
+extern const unsigned long symbols_addresses[];
+#define symbols_address(n) symbols_addresses[n]
+#endif
+extern const unsigned int symbols_num_syms;
+extern const u8 symbols_names[];
+
+extern const struct symbol_offset symbols_sorted_offsets[];
+
+extern const u8 symbols_token_table[];
+extern const u16 symbols_token_index[];
+
+extern const unsigned int symbols_markers[];
+
#endif /*_XEN_SYMBOLS_H*/
This change is even less clear to me: The producer is assembly code,
and the consumer already had appropriate declarations. Why would we
want to increase the scope of their visibility?
The missing decls are about common/symbols-dummy.c. Xen can choose
that
this file does
not need to conform (to this guideline or any guideline), in which
case
this change can be dropped.
Since symbols-dummy.c isn't used in the final binary, I'd prefer that.
Otherwise imo a new private header used by just the two files would
want
introducing, to keep exposure limited.
Jan
Ok
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
