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

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses





On 29/11/2022 16:23, Ayan Kumar Halder wrote:

On 29/11/2022 14:52, Julien Grall wrote:


On 29/11/2022 14:57, Ayan Kumar Halder wrote:
Hi All,

Hi,

Hi Julien,

Many thanks for your inputs.


I am trying to gather opinions on how to support 32 bit physical addresses to enable Xen running on R52.

Refer Cortex R52 TRM, Section 2.2.12 "Memory Model"

"...This is because the physical address is always the same as the virtual address...The virtual and physical address can be treated as synonyms for Cortex-R52."

Thus, I understand that R52 supports 32 bit physical address only. This is a bit different from Armv7 systems which supports Large Physical Address Extension (LPAE) ie 40 bit physical addresses. >
Please correct me if I misunderstand something. >
So currently, Xen supports 64 bit physical address for Arm_32 (ie Armv7) based system.

Xen supports *up to* 64-bit physical address. This may be lower in the HW (not all the Armv7 HW supports 40-bit address).

My aim is to enable support for 32 bit physical address.

Technically this is already supported because this is a subset of 64-bit. I can see a use case (even on non R* HW) where you may want to use 32-bit paddr_t to reduce the code size (and registers used).

But I think that's more an optimization that rather than been necessary.

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 6014c0f852..4f8b5fc4be 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -56,10 +56,10 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
  }

  void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
-                                u32 size_cells, u64 *start, u64 *size)
+                                u32 size_cells, paddr_t *start, paddr_t *size)

This needs to stay uint64_t because the Device-Tree may contain 64-bit values and you...

Are you saying that the device tree may contain 64 bit addresses even though the platform is 32 bit ?

There should not be any 32-bit address but you don't know what the device-tree is containing because this is user input.

This is not the business of the Device-Tree parser to decide whether the value should be downcasted or rejected. That's the goal of the callers.


I think then this approach (ie "typedef u32 paddr_t" for 32 bit system) is incorrect.
I am a bit surprised you came to this conclusion just based on the above. As I said before, there are benefits to allow Xen to be built with 32-bit (e.g. smaller code size and better use of the register).


Then, the other option would be to downcast 64 bit physical addresses to 32 bits, when we need to translate pa to va.

Do you think this approach looks better ?

Some of the changes you propose are questionable (see below).

Or any better suggestions ?

Rework you previous approach by not touching the Device-Tree code.

diff --git a/xen/arch/arm/include/asm/mm_mpu.h b/xen/arch/arm/include/asm/mm_mpu.h
index 306a4c497c..f4f5ae1488 100644
--- a/xen/arch/arm/include/asm/mm_mpu.h
+++ b/xen/arch/arm/include/asm/mm_mpu.h
@@ -89,7 +89,18 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
  static inline void *maddr_to_virt(paddr_t ma)
  {
      /* In MPU system, VA == PA. */
+#ifdef CONFIG_AARCH32_V8R
+    /*
+     * 64 bit physical addresses are not supported.
+     * Raise a bug if one encounters 64 bit address.
+     */
+    if (ma >> BITOP_BITS_PER_WORD)
+        BUG();
I don't particularly like the runtime check when you should be able to sanitize the values before hand.

+
+    return (void *) ((uint32_t)(ma & GENMASK(31,0)));

& GENMASK (...) is a bit pointless here given that you above confirmed the top 32-bit are zeroed.

+#else
      return (void *)ma;
+#endif
  }

  /*
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index b3330cd584..3f4ac7f475 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -119,7 +119,11 @@ extern struct bootinfo bootinfo;

  extern domid_t max_init_domid;

+#ifdef CONFIG_AARCH32_V8R
+void copy_from_paddr(void *dst, uint32_t paddr, unsigned long len);
+#else
  void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
+#endif

I don't understand why the probably needs to be changed here...


  size_t estimate_efi_size(unsigned int mem_nr_banks);

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 04c05d7a05..7a7386f33a 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -46,7 +46,11 @@ struct minimal_dtb_header {
   * @paddr: source physical address
   * @len: length to copy
   */
