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