[XEN v8 0/5] Add support for 32-bit physical address

[XEN v8 0/5] Add support for 32-bit physical address

[Ayan Kumar Halder]

Hi All,

Please have a look at https://lists.xenproject.org/archives/html/xen-devel/2022-11/msg01465.html
for the context.

The benefits of using 32 bit physical addresses are as follows :-

1. It helps to use Xen on platforms (for eg R52) which supports 32-bit
physical addresses and has no support for large physical address extension.
On 32-bit MPU systems which supports flat-mapping (for eg R52), it helps
to translate 32 bit VA into 32 bit PA.

2. It also helps in code optimization when the underlying platform does not
use large physical address extension.

The following points are to be noted :-
1. Device tree always use uint64_t for address and size. The caller needs to
translate between uint64_t and unsigned long (when 32 bit physical addressing is used).
2. Currently, we have enabled this option for Arm_32 as the MMU for Arm_64
uses 48-bit physical addressing.

Changes from :

v1 - 1. Reordered the patches such that the first three patches fixes issues in
the existing codebase. These can be applied independent of the remaining patches
in this serie.

2. Dropped translate_dt_address_size() for the address/size translation between
paddr_t and u64 (as parsed from the device tree). Also, dropped the check for
truncation (while converting u64 to paddr_t).
Instead now we have modified device_tree_get_reg() and typecasted the return for
dt_read_number(), to obtain paddr_t. Also, introduced wrappers for
fdt_get_mem_rsv() and dt_device_get_address() for the same purpose. These can be
found in patch 4/11 and patch 6/11.

3. Split "Other adaptations required to support 32bit paddr" into the following
individual patches for each adaptation :
  xen/arm: smmu: Use writeq_relaxed_non_atomic() for writing to
    SMMU_CBn_TTBR0
  xen/arm: guest_walk: LPAE specific bits should be enclosed within
    "ifndef CONFIG_ARM_PA_32"

4. Introduced "xen/arm: p2m: Enable support for 32bit IPA".

v2 - 1. Dropped patches 1/11, 2/11 and 3/11 from the v2 as it has already been
committed (except 2/11 - "[XEN v5] xen/arm: Use the correct format specifier"
which is waiting to be committed).

2. Introduced a new patch "xen/drivers: ns16550: Use paddr_t for io_base/io_size".

v3 - 1. Combined the patches from https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00656.html in this series.

v4 - 1. Dropped "xen/drivers: ns16550: Use paddr_t for io_base/io_size" from the patch series.

2. Introduced "xen/arm: domain_build: Check if the address fits the range of physical address".

3. "xen/arm: Use the correct format specifier" has been committed in v4.

v5 - 1. Based on the comments on "[XEN v5 08/10] xen/arm: domain_build: Check if the address fits the range of physical address",
the patch has been modified and split into the following :-

a.  xen: dt: Replace u64 with uint64_t as the callback function parameters
    for dt_for_each_range()
b.  xen/arm: pci: Use 'uint64_t' as the datatype for the function
    parameters.
c.  xen/arm: domain_build: Check if the address fits the range of physical
    address

v6 - 1. Reordered the patches such that only the patches which are dependent on
"CONFIG_PHYS_ADDR_T_32" appear after the Kconfig option is introduced.

v7 - 1. Changes from "[XEN v7 01/11] xen/arm: domain_build: Track unallocated pages using the frame number
" till "[XEN v7 06/11] xen: dt: Replace u64 with uint64_t as the callback function parameters for dt_for_each_range()"
have been committed. So the remaining 5 patches are sent out.

Ayan Kumar Halder (5):
  xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64
  xen/arm: Introduce choice to enable 64/32 bit physical addressing
  xen/arm: guest_walk: LPAE specific bits should be enclosed within
    "ifndef CONFIG_PHYS_ADDR_T_32"
  xen/arm: Restrict zeroeth_table_offset for ARM_64
  xen/arm: p2m: Enable support for 32bit IPA for ARM_32

 xen/arch/Kconfig                     |  3 ++
 xen/arch/arm/Kconfig                 | 33 ++++++++++++++++++
 xen/arch/arm/guest_walk.c            |  2 ++
 xen/arch/arm/include/asm/lpae.h      |  4 +++
 xen/arch/arm/include/asm/p2m.h       |  6 ----
 xen/arch/arm/include/asm/page-bits.h |  6 +---
 xen/arch/arm/include/asm/types.h     | 14 ++++++++
 xen/arch/arm/mm.c                    | 12 +++----
 xen/arch/arm/p2m.c                   | 51 +++++++++++++++++++++-------
 9 files changed, 101 insertions(+), 30 deletions(-)

-- 
2.17.1

[XEN v8 1/5] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Ayan Kumar Halder]

Restructure the code so that one can use pa_range_info[] table for both
ARM_32 as well as ARM_64.

Also, removed the hardcoding for P2M_ROOT_ORDER and P2M_ROOT_LEVEL as
p2m_root_order can be obtained from the pa_range_info[].root_order and
p2m_root_level can be obtained from pa_range_info[].sl0.

Refer ARM DDI 0406C.d ID040418, B3-1345,
"Use of concatenated first-level translation tables

...However, a 40-bit input address range with a translation granularity of 4KB
requires a total of 28 bits of address resolution. Therefore, a stage 2
translation that supports a 40-bit input address range requires two concatenated
first-level translation tables,..."

Thus, root-order is 1 for 40-bit IPA on ARM_32.

Refer ARM DDI 0406C.d ID040418, B3-1348,

"Determining the required first lookup level for stage 2 translations

For a stage 2 translation, the output address range from the stage 1
translations determines the required input address range for the stage 2
translation. The permitted values of VTCR.SL0 are:

0b00 Stage 2 translation lookup must start at the second level.
0b01 Stage 2 translation lookup must start at the first level.

VTCR.T0SZ must indicate the required input address range. The size of the input
address region is 2^(32-T0SZ) bytes."

Thus VTCR.SL0 = 1 (maximum value) and VTCR.T0SZ = -8 when the size of input
address region is 2^40 bytes.

Thus, pa_range_info[].t0sz = 1 (VTCR.S) | 8 (VTCR.T0SZ) ie 11000b which is 24.

VTCR.T0SZ, is bits [5:0] for ARM_64.
VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32.

For this, we have used struct bitfields to convert pa_range_info[].t0sz to its
ARM_32 variant.

pa_range_info[] is indexed by ID_AA64MMFR0_EL1.PARange which is present in Arm64
only. This is the reason we do not specify the indices for ARM_32. Also, we
duplicated the entry "{ 40,      24/*24*/,  1,          1 }" between ARM_64 and
ARM_32. This is done to avoid introducing extra #if-defs.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from -

v3 - 1. New patch introduced in v4.
2. Restructure the code such that pa_range_info[] is used both by ARM_32 as
well as ARM_64.

v4 - 1. Removed the hardcoded definitions of P2M_ROOT_ORDER and P2M_ROOT_LEVEL.
The reason being root_order will not be always 1 (See the next patch).
2. Updated the commit message to explain t0sz, sl0 and root_order values for
32-bit IPA on Arm32.
3. Some sanity fixes.

