[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 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;

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.

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.

Cheers,

--
Julien Grall



 


Rackspace

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