From: Stefano Stabellini <stefano.stabellini@xilinx.com>

DomUs static-mem ranges are added to the reserved_mem array for
accounting, but they shouldn't be assigned to dom0 as the other regular
reserved-memory ranges in device tree.

In make_memory_nodes, fix the error by skipping banks with xen_domain
set to true in the reserved-memory array. Also make sure to use the
first valid (!xen_domain) start address for the memory node name.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---

This patch should be considered for 4.16 as it fixes an incorrect
behavior.

The risk is low for two reasons:
- the change is simple
- make_memory_node is easily tested because it gets called at every
  boot, e.g. gitlab-ci and OSSTest exercise this path

I tested this patch successfully with and without xen,static-mem.

---
 xen/arch/arm/domain_build.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1fbafeaea8..56d3ff9d08 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -874,11 +874,17 @@ static int __init make_memory_node(const struct domain *d,
     if ( mem->nr_banks == 0 )
         return -ENOENT;
 
+    for ( i = 0; i < mem->nr_banks && mem->bank[i].xen_domain; i++ )
+        ;
+    /* No reserved-memory ranges to expose to Dom0 */
+    if ( i == mem->nr_banks )
+        return 0;
+
     dt_dprintk("Create memory node (reg size %d, nr cells %d)\n",
                reg_size, nr_cells);
 
     /* ePAPR 3.4 */
-    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[0].start);
+    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
     res = fdt_begin_node(fdt, buf);
     if ( res )
         return res;