v5 - pa_range_info is indexed by system_cpuinfo.mm64.pa_range. ie
when PARange is 0, the PA size is 32, 1 -> 36 and so on. So pa_range_info[] has
been updated accordingly.
For ARM_32 pa_range_info[0] = 0 and pa_range_info[1] = 0 as we do not support
32-bit, 36-bit physical address range yet.

v6 - 1. Added pa_range_info[] entries for ARM_32 without indices. Some entry
may be duplicated between ARM_64 and ARM_32.
2. Recalculate p2m_ipa_bits for ARM_32 from T0SZ (similar to ARM_64).
3. Introduced an union to reinterpret T0SZ bits between ARM_32 and ARM_64.

v7 - 1. Used struct bifield instead of union to reinterpret T0SZ bits between
ARM_32 and ARM_64.
2. Removed the invalid entry for ARM_32.

 xen/arch/arm/include/asm/p2m.h |  6 ----
 xen/arch/arm/p2m.c             | 50 +++++++++++++++++++++++++---------
 2 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
index f67e9ddc72..940495d42b 100644
--- a/xen/arch/arm/include/asm/p2m.h
+++ b/xen/arch/arm/include/asm/p2m.h
@@ -14,16 +14,10 @@
 /* Holds the bit size of IPAs in p2m tables.  */
 extern unsigned int p2m_ipa_bits;
 
-#ifdef CONFIG_ARM_64
 extern unsigned int p2m_root_order;
 extern unsigned int p2m_root_level;
 #define P2M_ROOT_ORDER    p2m_root_order
 #define P2M_ROOT_LEVEL p2m_root_level
-#else
-/* First level P2M is always 2 consecutive pages */
-#define P2M_ROOT_ORDER    1
-#define P2M_ROOT_LEVEL 1
-#endif
 
 struct domain;
 
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 418997843d..76388ba54b 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -19,9 +19,9 @@
 
 #define INVALID_VMID 0 /* VMID 0 is reserved */
 
-#ifdef CONFIG_ARM_64
 unsigned int __read_mostly p2m_root_order;
 unsigned int __read_mostly p2m_root_level;
+#ifdef CONFIG_ARM_64
 static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT;
 /* VMID is by default 8 bit width on AArch64 */
 #define MAX_VMID       max_vmid
@@ -2247,16 +2247,6 @@ void __init setup_virt_paging(void)
     /* Setup Stage 2 address translation */
     register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
 
-#ifdef CONFIG_ARM_32
-    if ( p2m_ipa_bits < 40 )
-        panic("P2M: Not able to support %u-bit IPA at the moment\n",
-              p2m_ipa_bits);
-
-    printk("P2M: 40-bit IPA\n");
-    p2m_ipa_bits = 40;
-    val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
-    val |= VTCR_SL0(0x1); /* P2M starts at first level */
-#else /* CONFIG_ARM_64 */
     static const struct {
         unsigned int pabits; /* Physical Address Size */
         unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
@@ -2265,6 +2255,7 @@ void __init setup_virt_paging(void)
     } pa_range_info[] __initconst = {
         /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
         /*      PA size, t0sz(min), root-order, sl0(max) */
+#ifdef CONFIG_ARM_64
         [0] = { 32,      32/*32*/,  0,          1 },
         [1] = { 36,      28/*28*/,  0,          1 },
         [2] = { 40,      24/*24*/,  1,          1 },
@@ -2273,11 +2264,28 @@ void __init setup_virt_paging(void)
         [5] = { 48,      16/*16*/,  0,          2 },
         [6] = { 52,      12/*12*/,  4,          2 },
         [7] = { 0 }  /* Invalid */
+#else
+        { 40,      24/*24*/,  1,          1 }
+#endif
     };
 
     unsigned int i;
     unsigned int pa_range = 0x10; /* Larger than any possible value */
 
+#ifdef CONFIG_ARM_32
+    /*
+     * Typecast pa_range_info[].t0sz into ARM_32 bit variant.
+     *
+     * VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32.
+     * Thus, pa_range_info[].t0sz is translated to its ARM_32 variant using
+     * struct bitfields.
+     */
+    struct
+    {
+        signed int val:5;
+    } t0sz_32;
+#else
+
     /*
      * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured
      * with IPA bits == PA bits, compare against "pabits".
@@ -2291,6 +2299,7 @@ void __init setup_virt_paging(void)
      */
     if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
         max_vmid = MAX_VMID_16_BIT;
+#endif
 
     /* Choose suitable "pa_range" according to the resulted "p2m_ipa_bits". */
     for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
@@ -2304,26 +2313,41 @@ void __init setup_virt_paging(void)
 
     /* pa_range is 4 bits but we don't support all modes */
     if ( pa_range >= ARRAY_SIZE(pa_range_info) || !pa_range_info[pa_range].pabits )
-        panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", pa_range);
+    {
+        /*
+         * In case of ARM_64, we do not support this encoding of
+         * ID_AA64MMFR0_EL1.PARange
+         */
+        panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits);
+    }
 
+#ifdef CONFIG_ARM_64
     val |= VTCR_PS(pa_range);
     val |= VTCR_TG0_4K;
 
     /* Set the VS bit only if 16 bit VMID is supported. */
     if ( MAX_VMID == MAX_VMID_16_BIT )
         val |= VTCR_VS;
+#endif
+
     val |= VTCR_SL0(pa_range_info[pa_range].sl0);
     val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
 
     p2m_root_order = pa_range_info[pa_range].root_order;
     p2m_root_level = 2 - pa_range_info[pa_range].sl0;
+
+#ifdef CONFIG_ARM_64
     p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz;
+#else
+    t0sz_32.val = pa_range_info[pa_range].t0sz;
+    p2m_ipa_bits = 32 - t0sz_32.val;
+#endif
 
     printk("P2M: %d-bit IPA with %d-bit PA and %d-bit VMID\n",
            p2m_ipa_bits,
            pa_range_info[pa_range].pabits,
            ( MAX_VMID == MAX_VMID_16_BIT ) ? 16 : 8);
-#endif
+
     printk("P2M: %d levels with order-%d root, VTCR 0x%"PRIregister"\n",
            4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
 
-- 
2.17.1

[XEN v8 2/5] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Ayan Kumar Halder]

Some Arm based hardware platforms which does not support LPAE
(eg Cortex-R52), uses 32 bit physical addresses.
Also, users may choose to use 32 bits to represent physical addresses
for optimization.

To support the above use cases, we have introduced arch independent
config to choose if the physical address can be represented using
32 bits (PHYS_ADDR_T_32) or 64 bits (!PHYS_ADDR_T_32).
For now only ARM_32 provides support to enable 32 bit physical
addressing.

When PHYS_ADDR_T_32 is defined, PADDR_BITS is set to 32. Note that we
use "unsigned long" (not "uint32_t") to denote the datatype of physical
address. This is done to avoid using a cast each time PAGE_* macros are
used on paddr_t. For eg PAGE_SIZE is defined as unsigned long. Thus,
each time PAGE_SIZE is used with paddr_t, the result will be
"unsigned long".
On 32-bit architecture, "unsigned long" is 32-bit wide. Thus, it can be
used to denote physical address.

When PHYS_ADDR_T_32 is not defined for ARM_32, PADDR_BITS is set to 40.
For ARM_64, PADDR_BITS is set to 48.
The last two are same as the current configuration used today on Xen.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes from -
v1 - 1. Extracted from "[XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr".

