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

[Timothy Pearson <tpearson@xxxxxxxxxxxxxxxxxxxxx>]

----- Original Message -----
> From: "Julien Grall" <julien@xxxxxxx>
> To: "Shawn Anastasio" <sanastasio@xxxxxxxxxxxxxxxxxxxxx>, "Jan Beulich" 
> <jbeulich@xxxxxxxx>
> Cc: "Timothy Pearson" <tpearson@xxxxxxxxxxxxxxxxxxxxx>, "xen-devel" 
> <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Sent: Wednesday, December 20, 2023 4:08:30 PM
> Subject: Re: [PATCH v2 3/7] xen/common: Move Arm's bootfdt to common

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

It's not a technical restriction, it's an architecture specifiction and 
compatibility (standardization) restriction.  POWER has its own interfaces 
(OPAL etc.) that provide the functionality ACPI provides on x86/ARM, and 
fragmenting the ecosystem across two interface standards is not something that 
any of the key stakeholders are currently willing to do.

Just some background, I have no comment on the actual technical implementation 
in the patch. :)

Thanks!