[XEN v4 00/11] Add support for 32 bit physical address

[XEN v4 00/11] 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 current patch serie depends on :-
"[XEN v5] xen/arm: Use the correct format specifier"
https://lists.xenproject.org/archives/html/xen-devel/2023-01/msg01896.html
I did not send out the patch again as it has already been reviewed and acked and
is waiting to be committed to staging.

The following points are to be noted :-
1. Device tree always use u64 for address and size. The caller needs to
translate between u64 and u32 (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.
3. https://lists.xenproject.org/archives/html/xen-devel/2022-12/msg00117.html
has been added to this series.

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.

Ayan Kumar Halder (11):
  xen/arm: Use the correct format specifier
  xen/arm: domain_build: Track unallocated pages using the frame number
  xen/arm: Typecast the DT values into paddr_t
  xen/drivers: ns16550: Use paddr_t for io_base/io_size
  xen/arm: Introduce a wrapper for dt_device_get_address() to handle
    paddr_t
  xen/arm: smmu: Use writeq_relaxed_non_atomic() for writing to
    SMMU_CBn_TTBR0
  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: Use the pa_range_info table to support Arm_32 and Arm_64
  xen/arm: p2m: Enable support for 32bit IPA for ARM_32

 xen/arch/Kconfig                           |   6 ++
 xen/arch/arm/Kconfig                       |  40 +++++++-
 xen/arch/arm/bootfdt.c                     |  41 ++++++--
 xen/arch/arm/domain_build.c                | 103 +++++++++++++--------
 xen/arch/arm/gic-v2.c                      |  16 ++--
 xen/arch/arm/gic-v3-its.c                  |   4 +-
 xen/arch/arm/gic-v3.c                      |  10 +-
 xen/arch/arm/guest_walk.c                  |   2 +
 xen/arch/arm/include/asm/lpae.h            |   4 +
 xen/arch/arm/include/asm/page-bits.h       |   6 +-
 xen/arch/arm/include/asm/setup.h           |   4 +-
 xen/arch/arm/include/asm/types.h           |   6 ++
 xen/arch/arm/mm.c                          |  10 +-
 xen/arch/arm/p2m.c                         |  29 +++---
 xen/arch/arm/pci/pci-host-common.c         |   6 +-
 xen/arch/arm/platforms/brcm-raspberry-pi.c |   2 +-
 xen/arch/arm/platforms/brcm.c              |   4 +-
 xen/arch/arm/platforms/exynos5.c           |  32 +++----
 xen/arch/arm/platforms/sunxi.c             |   2 +-
 xen/arch/arm/platforms/xgene-storm.c       |   2 +-
 xen/arch/arm/setup.c                       |  14 +--
 xen/arch/arm/smpboot.c                     |   2 +-
 xen/common/device_tree.c                   |  40 +++++++-
 xen/drivers/char/cadence-uart.c            |   4 +-
 xen/drivers/char/exynos4210-uart.c         |   4 +-
 xen/drivers/char/imx-lpuart.c              |   4 +-
 xen/drivers/char/meson-uart.c              |   4 +-
 xen/drivers/char/mvebu-uart.c              |   4 +-
 xen/drivers/char/ns16550.c                 |  43 ++++++---
 xen/drivers/char/omap-uart.c               |   4 +-
 xen/drivers/char/pl011.c                   |   6 +-
 xen/drivers/char/scif-uart.c               |   4 +-
 xen/drivers/passthrough/arm/ipmmu-vmsa.c   |   8 +-
 xen/drivers/passthrough/arm/smmu-v3.c      |   2 +-
 xen/drivers/passthrough/arm/smmu.c         |  23 ++---
 xen/include/xen/device_tree.h              |  27 +++++-
 xen/include/xen/libfdt/libfdt-xen.h        |  52 +++++++++++
 37 files changed, 397 insertions(+), 177 deletions(-)
 create mode 100644 xen/include/xen/libfdt/libfdt-xen.h

-- 
2.17.1

[XEN v4 01/11] xen/arm: Use the correct format specifier

[Ayan Kumar Halder]

1. One should use 'PRIpaddr' to display 'paddr_t' variables. However,
while creating nodes in fdt, the address (if present in the node name)
should be represented using 'PRIx64'. This is to be in conformance
with the following rule present in https://elinux.org/Device_Tree_Linux

. node names
"unit-address does not have leading zeros"

As 'PRIpaddr' introduces leading zeros, we cannot use it.

So, we have introduced a wrapper ie domain_fdt_begin_node() which will
represent physical address using 'PRIx64'.

2. One should use 'PRIx64' to display 'u64' in hex format. The current
use of 'PRIpaddr' for printing PTE is buggy as this is not a physical
address.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Julien Grall <jgrall@amazon.com>
---

Changes from -

v3 - 1. Extracted the patch from https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00655.html
and added to this series.
2. No changes done.

 xen/arch/arm/domain_build.c | 64 +++++++++++++++++++++++--------------
 xen/arch/arm/gic-v2.c       |  6 ++--
 xen/arch/arm/mm.c           |  2 +-
 3 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9707eb7b1b..15fa88e977 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1288,6 +1288,39 @@ static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
     return res;
 }
 
