[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: Fri, 6 Jun 2025 13:43:32 +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=0FXMRtkayhpDdmr07UxFIaSnVNcobeRmDbPAwfEKrCU=; b=swwpeMQBiGWPmgdaeK15jkd5AFmgQDqMNRXj7ashAgEaAGxM5RQP+a8a2pAkULsft8eF0hkzkTtYFKH2Imnc+eLQ4m6+ZSJr/g/0U7CAwY720HKR5W1YAgdDQQ6jVOZCAsmpOLA8xmqG1LRu8j8GDq0AYCZyN+iRnkDFPRx1gdahnN13w1jI3gQrCzCfqxcswoWPiRiQ8iBkOvZJdobdshhyMffVwe5RytoECl5OkZBCGhBzr4zIU/7LwNVU/ICcgVBP95HYDRF7IgsJNuklSJj/IyuPOsh29I/TUBvLSdJEuZdHx4acc9PUhAm5/7+TqAOjjxMtjMUhx4IxZvVyxw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=Btvxw47Pm4qB/W6gM+nsOGl32mLWbhpu1H6AKzoel2oKfFsoyHND9Rq1Vxj8fnlb5yCbAOzdd92qwKlKbx0LLxcKoYCzBVB27CtNqKZhz45x93na8cLhIVXPHjkdn3u3A0SZ/xWywcfoPsH1kNL+d7/40NkYm3NAMQOufnnukCplouRwplHP9uflurrkQzkTidMFkU+NQ7XrHPqMwLdjUePnqpH+UoBw8SDOzMHXx1cVhkgZ1ZQn9TRBX0x87jBfsKg+TTVz9CWxft6KVnzxXkldAQAUHgxrNW0E4KIqWZyns+L8n3zyHy4bdGPXxdUKwqewr0fm/Kzfpx4QsYDWtQ==
  • 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: Fri, 06 Jun 2025 12:43:50 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

Hi Michal,

On 06/06/2025 11:13, Orzel, Michal wrote:

On 05/06/2025 15:39, Ayan Kumar Halder wrote:
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.
I did not mean #ifdef-ing. I was more like suggesting to use a different macro
name that would be more meaningful than this one.

Now I understand you. However, I can't thing of a better name to make it more meaningful.

I have added a comment on top. Is this helpful ?

/* Unlike arm64, there are no reserved 0 bits beyond base in prbar register */


+
+/* 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
I don't understand. My comment was only about changing comment to say /* Access
Permission AP[0] */ because below you have a comment with AP[1].

Ah makes sense now.

- Ayan


~Michal




 


Rackspace

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