From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web10.8255.1678958885179117082 for ; Thu, 16 Mar 2023 02:28:05 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=UpPFEzJ8; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 3331EB820C0 for ; Thu, 16 Mar 2023 09:28:03 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8707DC433A7 for ; Thu, 16 Mar 2023 09:28:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1678958881; bh=BK0K+Iben4j3LImxLHNeNbt/OoYo0etuQz40KQOqxUQ=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=UpPFEzJ8GHwvW8EHILWq9pOz+knfjrNKMiEpaLToYIjILnP6ovk8w5gnkm8ffcPTp L8oBdSa1txAxpJ3zre1+2lyPtkdTWuloeaGg9rJ8gzGogGR3Yw9qoj5Omku9L4kKS5 SGojb0aB4HFoEBEgP8PA+EHltht6tXKfHI5HAipJPdiPiFWJKlq/1omKJRTD2qQe/L Xuc5ezSUdTrmsyNUfp+G3Uw48lg/JzOmwRdBPWaaa+zmByeA/Jta98foh1gnPuKuqp BmobInGqkVcYxoJji5wYed2ndzEIEqnjiu9MnlopskTI7kg5mQEPIrJVT8ury/hs9Z p/cZDe78yUEPA== Received: by mail-lj1-f179.google.com with SMTP id x36so935998ljq.7 for ; Thu, 16 Mar 2023 02:28:01 -0700 (PDT) X-Gm-Message-State: AO0yUKUZETWnnInUMUL8y6UdN0j7I2SdK2jWRpL5LYvKN1uu/VYy0zXc ZIvMhHRm4pqvbi0pkz8akJCaenKp20NuA3Mj0QA= X-Google-Smtp-Source: AK7set/mFZoWgAA2IOsuw6V0236QhyjnitFmBSy90M1VWWj37KuLoyTcqwsuhrCJxBV+btWpAFXeehf6S4Y5ZXXo040= X-Received: by 2002:a2e:9895:0:b0:298:af7d:a481 with SMTP id b21-20020a2e9895000000b00298af7da481mr1662123ljj.2.1678958879374; Thu, 16 Mar 2023 02:27:59 -0700 (PDT) MIME-Version: 1.0 References: <20230313171714.3866151-1-ardb@kernel.org> <20230313171714.3866151-12-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Thu, 16 Mar 2023 10:27:48 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v5 11/38] ArmPkg/CpuDxe: Implement EFI memory attributes protocol To: devel@edk2.groups.io, quic_llindhol@quicinc.com Cc: Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Sami Mujawar , Taylor Beebe Content-Type: text/plain; charset="UTF-8" On Thu, 16 Mar 2023 at 08:19, Ard Biesheuvel wrote: > > On Wed, 15 Mar 2023 at 19:31, Leif Lindholm wrote: > > > > On Mon, Mar 13, 2023 at 18:16:47 +0100, Ard Biesheuvel wrote: > > > Expose the protocol introduced in v2.10 that permits the caller to > > > manage mapping permissions in the page tables. > > > > Nitpicks and a question: > > > > > Signed-off-by: Ard Biesheuvel > > > --- > > > ArmPkg/Drivers/CpuDxe/CpuDxe.c | 2 + > > > ArmPkg/Drivers/CpuDxe/CpuDxe.h | 3 + > > > ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 + > > > ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 271 ++++++++++++++++++++ > > > 4 files changed, 278 insertions(+) > > > > > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c > > > index e6742f0a25fc..d04958e79e52 100644 > > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c > > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c > > > @@ -244,6 +244,8 @@ CpuDxeInitialize ( > > > &mCpuHandle, > > > &gEfiCpuArchProtocolGuid, > > > &mCpu, > > > + &gEfiMemoryAttributeProtocolGuid, > > > + &mMemoryAttribute, > > > NULL > > > ); > > > > > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h > > > index 8cb105dcc841..ce2981361aca 100644 > > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h > > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h > > > @@ -30,9 +30,12 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > extern BOOLEAN mIsFlushingGCD; > > > > > > +extern EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute; > > > + > > > /** > > > This function registers and enables the handler specified by InterruptHandler for a processor > > > interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the > > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf > > > index 10792b393fc8..e732e21cb94a 100644 > > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf > > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf > > > @@ -23,6 +23,7 @@ [Sources.Common] > > > CpuDxe.h > > > CpuMmuCommon.c > > > Exception.c > > > + MemoryAttribute.c > > > > > > [Sources.ARM] > > > Arm/Mmu.c > > > @@ -53,6 +54,7 @@ [LibraryClasses] > > > > > > [Protocols] > > > gEfiCpuArchProtocolGuid > > > + gEfiMemoryAttributeProtocolGuid > > > > > > [Guids] > > > gEfiDebugImageInfoTableGuid > > > diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c > > > new file mode 100644 > > > index 000000000000..b47464c0269e > > > --- /dev/null > > > +++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c > > > @@ -0,0 +1,271 @@ > > > +/** @file > > > + > > > + Copyright (c) 2023, Google LLC. All rights reserved. > > > + > > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > > + > > > +**/ > > > + > > > +#include "CpuDxe.h" > > > + > > > +/** > > > + This function retrieves the attributes of the memory region specified by > > > + BaseAddress and Length. If different attributes are got from different part > > > > "from different part" -> "for different parts"? > > > > Yeah. this is copy/paste from the protocol definition, which prose I > didn't write. I'll fix up both instances. > > > > + of the memory region, EFI_NO_MAPPING will be returned. > > > + > > > + @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance. > > > + @param BaseAddress The physical address that is the start address of > > > + a memory region. > > > + @param Length The size in bytes of the memory region. > > > + @param Attributes Pointer to attributes returned. > > > + > > > + @retval EFI_SUCCESS The attributes got for the memory region. > > > + @retval EFI_INVALID_PARAMETER Length is zero. > > > + Attributes is NULL. > > > + @retval EFI_NO_MAPPING Attributes are not consistent cross the memory > > > + region. > > > + @retval EFI_UNSUPPORTED The processor does not support one or more > > > + bytes of the memory resource range specified > > > + by BaseAddress and Length. > > > > Question: this implementation never returns EFI_UNSUPPORTED. > > Is this seen as a "some architectures may have some restricted ranges > > not configurable by MMU" which simply does not apply for ARM* - > > i.e. the operation is considered "supported" even if on a region not > > backed by anything? > > > > Yeah, good point. > > The UEFI spec does not really reason about this at all in the > specification of the protocol, but I guess it would make a lot of > sense to only allow this to be used on regions that are marked as > system memory in the GCD memory map. > > I'll have a stab at implementing it like that. Something like this applied on top should work: diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c index b47464c0269e..46f0760b8e3b 100644 --- a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c +++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c @@ -8,6 +8,41 @@ #include "CpuDxe.h" +/** + Check whether the provided memory range is covered by a single entry of type + EfiGcdSystemMemory in the GCD memory map. + + @param BaseAddress The physical address that is the start address of + a memory region. + @param Length The size in bytes of the memory region. + + @return Whether the region is system memory or not. +**/ +STATIC +BOOLEAN +RegionIsSystemMemory ( + IN EFI_PHYSICAL_ADDRESS BaseAddress, + IN UINT64 Length + ) +{ + EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor; + EFI_PHYSICAL_ADDRESS GcdEndAddress; + EFI_STATUS Status; + + Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor); + if (EFI_ERROR (Status) || + (GcdDescriptor.GcdMemoryType != EfiGcdMemoryTypeSystemMemory)) { + return FALSE; + } + + GcdEndAddress = GcdDescriptor.BaseAddress + GcdDescriptor.Length; + + // + // Return TRUE if the GCD descriptor covers the range entirely + // + return GcdEndAddress >= (BaseAddress + Length); +} + /** This function retrieves the attributes of the memory region specified by BaseAddress and Length. If different attributes are got from different part @@ -49,6 +84,10 @@ GetMemoryAttributes ( return EFI_INVALID_PARAMETER; } + if (!RegionIsSystemMemory (BaseAddress, Length)) { + return EFI_UNSUPPORTED; + } + DEBUG (( DEBUG_VERBOSE, "%a: BaseAddress == 0x%lx, Length == 0x%lx\n", @@ -160,6 +199,10 @@ SetMemoryAttributes ( return EFI_INVALID_PARAMETER; } + if (!RegionIsSystemMemory (BaseAddress, Length)) { + return EFI_UNSUPPORTED; + } + if ((Attributes & EFI_MEMORY_RP) != 0) { Status = ArmSetMemoryRegionNoAccess (BaseAddress, Length); if (EFI_ERROR (Status)) { @@ -240,6 +283,10 @@ ClearMemoryAttributes ( return EFI_INVALID_PARAMETER; } + if (!RegionIsSystemMemory (BaseAddress, Length)) { + return EFI_UNSUPPORTED; + } + if ((Attributes & EFI_MEMORY_RP) != 0) { Status = ArmClearMemoryRegionNoAccess (BaseAddress, Length); if (EFI_ERROR (Status)) {