Re: [PATCH v2 3/7] xen/common: Move Arm's bootfdt to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.12.2023 23:08, Julien Grall wrote:
> Hi,
> 
> On 20/12/2023 20:58, Shawn Anastasio wrote:
>> On 12/20/23 2:09 AM, Jan Beulich wrote:
>>> On 19.12.2023 19:29, Julien Grall wrote:
>>>> On 19/12/2023 17:03, Jan Beulich wrote:
>>>>> On 15.12.2023 03:43, Shawn Anastasio wrote:
>>>>>> --- a/xen/arch/arm/bootfdt.c
>>>>>> +++ b/xen/common/device-tree/bootfdt.c
>>>>>> @@ -431,12 +431,15 @@ static int __init early_scan_node(const void *fdt,
>>>>>>    {
>>>>>>        int rc = 0;
>>>>>>    
>>>>>> -    /*
>>>>>> -     * If Xen has been booted via UEFI, the memory banks are
>>>>>> -     * populated. So we should skip the parsing.
>>>>>> -     */
>>>>>> -    if ( !efi_enabled(EFI_BOOT) &&
>>>>>> -         device_tree_node_matches(fdt, node, "memory") )
>>>>>> +    if ( device_tree_node_matches(fdt, node, "memory") )
>>>>>> +#if defined(CONFIG_ARM_EFI)
>>>>>> +        /*
>>>>>> +         * If Xen has been booted via UEFI, the memory banks are
>>>>>> +         * populated. So we should skip the parsing.
>>>>>> +         */
>>>>>> +        if ( efi_enabled(EFI_BOOT) )
>>>>>> +            return rc;
>>>>>> +#endif
>>>>>
>>>>> I'm not a DT maintainer, but I don't like this kind of #ifdef, the more
>>>>> that maybe PPC and quite likely RISC-V are likely to also want to support
>>>>> EFI boot. But of course there may be something inherently Arm-specific
>>>>> here that I'm unaware of.
>>>>
>>>> Right now, I can't think how this is Arm specific. If you are using
>>>> UEFI, then you are expected to use the UEFI memory map rather than the
>>>> content of the device-tree.
>>>>
>>>> However, we don't have a CONFIG_EFI option. It would be nice to
>>>> introduce one but I am not sure I would introduce it just for this #ifdef.
>>>
>>> Right, hence why I also wasn't suggesting to go that route right away.
>>> efi/common-stub.c already has a stub for efi_enabled(). Using that file
>>> may be too involved to arrange for in PPC, but supplying such a stub
>>> elsewhere for the time being looks like it wouldn't too much effort
>>> (and would eliminate the need for any #ifdef here afaict). Shawn?
>>>
>>
>> To clarify, you're suggesting we add an efi_enabled stub somewhere in
>> arch/ppc? I'm not against that, though it does seem a little silly to
>> have to define EFI-specific functions on an architecture that will never
>> support EFI.
> 
> (This is not an argument for adding efi_enabled in arch/ppc)
> 
> I am curious to know why you think that. This is just software and 
> therefore doesn't seem to be technically impossible. I mean who 
> originally thought that ACPI would come to Arm? :) And yet we now have 
> HWs (mainly servers) which provides only ACPI + UEFI.
> 
> And before, I got asked where is the support in Xen. Yes, the work is 
> still on-going :).
> 
> Anyway, back to the original ask, one option would be to introduce 
> efi_enabled stub in an common header. Maybe xen/efi.h?

Right, and having a somewhat odd #ifdef there (covering for the lack of
CONFIG_EFI) would imo be preferable to having it in a random .c file.

Jan