v2 - 1. Introduced Kconfig choice. ARM_64 can select PHYS_ADDR_64 only whereas
ARM_32 can select PHYS_ADDR_32 or PHYS_ADDR_64.
2. For CONFIG_ARM_PA_32, paddr_t is defined as 'unsigned long'. 

v3 - 1. Allow user to define PADDR_BITS by selecting different config options
ARM_PA_BITS_32, ARM_PA_BITS_40 and ARM_PA_BITS_48.
2. Add the choice under "Architecture Features".

v4 - 1. Removed PHYS_ADDR_T_64 as !PHYS_ADDR_T_32 means PHYS_ADDR_T_32.

v5 - 1. Removed ARM_PA_BITS_48 as there is no choice for ARM_64.
2. In ARM_PA_BITS_32, "help" is moved to last, and "depends on" before "select".

v6 - 1. Explained why we use "unsigned long" to represent physical address
for ARM_32.

v7 - 1. Updated the reasoning for using "unsigned long" for paddr_t.
2. Added R-b by Michal.

 xen/arch/Kconfig                     |  3 +++
 xen/arch/arm/Kconfig                 | 33 ++++++++++++++++++++++++++++
 xen/arch/arm/include/asm/page-bits.h |  6 +----
 xen/arch/arm/include/asm/types.h     | 14 ++++++++++++
 xen/arch/arm/mm.c                    |  5 +++++
 5 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
index 7028f7b74f..67ba38f32f 100644
--- a/xen/arch/Kconfig
+++ b/xen/arch/Kconfig
@@ -1,6 +1,9 @@
 config 64BIT
 	bool
 
+config PHYS_ADDR_T_32
+	bool
+
 config NR_CPUS
 	int "Maximum number of CPUs"
 	range 1 4095
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 239d3aed3c..1e87fe0247 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -26,6 +26,39 @@ config ARCH_DEFCONFIG
 
 menu "Architecture Features"
 
+choice
+	prompt "Physical address space size" if ARM_32
+	default ARM_PA_BITS_40 if ARM_32
+	help
+	  User can choose to represent the width of physical address. This can
+	  sometimes help in optimizing the size of image when user chooses a
+	  smaller size to represent physical address.
+
+config ARM_PA_BITS_32
+	bool "32-bit"
+	depends on ARM_32
+	select PHYS_ADDR_T_32
+	help
+	  On platforms where any physical address can be represented within 32 bits,
+	  user should choose this option. This will help in reduced size of the
+	  binary.
+	  Xen uses "unsigned long" and not "uint32_t" to denote the datatype of
+	  physical address. This is done to avoid using a cast each time PAGE_*
+	  macros are used on paddr_t. For eg PAGE_SIZE is defined as unsigned long.
+	  On 32-bit architecture, "unsigned long" is 32-bit wide. Thus, it can be
+	  used to denote physical address.
+
+config ARM_PA_BITS_40
+	bool "40-bit"
+	depends on ARM_32
+endchoice
+
+config PADDR_BITS
+	int
+	default 32 if ARM_PA_BITS_32
+	default 40 if ARM_PA_BITS_40
+	default 48 if ARM_64
+
 source "arch/Kconfig"
 
 config ACPI
diff --git a/xen/arch/arm/include/asm/page-bits.h b/xen/arch/arm/include/asm/page-bits.h
index 5d6477e599..deb381ceeb 100644
--- a/xen/arch/arm/include/asm/page-bits.h
+++ b/xen/arch/arm/include/asm/page-bits.h
@@ -3,10 +3,6 @@
 
 #define PAGE_SHIFT              12
 
-#ifdef CONFIG_ARM_64
-#define PADDR_BITS              48
-#else
-#define PADDR_BITS              40
-#endif
+#define PADDR_BITS              CONFIG_PADDR_BITS
 
 #endif /* __ARM_PAGE_SHIFT_H__ */
diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h
index e218ed77bd..fb6618ef24 100644
--- a/xen/arch/arm/include/asm/types.h
+++ b/xen/arch/arm/include/asm/types.h
@@ -34,9 +34,23 @@ typedef signed long long s64;
 typedef unsigned long long u64;
 typedef u32 vaddr_t;
 #define PRIvaddr PRIx32
+#if defined(CONFIG_PHYS_ADDR_T_32)
+
+/*
+ * We use "unsigned long" and not "uint32_t" to denote the type. This is done
+ * to avoid having a cast each time PAGE_* macros are used on paddr_t. For eg
+ * PAGE_SIZE is defined as unsigned long.
+ * On 32-bit architecture, "unsigned long" is 32-bit wide. Thus, we can use it
+ * to denote physical address.
+ */
+typedef unsigned long paddr_t;
+#define INVALID_PADDR (~0UL)
+#define PRIpaddr "08lx"
+#else
 typedef u64 paddr_t;
 #define INVALID_PADDR (~0ULL)
 #define PRIpaddr "016llx"
+#endif
 typedef u32 register_t;
 #define PRIregister "08x"
 #elif defined (CONFIG_ARM_64)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 74f6ff2c6f..5ef5fd8c49 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -703,6 +703,11 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
     const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32);
     int rc;
 
+    /*
+     * The size of paddr_t should be sufficient for the complete range of
+     * physical address.
+     */
+    BUILD_BUG_ON((sizeof(paddr_t) * BITS_PER_BYTE) < PADDR_BITS);
     BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
 
     if ( frametable_size > FRAMETABLE_SIZE )
-- 
2.17.1

[XEN v8 3/5] xen/arm: guest_walk: LPAE specific bits should be enclosed within "ifndef CONFIG_PHYS_ADDR_T_32"

[Ayan Kumar Halder]

As the previous patch introduces CONFIG_PHYS_ADDR_T_32 to support 32 bit
physical addresses, the code specific to "Large Physical Address Extension"
(ie LPAE) should be enclosed within "ifndef CONFIG_PHYS_ADDR_T_32".

Refer xen/arch/arm/include/asm/short-desc.h, "short_desc_l1_supersec_t"
unsigned int extbase1:4;    /* Extended base address, PA[35:32] */
unsigned int extbase2:4;    /* Extended base address, PA[39:36] */

Thus, extbase1 and extbase2 are not valid when 32 bit physical addresses
are supported.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes from -
v1 - 1. Extracted from "[XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr".

v2 - 1. Reordered this patch so that it appears after CONFIG_ARM_PA_32 is
introduced (in 6/9).

v3 - 1. Updated the commit message.
2. Added Ack.

v4 - 1. No changes.

v5 - 1. No changes.

v6 - 1. No changes.

v7 - 1. No changes.

 xen/arch/arm/guest_walk.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 43d3215304..c80a0ce55b 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -154,8 +154,10 @@ static bool guest_walk_sd(const struct vcpu *v,
             mask = (1ULL << L1DESC_SUPERSECTION_SHIFT) - 1;
             *ipa = gva & mask;
             *ipa |= (paddr_t)(pte.supersec.base) << L1DESC_SUPERSECTION_SHIFT;
+#ifndef CONFIG_PHYS_ADDR_T_32
             *ipa |= (paddr_t)(pte.supersec.extbase1) << L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
             *ipa |= (paddr_t)(pte.supersec.extbase2) << L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
+#endif /* CONFIG_PHYS_ADDR_T_32 */
         }
 
         /* Set permissions so that the caller can check the flags by herself. */
