From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by mx.groups.io with SMTP id smtpd.web10.9988.1678966891749835567 for ; Thu, 16 Mar 2023 04:41:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=Iw/LpKig; spf=permerror, err=parse error for token &{10 18 %{ir}.%{v}.%{d}.spf.has.pphosted.com}: invalid domain name (domain: quicinc.com, ip: 205.220.180.131, mailfrom: quic_llindhol@quicinc.com) Received: from pps.filterd (m0279872.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 32G24krn023984; Thu, 16 Mar 2023 11:41:21 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=qcppdkim1; bh=aD74h0uLScDFgYMfiSq4pAlXoEWnxWrXOR8eid62aog=; b=Iw/LpKig/ZM2fhegMpjBgPra8rTDX3tLpYF/mpaEhCGdGQM1RmiVgqfd57/Fj3Za7xdY XMuDM2xYu7LbRlz0nxAuo7OT4sUPgqWlztVfmNzp88MHs3R2kCBkO9WABavQjXjoLobJ J7BTRVRJi7EhOBsRfHFN6J1oOXotKy994nO4BPCF5AYqa3wiftFJB//9dXOQNCErnkj6 W08lLT+UMQJ98gyzC+NtMYQMyMLRTs0MqODBhqHwB/B8zOwQTP3DexsukPCsu6wtjSb5 i24PGyJxqvfLnrFY0BxOnRl957IKtom7bWK8sdE6xoQt32Vvb3lNzB8yHNWbPGZsEZec hA== Received: from nasanppmta01.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3pbpy39n9j-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 16 Mar 2023 11:41:21 +0000 Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA01.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 32GBfKDE007797 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 16 Mar 2023 11:41:20 GMT Received: from qc-i7.hemma.eciton.net (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.41; Thu, 16 Mar 2023 04:41:18 -0700 Date: Thu, 16 Mar 2023 11:41:14 +0000 From: "Leif Lindholm" To: Ard Biesheuvel CC: , Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Sami Mujawar , Taylor Beebe Subject: Re: [edk2-devel] [PATCH v5 11/38] ArmPkg/CpuDxe: Implement EFI memory attributes protocol Message-ID: References: <20230313171714.3866151-1-ardb@kernel.org> <20230313171714.3866151-12-ardb@kernel.org> MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-ORIG-GUID: RB97Xtj_xpCDmSLxya7BpD-k1ObShxkD X-Proofpoint-GUID: RB97Xtj_xpCDmSLxya7BpD-k1ObShxkD X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-03-16_08,2023-03-16_01,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 mlxlogscore=941 spamscore=0 priorityscore=1501 impostorscore=0 adultscore=0 phishscore=0 suspectscore=0 bulkscore=0 lowpriorityscore=0 mlxscore=0 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303150002 definitions=main-2303160097 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline On Thu, Mar 16, 2023 at 10:27:48 +0100, Ard Biesheuvel wrote: > 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: Perfect, thanks. With this folded in: Reviewed-by: Leif Lindholm > 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)) {