Re: [kvm-unit-tests PATCH v3 25/27] arm64: Add support for efi in Makefile

Alexandru Elisei
2022-07-13T08:46:01Z

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