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

[Julien Grall <julien@xxxxxxx>]

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?
But I would also be ok with the existing #ifdef for now.

Cheers,

--
Julien Grall