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

[Jan Beulich <jbeulich@xxxxxxxx>]

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