[XEN v1 0/9] Add support for 32 bit physical address

[XEN v1 0/9] Add support for 32 bit physical address

[Ayan Kumar Halder]

Hi,

Please have a look at https://lists.xenproject.org/archives/html/xen-devel/2022-11/msg01465.html
for the context.

The benefits of using 32 bit physical addresses are as follows :-

1. It helps to use Xen on platforms (for eg R52) which supports 32 bit
physical addresses and has no support for large page address extension.
On 32 bit MPU systems which supports flat-mapping (for eg R52), it helps
to translate 32 bit VA into 32 bit PA.

2. It also helps in code optimization when the underlying platform does not
use large page address extension.


The following points are to be noted :-
1. Device tree always use u64 for address and size. The caller needs to
translate between u64 and u32 (when 32 bit physical addressing is used).
2. Currently, we have enabled this option for Arm_32 as the MMU for Arm_64
uses 48 bit physical addressing.
3. https://lists.xenproject.org/archives/html/xen-devel/2022-12/msg00117.html
has been added to this series.

Ayan Kumar Halder (9):
  xen/arm: Remove the extra assignment
  xen/arm: Define translate_dt_address_size() for the translation
    between u64 and paddr_t
  xen/arm: Always use 'u64' instead of 'paddr_t' for address and size in
    DT
  xen/arm: Use translate_dt_address_size() to translate between device
    tree addr/size and paddr_t
  xen/arm: Use 'PRIpaddr' to display 'paddr_t' variable
  xen/arm: Use 'u64' to represent 'unsigned long long'
  xen/arm: Restrict zeroeth_table_offset for ARM_64
  xen/arm: Other adaptations required to support 32bit paddr
  xen/arm: Introduce ARM_PA_32 to support 32 bit physical address

 xen/arch/arm/Kconfig                 |  9 ++++
 xen/arch/arm/bootfdt.c               | 22 +++++----
 xen/arch/arm/domain_build.c          | 69 ++++++++++++++++++++--------
 xen/arch/arm/gic-v2.c                | 39 ++++++++++++----
 xen/arch/arm/gic-v3.c                | 33 +++++++++++--
 xen/arch/arm/guest_walk.c            |  2 +
 xen/arch/arm/include/asm/lpae.h      | 10 ++++
 xen/arch/arm/include/asm/page-bits.h |  2 +
 xen/arch/arm/include/asm/platform.h  | 26 +++++++++++
 xen/arch/arm/include/asm/types.h     |  7 +++
 xen/arch/arm/mm.c                    |  6 ++-
 xen/arch/arm/platforms/brcm.c        |  9 +++-
 xen/arch/arm/platforms/exynos5.c     | 48 +++++++++++++------
 xen/arch/arm/platforms/sunxi.c       | 11 ++++-
 xen/arch/arm/setup.c                 | 18 +++++++-
 xen/drivers/char/exynos4210-uart.c   | 10 +++-
 xen/drivers/char/ns16550.c           | 16 ++++---
 xen/drivers/char/omap-uart.c         | 10 +++-
 xen/drivers/char/pl011.c             | 10 +++-
 xen/drivers/char/scif-uart.c         | 10 +++-
 xen/drivers/passthrough/arm/smmu.c   | 18 ++++++--
 xen/include/xen/serial.h             |  2 +-
 22 files changed, 305 insertions(+), 82 deletions(-)

-- 
2.17.1

[XEN v1 1/9] xen/arm: Remove the extra assignment

[Ayan Kumar Halder]

As "io_size" and "uart->io_size" are both u64, so there will be no truncation.
Thus, one can remove the ASSERT() and extra assignment.

In an earlier commit (7c1de0038895),
"ns16550.io_size" was u32 and "io_size" was u64. Thus, the ASSERT() was needed
to check if the values are the same.
However, in a later commit (c9f8e0aee507),
"ns16550.io_size" was changed to u64. Thus, the ASSERT() became redundant.

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

 xen/drivers/char/ns16550.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 01a05c9aa8..58d0ccd889 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1747,7 +1747,6 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
     struct ns16550 *uart;
     int res;
     u32 reg_shift, reg_width;
-    u64 io_size;
 
     uart = &ns16550_com[0];
 
@@ -1758,14 +1757,10 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
     uart->parity    = UART_PARITY_NONE;
     uart->stop_bits = 1;
 
-    res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
+    res = dt_device_get_address(dev, 0, &uart->io_base, &uart->io_size);
     if ( res )
         return res;
 
-    uart->io_size = io_size;
-
-    ASSERT(uart->io_size == io_size); /* Detect truncation */
-
     res = dt_property_read_u32(dev, "reg-shift", &reg_shift);
     if ( !res )
         uart->reg_shift = 0;
-- 
2.17.1

Re: [XEN v1 1/9] xen/arm: Remove the extra assignment

[Jan Beulich]

On 15.12.2022 20:32, Ayan Kumar Halder wrote:
> As "io_size" and "uart->io_size" are both u64, so there will be no truncation.
> Thus, one can remove the ASSERT() and extra assignment.
> 
> In an earlier commit (7c1de0038895),
> "ns16550.io_size" was u32 and "io_size" was u64. Thus, the ASSERT() was needed
> to check if the values are the same.
> However, in a later commit (c9f8e0aee507),
> "ns16550.io_size" was changed to u64. Thus, the ASSERT() became redundant.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

With the title improved to indicate what component is actually being touched
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [XEN v1 1/9] xen/arm: Remove the extra assignment

[Julien Grall]

Hi,

In the previous version, I have suggested the following title:

xen/ns16550: Remove unneeded truncation check in the DT init code

This would also address Jan's comment.

On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> As "io_size" and "uart->io_size" are both u64, so there will be no truncation.
> Thus, one can remove the ASSERT() and extra assignment.
> 
> In an earlier commit (7c1de0038895),

Why is this line shorter than the others?

> "ns16550.io_size" was u32 and "io_size" was u64. Thus, the ASSERT() was needed
> to check if the values are the same.
> However, in a later commit (c9f8e0aee507),
> "ns16550.io_size" was changed to u64. Thus, the ASSERT() became redundant.

You missed my comment here:

"Those two paragraphs are explaining why the truncation check is 
removed. So I think they should be moved first. Then you can add the 
initial paragraph to explain the resolution".

> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 
>   xen/drivers/char/ns16550.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 01a05c9aa8..58d0ccd889 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1747,7 +1747,6 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
>       struct ns16550 *uart;
>       int res;
>       u32 reg_shift, reg_width;
> -    u64 io_size;
>   
>       uart = &ns16550_com[0];
>   
> @@ -1758,14 +1757,10 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
>       uart->parity    = UART_PARITY_NONE;
>       uart->stop_bits = 1;
>   
> -    res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
> +    res = dt_device_get_address(dev, 0, &uart->io_base, &uart->io_size);
>       if ( res )
>           return res;
>   
> -    uart->io_size = io_size;
> -
> -    ASSERT(uart->io_size == io_size); /* Detect truncation */
> -
>       res = dt_property_read_u32(dev, "reg-shift", &reg_shift);
>       if ( !res )
>           uart->reg_shift = 0;

Cheers,

-- 
Julien Grall

[XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t

[Ayan Kumar Halder]

paddr_t may be u64 or u32 depending of the type of architecture.
Thus, while translating between u64 and paddr_t, one should check that the
truncated bits are 0. If not, then raise an appropriate error.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/include/asm/platform.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/xen/arch/arm/include/asm/platform.h b/xen/arch/arm/include/asm/platform.h
index 997eb25216..6be1549f09 100644
--- a/xen/arch/arm/include/asm/platform.h
+++ b/xen/arch/arm/include/asm/platform.h
@@ -42,6 +42,32 @@ struct platform_desc {
     unsigned int dma_bitsize;
 };
 
+static inline int translate_dt_address_size(u64 *dt_addr, u64 *dt_size,
+                                            paddr_t *addr, paddr_t *size)
+{
+#ifdef CONFIG_ARM_PA_32
+    if ( dt_addr && (*dt_addr >> PADDR_SHIFT) )
+    {
+        dprintk(XENLOG_ERR, "Error in DT. Invalid address\n");
+        return -ENXIO;
+    }
+
+    if ( dt_size && (*dt_size >> PADDR_SHIFT) )
+    {
+        dprintk(XENLOG_ERR, "Error in DT. Invalid size\n");
+        return -ENXIO;
+    }
+#endif
+
+    if ( dt_addr && addr )
+        *addr = (paddr_t) (*dt_addr);
+
+    if ( dt_size && size )
+        *size = (paddr_t) (*dt_size);
+
+    return 0;
+}
+
 /*
  * Quirk for platforms where device tree incorrectly reports 4K GICC
  * size, but actually the two GICC register ranges are placed at 64K
-- 
2.17.1

Re: [XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t

[Julien Grall]

Hi Ayan,

On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> paddr_t may be u64 or u32 depending of the type of architecture.
> Thus, while translating between u64 and paddr_t, one should check that the
> truncated bits are 0. If not, then raise an appropriate error.

I am not entirely convinced this extra helper is worth it. If the user 
can't provide 32-bit address in the DT, then there are also a lot of 
other part that can go wrong.

Bertrand, Stefano, what do you think?

> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>   xen/arch/arm/include/asm/platform.h | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/platform.h b/xen/arch/arm/include/asm/platform.h
> index 997eb25216..6be1549f09 100644
> --- a/xen/arch/arm/include/asm/platform.h
> +++ b/xen/arch/arm/include/asm/platform.h
> @@ -42,6 +42,32 @@ struct platform_desc {
>       unsigned int dma_bitsize;
>   };
>   
> +static inline int translate_dt_address_size(u64 *dt_addr, u64 *dt_size,
> +                                            paddr_t *addr, paddr_t *size)
> +{
> +#ifdef CONFIG_ARM_PA_32

This is not yet defined. So you want to mention it in the commit message.

> +    if ( dt_addr && (*dt_addr >> PADDR_SHIFT) )
> +    {
> +        dprintk(XENLOG_ERR, "Error in DT. Invalid address\n");
> +        return -ENXIO;
> +    }
> +
> +    if ( dt_size && (*dt_size >> PADDR_SHIFT) )
> +    {
> +        dprintk(XENLOG_ERR, "Error in DT. Invalid size\n");
> +        return -ENXIO;
> +    }
> +#endif
> +
> +    if ( dt_addr && addr )
> +        *addr = (paddr_t) (*dt_addr);
> +
> +    if ( dt_size && size )
> +        *size = (paddr_t) (*dt_size);
> +
> +    return 0;
> +}
> +
>   /*
>    * Quirk for platforms where device tree incorrectly reports 4K GICC
>    * size, but actually the two GICC register ranges are placed at 64K

Cheers,

-- 
Julien Grall

Re: [XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t

[Stefano Stabellini]

On Fri, 16 Dec 2022, Julien Grall wrote:
> Hi Ayan,
> 
> On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> > paddr_t may be u64 or u32 depending of the type of architecture.
> > Thus, while translating between u64 and paddr_t, one should check that the
> > truncated bits are 0. If not, then raise an appropriate error.
> 
> I am not entirely convinced this extra helper is worth it. If the user can't
> provide 32-bit address in the DT, then there are also a lot of other part that
> can go wrong.
> 
> Bertrand, Stefano, what do you think?

In general, it is not Xen's job to protect itself against bugs in device
tree. However, if Xen spots a problem in DT and prints a helpful message
that is better than just crashing because it gives a hint to the
developer about what the problem is.

In this case, I think a BUG_ON would be sufficient.


> > 
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> > ---
> >   xen/arch/arm/include/asm/platform.h | 26 ++++++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> > 
> > diff --git a/xen/arch/arm/include/asm/platform.h
> > b/xen/arch/arm/include/asm/platform.h
> > index 997eb25216..6be1549f09 100644
> > --- a/xen/arch/arm/include/asm/platform.h
> > +++ b/xen/arch/arm/include/asm/platform.h
> > @@ -42,6 +42,32 @@ struct platform_desc {
> >       unsigned int dma_bitsize;
> >   };
> >   +static inline int translate_dt_address_size(u64 *dt_addr, u64 *dt_size,
> > +                                            paddr_t *addr, paddr_t *size)
> > +{
> > +#ifdef CONFIG_ARM_PA_32
> 
> This is not yet defined. So you want to mention it in the commit message.
> 
> > +    if ( dt_addr && (*dt_addr >> PADDR_SHIFT) )
> > +    {
> > +        dprintk(XENLOG_ERR, "Error in DT. Invalid address\n");
> > +        return -ENXIO;
> > +    }
> > +
> > +    if ( dt_size && (*dt_size >> PADDR_SHIFT) )
> > +    {
> > +        dprintk(XENLOG_ERR, "Error in DT. Invalid size\n");
> > +        return -ENXIO;
> > +    }
> > +#endif
> > +
> > +    if ( dt_addr && addr )
> > +        *addr = (paddr_t) (*dt_addr);
> > +
> > +    if ( dt_size && size )
> > +        *size = (paddr_t) (*dt_size);
> > +
> > +    return 0;
> > +}
> > +
> >   /*
> >    * Quirk for platforms where device tree incorrectly reports 4K GICC
> >    * size, but actually the two GICC register ranges are placed at 64K
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

Re: [XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t

[Julien Grall]

Hi,

On 17/12/2022 00:46, Stefano Stabellini wrote:
> On Fri, 16 Dec 2022, Julien Grall wrote:
>> Hi Ayan,
>>
>> On 15/12/2022 19:32, Ayan Kumar Halder wrote:
>>> paddr_t may be u64 or u32 depending of the type of architecture.
>>> Thus, while translating between u64 and paddr_t, one should check that the
>>> truncated bits are 0. If not, then raise an appropriate error.
>>
>> I am not entirely convinced this extra helper is worth it. If the user can't
>> provide 32-bit address in the DT, then there are also a lot of other part that
>> can go wrong.
>>
>> Bertrand, Stefano, what do you think?
> 
> In general, it is not Xen's job to protect itself against bugs in device
> tree. However, if Xen spots a problem in DT and prints a helpful message
> that is better than just crashing because it gives a hint to the
> developer about what the problem is.

I agree with the principle. In practice this needs to be weight out with 
the long-term maintenance.

> 
> In this case, I think a BUG_ON would be sufficient.

BUG_ON() is the same as panic(). They both should be used only when 
there are no way to recover (see CODING_STYLE).

If we parse the device-tree at boot, then BUG_ON() is ok. However, if we 
need to parse it after boot (e.g. the DT overlay from Vikram), then we 
should definitely not call BUG_ON() for checking DT input.

The correct way is like Ayan did by checking returning an error and let
the caller deciding what to do.

My concern with his approach is the extra amount of code in each caller 
to check it (even with a new helper).

I am not fully convinced that checking the addresses in the DT is that 
useful. However, if you both want to do it, then I think it would be 
best to introduce wrappers over the Device-Tree ones that will check the 
truncation.

For example, we could introduce dt_device_get_address_checked()
that will call dt_device_get_address() and then check for 32-bit truncation.

For the function device_tree_get_reg(), this helper was aimed to deal 
with just Physical address 'reg' very early. So we can modify the 
prototype and update the function to check for 32-bit truncation.

Note that I am suggesting a different approach for the two helpers 
because the former is generic and it is not clear to me whether this 
could be used in another context than physical address.

Cheers,

-- 
Julien Grall

Re: [XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t

[Stefano Stabellini]

On Sat, 17 Dec 2022, Julien Grall wrote:
> On 17/12/2022 00:46, Stefano Stabellini wrote:
> > On Fri, 16 Dec 2022, Julien Grall wrote:
> > > Hi Ayan,
> > > 
> > > On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> > > > paddr_t may be u64 or u32 depending of the type of architecture.
> > > > Thus, while translating between u64 and paddr_t, one should check that
> > > > the
> > > > truncated bits are 0. If not, then raise an appropriate error.
> > > 
> > > I am not entirely convinced this extra helper is worth it. If the user
> > > can't
> > > provide 32-bit address in the DT, then there are also a lot of other part
> > > that
> > > can go wrong.
> > > 
> > > Bertrand, Stefano, what do you think?
> > 
> > In general, it is not Xen's job to protect itself against bugs in device
> > tree. However, if Xen spots a problem in DT and prints a helpful message
> > that is better than just crashing because it gives a hint to the
> > developer about what the problem is.
> 
> I agree with the principle. In practice this needs to be weight out with the
> long-term maintenance.
> 
> > 
> > In this case, I think a BUG_ON would be sufficient.
> 
> BUG_ON() is the same as panic(). They both should be used only when there are
> no way to recover (see CODING_STYLE).
> 
> If we parse the device-tree at boot, then BUG_ON() is ok. However, if we need
> to parse it after boot (e.g. the DT overlay from Vikram), then we should
> definitely not call BUG_ON() for checking DT input.

yeah, I wasn't thinking of that series


> The correct way is like Ayan did by checking returning an error and let
> the caller deciding what to do.
> 
> My concern with his approach is the extra amount of code in each caller to
> check it (even with a new helper).
> 
> I am not fully convinced that checking the addresses in the DT is that useful.

I am also happy not to do it to be honest


> However, if you both want to do it, then I think it would be best to introduce
> wrappers over the Device-Tree ones that will check the truncation.
> 
> For example, we could introduce dt_device_get_address_checked()
> that will call dt_device_get_address() and then check for 32-bit truncation.
> 
> For the function device_tree_get_reg(), this helper was aimed to deal with
> just Physical address 'reg' very early. So we can modify the prototype and
> update the function to check for 32-bit truncation.
> 
> Note that I am suggesting a different approach for the two helpers because the
> former is generic and it is not clear to me whether this could be used in
> another context than physical address.

Re: [XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t

[Ayan Kumar Halder]

Hi Julien/Stefano,

I want to make sure I understand correctly.

On 22/12/2022 23:20, Stefano Stabellini wrote:
> On Sat, 17 Dec 2022, Julien Grall wrote:
>> On 17/12/2022 00:46, Stefano Stabellini wrote:
>>> On Fri, 16 Dec 2022, Julien Grall wrote:
>>>> Hi Ayan,
>>>>
>>>> On 15/12/2022 19:32, Ayan Kumar Halder wrote:
>>>>> paddr_t may be u64 or u32 depending of the type of architecture.
>>>>> Thus, while translating between u64 and paddr_t, one should check that
>>>>> the
>>>>> truncated bits are 0. If not, then raise an appropriate error.
>>>> I am not entirely convinced this extra helper is worth it. If the user
>>>> can't
>>>> provide 32-bit address in the DT, then there are also a lot of other part
>>>> that
>>>> can go wrong.
>>>>
>>>> Bertrand, Stefano, what do you think?
>>> In general, it is not Xen's job to protect itself against bugs in device
>>> tree. However, if Xen spots a problem in DT and prints a helpful message
>>> that is better than just crashing because it gives a hint to the
>>> developer about what the problem is.
>> I agree with the principle. In practice this needs to be weight out with the
>> long-term maintenance.
>>
>>> In this case, I think a BUG_ON would be sufficient.
>> BUG_ON() is the same as panic(). They both should be used only when there are
>> no way to recover (see CODING_STYLE).
>>
>> If we parse the device-tree at boot, then BUG_ON() is ok. However, if we need
>> to parse it after boot (e.g. the DT overlay from Vikram), then we should
>> definitely not call BUG_ON() for checking DT input.
> yeah, I wasn't thinking of that series
>
>
>> The correct way is like Ayan did by checking returning an error and let
>> the caller deciding what to do.
>>
>> My concern with his approach is the extra amount of code in each caller to
>> check it (even with a new helper).
>>
>> I am not fully convinced that checking the addresses in the DT is that useful.
> I am also happy not to do it to be honest

So are you suggesting that we do the truncation, but do not check if any 
non zero bits have been truncated.

As an example, currently with this patch :-

-        device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase);
-        psize = dt_read_number(cells, size_cells);
+        device_tree_get_reg(&cells, addr_cells, addr_cells, &dt_pbase, &dt_gbase);
+        ret = translate_dt_address_size(&dt_pbase, &dt_gbase, &pbase, &gbase);
+        if ( ret )
+            return ret;
+        dt_psize = dt_read_number(cells, size_cells);
+        ret = translate_dt_address_size(NULL, &dt_psize, NULL, &psize);


With your proposed change, it should be

-        device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase);
-        psize = dt_read_number(cells, size_cells);
+        device_tree_get_reg(&cells, addr_cells, addr_cells, &dt_pbase, &dt_gbase);
+        dt_psize = dt_read_number(cells, size_cells);
+        pbase = (paddr_t) dt_pbase;
+        gbase = (paddr_t) dt_gbase;
+        psize = (paddr_t) dt_psize;

Because, we still need some way to convert u64 dt address/size to 
paddr_t (u64/u32) address/size.

Please correct me if I misunderstand something.

- Ayan

>
>
>> However, if you both want to do it, then I think it would be best to introduce
>> wrappers over the Device-Tree ones that will check the truncation.
>>
>> For example, we could introduce dt_device_get_address_checked()
>> that will call dt_device_get_address() and then check for 32-bit truncation.
>>
>> For the function device_tree_get_reg(), this helper was aimed to deal with
>> just Physical address 'reg' very early. So we can modify the prototype and
>> update the function to check for 32-bit truncation.
>>
>> Note that I am suggesting a different approach for the two helpers because the
>> former is generic and it is not clear to me whether this could be used in
>> another context than physical address.

Re: [XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t

[Julien Grall]

On 23/12/2022 10:01, Ayan Kumar Halder wrote:
> Hi Julien/Stefano,
> 
> I want to make sure I understand correctly.
> 
> On 22/12/2022 23:20, Stefano Stabellini wrote:
>> On Sat, 17 Dec 2022, Julien Grall wrote:
>>> On 17/12/2022 00:46, Stefano Stabellini wrote:
>>>> On Fri, 16 Dec 2022, Julien Grall wrote:
>>>>> Hi Ayan,
>>>>>
>>>>> On 15/12/2022 19:32, Ayan Kumar Halder wrote:
>>>>>> paddr_t may be u64 or u32 depending of the type of architecture.
>>>>>> Thus, while translating between u64 and paddr_t, one should check 
>>>>>> that
>>>>>> the
>>>>>> truncated bits are 0. If not, then raise an appropriate error.
>>>>> I am not entirely convinced this extra helper is worth it. If the user
>>>>> can't
>>>>> provide 32-bit address in the DT, then there are also a lot of 
>>>>> other part
>>>>> that
>>>>> can go wrong.
>>>>>
>>>>> Bertrand, Stefano, what do you think?
>>>> In general, it is not Xen's job to protect itself against bugs in 
>>>> device
>>>> tree. However, if Xen spots a problem in DT and prints a helpful 
>>>> message
>>>> that is better than just crashing because it gives a hint to the
>>>> developer about what the problem is.
>>> I agree with the principle. In practice this needs to be weight out 
>>> with the
>>> long-term maintenance.
>>>
>>>> In this case, I think a BUG_ON would be sufficient.
>>> BUG_ON() is the same as panic(). They both should be used only when 
>>> there are
>>> no way to recover (see CODING_STYLE).
>>>
>>> If we parse the device-tree at boot, then BUG_ON() is ok. However, if 
>>> we need
>>> to parse it after boot (e.g. the DT overlay from Vikram), then we should
>>> definitely not call BUG_ON() for checking DT input.
>> yeah, I wasn't thinking of that series
>>
>>
>>> The correct way is like Ayan did by checking returning an error and let
>>> the caller deciding what to do.
>>>
>>> My concern with his approach is the extra amount of code in each 
>>> caller to
>>> check it (even with a new helper).
>>>
>>> I am not fully convinced that checking the addresses in the DT is 
>>> that useful.
>> I am also happy not to do it to be honest
> 
> So are you suggesting that we do the truncation, but do not check if any 
> non zero bits have been truncated.
> 
> As an example, currently with this patch :-
> 
> -        device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, 
> &gbase);
> -        psize = dt_read_number(cells, size_cells);
> +        device_tree_get_reg(&cells, addr_cells, addr_cells, &dt_pbase, 
> &dt_gbase);
> +        ret = translate_dt_address_size(&dt_pbase, &dt_gbase, &pbase, 
> &gbase);
> +        if ( ret )
> +            return ret;
> +        dt_psize = dt_read_number(cells, size_cells);
> +        ret = translate_dt_address_size(NULL, &dt_psize, NULL, &psize);
> 
> 
> With your proposed change, it should be
> 
> -        device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, 
> &gbase);
> -        psize = dt_read_number(cells, size_cells);
> +        device_tree_get_reg(&cells, addr_cells, addr_cells, &dt_pbase, 
> &dt_gbase);
> +        dt_psize = dt_read_number(cells, size_cells);
> +        pbase = (paddr_t) dt_pbase;
> +        gbase = (paddr_t) dt_gbase;
> +        psize = (paddr_t) dt_psize;

-ETOOMANY cast and lines (the more if this is expected to be 
duplicated). But the last one seems unnecessary as the only reason you 
need separate variable for pbase and gbase is because the function are 
taking a reference rather than returning the value.

> 
> Because, we still need some way to convert u64 dt address/size to 
> paddr_t (u64/u32) address/size. 
How about following the approach I suggested in my previous e-mail to 
Stefano:
   - Convert device_tree_get_reg() to use paddr_t.
   - Introduce dt_device_get_address_checked() (Assuming you want to 
still add the check)

We may need an helper to wrap around dt_read_number() but I don't have a 
good idea for a name. There are only a couple of use. So I think it is 
fine to open-code. But you would not need a separate local variable. For 
the cast, I am in two mind. In one way, I don't like unnecessary 
explicit cast, but on the other way it serves as documentation.

Stefano, any opinions?

Cheers,

-- 
Julien Grall

Re: [XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4947 bytes --]

On Fri, 23 Dec 2022, Julien Grall wrote:
> On 23/12/2022 10:01, Ayan Kumar Halder wrote:
> > Hi Julien/Stefano,
> > 
> > I want to make sure I understand correctly.
> > 
> > On 22/12/2022 23:20, Stefano Stabellini wrote:
> > > On Sat, 17 Dec 2022, Julien Grall wrote:
> > > > On 17/12/2022 00:46, Stefano Stabellini wrote:
> > > > > On Fri, 16 Dec 2022, Julien Grall wrote:
> > > > > > Hi Ayan,
> > > > > > 
> > > > > > On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> > > > > > > paddr_t may be u64 or u32 depending of the type of architecture.
> > > > > > > Thus, while translating between u64 and paddr_t, one should check
> > > > > > > that
> > > > > > > the
> > > > > > > truncated bits are 0. If not, then raise an appropriate error.
> > > > > > I am not entirely convinced this extra helper is worth it. If the
> > > > > > user
> > > > > > can't
> > > > > > provide 32-bit address in the DT, then there are also a lot of other
> > > > > > part
> > > > > > that
> > > > > > can go wrong.
> > > > > > 
> > > > > > Bertrand, Stefano, what do you think?
> > > > > In general, it is not Xen's job to protect itself against bugs in
> > > > > device
> > > > > tree. However, if Xen spots a problem in DT and prints a helpful
> > > > > message
> > > > > that is better than just crashing because it gives a hint to the
> > > > > developer about what the problem is.
> > > > I agree with the principle. In practice this needs to be weight out with
> > > > the
> > > > long-term maintenance.
> > > > 
> > > > > In this case, I think a BUG_ON would be sufficient.
> > > > BUG_ON() is the same as panic(). They both should be used only when
> > > > there are
> > > > no way to recover (see CODING_STYLE).
> > > > 
> > > > If we parse the device-tree at boot, then BUG_ON() is ok. However, if we
> > > > need
> > > > to parse it after boot (e.g. the DT overlay from Vikram), then we should
> > > > definitely not call BUG_ON() for checking DT input.
> > > yeah, I wasn't thinking of that series
> > > 
> > > 
> > > > The correct way is like Ayan did by checking returning an error and let
> > > > the caller deciding what to do.
> > > > 
> > > > My concern with his approach is the extra amount of code in each caller
> > > > to
> > > > check it (even with a new helper).
> > > > 
> > > > I am not fully convinced that checking the addresses in the DT is that
> > > > useful.
> > > I am also happy not to do it to be honest
> > 
> > So are you suggesting that we do the truncation, but do not check if any non
> > zero bits have been truncated.
> > 
> > As an example, currently with this patch :-
> > 
> > -        device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase,
> > &gbase);
> > -        psize = dt_read_number(cells, size_cells);
> > +        device_tree_get_reg(&cells, addr_cells, addr_cells, &dt_pbase,
> > &dt_gbase);
> > +        ret = translate_dt_address_size(&dt_pbase, &dt_gbase, &pbase,
> > &gbase);
> > +        if ( ret )
> > +            return ret;
> > +        dt_psize = dt_read_number(cells, size_cells);
> > +        ret = translate_dt_address_size(NULL, &dt_psize, NULL, &psize);
> > 
> > 
> > With your proposed change, it should be
> > 
> > -        device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase,
> > &gbase);
> > -        psize = dt_read_number(cells, size_cells);
> > +        device_tree_get_reg(&cells, addr_cells, addr_cells, &dt_pbase,
> > &dt_gbase);
> > +        dt_psize = dt_read_number(cells, size_cells);
> > +        pbase = (paddr_t) dt_pbase;
> > +        gbase = (paddr_t) dt_gbase;
> > +        psize = (paddr_t) dt_psize;
> 
> -ETOOMANY cast and lines (the more if this is expected to be duplicated). But
> the last one seems unnecessary as the only reason you need separate variable
> for pbase and gbase is because the function are taking a reference rather than
> returning the value.
> 
> 
> > Because, we still need some way to convert u64 dt address/size to paddr_t
> > (u64/u32) address/size. 
> How about following the approach I suggested in my previous e-mail to Stefano:
>   - Convert device_tree_get_reg() to use paddr_t.

I think this is OK


>   - Introduce dt_device_get_address_checked() (Assuming you want to still add
> the check)

Let's just skip the check


> We may need an helper to wrap around dt_read_number() but I don't have a good
> idea for a name. There are only a couple of use. So I think it is fine to
> open-code. But you would not need a separate local variable. For the cast, I
> am in two mind. In one way, I don't like unnecessary explicit cast, but on the
> other way it serves as documentation.
>
> Stefano, any opinions?

I would not add a wrapper for dt_read_number() and open-code the cast,
like you said because something non-trivial is happening and it serves
as documentation:

  psize = (paddr_t) dt_read_number(cells, size_cells);

[XEN v1 3/9] xen/arm: Always use 'u64' instead of 'paddr_t' for address and size in DT

[Ayan Kumar Halder]

device_tree_get_reg(), dt_next_cell() uses u64 for address and size.
Thus, the caller needs to be fixed to pass u64 values and then invoke
translate_dt_address_size() to do the translation between u64 and paddr_t.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/bootfdt.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 0085c28d74..835bb5feb9 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -14,6 +14,7 @@
 #include <xen/libfdt/libfdt.h>
 #include <xen/sort.h>
 #include <xsm/xsm.h>
+#include <asm/platform.h>
 #include <asm/setup.h>
 
 static bool __init device_tree_node_matches(const void *fdt, int node,
@@ -68,7 +69,7 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
     unsigned int i, banks;
     const __be32 *cell;
     u32 reg_cells = address_cells + size_cells;
-    paddr_t start, size;
+    u64 start, size;
     struct meminfo *mem = data;
 
     if ( address_cells < 1 || size_cells < 1 )
@@ -219,7 +220,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
     const struct fdt_property *prop;
     const __be32 *cell;
     bootmodule_kind kind;
-    paddr_t start, size;
+    u64 start, size;
     int len;
     /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + '/0' => 92 */
     char path[92];
@@ -379,7 +380,8 @@ static int __init process_shm_node(const void *fdt, int node,
 {
     const struct fdt_property *prop, *prop_id, *prop_role;
     const __be32 *cell;
-    paddr_t paddr, gaddr, size;
+    paddr_t paddr = 0, gaddr = 0, size = 0;
+    u64 dt_paddr, dt_gaddr, dt_size;
     struct meminfo *mem = &bootinfo.reserved_mem;
     unsigned int i;
     int len;
@@ -443,10 +445,14 @@ static int __init process_shm_node(const void *fdt, int node,
     }
 
     cell = (const __be32 *)prop->data;
-    device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
-    size = dt_next_cell(size_cells, &cell);
+    device_tree_get_reg(&cell, address_cells, address_cells, &dt_paddr,
+                        &dt_gaddr);
+    translate_dt_address_size(&dt_paddr, &dt_gaddr, &paddr, &gaddr);
 
-    if ( !size )
+    dt_size = dt_next_cell(size_cells, &cell);
+    translate_dt_address_size(NULL, &dt_size, NULL, &size);
+
+    if ( !dt_size )
     {
         printk("fdt: the size for static shared memory region can not be zero\n");
         return -EINVAL;
@@ -593,12 +599,12 @@ static void __init early_print_info(void)
     nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
     for ( i = 0; i < nr_rsvd; i++ )
     {
-        paddr_t s, e;
+        u64 s, e;
         if ( fdt_get_mem_rsv(device_tree_flattened, i, &s, &e) < 0 )
             continue;
         /* fdt_get_mem_rsv returns length */
         e += s;
-        printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e);
+        printk(" RESVD[%u]: %"PRIx64" - %"PRIx64"\n", i, s, e);
     }
     for ( j = 0; j < mem_resv->nr_banks; j++, i++ )
     {
-- 
2.17.1

Re: [XEN v1 3/9] xen/arm: Always use 'u64' instead of 'paddr_t' for address and size in DT

[Julien Grall]

Hi,

This patch is actually a good example to demonstrate the extra amount of 
boiler plate required to use your new boiler.

On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> device_tree_get_reg(), dt_next_cell() uses u64 for address and size.
> Thus, the caller needs to be fixed to pass u64 values and then invoke
> translate_dt_address_size() to do the translation between u64 and paddr_t.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>   xen/arch/arm/bootfdt.c | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 0085c28d74..835bb5feb9 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -14,6 +14,7 @@
>   #include <xen/libfdt/libfdt.h>
>   #include <xen/sort.h>
>   #include <xsm/xsm.h>
> +#include <asm/platform.h>
>   #include <asm/setup.h>
>   
>   static bool __init device_tree_node_matches(const void *fdt, int node,
> @@ -68,7 +69,7 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
>       unsigned int i, banks;
>       const __be32 *cell;
>       u32 reg_cells = address_cells + size_cells;
> -    paddr_t start, size;
> +    u64 start, size;
>       struct meminfo *mem = data;
>   
>       if ( address_cells < 1 || size_cells < 1 )
> @@ -219,7 +220,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>       const struct fdt_property *prop;
>       const __be32 *cell;
>       bootmodule_kind kind;
> -    paddr_t start, size;
> +    u64 start, size;
>       int len;
>       /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + '/0' => 92 */
>       char path[92];
> @@ -379,7 +380,8 @@ static int __init process_shm_node(const void *fdt, int node,
>   {
>       const struct fdt_property *prop, *prop_id, *prop_role;
>       const __be32 *cell;
> -    paddr_t paddr, gaddr, size;
> +    paddr_t paddr = 0, gaddr = 0, size = 0;

For a first 0 is a valid address. So we should not use is as initialization.

> +    u64 dt_paddr, dt_gaddr, dt_size;
>       struct meminfo *mem = &bootinfo.reserved_mem;
>       unsigned int i;
>       int len;
> @@ -443,10 +445,14 @@ static int __init process_shm_node(const void *fdt, int node,
>       }
>   
>       cell = (const __be32 *)prop->data;
> -    device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
> -    size = dt_next_cell(size_cells, &cell);
> +    device_tree_get_reg(&cell, address_cells, address_cells, &dt_paddr,
> +                        &dt_gaddr);
> +    translate_dt_address_size(&dt_paddr, &dt_gaddr, &paddr, &gaddr
If we function return a value, then this should be checked. If not, then 
it should be explained.

In this case, it is not clear to me who is checking the conversion was 
successful.

Overall, I think this will increase the amount of code. So before doing 
the modification, I think we need to agree on whether this is worth it 
to check the device-tree values.

Cheers,

-- 
Julien Grall

Re: [XEN v1 3/9] xen/arm: Always use 'u64' instead of 'paddr_t' for address and size in DT

[Ayan Kumar Halder]

On 16/12/2022 09:57, Julien Grall wrote:
> Hi,
Hi Julien,
>
> This patch is actually a good example to demonstrate the extra amount 
> of boiler plate required to use your new boiler.
>
> On 15/12/2022 19:32, Ayan Kumar Halder wrote:
>> device_tree_get_reg(), dt_next_cell() uses u64 for address and size.
>> Thus, the caller needs to be fixed to pass u64 values and then invoke
>> translate_dt_address_size() to do the translation between u64 and 
>> paddr_t.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>>   xen/arch/arm/bootfdt.c | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>> index 0085c28d74..835bb5feb9 100644
>> --- a/xen/arch/arm/bootfdt.c
>> +++ b/xen/arch/arm/bootfdt.c
>> @@ -14,6 +14,7 @@
>>   #include <xen/libfdt/libfdt.h>
>>   #include <xen/sort.h>
>>   #include <xsm/xsm.h>
>> +#include <asm/platform.h>
>>   #include <asm/setup.h>
>>     static bool __init device_tree_node_matches(const void *fdt, int 
>> node,
>> @@ -68,7 +69,7 @@ static int __init device_tree_get_meminfo(const 
>> void *fdt, int node,
>>       unsigned int i, banks;
>>       const __be32 *cell;
>>       u32 reg_cells = address_cells + size_cells;
>> -    paddr_t start, size;
>> +    u64 start, size;
>>       struct meminfo *mem = data;
>>         if ( address_cells < 1 || size_cells < 1 )
>> @@ -219,7 +220,7 @@ static void __init process_multiboot_node(const 
>> void *fdt, int node,
>>       const struct fdt_property *prop;
>>       const __be32 *cell;
>>       bootmodule_kind kind;
>> -    paddr_t start, size;
>> +    u64 start, size;
>>       int len;
>>       /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + '/0' 
>> => 92 */
>>       char path[92];
>> @@ -379,7 +380,8 @@ static int __init process_shm_node(const void 
>> *fdt, int node,
>>   {
>>       const struct fdt_property *prop, *prop_id, *prop_role;
>>       const __be32 *cell;
>> -    paddr_t paddr, gaddr, size;
>> +    paddr_t paddr = 0, gaddr = 0, size = 0;
>
> For a first 0 is a valid address. So we should not use is as 
> initialization.
>
>> +    u64 dt_paddr, dt_gaddr, dt_size;
>>       struct meminfo *mem = &bootinfo.reserved_mem;
>>       unsigned int i;
>>       int len;
>> @@ -443,10 +445,14 @@ static int __init process_shm_node(const void 
>> *fdt, int node,
>>       }
>>         cell = (const __be32 *)prop->data;
>> -    device_tree_get_reg(&cell, address_cells, address_cells, &paddr, 
>> &gaddr);
>> -    size = dt_next_cell(size_cells, &cell);
>> +    device_tree_get_reg(&cell, address_cells, address_cells, &dt_paddr,
>> +                        &dt_gaddr);
>> +    translate_dt_address_size(&dt_paddr, &dt_gaddr, &paddr, &gaddr
> If we function return a value, then this should be checked. If not, 
> then it should be explained.
>
> In this case, it is not clear to me who is checking the conversion was 
> successful.
Sorry, I missed this. The caller should have checked for the return 
value and "return -EINVAL" for the conversion error.

I am in two minds.

I am thinking that instead of returing error from 
translate_dt_address_size(), the function can invoke panic() when it 
detects incorrect address/size.

Any errors related to incorrect address/size (ie providing 64 bit for 
PA_32) should be treated as fatal. But I do not see any precedent for 
this (ie libfdt does not panic for an error).

We could return an error from translate_dt_address_size() as we are 
doing today. It means that the errors need to be checked by all the 
callers (which adds extra code).

- Ayan

>
> Overall, I think this will increase the amount of code. So before 
> doing the modification, I think we need to agree on whether this is 
> worth it to check the device-tree values.
>
> Cheers,
>

Re: [XEN v1 3/9] xen/arm: Always use 'u64' instead of 'paddr_t' for address and size in DT

[Julien Grall]

On 16/12/2022 10:49, Ayan Kumar Halder wrote:
> 
> On 16/12/2022 09:57, Julien Grall wrote:
>> Hi,
> Hi Julien,
>>
>> This patch is actually a good example to demonstrate the extra amount 
>> of boiler plate required to use your new boiler.
>>
>> On 15/12/2022 19:32, Ayan Kumar Halder wrote:
>>> device_tree_get_reg(), dt_next_cell() uses u64 for address and size.
>>> Thus, the caller needs to be fixed to pass u64 values and then invoke
>>> translate_dt_address_size() to do the translation between u64 and 
>>> paddr_t.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> ---
>>>   xen/arch/arm/bootfdt.c | 22 ++++++++++++++--------
>>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>>> index 0085c28d74..835bb5feb9 100644
>>> --- a/xen/arch/arm/bootfdt.c
>>> +++ b/xen/arch/arm/bootfdt.c
>>> @@ -14,6 +14,7 @@
>>>   #include <xen/libfdt/libfdt.h>
>>>   #include <xen/sort.h>
>>>   #include <xsm/xsm.h>
>>> +#include <asm/platform.h>
>>>   #include <asm/setup.h>
>>>     static bool __init device_tree_node_matches(const void *fdt, int 
>>> node,
>>> @@ -68,7 +69,7 @@ static int __init device_tree_get_meminfo(const 
>>> void *fdt, int node,
>>>       unsigned int i, banks;
>>>       const __be32 *cell;
>>>       u32 reg_cells = address_cells + size_cells;
>>> -    paddr_t start, size;
>>> +    u64 start, size;
>>>       struct meminfo *mem = data;
>>>         if ( address_cells < 1 || size_cells < 1 )
>>> @@ -219,7 +220,7 @@ static void __init process_multiboot_node(const 
>>> void *fdt, int node,
>>>       const struct fdt_property *prop;
>>>       const __be32 *cell;
>>>       bootmodule_kind kind;
>>> -    paddr_t start, size;
>>> +    u64 start, size;
>>>       int len;
>>>       /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + '/0' 
>>> => 92 */
>>>       char path[92];
>>> @@ -379,7 +380,8 @@ static int __init process_shm_node(const void 
>>> *fdt, int node,
>>>   {
>>>       const struct fdt_property *prop, *prop_id, *prop_role;
>>>       const __be32 *cell;
>>> -    paddr_t paddr, gaddr, size;
>>> +    paddr_t paddr = 0, gaddr = 0, size = 0;
>>
>> For a first 0 is a valid address. So we should not use is as 
>> initialization.
>>
>>> +    u64 dt_paddr, dt_gaddr, dt_size;
>>>       struct meminfo *mem = &bootinfo.reserved_mem;
>>>       unsigned int i;
>>>       int len;
>>> @@ -443,10 +445,14 @@ static int __init process_shm_node(const void 
>>> *fdt, int node,
>>>       }
>>>         cell = (const __be32 *)prop->data;
>>> -    device_tree_get_reg(&cell, address_cells, address_cells, &paddr, 
>>> &gaddr);
>>> -    size = dt_next_cell(size_cells, &cell);
>>> +    device_tree_get_reg(&cell, address_cells, address_cells, &dt_paddr,
>>> +                        &dt_gaddr);
>>> +    translate_dt_address_size(&dt_paddr, &dt_gaddr, &paddr, &gaddr
>> If we function return a value, then this should be checked. If not, 
>> then it should be explained.
>>
>> In this case, it is not clear to me who is checking the conversion was 
>> successful.
> Sorry, I missed this. The caller should have checked for the return 
> value and "return -EINVAL" for the conversion error.
> 
> I am in two minds.
> 
> I am thinking that instead of returing error from 
> translate_dt_address_size(), the function can invoke panic() when it 
> detects incorrect address/size.

panic() is not an option if the function can be used after boot. But 
even if the use were restricted to boot ...

> 
> Any errors related to incorrect address/size (ie providing 64 bit for 
> PA_32) should be treated as fatal. 

... translate_dt_address_size() is too generic to make this decision. 
This is up to the caller to decide whether it is fine to ignore or crash.

> But I do not see any precedent for 
> this (ie libfdt does not panic for an error).

The same reasoning applies here.

> 
> We could return an error from translate_dt_address_size() as we are 
> doing today. It means that the errors need to be checked by all the 
> callers (which adds extra code).

Another option is the one I mentioned before. Don't check the truncation 
because that's not Xen job to very all the Device-Tree (in particular if 
this is intrusive).

Cheers,

-- 
Julien Grall

Re: [XEN v1 3/9] xen/arm: Always use 'u64' instead of 'paddr_t' for address and size in DT

[Julien Grall]

On 16/12/2022 11:12, Julien Grall wrote:
> 
> 
> On 16/12/2022 10:49, Ayan Kumar Halder wrote:
>>
>> On 16/12/2022 09:57, Julien Grall wrote:
>>> Hi,
>> Hi Julien,
>>>
>>> This patch is actually a good example to demonstrate the extra amount 
>>> of boiler plate required to use your new boiler.
>>>
>>> On 15/12/2022 19:32, Ayan Kumar Halder wrote:
>>>> device_tree_get_reg(), dt_next_cell() uses u64 for address and size.
>>>> Thus, the caller needs to be fixed to pass u64 values and then invoke
>>>> translate_dt_address_size() to do the translation between u64 and 
>>>> paddr_t.
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>> ---
>>>>   xen/arch/arm/bootfdt.c | 22 ++++++++++++++--------
>>>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>>>> index 0085c28d74..835bb5feb9 100644
>>>> --- a/xen/arch/arm/bootfdt.c
>>>> +++ b/xen/arch/arm/bootfdt.c
>>>> @@ -14,6 +14,7 @@
>>>>   #include <xen/libfdt/libfdt.h>
>>>>   #include <xen/sort.h>
>>>>   #include <xsm/xsm.h>
>>>> +#include <asm/platform.h>
>>>>   #include <asm/setup.h>
>>>>     static bool __init device_tree_node_matches(const void *fdt, int 
>>>> node,
>>>> @@ -68,7 +69,7 @@ static int __init device_tree_get_meminfo(const 
>>>> void *fdt, int node,
>>>>       unsigned int i, banks;
>>>>       const __be32 *cell;
>>>>       u32 reg_cells = address_cells + size_cells;
>>>> -    paddr_t start, size;
>>>> +    u64 start, size;
>>>>       struct meminfo *mem = data;
>>>>         if ( address_cells < 1 || size_cells < 1 )
>>>> @@ -219,7 +220,7 @@ static void __init process_multiboot_node(const 
>>>> void *fdt, int node,
>>>>       const struct fdt_property *prop;
>>>>       const __be32 *cell;
>>>>       bootmodule_kind kind;
>>>> -    paddr_t start, size;
>>>> +    u64 start, size;
>>>>       int len;
>>>>       /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + '/0' 
>>>> => 92 */
>>>>       char path[92];
>>>> @@ -379,7 +380,8 @@ static int __init process_shm_node(const void 
>>>> *fdt, int node,
>>>>   {
>>>>       const struct fdt_property *prop, *prop_id, *prop_role;
>>>>       const __be32 *cell;
>>>> -    paddr_t paddr, gaddr, size;
>>>> +    paddr_t paddr = 0, gaddr = 0, size = 0;
>>>
>>> For a first 0 is a valid address. So we should not use is as 
>>> initialization.
>>>
>>>> +    u64 dt_paddr, dt_gaddr, dt_size;
>>>>       struct meminfo *mem = &bootinfo.reserved_mem;
>>>>       unsigned int i;
>>>>       int len;
>>>> @@ -443,10 +445,14 @@ static int __init process_shm_node(const void 
>>>> *fdt, int node,
>>>>       }
>>>>         cell = (const __be32 *)prop->data;
>>>> -    device_tree_get_reg(&cell, address_cells, address_cells, 
>>>> &paddr, &gaddr);
>>>> -    size = dt_next_cell(size_cells, &cell);
>>>> +    device_tree_get_reg(&cell, address_cells, address_cells, 
>>>> &dt_paddr,
>>>> +                        &dt_gaddr);
>>>> +    translate_dt_address_size(&dt_paddr, &dt_gaddr, &paddr, &gaddr
>>> If we function return a value, then this should be checked. If not, 
>>> then it should be explained.
>>>
>>> In this case, it is not clear to me who is checking the conversion 
>>> was successful.
>> Sorry, I missed this. The caller should have checked for the return 
>> value and "return -EINVAL" for the conversion error.
>>
>> I am in two minds.
>>
>> I am thinking that instead of returing error from 
>> translate_dt_address_size(), the function can invoke panic() when it 
>> detects incorrect address/size.
> 
> panic() is not an option if the function can be used after boot. But 
> even if the use were restricted to boot ...
> 
>>
>> Any errors related to incorrect address/size (ie providing 64 bit for 
>> PA_32) should be treated as fatal. 
> 
> ... translate_dt_address_size() is too generic to make this decision. 
> This is up to the caller to decide whether it is fine to ignore or crash.
> 
>> But I do not see any precedent for this (ie libfdt does not panic for 
>> an error).
> 
> The same reasoning applies here.
> 
>>
>> We could return an error from translate_dt_address_size() as we are 
>> doing today. It means that the errors need to be checked by all the 
>> callers (which adds extra code).
> 
> Another option is the one I mentioned before. Don't check the truncation 
> because that's not Xen job to very all the Device-Tree (in particular if 


s/very/verify/

I only noticed it after sending (sorry).

> this is intrusive).
> 
> Cheers,
> 

-- 
Julien Grall

[XEN v1 4/9] xen/arm: Use translate_dt_address_size() to translate between device tree addr/size and paddr_t

[Ayan Kumar Halder]

dt_device_get_address() will return address and size which are always 'u64'.
One should use translate_dt_address_size() to translate address and size to
'paddr_t'.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/domain_build.c        | 53 +++++++++++++++++++++++-------
 xen/arch/arm/gic-v2.c              | 39 +++++++++++++++++-----
 xen/arch/arm/gic-v3.c              | 33 ++++++++++++++++---
 xen/arch/arm/platforms/brcm.c      |  9 +++--
 xen/arch/arm/platforms/exynos5.c   | 48 ++++++++++++++++++---------
 xen/arch/arm/platforms/sunxi.c     | 11 ++++++-
 xen/arch/arm/setup.c               | 18 ++++++++--
 xen/drivers/char/exynos4210-uart.c | 10 ++++--
 xen/drivers/char/ns16550.c         | 13 ++++++--
 xen/drivers/char/omap-uart.c       | 10 ++++--
 xen/drivers/char/pl011.c           | 10 ++++--
 xen/drivers/char/scif-uart.c       | 10 ++++--
 xen/drivers/passthrough/arm/smmu.c | 13 +++++---
 xen/include/xen/serial.h           |  2 +-
 14 files changed, 218 insertions(+), 61 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bef5e905a7..1bb97cd337 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -537,8 +537,11 @@ static mfn_t __init acquire_static_memory_bank(struct domain *d,
 {
     mfn_t smfn;
     int res;
+    u64 dt_pbase, dt_psize;
+
+    device_tree_get_reg(cell, addr_cells, size_cells, &dt_pbase, &dt_psize);
+    res = translate_dt_address_size(&dt_pbase, &dt_psize, pbase, psize);
 
-    device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
     ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize, PAGE_SIZE));
     if ( PFN_DOWN(*psize) > UINT_MAX )
     {
@@ -929,7 +932,8 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         const struct dt_property *prop;
         const __be32 *cells;
         uint32_t addr_cells, size_cells;
-        paddr_t gbase, pbase, psize;
+        u64 dt_gbase = 0, dt_pbase = 0, dt_psize = 0;
+        paddr_t gbase = 0, pbase = 0, psize = 0;
         int ret = 0;
         unsigned int i;
         const char *role_str;
@@ -948,8 +952,14 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
         BUG_ON(!prop);
         cells = (const __be32 *)prop->value;
-        device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase);
-        psize = dt_read_number(cells, size_cells);
+        device_tree_get_reg(&cells, addr_cells, addr_cells, &dt_pbase, &dt_gbase);
+        ret = translate_dt_address_size(&dt_pbase, &dt_gbase, &pbase, &gbase);
+        if ( ret )
+            return ret;
+
+        dt_psize = dt_read_number(cells, size_cells);
+        ret = translate_dt_address_size(NULL, &dt_psize, NULL, &psize);
+
         if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) )
         {
             printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n",
@@ -1666,13 +1676,14 @@ 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;
+        u64 dt_addr, dt_size;
 
         naddr = dt_number_of_address(np);
 
         for ( i = 0; i < naddr; i++ )
         {
-            res = dt_device_get_address(np, i, &addr, &size);
+            res = dt_device_get_address(np, i, &dt_addr, &dt_size);
             if ( res )
             {
                 printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
@@ -1680,6 +1691,10 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
                 goto out;
             }
 
+            res = translate_dt_address_size(&dt_addr, &dt_size, &addr, &size);
+            if ( res )
+                return res;
+
             start = addr & PAGE_MASK;
             end = PAGE_ALIGN(addr + size);
             res = rangeset_remove_range(mem_holes, start, end - 1);
@@ -2445,7 +2460,8 @@ 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;
+    u64 dt_addr, dt_size;
     bool own_device = !dt_device_for_passthrough(dev);
     /*
      * We want to avoid mapping the MMIO in dom0 for the following cases:
@@ -2500,7 +2516,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
     /* Give permission and map MMIOs */
     for ( i = 0; i < naddr; i++ )
     {
-        res = dt_device_get_address(dev, i, &addr, &size);
+        res = dt_device_get_address(dev, i, &dt_addr, &dt_size);
         if ( res )
         {
             printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
@@ -2508,6 +2524,10 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
             return res;
         }
 
+        res = translate_dt_address_size(&dt_addr, &dt_size, &addr, &size);
+        if ( res )
+            return res;
+
         res = map_range_to_domain(dev, addr, size, &mr_data);
         if ( res )
             return res;
@@ -2917,6 +2937,7 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
     struct dt_device_node *node;
     int res;
     paddr_t mstart, size, gstart;
+    u64 dt_mstart, dt_size, dt_gstart;
 
     /* xen,reg specifies where to map the MMIO region */
     cell = (const __be32 *)xen_reg->data;
@@ -2926,8 +2947,15 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
     for ( i = 0; i < len; i++ )
     {
         device_tree_get_reg(&cell, address_cells, size_cells,
-                            &mstart, &size);
-        gstart = dt_next_cell(address_cells, &cell);
+                            &dt_mstart, &dt_size);
+        res = translate_dt_address_size(&dt_mstart, &dt_size, &mstart, &size);
+        if ( res )
+            return res;
+
+        dt_gstart = dt_next_cell(address_cells, &cell);
+        res = translate_dt_address_size(&dt_gstart, NULL, &gstart, NULL);
+        if ( res )
+            return res;
 
         if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size & ~PAGE_MASK )
         {
@@ -2941,9 +2969,10 @@ 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 61802839cb..72de0707ed 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -979,6 +979,7 @@ static void gicv2_add_v2m_frame_to_list(paddr_t addr, paddr_t size,
 static void gicv2_extension_dt_init(const struct dt_device_node *node)
 {
     const struct dt_device_node *v2m = NULL;
+    int res;
 
     /*
      * Check whether this GIC implements the v2m extension. If so,
@@ -987,13 +988,18 @@ static void gicv2_extension_dt_init(const struct dt_device_node *node)
     dt_for_each_child_node(node, v2m)
     {
         u32 spi_start = 0, nr_spis = 0;
+        u64 dt_addr, dt_size;
         paddr_t addr, size;
 
         if ( !dt_device_is_compatible(v2m, "arm,gic-v2m-frame") )
             continue;
 
         /* Get register frame resource from DT. */
-        if ( dt_device_get_address(v2m, 0, &addr, &size) )
+        if ( dt_device_get_address(v2m, 0, &dt_addr, &dt_size) )
+            panic("GICv2: Cannot find a valid v2m frame address\n");
+
+        res = translate_dt_address_size(&dt_addr, &dt_size, &addr, &size);
+        if ( res )
             panic("GICv2: Cannot find a valid v2m frame address\n");
 
         /*
@@ -1017,20 +1023,37 @@ static void __init gicv2_dt_init(void)
     int res;
     paddr_t vsize;
     const struct dt_device_node *node = gicv2_info.node;
+    u64 dt_hbase, dt_dbase, dt_cbase, dt_csize, dt_vbase, dt_vsize;
 
-    res = dt_device_get_address(node, 0, &dbase, NULL);
+    res = dt_device_get_address(node, 0, &dt_dbase, NULL);
     if ( res )
         panic("GICv2: Cannot find a valid address for the distributor\n");
 
-    res = dt_device_get_address(node, 1, &cbase, &csize);
+    res = translate_dt_address_size(&dt_dbase, NULL, &dbase, NULL);
+    if ( res )
+        panic("GICv2: Cannot find a valid address for the distributor\n");
+
+    res = dt_device_get_address(node, 1, &dt_cbase, &dt_csize);
+    if ( res )
+        panic("GICv2: Cannot find a valid address for the CPU\n");
+
+    res = translate_dt_address_size(&dt_cbase, &dt_csize, &cbase, &csize);
     if ( res )
         panic("GICv2: Cannot find a valid address for the CPU\n");
 
-    res = dt_device_get_address(node, 2, &hbase, NULL);
+    res = dt_device_get_address(node, 2, &dt_hbase, NULL);
+    if ( res )
+        panic("GICv2: Cannot find a valid address for the hypervisor\n");
+
+    res = translate_dt_address_size(&dt_hbase, NULL, &hbase, NULL);
     if ( res )
         panic("GICv2: Cannot find a valid address for the hypervisor\n");
 
-    res = dt_device_get_address(node, 3, &vbase, &vsize);
+    res = dt_device_get_address(node, 3, &dt_vbase, &dt_vsize);
+    if ( res )
+        panic("GICv2: Cannot find a valid address for the virtual CPU\n");
+
+    res = translate_dt_address_size(&dt_vbase, &dt_vsize, &vbase, &vsize);
     if ( res )
         panic("GICv2: Cannot find a valid address for the virtual CPU\n");
 
@@ -1049,7 +1072,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) )
         {
@@ -1280,11 +1303,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 bb59ea94cd..db64009483 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -33,6 +33,7 @@
 #include <asm/gic_v3_defs.h>
 #include <asm/gic_v3_its.h>
 #include <asm/io.h>
+#include <asm/platform.h>
 #include <asm/sysregs.h>
 
 /* Global state */
@@ -1376,8 +1377,14 @@ static void __init gicv3_dt_init(void)
     struct rdist_region *rdist_regs;
     int res, i;
     const struct dt_device_node *node = gicv3_info.node;
+    u64 dt_dbase, dt_rdist_base, dt_rdist_size, dt_cbase, dt_csize, dt_vbase,
+    dt_vsize;
 
-    res = dt_device_get_address(node, 0, &dbase, NULL);
+    res = dt_device_get_address(node, 0, &dt_dbase, NULL);
+    if ( res )
+        panic("GICv3: Cannot find a valid distributor address\n");
+
+    res = translate_dt_address_size(&dt_dbase, NULL, &dbase, NULL);
     if ( res )
         panic("GICv3: Cannot find a valid distributor address\n");
 
@@ -1393,9 +1400,15 @@ 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, &dt_rdist_base,
+                                    &dt_rdist_size);
+        if ( res )
+            panic("GICv3: No rdist base found for region %d\n", i);
 
-        res = dt_device_get_address(node, 1 + i, &rdist_base, &rdist_size);
+        res = translate_dt_address_size(&dt_rdist_base, &dt_rdist_size,
+                                        &rdist_base, &rdist_size);
         if ( res )
             panic("GICv3: No rdist base found for region %d\n", i);
 
@@ -1418,10 +1431,20 @@ static void __init gicv3_dt_init(void)
      * provided.
      */
     res = dt_device_get_address(node, 1 + gicv3.rdist_count,
-                                &cbase, &csize);
+                                &dt_cbase, &dt_csize);
     if ( !res )
+    {
         dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
-                              &vbase, &vsize);
+                              &dt_vbase, &dt_vsize);
+
+        res = translate_dt_address_size(&dt_vbase, &dt_vsize, &vbase, &vsize);
+        if ( res )
+            panic("GICv3: Invalid vbase/vsize provided\n");
+    }
+
+    res = translate_dt_address_size(&dt_cbase, &dt_csize, &cbase, &csize);
+    if ( res )
+        panic("GICv3: Invalid cbase / csize provided\n");
 }
 
 static int gicv3_iomem_deny_access(struct domain *d)
diff --git a/xen/arch/arm/platforms/brcm.c b/xen/arch/arm/platforms/brcm.c
index d481b2c60f..3e208a060d 100644
--- a/xen/arch/arm/platforms/brcm.c
+++ b/xen/arch/arm/platforms/brcm.c
@@ -40,7 +40,8 @@ static __init int brcm_get_dt_node(char *compat_str,
                                    u32 *reg_base)
 {
     const struct dt_device_node *node;
-    u64 reg_base_64;
+    paddr_t reg_base_64;
+    u64 dt_reg_base_64;
     int rc;
 
     node = dt_find_compatible_node(NULL, NULL, compat_str);
@@ -50,13 +51,17 @@ static __init int brcm_get_dt_node(char *compat_str,
         return -ENOENT;
     }
 
-    rc = dt_device_get_address(node, 0, &reg_base_64, NULL);
+    rc = dt_device_get_address(node, 0, &dt_reg_base_64, NULL);
     if ( rc )
     {
         dprintk(XENLOG_ERR, "%s: missing \"reg\" prop\n", __func__);
         return rc;
     }
 
+    rc = translate_dt_address_size(&dt_reg_base_64, NULL, &reg_base_64, NULL);
+    if ( rc )
+        return rc;
+
     if ( dn )
         *dn = node;
 
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index 6560507092..a3fcab92ac 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -42,8 +42,9 @@ static int exynos5_init_time(void)
     void __iomem *mct;
     int rc;
     struct dt_device_node *node;
-    u64 mct_base_addr;
-    u64 size;
+    paddr_t mct_base_addr;
+    paddr_t size;
+    u64 dt_mct_base_addr, dt_size;
 
     node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-mct");
     if ( !node )
@@ -52,14 +53,19 @@ static int exynos5_init_time(void)
         return -ENXIO;
     }
 
-    rc = dt_device_get_address(node, 0, &mct_base_addr, &size);
+    rc = dt_device_get_address(node, 0, &dt_mct_base_addr, &dt_size);
     if ( rc )
     {
         dprintk(XENLOG_ERR, "Error in \"samsung,exynos4210-mct\"\n");
         return -ENXIO;
     }
 
-    dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
+    rc = translate_dt_address_size(&dt_mct_base_addr, &dt_size, &mct_base_addr,
+                                   &size);
+    if ( rc )
+        return rc;
+
+    dprintk(XENLOG_INFO, "mct_base_addr: 0x%"PRIpaddr" size: 0x%"PRIpaddr"\n",
             mct_base_addr, size);
 
     mct = ioremap_nocache(mct_base_addr, size);
@@ -97,8 +103,9 @@ static int __init exynos5_smp_init(void)
     struct dt_device_node *node;
     void __iomem *sysram;
     char *compatible;
-    u64 sysram_addr;
-    u64 size;
+    u64 dt_sysram_addr, dt_size;
+    paddr_t sysram_addr;
+    paddr_t size;
     u64 sysram_offset;
     int rc;
 
@@ -125,13 +132,18 @@ static int __init exynos5_smp_init(void)
         return -ENXIO;
     }
 
-    rc = dt_device_get_address(node, 0, &sysram_addr, &size);
+    rc = dt_device_get_address(node, 0, &dt_sysram_addr, &dt_size);
     if ( rc )
     {
         dprintk(XENLOG_ERR, "Error in %s\n", compatible);
         return -ENXIO;
     }
-    dprintk(XENLOG_INFO, "sysram_addr: %016llx size: %016llx offset: %016llx\n",
+
+    rc = translate_dt_address_size(&dt_sysram_addr, &dt_size, &sysram_addr, &size);
+    if ( rc )
+        return rc;
+
+    dprintk(XENLOG_INFO, "sysram_addr: 0x%"PRIpaddr" size: 0x%"PRIpaddr" offset: %016llx\n",
             sysram_addr, size, sysram_offset);
 
     sysram = ioremap_nocache(sysram_addr, size);
@@ -189,10 +201,11 @@ static int exynos5_cpu_power_up(void __iomem *power, int cpu)
     return 0;
 }
 
-static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size)
+static int exynos5_get_pmu_baseandsize(paddr_t *power_base_addr, paddr_t *size)
 {
     struct dt_device_node *node;
     int rc;
+    u64 dt_power_base_addr, dt_size;
     static const struct dt_device_match exynos_dt_pmu_matches[] =
     {
         DT_MATCH_COMPATIBLE("samsung,exynos5250-pmu"),
@@ -208,14 +221,19 @@ static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size)
         return -ENXIO;
     }
 
-    rc = dt_device_get_address(node, 0, power_base_addr, size);
+    rc = dt_device_get_address(node, 0, &dt_power_base_addr, &dt_size);
     if ( rc )
     {
         dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n");
         return -ENXIO;
     }
 
-    dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n",
+    rc = translate_dt_address_size(&dt_power_base_addr, &dt_size,
+                                   power_base_addr, size);
+    if ( rc )
+        return rc;
+
+    dprintk(XENLOG_DEBUG, "power_base_addr: 0x%"PRIpaddr" size: 0x%"PRIpaddr"\n",
             *power_base_addr, *size);
 
     return 0;
@@ -223,8 +241,8 @@ static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size)
 
 static int exynos5_cpu_up(int cpu)
 {
-    u64 power_base_addr;
-    u64 size;
+    paddr_t power_base_addr;
+    paddr_t size;
     void __iomem *power;
     int rc;
 
@@ -256,8 +274,8 @@ static int exynos5_cpu_up(int cpu)
 
 static void exynos5_reset(void)
 {
-    u64 power_base_addr;
-    u64 size;
+    paddr_t power_base_addr;
+    paddr_t size;
     void __iomem *pmu;
     int rc;
 
diff --git a/xen/arch/arm/platforms/sunxi.c b/xen/arch/arm/platforms/sunxi.c
index e8e4d88bef..7027170b68 100644
--- a/xen/arch/arm/platforms/sunxi.c
+++ b/xen/arch/arm/platforms/sunxi.c
@@ -34,6 +34,7 @@ static void __iomem *sunxi_map_watchdog(bool *new_wdt)
 {
     void __iomem *wdt;
     struct dt_device_node *node;
+    u64 dt_wdt_start, dt_wdt_len;
     paddr_t wdt_start, wdt_len;
     bool _new_wdt = false;
     int ret;
@@ -50,7 +51,15 @@ static void __iomem *sunxi_map_watchdog(bool *new_wdt)
         return NULL;
     }
 
-    ret = dt_device_get_address(node, 0, &wdt_start, &wdt_len);
+    ret = dt_device_get_address(node, 0, &dt_wdt_start, &dt_wdt_len);
+    if ( ret )
+    {
+        dprintk(XENLOG_ERR, "Cannot read watchdog register address\n");
+        return NULL;
+    }
+
+    ret = translate_dt_address_size(&dt_wdt_start, &dt_wdt_len, &wdt_start,
+                                    &wdt_len);
     if ( ret )
     {
         dprintk(XENLOG_ERR, "Cannot read watchdog register address\n");
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 1f26f67b90..da64aeec88 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -220,12 +220,19 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
 
     for ( i = first; i < nr ; i++ )
     {
+        u64 dt_r_s, dt_r_e;
         paddr_t r_s, r_e;
 
-        if ( fdt_get_mem_rsv(device_tree_flattened, i, &r_s, &r_e ) < 0 )
+        if ( fdt_get_mem_rsv(device_tree_flattened, i, &dt_r_s, &dt_r_e ) < 0 )
             /* If we can't read it, pretend it doesn't exist... */
             continue;
 
+        else
+        {
+            if ( translate_dt_address_size(&dt_r_s, &dt_r_e, &r_s, &r_e) )
+                panic("Invalid address or size provided\n");
+        }
+
         r_e += r_s; /* fdt_get_mem_rsv returns length */
 
         if ( s < r_e && r_s < e )
@@ -500,14 +507,21 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
 
     for ( ; i < mi->nr_mods + nr; i++ )
     {
+        u64 dt_mod_s, dt_mod_e;
         paddr_t mod_s, mod_e;
 
         if ( fdt_get_mem_rsv(device_tree_flattened,
                              i - mi->nr_mods,
-                             &mod_s, &mod_e ) < 0 )
+                             &dt_mod_s, &dt_mod_e ) < 0 )
             /* If we can't read it, pretend it doesn't exist... */
             continue;
 
+        else
+        {
+            if ( translate_dt_address_size(&dt_mod_s, &dt_mod_e, &mod_s, &mod_e) )
+                panic("Invalid address or size provided\n");
+        }
+
         /* fdt_get_mem_rsv returns length */
         mod_e += mod_s;
 
diff --git a/xen/drivers/char/exynos4210-uart.c b/xen/drivers/char/exynos4210-uart.c
index 43aaf02e18..38cc19410f 100644
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -26,6 +26,7 @@
 #include <asm/device.h>
 #include <asm/exynos4210-uart.h>
 #include <asm/io.h>
+#include <asm/platform.h>
 
 static struct exynos4210_uart {
     unsigned int baud, clock_hz, data_bits, parity, stop_bits;
@@ -303,7 +304,8 @@ static int __init exynos4210_uart_init(struct dt_device_node *dev,
     const char *config = data;
     struct exynos4210_uart *uart;
     int res;
-    u64 addr, size;
+    paddr_t addr, size;
+    u64 dt_addr, dt_size;
 
     if ( strcmp(config, "") )
         printk("WARNING: UART configuration is not supported\n");
@@ -316,7 +318,7 @@ static int __init exynos4210_uart_init(struct dt_device_node *dev,
     uart->parity    = PARITY_NONE;
     uart->stop_bits = 1;
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_address(dev, 0, &dt_addr, &dt_size);
     if ( res )
     {
         printk("exynos4210: Unable to retrieve the base"
@@ -324,6 +326,10 @@ static int __init exynos4210_uart_init(struct dt_device_node *dev,
         return res;
     }
 
+    res = translate_dt_address_size(&dt_addr, &dt_size, &addr, &size);
+    if ( res )
+        return res;
+
     res = platform_get_irq(dev, 0);
     if ( res < 0 )
     {
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 58d0ccd889..e62362db2f 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -35,6 +35,7 @@
 #include <asm/io.h>
 #ifdef CONFIG_HAS_DEVICE_TREE
 #include <asm/device.h>
+#include <asm/platform.h>
 #endif
 #ifdef CONFIG_X86
 #include <asm/fixmap.h>
@@ -42,8 +43,8 @@
 
 static struct ns16550 {
     int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
-    u64 io_base;   /* I/O port or memory-mapped I/O address. */
-    u64 io_size;
+    paddr_t io_base;   /* I/O port or memory-mapped I/O address. */
+    paddr_t io_size;
     int reg_shift; /* Bits to shift register offset by */
     int reg_width; /* Size of access to use, the registers
                     * themselves are still bytes */
@@ -1747,6 +1748,7 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
     struct ns16550 *uart;
     int res;
     u32 reg_shift, reg_width;
+    u64 dt_io_base, dt_io_size;
 
     uart = &ns16550_com[0];
 
@@ -1757,7 +1759,12 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
     uart->parity    = UART_PARITY_NONE;
     uart->stop_bits = 1;
 
-    res = dt_device_get_address(dev, 0, &uart->io_base, &uart->io_size);
+    res = dt_device_get_address(dev, 0, &dt_io_base, &dt_io_size);
+    if ( res )
+        return res;
+
+    res = translate_dt_address_size(&dt_io_base, &dt_io_size, &uart->io_base,
+                                    &uart->io_size);
     if ( res )
         return res;
 
diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
index d6a5d59aa2..e606c3ef1e 100644
--- a/xen/drivers/char/omap-uart.c
+++ b/xen/drivers/char/omap-uart.c
@@ -16,6 +16,7 @@
 #include <xen/irq.h>
 #include <xen/device_tree.h>
 #include <asm/device.h>
+#include <asm/platform.h>
 #include <xen/errno.h>
 #include <xen/mm.h>
 #include <xen/vmap.h>
@@ -324,7 +325,8 @@ static int __init omap_uart_init(struct dt_device_node *dev,
     struct omap_uart *uart;
     u32 clkspec;
     int res;
-    u64 addr, size;
+    u64 dt_addr, dt_size;
+    paddr_t addr, size;
 
     if ( strcmp(config, "") )
         printk("WARNING: UART configuration is not supported\n");
@@ -344,7 +346,7 @@ static int __init omap_uart_init(struct dt_device_node *dev,
     uart->parity = UART_PARITY_NONE;
     uart->stop_bits = 1;
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_address(dev, 0, &dt_addr, &dt_size);
     if ( res )
     {
         printk("omap-uart: Unable to retrieve the base"
@@ -352,6 +354,10 @@ static int __init omap_uart_init(struct dt_device_node *dev,
         return res;
     }
 
+    res = translate_dt_address_size(&dt_addr, &dt_size, &addr, &size);
+    if ( res )
+        return res;
+
     res = platform_get_irq(dev, 0);
     if ( res < 0 )
     {
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index be67242bc0..23037405e1 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -24,6 +24,7 @@
 #include <xen/device_tree.h>
 #include <xen/errno.h>
 #include <asm/device.h>
+#include <asm/platform.h>
 #include <xen/mm.h>
 #include <xen/vmap.h>
 #include <asm/pl011-uart.h>
@@ -258,14 +259,15 @@ 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;
+    u64 dt_addr, dt_size;
 
     if ( strcmp(config, "") )
     {
         printk("WARNING: UART configuration is not supported\n");
     }
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_address(dev, 0, &dt_addr, &dt_size);
     if ( res )
     {
         printk("pl011: Unable to retrieve the base"
@@ -273,6 +275,10 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
         return res;
     }
 
+    res = translate_dt_address_size(&dt_addr, &dt_size, &addr, &size);
+    if ( res )
+        return res;
+
     res = platform_get_irq(dev, 0);
     if ( res < 0 )
     {
diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index 2fccafe340..c16cac836a 100644
--- a/xen/drivers/char/scif-uart.c
+++ b/xen/drivers/char/scif-uart.c
@@ -28,6 +28,7 @@
 #include <asm/device.h>
 #include <asm/scif-uart.h>
 #include <asm/io.h>
+#include <asm/platform.h>
 
 #define scif_readb(uart, off)          readb((uart)->regs + (off))
 #define scif_writeb(uart, off, val)    writeb((val), (uart)->regs + (off))
@@ -311,14 +312,15 @@ static int __init scif_uart_init(struct dt_device_node *dev,
     const char *config = data;
     struct scif_uart *uart;
     int res;
-    u64 addr, size;
+    paddr_t addr, size;
+    u64 dt_addr, dt_size;
 
     if ( strcmp(config, "") )
         printk("WARNING: UART configuration is not supported\n");
 
     uart = &scif_com;
 
-    res = dt_device_get_address(dev, 0, &addr, &size);
+    res = dt_device_get_address(dev, 0, &dt_addr, &dt_size);
     if ( res )
     {
         printk("scif-uart: Unable to retrieve the base"
@@ -326,6 +328,10 @@ static int __init scif_uart_init(struct dt_device_node *dev,
         return res;
     }
 
+    res = translate_dt_address_size(&dt_addr, &dt_size, &addr, &size);
+    if ( res )
+        return res;
+
     res = platform_get_irq(dev, 0);
     if ( res < 0 )
     {
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 0a514821b3..5ae180a4cc 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;
 };
 
@@ -96,12 +96,17 @@ static struct resource *platform_get_resource(struct platform_device *pdev,
 	 */
 	static struct resource res;
 	int ret = 0;
+	u64 dt_addr, dt_size;
 
 	res.type = type;
 
 	switch (type) {
 	case IORESOURCE_MEM:
-		ret = dt_device_get_address(pdev, num, &res.addr, &res.size);
+		ret = dt_device_get_address(pdev, num, &dt_addr, &dt_size);
+
+        if ( !ret )
+            ret = translate_dt_address_size(&dt_addr, &dt_size, &res.addr,
+                                            &res.size);
 
 		return ((ret) ? NULL : &res);
 
@@ -169,7 +174,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);
 	}
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index f0aff7ea76..0b326e22fd 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -34,7 +34,7 @@ enum serial_port_state {
 
 struct vuart_info {
     paddr_t base_addr;          /* Base address of the UART */
-    unsigned long size;         /* Size of the memory region */
+    paddr_t size;               /* Size of the memory region */
     unsigned long data_off;     /* Data register offset */
     unsigned long status_off;   /* Status register offset */
     unsigned long status;       /* Ready status value */
-- 
2.17.1

[XEN v1 5/9] xen/arm: Use 'PRIpaddr' to display 'paddr_t' variable

[Ayan Kumar Halder]

One should use 'PRIpaddr' to display 'paddr_t' variables.
This may either be PRIx32 or PRIx64 depending of the type of architecture.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/domain_build.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1bb97cd337..c537514a52 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1325,7 +1325,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;
@@ -1393,7 +1393,7 @@ static int __init make_shm_memory_node(const struct domain *d,
         __be32 *cells;
         unsigned int len = (addrcells + sizecells) * sizeof(__be32);
 
-        snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start);
+        snprintf(buf, sizeof(buf), "xen-shmem@%"PRIpaddr, mem->bank[i].start);
         res = fdt_begin_node(fdt, buf);
         if ( res )
             return res;
@@ -2739,7 +2739,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 )
@@ -2795,7 +2795,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);
@@ -2881,7 +2881,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;
-- 
2.17.1

[XEN v1 6/9] xen/arm: Use 'u64' to represent 'unsigned long long'

[Ayan Kumar Halder]

bankbase, banksize and bankend are used to hold values of type
'unsigned long long'. Thus, one should use 'u64' instead of 'paddr_t'
(which may be either u64 or u32 depending on the architecture).

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/domain_build.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c537514a52..e968b9812d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1741,9 +1741,9 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
                                   struct meminfo *ext_regions)
 {
     unsigned int i;
-    paddr_t bankend;
-    const paddr_t bankbase[] = GUEST_RAM_BANK_BASES;
-    const paddr_t banksize[] = GUEST_RAM_BANK_SIZES;
+    uint64_t bankend;
+    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
+    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
     int res = -ENOENT;
 
     for ( i = 0; i < GUEST_RAM_BANKS; i++ )
-- 
2.17.1

Re: [XEN v1 6/9] xen/arm: Use 'u64' to represent 'unsigned long long'

[Julien Grall]

Hi,

Title: Please be more specific about the component you are modifying.

I.e:

xen/arm: domain_build: Replace use of paddr_t in find_domU_holes()

So it is clear this is no a tree-wide change.

On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> bankbase, banksize and bankend are used to hold values of type
> 'unsigned long long'. Thus, one should use 'u64' instead of 'paddr_t'

To me it sounds like the code should use 'unsigned long long' rather 
than 'paddr_t' or 'u64'.

Also, please replace any reference of u64 to uint64_t. (The same remark 
apply for u32 below).

> (which may be either u64 or u32 depending on the architecture).

That's not yet correct. So please add something like 'in the future'...

> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>   xen/arch/arm/domain_build.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c537514a52..e968b9812d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1741,9 +1741,9 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>                                     struct meminfo *ext_regions)
>   {
>       unsigned int i;
> -    paddr_t bankend;
> -    const paddr_t bankbase[] = GUEST_RAM_BANK_BASES;
> -    const paddr_t banksize[] = GUEST_RAM_BANK_SIZES;
> +    uint64_t bankend;
> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
>       int res = -ENOENT;
>   
>       for ( i = 0; i < GUEST_RAM_BANKS; i++ )

Cheers,

-- 
Julien Grall

[XEN v1 7/9] xen/arm: Restrict zeroeth_table_offset for ARM_64

[Ayan Kumar Halder]

zeroeth_table_offset is not accessed for ARM_32.
This is a left over of the following commit

"
commit 5fa6e9abfb11
Author: Ian Campbell <ian.campbell@citrix.com>
Date:   Thu Sep 18 01:09:48 2014 +0100

    xen: arm: Implement variable levels in dump_pt_walk
"

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/include/asm/lpae.h | 10 ++++++++++
 xen/arch/arm/mm.c               |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/xen/arch/arm/include/asm/lpae.h b/xen/arch/arm/include/asm/lpae.h
index 3fdd5d0de2..35769debf9 100644
--- a/xen/arch/arm/include/asm/lpae.h
+++ b/xen/arch/arm/include/asm/lpae.h
@@ -161,6 +161,7 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
 #define lpae_set_mfn(pte, mfn)  ((pte).walk.base = mfn_x(mfn))
 
 /* Generate an array @var containing the offset for each level from @addr */
+#ifdef CONFIG_ARM_64
 #define DECLARE_OFFSETS(var, addr)          \
     const unsigned int var[4] = {           \
         zeroeth_table_offset(addr),         \
@@ -168,6 +169,15 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
         second_table_offset(addr),          \
         third_table_offset(addr)            \
     }
+#else
+#define DECLARE_OFFSETS(var, addr)          \
+    const unsigned int var[4] = {           \
+        0,                                  \
+        first_table_offset(addr),           \
+        second_table_offset(addr),          \
+        third_table_offset(addr)            \
+    }
+#endif
 
 /*
  * Standard entry type that we'll use to build Xen's own pagetables.
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 630175276f..be939fb106 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -184,7 +184,11 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
     static const char *level_strs[4] = { "0TH", "1ST", "2ND", "3RD" };
     const mfn_t root_mfn = maddr_to_mfn(ttbr);
     const unsigned int offsets[4] = {
+#ifdef CONFIG_ARM_64
         zeroeth_table_offset(addr),
+#else
+        0,
+#endif
         first_table_offset(addr),
         second_table_offset(addr),
         third_table_offset(addr)
-- 
2.17.1

Re: [XEN v1 7/9] xen/arm: Restrict zeroeth_table_offset for ARM_64

[Julien Grall]

Hi,

On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> zeroeth_table_offset is not accessed for ARM_32.

Right, but what is the problem with keep it? With your proposal we need 
to duplicate the macro DECLARE_OFFSETS() which is not great.

So you want to provide a more compelling reason to have the duplication.

> This is a left over of the following commit

I am not sure why you are saying this is a left-over.

DECLARE_OFFSETS was introduced in 2019 so...

> 
> "
> commit 5fa6e9abfb11
> Author: Ian Campbell <ian.campbell@citrix.com>
> Date:   Thu Sep 18 01:09:48 2014 +0100


... 5 years after there.

> 
>      xen: arm: Implement variable levels in dump_pt_walk
> "
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>   xen/arch/arm/include/asm/lpae.h | 10 ++++++++++
>   xen/arch/arm/mm.c               |  4 ++++
>   2 files changed, 14 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/lpae.h b/xen/arch/arm/include/asm/lpae.h
> index 3fdd5d0de2..35769debf9 100644
> --- a/xen/arch/arm/include/asm/lpae.h
> +++ b/xen/arch/arm/include/asm/lpae.h
> @@ -161,6 +161,7 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
>   #define lpae_set_mfn(pte, mfn)  ((pte).walk.base = mfn_x(mfn))
>   
>   /* Generate an array @var containing the offset for each level from @addr */
> +#ifdef CONFIG_ARM_64
>   #define DECLARE_OFFSETS(var, addr)          \
>       const unsigned int var[4] = {           \
>           zeroeth_table_offset(addr),         \
> @@ -168,6 +169,15 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
>           second_table_offset(addr),          \
>           third_table_offset(addr)            \
>       }
> +#else
> +#define DECLARE_OFFSETS(var, addr)          \
> +    const unsigned int var[4] = {           \
> +        0,                                  \
> +        first_table_offset(addr),           \
> +        second_table_offset(addr),          \
> +        third_table_offset(addr)            \
> +    }
> +#endif
>   
>   /*
>    * Standard entry type that we'll use to build Xen's own pagetables.
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 630175276f..be939fb106 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -184,7 +184,11 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
>       static const char *level_strs[4] = { "0TH", "1ST", "2ND", "3RD" };
>       const mfn_t root_mfn = maddr_to_mfn(ttbr);
>       const unsigned int offsets[4] = {
> +#ifdef CONFIG_ARM_64
>           zeroeth_table_offset(addr),
> +#else
> +        0,
> +#endif
>           first_table_offset(addr),
>           second_table_offset(addr),
>           third_table_offset(addr)

Please use DECLARE_OFFSETS() here.

Cheers,

-- 
Julien Grall

[XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr

[Ayan Kumar Halder]

1. Supersections are supported only for paddr greater than 32 bits.
2. Use 0 wherever physical addresses are right shifted for 32 bits.
3. Use PRIx64 to print u64

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/guest_walk.c          | 2 ++
 xen/arch/arm/mm.c                  | 2 +-
 xen/drivers/passthrough/arm/smmu.c | 5 +++++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 43d3215304..4384068285 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -149,6 +149,7 @@ static bool guest_walk_sd(const struct vcpu *v,
             mask = (1ULL << L1DESC_SECTION_SHIFT) - 1;
             *ipa = ((paddr_t)pte.sec.base << L1DESC_SECTION_SHIFT) | (gva & mask);
         }
+#ifndef CONFIG_ARM_PA_32
         else /* Supersection */
         {
             mask = (1ULL << L1DESC_SUPERSECTION_SHIFT) - 1;
@@ -157,6 +158,7 @@ static bool guest_walk_sd(const struct vcpu *v,
             *ipa |= (paddr_t)(pte.supersec.extbase1) << L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
             *ipa |= (paddr_t)(pte.supersec.extbase2) << L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
         }
+#endif
 
         /* Set permissions so that the caller can check the flags by herself. */
         if ( !pte.sec.ro )
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index be939fb106..3bc9894008 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -229,7 +229,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
 
         pte = mapping[offsets[level]];
 
-        printk("%s[0x%03x] = 0x%"PRIpaddr"\n",
+        printk("%s[0x%03x] = 0x%"PRIx64"\n",
                level_strs[level], offsets[level], pte.bits);
 
         if ( level == 3 || !pte.walk.valid || !pte.walk.table )
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 5ae180a4cc..522a478ccf 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1184,7 +1184,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);
+
+#ifndef CONFIG_ARM_PA_32
 	reg = (p2maddr >> 32);
+#else
+	reg = 0;
+#endif
 	if (stage1)
 		reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
 	writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
-- 
2.17.1

Re: [XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr

[Julien Grall]

Hi,

Each adaptations are distinct, so I would prefer if they are in in 
separate patch.

This will also make clear which components you touched because I would 
be surprised if this is really the only place where we need adaptation. 
Maybe that's because you didn't compile everything (which is fine).

On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> 1. Supersections are supported only for paddr greater than 32 bits.

Two questions:
  * Can you outline why we can't keep the code around?
  * Can you give a pointer to the Arm Arm that says supersection is not 
supported?

> 2. Use 0 wherever physical addresses are right shifted for 32 bit > 3. Use PRIx64 to print u64
It would be good to explain that the current use was buggy because we 
are printing a PTE and not a physical address.

> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>   xen/arch/arm/guest_walk.c          | 2 ++
>   xen/arch/arm/mm.c                  | 2 +-
>   xen/drivers/passthrough/arm/smmu.c | 5 +++++
>   3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index 43d3215304..4384068285 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -149,6 +149,7 @@ static bool guest_walk_sd(const struct vcpu *v,
>               mask = (1ULL << L1DESC_SECTION_SHIFT) - 1;
>               *ipa = ((paddr_t)pte.sec.base << L1DESC_SECTION_SHIFT) | (gva & mask);
>           }
> +#ifndef CONFIG_ARM_PA_32

A "malicious" guest can still set that bit. So now, you will return from 
guest_walk_sd() with 'ipa' not set at all.

If this is effectively not supported, then we should return 'false' 
rather than claiming the translation was successful.

>           else /* Supersection */
>           {
>               mask = (1ULL << L1DESC_SUPERSECTION_SHIFT) - 1;
> @@ -157,6 +158,7 @@ static bool guest_walk_sd(const struct vcpu *v,
>               *ipa |= (paddr_t)(pte.supersec.extbase1) << L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
>               *ipa |= (paddr_t)(pte.supersec.extbase2) << L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
>           }
> +#endif
>   
>           /* Set permissions so that the caller can check the flags by herself. */
>           if ( !pte.sec.ro )
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index be939fb106..3bc9894008 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -229,7 +229,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
>   
>           pte = mapping[offsets[level]];
>   
> -        printk("%s[0x%03x] = 0x%"PRIpaddr"\n",
> +        printk("%s[0x%03x] = 0x%"PRIx64"\n",
>                  level_strs[level], offsets[level], pte.bits);
>   
>           if ( level == 3 || !pte.walk.valid || !pte.walk.table )
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 5ae180a4cc..522a478ccf 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -1184,7 +1184,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);
> +
> +#ifndef CONFIG_ARM_PA_32
>   	reg = (p2maddr >> 32);
> +#else
> +	reg = 0;
> +#endif

I think it would be better if we implement writeq(). This will also 
avoid the now strange ((1ULL << 32) - 1) when p2maddr is a paddr_t.

>   	if (stage1)
>   		reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
>   	writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);

Cheers,

-- 
Julien Grall

Re: [XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr

[Ayan Kumar Halder]

On 16/12/2022 10:23, Julien Grall wrote:
> Hi,
Hi Julien,
>
> Each adaptations are distinct, so I would prefer if they are in in 
> separate patch.
>
> This will also make clear which components you touched because I would 
> be surprised if this is really the only place where we need 
> adaptation. Maybe that's because you didn't compile everything (which 
> is fine).
>
> On 15/12/2022 19:32, Ayan Kumar Halder wrote:
>> 1. Supersections are supported only for paddr greater than 32 bits.
>
> Two questions:
>  * Can you outline why we can't keep the code around?

For PA_32, the following bitoperation will overflow :-

             *ipa |= (paddr_t)(pte.supersec.extbase1) << 
L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
             *ipa |= (paddr_t)(pte.supersec.extbase2) << 
L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;

Also, pte.supersec.extbase1 and pte.supersec.extbase2 are not valid for 
PA_32. Refer xen/arch/arm/include/asm/short-desc.h

unsigned int extbase2:4;    /* Extended base address, PA[39:36] */

unsigned int extbase1:4;    /* Extended base address, PA[35:32] */

>  * Can you give a pointer to the Arm Arm that says supersection is not 
> supported?

I could not find any sentence in Arm Arm which says supersection is 
**not** supported on 32 bit PA.

However,

Refer"ARM DDI 0487I.a ID081822", G5.4 "The VMSAv8-32 Short-descriptor 
translation table format", G5-9163

"Support for Supersections is **optional**, except that an 
implementation that supports more than 32 bits of PA must also support 
Supersections to provide access to the entire PA space."

Also, G5.1.3 "Address spaces in VMSAv8-32", G5-9149

"AArch32 defines two translation table formats. The Long-descriptor 
format gives access to the full 40-bit IPA or PA space at a granularity 
of 4KB. The Short-descriptor format:
    Gives access to a 32-bit PA space at 4KB granularity.
    Gives access to a 40-bit PA space, but only at 16MB granularity, by 
the use of Supersections."

from the above 2 snippets, I inferred that supersections are only 
supported for PAs greater than 32 bits.
I could not find any evidence of supersections supported for 32 bit PA.

- Ayan

>
>> 2. Use 0 wherever physical addresses are right shifted for 32 bit > 
>> 3. Use PRIx64 to print u64
> It would be good to explain that the current use was buggy because we 
> are printing a PTE and not a physical address.
>
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>>   xen/arch/arm/guest_walk.c          | 2 ++
>>   xen/arch/arm/mm.c                  | 2 +-
>>   xen/drivers/passthrough/arm/smmu.c | 5 +++++
>>   3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
>> index 43d3215304..4384068285 100644
>> --- a/xen/arch/arm/guest_walk.c
>> +++ b/xen/arch/arm/guest_walk.c
>> @@ -149,6 +149,7 @@ static bool guest_walk_sd(const struct vcpu *v,
>>               mask = (1ULL << L1DESC_SECTION_SHIFT) - 1;
>>               *ipa = ((paddr_t)pte.sec.base << L1DESC_SECTION_SHIFT) 
>> | (gva & mask);
>>           }
>> +#ifndef CONFIG_ARM_PA_32
>
> A "malicious" guest can still set that bit. So now, you will return 
> from guest_walk_sd() with 'ipa' not set at all.
>
> If this is effectively not supported, then we should return 'false' 
> rather than claiming the translation was successful.
>
>>           else /* Supersection */
>>           {
>>               mask = (1ULL << L1DESC_SUPERSECTION_SHIFT) - 1;
>> @@ -157,6 +158,7 @@ static bool guest_walk_sd(const struct vcpu *v,
>>               *ipa |= (paddr_t)(pte.supersec.extbase1) << 
>> L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
>>               *ipa |= (paddr_t)(pte.supersec.extbase2) << 
>> L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
>>           }
>> +#endif
>>             /* Set permissions so that the caller can check the flags 
>> by herself. */
>>           if ( !pte.sec.ro )
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index be939fb106..3bc9894008 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -229,7 +229,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
>>             pte = mapping[offsets[level]];
>>   -        printk("%s[0x%03x] = 0x%"PRIpaddr"\n",
>> +        printk("%s[0x%03x] = 0x%"PRIx64"\n",
>>                  level_strs[level], offsets[level], pte.bits);
>>             if ( level == 3 || !pte.walk.valid || !pte.walk.table )
>> diff --git a/xen/drivers/passthrough/arm/smmu.c 
>> b/xen/drivers/passthrough/arm/smmu.c
>> index 5ae180a4cc..522a478ccf 100644
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -1184,7 +1184,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);
>> +
>> +#ifndef CONFIG_ARM_PA_32
>>       reg = (p2maddr >> 32);
>> +#else
>> +    reg = 0;
>> +#endif
>
> I think it would be better if we implement writeq(). This will also 
> avoid the now strange ((1ULL << 32) - 1) when p2maddr is a paddr_t.
>
>>       if (stage1)
>>           reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
>>       writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
>
> Cheers,
>

Re: [XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr

[Julien Grall]

Hi,

On 20/12/2022 15:24, Ayan Kumar Halder wrote:
> On 16/12/2022 10:23, Julien Grall wrote:
>> Each adaptations are distinct, so I would prefer if they are in in 
>> separate patch.
>>
>> This will also make clear which components you touched because I would 
>> be surprised if this is really the only place where we need 
>> adaptation. Maybe that's because you didn't compile everything (which 
>> is fine).
>>
>> On 15/12/2022 19:32, Ayan Kumar Halder wrote:
>>> 1. Supersections are supported only for paddr greater than 32 bits.
>>
>> Two questions:
>>  * Can you outline why we can't keep the code around?
> 
> For PA_32, the following bitoperation will overflow :-
> 
>              *ipa |= (paddr_t)(pte.supersec.extbase1) << 
> L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
>              *ipa |= (paddr_t)(pte.supersec.extbase2) << 
> L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
> 
> Also, pte.supersec.extbase1 and pte.supersec.extbase2 are not valid for 
> PA_32. Refer xen/arch/arm/include/asm/short-desc.h

You are right about extbase1 and extbase2. See below for the 
supersection support.

> 
> unsigned int extbase2:4;    /* Extended base address, PA[39:36] */
> 
> unsigned int extbase1:4;    /* Extended base address, PA[35:32] */
> 
>>  * Can you give a pointer to the Arm Arm that says supersection is not 
>> supported?
> 
> I could not find any sentence in Arm Arm which says supersection is 
> **not** supported on 32 bit PA.
> 
> However,
> 
> Refer"ARM DDI 0487I.a ID081822", G5.4 "The VMSAv8-32 Short-descriptor 
> translation table format", G5-9163
> 
> "Support for Supersections is **optional**, except that an 
> implementation that supports more than 32 bits of PA must also support 
> Supersections to provide access to the entire PA space."
> 
> Also, G5.1.3 "Address spaces in VMSAv8-32", G5-9149
> 
> "AArch32 defines two translation table formats. The Long-descriptor 
> format gives access to the full 40-bit IPA or PA space at a granularity 
> of 4KB. The Short-descriptor format:
>     Gives access to a 32-bit PA space at 4KB granularity.
>     Gives access to a 40-bit PA space, but only at 16MB granularity, by 
> the use of Supersections."
> 
> from the above 2 snippets, I inferred that supersections are only 
> supported for PAs greater than 32 bits.
> I could not find any evidence of supersections supported for 32 bit PA.

 From what you quoted above supersection is optional unless the 
processor support more than 32-bit PA. IOW, an implementer is free to 
implement the feature even if it is not strictly necessary when the 
processor only supports 32-bit PA. This because it is useful to reduce 
the TLB contention.

So if we want to #ifdef some code, then only the two lines using 
L1DESC_SUPERSECTION_EXT_BASE{1, 2}_SHIFT should be protected. The rest 
should stay as-is.

Cheers,

-- 
Julien Grall

[XEN v1 9/9] xen/arm: Introduce ARM_PA_32 to support 32 bit physical address

[Ayan Kumar Halder]

We have introduced a new config option to support 32 bit physical address.
By default, it is disabled.
ARM_PA_32 cannot be enabled on ARM_64 as the memory management unit works
on 48bit physical addresses.
On ARM_32, it can be used on systems where large page address extension is
not supported.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/Kconfig                 | 9 +++++++++
 xen/arch/arm/include/asm/page-bits.h | 2 ++
 xen/arch/arm/include/asm/types.h     | 7 +++++++
 3 files changed, 18 insertions(+)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 239d3aed3c..aeb0f7252e 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -39,6 +39,15 @@ config ACPI
 config ARM_EFI
 	bool
 
+config ARM_PA_32
+	bool "32 bit Physical Address"
+	depends on ARM_32
+	default n
+	---help---
+
+	  Support 32 bit physical addresses.
+	  If unsure, say N
+
 config GICV3
 	bool "GICv3 driver"
 	depends on !NEW_VGIC
diff --git a/xen/arch/arm/include/asm/page-bits.h b/xen/arch/arm/include/asm/page-bits.h
index 5d6477e599..8f4dcebcfd 100644
--- a/xen/arch/arm/include/asm/page-bits.h
+++ b/xen/arch/arm/include/asm/page-bits.h
@@ -5,6 +5,8 @@
 
 #ifdef CONFIG_ARM_64
 #define PADDR_BITS              48
+#elif CONFIG_ARM_PA_32
+#define PADDR_BITS              32
 #else
 #define PADDR_BITS              40
 #endif
diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h
index 083acbd151..f9595b9098 100644
--- a/xen/arch/arm/include/asm/types.h
+++ b/xen/arch/arm/include/asm/types.h
@@ -37,9 +37,16 @@ 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 INVALID_PADDR (~0UL)
+#define PADDR_SHIFT BITS_PER_LONG
+#define PRIpaddr PRIx32
+#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)
-- 
2.17.1