-- 
2.17.1

[XEN v8 4/5] xen/arm: Restrict zeroeth_table_offset for ARM_64

[Ayan Kumar Halder]

When 32 bit physical addresses are used (ie PHYS_ADDR_T_32=y),
"va >> ZEROETH_SHIFT" causes an overflow.
Also, there is no zeroeth level page table on Arm32.

Also took the opportunity to clean up dump_pt_walk(). One could use
DECLARE_OFFSETS() macro instead of declaring an array of page table
offsets.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes from -

v1 - Removed the duplicate declaration for DECLARE_OFFSETS.

v2 - 1. Reworded the commit message. 
2. Use CONFIG_ARM_PA_32 to restrict zeroeth_table_offset.

v3 - 1. Added R-b and Ack.

v4 - 1. Removed R-b and Ack as we use CONFIG_PHYS_ADDR_T_32
instead of CONFIG_ARM_PA_BITS_32. This is to be in parity with our earlier
patches where we use CONFIG_PHYS_ADDR_T_32 to denote 32-bit physical addr
support.

v5 - 1. Added R-b and Ack.

v6 - 1. No changes.

v7 - 1. No changes.

 xen/arch/arm/include/asm/lpae.h | 4 ++++
 xen/arch/arm/mm.c               | 7 +------
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/include/asm/lpae.h b/xen/arch/arm/include/asm/lpae.h
index 3fdd5d0de2..7d2f6fd1bd 100644
--- a/xen/arch/arm/include/asm/lpae.h
+++ b/xen/arch/arm/include/asm/lpae.h
@@ -259,7 +259,11 @@ lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned int attr);
 #define first_table_offset(va)  TABLE_OFFSET(first_linear_offset(va))
 #define second_table_offset(va) TABLE_OFFSET(second_linear_offset(va))
 #define third_table_offset(va)  TABLE_OFFSET(third_linear_offset(va))
+#ifdef CONFIG_PHYS_ADDR_T_32
+#define zeroeth_table_offset(va)  0
+#else
 #define zeroeth_table_offset(va)  TABLE_OFFSET(zeroeth_linear_offset(va))
+#endif
 
 /*
  * Macros to define page-tables:
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 5ef5fd8c49..e460249736 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -233,12 +233,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
 {
     static const char *level_strs[4] = { "0TH", "1ST", "2ND", "3RD" };
     const mfn_t root_mfn = maddr_to_mfn(ttbr);
-    const unsigned int offsets[4] = {
-        zeroeth_table_offset(addr),
-        first_table_offset(addr),
-        second_table_offset(addr),
-        third_table_offset(addr)
-    };
+    DECLARE_OFFSETS(offsets, addr);
     lpae_t pte, *mapping;
     unsigned int level, root_table;
 
-- 
2.17.1

[XEN v8 5/5] xen/arm: p2m: Enable support for 32bit IPA for ARM_32

[Ayan Kumar Halder]

Refer ARM DDI 0406C.d ID040418, B3-1345,

"A stage 2 translation with an input address range of 31-34 bits can
start the translation either:

- With a first-level lookup, accessing a first-level translation
  table with 2-16 entries.

- With a second-level lookup, accessing a set of concatenated
  second-level translation tables"

Thus, for 32 bit IPA, there will be no concatenated root level tables.
So, the root-order is 0.

Also, Refer ARM DDI 0406C.d ID040418, B3-1348
"Determining the required first lookup level for stage 2 translations

For a stage 2 translation, the output address range from the stage 1
translations determines the required input address range for the stage 2
translation. The permitted values of VTCR.SL0 are:
0b00 Stage 2 translation lookup must start at the second level.
0b01 Stage 2 translation lookup must start at the first level.

VTCR.T0SZ must indicate the required input address range. The size of
the input address region is 2^(32-T0SZ) bytes."

Thus VTCR.SL0 = 1 (maximum value) and VTCR.T0SZ = 0 when the size of
input address region is 2^32 bytes.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from -

v1 - New patch.

v2 - 1. Added Ack.

v3 - 1. Dropped Ack. 
2. Rebased the patch based on the previous change.

v4 - 1. t0sz is 0 for 32-bit IPA on Arm32.
2. Updated the commit message to explain t0sz, sl0 and root_order.

v5 - 1. Rebased on top of the changes in the previous patch.

v6 - 1. Removed the index for ARM_32.

v7 - 1. No changes.

 xen/arch/arm/p2m.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 76388ba54b..a969068a68 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -2265,6 +2265,7 @@ void __init setup_virt_paging(void)
         [6] = { 52,      12/*12*/,  4,          2 },
         [7] = { 0 }  /* Invalid */
 #else
+        { 32,      0/*0*/,    0,          1 },
         { 40,      24/*24*/,  1,          1 }
 #endif
     };
-- 
2.17.1

Re: [XEN v8 1/5] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Michal Orzel]

Hi Ayan,

On 02/06/2023 14:07, Ayan Kumar Halder wrote:
> 
> 
> Restructure the code so that one can use pa_range_info[] table for both
> ARM_32 as well as ARM_64.
I grepped for ARM_{32,64} in our code base and could not find any use in source files except for things
introduced by this commit. While I'm ok with it in a commit message I think for consistency we should be
using arm32/arm64 in the code.

