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

[Julien Grall <julien@xxxxxxx>]

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 
> > CONFIG_COVERAGE=y"):
> > > 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 
> > CONFIG_COVERAGE=y"):
> > > On 30.01.2021 16:22, Julien Grall wrote:
> > > > @@ -1442,13 +1447,6 @@ long do_memory_op(unsigned long cmd, 
> > > > XEN_GUEST_HANDLE_PARAM(void) arg)
> > > >           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);
> > > >   
> > > >           rcu_unlock_domain(d);
> > > 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
> > > action.
> > 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 ?
> Yes.
>
> >   And that your proposed
> > additional change would have some impact whilch would have to be
> > assessed.
> 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.

Cheers,

--
Julien Grall