[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 12:15, Oleksandr wrote:
> On 27.01.21 12: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.
>>
>>>>> '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

Good. I'd be okay if you folded this in (plus a comment of
course), but others may have different views, not the least as
this is only papering over the issue (yet an issue that's not
ours, but the compiler's).

Jan



 


Rackspace

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