Arm: AArch32: Need suggestions to support 32 bit physical addresses

Ayan Kumar Halder posted 1 patch 1 year, 4 months ago
Failed in applying to current master (apply log)
Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Ayan Kumar Halder 1 year, 4 months ago
Hi All,

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. My aim is to enable support for 32 bit physical address.

To achieve this, I see two options :-

1. Make the changes in the specific functions for R52.

https://gitlab.com/ayankuma/xen-integration/-/commit/5498cf74c2c8feadc32934b948ce5f72bd0809ee

2. Introduce a special macro (CONFIG_ARM_32_BIT_PA) to support 32 bit 
physical address.

Thus, our change will look as follows :-

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)
  {
-    *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);
  }

  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",
                     kinfo->d->domain_id,
-                   mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1);
+                   (paddr_t) (mstart & PAGE_MASK), (paddr_t) 
(PAGE_ALIGN(mstart + size) - 1));
              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,
                                      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);
              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
@@ -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
  #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


Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Julien Grall 1 year, 4 months ago

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

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Ayan Kumar Halder 1 year, 4 months ago
On 29/11/2022 14:52, Julien Grall wrote:
>
>
> On 29/11/2022 14:57, Ayan Kumar Halder wrote:
>> Hi All,
>
> Hi,

Hi Julien,

Many thanks for your inputs.

>
>> 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...

Are you saying that the device tree may contain 64 bit addresses even 
though the platform is 32 bit ?

I think then this approach (ie "typedef u32 paddr_t" for 32 bit system) 
is incorrect.

Then, the other option would be to downcast 64 bit physical addresses to 
32 bits, when we need to translate pa to va.

Do you think this approach looks better ? Or any better suggestions ?

     xen/Arm: AArch32_V8R: Use 32 bits for the physical address

     In the case of MPU (unlike MMU), there is a 1-1 mapping of virtual 
address
     to physical address (ie VA = PA).

     However, the physical addresses defined for aarch32 is u64. This is 
a problem
     for aarch32 mpu systems as the physical addresses = virtual 
addresses = 32 bits.

     Thus, when the memory region for FDT is mapped, it returns the 
virtual address
     (which is the same as physical address but truncated upto the lower 
32 bits).
     Similar logic has been used to convert machine address to virtual 
address and
     for ioremap as well.

     We do not support physical addresses beyond 32 bits.
     As this logic will fail when FDT physical address is more than 32 
bits, we have
     added a BUG() to catch this.

     Thus, the following functions has been adapted :-
     early_fdt_map()
     copy_from_paddr()
     maddr_to_virt()
     ioremap_attr()

     Also disable "BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG)" as 
PADDR_BITS = 40
     and BITS_PER_LONG = 32.

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

diff --git a/xen/arch/arm/include/asm/mm_mpu.h 
b/xen/arch/arm/include/asm/mm_mpu.h
index 306a4c497c..f4f5ae1488 100644
--- a/xen/arch/arm/include/asm/mm_mpu.h
+++ b/xen/arch/arm/include/asm/mm_mpu.h
@@ -89,7 +89,18 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
  static inline void *maddr_to_virt(paddr_t ma)
  {
      /* In MPU system, VA == PA. */
+#ifdef CONFIG_AARCH32_V8R
+    /*
+     * 64 bit physical addresses are not supported.
+     * Raise a bug if one encounters 64 bit address.
+     */
+    if (ma >> BITOP_BITS_PER_WORD)
+        BUG();
+
+    return (void *) ((uint32_t)(ma & GENMASK(31,0)));
+#else
      return (void *)ma;
+#endif
  }

  /*
diff --git a/xen/arch/arm/include/asm/setup.h 
b/xen/arch/arm/include/asm/setup.h
index b3330cd584..3f4ac7f475 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -119,7 +119,11 @@ extern struct bootinfo bootinfo;

  extern domid_t max_init_domid;

+#ifdef CONFIG_AARCH32_V8R
+void copy_from_paddr(void *dst, uint32_t paddr, unsigned long len);
+#else
  void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
+#endif

  size_t estimate_efi_size(unsigned int mem_nr_banks);

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 04c05d7a05..7a7386f33a 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -46,7 +46,11 @@ struct minimal_dtb_header {
   * @paddr: source physical address
   * @len: length to copy
   */
+#ifdef CONFIG_AARCH32_V8R
+void __init copy_from_paddr(void *dst, uint32_t paddr, unsigned long len)
+#else
  void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
+#endif
  {
      void *src = (void *)(unsigned long)paddr;

diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c
index df43621ee7..62774aebc6 100644
--- a/xen/arch/arm/mm_mpu.c
+++ b/xen/arch/arm/mm_mpu.c
@@ -29,7 +29,7 @@
  #include <asm/arm64/fw_shareinfo.h>
  #endif

-#ifdef CONFIG_AARCH32_ARMV8_R
+#ifdef CONFIG_AARCH32_V8R
  #include <asm/arm32/armv8r/sysregs.h>
  #endif

@@ -414,7 +414,18 @@ void *ioremap_attr(paddr_t pa, size_t len, unsigned 
int attributes)
          return NULL;
      }