+/*
+ * Wrapper to convert physical address from paddr_t to uint64_t and
+ * invoke fdt_begin_node(). This is required as the physical address
+ * provided as part of node name should not contain any leading
+ * zeroes. Thus, one should use PRIx64 (instead of PRIpaddr) to append
+ * unit (which contains the physical address) with name to generate a
+ * node name.
+ */
+static int __init domain_fdt_begin_node(void *fdt, const char *name,
+                                        uint64_t unit)
+{
+    /*
+     * The size of the buffer to hold the longest possible string (i.e.
+     * interrupt-controller@ + a 64-bit number + \0).
+     */
+    char buf[38];
+    int ret;
+
+    /* ePAPR 3.4 */
+    ret = snprintf(buf, sizeof(buf), "%s@%"PRIx64, name, unit);
+
+    if ( ret >= sizeof(buf) )
+    {
+        printk(XENLOG_ERR
+               "Insufficient buffer. Minimum size required is %d\n",
+               (ret + 1));
+
+        return -FDT_ERR_TRUNCATED;
+    }
+
+    return fdt_begin_node(fdt, buf);
+}
+
 static int __init make_memory_node(const struct domain *d,
                                    void *fdt,
                                    int addrcells, int sizecells,
@@ -1296,8 +1329,6 @@ static int __init make_memory_node(const struct domain *d,
     unsigned int i;
     int res, reg_size = addrcells + sizecells;
     int nr_cells = 0;
-    /* Placeholder for memory@ + a 64-bit number + \0 */
-    char buf[24];
     __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
     __be32 *cells;
 
@@ -1314,9 +1345,7 @@ static int __init make_memory_node(const struct domain *d,
 
     dt_dprintk("Create memory node\n");
 
-    /* ePAPR 3.4 */
-    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
-    res = fdt_begin_node(fdt, buf);
+    res = domain_fdt_begin_node(fdt, "memory", mem->bank[i].start);
     if ( res )
         return res;
 
@@ -1375,16 +1404,13 @@ static int __init make_shm_memory_node(const struct domain *d,
     {
         uint64_t start = mem->bank[i].start;
         uint64_t size = mem->bank[i].size;
-        /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
-        char buf[27];
         const char compat[] = "xen,shared-memory-v1";
         /* Worst case addrcells + sizecells */
         __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
         __be32 *cells;
         unsigned int len = (addrcells + sizecells) * sizeof(__be32);
 
-        snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start);
-        res = fdt_begin_node(fdt, buf);
+        res = domain_fdt_begin_node(fdt, "xen-shmem", mem->bank[i].start);
         if ( res )
             return res;
 
@@ -2716,12 +2742,9 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
     __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
     __be32 *cells;
     const struct domain *d = kinfo->d;
-    /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
-    char buf[38];
 
-    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
-             vgic_dist_base(&d->arch.vgic));
-    res = fdt_begin_node(fdt, buf);
+    res = domain_fdt_begin_node(fdt, "interrupt-controller",
+                                vgic_dist_base(&d->arch.vgic));
     if ( res )
         return res;
 
@@ -2771,14 +2794,10 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
     int res = 0;
     __be32 *reg, *cells;
     const struct domain *d = kinfo->d;
-    /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
-    char buf[38];
     unsigned int i, len = 0;
 
-    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
-             vgic_dist_base(&d->arch.vgic));
-
-    res = fdt_begin_node(fdt, buf);
+    res = domain_fdt_begin_node(fdt, "interrupt-controller",
+                                vgic_dist_base(&d->arch.vgic));
     if ( res )
         return res;
 
@@ -2858,11 +2877,8 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
     __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
     __be32 *cells;
     struct domain *d = kinfo->d;
-    /* Placeholder for sbsa-uart@ + a 64-bit number + \0 */
-    char buf[27];
 
-    snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d->arch.vpl011.base_addr);
-    res = fdt_begin_node(fdt, buf);
+    res = domain_fdt_begin_node(fdt, "sbsa-uart", d->arch.vpl011.base_addr);
     if ( res )
         return res;
 
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 61802839cb..5d4d298b86 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1049,7 +1049,7 @@ static void __init gicv2_dt_init(void)
     if ( csize < SZ_8K )
     {
         printk(XENLOG_WARNING "GICv2: WARNING: "
-               "The GICC size is too small: %#"PRIx64" expected %#x\n",
+               "The GICC size is too small: %#"PRIpaddr" expected %#x\n",
                csize, SZ_8K);
         if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
         {
@@ -1280,11 +1280,11 @@ static int __init gicv2_init(void)
         gicv2.map_cbase += aliased_offset;
 
         printk(XENLOG_WARNING
-               "GICv2: Adjusting CPU interface base to %#"PRIx64"\n",
+               "GICv2: Adjusting CPU interface base to %#"PRIpaddr"\n",
                cbase + aliased_offset);
     } else if ( csize == SZ_128K )
         printk(XENLOG_WARNING
-               "GICv2: GICC size=%#"PRIx64" but not aliased\n",
+               "GICv2: GICC size=%#"PRIpaddr" but not aliased\n",
                csize);
 
     gicv2.map_hbase = ioremap_nocache(hbase, PAGE_SIZE);
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index f758cad545..b99806af99 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -263,7 +263,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
 
         pte = mapping[offsets[level]];
 
-        printk("%s[0x%03x] = 0x%"PRIpaddr"\n",
+        printk("%s[0x%03x] = 0x%"PRIx64"\n",
                level_strs[level], offsets[level], pte.bits);
 
         if ( level == 3 || !pte.walk.valid || !pte.walk.table )
-- 
2.17.1

[XEN v4 02/11] xen/arm: domain_build: Track unallocated pages using the frame number

[Ayan Kumar Halder]

rangeset_{xxx}_range() functions are invoked with 'start' and 'size' as
arguments which are either 'uint64_t' or 'paddr_t'. However, the function
accepts 'unsigned long' for 'start' and 'size'. 'unsigned long' is 32 bits for
ARM_32. Thus, there is an implicit downcasting from 'uint64_t'/'paddr_t' to
'unsigned long' when invoking rangeset_{xxx}_range().

So, it may seem there is a possibility of lose of data due to truncation.

In reality, 'start' and 'size' are always page aligned. And ARM_32 currently
supports 40 bits as the width of physical address.
So if the addresses are page aligned, the last 12 bits contain zeroes.
Thus, we could instead pass page frame number which will contain 28 bits (40-12
on Arm_32) and this can be represented using 'unsigned long'.

On Arm_64, this change will not induce any adverse side effect as the width of
physical address is 48 bits. Thus, the width of 'mfn' (ie 48 - 12 = 36) can be
represented using 'unsigned long' (which is 64 bits wide).

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

v3 - 1. Extracted the patch from https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00657.html
and added it to this series.
2. Modified add_ext_regions(). This accepts a frame number instead of physical
address.

 xen/arch/arm/domain_build.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 15fa88e977..24b12b7512 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1500,10 +1500,13 @@ static int __init make_resv_memory_node(const struct domain *d,
     return res;
 }
 
-static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
+static int __init add_ext_regions(unsigned long s_pfn, unsigned long e_pfn,
+                                  void *data)
 {
     struct meminfo *ext_regions = data;
     paddr_t start, size;
+    paddr_t s = PFN_UP(s_pfn);
+    paddr_t e = PFN_UP(e_pfn);
 
     if ( ext_regions->nr_banks >= ARRAY_SIZE(ext_regions->bank) )
         return 0;
@@ -1566,7 +1569,8 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
     {
         start = bootinfo.mem.bank[i].start;
         end = bootinfo.mem.bank[i].start + bootinfo.mem.bank[i].size;
-        res = rangeset_add_range(unalloc_mem, start, end - 1);
+        res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
+                                 PFN_DOWN(end - 1));
         if ( res )
         {
             printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1580,7 +1584,8 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
     {
         start = assign_mem->bank[i].start;
         end = assign_mem->bank[i].start + assign_mem->bank[i].size;
-        res = rangeset_remove_range(unalloc_mem, start, end - 1);
+        res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
+                                    PFN_DOWN(end - 1));
         if ( res )
         {
             printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1595,7 +1600,8 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
         start = bootinfo.reserved_mem.bank[i].start;
         end = bootinfo.reserved_mem.bank[i].start +
             bootinfo.reserved_mem.bank[i].size;
-        res = rangeset_remove_range(unalloc_mem, start, end - 1);
+        res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
+                                    PFN_DOWN(end - 1));
         if ( res )
         {
             printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1607,7 +1613,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
     /* Remove grant table region */
     start = kinfo->gnttab_start;
     end = kinfo->gnttab_start + kinfo->gnttab_size;
-    res = rangeset_remove_range(unalloc_mem, start, end - 1);
+    res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), PFN_DOWN(end - 1));
     if ( res )
     {
         printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1617,7 +1623,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
 
     start = 0;
     end = (1ULL << p2m_ipa_bits) - 1;
-    res = rangeset_report_ranges(unalloc_mem, start, end,
+    res = rangeset_report_ranges(unalloc_mem, PFN_DOWN(start), PFN_DOWN(end),
                                  add_ext_regions, ext_regions);
     if ( res )
         ext_regions->nr_banks = 0;
@@ -1639,7 +1645,7 @@ static int __init handle_pci_range(const struct dt_device_node *dev,
 
     start = addr & PAGE_MASK;
     end = PAGE_ALIGN(addr + len);
-    res = rangeset_remove_range(mem_holes, start, end - 1);
+    res = rangeset_remove_range(mem_holes, PFN_DOWN(start), PFN_DOWN(end - 1));
     if ( res )
     {
         printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1677,7 +1683,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
     /* Start with maximum possible addressable physical memory range */
     start = 0;
     end = (1ULL << p2m_ipa_bits) - 1;
-    res = rangeset_add_range(mem_holes, start, end);
+    res = rangeset_add_range(mem_holes, PFN_DOWN(start), PFN_DOWN(end));
     if ( res )
     {
         printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1708,7 +1714,8 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
 
             start = addr & PAGE_MASK;
             end = PAGE_ALIGN(addr + size);
-            res = rangeset_remove_range(mem_holes, start, end - 1);
+            res = rangeset_remove_range(mem_holes, PFN_DOWN(start),
+                                        PFN_DOWN(end - 1));
             if ( res )
             {
                 printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
@@ -1735,7 +1742,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
 
     start = 0;
     end = (1ULL << p2m_ipa_bits) - 1;
-    res = rangeset_report_ranges(mem_holes, start, end,
+    res = rangeset_report_ranges(mem_holes, PFN_DOWN(start), PFN_DOWN(end),
                                  add_ext_regions,  ext_regions);
     if ( res )
         ext_regions->nr_banks = 0;
-- 
2.17.1

[XEN v4 03/11] xen/arm: Typecast the DT values into paddr_t

[Ayan Kumar Halder]

In future, we wish to support 32 bit physical address.
However, the current dt and fdt functions can only read u64 values.
We wish to make the DT functions read 32bit as well 64bit values
(depending on the width of physical address). Also, we wish to detect
if any truncation has occurred (ie while reading 32bit physical
addresses from 64bit values read from DT).

device_tree_get_reg() should now be able to return paddr_t. This is
invoked by various callers to get DT address and size.

For fdt_get_mem_rsv(), we have introduced wrapper ie
fdt_get_mem_rsv_paddr() while will invoke fdt_get_mem_rsv() and
translate uint64_t to paddr_t. The reason being we cannot modify
fdt_get_mem_rsv() as it has been imported from external source.

For dt_read_number(), we have also introduced a wrapper ie
dt_read_paddr() to read physical addresses. We chose not to modify the
original function as it been used in places where it needs to
specifically 64bit values from dt (For eg dt_property_read_u64()).

Xen prints an error when it detects a truncation (during typecast
between uint64_t and paddr_t). It is not possible to return an error in
all scenarios. So, it is user's responsibility to check the error logs.

Also, replaced u32/u64 with uint32_t/uint64_t in the functions touched
by the code changes.

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

Changes from

v1 - 1. Dropped "[XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t" and
"[XEN v1 4/9] xen/arm: Use translate_dt_address_size() to translate between device tree addr/size and paddr_t", instead
this approach achieves the same purpose.

2. No need to check for truncation while converting values from u64 to paddr_t.

v2 - 1. Use "( (dt_start >> (PADDR_SHIFT - 1)) > 1 )" to detect truncation.
2. Introduced libfdt_xen.h to implement fdt_get_mem_rsv_paddr
3. Logged error messages in case truncation is detected.

v3 - 1. Renamed libfdt_xen.h to libfdt-xen.h.
2. Replaced u32/u64 with uint32_t/uint64_t
3. Use "(paddr_t)val != val" to check for truncation.
4. Removed the alias "#define PADDR_SHIFT PADDR_BITS". 

 xen/arch/arm/bootfdt.c              | 41 ++++++++++++++++++-----
 xen/arch/arm/domain_build.c         |  2 +-
 xen/arch/arm/include/asm/setup.h    |  4 +--
 xen/arch/arm/setup.c                | 14 ++++----
 xen/arch/arm/smpboot.c              |  2 +-
 xen/include/xen/device_tree.h       | 21 ++++++++++++
 xen/include/xen/libfdt/libfdt-xen.h | 52 +++++++++++++++++++++++++++++
 7 files changed, 116 insertions(+), 20 deletions(-)
 create mode 100644 xen/include/xen/libfdt/libfdt-xen.h

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 0085c28d74..33bef1c15e 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -11,7 +11,7 @@
 #include <xen/efi.h>
 #include <xen/device_tree.h>
 #include <xen/lib.h>
-#include <xen/libfdt/libfdt.h>
+#include <xen/libfdt/libfdt-xen.h>
 #include <xen/sort.h>
 #include <xsm/xsm.h>
 #include <asm/setup.h>
@@ -52,11 +52,32 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
     return false;
 }
 
-void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
-                                u32 size_cells, u64 *start, u64 *size)
+void __init device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
+                                uint32_t size_cells, paddr_t *start,
+                                paddr_t *size)
 {
-    *start = dt_next_cell(address_cells, cell);
-    *size = dt_next_cell(size_cells, cell);
+    uint64_t dt_start, dt_size;
+
+    /*
+     * dt_next_cell will return u64 whereas paddr_t may be u64 or u32. Thus,
+     * there is an implicit cast from u64 to paddr_t.
+     */
+    dt_start = dt_next_cell(address_cells, cell);
+    dt_size = dt_next_cell(size_cells, cell);
+
+    if ( dt_start != (paddr_t)dt_start  )
+        printk("Error: Physical address greater than max width supported\n");
+
+    if ( dt_size != (paddr_t)dt_size )
+        printk("Error: Physical size greater than max width supported\n");
+
+    /*
+     * Note: It is user's responsibility to check for the error messages.
+     * Xen will sliently truncate in case if the address/size is greater than
+     * the max supported width.
+     */
+    *start = dt_start;
+    *size = dt_size;
 }
 
 static int __init device_tree_get_meminfo(const void *fdt, int node,
@@ -326,7 +347,7 @@ static int __init process_chosen_node(const void *fdt, int node,
         printk("linux,initrd-start property has invalid length %d\n", len);
         return -EINVAL;
     }
-    start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
+    start = dt_read_paddr((void *)&prop->data, dt_size_to_cells(len));
 
     prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
     if ( !prop )
@@ -339,7 +360,7 @@ static int __init process_chosen_node(const void *fdt, int node,
         printk("linux,initrd-end property has invalid length %d\n", len);
         return -EINVAL;
     }
-    end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
+    end = dt_read_paddr((void *)&prop->data, dt_size_to_cells(len));
 
     if ( start >= end )
     {
@@ -594,9 +615,11 @@ static void __init early_print_info(void)
     for ( i = 0; i < nr_rsvd; i++ )
     {
         paddr_t s, e;
-        if ( fdt_get_mem_rsv(device_tree_flattened, i, &s, &e) < 0 )
+
+        if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &e) < 0 )
             continue;
-        /* fdt_get_mem_rsv returns length */
+
+        /* fdt_get_mem_rsv_paddr returns length */
         e += s;
         printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e);
     }
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 24b12b7512..6573d15302 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -949,7 +949,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         BUG_ON(!prop);
         cells = (const __be32 *)prop->value;
         device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase);
-        psize = dt_read_number(cells, size_cells);
+        psize = dt_read_paddr(cells, size_cells);
         if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) )
         {
             printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n",
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index a926f30a2b..7b697d879e 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -157,8 +157,8 @@ const char *boot_module_kind_as_string(bootmodule_kind kind);
 extern uint32_t hyp_traps_vector[];
 void init_traps(void);
 
-void device_tree_get_reg(const __be32 **cell, u32 address_cells,
-                         u32 size_cells, u64 *start, u64 *size);
+void device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
+                         uint32_t size_cells, paddr_t *start, paddr_t *size);
 
 u32 device_tree_get_u32(const void *fdt, int node,
                         const char *prop_name, u32 dflt);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 1f26f67b90..755173e5a3 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -29,7 +29,7 @@
 #include <xen/virtual_region.h>
 #include <xen/vmap.h>
 #include <xen/trace.h>
-#include <xen/libfdt/libfdt.h>
+#include <xen/libfdt/libfdt-xen.h>
 #include <xen/acpi.h>
 #include <xen/warning.h>
 #include <asm/alternative.h>
@@ -222,11 +222,11 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
     {
         paddr_t r_s, r_e;
 
-        if ( fdt_get_mem_rsv(device_tree_flattened, i, &r_s, &r_e ) < 0 )
+        if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &r_s, &r_e ) < 0 )
             /* If we can't read it, pretend it doesn't exist... */
             continue;
 
-        r_e += r_s; /* fdt_get_mem_rsv returns length */
+        r_e += r_s; /* fdt_get_mem_rsv_paddr returns length */
 
         if ( s < r_e && r_s < e )
         {
@@ -502,13 +502,13 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
     {
         paddr_t mod_s, mod_e;
 
-        if ( fdt_get_mem_rsv(device_tree_flattened,
-                             i - mi->nr_mods,
-                             &mod_s, &mod_e ) < 0 )
+        if ( fdt_get_mem_rsv_paddr(device_tree_flattened,
+                                   i - mi->nr_mods,
+                                   &mod_s, &mod_e ) < 0 )
             /* If we can't read it, pretend it doesn't exist... */
             continue;
 
-        /* fdt_get_mem_rsv returns length */
+        /* fdt_get_mem_rsv_paddr returns length */
         mod_e += mod_s;
 
         if ( s < mod_e && mod_s < e )
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 412ae22869..c15c177487 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -159,7 +159,7 @@ static void __init dt_smp_init_cpus(void)
             continue;
         }
 
-        addr = dt_read_number(prop, dt_n_addr_cells(cpu));
+        addr = dt_read_paddr(prop, dt_n_addr_cells(cpu));
 
         hwid = addr;
         if ( hwid != addr )
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 19a74909ce..bbc7d7377a 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -241,6 +241,27 @@ static inline u64 dt_read_number(const __be32 *cell, int size)
     return r;
 }
 
+/* Wrapper for dt_read_number() to return paddr_t (instead of u64) */
+static inline paddr_t dt_read_paddr(const __be32 *cell, int size)
+{
+    uint64_t dt_r = 0;
+    paddr_t r;
+
+    dt_r = dt_read_number(cell, size);
+
+    if ( dt_r != (paddr_t)dt_r )
+        printk("Error: Physical address greater than max width supported\n");
+
+    /*
+     * Note: It is user's responsibility to check for the error messages.
+     * Xen will sliently truncate in case if the address/size is greater than
+     * the max supported width.
+     */
+    r = dt_r;
+
+    return r;
+}
+
 /* Helper to convert a number of cells to bytes */
 static inline int dt_cells_to_size(int size)
 {
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
new file mode 100644
index 0000000000..648bf41be6
--- /dev/null
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * xen/include/xen/libfdt/libfdt-xen.h
+ *
+ * Wrapper functions for device tree. This helps to convert dt values
+ * between u64 and paddr_t.
+ *
+ * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved.
+ */
+
+#ifndef LIBFDT_XEN_H
+#define LIBFDT_XEN_H
+
+#include <xen/libfdt/libfdt.h>
+
+inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
+                                 paddr_t *address,
+                                 paddr_t *size)
+{
+    uint64_t dt_addr;
+    uint64_t dt_size;
+    int ret = 0;
+
+    ret = fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size);
+
+    if ( dt_addr != (paddr_t)dt_addr )
+    {
+        printk("Error: Physical address greater than max width supported\n");
+        return -1;
+    }
+
+    if ( dt_size != (paddr_t)dt_size )
+    {
+        printk("Error: Physical size greater than max width supported\n");
+        return -1;
+    }
+
+    *address = dt_addr;
+    *size = dt_size;
+
+    return ret;
+}
+
+#endif /* LIBFDT_XEN_H */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.17.1

[XEN v4 04/11] xen/drivers: ns16550: Use paddr_t for io_base/io_size

[Ayan Kumar Halder]

io_base and io_size represent physical addresses. So they should use
paddr_t (instead of u64).

However in future, paddr_t may be defined as u32. So when typecasting
values from u64 to paddr_t, one should always check for any possible
truncation. If any truncation is discovered, Xen needs to return an
appropriate an error message for this.

Also moved the definition of PARSE_ERR_RET before its first usage.

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

v2 - 1. Extracted the patch from "[XEN v2 05/11] xen/arm: Use paddr_t instead of u64 for address/size"
into a separate patch of its own.

v3 - 1. Reduced the scope of pci_uart_io_base and uart_io_base definitions.
2. Instead of crashing, invoke PARSE_ERR_RET().
3. Moved PARSE_ERR_RET() so that it is defined before its first use.

 xen/drivers/char/ns16550.c | 41 ++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 092f6b9c4b..2e8a9cfb24 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -42,8 +42,8 @@
 
 static struct ns16550 {
     int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
-    u64 io_base;   /* I/O port or memory-mapped I/O address. */
-    u64 io_size;
+    paddr_t io_base;   /* I/O port or memory-mapped I/O address. */
+    paddr_t io_size;
     int reg_shift; /* Bits to shift register offset by */
     int reg_width; /* Size of access to use, the registers
                     * themselves are still bytes */
@@ -1163,10 +1163,16 @@ static const struct ns16550_config __initconst uart_config[] =
     },
 };
 
+#define PARSE_ERR_RET(_f, _a...)             \
+    do {                                     \
+        printk( "ERROR: " _f "\n" , ## _a ); \
+        return false;                        \
+    } while ( 0 )
+
 static int __init
 pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
 {
-    u64 orig_base = uart->io_base;
+    paddr_t orig_base = uart->io_base;
     unsigned int b, d, f, nextf, i;
 
     /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
@@ -1235,6 +1241,8 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
                 /* MMIO based */
                 if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
+                    uint64_t pci_uart_io_base;
+
                     pci_conf_write32(PCI_SBDF(0, b, d, f),
                                      PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
                     len = pci_conf_read32(PCI_SBDF(0, b, d, f),
@@ -1259,8 +1267,14 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
                     else
                         size = len & PCI_BASE_ADDRESS_MEM_MASK;
 
-                    uart->io_base = ((u64)bar_64 << 32) |
-                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
+                    pci_uart_io_base = ((u64)bar_64 << 32) |
+                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
+
+                    /* Truncation detected while converting to paddr_t */
+                    if ( (pci_uart_io_base >> (PADDR_BITS - 1)) > 1 )
+                        PARSE_ERR_RET("Truncation detected for io_base address");
+
+                    uart->io_base = pci_uart_io_base;
                 }
                 /* IO based */
                 else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) )
@@ -1456,13 +1470,6 @@ static enum __init serial_param_type get_token(char *token, char **value)
         return;                              \
     } while ( 0 )
 
-#define PARSE_ERR_RET(_f, _a...)             \
-    do {                                     \
-        printk( "ERROR: " _f "\n" , ## _a ); \
-        return false;                        \
-    } while ( 0 )
-
-
 static bool __init parse_positional(struct ns16550 *uart, char **str)
 {
     int baud;
@@ -1532,7 +1539,15 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
         else
 #endif
         {
-            uart->io_base = simple_strtoull(conf, &conf, 0);
+            uint64_t uart_io_base;
+
+            uart_io_base = simple_strtoull(conf, &conf, 0);
+
+            /* Truncation detected while converting to paddr_t */
+            if ( (uart_io_base >> (PADDR_BITS - 1)) > 1 )
+                PARSE_ERR_RET("Truncation detected for uart_io_base address");
+
+            uart->io_base = uart_io_base;
         }
     }
 
-- 
2.17.1

[XEN v4 05/11] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t

[Ayan Kumar Halder]

dt_device_get_address() can accept uint64_t only for address and size.
However, the address/size denotes physical addresses. Thus, they should
be represented by 'paddr_t'.
Consequently, we introduce a wrapper for dt_device_get_address() ie
dt_device_get_paddr() which accepts address/size as paddr_t and inturn
invokes dt_device_get_address() after converting address/size to
uint64_t.

The reason for introducing doing this is that in future 'paddr_t' may
be defined as uint32_t. Thus, we need an explicit wrapper to do the type
conversion and return an error in case of truncation.

With this, callers now invoke dt_device_get_paddr().
dt_device_get_address() is invoked by dt_device_get_paddr() only.

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

v1 - 1. New patch.

v2 - 1. Extracted part of "[XEN v2 05/11] xen/arm: Use paddr_t instead of u64 for address/size"
into this patch.

2. dt_device_get_address() callers now invoke dt_device_get_paddr() instead.

3. Logged error in case of truncation.

v3 - 1. Modified the truncation checks as "dt_addr != (paddr_t)dt_addr".
2. Some sanity fixes. 

 xen/arch/arm/domain_build.c                | 10 +++---
 xen/arch/arm/gic-v2.c                      | 10 +++---
 xen/arch/arm/gic-v3-its.c                  |  4 +--
 xen/arch/arm/gic-v3.c                      | 10 +++---
 xen/arch/arm/pci/pci-host-common.c         |  6 ++--
 xen/arch/arm/platforms/brcm-raspberry-pi.c |  2 +-
 xen/arch/arm/platforms/brcm.c              |  4 +--
 xen/arch/arm/platforms/exynos5.c           | 32 ++++++++---------
 xen/arch/arm/platforms/sunxi.c             |  2 +-
 xen/arch/arm/platforms/xgene-storm.c       |  2 +-
 xen/common/device_tree.c                   | 40 ++++++++++++++++++++--
 xen/drivers/char/cadence-uart.c            |  4 +--
 xen/drivers/char/exynos4210-uart.c         |  4 +--
 xen/drivers/char/imx-lpuart.c              |  4 +--
 xen/drivers/char/meson-uart.c              |  4 +--
 xen/drivers/char/mvebu-uart.c              |  4 +--
 xen/drivers/char/ns16550.c                 |  2 +-
 xen/drivers/char/omap-uart.c               |  4 +--
 xen/drivers/char/pl011.c                   |  6 ++--
 xen/drivers/char/scif-uart.c               |  4 +--
 xen/drivers/passthrough/arm/ipmmu-vmsa.c   |  8 ++---
 xen/drivers/passthrough/arm/smmu-v3.c      |  2 +-
 xen/drivers/passthrough/arm/smmu.c         |  8 ++---
 xen/include/xen/device_tree.h              |  6 ++--
 24 files changed, 109 insertions(+), 73 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6573d15302..b4ae6a2548 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1698,13 +1698,13 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
     dt_for_each_device_node( dt_host, np )
     {
         unsigned int naddr;
-        u64 addr, size;
+        paddr_t addr, size;
 
         naddr = dt_number_of_address(np);
 
         for ( i = 0; i < naddr; i++ )
         {
-            res = dt_device_get_address(np, i, &addr, &size);
+            res = dt_device_get_paddr(np, i, &addr, &size);
             if ( res )
             {
                 printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
@@ -2478,7 +2478,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
     unsigned int naddr;
     unsigned int i;
     int res;
-    u64 addr, size;
+    paddr_t addr, size;
     bool own_device = !dt_device_for_passthrough(dev);
     /*
      * We want to avoid mapping the MMIO in dom0 for the following cases:
@@ -2533,7 +2533,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
     /* Give permission and map MMIOs */
     for ( i = 0; i < naddr; i++ )
     {
-        res = dt_device_get_address(dev, i, &addr, &size);
+        res = dt_device_get_paddr(dev, i, &addr, &size);
         if ( res )
         {
             printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
@@ -2964,7 +2964,7 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
         if ( res )
         {
             printk(XENLOG_ERR "Unable to permit to dom%d access to"
-                   " 0x%"PRIx64" - 0x%"PRIx64"\n",
+                   " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
                    kinfo->d->domain_id,
                    mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
             return res;
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 5d4d298b86..6476ff4230 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -993,7 +993,7 @@ static void gicv2_extension_dt_init(const struct dt_device_node *node)
             continue;
 
         /* Get register frame resource from DT. */
-        if ( dt_device_get_address(v2m, 0, &addr, &size) )
+        if ( dt_device_get_paddr(v2m, 0, &addr, &size) )
             panic("GICv2: Cannot find a valid v2m frame address\n");
 
         /*
@@ -1018,19 +1018,19 @@ static void __init gicv2_dt_init(void)
     paddr_t vsize;
     const struct dt_device_node *node = gicv2_info.node;
 
-    res = dt_device_get_address(node, 0, &dbase, NULL);
+    res = dt_device_get_paddr(node, 0, &dbase, NULL);
     if ( res )
         panic("GICv2: Cannot find a valid address for the distributor\n");
 
-    res = dt_device_get_address(node, 1, &cbase, &csize);
+    res = dt_device_get_paddr(node, 1, &cbase, &csize);
     if ( res )
         panic("GICv2: Cannot find a valid address for the CPU\n");
 
-    res = dt_device_get_address(node, 2, &hbase, NULL);
+    res = dt_device_get_paddr(node, 2, &hbase, NULL);
     if ( res )
         panic("GICv2: Cannot find a valid address for the hypervisor\n");
 
-    res = dt_device_get_address(node, 3, &vbase, &vsize);
+    res = dt_device_get_paddr(node, 3, &vbase, &vsize);
     if ( res )
         panic("GICv2: Cannot find a valid address for the virtual CPU\n");
 
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 1ec9934191..3aa4edda10 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -1004,12 +1004,12 @@ static void gicv3_its_dt_init(const struct dt_device_node *node)
      */
     dt_for_each_child_node(node, its)
     {
-        uint64_t addr, size;
+        paddr_t addr, size;
 
         if ( !dt_device_is_compatible(its, "arm,gic-v3-its") )
             continue;
 
-        if ( dt_device_get_address(its, 0, &addr, &size) )
+        if ( dt_device_get_paddr(its, 0, &addr, &size) )
             panic("GICv3: Cannot find a valid ITS frame address\n");
 
         add_to_host_its_list(addr, size, its);
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index bb59ea94cd..4e6c98bada 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1377,7 +1377,7 @@ static void __init gicv3_dt_init(void)
     int res, i;
     const struct dt_device_node *node = gicv3_info.node;
 
-    res = dt_device_get_address(node, 0, &dbase, NULL);
+    res = dt_device_get_paddr(node, 0, &dbase, NULL);
     if ( res )
         panic("GICv3: Cannot find a valid distributor address\n");
 
@@ -1393,9 +1393,9 @@ static void __init gicv3_dt_init(void)
 
     for ( i = 0; i < gicv3.rdist_count; i++ )
     {
-        uint64_t rdist_base, rdist_size;
+        paddr_t rdist_base, rdist_size;
 
-        res = dt_device_get_address(node, 1 + i, &rdist_base, &rdist_size);
+        res = dt_device_get_paddr(node, 1 + i, &rdist_base, &rdist_size);
         if ( res )
             panic("GICv3: No rdist base found for region %d\n", i);
 
@@ -1417,10 +1417,10 @@ static void __init gicv3_dt_init(void)
      * For GICv3 supporting GICv2, GICC and GICV base address will be
      * provided.
      */
-    res = dt_device_get_address(node, 1 + gicv3.rdist_count,
+    res = dt_device_get_paddr(node, 1 + gicv3.rdist_count,
                                 &cbase, &csize);
     if ( !res )
-        dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
+        dt_device_get_paddr(node, 1 + gicv3.rdist_count + 2,
                               &vbase, &vsize);
 }
 
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index a8ece94303..5550f9478d 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -93,7 +93,7 @@ gen_pci_init(struct dt_device_node *dev, const struct pci_ecam_ops *ops)
         cfg_reg_idx = 0;
 
     /* Parse our PCI ecam register address */
-    err = dt_device_get_address(dev, cfg_reg_idx, &addr, &size);
+    err = dt_device_get_paddr(dev, cfg_reg_idx, &addr, &size);
     if ( err )
         goto err_exit;
 
@@ -349,10 +349,10 @@ int __init pci_host_bridge_mappings(struct domain *d)
 
         for ( i = 0; i < dt_number_of_address(dev); i++ )
         {
-            uint64_t addr, size;
+            paddr_t addr, size;
             int err;
 
-            err = dt_device_get_address(dev, i, &addr, &size);
+            err = dt_device_get_paddr(dev, i, &addr, &size);
             if ( err )
             {
                 printk(XENLOG_ERR
diff --git a/xen/arch/arm/platforms/brcm-raspberry-pi.c b/xen/arch/arm/platforms/brcm-raspberry-pi.c
index 811b40b1a6..407ec07f63 100644
--- a/xen/arch/arm/platforms/brcm-raspberry-pi.c
+++ b/xen/arch/arm/platforms/brcm-raspberry-pi.c
@@ -64,7 +64,7 @@ static void __iomem *rpi4_map_watchdog(void)
     if ( !node )
         return NULL;
 
-    ret = dt_device_get_address(node, 0, &start, &len);
+    ret = dt_device_get_paddr(node, 0, &start, &len);
     if ( ret )
     {
         printk("Cannot read watchdog register address\n");
diff --git a/xen/arch/arm/platforms/brcm.c b/xen/arch/arm/platforms/brcm.c
index d481b2c60f..4310feee73 100644
--- a/xen/arch/arm/platforms/brcm.c
+++ b/xen/arch/arm/platforms/brcm.c
@@ -40,7 +40,7 @@ static __init int brcm_get_dt_node(char *compat_str,
                                    u32 *reg_base)
 {
     const struct dt_device_node *node;
-    u64 reg_base_64;
+    paddr_t reg_base_64;
     int rc;
 
     node = dt_find_compatible_node(NULL, NULL, compat_str);
@@ -50,7 +50,7 @@ static __init int brcm_get_dt_node(char *compat_str,
         return -ENOENT;
     }
 
-    rc = dt_device_get_address(node, 0, &reg_base_64, NULL);
+    rc = dt_device_get_paddr(node, 0, &reg_base_64, NULL);
     if ( rc )
     {
         dprintk(XENLOG_ERR, "%s: missing \"reg\" prop\n", __func__);
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index 6560507092..c48093cd4f 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -42,8 +42,8 @@ static int exynos5_init_time(void)
     void __iomem *mct;
     int rc;
     struct dt_device_node *node;
-    u64 mct_base_addr;
-    u64 size;
+    paddr_t mct_base_addr;
+    paddr_t size;
 
     node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-mct");
     if ( !node )
@@ -52,14 +52,14 @@ static int exynos5_init_time(void)
         return -ENXIO;
     }
 
-    rc = dt_device_get_address(node, 0, &mct_base_addr, &size);
+    rc = dt_device_get_paddr(node, 0, &mct_base_addr, &size);
     if ( rc )
     {
         dprintk(XENLOG_ERR, "Error in \"samsung,exynos4210-mct\"\n");
         return -ENXIO;
     }
 
-    dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
+    dprintk(XENLOG_INFO, "mct_base_addr: 0x%"PRIpaddr" size: 0x%"PRIpaddr"\n",
             mct_base_addr, size);
 
     mct = ioremap_nocache(mct_base_addr, size);
@@ -97,9 +97,9 @@ static int __init exynos5_smp_init(void)
     struct dt_device_node *node;
     void __iomem *sysram;
     char *compatible;
-    u64 sysram_addr;
-    u64 size;
-    u64 sysram_offset;
+    paddr_t sysram_addr;
+    paddr_t size;
+    paddr_t sysram_offset;
     int rc;
 
     node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmware");
@@ -125,13 +125,13 @@ static int __init exynos5_smp_init(void)
         return -ENXIO;
     }
 
-    rc = dt_device_get_address(node, 0, &sysram_addr, &size);
+    rc = dt_device_get_paddr(node, 0, &sysram_addr, &size);
     if ( rc )
     {
         dprintk(XENLOG_ERR, "Error in %s\n", compatible);
         return -ENXIO;
     }
-    dprintk(XENLOG_INFO, "sysram_addr: %016llx size: %016llx offset: %016llx\n",
+    dprintk(XENLOG_INFO,"sysram_addr: 0x%"PRIpaddr" size: 0x%"PRIpaddr"offset: 0x%"PRIpaddr"\n",
             sysram_addr, size, sysram_offset);
 
     sysram = ioremap_nocache(sysram_addr, size);
@@ -189,7 +189,7 @@ static int exynos5_cpu_power_up(void __iomem *power, int cpu)
     return 0;
 }
 
-static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size)
+static int exynos5_get_pmu_baseandsize(paddr_t *power_base_addr, paddr_t *size)
 {
     struct dt_device_node *node;
     int rc;
@@ -208,14 +208,14 @@ static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size)
         return -ENXIO;
     }
 
-    rc = dt_device_get_address(node, 0, power_base_addr, size);
+    rc = dt_device_get_paddr(node, 0, power_base_addr, size);
     if ( rc )
     {
         dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n");
         return -ENXIO;
     }
 
-    dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n",
+    dprintk(XENLOG_DEBUG, "power_base_addr: 0x%"PRIpaddr" size: 0x%"PRIpaddr"\n",
             *power_base_addr, *size);
 
     return 0;
@@ -223,8 +223,8 @@ static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size)
 
 static int exynos5_cpu_up(int cpu)
 {
-    u64 power_base_addr;
-    u64 size;
+    paddr_t power_base_addr;
+    paddr_t size;
     void __iomem *power;
     int rc;
 
@@ -256,8 +256,8 @@ static int exynos5_cpu_up(int cpu)
 
 static void exynos5_reset(void)
 {
-    u64 power_base_addr;
-    u64 size;
+    paddr_t power_base_addr;
+    paddr_t size;
     void __iomem *pmu;
     int rc;
 
diff --git a/xen/arch/arm/platforms/sunxi.c b/xen/arch/arm/platforms/sunxi.c
index e8e4d88bef..2b2c215f20 100644
--- a/xen/arch/arm/platforms/sunxi.c
+++ b/xen/arch/arm/platforms/sunxi.c
@@ -50,7 +50,7 @@ static void __iomem *sunxi_map_watchdog(bool *new_wdt)
         return NULL;
     }
 
-    ret = dt_device_get_address(node, 0, &wdt_start, &wdt_len);
+    ret = dt_device_get_paddr(node, 0, &wdt_start, &wdt_len);
     if ( ret )
     {
         dprintk(XENLOG_ERR, "Cannot read watchdog register address\n");
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index befd0c3c2d..6fc2f9679e 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -50,7 +50,7 @@ static void __init xgene_check_pirq_eoi(void)
     if ( !node )
         panic("%s: Can not find interrupt controller node\n", __func__);
 
-    res = dt_device_get_address(node, 0, &dbase, NULL);
+    res = dt_device_get_paddr(node, 0, &dbase, NULL);
     if ( res )
         panic("%s: Cannot find a valid address for the distributor\n", __func__);
 
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 6c9712ab7b..0d2922ad85 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -934,8 +934,9 @@ bail:
 }
 
 /* dt_device_address - Translate device tree address and return it */
-int dt_device_get_address(const struct dt_device_node *dev, unsigned int index,
-                          u64 *addr, u64 *size)
+static int dt_device_get_address(const struct dt_device_node *dev,
+                                 unsigned int index,
+                                 u64 *addr, u64 *size)
 {
     const __be32 *addrp;
     unsigned int flags;
@@ -955,6 +956,41 @@ int dt_device_get_address(const struct dt_device_node *dev, unsigned int index,
     return 0;
 }
 
+int dt_device_get_paddr(const struct dt_device_node *dev, unsigned int index,
+                        paddr_t *addr, paddr_t *size)
+{
+    uint64_t dt_addr = 0, dt_size = 0;
+    int ret;
+
+    ret = dt_device_get_address(dev, index, &dt_addr, &dt_size);
+    if ( ret )
+        return ret;
+
+    if ( addr )
+    {
+        if ( dt_addr != (paddr_t)dt_addr )
+        {
+            printk("Error: Physical address 0x%"PRIx64" for node=%s is greater than max width (%zu bytes) supported\n",
+                   dt_addr, dev->name, sizeof(paddr_t));
+            return -ERANGE;
+        }
+
+        *addr = dt_addr;
+    }
+
+    if ( size )
+    {
+        if ( dt_size != (paddr_t)dt_size )
+        {
+            printk("Error: Physical size 0x%"PRIx64" for node=%s is greater than max width (%zu bytes) supported\n",
+                   dt_size, dev->name, sizeof(paddr_t));
+            return -ERANGE;
+        }
+        *size = dt_size;
+    }
+
+    return ret;
+}
 
 int dt_for_each_range(const struct dt_device_node *dev,
                       int (*cb)(const struct dt_device_node *,
diff --git a/xen/drivers/char/cadence-uart.c b/xen/drivers/char/cadence-uart.c
index 22905ba66c..c38d7ed143 100644
--- a/xen/drivers/char/cadence-uart.c
+++ b/xen/drivers/char/cadence-uart.c
@@ -158,14 +158,14 @@ static int __init cuart_init(struct dt_device_node *dev, const void *data)
     const char *config = data;
     struct cuart *uart;
     int res;
-    u64 addr, size;
+    paddr_t addr, size;
 
     if ( strcmp(config, "") )
         printk("WARNING: UART configuration is not supported\n");
 
     uart = &cuart_com;
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_paddr(dev, 0, &addr, &size);
     if ( res )
     {
         printk("cadence: Unable to retrieve the base"
diff --git a/xen/drivers/char/exynos4210-uart.c b/xen/drivers/char/exynos4210-uart.c
index 43aaf02e18..2503392ccd 100644
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -303,7 +303,7 @@ static int __init exynos4210_uart_init(struct dt_device_node *dev,
     const char *config = data;
     struct exynos4210_uart *uart;
     int res;
-    u64 addr, size;
+    paddr_t addr, size;
 
     if ( strcmp(config, "") )
         printk("WARNING: UART configuration is not supported\n");
@@ -316,7 +316,7 @@ static int __init exynos4210_uart_init(struct dt_device_node *dev,
     uart->parity    = PARITY_NONE;
     uart->stop_bits = 1;
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_paddr(dev, 0, &addr, &size);
     if ( res )
     {
         printk("exynos4210: Unable to retrieve the base"
diff --git a/xen/drivers/char/imx-lpuart.c b/xen/drivers/char/imx-lpuart.c
index 9c1f3b71a3..77f70c2719 100644
--- a/xen/drivers/char/imx-lpuart.c
+++ b/xen/drivers/char/imx-lpuart.c
@@ -204,7 +204,7 @@ static int __init imx_lpuart_init(struct dt_device_node *dev,
     const char *config = data;
     struct imx_lpuart *uart;
     int res;
-    u64 addr, size;
+    paddr_t addr, size;
 
     if ( strcmp(config, "") )
         printk("WARNING: UART configuration is not supported\n");
@@ -216,7 +216,7 @@ static int __init imx_lpuart_init(struct dt_device_node *dev,
     uart->parity = 0;
     uart->stop_bits = 1;
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_paddr(dev, 0, &addr, &size);
     if ( res )
     {
         printk("imx8-lpuart: Unable to retrieve the base"
diff --git a/xen/drivers/char/meson-uart.c b/xen/drivers/char/meson-uart.c
index b1e25e0468..c627328122 100644
--- a/xen/drivers/char/meson-uart.c
+++ b/xen/drivers/char/meson-uart.c
@@ -209,14 +209,14 @@ static int __init meson_uart_init(struct dt_device_node *dev, const void *data)
     const char *config = data;
     struct meson_uart *uart;
     int res;
-    u64 addr, size;
+    paddr_t addr, size;
 
     if ( strcmp(config, "") )
         printk("WARNING: UART configuration is not supported\n");
 
     uart = &meson_com;
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_paddr(dev, 0, &addr, &size);
     if ( res )
     {
         printk("meson: Unable to retrieve the base address of the UART\n");
diff --git a/xen/drivers/char/mvebu-uart.c b/xen/drivers/char/mvebu-uart.c
index a00618b96f..cc55173513 100644
--- a/xen/drivers/char/mvebu-uart.c
+++ b/xen/drivers/char/mvebu-uart.c
@@ -231,14 +231,14 @@ static int __init mvebu_uart_init(struct dt_device_node *dev, const void *data)
     const char *config = data;
     struct mvebu3700_uart *uart;
     int res;
-    u64 addr, size;
+    paddr_t addr, size;
 
     if ( strcmp(config, "") )
         printk("WARNING: UART configuration is not supported\n");
 
     uart = &mvebu3700_com;
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_paddr(dev, 0, &addr, &size);
     if ( res )
     {
         printk("mvebu3700: Unable to retrieve the base address of the UART\n");
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 2e8a9cfb24..732c1e5c71 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1772,7 +1772,7 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
     uart->parity    = UART_PARITY_NONE;
     uart->stop_bits = 1;
 
-    res = dt_device_get_address(dev, 0, &uart->io_base, &uart->io_size);
+    res = dt_device_get_paddr(dev, 0, &uart->io_base, &uart->io_size);
     if ( res )
         return res;
 
diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
index d6a5d59aa2..8e643cb039 100644
--- a/xen/drivers/char/omap-uart.c
+++ b/xen/drivers/char/omap-uart.c
@@ -324,7 +324,7 @@ static int __init omap_uart_init(struct dt_device_node *dev,
     struct omap_uart *uart;
     u32 clkspec;
     int res;
-    u64 addr, size;
+    paddr_t addr, size;
 
     if ( strcmp(config, "") )
         printk("WARNING: UART configuration is not supported\n");
@@ -344,7 +344,7 @@ static int __init omap_uart_init(struct dt_device_node *dev,
     uart->parity = UART_PARITY_NONE;
     uart->stop_bits = 1;
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_paddr(dev, 0, &addr, &size);
     if ( res )
     {
         printk("omap-uart: Unable to retrieve the base"
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index be67242bc0..052a651251 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -222,7 +222,7 @@ static struct uart_driver __read_mostly pl011_driver = {
     .vuart_info   = pl011_vuart,
 };
 
-static int __init pl011_uart_init(int irq, u64 addr, u64 size, bool sbsa)
+static int __init pl011_uart_init(int irq, paddr_t addr, paddr_t size, bool sbsa)
 {
     struct pl011 *uart;
 
@@ -258,14 +258,14 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
 {
     const char *config = data;
     int res;
-    u64 addr, size;
+    paddr_t addr, size;
 
     if ( strcmp(config, "") )
     {
         printk("WARNING: UART configuration is not supported\n");
     }
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_paddr(dev, 0, &addr, &size);
     if ( res )
     {
         printk("pl011: Unable to retrieve the base"
diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index 2fccafe340..1b28ba90e9 100644
--- a/xen/drivers/char/scif-uart.c
+++ b/xen/drivers/char/scif-uart.c
@@ -311,14 +311,14 @@ static int __init scif_uart_init(struct dt_device_node *dev,
     const char *config = data;
     struct scif_uart *uart;
     int res;
-    u64 addr, size;
+    paddr_t addr, size;
 
     if ( strcmp(config, "") )
         printk("WARNING: UART configuration is not supported\n");
 
     uart = &scif_com;
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_paddr(dev, 0, &addr, &size);
     if ( res )
     {
         printk("scif-uart: Unable to retrieve the base"
diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
index 091f09b217..611d9eeba5 100644
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -794,7 +794,7 @@ static void ipmmu_device_reset(struct ipmmu_vmsa_device *mmu)
 static __init bool ipmmu_stage2_supported(void)
 {
     struct dt_device_node *np;
-    uint64_t addr, size;
+    paddr_t addr, size;
     void __iomem *base;
     uint32_t product, cut;
     bool stage2_supported = false;
@@ -806,7 +806,7 @@ static __init bool ipmmu_stage2_supported(void)
         return false;
     }
 
-    if ( dt_device_get_address(np, 0, &addr, &size) )
+    if ( dt_device_get_paddr(np, 0, &addr, &size) )
     {
         printk(XENLOG_ERR "ipmmu: Failed to get PRR MMIO\n");
         return false;
@@ -884,7 +884,7 @@ static int ipmmu_probe(struct dt_device_node *node)
 {
     const struct dt_device_match *match;
     struct ipmmu_vmsa_device *mmu;
-    uint64_t addr, size;
+    paddr_t addr, size;
     uint32_t reg;
     int irq, ret;
 
@@ -905,7 +905,7 @@ static int ipmmu_probe(struct dt_device_node *node)
     bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
 
     /* Map I/O memory and request IRQ. */
-    ret = dt_device_get_address(node, 0, &addr, &size);
+    ret = dt_device_get_paddr(node, 0, &addr, &size);
     if ( ret )
     {
         dev_err(&node->dev, "Failed to get MMIO\n");
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index bfdb62b395..b7fa2e90f7 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -2428,7 +2428,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	}
 
 	/* Base address */
-	ret = dt_device_get_address(np, 0, &ioaddr, &iosize);
+	ret = dt_device_get_paddr(np, 0, &ioaddr, &iosize);
 	if (ret)
 		goto out_free_smmu;
 
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 0a514821b3..79281075ba 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -73,8 +73,8 @@
 /* Xen: Helpers to get device MMIO and IRQs */
 struct resource
 {
-	u64 addr;
-	u64 size;
+	paddr_t addr;
+	paddr_t size;
 	unsigned int type;
 };
 
@@ -101,7 +101,7 @@ static struct resource *platform_get_resource(struct platform_device *pdev,
 
 	switch (type) {
 	case IORESOURCE_MEM:
-		ret = dt_device_get_address(pdev, num, &res.addr, &res.size);
+		ret = dt_device_get_paddr(pdev, num, &res.addr, &res.size);
 
 		return ((ret) ? NULL : &res);
 
@@ -169,7 +169,7 @@ static void __iomem *devm_ioremap_resource(struct device *dev,
 	ptr = ioremap_nocache(res->addr, res->size);
 	if (!ptr) {
 		dev_err(dev,
-			"ioremap failed (addr 0x%"PRIx64" size 0x%"PRIx64")\n",
+			"ioremap failed (addr 0x%"PRIpaddr" size 0x%"PRIpaddr")\n",
 			res->addr, res->size);
 		return ERR_PTR(-ENOMEM);
 	}
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index bbc7d7377a..d72dc7c788 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -580,7 +580,7 @@ int dt_find_node_by_gpath(XEN_GUEST_HANDLE(char) u_path, uint32_t u_plen,
 const struct dt_device_node *dt_get_parent(const struct dt_device_node *node);
 
 /**
- * dt_device_get_address - Resolve an address for a device
+ * dt_device_get_paddr - Resolve an address for a device
  * @device: the device whose address is to be resolved
  * @index: index of the address to resolve
  * @addr: address filled by this function
@@ -589,8 +589,8 @@ const struct dt_device_node *dt_get_parent(const struct dt_device_node *node);
  * This function resolves an address, walking the tree, for a give
  * device-tree node. It returns 0 on success.
  */
-int dt_device_get_address(const struct dt_device_node *dev, unsigned int index,
-                          u64 *addr, u64 *size);
+int dt_device_get_paddr(const struct dt_device_node *dev, unsigned int index,
+                        paddr_t *addr, paddr_t *size);
 
 /**
  * dt_number_of_irq - Get the number of IRQ for a device
-- 
2.17.1

[XEN v4 06/11] xen/arm: smmu: Use writeq_relaxed_non_atomic() for writing to SMMU_CBn_TTBR0

[Ayan Kumar Halder]

Refer ARM IHI 0062D.c ID070116 (SMMU 2.0 spec), 17-360, 17.3.9,
SMMU_CBn_TTBR0 is a 64 bit register. Thus, one can use
writeq_relaxed_non_atomic() to write to it instead of invoking
writel_relaxed() twice for lower half and upper half of the register.

This also helps us as p2maddr is 'paddr_t' (which may be u32 in future).
Thus, one can assign p2maddr to a 64 bit register and do the bit
manipulations on it, to generate the value for SMMU_CBn_TTBR0.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from -

v1 - 1. Extracted the patch from "[XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr".
Use writeq_relaxed_non_atomic() to write u64 register in a non-atomic
fashion.

v2 - 1. Added R-b.

v3 - 1. No changes.

 xen/drivers/passthrough/arm/smmu.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 79281075ba..c8ef2a925f 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -499,8 +499,7 @@ enum arm_smmu_s2cr_privcfg {
 #define ARM_SMMU_CB_SCTLR		0x0
 #define ARM_SMMU_CB_RESUME		0x8
 #define ARM_SMMU_CB_TTBCR2		0x10
-#define ARM_SMMU_CB_TTBR0_LO		0x20
-#define ARM_SMMU_CB_TTBR0_HI		0x24
+#define ARM_SMMU_CB_TTBR0		0x20
 #define ARM_SMMU_CB_TTBCR		0x30
 #define ARM_SMMU_CB_S1_MAIR0		0x38
 #define ARM_SMMU_CB_FSR			0x58
@@ -1083,6 +1082,7 @@ static void arm_smmu_flush_pgtable(struct arm_smmu_device *smmu, void *addr,
 static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
 {
 	u32 reg;
+	u64 reg64;
 	bool stage1;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
@@ -1177,12 +1177,13 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
 	dev_notice(smmu->dev, "d%u: p2maddr 0x%"PRIpaddr"\n",
 		   smmu_domain->cfg.domain->domain_id, p2maddr);
 
-	reg = (p2maddr & ((1ULL << 32) - 1));
-	writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
-	reg = (p2maddr >> 32);
+	reg64 = p2maddr;
+
 	if (stage1)
-		reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
-	writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
+		reg64 |= (((uint64_t) (ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT))
+		         << 32);
+
+	writeq_relaxed_non_atomic(reg64, cb_base + ARM_SMMU_CB_TTBR0);
 
 	/*
 	 * TTBCR
-- 
2.17.1

[XEN v4 07/11] 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
configs to choose if the physical address can be represented using
32 bits (PHYS_ADDR_T_32) or 64 bits (PHYS_ADDR_T_64).
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.
When PHYS_ADDR_T_64 is defined with ARM_32, PADDR_BITS is set to 40.
When PHYS_ADDR_T_64 is defined with ARM_64, PADDR_BITS is set to 48.
The last two are same as the current configuration used today on Xen.

PADDR_BITS is also set to 48 when ARM_64 is defined. The reason being
the choice to select ARM_PA_BITS_32/ARM_PA_BITS_40/ARM_PA_BITS_48 is
currently allowed when ARM_32 is defined.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@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".

 xen/arch/Kconfig                     |  6 +++++
 xen/arch/arm/Kconfig                 | 40 ++++++++++++++++++++++++++--
 xen/arch/arm/include/asm/page-bits.h |  6 +----
 xen/arch/arm/include/asm/types.h     |  6 +++++
 xen/arch/arm/mm.c                    |  1 +
 5 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
index 7028f7b74f..89096c77a4 100644
--- a/xen/arch/Kconfig
+++ b/xen/arch/Kconfig
@@ -1,6 +1,12 @@
 config 64BIT
 	bool
 
+config PHYS_ADDR_T_32
+	bool
+
+config PHYS_ADDR_T_64
+	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..13e3a23911 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -9,6 +9,7 @@ config ARM_64
 	select 64BIT
 	select ARM_EFI
 	select HAS_FAST_MULTIPLY
+	select PHYS_ADDR_T_64
 
 config ARM
 	def_bool y
@@ -19,13 +20,48 @@ config ARM
 	select HAS_PMAP
 	select IOMMU_FORCE_PT_SHARE
 
+menu "Architecture Features"
+
+choice
+	prompt "Physical address space size" if ARM_32
+	default ARM_PA_BITS_48 if ARM_64
+	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"
+	help
+	  On platforms where any physical address can be represented within 32 bits
+	  , user should choose this option. This will help is reduced size of the
+	  binary.
+	select PHYS_ADDR_T_32
+	depends on ARM_32
+
+config ARM_PA_BITS_40
+	bool "40-bit"
+	select PHYS_ADDR_T_64
+	depends on ARM_32
+
+config ARM_PA_BITS_48
+	bool "40-bit"
+	select PHYS_ADDR_T_64
+	depends on ARM_48
+endchoice
+
+config PADDR_BITS
+	int
+	default 32 if ARM_PA_BITS_32
+	default 40 if ARM_PA_BITS_40
+	default 48 if ARM_PA_BITS_48 || ARM_64
+
 config ARCH_DEFCONFIG
 	string
 	default "arch/arm/configs/arm32_defconfig" if ARM_32
 	default "arch/arm/configs/arm64_defconfig" if ARM_64
 
-menu "Architecture Features"
-
 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..e3cfbbb060 100644
--- a/xen/arch/arm/include/asm/types.h
+++ b/xen/arch/arm/include/asm/types.h
@@ -34,9 +34,15 @@ typedef signed long long s64;
 typedef unsigned long long u64;
 typedef u32 vaddr_t;
 #define PRIvaddr PRIx32
+#if defined(CONFIG_PHYS_ADDR_T_32)
+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 b99806af99..d8b43ef38c 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -690,6 +690,7 @@ 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;
 
+    BUILD_BUG_ON((sizeof(paddr_t) * 8) < PADDR_BITS);
     BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
 
     if ( frametable_size > FRAMETABLE_SIZE )
-- 
2.17.1

[XEN v4 08/11] 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.

 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 v4 09/11] xen/arm: Restrict zeroeth_table_offset for ARM_64

[Ayan Kumar Halder]

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

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

Acked-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.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.

 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..0d40388f93 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_ARM_PA_BITS_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 d8b43ef38c..41e0896b0f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -221,12 +221,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 v4 10/11] 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.

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.

 xen/arch/arm/p2m.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 948f199d84..f34b6e6f11 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -2265,22 +2265,16 @@ 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 */
         unsigned int root_order; /* Page order of the root of the p2m */
         unsigned int sl0;    /* Desired SL0, maximum in comment */
     } pa_range_info[] __initconst = {
+#ifdef CONFIG_ARM_32
+        [0] = { 40,      24/*24*/,  1,          1 },
+        [1] = { 0 } /* Invalid */
+#else
         /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
         /*      PA size, t0sz(min), root-order, sl0(max) */
         [0] = { 32,      32/*32*/,  0,          1 },
@@ -2291,11 +2285,13 @@ void __init setup_virt_paging(void)
         [5] = { 48,      16/*16*/,  0,          2 },
         [6] = { 52,      12/*12*/,  4,          2 },
         [7] = { 0 }  /* Invalid */
+#endif
     };
 
     unsigned int i;
     unsigned int pa_range = 0x10; /* Larger than any possible value */
 
+#ifdef CONFIG_ARM_64
     /*
      * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured
      * with IPA bits == PA bits, compare against "pabits".
@@ -2309,6 +2305,9 @@ void __init setup_virt_paging(void)
      */
     if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
         max_vmid = MAX_VMID_16_BIT;
+#else
+    p2m_ipa_bits = PADDR_BITS;
+#endif
 
     /* Choose suitable "pa_range" according to the resulted "p2m_ipa_bits". */
     for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
@@ -2324,14 +2323,13 @@ void __init setup_virt_paging(void)
     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);
 
-    val |= VTCR_PS(pa_range);
+#ifdef CONFIG_ARM_64
     val |= VTCR_TG0_4K;
+    val |= VTCR_PS(pa_range);
 
     /* Set the VS bit only if 16 bit VMID is supported. */
     if ( MAX_VMID == MAX_VMID_16_BIT )
         val |= VTCR_VS;
-    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;
@@ -2342,6 +2340,10 @@ void __init setup_virt_paging(void)
            pa_range_info[pa_range].pabits,
            ( MAX_VMID == MAX_VMID_16_BIT ) ? 16 : 8);
 #endif
+
+    val |= VTCR_SL0(pa_range_info[pa_range].sl0);
+    val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
+
     printk("P2M: %d levels with order-%d root, VTCR 0x%"PRIregister"\n",
            4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
 
-- 
2.17.1

[XEN v4 11/11] xen/arm: p2m: Enable support for 32bit IPA for ARM_32

[Ayan Kumar Halder]

The pabits, t0sz, root_order and sl0 values are the same as those for
ARM_64.

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.

 xen/arch/arm/p2m.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f34b6e6f11..20beecc6e8 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -2272,8 +2272,9 @@ void __init setup_virt_paging(void)
         unsigned int sl0;    /* Desired SL0, maximum in comment */
     } pa_range_info[] __initconst = {
 #ifdef CONFIG_ARM_32
-        [0] = { 40,      24/*24*/,  1,          1 },
-        [1] = { 0 } /* Invalid */
+        [0] = { 32,      32/*32*/,  0,          1 },
+        [1] = { 40,      24/*24*/,  1,          1 },
+        [2] = { 0 } /* Invalid */
 #else
         /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
         /*      PA size, t0sz(min), root-order, sl0(max) */
-- 
2.17.1

Re: [XEN v4 04/11] xen/drivers: ns16550: Use paddr_t for io_base/io_size

[Jan Beulich]

On 21.03.2023 15:03, Ayan Kumar Halder wrote:
> @@ -1163,10 +1163,16 @@ static const struct ns16550_config __initconst uart_config[] =
>      },
>  };
>  
> +#define PARSE_ERR_RET(_f, _a...)             \
> +    do {                                     \
> +        printk( "ERROR: " _f "\n" , ## _a ); \
> +        return false;                        \
> +    } while ( 0 )

You can't really re-use this construct unchanged (and perhaps it's also
not worth changing for this single use that you need): Note the "return
false", which ...

>  static int __init
>  pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)

... for a function returning "int" is equivalent to "return 0", which
is kind of a success indicator here. Whatever adjustment you make
needs to be in line with (at least) the two callers checking the
return value (the other two not doing so is suspicious, but then the
way the return values are used is somewhat odd, too).

> @@ -1235,6 +1241,8 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
>                  /* MMIO based */
>                  if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
>                  {
> +                    uint64_t pci_uart_io_base;
> +
>                      pci_conf_write32(PCI_SBDF(0, b, d, f),
>                                       PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
>                      len = pci_conf_read32(PCI_SBDF(0, b, d, f),
> @@ -1259,8 +1267,14 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
>                      else
>                          size = len & PCI_BASE_ADDRESS_MEM_MASK;
>  
> -                    uart->io_base = ((u64)bar_64 << 32) |
> -                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
> +                    pci_uart_io_base = ((u64)bar_64 << 32) |

As you touch this code, please be so kind and also switch to using
uint64_t here.

Also why do you change parse_positional() but not (also)
parse_namevalue_pairs()?

Jan

Re: [XEN v4 07/11] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Jan Beulich]

On 21.03.2023 15:03, Ayan Kumar Halder wrote:
> --- a/xen/arch/Kconfig
> +++ b/xen/arch/Kconfig
> @@ -1,6 +1,12 @@
>  config 64BIT
>  	bool
>  
> +config PHYS_ADDR_T_32
> +	bool
> +
> +config PHYS_ADDR_T_64
> +	bool

Do we really need both? If so, what guards against both being selected
at the same time?

Them being put in common code I consider it an at least latent issue
that you add "select"s ...

> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -9,6 +9,7 @@ config ARM_64
>  	select 64BIT
>  	select ARM_EFI
>  	select HAS_FAST_MULTIPLY
> +	select PHYS_ADDR_T_64
>  
>  config ARM
>  	def_bool y
> @@ -19,13 +20,48 @@ config ARM
>  	select HAS_PMAP
>  	select IOMMU_FORCE_PT_SHARE
>  
> +menu "Architecture Features"
> +
> +choice
> +	prompt "Physical address space size" if ARM_32
> +	default ARM_PA_BITS_48 if ARM_64
> +	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"
> +	help
> +	  On platforms where any physical address can be represented within 32 bits
> +	  , user should choose this option. This will help is reduced size of the
> +	  binary.
> +	select PHYS_ADDR_T_32
> +	depends on ARM_32
> +
> +config ARM_PA_BITS_40
> +	bool "40-bit"
> +	select PHYS_ADDR_T_64
> +	depends on ARM_32
> +
> +config ARM_PA_BITS_48
> +	bool "40-bit"
> +	select PHYS_ADDR_T_64
> +	depends on ARM_48
> +endchoice

... only for Arm. You get away with this only because ...

> --- a/xen/arch/arm/include/asm/types.h
> +++ b/xen/arch/arm/include/asm/types.h
> @@ -34,9 +34,15 @@ typedef signed long long s64;
>  typedef unsigned long long u64;
>  typedef u32 vaddr_t;
>  #define PRIvaddr PRIx32
> +#if defined(CONFIG_PHYS_ADDR_T_32)
> +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)

... you tweak things here, when we're in the process of moving stuff
out of asm/types.h. (Using "unsigned long" for a 32-bit paddr_t is of
course suspicious as well - this ought to be uint32_t.)

Jan

Re: [XEN v4 07/11] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Ayan Kumar Halder]

Hi Jan,

On 21/03/2023 14:22, Jan Beulich wrote:
> On 21.03.2023 15:03, Ayan Kumar Halder wrote:
>> --- a/xen/arch/Kconfig
>> +++ b/xen/arch/Kconfig
>> @@ -1,6 +1,12 @@
>>   config 64BIT
>>   	bool
>>   
>> +config PHYS_ADDR_T_32
>> +	bool
>> +
>> +config PHYS_ADDR_T_64
>> +	bool
> Do we really need both?
I was thinking the same. I am assuming that in future we may have

PHYS_ADDR_T_16, PHYS_ADDR_T_128. So, I am hoping that defining them explicitly might help.
Also, the user cannot select these configs directly.

However, I am open to defining only one of them if it makes sense.

> If so, what guards against both being selected
> at the same time?
At present, we rely on "select".
>
> Them being put in common code I consider it an at least latent issue
> that you add "select"s ...
>
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -9,6 +9,7 @@ config ARM_64
>>   	select 64BIT
>>   	select ARM_EFI
>>   	select HAS_FAST_MULTIPLY
>> +	select PHYS_ADDR_T_64
>>   
>>   config ARM
>>   	def_bool y
>> @@ -19,13 +20,48 @@ config ARM
>>   	select HAS_PMAP
>>   	select IOMMU_FORCE_PT_SHARE
>>   
>> +menu "Architecture Features"
>> +
>> +choice
>> +	prompt "Physical address space size" if ARM_32
>> +	default ARM_PA_BITS_48 if ARM_64
>> +	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"
>> +	help
>> +	  On platforms where any physical address can be represented within 32 bits
>> +	  , user should choose this option. This will help is reduced size of the
>> +	  binary.
>> +	select PHYS_ADDR_T_32
>> +	depends on ARM_32
>> +
>> +config ARM_PA_BITS_40
>> +	bool "40-bit"
>> +	select PHYS_ADDR_T_64
>> +	depends on ARM_32
>> +
>> +config ARM_PA_BITS_48
>> +	bool "40-bit"
>> +	select PHYS_ADDR_T_64
>> +	depends on ARM_48
>> +endchoice
> ... only for Arm. You get away with this only because ...
>
>> --- a/xen/arch/arm/include/asm/types.h
>> +++ b/xen/arch/arm/include/asm/types.h
>> @@ -34,9 +34,15 @@ typedef signed long long s64;
>>   typedef unsigned long long u64;
>>   typedef u32 vaddr_t;
>>   #define PRIvaddr PRIx32in
>> +#if defined(CONFIG_PHYS_ADDR_T_32)
>> +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)
> ... you tweak things here, when we're in the process of moving stuff
> out of asm/types.h.

Are you suggesting not to add anything to asm/types.h ? IOW, the above 
snippet should

be added to xen/include/xen/types.h.

> (Using "unsigned long" for a 32-bit paddr_t is of
> course suspicious as well - this ought to be uint32_t.)

The problem with using uint32_t for paddr_t is that there are instances 
where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.

For eg , handle_passthrough_prop()

             printk(XENLOG_ERR "Unable to permit to dom%d access to"
                    " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
                    kinfo->d->domain_id,
                    mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);

And in xen/include/xen/page-size.h,

#define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
#define PAGE_MASK           (~(PAGE_SIZE-1))

Thus, the resulting types are unsigned long. This cannot be printed 
using %u for PRIpaddr.

I remember some discussion (or comment) that the physical addresses 
should be represented using 'unsigned long'.

- Ayan


>
> Jan

Re: [XEN v4 07/11] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Jan Beulich]

On 21.03.2023 17:15, Ayan Kumar Halder wrote:
> On 21/03/2023 14:22, Jan Beulich wrote:
>> On 21.03.2023 15:03, Ayan Kumar Halder wrote:
>>> --- a/xen/arch/Kconfig
>>> +++ b/xen/arch/Kconfig
>>> @@ -1,6 +1,12 @@
>>>   config 64BIT
>>>   	bool
>>>   
>>> +config PHYS_ADDR_T_32
>>> +	bool
>>> +
>>> +config PHYS_ADDR_T_64
>>> +	bool
>> Do we really need both?
> I was thinking the same. I am assuming that in future we may have
> 
> PHYS_ADDR_T_16,

Really? What contemporary system would be able to run in just 64k?
Certainly not Xen, let alone any Dom0 on top of it.

> PHYS_ADDR_T_128. So, I am hoping that defining them explicitly might help.

Yes, 128-bit addresses may appear at some point. Then (and only then)
we'll need two controls, and we can then think about how to represent
them properly without risking issues.

> Also, the user cannot select these configs directly.

Sure, but at some point some strange combination of options might that
randconfig manages to construct.

>> If so, what guards against both being selected
>> at the same time?
> At present, we rely on "select".

You mean 'we rely on there being only one "select"'? Else I'm afraid I
don't understand your reply.

>> Them being put in common code I consider it an at least latent issue
>> that you add "select"s ...
>>
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -9,6 +9,7 @@ config ARM_64
>>>   	select 64BIT
>>>   	select ARM_EFI
>>>   	select HAS_FAST_MULTIPLY
>>> +	select PHYS_ADDR_T_64
>>>   
>>>   config ARM
>>>   	def_bool y
>>> @@ -19,13 +20,48 @@ config ARM
>>>   	select HAS_PMAP
>>>   	select IOMMU_FORCE_PT_SHARE
>>>   
>>> +menu "Architecture Features"
>>> +
>>> +choice
>>> +	prompt "Physical address space size" if ARM_32
>>> +	default ARM_PA_BITS_48 if ARM_64
>>> +	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"
>>> +	help
>>> +	  On platforms where any physical address can be represented within 32 bits
>>> +	  , user should choose this option. This will help is reduced size of the
>>> +	  binary.
>>> +	select PHYS_ADDR_T_32
>>> +	depends on ARM_32
>>> +
>>> +config ARM_PA_BITS_40
>>> +	bool "40-bit"
>>> +	select PHYS_ADDR_T_64
>>> +	depends on ARM_32
>>> +
>>> +config ARM_PA_BITS_48
>>> +	bool "40-bit"
>>> +	select PHYS_ADDR_T_64
>>> +	depends on ARM_48
>>> +endchoice
>> ... only for Arm. You get away with this only because ...
>>
>>> --- a/xen/arch/arm/include/asm/types.h
>>> +++ b/xen/arch/arm/include/asm/types.h
>>> @@ -34,9 +34,15 @@ typedef signed long long s64;
>>>   typedef unsigned long long u64;
>>>   typedef u32 vaddr_t;
>>>   #define PRIvaddr PRIx32in
>>> +#if defined(CONFIG_PHYS_ADDR_T_32)
>>> +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)
>> ... you tweak things here, when we're in the process of moving stuff
>> out of asm/types.h.
> 
> Are you suggesting not to add anything to asm/types.h ? IOW, the above 
> snippet should
> 
> be added to xen/include/xen/types.h.

It's not your snippet alone, but the definition of paddr_t in general
which should imo be moved (as a follow-on to "common: move standard C
fixed width type declarations to common header"). If your patch in its
present shape landed first, that movement would become more
complicated - first and foremost "select"ing the appropriate
PHYS_ADDR_T_* on x86 and RISC-V would then need to be done there, when
it really doesn't belong there.

>> (Using "unsigned long" for a 32-bit paddr_t is of
>> course suspicious as well - this ought to be uint32_t.)
> 
> The problem with using uint32_t for paddr_t is that there are instances 
> where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.
> 
> For eg , handle_passthrough_prop()
> 
>              printk(XENLOG_ERR "Unable to permit to dom%d access to"
>                     " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
>                     kinfo->d->domain_id,
>                     mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
> 
> And in xen/include/xen/page-size.h,
> 
> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
> #define PAGE_MASK           (~(PAGE_SIZE-1))
> 
> Thus, the resulting types are unsigned long. This cannot be printed 
> using %u for PRIpaddr.

Is there anything wrong with making PAGE_SIZE expand to (1 << PAGE_SHIFT)
when physical addresses are only 32 bits wide?

> I remember some discussion (or comment) that the physical addresses 
> should be represented using 'unsigned long'.

A reference would be helpful.

Jan

Re: [XEN v4 07/11] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Ayan Kumar Halder]

Hi Jan,

On 21/03/2023 16:53, Jan Beulich wrote:
> On 21.03.2023 17:15, Ayan Kumar Halder wrote:
>> On 21/03/2023 14:22, Jan Beulich wrote:
>>> On 21.03.2023 15:03, Ayan Kumar Halder wrote:
>>>> --- a/xen/arch/Kconfig
>>>> +++ b/xen/arch/Kconfig
>>>> @@ -1,6 +1,12 @@
>>>>    config 64BIT
>>>>    	bool
>>>>    
>>>> +config PHYS_ADDR_T_32
>>>> +	bool
>>>> +
>>>> +config PHYS_ADDR_T_64
>>>> +	bool
>>> Do we really need both?
>> I was thinking the same. I am assuming that in future we may have
>>
>> PHYS_ADDR_T_16,
> Really? What contemporary system would be able to run in just 64k?
> Certainly not Xen, let alone any Dom0 on top of it.
>
>> PHYS_ADDR_T_128. So, I am hoping that defining them explicitly might help.
> Yes, 128-bit addresses may appear at some point. Then (and only then)
> we'll need two controls, and we can then think about how to represent
> them properly without risking issues.
>
>> Also, the user cannot select these configs directly.
> Sure, but at some point some strange combination of options might that
> randconfig manages to construct.
>
>>> If so, what guards against both being selected
>>> at the same time?
>> At present, we rely on "select".
> You mean 'we rely on there being only one "select"'?
Yes, that was what I meant.
> Else I'm afraid I
> don't understand your reply.
>
>>> Them being put in common code I consider it an at least latent issue
>>> that you add "select"s ...
>>>
>>>> --- a/xen/arch/arm/Kconfig
>>>> +++ b/xen/arch/arm/Kconfig
>>>> @@ -9,6 +9,7 @@ config ARM_64
>>>>    	select 64BIT
>>>>    	select ARM_EFI
>>>>    	select HAS_FAST_MULTIPLY
>>>> +	select PHYS_ADDR_T_64
>>>>    
>>>>    config ARM
>>>>    	def_bool y
>>>> @@ -19,13 +20,48 @@ config ARM
>>>>    	select HAS_PMAP
>>>>    	select IOMMU_FORCE_PT_SHARE
>>>>    
>>>> +menu "Architecture Features"
>>>> +
>>>> +choice
>>>> +	prompt "Physical address space size" if ARM_32
>>>> +	default ARM_PA_BITS_48 if ARM_64
>>>> +	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"
>>>> +	help
>>>> +	  On platforms where any physical address can be represented within 32 bits
>>>> +	  , user should choose this option. This will help is reduced size of the
>>>> +	  binary.
>>>> +	select PHYS_ADDR_T_32
>>>> +	depends on ARM_32
>>>> +
>>>> +config ARM_PA_BITS_40
>>>> +	bool "40-bit"
>>>> +	select PHYS_ADDR_T_64
>>>> +	depends on ARM_32
>>>> +
>>>> +config ARM_PA_BITS_48
>>>> +	bool "40-bit"
>>>> +	select PHYS_ADDR_T_64
>>>> +	depends on ARM_48
>>>> +endchoice
>>> ... only for Arm. You get away with this only because ...
>>>
>>>> --- a/xen/arch/arm/include/asm/types.h
>>>> +++ b/xen/arch/arm/include/asm/types.h
>>>> @@ -34,9 +34,15 @@ typedef signed long long s64;
>>>>    typedef unsigned long long u64;
>>>>    typedef u32 vaddr_t;
>>>>    #define PRIvaddr PRIx32in
>>>> +#if defined(CONFIG_PHYS_ADDR_T_32)
>>>> +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)
>>> ... you tweak things here, when we're in the process of moving stuff
>>> out of asm/types.h.
>> Are you suggesting not to add anything to asm/types.h ? IOW, the above
>> snippet should
>>
>> be added to xen/include/xen/types.h.
> It's not your snippet alone, but the definition of paddr_t in general
> which should imo be moved (as a follow-on to "common: move standard C
> fixed width type declarations to common header"). If your patch in its
> present shape landed first, that movement would become more
> complicated - first and foremost "select"ing the appropriate
> PHYS_ADDR_T_* on x86 and RISC-V would then need to be done there, when
> it really doesn't belong there.

I understand your point. I am assuming that as PHYS_ADDR_T_* is now 
introduced at xen/arch/Kconfig, so x86 or RISC-V should be able to 
select the option.

As I see today, for :-

RISCV defines PADDR_BITS to 56. Thus, it will select PHYS_ADDR_T_64 only.

X86 defines PADDR_BITS to 52. Thus, it will also select PHYS_ADDR_T_64 only.

For Arm, there will be at least two configurations 1. which selects 
PHYS_ADDR_T_64   2. not select PHYS_ADDR_T_64 (ie for 32 bit physical 
address).

>
>>> (Using "unsigned long" for a 32-bit paddr_t is of
>>> course suspicious as well - this ought to be uint32_t.)
>> The problem with using uint32_t for paddr_t is that there are instances
>> where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.
>>
>> For eg , handle_passthrough_prop()
>>
>>               printk(XENLOG_ERR "Unable to permit to dom%d access to"
>>                      " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
>>                      kinfo->d->domain_id,
>>                      mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
>>
>> And in xen/include/xen/page-size.h,
>>
>> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
>> #define PAGE_MASK           (~(PAGE_SIZE-1))
>>
>> Thus, the resulting types are unsigned long. This cannot be printed
>> using %u for PRIpaddr.
> Is there anything wrong with making PAGE_SIZE expand to (1 << PAGE_SHIFT)
> when physical addresses are only 32 bits wide?

I don't have a strong objection except that this is similar to what 
linux is doing today.

https://elixir.bootlin.com/linux/latest/source/arch/arm/include/asm/page.h#L12

>
>> I remember some discussion (or comment) that the physical addresses
>> should be represented using 'unsigned long'.
> A reference would be helpful.

https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00305.html

- Ayan

>
> Jan

Re: [XEN v4 07/11] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Jan Beulich]

On 21.03.2023 19:33, Ayan Kumar Halder wrote:
> On 21/03/2023 16:53, Jan Beulich wrote:
>> On 21.03.2023 17:15, Ayan Kumar Halder wrote:
>>> On 21/03/2023 14:22, Jan Beulich wrote:
>>>> (Using "unsigned long" for a 32-bit paddr_t is of
>>>> course suspicious as well - this ought to be uint32_t.)
>>> The problem with using uint32_t for paddr_t is that there are instances
>>> where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.
>>>
>>> For eg , handle_passthrough_prop()
>>>
>>>               printk(XENLOG_ERR "Unable to permit to dom%d access to"
>>>                      " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
>>>                      kinfo->d->domain_id,
>>>                      mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
>>>
>>> And in xen/include/xen/page-size.h,
>>>
>>> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
>>> #define PAGE_MASK           (~(PAGE_SIZE-1))
>>>
>>> Thus, the resulting types are unsigned long. This cannot be printed
>>> using %u for PRIpaddr.
>> Is there anything wrong with making PAGE_SIZE expand to (1 << PAGE_SHIFT)
>> when physical addresses are only 32 bits wide?
> 
> I don't have a strong objection except that this is similar to what 
> linux is doing today.
> 
> https://elixir.bootlin.com/linux/latest/source/arch/arm/include/asm/page.h#L12
> 
>>
>>> I remember some discussion (or comment) that the physical addresses
>>> should be represented using 'unsigned long'.
>> A reference would be helpful.
> 
> https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00305.html

I see. I guess this will be okay as long as only 32-bit arches elect to
use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON()
somewhere, accompanied by a suitable comment?

Jan

Re: [XEN v4 07/11] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Julien Grall]

Hi Jan,

On 22/03/2023 06:59, Jan Beulich wrote:
> On 21.03.2023 19:33, Ayan Kumar Halder wrote:
>> On 21/03/2023 16:53, Jan Beulich wrote:
>>> On 21.03.2023 17:15, Ayan Kumar Halder wrote:
>>>> On 21/03/2023 14:22, Jan Beulich wrote:
>>>>> (Using "unsigned long" for a 32-bit paddr_t is of
>>>>> course suspicious as well - this ought to be uint32_t.)
>>>> The problem with using uint32_t for paddr_t is that there are instances
>>>> where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.
>>>>
>>>> For eg , handle_passthrough_prop()
>>>>
>>>>                printk(XENLOG_ERR "Unable to permit to dom%d access to"
>>>>                       " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
>>>>                       kinfo->d->domain_id,
>>>>                       mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
>>>>
>>>> And in xen/include/xen/page-size.h,
>>>>
>>>> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
>>>> #define PAGE_MASK           (~(PAGE_SIZE-1))
>>>>
>>>> Thus, the resulting types are unsigned long. This cannot be printed
>>>> using %u for PRIpaddr.
>>> Is there anything wrong with making PAGE_SIZE expand to (1 << PAGE_SHIFT)
>>> when physical addresses are only 32 bits wide?
>>
>> I don't have a strong objection except that this is similar to what
>> linux is doing today.
>>
>> https://elixir.bootlin.com/linux/latest/source/arch/arm/include/asm/page.h#L12
>>
>>>
>>>> I remember some discussion (or comment) that the physical addresses
>>>> should be represented using 'unsigned long'.
>>> A reference would be helpful.
>>
>> https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00305.html
> 
> I see. I guess this will be okay as long as only 32-bit arches elect to
> use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON()
> somewhere, accompanied by a suitable comment?

Hmmm... We definitely have 40-bits physical address space on Arm32. In 
fact, my suggestion was not to define paddr_t as unsigned long for 
everyone but only when PHYS_ADDR_T_32 is selected (AFAICT this is what 
is done in this patch).

This is to avoid having to add cast everywhere we are using PAGE_* on 
paddr_t and print it.

That said, I realize this means that for 64-bit, we would still use 
64-bit integer. I view it as a less a problem (at least on Arm) because 
registers are always 64-bit. I am open to other suggestion.

Cheers,

-- 
Julien Grall

Re: [XEN v4 07/11] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Jan Beulich]

On 22.03.2023 14:29, Julien Grall wrote:
> On 22/03/2023 06:59, Jan Beulich wrote:
>> On 21.03.2023 19:33, Ayan Kumar Halder wrote:
>>> On 21/03/2023 16:53, Jan Beulich wrote:
>>>> On 21.03.2023 17:15, Ayan Kumar Halder wrote:
>>>>> On 21/03/2023 14:22, Jan Beulich wrote:
>>>>>> (Using "unsigned long" for a 32-bit paddr_t is of
>>>>>> course suspicious as well - this ought to be uint32_t.)
>>>>> The problem with using uint32_t for paddr_t is that there are instances
>>>>> where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.
>>>>>
>>>>> For eg , handle_passthrough_prop()
>>>>>
>>>>>                printk(XENLOG_ERR "Unable to permit to dom%d access to"
>>>>>                       " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
>>>>>                       kinfo->d->domain_id,
>>>>>                       mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
>>>>>
>>>>> And in xen/include/xen/page-size.h,
>>>>>
>>>>> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
>>>>> #define PAGE_MASK           (~(PAGE_SIZE-1))
>>>>>
>>>>> Thus, the resulting types are unsigned long. This cannot be printed
>>>>> using %u for PRIpaddr.
>>>> Is there anything wrong with making PAGE_SIZE expand to (1 << PAGE_SHIFT)
>>>> when physical addresses are only 32 bits wide?
>>>
>>> I don't have a strong objection except that this is similar to what
>>> linux is doing today.
>>>
>>> https://elixir.bootlin.com/linux/latest/source/arch/arm/include/asm/page.h#L12
>>>
>>>>
>>>>> I remember some discussion (or comment) that the physical addresses
>>>>> should be represented using 'unsigned long'.
>>>> A reference would be helpful.
>>>
>>> https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00305.html
>>
>> I see. I guess this will be okay as long as only 32-bit arches elect to
>> use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON()
>> somewhere, accompanied by a suitable comment?
> 
> Hmmm... We definitely have 40-bits physical address space on Arm32. In 
> fact, my suggestion was not to define paddr_t as unsigned long for 
> everyone but only when PHYS_ADDR_T_32 is selected (AFAICT this is what 
> is done in this patch).
> 
> This is to avoid having to add cast everywhere we are using PAGE_* on 
> paddr_t and print it.
> 
> That said, I realize this means that for 64-bit, we would still use 
> 64-bit integer. I view it as a less a problem (at least on Arm) because 
> registers are always 64-bit. I am open to other suggestion.

It simply struck me as odd to use a 64-bit type for something that was
explicitly said is only going to be 32 bits wide. I would therefore
prefer if we could limit 32-bit paddr_t to 32-bit architectures for
now, as expressed before when asking for a respective BUILD_BUG_ON().
Especially if, as intended, the type definition moves to xen/types.h
(and hence isn't Arm-specific anymore).

Jan

Re: [XEN v4 07/11] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Ayan Kumar Halder]

Hi Jan, Julien,

On 22/03/2023 13:53, Jan Beulich wrote:
> On 22.03.2023 14:29, Julien Grall wrote:
>> On 22/03/2023 06:59, Jan Beulich wrote:
>>> On 21.03.2023 19:33, Ayan Kumar Halder wrote:
>>>> On 21/03/2023 16:53, Jan Beulich wrote:
>>>>> On 21.03.2023 17:15, Ayan Kumar Halder wrote:
>>>>>> On 21/03/2023 14:22, Jan Beulich wrote:
>>>>>>> (Using "unsigned long" for a 32-bit paddr_t is of
>>>>>>> course suspicious as well - this ought to be uint32_t.)
>>>>>> The problem with using uint32_t for paddr_t is that there are instances
>>>>>> where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.
>>>>>>
>>>>>> For eg , handle_passthrough_prop()
>>>>>>
>>>>>>                 printk(XENLOG_ERR "Unable to permit to dom%d access to"
>>>>>>                        " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
>>>>>>                        kinfo->d->domain_id,
>>>>>>                        mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
>>>>>>
>>>>>> And in xen/include/xen/page-size.h,
>>>>>>
>>>>>> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
>>>>>> #define PAGE_MASK           (~(PAGE_SIZE-1))
>>>>>>
>>>>>> Thus, the resulting types are unsigned long. This cannot be printed
>>>>>> using %u for PRIpaddr.
>>>>> Is there anything wrong with making PAGE_SIZE expand to (1 << PAGE_SHIFT)
>>>>> when physical addresses are only 32 bits wide?
>>>> I don't have a strong objection except that this is similar to what
>>>> linux is doing today.
>>>>
>>>> https://elixir.bootlin.com/linux/latest/source/arch/arm/include/asm/page.h#L12
>>>>
>>>>>> I remember some discussion (or comment) that the physical addresses
>>>>>> should be represented using 'unsigned long'.
>>>>> A reference would be helpful.
>>>> https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00305.html
>>> I see. I guess this will be okay as long as only 32-bit arches elect to
>>> use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON()
>>> somewhere, accompanied by a suitable comment?
>> Hmmm... We definitely have 40-bits physical address space on Arm32. In
>> fact, my suggestion was not to define paddr_t as unsigned long for
>> everyone but only when PHYS_ADDR_T_32 is selected (AFAICT this is what
>> is done in this patch).
>>
>> This is to avoid having to add cast everywhere we are using PAGE_* on
>> paddr_t and print it.
>>
>> That said, I realize this means that for 64-bit, we would still use
>> 64-bit integer. I view it as a less a problem (at least on Arm) because
>> registers are always 64-bit. I am open to other suggestion.
> It simply struck me as odd to use a 64-bit type for something that was
> explicitly said is only going to be 32 bits wide. I would therefore
> prefer if we could limit 32-bit paddr_t to 32-bit architectures for
> now, as expressed before when asking for a respective BUILD_BUG_ON().
> Especially if, as intended, the type definition moves to xen/types.h
> (and hence isn't Arm-specific anymore).
>
> Jan

Please have a look at the below patch and let me know your thoughts. 
This patch now :-

1. Removes the config "PHYS_ADDR_T_64".  So when PHYS_ADDR_T_32 is not 
selected, it means that physical addresses are to be denoted by 64 bits.

2. Added a BUILD_BUG_ON() to check that paddr_t is exactly 32-bit wide 
when CONFIG_PHYS_ADDR_T_32 is selected. As 32-bit Arm architecture 
(Arm_32) can support 40 bits PA with LPAE, thus we cannot always use 
32-bit paddr_t.

3. For Jan's concern that the changes to 
xen/arch/arm/include/asm/types.h will complicate movement to common 
header, I think we will need to use CONFIG_PHYS_ADDR_T_32 to define 
types for 32-bit physical addresses.

I am open to any alternative suggestions that you propose.


commit 3a61721a5169072b4aa3bbd0df38de5e69a5abc1
Author: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Date:   Wed Feb 8 12:05:26 2023 +0000

     xen/arm: Introduce choice to enable 64/32 bit physical addressing

     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
     configs 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.
     When PHYS_ADDR_T_32 is not defined for ARM_32, PADDR_BITS is set to 40.
     When PHYS_ADDR_T_32 is not defined for ARM_64, PADDR_BITS is set to 48.
     The last two are same as the current configuration used today on Xen.

     PADDR_BITS is also set to 48 when ARM_64 is defined. The reason being
     the choice to select ARM_PA_BITS_32/ARM_PA_BITS_40/ARM_PA_BITS_48 is
     currently allowed when ARM_32 is defined.

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

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..e6dadeb8b1 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -19,13 +19,46 @@ config ARM
     select HAS_PMAP
     select IOMMU_FORCE_PT_SHARE

+menu "Architecture Features"
+
+choice
+   prompt "Physical address space size" if ARM_32
+   default ARM_PA_BITS_48 if ARM_64
+   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"
+   help
+     On platforms where any physical address can be represented within 
32 bits
+     , user should choose this option. This will help is reduced size 
of the
+     binary.
+   select PHYS_ADDR_T_32
+   depends on ARM_32
+
+config ARM_PA_BITS_40
+   bool "40-bit"
+   depends on ARM_32
+
+config ARM_PA_BITS_48
+   bool "40-bit"
+   depends on ARM_48
+endchoice
+
+config PADDR_BITS
+   int
+   default 32 if ARM_PA_BITS_32
+   default 40 if ARM_PA_BITS_40
+   default 48 if ARM_PA_BITS_48 || ARM_64
+
  config ARCH_DEFCONFIG
     string
     default "arch/arm/configs/arm32_defconfig" if ARM_32
     default "arch/arm/configs/arm64_defconfig" if ARM_64

-menu "Architecture Features"
-
  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..e3cfbbb060 100644
--- a/xen/arch/arm/include/asm/types.h
+++ b/xen/arch/arm/include/asm/types.h
@@ -34,9 +34,15 @@ typedef signed long long s64;
  typedef unsigned long long u64;
  typedef u32 vaddr_t;
  #define PRIvaddr PRIx32
+#if defined(CONFIG_PHYS_ADDR_T_32)
+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 b99806af99..6dc37be97e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -690,6 +690,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;

+   /* Check that paddr_t is exactly 32 bits when CONFIG_PHYS_ADDR_T_32 
is defined */

+   #ifdef  CONFIG_PHYS_ADDR_T_32

+   BUILD_BUG_ON((sizeof(paddr_t) * 8) != 32);

+  #endif

+    /*
+     * The size of paddr_t should be sufficient for the complete range of
+     * physical address.
+     */
+    BUILD_BUG_ON((sizeof(paddr_t) * 8) < PADDR_BITS);
      BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);

      if ( frametable_size > FRAMETABLE_SIZE )

- Ayan

Re: [XEN v4 07/11] xen/arm: Introduce choice to enable 64/32 bit physical addressing

[Julien Grall]

Hi,

On 27/03/2023 12:46, Ayan Kumar Halder wrote:
> On 22/03/2023 13:53, Jan Beulich wrote:
>> On 22.03.2023 14:29, Julien Grall wrote:
>>> On 22/03/2023 06:59, Jan Beulich wrote:
>>>> On 21.03.2023 19:33, Ayan Kumar Halder wrote:
>>>>> On 21/03/2023 16:53, Jan Beulich wrote:
>>>>>> On 21.03.2023 17:15, Ayan Kumar Halder wrote:
>>>>>>> On 21/03/2023 14:22, Jan Beulich wrote:
>>>>>>>> (Using "unsigned long" for a 32-bit paddr_t is of
>>>>>>>> course suspicious as well - this ought to be uint32_t.)
>>>>>>> The problem with using uint32_t for paddr_t is that there are 
>>>>>>> instances
>>>>>>> where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.
>>>>>>>
>>>>>>> For eg , handle_passthrough_prop()
>>>>>>>
>>>>>>>                 printk(XENLOG_ERR "Unable to permit to dom%d 
>>>>>>> access to"
>>>>>>>                        " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
>>>>>>>                        kinfo->d->domain_id,
>>>>>>>                        mstart & PAGE_MASK, PAGE_ALIGN(mstart + 
>>>>>>> size) - 1);
>>>>>>>
>>>>>>> And in xen/include/xen/page-size.h,
>>>>>>>
>>>>>>> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
>>>>>>> #define PAGE_MASK           (~(PAGE_SIZE-1))
>>>>>>>
>>>>>>> Thus, the resulting types are unsigned long. This cannot be printed
>>>>>>> using %u for PRIpaddr.
>>>>>> Is there anything wrong with making PAGE_SIZE expand to (1 << 
>>>>>> PAGE_SHIFT)
>>>>>> when physical addresses are only 32 bits wide?
>>>>> I don't have a strong objection except that this is similar to what
>>>>> linux is doing today.
>>>>>
>>>>> https://elixir.bootlin.com/linux/latest/source/arch/arm/include/asm/page.h#L12
>>>>>
>>>>>>> I remember some discussion (or comment) that the physical addresses
>>>>>>> should be represented using 'unsigned long'.
>>>>>> A reference would be helpful.
>>>>> https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00305.html
>>>> I see. I guess this will be okay as long as only 32-bit arches elect to
>>>> use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON()
>>>> somewhere, accompanied by a suitable comment?
>>> Hmmm... We definitely have 40-bits physical address space on Arm32. In
>>> fact, my suggestion was not to define paddr_t as unsigned long for
>>> everyone but only when PHYS_ADDR_T_32 is selected (AFAICT this is what
>>> is done in this patch).
>>>
>>> This is to avoid having to add cast everywhere we are using PAGE_* on
>>> paddr_t and print it.
>>>
>>> That said, I realize this means that for 64-bit, we would still use
>>> 64-bit integer. I view it as a less a problem (at least on Arm) because
>>> registers are always 64-bit. I am open to other suggestion.
>> It simply struck me as odd to use a 64-bit type for something that was
>> explicitly said is only going to be 32 bits wide. I would therefore
>> prefer if we could limit 32-bit paddr_t to 32-bit architectures for
>> now, as expressed before when asking for a respective BUILD_BUG_ON().
>> Especially if, as intended, the type definition moves to xen/types.h
>> (and hence isn't Arm-specific anymore).
>>
>> Jan
> 
> Please have a look at the below patch and let me know your thoughts. 
> This patch now :-
> 
> 1. Removes the config "PHYS_ADDR_T_64".  So when PHYS_ADDR_T_32 is not 
> selected, it means that physical addresses are to be denoted by 64 bits.
> 
> 2. Added a BUILD_BUG_ON() to check that paddr_t is exactly 32-bit wide 
> when CONFIG_PHYS_ADDR_T_32 is selected. As 32-bit Arm architecture 
> (Arm_32) can support 40 bits PA with LPAE, thus we cannot always use 
> 32-bit paddr_t.
> 
> 3. For Jan's concern that the changes to 
> xen/arch/arm/include/asm/types.h will complicate movement to common 
> header, I think we will need to use CONFIG_PHYS_ADDR_T_32 to define 
> types for 32-bit physical addresses.
> 
> I am open to any alternative suggestions that you propose.
> 
> 
> commit 3a61721a5169072b4aa3bbd0df38de5e69a5abc1
> Author: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Date:   Wed Feb 8 12:05:26 2023 +0000
> 
>      xen/arm: Introduce choice to enable 64/32 bit physical addressing
> 
>      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
>      configs 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.
>      When PHYS_ADDR_T_32 is not defined for ARM_32, PADDR_BITS is set to 
> 40.
>      When PHYS_ADDR_T_32 is not defined for ARM_64, PADDR_BITS is set to 
> 48.
>      The last two are same as the current configuration used today on Xen.
> 
>      PADDR_BITS is also set to 48 when ARM_64 is defined. The reason being
>      the choice to select ARM_PA_BITS_32/ARM_PA_BITS_40/ARM_PA_BITS_48 is
>      currently allowed when ARM_32 is defined.
> 
>      Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> 
> 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..e6dadeb8b1 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -19,13 +19,46 @@ config ARM
>      select HAS_PMAP
>      select IOMMU_FORCE_PT_SHARE
> 
> +menu "Architecture Features"
> +
> +choice
> +   prompt "Physical address space size" if ARM_32
> +   default ARM_PA_BITS_48 if ARM_64
> +   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"
> +   help
> +     On platforms where any physical address can be represented within 
> 32 bits
> +     , user should choose this option. This will help is reduced size 
> of the

Typo: I think it is more common to have the ',' at the end of the line 
rather than a the beginning followed by a space.

> +     binary.
> +   select PHYS_ADDR_T_32
> +   depends on ARM_32
> +
> +config ARM_PA_BITS_40
> +   bool "40-bit"
> +   depends on ARM_32
> +
> +config ARM_PA_BITS_48
> +   bool "40-bit"
> +   depends on ARM_48
> +endchoice
> +
> +config PADDR_BITS
> +   int
> +   default 32 if ARM_PA_BITS_32
> +   default 40 if ARM_PA_BITS_40
> +   default 48 if ARM_PA_BITS_48 || ARM_64
> +
>   config ARCH_DEFCONFIG
>      string
>      default "arch/arm/configs/arm32_defconfig" if ARM_32
>      default "arch/arm/configs/arm64_defconfig" if ARM_64
> 
> -menu "Architecture Features"
> -
>   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..e3cfbbb060 100644
> --- a/xen/arch/arm/include/asm/types.h
> +++ b/xen/arch/arm/include/asm/types.h
> @@ -34,9 +34,15 @@ typedef signed long long s64;
>   typedef unsigned long long u64;
>   typedef u32 vaddr_t;
>   #define PRIvaddr PRIx32
> +#if defined(CONFIG_PHYS_ADDR_T_32)
> +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 b99806af99..6dc37be97e 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -690,6 +690,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;
> 
> +   /* Check that paddr_t is exactly 32 bits when CONFIG_PHYS_ADDR_T_32 
> is defined */

This is only describing the BUILD_BUG_ON() in words but don't really say 
why. In fact...

> 
> +   #ifdef  CONFIG_PHYS_ADDR_T_32
> 
> +   BUILD_BUG_ON((sizeof(paddr_t) * 8) != 32);
> 
> +  #endif

... nothing really wrong will happen if paddr_t is bigger. The code will 
just be less optimized. So I would drop it.

If there is a desire to keep it then it should be moved in 
build_assertions() with a suitable comment (e.g. what could go wrong if 
the build assertion fail).

Cheers,

-- 
Julien Grall

Re: [XEN v4 04/11] xen/drivers: ns16550: Use paddr_t for io_base/io_size

[Ayan Kumar Halder]

Hi Jan,

On 21/03/2023 14:16, Jan Beulich wrote:
> On 21.03.2023 15:03, Ayan Kumar Halder wrote:
>> @@ -1163,10 +1163,16 @@ static const struct ns16550_config __initconst uart_config[] =
>>       },
>>   };
>>   
>> +#define PARSE_ERR_RET(_f, _a...)             \
>> +    do {                                     \
>> +        printk( "ERROR: " _f "\n" , ## _a ); \
>> +        return false;                        \
>> +    } while ( 0 )
> You can't really re-use this construct unchanged (and perhaps it's also
> not worth changing for this single use that you need): Note the "return
> false", which ...
>
>>   static int __init
>>   pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
> ... for a function returning "int" is equivalent to "return 0", which
> is kind of a success indicator here. Whatever adjustment you make
> needs to be in line with (at least) the two callers checking the
> return value (the other two not doing so is suspicious, but then the
> way the return values are used is somewhat odd, too).
>
>> @@ -1235,6 +1241,8 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
>>                   /* MMIO based */
>>                   if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
>>                   {
>> +                    uint64_t pci_uart_io_base;
>> +
>>                       pci_conf_write32(PCI_SBDF(0, b, d, f),
>>                                        PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
>>                       len = pci_conf_read32(PCI_SBDF(0, b, d, f),
>> @@ -1259,8 +1267,14 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
>>                       else
>>                           size = len & PCI_BASE_ADDRESS_MEM_MASK;
>>   
>> -                    uart->io_base = ((u64)bar_64 << 32) |
>> -                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
>> +                    pci_uart_io_base = ((u64)bar_64 << 32) |
> As you touch this code, please be so kind and also switch to using
> uint64_t here.
>
> Also why do you change parse_positional() but not (also)
> parse_namevalue_pairs()?
>
> Jan

Please let me know if the below patch looks fine.

     xen/drivers: ns16550: Use paddr_t for io_base/io_size

     io_base and io_size represent physical addresses. So they should use
     paddr_t (instead of u64).

     However in future, paddr_t may be defined as u32. So when typecasting
     values from u64 to paddr_t, one should always check for any possible
     truncation. If any truncation is discovered, Xen needs to return an
     appropriate an error message for this.

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

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 092f6b9c4b..5c52e7e642 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -42,8 +42,8 @@

  static struct ns16550 {
      int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
-    u64 io_base;   /* I/O port or memory-mapped I/O address. */
-    u64 io_size;
+    paddr_t io_base;   /* I/O port or memory-mapped I/O address. */
+    paddr_t io_size;
      int reg_shift; /* Bits to shift register offset by */
      int reg_width; /* Size of access to use, the registers
                      * themselves are still bytes */
@@ -1166,7 +1166,7 @@ static const struct ns16550_config __initconst 
uart_config[] =
  static int __init
  pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
  {
-    u64 orig_base = uart->io_base;
+    paddr_t orig_base = uart->io_base;
      unsigned int b, d, f, nextf, i;

      /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on 
bus 0. */
@@ -1235,6 +1235,8 @@ pci_uart_config(struct ns16550 *uart, bool_t 
skip_amt, unsigned int idx)
                  /* MMIO based */
                  if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
                  {
+                    uint64_t pci_uart_io_base;
+
                      pci_conf_write32(PCI_SBDF(0, b, d, f),
                                       PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
                      len = pci_conf_read32(PCI_SBDF(0, b, d, f),
@@ -1259,8 +1261,17 @@ pci_uart_config(struct ns16550 *uart, bool_t 
skip_amt, unsigned int idx)
                      else
                          size = len & PCI_BASE_ADDRESS_MEM_MASK;

-                    uart->io_base = ((u64)bar_64 << 32) |
-                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
+                    pci_uart_io_base = ((uint64_t)bar_64 << 32) |
+                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
+
+                    /* Truncation detected while converting to paddr_t */
+                    if ( pci_uart_io_base != (paddr_t)pci_uart_io_base )
+                    {
+                        printk("ERROR: Truncation detected for io_base 
address");
+                        return -EINVAL;
+                    }
+
+                    uart->io_base = pci_uart_io_base;
                  }
                  /* IO based */
                  else if ( !param->mmio && (bar & 
PCI_BASE_ADDRESS_SPACE_IO) )
@@ -1519,20 +1530,40 @@ static bool __init parse_positional(struct 
ns16550 *uart, char **str)
  #ifdef CONFIG_HAS_PCI
          if ( strncmp(conf, "pci", 3) == 0 )
          {
-            if ( pci_uart_config(uart, 1/* skip AMT */, uart - 
ns16550_com) )
+            int ret;
+
+            ret = pci_uart_config(uart, 1/* skip AMT */, uart - 
ns16550_com);
+
+            if ( ret == -EINVAL )
+                return false;
+            else if ( ret )
                  return true;
+
              conf += 3;
          }
          else if ( strncmp(conf, "amt", 3) == 0 )
          {
-            if ( pci_uart_config(uart, 0, uart - ns16550_com) )
+            int ret = pci_uart_config(uart, 0, uart - ns16550_com);
+
+            if ( ret == -EINVAL )
+                return false;
+            else if ( ret )
                  return true;
+
              conf += 3;
          }
          else
  #endif
          {
-            uart->io_base = simple_strtoull(conf, &conf, 0);
+            uint64_t uart_io_base;
+
+            uart_io_base = simple_strtoull(conf, &conf, 0);
+
+            /* Truncation detected while converting to paddr_t */
+            if ( uart_io_base != (paddr_t)uart_io_base )
+                PARSE_ERR_RET("Truncation detected for uart_io_base 
address");
+
+            uart->io_base = uart_io_base;
          }
      }

@@ -1577,6 +1608,7 @@ static bool __init parse_namevalue_pairs(char 
*str, struct ns16550 *uart)
      char *token, *start = str;
      char *param_value = NULL;
      bool dev_set = false;
+    uint64_t uart_io_base;

      if ( (str == NULL) || (*str == '\0') )
          return true;
@@ -1603,7 +1635,14 @@ static bool __init parse_namevalue_pairs(char 
*str, struct ns16550 *uart)
                         "Can't use io_base with dev=pci or dev=amt 
options\n");
                  break;
              }
-            uart->io_base = simple_strtoull(param_value, NULL, 0);
+

+            uart_io_base = simple_strtoull(param_value, NULL, 0);

+            uart->io_base = uart_io_base;
+            if ( uart_io_base != uart->io_base )
+                PARSE_ERR_RET("Truncation detected for io_base address");
+
              break;

          case irq:


- Ayan

Re: [XEN v4 04/11] xen/drivers: ns16550: Use paddr_t for io_base/io_size

[Jan Beulich]

On 29.03.2023 16:35, Ayan Kumar Halder wrote:
> Please let me know if the below patch looks fine.

Apart from the comments below there may be formatting issues, which
I can't sensibly comment on when the patch was mangled by your mailer
anyway. (Which in turn is why it is generally better to properly send
a new version, rather than replying with kind-of-a-new-version on an
earlier thread.)

Additionally, up front: I'm sorry for the extra requests, but I'm
afraid to sensibly make the changes you want to make some things need
sorting first, to avoid extending pre-existing clumsiness. This is
irrespective of the present state of things clearly not being your
fault.

> @@ -1235,6 +1235,8 @@ pci_uart_config(struct ns16550 *uart, bool_t 
> skip_amt, unsigned int idx)
>                   /* MMIO based */
>                   if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
>                   {
> +                    uint64_t pci_uart_io_base;
> +
>                       pci_conf_write32(PCI_SBDF(0, b, d, f),
>                                        PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
>                       len = pci_conf_read32(PCI_SBDF(0, b, d, f),
> @@ -1259,8 +1261,17 @@ pci_uart_config(struct ns16550 *uart, bool_t 
> skip_amt, unsigned int idx)
>                       else
>                           size = len & PCI_BASE_ADDRESS_MEM_MASK;
> 
> -                    uart->io_base = ((u64)bar_64 << 32) |
> -                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
> +                    pci_uart_io_base = ((uint64_t)bar_64 << 32) |
> +                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
> +
> +                    /* Truncation detected while converting to paddr_t */
> +                    if ( pci_uart_io_base != (paddr_t)pci_uart_io_base )
> +                    {
> +                        printk("ERROR: Truncation detected for io_base 
> address");
> +                        return -EINVAL;
> +                    }

Further down the function returns -1, so here you assume EINVAL != 1.
Such assumptions (and mixing of value spaces) is generally not a good
idea. Since there are other issues (see below), maybe you really want
to add a prereq patch addressing those? That would include changing the
"return -1" to either "return 1" or making it use some sensible and
properly distinguishable errno value.

> @@ -1519,20 +1530,40 @@ static bool __init parse_positional(struct 
> ns16550 *uart, char **str)
>   #ifdef CONFIG_HAS_PCI
>           if ( strncmp(conf, "pci", 3) == 0 )
>           {
> -            if ( pci_uart_config(uart, 1/* skip AMT */, uart - 
> ns16550_com) )
> +            int ret;
> +
> +            ret = pci_uart_config(uart, 1/* skip AMT */, uart - 
> ns16550_com);
> +
> +            if ( ret == -EINVAL )
> +                return false;
> +            else if ( ret )
>                   return true;

With skip_amt != 0 the function presently can only return 0. You're
therefore converting pre-existing dead code to another form of dead
code. Otoh (and as, I think, previously indicated) ...

> +
>               conf += 3;
>           }
>           else if ( strncmp(conf, "amt", 3) == 0 )
>           {
> -            if ( pci_uart_config(uart, 0, uart - ns16550_com) )
> +            int ret = pci_uart_config(uart, 0, uart - ns16550_com);
> +
> +            if ( ret == -EINVAL )
> +                return false;
> +            else if ( ret )
>                   return true;

... the equivalent of this in parse_namevalue_pairs() not checking
the return value is bogus. But it is further bogus that the case
where skip_amt has passed 1 for it sets dev_set to true
unconditionally, i.e. even when no device was found. IOW I also
question the correctness of the final "return 0" in pci_uart_config().
I looks to me as if this wants to be a skip_amt-independent
"return -ENODEV". skip_amt would only control whether uart->io_base is
restored before returning (leaving aside the question of why that is).

Jan

Re: [XEN v4 01/11] xen/arm: Use the correct format specifier

[Julien Grall]

Hi,

On 21/03/2023 14:03, Ayan Kumar Halder wrote:
> 1. One should use 'PRIpaddr' to display 'paddr_t' variables. However,
> while creating nodes in fdt, the address (if present in the node name)
> should be represented using 'PRIx64'. This is to be in conformance
> with the following rule present in https://elinux.org/Device_Tree_Linux
> 
> . node names
> "unit-address does not have leading zeros"
> 
> As 'PRIpaddr' introduces leading zeros, we cannot use it.
> 
> So, we have introduced a wrapper ie domain_fdt_begin_node() which will
> represent physical address using 'PRIx64'.
> 
> 2. One should use 'PRIx64' to display 'u64' in hex format. The current
> use of 'PRIpaddr' for printing PTE is buggy as this is not a physical
> address.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Acked-by: Julien Grall <jgrall@amazon.com>

I have committed this patch.

Cheers,

> ---
> 
> Changes from -
> 
> v3 - 1. Extracted the patch from https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00655.html
> and added to this series.
> 2. No changes done.
> 
>   xen/arch/arm/domain_build.c | 64 +++++++++++++++++++++++--------------
>   xen/arch/arm/gic-v2.c       |  6 ++--
>   xen/arch/arm/mm.c           |  2 +-
>   3 files changed, 44 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9707eb7b1b..15fa88e977 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1288,6 +1288,39 @@ static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
>       return res;
>   }
>   
> +/*
> + * Wrapper to convert physical address from paddr_t to uint64_t and
> + * invoke fdt_begin_node(). This is required as the physical address
> + * provided as part of node name should not contain any leading
> + * zeroes. Thus, one should use PRIx64 (instead of PRIpaddr) to append
> + * unit (which contains the physical address) with name to generate a
> + * node name.
> + */
> +static int __init domain_fdt_begin_node(void *fdt, const char *name,
> +                                        uint64_t unit)
> +{
> +    /*
> +     * The size of the buffer to hold the longest possible string (i.e.
> +     * interrupt-controller@ + a 64-bit number + \0).
> +     */
> +    char buf[38];
> +    int ret;
> +
> +    /* ePAPR 3.4 */
> +    ret = snprintf(buf, sizeof(buf), "%s@%"PRIx64, name, unit);
> +
> +    if ( ret >= sizeof(buf) )
> +    {
> +        printk(XENLOG_ERR
> +               "Insufficient buffer. Minimum size required is %d\n",
> +               (ret + 1));
> +
> +        return -FDT_ERR_TRUNCATED;
> +    }
> +
> +    return fdt_begin_node(fdt, buf);
> +}
> +
>   static int __init make_memory_node(const struct domain *d,
>                                      void *fdt,
>                                      int addrcells, int sizecells,
> @@ -1296,8 +1329,6 @@ static int __init make_memory_node(const struct domain *d,
>       unsigned int i;
>       int res, reg_size = addrcells + sizecells;
>       int nr_cells = 0;
> -    /* Placeholder for memory@ + a 64-bit number + \0 */
> -    char buf[24];
>       __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
>       __be32 *cells;
>   
> @@ -1314,9 +1345,7 @@ static int __init make_memory_node(const struct domain *d,
>   
>       dt_dprintk("Create memory node\n");
>   
> -    /* ePAPR 3.4 */
> -    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
> -    res = fdt_begin_node(fdt, buf);
> +    res = domain_fdt_begin_node(fdt, "memory", mem->bank[i].start);
>       if ( res )
>           return res;
>   
> @@ -1375,16 +1404,13 @@ static int __init make_shm_memory_node(const struct domain *d,
>       {
>           uint64_t start = mem->bank[i].start;
>           uint64_t size = mem->bank[i].size;
> -        /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
> -        char buf[27];
>           const char compat[] = "xen,shared-memory-v1";
>           /* Worst case addrcells + sizecells */
>           __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
>           __be32 *cells;
>           unsigned int len = (addrcells + sizecells) * sizeof(__be32);
>   
> -        snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start);
> -        res = fdt_begin_node(fdt, buf);
> +        res = domain_fdt_begin_node(fdt, "xen-shmem", mem->bank[i].start);
>           if ( res )
>               return res;
>   
> @@ -2716,12 +2742,9 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
>       __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
>       __be32 *cells;
>       const struct domain *d = kinfo->d;
> -    /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
> -    char buf[38];
>   
> -    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
> -             vgic_dist_base(&d->arch.vgic));
> -    res = fdt_begin_node(fdt, buf);
> +    res = domain_fdt_begin_node(fdt, "interrupt-controller",
> +                                vgic_dist_base(&d->arch.vgic));
>       if ( res )
>           return res;
>   
> @@ -2771,14 +2794,10 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
>       int res = 0;
>       __be32 *reg, *cells;
>       const struct domain *d = kinfo->d;
> -    /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
> -    char buf[38];
>       unsigned int i, len = 0;
>   
> -    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
> -             vgic_dist_base(&d->arch.vgic));
> -
> -    res = fdt_begin_node(fdt, buf);
> +    res = domain_fdt_begin_node(fdt, "interrupt-controller",
> +                                vgic_dist_base(&d->arch.vgic));
>       if ( res )
>           return res;
>   
> @@ -2858,11 +2877,8 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
>       __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
>       __be32 *cells;
>       struct domain *d = kinfo->d;
> -    /* Placeholder for sbsa-uart@ + a 64-bit number + \0 */
> -    char buf[27];
>   
> -    snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d->arch.vpl011.base_addr);
> -    res = fdt_begin_node(fdt, buf);
> +    res = domain_fdt_begin_node(fdt, "sbsa-uart", d->arch.vpl011.base_addr);
>       if ( res )
>           return res;
>   
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 61802839cb..5d4d298b86 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1049,7 +1049,7 @@ static void __init gicv2_dt_init(void)
>       if ( csize < SZ_8K )
>       {
>           printk(XENLOG_WARNING "GICv2: WARNING: "
> -               "The GICC size is too small: %#"PRIx64" expected %#x\n",
> +               "The GICC size is too small: %#"PRIpaddr" expected %#x\n",
>                  csize, SZ_8K);
>           if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
>           {
> @@ -1280,11 +1280,11 @@ static int __init gicv2_init(void)
>           gicv2.map_cbase += aliased_offset;
>   
>           printk(XENLOG_WARNING
> -               "GICv2: Adjusting CPU interface base to %#"PRIx64"\n",
> +               "GICv2: Adjusting CPU interface base to %#"PRIpaddr"\n",
>                  cbase + aliased_offset);
>       } else if ( csize == SZ_128K )
>           printk(XENLOG_WARNING
> -               "GICv2: GICC size=%#"PRIx64" but not aliased\n",
> +               "GICv2: GICC size=%#"PRIpaddr" but not aliased\n",
>                  csize);
>   
>       gicv2.map_hbase = ioremap_nocache(hbase, PAGE_SIZE);
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index f758cad545..b99806af99 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -263,7 +263,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
>   
>           pte = mapping[offsets[level]];
>   
> -        printk("%s[0x%03x] = 0x%"PRIpaddr"\n",
> +        printk("%s[0x%03x] = 0x%"PRIx64"\n",
>                  level_strs[level], offsets[level], pte.bits);
>   
>           if ( level == 3 || !pte.walk.valid || !pte.walk.table )

-- 
Julien Grall

Re: [XEN v4 02/11] xen/arm: domain_build: Track unallocated pages using the frame number

[Julien Grall]

Hi,

On 21/03/2023 14:03, Ayan Kumar Halder wrote:
> rangeset_{xxx}_range() functions are invoked with 'start' and 'size' as
> arguments which are either 'uint64_t' or 'paddr_t'. However, the function
> accepts 'unsigned long' for 'start' and 'size'. 'unsigned long' is 32 bits for
> ARM_32. Thus, there is an implicit downcasting from 'uint64_t'/'paddr_t' to
> 'unsigned long' when invoking rangeset_{xxx}_range().
> 
> So, it may seem there is a possibility of lose of data due to truncation.
> 
> In reality, 'start' and 'size' are always page aligned. And ARM_32 currently
> supports 40 bits as the width of physical address.
> So if the addresses are page aligned, the last 12 bits contain zeroes.
> Thus, we could instead pass page frame number which will contain 28 bits (40-12
> on Arm_32) and this can be represented using 'unsigned long'.

Is Arm_32 meant to refer to the config or the architecture? If the 
former, then it should be ARM_32 if the latter, it should be Arm32. I 
would prefer the latter.

> 
> On Arm_64, this change will not induce any adverse side effect as the width of

Same here for Arm_64.

> physical address is 48 bits. Thus, the width of 'mfn' (ie 48 - 12 = 36) can be
> represented using 'unsigned long' (which is 64 bits wide).
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from -
> 
> v3 - 1. Extracted the patch from https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00657.html
> and added it to this series.
> 2. Modified add_ext_regions(). This accepts a frame number instead of physical
> address.
> 
>   xen/arch/arm/domain_build.c | 27 +++++++++++++++++----------
>   1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 15fa88e977..24b12b7512 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1500,10 +1500,13 @@ static int __init make_resv_memory_node(const struct domain *d,
>       return res;
>   }
>   
> -static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
> +static int __init add_ext_regions(unsigned long s_pfn, unsigned long e_pfn,

We are trying to phase out any using of 'pfn' in the code. In this case, 
this is mean (see include/xen/mm.h for more details). Here, you want to 
use 'gfn' as we are looking for space in the dom0 memory address space.

> +                                  void *data)
>   {
>       struct meminfo *ext_regions = data;
>       paddr_t start, size;
> +    paddr_t s = PFN_UP(s_pfn);
> +    paddr_t e = PFN_UP(e_pfn);

PFN_UP() takes a physical address in parameter and return a page frame 
number. So this is not what you want here. You want pfn_to_paddr().

The rest looks good to me.

Cheers,

-- 
Julien Grall

Re: [XEN v4 03/11] xen/arm: Typecast the DT values into paddr_t

[Julien Grall]

Hi Ayan,

On 21/03/2023 14:03, Ayan Kumar Halder wrote:
> In future, we wish to support 32 bit physical address.
> However, the current dt and fdt functions can only read u64 values.

Reading this again this is a bit misleading. At least the DT functions 
are able to deal read 32-bit or 64-bit values (this is based on the 
number of cells). The difference is that the functions are expecting 
64-bit variable and this will be incompatible with paddr_t (which after 
your patch may be 32-bit).

> We wish to make the DT functions read 32bit as well 64bit values
> (depending on the width of physical address). Also, we wish to detect
> if any truncation has occurred (ie while reading 32bit physical
> addresses from 64bit values read from DT).
> 
> device_tree_get_reg() should now be able to return paddr_t. This is
> invoked by various callers to get DT address and size.
> 
> For fdt_get_mem_rsv(), we have introduced wrapper ie
> fdt_get_mem_rsv_paddr() while will invoke fdt_get_mem_rsv() and
> translate uint64_t to paddr_t. The reason being we cannot modify
> fdt_get_mem_rsv() as it has been imported from external source.
> 
> For dt_read_number(), we have also introduced a wrapper ie

NIT: 'ie' seems to be out of place. I would drop it.

> dt_read_paddr() to read physical addresses. We chose not to modify the
> original function as it been used in places where it needs to

"it been used" -> "it is used" ?

> specifically 64bit values from dt (For eg dt_property_read_u64()).

I can't parse the sencen after "where it needs to...".
> 
> Xen prints an error when it detects a truncation (during typecast
> between uint64_t and paddr_t). It is not possible to return an error in
> all scenarios. So, it is user's responsibility to check the error logs.

The last sentence is a bit odd to say. Quite likely a user will not be 
aware of this requirements. And a single line in the log is likely going 
to be difficult to spot (see more below).

> 
> Also, replaced u32/u64 with uint32_t/uint64_t in the functions touched
> by the code changes.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 
> Changes from
> 
> v1 - 1. Dropped "[XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t" and
> "[XEN v1 4/9] xen/arm: Use translate_dt_address_size() to translate between device tree addr/size and paddr_t", instead
> this approach achieves the same purpose.
> 
> 2. No need to check for truncation while converting values from u64 to paddr_t.
> 
> v2 - 1. Use "( (dt_start >> (PADDR_SHIFT - 1)) > 1 )" to detect truncation.
> 2. Introduced libfdt_xen.h to implement fdt_get_mem_rsv_paddr
> 3. Logged error messages in case truncation is detected.
> 
> v3 - 1. Renamed libfdt_xen.h to libfdt-xen.h.
> 2. Replaced u32/u64 with uint32_t/uint64_t
> 3. Use "(paddr_t)val != val" to check for truncation.
> 4. Removed the alias "#define PADDR_SHIFT PADDR_BITS".
> 
>   xen/arch/arm/bootfdt.c              | 41 ++++++++++++++++++-----
>   xen/arch/arm/domain_build.c         |  2 +-
>   xen/arch/arm/include/asm/setup.h    |  4 +--
>   xen/arch/arm/setup.c                | 14 ++++----
>   xen/arch/arm/smpboot.c              |  2 +-
>   xen/include/xen/device_tree.h       | 21 ++++++++++++
>   xen/include/xen/libfdt/libfdt-xen.h | 52 +++++++++++++++++++++++++++++
>   7 files changed, 116 insertions(+), 20 deletions(-)
>   create mode 100644 xen/include/xen/libfdt/libfdt-xen.h
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 0085c28d74..33bef1c15e 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -11,7 +11,7 @@
>   #include <xen/efi.h>
>   #include <xen/device_tree.h>
>   #include <xen/lib.h>
> -#include <xen/libfdt/libfdt.h>
> +#include <xen/libfdt/libfdt-xen.h>
>   #include <xen/sort.h>
>   #include <xsm/xsm.h>
>   #include <asm/setup.h>
> @@ -52,11 +52,32 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
>       return false;
>   }
>   
> -void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
> -                                u32 size_cells, u64 *start, u64 *size)
> +void __init device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
> +                                uint32_t size_cells, paddr_t *start,
> +                                paddr_t *size)
>   {
> -    *start = dt_next_cell(address_cells, cell);
> -    *size = dt_next_cell(size_cells, cell);
> +    uint64_t dt_start, dt_size;
> +
> +    /*
> +     * dt_next_cell will return u64 whereas paddr_t may be u64 or u32. Thus,

s/u64/uint64_t/ I would also say "whereas paddr_t may not be 64-bit" 
just to shorten the sentence.

> +     * there is an implicit cast from u64 to paddr_t.

s/u64/uint64_t/

> +     */
> +    dt_start = dt_next_cell(address_cells, cell);
> +    dt_size = dt_next_cell(size_cells, cell);
> +
> +    if ( dt_start != (paddr_t)dt_start  )
> +        printk("Error: Physical address greater than max width supported\n");

I would add a WARN(). Same...

> +
> +    if ( dt_size != (paddr_t)dt_size )
> +        printk("Error: Physical size greater than max width supported\n");

... here as this is easier to spot in the logs.

> +
> +    /*
> +     * Note: It is user's responsibility to check for the error messages.

See my remark above about this.

> +     * Xen will sliently truncate in case if the address/size is greater than
> +     * the max supported width.
> +     */
> +    *start = dt_start;
> +    *size = dt_size;
>   }
>   
>   static int __init device_tree_get_meminfo(const void *fdt, int node,
> @@ -326,7 +347,7 @@ static int __init process_chosen_node(const void *fdt, int node,
>           printk("linux,initrd-start property has invalid length %d\n", len);
>           return -EINVAL;
>       }
> -    start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> +    start = dt_read_paddr((void *)&prop->data, dt_size_to_cells(len));
>   
>       prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
>       if ( !prop )
> @@ -339,7 +360,7 @@ static int __init process_chosen_node(const void *fdt, int node,
>           printk("linux,initrd-end property has invalid length %d\n", len);
>           return -EINVAL;
>       }
> -    end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> +    end = dt_read_paddr((void *)&prop->data, dt_size_to_cells(len));
>   
>       if ( start >= end )
>       {
> @@ -594,9 +615,11 @@ static void __init early_print_info(void)
>       for ( i = 0; i < nr_rsvd; i++ )
>       {
>           paddr_t s, e;
> -        if ( fdt_get_mem_rsv(device_tree_flattened, i, &s, &e) < 0 )
> +
> +        if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &e) < 0 )
>               continue;
> -        /* fdt_get_mem_rsv returns length */
> +
> +        /* fdt_get_mem_rsv_paddr returns length */
>           e += s;
>           printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e);
>       }
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 24b12b7512..6573d15302 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -949,7 +949,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>           BUG_ON(!prop);
>           cells = (const __be32 *)prop->value;
>           device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase);
> -        psize = dt_read_number(cells, size_cells);
> +        psize = dt_read_paddr(cells, size_cells);
>           if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) )
>           {
>               printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n",
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index a926f30a2b..7b697d879e 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -157,8 +157,8 @@ const char *boot_module_kind_as_string(bootmodule_kind kind);
>   extern uint32_t hyp_traps_vector[];
>   void init_traps(void);
>   
> -void device_tree_get_reg(const __be32 **cell, u32 address_cells,
> -                         u32 size_cells, u64 *start, u64 *size);
> +void device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
> +                         uint32_t size_cells, paddr_t *start, paddr_t *size);
>   
>   u32 device_tree_get_u32(const void *fdt, int node,
>                           const char *prop_name, u32 dflt);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 1f26f67b90..755173e5a3 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -29,7 +29,7 @@
>   #include <xen/virtual_region.h>
>   #include <xen/vmap.h>
>   #include <xen/trace.h>
> -#include <xen/libfdt/libfdt.h>
> +#include <xen/libfdt/libfdt-xen.h>
>   #include <xen/acpi.h>
>   #include <xen/warning.h>
>   #include <asm/alternative.h>
> @@ -222,11 +222,11 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>       {
>           paddr_t r_s, r_e;
>   
> -        if ( fdt_get_mem_rsv(device_tree_flattened, i, &r_s, &r_e ) < 0 )
> +        if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &r_s, &r_e ) < 0 )
>               /* If we can't read it, pretend it doesn't exist... */
>               continue;
>   
> -        r_e += r_s; /* fdt_get_mem_rsv returns length */
> +        r_e += r_s; /* fdt_get_mem_rsv_paddr returns length */
>   
>           if ( s < r_e && r_s < e )
>           {
> @@ -502,13 +502,13 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
>       {
>           paddr_t mod_s, mod_e;
>   
> -        if ( fdt_get_mem_rsv(device_tree_flattened,
> -                             i - mi->nr_mods,
> -                             &mod_s, &mod_e ) < 0 )
> +        if ( fdt_get_mem_rsv_paddr(device_tree_flattened,
> +                                   i - mi->nr_mods,
> +                                   &mod_s, &mod_e ) < 0 )
>               /* If we can't read it, pretend it doesn't exist... */
>               continue;
>   
> -        /* fdt_get_mem_rsv returns length */
> +        /* fdt_get_mem_rsv_paddr returns length */
>           mod_e += mod_s;
>   
>           if ( s < mod_e && mod_s < e )
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 412ae22869..c15c177487 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -159,7 +159,7 @@ static void __init dt_smp_init_cpus(void)
>               continue;
>           }
>   
> -        addr = dt_read_number(prop, dt_n_addr_cells(cpu));
> +        addr = dt_read_paddr(prop, dt_n_addr_cells(cpu));
>   
>           hwid = addr;
>           if ( hwid != addr )
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 19a74909ce..bbc7d7377a 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -241,6 +241,27 @@ static inline u64 dt_read_number(const __be32 *cell, int size)
>       return r;
>   }
>   
> +/* Wrapper for dt_read_number() to return paddr_t (instead of u64) */
> +static inline paddr_t dt_read_paddr(const __be32 *cell, int size)
> +{
> +    uint64_t dt_r = 0;
> +    paddr_t r;
> +
> +    dt_r = dt_read_number(cell, size);
> +
> +    if ( dt_r != (paddr_t)dt_r )
> +        printk("Error: Physical address greater than max width supported\n");

Same remark as abovo here and ...

> +
> +    /*
> +     * Note: It is user's responsibility to check for the error messages.

... here. I also find interesting, you explain the check in 
device_tree_get_reg() but not in dt_read_paddr().

> +     * Xen will sliently truncate in case if the address/size is greater than
> +     * the max supported width.
> +     */
> +    r = dt_r;
> +
> +    return r;
> +}
> +
>   /* Helper to convert a number of cells to bytes */
>   static inline int dt_cells_to_size(int size)
>   {
> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
> new file mode 100644
> index 0000000000..648bf41be6
> --- /dev/null
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0

This should be /* ... */ rather than //. Also please use GPL-2.0-only.

> +/*
> + * xen/include/xen/libfdt/libfdt-xen.h
> + *
> + * Wrapper functions for device tree. This helps to convert dt values
> + * between u64 and paddr_t.
> + *
> + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved.
> + */
> +
> +#ifndef LIBFDT_XEN_H
> +#define LIBFDT_XEN_H
> +
> +#include <xen/libfdt/libfdt.h>
> +
> +inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,

This should be static inline to avoid any issue if the header is 
included in multiple units and not inlined (a compiler is free to ignore 
the inline request).

> +                                 paddr_t *address,
> +                                 paddr_t *size)
> +{
> +    uint64_t dt_addr;
> +    uint64_t dt_size;
> +    int ret = 0;
> +
> +    ret = fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size);

dt_addr and dt_size will not be set if fdt_get_mem_rsv() return an 
error. So the checks below will happen on unknown value (dt_addr/dt_size 
are not initialized) and the checks will be unpredicatable.

So you want to check the return value first and then the truncation.

> +    if ( dt_addr != (paddr_t)dt_addr ) > +    {
> +        printk("Error: Physical address greater than max width supported\n");
> +        return -1;

-1 already means something in the libfdt case (it is defined as 
-FDT_ERR_NOTFOUND). So you want to use a different error value. I would 
use -FDT_ERR_MAX in this case.

BTW, I am not suggesting to use WARN() here because the error is 
properly propagated.

> +    }
> +
> +    if ( dt_size != (paddr_t)dt_size )
> +    {
> +        printk("Error: Physical size greater than max width supported\n");
> +        return -1;

Same here.

> +    }
> +
> +    *address = dt_addr;
> +    *size = dt_size;
> +
> +    return ret;
> +}
> +
> +#endif /* LIBFDT_XEN_H */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Cheers,

-- 
Julien Grall

Re: [XEN v4 05/11] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t

[Julien Grall]

Hi Ayan,

On 21/03/2023 14:03, Ayan Kumar Halder wrote:
> dt_device_get_address() can accept uint64_t only for address and size.
> However, the address/size denotes physical addresses. Thus, they should
> be represented by 'paddr_t'.
> Consequently, we introduce a wrapper for dt_device_get_address() ie
> dt_device_get_paddr() which accepts address/size as paddr_t and inturn
> invokes dt_device_get_address() after converting address/size to
> uint64_t.
> 
> The reason for introducing doing this is that in future 'paddr_t' may
> be defined as uint32_t.

Technically, you will define it as 'unsigned long' after. To avoid 
relying on how paddr_t is defined, I would suggest ot write "'paddr_t' 
may not always be 64-bit" or similar.

> diff --git a/xen/arch/arm/platforms/brcm-raspberry-pi.c b/xen/arch/arm/platforms/brcm-raspberry-pi.c
> index 811b40b1a6..407ec07f63 100644
> --- a/xen/arch/arm/platforms/brcm-raspberry-pi.c
> +++ b/xen/arch/arm/platforms/brcm-raspberry-pi.c
> @@ -64,7 +64,7 @@ static void __iomem *rpi4_map_watchdog(void)
>       if ( !node )
>           return NULL;
>   
> -    ret = dt_device_get_address(node, 0, &start, &len);
> +    ret = dt_device_get_paddr(node, 0, &start, &len);
>       if ( ret )
>       {
>           printk("Cannot read watchdog register address\n");
> diff --git a/xen/arch/arm/platforms/brcm.c b/xen/arch/arm/platforms/brcm.c
> index d481b2c60f..4310feee73 100644
> --- a/xen/arch/arm/platforms/brcm.c
> +++ b/xen/arch/arm/platforms/brcm.c
> @@ -40,7 +40,7 @@ static __init int brcm_get_dt_node(char *compat_str,
>                                      u32 *reg_base)
>   {
>       const struct dt_device_node *node;
> -    u64 reg_base_64;
> +    paddr_t reg_base_64;

'64' reads a bit odd now. I think you want to rename to reg_base_paddr.

Cheers,

-- 
Julien Grall

Re: [XEN v4 06/11] xen/arm: smmu: Use writeq_relaxed_non_atomic() for writing to SMMU_CBn_TTBR0

[Julien Grall]

Hi Ayan,

On 21/03/2023 14:03, Ayan Kumar Halder wrote:
> Refer ARM IHI 0062D.c ID070116 (SMMU 2.0 spec), 17-360, 17.3.9,
> SMMU_CBn_TTBR0 is a 64 bit register. Thus, one can use
> writeq_relaxed_non_atomic() to write to it instead of invoking
> writel_relaxed() twice for lower half and upper half of the register.
> 
> This also helps us as p2maddr is 'paddr_t' (which may be u32 in future).
> Thus, one can assign p2maddr to a 64 bit register and do the bit
> manipulations on it, to generate the value for SMMU_CBn_TTBR0.
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

The tags should be ordered in a timeline. So Signed-off-by should be first.

I am happy to do it on commit if you can confirm that this patch doesn't 
dependent on the patches before.

Cheers,

-- 
Julien Grall

Re: [XEN v4 09/11] xen/arm: Restrict zeroeth_table_offset for ARM_64

[Julien Grall]

Hi,

On 21/03/2023 14:03, Ayan Kumar Halder wrote:
> When 32 bit physical addresses are used (ie ARM_PA_32=y),
> "va >> ZEROETH_SHIFT" causes an overflow.
> Also, there is no zeroeth level page table on Arm 32-bit.
> 
> Also took the opportunity to clean up dump_pt_walk(). One could use
> DECLARE_OFFSETS() macro instead of declaring the declaring an array
> of page table offsets.
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

This should be re-ordered so the signed-off-by tag is first.

> ---
> 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.
> 
>   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..0d40388f93 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_ARM_PA_BITS_32

I know I already acked this patch. However, looking at the previous 
patch, you are using CONFIG_PHYS_ADDR_T_32 but here you are using 
CONFIG_ARM_PA_BITS_32.

It is not fully clear to me why you differ in the #ifdef approach. Can 
you clarify?

> +#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 d8b43ef38c..41e0896b0f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -221,12 +221,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;
>   

Cheers,

-- 
Julien Grall

Re: [XEN v4 10/11] xen/arm: p2m: Use the pa_range_info table to support Arm_32 and Arm_64

[Julien Grall]

Hi Ayan:

Title: Shouldn't you remove the _ or use uppercase?

On 21/03/2023 14:03, 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. >
> 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.
> 
>   xen/arch/arm/p2m.c | 28 +++++++++++++++-------------
>   1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 948f199d84..f34b6e6f11 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -2265,22 +2265,16 @@ 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 */
>           unsigned int root_order; /* Page order of the root of the p2m */
>           unsigned int sl0;    /* Desired SL0, maximum in comment */
>       } pa_range_info[] __initconst = {
> +#ifdef CONFIG_ARM_32
> +        [0] = { 40,      24/*24*/,  1,          1 },
> +        [1] = { 0 } /* Invalid */
> +#else
>           /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
>           /*      PA size, t0sz(min), root-order, sl0(max) */
>           [0] = { 32,      32/*32*/,  0,          1 },
> @@ -2291,11 +2285,13 @@ void __init setup_virt_paging(void)
>           [5] = { 48,      16/*16*/,  0,          2 },
>           [6] = { 52,      12/*12*/,  4,          2 },
>           [7] = { 0 }  /* Invalid */
> +#endif
>       };
>   
>       unsigned int i;
>       unsigned int pa_range = 0x10; /* Larger than any possible value */
>   
> +#ifdef CONFIG_ARM_64
>       /*
>        * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured
>        * with IPA bits == PA bits, compare against "pabits".
> @@ -2309,6 +2305,9 @@ void __init setup_virt_paging(void)
>        */
>       if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
>           max_vmid = MAX_VMID_16_BIT;
> +#else
> +    p2m_ipa_bits = PADDR_BITS;
> +#endif
>   
>       /* Choose suitable "pa_range" according to the resulted "p2m_ipa_bits". */
>       for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> @@ -2324,14 +2323,13 @@ void __init setup_virt_paging(void)
>       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);
>   
> -    val |= VTCR_PS(pa_range);
> +#ifdef CONFIG_ARM_64
>       val |= VTCR_TG0_4K;
> +    val |= VTCR_PS(pa_range);
Why is this moved rather than adding #ifdef a line before?

>   
>       /* Set the VS bit only if 16 bit VMID is supported. */
>       if ( MAX_VMID == MAX_VMID_16_BIT )
>           val |= VTCR_VS;
> -    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;
> @@ -2342,6 +2340,10 @@ void __init setup_virt_paging(void)
>              pa_range_info[pa_range].pabits,
>              ( MAX_VMID == MAX_VMID_16_BIT ) ? 16 : 8);
>   #endif
> +
> +    val |= VTCR_SL0(pa_range_info[pa_range].sl0);
> +    val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
> +
>       printk("P2M: %d levels with order-%d root, VTCR 0x%"PRIregister"\n",
>              4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
>   

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi Ayan,

On 21/03/2023 14:03, Ayan Kumar Halder wrote:
> The pabits, t0sz, root_order and sl0 values are the same as those for
> ARM_64.

To me this read as the line should be common. But you still duplicate it.

In any case, you should justify this change with a pointer to the Arm 
Arm. Not just saying they are common.

> 
> 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.
> 
>   xen/arch/arm/p2m.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index f34b6e6f11..20beecc6e8 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -2272,8 +2272,9 @@ void __init setup_virt_paging(void)
>           unsigned int sl0;    /* Desired SL0, maximum in comment */
>       } pa_range_info[] __initconst = {
>   #ifdef CONFIG_ARM_32
> -        [0] = { 40,      24/*24*/,  1,          1 },
> -        [1] = { 0 } /* Invalid */
> +        [0] = { 32,      32/*32*/,  0,          1 },

As I pointed out in one of the previous version, the root order is 
different than ...

> +        [1] = { 40,      24/*24*/,  1,          1 },

... here. Yet, you still keep P2M_ROOT_ORDER and P2M_ROOT_LEVEL 
hardcoded. Your previous patch wants to define p2M_root_order and 
p2m_root_level (lower-case intended). IOW making more code common 
between arm64 and arm32.

Cheers,

-- 
Julien Grall

Re: [XEN v4 10/11] xen/arm: p2m: Use the pa_range_info table to support Arm_32 and Arm_64

[Julien Grall]

Hi,

On 21/03/2023 14:03, Ayan Kumar Halder wrote:
>       /* Choose suitable "pa_range" according to the resulted "p2m_ipa_bits". */
>       for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> @@ -2324,14 +2323,13 @@ void __init setup_virt_paging(void)
>       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);
>   
> -    val |= VTCR_PS(pa_range);
> +#ifdef CONFIG_ARM_64
>       val |= VTCR_TG0_4K;
> +    val |= VTCR_PS(pa_range);
>   
>       /* Set the VS bit only if 16 bit VMID is supported. */
>       if ( MAX_VMID == MAX_VMID_16_BIT )
>           val |= VTCR_VS;
> -    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;

As mentioned in the next patch, this now wants to be outside of the 
ARM_64 specific section because at least the root order will be 
different. The root level is the same, but I think you would be better 
if this is moved out at the same time.

Cheers,

-- 
Julien Grall

Re: [XEN v4 06/11] xen/arm: smmu: Use writeq_relaxed_non_atomic() for writing to SMMU_CBn_TTBR0

[Ayan Kumar Halder]

On 30/03/2023 22:27, Julien Grall 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.
>
>
> Hi Ayan,
Hi Julien,
>
> On 21/03/2023 14:03, Ayan Kumar Halder wrote:
>> Refer ARM IHI 0062D.c ID070116 (SMMU 2.0 spec), 17-360, 17.3.9,
>> SMMU_CBn_TTBR0 is a 64 bit register. Thus, one can use
>> writeq_relaxed_non_atomic() to write to it instead of invoking
>> writel_relaxed() twice for lower half and upper half of the register.
>>
>> This also helps us as p2maddr is 'paddr_t' (which may be u32 in future).
>> Thus, one can assign p2maddr to a 64 bit register and do the bit
>> manipulations on it, to generate the value for SMMU_CBn_TTBR0.
>>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>
> The tags should be ordered in a timeline. So Signed-off-by should be 
> first.
Ack. I will take care henceforth.
>
> I am happy to do it on commit if you can confirm that this patch doesn't
> dependent on the patches before.

Yes, please commit this patch as it is independent of the patch series.

- Ayan

>
> Cheers,
>
> -- 
> Julien Grall
>

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

[Ayan Kumar Halder]

On 30/03/2023 22:45, Julien Grall 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.
>
>
> Hi Ayan,

Hi Julien,

I need some clarifications.

>
> On 21/03/2023 14:03, Ayan Kumar Halder wrote:
>> The pabits, t0sz, root_order and sl0 values are the same as those for
>> ARM_64.
>
> To me this read as the line should be common. But you still duplicate it.
>
> In any case, you should justify this change with a pointer to the Arm
> Arm. Not just saying they are common.

Does the following commit message read fine ?

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

 0.

    "Use of concatenated second-level translation tables

    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 only one root level translation 
tables. This is because as the paragraph explains above,  35 bit IPA is 
the minimum required to support two root level translation tables.

The root order for 32 bit IPA will be 0. (Refer xen/arch/arm/p2m.c 
"#define P2M_ROOT_PAGES (1<<P2M_ROOT_ORDER)")

Please clarify if I misunderstood something.

- Ayan

>
>>
>> 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.
>>
>>   xen/arch/arm/p2m.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index f34b6e6f11..20beecc6e8 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -2272,8 +2272,9 @@ void __init setup_virt_paging(void)
>>           unsigned int sl0;    /* Desired SL0, maximum in comment */
>>       } pa_range_info[] __initconst = {
>>   #ifdef CONFIG_ARM_32
>> -        [0] = { 40,      24/*24*/,  1,          1 },
>> -        [1] = { 0 } /* Invalid */
>> +        [0] = { 32,      32/*32*/,  0,          1 },
>
> As I pointed out in one of the previous version, the root order is
> different than ...
>
>> +        [1] = { 40,      24/*24*/, 1,          1 },
>
> ... here. Yet, you still keep P2M_ROOT_ORDER and P2M_ROOT_LEVEL
> hardcoded. Your previous patch wants to define p2M_root_order and
> p2m_root_level (lower-case intended). IOW making more code common
> between arm64 and arm32.
>
> Cheers,
>
> -- 
> Julien Grall
>

Re: [XEN v4 04/11] xen/drivers: ns16550: Use paddr_t for io_base/io_size

[Ayan Kumar Halder]

Hi Jan,

On 30/03/2023 07:55, Jan Beulich wrote:
> On 29.03.2023 16:35, Ayan Kumar Halder wrote:
>> Please let me know if the below patch looks fine.
> Apart from the comments below there may be formatting issues, which
> I can't sensibly comment on when the patch was mangled by your mailer
> anyway. (Which in turn is why it is generally better to properly send
> a new version, rather than replying with kind-of-a-new-version on an
> earlier thread.)
>
> Additionally, up front: I'm sorry for the extra requests, but I'm
> afraid to sensibly make the changes you want to make some things need
> sorting first, to avoid extending pre-existing clumsiness. This is
> irrespective of the present state of things clearly not being your
> fault.
>
>> @@ -1235,6 +1235,8 @@ pci_uart_config(struct ns16550 *uart, bool_t
>> skip_amt, unsigned int idx)
>>                    /* MMIO based */
>>                    if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
>>                    {
>> +                    uint64_t pci_uart_io_base;
>> +
>>                        pci_conf_write32(PCI_SBDF(0, b, d, f),
>>                                         PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
>>                        len = pci_conf_read32(PCI_SBDF(0, b, d, f),
>> @@ -1259,8 +1261,17 @@ pci_uart_config(struct ns16550 *uart, bool_t
>> skip_amt, unsigned int idx)
>>                        else
>>                            size = len & PCI_BASE_ADDRESS_MEM_MASK;
>>
>> -                    uart->io_base = ((u64)bar_64 << 32) |
>> -                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
>> +                    pci_uart_io_base = ((uint64_t)bar_64 << 32) |
>> +                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
>> +
>> +                    /* Truncation detected while converting to paddr_t */
>> +                    if ( pci_uart_io_base != (paddr_t)pci_uart_io_base )
>> +                    {
>> +                        printk("ERROR: Truncation detected for io_base
>> address");
>> +                        return -EINVAL;
>> +                    }
> Further down the function returns -1, so here you assume EINVAL != 1.
> Such assumptions (and mixing of value spaces) is generally not a good
> idea. Since there are other issues (see below), maybe you really want
> to add a prereq patch addressing those? That would include changing the
> "return -1" to either "return 1" or making it use some sensible and
> properly distinguishable errno value.
>
>> @@ -1519,20 +1530,40 @@ static bool __init parse_positional(struct
>> ns16550 *uart, char **str)
>>    #ifdef CONFIG_HAS_PCI
>>            if ( strncmp(conf, "pci", 3) == 0 )
>>            {
>> -            if ( pci_uart_config(uart, 1/* skip AMT */, uart -
>> ns16550_com) )
>> +            int ret;
>> +
>> +            ret = pci_uart_config(uart, 1/* skip AMT */, uart -
>> ns16550_com);
>> +
>> +            if ( ret == -EINVAL )
>> +                return false;
>> +            else if ( ret )
>>                    return true;
> With skip_amt != 0 the function presently can only return 0. You're
> therefore converting pre-existing dead code to another form of dead
> code. Otoh (and as, I think, previously indicated) ...
>
>> +
>>                conf += 3;
>>            }
>>            else if ( strncmp(conf, "amt", 3) == 0 )
>>            {
>> -            if ( pci_uart_config(uart, 0, uart - ns16550_com) )
>> +            int ret = pci_uart_config(uart, 0, uart - ns16550_com);
>> +
>> +            if ( ret == -EINVAL )
>> +                return false;
>> +            else if ( ret )
>>                    return true;
> ... the equivalent of this in parse_namevalue_pairs() not checking
> the return value is bogus. But it is further bogus that the case
> where skip_amt has passed 1 for it sets dev_set to true
> unconditionally, i.e. even when no device was found. IOW I also
> question the correctness of the final "return 0" in pci_uart_config().
> I looks to me as if this wants to be a skip_amt-independent
> "return -ENODEV". skip_amt would only control whether uart->io_base is
> restored before returning (leaving aside the question of why that is).

I have sent out a patch to fix the return logic pci_uart_config()

[PATCH v1] xen/drivers: ns16550: Fix the return logic for pci_uart_config()

Let me know if I understood you correctly.

- Ayan

>
> Jan