| Message ID | 20220630100324.3153655-26-nikos.nikoleris@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EFI and ACPI support for arm64 | expand |
Hi, On Thu, Jun 30, 2022 at 11:03:22AM +0100, Nikos Nikoleris wrote: > Users can now build kvm-unit-tests as efi apps by supplying an extra > argument when invoking configure: > > $> ./configure --enable-efi > > This patch is based on an earlier version by > Andrew Jones <drjones@redhat.com> > > Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com> > Reviewed-by: Ricardo Koller <ricarkol@google.com> > --- > configure | 15 ++++++++++++--- > arm/Makefile.arm | 6 ++++++ > arm/Makefile.arm64 | 18 ++++++++++++++---- > arm/Makefile.common | 45 ++++++++++++++++++++++++++++++++++----------- > 4 files changed, 66 insertions(+), 18 deletions(-) > > diff --git a/configure b/configure > index 5b7daac..2ff9881 100755 > --- a/configure > +++ b/configure [..] > @@ -218,6 +223,10 @@ else > echo "arm64 doesn't support page size of $page_size" > usage > fi > + if [ "$efi" = 'y' ] && [ "$page_size" != "4096" ]; then > + echo "efi must use 4K pages" > + exit 1 Why this restriction? The Makefile compiles kvm-unit-tests to run as an UEFI app, it doesn't compile UEFI itself. As far as I can tell, UEFI is designed to run payloads with larger page size (it would be pretty silly to not be able to boot a kernel built for 16k or 64k pages with UEFI). Is there some limitation that I'm missing? Thanks, Alex
Hi Alex, On 12/07/2022 14:39, Alexandru Elisei wrote: > Hi, > > On Thu, Jun 30, 2022 at 11:03:22AM +0100, Nikos Nikoleris wrote: >> Users can now build kvm-unit-tests as efi apps by supplying an extra >> argument when invoking configure: >> >> $> ./configure --enable-efi >> >> This patch is based on an earlier version by >> Andrew Jones <drjones@redhat.com> >> >> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com> >> Reviewed-by: Ricardo Koller <ricarkol@google.com> >> --- >> configure | 15 ++++++++++++--- >> arm/Makefile.arm | 6 ++++++ >> arm/Makefile.arm64 | 18 ++++++++++++++---- >> arm/Makefile.common | 45 ++++++++++++++++++++++++++++++++++----------- >> 4 files changed, 66 insertions(+), 18 deletions(-) >> >> diff --git a/configure b/configure >> index 5b7daac..2ff9881 100755 >> --- a/configure >> +++ b/configure > [..] >> @@ -218,6 +223,10 @@ else >> echo "arm64 doesn't support page size of $page_size" >> usage >> fi >> + if [ "$efi" = 'y' ] && [ "$page_size" != "4096" ]; then >> + echo "efi must use 4K pages" >> + exit 1 > > Why this restriction? > > The Makefile compiles kvm-unit-tests to run as an UEFI app, it doesn't > compile UEFI itself. As far as I can tell, UEFI is designed to run payloads > with larger page size (it would be pretty silly to not be able to boot a > kernel built for 16k or 64k pages with UEFI). > > Is there some limitation that I'm missing? > Technically, we could allow 16k or 64k granules. But to do that we would have to handle cases where the memory map we get from EFI cannot be remapped with the new granules. For example, a region might be 12kB and mapping it with 16k or 64k granules without moving it is impossible. Thanks, Nikos > Thanks, > Alex
Hi, On Tue, Jul 12, 2022 at 09:50:51PM +0100, Nikos Nikoleris wrote: > Hi Alex, > > On 12/07/2022 14:39, Alexandru Elisei wrote: > > Hi, > > > > On Thu, Jun 30, 2022 at 11:03:22AM +0100, Nikos Nikoleris wrote: > > > Users can now build kvm-unit-tests as efi apps by supplying an extra > > > argument when invoking configure: > > > > > > $> ./configure --enable-efi > > > > > > This patch is based on an earlier version by > > > Andrew Jones <drjones@redhat.com> > > > > > > Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com> > > > Reviewed-by: Ricardo Koller <ricarkol@google.com> > > > --- > > > configure | 15 ++++++++++++--- > > > arm/Makefile.arm | 6 ++++++ > > > arm/Makefile.arm64 | 18 ++++++++++++++---- > > > arm/Makefile.common | 45 ++++++++++++++++++++++++++++++++++----------- > > > 4 files changed, 66 insertions(+), 18 deletions(-) > > > > > > diff --git a/configure b/configure > > > index 5b7daac..2ff9881 100755 > > > --- a/configure > > > +++ b/configure > > [..] > > > @@ -218,6 +223,10 @@ else > > > echo "arm64 doesn't support page size of $page_size" > > > usage > > > fi > > > + if [ "$efi" = 'y' ] && [ "$page_size" != "4096" ]; then > > > + echo "efi must use 4K pages" > > > + exit 1 > > > > Why this restriction? > > > > The Makefile compiles kvm-unit-tests to run as an UEFI app, it doesn't > > compile UEFI itself. As far as I can tell, UEFI is designed to run payloads > > with larger page size (it would be pretty silly to not be able to boot a > > kernel built for 16k or 64k pages with UEFI). > > > > Is there some limitation that I'm missing? > > > > Technically, we could allow 16k or 64k granules. But to do that we would > have to handle cases where the memory map we get from EFI cannot be remapped > with the new granules. For example, a region might be 12kB and mapping it > with 16k or 64k granules without moving it is impossible. Hm... From UEFI Specification, Version 2.8, page 35: "The ARM architecture allows mapping pages at a variety of granularities, including 4KiB and 64KiB. If a 64KiB physical page contains any 4KiB page with any of the following types listed below, then all 4KiB pages in the 64KiB page must use identical ARM Memory Page Attributes (as described in Table 7): — EfiRuntimeServicesCode — EfiRuntimeServicesData — EfiReserved — EfiACPIMemoryNVS Mixed attribute mappings within a larger page are not allowed. Note: This constraint allows a 64K paged based Operating System to safely map runtime services memory." Looking at Table 30. Memory Type Usage after ExitBootServices(), on page 160 (I am going to assume that EfiReservedMemoryType is the same as EfiReserved), the only region that is required to be mapped for runtime services, but isn't mentioned above, is EfiPalCode. The bit about mixed attribute mappings within a larger page not being allowed makes me think that EfiPalCode can be mapped even if isn't mapped at the start of a 64KiB page, as no other memory type can be withing a 64KiB granule. What do you think? There's no pressing need to have support for all page sizes, from my point of view, it's fine if it's missing from the initial UEFI support. But I would appreciate a comment in the code or an explanation in the commit message (or both), because it looks very arbitrary as it is right now. At the very least this will serve as a nice reminder of what still needs to be done for full UEFI support. Thanks, Alex > > Thanks, > > Nikos > > > Thanks, > > Alex
On 13/07/2022 09:46, Alexandru Elisei wrote: > Hi, > > On Tue, Jul 12, 2022 at 09:50:51PM +0100, Nikos Nikoleris wrote: >> Hi Alex, >> >> On 12/07/2022 14:39, Alexandru Elisei wrote: >>> Hi, >>> >>> On Thu, Jun 30, 2022 at 11:03:22AM +0100, Nikos Nikoleris wrote: >>>> Users can now build kvm-unit-tests as efi apps by supplying an extra >>>> argument when invoking configure: >>>> >>>> $> ./configure --enable-efi >>>> >>>> This patch is based on an earlier version by >>>> Andrew Jones <drjones@redhat.com> >>>> >>>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com> >>>> Reviewed-by: Ricardo Koller <ricarkol@google.com> >>>> --- >>>> configure | 15 ++++++++++++--- >>>> arm/Makefile.arm | 6 ++++++ >>>> arm/Makefile.arm64 | 18 ++++++++++++++---- >>>> arm/Makefile.common | 45 ++++++++++++++++++++++++++++++++++----------- >>>> 4 files changed, 66 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/configure b/configure >>>> index 5b7daac..2ff9881 100755 >>>> --- a/configure >>>> +++ b/configure >>> [..] >>>> @@ -218,6 +223,10 @@ else >>>> echo "arm64 doesn't support page size of $page_size" >>>> usage >>>> fi >>>> + if [ "$efi" = 'y' ] && [ "$page_size" != "4096" ]; then >>>> + echo "efi must use 4K pages" >>>> + exit 1 >>> >>> Why this restriction? >>> >>> The Makefile compiles kvm-unit-tests to run as an UEFI app, it doesn't >>> compile UEFI itself. As far as I can tell, UEFI is designed to run payloads >>> with larger page size (it would be pretty silly to not be able to boot a >>> kernel built for 16k or 64k pages with UEFI). >>> >>> Is there some limitation that I'm missing? >>> >> >> Technically, we could allow 16k or 64k granules. But to do that we would >> have to handle cases where the memory map we get from EFI cannot be remapped >> with the new granules. For example, a region might be 12kB and mapping it >> with 16k or 64k granules without moving it is impossible. > > Hm... From UEFI Specification, Version 2.8, page 35: > > "The ARM architecture allows mapping pages at a variety of granularities, > including 4KiB and 64KiB. If a 64KiB physical page contains any 4KiB page > with any of the following types listed below, then all 4KiB pages in the > 64KiB page must use identical ARM Memory Page Attributes (as described in > Table 7): > > — EfiRuntimeServicesCode > — EfiRuntimeServicesData > — EfiReserved > — EfiACPIMemoryNVS > > Mixed attribute mappings within a larger page are not allowed. > > Note: This constraint allows a 64K paged based Operating System to safely > map runtime services memory." > > Looking at Table 30. Memory Type Usage after ExitBootServices(), on page > 160 (I am going to assume that EfiReservedMemoryType is the same as > EfiReserved), the only region that is required to be mapped for runtime > services, but isn't mentioned above, is EfiPalCode. The bit about mixed > attribute mappings within a larger page not being allowed makes me think > that EfiPalCode can be mapped even if isn't mapped at the start of a 64KiB > page, as no other memory type can be withing a 64KiB granule. What do you > think? > I wasn't aware of this. So from your explanation, it sounds like if we have multiple regions in any 64k aligned block then it should be possible to map them all using the same mapping? I'll check if we can add rely on this and add some assertions. > There's no pressing need to have support for all page sizes, from my point > of view, it's fine if it's missing from the initial UEFI support. But I > would appreciate a comment in the code or an explanation in the commit > message (or both), because it looks very arbitrary as it is right now. At > the very least this will serve as a nice reminder of what still needs to be > done for full UEFI support. If it's just removing the check in configure and adding assertions in lib/arm/setup.c it shouldn't be a big problem. Thanks, Nikos IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Hi, On 13/07/2022 10:17, Nikos Nikoleris wrote: > On 13/07/2022 09:46, Alexandru Elisei wrote: >> Hi, >> >> On Tue, Jul 12, 2022 at 09:50:51PM +0100, Nikos Nikoleris wrote: >>> Hi Alex, >>> >>> On 12/07/2022 14:39, Alexandru Elisei wrote: >>>> Hi, >>>> >>>> On Thu, Jun 30, 2022 at 11:03:22AM +0100, Nikos Nikoleris wrote: >>>>> Users can now build kvm-unit-tests as efi apps by supplying an extra >>>>> argument when invoking configure: >>>>> >>>>> $> ./configure --enable-efi >>>>> >>>>> This patch is based on an earlier version by >>>>> Andrew Jones <drjones@redhat.com> >>>>> >>>>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com> >>>>> Reviewed-by: Ricardo Koller <ricarkol@google.com> >>>>> --- >>>>> configure | 15 ++++++++++++--- >>>>> arm/Makefile.arm | 6 ++++++ >>>>> arm/Makefile.arm64 | 18 ++++++++++++++---- >>>>> arm/Makefile.common | 45 ++++++++++++++++++++++++++++++++++----------- >>>>> 4 files changed, 66 insertions(+), 18 deletions(-) >>>>> >>>>> diff --git a/configure b/configure >>>>> index 5b7daac..2ff9881 100755 >>>>> --- a/configure >>>>> +++ b/configure >>>> [..] >>>>> @@ -218,6 +223,10 @@ else >>>>> echo "arm64 doesn't support page size of $page_size" >>>>> usage >>>>> fi >>>>> + if [ "$efi" = 'y' ] && [ "$page_size" != "4096" ]; then >>>>> + echo "efi must use 4K pages" >>>>> + exit 1 >>>> >>>> Why this restriction? >>>> >>>> The Makefile compiles kvm-unit-tests to run as an UEFI app, it doesn't >>>> compile UEFI itself. As far as I can tell, UEFI is designed to run payloads >>>> with larger page size (it would be pretty silly to not be able to boot a >>>> kernel built for 16k or 64k pages with UEFI). >>>> >>>> Is there some limitation that I'm missing? >>>> >>> >>> Technically, we could allow 16k or 64k granules. But to do that we would >>> have to handle cases where the memory map we get from EFI cannot be remapped >>> with the new granules. For example, a region might be 12kB and mapping it >>> with 16k or 64k granules without moving it is impossible. >> >> Hm... From UEFI Specification, Version 2.8, page 35: >> >> "The ARM architecture allows mapping pages at a variety of granularities, >> including 4KiB and 64KiB. If a 64KiB physical page contains any 4KiB page >> with any of the following types listed below, then all 4KiB pages in the >> 64KiB page must use identical ARM Memory Page Attributes (as described in >> Table 7): >> >> — EfiRuntimeServicesCode >> — EfiRuntimeServicesData >> — EfiReserved >> — EfiACPIMemoryNVS >> >> Mixed attribute mappings within a larger page are not allowed. >> >> Note: This constraint allows a 64K paged based Operating System to safely >> map runtime services memory." >> >> Looking at Table 30. Memory Type Usage after ExitBootServices(), on page >> 160 (I am going to assume that EfiReservedMemoryType is the same as >> EfiReserved), the only region that is required to be mapped for runtime >> services, but isn't mentioned above, is EfiPalCode. The bit about mixed >> attribute mappings within a larger page not being allowed makes me think >> that EfiPalCode can be mapped even if isn't mapped at the start of a 64KiB >> page, as no other memory type can be withing a 64KiB granule. What do you >> think? >> > I wasn't aware of this. So from your explanation, it sounds like if we > have multiple regions in any 64k aligned block then it should be > possible to map them all using the same mapping? > > I'll check if we can add rely on this and add some assertions. > I've been looking into this and it doesn't seem very straightforward. For example, in my system I run into the case where the first two regions as we get them from EFI are: Region 0x40000000 - 0x48386000 type: EfiConventionalMemory Region 0x48386000 - 0x48450000 type: EfiLoaderCode If we map these two regions in a system with 16k granule that uses identity mapping (virtual address = physical address), the end of the 1st region overlaps with the start of the 2nd region for the range 0x48380000 - 0x48390000. While the 2 regions will have the same memory type, they need to have different permissions. We map EfiConventionalMemory as read/write but EfiLoaderCode as read only (this is to allow shared code between EL0 and EL1). >> There's no pressing need to have support for all page sizes, from my point >> of view, it's fine if it's missing from the initial UEFI support. But I >> would appreciate a comment in the code or an explanation in the commit >> message (or both), because it looks very arbitrary as it is right now. At >> the very least this will serve as a nice reminder of what still needs to be >> done for full UEFI support. > > If it's just removing the check in configure and adding assertions in > lib/arm/setup.c it shouldn't be a big problem. > It seems that dealing with this limitation is not straightforward and unless I am missing something I would leave is as future work. What do you think? Thanks, Nikos
diff --git a/configure b/configure index 5b7daac..2ff9881 100755 --- a/configure +++ b/configure @@ -196,14 +196,19 @@ else fi fi -if [ "$efi" ] && [ "$arch" != "x86_64" ]; then +if [ "$efi" ] && [ "$arch" != "x86_64" ] && [ "$arch" != "arm64" ]; then echo "--[enable|disable]-efi is not supported for $arch" usage fi if [ -z "$page_size" ]; then - [ "$arch" = "arm64" ] && page_size="65536" - [ "$arch" = "arm" ] && page_size="4096" + if [ "$efi" = 'y' ] && [ "$arch" = "arm64" ]; then + page_size="4096" + elif [ "$arch" = "arm64" ]; then + page_size="65536" + elif [ "$arch" = "arm" ]; then + page_size="4096" + fi else if [ "$arch" != "arm64" ]; then echo "--page-size is not supported for $arch" @@ -218,6 +223,10 @@ else echo "arm64 doesn't support page size of $page_size" usage fi + if [ "$efi" = 'y' ] && [ "$page_size" != "4096" ]; then + echo "efi must use 4K pages" + exit 1 + fi fi [ -z "$processor" ] && processor="$arch" diff --git a/arm/Makefile.arm b/arm/Makefile.arm index 01fd4c7..2ce00f5 100644 --- a/arm/Makefile.arm +++ b/arm/Makefile.arm @@ -7,6 +7,10 @@ bits = 32 ldarch = elf32-littlearm machine = -marm -mfpu=vfp +ifeq ($(CONFIG_EFI),y) +$(error Cannot build arm32 tests as EFI apps) +endif + # stack.o relies on frame pointers. KEEP_FRAME_POINTER := y @@ -32,6 +36,8 @@ cflatobjs += lib/arm/stack.o cflatobjs += lib/ldiv32.o cflatobjs += lib/arm/ldivmod.o +exe = flat + # arm specific tests tests = diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64 index 42e18e7..fd3d681 100644 --- a/arm/Makefile.arm64 +++ b/arm/Makefile.arm64 @@ -27,11 +27,21 @@ cflatobjs += lib/arm64/gic-v3-its.o lib/arm64/gic-v3-its-cmd.o OBJDIRS += lib/arm64 +ifeq ($(CONFIG_EFI),y) +# avoid jump tables before all relocations have been processed +arm/efi/reloc_aarch64.o: CFLAGS += -fno-jump-tables +cflatobjs += arm/efi/reloc_aarch64.o + +exe = efi +else +exe = flat +endif + # arm64 specific tests -tests = $(TEST_DIR)/timer.flat -tests += $(TEST_DIR)/micro-bench.flat -tests += $(TEST_DIR)/cache.flat -tests += $(TEST_DIR)/debug.flat +tests = $(TEST_DIR)/timer.$(exe) +tests += $(TEST_DIR)/micro-bench.$(exe) +tests += $(TEST_DIR)/cache.$(exe) +tests += $(TEST_DIR)/debug.$(exe) include $(SRCDIR)/$(TEST_DIR)/Makefile.common diff --git a/arm/Makefile.common b/arm/Makefile.common index 5be42c0..a8007f4 100644 --- a/arm/Makefile.common +++ b/arm/Makefile.common @@ -4,14 +4,14 @@ # Authors: Andrew Jones <drjones@redhat.com> # -tests-common = $(TEST_DIR)/selftest.flat -tests-common += $(TEST_DIR)/spinlock-test.flat -tests-common += $(TEST_DIR)/pci-test.flat -tests-common += $(TEST_DIR)/pmu.flat -tests-common += $(TEST_DIR)/gic.flat -tests-common += $(TEST_DIR)/psci.flat -tests-common += $(TEST_DIR)/sieve.flat -tests-common += $(TEST_DIR)/pl031.flat +tests-common = $(TEST_DIR)/selftest.$(exe) +tests-common += $(TEST_DIR)/spinlock-test.$(exe) +tests-common += $(TEST_DIR)/pci-test.$(exe) +tests-common += $(TEST_DIR)/pmu.$(exe) +tests-common += $(TEST_DIR)/gic.$(exe) +tests-common += $(TEST_DIR)/psci.$(exe) +tests-common += $(TEST_DIR)/sieve.$(exe) +tests-common += $(TEST_DIR)/pl031.$(exe) tests-all = $(tests-common) $(tests) all: directories $(tests-all) @@ -54,6 +54,9 @@ cflatobjs += lib/arm/smp.o cflatobjs += lib/arm/delay.o cflatobjs += lib/arm/gic.o lib/arm/gic-v2.o lib/arm/gic-v3.o cflatobjs += lib/arm/timer.o +ifeq ($(CONFIG_EFI),y) +cflatobjs += lib/efi.o +endif OBJDIRS += lib/arm @@ -61,6 +64,25 @@ libeabi = lib/arm/libeabi.a eabiobjs = lib/arm/eabi_compat.o FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libeabi) + +ifeq ($(CONFIG_EFI),y) +%.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined +%.so: %.o $(FLATLIBS) $(SRCDIR)/arm/efi/elf_aarch64_efi.lds $(cstart.o) + $(CC) $(CFLAGS) -c -o $(@:.so=.aux.o) $(SRCDIR)/lib/auxinfo.c \ + -DPROGNAME=\"$(@:.so=.efi)\" -DAUXFLAGS=$(AUXFLAGS) + $(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/arm/efi/elf_aarch64_efi.lds \ + $(filter %.o, $^) $(FLATLIBS) $(@:.so=.aux.o) \ + $(EFI_LIBS) + $(RM) $(@:.so=.aux.o) + +%.efi: %.so + $(call arch_elf_check, $^) + $(OBJCOPY) \ + -j .text -j .sdata -j .data -j .dynamic -j .dynsym \ + -j .rel -j .rela -j .rel.* -j .rela.* -j .rel* -j .rela* \ + -j .reloc \ + -O binary $^ $@ +else %.elf: LDFLAGS = -nostdlib $(arch_LDFLAGS) %.elf: %.o $(FLATLIBS) $(SRCDIR)/arm/flat.lds $(cstart.o) $(CC) $(CFLAGS) -c -o $(@:.elf=.aux.o) $(SRCDIR)/lib/auxinfo.c \ @@ -74,13 +96,14 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libeabi) $(call arch_elf_check, $^) $(OBJCOPY) -O binary $^ $@ @chmod a-x $@ +endif $(libeabi): $(eabiobjs) $(AR) rcs $@ $^ arm_clean: asm_offsets_clean - $(RM) $(TEST_DIR)/*.{o,flat,elf} $(libeabi) $(eabiobjs) \ - $(TEST_DIR)/.*.d lib/arm/.*.d + $(RM) $(TEST_DIR)/*.{o,flat,elf,so,efi} $(libeabi) $(eabiobjs) \ + $(TEST_DIR)/.*.d $(TEST_DIR)/efi/.*.d lib/arm/.*.d generated-files = $(asm-offsets) -$(tests-all:.flat=.o) $(cstart.o) $(cflatobjs): $(generated-files) +$(tests-all:.$(exe)=.o) $(cstart.o) $(cflatobjs): $(generated-files)