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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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)