> 
> Also, removed the hardcoding for P2M_ROOT_ORDER and P2M_ROOT_LEVEL as
> p2m_root_order can be obtained from the pa_range_info[].root_order and
> p2m_root_level can be obtained from pa_range_info[].sl0.
> 
> Refer ARM DDI 0406C.d ID040418, B3-1345,
> "Use of concatenated first-level translation tables
> 
> ...However, a 40-bit input address range with a translation granularity of 4KB
> requires a total of 28 bits of address resolution. Therefore, a stage 2
> translation that supports a 40-bit input address range requires two concatenated
> first-level translation tables,..."
> 
> Thus, root-order is 1 for 40-bit IPA on ARM_32.
> 
> Refer ARM DDI 0406C.d ID040418, B3-1348,
> 
> "Determining the required first lookup level for stage 2 translations
> 
> For a stage 2 translation, the output address range from the stage 1
> translations determines the required input address range for the stage 2
> translation. The permitted values of VTCR.SL0 are:
> 
> 0b00 Stage 2 translation lookup must start at the second level.
> 0b01 Stage 2 translation lookup must start at the first level.
> 
> VTCR.T0SZ must indicate the required input address range. The size of the input
> address region is 2^(32-T0SZ) bytes."
> 
> Thus VTCR.SL0 = 1 (maximum value) and VTCR.T0SZ = -8 when the size of input
> address region is 2^40 bytes.
> 
> Thus, pa_range_info[].t0sz = 1 (VTCR.S) | 8 (VTCR.T0SZ) ie 11000b which is 24.
> 
> VTCR.T0SZ, is bits [5:0] for ARM_64.
> VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32.
> 
> For this, we have used struct bitfields to convert pa_range_info[].t0sz to its
> ARM_32 variant.
> 
> pa_range_info[] is indexed by ID_AA64MMFR0_EL1.PARange which is present in Arm64
> only. This is the reason we do not specify the indices for ARM_32. Also, we
> duplicated the entry "{ 40,      24/*24*/,  1,          1 }" between ARM_64 and
> ARM_32. This is done to avoid introducing extra #if-defs.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from -
> 
> v3 - 1. New patch introduced in v4.
> 2. Restructure the code such that pa_range_info[] is used both by ARM_32 as
> well as ARM_64.
> 
> v4 - 1. Removed the hardcoded definitions of P2M_ROOT_ORDER and P2M_ROOT_LEVEL.
> The reason being root_order will not be always 1 (See the next patch).
> 2. Updated the commit message to explain t0sz, sl0 and root_order values for
> 32-bit IPA on Arm32.
> 3. Some sanity fixes.
> 
> v5 - pa_range_info is indexed by system_cpuinfo.mm64.pa_range. ie
> when PARange is 0, the PA size is 32, 1 -> 36 and so on. So pa_range_info[] has
> been updated accordingly.
> For ARM_32 pa_range_info[0] = 0 and pa_range_info[1] = 0 as we do not support
> 32-bit, 36-bit physical address range yet.
> 
> v6 - 1. Added pa_range_info[] entries for ARM_32 without indices. Some entry
> may be duplicated between ARM_64 and ARM_32.
> 2. Recalculate p2m_ipa_bits for ARM_32 from T0SZ (similar to ARM_64).
> 3. Introduced an union to reinterpret T0SZ bits between ARM_32 and ARM_64.
> 
> v7 - 1. Used struct bifield instead of union to reinterpret T0SZ bits between
> ARM_32 and ARM_64.
> 2. Removed the invalid entry for ARM_32.
> 
>  xen/arch/arm/include/asm/p2m.h |  6 ----
>  xen/arch/arm/p2m.c             | 50 +++++++++++++++++++++++++---------
>  2 files changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> index f67e9ddc72..940495d42b 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -14,16 +14,10 @@
>  /* Holds the bit size of IPAs in p2m tables.  */
>  extern unsigned int p2m_ipa_bits;
> 
> -#ifdef CONFIG_ARM_64
>  extern unsigned int p2m_root_order;
>  extern unsigned int p2m_root_level;
>  #define P2M_ROOT_ORDER    p2m_root_order
>  #define P2M_ROOT_LEVEL p2m_root_level
> -#else
> -/* First level P2M is always 2 consecutive pages */
> -#define P2M_ROOT_ORDER    1
> -#define P2M_ROOT_LEVEL 1
> -#endif
> 
>  struct domain;
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 418997843d..76388ba54b 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -19,9 +19,9 @@
> 
>  #define INVALID_VMID 0 /* VMID 0 is reserved */
> 
> -#ifdef CONFIG_ARM_64
>  unsigned int __read_mostly p2m_root_order;
>  unsigned int __read_mostly p2m_root_level;
> +#ifdef CONFIG_ARM_64
>  static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT;
>  /* VMID is by default 8 bit width on AArch64 */
>  #define MAX_VMID       max_vmid
> @@ -2247,16 +2247,6 @@ void __init setup_virt_paging(void)
>      /* Setup Stage 2 address translation */
>      register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
> 
> -#ifdef CONFIG_ARM_32
> -    if ( p2m_ipa_bits < 40 )
> -        panic("P2M: Not able to support %u-bit IPA at the moment\n",
> -              p2m_ipa_bits);
> -
> -    printk("P2M: 40-bit IPA\n");
> -    p2m_ipa_bits = 40;
> -    val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
> -    val |= VTCR_SL0(0x1); /* P2M starts at first level */
> -#else /* CONFIG_ARM_64 */
>      static const struct {
>          unsigned int pabits; /* Physical Address Size */
>          unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
> @@ -2265,6 +2255,7 @@ void __init setup_virt_paging(void)
>      } pa_range_info[] __initconst = {
>          /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
>          /*      PA size, t0sz(min), root-order, sl0(max) */
> +#ifdef CONFIG_ARM_64
>          [0] = { 32,      32/*32*/,  0,          1 },
>          [1] = { 36,      28/*28*/,  0,          1 },
>          [2] = { 40,      24/*24*/,  1,          1 },
> @@ -2273,11 +2264,28 @@ void __init setup_virt_paging(void)
>          [5] = { 48,      16/*16*/,  0,          2 },
>          [6] = { 52,      12/*12*/,  4,          2 },
>          [7] = { 0 }  /* Invalid */
> +#else
> +        { 40,      24/*24*/,  1,          1 }
> +#endif
>      };
> 
>      unsigned int i;
>      unsigned int pa_range = 0x10; /* Larger than any possible value */
> 
> +#ifdef CONFIG_ARM_32
> +    /*
> +     * Typecast pa_range_info[].t0sz into ARM_32 bit variant.
> +     *
> +     * VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32.
> +     * Thus, pa_range_info[].t0sz is translated to its ARM_32 variant using
> +     * struct bitfields.
> +     */
> +    struct
> +    {
> +        signed int val:5;
> +    } t0sz_32;
> +#else
> +
no need for this empty line

>      /*
>       * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured
>       * with IPA bits == PA bits, compare against "pabits".
> @@ -2291,6 +2299,7 @@ void __init setup_virt_paging(void)
>       */
>      if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
>          max_vmid = MAX_VMID_16_BIT;
> +#endif
> 
>      /* Choose suitable "pa_range" according to the resulted "p2m_ipa_bits". */
>      for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> @@ -2304,26 +2313,41 @@ void __init setup_virt_paging(void)
> 
>      /* pa_range is 4 bits but we don't support all modes */
this comment makes sense really only on arm64 as it refers to PARange field of ID_AA64MMFR0_EL1.

>      if ( pa_range >= ARRAY_SIZE(pa_range_info) || !pa_range_info[pa_range].pabits )
> -        panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", pa_range);
> +    {
> +        /*
> +         * In case of ARM_64, we do not support this encoding of
> +         * ID_AA64MMFR0_EL1.PARange
> +         */
> +        panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits);
NIT: Putting variable names in messages visible by users is not a great idea IMO.
"Unsupported IPA size" would read better. Furthermore, I do not think printing IPA size in hex
is beneficial. I would use "%u bits" (i.e. 32 bits reads better than 0x20 bits).

