[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1 1/3] arm/mpu: Introduce MPU memory region map structure


  • To: "Orzel, Michal" <michal.orzel@xxxxxxx>, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • From: Ayan Kumar Halder <ayankuma@xxxxxxx>
  • Date: Thu, 5 Jun 2025 14:39:41 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=l3GLrIAh+ueQVINWLFF4tU/zqXrndxrvyjSzTP+6QRA=; b=qfX3NKoN4RESow6ELwriqSTbjAfpX1Gz0X3Jt4ZRnOdNI+yaeNYGcdKC8Eu0Qudpz2GjALcS3griPxP5QnPPKot4hUx6PXBMMqxwbenKpRi+o5O4Ji0hZ4HcuU+z7EfGy4DExlL/weMIjDtidZPBIjBK1vtRTsdQb3c6l9Y+3KSGyFfnltGGscHMClEHpY9hJoLbbBxHh7kbE/v0oRvW0GgOAYg3V0jrDy6fJsQe/Le2fXr9w7L8Nhfqd1WpT57YtBz4HwJlLXbx5MIIP5YI8L1Tc3K7ukMmOcPPAGFbKPW7HkmsM3E81sEfFnNpWNo/kDPtLqpZ1x+NhEbVeiJFjg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=joiX8cbpeZtGFbfbBqnTLKCHWDxUoHZ53IAmXuvLIFwHJxArL7vQT22YHzonzWDoZ/LBuviICGy1bRTYL8kxJjFGZY6HELDudw50MbmZ3ffPW6ORpnjovh1BxOoo69q1nlnWvKeP6udLrWQ/VB2C/UeL54ChN3mJysByWF/0S98qByXxvhZL/uHgRnE+w00NYmXLm/pKVXeQ1BKnRTXpltjqLkX74B1HnQ9rW1dK7u2O+PDIjk+lF14oGLDxD6Acs2D02U1U6vPbHd5yU6JQ4ixrmQqlsqI4bS3ECYgrS5u93rD2MQHddkGDzIQ531iqT5l9jhoYJ0q3+rstx49eaQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Thu, 05 Jun 2025 13:40:00 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

Hi Michal,

On 05/06/2025 08:06, Orzel, Michal wrote:

On 04/06/2025 19:43, Ayan Kumar Halder wrote:
Introduce pr_t typedef which is a structure having the prbar and prlar members,
each being structured as the registers of the AArch32 Armv8-R architecture.

Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the
BASE or LIMIT bitfields in prbar or prlar respectively.

Move pr_t definition to common code.
Also, enclose xn_0 within ARM64 as it is not present for ARM32.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
---
  xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++-----
  xen/arch/arm/include/asm/arm64/mpu.h |  6 ------
  xen/arch/arm/include/asm/mpu.h       |  6 ++++++
  xen/arch/arm/mpu/mm.c                |  2 ++
  4 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/include/asm/arm32/mpu.h 
b/xen/arch/arm/include/asm/arm32/mpu.h
index f0d4d4055c..ae3b661fde 100644
--- a/xen/arch/arm/include/asm/arm32/mpu.h
+++ b/xen/arch/arm/include/asm/arm32/mpu.h
@@ -5,11 +5,31 @@
#ifndef __ASSEMBLY__ -/* MPU Protection Region */
-typedef struct {
-    uint32_t prbar;
-    uint32_t prlar;
-} pr_t;
+#define MPU_REGION_RES0       0x0
The name of the macro does not make a lot of sense in AArch32 context
and can create a confusion for the reader.

I know, but I want to avoid introducing ifdef or have separate implementation (for arm32 and arm64) for the following

Refer xen/arch/arm/include/asm/mpu.h

static inline void pr_set_base(pr_t *pr, paddr_t base)
{
    pr->prbar.reg.base = ((base & ~MPU_REGION_RES0) >> MPU_REGION_SHIFT);
}

Let me know your preference.


+
+/* Hypervisor Protection Region Base Address Register */
+typedef union {
+    struct {
+        unsigned int xn:1;       /* Execute-Never */
+        unsigned int ap_0:1;     /* Acess Permission */
If you write AP[1] below, I would expect here AP[0]

Again same reason as before, let me know if you want to have additional ifdef in pr_of_addr() or separate functions for arm32 and arm64

pr_t pr_of_addr(...)

{...

    prbar = (prbar_t) {
        .reg = {
#ifdef CONFIG_ARM64
            .xn_0 = 0,
#endif
            .xn = PAGE_XN_MASK(flags),
            .ap_0 = 0,
            .ro = PAGE_RO_MASK(flags)
        }};

...

}

Also s/acess/access/
Ack.

+        unsigned long ro:1;      /* Access Permission AP[1] */
+        unsigned int sh:2;       /* Sharebility */
+        unsigned int res0:1;     /* Reserved as 0 */
Here your comment for RES0 (which IMO could be just RES0) does not match...
I will remove the comment here and ..

+        unsigned int base:26;    /* Base Address */
+    } reg;
+    uint32_t bits;
+} prbar_t;
+
+/* Hypervisor Protection Region Limit Address Register */
+typedef union {
+    struct {
+        unsigned int en:1;     /* Region enable */
+        unsigned int ai:3;     /* Memory Attribute Index */
+        unsigned int res0:2;   /* Reserved 0 by hardware */
this one. I don't think you need to explain what RES0 means.

here.

- Ayan


~Michal




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.