Re: [XEN PATCH v2 1/7] xen: add declarations for variables where needed

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 16/10/2023 16:50, Jan Beulich wrote:
> On 09.10.2023 08:54, Nicola Vetrini wrote:
>> Some variables with external linkage used in C code do not have
>> a visible declaration where they are defined. Providing such
>> declaration also resolves violations of MISRA C:2012 Rule 8.4.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> This is a mix of different approaches to the same underlying issue. I
> think respectively splitting would have helped.
> 
>> --- 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).
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.

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

>> --- 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?
> 
> Jan

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.

[1] https://lore.kernel.org/xen-devel/ZRqkbeVUZbjizjNv@MacBookPdeRoger/

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)