Various version of gcc, when compiling with -Og, complain:
xg_dom_arm.c: In function 'meminit':
xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]
420 | dom->p2m_size = p2m_size;
| ~~~~~~~~~~~~~~^~~~~~~~~~
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
Julien/Stefano: I can't work out how this variable is supposed to work, and
the fact that it isn't a straight accumulation across the RAM banks looks
suspect.
---
tools/libs/guest/xg_dom_arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
index 94948d2b20..f1b8d06f75 100644
--- a/tools/libs/guest/xg_dom_arm.c
+++ b/tools/libs/guest/xg_dom_arm.c
@@ -373,7 +373,7 @@ static int meminit(struct xc_dom_image *dom)
const uint64_t modsize = dtb_size + ramdisk_size;
const uint64_t ram128mb = bankbase[0] + (128<<20);
- xen_pfn_t p2m_size;
+ xen_pfn_t p2m_size = 0;
uint64_t bank0end;
assert(dom->rambase_pfn << XC_PAGE_SHIFT == bankbase[0]);
--
2.11.0
Hi Andrew, On 12/02/2021 15:39, Andrew Cooper wrote: > Various version of gcc, when compiling with -Og, complain: > > xg_dom_arm.c: In function 'meminit': > xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized in this function [-Werror=maybe-uninitialized] > 420 | dom->p2m_size = p2m_size; > | ~~~~~~~~~~~~~~^~~~~~~~~~ > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> This was reported nearly 3 years ago (see [1]) and it is pretty sad this was never merged :(. > --- > CC: Ian Jackson <iwj@xenproject.org> > CC: Wei Liu <wl@xen.org> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > > Julien/Stefano: I can't work out how this variable is supposed to work, and > the fact that it isn't a straight accumulation across the RAM banks looks > suspect. It looks buggy, but the P2M is never used on Arm. In fact, you sent a patch a year ago to drop it (see [2]). It would be nice to revive it. > --- > tools/libs/guest/xg_dom_arm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c > index 94948d2b20..f1b8d06f75 100644 > --- a/tools/libs/guest/xg_dom_arm.c > +++ b/tools/libs/guest/xg_dom_arm.c > @@ -373,7 +373,7 @@ static int meminit(struct xc_dom_image *dom) > const uint64_t modsize = dtb_size + ramdisk_size; > const uint64_t ram128mb = bankbase[0] + (128<<20); > > - xen_pfn_t p2m_size; > + xen_pfn_t p2m_size = 0; > uint64_t bank0end; > > assert(dom->rambase_pfn << XC_PAGE_SHIFT == bankbase[0]); > If your original series is too risky for 4.15, I would consider to remote p2m_size completely and always 0 dom->p2m_size. Cheers, [1] https://lore.kernel.org/xen-devel/20180314123203.30646-1-wei.liu2@citrix.com/ [2] https://patchwork.kernel.org/project/xen-devel/patch/20191217201550.15864-3-andrew.cooper3@citrix.com/ -- Julien Grall
On 12/02/2021 15:55, Julien Grall wrote: > Hi Andrew, > > On 12/02/2021 15:39, Andrew Cooper wrote: >> Various version of gcc, when compiling with -Og, complain: >> >> xg_dom_arm.c: In function 'meminit': >> xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized >> in this function [-Werror=maybe-uninitialized] >> 420 | dom->p2m_size = p2m_size; >> | ~~~~~~~~~~~~~~^~~~~~~~~~ >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > This was reported nearly 3 years ago (see [1]) and it is pretty sad > this was never merged :(. :( We've got far too many patches which fall through the cracks like this. > >> --- >> CC: Ian Jackson <iwj@xenproject.org> >> CC: Wei Liu <wl@xen.org> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Julien Grall <julien@xen.org> >> >> Julien/Stefano: I can't work out how this variable is supposed to >> work, and >> the fact that it isn't a straight accumulation across the RAM banks >> looks >> suspect. > > It looks buggy, but the P2M is never used on Arm. In fact, you sent a > patch a year ago to drop it (see [2]). It would be nice to revive it. That series was committed more than a year ago - ee21f10d70^..97e34ad22d - and tbh, I'd forgotten about it. In light of that, I think I'll just delete the p2m_size references here. It's easy to prove correctness via inspection, and removes a dubious construct entirely. ~Andrew
Various version of gcc, when compiling with -Og, complain:
xg_dom_arm.c: In function 'meminit':
xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]
420 | dom->p2m_size = p2m_size;
| ~~~~~~~~~~~~~~^~~~~~~~~~
This is actually entirely stale code since ee21f10d70^..97e34ad22d which
removed the 1:1 identity p2m for translated domains.
Drop the write of d->p2m_size, and the p2m_size local variable. Reposition
the p2m_size field in struct xc_dom_image and correct some stale
documentation.
This change really ought to have been part of the original cleanup series.
No actual change to how ARM domains are constructed.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
v2:
* Delete stale p2m_size infrastructure.
---
tools/include/xenguest.h | 5 ++---
tools/libs/guest/xg_dom_arm.c | 5 -----
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 775cf34c04..217022b6e7 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -145,6 +145,7 @@ struct xc_dom_image {
* eventually copied into guest context.
*/
xen_pfn_t *pv_p2m;
+ xen_pfn_t p2m_size; /* number of pfns covered by pv_p2m */
/* physical memory
*
@@ -154,12 +155,10 @@ struct xc_dom_image {
*
* An ARM guest has GUEST_RAM_BANKS regions of RAM, with
* rambank_size[i] pages in each. The lowest RAM address
- * (corresponding to the base of the p2m arrays above) is stored
- * in rambase_pfn.
+ * is stored in rambase_pfn.
*/
xen_pfn_t rambase_pfn;
xen_pfn_t total_pages;
- xen_pfn_t p2m_size; /* number of pfns covered by p2m */
struct xc_dom_phys *phys_pages;
#if defined (__arm__) || defined(__aarch64__)
xen_pfn_t rambank_size[GUEST_RAM_BANKS];
diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
index 94948d2b20..b4c24f15fb 100644
--- a/tools/libs/guest/xg_dom_arm.c
+++ b/tools/libs/guest/xg_dom_arm.c
@@ -373,7 +373,6 @@ static int meminit(struct xc_dom_image *dom)
const uint64_t modsize = dtb_size + ramdisk_size;
const uint64_t ram128mb = bankbase[0] + (128<<20);
- xen_pfn_t p2m_size;
uint64_t bank0end;
assert(dom->rambase_pfn << XC_PAGE_SHIFT == bankbase[0]);
@@ -409,16 +408,12 @@ static int meminit(struct xc_dom_image *dom)
ramsize -= banksize;
- p2m_size = ( bankbase[i] + banksize - bankbase[0] ) >> XC_PAGE_SHIFT;
-
dom->rambank_size[i] = banksize >> XC_PAGE_SHIFT;
}
assert(dom->rambank_size[0] != 0);
assert(ramsize == 0); /* Too much RAM is rejected above */
- dom->p2m_size = p2m_size;
-
/* setup initial p2m and allocate guest memory */
for ( i = 0; i < GUEST_RAM_BANKS && dom->rambank_size[i]; i++ )
{
--
2.11.0
Hi Andrew,
On 12/02/2021 20:01, Andrew Cooper wrote:
> Various version of gcc, when compiling with -Og, complain:
>
> xg_dom_arm.c: In function 'meminit':
> xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 420 | dom->p2m_size = p2m_size;
> | ~~~~~~~~~~~~~~^~~~~~~~~~
>
> This is actually entirely stale code since ee21f10d70^..97e34ad22d which
> removed the 1:1 identity p2m for translated domains.
>
> Drop the write of d->p2m_size, and the p2m_size local variable. Reposition
> the p2m_size field in struct xc_dom_image and correct some stale
> documentation.
>
> This change really ought to have been part of the original cleanup series.
>
> No actual change to how ARM domains are constructed.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
> ---
> CC: Ian Jackson <iwj@xenproject.org>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
>
> v2:
> * Delete stale p2m_size infrastructure.
> ---
> tools/include/xenguest.h | 5 ++---
> tools/libs/guest/xg_dom_arm.c | 5 -----
> 2 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
> index 775cf34c04..217022b6e7 100644
> --- a/tools/include/xenguest.h
> +++ b/tools/include/xenguest.h
> @@ -145,6 +145,7 @@ struct xc_dom_image {
> * eventually copied into guest context.
> */
> xen_pfn_t *pv_p2m;
> + xen_pfn_t p2m_size; /* number of pfns covered by pv_p2m */
>
> /* physical memory
> *
> @@ -154,12 +155,10 @@ struct xc_dom_image {
> *
> * An ARM guest has GUEST_RAM_BANKS regions of RAM, with
> * rambank_size[i] pages in each. The lowest RAM address
> - * (corresponding to the base of the p2m arrays above) is stored
> - * in rambase_pfn.
> + * is stored in rambase_pfn.
> */
> xen_pfn_t rambase_pfn;
> xen_pfn_t total_pages;
> - xen_pfn_t p2m_size; /* number of pfns covered by p2m */
> struct xc_dom_phys *phys_pages;
> #if defined (__arm__) || defined(__aarch64__)
> xen_pfn_t rambank_size[GUEST_RAM_BANKS];
> diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
> index 94948d2b20..b4c24f15fb 100644
> --- a/tools/libs/guest/xg_dom_arm.c
> +++ b/tools/libs/guest/xg_dom_arm.c
> @@ -373,7 +373,6 @@ static int meminit(struct xc_dom_image *dom)
> const uint64_t modsize = dtb_size + ramdisk_size;
> const uint64_t ram128mb = bankbase[0] + (128<<20);
>
> - xen_pfn_t p2m_size;
> uint64_t bank0end;
>
> assert(dom->rambase_pfn << XC_PAGE_SHIFT == bankbase[0]);
> @@ -409,16 +408,12 @@ static int meminit(struct xc_dom_image *dom)
>
> ramsize -= banksize;
>
> - p2m_size = ( bankbase[i] + banksize - bankbase[0] ) >> XC_PAGE_SHIFT;
> -
> dom->rambank_size[i] = banksize >> XC_PAGE_SHIFT;
> }
>
> assert(dom->rambank_size[0] != 0);
> assert(ramsize == 0); /* Too much RAM is rejected above */
>
> - dom->p2m_size = p2m_size;
> -
> /* setup initial p2m and allocate guest memory */
> for ( i = 0; i < GUEST_RAM_BANKS && dom->rambank_size[i]; i++ )
> {
>
--
Julien Grall
Andrew Cooper writes ("[PATCH v1.1 03/10] tools/libxg: Drop stale p2m logic from ARM's meminit()"):
> Various version of gcc, when compiling with -Og, complain:
>
> xg_dom_arm.c: In function 'meminit':
> xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 420 | dom->p2m_size = p2m_size;
> | ~~~~~~~~~~~~~~^~~~~~~~~~
>
> This is actually entirely stale code since ee21f10d70^..97e34ad22d which
> removed the 1:1 identity p2m for translated domains.
>
> Drop the write of d->p2m_size, and the p2m_size local variable. Reposition
> the p2m_size field in struct xc_dom_image and correct some stale
> documentation.
>
> This change really ought to have been part of the original cleanup series.
>
> No actual change to how ARM domains are constructed.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
© 2016 - 2026 Red Hat, Inc.