On 19/04/2023 16:18, Julien Grall wrote:
Hi Ayan,
Hi Julien,
On 19/04/2023 15:58, Ayan Kumar Halder wrote:
On 19/04/2023 14:54, Michal Orzel wrote:
On 19/04/2023 15:19, Michal Orzel wrote:
Hi Ayan,
On 13/04/2023 19:37, Ayan Kumar Halder wrote:
The DT functions (dt_read_number(), device_tree_get_reg(),
fdt_get_mem_rsv())
currently accept or return 64-bit values.
In future when we support 32-bit physical address, these DT
functions are
expected to accept/return 32-bit or 64-bit values (depending on
the width of
physical address). Also, we wish to detect if any truncation has
occurred
(i.e. while parsing 32-bit physical addresses from 64-bit values
read from DT).
device_tree_get_reg() should now be able to return paddr_t. This
is invoked by
various callers to get DT address and size.
For fdt_get_mem_rsv(), we have introduced a wrapper named
fdt_get_mem_rsv_paddr() which will invoke fdt_get_mem_rsv() and
translate
uint64_t to paddr_t. The reason being we cannot modify
fdt_get_mem_rsv() as it
has been imported from external source.
For dt_read_number(), we have also introduced a wrapper named
dt_read_paddr()
dt_read_paddr() to read physical addresses. We chose not to modify
the original
function as it is used in places where it needs to specifically
read 64-bit
values from dt (For e.g. dt_property_read_u64()).
Xen prints warning when it detects truncation in cases where it is
not able to
return error.
Also, replaced u32/u64 with uint32_t/uint64_t in the functions
touched
by the code changes.
Also, initialized variables to fix the warning
"-Werror=maybe-uninitialized".
I can see that now you explicitly set to 0 variables passed to
fdt_get_mem_rsv_paddr()
which haven't been initialized before being passed to
fdt_get_mem_rsv(). Is this what
you are reffering to? I cannot reproduce it, hence my question.
I can see why did you get this error.
Before your change, we always checked for an error from
fdt_get_mem_rsv() by checking if < 0.
In your wrapper fdt_get_mem_rsv_paddr(), you switched (not sure why)
to checking if not zero.
Becasue of this, you got an error and tried to fix it by
initializing the variables to 0.
I actually wanted to return the error code obtained from
fdt_get_mem_rsv() to the caller.
In this case, it returns a single error code.
I would rather not rely on this.
So does this look sane to you ?
diff --git a/xen/include/xen/libfdt/libfdt-xen.h
b/xen/include/xen/libfdt/libfdt-xen.h
index 3296a368a6..1da87d6668 100644
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -22,9 +22,8 @@ static inline int fdt_get_mem_rsv_paddr(const void
*fdt, int n,
uint64_t dt_size;
int ret = 0;
- ret = fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size);
- if ( ret )
- return ret;
+ if ( fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size) < 0 )
+ return -FDT_ERR_BADOFFSET;
So the problem if you check for ret to be non-zero. But the caller of
fdt_get_mem_rsv_paddr() check for < 0.
Given that fdt_get_mem_rsv() is not inline, the compiler doesn't know
that it will not return a positive value (other than 0). Hence why I
think you get an unitialize value.
The snippet below should work:
if ( ret < 0 )
return ret;
Awesome, thanks for the explanation. This works. :)
- Ayan
Cheers,
