Re: [kvm-unit-tests PATCH v2 15/18] arm64: efi: Don't map reserved regions

Nikos Nikoleris
2024-03-04T08:18:37Z

On 27/02/2024 19:21, Andrew Jones wrote:
> We shouldn't need to map all the regions that the EFI memory map
> contains. Just map EFI_LOADER_CODE and EFI_LOADER_DATA, since
> those are for the loaded unit test, and any region types which
> could be used by the unit test for its own memory allocations. We
> still map EFI_BOOT_SERVICES_DATA since the primary stack is on a
> region of that type. In a later patch we'll switch to a stack we
> allocate ourselves to drop that one too.
>
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>

Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>

Thanks,

Nikos

> ---
> lib/arm/mmu.c | 6 +-----
> lib/arm/setup.c | 4 ++--
> lib/memregions.c | 8 ++++++++
> 3 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index eb5e82a95f06..9dce7da85709 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -221,12 +221,8 @@ void *setup_mmu(phys_addr_t phys_end, void *unused)
> mmu_idmap = alloc_page();
>
> for (r = mem_regions; r->end; ++r) {
> - if (r->flags & MR_F_IO) {
> + if (r->flags & (MR_F_IO | MR_F_RESERVED)) {
> continue;
> - } else if (r->flags & MR_F_RESERVED) {
> - /* Reserved pages need to be writable for whatever reserved them */
> - mmu_set_range_ptes(mmu_idmap, r->start, r->start, r->end,
> - __pgprot(PTE_WBWA));
> } else if (r->flags & MR_F_CODE) {
> /* armv8 requires code shared between EL1 and EL0 to be read-only */
> mmu_set_range_ptes(mmu_idmap, r->start, r->start, r->end,
> diff --git a/lib/arm/setup.c b/lib/arm/setup.c
> index 521928186fb0..08658b9a222b 100644
> --- a/lib/arm/setup.c
> +++ b/lib/arm/setup.c
> @@ -309,8 +309,8 @@ static efi_status_t efi_mem_init(efi_bootinfo_t *efi_bootinfo)
> data->flags &= ~MR_F_CODE;
>
> for (struct mem_region *m = mem_regions; m->end; ++m) {
> - if (m != code && (m->flags & MR_F_CODE))
> - m->flags = MR_F_RESERVED;
> + if (m != code)
> + assert(!(m->flags & MR_F_CODE));
>
> if (!(m->flags & MR_F_IO)) {
> if (m->start < __phys_offset)
> diff --git a/lib/memregions.c b/lib/memregions.c
> index 9cdbb639ab62..3c6f751eb4f2 100644
> --- a/lib/memregions.c
> +++ b/lib/memregions.c
> @@ -112,6 +112,14 @@ void memregions_efi_init(struct efi_boot_memmap *mem_map,
> case EFI_LOADER_CODE:
> r.flags = MR_F_CODE;
> break;
> + case EFI_LOADER_DATA:
> + break;
> + case EFI_BOOT_SERVICES_DATA:
> + /*
> + * FIXME: This would ideally be MR_F_RESERVED, but the
> + * primary stack is in a region of this EFI type.
> + */
> + break;
> case EFI_PERSISTENT_MEMORY:
> r.flags = MR_F_PERSISTENT;
> break;