> +    }
> 
> +#ifdef CONFIG_ARM_64
>      val |= VTCR_PS(pa_range);
>      val |= VTCR_TG0_4K;
> 
>      /* Set the VS bit only if 16 bit VMID is supported. */
>      if ( MAX_VMID == MAX_VMID_16_BIT )
>          val |= VTCR_VS;
> +#endif
> +
>      val |= VTCR_SL0(pa_range_info[pa_range].sl0);
>      val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
> 
>      p2m_root_order = pa_range_info[pa_range].root_order;
>      p2m_root_level = 2 - pa_range_info[pa_range].sl0;
> +
> +#ifdef CONFIG_ARM_64
>      p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz;
> +#else
> +    t0sz_32.val = pa_range_info[pa_range].t0sz;
> +    p2m_ipa_bits = 32 - t0sz_32.val;
> +#endif
> 
>      printk("P2M: %d-bit IPA with %d-bit PA and %d-bit VMID\n",
>             p2m_ipa_bits,
>             pa_range_info[pa_range].pabits,
>             ( MAX_VMID == MAX_VMID_16_BIT ) ? 16 : 8);
> -#endif
> +
>      printk("P2M: %d levels with order-%d root, VTCR 0x%"PRIregister"\n",
>             4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
> 
> --
> 2.17.1
> 
> 

~Michal

Re: [XEN v8 5/5] xen/arm: p2m: Enable support for 32bit IPA for ARM_32

[Michal Orzel]

On 02/06/2023 14:07, Ayan Kumar Halder wrote:
> 
> 
> Refer ARM DDI 0406C.d ID040418, B3-1345,
> 
> "A stage 2 translation with an input address range of 31-34 bits can
> start the translation either:
> 
> - With a first-level lookup, accessing a first-level translation
>   table with 2-16 entries.
> 
> - With a second-level lookup, accessing a set of concatenated
>   second-level translation tables"
> 
> Thus, for 32 bit IPA, there will be no concatenated root level tables.
> So, the root-order is 0.
> 
> Also, Refer ARM DDI 0406C.d ID040418, B3-1348
> "Determining the required first lookup level for stage 2 translations
> 
> For a stage 2 translation, the output address range from the stage 1
> translations determines the required input address range for the stage 2
> translation. The permitted values of VTCR.SL0 are:
> 0b00 Stage 2 translation lookup must start at the second level.
> 0b01 Stage 2 translation lookup must start at the first level.
> 
> VTCR.T0SZ must indicate the required input address range. The size of
> the input address region is 2^(32-T0SZ) bytes."
> 
> Thus VTCR.SL0 = 1 (maximum value) and VTCR.T0SZ = 0 when the size of
> input address region is 2^32 bytes.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [XEN v8 1/5] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Ayan Kumar Halder]

On 15/06/2023 09:05, Michal Orzel wrote:
> Hi Ayan,
Hi Michal,
>
> On 02/06/2023 14:07, Ayan Kumar Halder wrote:
>>
>> Restructure the code so that one can use pa_range_info[] table for both
>> ARM_32 as well as ARM_64.
> I grepped for ARM_{32,64} in our code base and could not find any use in source files except for things
> introduced by this commit. While I'm ok with it in a commit message I think for consistency we should be
> using arm32/arm64 in the code.

AFAIU, arm32/arm64 refers to the Architecture. ARM_32/ARM_64 refers to 
the configuration.

If you see the original code (xen/arch/arm/include/asm/p2m.h, 
xen/arch/arm/p2m.c)

ARM_32/ARM_64 has been used.

Thus, I used ARM_32/ARM_64 in this commit. Let me know if it makes sense.
The rest of your comments look sane to me.

- Ayan

