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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

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.

Stil, if you think it's preferable to adding the ifdef here then I'm not
against it.

> Jan

Thanks,
Shawn