+#ifdef CONFIG_AARCH32_V8R
+void __init copy_from_paddr(void *dst, uint32_t paddr, unsigned long len)
+#else
  void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
+#endif
  {
      void *src = (void *)(unsigned long)paddr;

... because the code should compile without any issue. If you were really concern about ignore the top 32-bit, then you could add a BUG_ON() (This is OK because this is init code).


diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c
index df43621ee7..62774aebc6 100644
--- a/xen/arch/arm/mm_mpu.c
+++ b/xen/arch/arm/mm_mpu.c
@@ -29,7 +29,7 @@
  #include <asm/arm64/fw_shareinfo.h>
  #endif

-#ifdef CONFIG_AARCH32_ARMV8_R
+#ifdef CONFIG_AARCH32_V8R
  #include <asm/arm32/armv8r/sysregs.h>
  #endif

@@ -414,7 +414,18 @@ void *ioremap_attr(paddr_t pa, size_t len, unsigned int attributes)
          return NULL;
      }

+#ifdef CONFIG_AARCH32_V8R
+    /*
+     * 64 bit physical addresses are not supported.
+     * Raise a bug if one encounters 64 bit address.
+     */
+    if (pa >> BITOP_BITS_PER_WORD)
+        BUG();

Why not returning NULL?

+
+    return (void *) ((uint32_t)(pa & GENMASK(31,0)));
+#else
      return (void *)pa;
+#endif
  }

  static void clear_boot_mpumap(void)
@@ -1007,7 +1018,19 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
      nr_xen_mpumap++;

      /* VA == PA */
+#ifdef CONFIG_AARCH32_V8R
+
+    /*
+     * 64 bit physical addresses are not supported.
+     * Raise a bug if one encounters 64 bit address.
+     */
+    if (fdt_paddr >> BITOP_BITS_PER_WORD)
+        BUG();

Same here question here.

+
+    fdt_virt = (void *) ((uint32_t)(fdt_paddr & GENMASK(31,0)));
+#else
      fdt_virt = (void *)fdt_paddr;
+#endif

      if ( fdt_magic(fdt_virt) != FDT_MAGIC )
          return NULL;
@@ -1165,13 +1188,13 @@ void __init setup_protection_regions()
          {
              pr_t region;
              access_protection_region(true, &region, NULL, i);
-#ifdef CONFIG_AARCH32_ARMV8_R
+#ifdef CONFIG_AARCH32_V8R
             printk("Boot-time Xen MPU memory configuration. #%u : 0x%"PRIx32" - 0x%"PRIx32".\n",
                     i, pr_get_base(&region), pr_get_limit(&region));
-#else /* CONFIG_AARCH32_ARMV8_R */
+#else
             printk("Boot-time Xen MPU memory configuration. #%u : 0x%"PRIx64" - 0x%"PRIx64".\n",
                     i, pr_get_base(&region), pr_get_limit(&region));
-#endif /* CONFIG_AARCH32_ARMV8_R */
+#endif
          }
  }

@@ -1262,8 +1285,13 @@ static int __init relocate_xen_mpumap(void)
      if ( !xen_mpumap )
          return -EINVAL;

+#ifdef CONFIG_AARCH32_V8R
+    copy_from_paddr(xen_mpumap, (uint32_t)(pr_t *)boot_mpumap,
+                    sizeof(pr_t) * next_xen_mpumap_index);
+#else
      copy_from_paddr(xen_mpumap, (paddr_t)(pr_t *)boot_mpumap,
                      sizeof(pr_t) * next_xen_mpumap_index);
+#endif

      clear_boot_mpumap();

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 62afb07bc6..a73bf7de01 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
  {
      ASSERT(!first_node_initialised);
      ASSERT(!xenheap_bits);
+#ifndef CONFIG_AARCH32_V8R
      BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
+#endif

BUILD_BUG_ON() are used to indicate that the code would fall over the check pass. I can't find the justification for this change in the commit message.

It is also not clear why you are modifying this path because so far on Arm32 the xenheap and domheap are separated for good reason (i.e. lack of address space). Is this going to change with Armv8-R?

Cheers,

--
Julien Grall



 


Rackspace

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