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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Oct 03, 2023 at 09:05:34AM +0200, Nicola Vetrini wrote:
> On 02/10/2023 13:07, Roger Pau Monné wrote:
> > On Mon, Oct 02, 2023 at 09:49:44AM +0200, 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>
> > > ---
> > >  xen/arch/arm/include/asm/setup.h  |  3 +++
> > >  xen/arch/arm/include/asm/smp.h    |  3 +++
> > >  xen/arch/x86/cpu/mcheck/mce.c     |  6 +++---
> > >  xen/arch/x86/include/asm/setup.h  |  3 +++
> > >  xen/arch/x86/irq.c                |  2 +-
> > >  xen/arch/x86/platform_hypercall.c |  3 ---
> > >  xen/common/symbols.c              | 17 -----------------
> > >  xen/include/xen/hypercall.h       |  3 +++
> > >  xen/include/xen/symbols.h         | 18 ++++++++++++++++++
> > >  9 files changed, 34 insertions(+), 24 deletions(-)
> > > 
> 
> > > diff --git a/xen/arch/x86/include/asm/setup.h
> > > b/xen/arch/x86/include/asm/setup.h
> > > index dfdd9e555149..3c27fe915ed4 100644
> > > --- a/xen/arch/x86/include/asm/setup.h
> > > +++ b/xen/arch/x86/include/asm/setup.h
> > > @@ -13,8 +13,11 @@ 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];
> > >  extern void *stack_start;
> > > 
> > > +extern unsigned long cr4_pv32_mask;
> > 
> > This one might better go in compat.h, albeit that would require it's
> > setting to be gated to CONFIG_PV32.  setup.h is IMO for init time
> > stuff.
> > 
> 
> Ok
> 
> > > +
> > >  void early_cpu_init(void);
> > >  void early_time_init(void);
> > > 
> > > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> > > index 6abfd8162120..604dba94b052 100644
> > > --- a/xen/arch/x86/irq.c
> > > +++ b/xen/arch/x86/irq.c
> > > @@ -43,7 +43,7 @@ int __read_mostly opt_irq_vector_map =
> > > OPT_IRQ_VECTOR_MAP_DEFAULT;
> > >  static unsigned char __read_mostly irq_max_guests;
> > >  integer_param("irq-max-guests", irq_max_guests);
> > > 
> > > -vmask_t global_used_vector_map;
> > > +static vmask_t global_used_vector_map;
> > > 
> > >  struct irq_desc __read_mostly *irq_desc = NULL;
> > > 
> > > diff --git a/xen/arch/x86/platform_hypercall.c
> > > b/xen/arch/x86/platform_hypercall.c
> > > index 9469de9045c7..e4dbec73d784 100644
> > > --- a/xen/arch/x86/platform_hypercall.c
> > > +++ b/xen/arch/x86/platform_hypercall.c
> > > @@ -36,9 +36,6 @@
> > >  #include "cpu/mtrr/mtrr.h"
> > >  #include <xsm/xsm.h>
> > > 
> > > -/* Declarations for items shared with the compat mode handler. */
> > > -extern spinlock_t xenpf_lock;
> > > -
> > >  #define RESOURCE_ACCESS_MAX_ENTRIES 3
> > >  struct resource_access {
> > >      unsigned int nr_done;
> > > diff --git a/xen/common/symbols.c b/xen/common/symbols.c
> > > index 691e61792506..7c3514c65f2e 100644
> > > --- 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)
> > > diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
> > > index f307dfb59760..12de5a69b5b1 100644
> > > --- a/xen/include/xen/hypercall.h
> > > +++ b/xen/include/xen/hypercall.h
> > > @@ -24,6 +24,9 @@
> > >  /* Needs to be after asm/hypercall.h. */
> > >  #include <xen/hypercall-defs.h>
> > > 
> > > +/* Declarations for items shared with the compat mode handler. */
> > > +extern spinlock_t xenpf_lock;
> > 
> > I'm confused about why this needs to be moved, AFAICT xenpf_lock is
> > only used in platform_hypercall.c, and the declaration is
> > unconditional, so there's no definition without declaration issue.
> > 
> 
> The violation was on ARM code, because of a slight inconsistency:
> xen/arch/arm/platform_hypercall.c:DEFINE_SPINLOCK(xenpf_lock);
> 
> xen/arch/x86/platform_hypercall.c:extern spinlock_t xenpf_lock;
> xen/arch/x86/platform_hypercall.c:DEFINE_SPINLOCK(xenpf_lock);
> 
> therefore, by moving the extern declaration to the common header there
> should be no issue.

Just make xenpf_lock static on arm, that would be a better solution.
The only need for the lock to be global is because of compat code, and
arm doesn't seem to need it.

Thanks, Roger.