[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Julien Grall <julien@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Michal Orzel <michal.orzel@xxxxxxx>
  • Date: Thu, 20 Jun 2024 09:26:25 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=xen.org smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0)
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=O4a2PhBrpHCu3XKgwdNkoRXO5cL+e+BTf3xeee/EQow=; b=AV+CckSiCPsCP2fZjTLqGQut2IQWKAlYAfz36ZYaU0i2IwmyUTHoqCZ+cjg9iW8XDsj4F/XXVVhMuvJqiRIx5vYL0o5WP10OVZkh+mKeP1VpKI7VKy04c+lPSRHfrGDvNrXjKzUSqp6L8wC0sKQ9XYoqycJ4iTimSP+gS8NgI7J6r7d1Ti6IGRqtUj7Ws4zdn7gv//XpUuJRrtdyd6Tfv56ueeWOeiLYHgILbSGVV+khNap6OzNIwOb5A6QMq85C8R6Wndozfyf41heCfQIK7JxlUuuxXCGIzPgqv7D7qeM8k/JcBepWFT9u23oSo2blLdYLa8OkOUVNi/T3mvXHlw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=TQuGi3Ye/E0+8xrDenrAkHe8l/0o0jiVU/eStu3ZEl5JCp83eVURr0OEKRMmDyRDosQAjkn5Pa8mc0s6FvfLR1wuKm7K948KQQPeq4cQrK4g6GvmQ/IoxY61Lomx8Y2Qin4vIyr7l60wIijs4O2J2YG3JqpWBuZWYVdUJ4tbKGFeafRmkhuqfC3aYln334KTL/M7HT7XqDf9Jpm85td5qHvim2GbuaTWSz5Lc1Bz1l9pH0iPKduGRURbxafrxFDBcsXhzLg/RxKAZcd7qSKKyPHeYVDwsxhJfgr+Fqq0sdazMHFVRPJeo5LGGb8w1TTffujSCOnEQ1sfuRg8mxgQQQ==
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Thu, 20 Jun 2024 07:26:42 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>


On 19/06/2024 14:34, Julien Grall wrote:
> 
> 
> 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.
Fair enough. I can add a check for 1:1 in the else case to return error with a 
message that
host and guest physical address must be supplied for direct-mapped domains. 
Would we consider it for 4.19?
In my opinion yes as it would remove the possibility of a feature misuse.

~Michal



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.