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

[Julien Grall <julien@xxxxxxx>]

On 19/06/2024 13:06, Michal Orzel wrote:
> Hi Julien,
>
> On 19/06/2024 13:55, Julien Grall wrote:
> > 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?
> The reason why I added this check only in the first case is due to what doc 
> states.
> It says that if a domain is 1:1, the shmem should be also 1:1 i.e. pbase == 
> gbase. In the else
> case the pbase is omitted and thus a user cannot know and has no guarantee what 
> will be the backing physical address.
The property "direct-map" has the following definition:

"- direct-map

    Only available when statically allocated memory is used for the domain.
    An empty property to request the memory of the domain to be
    direct-map (guest physical address == physical address).
"

So I think it would be fair for someone to interpret it as shared memory 
would also be 1:1 mapped.
> Thus, reading this doc makes me feel that for 1:1 guests user needs to specify 
> pbase == gbase.
See above, I think this is not 100% clear. I am concerned that someone 
may try to use the version where only the guest address is specified.
It would likely be hard to realize that the extended regions would not 
work properly. So something needs to be done.
I don't have any preference on how to address. It could simply be a 
check in Xen to request that both "gaddr" and "paddr" are specified for 
direct mapped domain.
Cheers,

--
Julien Grall