Re: [PATCH v12 7/8] xen/riscv: enable full Xen build

["Oleksii K." <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2024-05-30 at 18:45 +0100, Andrew Cooper wrote:
> On 30/05/2024 6:12 pm, Oleksii K. wrote:
> > On Thu, 2024-05-30 at 17:48 +0100, Andrew Cooper wrote:
> > > On 29/05/2024 8:55 pm, Oleksii Kurochko wrote:
> > > > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> > > > index 8285bcffef..bda35fc347 100644
> > > > --- a/xen/arch/riscv/stubs.c
> > > > +++ b/xen/arch/riscv/stubs.c
> > > > @@ -24,12 +24,6 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t,
> > > > cpu_core_mask);
> > > >  
> > > >  nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
> > > >  
> > > > -/*
> > > > - * max_page is defined in page_alloc.c which isn't complied
> > > > for
> > > > now.
> > > > - * definition of max_page will be remove as soon as page_alloc
> > > > is
> > > > built.
> > > > - */
> > > > -unsigned long __read_mostly max_page;
> > > > -
> > > >  /* time.c */
> > > >  
> > > >  unsigned long __ro_after_init cpu_khz;  /* CPU clock frequency
> > > > in
> > > > kHz. */
> > > > @@ -419,21 +413,3 @@ void __cpu_die(unsigned int cpu)
> > > >  {
> > > >      BUG_ON("unimplemented");
> > > >  }
> > > > -
> > > > -/*
> > > > - * The following functions are defined in common/irq.c, but
> > > > common/irq.c isn't
> > > > - * built for now. These changes will be removed there when
> > > > common/irq.c is
> > > > - * ready.
> > > > - */
> > > > -
> > > > -void cf_check irq_actor_none(struct irq_desc *desc)
> > > > -{
> > > > -    BUG_ON("unimplemented");
> > > > -}
> > > > -
> > > > -unsigned int cf_check irq_startup_none(struct irq_desc *desc)
> > > > -{
> > > > -    BUG_ON("unimplemented");
> > > > -
> > > > -    return 0;
> > > > -}
> > > All 3 of these are introduced in the previous patch and deleted
> > > again
> > > here.  Looks like a rebasing accident.
> > Not really.
> > 
> > This was done to avoid build failures for RISC-V. If the HEAD is on
> > the
> > previous patch where these changes are introduced and then we just
> > drop
> > them, it will lead to a linkage error because these functions are
> > defined in common/irq.c, which isn't built for RISC-V if the HEAD
> > is on
> > the previous patch:
> >    /build/xen/arch/riscv/entry.S:86: undefined reference to
> > `max_page'
> 
> Nothing in the series touches entry.S, so I'm not sure what this is
> complaining about.
> 
> The stub for get_upper_mfn_bound() references max_page, but it's only
> used in a SYSCTL so you can avoid the problem with a BUG_ON().
I didn't get how I can use BUG_ON() with declaration of variable to
avoid the compilation issue the undefined reference to max_page?

> 
> BTW, you also don't need a return after a BUG_ON(). 
> __builtin_unreachable() takes care of everything properly for you.
> 
> 
> >    riscv64-linux-gnu-ld: prelink.o:(.rodata+0x8): undefined
> > reference to
> >    `irq_startup_none'
> >    riscv64-linux-gnu-ld: prelink.o:(.rodata+0x10): undefined
> > reference to
> >    `irq_actor_none'
> >    riscv64-linux-gnu-ld: prelink.o:(.rodata+0x18): undefined
> > reference to
> >    `irq_actor_none'
> >    riscv64-linux-gnu-ld: prelink.o:(.rodata+0x20): undefined
> > reference to
> >    `irq_actor_none'
> >    riscv64-linux-gnu-ld: xen-syms: hidden symbol `irq_actor_none'
> > isn't
> >    defined
> > 
> > That is why these stubs were introduced in the previous patch
> > (because
> > common/irq.c isn't built at that moment) and are removed in this
> > patch
> > (since at the moment of this patch, common/irq.c is now being
> > built).
> 
> These OTOH are a side effect of how no_irq_type works, which is
> horrifying, and not something I can unsee.
> 
> I'll see about fixing it, because I really can't bare to leave it
> like
> this...
I am really sorry, but I don't understand should I deal with this
somehow now or the provided changes are okay?

~ Oleksii