Message ID | 20211109004808.115906-1-sstabellini@kernel.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [for-4.16] xen/arm: don't assign domU static-mem to dom0 as reserved-memory | expand |
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 = ®[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,
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>
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 = ®[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);
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 = ®[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);