+#ifdef CONFIG_AARCH32_V8R
+    /*
+     * 64 bit physical addresses are not supported.
+     * Raise a bug if one encounters 64 bit address.
+     */
+    if (pa >> BITOP_BITS_PER_WORD)
+        BUG();
+
+    return (void *) ((uint32_t)(pa & GENMASK(31,0)));
+#else
      return (void *)pa;
+#endif
  }

  static void clear_boot_mpumap(void)
@@ -1007,7 +1018,19 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
      nr_xen_mpumap++;

      /* VA == PA */
+#ifdef CONFIG_AARCH32_V8R
+
+    /*
+     * 64 bit physical addresses are not supported.
+     * Raise a bug if one encounters 64 bit address.
+     */
+    if (fdt_paddr >> BITOP_BITS_PER_WORD)
+        BUG();
+
+    fdt_virt = (void *) ((uint32_t)(fdt_paddr & GENMASK(31,0)));
+#else
      fdt_virt = (void *)fdt_paddr;
+#endif

      if ( fdt_magic(fdt_virt) != FDT_MAGIC )
          return NULL;
@@ -1165,13 +1188,13 @@ void __init setup_protection_regions()
          {
              pr_t region;
              access_protection_region(true, &region, NULL, i);
-#ifdef CONFIG_AARCH32_ARMV8_R
+#ifdef CONFIG_AARCH32_V8R
              printk("Boot-time Xen MPU memory configuration. #%u : 
0x%"PRIx32" - 0x%"PRIx32".\n",
                     i, pr_get_base(&region), pr_get_limit(&region));
-#else /* CONFIG_AARCH32_ARMV8_R */
+#else
              printk("Boot-time Xen MPU memory configuration. #%u : 
0x%"PRIx64" - 0x%"PRIx64".\n",
                     i, pr_get_base(&region), pr_get_limit(&region));
-#endif /* CONFIG_AARCH32_ARMV8_R */
+#endif
          }
  }

@@ -1262,8 +1285,13 @@ static int __init relocate_xen_mpumap(void)
      if ( !xen_mpumap )
          return -EINVAL;

+#ifdef CONFIG_AARCH32_V8R
+    copy_from_paddr(xen_mpumap, (uint32_t)(pr_t *)boot_mpumap,
+                    sizeof(pr_t) * next_xen_mpumap_index);
+#else
      copy_from_paddr(xen_mpumap, (paddr_t)(pr_t *)boot_mpumap,
                      sizeof(pr_t) * next_xen_mpumap_index);
+#endif

      clear_boot_mpumap();

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 62afb07bc6..a73bf7de01 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
  {
      ASSERT(!first_node_initialised);
      ASSERT(!xenheap_bits);
+#ifndef CONFIG_AARCH32_V8R
      BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
+#endif
      xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
      printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
  }

- Ayan


Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Julien Grall 1 year, 4 months ago

On 29/11/2022 16:23, Ayan Kumar Halder wrote:
> 
> On 29/11/2022 14:52, Julien Grall wrote:
>>
>>
>> On 29/11/2022 14:57, Ayan Kumar Halder wrote:
>>> Hi All,
>>
>> Hi,
> 
> Hi Julien,
> 
> Many thanks for your inputs.
> 
>>
>>> 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...
> 
> Are you saying that the device tree may contain 64 bit addresses even 
> though the platform is 32 bit ?

There should not be any 32-bit address but you don't know what the 
device-tree is containing because this is user input.

This is not the business of the Device-Tree parser to decide whether the 
value should be downcasted or rejected. That's the goal of the callers.

> 
> I think then this approach (ie "typedef u32 paddr_t" for 32 bit system) 
> is incorrect.
I am a bit surprised you came to this conclusion just based on the 
above. As I said before, there are benefits to allow Xen to be built 
with 32-bit (e.g. smaller code size and better use of the register).

> 
> Then, the other option would be to downcast 64 bit physical addresses to 
> 32 bits, when we need to translate pa to va.
> 
> Do you think this approach looks better ?

Some of the changes you propose are questionable (see below).

> Or any better suggestions ?

Rework you previous approach by not touching the Device-Tree code.