>
>> Also, removed the hardcoding for P2M_ROOT_ORDER and P2M_ROOT_LEVEL as
>> p2m_root_order can be obtained from the pa_range_info[].root_order and
>> p2m_root_level can be obtained from pa_range_info[].sl0.
>>
>> Refer ARM DDI 0406C.d ID040418, B3-1345,
>> "Use of concatenated first-level translation tables
>>
>> ...However, a 40-bit input address range with a translation granularity of 4KB
>> requires a total of 28 bits of address resolution. Therefore, a stage 2
>> translation that supports a 40-bit input address range requires two concatenated
>> first-level translation tables,..."
>>
>> Thus, root-order is 1 for 40-bit IPA on ARM_32.
>>
>> Refer ARM DDI 0406C.d ID040418, B3-1348,
>>
>> "Determining the required first lookup level for stage 2 translations
>>
>> For a stage 2 translation, the output address range from the stage 1
>> translations determines the required input address range for the stage 2
>> translation. The permitted values of VTCR.SL0 are:
>>
>> 0b00 Stage 2 translation lookup must start at the second level.
>> 0b01 Stage 2 translation lookup must start at the first level.
>>
>> VTCR.T0SZ must indicate the required input address range. The size of the input
>> address region is 2^(32-T0SZ) bytes."
>>
>> Thus VTCR.SL0 = 1 (maximum value) and VTCR.T0SZ = -8 when the size of input
>> address region is 2^40 bytes.
>>
>> Thus, pa_range_info[].t0sz = 1 (VTCR.S) | 8 (VTCR.T0SZ) ie 11000b which is 24.
>>
>> VTCR.T0SZ, is bits [5:0] for ARM_64.
>> VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32.
>>
>> For this, we have used struct bitfields to convert pa_range_info[].t0sz to its
>> ARM_32 variant.
>>
>> pa_range_info[] is indexed by ID_AA64MMFR0_EL1.PARange which is present in Arm64
>> only. This is the reason we do not specify the indices for ARM_32. Also, we
>> duplicated the entry "{ 40,      24/*24*/,  1,          1 }" between ARM_64 and
>> ARM_32. This is done to avoid introducing extra #if-defs.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> Changes from -
>>
>> v3 - 1. New patch introduced in v4.
>> 2. Restructure the code such that pa_range_info[] is used both by ARM_32 as
>> well as ARM_64.
>>
>> v4 - 1. Removed the hardcoded definitions of P2M_ROOT_ORDER and P2M_ROOT_LEVEL.
>> The reason being root_order will not be always 1 (See the next patch).
>> 2. Updated the commit message to explain t0sz, sl0 and root_order values for
>> 32-bit IPA on Arm32.
>> 3. Some sanity fixes.
>>
>> v5 - pa_range_info is indexed by system_cpuinfo.mm64.pa_range. ie
>> when PARange is 0, the PA size is 32, 1 -> 36 and so on. So pa_range_info[] has
>> been updated accordingly.
>> For ARM_32 pa_range_info[0] = 0 and pa_range_info[1] = 0 as we do not support
>> 32-bit, 36-bit physical address range yet.
>>
>> v6 - 1. Added pa_range_info[] entries for ARM_32 without indices. Some entry
>> may be duplicated between ARM_64 and ARM_32.
>> 2. Recalculate p2m_ipa_bits for ARM_32 from T0SZ (similar to ARM_64).
>> 3. Introduced an union to reinterpret T0SZ bits between ARM_32 and ARM_64.
>>
>> v7 - 1. Used struct bifield instead of union to reinterpret T0SZ bits between
>> ARM_32 and ARM_64.
>> 2. Removed the invalid entry for ARM_32.
>>
>>   xen/arch/arm/include/asm/p2m.h |  6 ----
>>   xen/arch/arm/p2m.c             | 50 +++++++++++++++++++++++++---------
>>   2 files changed, 37 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
>> index f67e9ddc72..940495d42b 100644
>> --- a/xen/arch/arm/include/asm/p2m.h
>> +++ b/xen/arch/arm/include/asm/p2m.h
>> @@ -14,16 +14,10 @@
>>   /* Holds the bit size of IPAs in p2m tables.  */
>>   extern unsigned int p2m_ipa_bits;
>>
>> -#ifdef CONFIG_ARM_64
>>   extern unsigned int p2m_root_order;
>>   extern unsigned int p2m_root_level;
>>   #define P2M_ROOT_ORDER    p2m_root_order
>>   #define P2M_ROOT_LEVEL p2m_root_level
>> -#else
>> -/* First level P2M is always 2 consecutive pages */
>> -#define P2M_ROOT_ORDER    1
>> -#define P2M_ROOT_LEVEL 1
>> -#endif
>>
>>   struct domain;
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 418997843d..76388ba54b 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -19,9 +19,9 @@
>>
>>   #define INVALID_VMID 0 /* VMID 0 is reserved */
>>
>> -#ifdef CONFIG_ARM_64
>>   unsigned int __read_mostly p2m_root_order;
>>   unsigned int __read_mostly p2m_root_level;
>> +#ifdef CONFIG_ARM_64
>>   static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT;
>>   /* VMID is by default 8 bit width on AArch64 */
>>   #define MAX_VMID       max_vmid
>> @@ -2247,16 +2247,6 @@ void __init setup_virt_paging(void)
>>       /* Setup Stage 2 address translation */
>>       register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
>>
>> -#ifdef CONFIG_ARM_32
>> -    if ( p2m_ipa_bits < 40 )
>> -        panic("P2M: Not able to support %u-bit IPA at the moment\n",
>> -              p2m_ipa_bits);
>> -
>> -    printk("P2M: 40-bit IPA\n");
>> -    p2m_ipa_bits = 40;
>> -    val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>> -    val |= VTCR_SL0(0x1); /* P2M starts at first level */
>> -#else /* CONFIG_ARM_64 */
>>       static const struct {
>>           unsigned int pabits; /* Physical Address Size */
>>           unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
>> @@ -2265,6 +2255,7 @@ void __init setup_virt_paging(void)
>>       } pa_range_info[] __initconst = {
>>           /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
>>           /*      PA size, t0sz(min), root-order, sl0(max) */
>> +#ifdef CONFIG_ARM_64
>>           [0] = { 32,      32/*32*/,  0,          1 },
>>           [1] = { 36,      28/*28*/,  0,          1 },
>>           [2] = { 40,      24/*24*/,  1,          1 },
>> @@ -2273,11 +2264,28 @@ void __init setup_virt_paging(void)
>>           [5] = { 48,      16/*16*/,  0,          2 },
>>           [6] = { 52,      12/*12*/,  4,          2 },
>>           [7] = { 0 }  /* Invalid */
>> +#else
>> +        { 40,      24/*24*/,  1,          1 }
>> +#endif
>>       };
>>
>>       unsigned int i;
>>       unsigned int pa_range = 0x10; /* Larger than any possible value */
>>
>> +#ifdef CONFIG_ARM_32
>> +    /*
>> +     * Typecast pa_range_info[].t0sz into ARM_32 bit variant.
>> +     *
>> +     * VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32.
>> +     * Thus, pa_range_info[].t0sz is translated to its ARM_32 variant using
>> +     * struct bitfields.
>> +     */
>> +    struct
>> +    {
>> +        signed int val:5;
>> +    } t0sz_32;
>> +#else
>> +
> no need for this empty line
>
>>       /*
>>        * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured
>>        * with IPA bits == PA bits, compare against "pabits".
>> @@ -2291,6 +2299,7 @@ void __init setup_virt_paging(void)
>>        */
>>       if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
>>           max_vmid = MAX_VMID_16_BIT;
>> +#endif
>>
>>       /* Choose suitable "pa_range" according to the resulted "p2m_ipa_bits". */
>>       for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
>> @@ -2304,26 +2313,41 @@ void __init setup_virt_paging(void)
>>
>>       /* pa_range is 4 bits but we don't support all modes */
> this comment makes sense really only on arm64 as it refers to PARange field of ID_AA64MMFR0_EL1.
>
>>       if ( pa_range >= ARRAY_SIZE(pa_range_info) || !pa_range_info[pa_range].pabits )
>> -        panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", pa_range);
>> +    {
>> +        /*
>> +         * In case of ARM_64, we do not support this encoding of
>> +         * ID_AA64MMFR0_EL1.PARange
>> +         */
>> +        panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits);
> NIT: Putting variable names in messages visible by users is not a great idea IMO.
> "Unsupported IPA size" would read better. Furthermore, I do not think printing IPA size in hex
> is beneficial. I would use "%u bits" (i.e. 32 bits reads better than 0x20 bits).
>
>> +    }
>>
>> +#ifdef CONFIG_ARM_64
>>       val |= VTCR_PS(pa_range);
>>       val |= VTCR_TG0_4K;
>>
>>       /* Set the VS bit only if 16 bit VMID is supported. */
>>       if ( MAX_VMID == MAX_VMID_16_BIT )
>>           val |= VTCR_VS;
>> +#endif
>> +
>>       val |= VTCR_SL0(pa_range_info[pa_range].sl0);
>>       val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
>>
>>       p2m_root_order = pa_range_info[pa_range].root_order;
>>       p2m_root_level = 2 - pa_range_info[pa_range].sl0;
>> +
>> +#ifdef CONFIG_ARM_64
>>       p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz;
>> +#else
>> +    t0sz_32.val = pa_range_info[pa_range].t0sz;
>> +    p2m_ipa_bits = 32 - t0sz_32.val;
>> +#endif
>>
>>       printk("P2M: %d-bit IPA with %d-bit PA and %d-bit VMID\n",
>>              p2m_ipa_bits,
>>              pa_range_info[pa_range].pabits,
>>              ( MAX_VMID == MAX_VMID_16_BIT ) ? 16 : 8);
>> -#endif
>> +
>>       printk("P2M: %d levels with order-%d root, VTCR 0x%"PRIregister"\n",
>>              4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
>>
>> --
>> 2.17.1
>>
>>
> ~Michal

Re: [XEN v8 1/5] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Julien Grall]

Hi Ayan,

On 15/06/2023 10:29, Ayan Kumar Halder wrote:
> 
> On 15/06/2023 09:05, Michal Orzel wrote:
>> Hi Ayan,
> Hi Michal,
>>
>> On 02/06/2023 14:07, Ayan Kumar Halder wrote:
>>>
>>> Restructure the code so that one can use pa_range_info[] table for both
>>> ARM_32 as well as ARM_64.
>> I grepped for ARM_{32,64} in our code base and could not find any use 
>> in source files except for things
>> introduced by this commit. While I'm ok with it in a commit message I 
>> think for consistency we should be
>> using arm32/arm64 in the code.
> 
> AFAIU, arm32/arm64 refers to the Architecture. ARM_32/ARM_64 refers to 
> the configuration.
> 
> If you see the original code (xen/arch/arm/include/asm/p2m.h, 
> xen/arch/arm/p2m.c)
> 
> ARM_32/ARM_64 has been used.
> 
> Thus, I used ARM_32/ARM_64 in this commit. Let me know if it makes sense.
> The rest of your comments look sane to me.
In text, we commonly don't use the name of the config. Instead we use 
the name of the architecture (i.e. arm32/arm64) because this is a strict 
correspondence.

I agree with Michal, about using arm32/arm64 rather than ARM_32/ARM_64 
in the comments at least to stay consistent with the rest of the code.

Assuming there is no other changes required in this patch (I haven't 
looked at it yet), then I am happy to handle this request on commit if 
you are OK with it.

Cheers,

-- 
Julien Grall

Re: [XEN v8 1/5] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Ayan Kumar Halder]

