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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/05/2024 7:27 pm, Oleksii K. wrote:
> 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?

You don't need to implement get_upper_mfn_bound() yet.

If you implement it as:

unsigned long get_upper_mfn_bound(void)
{
    BUG_ON("unimplemented");
}

in the previous patch, then you also don't have any problems with
max_page between these two patches either.

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

I've emailed out a 2-patch series to unbreak this.

~Andrew