Re: [PATCH for-4.19] xen/arm: static-shmem: fix "gbase/pbase used uninitialized" build failure

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

On 19/06/2024 07:46, Michal Orzel wrote:
> Building Xen with CONFIG_STATIC_SHM=y results in a build failure:
>
> arch/arm/static-shmem.c: In function 'process_shm':
> arch/arm/static-shmem.c:327:41: error: 'gbase' may be used uninitialized 
> [-Werror=maybe-uninitialized]
>    327 |         if ( is_domain_direct_mapped(d) && (pbase != gbase) )
> arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here
>    305 |         paddr_t gbase, pbase, psize;
>
> This is because the commit cb1ddafdc573 adds a check referencing
> gbase/pbase variables which were not yet assigned a value. Fix it.
>
> Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be direct-mapped for 
> direct-mapped domains")
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
> Rationale for 4.19: this patch fixes a build failure reported by CI:
> https://gitlab.com/xen-project/xen/-/jobs/7131807878
> ---
>   xen/arch/arm/static-shmem.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index c434b96e6204..cd48d2896b7e 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -324,12 +324,6 @@ int __init process_shm(struct domain *d, struct 
> kernel_info *kinfo,
>               printk("%pd: static shared memory bank not found: '%s'", d, 
> shm_id);
>               return -ENOENT;
>           }
> -        if ( is_domain_direct_mapped(d) && (pbase != gbase) )
> -        {
> -            printk("%pd: physical address 0x%"PRIpaddr" and guest address 
> 0x%"PRIpaddr" are not direct-mapped.\n",
> -                   d, pbase, gbase);
> -            return -EINVAL;
> -        }
>   
>           pbase = boot_shm_bank->start;
>           psize = boot_shm_bank->size;
> @@ -353,6 +347,13 @@ int __init process_shm(struct domain *d, struct 
> kernel_info *kinfo,
>               /* guest phys address is after host phys address */
>               gbase = dt_read_paddr(cells + addr_cells, addr_cells);
>   
> +            if ( is_domain_direct_mapped(d) && (pbase != gbase) )
> +            {
> +                printk("%pd: physical address 0x%"PRIpaddr" and guest address 
> 0x%"PRIpaddr" are not direct-mapped.\n",
> +                       d, pbase, gbase);
> +                return -EINVAL;
> +            }
> +
Before this patch, the check was globally. I guess the intention was it 
covers the two part of the "if". But now, you only have it in when 
"paddr" is specified in the DT.
From a brief look at the code, I can't figure out why we don't need a 
similar check on the else path. Is this because it is guarantee that 
will be paddr == gaddr?
Cheers,

--
Julien Grall