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. >>> --- 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. >>> --- 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