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

[Oleksandr <olekstysh@gmail.com>]

On 27.01.21 12:51, Jan Beulich wrote:

Hi Jan

> 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@kernel.org> 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)?

Yes, this helps (at least in my environment):

aarch64-poky-linux-gcc v8.2


> 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.

I got it.

-- 
Regards,

Oleksandr Tyshchenko