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

Re: [PATCH v3 31/52] xen/mpu: make early_fdt_map support in MPU systems



Hi,

On 30/06/2023 15:42, Ayan Kumar Halder wrote:
Hi Julien,

On 30/06/2023 12:22, Julien Grall wrote:


On 30/06/2023 11:49, Ayan Kumar Halder wrote:

On 30/06/2023 05:07, Penny Zheng wrote:
Hi,
Hi Penny,


On 2023/6/30 01:22, Ayan Kumar Halder wrote:

On 26/06/2023 04:34, Penny Zheng wrote:
CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.


In MPU system, MPU memory region is always mapped PAGE_ALIGN, so in order to not access unexpected memory area, dtb section in xen.lds.S should be made
page-aligned too.
We add . = ALIGN(PAGE_SIZE); in the head of dtb section to make it happen.

In this commit, we map early FDT with a transient MPU memory region, as
it will be relocated into heap and unmapped at the end of boot.

Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
---
v3:
- map the first 2MB. Check the size and then re-map with an extra 2MB if needed
---
  xen/arch/arm/include/asm/arm64/mpu.h |  3 ++-
  xen/arch/arm/include/asm/page.h      |  5 +++++
  xen/arch/arm/mm.c                    | 26 ++++++++++++++++++++------
  xen/arch/arm/mpu/mm.c                |  1 +
  xen/arch/arm/xen.lds.S               |  5 ++++-
  5 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h
index a6b07bab02..715ea69884 100644
--- a/xen/arch/arm/include/asm/arm64/mpu.h
+++ b/xen/arch/arm/include/asm/arm64/mpu.h
@@ -72,7 +72,8 @@ typedef union {
          unsigned long ns:1;     /* Not-Secure */
          unsigned long res:1;    /* Reserved 0 by hardware */
          unsigned long limit:42; /* Limit Address */
-        unsigned long pad:16;
+        unsigned long pad:15;
+        unsigned long tran:1;   /* Transient region */
      } reg;
      uint64_t bits;
  } prlar_t;
diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h
index 85ecd5e4de..a434e2205a 100644
--- a/xen/arch/arm/include/asm/page.h
+++ b/xen/arch/arm/include/asm/page.h
@@ -97,19 +97,24 @@
   * [3:4] Execute Never
   * [5:6] Access Permission
   * [7]   Region Present
+ * [8]   Transient Region, e.g. MPU memory region is temproraily
+ *                              mapped for a short time
   */
  #define _PAGE_AI_BIT            0
  #define _PAGE_XN_BIT            3
  #define _PAGE_AP_BIT            5
  #define _PAGE_PRESENT_BIT       7
+#define _PAGE_TRANSIENT_BIT     8
I don't think this is related to MPU. At least when I look at the bit representation of PRBAR_EL1/2,

This set of _PAGE_xxx flags aren't compliant with PRBAR_EL1/2 register map.
It is a flag passed to function map_pages_to_xen() to indicate memory
attributes and permission.

But aren't you writing these flags to PRBAR_EL1/EL2 when you call xen_mpumap_update_entry().

In the below snippet of xen_mpumap_update_entry(), IIUC, you are writing these flags.

         xen_mpumap[idx].prbar.reg.ap = PAGE_AP_MASK(flags);
         xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags);

         write_protection_region((const pr_t*)(&xen_mpumap[idx]), idx);

Please clarify here.

In this case, I don't prefer mixing hardware specific bits with software only representation for these reasons :-

1. It makes it confusing and hard to differentiate the hardware specific attrbutes from software only.

Penny's approach matches what we are doing in the MMU code. We want to have a way for the caller to pass just set of flags and let the callee to decide what to do with them.

This may be flags converted for HW fields or just used by the logic.

If you disagree with this approach, then can you propose a different way that we can discuss?

In MMU, I could see "struct lpae_pt_t{ .avail }" is used for reference count.

Something like this might look better (where the hardware specific bits are not reused for sw logic)

struct {

           struct __packed {

                  ... /* the existing bits as they are */

           } lpae_pt_t;

           unsigned int ref_count;

} p2m_entry
 > And use "p2m_entry.ref_count" for the reference counting.

So where would you store 'ref_count'? In the existing approach, this is store in the PTE and that's fine because the bits 58:55 are reserved for software use.

This is right in the middle of the PTE. So we have no other choice other than describing right in the middle of lpae_pt_t.

Cheers,

--
Julien Grall



 


Rackspace

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