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

From: Jan Beulich <jbeulich@suse.com>
To: Julien Grall <julien@xen.org>,
	Shawn Anastasio <sanastasio@raptorengineering.com>
Cc: Timothy Pearson <tpearson@raptorengineering.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 3/7] xen/common: Move Arm's bootfdt to common
Date: Wed, 20 Dec 2023 09:09:51 +0100	[thread overview]
Message-ID: <69c5afde-3a24-4792-b4e5-3ce17cb1602d@suse.com> (raw)
In-Reply-To: <5fc9ad7a-1281-46da-abff-0aa2d2beca14@xen.org>

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