Hi Jan,
On 21/03/2023 14:22, Jan Beulich wrote:
On 21.03.2023 15:03, Ayan Kumar Halder wrote:
--- a/xen/arch/Kconfig
+++ b/xen/arch/Kconfig
@@ -1,6 +1,12 @@
config 64BIT
bool
+config PHYS_ADDR_T_32
+ bool
+
+config PHYS_ADDR_T_64
+ bool
Do we really need both?
I was thinking the same. I am assuming that in future we may have
PHYS_ADDR_T_16, PHYS_ADDR_T_128. So, I am hoping that defining them explicitly
might help.
Also, the user cannot select these configs directly.
However, I am open to defining only one of them if it makes sense.
If so, what guards against both being selected
at the same time?
At present, we rely on "select".
Them being put in common code I consider it an at least latent issue
that you add "select"s ...
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -9,6 +9,7 @@ config ARM_64
select 64BIT
select ARM_EFI
select HAS_FAST_MULTIPLY
+ select PHYS_ADDR_T_64
config ARM
def_bool y
@@ -19,13 +20,48 @@ config ARM
select HAS_PMAP
select IOMMU_FORCE_PT_SHARE
+menu "Architecture Features"
+
+choice
+ prompt "Physical address space size" if ARM_32
+ default ARM_PA_BITS_48 if ARM_64
+ default ARM_PA_BITS_40 if ARM_32
+ help
+ User can choose to represent the width of physical address. This can
+ sometimes help in optimizing the size of image when user chooses a
+ smaller size to represent physical address.
+
+config ARM_PA_BITS_32
+ bool "32-bit"
+ help
+ On platforms where any physical address can be represented within 32
bits
+ , user should choose this option. This will help is reduced size of
the
+ binary.
+ select PHYS_ADDR_T_32
+ depends on ARM_32
+
+config ARM_PA_BITS_40
+ bool "40-bit"
+ select PHYS_ADDR_T_64
+ depends on ARM_32
+
+config ARM_PA_BITS_48
+ bool "40-bit"
+ select PHYS_ADDR_T_64
+ depends on ARM_48
+endchoice
... only for Arm. You get away with this only because ...
--- a/xen/arch/arm/include/asm/types.h
+++ b/xen/arch/arm/include/asm/types.h
@@ -34,9 +34,15 @@ typedef signed long long s64;
typedef unsigned long long u64;
typedef u32 vaddr_t;
#define PRIvaddr PRIx32in
+#if defined(CONFIG_PHYS_ADDR_T_32)
+typedef unsigned long paddr_t;
+#define INVALID_PADDR (~0UL)
+#define PRIpaddr "08lx"
+#else
typedef u64 paddr_t;
#define INVALID_PADDR (~0ULL)
#define PRIpaddr "016llx"
+#endif
typedef u32 register_t;
#define PRIregister "08x"
#elif defined (CONFIG_ARM_64)
... you tweak things here, when we're in the process of moving stuff
out of asm/types.h.
Are you suggesting not to add anything to asm/types.h ? IOW, the above
snippet should
be added to xen/include/xen/types.h.
(Using "unsigned long" for a 32-bit paddr_t is of
course suspicious as well - this ought to be uint32_t.)
The problem with using uint32_t for paddr_t is that there are instances
where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.
For eg , handle_passthrough_prop()
printk(XENLOG_ERR "Unable to permit to dom%d access to"
" 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
kinfo->d->domain_id,
mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
And in xen/include/xen/page-size.h,
#define PAGE_SIZE (_AC(1,L) << PAGE_SHIFT)
#define PAGE_MASK (~(PAGE_SIZE-1))
Thus, the resulting types are unsigned long. This cannot be printed
using %u for PRIpaddr.
I remember some discussion (or comment) that the physical addresses
should be represented using 'unsigned long'.
- Ayan
Jan
