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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Jan, Julien,

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


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

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

    Some Arm based hardware platforms which does not support LPAE
    (eg Cortex-R52), uses 32 bit physical addresses.
    Also, users may choose to use 32 bits to represent physical addresses
    for optimization.

    To support the above use cases, we have introduced arch independent
    configs to choose if the physical address can be represented using
    32 bits (PHYS_ADDR_T_32) or 64 bits (!PHYS_ADDR_T_32).
    For now only ARM_32 provides support to enable 32 bit physical
    addressing.

    When PHYS_ADDR_T_32 is defined, PADDR_BITS is set to 32.
    When PHYS_ADDR_T_32 is not defined for ARM_32, PADDR_BITS is set to 40.
    When PHYS_ADDR_T_32 is not defined for ARM_64, PADDR_BITS is set to 48.
    The last two are same as the current configuration used today on Xen.

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

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

diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
index 7028f7b74f..67ba38f32f 100644
--- a/xen/arch/Kconfig
+++ b/xen/arch/Kconfig
@@ -1,6 +1,9 @@
 config 64BIT
    bool

+config PHYS_ADDR_T_32
+   bool
+
 config NR_CPUS
    int "Maximum number of CPUs"
    range 1 4095
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 239d3aed3c..e6dadeb8b1 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -19,13 +19,46 @@ config ARM
    select HAS_PMAP
    select IOMMU_FORCE_PT_SHARE

+menu "Architecture Features"
+
+choice
+   prompt "Physical address space size" if ARM_32
+   default ARM_PA_BITS_48 if ARM_64
+   default ARM_PA_BITS_40 if ARM_32
+   help
+     User can choose to represent the width of physical address. This can
+     sometimes help in optimizing the size of image when user chooses a
+     smaller size to represent physical address.
+
+config ARM_PA_BITS_32
+   bool "32-bit"
+   help
+     On platforms where any physical address can be represented within 
32 bits
+     , user should choose this option. This will help is reduced size 
of the
+     binary.
+   select PHYS_ADDR_T_32
+   depends on ARM_32
+
+config ARM_PA_BITS_40
+   bool "40-bit"
+   depends on ARM_32
+
+config ARM_PA_BITS_48
+   bool "40-bit"
+   depends on ARM_48
+endchoice
+
+config PADDR_BITS
+   int
+   default 32 if ARM_PA_BITS_32
+   default 40 if ARM_PA_BITS_40
+   default 48 if ARM_PA_BITS_48 || ARM_64
+
 config ARCH_DEFCONFIG
    string
    default "arch/arm/configs/arm32_defconfig" if ARM_32
    default "arch/arm/configs/arm64_defconfig" if ARM_64

-menu "Architecture Features"
-
 source "arch/Kconfig"

 config ACPI
diff --git a/xen/arch/arm/include/asm/page-bits.h 
b/xen/arch/arm/include/asm/page-bits.h
index 5d6477e599..deb381ceeb 100644
--- a/xen/arch/arm/include/asm/page-bits.h
+++ b/xen/arch/arm/include/asm/page-bits.h
@@ -3,10 +3,6 @@

 #define PAGE_SHIFT              12

-#ifdef CONFIG_ARM_64
-#define PADDR_BITS              48
-#else
-#define PADDR_BITS              40
-#endif
+#define PADDR_BITS              CONFIG_PADDR_BITS

 #endif /* __ARM_PAGE_SHIFT_H__ */
diff --git a/xen/arch/arm/include/asm/types.h 
b/xen/arch/arm/include/asm/types.h
index e218ed77bd..e3cfbbb060 100644
--- a/xen/arch/arm/include/asm/types.h
+++ b/xen/arch/arm/include/asm/types.h
@@ -34,9 +34,15 @@ typedef signed long long s64;
 typedef unsigned long long u64;
 typedef u32 vaddr_t;
 #define PRIvaddr PRIx32
+#if defined(CONFIG_PHYS_ADDR_T_32)
+typedef unsigned long paddr_t;
+#define INVALID_PADDR (~0UL)
+#define PRIpaddr "08lx"
+#else
 typedef u64 paddr_t;
 #define INVALID_PADDR (~0ULL)
 #define PRIpaddr "016llx"
+#endif
 typedef u32 register_t;
 #define PRIregister "08x"
 #elif defined (CONFIG_ARM_64)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b99806af99..6dc37be97e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -690,6 +690,11 @@ void __init setup_frametable_mappings(paddr_t ps, 
paddr_t pe)
     const unsigned long mapping_size = frametable_size < MB(32) ? 
MB(2) : MB(32);
     int rc;

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

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

+  #endif

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

     if ( frametable_size > FRAMETABLE_SIZE )

- Ayan