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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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

~Andrew