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

[Michal Orzel <michal.orzel@xxxxxxx>]

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.
Thus, reading this doc makes me feel that for 1:1 guests user needs to specify 
pbase == gbase.

~Michal