> diff --git a/xen/arch/arm/include/asm/mm_mpu.h 
> b/xen/arch/arm/include/asm/mm_mpu.h
> index 306a4c497c..f4f5ae1488 100644
> --- a/xen/arch/arm/include/asm/mm_mpu.h
> +++ b/xen/arch/arm/include/asm/mm_mpu.h
> @@ -89,7 +89,18 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
>   static inline void *maddr_to_virt(paddr_t ma)
>   {
>       /* In MPU system, VA == PA. */
> +#ifdef CONFIG_AARCH32_V8R
> +    /*
> +     * 64 bit physical addresses are not supported.
> +     * Raise a bug if one encounters 64 bit address.
> +     */
> +    if (ma >> BITOP_BITS_PER_WORD)
> +        BUG();
I don't particularly like the runtime check when you should be able to 
sanitize the values before hand.

> +
> +    return (void *) ((uint32_t)(ma & GENMASK(31,0)));

& GENMASK (...) is a bit pointless here given that you above confirmed 
the top 32-bit are zeroed.

> +#else
>       return (void *)ma;
> +#endif
>   }
> 
>   /*
> diff --git a/xen/arch/arm/include/asm/setup.h 
> b/xen/arch/arm/include/asm/setup.h
> index b3330cd584..3f4ac7f475 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -119,7 +119,11 @@ extern struct bootinfo bootinfo;
> 
>   extern domid_t max_init_domid;
> 
> +#ifdef CONFIG_AARCH32_V8R
> +void copy_from_paddr(void *dst, uint32_t paddr, unsigned long len);
> +#else
>   void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
> +#endif

I don't understand why the probably needs to be changed here...

> 
>   size_t estimate_efi_size(unsigned int mem_nr_banks);
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 04c05d7a05..7a7386f33a 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -46,7 +46,11 @@ struct minimal_dtb_header {
>    * @paddr: source physical address
>    * @len: length to copy
>    */
> +#ifdef CONFIG_AARCH32_V8R
> +void __init copy_from_paddr(void *dst, uint32_t paddr, unsigned long len)
> +#else
>   void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
> +#endif
>   {
>       void *src = (void *)(unsigned long)paddr;

... because the code should compile without any issue. If you were 
really concern about ignore the top 32-bit, then you could add a 
BUG_ON() (This is OK because this is init code).

> 
> diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c
> index df43621ee7..62774aebc6 100644
> --- a/xen/arch/arm/mm_mpu.c
> +++ b/xen/arch/arm/mm_mpu.c
> @@ -29,7 +29,7 @@
>   #include <asm/arm64/fw_shareinfo.h>
>   #endif
> 
> -#ifdef CONFIG_AARCH32_ARMV8_R
> +#ifdef CONFIG_AARCH32_V8R
>   #include <asm/arm32/armv8r/sysregs.h>
>   #endif
> 
> @@ -414,7 +414,18 @@ void *ioremap_attr(paddr_t pa, size_t len, unsigned 
> int attributes)
>           return NULL;
>       }
> 
> +#ifdef CONFIG_AARCH32_V8R
> +    /*
> +     * 64 bit physical addresses are not supported.
> +     * Raise a bug if one encounters 64 bit address.
> +     */
> +    if (pa >> BITOP_BITS_PER_WORD)
> +        BUG();

Why not returning NULL?

> +
> +    return (void *) ((uint32_t)(pa & GENMASK(31,0)));
> +#else
>       return (void *)pa;
> +#endif
>   }
> 
>   static void clear_boot_mpumap(void)
> @@ -1007,7 +1018,19 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>       nr_xen_mpumap++;
> 
>       /* VA == PA */
> +#ifdef CONFIG_AARCH32_V8R
> +
> +    /*
> +     * 64 bit physical addresses are not supported.
> +     * Raise a bug if one encounters 64 bit address.
> +     */
> +    if (fdt_paddr >> BITOP_BITS_PER_WORD)
> +        BUG();

Same here question here.

