Hi Jan,
On 21/03/2023 16:53, Jan Beulich wrote:
On 21.03.2023 17:15, Ayan Kumar Halder wrote:
On 21/03/2023 14:22, Jan Beulich wrote:
On 21.03.2023 15:03, Ayan Kumar Halder wrote:
--- a/xen/arch/Kconfig
+++ b/xen/arch/Kconfig
@@ -1,6 +1,12 @@
config 64BIT
bool
+config PHYS_ADDR_T_32
+ bool
+
+config PHYS_ADDR_T_64
+ bool
Do we really need both?
I was thinking the same. I am assuming that in future we may have
PHYS_ADDR_T_16,
Really? What contemporary system would be able to run in just 64k?
Certainly not Xen, let alone any Dom0 on top of it.
PHYS_ADDR_T_128. So, I am hoping that defining them explicitly might help.
Yes, 128-bit addresses may appear at some point. Then (and only then)
we'll need two controls, and we can then think about how to represent
them properly without risking issues.
Also, the user cannot select these configs directly.
Sure, but at some point some strange combination of options might that
randconfig manages to construct.
If so, what guards against both being selected
at the same time?
At present, we rely on "select".
You mean 'we rely on there being only one "select"'?
Yes, that was what I meant.
Else I'm afraid I
don't understand your reply.
Them being put in common code I consider it an at least latent issue
that you add "select"s ...
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -9,6 +9,7 @@ config ARM_64
select 64BIT
select ARM_EFI
select HAS_FAST_MULTIPLY
+ select PHYS_ADDR_T_64
config ARM
def_bool y
@@ -19,13 +20,48 @@ config ARM
select HAS_PMAP
select IOMMU_FORCE_PT_SHARE
+menu "Architecture Features"
+
+choice
+ prompt "Physical address space size" if ARM_32
+ default ARM_PA_BITS_48 if ARM_64
+ default ARM_PA_BITS_40 if ARM_32
+ help
+ User can choose to represent the width of physical address. This can
+ sometimes help in optimizing the size of image when user chooses a
+ smaller size to represent physical address.
+
+config ARM_PA_BITS_32
+ bool "32-bit"
+ help
+ On platforms where any physical address can be represented within 32
bits
+ , user should choose this option. This will help is reduced size of
the
+ binary.
+ select PHYS_ADDR_T_32
+ depends on ARM_32
+
+config ARM_PA_BITS_40
+ bool "40-bit"
+ select PHYS_ADDR_T_64
+ depends on ARM_32
+
+config ARM_PA_BITS_48
+ bool "40-bit"
+ select PHYS_ADDR_T_64
+ depends on ARM_48
+endchoice
... only for Arm. You get away with this only because ...
--- a/xen/arch/arm/include/asm/types.h
+++ b/xen/arch/arm/include/asm/types.h
@@ -34,9 +34,15 @@ typedef signed long long s64;
typedef unsigned long long u64;
typedef u32 vaddr_t;
#define PRIvaddr PRIx32in
+#if defined(CONFIG_PHYS_ADDR_T_32)
+typedef unsigned long paddr_t;
+#define INVALID_PADDR (~0UL)
+#define PRIpaddr "08lx"
+#else
typedef u64 paddr_t;
#define INVALID_PADDR (~0ULL)
#define PRIpaddr "016llx"
+#endif
typedef u32 register_t;
#define PRIregister "08x"
#elif defined (CONFIG_ARM_64)
... you tweak things here, when we're in the process of moving stuff
out of asm/types.h.
Are you suggesting not to add anything to asm/types.h ? IOW, the above
snippet should
be added to xen/include/xen/types.h.
It's not your snippet alone, but the definition of paddr_t in general
which should imo be moved (as a follow-on to "common: move standard C
fixed width type declarations to common header"). If your patch in its
present shape landed first, that movement would become more
complicated - first and foremost "select"ing the appropriate
PHYS_ADDR_T_* on x86 and RISC-V would then need to be done there, when
it really doesn't belong there.
I understand your point. I am assuming that as PHYS_ADDR_T_* is now
introduced at xen/arch/Kconfig, so x86 or RISC-V should be able to
select the option.
As I see today, for :-
RISCV defines PADDR_BITS to 56. Thus, it will select PHYS_ADDR_T_64 only.
X86 defines PADDR_BITS to 52. Thus, it will also select PHYS_ADDR_T_64 only.
For Arm, there will be at least two configurations 1. which selects
PHYS_ADDR_T_64 2. not select PHYS_ADDR_T_64 (ie for 32 bit physical
address).
(Using "unsigned long" for a 32-bit paddr_t is of
course suspicious as well - this ought to be uint32_t.)
The problem with using uint32_t for paddr_t is that there are instances
where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN.
For eg , handle_passthrough_prop()
printk(XENLOG_ERR "Unable to permit to dom%d access to"
" 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
kinfo->d->domain_id,
mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
And in xen/include/xen/page-size.h,
#define PAGE_SIZE (_AC(1,L) << PAGE_SHIFT)
#define PAGE_MASK (~(PAGE_SIZE-1))
Thus, the resulting types are unsigned long. This cannot be printed
using %u for PRIpaddr.
Is there anything wrong with making PAGE_SIZE expand to (1 << PAGE_SHIFT)
when physical addresses are only 32 bits wide?
I don't have a strong objection except that this is similar to what
linux is doing today.
https://elixir.bootlin.com/linux/latest/source/arch/arm/include/asm/page.h#L12
I remember some discussion (or comment) that the physical addresses
should be represented using 'unsigned long'.
A reference would be helpful.
https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00305.html
- Ayan
Jan
