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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan