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