On 15/06/2023 10:40, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
>
> On 15/06/2023 10:29, Ayan Kumar Halder wrote:
>>
>> On 15/06/2023 09:05, Michal Orzel wrote:
>>> Hi Ayan,
>> Hi Michal,
>>>
>>> On 02/06/2023 14:07, Ayan Kumar Halder wrote:
>>>>
>>>> Restructure the code so that one can use pa_range_info[] table for 
>>>> both
>>>> ARM_32 as well as ARM_64.
>>> I grepped for ARM_{32,64} in our code base and could not find any 
>>> use in source files except for things
>>> introduced by this commit. While I'm ok with it in a commit message 
>>> I think for consistency we should be
>>> using arm32/arm64 in the code.
>>
>> AFAIU, arm32/arm64 refers to the Architecture. ARM_32/ARM_64 refers 
>> to the configuration.
>>
>> If you see the original code (xen/arch/arm/include/asm/p2m.h, 
>> xen/arch/arm/p2m.c)
>>
>> ARM_32/ARM_64 has been used.
>>
>> Thus, I used ARM_32/ARM_64 in this commit. Let me know if it makes 
>> sense.
>> The rest of your comments look sane to me.
> In text, we commonly don't use the name of the config. Instead we use 
> the name of the architecture (i.e. arm32/arm64) because this is a 
> strict correspondence.
>
> I agree with Michal, about using arm32/arm64 rather than ARM_32/ARM_64 
> in the comments at least to stay consistent with the rest of the code.
I see what you mean.
>
> Assuming there is no other changes required in this patch (I haven't 
> looked at it yet), then I am happy to handle this request on commit if 
> you are OK with it.

I see that the rest of the comments from Michal are related to style (ie 
no functional changes). Thus, I am happy for you to fix them on commit.

- Ayan

>
> Cheers,
>

Re: [XEN v8 2/5] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Julien Grall]

Hi Ayan,

On 02/06/2023 13:07, Ayan Kumar Halder wrote:
> Some Arm based hardware platforms which does not support LPAE
> (eg Cortex-R52), uses 32 bit physical addresses.
> Also, users may choose to use 32 bits to represent physical addresses
> for optimization.
> 
> To support the above use cases, we have introduced arch independent
> config to choose if the physical address can be represented using
> 32 bits (PHYS_ADDR_T_32) or 64 bits (!PHYS_ADDR_T_32).
> For now only ARM_32 provides support to enable 32 bit physical
> addressing.
> 
> When PHYS_ADDR_T_32 is defined, PADDR_BITS is set to 32. Note that we
> use "unsigned long" (not "uint32_t") to denote the datatype of physical
> address. This is done to avoid using a cast each time PAGE_* macros are
> used on paddr_t. For eg PAGE_SIZE is defined as unsigned long. Thus,
> each time PAGE_SIZE is used with paddr_t, the result will be
> "unsigned long".
> On 32-bit architecture, "unsigned long" is 32-bit wide. Thus, it can be
> used to denote physical address.
> 
> When PHYS_ADDR_T_32 is not defined for ARM_32, PADDR_BITS is set to 40.
> For ARM_64, PADDR_BITS is set to 48.
> The last two are same as the current configuration used today on Xen.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [XEN v8 5/5] xen/arm: p2m: Enable support for 32bit IPA for ARM_32

[Julien Grall]

Hi Ayan,

On 02/06/2023 13:07, Ayan Kumar Halder wrote:
> Refer ARM DDI 0406C.d ID040418, B3-1345,
> 
> "A stage 2 translation with an input address range of 31-34 bits can
> start the translation either:
> 
> - With a first-level lookup, accessing a first-level translation
>    table with 2-16 entries.
> 
> - With a second-level lookup, accessing a set of concatenated
>    second-level translation tables"
> 
> Thus, for 32 bit IPA, there will be no concatenated root level tables.
> So, the root-order is 0.
> 
> Also, Refer ARM DDI 0406C.d ID040418, B3-1348
> "Determining the required first lookup level for stage 2 translations
> 
> For a stage 2 translation, the output address range from the stage 1
> translations determines the required input address range for the stage 2
> translation. The permitted values of VTCR.SL0 are:
> 0b00 Stage 2 translation lookup must start at the second level.
> 0b01 Stage 2 translation lookup must start at the first level.
> 
> VTCR.T0SZ must indicate the required input address range. The size of
> the input address region is 2^(32-T0SZ) bytes."
> 
> Thus VTCR.SL0 = 1 (maximum value) and VTCR.T0SZ = 0 when the size of
> input address region is 2^32 bytes.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [XEN v8 1/5] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Julien Grall]

Hi Michal,

I notice you posted some comments but didn't add a Acked-by/Reviewed-by. 
Can you indicate if you are happy with the patch so long your comments 
are addressed?

If so, are you OK if I deal with them on commit?

Cheers,

-- 
Julien Grall

Re: [XEN v8 1/5] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Michal Orzel]

Hi Julien,

On 15/06/2023 22:32, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> I notice you posted some comments but didn't add a Acked-by/Reviewed-by.
> Can you indicate if you are happy with the patch so long your comments
> are addressed?
> 
> If so, are you OK if I deal with them on commit?
I thought I added my tag but clearly not. With the remarks addressed on commit:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [XEN v8 1/5] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Julien Grall]

Hi,

On 15/06/2023 09:05, Michal Orzel wrote:
>>       /*
>>        * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured
>>        * with IPA bits == PA bits, compare against "pabits".
>> @@ -2291,6 +2299,7 @@ void __init setup_virt_paging(void)
>>        */
>>       if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
>>           max_vmid = MAX_VMID_16_BIT;
>> +#endif
>>
>>       /* Choose suitable "pa_range" according to the resulted "p2m_ipa_bits". */
>>       for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
>> @@ -2304,26 +2313,41 @@ void __init setup_virt_paging(void)
>>
>>       /* pa_range is 4 bits but we don't support all modes */
> this comment makes sense really only on arm64 as it refers to PARange field of ID_AA64MMFR0_EL1.
> 
>>       if ( pa_range >= ARRAY_SIZE(pa_range_info) || !pa_range_info[pa_range].pabits )
>> -        panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", pa_range);
>> +    {
>> +        /*
>> +         * In case of ARM_64, we do not support this encoding of
>> +         * ID_AA64MMFR0_EL1.PARange
>> +         */
>> +        panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits);
> NIT: Putting variable names in messages visible by users is not a great idea IMO.
> "Unsupported IPA size" would read better. Furthermore, I do not think printing IPA size in hex
> is beneficial. I would use "%u bits" (i.e. 32 bits reads better than 0x20 bits).

I went with the following:

-    /* pa_range is 4 bits but we don't support all modes */
+    /* Check if we found the associated entry in the array */
      if ( pa_range >= ARRAY_SIZE(pa_range_info) || 
!pa_range_info[pa_range].pabits )
-    {
-        /*
-         * In case of ARM_64, we do not support this encoding of
-         * ID_AA64MMFR0_EL1.PARange
-         */
-        panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits);
-    }
+        panic("%-bit P2M is not supported\n", p2m_ipa_bits);

This should be generic enough.

-- 
Julien Grall

Re: [XEN v8 1/5] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Julien Grall]

On 16/06/2023 08:59, Michal Orzel wrote:
> Hi Julien,
> 
> On 15/06/2023 22:32, Julien Grall wrote:
>>
>>
>> Hi Michal,
>>
>> I notice you posted some comments but didn't add a Acked-by/Reviewed-by.
>> Can you indicate if you are happy with the patch so long your comments
>> are addressed?
>>
>> If so, are you OK if I deal with them on commit?
> I thought I added my tag but clearly not. With the remarks addressed on commit:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks. The series is now committed.

Cheers,

-- 
Julien Grall