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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.01.2021 16:29, Julien Grall wrote:
> 
> 
> On 27/01/2021 10:51, Jan Beulich wrote:
>> 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.
> 
> This message is actually coming from GCC. There are quite a few reports 
> online.
> 
> Although, I am not sure whether this was intended.
> 
>>
>>>>> '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 ...
> 
> So GCC is complaining specifically about mfn_list[0]. For instance, I 
> wrote the following:
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 33296e65cb04..81b4645047e7 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1139,6 +1139,8 @@ static int acquire_resource(
>       xen_pfn_t mfn_list[32];
>       int rc;
> 
> +    mfn_list[0] = 0;
> +
>       if ( !arch_acquire_resource_check(currd) )
>           return -EACCES;
> 
> It will silence GCC. But the follwing will not:
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 33296e65cb04..cf558a6b9fa4 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1139,6 +1139,8 @@ static int acquire_resource(
>       xen_pfn_t mfn_list[32];
>       int rc;
> 
> +    mfn_list[1] = 0;
> +
>       if ( !arch_acquire_resource_check(currd) )
>           return -EACCES;

Interesting.

> I also try to set mfn_list[0] to 0 in different part of the switch. It 
> doesn't help, if I add it in the default. But it does, if I put in the 
> grant table path.

Even more interesting. All pretty odd, so yes, ...

> So it looks like the grant table path is the issue.
> 
> I can confirm that your solution will silenece GCC, but I am a bit worry 
> that this may only hide a different bug because the failure seems to be 
> widespread on arm (gitlab reported the error with GCC 9.2.1 and I have 
> managed to reproduced on GCC 7.5.0).
> 
> Given this is a randconfig where CONFIG_GRANT_TABLE is disabled (we 
> technically don't grant table disabled), I would rather take a bit more 
> time to understand the problem rather than rushing it.

... this would certainly be fine with me.

Jan