* [Xen-devel] [PATCH v6 1/8] xen/arm: pass node to device_tree_for_each_node
2019-08-15 23:36 [Xen-devel] [PATCH v6 0/8] reserved-memory in dom0 Stefano Stabellini
@ 2019-08-15 23:36 ` Stefano Stabellini
2019-08-16 9:06 ` Julien Grall
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 2/8] xen/arm: make process_memory_node a device_tree_node_func Stefano Stabellini
` (6 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-15 23:36 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, Volodymyr_Babchuk
Add a new parameter to device_tree_for_each_node: node, the node to
start the search from. Passing 0 triggers the old behavior.
Set min_depth to depth of the current node + 1 to avoid scanning
siblings of the initial node passed as an argument.
Don't call func() on the parent node passed as an argument. Clarify the
change in the comment on top of the function.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v6:
- fix code style
- don't call func() on the first node
Changes in v5:
- go back to v3
- code style improvement in acpi/boot.c
- improve comments and commit message
- increase min_depth to avoid parsing siblings
- replace for with do/while loop and increase min_depth to avoid
scanning siblings of the initial node
- pass only node, calculate depth
Changes in v3:
- improve commit message
- improve in-code comments
- improve code style
Changes in v2:
- new
---
xen/arch/arm/acpi/boot.c | 8 +++++---
xen/arch/arm/bootfdt.c | 34 ++++++++++++++++++++--------------
xen/include/xen/device_tree.h | 6 +++---
3 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 9b29769a10..bf9c78b02c 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void)
* - the device tree is not empty (it has more than just a /chosen node)
* and ACPI has not been force enabled (acpi=force)
*/
- if ( param_acpi_off || ( !param_acpi_force
- && device_tree_for_each_node(device_tree_flattened,
- dt_scan_depth1_nodes, NULL)))
+ if ( param_acpi_off)
+ goto disable;
+ if ( !param_acpi_force &&
+ device_tree_for_each_node(device_tree_flattened, 0,
+ dt_scan_depth1_nodes, NULL) )
goto disable;
/*
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 891b4b66ff..f1614ef7fc 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -75,9 +75,10 @@ static u32 __init device_tree_get_u32(const void *fdt, int node,
}
/**
- * device_tree_for_each_node - iterate over all device tree nodes
+ * device_tree_for_each_node - iterate over all device tree sub-nodes
* @fdt: flat device tree.
- * @func: function to call for each node.
+ * @node: parent node to start the search from
+ * @func: function to call for each sub-node.
* @data: data to pass to @func.
*
* Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
@@ -85,20 +86,18 @@ static u32 __init device_tree_get_u32(const void *fdt, int node,
* Returns 0 if all nodes were iterated over successfully. If @func
* returns a value different from 0, that value is returned immediately.
*/
-int __init device_tree_for_each_node(const void *fdt,
+int __init device_tree_for_each_node(const void *fdt, int node,
device_tree_node_func func,
void *data)
{
- int node;
- int depth;
+ int depth = fdt_node_depth(fdt, node);
+ int min_depth = depth + 1;
+ int first_node = node;
u32 address_cells[DEVICE_TREE_MAX_DEPTH];
u32 size_cells[DEVICE_TREE_MAX_DEPTH];
int ret;
- for ( node = 0, depth = 0;
- node >=0 && depth >= 0;
- node = fdt_next_node(fdt, node, &depth) )
- {
+ do {
const char *name = fdt_get_name(fdt, node, NULL);
u32 as, ss;
@@ -117,10 +116,17 @@ int __init device_tree_for_each_node(const void *fdt,
size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells", ss);
- ret = func(fdt, node, name, depth, as, ss, data);
- if ( ret != 0 )
- return ret;
- }
+ /* skip the first node */
+ if ( node != first_node )
+ {
+ ret = func(fdt, node, name, depth, as, ss, data);
+ if ( ret != 0 )
+ return ret;
+ }
+
+ node = fdt_next_node(fdt, node, &depth);
+ } while ( node >= 0 && depth >= min_depth );
+
return 0;
}
@@ -357,7 +363,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
- device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
+ device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
early_print_info();
return fdt_totalsize(fdt);
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 83156297e2..9a7a8f2dab 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -158,9 +158,9 @@ typedef int (*device_tree_node_func)(const void *fdt,
extern const void *device_tree_flattened;
-int device_tree_for_each_node(const void *fdt,
- device_tree_node_func func,
- void *data);
+int device_tree_for_each_node(const void *fdt, int node,
+ device_tree_node_func func,
+ void *data);
/**
* dt_unflatten_host_device_tree - Unflatten the host device tree
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Xen-devel] [PATCH v6 1/8] xen/arm: pass node to device_tree_for_each_node
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 1/8] xen/arm: pass node to device_tree_for_each_node Stefano Stabellini
@ 2019-08-16 9:06 ` Julien Grall
2019-08-17 0:29 ` Stefano Stabellini
0 siblings, 1 reply; 19+ messages in thread
From: Julien Grall @ 2019-08-16 9:06 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini, Volodymyr_Babchuk
Hi,
On 16/08/2019 00:36, Stefano Stabellini wrote:
> Add a new parameter to device_tree_for_each_node: node, the node to
> start the search from. Passing 0 triggers the old behavior.
Here you say 0 triggers the old behavior but...
>
> Set min_depth to depth of the current node + 1 to avoid scanning
> siblings of the initial node passed as an argument.
>
> Don't call func() on the parent node passed as an argument. Clarify the
> change in the comment on top of the function.
... here you mention that the first node will be skipped. So the behavior is now
different and should be explained in the commit message why this is fine to skip
the root node.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> Changes in v6:
> - fix code style
> - don't call func() on the first node
>
> Changes in v5:
> - go back to v3
> - code style improvement in acpi/boot.c
> - improve comments and commit message
> - increase min_depth to avoid parsing siblings
> - replace for with do/while loop and increase min_depth to avoid
> scanning siblings of the initial node
> - pass only node, calculate depth
>
> Changes in v3:
> - improve commit message
> - improve in-code comments
> - improve code style
>
> Changes in v2:
> - new
> ---
> xen/arch/arm/acpi/boot.c | 8 +++++---
> xen/arch/arm/bootfdt.c | 34 ++++++++++++++++++++--------------
> xen/include/xen/device_tree.h | 6 +++---
> 3 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 9b29769a10..bf9c78b02c 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void)
> * - the device tree is not empty (it has more than just a /chosen node)
> * and ACPI has not been force enabled (acpi=force)
> */
> - if ( param_acpi_off || ( !param_acpi_force
> - && device_tree_for_each_node(device_tree_flattened,
> - dt_scan_depth1_nodes, NULL)))
> + if ( param_acpi_off)
> + goto disable;
> + if ( !param_acpi_force &&
> + device_tree_for_each_node(device_tree_flattened, 0,
> + dt_scan_depth1_nodes, NULL) )
> goto disable;
>
> /*
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 891b4b66ff..f1614ef7fc 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -75,9 +75,10 @@ static u32 __init device_tree_get_u32(const void *fdt, int node,
> }
>
> /**
> - * device_tree_for_each_node - iterate over all device tree nodes
> + * device_tree_for_each_node - iterate over all device tree sub-nodes
> * @fdt: flat device tree.
> - * @func: function to call for each node.
> + * @node: parent node to start the search from
> + * @func: function to call for each sub-node.
> * @data: data to pass to @func.
> *
> * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
> @@ -85,20 +86,18 @@ static u32 __init device_tree_get_u32(const void *fdt, int node,
> * Returns 0 if all nodes were iterated over successfully. If @func
> * returns a value different from 0, that value is returned immediately.
> */
> -int __init device_tree_for_each_node(const void *fdt,
> +int __init device_tree_for_each_node(const void *fdt, int node,
> device_tree_node_func func,
> void *data)
> {
> - int node;
> - int depth;
> + int depth = fdt_node_depth(fdt, node);
Sorry I didn't spot this in the previous version. As I pointed out on v4 (and
you actually agreed!), you don't need the absolute depth...
You only need the relative depth. So this is a waste of processing as you have
to go through the FDT to calculate the depth.
This is not entirely critical so I would be willing to consider a patch on top
of this series.
> + int min_depth = depth + 1;
> + int first_node = node;
NIT: Anything that can't change should really be const to catch any mistake.
> u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> u32 size_cells[DEVICE_TREE_MAX_DEPTH];
> int ret;
>
> - for ( node = 0, depth = 0;
> - node >=0 && depth >= 0;
> - node = fdt_next_node(fdt, node, &depth) )
> - {
> + do {
> const char *name = fdt_get_name(fdt, node, NULL);
> u32 as, ss;
>
> @@ -117,10 +116,17 @@ int __init device_tree_for_each_node(const void *fdt,
> size_cells[depth] = device_tree_get_u32(fdt, node,
> "#size-cells", ss);
>
> - ret = func(fdt, node, name, depth, as, ss, data);
> - if ( ret != 0 )
> - return ret;
> - }
> + /* skip the first node */
> + if ( node != first_node )
> + {
> + ret = func(fdt, node, name, depth, as, ss, data);
> + if ( ret != 0 )
> + return ret;
> + }
> +
> + node = fdt_next_node(fdt, node, &depth);
> + } while ( node >= 0 && depth >= min_depth );
> +
> return 0;
> }
>
> @@ -357,7 +363,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
>
> add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
>
> - device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
> + device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
> early_print_info();
>
> return fdt_totalsize(fdt);
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 83156297e2..9a7a8f2dab 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -158,9 +158,9 @@ typedef int (*device_tree_node_func)(const void *fdt,
>
> extern const void *device_tree_flattened;
>
> -int device_tree_for_each_node(const void *fdt,
> - device_tree_node_func func,
> - void *data);
> +int device_tree_for_each_node(const void *fdt, int node,
> + device_tree_node_func func,
> + void *data);
>
> /**
> * dt_unflatten_host_device_tree - Unflatten the host device tree
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Xen-devel] [PATCH v6 1/8] xen/arm: pass node to device_tree_for_each_node
2019-08-16 9:06 ` Julien Grall
@ 2019-08-17 0:29 ` Stefano Stabellini
2019-08-19 9:51 ` Julien Grall
0 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-17 0:29 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr_Babchuk
On Fri, 16 Aug 2019, Julien Grall wrote:
> Hi,
>
> On 16/08/2019 00:36, Stefano Stabellini wrote:
> > Add a new parameter to device_tree_for_each_node: node, the node to
> > start the search from. Passing 0 triggers the old behavior.
>
> Here you say 0 triggers the old behavior but...
>
> >
> > Set min_depth to depth of the current node + 1 to avoid scanning
> > siblings of the initial node passed as an argument.
> >
> > Don't call func() on the parent node passed as an argument. Clarify the
> > change in the comment on top of the function.
>
> ... here you mention that the first node will be skipped. So the behavior is
> now different and should be explained in the commit message why this is fine
> to skip the root node.
Yes I'll update
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > Changes in v6:
> > - fix code style
> > - don't call func() on the first node
> >
> > Changes in v5:
> > - go back to v3
> > - code style improvement in acpi/boot.c
> > - improve comments and commit message
> > - increase min_depth to avoid parsing siblings
> > - replace for with do/while loop and increase min_depth to avoid
> > scanning siblings of the initial node
> > - pass only node, calculate depth
> >
> > Changes in v3:
> > - improve commit message
> > - improve in-code comments
> > - improve code style
> >
> > Changes in v2:
> > - new
> > ---
> > xen/arch/arm/acpi/boot.c | 8 +++++---
> > xen/arch/arm/bootfdt.c | 34 ++++++++++++++++++++--------------
> > xen/include/xen/device_tree.h | 6 +++---
> > 3 files changed, 28 insertions(+), 20 deletions(-)
> >
> > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> > index 9b29769a10..bf9c78b02c 100644
> > --- a/xen/arch/arm/acpi/boot.c
> > +++ b/xen/arch/arm/acpi/boot.c
> > @@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void)
> > * - the device tree is not empty (it has more than just a /chosen
> > node)
> > * and ACPI has not been force enabled (acpi=force)
> > */
> > - if ( param_acpi_off || ( !param_acpi_force
> > - &&
> > device_tree_for_each_node(device_tree_flattened,
> > - dt_scan_depth1_nodes,
> > NULL)))
> > + if ( param_acpi_off)
> > + goto disable;
> > + if ( !param_acpi_force &&
> > + device_tree_for_each_node(device_tree_flattened, 0,
> > + dt_scan_depth1_nodes, NULL) )
> > goto disable;
> > /*
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 891b4b66ff..f1614ef7fc 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -75,9 +75,10 @@ static u32 __init device_tree_get_u32(const void *fdt,
> > int node,
> > }
> > /**
> > - * device_tree_for_each_node - iterate over all device tree nodes
> > + * device_tree_for_each_node - iterate over all device tree sub-nodes
> > * @fdt: flat device tree.
> > - * @func: function to call for each node.
> > + * @node: parent node to start the search from
> > + * @func: function to call for each sub-node.
> > * @data: data to pass to @func.
> > *
> > * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
> > @@ -85,20 +86,18 @@ static u32 __init device_tree_get_u32(const void *fdt,
> > int node,
> > * Returns 0 if all nodes were iterated over successfully. If @func
> > * returns a value different from 0, that value is returned immediately.
> > */
> > -int __init device_tree_for_each_node(const void *fdt,
> > +int __init device_tree_for_each_node(const void *fdt, int node,
> > device_tree_node_func func,
> > void *data)
> > {
> > - int node;
> > - int depth;
> > + int depth = fdt_node_depth(fdt, node);
>
> Sorry I didn't spot this in the previous version. As I pointed out on v4 (and
> you actually agreed!), you don't need the absolute depth...
>
> You only need the relative depth. So this is a waste of processing as you have
> to go through the FDT to calculate the depth.
>
> This is not entirely critical so I would be willing to consider a patch on top
> of this series.
I tried when I sent this version of the series, but I couldn't quite do
it that way. I wanted to get rid of the depth parameter to
device_tree_for_each_node, and we need to know the depth of the first
node passed as an argument to compare it with the depth of the next node
and figure out if it is at the same level or one level deeper.
How do you see this being implemented?
> > + int min_depth = depth + 1;
> > + int first_node = node;
>
> NIT: Anything that can't change should really be const to catch any mistake.
OK
>
> > u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> > u32 size_cells[DEVICE_TREE_MAX_DEPTH];
> > int ret;
> > - for ( node = 0, depth = 0;
> > - node >=0 && depth >= 0;
> > - node = fdt_next_node(fdt, node, &depth) )
> > - {
> > + do {
> > const char *name = fdt_get_name(fdt, node, NULL);
> > u32 as, ss;
> > @@ -117,10 +116,17 @@ int __init device_tree_for_each_node(const void
> > *fdt,
> > size_cells[depth] = device_tree_get_u32(fdt, node,
> > "#size-cells", ss);
> > - ret = func(fdt, node, name, depth, as, ss, data);
> > - if ( ret != 0 )
> > - return ret;
> > - }
> > + /* skip the first node */
> > + if ( node != first_node )
> > + {
> > + ret = func(fdt, node, name, depth, as, ss, data);
> > + if ( ret != 0 )
> > + return ret;
> > + }
> > +
> > + node = fdt_next_node(fdt, node, &depth);
> > + } while ( node >= 0 && depth >= min_depth );
> > +
> > return 0;
> > }
> > @@ -357,7 +363,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t
> > paddr)
> > add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
> > - device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
> > + device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
> > early_print_info();
> > return fdt_totalsize(fdt);
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 83156297e2..9a7a8f2dab 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -158,9 +158,9 @@ typedef int (*device_tree_node_func)(const void *fdt,
> > extern const void *device_tree_flattened;
> > -int device_tree_for_each_node(const void *fdt,
> > - device_tree_node_func func,
> > - void *data);
> > +int device_tree_for_each_node(const void *fdt, int node,
> > + device_tree_node_func func,
> > + void *data);
> > /**
> > * dt_unflatten_host_device_tree - Unflatten the host device tree
> >
>
> Cheers,
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Xen-devel] [PATCH v6 1/8] xen/arm: pass node to device_tree_for_each_node
2019-08-17 0:29 ` Stefano Stabellini
@ 2019-08-19 9:51 ` Julien Grall
2019-08-19 17:06 ` Stefano Stabellini
0 siblings, 1 reply; 19+ messages in thread
From: Julien Grall @ 2019-08-19 9:51 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Volodymyr_Babchuk
Hi,
On 8/17/19 1:29 AM, Stefano Stabellini wrote:
> On Fri, 16 Aug 2019, Julien Grall wrote:
>> Hi,
>>
>> On 16/08/2019 00:36, Stefano Stabellini wrote:
>>> Add a new parameter to device_tree_for_each_node: node, the node to
>>> start the search from. Passing 0 triggers the old behavior.
>>
>> Here you say 0 triggers the old behavior but...
>>
>>>
>>> Set min_depth to depth of the current node + 1 to avoid scanning
>>> siblings of the initial node passed as an argument.
>>>
>>> Don't call func() on the parent node passed as an argument. Clarify the
>>> change in the comment on top of the function.
>>
>> ... here you mention that the first node will be skipped. So the behavior is
>> now different and should be explained in the commit message why this is fine
>> to skip the root node.
>
> Yes I'll update
>
>
>>>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> ---
>>> Changes in v6:
>>> - fix code style
>>> - don't call func() on the first node
>>>
>>> Changes in v5:
>>> - go back to v3
>>> - code style improvement in acpi/boot.c
>>> - improve comments and commit message
>>> - increase min_depth to avoid parsing siblings
>>> - replace for with do/while loop and increase min_depth to avoid
>>> scanning siblings of the initial node
>>> - pass only node, calculate depth
>>>
>>> Changes in v3:
>>> - improve commit message
>>> - improve in-code comments
>>> - improve code style
>>>
>>> Changes in v2:
>>> - new
>>> ---
>>> xen/arch/arm/acpi/boot.c | 8 +++++---
>>> xen/arch/arm/bootfdt.c | 34 ++++++++++++++++++++--------------
>>> xen/include/xen/device_tree.h | 6 +++---
>>> 3 files changed, 28 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
>>> index 9b29769a10..bf9c78b02c 100644
>>> --- a/xen/arch/arm/acpi/boot.c
>>> +++ b/xen/arch/arm/acpi/boot.c
>>> @@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void)
>>> * - the device tree is not empty (it has more than just a /chosen
>>> node)
>>> * and ACPI has not been force enabled (acpi=force)
>>> */
>>> - if ( param_acpi_off || ( !param_acpi_force
>>> - &&
>>> device_tree_for_each_node(device_tree_flattened,
>>> - dt_scan_depth1_nodes,
>>> NULL)))
>>> + if ( param_acpi_off)
>>> + goto disable;
>>> + if ( !param_acpi_force &&
>>> + device_tree_for_each_node(device_tree_flattened, 0,
>>> + dt_scan_depth1_nodes, NULL) )
>>> goto disable;
>>> /*
>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>>> index 891b4b66ff..f1614ef7fc 100644
>>> --- a/xen/arch/arm/bootfdt.c
>>> +++ b/xen/arch/arm/bootfdt.c
>>> @@ -75,9 +75,10 @@ static u32 __init device_tree_get_u32(const void *fdt,
>>> int node,
>>> }
>>> /**
>>> - * device_tree_for_each_node - iterate over all device tree nodes
>>> + * device_tree_for_each_node - iterate over all device tree sub-nodes
>>> * @fdt: flat device tree.
>>> - * @func: function to call for each node.
>>> + * @node: parent node to start the search from
>>> + * @func: function to call for each sub-node.
>>> * @data: data to pass to @func.
>>> *
>>> * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
>>> @@ -85,20 +86,18 @@ static u32 __init device_tree_get_u32(const void *fdt,
>>> int node,
>>> * Returns 0 if all nodes were iterated over successfully. If @func
>>> * returns a value different from 0, that value is returned immediately.
>>> */
>>> -int __init device_tree_for_each_node(const void *fdt,
>>> +int __init device_tree_for_each_node(const void *fdt, int node,
>>> device_tree_node_func func,
>>> void *data)
>>> {
>>> - int node;
>>> - int depth;
>>> + int depth = fdt_node_depth(fdt, node);
>>
>> Sorry I didn't spot this in the previous version. As I pointed out on v4 (and
>> you actually agreed!), you don't need the absolute depth...
>>
>> You only need the relative depth. So this is a waste of processing as you have
>> to go through the FDT to calculate the depth.
>>
>> This is not entirely critical so I would be willing to consider a patch on top
>> of this series.
>
> I tried when I sent this version of the series, but I couldn't quite do
> it that way. I wanted to get rid of the depth parameter to
> device_tree_for_each_node, and we need to know the depth of the first
> node passed as an argument to compare it with the depth of the next node
> and figure out if it is at the same level or one level deeper.
fdt_next_node() will increment/decrement whichever depth you pass in
argument.
So if you pass 0, then any child of the node will be at depth 1. Any
node at the same level as the parent node will also be depth 0...
Therefore initializing depth to 0 and checking it is strictly positive
(i.e depth > 0) is enough for our use case.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Xen-devel] [PATCH v6 1/8] xen/arm: pass node to device_tree_for_each_node
2019-08-19 9:51 ` Julien Grall
@ 2019-08-19 17:06 ` Stefano Stabellini
0 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-19 17:06 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr_Babchuk
On Mon, 19 Aug 2019, Julien Grall wrote:
> On 8/17/19 1:29 AM, Stefano Stabellini wrote:
> > On Fri, 16 Aug 2019, Julien Grall wrote:
> > > Hi,
> > >
> > > On 16/08/2019 00:36, Stefano Stabellini wrote:
> > > > Add a new parameter to device_tree_for_each_node: node, the node to
> > > > start the search from. Passing 0 triggers the old behavior.
> > >
> > > Here you say 0 triggers the old behavior but...
> > >
> > > >
> > > > Set min_depth to depth of the current node + 1 to avoid scanning
> > > > siblings of the initial node passed as an argument.
> > > >
> > > > Don't call func() on the parent node passed as an argument. Clarify the
> > > > change in the comment on top of the function.
> > >
> > > ... here you mention that the first node will be skipped. So the behavior
> > > is
> > > now different and should be explained in the commit message why this is
> > > fine
> > > to skip the root node.
> >
> > Yes I'll update
> >
> >
> > > >
> > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > > ---
> > > > Changes in v6:
> > > > - fix code style
> > > > - don't call func() on the first node
> > > >
> > > > Changes in v5:
> > > > - go back to v3
> > > > - code style improvement in acpi/boot.c
> > > > - improve comments and commit message
> > > > - increase min_depth to avoid parsing siblings
> > > > - replace for with do/while loop and increase min_depth to avoid
> > > > scanning siblings of the initial node
> > > > - pass only node, calculate depth
> > > >
> > > > Changes in v3:
> > > > - improve commit message
> > > > - improve in-code comments
> > > > - improve code style
> > > >
> > > > Changes in v2:
> > > > - new
> > > > ---
> > > > xen/arch/arm/acpi/boot.c | 8 +++++---
> > > > xen/arch/arm/bootfdt.c | 34 ++++++++++++++++++++--------------
> > > > xen/include/xen/device_tree.h | 6 +++---
> > > > 3 files changed, 28 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> > > > index 9b29769a10..bf9c78b02c 100644
> > > > --- a/xen/arch/arm/acpi/boot.c
> > > > +++ b/xen/arch/arm/acpi/boot.c
> > > > @@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void)
> > > > * - the device tree is not empty (it has more than just a
> > > > /chosen
> > > > node)
> > > > * and ACPI has not been force enabled (acpi=force)
> > > > */
> > > > - if ( param_acpi_off || ( !param_acpi_force
> > > > - &&
> > > > device_tree_for_each_node(device_tree_flattened,
> > > > -
> > > > dt_scan_depth1_nodes,
> > > > NULL)))
> > > > + if ( param_acpi_off)
> > > > + goto disable;
> > > > + if ( !param_acpi_force &&
> > > > + device_tree_for_each_node(device_tree_flattened, 0,
> > > > + dt_scan_depth1_nodes, NULL) )
> > > > goto disable;
> > > > /*
> > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > > > index 891b4b66ff..f1614ef7fc 100644
> > > > --- a/xen/arch/arm/bootfdt.c
> > > > +++ b/xen/arch/arm/bootfdt.c
> > > > @@ -75,9 +75,10 @@ static u32 __init device_tree_get_u32(const void
> > > > *fdt,
> > > > int node,
> > > > }
> > > > /**
> > > > - * device_tree_for_each_node - iterate over all device tree nodes
> > > > + * device_tree_for_each_node - iterate over all device tree sub-nodes
> > > > * @fdt: flat device tree.
> > > > - * @func: function to call for each node.
> > > > + * @node: parent node to start the search from
> > > > + * @func: function to call for each sub-node.
> > > > * @data: data to pass to @func.
> > > > *
> > > > * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
> > > > @@ -85,20 +86,18 @@ static u32 __init device_tree_get_u32(const void
> > > > *fdt,
> > > > int node,
> > > > * Returns 0 if all nodes were iterated over successfully. If @func
> > > > * returns a value different from 0, that value is returned
> > > > immediately.
> > > > */
> > > > -int __init device_tree_for_each_node(const void *fdt,
> > > > +int __init device_tree_for_each_node(const void *fdt, int node,
> > > > device_tree_node_func func,
> > > > void *data)
> > > > {
> > > > - int node;
> > > > - int depth;
> > > > + int depth = fdt_node_depth(fdt, node);
> > >
> > > Sorry I didn't spot this in the previous version. As I pointed out on v4
> > > (and
> > > you actually agreed!), you don't need the absolute depth...
> > >
> > > You only need the relative depth. So this is a waste of processing as you
> > > have
> > > to go through the FDT to calculate the depth.
> > >
> > > This is not entirely critical so I would be willing to consider a patch on
> > > top
> > > of this series.
> >
> > I tried when I sent this version of the series, but I couldn't quite do
> > it that way. I wanted to get rid of the depth parameter to
> > device_tree_for_each_node, and we need to know the depth of the first
> > node passed as an argument to compare it with the depth of the next node
> > and figure out if it is at the same level or one level deeper.
>
> fdt_next_node() will increment/decrement whichever depth you pass in argument.
>
> So if you pass 0, then any child of the node will be at depth 1. Any node at
> the same level as the parent node will also be depth 0...
>
> Therefore initializing depth to 0 and checking it is strictly positive (i.e
> depth > 0) is enough for our use case.
That makes a lot of sense and will improve the code. Thanks for the
suggestion.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Xen-devel] [PATCH v6 2/8] xen/arm: make process_memory_node a device_tree_node_func
2019-08-15 23:36 [Xen-devel] [PATCH v6 0/8] reserved-memory in dom0 Stefano Stabellini
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 1/8] xen/arm: pass node to device_tree_for_each_node Stefano Stabellini
@ 2019-08-15 23:36 ` Stefano Stabellini
2019-08-16 9:17 ` Julien Grall
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 3/8] xen/arm: keep track of reserved-memory regions Stefano Stabellini
` (5 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-15 23:36 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, Volodymyr_Babchuk
Change the signature of process_memory_node to match
device_tree_node_func. Thanks to this change, the next patch will be
able to use device_tree_for_each_node to call process_memory_node on all
the children of a provided node.
Return error if there is no reg property or if nr_banks is reached. Let
the caller deal with the error.
Add a printk when device tree parsing fails.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v6:
- fix out of space check
- bring back printk when address_cells or size_cells are not properly set
- return -EINVAL in that case (different from reg missing)
- add printk when parsing fails
- return -ENOENT when memory size is 0
Changes in v5:
- return -ENOENT if address_cells or size_cells are not properly set
Changes in v4:
- return error if there is no reg propery, remove printk
- return error if nr_banks is reached
Changes in v3:
- improve commit message
- check return value of process_memory_node
Changes in v2:
- new
---
xen/arch/arm/bootfdt.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index f1614ef7fc..9dc2c1352d 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -130,9 +130,10 @@ int __init device_tree_for_each_node(const void *fdt, int node,
return 0;
}
-static void __init process_memory_node(const void *fdt, int node,
- const char *name,
- u32 address_cells, u32 size_cells)
+static int __init process_memory_node(const void *fdt, int node,
+ const char *name, int depth,
+ u32 address_cells, u32 size_cells,
+ void *data)
{
const struct fdt_property *prop;
int i;
@@ -145,15 +146,12 @@ static void __init process_memory_node(const void *fdt, int node,
{
printk("fdt: node `%s': invalid #address-cells or #size-cells",
name);
- return;
+ return -EINVAL;
}
prop = fdt_get_property(fdt, node, "reg", NULL);
if ( !prop )
- {
- printk("fdt: node `%s': missing `reg' property\n", name);
- return;
- }
+ return -ENOENT;
cell = (const __be32 *)prop->data;
banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
@@ -162,11 +160,15 @@ static void __init process_memory_node(const void *fdt, int node,
{
device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
if ( !size )
- continue;
+ return -ENOENT;
bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
bootinfo.mem.nr_banks++;
}
+
+ if ( i < banks )
+ return -ENOSPC;
+ return 0;
}
static void __init process_multiboot_node(const void *fdt, int node,
@@ -298,15 +300,20 @@ static int __init early_scan_node(const void *fdt,
u32 address_cells, u32 size_cells,
void *data)
{
+ int rc = 0;
+
if ( device_tree_node_matches(fdt, node, "memory") )
- process_memory_node(fdt, node, name, address_cells, size_cells);
+ rc = process_memory_node(fdt, node, name, depth,
+ address_cells, size_cells, NULL);
else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
device_tree_node_compatible(fdt, node, "multiboot,module" )))
process_multiboot_node(fdt, node, name, address_cells, size_cells);
else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
process_chosen_node(fdt, node, name, address_cells, size_cells);
- return 0;
+ if ( rc < 0 )
+ printk("fdt: node `%s': parsing failed\n", name);
+ return rc;
}
static void __init early_print_info(void)
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Xen-devel] [PATCH v6 2/8] xen/arm: make process_memory_node a device_tree_node_func
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 2/8] xen/arm: make process_memory_node a device_tree_node_func Stefano Stabellini
@ 2019-08-16 9:17 ` Julien Grall
2019-08-17 0:48 ` Stefano Stabellini
0 siblings, 1 reply; 19+ messages in thread
From: Julien Grall @ 2019-08-16 9:17 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini, Volodymyr_Babchuk
Hi,
On 16/08/2019 00:36, Stefano Stabellini wrote:
> Change the signature of process_memory_node to match
> device_tree_node_func. Thanks to this change, the next patch will be
> able to use device_tree_for_each_node to call process_memory_node on all
> the children of a provided node.
>
> Return error if there is no reg property or if nr_banks is reached. Let
> the caller deal with the error.
This sentence does not match the change below. Only 2 of the new error paths are
described here.
>
> Add a printk when device tree parsing fails.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> Changes in v6:
> - fix out of space check
> - bring back printk when address_cells or size_cells are not properly set
> - return -EINVAL in that case (different from reg missing)
> - add printk when parsing fails
> - return -ENOENT when memory size is 0
>
> Changes in v5:
> - return -ENOENT if address_cells or size_cells are not properly set
>
> Changes in v4:
> - return error if there is no reg propery, remove printk
> - return error if nr_banks is reached
>
> Changes in v3:
> - improve commit message
> - check return value of process_memory_node
>
> Changes in v2:
> - new
> ---
> xen/arch/arm/bootfdt.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index f1614ef7fc..9dc2c1352d 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -130,9 +130,10 @@ int __init device_tree_for_each_node(const void *fdt, int node,
> return 0;
> }
>
> -static void __init process_memory_node(const void *fdt, int node,
> - const char *name,
> - u32 address_cells, u32 size_cells)
> +static int __init process_memory_node(const void *fdt, int node,
> + const char *name, int depth,
> + u32 address_cells, u32 size_cells,
> + void *data)
> {
> const struct fdt_property *prop;
> int i;
> @@ -145,15 +146,12 @@ static void __init process_memory_node(const void *fdt, int node,
> {
> printk("fdt: node `%s': invalid #address-cells or #size-cells",
> name);
> - return;
> + return -EINVAL;
> }
>
> prop = fdt_get_property(fdt, node, "reg", NULL);
> if ( !prop )
> - {
> - printk("fdt: node `%s': missing `reg' property\n", name);
> - return;
> - }
> + return -ENOENT;
>
> cell = (const __be32 *)prop->data;
> banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> @@ -162,11 +160,15 @@ static void __init process_memory_node(const void *fdt, int node,
> {
> device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> if ( !size )
> - continue;
> + return -ENOENT;
I don't think we can treat the same way the lack of "regs" properties and a size
of 0.
The former is expected as binding allow you to do it for reserved-memory. The
latter is the user not writing the property correctly. So ignoring the latter
will result to Xen potentially missing some reserved-regions (not great!).
So, similar to #address-cells/#size-cells discussion, we should return an error
we are able to distinguish. Probably -EINVAL.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Xen-devel] [PATCH v6 2/8] xen/arm: make process_memory_node a device_tree_node_func
2019-08-16 9:17 ` Julien Grall
@ 2019-08-17 0:48 ` Stefano Stabellini
2019-08-19 9:54 ` Julien Grall
0 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-17 0:48 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Volodymyr_Babchuk, Stefano Stabellini
On Fri, 16 Aug 2019, Julien Grall wrote:
> Hi,
>
> On 16/08/2019 00:36, Stefano Stabellini wrote:
> > Change the signature of process_memory_node to match
> > device_tree_node_func. Thanks to this change, the next patch will be
> > able to use device_tree_for_each_node to call process_memory_node on all
> > the children of a provided node.
> >
> > Return error if there is no reg property or if nr_banks is reached. Let
> > the caller deal with the error.
>
> This sentence does not match the change below. Only 2 of the new error paths
> are described here.
>
> >
> > Add a printk when device tree parsing fails.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > Changes in v6:
> > - fix out of space check
> > - bring back printk when address_cells or size_cells are not properly set
> > - return -EINVAL in that case (different from reg missing)
> > - add printk when parsing fails
> > - return -ENOENT when memory size is 0
> >
> > Changes in v5:
> > - return -ENOENT if address_cells or size_cells are not properly set
> >
> > Changes in v4:
> > - return error if there is no reg propery, remove printk
> > - return error if nr_banks is reached
> >
> > Changes in v3:
> > - improve commit message
> > - check return value of process_memory_node
> >
> > Changes in v2:
> > - new
> > ---
> > xen/arch/arm/bootfdt.c | 29 ++++++++++++++++++-----------
> > 1 file changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index f1614ef7fc..9dc2c1352d 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -130,9 +130,10 @@ int __init device_tree_for_each_node(const void *fdt,
> > int node,
> > return 0;
> > }
> > -static void __init process_memory_node(const void *fdt, int node,
> > - const char *name,
> > - u32 address_cells, u32 size_cells)
> > +static int __init process_memory_node(const void *fdt, int node,
> > + const char *name, int depth,
> > + u32 address_cells, u32 size_cells,
> > + void *data)
> > {
> > const struct fdt_property *prop;
> > int i;
> > @@ -145,15 +146,12 @@ static void __init process_memory_node(const void
> > *fdt, int node,
> > {
> > printk("fdt: node `%s': invalid #address-cells or #size-cells",
> > name);
> > - return;
> > + return -EINVAL;
> > }
> > prop = fdt_get_property(fdt, node, "reg", NULL);
> > if ( !prop )
> > - {
> > - printk("fdt: node `%s': missing `reg' property\n", name);
> > - return;
> > - }
> > + return -ENOENT;
> > cell = (const __be32 *)prop->data;
> > banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> > @@ -162,11 +160,15 @@ static void __init process_memory_node(const void
> > *fdt, int node,
> > {
> > device_tree_get_reg(&cell, address_cells, size_cells, &start,
> > &size);
> > if ( !size )
> > - continue;
> > + return -ENOENT;
>
> I don't think we can treat the same way the lack of "regs" properties and a
> size of 0.
>
> The former is expected as binding allow you to do it for reserved-memory. The
> latter is the user not writing the property correctly. So ignoring the latter
> will result to Xen potentially missing some reserved-regions (not great!).
>
> So, similar to #address-cells/#size-cells discussion, we should return an
> error we are able to distinguish. Probably -EINVAL.
I think you are right, and honestly I was thinking about it while I
updated this patch. If I use -EINVAL, it would be the same return error
as the "invalid #address-cells or #size-cells". I just wanted to
double-check that it is OK for you.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Xen-devel] [PATCH v6 2/8] xen/arm: make process_memory_node a device_tree_node_func
2019-08-17 0:48 ` Stefano Stabellini
@ 2019-08-19 9:54 ` Julien Grall
0 siblings, 0 replies; 19+ messages in thread
From: Julien Grall @ 2019-08-19 9:54 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Volodymyr_Babchuk, Stefano Stabellini
On 8/17/19 1:48 AM, Stefano Stabellini wrote:
> On Fri, 16 Aug 2019, Julien Grall wrote:
> I think you are right, and honestly I was thinking about it while I
> updated this patch. If I use -EINVAL, it would be the same return error
> as the "invalid #address-cells or #size-cells". I just wanted to
> double-check that it is OK for you.
We don't need to differentiate "invalid #{address, size}-cells" from
"zero size", so using the same errno is fine.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Xen-devel] [PATCH v6 3/8] xen/arm: keep track of reserved-memory regions
2019-08-15 23:36 [Xen-devel] [PATCH v6 0/8] reserved-memory in dom0 Stefano Stabellini
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 1/8] xen/arm: pass node to device_tree_for_each_node Stefano Stabellini
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 2/8] xen/arm: make process_memory_node a device_tree_node_func Stefano Stabellini
@ 2019-08-15 23:36 ` Stefano Stabellini
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 4/8] xen/arm: fix indentation in early_print_info Stefano Stabellini
` (4 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-15 23:36 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, Volodymyr_Babchuk
As we parse the device tree in Xen, keep track of the reserved-memory
regions as they need special treatment (follow-up patches will make use
of the stored information.)
Reuse process_memory_node to add reserved-memory regions to the
bootinfo.reserved_mem array.
Refuse to continue once we reach the max number of reserved memory
regions to avoid accidentally mapping any portions of them into a VM.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Changes in v6:
- use dt_node_cmp
- add acked-by
Changes in v5:
- remove unneeded cast
- remove unneeded strlen check
- don't pass address_cells, size_cells, depth to device_tree_for_each_node
Changes in v4:
- depth + 1 in process_reserved_memory_node
- pass address_cells and size_cells to device_tree_for_each_node
- pass struct meminfo * instead of a boolean to process_memory_node
- improve in-code comment
- use a separate process_reserved_memory_node (separate from
process_memory_node) function wrapper to have different error handling
Changes in v3:
- match only /reserved-memory
- put the warning back in place for reg not present on a normal memory
region
- refuse to continue once we reach the max number of reserved memory
regions
Changes in v2:
- call process_memory_node from process_reserved_memory_node to avoid
duplication
---
xen/arch/arm/bootfdt.c | 39 ++++++++++++++++++++++++++++++++-----
xen/include/asm-arm/setup.h | 1 +
2 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 9dc2c1352d..f23ebaa188 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -141,6 +141,7 @@ static int __init process_memory_node(const void *fdt, int node,
const __be32 *cell;
paddr_t start, size;
u32 reg_cells = address_cells + size_cells;
+ struct meminfo *mem = data;
if ( address_cells < 1 || size_cells < 1 )
{
@@ -156,14 +157,14 @@ static int __init process_memory_node(const void *fdt, int node,
cell = (const __be32 *)prop->data;
banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
- for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
+ for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
{
device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
if ( !size )
return -ENOENT;
- bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
- bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
- bootinfo.mem.nr_banks++;
+ mem->bank[mem->nr_banks].start = start;
+ mem->bank[mem->nr_banks].size = size;
+ mem->nr_banks++;
}
if ( i < banks )
@@ -171,6 +172,31 @@ static int __init process_memory_node(const void *fdt, int node,
return 0;
}
+static int __init process_reserved_memory_node(const void *fdt, int node,
+ const char *name, int depth,
+ u32 address_cells,
+ u32 size_cells,
+ void *data)
+{
+ int rc = process_memory_node(fdt, node, name, depth, address_cells,
+ size_cells, data);
+
+ if ( rc == -ENOSPC )
+ panic("Max number of supported reserved-memory regions reached.");
+ else if ( rc != -ENOENT )
+ return rc;
+ return 0;
+}
+
+static int __init process_reserved_memory(const void *fdt, int node,
+ const char *name, int depth,
+ u32 address_cells, u32 size_cells)
+{
+ return device_tree_for_each_node(fdt, node,
+ process_reserved_memory_node,
+ &bootinfo.reserved_mem);
+}
+
static void __init process_multiboot_node(const void *fdt, int node,
const char *name,
u32 address_cells, u32 size_cells)
@@ -304,7 +330,10 @@ static int __init early_scan_node(const void *fdt,
if ( device_tree_node_matches(fdt, node, "memory") )
rc = process_memory_node(fdt, node, name, depth,
- address_cells, size_cells, NULL);
+ address_cells, size_cells, &bootinfo.mem);
+ else if ( depth == 1 && !dt_node_cmp(name, "reserved-memory") )
+ rc = process_reserved_memory(fdt, node, name, depth,
+ address_cells, size_cells);
else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
device_tree_node_compatible(fdt, node, "multiboot,module" )))
process_multiboot_node(fdt, node, name, address_cells, size_cells);
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 8bf3d5910a..efcba545c2 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -66,6 +66,7 @@ struct bootcmdlines {
struct bootinfo {
struct meminfo mem;
+ struct meminfo reserved_mem;
struct bootmodules modules;
struct bootcmdlines cmdlines;
#ifdef CONFIG_ACPI
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Xen-devel] [PATCH v6 4/8] xen/arm: fix indentation in early_print_info
2019-08-15 23:36 [Xen-devel] [PATCH v6 0/8] reserved-memory in dom0 Stefano Stabellini
` (2 preceding siblings ...)
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 3/8] xen/arm: keep track of reserved-memory regions Stefano Stabellini
@ 2019-08-15 23:36 ` Stefano Stabellini
2019-08-16 9:18 ` Julien Grall
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 5/8] xen/arm: early_print_info print reserved_mem Stefano Stabellini
` (3 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-15 23:36 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, Volodymyr_Babchuk
No functional changes.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
xen/arch/arm/bootfdt.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index f23ebaa188..a70a739bb0 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -354,15 +354,15 @@ static void __init early_print_info(void)
for ( i = 0; i < mi->nr_banks; i++ )
printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
- mi->bank[i].start,
- mi->bank[i].start + mi->bank[i].size - 1);
+ mi->bank[i].start,
+ mi->bank[i].start + mi->bank[i].size - 1);
printk("\n");
for ( i = 0 ; i < mods->nr_mods; i++ )
printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %-12s\n",
- i,
- mods->module[i].start,
- mods->module[i].start + mods->module[i].size,
- boot_module_kind_as_string(mods->module[i].kind));
+ i,
+ mods->module[i].start,
+ mods->module[i].start + mods->module[i].size,
+ boot_module_kind_as_string(mods->module[i].kind));
nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
for ( i = 0; i < nr_rsvd; i++ )
@@ -372,8 +372,7 @@ static void __init early_print_info(void)
continue;
/* fdt_get_mem_rsv returns length */
e += s;
- printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
- i, s, e);
+ printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e);
}
printk("\n");
for ( i = 0 ; i < cmds->nr_mods; i++ )
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Xen-devel] [PATCH v6 5/8] xen/arm: early_print_info print reserved_mem
2019-08-15 23:36 [Xen-devel] [PATCH v6 0/8] reserved-memory in dom0 Stefano Stabellini
` (3 preceding siblings ...)
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 4/8] xen/arm: fix indentation in early_print_info Stefano Stabellini
@ 2019-08-15 23:36 ` Stefano Stabellini
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 6/8] xen/arm: handle reserved-memory in consider_modules and dt_unreserved_regions Stefano Stabellini
` (2 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-15 23:36 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, Volodymyr_Babchuk
Improve early_print_info to also print the banks saved in
bootinfo.reserved_mem. Print them right after RESVD, increasing the same
index.
Since we are at it, also switch the existing RESVD print to use unsigned
int.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Changes in v6:
- add acked-by and reviewed-by
- fix indentation
Changes in v5:
- switch to unsigned
Changes in v4:
- new patch
---
xen/arch/arm/bootfdt.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index a70a739bb0..a02c6a1065 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -348,9 +348,10 @@ static int __init early_scan_node(const void *fdt,
static void __init early_print_info(void)
{
struct meminfo *mi = &bootinfo.mem;
+ struct meminfo *mem_resv = &bootinfo.reserved_mem;
struct bootmodules *mods = &bootinfo.modules;
struct bootcmdlines *cmds = &bootinfo.cmdlines;
- int i, nr_rsvd;
+ unsigned int i, j, nr_rsvd;
for ( i = 0; i < mi->nr_banks; i++ )
printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
@@ -372,7 +373,13 @@ static void __init early_print_info(void)
continue;
/* fdt_get_mem_rsv returns length */
e += s;
- printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e);
+ printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e);
+ }
+ for ( j = 0; j < mem_resv->nr_banks; j++, i++ )
+ {
+ printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i,
+ mem_resv->bank[j].start,
+ mem_resv->bank[j].start + mem_resv->bank[j].size - 1);
}
printk("\n");
for ( i = 0 ; i < cmds->nr_mods; i++ )
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Xen-devel] [PATCH v6 6/8] xen/arm: handle reserved-memory in consider_modules and dt_unreserved_regions
2019-08-15 23:36 [Xen-devel] [PATCH v6 0/8] reserved-memory in dom0 Stefano Stabellini
` (4 preceding siblings ...)
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 5/8] xen/arm: early_print_info print reserved_mem Stefano Stabellini
@ 2019-08-15 23:36 ` Stefano Stabellini
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 7/8] xen/arm: don't iomem_permit_access for reserved-memory regions Stefano Stabellini
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 8/8] xen/arm: add reserved-memory regions to the dom0 memory node Stefano Stabellini
7 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-15 23:36 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, Volodymyr_Babchuk
reserved-memory regions overlap with memory nodes. The overlapping
memory is reserved-memory and should be handled accordingly:
consider_modules and dt_unreserved_regions should skip these regions the
same way they are already skipping mem-reserve regions.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Changes in v4:
- code style
- add acked-by
Changes in v3:
- coding style
- in-code comments
Changes in v2:
- fix commit message: full overlap
- remove check_reserved_memory
- extend consider_modules and dt_unreserved_regions
---
xen/arch/arm/setup.c | 53 +++++++++++++++++++++++++++++++++++++++++---
1 file changed, 50 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 215746a5c3..bc4082296e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -206,6 +206,28 @@ void __init dt_unreserved_regions(paddr_t s, paddr_t e,
}
}
+ /*
+ * i is the current bootmodule we are evaluating across all possible
+ * kinds.
+ *
+ * When retrieving the corresponding reserved-memory addresses
+ * below, we need to index the bootinfo.reserved_mem bank starting
+ * from 0, and only counting the reserved-memory modules. Hence,
+ * we need to use i - nr.
+ */
+ for ( ; i - nr < bootinfo.reserved_mem.nr_banks; i++ )
+ {
+ paddr_t r_s = bootinfo.reserved_mem.bank[i - nr].start;
+ paddr_t r_e = r_s + bootinfo.reserved_mem.bank[i - nr].size;
+
+ if ( s < r_e && r_s < e )
+ {
+ dt_unreserved_regions(r_e, e, cb, i + 1);
+ dt_unreserved_regions(s, r_s, cb, i + 1);
+ return;
+ }
+ }
+
cb(s, e);
}
@@ -392,7 +414,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
{
const struct bootmodules *mi = &bootinfo.modules;
int i;
- int nr_rsvd;
+ int nr;
s = (s+align-1) & ~(align-1);
e = e & ~(align-1);
@@ -418,9 +440,9 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
/* Now check any fdt reserved areas. */
- nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
+ nr = fdt_num_mem_rsv(device_tree_flattened);
- for ( ; i < mi->nr_mods + nr_rsvd; i++ )
+ for ( ; i < mi->nr_mods + nr; i++ )
{
paddr_t mod_s, mod_e;
@@ -442,6 +464,31 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
return consider_modules(s, mod_s, size, align, i+1);
}
}
+
+ /*
+ * i is the current bootmodule we are evaluating, across all
+ * possible kinds of bootmodules.
+ *
+ * When retrieving the corresponding reserved-memory addresses, we
+ * need to index the bootinfo.reserved_mem bank starting from 0, and
+ * only counting the reserved-memory modules. Hence, we need to use
+ * i - nr.
+ */
+ nr += mi->nr_mods;
+ for ( ; i - nr < bootinfo.reserved_mem.nr_banks; i++ )
+ {
+ paddr_t r_s = bootinfo.reserved_mem.bank[i - nr].start;
+ paddr_t r_e = r_s + bootinfo.reserved_mem.bank[i - nr].size;
+
+ if ( s < r_e && r_s < e )
+ {
+ r_e = consider_modules(r_e, e, size, align, i + 1);
+ if ( r_e )
+ return r_e;
+
+ return consider_modules(s, r_s, size, align, i + 1);
+ }
+ }
return e;
}
#endif
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Xen-devel] [PATCH v6 7/8] xen/arm: don't iomem_permit_access for reserved-memory regions
2019-08-15 23:36 [Xen-devel] [PATCH v6 0/8] reserved-memory in dom0 Stefano Stabellini
` (5 preceding siblings ...)
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 6/8] xen/arm: handle reserved-memory in consider_modules and dt_unreserved_regions Stefano Stabellini
@ 2019-08-15 23:36 ` Stefano Stabellini
2019-08-16 9:42 ` Julien Grall
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 8/8] xen/arm: add reserved-memory regions to the dom0 memory node Stefano Stabellini
7 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-15 23:36 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, Volodymyr_Babchuk
Don't allow reserved-memory regions to be remapped into any unprivileged
guests, until reserved-memory regions are properly supported in Xen. For
now, do not call iomem_permit_access on them, because giving
iomem_permit_access to dom0 means that the toolstack will be able to
assign the region to a domU.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v6:
- compare against "/reserved-memory/"
Changes in v5:
- fix check condition
- use strnicmp
- return error
- improve commit message
Changes in v4:
- compare the parent name with reserved-memory
- use dt_node_cmp
Changes in v3:
- new patch
---
xen/arch/arm/domain_build.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4c8404155a..673ffa453f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1155,15 +1155,23 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
bool need_mapping = !dt_device_for_passthrough(dev);
int res;
- res = iomem_permit_access(d, paddr_to_pfn(addr),
- paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
- if ( res )
+ /*
+ * Don't give iomem permissions for reserved-memory ranges to domUs
+ * until reserved-memory support is complete.
+ */
+ if ( strnicmp(dt_node_full_name(dev), "/reserved-memory/",
+ strlen("/reserved-memory/")) != 0 )
{
- printk(XENLOG_ERR "Unable to permit to dom%d access to"
- " 0x%"PRIx64" - 0x%"PRIx64"\n",
- d->domain_id,
- addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
- return res;
+ res = iomem_permit_access(d, paddr_to_pfn(addr),
+ paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
+ if ( res )
+ {
+ printk(XENLOG_ERR "Unable to permit to dom%d access to"
+ " 0x%"PRIx64" - 0x%"PRIx64"\n",
+ d->domain_id,
+ addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
+ return res;
+ }
}
if ( need_mapping )
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Xen-devel] [PATCH v6 7/8] xen/arm: don't iomem_permit_access for reserved-memory regions
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 7/8] xen/arm: don't iomem_permit_access for reserved-memory regions Stefano Stabellini
@ 2019-08-16 9:42 ` Julien Grall
2019-08-16 23:56 ` Stefano Stabellini
0 siblings, 1 reply; 19+ messages in thread
From: Julien Grall @ 2019-08-16 9:42 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini, Volodymyr_Babchuk
Hi,
On 16/08/2019 00:36, Stefano Stabellini wrote:
> Don't allow reserved-memory regions to be remapped into any unprivileged
> guests, until reserved-memory regions are properly supported in Xen. For
> now, do not call iomem_permit_access on them, because giving
> iomem_permit_access to dom0 means that the toolstack will be able to
> assign the region to a domU.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>
> Changes in v6:
> - compare against "/reserved-memory/"
>
> Changes in v5:
> - fix check condition
> - use strnicmp
> - return error
> - improve commit message
>
> Changes in v4:
> - compare the parent name with reserved-memory
> - use dt_node_cmp
>
> Changes in v3:
> - new patch
> ---
> xen/arch/arm/domain_build.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4c8404155a..673ffa453f 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1155,15 +1155,23 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
> bool need_mapping = !dt_device_for_passthrough(dev);
> int res;
>
> - res = iomem_permit_access(d, paddr_to_pfn(addr),
> - paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> - if ( res )
> + /*
> + * Don't give iomem permissions for reserved-memory ranges to domUs
In upstream Xen, this is only called for Dom0. So what are you referring to?
> + * until reserved-memory support is complete.
But as I pointed out before reserved-memory support is not going to happen via
iomem. This is an abuse of the interface.
> + */
A better comment would be:
"reserved-memory regions are RAM carved out for special purpose. They are not
MMIO and therefore a domain should not be able to manage them via the IOMEM
interface".
> + if ( strnicmp(dt_node_full_name(dev), "/reserved-memory/",
> + strlen("/reserved-memory/")) != 0 )
> {
> - printk(XENLOG_ERR "Unable to permit to dom%d access to"
> - " 0x%"PRIx64" - 0x%"PRIx64"\n",
> - d->domain_id,
> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> - return res;
> + res = iomem_permit_access(d, paddr_to_pfn(addr),
> + paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> + if ( res )
> + {
> + printk(XENLOG_ERR "Unable to permit to dom%d access to"
> + " 0x%"PRIx64" - 0x%"PRIx64"\n",
> + d->domain_id,
> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> + return res;
> + }
> }
>
> if ( need_mapping )
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Xen-devel] [PATCH v6 7/8] xen/arm: don't iomem_permit_access for reserved-memory regions
2019-08-16 9:42 ` Julien Grall
@ 2019-08-16 23:56 ` Stefano Stabellini
0 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-16 23:56 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Volodymyr_Babchuk, Stefano Stabellini
On Fri, 16 Aug 2019, Julien Grall wrote:
> On 16/08/2019 00:36, Stefano Stabellini wrote:
> > Don't allow reserved-memory regions to be remapped into any unprivileged
> > guests, until reserved-memory regions are properly supported in Xen. For
> > now, do not call iomem_permit_access on them, because giving
> > iomem_permit_access to dom0 means that the toolstack will be able to
> > assign the region to a domU.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> >
> > Changes in v6:
> > - compare against "/reserved-memory/"
> >
> > Changes in v5:
> > - fix check condition
> > - use strnicmp
> > - return error
> > - improve commit message
> >
> > Changes in v4:
> > - compare the parent name with reserved-memory
> > - use dt_node_cmp
> >
> > Changes in v3:
> > - new patch
> > ---
> > xen/arch/arm/domain_build.c | 24 ++++++++++++++++--------
> > 1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 4c8404155a..673ffa453f 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1155,15 +1155,23 @@ static int __init map_range_to_domain(const struct
> > dt_device_node *dev,
> > bool need_mapping = !dt_device_for_passthrough(dev);
> > int res;
> > - res = iomem_permit_access(d, paddr_to_pfn(addr),
> > - paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> > - if ( res )
> > + /*
> > + * Don't give iomem permissions for reserved-memory ranges to domUs
>
> In upstream Xen, this is only called for Dom0. So what are you referring to?
>
> > + * until reserved-memory support is complete.
>
> But as I pointed out before reserved-memory support is not going to happen via
> iomem. This is an abuse of the interface.
>
> > + */
>
> A better comment would be:
>
> "reserved-memory regions are RAM carved out for special purpose. They are not
> MMIO and therefore a domain should not be able to manage them via the IOMEM
> interface".
Thank you for providing the comment. I'll use your version.
> > + if ( strnicmp(dt_node_full_name(dev), "/reserved-memory/",
> > + strlen("/reserved-memory/")) != 0 )
> > {
> > - printk(XENLOG_ERR "Unable to permit to dom%d access to"
> > - " 0x%"PRIx64" - 0x%"PRIx64"\n",
> > - d->domain_id,
> > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> > - return res;
> > + res = iomem_permit_access(d, paddr_to_pfn(addr),
> > + paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> > + if ( res )
> > + {
> > + printk(XENLOG_ERR "Unable to permit to dom%d access to"
> > + " 0x%"PRIx64" - 0x%"PRIx64"\n",
> > + d->domain_id,
> > + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> > + return res;
> > + }
> > }
> > if ( need_mapping )
> >
>
> Cheers,
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Xen-devel] [PATCH v6 8/8] xen/arm: add reserved-memory regions to the dom0 memory node
2019-08-15 23:36 [Xen-devel] [PATCH v6 0/8] reserved-memory in dom0 Stefano Stabellini
` (6 preceding siblings ...)
2019-08-15 23:36 ` [Xen-devel] [PATCH v6 7/8] xen/arm: don't iomem_permit_access for reserved-memory regions Stefano Stabellini
@ 2019-08-15 23:36 ` Stefano Stabellini
7 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2019-08-15 23:36 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, Volodymyr_Babchuk
Reserved memory regions are automatically remapped to dom0. Their device
tree nodes are also added to dom0 device tree. However, the dom0 memory
node is not currently extended to cover the reserved memory regions
ranges as required by the spec. This commit fixes it.
Change make_memory_node to take a struct meminfo * instead of a
kernel_info. Call it twice for dom0, once to create the first regular
memory node, and the second time to create a second memory node with the
ranges covering reserved-memory regions.
Also, make a small code style fix in make_memory_node.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Changes in v5:
- add acked-by
Changes in v4:
- pass struct meminfo * to make_memory_node
- call make_memory_node twice for dom0, once for normal memory, once for
reserved-memory regions
---
xen/arch/arm/domain_build.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 673ffa453f..a654217820 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -639,11 +639,11 @@ static int __init fdt_property_interrupts(void *fdt, gic_interrupt_t *intr,
static int __init make_memory_node(const struct domain *d,
void *fdt,
int addrcells, int sizecells,
- const struct kernel_info *kinfo)
+ struct meminfo *mem)
{
int res, i;
int reg_size = addrcells + sizecells;
- int nr_cells = reg_size*kinfo->mem.nr_banks;
+ int nr_cells = reg_size * mem->nr_banks;
__be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
__be32 *cells;
@@ -662,10 +662,10 @@ static int __init make_memory_node(const struct domain *d,
return res;
cells = ®[0];
- for ( i = 0 ; i < kinfo->mem.nr_banks; i++ )
+ for ( i = 0 ; i < mem->nr_banks; i++ )
{
- u64 start = kinfo->mem.bank[i].start;
- u64 size = kinfo->mem.bank[i].size;
+ u64 start = mem->bank[i].start;
+ u64 size = mem->bank[i].size;
dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n",
i, start, start + size);
@@ -1485,10 +1485,18 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
if ( res )
return res;
- res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
+ res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, &kinfo->mem);
if ( res )
return res;
+ /*
+ * Create a second memory node to store the ranges covering
+ * reserved-memory regions.
+ */
+ res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
+ &bootinfo.reserved_mem);
+ if ( res )
+ return res;
}
res = fdt_end_node(kinfo->fdt);
@@ -1744,7 +1752,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
if ( ret )
goto err;
- ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
+ ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells, &kinfo->mem);
if ( ret )
goto err;
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread