[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y [and 1 more messages]

On 01/02/2021 15:02, Jan Beulich wrote:
On 01.02.2021 15:54, Ian Jackson wrote:
Julien Grall writes ("[PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and 
Xen is heavily relying on the DCE stage to remove unused code so the
linker doesn't throw an error because a function is not implemented
yet we defined a prototype for it.

Thanks for the clear explanation.

It is not entirely clear why the compiler DCE is not detecting the
unused code. However, moving the permission check from do_memory_op()
to xenmem_add_to_physmap_batch() does the trick.

How unfortunate.

Fixes: d4f699a0df6c ("x86/mm: p2m_add_foreign() is HVM-only")
Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

I have reviewed the diff, but not the code in context.

The gitlab CI is used to provide basic testing on a per-series basis. So
I would like to request this patch to be merged in Xen 4.15 in order to
reduce the number of failure not related to the series tested.

Quite so.

Jan Beulich writes ("Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and 
On 30.01.2021 16:22, Julien Grall wrote:
@@ -1442,13 +1447,6 @@ long do_memory_op(unsigned long cmd, 
          if ( d == NULL )
              return -ESRCH;
- rc = xatp_permission_check(d, xatpb.space);
-        if ( rc )
-        {
-            rcu_unlock_domain(d);
-            return rc;
-        }
          rc = xenmem_add_to_physmap_batch(d, &xatpb, start_extent);

I'd be okay with the code movement if you did so consistently,
i.e. also for the other invocation. I realize this would have
an effect on the dm-op call of the function, but I wonder
whether this wouldn't even be a good thing. If not, I think
duplicating xenmem_add_to_physmap()'s early ASSERT() into
xenmem_add_to_physmap_batch() would be the better course of

Jan, can you confirm whether in your opinion this patch as originally
posted by Julien is *correct* as is ?  In particular, Julien did not
intend a functional change.  Have you satisfied yourself that there is
no functional change here ?

Yes and yes.

I understand your objectiion above to relate to style or neatness,
rather than function.  Is that correct ?


  And that your proposed
additional change would have some impact whilch would have to be

The first of the proposed alternatives may need further
investigation, yes. The second of the alternatives would
shrink this patch to a 2-line one, i.e. far less code
churn, and is not in need of any assessment afaia. In
fact I believe this latter alternative was discussed as
the approach to take here, before the patch was submitted.

Right, I chose this approach over the one discussed previously because it doesn't duplicate the check for auto-translated domain and I couldn't really find a good way to justify it in the code (you requested one).

Regarding calling the xatp_permission_check() from xenmem_add_to_physmap(). It would means that the DM code will have two XSM check (one XSM_DM_PRIV, one XSM_TARGET). It is not clear to me whether this is going to be introducing more headache for the XSM folks.

Therefore, for Xen 4.15, I would prefer to stick with my patch.


Julien Grall



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