@@ -888,11 +894,14 @@ static int __init make_memory_node(const struct domain *d,
         return res;
 
     cells = &reg[0];
-    for ( i = 0 ; i < mem->nr_banks; i++ )
+    for ( ; i < mem->nr_banks; i++ )
     {
         u64 start = mem->bank[i].start;
         u64 size = mem->bank[i].size;
 
+        if ( mem->bank[i].xen_domain )
+            continue;
+
         dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
                    i, start, start + size);
 
-- 
2.25.1

Hi Stefano,

On 09/11/2021 00:48, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> DomUs static-mem ranges are added to the reserved_mem array for
> accounting, but they shouldn't be assigned to dom0 as the other regular
> reserved-memory ranges in device tree.
> 
> In make_memory_nodes, fix the error by skipping banks with xen_domain
> set to true in the reserved-memory array. Also make sure to use the
> first valid (!xen_domain) start address for the memory node name.
> 

This is a bug fix. So please add a Fixes tag. In this case, I think it 
should be:

Fixes: 41c031ff437b ("xen/arm: introduce domain on Static Allocation")

> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> 
> This patch should be considered for 4.16 as it fixes an incorrect
> behavior.
> 
> The risk is low for two reasons:
> - the change is simple
> - make_memory_node is easily tested because it gets called at every
>    boot, e.g. gitlab-ci and OSSTest exercise this path
> 
> I tested this patch successfully with and without xen,static-mem.
> 
> ---
>   xen/arch/arm/domain_build.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 1fbafeaea8..56d3ff9d08 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -874,11 +874,17 @@ static int __init make_memory_node(const struct domain *d,
>       if ( mem->nr_banks == 0 )
>           return -ENOENT;
>   
> +    for ( i = 0; i < mem->nr_banks && mem->bank[i].xen_domain; i++ )
> +        ;
> +    /* No reserved-memory ranges to expose to Dom0 */
I find this comment a bit misleading because make_memory_node() will 
also be called to create normal memory region. I would drop 
"reserved-memory" and add a comment on top of the loop explaining what 
the loop does.

> +    if ( i == mem->nr_banks )
> +        return 0;
> +
>       dt_dprintk("Create memory node (reg size %d, nr cells %d)\n",
>                  reg_size, nr_cells);

I think you need to adjust nr_cells otherwise we would write garbagge in 
the DT if we need to exclude some regions.

>   
>       /* ePAPR 3.4 */
> -    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[0].start);
> +    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
>       res = fdt_begin_node(fdt, buf);
>       if ( res )
>           return res;
> @@ -888,11 +894,14 @@ static int __init make_memory_node(const struct domain *d,
>           return res;
>   
>       cells = &reg[0];
> -    for ( i = 0 ; i < mem->nr_banks; i++ )
> +    for ( ; i < mem->nr_banks; i++ )
>       {
>           u64 start = mem->bank[i].start;
>           u64 size = mem->bank[i].size;
>   
> +        if ( mem->bank[i].xen_domain )
> +            continue;
> +
>           dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
>                      i, start, start + size);
>   
> 

Cheers,

-- 
Julien Grall

On Tue, 9 Nov 2021, Julien Grall wrote:
> On 09/11/2021 00:48, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > 
> > DomUs static-mem ranges are added to the reserved_mem array for
> > accounting, but they shouldn't be assigned to dom0 as the other regular
> > reserved-memory ranges in device tree.
> > 
> > In make_memory_nodes, fix the error by skipping banks with xen_domain
> > set to true in the reserved-memory array. Also make sure to use the
> > first valid (!xen_domain) start address for the memory node name.
> > 
> 
> This is a bug fix. So please add a Fixes tag. In this case, I think it should
> be:
> 
> Fixes: 41c031ff437b ("xen/arm: introduce domain on Static Allocation")

Thanks, will add



> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > ---
> > 
> > This patch should be considered for 4.16 as it fixes an incorrect
> > behavior.
> > 
> > The risk is low for two reasons:
> > - the change is simple
> > - make_memory_node is easily tested because it gets called at every
> >    boot, e.g. gitlab-ci and OSSTest exercise this path
> > 
> > I tested this patch successfully with and without xen,static-mem.
> > 
> > ---
> >   xen/arch/arm/domain_build.c | 13 +++++++++++--
> >   1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 1fbafeaea8..56d3ff9d08 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -874,11 +874,17 @@ static int __init make_memory_node(const struct domain
> > *d,
> >       if ( mem->nr_banks == 0 )
> >           return -ENOENT;
> >   +    for ( i = 0; i < mem->nr_banks && mem->bank[i].xen_domain; i++ )
> > +        ;
> > +    /* No reserved-memory ranges to expose to Dom0 */
> I find this comment a bit misleading because make_memory_node() will also be
> called to create normal memory region. I would drop "reserved-memory" and add
> a comment on top of the loop explaining what the loop does.

Yeah, I agree, I moved it and changed it


> > +    if ( i == mem->nr_banks )
> > +        return 0;
> > +
> >       dt_dprintk("Create memory node (reg size %d, nr cells %d)\n",
> >                  reg_size, nr_cells);
> 
> I think you need to adjust nr_cells otherwise we would write garbagge in the
> DT if we need to exclude some regions.

Good point! Fixed in the next version


> >         /* ePAPR 3.4 */
> > -    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[0].start);
> > +    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
> >       res = fdt_begin_node(fdt, buf);
> >       if ( res )
> >           return res;
> > @@ -888,11 +894,14 @@ static int __init make_memory_node(const struct domain
> > *d,
> >           return res;
> >         cells = &reg[0];
> > -    for ( i = 0 ; i < mem->nr_banks; i++ )
> > +    for ( ; i < mem->nr_banks; i++ )
> >       {
> >           u64 start = mem->bank[i].start;
> >           u64 size = mem->bank[i].size;
> >   +        if ( mem->bank[i].xen_domain )
> > +            continue;
> > +
> >           dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
> >                      i, start, start + size);

Stefano Stabellini writes ("[PATCH for-4.16] xen/arm: don't assign domU static-mem to dom0 as reserved-memory"):
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> DomUs static-mem ranges are added to the reserved_mem array for
> accounting, but they shouldn't be assigned to dom0 as the other regular
> reserved-memory ranges in device tree.
> 
> In make_memory_nodes, fix the error by skipping banks with xen_domain
> set to true in the reserved-memory array. Also make sure to use the
> first valid (!xen_domain) start address for the memory node name.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>