| Message ID | b26a07209b54cd036e42a8b00f036201821eb775.1702607884.git.sanastasio@raptorengineering.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Early Boot Allocation on Power | expand |
On 15.12.2023 03:43, Shawn Anastasio wrote: > Move Arm's bootfdt.c to xen/common so that it can be used by other > device tree architectures like PPC and RISCV. Only a minor change to > conditionalize a call to a function only available on EFI-supporting > targets was made to the code itself. > > Suggested-by: Julien Grall <julien@xen.org> > Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com> > --- > xen/arch/arm/Makefile | 1 - > xen/common/Makefile | 1 + > xen/common/device-tree/Makefile | 1 + > xen/{arch/arm => common/device-tree}/bootfdt.c | 15 +++++++++------ > 4 files changed, 11 insertions(+), 7 deletions(-) > create mode 100644 xen/common/device-tree/Makefile > rename xen/{arch/arm => common/device-tree}/bootfdt.c (98%) I think this wants to come with an update to ./MAINTAINERS, such that the file doesn't change maintainership. > --- 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. Jan
Hi Jan, On 19/12/2023 17:03, Jan Beulich wrote: > On 15.12.2023 03:43, Shawn Anastasio wrote: >> Move Arm's bootfdt.c to xen/common so that it can be used by other >> device tree architectures like PPC and RISCV. Only a minor change to >> conditionalize a call to a function only available on EFI-supporting >> targets was made to the code itself. >> >> Suggested-by: Julien Grall <julien@xen.org> >> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com> >> --- >> xen/arch/arm/Makefile | 1 - >> xen/common/Makefile | 1 + >> xen/common/device-tree/Makefile | 1 + >> xen/{arch/arm => common/device-tree}/bootfdt.c | 15 +++++++++------ >> 4 files changed, 11 insertions(+), 7 deletions(-) >> create mode 100644 xen/common/device-tree/Makefile >> rename xen/{arch/arm => common/device-tree}/bootfdt.c (98%) > > I think this wants to come with an update to ./MAINTAINERS, such that > the file doesn't change maintainership. > >> --- 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. Cheers,
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
Hi Shawn, On 15/12/2023 02:43, Shawn Anastasio wrote: > Move Arm's bootfdt.c to xen/common so that it can be used by other > device tree architectures like PPC and RISCV. Only a minor change to > conditionalize a call to a function only available on EFI-supporting > targets was made to the code itself. > > Suggested-by: Julien Grall <julien@xen.org> > Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com> With the MAINTAINERS file updated (I would add under DEVICE TREE) and one note below: Acked-by: Julien Grall <jgrall@amazon.com> > --- 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) As discussed in a separate subthread, I would be ok with any of the options proposed. So feel free ot keep my Acked-by once this is settled. Cheers,
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
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,
----- Original Message ----- > From: "Julien Grall" <julien@xen.org> > To: "Shawn Anastasio" <sanastasio@raptorengineering.com>, "Jan Beulich" <jbeulich@suse.com> > Cc: "Timothy Pearson" <tpearson@raptorengineering.com>, "xen-devel" <xen-devel@lists.xenproject.org> > 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!
On 20.12.2023 23:08, Julien Grall wrote: > 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? Right, and having a somewhat odd #ifdef there (covering for the lack of CONFIG_EFI) would imo be preferable to having it in a random .c file. Jan
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 33c677672f..64fdf84170 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -10,7 +10,6 @@ obj-$(CONFIG_TEE) += tee/ obj-$(CONFIG_HAS_VPCI) += vpci.o obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o -obj-y += bootfdt.init.o obj-y += cpuerrata.o obj-y += cpufeature.o obj-y += decode.o diff --git a/xen/common/Makefile b/xen/common/Makefile index 69d6aa626c..6e175626d5 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -77,6 +77,7 @@ obj-$(CONFIG_UBSAN) += ubsan/ obj-$(CONFIG_NEEDS_LIBELF) += libelf/ obj-$(CONFIG_HAS_DEVICE_TREE) += libfdt/ +obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/ CONF_FILE := $(if $(patsubst /%,,$(KCONFIG_CONFIG)),$(objtree)/)$(KCONFIG_CONFIG) $(obj)/config.gz: $(CONF_FILE) diff --git a/xen/common/device-tree/Makefile b/xen/common/device-tree/Makefile new file mode 100644 index 0000000000..66c2500df9 --- /dev/null +++ b/xen/common/device-tree/Makefile @@ -0,0 +1 @@ +obj-y += bootfdt.init.o diff --git a/xen/arch/arm/bootfdt.c b/xen/common/device-tree/bootfdt.c similarity index 98% rename from xen/arch/arm/bootfdt.c rename to xen/common/device-tree/bootfdt.c index f496a8cf94..ae9fa1e3d6 100644 --- 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 rc = process_memory_node(fdt, node, name, depth, address_cells, size_cells, &bootinfo.mem); else if ( depth == 1 && !dt_node_cmp(name, "reserved-memory") )
Move Arm's bootfdt.c to xen/common so that it can be used by other device tree architectures like PPC and RISCV. Only a minor change to conditionalize a call to a function only available on EFI-supporting targets was made to the code itself. Suggested-by: Julien Grall <julien@xen.org> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com> --- xen/arch/arm/Makefile | 1 - xen/common/Makefile | 1 + xen/common/device-tree/Makefile | 1 + xen/{arch/arm => common/device-tree}/bootfdt.c | 15 +++++++++------ 4 files changed, 11 insertions(+), 7 deletions(-) create mode 100644 xen/common/device-tree/Makefile rename xen/{arch/arm => common/device-tree}/bootfdt.c (98%)