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.web11.644.1678814697016774216 for ; Tue, 14 Mar 2023 10:24:57 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=m7NiGWWw; 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 (m0279868.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 32EEaTxG009792; Tue, 14 Mar 2023 17:24:46 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=Jv00xTKBn4J8UdSjxdBE2gzoOguStH5+h73DuDyC3sk=; b=m7NiGWWwrK2KT/f1VUuJ+WmdqLZ4ulyffUBq32CZHIEcN8yLXNOXAl8xJJJj2ibXLBbC JkWKDDJdpalVaeUS1GD9kTdkEOZnptsCtbnE1jb9FhNlty8XwY4N5DZCYv+NwY5aeGeB ZaqK/sZ5+o99mH8GHnR10QLoH0Jb8iRRQFPFAkrbrDlQWaHK1qUDura0jlIBacvAFxe4 eiUVc6C6aahyvqwW7uj+NrYHh+41A0MdHwGvMXDVB3IchXqLKkN+1omJUn8F05QrYokD DPBoIhFt+MWwg0qCQ9tc4RcxNDvYe5FRnEKApXRqb37ZQDXdsUJr4kPIdTfjMoB0bbkY 9g== Received: from nasanppmta05.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3pa9gfk7u0-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 14 Mar 2023 17:24:46 +0000 Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA05.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 32EHOjVf030990 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 14 Mar 2023 17:24:45 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; Tue, 14 Mar 2023 10:24:42 -0700 Date: Tue, 14 Mar 2023 17:24:39 +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: [PATCH v5 02/38] ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field Message-ID: References: <20230313171714.3866151-1-ardb@kernel.org> <20230313171714.3866151-3-ardb@kernel.org> MIME-Version: 1.0 In-Reply-To: <20230313171714.3866151-3-ardb@kernel.org> X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) 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-GUID: Ww5z8RBzUp7WF4_FlyXqFYd7AiW4L9ZV X-Proofpoint-ORIG-GUID: Ww5z8RBzUp7WF4_FlyXqFYd7AiW4L9ZV 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-14_10,2023-03-14_02,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 mlxscore=0 mlxlogscore=437 suspectscore=0 adultscore=0 clxscore=1011 phishscore=0 bulkscore=0 spamscore=0 impostorscore=0 priorityscore=1501 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000 definitions=main-2303140142 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline On Mon, Mar 13, 2023 at 18:16:38 +0100, Ard Biesheuvel wrote: > With large page support out of the picture, we can treat bits 1 and 0 of > the page descriptor as individual valid and XN bits, instead of treating > XN as a page type. Doing so aligns the handling of the attribute with > the section descriptor layout, as well as the XN handling on AArch64, > and this is beneficial for maintainability. On average, this is a big enough improvement that I'm happy for it to go in, but it *does* take a step away from the actual architectural definition. I'll choose to see it as viewing aarch32 through aarch64 goggles, which feels reasonable these days. Reviewed-by: Leif Lindholm > Signed-off-by: Ard Biesheuvel > --- > ArmPkg/Include/Chipset/ArmV7Mmu.h | 8 +++----- > ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 12 ++++++------ > 2 files changed, 9 insertions(+), 11 deletions(-) > > diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h b/ArmPkg/Include/Chipset/ArmV7Mmu.h > index 7501ebfdf97f..6a2584ceb303 100644 > --- a/ArmPkg/Include/Chipset/ArmV7Mmu.h > +++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h > @@ -54,11 +54,9 @@ > #define TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Desc) (((Desc) & 3UL) == TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE) > > // Translation table descriptor types > -#define TT_DESCRIPTOR_PAGE_TYPE_MASK (3UL << 0) > -#define TT_DESCRIPTOR_PAGE_TYPE_FAULT (0UL << 0) > -#define TT_DESCRIPTOR_PAGE_TYPE_PAGE (2UL << 0) > -#define TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN (3UL << 0) > -#define TT_DESCRIPTOR_PAGE_TYPE_LARGEPAGE (1UL << 0) > +#define TT_DESCRIPTOR_PAGE_TYPE_MASK (1UL << 1) > +#define TT_DESCRIPTOR_PAGE_TYPE_FAULT (0UL << 1) > +#define TT_DESCRIPTOR_PAGE_TYPE_PAGE (1UL << 1) > > // Section descriptor definitions > #define TT_DESCRIPTOR_SECTION_SIZE (0x00100000) > diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c > index 9ca00c976d5f..12d0f4c30f8e 100644 > --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c > +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c > @@ -104,12 +104,8 @@ UpdatePageEntries ( > > // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone) > // EntryValue: values at bit positions specified by EntryMask > - EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK; > - if ((Attributes & EFI_MEMORY_XP) != 0) { > - EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN; > - } else { > - EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE; > - } > + EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK | TT_DESCRIPTOR_PAGE_XN_MASK; > + EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE; > > // Although the PI spec is unclear on this, the GCD guarantees that only > // one Attribute bit is set at a time, so the order of the conditionals below > @@ -148,6 +144,10 @@ UpdatePageEntries ( > EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW; > } > > + if ((Attributes & EFI_MEMORY_XP) != 0) { > + EntryValue |= TT_DESCRIPTOR_PAGE_XN_MASK; > + } > + > // Obtain page table base > FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress (); > > -- > 2.39.2