* [XEN v3 0/9] Add support for 32 bit physical address
@ 2023-02-08 12:05 Ayan Kumar Halder
2023-02-08 12:05 ` [XEN v3 1/9] xen/ns16550: Remove unneeded truncation check in the DT init code Ayan Kumar Halder
` (8 more replies)
0 siblings, 9 replies; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 12:05 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh, 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".
Ayan Kumar Halder (9):
xen/ns16550: Remove unneeded truncation check in the DT init code
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_ARM_PA_32"
xen/arm: Restrict zeroeth_table_offset for ARM_64
xen/arm: p2m: Enable support for 32bit IPA
xen/arch/Kconfig | 12 +++++
xen/arch/arm/Kconfig | 31 +++++++++++++
xen/arch/arm/bootfdt.c | 38 ++++++++++++----
xen/arch/arm/domain_build.c | 12 ++---
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/guest_walk.c | 2 +
xen/arch/arm/include/asm/lpae.h | 4 ++
xen/arch/arm/include/asm/page-bits.h | 2 +
xen/arch/arm/include/asm/setup.h | 2 +-
xen/arch/arm/include/asm/types.h | 6 +++
xen/arch/arm/mm.c | 7 +--
xen/arch/arm/p2m.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/arch/arm/setup.c | 14 +++---
xen/arch/arm/smpboot.c | 2 +-
xen/common/device_tree.c | 36 ++++++++++++++-
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 | 31 ++++++++-----
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 ++++++++++++++++++++++
xen/include/xen/types.h | 2 +
38 files changed, 311 insertions(+), 118 deletions(-)
create mode 100644 xen/include/xen/libfdt/libfdt_xen.h
--
2.17.1
^ permalink raw reply [flat|nested] 31+ messages in thread
* [XEN v3 1/9] xen/ns16550: Remove unneeded truncation check in the DT init code
2023-02-08 12:05 [XEN v3 0/9] Add support for 32 bit physical address Ayan Kumar Halder
@ 2023-02-08 12:05 ` Ayan Kumar Halder
2023-02-11 8:55 ` Julien Grall
2023-02-08 12:05 ` [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t Ayan Kumar Halder
` (7 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 12:05 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh, Ayan Kumar Halder
In an earlier commit (7c1de0038895), "ns16550.io_size" was u32 and
"io_size" was u64. Thus, the ASSERT() was needed to check if the values
are the same.
However, in a later commit (c9f8e0aee507), "ns16550.io_size" was changed
to u64. Thus, the ASSERT() became redundant.
So, now as "io_size" and "uart->io_size" are both u64, there will be no
truncation. Thus, one can remove the ASSERT() and extra assignment.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes from :
v1 - 1. Updated commit message/title.
2. Added Rb.
v2 - No change.
xen/drivers/char/ns16550.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 4ce74b3cd3..092f6b9c4b 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1747,7 +1747,6 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
struct ns16550 *uart;
int res;
u32 reg_shift, reg_width;
- u64 io_size;
uart = &ns16550_com[0];
@@ -1758,14 +1757,10 @@ 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, &io_size);
+ res = dt_device_get_address(dev, 0, &uart->io_base, &uart->io_size);
if ( res )
return res;
- uart->io_size = io_size;
-
- ASSERT(uart->io_size == io_size); /* Detect truncation */
-
res = dt_property_read_u32(dev, "reg-shift", ®_shift);
if ( !res )
uart->reg_shift = 0;
--
2.17.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t
2023-02-08 12:05 [XEN v3 0/9] Add support for 32 bit physical address Ayan Kumar Halder
2023-02-08 12:05 ` [XEN v3 1/9] xen/ns16550: Remove unneeded truncation check in the DT init code Ayan Kumar Halder
@ 2023-02-08 12:05 ` Ayan Kumar Halder
2023-02-08 13:33 ` Jan Beulich
2023-02-11 0:08 ` Stefano Stabellini
2023-02-08 12:05 ` [XEN v3 3/9] xen/drivers: ns16550: Use paddr_t for io_base/io_size Ayan Kumar Halder
` (6 subsequent siblings)
8 siblings, 2 replies; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 12:05 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh, 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 u64 as well u32 values (depending
on the width of physical address). Also, we wish to detect if any
truncation has occurred (ie while reading u32 physical addresses from
u64 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
u64 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 u64 values from dt (For eg dt_property_read_u64()).
Xen prints an error when it detects a truncation (during typecast
between u64 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.
To detect truncation, we right shift physical address by
"PADDR_BITS - 1" and then if the resulting number is greater than 1,
we know that truncation has occurred and an appropriate error log is
printed.
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.
xen/arch/arm/bootfdt.c | 38 ++++++++++++++++-----
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/include/asm/setup.h | 2 +-
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 +++++++++++++++++++++++++++++
xen/include/xen/types.h | 2 ++
8 files changed, 115 insertions(+), 18 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..f63da3e831 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>
@@ -53,10 +53,30 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
}
void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
- u32 size_cells, u64 *start, u64 *size)
+ u32 size_cells, paddr_t *start, paddr_t *size)
{
- *start = dt_next_cell(address_cells, cell);
- *size = dt_next_cell(size_cells, cell);
+ u64 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_SHIFT - 1)) > 1 )
+ printk("Error: Physical address greater than max width supported\n");
+
+ if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
+ 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 +346,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 +359,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 +614,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 a798e0b256..4d7e67560f 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..6105e5cae3 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -158,7 +158,7 @@ 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);
+ u32 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..5ade20e6e7 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 a28937d12a..b61bac2931 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -244,6 +244,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)
+{
+ u64 dt_r = 0;
+ paddr_t r;
+
+ dt_r = dt_read_number(cell, size);
+
+ if ( (dt_r >> (PADDR_SHIFT - 1)) > 1 )
+ 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..a3196dd96c
--- /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_SHIFT - 1)) > 1 )
+ {
+ printk("Error: Physical address greater than max width supported\n");
+ return -1;
+ }
+
+ if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
+ {
+ 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:
+ */
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 6aba80500a..b7255c7c38 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -71,4 +71,6 @@ typedef bool bool_t;
#define test_and_set_bool(b) xchg(&(b), true)
#define test_and_clear_bool(b) xchg(&(b), false)
+#define PADDR_SHIFT PADDR_BITS
+
#endif /* __TYPES_H__ */
--
2.17.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [XEN v3 3/9] xen/drivers: ns16550: Use paddr_t for io_base/io_size
2023-02-08 12:05 [XEN v3 0/9] Add support for 32 bit physical address Ayan Kumar Halder
2023-02-08 12:05 ` [XEN v3 1/9] xen/ns16550: Remove unneeded truncation check in the DT init code Ayan Kumar Halder
2023-02-08 12:05 ` [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t Ayan Kumar Halder
@ 2023-02-08 12:05 ` Ayan Kumar Halder
2023-02-08 13:52 ` Jan Beulich
2023-02-08 12:05 ` [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t Ayan Kumar Halder
` (5 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 12:05 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh, 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 raise a BUG
for this (as the address is provided either by reading the PCIE registers
or parsing parameters from string, so the error is unrecoverable).
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.
xen/drivers/char/ns16550.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 092f6b9c4b..2aee5642f9 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,8 +1166,9 @@ 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;
+ u64 pci_uart_io_base;
/* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
@@ -1259,8 +1260,13 @@ 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 */
+ BUG_ON((pci_uart_io_base >> (PADDR_SHIFT - 1)) > 1);
+
+ uart->io_base = pci_uart_io_base;
}
/* IO based */
else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) )
@@ -1468,6 +1474,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
int baud;
const char *conf;
char *name_val_pos;
+ u64 uart_io_base;
conf = *str;
name_val_pos = strchr(conf, '=');
@@ -1532,7 +1539,12 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
else
#endif
{
- uart->io_base = simple_strtoull(conf, &conf, 0);
+ uart_io_base = simple_strtoull(conf, &conf, 0);
+
+ /* Truncation detected while converting to paddr_t */
+ BUG_ON((uart_io_base >> (PADDR_SHIFT - 1)) > 1);
+
+ uart->io_base = uart_io_base;
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t
2023-02-08 12:05 [XEN v3 0/9] Add support for 32 bit physical address Ayan Kumar Halder
` (2 preceding siblings ...)
2023-02-08 12:05 ` [XEN v3 3/9] xen/drivers: ns16550: Use paddr_t for io_base/io_size Ayan Kumar Halder
@ 2023-02-08 12:05 ` Ayan Kumar Halder
2023-02-11 0:14 ` Stefano Stabellini
` (2 more replies)
2023-02-08 12:05 ` [XEN v3 5/9] xen/arm: smmu: Use writeq_relaxed_non_atomic() for writing to SMMU_CBn_TTBR0 Ayan Kumar Halder
` (4 subsequent siblings)
8 siblings, 3 replies; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 12:05 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh, Ayan Kumar Halder
dt_device_get_address() can accept u64 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 u64.
The reason for introducing doing this is that in future 'paddr_t' may
be defined as u32. 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.
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 | 36 ++++++++++++++++++++--
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, 105 insertions(+), 73 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4d7e67560f..c7e88f5011 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1692,13 +1692,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",
@@ -2471,7 +2471,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:
@@ -2526,7 +2526,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",
@@ -2957,7 +2957,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, ®_base_64, NULL);
+ rc = dt_device_get_paddr(node, 0, ®_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..d0f19d5cfc 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,37 @@ 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)
+{
+ u64 dt_addr = 0, dt_size = 0;
+ int ret;
+
+ ret = dt_device_get_address(dev, index, &dt_addr, &dt_size);
+
+ if ( addr )
+ {
+ if ( (dt_addr >> (PADDR_SHIFT - 1)) > 1 )
+ {
+ printk("Error: Physical address greater than max width supported\n");
+ return -EINVAL;
+ }
+
+ *addr = dt_addr;
+ }
+
+ if ( size )
+ {
+ if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
+ {
+ printk("Error: Physical size greater than max width supported\n");
+ return -EINVAL;
+ }
+ *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 2aee5642f9..9a5e26a8d2 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1769,7 +1769,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 d58c5cd0bf..79d20eed20 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -2451,7 +2451,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
}
/* Base address */
- ret = dt_device_get_address(dev_to_dt(pdev), 0, &ioaddr, &iosize);
+ ret = dt_device_get_paddr(dev_to_dt(pdev), 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 b61bac2931..04aa43e0ee 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -583,7 +583,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
@@ -592,8 +592,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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [XEN v3 5/9] xen/arm: smmu: Use writeq_relaxed_non_atomic() for writing to SMMU_CBn_TTBR0
2023-02-08 12:05 [XEN v3 0/9] Add support for 32 bit physical address Ayan Kumar Halder
` (3 preceding siblings ...)
2023-02-08 12:05 ` [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t Ayan Kumar Halder
@ 2023-02-08 12:05 ` Ayan Kumar Halder
2023-02-08 12:05 ` [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing Ayan Kumar Halder
` (3 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 12:05 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh, 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.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
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.
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing
2023-02-08 12:05 [XEN v3 0/9] Add support for 32 bit physical address Ayan Kumar Halder
` (4 preceding siblings ...)
2023-02-08 12:05 ` [XEN v3 5/9] xen/arm: smmu: Use writeq_relaxed_non_atomic() for writing to SMMU_CBn_TTBR0 Ayan Kumar Halder
@ 2023-02-08 12:05 ` Ayan Kumar Halder
2023-02-11 0:34 ` Stefano Stabellini
2023-02-11 10:01 ` Julien Grall
2023-02-08 12:05 ` [XEN v3 7/9] xen/arm: guest_walk: LPAE specific bits should be enclosed within "ifndef CONFIG_ARM_PA_32" Ayan Kumar Halder
` (2 subsequent siblings)
8 siblings, 2 replies; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 12:05 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh, 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_32) or 64 bits (PHYS_ADDR_64).
For now only ARM_32 provides support to enable 32 bit physical
addressing.
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'.
(Jan,Julien please let me know if I understood your suggestion about Kconfig correctly).
xen/arch/Kconfig | 12 +++++++++++
xen/arch/arm/Kconfig | 31 ++++++++++++++++++++++++++++
xen/arch/arm/include/asm/page-bits.h | 2 ++
xen/arch/arm/include/asm/types.h | 6 ++++++
4 files changed, 51 insertions(+)
diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
index 7028f7b74f..1eff312b51 100644
--- a/xen/arch/Kconfig
+++ b/xen/arch/Kconfig
@@ -1,6 +1,18 @@
config 64BIT
bool
+config PHYS_ADDR_32
+ bool
+ help
+ Select this option if the physical addresses can be represented by
+ 32 bits.
+
+config PHYS_ADDR_64
+ bool
+ help
+ Select this option if the physical addresses can be represented
+ 64 bits.
+
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..0955891e86 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -18,6 +18,37 @@ config ARM
select HAS_PDX
select HAS_PMAP
select IOMMU_FORCE_PT_SHARE
+ choice
+ bool "Representative width for any physical address (default 64bit)"
+ optional
+ ---help---
+ You may want to specify the width to represent the physical
+ address space.
+ By default, the physical addresses are represented using
+ 64 bits.
+
+ However in certain platforms, the physical addresses may be
+ represented using 32 bits.
+ Also, the user may decide that the physical addresses can be
+ represented using 32 bits for a given SoC. In those cases,
+ user may want to enable 32 bit physical address for
+ optimization.
+ For now, we have enabled this choice for ARM_32 only.
+
+ config ARM_PA_64
+ select PHYS_ADDR_64
+ bool "Select 64 bits to represent physical address"
+ ---help---
+ Use 64 bits to represent physical address.
+
+ config ARM_PA_32
+ select PHYS_ADDR_32
+ depends on ARM_32
+ bool "Select 32 bits to represent physical address"
+ ---help---
+ Use 32 bits to represent physical address.
+
+ endchoice
config ARCH_DEFCONFIG
string
diff --git a/xen/arch/arm/include/asm/page-bits.h b/xen/arch/arm/include/asm/page-bits.h
index 5d6477e599..8f4dcebcfd 100644
--- a/xen/arch/arm/include/asm/page-bits.h
+++ b/xen/arch/arm/include/asm/page-bits.h
@@ -5,6 +5,8 @@
#ifdef CONFIG_ARM_64
#define PADDR_BITS 48
+#elif CONFIG_ARM_PA_32
+#define PADDR_BITS 32
#else
#define PADDR_BITS 40
#endif
diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h
index e218ed77bd..26144bc87e 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_ARM_PA_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)
--
2.17.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [XEN v3 7/9] xen/arm: guest_walk: LPAE specific bits should be enclosed within "ifndef CONFIG_ARM_PA_32"
2023-02-08 12:05 [XEN v3 0/9] Add support for 32 bit physical address Ayan Kumar Halder
` (5 preceding siblings ...)
2023-02-08 12:05 ` [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing Ayan Kumar Halder
@ 2023-02-08 12:05 ` Ayan Kumar Halder
2023-02-11 0:35 ` Stefano Stabellini
2023-02-08 12:05 ` [XEN v3 8/9] xen/arm: Restrict zeroeth_table_offset for ARM_64 Ayan Kumar Halder
2023-02-08 12:05 ` [XEN v3 9/9] xen/arm: p2m: Enable support for 32bit IPA Ayan Kumar Halder
8 siblings, 1 reply; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 12:05 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh, Ayan Kumar Halder
In the subsequent patch, we introduce "CONFIG_ARM_PA_32" to support
32 bit physical addresses. Thus, the code specific to
"Large Physical Address Extension" (ie LPAE) should be enclosed within
"ifndef CONFIG_ARM_PA_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>
---
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).
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..0feb7b76ec 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_ARM_PA_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_ARM_PA_32 */
}
/* Set permissions so that the caller can check the flags by herself. */
--
2.17.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [XEN v3 8/9] xen/arm: Restrict zeroeth_table_offset for ARM_64
2023-02-08 12:05 [XEN v3 0/9] Add support for 32 bit physical address Ayan Kumar Halder
` (6 preceding siblings ...)
2023-02-08 12:05 ` [XEN v3 7/9] xen/arm: guest_walk: LPAE specific bits should be enclosed within "ifndef CONFIG_ARM_PA_32" Ayan Kumar Halder
@ 2023-02-08 12:05 ` Ayan Kumar Halder
2023-02-11 0:38 ` Stefano Stabellini
2023-02-11 10:18 ` Julien Grall
2023-02-08 12:05 ` [XEN v3 9/9] xen/arm: p2m: Enable support for 32bit IPA Ayan Kumar Halder
8 siblings, 2 replies; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 12:05 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh, 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.
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.
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..998edeed6e 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_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 b99806af99..44942c6a4f 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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [XEN v3 9/9] xen/arm: p2m: Enable support for 32bit IPA
2023-02-08 12:05 [XEN v3 0/9] Add support for 32 bit physical address Ayan Kumar Halder
` (7 preceding siblings ...)
2023-02-08 12:05 ` [XEN v3 8/9] xen/arm: Restrict zeroeth_table_offset for ARM_64 Ayan Kumar Halder
@ 2023-02-08 12:05 ` Ayan Kumar Halder
2023-02-11 10:21 ` Julien Grall
8 siblings, 1 reply; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 12:05 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh, Ayan Kumar Halder
VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
"Virtualization Translation Control Register" for the bit descriptions.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes from -
v1 - New patch.
v2 - 1. Added Ack.
xen/arch/arm/p2m.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 948f199d84..cfdea55e71 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void)
register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
#ifdef CONFIG_ARM_32
- if ( p2m_ipa_bits < 40 )
+ if ( p2m_ipa_bits < PADDR_BITS )
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;
+ printk("P2M: %u-bit IPA\n",PADDR_BITS);
+ p2m_ipa_bits = PADDR_BITS;
+#ifdef CONFIG_ARM_PA_32
+ val |= VTCR_T0SZ(0x20); /* 32 bit IPA */
+#else
val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
+#endif
val |= VTCR_SL0(0x1); /* P2M starts at first level */
#else /* CONFIG_ARM_64 */
static const struct {
--
2.17.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t
2023-02-08 12:05 ` [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t Ayan Kumar Halder
@ 2023-02-08 13:33 ` Jan Beulich
2023-02-08 15:51 ` Ayan Kumar Halder
2023-02-11 0:08 ` Stefano Stabellini
1 sibling, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2023-02-08 13:33 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, wl, rahul.singh,
xen-devel
On 08.02.2023 13:05, 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.
> We wish to make the DT functions read u64 as well u32 values (depending
> on the width of physical address). Also, we wish to detect if any
> truncation has occurred (ie while reading u32 physical addresses from
> u64 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
> u64 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 u64 values from dt (For eg dt_property_read_u64()).
>
> Xen prints an error when it detects a truncation (during typecast
> between u64 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.
>
> To detect truncation, we right shift physical address by
> "PADDR_BITS - 1" and then if the resulting number is greater than 1,
> we know that truncation has occurred and an appropriate error log is
> printed.
>
> 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.
>
> xen/arch/arm/bootfdt.c | 38 ++++++++++++++++-----
> xen/arch/arm/domain_build.c | 2 +-
> xen/arch/arm/include/asm/setup.h | 2 +-
> 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 +++++++++++++++++++++++++++++
> xen/include/xen/types.h | 2 ++
> 8 files changed, 115 insertions(+), 18 deletions(-)
> create mode 100644 xen/include/xen/libfdt/libfdt_xen.h
Can you please avoid underscores in the names of new files, unless they're
strictly required for some reason?
> @@ -53,10 +53,30 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
> }
>
> void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
> - u32 size_cells, u64 *start, u64 *size)
> + u32 size_cells, paddr_t *start, paddr_t *size)
> {
> - *start = dt_next_cell(address_cells, cell);
> - *size = dt_next_cell(size_cells, cell);
> + u64 dt_start, dt_size;
No new uses of this type (and its siblings), please. We're in the process
of phasing out their use. See ./CODING_STYLE.
> + /*
> + * 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_SHIFT - 1)) > 1 )
> + printk("Error: Physical address greater than max width supported\n");
> +
> + if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
> + printk("Error: Physical size greater than max width supported\n");
While I assume you wrote the checks this way to avoid "shift count too
large" compiler diagnostics, personally I think this is making it
harder to recognize what is wanted. Already (val >> (PADDR_SHIFT - 1)) >> 1
would be better imo, by why not simply (paddr_t)val != val?
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -71,4 +71,6 @@ typedef bool bool_t;
> #define test_and_set_bool(b) xchg(&(b), true)
> #define test_and_clear_bool(b) xchg(&(b), false)
>
> +#define PADDR_SHIFT PADDR_BITS
Why do we need this alias? And if we need it, on what basis did you pick
the file you've put it in?
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 3/9] xen/drivers: ns16550: Use paddr_t for io_base/io_size
2023-02-08 12:05 ` [XEN v3 3/9] xen/drivers: ns16550: Use paddr_t for io_base/io_size Ayan Kumar Halder
@ 2023-02-08 13:52 ` Jan Beulich
2023-02-08 17:07 ` Ayan Kumar Halder
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2023-02-08 13:52 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, wl, rahul.singh,
xen-devel
On 08.02.2023 13:05, Ayan Kumar Halder wrote:
> @@ -1166,8 +1166,9 @@ 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;
> + u64 pci_uart_io_base;
uint64_t please (also elsewhere as needed), assuming the variable
actually needs to survive. And if it needs to, please move it into
a more narrow scope (and perhaps shorten its name).
> @@ -1259,8 +1260,13 @@ 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 */
> + BUG_ON((pci_uart_io_base >> (PADDR_SHIFT - 1)) > 1);
Why would we want to crash during early boot at all? And then even at a
point where it'll be hard to figure out what's going on, as we're only
in the process of configuring the serial console?
> @@ -1532,7 +1539,12 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
> else
> #endif
> {
> - uart->io_base = simple_strtoull(conf, &conf, 0);
> + uart_io_base = simple_strtoull(conf, &conf, 0);
> +
> + /* Truncation detected while converting to paddr_t */
> + BUG_ON((uart_io_base >> (PADDR_SHIFT - 1)) > 1);
All the same here.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t
2023-02-08 13:33 ` Jan Beulich
@ 2023-02-08 15:51 ` Ayan Kumar Halder
2023-02-09 7:38 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 15:51 UTC (permalink / raw)
To: Jan Beulich, Ayan Kumar Halder
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, wl, rahul.singh,
xen-devel
Hi Jan,
On 08/02/2023 13:33, Jan Beulich wrote:
> On 08.02.2023 13:05, 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.
>> We wish to make the DT functions read u64 as well u32 values (depending
>> on the width of physical address). Also, we wish to detect if any
>> truncation has occurred (ie while reading u32 physical addresses from
>> u64 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
>> u64 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 u64 values from dt (For eg dt_property_read_u64()).
>>
>> Xen prints an error when it detects a truncation (during typecast
>> between u64 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.
>>
>> To detect truncation, we right shift physical address by
>> "PADDR_BITS - 1" and then if the resulting number is greater than 1,
>> we know that truncation has occurred and an appropriate error log is
>> printed.
>>
>> 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.
>>
>> xen/arch/arm/bootfdt.c | 38 ++++++++++++++++-----
>> xen/arch/arm/domain_build.c | 2 +-
>> xen/arch/arm/include/asm/setup.h | 2 +-
>> 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 +++++++++++++++++++++++++++++
>> xen/include/xen/types.h | 2 ++
>> 8 files changed, 115 insertions(+), 18 deletions(-)
>> create mode 100644 xen/include/xen/libfdt/libfdt_xen.h
> Can you please avoid underscores in the names of new files, unless they're
> strictly required for some reason?
Actually I introduced libfdt_xen.h as I did not wish to modify the
fdt_get_mem_rsv() (present in fdt_ro.c).
So I created libfdt_xen.h to implement fdt_get_mem_rsv_paddr(). The
*_xen.h denotes that it is derived from * (which can be modified like
any other file).
Let me know what name you suggest.
>
>> @@ -53,10 +53,30 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
>> }
>>
>> void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
>> - u32 size_cells, u64 *start, u64 *size)
>> + u32 size_cells, paddr_t *start, paddr_t *size)
>> {
>> - *start = dt_next_cell(address_cells, cell);
>> - *size = dt_next_cell(size_cells, cell);
>> + u64 dt_start, dt_size;
> No new uses of this type (and its siblings), please. We're in the process
> of phasing out their use. See ./CODING_STYLE.
Ack
>
>> + /*
>> + * 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_SHIFT - 1)) > 1 )
>> + printk("Error: Physical address greater than max width supported\n");
>> +
>> + if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
>> + printk("Error: Physical size greater than max width supported\n");
> While I assume you wrote the checks this way to avoid "shift count too
> large" compiler diagnostics, personally I think this is making it
> harder to recognize what is wanted. Already (val >> (PADDR_SHIFT - 1)) >> 1
> would be better imo, by why not simply (paddr_t)val != val?
Yes, makes sense.
I will use "(paddr_t)val != val" to check for truncation.
>
>> --- a/xen/include/xen/types.h
>> +++ b/xen/include/xen/types.h
>> @@ -71,4 +71,6 @@ typedef bool bool_t;
>> #define test_and_set_bool(b) xchg(&(b), true)
>> #define test_and_clear_bool(b) xchg(&(b), false)
>>
>> +#define PADDR_SHIFT PADDR_BITS
> Why do we need this alias?
I could have use PADDR_BITS instead, but decided to use PADDR_SHIFT for
cosmetic reasons.
> And if we need it, on what basis did you pick
> the file you've put it in?
I wanted to put in an arch independent file (where we have defined
macros related to data types).
- Ayan
>
> Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 3/9] xen/drivers: ns16550: Use paddr_t for io_base/io_size
2023-02-08 13:52 ` Jan Beulich
@ 2023-02-08 17:07 ` Ayan Kumar Halder
2023-02-09 7:48 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-08 17:07 UTC (permalink / raw)
To: Jan Beulich, Ayan Kumar Halder
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, wl, rahul.singh,
xen-devel
Hi Jan,
On 08/02/2023 13:52, Jan Beulich wrote:
> On 08.02.2023 13:05, Ayan Kumar Halder wrote:
>> @@ -1166,8 +1166,9 @@ 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;
>> + u64 pci_uart_io_base;
> uint64_t please (also elsewhere as needed), assuming the variable
> actually needs to survive. And if it needs to, please move it into
> a more narrow scope (and perhaps shorten its name).
Ack.
>
>> @@ -1259,8 +1260,13 @@ 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 */
>> + BUG_ON((pci_uart_io_base >> (PADDR_SHIFT - 1)) > 1);
> Why would we want to crash during early boot at all? And then even at a
> point where it'll be hard to figure out what's going on, as we're only
> in the process of configuring the serial console?
I do not understand the best way to return an error from pci_uart_config().
Out of the 4 invocations of pci_uart_config(), the return value is
checked in two invocations only like this.
if ( pci_uart_config(uart, 0, uart - ns16550_com) )
return true;
pci_uart_config() returns 0 in case of success and -1 in case of error.
But the caller seems to be checking the opposite.
I could not use domain_crash() as I understand that pci_uart_config() is
invoked before domain is created.
>
>> @@ -1532,7 +1539,12 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>> else
>> #endif
>> {
>> - uart->io_base = simple_strtoull(conf, &conf, 0);
>> + uart_io_base = simple_strtoull(conf, &conf, 0);
>> +
>> + /* Truncation detected while converting to paddr_t */
>> + BUG_ON((uart_io_base >> (PADDR_SHIFT - 1)) > 1);
> All the same here.
I can return false from here and let ns16550_parse_port_config() return.
I think that might be the correct thing to do here.
- Ayan
>
> Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t
2023-02-08 15:51 ` Ayan Kumar Halder
@ 2023-02-09 7:38 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2023-02-09 7:38 UTC (permalink / raw)
To: Ayan Kumar Halder, Ayan Kumar Halder
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, wl, rahul.singh,
xen-devel
On 08.02.2023 16:51, Ayan Kumar Halder wrote:
> On 08/02/2023 13:33, Jan Beulich wrote:
>> On 08.02.2023 13:05, 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.
>>> We wish to make the DT functions read u64 as well u32 values (depending
>>> on the width of physical address). Also, we wish to detect if any
>>> truncation has occurred (ie while reading u32 physical addresses from
>>> u64 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
>>> u64 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 u64 values from dt (For eg dt_property_read_u64()).
>>>
>>> Xen prints an error when it detects a truncation (during typecast
>>> between u64 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.
>>>
>>> To detect truncation, we right shift physical address by
>>> "PADDR_BITS - 1" and then if the resulting number is greater than 1,
>>> we know that truncation has occurred and an appropriate error log is
>>> printed.
>>>
>>> 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.
>>>
>>> xen/arch/arm/bootfdt.c | 38 ++++++++++++++++-----
>>> xen/arch/arm/domain_build.c | 2 +-
>>> xen/arch/arm/include/asm/setup.h | 2 +-
>>> 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 +++++++++++++++++++++++++++++
>>> xen/include/xen/types.h | 2 ++
>>> 8 files changed, 115 insertions(+), 18 deletions(-)
>>> create mode 100644 xen/include/xen/libfdt/libfdt_xen.h
>> Can you please avoid underscores in the names of new files, unless they're
>> strictly required for some reason?
>
> Actually I introduced libfdt_xen.h as I did not wish to modify the
> fdt_get_mem_rsv() (present in fdt_ro.c).
>
> So I created libfdt_xen.h to implement fdt_get_mem_rsv_paddr(). The
> *_xen.h denotes that it is derived from * (which can be modified like
> any other file).
>
> Let me know what name you suggest.
I don't mind the new file, all I do mind is the underscore in its name
when a dash will do. Underscores should be used in place of dashes only
when dashes can't be used (e.g. in identifiers, where dash represents
the minus operator and hence can't be used in identifiers).
>>> --- a/xen/include/xen/types.h
>>> +++ b/xen/include/xen/types.h
>>> @@ -71,4 +71,6 @@ typedef bool bool_t;
>>> #define test_and_set_bool(b) xchg(&(b), true)
>>> #define test_and_clear_bool(b) xchg(&(b), false)
>>>
>>> +#define PADDR_SHIFT PADDR_BITS
>> Why do we need this alias?
> I could have use PADDR_BITS instead, but decided to use PADDR_SHIFT for
> cosmetic reasons.
I, for one, disagree with this (it's pretty unclear to me what "shift" here
is to express) as well as ...
>> And if we need it, on what basis did you pick
>> the file you've put it in?
>
> I wanted to put in an arch independent file (where we have defined
> macros related to data types).
... this placement.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 3/9] xen/drivers: ns16550: Use paddr_t for io_base/io_size
2023-02-08 17:07 ` Ayan Kumar Halder
@ 2023-02-09 7:48 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2023-02-09 7:48 UTC (permalink / raw)
To: Ayan Kumar Halder, Ayan Kumar Halder
Cc: sstabellini, stefano.stabellini, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, wl, rahul.singh,
xen-devel
On 08.02.2023 18:07, Ayan Kumar Halder wrote:
> Hi Jan,
>
> On 08/02/2023 13:52, Jan Beulich wrote:
>> On 08.02.2023 13:05, Ayan Kumar Halder wrote:
>>> @@ -1166,8 +1166,9 @@ 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;
>>> + u64 pci_uart_io_base;
>> uint64_t please (also elsewhere as needed), assuming the variable
>> actually needs to survive. And if it needs to, please move it into
>> a more narrow scope (and perhaps shorten its name).
> Ack.
>>
>>> @@ -1259,8 +1260,13 @@ 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 */
>>> + BUG_ON((pci_uart_io_base >> (PADDR_SHIFT - 1)) > 1);
>> Why would we want to crash during early boot at all? And then even at a
>> point where it'll be hard to figure out what's going on, as we're only
>> in the process of configuring the serial console?
>
> I do not understand the best way to return an error from pci_uart_config().
>
> Out of the 4 invocations of pci_uart_config(), the return value is
> checked in two invocations only like this.
>
> if ( pci_uart_config(uart, 0, uart - ns16550_com) )
> return true;
>
> pci_uart_config() returns 0 in case of success and -1 in case of error.
> But the caller seems to be checking the opposite.
The callers look a little odd, I agree, but iirc that's intentional. Since
this is all PCI _and_ x86-only code, which you don't care all that much
about, one option is to leave that code alone for the time being. The other
is to properly propagate such an error through all involved code paths, e.g.
by way of invoking PARSE_ERR_RET() in parse_namevalue_pairs() in the failure
case.
>>> @@ -1532,7 +1539,12 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>>> else
>>> #endif
>>> {
>>> - uart->io_base = simple_strtoull(conf, &conf, 0);
>>> + uart_io_base = simple_strtoull(conf, &conf, 0);
>>> +
>>> + /* Truncation detected while converting to paddr_t */
>>> + BUG_ON((uart_io_base >> (PADDR_SHIFT - 1)) > 1);
>> All the same here.
>
> I can return false from here and let ns16550_parse_port_config() return.
>
> I think that might be the correct thing to do here.
Perhaps, and likely again by way of using PARSE_ERR_RET().
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t
2023-02-08 12:05 ` [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t Ayan Kumar Halder
2023-02-08 13:33 ` Jan Beulich
@ 2023-02-11 0:08 ` Stefano Stabellini
1 sibling, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2023-02-11 0:08 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefano.stabellini, julien,
Volodymyr_Babchuk, bertrand.marquis, andrew.cooper3,
george.dunlap, jbeulich, wl, rahul.singh
On Wed, 8 Feb 2023, 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.
> We wish to make the DT functions read u64 as well u32 values (depending
> on the width of physical address). Also, we wish to detect if any
> truncation has occurred (ie while reading u32 physical addresses from
> u64 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
> u64 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 u64 values from dt (For eg dt_property_read_u64()).
>
> Xen prints an error when it detects a truncation (during typecast
> between u64 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.
>
> To detect truncation, we right shift physical address by
> "PADDR_BITS - 1" and then if the resulting number is greater than 1,
> we know that truncation has occurred and an appropriate error log is
> printed.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
I just wanted to record that aside from Jan's feedback, this patch looks
good to me
> ---
>
> 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.
>
> xen/arch/arm/bootfdt.c | 38 ++++++++++++++++-----
> xen/arch/arm/domain_build.c | 2 +-
> xen/arch/arm/include/asm/setup.h | 2 +-
> 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 +++++++++++++++++++++++++++++
> xen/include/xen/types.h | 2 ++
> 8 files changed, 115 insertions(+), 18 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..f63da3e831 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>
> @@ -53,10 +53,30 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
> }
>
> void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
> - u32 size_cells, u64 *start, u64 *size)
> + u32 size_cells, paddr_t *start, paddr_t *size)
> {
> - *start = dt_next_cell(address_cells, cell);
> - *size = dt_next_cell(size_cells, cell);
> + u64 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_SHIFT - 1)) > 1 )
> + printk("Error: Physical address greater than max width supported\n");
> +
> + if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
> + 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 +346,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 +359,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 +614,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 a798e0b256..4d7e67560f 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..6105e5cae3 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -158,7 +158,7 @@ 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);
> + u32 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..5ade20e6e7 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 a28937d12a..b61bac2931 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -244,6 +244,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)
> +{
> + u64 dt_r = 0;
> + paddr_t r;
> +
> + dt_r = dt_read_number(cell, size);
> +
> + if ( (dt_r >> (PADDR_SHIFT - 1)) > 1 )
> + 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..a3196dd96c
> --- /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_SHIFT - 1)) > 1 )
> + {
> + printk("Error: Physical address greater than max width supported\n");
> + return -1;
> + }
> +
> + if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
> + {
> + 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:
> + */
> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> index 6aba80500a..b7255c7c38 100644
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -71,4 +71,6 @@ typedef bool bool_t;
> #define test_and_set_bool(b) xchg(&(b), true)
> #define test_and_clear_bool(b) xchg(&(b), false)
>
> +#define PADDR_SHIFT PADDR_BITS
> +
> #endif /* __TYPES_H__ */
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t
2023-02-08 12:05 ` [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t Ayan Kumar Halder
@ 2023-02-11 0:14 ` Stefano Stabellini
2023-02-11 0:20 ` Stefano Stabellini
2023-02-11 9:25 ` Julien Grall
2 siblings, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2023-02-11 0:14 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefano.stabellini, julien,
Volodymyr_Babchuk, bertrand.marquis, andrew.cooper3,
george.dunlap, jbeulich, wl, rahul.singh
On Wed, 8 Feb 2023, Ayan Kumar Halder wrote:
> dt_device_get_address() can accept u64 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 u64.
>
> The reason for introducing doing this is that in future 'paddr_t' may
> be defined as u32. 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.
>
> 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 | 36 ++++++++++++++++++++--
> 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, 105 insertions(+), 73 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4d7e67560f..c7e88f5011 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1692,13 +1692,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",
> @@ -2471,7 +2471,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:
> @@ -2526,7 +2526,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",
> @@ -2957,7 +2957,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, ®_base_64, NULL);
> + rc = dt_device_get_paddr(node, 0, ®_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..d0f19d5cfc 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,37 @@ 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)
> +{
> + u64 dt_addr = 0, dt_size = 0;
> + int ret;
> +
> + ret = dt_device_get_address(dev, index, &dt_addr, &dt_size);
Here we should have:
if ( ret != 0 )
return ret;
> + if ( addr )
> + {
> + if ( (dt_addr >> (PADDR_SHIFT - 1)) > 1 )
If we are going to update the way this check is written in patch #2,
please also update it here...
> + {
> + printk("Error: Physical address greater than max width supported\n");
> + return -EINVAL;
> + }
> +
> + *addr = dt_addr;
> + }
> +
> + if ( size )
> + {
> + if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
... and here
In any case this patch looks good to me
> + {
> + printk("Error: Physical size greater than max width supported\n");
> + return -EINVAL;
> + }
> + *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 2aee5642f9..9a5e26a8d2 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1769,7 +1769,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 d58c5cd0bf..79d20eed20 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -2451,7 +2451,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> }
>
> /* Base address */
> - ret = dt_device_get_address(dev_to_dt(pdev), 0, &ioaddr, &iosize);
> + ret = dt_device_get_paddr(dev_to_dt(pdev), 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 b61bac2931..04aa43e0ee 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -583,7 +583,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
> @@ -592,8 +592,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
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t
2023-02-08 12:05 ` [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t Ayan Kumar Halder
2023-02-11 0:14 ` Stefano Stabellini
@ 2023-02-11 0:20 ` Stefano Stabellini
2023-02-11 9:10 ` Julien Grall
2023-02-11 9:25 ` Julien Grall
2 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2023-02-11 0:20 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefano.stabellini, julien,
Volodymyr_Babchuk, bertrand.marquis, andrew.cooper3,
george.dunlap, jbeulich, wl, rahul.singh
On Wed, 8 Feb 2023, Ayan Kumar Halder wrote:
> dt_device_get_address() can accept u64 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 u64.
>
> The reason for introducing doing this is that in future 'paddr_t' may
> be defined as u32. 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.
>
> 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 | 36 ++++++++++++++++++++--
> 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, 105 insertions(+), 73 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4d7e67560f..c7e88f5011 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1692,13 +1692,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);
One thing to be careful here is that "start" and "end" in
find_memory_holes are defined now as paddr_t and passed to
rangeset_add_range and rangeset_remove_range.
rangeset_add_range and rangeset_remove_range take an unsigned long as
parameter, so in an arm32 configuration with paddr_t set to 64-bit it
would result in truncation
> if ( res )
> {
> printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
> @@ -2471,7 +2471,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:
> @@ -2526,7 +2526,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",
> @@ -2957,7 +2957,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, ®_base_64, NULL);
> + rc = dt_device_get_paddr(node, 0, ®_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..d0f19d5cfc 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,37 @@ 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)
> +{
> + u64 dt_addr = 0, dt_size = 0;
> + int ret;
> +
> + ret = dt_device_get_address(dev, index, &dt_addr, &dt_size);
> +
> + if ( addr )
> + {
> + if ( (dt_addr >> (PADDR_SHIFT - 1)) > 1 )
> + {
> + printk("Error: Physical address greater than max width supported\n");
> + return -EINVAL;
> + }
> +
> + *addr = dt_addr;
> + }
> +
> + if ( size )
> + {
> + if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
> + {
> + printk("Error: Physical size greater than max width supported\n");
> + return -EINVAL;
> + }
> + *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 2aee5642f9..9a5e26a8d2 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1769,7 +1769,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 d58c5cd0bf..79d20eed20 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -2451,7 +2451,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> }
>
> /* Base address */
> - ret = dt_device_get_address(dev_to_dt(pdev), 0, &ioaddr, &iosize);
> + ret = dt_device_get_paddr(dev_to_dt(pdev), 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 b61bac2931..04aa43e0ee 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -583,7 +583,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
> @@ -592,8 +592,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
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing
2023-02-08 12:05 ` [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing Ayan Kumar Halder
@ 2023-02-11 0:34 ` Stefano Stabellini
2023-02-11 10:07 ` Julien Grall
2023-02-11 10:01 ` Julien Grall
1 sibling, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2023-02-11 0:34 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefano.stabellini, julien,
Volodymyr_Babchuk, bertrand.marquis, andrew.cooper3,
george.dunlap, jbeulich, wl, rahul.singh
On Wed, 8 Feb 2023, Ayan Kumar Halder wrote:
> Some Arm based hardware platforms which does not support LPAE
> (eg Cortex-R52), uses 32 bit physical addresses.
> Also, users may choose to use 32 bits to represent physical addresses
> for optimization.
>
> To support the above use cases, we have introduced arch independent
> configs to choose if the physical address can be represented using
> 32 bits (PHYS_ADDR_32) or 64 bits (PHYS_ADDR_64).
> For now only ARM_32 provides support to enable 32 bit physical
> addressing.
>
> 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'.
>
> (Jan,Julien please let me know if I understood your suggestion about Kconfig correctly).
>
> xen/arch/Kconfig | 12 +++++++++++
> xen/arch/arm/Kconfig | 31 ++++++++++++++++++++++++++++
> xen/arch/arm/include/asm/page-bits.h | 2 ++
> xen/arch/arm/include/asm/types.h | 6 ++++++
> 4 files changed, 51 insertions(+)
>
> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
> index 7028f7b74f..1eff312b51 100644
> --- a/xen/arch/Kconfig
> +++ b/xen/arch/Kconfig
> @@ -1,6 +1,18 @@
> config 64BIT
> bool
>
> +config PHYS_ADDR_32
> + bool
> + help
> + Select this option if the physical addresses can be represented by
> + 32 bits.
> +
> +config PHYS_ADDR_64
> + bool
> + help
> + Select this option if the physical addresses can be represented
> + 64 bits.
These two config symbols should be defined in xen/arch/arm/Kconfig
(unless you plan to also define them for x86).
> 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..0955891e86 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -18,6 +18,37 @@ config ARM
> select HAS_PDX
> select HAS_PMAP
> select IOMMU_FORCE_PT_SHARE
> + choice
This shows up at the very beginning of the global menu. I think it would
be better to move it under "Architecture Features"
Also given that there is not choice on arm64, I don't think the choice
option should be visible to the user (because the user cannot change
selection for arm64).
> + bool "Representative width for any physical address (default 64bit)"
> + optional
should not be "optional", optional means that neither ARM_PA_64 nor
ARM_PA_32 could be selected. I think we want one of the two to always be
selected, ARM_PA_64 being the default option, and the only option on
arm64.
> + ---help---
> + You may want to specify the width to represent the physical
> + address space.
> + By default, the physical addresses are represented using
> + 64 bits.
> +
> + However in certain platforms, the physical addresses may be
> + represented using 32 bits.
> + Also, the user may decide that the physical addresses can be
> + represented using 32 bits for a given SoC. In those cases,
> + user may want to enable 32 bit physical address for
> + optimization.
> + For now, we have enabled this choice for ARM_32 only.
> +
> + config ARM_PA_64
> + select PHYS_ADDR_64
> + bool "Select 64 bits to represent physical address"
> + ---help---
> + Use 64 bits to represent physical address.
> +
> + config ARM_PA_32
> + select PHYS_ADDR_32
> + depends on ARM_32
> + bool "Select 32 bits to represent physical address"
> + ---help---
> + Use 32 bits to represent physical address.
> +
> + endchoice
>
> config ARCH_DEFCONFIG
> string
> diff --git a/xen/arch/arm/include/asm/page-bits.h b/xen/arch/arm/include/asm/page-bits.h
> index 5d6477e599..8f4dcebcfd 100644
> --- a/xen/arch/arm/include/asm/page-bits.h
> +++ b/xen/arch/arm/include/asm/page-bits.h
> @@ -5,6 +5,8 @@
>
> #ifdef CONFIG_ARM_64
> #define PADDR_BITS 48
> +#elif CONFIG_ARM_PA_32
> +#define PADDR_BITS 32
> #else
> #define PADDR_BITS 40
> #endif
> diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h
> index e218ed77bd..26144bc87e 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_ARM_PA_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)
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 7/9] xen/arm: guest_walk: LPAE specific bits should be enclosed within "ifndef CONFIG_ARM_PA_32"
2023-02-08 12:05 ` [XEN v3 7/9] xen/arm: guest_walk: LPAE specific bits should be enclosed within "ifndef CONFIG_ARM_PA_32" Ayan Kumar Halder
@ 2023-02-11 0:35 ` Stefano Stabellini
0 siblings, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2023-02-11 0:35 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefano.stabellini, julien,
Volodymyr_Babchuk, bertrand.marquis, andrew.cooper3,
george.dunlap, jbeulich, wl, rahul.singh
On Wed, 8 Feb 2023, Ayan Kumar Halder wrote:
> In the subsequent patch, we introduce "CONFIG_ARM_PA_32" to support
> 32 bit physical addresses. Thus, the code specific to
> "Large Physical Address Extension" (ie LPAE) should be enclosed within
> "ifndef CONFIG_ARM_PA_32".
this statement needs an update
> 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>
Aside from the above:
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).
>
> 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..0feb7b76ec 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_ARM_PA_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_ARM_PA_32 */
> }
>
> /* Set permissions so that the caller can check the flags by herself. */
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 8/9] xen/arm: Restrict zeroeth_table_offset for ARM_64
2023-02-08 12:05 ` [XEN v3 8/9] xen/arm: Restrict zeroeth_table_offset for ARM_64 Ayan Kumar Halder
@ 2023-02-11 0:38 ` Stefano Stabellini
2023-02-11 10:18 ` Julien Grall
1 sibling, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2023-02-11 0:38 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefano.stabellini, julien,
Volodymyr_Babchuk, bertrand.marquis, andrew.cooper3,
george.dunlap, jbeulich, wl, rahul.singh
On Wed, 8 Feb 2023, 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.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> 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.
>
> 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..998edeed6e 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_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 b99806af99..44942c6a4f 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
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 1/9] xen/ns16550: Remove unneeded truncation check in the DT init code
2023-02-08 12:05 ` [XEN v3 1/9] xen/ns16550: Remove unneeded truncation check in the DT init code Ayan Kumar Halder
@ 2023-02-11 8:55 ` Julien Grall
0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2023-02-11 8:55 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefano.stabellini, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh
Hi Ayan
On 08/02/2023 12:05, Ayan Kumar Halder wrote:
> In an earlier commit (7c1de0038895), "ns16550.io_size" was u32 and
> "io_size" was u64. Thus, the ASSERT() was needed to check if the values
> are the same.
> However, in a later commit (c9f8e0aee507), "ns16550.io_size" was changed
> to u64. Thus, the ASSERT() became redundant.
>
> So, now as "io_size" and "uart->io_size" are both u64, there will be no
> truncation. Thus, one can remove the ASSERT() and extra assignment.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t
2023-02-11 0:20 ` Stefano Stabellini
@ 2023-02-11 9:10 ` Julien Grall
2023-02-13 12:47 ` Ayan Kumar Halder
0 siblings, 1 reply; 31+ messages in thread
From: Julien Grall @ 2023-02-11 9:10 UTC (permalink / raw)
To: Stefano Stabellini, Ayan Kumar Halder
Cc: xen-devel, stefano.stabellini, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh
Hi,
On 11/02/2023 00:20, Stefano Stabellini wrote:
> On Wed, 8 Feb 2023, Ayan Kumar Halder wrote:
>> dt_device_get_address() can accept u64 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 u64.
>>
>> The reason for introducing doing this is that in future 'paddr_t' may
>> be defined as u32. 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.
>>
>> 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 | 36 ++++++++++++++++++++--
>> 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, 105 insertions(+), 73 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 4d7e67560f..c7e88f5011 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1692,13 +1692,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);
>
> One thing to be careful here is that "start" and "end" in
> find_memory_holes are defined now as paddr_t and passed to
> rangeset_add_range and rangeset_remove_range.
I am a bit puzzled why you are saying "now". Without Ayan's patch addr
was 64-bit so...
>
> rangeset_add_range and rangeset_remove_range take an unsigned long as
> parameter, so in an arm32 configuration with paddr_t set to 64-bit it
> would result in truncation
... the problem you are talking about would already exist.
I would consider to store a frame number rather than the full address
because we can only deal with page-aligned region.
Stefano (or Ayan) can you look at it?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t
2023-02-08 12:05 ` [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t Ayan Kumar Halder
2023-02-11 0:14 ` Stefano Stabellini
2023-02-11 0:20 ` Stefano Stabellini
@ 2023-02-11 9:25 ` Julien Grall
2 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2023-02-11 9:25 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefano.stabellini, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh
Hi Ayan,
On 08/02/2023 12:05, Ayan Kumar Halder wrote:
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 6c9712ab7b..d0f19d5cfc 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,37 @@ 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)
> +{
> + u64 dt_addr = 0, dt_size = 0;
Please use uint64_t for new code.
> + int ret;
> +
> + ret = dt_device_get_address(dev, index, &dt_addr, &dt_size);
> +
> + if ( addr )
> + {
> + if ( (dt_addr >> (PADDR_SHIFT - 1)) > 1 )
> + {
> + printk("Error: Physical address greater than max width supported\n");
I would print the width, address and the node name to help debugging.
> + return -EINVAL;
NIT: -EINVAL tends to be quite overloaded. How about using -ERANGE?
> + }
> +
> + *addr = dt_addr;
> + }
> +
> + if ( size )
> + {
> + if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
> + {
> + printk("Error: Physical size greater than max width supported\n");
Same here for the message but with s/address/size/.
> + return -EINVAL;
Same here for the errno.
The rest looks fine so long Stefano's comment are addressed.
> + }
> + *size = dt_size;
> + }
> +
> + return ret;
> +}
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing
2023-02-08 12:05 ` [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing Ayan Kumar Halder
2023-02-11 0:34 ` Stefano Stabellini
@ 2023-02-11 10:01 ` Julien Grall
2023-02-27 15:21 ` Ayan Kumar Halder
1 sibling, 1 reply; 31+ messages in thread
From: Julien Grall @ 2023-02-11 10:01 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefano.stabellini, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh
Hi Ayan,
On 08/02/2023 12:05, Ayan Kumar Halder wrote:
> Some Arm based hardware platforms which does not support LPAE
> (eg Cortex-R52), uses 32 bit physical addresses.
> Also, users may choose to use 32 bits to represent physical addresses
> for optimization.
>
> To support the above use cases, we have introduced arch independent
> configs to choose if the physical address can be represented using
> 32 bits (PHYS_ADDR_32) or 64 bits (PHYS_ADDR_64).
> For now only ARM_32 provides support to enable 32 bit physical
> addressing.
>
> 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'.
>
> (Jan,Julien please let me know if I understood your suggestion about Kconfig correctly).
>
> xen/arch/Kconfig | 12 +++++++++++
> xen/arch/arm/Kconfig | 31 ++++++++++++++++++++++++++++
> xen/arch/arm/include/asm/page-bits.h | 2 ++
> xen/arch/arm/include/asm/types.h | 6 ++++++
> 4 files changed, 51 insertions(+)
>
> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
> index 7028f7b74f..1eff312b51 100644
> --- a/xen/arch/Kconfig
> +++ b/xen/arch/Kconfig
> @@ -1,6 +1,18 @@
> config 64BIT
> bool
>
> +config PHYS_ADDR_32
> + bool
> + help
> + Select this option if the physical addresses can be represented by
> + 32 bits.
> +
> +config PHYS_ADDR_64
> + bool
> + help
> + Select this option if the physical addresses can be represented
> + 64 bits.
> +
So you likely don't want to allow the user to select them directly (IOW
remove the help section). However, I don't see any code using it. Did
you actually intended to use PHYS_ADDR_{32, 64} in the code?
> 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..0955891e86 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -18,6 +18,37 @@ config ARM
> select HAS_PDX
> select HAS_PMAP
> select IOMMU_FORCE_PT_SHARE
> + choice
I think this is a bit odd that this choice is part of CONFIG_ARM. It
would be better it is separate. You can do that by removing one indentation.
> + bool "Representative width for any physical address (default 64bit)"
> + optional
> + ---help---
> + You may want to specify the width to represent the physical
> + address space.
> + By default, the physical addresses are represented using
> + 64 bits.
> +
> + However in certain platforms, the physical addresses may be
> + represented using 32 bits.
> + Also, the user may decide that the physical addresses can be
> + represented using 32 bits for a given SoC. In those cases,
> + user may want to enable 32 bit physical address for
> + optimization.
> + For now, we have enabled this choice for ARM_32 only.
> +
> + config ARM_PA_64
> + select PHYS_ADDR_64
> + bool "Select 64 bits to represent physical address"
> + ---help---
> + Use 64 bits to represent physical address.
> +
> + config ARM_PA_32
> + select PHYS_ADDR_32
> + depends on ARM_32
> + bool "Select 32 bits to represent physical address"
> + ---help---
> + Use 32 bits to represent physical address.
As I wrote in v2, I think this is a bit odd to ask the user what would
be the width of paddr_t. It is also not scalable if we decide in the
future to define different PADDR_BITS (i.e. 48, 40, 36, 32).
So it would be better to allow the user to define PADDR_BITS that can
then be translated by Xen to which ever paddr_t is suitable.
Something like:
choice
prompt: "Physical address space size" if ARM_32
default ARM_PA_48 if ARM_64
default AMR_PA_40 if ARM_32
help
...
config ARM_PA_BITS_32
bool "32-bit"
help
XXX Add help here to explain the benefits of using 32-bit.
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
With this approach, there would be no structural change in the Kconfig
if we wanted to support 32/40-bit on Arm64.
> +
> + endchoice
>
> config ARCH_DEFCONFIG
> string
> diff --git a/xen/arch/arm/include/asm/page-bits.h b/xen/arch/arm/include/asm/page-bits.h
> index 5d6477e599..8f4dcebcfd 100644
> --- a/xen/arch/arm/include/asm/page-bits.h
> +++ b/xen/arch/arm/include/asm/page-bits.h
> @@ -5,6 +5,8 @@
>
> #ifdef CONFIG_ARM_64
> #define PADDR_BITS 48
> +#elif CONFIG_ARM_PA_32
> +#define PADDR_BITS 32
> #else
> #define PADDR_BITS 40
> #endif
With what I suggested above. This would be replaced with:
#define PADDR_BITS CONFIG_PADDR_BITS
> diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h
> index e218ed77bd..26144bc87e 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_ARM_PA_32)
And this could be replaced with
#ifdef CONFIG_PHY_ADDR_T_32
I would also consider to add the following in mm.c
BUILD_BUG_ON((sizeof(paddr_t) * 8) < PADDR_BITS);
This is to make sure that the PADDR_BITS will always fit in paddr_t.
> +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)
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing
2023-02-11 0:34 ` Stefano Stabellini
@ 2023-02-11 10:07 ` Julien Grall
0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2023-02-11 10:07 UTC (permalink / raw)
To: Stefano Stabellini, Ayan Kumar Halder
Cc: xen-devel, stefano.stabellini, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh
Hi Stefano,
On 11/02/2023 00:34, Stefano Stabellini wrote:
> On Wed, 8 Feb 2023, Ayan Kumar Halder wrote:
>> Some Arm based hardware platforms which does not support LPAE
>> (eg Cortex-R52), uses 32 bit physical addresses.
>> Also, users may choose to use 32 bits to represent physical addresses
>> for optimization.
>>
>> To support the above use cases, we have introduced arch independent
>> configs to choose if the physical address can be represented using
>> 32 bits (PHYS_ADDR_32) or 64 bits (PHYS_ADDR_64).
>> For now only ARM_32 provides support to enable 32 bit physical
>> addressing.
>>
>> 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'.
>>
>> (Jan,Julien please let me know if I understood your suggestion about Kconfig correctly).
>>
>> xen/arch/Kconfig | 12 +++++++++++
>> xen/arch/arm/Kconfig | 31 ++++++++++++++++++++++++++++
>> xen/arch/arm/include/asm/page-bits.h | 2 ++
>> xen/arch/arm/include/asm/types.h | 6 ++++++
>> 4 files changed, 51 insertions(+)
>>
>> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
>> index 7028f7b74f..1eff312b51 100644
>> --- a/xen/arch/Kconfig
>> +++ b/xen/arch/Kconfig
>> @@ -1,6 +1,18 @@
>> config 64BIT
>> bool
>>
>> +config PHYS_ADDR_32
>> + bool
>> + help
>> + Select this option if the physical addresses can be represented by
>> + 32 bits.
>> +
>> +config PHYS_ADDR_64
>> + bool
>> + help
>> + Select this option if the physical addresses can be represented
>> + 64 bits.
>
> These two config symbols should be defined in xen/arch/arm/Kconfig
> (unless you plan to also define them for x86).
We discussed with Jan to consolidate types.h as RISC-V may want to use a
32-bit paddr_t. This would mean paddr_t would be defined a common header
and therefore those config needs to be in common.
I am not asking Ayan to work on the consolidation, but I think they
should be defined in common (assuming arm will make use of it) and I
would at least consider to add "select PHYS_ADDR_64" in "config X86".
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 8/9] xen/arm: Restrict zeroeth_table_offset for ARM_64
2023-02-08 12:05 ` [XEN v3 8/9] xen/arm: Restrict zeroeth_table_offset for ARM_64 Ayan Kumar Halder
2023-02-11 0:38 ` Stefano Stabellini
@ 2023-02-11 10:18 ` Julien Grall
1 sibling, 0 replies; 31+ messages in thread
From: Julien Grall @ 2023-02-11 10:18 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefano.stabellini, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh
Hi,
On 08/02/2023 12:05, 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.
Technically this is unrelated to the goal of the patch. So this should
have been split in a separate patch.
No need to split it this time, but in the future please consider to
split a patch when a change is unrelated.
In general, I would only consider to bundle clean-up if all the below
applies:
1) the patch is not doing code movement
2) the change is simple (e.g. typo, coding style fix)
3) the change is in the vicinity of the the rest of code (i.e. it
would be visible in the diff).
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Acked-by: Julien Grall <jgrall@amazon.com>
> ---
> Changes from -
>
> v1 - Removed the duplicate declaration for DECLARE_OFFSETS.
>
> v2 - 1. Reworded the commit message.
> 2. Use CONFIG_ARM_PA_32 to restrict zeroeth_table_offset.
>
> 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..998edeed6e 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_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 b99806af99..44942c6a4f 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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 9/9] xen/arm: p2m: Enable support for 32bit IPA
2023-02-08 12:05 ` [XEN v3 9/9] xen/arm: p2m: Enable support for 32bit IPA Ayan Kumar Halder
@ 2023-02-11 10:21 ` Julien Grall
0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2023-02-11 10:21 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefano.stabellini, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh
Hi Ayan,
On 08/02/2023 12:05, Ayan Kumar Halder wrote:
> VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
> "Virtualization Translation Control Register" for the bit descriptions.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
This will need a respin based on the discussion we had in v2. The main
things are:
- 32-bit IPA will be using one page at the root (not 2 like for
40-bit IPA)
- the code should be consolidate with arm64 as the logic is mainly
the same.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t
2023-02-11 9:10 ` Julien Grall
@ 2023-02-13 12:47 ` Ayan Kumar Halder
0 siblings, 0 replies; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-13 12:47 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini, Ayan Kumar Halder
Cc: xen-devel, stefano.stabellini, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh
On 11/02/2023 09:10, Julien Grall wrote:
> Hi,
Hi Julien/Stefano,
>
> On 11/02/2023 00:20, Stefano Stabellini wrote:
>> On Wed, 8 Feb 2023, Ayan Kumar Halder wrote:
>>> dt_device_get_address() can accept u64 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 u64.
>>>
>>> The reason for introducing doing this is that in future 'paddr_t' may
>>> be defined as u32. 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.
>>>
>>> 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 | 36
>>> ++++++++++++++++++++--
>>> 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, 105 insertions(+), 73 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 4d7e67560f..c7e88f5011 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -1692,13 +1692,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);
>>
>> One thing to be careful here is that "start" and "end" in
>> find_memory_holes are defined now as paddr_t and passed to
>> rangeset_add_range and rangeset_remove_range.
>
> I am a bit puzzled why you are saying "now". Without Ayan's patch addr
> was 64-bit so...
>
>>
>> rangeset_add_range and rangeset_remove_range take an unsigned long as
>> parameter, so in an arm32 configuration with paddr_t set to 64-bit it
>> would result in truncation
>
> ... the problem you are talking about would already exist.
>
> I would consider to store a frame number rather than the full address
> because we can only deal with page-aligned region.
>
> Stefano (or Ayan) can you look at it?
I have sent out a patch to address this "[XEN v6 2/2] xen/arm:
domain_build: Use pfn start and end address for rangeset_{xxx}_range()".
- Ayan
>
> Cheers,
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing
2023-02-11 10:01 ` Julien Grall
@ 2023-02-27 15:21 ` Ayan Kumar Halder
0 siblings, 0 replies; 31+ messages in thread
From: Ayan Kumar Halder @ 2023-02-27 15:21 UTC (permalink / raw)
To: Julien Grall, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefano.stabellini, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
rahul.singh
On 11/02/2023 10:01, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
I need some clarification.
>
> On 08/02/2023 12:05, Ayan Kumar Halder wrote:
>> Some Arm based hardware platforms which does not support LPAE
>> (eg Cortex-R52), uses 32 bit physical addresses.
>> Also, users may choose to use 32 bits to represent physical addresses
>> for optimization.
>>
>> To support the above use cases, we have introduced arch independent
>> configs to choose if the physical address can be represented using
>> 32 bits (PHYS_ADDR_32) or 64 bits (PHYS_ADDR_64).
>> For now only ARM_32 provides support to enable 32 bit physical
>> addressing.
>>
>> 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'.
>>
>> (Jan,Julien please let me know if I understood your suggestion about
>> Kconfig correctly).
>>
>> xen/arch/Kconfig | 12 +++++++++++
>> xen/arch/arm/Kconfig | 31 ++++++++++++++++++++++++++++
>> xen/arch/arm/include/asm/page-bits.h | 2 ++
>> xen/arch/arm/include/asm/types.h | 6 ++++++
>> 4 files changed, 51 insertions(+)
>>
>> diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
>> index 7028f7b74f..1eff312b51 100644
>> --- a/xen/arch/Kconfig
>> +++ b/xen/arch/Kconfig
>> @@ -1,6 +1,18 @@
>> config 64BIT
>> bool
>> +config PHYS_ADDR_32
>> + bool
>> + help
>> + Select this option if the physical addresses can be
>> represented by
>> + 32 bits.
>> +
>> +config PHYS_ADDR_64
>> + bool
>> + help
>> + Select this option if the physical addresses can be represented
>> + 64 bits.
>> +
> So you likely don't want to allow the user to select them directly
> (IOW remove the help section). However, I don't see any code using it.
> Did you actually intended to use PHYS_ADDR_{32, 64} in the code?
>
>> 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..0955891e86 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -18,6 +18,37 @@ config ARM
>> select HAS_PDX
>> select HAS_PMAP
>> select IOMMU_FORCE_PT_SHARE
>> + choice
>
> I think this is a bit odd that this choice is part of CONFIG_ARM. It
> would be better it is separate. You can do that by removing one
> indentation.
>
>> + bool "Representative width for any physical address (default
>> 64bit)"
>> + optional
>> + ---help---
>> + You may want to specify the width to represent the physical
>> + address space.
>> + By default, the physical addresses are represented using
>> + 64 bits.
>> +
>> + However in certain platforms, the physical addresses may be
>> + represented using 32 bits.
>> + Also, the user may decide that the physical addresses can be
>> + represented using 32 bits for a given SoC. In those cases,
>> + user may want to enable 32 bit physical address for
>> + optimization.
>> + For now, we have enabled this choice for ARM_32 only.
>> +
>> + config ARM_PA_64
>> + select PHYS_ADDR_64
>> + bool "Select 64 bits to represent physical address"
>> + ---help---
>> + Use 64 bits to represent physical address.
>> +
>> + config ARM_PA_32
>> + select PHYS_ADDR_32
>> + depends on ARM_32
>> + bool "Select 32 bits to represent physical address"
>> + ---help---
>> + Use 32 bits to represent physical address.
>
> As I wrote in v2, I think this is a bit odd to ask the user what would
> be the width of paddr_t. It is also not scalable if we decide in the
> future to define different PADDR_BITS (i.e. 48, 40, 36, 32).
>
> So it would be better to allow the user to define PADDR_BITS that can
> then be translated by Xen to which ever paddr_t is suitable.
>
> Something like:
>
> choice
> prompt: "Physical address space size" if ARM_32
> default ARM_PA_48 if ARM_64
> default AMR_PA_40 if ARM_32
> help
> ...
>
> config ARM_PA_BITS_32
> bool "32-bit"
> help
> XXX Add help here to explain the benefits of using 32-bit.
>
> 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
>
> With this approach, there would be no structural change in the Kconfig
> if we wanted to support 32/40-bit on Arm64.
>
>> +
>> + endchoice
>> config ARCH_DEFCONFIG
>> string
>> diff --git a/xen/arch/arm/include/asm/page-bits.h
>> b/xen/arch/arm/include/asm/page-bits.h
>> index 5d6477e599..8f4dcebcfd 100644
>> --- a/xen/arch/arm/include/asm/page-bits.h
>> +++ b/xen/arch/arm/include/asm/page-bits.h
>> @@ -5,6 +5,8 @@
>> #ifdef CONFIG_ARM_64
>> #define PADDR_BITS 48
>> +#elif CONFIG_ARM_PA_32
>> +#define PADDR_BITS 32
>> #else
>> #define PADDR_BITS 40
>> #endif
>
> With what I suggested above. This would be replaced with:
>
> #define PADDR_BITS CONFIG_PADDR_BITS
>
>> diff --git a/xen/arch/arm/include/asm/types.h
>> b/xen/arch/arm/include/asm/types.h
>> index e218ed77bd..26144bc87e 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_ARM_PA_32)
>
> And this could be replaced with
> #ifdef CONFIG_PHY_ADDR_T_32
>
> I would also consider to add the following in mm.c
>
> BUILD_BUG_ON((sizeof(paddr_t) * 8) < PADDR_BITS);
>
> This is to make sure that the PADDR_BITS will always fit in paddr_t.
>
>> +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)
>
> Cheers,
I have followed your approach with some modifications. Let me know if it
makes sense.
Subject: [PATCH] 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_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 is same as the current configuration used today on Xen.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
xen/arch/Kconfig | 6 ++++
xen/arch/arm/Kconfig | 41 ++++++++++++++++++++++++++--
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, 53 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..b23ee56376 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -2,6 +2,7 @@ config ARM_32
def_bool y
depends on "$(ARCH)" = "arm32"
select ARCH_MAP_DOMAIN_PAGE
+ select PHYS_ADDR_T_64
config ARM_64
def_bool y
@@ -9,6 +10,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 +21,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 present 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 || PHYS_ADDR_T_32
+ default 40 if ARM_PA_BITS_40 || (PHYS_ADDR_T_64 && ARM_32)
+ default 48 if ARM_PA_BITS_48 || (PHYS_ADDR_T_64 && ARM_64)
/*
* The reason for this '||' condition is that ARM_PA_BITS_32,
ARM_PA_BITS_40 or ARM_PA_BITS_48 aren't visible for ARM_64.
* We don't want PADDR_BITS to be undefined in ARM_64. Thus, we have
added the || condition.
* We could also remove ARM_PA_BITS_32, ARM_PA_BITS_40, ARM_PA_BITS_48
(from the || ), but have kept for documentation sake.
* Let me know if you want it to be removed.
*/
+
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..56d27af162 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(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 )
--
69,1 68%
- Ayan
^ permalink raw reply related [flat|nested] 31+ messages in thread
end of thread, other threads:[~2023-02-27 15:22 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-08 12:05 [XEN v3 0/9] Add support for 32 bit physical address Ayan Kumar Halder
2023-02-08 12:05 ` [XEN v3 1/9] xen/ns16550: Remove unneeded truncation check in the DT init code Ayan Kumar Halder
2023-02-11 8:55 ` Julien Grall
2023-02-08 12:05 ` [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t Ayan Kumar Halder
2023-02-08 13:33 ` Jan Beulich
2023-02-08 15:51 ` Ayan Kumar Halder
2023-02-09 7:38 ` Jan Beulich
2023-02-11 0:08 ` Stefano Stabellini
2023-02-08 12:05 ` [XEN v3 3/9] xen/drivers: ns16550: Use paddr_t for io_base/io_size Ayan Kumar Halder
2023-02-08 13:52 ` Jan Beulich
2023-02-08 17:07 ` Ayan Kumar Halder
2023-02-09 7:48 ` Jan Beulich
2023-02-08 12:05 ` [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t Ayan Kumar Halder
2023-02-11 0:14 ` Stefano Stabellini
2023-02-11 0:20 ` Stefano Stabellini
2023-02-11 9:10 ` Julien Grall
2023-02-13 12:47 ` Ayan Kumar Halder
2023-02-11 9:25 ` Julien Grall
2023-02-08 12:05 ` [XEN v3 5/9] xen/arm: smmu: Use writeq_relaxed_non_atomic() for writing to SMMU_CBn_TTBR0 Ayan Kumar Halder
2023-02-08 12:05 ` [XEN v3 6/9] xen/arm: Introduce choice to enable 64/32 bit physical addressing Ayan Kumar Halder
2023-02-11 0:34 ` Stefano Stabellini
2023-02-11 10:07 ` Julien Grall
2023-02-11 10:01 ` Julien Grall
2023-02-27 15:21 ` Ayan Kumar Halder
2023-02-08 12:05 ` [XEN v3 7/9] xen/arm: guest_walk: LPAE specific bits should be enclosed within "ifndef CONFIG_ARM_PA_32" Ayan Kumar Halder
2023-02-11 0:35 ` Stefano Stabellini
2023-02-08 12:05 ` [XEN v3 8/9] xen/arm: Restrict zeroeth_table_offset for ARM_64 Ayan Kumar Halder
2023-02-11 0:38 ` Stefano Stabellini
2023-02-11 10:18 ` Julien Grall
2023-02-08 12:05 ` [XEN v3 9/9] xen/arm: p2m: Enable support for 32bit IPA Ayan Kumar Halder
2023-02-11 10:21 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.