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

[Nikos Nikoleris <nikos.nikoleris@arm.com>]

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.