On 29/11/2022 14:57, Ayan Kumar Halder wrote:
Hi All,
Hi,
I am trying to gather opinions on how to support 32 bit physical
addresses to enable Xen running on R52.
Refer Cortex R52 TRM, Section 2.2.12 "Memory Model"
"...This is because the physical address is always the same as the
virtual address...The virtual and physical address can be treated as
synonyms for Cortex-R52."
Thus, I understand that R52 supports 32 bit physical address only. This
is a bit different from Armv7 systems which supports Large Physical
Address Extension (LPAE) ie 40 bit physical addresses. >
Please correct me if I misunderstand something. >
So currently, Xen supports 64 bit physical address for Arm_32 (ie Armv7)
based system.
Xen supports *up to* 64-bit physical address. This may be lower in the
HW (not all the Armv7 HW supports 40-bit address).
My aim is to enable support for 32 bit physical address.
Technically this is already supported because this is a subset of
64-bit. I can see a use case (even on non R* HW) where you may want to
use 32-bit paddr_t to reduce the code size (and registers used).
But I think that's more an optimization that rather than been necessary.
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 6014c0f852..4f8b5fc4be 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -56,10 +56,10 @@ static bool __init device_tree_node_compatible(const
void *fdt, int node,
}
void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
- u32 size_cells, u64 *start, u64 *size)
+ u32 size_cells, paddr_t *start, paddr_t
*size)
This needs to stay uint64_t because the Device-Tree may contain 64-bit
values and you...
{
- *start = dt_next_cell(address_cells, cell);
- *size = dt_next_cell(size_cells, cell);
+ *start = (paddr_t) dt_next_cell(address_cells, cell);
+ *size = (paddr_t) dt_next_cell(size_cells, cell);
... don't want to always blindly cast it. That's up to the caller to
check that the top 32-bit are zeroed and downcast it.
}
static int __init device_tree_get_meminfo(const void *fdt, int node,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bd30d3798c..3cbcf8f854 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1314,7 +1314,7 @@ static int __init make_memory_node(const struct
domain *d,
dt_dprintk("Create memory node\n");
/* ePAPR 3.4 */
- snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
+ snprintf(buf, sizeof(buf), "memory@%"PRIpaddr, mem->bank[i].start);
res = fdt_begin_node(fdt, buf);
if ( res )
return res;
@@ -1665,7 +1665,7 @@ 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);
@@ -2444,7 +2444,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:
@@ -2718,7 +2718,7 @@ static int __init make_gicv2_domU_node(struct
kernel_info *kinfo)
/* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
char buf[38];
- snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
+ snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIpaddr,
vgic_dist_base(&d->arch.vgic));
res = fdt_begin_node(fdt, buf);
if ( res )
@@ -2774,7 +2774,7 @@ static int __init make_gicv3_domU_node(struct
kernel_info *kinfo)
char buf[38];
unsigned int i, len = 0;
- snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
+ snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIpaddr,
vgic_dist_base(&d->arch.vgic));
res = fdt_begin_node(fdt, buf);
@@ -2860,7 +2860,7 @@ static int __init make_vpl011_uart_node(struct
kernel_info *kinfo)
/* Placeholder for sbsa-uart@ + a 64-bit number + \0 */
char buf[27];
- snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64,
d->arch.vpl011.base_addr);
+ snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIpaddr,
d->arch.vpl011.base_addr);
res = fdt_begin_node(fdt, buf);
if ( res )
return res;
@@ -2940,9 +2940,9 @@ 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",
What's wrong with printing using PRIx64? At least...
kinfo->d->domain_id,
- mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
+ (paddr_t) (mstart & PAGE_MASK), (paddr_t)
(PAGE_ALIGN(mstart + size) - 1));
... this would avoid adding explicit cast which I quite dislike.
return res;
}
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index ae5bd8e95f..839623c32e 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1058,7 +1058,7 @@ static void __init gicv2_dt_init(void)
if ( csize < SZ_8K )
{
printk(XENLOG_WARNING "GICv2: WARNING: "
- "The GICC size is too small: %#"PRIx64" expected %#x\n",
+ "The GICC size is too small: %#"PRIpaddr" expected %#x\n",
csize, SZ_8K);
if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
{
@@ -1289,11 +1289,11 @@ static int __init gicv2_init(void)
gicv2.map_cbase += aliased_offset;
printk(XENLOG_WARNING
- "GICv2: Adjusting CPU interface base to %#"PRIx64"\n",
+ "GICv2: Adjusting CPU interface base to %#"PRIpaddr"\n",
cbase + aliased_offset);
} else if ( csize == SZ_128K )
printk(XENLOG_WARNING
- "GICv2: GICC size=%#"PRIx64" but not aliased\n",
+ "GICv2: GICC size=%#"PRIpaddr" but not aliased\n",
csize);
gicv2.map_hbase = ioremap_nocache(hbase, PAGE_SIZE);
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 3c5b88148c..322ed15e6c 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1402,7 +1402,7 @@ 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);
if ( res )
diff --git a/xen/arch/arm/include/asm/setup.h
b/xen/arch/arm/include/asm/setup.h
index fdbf68aadc..ddffffe44c 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/include/asm/types.h
b/xen/arch/arm/include/asm/types.h
index 083acbd151..a7466d65c2 100644
--- a/xen/arch/arm/include/asm/types.h
+++ b/xen/arch/arm/include/asm/types.h
@@ -37,9 +37,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 u32 paddr_t;
+#define PRIpaddr PRIx32
+#define INVALID_PADDR (~0UL)
+#else
typedef u64 paddr_t;
#define PRIpaddr 016llx
#define INVALID_PADDR (~0ULL)
+#endif
typedef u32 register_t;
#define PRIregister "08x"
#elif defined (CONFIG_ARM_64)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 6c9712ab7b..0c50b196b5 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -747,7 +747,7 @@ static const struct dt_bus *dt_match_bus(const
struct dt_device_node *np)
}
static const __be32 *dt_get_address(const struct dt_device_node *dev,
- unsigned int index, u64 *size,
+ unsigned int index, paddr_t *size,
Same as for the other dt helper.
unsigned int *flags)
{
const __be32 *prop;
@@ -781,7 +781,7 @@ static const __be32 *dt_get_address(const struct
dt_device_node *dev,
if ( i == index )
{
if ( size )
- *size = dt_read_number(prop + na, ns);
+ *size = (paddr_t) dt_read_number(prop + na, ns);
I strongly dislike adding unnecessary cast in C because they could hide
away issue. In this case, this raise the question why always ignoring
the top 32-bit is always fine?
This remark is also valid for all the other changes in device_tree.c.
if ( flags )
*flags = bus->get_flags(prop);
return prop;
@@ -935,7 +935,7 @@ 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)
+ paddr_t *addr, paddr_t *size)
{
const __be32 *addrp;
unsigned int flags;
@@ -947,7 +947,7 @@ int dt_device_get_address(const struct
dt_device_node *dev, unsigned int index,
if ( !addr )
return -EINVAL;
- *addr = __dt_translate_address(dev, addrp, "ranges");
+ *addr = (paddr_t) __dt_translate_address(dev, addrp, "ranges");
if ( *addr == DT_BAD_ADDR )
return -EINVAL;
diff --git a/xen/common/libfdt/fdt_ro.c b/xen/common/libfdt/fdt_ro.c
index 17584da257..a1e0e154c5 100644
--- a/xen/common/libfdt/fdt_ro.c
+++ b/xen/common/libfdt/fdt_ro.c
Please don't change fdt_ro.c. This is a copy of libfdt and should stay
as is.
@@ -172,7 +172,7 @@ static const struct fdt_reserve_entry
*fdt_mem_rsv(const void *fdt, int n)
return fdt_mem_rsv_(fdt, n);
}
-int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t
*size)
+int fdt_get_mem_rsv(const void *fdt, int n, paddr_t *address, paddr_t
*size)
{
const struct fdt_reserve_entry *re;
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index be67242bc0..1f86443136 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -258,7 +258,7 @@ 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, "") )
{
diff --git a/xen/drivers/passthrough/arm/smmu.c
b/xen/drivers/passthrough/arm/smmu.c
index 0a514821b3..59b9a24099 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;
};
@@ -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);
}
@@ -1179,10 +1179,12 @@ static void arm_smmu_init_context_bank(struct
arm_smmu_domain *smmu_domain)
reg = (p2maddr & ((1ULL << 32) - 1));
writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
+#if !CONFIG_ARM_PA_32
reg = (p2maddr >> 32);
if (stage1)
reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
+#endif
/*
* TTBCR
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 1528ced509..20b4462fdd 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -466,8 +466,8 @@ typedef uint64_t xen_callback_t;
/* Largest amount of actual RAM, not including holes */
#define GUEST_RAM_MAX (GUEST_RAM0_SIZE + GUEST_RAM1_SIZE)
/* Suitable for e.g. const uint64_t ramfoo[] = GUEST_RAM_BANK_FOOS; */
+#if !CONFIG_ARM_PA_32
CONFIG_ARM_PA_32 is not going to be defined for the tools and I don't
think we should define it.
If there are any restriction on which bank to use, then this should be
done in the toolstack.
#define GUEST_RAM_BANK_BASES { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
#define GUEST_RAM_BANK_SIZES { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
+#else
+#define GUEST_RAM_BANK_BASES { GUEST_RAM0_BASE }
+#define GUEST_RAM_BANK_SIZES { GUEST_RAM0_SIZE }
+#endif
/* Current supported guest VCPUs */
#define GUEST_MAX_VCPUS 128
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index a28937d12a..7f86af54b6 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -572,7 +572,7 @@ const struct dt_device_node *dt_get_parent(const
struct dt_device_node *node);
* 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);
+ paddr_t *addr, paddr_t *size);
/**
* dt_number_of_irq - Get the number of IRQ for a device
diff --git a/xen/include/xen/libfdt/libfdt.h
b/xen/include/xen/libfdt/libfdt.h
index c71689e2be..fabde78edf 100644
--- a/xen/include/xen/libfdt/libfdt.h
+++ b/xen/include/xen/libfdt/libfdt.h
@@ -444,7 +444,7 @@ int fdt_num_mem_rsv(const void *fdt);
* -FDT_ERR_BADVERSION,
* -FDT_ERR_BADSTATE, standard meanings
*/
-int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t
*size);
+int fdt_get_mem_rsv(const void *fdt, int n, paddr_t *address, paddr_t
*size);
/**
* fdt_subnode_offset_namelen - find a subnode based on substring
3. I am happy with any other suggestion.
In linux source code
(https://elixir.bootlin.com/linux/v6.1-rc1/source/include/linux/types.h#L152), CONFIG_PHYS_ADDR_T_64BIT <https://elixir.bootlin.com/linux/v6.1-rc1/K/ident/CONFIG_PHYS_ADDR_T_64BIT> is used for distinguish 64/32 bit physical address.
Kind regards,
Ayan
--
Julien Grall
