Re: [XEN v2 04/11] xen/arm: Typecast the DT values into paddr_t

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 20/01/2023 10:16, Julien Grall wrote:
> Hi,
Hi Julien/Stefano,
> I am answering to multiple e-mails at the same time.
>
> On 19/01/2023 23:34, Stefano Stabellini wrote:
> > On Thu, 19 Jan 2023, Stefano Stabellini wrote:
> > > On Tue, 17 Jan 2023, Ayan Kumar Halder wrote:
> > > > In future, we wish to support 32 bit physical address.
> > > > However, one can only read u64 values from the DT. Thus, we need to
> A cell in the DT is a 32-bit value. So you could read 32-bit value 
> (address-size could be #1). It is just that our wrapper return 64-bit 
> values because this is how we use the most.
ok
> > > > typecast the values appropriately from u64 to paddr_t.
> C will perfectly be able to truncate a 64-bit to 32-bit value. So this 
> is not a very good argument to explain why all of this is necessary.
I can remove this line from the commit message. However, I have a 
related point below...
> > > > device_tree_get_reg() should now be able to return paddr_t. This is
> > > > invoked by various callers to get DT address and size.
> > > > Similarly, dt_read_number() is invoked as well to get DT address and
> > > > size. The return value is typecasted to paddr_t.
> > > > fdt_get_mem_rsv() can only accept u64 values. So, we provide a warpper
> Typo: s/warpper/wrapper/
ok
> > > > for this called fdt_get_mem_rsv_paddr() which will do the necessary
> > > > typecasting before invoking fdt_get_mem_rsv() and while returning.
> > > >
> > > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > > I know we discussed this before and you implemented exactly what we
> > > suggested, but now looking at this patch I think we should do the
> > > following:
> > >
> > > - also add a wrapper for dt_read_number, something like
> > >    dt_read_number_paddr that returns paddr
> "number" and "paddr" pretty much means the same. I think it would be 
> better to name it "dt_read_paddr".
ok
> > > - add a check for the top 32-bit being zero in all the wrappers
> > >    (dt_read_number_paddr, device_tree_get_reg, fdt_get_mem_rsv_paddr)
> > >    when paddr!=uint64_t. In case the top 32-bit are != zero I think we
> > >    should print an error
> > >
> > > Julien, I remember you were concerned about BUG_ON/panic/ASSERT and I
> > > agree with you there (especially considering Vikram's device tree
> > > overlay series). So here I am only suggesting to check truncation and
> > > printk a message, not panic. Would you be OK with that?
> Aside dt_read_number(), I would expect that most of the helper can 
> return an error. So if you want to check the truncation, then we 
> should propagate the error.
ok
> > > Last comment, maybe we could add fdt_get_mem_rsv_paddr to setup.h
> > > instead of introducing xen/arch/arm/include/asm/device_tree.h, given
> > > that we already have device tree definitions there
> > > (device_tree_get_reg). I am OK either way.
> >   Actually I noticed you also add dt_device_get_paddr to
> > xen/arch/arm/include/asm/device_tree.h. So it sounds like it is a good
> > idea to introduce xen/arch/arm/include/asm/device_tree.h, and we could
> > also move the declarations of device_tree_get_reg, device_tree_get_u32
> > there.
> None of the helpers you mention sounds arm specific. So why should the 
> be move an arch specific headers?
The functions (fdt_get_mem_rsv_paddr(), device_tree_get_reg(), 
device_tree_get_u32()) are currently used by Arm specific code only.
So, I thought of implementing fdt_get_mem_rsv_paddr() in 
xen/arch/arm/include/asm/device_tree.h.
However, I see your point as well. So the alternative approach would be :-

Approach 1) Implement fdt_get_mem_rsv_paddr() in 
./xen/include/xen/libfdt/libfdt.h.
However libfdt is imported from external sources, so I am not sure if 
this is the  best approach.
Approach 2) Rename fdt_get_mem_rsv_paddr() to dt_get_mem_rsv_paddr() and 
implement it in ./xen/include/xen/device_tree.h.
However, this will be an odd one as it invokes fdt_get_mem_rsv() for 
which we will have to include libfdt.h in device_tree.h.

So, I am biased towards having xen/arch/arm/include/asm/device_tree.h in 
which we can implement all the non-static fdt and dt functions (which 
are required by xen/arch/arm/*).
And then (as Stefano suggested), we can move the definitions of the 
following to xen/arch/arm/include/asm/device_tree.h.
device_tree_get_reg()

device_tree_get_u32()

device_tree_for_each_node()


Please let me know your thoughts.

> [...]
>
> > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > > > index 0085c28d74..f536a3f3ab 100644
> > > > --- a/xen/arch/arm/bootfdt.c
> > > > +++ b/xen/arch/arm/bootfdt.c
> > > > @@ -11,9 +11,9 @@
> > > >   #include <xen/efi.h>
> > > >   #include <xen/device_tree.h>
> > > >   #include <xen/lib.h>
> > > > -#include <xen/libfdt/libfdt.h>
> > > >   #include <xen/sort.h>
> > > >   #include <xsm/xsm.h>
> > > > +#include <asm/device_tree.h>
> > > >   #include <asm/setup.h>
> > > >     static bool __init device_tree_node_matches(const void *fdt, 
> > > > int node,
> > > > @@ -53,10 +53,15 @@ static bool __init 
> > > > device_tree_node_compatible(const void *fdt, int node,
> > > >   }
> > > >     void __init device_tree_get_reg(const __be32 **cell, u32 
> > > > address_cells,
> > > > -                                u32 size_cells, u64 *start, u64 
> > > > *size)
> > > > +                                u32 size_cells, paddr_t *start, 
> > > > paddr_t *size)
> > > >   {
> > > > -    *start = dt_next_cell(address_cells, cell);
> > > > -    *size = dt_next_cell(size_cells, cell);
> > > > +    /*
> > > > +     * dt_next_cell will return u64 whereas paddr_t may be u64 or 
> > > > u32. Thus, one
> > > > +     * needs to cast paddr_t to u32. Note that we do not check for 
> > > > truncation as
> > > > +     * it is user's responsibility to provide the correct values 
> > > > in the DT.
> > > > +     */
> > > > +    *start = (paddr_t) dt_next_cell(address_cells, cell);
> > > > +    *size = (paddr_t) dt_next_cell(size_cells, cell);
> There is no need for explicit cast here.
Should we have it for the sake of documentation (that it casts u64 to 
paddr_t) ?
- Ayan

> Cheers,