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

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm



On 27.01.2021 11:13, Oleksandr wrote:
> On 26.01.21 02:14, Oleksandr wrote:
>> On 26.01.21 01:20, Julien Grall wrote:
>>> On Mon, 25 Jan 2021 at 20:56, Stefano Stabellini 
>>> <sstabellini@xxxxxxxxxx> wrote:
>>>> This seems to be an arm randconfig failure:
>>>>
>>>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
>>>> https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044
>>> Thanks! The error is:
>>>
>>> #'target_mem_ref' not supported by expression#'memory.c: In function

Btw, I found the first part of this line pretty confusing, to a
degree that when seeing it initially I thought this must be some
odd tool producing the odd error. But perhaps this is just
unfortunate output ordering from different tools running in
parallel.

>>> 'do_memory_op':
>>> memory.c:1210:18: error:  may be used uninitialized in this function
>>> [-Werror=maybe-uninitialized]
>>>   1210 |             rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>   1211 | _mfn(mfn_list[i]));
>>>        | ~~~~~~~~~~~~~~~~~~
>>>
>>> I found a few references online of the error message, but it is not
>>> clear what it means. From a quick look at Oleksandr's branch, I also
>>> can't spot anything unitialized. Any ideas?
>> It seems that error happens if *both* CONFIG_GRANT_TABLE and 
>> CONFIG_IOREQ_SERVER are disabled. Looks like that mfn_list is 
>> initialized either in acquire_grant_table() or in acquire_ioreq_server().
>> If these options disabled then corresponding helpers are just stubs, 
>> so indeed that mfn_list gets uninitialized. But, I am not sure why gcc 
>> complains about it as set_foreign_p2m_entry() is *not* going to be 
>> called in that case???
> 
> This weird build error goes away if I simply add:
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 33296e6..d1bd57b 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1136,7 +1136,7 @@ static int acquire_resource(
>        * moment since they are small, but if they need to grow in future
>        * use-cases then per-CPU arrays or heap allocations may be required.
>        */
> -    xen_pfn_t mfn_list[32];
> +    xen_pfn_t mfn_list[32] = {0};
>       int rc;
> 
>       if ( !arch_acquire_resource_check(currd) )
> 
> 
> Shall I make the corresponding patch?

I'd prefer if we could find another solution, avoiding this
pointless writing of 256 bytes of zeros (and really to be on the
safe side I think it should rather be ~0 that gets put in there).
Could you check whether clearing the array along the lines of
this

    default:
        memset(mfn_list, ~0, sizeof(mfn_list));
        rc = -EOPNOTSUPP;
        break;

helps (avoiding the writes in all normal cases)? Of course this
wouldn't be a guarantee that another compiler (version) won't
warn yet again. But the only other alternative I can think of
without having the writes on the common path would be something
along the lines of older Linux'es uninitialized_var(). Maybe
someone else has a better idea ...

> But it is still unclear to me why the compiler doesn't recognize that 
> *non-yet-uninitialized* mfn_list[] won't be used if both 
> CONFIG_GRANT_TABLE and CONFIG_IOREQ_SERVER are not set...

The combination of conditions may be too complex for it to
figure. I suppose a warning like this can't be had without at
least one of false positives or false negatives.

Jan



 


Rackspace

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