> +
> +    fdt_virt = (void *) ((uint32_t)(fdt_paddr & GENMASK(31,0)));
> +#else
>       fdt_virt = (void *)fdt_paddr;
> +#endif
> 
>       if ( fdt_magic(fdt_virt) != FDT_MAGIC )
>           return NULL;
> @@ -1165,13 +1188,13 @@ void __init setup_protection_regions()
>           {
>               pr_t region;
>               access_protection_region(true, &region, NULL, i);
> -#ifdef CONFIG_AARCH32_ARMV8_R
> +#ifdef CONFIG_AARCH32_V8R
>               printk("Boot-time Xen MPU memory configuration. #%u : 
> 0x%"PRIx32" - 0x%"PRIx32".\n",
>                      i, pr_get_base(&region), pr_get_limit(&region));
> -#else /* CONFIG_AARCH32_ARMV8_R */
> +#else
>               printk("Boot-time Xen MPU memory configuration. #%u : 
> 0x%"PRIx64" - 0x%"PRIx64".\n",
>                      i, pr_get_base(&region), pr_get_limit(&region));
> -#endif /* CONFIG_AARCH32_ARMV8_R */
> +#endif
>           }
>   }
> 
> @@ -1262,8 +1285,13 @@ static int __init relocate_xen_mpumap(void)
>       if ( !xen_mpumap )
>           return -EINVAL;
> 
> +#ifdef CONFIG_AARCH32_V8R
> +    copy_from_paddr(xen_mpumap, (uint32_t)(pr_t *)boot_mpumap,
> +                    sizeof(pr_t) * next_xen_mpumap_index);
> +#else
>       copy_from_paddr(xen_mpumap, (paddr_t)(pr_t *)boot_mpumap,
>                       sizeof(pr_t) * next_xen_mpumap_index);
> +#endif
> 
>       clear_boot_mpumap();
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 62afb07bc6..a73bf7de01 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
>   {
>       ASSERT(!first_node_initialised);
>       ASSERT(!xenheap_bits);
> +#ifndef CONFIG_AARCH32_V8R
>       BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
> +#endif

BUILD_BUG_ON() are used to indicate that the code would fall over the 
check pass. I can't find the justification for this change in the commit 
message.

It is also not clear why you are modifying this path because so far on 
Arm32 the xenheap and domheap are separated for good reason (i.e. lack 
of address space). Is this going to change with Armv8-R?

Cheers,

-- 
Julien Grall

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Ayan Kumar Halder 1 year, 4 months ago
On 29/11/2022 15:52, Julien Grall wrote:
>
>
> On 29/11/2022 16:23, Ayan Kumar Halder wrote:
>>
>> On 29/11/2022 14:52, Julien Grall wrote:
>>>
>>>
>>> On 29/11/2022 14:57, Ayan Kumar Halder wrote:
>>>> Hi All,
>>>
>>> Hi,
Hi Julien,
>>
>> Hi Julien,
>>
>> Many thanks for your inputs.
>>
>>>
>>>> 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...
>>
>> Are you saying that the device tree may contain 64 bit addresses even 
>> though the platform is 32 bit ?
>
> There should not be any 32-bit address but you don't know what the 
> device-tree is containing because this is user input.
>
> This is not the business of the Device-Tree parser to decide whether 
> the value should be downcasted or rejected. That's the goal of the 
> callers.
>
>>
>> I think then this approach (ie "typedef u32 paddr_t" for 32 bit 
>> system) is incorrect.
> I am a bit surprised you came to this conclusion just based on the 
> above. As I said before, there are benefits to allow Xen to be built 
> with 32-bit (e.g. smaller code size and better use of the register).
>
>>
>> Then, the other option would be to downcast 64 bit physical addresses 
>> to 32 bits, when we need to translate pa to va.
>>
>> Do you think this approach looks better ?
>
> Some of the changes you propose are questionable (see below).
>
>> Or any better suggestions ?
>
> Rework you previous approach by not touching the Device-Tree code.
>
>> diff --git a/xen/arch/arm/include/asm/mm_mpu.h 
>> b/xen/arch/arm/include/asm/mm_mpu.h
>> index 306a4c497c..f4f5ae1488 100644
>> --- a/xen/arch/arm/include/asm/mm_mpu.h
>> +++ b/xen/arch/arm/include/asm/mm_mpu.h
>> @@ -89,7 +89,18 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
>>   static inline void *maddr_to_virt(paddr_t ma)
>>   {
>>       /* In MPU system, VA == PA. */
>> +#ifdef CONFIG_AARCH32_V8R
>> +    /*
>> +     * 64 bit physical addresses are not supported.
>> +     * Raise a bug if one encounters 64 bit address.
>> +     */
>> +    if (ma >> BITOP_BITS_PER_WORD)
>> +        BUG();
> I don't particularly like the runtime check when you should be able to 
> sanitize the values before hand.

I think we can replace BUG() with ASSERT(). This is similar to what is 
being done today.

>
>> +
>> +    return (void *) ((uint32_t)(ma & GENMASK(31,0)));
>
> & GENMASK (...) is a bit pointless here given that you above confirmed 
> the top 32-bit are zeroed.
>
>> +#else
>>       return (void *)ma;
>> +#endif
>>   }
>>
>>   /*
>> diff --git a/xen/arch/arm/include/asm/setup.h 
>> b/xen/arch/arm/include/asm/setup.h
>> index b3330cd584..3f4ac7f475 100644
>> --- a/xen/arch/arm/include/asm/setup.h
>> +++ b/xen/arch/arm/include/asm/setup.h
>> @@ -119,7 +119,11 @@ extern struct bootinfo bootinfo;
>>
>>   extern domid_t max_init_domid;
>>
>> +#ifdef CONFIG_AARCH32_V8R
>> +void copy_from_paddr(void *dst, uint32_t paddr, unsigned long len);
>> +#else
>>   void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
>> +#endif
>
> I don't understand why the probably needs to be changed here...
>
>>
>>   size_t estimate_efi_size(unsigned int mem_nr_banks);
>>
>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>> index 04c05d7a05..7a7386f33a 100644
>> --- a/xen/arch/arm/kernel.c
>> +++ b/xen/arch/arm/kernel.c
>> @@ -46,7 +46,11 @@ struct minimal_dtb_header {
>>    * @paddr: source physical address
>>    * @len: length to copy
>>    */
>> +#ifdef CONFIG_AARCH32_V8R
>> +void __init copy_from_paddr(void *dst, uint32_t paddr, unsigned long 
>> len)
>> +#else
>>   void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long 
>> len)
>> +#endif
>>   {
>>       void *src = (void *)(unsigned long)paddr;
>
> ... because the code should compile without any issue. If you were 
> really concern about ignore the top 32-bit, then you could add a 
> BUG_ON() (This is OK because this is init code).
Yes, the code compiles fine without the change. I can put a BUG_ON() here.
>
>>
>> diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c
>> index df43621ee7..62774aebc6 100644
>> --- a/xen/arch/arm/mm_mpu.c
>> +++ b/xen/arch/arm/mm_mpu.c
>> @@ -29,7 +29,7 @@
>>   #include <asm/arm64/fw_shareinfo.h>
>>   #endif
>>
>> -#ifdef CONFIG_AARCH32_ARMV8_R
>> +#ifdef CONFIG_AARCH32_V8R
>>   #include <asm/arm32/armv8r/sysregs.h>
>>   #endif
>>
>> @@ -414,7 +414,18 @@ void *ioremap_attr(paddr_t pa, size_t len, 
>> unsigned int attributes)
>>           return NULL;
>>       }
>>
>> +#ifdef CONFIG_AARCH32_V8R
>> +    /*
>> +     * 64 bit physical addresses are not supported.
>> +     * Raise a bug if one encounters 64 bit address.
>> +     */
>> +    if (pa >> BITOP_BITS_PER_WORD)
>> +        BUG();
>
> Why not returning NULL?
Ack
>
>> +
>> +    return (void *) ((uint32_t)(pa & GENMASK(31,0)));
>> +#else
>>       return (void *)pa;
>> +#endif
>>   }
>>
>>   static void clear_boot_mpumap(void)
>> @@ -1007,7 +1018,19 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>>       nr_xen_mpumap++;
>>
>>       /* VA == PA */
>> +#ifdef CONFIG_AARCH32_V8R
>> +
>> +    /*
>> +     * 64 bit physical addresses are not supported.
>> +     * Raise a bug if one encounters 64 bit address.
>> +     */
>> +    if (fdt_paddr >> BITOP_BITS_PER_WORD)
>> +        BUG();
>
> Same here question here.
>
>> +
>> +    fdt_virt = (void *) ((uint32_t)(fdt_paddr & GENMASK(31,0)));
>> +#else
>>       fdt_virt = (void *)fdt_paddr;
>> +#endif
>>
>>       if ( fdt_magic(fdt_virt) != FDT_MAGIC )
>>           return NULL;
>> @@ -1165,13 +1188,13 @@ void __init setup_protection_regions()
>>           {
>>               pr_t region;
>>               access_protection_region(true, &region, NULL, i);
>> -#ifdef CONFIG_AARCH32_ARMV8_R
>> +#ifdef CONFIG_AARCH32_V8R
>>               printk("Boot-time Xen MPU memory configuration. #%u : 
>> 0x%"PRIx32" - 0x%"PRIx32".\n",
>>                      i, pr_get_base(&region), pr_get_limit(&region));
>> -#else /* CONFIG_AARCH32_ARMV8_R */
>> +#else
>>               printk("Boot-time Xen MPU memory configuration. #%u : 
>> 0x%"PRIx64" - 0x%"PRIx64".\n",
>>                      i, pr_get_base(&region), pr_get_limit(&region));
>> -#endif /* CONFIG_AARCH32_ARMV8_R */
>> +#endif
>>           }
>>   }
>>
>> @@ -1262,8 +1285,13 @@ static int __init relocate_xen_mpumap(void)
>>       if ( !xen_mpumap )
>>           return -EINVAL;
>>
>> +#ifdef CONFIG_AARCH32_V8R
>> +    copy_from_paddr(xen_mpumap, (uint32_t)(pr_t *)boot_mpumap,
>> +                    sizeof(pr_t) * next_xen_mpumap_index);
>> +#else
>>       copy_from_paddr(xen_mpumap, (paddr_t)(pr_t *)boot_mpumap,
>>                       sizeof(pr_t) * next_xen_mpumap_index);
>> +#endif
>>
>>       clear_boot_mpumap();
>>
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 62afb07bc6..a73bf7de01 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
>>   {
>>       ASSERT(!first_node_initialised);
>>       ASSERT(!xenheap_bits);
>> +#ifndef CONFIG_AARCH32_V8R
>>       BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
>> +#endif
>
> BUILD_BUG_ON() are used to indicate that the code would fall over the 
> check pass. I can't find the justification for this change in the 
> commit message.

I had a look at the following commit which introduced this, but I 
couldn't get the explaination for this.

commit 88e3ed61642bb393458acc7a9bd2f96edc337190
Author: Jan Beulich <jbeulich@suse.com>
Date:   Tue Sep 1 14:02:57 2015 +0200

@Jan :- Do you know why BUILD_BUG_ON() was introduced ?

>
> It is also not clear why you are modifying this path because so far on 
> Arm32 the xenheap and domheap are separated for good reason (i.e. lack 
> of address space). Is this going to change with Armv8-R?
>
See this comment

  * CONFIG_SEPARATE_XENHEAP=y
  *
  *   The xen heap is maintained as an entirely separate heap.
  *
  *   Arch code arranges for some (perhaps small) amount of physical
  *   memory to be covered by a direct mapping and registers that
  *   memory as the Xen heap (via init_xenheap_pages()) and the
  *   remainder as the dom heap.
  *
  *   This mode of operation is most commonly used by 32-bit arches
  *   where the virtual address space is insufficient to map all RAM.

This is not true for R52 as the memory is mapped 1-1. Thus "VA == PA".

I don't think the lack of virtual address space applies to R52. Thus, 
CONFIG_SEPARATE_XENHEAP=N

- Ayan

> Cheers,
>

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Jan Beulich 1 year, 4 months ago
On 29.11.2022 19:18, Ayan Kumar Halder wrote:
> On 29/11/2022 15:52, Julien Grall wrote:
>> On 29/11/2022 16:23, Ayan Kumar Halder wrote:
>>> On 29/11/2022 14:52, Julien Grall wrote:
>>>> On 29/11/2022 14:57, Ayan Kumar Halder wrote:
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
>>>   {
>>>       ASSERT(!first_node_initialised);
>>>       ASSERT(!xenheap_bits);
>>> +#ifndef CONFIG_AARCH32_V8R
>>>       BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
>>> +#endif
>>
>> BUILD_BUG_ON() are used to indicate that the code would fall over the 
>> check pass. I can't find the justification for this change in the 
>> commit message.
> 
> I had a look at the following commit which introduced this, but I 
> couldn't get the explaination for this.
> 
> commit 88e3ed61642bb393458acc7a9bd2f96edc337190
> Author: Jan Beulich <jbeulich@suse.com>
> Date:   Tue Sep 1 14:02:57 2015 +0200
> 
> @Jan :- Do you know why BUILD_BUG_ON() was introduced ?

You've cut too much context - the next line explains this all by itself,
I think:

    xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);

Clearly addresses used for the Xen heap need to be representable in an
unsigned long (which we assume to be the same size as void *).

But I'm afraid there's further context missing for your question: Why
would that construct be a problem in your case? Is it just that you'd
need it to be > rather than the >= that's used presently? If so, why
do you add an #ifdef rather than dealing with the (apparent) off-by-1?
(I say "apparent" because I haven't checked whether the >= is really
depended upon anywhere.)

Jan

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Andre Przywara 1 year, 4 months ago
On Wed, 30 Nov 2022 08:09:53 +0100
Jan Beulich <jbeulich@suse.com> wrote:

Hi Ayan,

> On 29.11.2022 19:18, Ayan Kumar Halder wrote:
> > On 29/11/2022 15:52, Julien Grall wrote:  
> >> On 29/11/2022 16:23, Ayan Kumar Halder wrote:  
> >>> On 29/11/2022 14:52, Julien Grall wrote:  
> >>>> On 29/11/2022 14:57, Ayan Kumar Halder wrote:  
> >>> --- a/xen/common/page_alloc.c
> >>> +++ b/xen/common/page_alloc.c
> >>> @@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
> >>>   {
> >>>       ASSERT(!first_node_initialised);
> >>>       ASSERT(!xenheap_bits);
> >>> +#ifndef CONFIG_AARCH32_V8R
> >>>       BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
> >>> +#endif  
> >>
> >> BUILD_BUG_ON() are used to indicate that the code would fall over the 
> >> check pass. I can't find the justification for this change in the 
> >> commit message.  
> > 
> > I had a look at the following commit which introduced this, but I 
> > couldn't get the explaination for this.
> > 
> > commit 88e3ed61642bb393458acc7a9bd2f96edc337190
> > Author: Jan Beulich <jbeulich@suse.com>
> > Date:   Tue Sep 1 14:02:57 2015 +0200
> > 
> > @Jan :- Do you know why BUILD_BUG_ON() was introduced ?  
> 
> You've cut too much context - the next line explains this all by itself,
> I think:
> 
>     xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
> 
> Clearly addresses used for the Xen heap need to be representable in an
> unsigned long (which we assume to be the same size as void *).

So I am wondering why you hit that code for your port in the first case?
If you check, then ARM32 won't pass this, because PADDR_BITS is 40, while
a long is still 32 bits.
So digging deeper this is code for the case when we want to map the entire
physical memory into Xen (the Xen heap, or direct map in Linux terms).
Which we cannot do for ARM32, and that's why the code is protected by
!CONFIG_SEPARATE_XENHEAP, which is forced to 1 for CONFIG_ARM_32 (in a
hacked up way, btw).

So I think you must just force the same thing for your port, then this
code will never be compiled.

Does that make sense?

Cheers,
Andre

> But I'm afraid there's further context missing for your question: Why
> would that construct be a problem in your case? Is it just that you'd
> need it to be > rather than the >= that's used presently? If so, why
> do you add an #ifdef rather than dealing with the (apparent) off-by-1?
> (I say "apparent" because I haven't checked whether the >= is really
> depended upon anywhere.)
> 
> Jan
Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Ayan Kumar Halder 1 year, 3 months ago
On 30/11/2022 13:13, Andre Przywara wrote:
> On Wed, 30 Nov 2022 08:09:53 +0100
> Jan Beulich <jbeulich@suse.com> wrote:
>
> Hi Ayan,
Hi Andre,
>
>> On 29.11.2022 19:18, Ayan Kumar Halder wrote:
>>> On 29/11/2022 15:52, Julien Grall wrote:
>>>> On 29/11/2022 16:23, Ayan Kumar Halder wrote:
>>>>> On 29/11/2022 14:52, Julien Grall wrote:
>>>>>> On 29/11/2022 14:57, Ayan Kumar Halder wrote:
>>>>> --- a/xen/common/page_alloc.c
>>>>> +++ b/xen/common/page_alloc.c
>>>>> @@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
>>>>>    {
>>>>>        ASSERT(!first_node_initialised);
>>>>>        ASSERT(!xenheap_bits);
>>>>> +#ifndef CONFIG_AARCH32_V8R
>>>>>        BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
>>>>> +#endif
>>>> BUILD_BUG_ON() are used to indicate that the code would fall over the
>>>> check pass. I can't find the justification for this change in the
>>>> commit message.
>>> I had a look at the following commit which introduced this, but I
>>> couldn't get the explaination for this.
>>>
>>> commit 88e3ed61642bb393458acc7a9bd2f96edc337190
>>> Author: Jan Beulich <jbeulich@suse.com>
>>> Date:   Tue Sep 1 14:02:57 2015 +0200
>>>
>>> @Jan :- Do you know why BUILD_BUG_ON() was introduced ?
>> You've cut too much context - the next line explains this all by itself,
>> I think:
>>
>>      xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
>>
>> Clearly addresses used for the Xen heap need to be representable in an
>> unsigned long (which we assume to be the same size as void *).
> So I am wondering why you hit that code for your port in the first case?
> If you check, then ARM32 won't pass this, because PADDR_BITS is 40, while
> a long is still 32 bits.

But, PADDR_BITS should be equal to 32 for 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."

 From this, I understood that as virtual address is 32 bits for AArch32, 
so physical address will also be 32 bits.

Please correct me if I am misunderstanding ?

If this is correct, then ...

> So digging deeper this is code for the case when we want to map the entire
> physical memory into Xen (the Xen heap, or direct map in Linux terms).
> Which we cannot do for ARM32, and that's why the code is protected by
> !CONFIG_SEPARATE_XENHEAP, which is forced to 1 for CONFIG_ARM_32 (in a
> hacked up way, btw).

we can map entire physical memory into Xen as VA == PA.

- Ayan

>
> So I think you must just force the same thing for your port, then this
> code will never be compiled.
>
> Does that make sense?
>
> Cheers,
> Andre
>
>> But I'm afraid there's further context missing for your question: Why
>> would that construct be a problem in your case? Is it just that you'd
>> need it to be > rather than the >= that's used presently? If so, why
>> do you add an #ifdef rather than dealing with the (apparent) off-by-1?
>> (I say "apparent" because I haven't checked whether the >= is really
>> depended upon anywhere.)
>>
>> Jan

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Andre Przywara 1 year, 3 months ago
On Wed, 30 Nov 2022 15:39:56 +0000
Ayan Kumar Halder <ayankuma@amd.com> wrote:

Hi Ayan,

> On 30/11/2022 13:13, Andre Przywara wrote:
> > On Wed, 30 Nov 2022 08:09:53 +0100
> > Jan Beulich <jbeulich@suse.com> wrote:
> >
> > Hi Ayan,  
> Hi Andre,
> >  
> >> On 29.11.2022 19:18, Ayan Kumar Halder wrote:  
> >>> On 29/11/2022 15:52, Julien Grall wrote:  
> >>>> On 29/11/2022 16:23, Ayan Kumar Halder wrote:  
> >>>>> On 29/11/2022 14:52, Julien Grall wrote:  
> >>>>>> On 29/11/2022 14:57, Ayan Kumar Halder wrote:  
> >>>>> --- a/xen/common/page_alloc.c
> >>>>> +++ b/xen/common/page_alloc.c
> >>>>> @@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
> >>>>>    {
> >>>>>        ASSERT(!first_node_initialised);
> >>>>>        ASSERT(!xenheap_bits);
> >>>>> +#ifndef CONFIG_AARCH32_V8R
> >>>>>        BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
> >>>>> +#endif  
> >>>> BUILD_BUG_ON() are used to indicate that the code would fall over the
> >>>> check pass. I can't find the justification for this change in the
> >>>> commit message.  
> >>> I had a look at the following commit which introduced this, but I
> >>> couldn't get the explaination for this.
> >>>
> >>> commit 88e3ed61642bb393458acc7a9bd2f96edc337190
> >>> Author: Jan Beulich <jbeulich@suse.com>
> >>> Date:   Tue Sep 1 14:02:57 2015 +0200
> >>>
> >>> @Jan :- Do you know why BUILD_BUG_ON() was introduced ?  
> >> You've cut too much context - the next line explains this all by itself,
> >> I think:
> >>
> >>      xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
> >>
> >> Clearly addresses used for the Xen heap need to be representable in an
> >> unsigned long (which we assume to be the same size as void *).  
> > So I am wondering why you hit that code for your port in the first case?
> > If you check, then ARM32 won't pass this, because PADDR_BITS is 40, while
> > a long is still 32 bits.  
> 
> But, PADDR_BITS should be equal to 32 for 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."
> 
>  From this, I understood that as virtual address is 32 bits for AArch32, 
> so physical address will also be 32 bits.
> 
> Please correct me if I am misunderstanding ?

This is correct, but it's also a big distraction, as no VMSA means that
most of the traditional Xen code becomes irrelevant, since you cannot map
anything anyways. You have to check how the v8-R64 code handles this, I
guess it uses separate code paths?

> If this is correct, then ...
> 
> > So digging deeper this is code for the case when we want to map the entire
> > physical memory into Xen (the Xen heap, or direct map in Linux terms).
> > Which we cannot do for ARM32, and that's why the code is protected by
> > !CONFIG_SEPARATE_XENHEAP, which is forced to 1 for CONFIG_ARM_32 (in a
> > hacked up way, btw).  
> 
> we can map entire physical memory into Xen as VA == PA.

We cannot map anything to begin with, since we have no tables. So without
mapping, indeed the whole physical memory naturally fits into the CPU
address space. But in any case you must not use this code, as this tries
to *map* something, which we do not support on R-class. Again check what
the v8-R64 code has to say on this topic.

Cheers,
Andre

> > So I think you must just force the same thing for your port, then this
> > code will never be compiled.
> >
> > Does that make sense?
> >
> > Cheers,
> > Andre
> >  
> >> But I'm afraid there's further context missing for your question: Why
> >> would that construct be a problem in your case? Is it just that you'd
> >> need it to be > rather than the >= that's used presently? If so, why
> >> do you add an #ifdef rather than dealing with the (apparent) off-by-1?
> >> (I say "apparent" because I haven't checked whether the >= is really
> >> depended upon anywhere.)
> >>
> >> Jan  
Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Ayan Kumar Halder 1 year, 4 months ago
On 30/11/2022 07:09, Jan Beulich wrote:
> On 29.11.2022 19:18, Ayan Kumar Halder wrote:
>> On 29/11/2022 15:52, Julien Grall wrote:
>>> On 29/11/2022 16:23, Ayan Kumar Halder wrote:
>>>> On 29/11/2022 14:52, Julien Grall wrote:
>>>>> On 29/11/2022 14:57, Ayan Kumar Halder wrote:
>>>> --- a/xen/common/page_alloc.c
>>>> +++ b/xen/common/page_alloc.c
>>>> @@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
>>>>    {
>>>>        ASSERT(!first_node_initialised);
>>>>        ASSERT(!xenheap_bits);
>>>> +#ifndef CONFIG_AARCH32_V8R
>>>>        BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
>>>> +#endif
>>> BUILD_BUG_ON() are used to indicate that the code would fall over the
>>> check pass. I can't find the justification for this change in the
>>> commit message.
>> I had a look at the following commit which introduced this, but I
>> couldn't get the explaination for this.
>>
>> commit 88e3ed61642bb393458acc7a9bd2f96edc337190
>> Author: Jan Beulich <jbeulich@suse.com>
>> Date:   Tue Sep 1 14:02:57 2015 +0200
>>
>> @Jan :- Do you know why BUILD_BUG_ON() was introduced ?
> You've cut too much context - the next line explains this all by itself,
> I think:
>
>      xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
>
> Clearly addresses used for the Xen heap need to be representable in an
> unsigned long (which we assume to be the same size as void *).
>
> But I'm afraid there's further context missing for your question: Why
> would that construct be a problem in your case? Is it just that you'd
> need it to be > rather than the >= that's used presently?

In my case (for Cortex-R52 build) :-

PADDR_BITS = 32

BITS_PER_LONG = 32

Thus, "BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG)" gets triggered.

I think the physical adresses are representable using "unsigned long".

Yes, using "BUILD_BUG_ON(PADDR_BITS > BITS_PER_LONG)" looks correct to me.

> If so, why
> do you add an #ifdef rather than dealing with the (apparent) off-by-1?
> (I say "apparent" because I haven't checked whether the >= is really
> depended upon anywhere.)

The only callers of xenheap_max_mfn() are from xen/arch/x86/setup.c as 
follows :-

1. xenheap_max_mfn(PFN_DOWN(highmem_start - 1));

2. xenheap_max_mfn(limit);

Looking at "min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);", I do not 
understand the reason for "... -1" (ie subtracting by 1).

Do you know the reason ?

- Ayan

>
> Jan

Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Jan Beulich 1 year, 4 months ago
On 30.11.2022 12:27, Ayan Kumar Halder wrote:
> Looking at "min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);", I do not 
> understand the reason for "... -1" (ie subtracting by 1).
> 
> Do you know the reason ?

That's because fls() and flsl() and friends return 1-based bit indexes
(with 0 being returned when the input was zero), when we need 0-based
values here.

Jan
Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses
Posted by Ayan Kumar Halder 1 year, 4 months ago
On 30/11/2022 12:01, Jan Beulich wrote:
> On 30.11.2022 12:27, Ayan Kumar Halder wrote:
>> Looking at "min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);", I do not
>> understand the reason for "... -1" (ie subtracting by 1).
>>
>> Do you know the reason ?
> That's because fls() and flsl() and friends return 1-based bit indexes
> (with 0 being returned when the input was zero), when we need 0-based
> values here.

IIUC, you are comparing the count of bits here (not bit indexes).

PADDR_BITS = 52 (x86), 48 (arm64) and 40 (armv7 arm32).

Also fls() returning 0 is correct as none of the bits are set.

In this case "flsl(mfn + 1)" will return 1 when mfn == 0(min value), so 
I think you are subtracting by 1.


Should I send a patch to fix the below ?

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 62afb07bc6..cd390a0956 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2245,7 +2245,7 @@ void __init xenheap_max_mfn(unsigned long mfn)
  {
      ASSERT(!first_node_initialised);
      ASSERT(!xenheap_bits);
-    BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
+    BUILD_BUG_ON(PADDR_BITS > BITS_PER_LONG);
      xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
      printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
  }

- Ayan

>
> Jan