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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 27.01.21 13:22, Jan Beulich wrote:

Hi Jan

> 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).
Great, I will wait a bit and if there are no objections I will fold this 
in.
--
Regards,

Oleksandr Tyshchenko